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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <20250302024434.67ef4c90@jic23-huawei>
Date: Sun, 2 Mar 2025 02:44:34 +0000
From: Jonathan Cameron <jic23@...nel.org>
To: Matti Vaittinen <mazziesaccount@...il.com>
Cc: Matti Vaittinen <matti.vaittinen@...rohmeurope.com>, Lars-Peter Clausen
 <lars@...afoo.de>, Rob Herring <robh@...nel.org>, Krzysztof Kozlowski
 <krzk+dt@...nel.org>, Conor Dooley <conor+dt@...nel.org>, Lad Prabhakar
 <prabhakar.mahadev-lad.rj@...renesas.com>, Chen-Yu Tsai <wens@...e.org>,
 Jernej Skrabec <jernej.skrabec@...il.com>, Samuel Holland
 <samuel@...lland.org>, Hugo Villeneuve <hvilleneuve@...onoff.com>, Nuno Sa
 <nuno.sa@...log.com>, David Lechner <dlechner@...libre.com>, Javier
 Carrasco <javier.carrasco.cruz@...il.com>, Andy Shevchenko
 <andriy.shevchenko@...ux.intel.com>, linux-iio@...r.kernel.org,
 devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
 linux-renesas-soc@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
 linux-sunxi@...ts.linux.dev, Linus Walleij <linus.walleij@...aro.org>
Subject: Re: [PATCH v3 3/9] iio: adc: Support ROHM BD79124 ADC

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

> On 23/02/2025 18:28, Jonathan Cameron wrote:
> > On Wed, 19 Feb 2025 14:30:43 +0200
> > Matti Vaittinen <mazziesaccount@...il.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@...il.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