Re: [PATCH v10 6/8] iio: adc: Support ROHM BD79124 ADC

[Jonathan Cameron <jic23@kernel.org>]

On Mon, 24 Mar 2025 10:23:52 +0200
Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:

> On Mon, Mar 24, 2025 at 09:13:42AM +0200, Matti Vaittinen 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 do not support the BD79124's manual measurement mode but implement
> > the measurements using automatic measurement mode, relying on the
> > BD79124's ability of storing latest measurements into register.
> > 
> > Support also 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, prevent the user-space from choking
> > on the events and mask the received event for a fixed duration (1 second)
> > when an event is handled.
> > 
> > The ADC input pins can be also configured as general purpose outputs.
> > Make those pins which don't have corresponding ADC channel node in the
> > device-tree controllable as GPO.  
> 
> Thank you for the nicely written driver!
> However, I have one big issue with it (see below).
> 
> ...
> 
> > +static void bd79124gpo_set(struct gpio_chip *gc, unsigned int offset, int value)
> > +static void bd79124gpo_set_multiple(struct gpio_chip *gc, unsigned long *mask,
> > +				    unsigned long *bits)  
> 
> These will be _rv variants anyway as there is no chance this series goes before that.

I don't mind seeing this as a follow up series, but I would like that
to hit this cycle if possible.

> 
> ...
> 
> > +struct bd79124_raw {
> > +	u8 val_bit3_0; /* Is set in high bits of the byte */
> > +	u8 val_bit11_4;
> > +};
> > +#define BD79124_RAW_TO_INT(r) ((r.val_bit11_4 << 4) | (r.val_bit3_0 >> 4))
> > +#define BD79124_INT_TO_RAW(val) {					\
> > +	.val_bit11_4 = (val) >> 4,					\
> > +	.val_bit3_0 = (val) << 4,					\
> > +}  
> 
> All the rest is fine to me and looks good, but above is a principal impediment
> to give you my tag. In case you update the type to __le16, feel free to add
> my Reviewed-by tag.

Matti, I think after all the back and forth it is clear I should have
been fussier about this in the RFC as it would have saved time.
My advice in future is once something has been poked twice by reviewers
just change it.  For what it is worth I prefer what Andy is asking for
but not enough to hold this longer.

So applied (with tweak as per other mail) and with assumption you'll chase
this with the gpio chip stuff cleaned up to use the new functions.

Thanks,

Jonathan