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

[Jonathan Cameron <jonathan.cameron@oss.qualcomm.com>]

On Fri, 17 Jul 2026 11:34:56 +0300
Andy Shevchenko <andriy.shevchenko@intel.com> wrote:

> On Thu, Jul 16, 2026 at 10:53:02PM -0700, Petar Stepanovic wrote:
> > Add support for the SARADC controller found on Axiado AX3000 and
> > AX3005 SoCs.
> > 
> > The driver supports single-shot voltage reads through the IIO
> > subsystem. The number of available input channels is selected from
> > the SoC match data, allowing AX3000 and AX3005 variants to use the
> > same driver.  
> 
Andy already commented everywhere I'd have said things, so just
a few places where I'd take a different approach to resolving stuff.

Thanks,

Jonathan

> 
> ...
> 
> > +/* GLOBAL_CTRL register values */
> > +#define AX_SARADC_GLOBAL_CTRL_SAMPLE_16	\
> > +	FIELD_PREP(AX_SARADC_GLOBAL_CTRL_SAMPLE_MASK, 0)
> > +
> > +#define AX_SARADC_GLOBAL_CTRL_MODE_MANUAL	\
> > +	FIELD_PREP(AX_SARADC_GLOBAL_CTRL_MODE_MASK, 1)  
> 
> FIELD_PREP_CONST() in both cases.
> 
For this second one I'd rather see defines for the values the
field takes and then FIELD_PREP only used inline.

#define AX_SARADC_GLOBAL_CTL_MODE_something 0
#define AX_SARADC_GLOBAL_CTL_MODE_MANUAL 1

To me that acts as clearer documentation of what is
going on at the point of the write.

Similar applies for the SAMPLE_MASK field where I'd define the
value 0 with a name then have the FIELD_PREP() inline.


> ...
> 
> > +#define AX_SARADC_MANUAL_CTRL_EN(ch)	\
> > +	(AX_SARADC_MANUAL_CTRL_ENABLE |	\
> > +	FIELD_PREP(AX_SARADC_MANUAL_CTRL_CH_SEL_MASK, ch))  
> 
> Missing parentheses for ch. Also wrong indentation of the second line, missing
> one space.
> 

I wonder if it would be more readable to just push that inline rather
than having this define at all.