Re: [PATCH v4 2/2] iio: adc: add Axiado SARADC driver

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

On Fri, Jul 17, 2026 at 11:35:01AM +0300, Andy Shevchenko wrote:
> On Thu, Jul 16, 2026 at 10:53:02PM -0700, Petar Stepanovic wrote:

...

> > +static void axiado_saradc_disable(void *data)
> > +{
> > +	struct axiado_saradc *info = data;
> > +
> > +	regmap_write(info->regmap, AX_SARADC_GLOBAL_CTRL_REG,
> > +		     AX_SARADC_GLOBAL_CTRL_PD);
> > +}
> 
> Supply regmap instead of info and make this simpler
> 
> static void axiado_saradc_disable(void *map)
> {
> 	regmap_write(map, AX_SARADC_GLOBAL_CTRL_REG, AX_SARADC_GLOBAL_CTRL_PD);
> }
> 
> ...
> 
> > +	regval = FIELD_PREP(AX_SARADC_GLOBAL_CTRL_CH_EN_MASK,
> > +			    GENMASK(soc_data->num_channels - 1, 0)) |
> > +		 AX_SARADC_GLOBAL_CTRL_SAMPLE_16 |
> > +		 AX_SARADC_GLOBAL_CTRL_MODE_MANUAL |
> > +		 AX_SARADC_GLOBAL_CTRL_ENABLE;
> 
> This is not used in the below call, move it closer to its user.
> 
> > +	ret = regmap_write(info->regmap, AX_SARADC_GLOBAL_CTRL_REG,
> > +			   AX_SARADC_GLOBAL_CTRL_PD);
> 
> With
> 
> 	struct regmap *map;
> 
> at the top, this and other will be shorter and easier to follow.
> And I would dare to use a single line:
> 
> 	ret = regmap_write(map, AX_SARADC_GLOBAL_CTRL_REG, AX_SARADC_GLOBAL_CTRL_PD);

Looking closer at this I think it's a leftover? Since it's an action that does
disable. This one should only enable chip, right?

> > +	if (ret)
> > +		return ret;
> > +
> > +	ret = regmap_write(info->regmap, AX_SARADC_GLOBAL_CTRL_REG, regval);
> > +	if (ret)
> > +		return ret;
> 
> > +	ret = devm_add_action_or_reset(dev, axiado_saradc_disable, info);
> 
> 	ret = devm_add_action_or_reset(dev, axiado_saradc_disable, map);
> 
> > +	if (ret)
> > +		return ret;

-- 
With Best Regards,
Andy Shevchenko