Re: [RFC PATCH 3/5] iio: adc: Support ROHM BD79124 ADC

[Matti Vaittinen]

On 31/01/2025 19:41, Jonathan Cameron wrote:
> On Fri, 31 Jan 2025 15:37:48 +0200
> Matti Vaittinen <mazziesaccount@xxxxxxxxx> 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