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

[Andy Shevchenko <andriy.shevchenko@intel.com>]

On Sat, Oct 24, 2026 at 08:13:33PM -0300, Stephan Costa Barros 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.

...

> +#define ADS1298_VREF_4V_MV			4000

_mV

...

> +/* What differs between the chip families the driver supports */
> +struct ads1298_chip_info {
> +	/* Checks the ID register, and sets the channel count and the name */
> +	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;
> +	bool has_hr;
> +	/* Oversampling ratio at DR = 0 */
> +	unsigned int osr_base;
> +	/* Internal reference, in millivolts */
> +	unsigned int vref_mv;

_mV

> +	/* CONFIG3 has the VREF_4V bit, which selects a 4 V internal reference */
> +	bool has_vref_4v;
> +	/* Reserved bits of CONFIG3 that must be written as 1 */
> +	unsigned int config3_reserved;
> +};

Run `pahole` and act accordingly.

...

>  		*val = ret / MILLI; /* Convert to millivolts */

At some point this should use (MICRO / MILLI) instead of MILLI.

...

>  		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;

Can't we assign the default and always use info->vref_mV here?

...

> +	priv->chip_info = spi_get_device_match_data(spi);
> +	if (!priv->chip_info)
> +		return -ENODEV;

-ENODATA

...

> -	priv->regmap = devm_regmap_init(dev, NULL, priv, &ads1298_regmap_config);
> +	priv->regmap = devm_regmap_init(dev, NULL, priv,
> +					priv->chip_info->regmap_config);

Leave it a single line.

>  	if (IS_ERR(priv->regmap))

...

>  static const struct spi_device_id ads1298_id[] = {
> -	{ .name = "ads1298" },
> +	{ .name = "ads1298", .driver_data = (kernel_ulong_t)&ads1298_chip_info },
>  	{ }
>  };
>  MODULE_DEVICE_TABLE(spi, ads1298_id);
>  
>  static const struct of_device_id ads1298_of_table[] = {
> -	{ .compatible = "ti,ads1298" },
> +	{ .compatible = "ti,ads1298", .data = &ads1298_chip_info },
>  	{ }
>  };
>  MODULE_DEVICE_TABLE(of, ads1298_of_table);

I think this patch is overloaded. You need to split it to a series (to make
chip_info a separate preparatory change, for example).

-- 
With Best Regards,
Andy Shevchenko