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

[Guenter Roeck]

On 5/2/22 04:21, Maxime Ripard wrote:
> On Mon, May 02, 2022 at 07:15:01PM +0800, Icenowy Zheng wrote:
> > 于 2022年5月2日 GMT+08:00 下午7:00:10, Maxime Ripard <maxime@xxxxxxxxxx> 写到:
> > > Hi,
> > >
> > > On Thu, Apr 28, 2022 at 09:09:03PM +0000, Ruslan Zalata wrote:
> > > > Some Allwinner SoCs like A13, A20 or T2 are equipped with two-channel
> > > > low rate (6 bit) ADC that is often used for extra keys. There's a driver
> > > > for that already implementing standard input device, but it has these
> > > > limitations: 1) it cannot be used for general ADC data equisition, and
> > > > 2) it uses only one LRADC channel of two available.
> > > >
> > > > This driver provides basic hwmon interface to both channels of LRADC on
> > > > such Allwinner SoCs.
> > > >
> > > > Signed-off-by: Ruslan Zalata <rz@xxxxxxxxxxx>
> > > > ---
> > > >   MAINTAINERS                       |   6 +
> > > >   drivers/hwmon/Kconfig             |  13 ++
> > > >   drivers/hwmon/Makefile            |   1 +
> > > >   drivers/hwmon/sun4i-lradc-hwmon.c | 280 ++++++++++++++++++++++++++++++
> > > >   4 files changed, 300 insertions(+)
> > > >   create mode 100644 drivers/hwmon/sun4i-lradc-hwmon.c
> > > >
> > > > diff --git a/MAINTAINERS b/MAINTAINERS
> > > > index 5e8c2f61176..d9c71e94133 100644
> > > > --- a/MAINTAINERS
> > > > +++ b/MAINTAINERS
> > > > @@ -18861,6 +18861,12 @@ S:	Maintained
> > > >   F:	Documentation/devicetree/bindings/input/allwinner,sun4i-a10-lradc-keys.yaml
> > > >   F:	drivers/input/keyboard/sun4i-lradc-keys.c
> > > >   
> > > > +SUN4I LOW RES ADC HWMON DRIVER
> > > > +M:	Ruslan Zalata <rz@xxxxxxxxxxx>
> > > > +L:	linux-hwmon@xxxxxxxxxxxxxxx
> > > > +S:	Maintained
> > > > +F:	drivers/hwmon/sun4i-lradc-hwmon.c
> > > > +
> > > >   SUNDANCE NETWORK DRIVER
> > > >   M:	Denis Kirjanov <kda@xxxxxxxxxxxxxxxxx>
> > > >   L:	netdev@xxxxxxxxxxxxxxx
> > > > diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
> > > > index 68a8a27ab3b..86776488a81 100644
> > > > --- a/drivers/hwmon/Kconfig
> > > > +++ b/drivers/hwmon/Kconfig
> > > > @@ -1691,6 +1691,19 @@ config SENSORS_SIS5595
> > > >   	  This driver can also be built as a module. If so, the module
> > > >   	  will be called sis5595.
> > > >   
> > > > +config SENSORS_SUN4I_LRADC
> > > > +	tristate "Allwinner A13/A20 LRADC hwmon"
> > > > +	depends on ARCH_SUNXI && !KEYBOARD_SUN4I_LRADC
> > > > +	help
> > > > +	  Say y here to support the LRADC found in Allwinner A13/A20 SoCs.
> > > > +	  Both channels are supported.
> > > > +
> > > > +	  This driver can also be built as module. If so, the module
> > > > +	  will be called sun4i-lradc-hwmon.
> > > > +
> > > > +	  This option is not compatible with KEYBOARD_SUN4I_LRADC, one
> > > > +	  of these must be used at a time.
> > >
> > > How do you plan on enforcing that?
> > >
> > > I guess a better path forward would be to either register an hwmon
> > > device in the original driver, or convert that driver to iio and use
> > > iio-hwmon.
> >
> > I think this driver should be use IIO, and then try to probe an IIO input
> > if possible.
>
> It's been a while, but if I remember well we couldn't use IIO for that
> driver because it's not generating interrupts all the time but only when
> it goes over a given threshold:
>
> https://lore.kernel.org/all/52C5E9F1.9010700@xxxxxxxxxx/
>
> I'm not sure if it's still relevant, so we might just need to add an
> hwmon driver to the existing driver

So now we have conflicting claims that the hwmon driver would need
to implement continuous interrupts because the chip otherwise doesn't
continuously measure ADC input, and that implementing an IIO driver
isn't possible or doesn't make sense because the chip would not support
generating continuous interrupts. Which one is it ? Am I missing something ?

Guenter