linux-kernel - Re: [RFC PATCH 3/5] 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: <20250208130143.121058d6@jic23-huawei>
Date: Sat, 8 Feb 2025 13:01:43 +0000
From: Jonathan Cameron <jic23@...nel.org>
To: Matti Vaittinen <mazziesaccount@...il.com>
Cc: Jonathan Cameron <Jonathan.Cameron@...wei.com>, Matti Vaittinen
 <matti.vaittinen@...rohmeurope.com>, Lee Jones <lee@...nel.org>, Rob
 Herring <robh@...nel.org>, Krzysztof Kozlowski <krzk+dt@...nel.org>, Conor
 Dooley <conor+dt@...nel.org>, Lars-Peter Clausen <lars@...afoo.de>, Linus
 Walleij <linus.walleij@...aro.org>, Nuno Sa <nuno.sa@...log.com>, David
 Lechner <dlechner@...libre.com>, Dumitru Ceclan <mitrutzceclan@...il.com>,
 Trevor Gamblin <tgamblin@...libre.com>, Matteo Martelli
 <matteomartelli3@...il.com>, AngeloGioacchino Del Regno
 <angelogioacchino.delregno@...labora.com>, devicetree@...r.kernel.org,
 linux-kernel@...r.kernel.org, linux-iio@...r.kernel.org,
 linux-gpio@...r.kernel.org
Subject: Re: [RFC PATCH 3/5] iio: adc: Support ROHM BD79124 ADC

On Wed, 5 Feb 2025 15:58:18 +0200
Matti Vaittinen <mazziesaccount@...il.com> wrote:

> On 31/01/2025 19:41, Jonathan Cameron wrote:
> > On Fri, 31 Jan 2025 15:37:48 +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.
> >>  
> 
> Hi Jonathan,
> 
> I just sent the v2, where I (think I) addressed all comments except ones 
> below. Just wanted to point out what was not changed and why :)
> 
> ...
> 
> >   
> >> +struct bd79124_raw {
> >> +	u8 bit0_3; /* Is set in high bits of the byte */
> >> +	u8 bit4_11;
> >> +};
> >> +#define BD79124_RAW_TO_INT(r) ((r.bit4_11 << 4) | (r.bit0_3 >> 4))  
> > You could do this as an endian conversion and a single shift I think.
> > Might be slightly simpler.  
> 
> I kept this struct with bytes matching the register spec. Doing the 
> endian conversion and then shifting would probably have worked, but my 
> head hurts when I try thinking how the bits settle there. Especially if 
> this is done on a big-endian machine. I can rework this for v3 if you 
> feel very strongly about this.

The key is that an endian conversion is always the same as OR + SHIFT
because that is being done in the system endiannes.

Doesn't matter that much, but we may see follow up patches switching
this over to the endian handlers.

From datasheet point of view it tends to depend on whether they show
an illustration of a bulk read or not to whether it's described
as a multi byte value, or as bits in smaller registers.

> 
> ...
> 
> >   
> >> +static irqreturn_t bd79124_event_handler(int irq, void *priv)
> >> +{
> >> +	int ret, i_hi, i_lo, i;
> >> +	struct iio_dev *idev = priv;
> >> +	struct bd79124_data *d = iio_priv(idev);
> >> +
> >> +	/*
> >> +	 * Return IRQ_NONE if bailing-out without acking. This allows the IRQ
> >> +	 * subsystem to disable the offending IRQ line if we get a hardware
> >> +	 * problem. This behaviour has saved my poor bottom a few times in the
> >> +	 * past as, instead of getting unusably unresponsive, the system has
> >> +	 * spilled out the magic words "...nobody cared".  
> > *laughs*.  Maybe the comment isn't strictly necessary but it cheered
> > up my Friday.  
> >> +	 */
> >> +	ret = regmap_read(d->map, BD79124_REG_EVENT_FLAG_HI, &i_hi);
> >> +	if (ret)
> >> +		return IRQ_NONE;
> >> +
> >> +	ret = regmap_read(d->map, BD79124_REG_EVENT_FLAG_LO, &i_lo);
> >> +	if (ret)
> >> +		return IRQ_NONE;
> >> +
> >> +	if (!i_lo && !i_hi)
> >> +		return IRQ_NONE;
> >> +
> >> +	for (i = 0; i < BD79124_MAX_NUM_CHANNELS; i++) {
> >> +		u64 ecode;
> >> +
> >> +		if (BIT(i) & i_hi) {  
> > Maybe cleaner as a pair of
> > 
> > for_each_set_bit() loops.
> >   
> 
> I kept the original for 2 reasons.
> 
> 1. the main reason is that the for_each_set_bit() would want the value 
> read from a register to be in long. Regmap wants to use int. Solving 
> this produced (in my 'humblish' opinion) less readable code.
> 
> 2. The current implementation has only one loop, which should perhaps be 
> a tiny bit more efficient.

OK.
> 
> >> +			ecode = IIO_UNMOD_EVENT_CODE(IIO_VOLTAGE, i,
> >> +					IIO_EV_TYPE_THRESH, IIO_EV_DIR_RISING);
> >> +
> >> +			iio_push_event(idev, ecode, d->timestamp);
> >> +			/*
> >> +			 * The BD79124 keeps the IRQ asserted for as long as
> >> +			 * the voltage exceeds the threshold. It may not serve
> >> +			 * the purpose to keep the IRQ firing and events
> >> +			 * generated in a loop because it may yield the
> >> +			 * userspace to have some problems when event handling
> >> +			 * there is slow.
> >> +			 *
> >> +			 * Thus, we disable the event for the channel. Userspace
> >> +			 * needs to re-enable the event.  
> > 
> > That's not pretty. So I'd prefer a timeout and autoreenable if we can.  
> 
> And I did this, but with constant 1 sec 'grace time' instead of 
> modifiable time-out. I believe this suffices and keeps it simpler.

We might want to present that value to userspace anyway at somepoint, but
a fixed value is fine.

Jonathan

> 
> 
> Yours,
> 	-- Matti