Re: [PATCH v3 3/9] iio: adc: Support ROHM BD79124 ADC

[Jonathan Cameron <jic23@kernel.org>]

On Mon, 24 Feb 2025 08:14:23 +0200
Matti Vaittinen <mazziesaccount@gmail.com> wrote:

> On 23/02/2025 18:28, Jonathan Cameron wrote:
> > On Wed, 19 Feb 2025 14:30:43 +0200
> > Matti Vaittinen <mazziesaccount@gmail.com> 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@gmail.com>
> >>  
> > Hi Matti,
> > 
> > Some fairly superficial review follows. I'm travelling for next few weeks
> > so not sure when I'll get time to take a more thorough look.  
> 
> Yeah, unfortunately people are allowed to have other life beyond the 
> ROHM drivers :D
> Enjoy your journey(s) ;)

So far so good.  Hi from Shenzhen.  Obligatory pilgrimage to SEG market
done ;)

> >> +	/* Set no channels to be manually measured */
> >> +	ret = regmap_write(data->map, BD79124_REG_MANUAL_CHANNELS, 0x0);
> >> +	if (ret)
> >> +		return ret;
> >> +
> >> +	/* Set the measurement interval to 0.75 mS */
> >> +	regval = FIELD_PREP(BD79124_MASK_AUTO_INTERVAL, BD79124_INTERVAL_075);
> >> +	ret = regmap_update_bits(data->map, BD79124_REG_OPMODE_CFG,
> >> +			BD79124_MASK_AUTO_INTERVAL, regval);  
> > 
> > Where it doesn't make any other difference, align after (
> > 
> > If you are going shorter, single tab only.  
> 
> Single tab only? You mean like:
> 
> ret = regmap_update_bits(data->map, BD79124_REG_OPMODE_CFG,
> 	BD79124_MASK_AUTO_INTERVAL, regval);
> 
> Do you prefer that even if the variable holding the return value was 
> longer than 8 chars? To me it looks odd if arguments on the next line 
> begin earlier than the function on previous line:
> 
> longvariable = regmap_update_bits(data->map, BD79124_REG_OPMODE_CFG,
> 	BD79124_MASK_AUTO_INTERVAL, regval);
> 
> (Just ensuring I understood your preference).
It's hard to come up with an absolute policy / preference on this but
whilst I agree it looks a bit odd, I think it's easier to say one
tab as 'default' choice.  Obviously if it's really hideous for some
reason feel free to do something else ;)

Jonathan