Re: [PATCH v2 4/6] iio: adc: ti-ads1298: prepare for more chip families

[Jonathan Cameron <jic23@kernel.org>]

On Sat, 24 Oct 2026 20:13:33 -0300
Stephan Costa Barros <stephanbrrs8@gmail.com> wrote:

> The ADS1299 has the register map of the ADS129x, with other gains, data
> rates, reference and ID layout. To support it in this driver, collect
> what differs between families in a chip info structure, given as match
> data. The private structure has carried an unused chip_info pointer
> since the driver was added.
> 
> The chip info holds:
> - the regmap configuration and the PGA gain table;
> - the data rate rules: the base shift, whether CONFIG1 has an HR bit,
>   and the oversampling ratio at DR = 0;
> - the internal reference, whether CONFIG3 has the VREF_4V bit, and the
>   reserved bits of CONFIG3 that must be written as 1;
> - a callback that checks the ID register and sets the channel count and
>   the name, as the ID layouts differ; bit 3 alone tells them apart.
> 
> No functional change.
> 
> Signed-off-by: Stephan Costa Barros <stephanbrrs8@gmail.com>
Mostly fine. A few comments in line in trying to
naming the structure elements to help with readability
and reduce need for docs around them.

Jonathan

> ---
>  drivers/iio/adc/ti-ads1298.c | 132 +++++++++++++++++++++++++----------
>  1 file changed, 96 insertions(+), 36 deletions(-)
> 
> diff --git a/drivers/iio/adc/ti-ads1298.c b/drivers/iio/adc/ti-ads1298.c
> index b390c13ae..5ad543bdf 100644
> --- a/drivers/iio/adc/ti-ads1298.c
> +++ b/drivers/iio/adc/ti-ads1298.c
> @@ -49,8 +49,6 @@
>  #define ADS1298_REG_CONFIG1	0x01
>  #define ADS1298_MASK_CONFIG1_HR			BIT(7)
>  #define ADS1298_MASK_CONFIG1_DR			GENMASK(2, 0)
> -#define ADS1298_SHIFT_DR_HR			6
> -#define ADS1298_SHIFT_DR_LP			7
>  #define ADS1298_LOWEST_DR			0x06
>  
>  #define ADS1298_REG_CONFIG2	0x02
> @@ -66,6 +64,7 @@
>  #define ADS1298_MASK_CONFIG3_PWR_REFBUF		BIT(7)
>  #define ADS1298_MASK_CONFIG3_RESERVED		BIT(6)
>  #define ADS1298_MASK_CONFIG3_VREF_4V		BIT(5)
> +#define ADS1298_VREF_4V_MV			4000
>  
>  #define ADS1298_REG_LOFF	0x04
>  #define ADS1298_REG_CHnSET(n)	(0x05 + (n))
> @@ -105,6 +104,31 @@
>  #define ADS1298_SPI_RDATA_BUFFER_SIZE_MAX \
>  		ADS1298_SPI_RDATA_BUFFER_SIZE(ADS1298_MAX_CHANNELS)
>  
> +/* What differs between the chip families the driver supports */

This is very standard naming and purpose. I'd drop the comment.
These sorts of general comments tend to become misleading as code
changes and we never remember to update them.
Keep comments for the surprising or places where they add stuff
we can't do with naming.


> +struct ads1298_chip_info {
> +	/* Checks the ID register, and sets the channel count and the name */
Given this is a 
> +	int (*parse_id)(struct iio_dev *indio_dev, unsigned int id);
> +	const struct regmap_config *regmap_config;
> +	/* PGA gain for each value of the GAIN field of CHnSET */
> +	const u8 *gains;
> +	unsigned int num_gains;
> +	/*
> +	 * The data rate is f_CLK >> (dr_shift + DR). With an HR bit in
> +	 * CONFIG1, dr_shift is that of the high-resolution mode, and the
> +	 * low-power mode shifts one more.
> +	 */
> +	unsigned int dr_shift;

Probably spell out dr.

> +	bool has_hr;

I'd spell out hr.

> +	/* Oversampling ratio at DR = 0 */
> +	unsigned int osr_base;
> +	/* Internal reference, in millivolts */
> +	unsigned int vref_mv;

Rename to something that won't need the comment then drop that coment.


	unsigned int internal_vref_mV;

> +	/* CONFIG3 has the VREF_4V bit, which selects a 4 V internal reference */
> +	bool has_vref_4v;

I'm lost here. If it's about the bit, then name it after the bit.
I suspect it's about internal reference that can be disabled or not?
In that case can you name it for that functionality instead?

From a quick look below, seems it is about what of two modes can be
used?  If so how does that fit with just one vref_mv above?

> +	/* Reserved bits of CONFIG3 that must be written as 1 */

This one is interesting. Normally we deal with these by just using
RMW cycles on such a register.   Can we do that here?

> +	unsigned int config3_reserved;
> +};
> +


> -static const u8 ads1298_pga_settings[] = { 6, 1, 2, 3, 4, 8, 12 };
> -
>  static int ads1298_get_scale(struct ads1298_private *priv,
>  			     int channel, int *val, int *val2)
>  {
> +	const struct ads1298_chip_info *info = priv->chip_info;
>  	unsigned int pga_idx;
>  	int ret;
>  	unsigned int regval;
> @@ -356,13 +381,16 @@ static int ads1298_get_scale(struct ads1298_private *priv,
>  			return ret;
>  
>  		*val = ret / MILLI; /* Convert to millivolts */
> -	} else {
> +	} else if (info->has_vref_4v) {
>  		ret = regmap_read(priv->regmap, ADS1298_REG_CONFIG3, &regval);
>  		if (ret)
>  			return ret;
>  
>  		/* Reference in millivolts */
> -		*val = regval & ADS1298_MASK_CONFIG3_VREF_4V ? 4000 : 2400;
> +		*val = regval & ADS1298_MASK_CONFIG3_VREF_4V ?
> +		       ADS1298_VREF_4V_MV : info->vref_mv;

See above.  Why is vref_mv which is an internal reference not the 4V one?
Probably need clearer naming of that field.

> +	} else {
> +		*val = info->vref_mv;
>  	}
>  

> -static int ads1298_init(struct iio_dev *indio_dev)
> +static int ads1298_parse_id(struct iio_dev *indio_dev, unsigned int id)
>  {
>  	struct ads1298_private *priv = iio_priv(indio_dev);
>  	struct device *dev = &priv->spi->dev;
>  	const char *suffix;
>  	unsigned int nu_ch;
> -	unsigned int val;
> -	int ret;
> -
> -	/* Device initializes into RDATAC mode, which we don't want */
> -	ret = ads1298_write_cmd(priv, ADS1298_CMD_SDATAC);
> -	if (ret)
> -		return ret;
> -
> -	ret = regmap_read(priv->regmap, ADS1298_REG_ID, &val);
> -	if (ret)
> -		return ret;
>  
>  	/* Fill in name and channel count based on what the chip told us */
> -	nu_ch = FIELD_GET(ADS1298_MASK_ID_CHANNELS, val);
> +	nu_ch = FIELD_GET(ADS1298_MASK_ID_CHANNELS, id);
>  	/* 0, 1 and 2 are 4, 6 and 8 channels; the other codes are reserved */
>  	if (nu_ch > 2)
> -		return dev_err_probe(dev, -ENODEV, "Unknown ID: 0x%x\n", val);
> +		return dev_err_probe(dev, -ENODEV, "Unknown ID: 0x%x\n", id);
>  	indio_dev->num_channels = 4 + 2 * nu_ch;
> -	switch (val & ADS1298_MASK_ID_FAMILY) {
> +	switch (id & ADS1298_MASK_ID_FAMILY) {

Please use FIELD_GET() and fix the family masks to be values in the field
rather than the shifted version.  You might want to do that
as a separate precursor patch.  Looks like there may be other similar
changes in driver around fields vs value defines.


>  	case ADS1298_ID_FAMILY_ADS129X:
>  		suffix = "";
>  		break;
> @@ -745,19 +762,57 @@ static int ads1298_init(struct iio_dev *indio_dev)
>  		suffix = "r";
>  		break;
>  	default:
> -		return dev_err_probe(dev, -ENODEV, "Unknown ID: 0x%x\n", val);
> +		return dev_err_probe(dev, -ENODEV, "Unknown ID: 0x%x\n", id);
>  	}
>  	indio_dev->name = devm_kasprintf(dev, GFP_KERNEL, "ads129%u%s",
>  					 indio_dev->num_channels, suffix);
>  	if (!indio_dev->name)
>  		return -ENOMEM;
>  
> -	val = ADS1298_MASK_CONFIG3_RESERVED; /* Must write 1 always */
> +	return 0;
> +}