Date:   Fri, 29 Apr 2022 05:28:41 +0500
From:   Ruslan Zalata <rz@...micro.ru>
To:     Guenter Roeck <linux@...ck-us.net>
Cc:     Jean Delvare <jdelvare@...e.com>, Chen-Yu Tsai <wens@...e.org>,
        Jernej Skrabec <jernej.skrabec@...il.com>,
        Samuel Holland <samuel@...lland.org>,
        linux-kernel@...r.kernel.org, linux-hwmon@...r.kernel.org,
        linux-arm-kernel@...ts.infradead.org, linux-sunxi@...ts.linux.dev
Subject: Re: [PATCH v2] hwmon: (sun4i-lradc) Add driver for LRADC found on
 Allwinner A13/A20 SoC

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@...micro.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@...micro.ru>
>> +L:	linux-hwmon@...r.kernel.org
>> +S:	Maintained
>> +F:	drivers/hwmon/sun4i-lradc-hwmon.c
>> +
>>   SUNDANCE NETWORK DRIVER
>>   M:	Denis Kirjanov <kda@...ux-powerpc.org>
>>   L:	netdev@...r.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@...micro.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@...micro.ru>");
>> +MODULE_LICENSE("GPL");
>> +No empty line at end, please

Powered by blists - more mailing lists