* Re: [PATCH v2] hwmon: (sun4i-lradc) Add driver for LRADC found on Allwinner A13/A20 SoC [not found] <20220428210906.29527-1-rz@fabmicro.ru> @ 2022-04-28 21:30 ` Guenter Roeck 2022-04-29 0:28 ` Ruslan Zalata 2022-04-29 13:24 ` Jonathan Cameron 2022-05-02 11:00 ` Maxime Ripard 1 sibling, 2 replies; 22+ messages in thread From: Guenter Roeck @ 2022-04-28 21:30 UTC (permalink / raw) To: Ruslan Zalata Cc: Jean Delvare, Chen-Yu Tsai, Jernej Skrabec, Samuel Holland, linux-kernel, linux-hwmon, linux-arm-kernel, linux-sunxi 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 ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v2] hwmon: (sun4i-lradc) Add driver for LRADC found on Allwinner A13/A20 SoC 2022-04-28 21:30 ` [PATCH v2] hwmon: (sun4i-lradc) Add driver for LRADC found on Allwinner A13/A20 SoC Guenter Roeck @ 2022-04-29 0:28 ` Ruslan Zalata 2022-04-29 5:32 ` Guenter Roeck 2022-04-29 13:24 ` Jonathan Cameron 1 sibling, 1 reply; 22+ messages in thread From: Ruslan Zalata @ 2022-04-29 0:28 UTC (permalink / raw) To: Guenter Roeck Cc: Jean Delvare, Chen-Yu Tsai, Jernej Skrabec, Samuel Holland, linux-kernel, linux-hwmon, linux-arm-kernel, linux-sunxi 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 ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v2] hwmon: (sun4i-lradc) Add driver for LRADC found on Allwinner A13/A20 SoC 2022-04-29 0:28 ` Ruslan Zalata @ 2022-04-29 5:32 ` Guenter Roeck 2022-04-29 6:03 ` Guenter Roeck 0 siblings, 1 reply; 22+ messages in thread From: Guenter Roeck @ 2022-04-29 5:32 UTC (permalink / raw) To: Ruslan Zalata Cc: Jean Delvare, Chen-Yu Tsai, Jernej Skrabec, Samuel Holland, linux-kernel, linux-hwmon, linux-arm-kernel, linux-sunxi 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 ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v2] hwmon: (sun4i-lradc) Add driver for LRADC found on Allwinner A13/A20 SoC 2022-04-29 5:32 ` Guenter Roeck @ 2022-04-29 6:03 ` Guenter Roeck 2022-05-02 23:47 ` Ruslan Zalata 0 siblings, 1 reply; 22+ messages in thread From: Guenter Roeck @ 2022-04-29 6:03 UTC (permalink / raw) To: Ruslan Zalata Cc: Jean Delvare, Chen-Yu Tsai, Jernej Skrabec, Samuel Holland, linux-kernel, linux-hwmon, linux-arm-kernel, linux-sunxi 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 ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v2] hwmon: (sun4i-lradc) Add driver for LRADC found on Allwinner A13/A20 SoC 2022-04-29 6:03 ` Guenter Roeck @ 2022-05-02 23:47 ` Ruslan Zalata 0 siblings, 0 replies; 22+ messages in thread From: Ruslan Zalata @ 2022-05-02 23:47 UTC (permalink / raw) To: Guenter Roeck Cc: Jean Delvare, Chen-Yu Tsai, Jernej Skrabec, Samuel Holland, linux-kernel, linux-hwmon, linux-arm-kernel, linux-sunxi 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 ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v2] hwmon: (sun4i-lradc) Add driver for LRADC found on Allwinner A13/A20 SoC 2022-04-28 21:30 ` [PATCH v2] hwmon: (sun4i-lradc) Add driver for LRADC found on Allwinner A13/A20 SoC Guenter Roeck 2022-04-29 0:28 ` Ruslan Zalata @ 2022-04-29 13:24 ` Jonathan Cameron 2022-05-02 23:20 ` Ruslan Zalata 1 sibling, 1 reply; 22+ messages in thread From: Jonathan Cameron @ 2022-04-29 13:24 UTC (permalink / raw) To: Guenter Roeck Cc: Ruslan Zalata, Jean Delvare, Chen-Yu Tsai, Jernej Skrabec, Samuel Holland, linux-kernel, linux-hwmon, linux-arm-kernel, linux-sunxi 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 ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v2] hwmon: (sun4i-lradc) Add driver for LRADC found on Allwinner A13/A20 SoC 2022-04-29 13:24 ` Jonathan Cameron @ 2022-05-02 23:20 ` Ruslan Zalata 0 siblings, 0 replies; 22+ messages in thread From: Ruslan Zalata @ 2022-05-02 23:20 UTC (permalink / raw) To: Jonathan Cameron Cc: Guenter Roeck, Jean Delvare, Chen-Yu Tsai, Jernej Skrabec, Samuel Holland, linux-kernel, linux-hwmon, linux-arm-kernel, linux-sunxi 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 ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v2] hwmon: (sun4i-lradc) Add driver for LRADC found on Allwinner A13/A20 SoC [not found] <20220428210906.29527-1-rz@fabmicro.ru> 2022-04-28 21:30 ` [PATCH v2] hwmon: (sun4i-lradc) Add driver for LRADC found on Allwinner A13/A20 SoC Guenter Roeck @ 2022-05-02 11:00 ` Maxime Ripard 2022-05-02 11:15 ` Icenowy Zheng ` (2 more replies) 1 sibling, 3 replies; 22+ messages in thread From: Maxime Ripard @ 2022-05-02 11:00 UTC (permalink / raw) To: Ruslan Zalata Cc: Guenter Roeck, Jean Delvare, Chen-Yu Tsai, Jernej Skrabec, Samuel Holland, linux-kernel, linux-hwmon, linux-arm-kernel, linux-sunxi [-- 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 --] ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v2] hwmon: (sun4i-lradc) Add driver for LRADC found on Allwinner A13/A20 SoC 2022-05-02 11:00 ` Maxime Ripard @ 2022-05-02 11:15 ` Icenowy Zheng 2022-05-02 11:21 ` Maxime Ripard 2022-05-02 13:31 ` Guenter Roeck 2022-05-03 8:50 ` Ruslan Zalata 2 siblings, 1 reply; 22+ messages in thread From: Icenowy Zheng @ 2022-05-02 11:15 UTC (permalink / raw) To: Maxime Ripard, Ruslan Zalata Cc: Guenter Roeck, Jean Delvare, Chen-Yu Tsai, Jernej Skrabec, Samuel Holland, linux-kernel, linux-hwmon, linux-arm-kernel, linux-sunxi 于 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 ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v2] hwmon: (sun4i-lradc) Add driver for LRADC found on Allwinner A13/A20 SoC 2022-05-02 11:15 ` Icenowy Zheng @ 2022-05-02 11:21 ` Maxime Ripard 2022-05-03 2:02 ` Guenter Roeck 0 siblings, 1 reply; 22+ messages in thread From: Maxime Ripard @ 2022-05-02 11:21 UTC (permalink / raw) To: Icenowy Zheng Cc: Ruslan Zalata, Guenter Roeck, Jean Delvare, Chen-Yu Tsai, Jernej Skrabec, Samuel Holland, linux-kernel, linux-hwmon, linux-arm-kernel, linux-sunxi [-- 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 --] ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v2] hwmon: (sun4i-lradc) Add driver for LRADC found on Allwinner A13/A20 SoC 2022-05-02 11:21 ` Maxime Ripard @ 2022-05-03 2:02 ` Guenter Roeck 2022-05-03 15:26 ` Ruslan Zalata 0 siblings, 1 reply; 22+ messages in thread From: Guenter Roeck @ 2022-05-03 2:02 UTC (permalink / raw) To: Maxime Ripard, Icenowy Zheng Cc: Ruslan Zalata, Jean Delvare, Chen-Yu Tsai, Jernej Skrabec, Samuel Holland, linux-kernel, linux-hwmon, linux-arm-kernel, linux-sunxi 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 ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v2] hwmon: (sun4i-lradc) Add driver for LRADC found on Allwinner A13/A20 SoC 2022-05-03 2:02 ` Guenter Roeck @ 2022-05-03 15:26 ` Ruslan Zalata 2022-05-03 16:14 ` Maxime Ripard 0 siblings, 1 reply; 22+ messages in thread From: Ruslan Zalata @ 2022-05-03 15:26 UTC (permalink / raw) To: Guenter Roeck Cc: Maxime Ripard, Icenowy Zheng, Jean Delvare, Chen-Yu Tsai, Jernej Skrabec, Samuel Holland, linux-kernel, linux-hwmon, linux-arm-kernel, linux-sunxi 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 ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v2] hwmon: (sun4i-lradc) Add driver for LRADC found on Allwinner A13/A20 SoC 2022-05-03 15:26 ` Ruslan Zalata @ 2022-05-03 16:14 ` Maxime Ripard 2022-05-03 21:37 ` Ruslan Zalata 0 siblings, 1 reply; 22+ messages in thread From: Maxime Ripard @ 2022-05-03 16:14 UTC (permalink / raw) To: Ruslan Zalata Cc: Guenter Roeck, Icenowy Zheng, Jean Delvare, Chen-Yu Tsai, Jernej Skrabec, Samuel Holland, linux-kernel, linux-hwmon, linux-arm-kernel, linux-sunxi [-- 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 --] ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v2] hwmon: (sun4i-lradc) Add driver for LRADC found on Allwinner A13/A20 SoC 2022-05-03 16:14 ` Maxime Ripard @ 2022-05-03 21:37 ` Ruslan Zalata 2022-05-04 14:12 ` Maxime Ripard 0 siblings, 1 reply; 22+ messages in thread From: Ruslan Zalata @ 2022-05-03 21:37 UTC (permalink / raw) To: Maxime Ripard Cc: Guenter Roeck, Icenowy Zheng, Jean Delvare, Chen-Yu Tsai, Jernej Skrabec, Samuel Holland, linux-kernel, linux-hwmon, linux-arm-kernel, linux-sunxi 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. ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v2] hwmon: (sun4i-lradc) Add driver for LRADC found on Allwinner A13/A20 SoC 2022-05-03 21:37 ` Ruslan Zalata @ 2022-05-04 14:12 ` Maxime Ripard 0 siblings, 0 replies; 22+ messages in thread From: Maxime Ripard @ 2022-05-04 14:12 UTC (permalink / raw) To: Ruslan Zalata Cc: Guenter Roeck, Icenowy Zheng, Jean Delvare, Chen-Yu Tsai, Jernej Skrabec, Samuel Holland, linux-kernel, linux-hwmon, linux-arm-kernel, linux-sunxi [-- 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 --] ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v2] hwmon: (sun4i-lradc) Add driver for LRADC found on Allwinner A13/A20 SoC 2022-05-02 11:00 ` Maxime Ripard 2022-05-02 11:15 ` Icenowy Zheng @ 2022-05-02 13:31 ` Guenter Roeck 2022-05-02 13:39 ` Maxime Ripard 2022-05-03 8:50 ` Ruslan Zalata 2 siblings, 1 reply; 22+ messages in thread From: Guenter Roeck @ 2022-05-02 13:31 UTC (permalink / raw) To: Maxime Ripard, Ruslan Zalata Cc: Jean Delvare, Chen-Yu Tsai, Jernej Skrabec, Samuel Holland, linux-kernel, linux-hwmon, linux-arm-kernel, linux-sunxi 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 ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v2] hwmon: (sun4i-lradc) Add driver for LRADC found on Allwinner A13/A20 SoC 2022-05-02 13:31 ` Guenter Roeck @ 2022-05-02 13:39 ` Maxime Ripard 2022-05-02 16:21 ` Guenter Roeck 0 siblings, 1 reply; 22+ messages in thread From: Maxime Ripard @ 2022-05-02 13:39 UTC (permalink / raw) To: Guenter Roeck Cc: Ruslan Zalata, Jean Delvare, Chen-Yu Tsai, Jernej Skrabec, Samuel Holland, linux-kernel, linux-hwmon, linux-arm-kernel, linux-sunxi [-- 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 --] ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v2] hwmon: (sun4i-lradc) Add driver for LRADC found on Allwinner A13/A20 SoC 2022-05-02 13:39 ` Maxime Ripard @ 2022-05-02 16:21 ` Guenter Roeck 0 siblings, 0 replies; 22+ messages in thread From: Guenter Roeck @ 2022-05-02 16:21 UTC (permalink / raw) To: Maxime Ripard Cc: Ruslan Zalata, Jean Delvare, Chen-Yu Tsai, Jernej Skrabec, Samuel Holland, linux-kernel, linux-hwmon, linux-arm-kernel, linux-sunxi 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 ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v2] hwmon: (sun4i-lradc) Add driver for LRADC found on Allwinner A13/A20 SoC 2022-05-02 11:00 ` Maxime Ripard 2022-05-02 11:15 ` Icenowy Zheng 2022-05-02 13:31 ` Guenter Roeck @ 2022-05-03 8:50 ` Ruslan Zalata 2022-05-03 16:07 ` Maxime Ripard 2 siblings, 1 reply; 22+ messages in thread From: Ruslan Zalata @ 2022-05-03 8:50 UTC (permalink / raw) To: Maxime Ripard Cc: Guenter Roeck, Jean Delvare, Chen-Yu Tsai, Jernej Skrabec, Samuel Holland, linux-kernel, linux-hwmon, linux-arm-kernel, linux-sunxi 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. ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v2] hwmon: (sun4i-lradc) Add driver for LRADC found on Allwinner A13/A20 SoC 2022-05-03 8:50 ` Ruslan Zalata @ 2022-05-03 16:07 ` Maxime Ripard 0 siblings, 0 replies; 22+ messages in thread From: Maxime Ripard @ 2022-05-03 16:07 UTC (permalink / raw) To: Ruslan Zalata Cc: Guenter Roeck, Jean Delvare, Chen-Yu Tsai, Jernej Skrabec, Samuel Holland, linux-kernel, linux-hwmon, linux-arm-kernel, linux-sunxi [-- 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 --] ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v2] hwmon: (sun4i-lradc) Add driver for LRADC found on Allwinner A13/A20 SoC @ 2022-05-04 19:33 Jamie Cuthbert 0 siblings, 0 replies; 22+ messages in thread From: Jamie Cuthbert @ 2022-05-04 19:33 UTC (permalink / raw) To: rz Cc: jdelvare, jernej.skrabec, linux-arm-kernel, linux-hwmon, linux-kernel, linux-sunxi, linux, samuel, wens Sent from my iPhone ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v2] hwmon: (sun4i-lradc) Add driver for LRADC found on Allwinner A13/A20 SoC @ 2022-05-04 19:33 Jamie Cuthbert 0 siblings, 0 replies; 22+ messages in thread From: Jamie Cuthbert @ 2022-05-04 19:33 UTC (permalink / raw) To: rz Cc: jdelvare, jernej.skrabec, linux-arm-kernel, linux-hwmon, linux-kernel, linux-sunxi, linux, samuel, wens Sent from my iPhone ^ permalink raw reply [flat|nested] 22+ messages in thread
end of thread, other threads:[~2022-05-04 19:42 UTC | newest] Thread overview: 22+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- [not found] <20220428210906.29527-1-rz@fabmicro.ru> 2022-04-28 21:30 ` [PATCH v2] hwmon: (sun4i-lradc) Add driver for LRADC found on Allwinner A13/A20 SoC Guenter Roeck 2022-04-29 0:28 ` Ruslan Zalata 2022-04-29 5:32 ` Guenter Roeck 2022-04-29 6:03 ` Guenter Roeck 2022-05-02 23:47 ` Ruslan Zalata 2022-04-29 13:24 ` Jonathan Cameron 2022-05-02 23:20 ` Ruslan Zalata 2022-05-02 11:00 ` Maxime Ripard 2022-05-02 11:15 ` Icenowy Zheng 2022-05-02 11:21 ` Maxime Ripard 2022-05-03 2:02 ` Guenter Roeck 2022-05-03 15:26 ` Ruslan Zalata 2022-05-03 16:14 ` Maxime Ripard 2022-05-03 21:37 ` Ruslan Zalata 2022-05-04 14:12 ` Maxime Ripard 2022-05-02 13:31 ` Guenter Roeck 2022-05-02 13:39 ` Maxime Ripard 2022-05-02 16:21 ` Guenter Roeck 2022-05-03 8:50 ` Ruslan Zalata 2022-05-03 16:07 ` Maxime Ripard 2022-05-04 19:33 Jamie Cuthbert 2022-05-04 19:33 Jamie Cuthbert
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).