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

From: Ruslan Zalata
Date: Tue May 03 2022 - 11:27:28 EST


Hi Guenter,

LRADC does generate continuous interrupts as long as input voltage is below LevelB threshold. The max possible LevelB is 0x3C which in case of A20 SoC is close to 1.90V and that's what my driver sets LevelB to. Perhaps this needs to be documented more thoroughly.

It is possible to implement this same driver for IIO subsystem, but I would prefer to keep it in hwmon along with many other simple ADC drivers used for temp and battery status monitoring.

If we talk about IIO, it will be necessary to implement serialization of reads and updates which brings up some complexity I would try to avoid at the moment. :)

---
Regards,
Ruslan.

Fabmicro, LLC.

On 2022-05-03 07:02, Guenter Roeck wrote:
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