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 for Android: free password hash cracker in your pocket

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

Message-ID: <8353a96d-fe39-45c2-b6da-e8083a6bdcd8@gmail.com>
Date: Wed, 5 Feb 2025 15:58:18 +0200
From: Matti Vaittinen <mazziesaccount@...il.com>
To: Jonathan Cameron <Jonathan.Cameron@...wei.com>
Cc: 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>, Jonathan Cameron <jic23@...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 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.

...

> 
>> +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.

>> +			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.


Yours,
	-- Matti