Re: [PATCH v3 1/2] iio: addac: stx104: Migrate to the regmap API

[William Breathitt Gray <william.gray@linaro.org>]

[-- Attachment #1: Type: text/plain, Size: 2902 bytes --]

On Mon, Mar 27, 2023 at 02:42:24PM +0300, Andy Shevchenko wrote:
> On Sun, Mar 26, 2023 at 06:05:57PM -0400, William Breathitt Gray wrote:
> > The regmap API supports IO port accessors so we can take advantage of
> > regmap abstractions rather than handling access to the device registers
> > directly in the driver.
> 
> ...
> 
> > +static const struct regmap_config aio_ctl_regmap_config = {
> > +	.name = "aio_ctl",
> > +	.reg_bits = 8,
> > +	.reg_stride = 1,
> > +	.reg_base = STX104_AIO_BASE,
> > +	.val_bits = 8,
> > +	.io_port = true,
> > +	.max_register = 0x11,
> 
> Not sure if define would be better for this, so it will be grouped with
> register offset definitions. (Same for the other configs)

Actually, I'll remove this max_register line because it's superfluous
when we define both rd_table and wr_table.

However, I explain my reasoning for hardcoding the max_register value in
a response to the ebc-c384_wdt watchdog driver conversion [0]. To
summarize, it's not immediately obvious whether a FOO_REGISTER comes
before or after a BAR_REGISTER so someone reading the code would need to
verify the define; there are cases as well, such as GPIO drivers, where
the only the base register is defined but the max register would be an
extent from there; and finally, a reviewer would ultimately need to
check against the datasheet to verify that the max_register is actually
at the named defined register location, whereas it is far easier to
lookup the address range in the datasheet rather than named registers.
These things make hardcoding max_register to be not only clearer code to
read and verify but also less likely to be set to the wrong address.

[0] https://lore.kernel.org/all/ZAyY3VGlo4N4SLZQ@fedora/

> > +	.wr_table = &aio_ctl_wr_table,
> > +	.rd_table = &aio_ctl_rd_table,
> > +	.volatile_table = &aio_ctl_volatile_table,
> > +	.cache_type = REGCACHE_FLAT,
> > +};
> 
> Do we need regmap lock?

I think the regmap lock is opt-out, so I don't think we need to set an
custom lock callback for the regmaps in this driver.

Jonathan, do read_raw() and write_raw() require explicit locking?

> > +		err = regmap_read(priv->aio_ctl_map, STX104_ADC_CONFIGURATION, &adc_config);
> > +		if (err)
> > +			return err;
> >  
> > -		*val = 1 << gain;
> > +		*val = 1 << u8_get_bits(adc_config, STX104_GAIN);
> 
> Maybe not for this change, but why not BIT()?

This probably should have been BIT(gain) when it was originally
introduced. I'm avoiding it here just to keep this migration patch more
straight-forward to review; but perhaps I'll make this change afterall
in a v4 submission.

> >  	case IIO_CHAN_INFO_RAW:
> >  		if (chan->output) {
> 
> You can decrease indentation by
> 
> 		if (!chan->output)
> 			return -EINVAL;
> 
> here.

Sure I can make this change as well in a v4.

William Breathitt Gray

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]