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.
> >
> > 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.
>
>  From what I recall that is in the I2C spec, so in theory should be supported.
> Ah well.

Could be I am mistaken then. Or, maybe I just misused the term "master 
to do clock stretching".

I know that it is not rare the slave device is keeping the clock down 
for extended period (in this case so that the measurement would be 
completed) - but at least I am not aware of any APIs which could be used 
to cause the _master_ side to keep the SCL low for an extended period 
after receiving the ACK (after sending the slave address). In this case 
it would require this driver to be able to set a time for how long the 
master would keep SCL low after sensing the slave address, before 
sending the "command" bytes.

|S|ADDRESS+R|a|STRETCH|8-bit-i2c-frame|A|8-bit-i2c-frame|A|STRETCH|8-bit-i2c...

Above denotes this "master stretching". CAPITALs are initiated by 
master, lowercase by slave. S, is start, a is ack and R is read-bit.

If there is a standard way to implement this in Linux side, then I might 
consider using it as it'd allowed much higher capture rates.

> > It is worth noting that the ADC input pins can be also configured as
> > general purpose outputs. The pin mode should be configured using pincmux
> > driver.
>
> We shouldn't be presenting channels that are configure for GPIOs as
> ADC channels.  It is very rare that there is a usecase for any
> dynamic switching.

Thanks :) If the dynamic switching is rare, then you're definitely 
right. I need to see if using the pinmux still makes sense, and if we 
can implement this while using (separate) pinmux driver.

> Normally it's a case of what is wired and
> so static.

I should implement a device which can be controlled via it's analog 
output line :) If nothing else then a device shutting down when it's 
output is pulled low ;)

...Well, I have no real use-case for dynamic config either.

>  Hence build the iio_chan_spec array for just the
> channels you want, not the the lot.  Channel sub nodes in the
> DT are how we most commonly specify what is wired.

Hmm. That'd mean the ADC channels _must_ be defined in DT in order to be 
usable(?) Well, if this is the usual way, then it should be well known 
by users. Thanks.

...

> > +		if (BIT(i) & i_lo) {
> > +			ecode = IIO_UNMOD_EVENT_CODE(IIO_VOLTAGE, i,
> > +					IIO_EV_TYPE_THRESH, IIO_EV_DIR_FALLING);
> > +
> > +			iio_push_event(idev, ecode, d->timestamp);
> The same interrupt thing doesn't apply to falling? That's odd.

It does. So, not odd, just a regular bug :) Thanks!

> > +		}
> > +	}
> > +

...

> > +
> > +	irq = platform_get_irq_byname_optional(pdev, "thresh-alert");
>
> Is there more than one?  If not why do we need to do it by name?

I kind of like doing it by name when the IRQs come from a MFD device - 
which can name them. It is no real extra cost (well, maybe bytes of the 
added string in kernel, but I guess it's not relevant with only few 
interrupts) - but it makes it very hard to get it wrong, and it 
documents the purpose of the IRQ.

> > +	if (irq < 0) {
> > +		if (irq == -EPROBE_DEFER)
> > +			return irq;

...

I do thank you for, and agree with, all the rest of the comments!

Yours,
	-- Matti