Re: [PATCH v5 07/10] iio: adc: Support ROHM BD79124 ADC

[Jonathan Cameron]

On Mon, 10 Mar 2025 10:46:45 +0200
Matti Vaittinen <mazziesaccount@xxxxxxxxx> wrote:

> On 08/03/2025 18:44, Jonathan Cameron wrote:
> > On Mon, 3 Mar 2025 13:33:39 +0200
> > Matti Vaittinen <mazziesaccount@xxxxxxxxx> wrote:
> >   
> >> The ROHM BD79124 is a 12-bit, 8-channel, SAR ADC. The ADC supports
> >> an automatic measurement mode, with an alarm interrupt for out-of-window
> >> measurements. The window is configurable for each channel.
> >>
> >> The I2C protocol for manual start of the measurement and data reading is
> >> somewhat peculiar. It requires the master to do clock stretching after
> >> sending the I2C slave-address until the slave has captured the data.
> >> Needless to say this is not well suopported by the I2C controllers.
> >>
> >> Thus the driver does not support the BD79124's manual measurement mode
> >> but implements the measurements using automatic measurement mode relying
> >> on the BD79124's ability of storing latest measurements into register.
> >>
> >> The driver does also support configuring the threshold events for
> >> detecting the out-of-window events.
> >>
> >> The BD79124 keeps asserting IRQ for as long as the measured voltage is
> >> out of the configured window. Thus the driver masks the received event
> >> for a fixed duration (1 second) when an event is handled. This prevents
> >> the user-space from choking on the events
> >>
> >> The ADC input pins can be also configured as general purpose outputs.
> >> Those pins which don't have corresponding ADC channel node in the
> >> device-tree will be controllable as GPO.
> >>
> >> Signed-off-by: Matti Vaittinen <mazziesaccount@xxxxxxxxx>  
> > Hi Matti
> > 
> > Just a few really trivial comments.  If all else in the
> > set was resolved I'd probably have applied with a tweak or two
> > 
> > Thanks,
> > 
> > Jonathan
> >   
> >>   obj-$(CONFIG_SC27XX_ADC) += sc27xx_adc.o
> >> diff --git a/drivers/iio/adc/rohm-bd79124.c b/drivers/iio/adc/rohm-bd79124.c
> >> new file mode 100644
> >> index 000000000000..466c7decf8fc
> >> --- /dev/null
> >> +++ b/drivers/iio/adc/rohm-bd79124.c
> >> @@ -0,0 +1,1108 @@  
> > ...
> >   
> >> +
> >> +/* Read-only regs */  
> > 
> > Given naming this is pretty obvious.
>  > I would drop the comment  
> 
> I will drop this, although I am not sure it is as self explatonary as 
> one thinks. I've seen people getting this wrong because the logic of 
> regmap-ranges is kind of reversed. (Eg, read-only is done by adding 
> range to wr_tables and not to rd_tables - as a no-range).
It's obvious in the structure naming + how it is used. I agree
the interface in general is non obvious.

Jonathan

> 
> Thanks for the review, and rest of the comments just agreed with.
> 
> -- Matti