Re: [PATCH v2] hwmon: (sun4i-lradc) Add driver for LRADC found on Allwinner A13/A20 SoC

[Maxime Ripard <maxime@cerno.tech>]

[-- Attachment #1: Type: text/plain, Size: 3735 bytes --]

On Wed, May 04, 2022 at 02:37:32AM +0500, Ruslan Zalata wrote:
> Hi Maxime,
> 
> > At the hardware level, I'd assume you would either use the LRADC as an
> > actual ADC, or use it to drive buttons, right?
> 
> Yes, exactly.
> 
> > So I don't think a new device tree binding is such a deal breaker since
> > you have to describe it differently anyway.
> 
> ...
> 
> > Since that would be a completely different use-case, the IIO driver
> > doesn't have to support input right away, it can be done later if
> > needed.
> > 
> > And you could have the two drivers compiled at the same time.
> 
> As I got you right, you propose do add new bindings, say
> "allwinner,sun4i-a10-lradc-hwmon" and "allwinner,sun8i-a83t-lradc-hwmon" for
> new driver, which will allow two drivers (hwmon and keyboard) be compiled
> and loaded at same time, only that one listed in DT will be instantiated.

Compatibles are meant to describe the hardware and remain OS-agnostic,
while hwmon is linux-specific so we should probably drop the hwmon from
the compatible. But otherwise, yes.

> If two are listed at same time, one of the calls to devm_request_irq()
> will return with an error preventing second driver to be probed (some
> error message would be necessary to let user know what's going on). If
> this is ok, I will implement it.
> 
> I think moving this driver to IIO framework is overkill. We use LRADC to
> monitor battery temp and state (voltage) and that's what HWMON was made for.
> It's simple, easy and elegant.

Yeah, that's that *you* use it for. If someone wants to use it for some
other use, what's going to happen? Will we create a third driver for the
exact same controller? That's not reasonable.

> IIO, on the other hand, is for data acquisition and is much more
> complex beast. Can we stay with HWMON, please ? :)

But it's generic, and you can plug hwmon on top of it. So you don't lose
any feature, but it also doesn't prevent any one else to use it for some
slightly different usecase either.

> I looked through the code for a number of iio/adc drivers and I could see
> that all of them initiate ADC conversion inside read(), then wait for
> completion and return single sample. For me this very flawed approach
> because a) much more overhead/load on the system,

To monitor a battery? What kind of polling interval do you expect on this?

> b) initiating conversion may (and will) take more time than a single
> consequent conversion,

Ditto, I'd expect to have a request in the order of seconds for a
battery, so the setup time will be negligible.

> c) samples will be read in irregular periods of time, hence acquired
> data will not be consistent for any further processing (like FFT). So,
> this whole IIO framework is no way better than HWMON, yet more
> complex. At least for ADCs. :-)
>
> A better approach for an IIO/ADC driver would be to implement some
> serialization mechanism to let reads go in sync with updates (IRQs), with
> buffering, guaranteeing no same sample is read twice and no sample is lost.
> The read() would return next available sample from buffer with nearly zero
> overhead or sleep till data is available.

Like this:
https://www.kernel.org/doc/html/latest/driver-api/iio/buffers.html ?

> And the best way is to extend IIO framework to support ring buffers
> mechanism like the one proposed by Analog Devices, but that's a way
> different story. Link: https://events.static.linuxfound.org/sites/events/files/slides/iio_high_speed.pdf

Those suggestions were to handle more than a ~100 kilosamples per
seconds order of magnitude. How many samples per seconds do you expect
to get out from this ADC?

Maxime

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]