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

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

[Jamie Cuthbert]

Sent from my iPhone

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

[Jamie Cuthbert]

Sent from my iPhone

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

[Guenter Roeck]

On 4/28/22 14:09, 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

acquisition

> 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@fabmicro.ru>

Ok, next phase of review.


> ---
>   MAINTAINERS                       |   6 +
>   drivers/hwmon/Kconfig             |  13 ++
>   drivers/hwmon/Makefile            |   1 +
>   drivers/hwmon/sun4i-lradc-hwmon.c | 280 ++++++++++++++++++++++++++++++

Needs documentation in Documentation/hwmon/sun4i-lradc-hwmon.rst,
and don't forget to add it to Documentation/hwmon/index.rst.

>   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@fabmicro.ru>
> +L:	linux-hwmon@vger.kernel.org
> +S:	Maintained
> +F:	drivers/hwmon/sun4i-lradc-hwmon.c
> +
>   SUNDANCE NETWORK DRIVER
>   M:	Denis Kirjanov <kda@linux-powerpc.org>
>   L:	netdev@vger.kernel.org
> 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

only one of these

> +	  of these must be used at a time.
> +
>   config SENSORS_SY7636A
>   	tristate "Silergy SY7636A"
>   	help
> diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile
> index 8a03289e2aa..3e5dc902acf 100644
> --- a/drivers/hwmon/Makefile
> +++ b/drivers/hwmon/Makefile
> @@ -187,6 +187,7 @@ obj-$(CONFIG_SENSORS_SMSC47M1)	+= smsc47m1.o
>   obj-$(CONFIG_SENSORS_SMSC47M192)+= smsc47m192.o
>   obj-$(CONFIG_SENSORS_SPARX5)	+= sparx5-temp.o
>   obj-$(CONFIG_SENSORS_STTS751)	+= stts751.o
> +obj-$(CONFIG_SENSORS_SUN4I_LRADC) += sun4i-lradc-hwmon.o
>   obj-$(CONFIG_SENSORS_SY7636A)	+= sy7636a-hwmon.o
>   obj-$(CONFIG_SENSORS_AMC6821)	+= amc6821.o
>   obj-$(CONFIG_SENSORS_TC74)	+= tc74.o
> diff --git a/drivers/hwmon/sun4i-lradc-hwmon.c b/drivers/hwmon/sun4i-lradc-hwmon.c
> new file mode 100644
> index 00000000000..8d5268b037b
> --- /dev/null
> +++ b/drivers/hwmon/sun4i-lradc-hwmon.c
> @@ -0,0 +1,280 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * Allwinner sun4i (A13/A20) LRADC hwmon driver
> + *
> + * Copyright (C) 2022 Fabmicro, LLC., Tyumen, Russia.
> + * Copyright (C) 2022 Ruslan Zalata <rz@fabmicro.ru>
> + */
> +
> +#include <linux/err.h>
> +#include <linux/init.h>
> +#include <linux/input.h>
> +#include <linux/interrupt.h>
> +#include <linux/module.h>
> +#include <linux/of_platform.h>
> +#include <linux/platform_device.h>
> +#include <linux/slab.h>
> +#include <linux/hwmon.h>
> +#include <linux/hwmon-sysfs.h>

Should not be needed.

> +
> +#define LRADC_CTRL	0x00
> +#define LRADC_INTC	0x04
> +#define LRADC_INTS	0x08
> +#define LRADC_DATA0	0x0c
> +#define LRADC_DATA1	0x10
> +
> +/* LRADC_CTRL bits */
> +#define FIRST_CONVERT_DLY(x)	((x) << 24) /* 8 bits */
> +#define CHAN_SELECT(x)		((x) << 22) /* 2 bits */
> +#define CONTINUE_TIME_SEL(x)	((x) << 16) /* 4 bits */
> +#define KEY_MODE_SEL(x)		((x) << 12) /* 2 bits */
> +#define LEVELA_B_CNT(x)		((x) << 8)  /* 4 bits */
> +#define HOLD_KEY_EN(x)		((x) << 7)
> +#define HOLD_EN(x)		((x) << 6)
> +#define LEVELB_VOL(x)		((x) << 4)  /* 2 bits */
> +#define SAMPLE_RATE(x)		((x) << 2)  /* 2 bits */
> +#define ENABLE(x)		((x) << 0)
> +
> +/* LRADC_INTC and LRADC_INTS bits */
> +#define CHAN1_KEYUP_IRQ		BIT(12)
> +#define CHAN1_ALRDY_HOLD_IRQ	BIT(11)
> +#define CHAN1_HOLD_IRQ		BIT(10)
> +#define CHAN1_KEYDOWN_IRQ	BIT(9)
> +#define CHAN1_DATA_IRQ		BIT(8)
> +#define CHAN0_KEYUP_IRQ		BIT(4)
> +#define CHAN0_ALRDY_HOLD_IRQ	BIT(3)
> +#define CHAN0_HOLD_IRQ		BIT(2)
> +#define CHAN0_KEYDOWN_IRQ	BIT(1)
> +#define CHAN0_DATA_IRQ		BIT(0)
> +
> +struct lradc_variant {
> +	u32 bits;
> +	u32 resolution;
> +	u32 vref;
> +};
> +
> +struct sun4i_lradc_data {
> +	struct device *dev;
> +	void __iomem *base;
> +	const struct lradc_variant *variant;
> +	struct mutex update_lock; /* atomic read and data updates */
> +	u32 in[2];
> +};
> +
> +struct lradc_variant variant_sun4i_a10_lradc = {
> +	.bits = 0x3f,
> +	.resolution = 63,
> +	.vref = 2000000, /* Vref = 2.0V from SoC datasheet */
> +};
> +
> +static int sun4i_lradc_read(struct device *dev, enum hwmon_sensor_types type,
> +			    u32 attr, int channel, long *val)
> +{
> +	struct sun4i_lradc_data *lradc = dev_get_drvdata(dev);
> +	int in;
> +
> +	if (IS_ERR(lradc))
> +		return PTR_ERR(lradc);

That won't happen. Unnecessary check.

> +
> +	mutex_lock(&lradc->update_lock);
> +	in = lradc->in[channel];


Pointless. Just use

> +	mutex_unlock(&lradc->update_lock);
> +
> +	switch (attr) {
> +	case hwmon_in_input:
> +		*val = in;

		*val = lradc->in[channel];

here. The operation is atomic.

> +		break;
> +
> +	default:
> +		return -EOPNOTSUPP;
> +	}
> +	return 0;
> +}
> +
> +umode_t sun4i_lradc_is_visible(const void *data, enum hwmon_sensor_types type,
> +			       u32 attr, int channel)
> +{
> +	return 0444;
> +}
> +
> +static const u32 sun4i_lradc_chip_config[] = {
> +	HWMON_I_INPUT,

This is wrong. Chip wide attributes are named HWMON_C_XXX.
This generated a reset_history attribute which isn't really
supported or handled. Chip attributes should only be specified
if actually used. Drop this.

> +	0
> +};
> +
> +static const struct hwmon_channel_info sun4i_lradc_chip = {
> +	.type = hwmon_chip,
> +	.config = sun4i_lradc_chip_config,
> +};
> +
> +static const u32 sun4i_lradc_input_config[] = {
> +	HWMON_I_INPUT,
> +	HWMON_I_INPUT,
> +	0
> +};
> +
> +static const struct hwmon_channel_info sun4i_lradc_input = {
> +	.type = hwmon_in,
> +	.config = sun4i_lradc_input_config,
> +};
> + > +static const struct hwmon_channel_info *sun4i_lradc_info[] = {
> +	&sun4i_lradc_chip,
> +	&sun4i_lradc_input,
> +	NULL
> +};
> +
Use the HWMON_CHANNEL_INFO macro instead.

> +static const struct hwmon_ops sun4i_lradc_hwmon_ops = {
> +	.is_visible = sun4i_lradc_is_visible,
> +	.read = sun4i_lradc_read,
> +};
> +
> +static const struct hwmon_chip_info sun4i_lradc_chip_info = {
> +	.ops = &sun4i_lradc_hwmon_ops,
> +	.info = sun4i_lradc_info,
> +};
> +
> +static irqreturn_t sun4i_lradc_irq(int irq, void *dev_id)
> +{
> +	struct sun4i_lradc_data *lradc = dev_id;
> +	u32 ints = readl(lradc->base + LRADC_INTS);
> +
> +	mutex_lock(&lradc->update_lock);
> +
> +	if (ints & CHAN0_DATA_IRQ)
> +		lradc->in[0] = (readl(lradc->base + LRADC_DATA0) & lradc->variant->bits) *
> +			lradc->variant->vref / lradc->variant->resolution / 1000; /* to mV */
> +
> +	if (ints & CHAN1_DATA_IRQ)
> +		lradc->in[1] = (readl(lradc->base + LRADC_DATA1) & lradc->variant->bits) *
> +			lradc->variant->vref / lradc->variant->resolution / 1000; /* to mV */
> +
> +	mutex_unlock(&lradc->update_lock);
> +
> +	writel(ints, lradc->base + LRADC_INTS);
> +
> +	return IRQ_HANDLED;
> +}
> +

You don't explain why the interrupt handler is needed. It is undesirable
to do this for hwnon devices because it results in extra load on the system.
The ADC should be read only when needed and not continuously. If that is not
possible for some reason it needs to be explained in detail.

> +static int sun4i_lradc_start(struct sun4i_lradc_data *lradc)
> +{
> +	writel(FIRST_CONVERT_DLY(2) | CHAN_SELECT(3) | LEVELA_B_CNT(1) |
> +		HOLD_EN(1) | KEY_MODE_SEL(2) | SAMPLE_RATE(3) | ENABLE(1),
> +		lradc->base + LRADC_CTRL);
> +

If the update rate is configurable, it might make sense to support
the update_interval attribute.

> +	writel(CHAN0_DATA_IRQ | CHAN1_DATA_IRQ, lradc->base + LRADC_INTC);
> +
> +	return 0;
> +}
> +
> +static void sun4i_lradc_stop(void *data)
> +{
> +	struct sun4i_lradc_data *lradc = (struct sun4i_lradc_data *)data;
> +
> +	writel(FIRST_CONVERT_DLY(2) | LEVELA_B_CNT(1) | HOLD_EN(1) |
> +		SAMPLE_RATE(2), lradc->base + LRADC_CTRL);
> +	writel(0, lradc->base + LRADC_INTC);
> +}
> +
> +static int sun4i_lradc_probe(struct platform_device *pdev)
> +{
> +	struct sun4i_lradc_data *lradc;
> +	struct device *dev = &pdev->dev;
> +	struct device *hwmon_dev;
> +	struct resource *res;
> +	int hwmon_irq, error;
> +
> +	lradc = devm_kzalloc(dev, sizeof(struct sun4i_lradc_data), GFP_KERNEL);
> +	if (!lradc)
> +		return -ENOMEM;
> +
> +	dev_set_drvdata(dev, lradc);
> +
> +	lradc->variant = of_device_get_match_data(&pdev->dev);
> +	if (!lradc->variant)
> +		return -EINVAL;
> +
> +	lradc->dev = dev;
> +

I don't immediately see where this is used.

> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	if (IS_ERR(res))
> +		return PTR_ERR(res);
> +
> +	lradc->base = devm_ioremap_resource(dev, res);
> +	if (IS_ERR(lradc->base))
> +		return PTR_ERR(lradc->base);
> +
> +	hwmon_irq = platform_get_irq(pdev, 0);
> +	if (hwmon_irq < 0)
> +		return hwmon_irq;
> +
> +	error = devm_request_irq(dev, hwmon_irq, sun4i_lradc_irq, 0, "sun4i-lradc-hwmon", lradc);
> +	if (error) {
> +		dev_err(dev, "sun4i-lradc-hwmon IRQ failed err=%d\n", error);
> +		return error;
> +	}
> +
> +	hwmon_dev = devm_hwmon_device_register_with_info(dev, "lradc-hwmon", lradc,
> +							 &sun4i_lradc_chip_info, NULL);
> +
> +	if (IS_ERR(hwmon_dev)) {
> +		error = PTR_ERR(hwmon_dev);
> +		return error;
> +	}
> +
> +	error = devm_add_action_or_reset(dev, sun4i_lradc_stop, lradc);
> +	if (error)
> +		return error;
> +
> +	error = sun4i_lradc_start(lradc);
> +
> +	return error;
> +}
> +
> +#ifdef CONFIG_PM
> +static int sun4i_lradc_resume(struct device *dev)
> +{
> +	struct sun4i_lradc_data *lradc = dev_get_drvdata(dev);
> +
> +	return sun4i_lradc_start(lradc);
> +}
> +
> +static int sun4i_lradc_suspend(struct device *dev)
> +{
> +	struct sun4i_lradc_data *lradc = dev_get_drvdata(dev);
> +
> +	sun4i_lradc_stop(lradc);
> +	return 0;
> +}
> +
> +#define SUN4I_LRADC_DEV_PM_OPS	(&sun4i_lradc_dev_pm_ops)
> +#else
> +#define SUN4I_LRADC_DEV_PM_OPS	NULL
> +#endif /* CONFIG_PM */
> +
> +static const struct dev_pm_ops sun4i_lradc_dev_pm_ops = {
> +	.suspend = sun4i_lradc_suspend,
> +	.resume = sun4i_lradc_resume,
> +};
> +
> +static const struct of_device_id sun4i_lradc_of_match[] = {
> +	{ .compatible = "allwinner,sun4i-a10-lradc-keys", .data = &variant_sun4i_a10_lradc},
> +	{ /* sentinel */ }
> +};
> +MODULE_DEVICE_TABLE(of, sun4i_lradc_of_match);
> +
> +static struct platform_driver sun4i_lradc_driver = {
> +	.driver = {
> +		.name	= "sun4i-lradc-hwmon",
> +		.of_match_table = of_match_ptr(sun4i_lradc_of_match),
> +		.pm = SUN4I_LRADC_DEV_PM_OPS,
> +	},
> +	.probe	= sun4i_lradc_probe,
> +};
> +
> +module_platform_driver(sun4i_lradc_driver);
> +
> +MODULE_DESCRIPTION("Allwinner A13/A20 LRADC hwmon driver");
> +MODULE_AUTHOR("Ruslan Zalata <rz@fabmicro.ru>");
> +MODULE_LICENSE("GPL");
> +No empty line at end, please

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

[Ruslan Zalata]

Thank you Guenter for your valuable time.

I have added update_interval option (it's in ms units, right?) and fixed 
all other issues you pointed to. Will test it on real hardware and send 
third version of the patch for review.

Regarding IRQ. Alternatively the driver would need to sit and poll 
conversion ready bit in a loop which might cause a much worse load on 
system, is not it ? Anyway, the real problem with this piece of hardware 
is that there's no "conversion ready bit" provided, the only way to know 
data ready status is to receive an interrupt.

I think it still needs a semaphore/seqlock to synchronize conversions 
and reads. I.e. two consequent reads should not return same old value. 
Although it's not an issue in my case, but could be a problem for 
others.

---
Regards,
Ruslan.

Fabmicro, LLC.

On 2022-04-29 02:30, Guenter Roeck wrote:
> On 4/28/22 14:09, 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
> 
> acquisition
> 
>> 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@fabmicro.ru>
> 
> Ok, next phase of review.
> 
> 
>> ---
>>   MAINTAINERS                       |   6 +
>>   drivers/hwmon/Kconfig             |  13 ++
>>   drivers/hwmon/Makefile            |   1 +
>>   drivers/hwmon/sun4i-lradc-hwmon.c | 280 
>> ++++++++++++++++++++++++++++++
> 
> Needs documentation in Documentation/hwmon/sun4i-lradc-hwmon.rst,
> and don't forget to add it to Documentation/hwmon/index.rst.
> 
>>   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@fabmicro.ru>
>> +L:	linux-hwmon@vger.kernel.org
>> +S:	Maintained
>> +F:	drivers/hwmon/sun4i-lradc-hwmon.c
>> +
>>   SUNDANCE NETWORK DRIVER
>>   M:	Denis Kirjanov <kda@linux-powerpc.org>
>>   L:	netdev@vger.kernel.org
>> 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
> 
> only one of these
> 
>> +	  of these must be used at a time.
>> +
>>   config SENSORS_SY7636A
>>   	tristate "Silergy SY7636A"
>>   	help
>> diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile
>> index 8a03289e2aa..3e5dc902acf 100644
>> --- a/drivers/hwmon/Makefile
>> +++ b/drivers/hwmon/Makefile
>> @@ -187,6 +187,7 @@ obj-$(CONFIG_SENSORS_SMSC47M1)	+= smsc47m1.o
>>   obj-$(CONFIG_SENSORS_SMSC47M192)+= smsc47m192.o
>>   obj-$(CONFIG_SENSORS_SPARX5)	+= sparx5-temp.o
>>   obj-$(CONFIG_SENSORS_STTS751)	+= stts751.o
>> +obj-$(CONFIG_SENSORS_SUN4I_LRADC) += sun4i-lradc-hwmon.o
>>   obj-$(CONFIG_SENSORS_SY7636A)	+= sy7636a-hwmon.o
>>   obj-$(CONFIG_SENSORS_AMC6821)	+= amc6821.o
>>   obj-$(CONFIG_SENSORS_TC74)	+= tc74.o
>> diff --git a/drivers/hwmon/sun4i-lradc-hwmon.c 
>> b/drivers/hwmon/sun4i-lradc-hwmon.c
>> new file mode 100644
>> index 00000000000..8d5268b037b
>> --- /dev/null
>> +++ b/drivers/hwmon/sun4i-lradc-hwmon.c
>> @@ -0,0 +1,280 @@
>> +// SPDX-License-Identifier: GPL-2.0-or-later
>> +/*
>> + * Allwinner sun4i (A13/A20) LRADC hwmon driver
>> + *
>> + * Copyright (C) 2022 Fabmicro, LLC., Tyumen, Russia.
>> + * Copyright (C) 2022 Ruslan Zalata <rz@fabmicro.ru>
>> + */
>> +
>> +#include <linux/err.h>
>> +#include <linux/init.h>
>> +#include <linux/input.h>
>> +#include <linux/interrupt.h>
>> +#include <linux/module.h>
>> +#include <linux/of_platform.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/slab.h>
>> +#include <linux/hwmon.h>
>> +#include <linux/hwmon-sysfs.h>
> 
> Should not be needed.
> 
>> +
>> +#define LRADC_CTRL	0x00
>> +#define LRADC_INTC	0x04
>> +#define LRADC_INTS	0x08
>> +#define LRADC_DATA0	0x0c
>> +#define LRADC_DATA1	0x10
>> +
>> +/* LRADC_CTRL bits */
>> +#define FIRST_CONVERT_DLY(x)	((x) << 24) /* 8 bits */
>> +#define CHAN_SELECT(x)		((x) << 22) /* 2 bits */
>> +#define CONTINUE_TIME_SEL(x)	((x) << 16) /* 4 bits */
>> +#define KEY_MODE_SEL(x)		((x) << 12) /* 2 bits */
>> +#define LEVELA_B_CNT(x)		((x) << 8)  /* 4 bits */
>> +#define HOLD_KEY_EN(x)		((x) << 7)
>> +#define HOLD_EN(x)		((x) << 6)
>> +#define LEVELB_VOL(x)		((x) << 4)  /* 2 bits */
>> +#define SAMPLE_RATE(x)		((x) << 2)  /* 2 bits */
>> +#define ENABLE(x)		((x) << 0)
>> +
>> +/* LRADC_INTC and LRADC_INTS bits */
>> +#define CHAN1_KEYUP_IRQ		BIT(12)
>> +#define CHAN1_ALRDY_HOLD_IRQ	BIT(11)
>> +#define CHAN1_HOLD_IRQ		BIT(10)
>> +#define CHAN1_KEYDOWN_IRQ	BIT(9)
>> +#define CHAN1_DATA_IRQ		BIT(8)
>> +#define CHAN0_KEYUP_IRQ		BIT(4)
>> +#define CHAN0_ALRDY_HOLD_IRQ	BIT(3)
>> +#define CHAN0_HOLD_IRQ		BIT(2)
>> +#define CHAN0_KEYDOWN_IRQ	BIT(1)
>> +#define CHAN0_DATA_IRQ		BIT(0)
>> +
>> +struct lradc_variant {
>> +	u32 bits;
>> +	u32 resolution;
>> +	u32 vref;
>> +};
>> +
>> +struct sun4i_lradc_data {
>> +	struct device *dev;
>> +	void __iomem *base;
>> +	const struct lradc_variant *variant;
>> +	struct mutex update_lock; /* atomic read and data updates */
>> +	u32 in[2];
>> +};
>> +
>> +struct lradc_variant variant_sun4i_a10_lradc = {
>> +	.bits = 0x3f,
>> +	.resolution = 63,
>> +	.vref = 2000000, /* Vref = 2.0V from SoC datasheet */
>> +};
>> +
>> +static int sun4i_lradc_read(struct device *dev, enum 
>> hwmon_sensor_types type,
>> +			    u32 attr, int channel, long *val)
>> +{
>> +	struct sun4i_lradc_data *lradc = dev_get_drvdata(dev);
>> +	int in;
>> +
>> +	if (IS_ERR(lradc))
>> +		return PTR_ERR(lradc);
> 
> That won't happen. Unnecessary check.
> 
>> +
>> +	mutex_lock(&lradc->update_lock);
>> +	in = lradc->in[channel];
> 
> 
> Pointless. Just use
> 
>> +	mutex_unlock(&lradc->update_lock);
>> +
>> +	switch (attr) {
>> +	case hwmon_in_input:
>> +		*val = in;
> 
> 		*val = lradc->in[channel];
> 
> here. The operation is atomic.
> 
>> +		break;
>> +
>> +	default:
>> +		return -EOPNOTSUPP;
>> +	}
>> +	return 0;
>> +}
>> +
>> +umode_t sun4i_lradc_is_visible(const void *data, enum 
>> hwmon_sensor_types type,
>> +			       u32 attr, int channel)
>> +{
>> +	return 0444;
>> +}
>> +
>> +static const u32 sun4i_lradc_chip_config[] = {
>> +	HWMON_I_INPUT,
> 
> This is wrong. Chip wide attributes are named HWMON_C_XXX.
> This generated a reset_history attribute which isn't really
> supported or handled. Chip attributes should only be specified
> if actually used. Drop this.
> 
>> +	0
>> +};
>> +
>> +static const struct hwmon_channel_info sun4i_lradc_chip = {
>> +	.type = hwmon_chip,
>> +	.config = sun4i_lradc_chip_config,
>> +};
>> +
>> +static const u32 sun4i_lradc_input_config[] = {
>> +	HWMON_I_INPUT,
>> +	HWMON_I_INPUT,
>> +	0
>> +};
>> +
>> +static const struct hwmon_channel_info sun4i_lradc_input = {
>> +	.type = hwmon_in,
>> +	.config = sun4i_lradc_input_config,
>> +};
>> + > +static const struct hwmon_channel_info *sun4i_lradc_info[] = {
>> +	&sun4i_lradc_chip,
>> +	&sun4i_lradc_input,
>> +	NULL
>> +};
>> +
> Use the HWMON_CHANNEL_INFO macro instead.
> 
>> +static const struct hwmon_ops sun4i_lradc_hwmon_ops = {
>> +	.is_visible = sun4i_lradc_is_visible,
>> +	.read = sun4i_lradc_read,
>> +};
>> +
>> +static const struct hwmon_chip_info sun4i_lradc_chip_info = {
>> +	.ops = &sun4i_lradc_hwmon_ops,
>> +	.info = sun4i_lradc_info,
>> +};
>> +
>> +static irqreturn_t sun4i_lradc_irq(int irq, void *dev_id)
>> +{
>> +	struct sun4i_lradc_data *lradc = dev_id;
>> +	u32 ints = readl(lradc->base + LRADC_INTS);
>> +
>> +	mutex_lock(&lradc->update_lock);
>> +
>> +	if (ints & CHAN0_DATA_IRQ)
>> +		lradc->in[0] = (readl(lradc->base + LRADC_DATA0) & 
>> lradc->variant->bits) *
>> +			lradc->variant->vref / lradc->variant->resolution / 1000; /* to mV 
>> */
>> +
>> +	if (ints & CHAN1_DATA_IRQ)
>> +		lradc->in[1] = (readl(lradc->base + LRADC_DATA1) & 
>> lradc->variant->bits) *
>> +			lradc->variant->vref / lradc->variant->resolution / 1000; /* to mV 
>> */
>> +
>> +	mutex_unlock(&lradc->update_lock);
>> +
>> +	writel(ints, lradc->base + LRADC_INTS);
>> +
>> +	return IRQ_HANDLED;
>> +}
>> +
> 
> You don't explain why the interrupt handler is needed. It is 
> undesirable
> to do this for hwnon devices because it results in extra load on the 
> system.
> The ADC should be read only when needed and not continuously. If that 
> is not
> possible for some reason it needs to be explained in detail.
> 
>> +static int sun4i_lradc_start(struct sun4i_lradc_data *lradc)
>> +{
>> +	writel(FIRST_CONVERT_DLY(2) | CHAN_SELECT(3) | LEVELA_B_CNT(1) |
>> +		HOLD_EN(1) | KEY_MODE_SEL(2) | SAMPLE_RATE(3) | ENABLE(1),
>> +		lradc->base + LRADC_CTRL);
>> +
> 
> If the update rate is configurable, it might make sense to support
> the update_interval attribute.
> 
>> +	writel(CHAN0_DATA_IRQ | CHAN1_DATA_IRQ, lradc->base + LRADC_INTC);
>> +
>> +	return 0;
>> +}
>> +
>> +static void sun4i_lradc_stop(void *data)
>> +{
>> +	struct sun4i_lradc_data *lradc = (struct sun4i_lradc_data *)data;
>> +
>> +	writel(FIRST_CONVERT_DLY(2) | LEVELA_B_CNT(1) | HOLD_EN(1) |
>> +		SAMPLE_RATE(2), lradc->base + LRADC_CTRL);
>> +	writel(0, lradc->base + LRADC_INTC);
>> +}
>> +
>> +static int sun4i_lradc_probe(struct platform_device *pdev)
>> +{
>> +	struct sun4i_lradc_data *lradc;
>> +	struct device *dev = &pdev->dev;
>> +	struct device *hwmon_dev;
>> +	struct resource *res;
>> +	int hwmon_irq, error;
>> +
>> +	lradc = devm_kzalloc(dev, sizeof(struct sun4i_lradc_data), 
>> GFP_KERNEL);
>> +	if (!lradc)
>> +		return -ENOMEM;
>> +
>> +	dev_set_drvdata(dev, lradc);
>> +
>> +	lradc->variant = of_device_get_match_data(&pdev->dev);
>> +	if (!lradc->variant)
>> +		return -EINVAL;
>> +
>> +	lradc->dev = dev;
>> +
> 
> I don't immediately see where this is used.
> 
>> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>> +	if (IS_ERR(res))
>> +		return PTR_ERR(res);
>> +
>> +	lradc->base = devm_ioremap_resource(dev, res);
>> +	if (IS_ERR(lradc->base))
>> +		return PTR_ERR(lradc->base);
>> +
>> +	hwmon_irq = platform_get_irq(pdev, 0);
>> +	if (hwmon_irq < 0)
>> +		return hwmon_irq;
>> +
>> +	error = devm_request_irq(dev, hwmon_irq, sun4i_lradc_irq, 0, 
>> "sun4i-lradc-hwmon", lradc);
>> +	if (error) {
>> +		dev_err(dev, "sun4i-lradc-hwmon IRQ failed err=%d\n", error);
>> +		return error;
>> +	}
>> +
>> +	hwmon_dev = devm_hwmon_device_register_with_info(dev, "lradc-hwmon", 
>> lradc,
>> +							 &sun4i_lradc_chip_info, NULL);
>> +
>> +	if (IS_ERR(hwmon_dev)) {
>> +		error = PTR_ERR(hwmon_dev);
>> +		return error;
>> +	}
>> +
>> +	error = devm_add_action_or_reset(dev, sun4i_lradc_stop, lradc);
>> +	if (error)
>> +		return error;
>> +
>> +	error = sun4i_lradc_start(lradc);
>> +
>> +	return error;
>> +}
>> +
>> +#ifdef CONFIG_PM
>> +static int sun4i_lradc_resume(struct device *dev)
>> +{
>> +	struct sun4i_lradc_data *lradc = dev_get_drvdata(dev);
>> +
>> +	return sun4i_lradc_start(lradc);
>> +}
>> +
>> +static int sun4i_lradc_suspend(struct device *dev)
>> +{
>> +	struct sun4i_lradc_data *lradc = dev_get_drvdata(dev);
>> +
>> +	sun4i_lradc_stop(lradc);
>> +	return 0;
>> +}
>> +
>> +#define SUN4I_LRADC_DEV_PM_OPS	(&sun4i_lradc_dev_pm_ops)
>> +#else
>> +#define SUN4I_LRADC_DEV_PM_OPS	NULL
>> +#endif /* CONFIG_PM */
>> +
>> +static const struct dev_pm_ops sun4i_lradc_dev_pm_ops = {
>> +	.suspend = sun4i_lradc_suspend,
>> +	.resume = sun4i_lradc_resume,
>> +};
>> +
>> +static const struct of_device_id sun4i_lradc_of_match[] = {
>> +	{ .compatible = "allwinner,sun4i-a10-lradc-keys", .data = 
>> &variant_sun4i_a10_lradc},
>> +	{ /* sentinel */ }
>> +};
>> +MODULE_DEVICE_TABLE(of, sun4i_lradc_of_match);
>> +
>> +static struct platform_driver sun4i_lradc_driver = {
>> +	.driver = {
>> +		.name	= "sun4i-lradc-hwmon",
>> +		.of_match_table = of_match_ptr(sun4i_lradc_of_match),
>> +		.pm = SUN4I_LRADC_DEV_PM_OPS,
>> +	},
>> +	.probe	= sun4i_lradc_probe,
>> +};
>> +
>> +module_platform_driver(sun4i_lradc_driver);
>> +
>> +MODULE_DESCRIPTION("Allwinner A13/A20 LRADC hwmon driver");
>> +MODULE_AUTHOR("Ruslan Zalata <rz@fabmicro.ru>");
>> +MODULE_LICENSE("GPL");
>> +No empty line at end, please

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

[Guenter Roeck]

On 4/28/22 17:28, Ruslan Zalata wrote:
> Thank you Guenter for your valuable time.
> 
> I have added update_interval option (it's in ms units, right?) and fixed all other issues you pointed to. Will test it on real hardware and send third version of the patch for review.
> 
> Regarding IRQ. Alternatively the driver would need to sit and poll conversion ready bit in a loop which might cause a much worse load on system, is not it ? Anyway, the real problem with this piece of hardware is that there's no "conversion ready bit" provided, the only way to know data ready status is to receive an interrupt.
> 

Not necessarily. The data does not have to be "current", after all,
if the hardware is able to continuously convert. If not, the question
is how long a conversion takes. If it doesn't take too long, it would
be better to initiate a conversion and then wait for the completion.

> I think it still needs a semaphore/seqlock to synchronize conversions and reads. I.e. two consequent reads should not return same old value. Although it's not an issue in my case, but could be a problem for others.
> 
Why ? That happens for almost all hwmon devices. They will all report
the most recent conversion value. Some of them can take seconds
to complete a new conversion, so the reported value is always "old"
for a given defition of old (ie any time smaller than a conversion
interval).

Sigh. Looks like I'll have to dig up the documentation and read about
the ADC myself.

Guenter

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

[Guenter Roeck]

On 4/28/22 22:32, Guenter Roeck wrote:
> On 4/28/22 17:28, Ruslan Zalata wrote:
>> Thank you Guenter for your valuable time.
>>
>> I have added update_interval option (it's in ms units, right?) and fixed all other issues you pointed to. Will test it on real hardware and send third version of the patch for review.
>>
>> Regarding IRQ. Alternatively the driver would need to sit and poll conversion ready bit in a loop which might cause a much worse load on system, is not it ? Anyway, the real problem with this piece of hardware is that there's no "conversion ready bit" provided, the only way to know data ready status is to receive an interrupt.
>>
> 
> Not necessarily. The data does not have to be "current", after all,
> if the hardware is able to continuously convert. If not, the question
> is how long a conversion takes. If it doesn't take too long, it would
> be better to initiate a conversion and then wait for the completion.
> 
>> I think it still needs a semaphore/seqlock to synchronize conversions and reads. I.e. two consequent reads should not return same old value. Although it's not an issue in my case, but could be a problem for others.
>>
> Why ? That happens for almost all hwmon devices. They will all report
> the most recent conversion value. Some of them can take seconds
> to complete a new conversion, so the reported value is always "old"
> for a given defition of old (ie any time smaller than a conversion
> interval).
> 
> Sigh. Looks like I'll have to dig up the documentation and read about
> the ADC myself.
> 

I did, for both A13 and A20. The ADC supports continuous mode. That
means it can be configured accordingly, and reading the ADC value
just returns the most recent conversion value. There is absolutely
no need to keep reading the conversion values using interrupts.

Also,

+struct lradc_variant {
+	u32 bits;
+	u32 resolution;
+	u32 vref;
+};

is unnecessary because the values are the same for both supported chips.
That means that defines can and should be used. Yes, I can see that
A83T uses a different voltage, but even that doesn't need a structure -
the voltage can be set in struct sun4i_lradc_data if/when needed,
and the resolution and number of bits is still the same.

Guenter

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

[Ruslan Zalata]

Hi Guenter,

I'm sorry to disappoint you, but continuous mode for LRADC works only 
for key presses (significant voltage change), it does not work for raw 
data. Here's an excerpt from the manual that supports my discovery 
(punctuation and grammar preserved):

> The LRADC have three mode, Normal Mode、Single Mode and Continue Mode. 
> Normal mode is that the LRADC will
> report the result data of each convert all the time when the key is 
> down. Single Mode is that the LRADC
> will only report the first convert result data when the key is down. 
> Continue Mode is that the LRADC will
> report one of 8*(N+1) (N is program can set) sample convert result data 
> when key is down.

In other words, all three modes require key down event (voltage change) 
and IRQ is the only way to get continuous raw data updates from LRADC. 
I've experimented quite a lot with this for past few days, there are no 
changes to values in ADC data regs except in DATA IRQ mode.

Regarding variant structure. Vref is not the only difference between 
different implementations of LRADC in different Allwinner SoCs. For 
example, A83T has only one channel LRADC instead of two channels in 
A10/A13/A20. Some other SoCs may have even more differences. I 
introduced hwmon_chip_info structure into variant to encapsulate such 
differences in one place. Patch version 3 will follow. Thank you.

---
Regards,
Ruslan.

Fabmicro, LLC.

On 2022-04-29 11:03, Guenter Roeck wrote:
> On 4/28/22 22:32, Guenter Roeck wrote:
>> On 4/28/22 17:28, Ruslan Zalata wrote:
>>> Thank you Guenter for your valuable time.
>>> 
>>> I have added update_interval option (it's in ms units, right?) and 
>>> fixed all other issues you pointed to. Will test it on real hardware 
>>> and send third version of the patch for review.
>>> 
>>> Regarding IRQ. Alternatively the driver would need to sit and poll 
>>> conversion ready bit in a loop which might cause a much worse load on 
>>> system, is not it ? Anyway, the real problem with this piece of 
>>> hardware is that there's no "conversion ready bit" provided, the only 
>>> way to know data ready status is to receive an interrupt.
>>> 
>> 
>> Not necessarily. The data does not have to be "current", after all,
>> if the hardware is able to continuously convert. If not, the question
>> is how long a conversion takes. If it doesn't take too long, it would
>> be better to initiate a conversion and then wait for the completion.
>> 
>>> I think it still needs a semaphore/seqlock to synchronize conversions 
>>> and reads. I.e. two consequent reads should not return same old 
>>> value. Although it's not an issue in my case, but could be a problem 
>>> for others.
>>> 
>> Why ? That happens for almost all hwmon devices. They will all report
>> the most recent conversion value. Some of them can take seconds
>> to complete a new conversion, so the reported value is always "old"
>> for a given defition of old (ie any time smaller than a conversion
>> interval).
>> 
>> Sigh. Looks like I'll have to dig up the documentation and read about
>> the ADC myself.
>> 
> 
> I did, for both A13 and A20. The ADC supports continuous mode. That
> means it can be configured accordingly, and reading the ADC value
> just returns the most recent conversion value. There is absolutely
> no need to keep reading the conversion values using interrupts.
> 
> Also,
> 
> +struct lradc_variant {
> +	u32 bits;
> +	u32 resolution;
> +	u32 vref;
> +};
> 
> is unnecessary because the values are the same for both supported 
> chips.
> That means that defines can and should be used. Yes, I can see that
> A83T uses a different voltage, but even that doesn't need a structure -
> the voltage can be set in struct sun4i_lradc_data if/when needed,
> and the resolution and number of bits is still the same.
> 
> Guenter

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

[Jonathan Cameron]

On Thu, 28 Apr 2022 14:30:06 -0700
Guenter Roeck <linux@roeck-us.net> wrote:

> On 4/28/22 14:09, 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  
> 
> acquisition
> 
> > 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@fabmicro.ru>  
> 
> Ok, next phase of review.
> 

One thing noticed whilst randomly glancing at this patch.


> > +#ifdef CONFIG_PM
> > +static int sun4i_lradc_resume(struct device *dev)
> > +{
> > +	struct sun4i_lradc_data *lradc = dev_get_drvdata(dev);
> > +
> > +	return sun4i_lradc_start(lradc);
> > +}
> > +
> > +static int sun4i_lradc_suspend(struct device *dev)
> > +{
> > +	struct sun4i_lradc_data *lradc = dev_get_drvdata(dev);
> > +
> > +	sun4i_lradc_stop(lradc);
> > +	return 0;
> > +}
> > +
> > +#define SUN4I_LRADC_DEV_PM_OPS	(&sun4i_lradc_dev_pm_ops)
> > +#else
> > +#define SUN4I_LRADC_DEV_PM_OPS	NULL
> > +#endif /* CONFIG_PM */
> > +
> > +static const struct dev_pm_ops sun4i_lradc_dev_pm_ops = {

We have much better infrastructure for this these days.

Take a look at DEFINE_SIMPLE_DEV_PM_OPS()
and pm_sleep_ptr()

Those two in combination will let you get rid of all the ifdef stuff
here by letting the compiler remove the unused code automatically.

> > +	.suspend = sun4i_lradc_suspend,
> > +	.resume = sun4i_lradc_resume,
> > +};
> > +
> > +static const struct of_device_id sun4i_lradc_of_match[] = {
> > +	{ .compatible = "allwinner,sun4i-a10-lradc-keys", .data = &variant_sun4i_a10_lradc},
> > +	{ /* sentinel */ }
> > +};
> > +MODULE_DEVICE_TABLE(of, sun4i_lradc_of_match);
> > +
> > +static struct platform_driver sun4i_lradc_driver = {
> > +	.driver = {
> > +		.name	= "sun4i-lradc-hwmon",
> > +		.of_match_table = of_match_ptr(sun4i_lradc_of_match),
> > +		.pm = SUN4I_LRADC_DEV_PM_OPS,
> > +	},
> > +	.probe	= sun4i_lradc_probe,
> > +};
> > +
> > +module_platform_driver(sun4i_lradc_driver);
> > +
> > +MODULE_DESCRIPTION("Allwinner A13/A20 LRADC hwmon driver");
> > +MODULE_AUTHOR("Ruslan Zalata <rz@fabmicro.ru>");
> > +MODULE_LICENSE("GPL");
> > +No empty line at end, please  
> 
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Ruslan Zalata]

Hi Jonathan,

Thank you for pointing to DEFINE_SIMPLE_DEV_PM_OPS macros, will use it.

---
Regards,
Ruslan.

Fabmicro, LLC.

On 2022-04-29 18:24, Jonathan Cameron wrote:
> On Thu, 28 Apr 2022 14:30:06 -0700
> Guenter Roeck <linux@roeck-us.net> wrote:
> 
>> On 4/28/22 14:09, 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
>> 
>> acquisition
>> 
>> > 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@fabmicro.ru>
>> 
>> Ok, next phase of review.
>> 
> 
> One thing noticed whilst randomly glancing at this patch.
> 
> 
>> > +#ifdef CONFIG_PM
>> > +static int sun4i_lradc_resume(struct device *dev)
>> > +{
>> > +	struct sun4i_lradc_data *lradc = dev_get_drvdata(dev);
>> > +
>> > +	return sun4i_lradc_start(lradc);
>> > +}
>> > +
>> > +static int sun4i_lradc_suspend(struct device *dev)
>> > +{
>> > +	struct sun4i_lradc_data *lradc = dev_get_drvdata(dev);
>> > +
>> > +	sun4i_lradc_stop(lradc);
>> > +	return 0;
>> > +}
>> > +
>> > +#define SUN4I_LRADC_DEV_PM_OPS	(&sun4i_lradc_dev_pm_ops)
>> > +#else
>> > +#define SUN4I_LRADC_DEV_PM_OPS	NULL
>> > +#endif /* CONFIG_PM */
>> > +
>> > +static const struct dev_pm_ops sun4i_lradc_dev_pm_ops = {
> 
> We have much better infrastructure for this these days.
> 
> Take a look at DEFINE_SIMPLE_DEV_PM_OPS()
> and pm_sleep_ptr()
> 
> Those two in combination will let you get rid of all the ifdef stuff
> here by letting the compiler remove the unused code automatically.
> 
>> > +	.suspend = sun4i_lradc_suspend,
>> > +	.resume = sun4i_lradc_resume,
>> > +};
>> > +
>> > +static const struct of_device_id sun4i_lradc_of_match[] = {
>> > +	{ .compatible = "allwinner,sun4i-a10-lradc-keys", .data = &variant_sun4i_a10_lradc},
>> > +	{ /* sentinel */ }
>> > +};
>> > +MODULE_DEVICE_TABLE(of, sun4i_lradc_of_match);
>> > +
>> > +static struct platform_driver sun4i_lradc_driver = {
>> > +	.driver = {
>> > +		.name	= "sun4i-lradc-hwmon",
>> > +		.of_match_table = of_match_ptr(sun4i_lradc_of_match),
>> > +		.pm = SUN4I_LRADC_DEV_PM_OPS,
>> > +	},
>> > +	.probe	= sun4i_lradc_probe,
>> > +};
>> > +
>> > +module_platform_driver(sun4i_lradc_driver);
>> > +
>> > +MODULE_DESCRIPTION("Allwinner A13/A20 LRADC hwmon driver");
>> > +MODULE_AUTHOR("Ruslan Zalata <rz@fabmicro.ru>");
>> > +MODULE_LICENSE("GPL");
>> > +No empty line at end, please
>> 
>> 
>> _______________________________________________
>> linux-arm-kernel mailing list
>> linux-arm-kernel@lists.infradead.org
>> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Maxime Ripard]

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

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@fabmicro.ru>
> ---
>  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@fabmicro.ru>
> +L:	linux-hwmon@vger.kernel.org
> +S:	Maintained
> +F:	drivers/hwmon/sun4i-lradc-hwmon.c
> +
>  SUNDANCE NETWORK DRIVER
>  M:	Denis Kirjanov <kda@linux-powerpc.org>
>  L:	netdev@vger.kernel.org
> 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.

Maxime

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

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

[Icenowy Zheng]

于 2022年5月2日 GMT+08:00 下午7:00:10, Maxime Ripard <maxime@cerno.tech> 写到:
>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@fabmicro.ru>
>> ---
>>  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@fabmicro.ru>
>> +L:	linux-hwmon@vger.kernel.org
>> +S:	Maintained
>> +F:	drivers/hwmon/sun4i-lradc-hwmon.c
>> +
>>  SUNDANCE NETWORK DRIVER
>>  M:	Denis Kirjanov <kda@linux-powerpc.org>
>>  L:	netdev@vger.kernel.org
>> 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.

>
>Maxime

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

[Maxime Ripard]

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

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@cerno.tech> 写到:
> >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@fabmicro.ru>
> >> ---
> >>  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@fabmicro.ru>
> >> +L:	linux-hwmon@vger.kernel.org
> >> +S:	Maintained
> >> +F:	drivers/hwmon/sun4i-lradc-hwmon.c
> >> +
> >>  SUNDANCE NETWORK DRIVER
> >>  M:	Denis Kirjanov <kda@linux-powerpc.org>
> >>  L:	netdev@vger.kernel.org
> >> 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@redhat.com/

I'm not sure if it's still relevant, so we might just need to add an
hwmon driver to the existing driver

Maxime

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

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@cerno.tech> 写到:
>>> 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@fabmicro.ru>
>>>> ---
>>>>   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@fabmicro.ru>
>>>> +L:	linux-hwmon@vger.kernel.org
>>>> +S:	Maintained
>>>> +F:	drivers/hwmon/sun4i-lradc-hwmon.c
>>>> +
>>>>   SUNDANCE NETWORK DRIVER
>>>>   M:	Denis Kirjanov <kda@linux-powerpc.org>
>>>>   L:	netdev@vger.kernel.org
>>>> 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@redhat.com/
> 
> 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

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

[Ruslan Zalata]

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@cerno.tech> 
>>> 写到:
>>>> 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@fabmicro.ru>
>>>>> ---
>>>>>   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@fabmicro.ru>
>>>>> +L:	linux-hwmon@vger.kernel.org
>>>>> +S:	Maintained
>>>>> +F:	drivers/hwmon/sun4i-lradc-hwmon.c
>>>>> +
>>>>>   SUNDANCE NETWORK DRIVER
>>>>>   M:	Denis Kirjanov <kda@linux-powerpc.org>
>>>>>   L:	netdev@vger.kernel.org
>>>>> 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@redhat.com/
>> 
>> 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

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

[Maxime Ripard]

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

On Tue, May 03, 2022 at 08:26:18PM +0500, Ruslan Zalata wrote:
> 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 you can get it work reliably enough, I think IIO+iio-hwmon is still
the way to go

The main issue here is that drivers that are decided at compile time are
kind of a pain as soon as you try to install a generic distro.

At the hardware level, I'd assume you would either use the LRADC as an
actual ADC, or use it to drive buttons, right?

So, you would have to change the device tree accordingly anyway, to
either list buttons and their associated voltages or use it to probe
whatever signal to have there.

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.

Maxime

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

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

[Ruslan Zalata]

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. 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. IIO, on the other hand, is for data 
acquisition and is much more complex beast. Can we stay with HWMON, 
please ? :)

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, b) initiating 
conversion may (and will) take more time than a single consequent 
conversion, 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.

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

Regards,
Ruslan.

Fabmicro, LLC.

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

[Maxime Ripard]

[-- 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 --]

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

[Guenter Roeck]

On 5/2/22 04:00, Maxime Ripard wrote:
> 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@fabmicro.ru>
>> ---
>>   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@fabmicro.ru>
>> +L:	linux-hwmon@vger.kernel.org
>> +S:	Maintained
>> +F:	drivers/hwmon/sun4i-lradc-hwmon.c
>> +
>>   SUNDANCE NETWORK DRIVER
>>   M:	Denis Kirjanov <kda@linux-powerpc.org>
>>   L:	netdev@vger.kernel.org
>> 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?
> 
	depends on ARCH_SUNXI && !KEYBOARD_SUN4I_LRADC
                                  ^^^^^^^^^^^^^^^^^^^^^

> 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.
> 
Both is fine with me.

Guenter

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

[Maxime Ripard]

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

On Mon, May 02, 2022 at 06:31:56AM -0700, Guenter Roeck wrote:
> On 5/2/22 04:00, Maxime Ripard wrote:
> > 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@fabmicro.ru>
> > > ---
> > >   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@fabmicro.ru>
> > > +L:	linux-hwmon@vger.kernel.org
> > > +S:	Maintained
> > > +F:	drivers/hwmon/sun4i-lradc-hwmon.c
> > > +
> > >   SUNDANCE NETWORK DRIVER
> > >   M:	Denis Kirjanov <kda@linux-powerpc.org>
> > >   L:	netdev@vger.kernel.org
> > > 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?
> > 
> 	depends on ARCH_SUNXI && !KEYBOARD_SUN4I_LRADC

Right, but that just doesn't fly for any generic distro / build-system.

Maxime

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

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

[Guenter Roeck]

On 5/2/22 06:39, Maxime Ripard wrote:
> On Mon, May 02, 2022 at 06:31:56AM -0700, Guenter Roeck wrote:
>> On 5/2/22 04:00, Maxime Ripard wrote:
>>> 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@fabmicro.ru>
>>>> ---
>>>>    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@fabmicro.ru>
>>>> +L:	linux-hwmon@vger.kernel.org
>>>> +S:	Maintained
>>>> +F:	drivers/hwmon/sun4i-lradc-hwmon.c
>>>> +
>>>>    SUNDANCE NETWORK DRIVER
>>>>    M:	Denis Kirjanov <kda@linux-powerpc.org>
>>>>    L:	netdev@vger.kernel.org
>>>> 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?
>>>
>> 	depends on ARCH_SUNXI && !KEYBOARD_SUN4I_LRADC
> 
> Right, but that just doesn't fly for any generic distro / build-system.
> 

That is correct. Alternative might be to use devicetree bindings, which
presumably will be needed anyway to tell the driver(s) what to bind to.

Guenter

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

[Ruslan Zalata]

Hi Maxime,

> 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.

My first idea was to add hwmon to sun4i-lradc-keys.c driver. But soon as 
I began hacking the driver I quickly realized that it would be a mess 
since keyboard and hwmon belong to two different subsystems. Besides we 
would need to invent a way to control which way the driver works (new 
bindings?). So I decided that a new independent small driver would be a 
better solution.

---
Regards,
Ruslan.

Fabmicro, LLC.

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

[Maxime Ripard]

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

Hi,

On Tue, May 03, 2022 at 01:50:39PM +0500, Ruslan Zalata wrote:
> > 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.
> 
> My first idea was to add hwmon to sun4i-lradc-keys.c driver. But soon as I
> began hacking the driver I quickly realized that it would be a mess since
> keyboard and hwmon belong to two different subsystems.

That's not really an issue in itself. There's plenty of drivers in Linux
that register into two frameworks.

> Besides we would need to invent a way to control which way the driver
> works (new bindings?).

I got confused there, and thought you were adding temperature reading
that is in another ADC on those SoCs, sorry. Yeah, that doesn't make
much sense to have both in the same drivers here.

Maxime

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