re: [PATCH] ARM: Add hwmon driver for LRADC found on Allwinner A13/A20 SoCs

[Ruslan Zalata <rz@fabmicro.ru>]

Hi Guenter!

Thank you very much for your detailed review of my code. I rewrote the 
driver using devm_hwmon_device_register_with_info and pm API as you 
adviced. I think I took into account all your comments, let me know if I 
missed anything. New diver has been tested on real device with A20 SoC. 
Version 2 of the patch will follow.

BTW. By reading datasheet for A83T I could find some differences in 
LRADC hardware between A10/A20 and A83T. Since I don't have a board with 
such SoC at the moment to perform tests, I removed reference to 
sun8i-a83t-r compatibility.

PS: We use LRADC on our boards to monitor external battery voltage and 
temp, that's how this driver became useful. Yes, LRADC is 6 bits only, 
but it does the job saving us one extra IC in the design. :)

---
Regards,
Ruslan.

Fabmicro, LLC.

On 2022-04-28 03:54, Guenter Roeck wrote:
> On 4/27/22 14:50, Ruslan Zalata wrote:
>> Jernej,
>> 
>> Are these CHECKs ok to pass ?
>> 
>> CHECK: Alignment should match open parenthesis
>> #314: FILE: drivers/hwmon/sun4i-lradc-hwmon.c:231:
>> +       lradc->base = devm_ioremap_resource(dev,
>> +                       platform_get_resource(pdev, IORESOURCE_MEM, 
>> 0));
>> 
> 
> Use an intermediate variable. You need to do that anyway because
> platform_get_resource() can return NULL which needs to be checked.
> 
>> CHECK: Alignment should match open parenthesis
>> #319: FILE: drivers/hwmon/sun4i-lradc-hwmon.c:236:
>> +       error = devm_request_irq(dev, platform_get_irq(pdev, 0),
>> +                       sun4i_lradc_irq, 0, "sun4i-lradc-hwmon", 
>> lradc);
>> 
>> total: 0 errors, 0 warnings, 2 checks, 307 lines checked
>> 
>> Aligning above by paranthesis will make code look ugly and exceed 75 
>> chars limit. Please advise.
>> 
> 
> What 75 characters limit ?
> 
> You can go up to 100 characters if needed. Also, platform_get_irq()
> can return an error, which needs to be checked anyway.
> 
> Please prepend the subject replies with Re: as is current practice.
> Your replies all look like patches, not like replies to patches.
> 
> More comments inline.
> 
> Guenter
> 
>> ---
>> Regards,
>> Ruslan.
>> 
>> Fabmicro, LLC.
>> 
>> On 2022-04-28 02:12, Jernej Škrabec wrote:
>>> Dne sreda, 27. april 2022 ob 23:01:58 CEST je Ruslan Zalata 
>>> napisal(a):
>>>> Dear Jernej and all.
>>>> 
>>>> I used sendmail instead of "git send-email" and it did the whole 
>>>> this
>>>> wrong - several separate emails were sent. I'm sorry for that, will
>>>> stick to git command next time.
>>>> 
>>>> I read the guide of course and I fed the patch through
>>>> scripts/checkpatch.pl many times in an effort to elaborate all the
>>>> errors and warnings. The only warning left is this:
>>>> 
>>>> WARNING: added, moved or deleted file(s), does MAINTAINERS need
>>>> updating?
>>>> #61:
>>>> new file mode 100644
>>>> 
>>>> total: 0 errors, 1 warnings, 304 lines checked
>>> 
>>> Did you used --strict? Anyway, you have multiple consecutive empty 
>>> lines in
>>> several places. Only one empty line is allowed.
>>> 
>>>> 
>>>> Since this is my first attempt to submit my code I'm quite confused 
>>>> whom
>>>> should I add as a maintainer for this driver. :)
>>> 
>>> Usually yourself.
>>> 
>>>> 
>>>> PS: What's wrong with the subject ?
>>> 
>>> Too long and ARM is not a proper tag. Check git history of 
>>> drivers/hwmon
>>> folder for ideas.
>>> 
>>> Best regards,
>>> Jernej
>>> 
>>>> 
>>>> ---
>>>> Regards,
>>>> Ruslan.
>>>> 
>>>> Fabmicro, LLC.
>>>> 
>>>> On 2022-04-28 01:23, Jernej Škrabec wrote:
>>>> > Hi Ruslan!
>>>> >
>>>> > Dne sreda, 27. april 2022 ob 21:25:12 CEST je Ruslan Zalata napisal(a):
>>>> >> 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>
>>>> >
>>>> > Before even going to check actual driver, please read
>>>> > https://www.kernel.org/
>>>> > doc/html/latest/process/submitting-patches.html in detail.
>>>> >
>>>> > Just few basic things to fix first:
>>>> > 1. subject doesn't conform to rules
>>>> > 2. send patch to maintainers and mailing lists (use
>>>> > scripts/get_maintainer.pl)
>>>> > 3. code has style issues (multiple empty lines). Run
>>>> > scripts/checkpatch.pl --
>>>> > strict on your patch before submission and fix all reported issues.
>>>> > 4. you sent same patch two times. If it's same, it should be marked
>>>> > with
>>>> > RESEND, if not, it should be versioned (V2, V3, etc.) with short
>>>> > changelog
>>>> > after --- line (as explained in above link)
>>>> > 5. it's customary to add new entry in MAINTAINERS when adding new
>>>> > driver
>>>> >
>>>> > Best regards,
>>>> > Jernej
>>>> >
>>>> >> ---
>>>> >>
>>>> >>  drivers/hwmon/Kconfig             |  13 ++
>>>> >>  drivers/hwmon/Makefile            |   1 +
>>>> >>  drivers/hwmon/sun4i-lradc-hwmon.c | 278
>>>> >>
>>>> >> ++++++++++++++++++++++++++++++
>>>> >>
>>>> >>  3 files changed, 292 insertions(+)
>>>> >>  create mode 100644 drivers/hwmon/sun4i-lradc-hwmon.c
>>>> >>
>>>> >> diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
>>>> >> index 68a8a27ab3b..b7732a9e992 100644
>>>> >> --- a/drivers/hwmon/Kconfig
>>>> >> +++ b/drivers/hwmon/Kconfig
>>>> >> @@ -521,6 +521,19 @@ config I8K
>>>> >>
>>>> >>        Say Y if you intend to run userspace programs that use this
>>>> >
>>>> > interface.
>>>> >
>>>> >>        Say N otherwise.
>>>> >>
>>>> >> +config SENSORS_SUN4I_LRADC
> 
> Try to put in alphabetic order.
> 
>>>> >> +    tristate "Allwinner A13/A20 LRADC hwmon"
>>>> >> +    depends on ARCH_SUNXI && (!KEYBOARD_SUN4I_LRADC)
> 
> 
> Drop the unnecessary ()
> 
>>>> >> +    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 should be used at a time.
> 
> Only one of these must be ...
> 
>>>> >> +
>>>> >>
>>>> >>  config SENSORS_DA9052_ADC
>>>> >>
>>>> >>      tristate "Dialog DA9052/DA9053 ADC"
>>>> >>      depends on PMIC_DA9052
>>>> >>
>>>> >> diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile
>>>> >> index 8a03289e2aa..43bed6fff27 100644
>>>> >> --- a/drivers/hwmon/Makefile
>>>> >> +++ b/drivers/hwmon/Makefile
>>>> >> @@ -30,6 +30,7 @@ obj-$(CONFIG_SENSORS_AD7314)    += ad7314.o
>>>> >>
>>>> >>  obj-$(CONFIG_SENSORS_AD7414)    += ad7414.o
>>>> >>  obj-$(CONFIG_SENSORS_AD7418)    += ad7418.o
>>>> >>  obj-$(CONFIG_SENSORS_ADC128D818) += adc128d818.o
>>>> >>
>>>> >> +obj-$(CONFIG_SENSORS_SUN4I_LRADC) += sun4i-lradc-hwmon.o
> 
> Alphabetic order please.
> 
>>>> >>
>>>> >>  obj-$(CONFIG_SENSORS_ADCXX)    += adcxx.o
>>>> >>  obj-$(CONFIG_SENSORS_ADM1021)    += adm1021.o
>>>> >>  obj-$(CONFIG_SENSORS_ADM1025)    += adm1025.o
>>>> >>
>>>> >> diff --git a/drivers/hwmon/sun4i-lradc-hwmon.c
>>>> >> b/drivers/hwmon/sun4i-lradc-hwmon.c new file mode 100644
>>>> >> index 00000000000..1947b716dbf
>>>> >> --- /dev/null
>>>> >> +++ b/drivers/hwmon/sun4i-lradc-hwmon.c
>>>> >> @@ -0,0 +1,278 @@
>>>> >> +// 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/regulator/consumer.h>
>>>> >> +#include <linux/slab.h>
>>>> >> +#include <linux/hwmon.h>
>>>> >> +#include <linux/hwmon-sysfs.h>
> 
> Alphabetic order, and only include files which are actually needed.
> 
>>>> >> +
>>>> >> +#define LRADC_CTRL        0x00
>>>> >> +#define LRADC_INTC        0x04
>>>> >> +#define LRADC_INTS        0x08
>>>> >> +#define LRADC_DATA0        0x0c
>>>> >> +#define LRADC_DATA1        0x10
> 
> Alignment seems off. Did you use spaces ?
> 
>>>> >> +
>>>> >> +/* 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)
> 
> Odd alignment. It should be all
> 
> #define<space>NAME<tab>value
> 
>>>> >> +
>>>> >> +
>>>> >> +struct lradc_variant {
>>>> >> +    u32 bits;
>>>> >> +    u32 resolution;
>>>> >> +    u32 vref;
>>>> >> +};
>>>> >> +
>>>> >> +struct lradc_variant variant_sun4i_a10_lradc = {
>>>> >> +    .bits = 0x3f,
>>>> >> +    .resolution = 63,
>>>> >> +    .vref = 2000000,
>>>> >> +};
>>>> >> +
>>>> >> +struct lradc_variant variant_sun8i_a83t_r_lradc = {
>>>> >> +    .bits = 0x3f,
>>>> >> +    .resolution = 63,
>>>> >> +    .vref = 2000000,
>>>> >> +};
> 
> What is the point of all this as it is all the same ?
> And the ADC has really only 6 bit of resolution ?
> 
> Also, what decides vref ? Is this a fixed value or is it
> board dependent or configurable ?
> 
>>>> >> +
>>>> >> +static const struct of_device_id sun4i_lradc_of_match[];
>>>> >> +
> 
> Please reorder code to not require forward declarations.
> 
>>>> >> +
>>>> >> +struct sun4i_lradc_data {
>>>> >> +    struct device *dev;
>>>> >> +    struct device *hwmon_dev;
>>>> >> +    void __iomem *base;
>>>> >> +    const struct lradc_variant *variant;
>>>> >> +    u32 in[2];
>>>> >> +    u32 in_mvolts[2];
>>>> >> +};
>>>> >> +
>>>> >> +
>>>> >> +static ssize_t lradc_in_mvolts_show(struct device *dev,
>>>> >> +            struct device_attribute *attr, char *buf)
>>>> >> +{
>>>> >> +    struct sun4i_lradc_data *data = dev_get_drvdata(dev);
>>>> >> +    int nr = to_sensor_dev_attr_2(attr)->nr;
>>>> >> +
>>>> >> +    if (IS_ERR(data))
>>>> >> +        return PTR_ERR(data);
>>>> >> +
>>>> >> +    return sprintf(buf, "%d\n", data->in_mvolts[nr]);
>>>> >> +}
>>>> >> +
>>>> >> +static ssize_t lradc_in_show(struct device *dev,
>>>> >> +        struct device_attribute *attr, char *buf)
>>>> >> +{
>>>> >> +    struct sun4i_lradc_data *data = dev_get_drvdata(dev);
>>>> >> +    int nr = to_sensor_dev_attr_2(attr)->nr;
>>>> >> +
>>>> >> +    if (IS_ERR(data))
>>>> >> +        return PTR_ERR(data);
>>>> >> +
>>>> >> +    return sprintf(buf, "%d\n", data->in[nr]);
>>>> >> +}
>>>> >> +
>>>> >> +
>>>> >> +static umode_t lradc_is_visible(struct kobject *kobj,
>>>> >> +        struct attribute *attr, int index)
>>>> >> +{
>>>> >> +    return attr->mode;
>>>> >> +}
> 
> Completely pointless function.
> 
>>>> >> +
>>>> >> +
>>>> >> +
>>>> >> +static SENSOR_DEVICE_ATTR_2_RO(in0_input, lradc_in, 0, 0);
>>>> >> +static SENSOR_DEVICE_ATTR_2_RO(in0_input_mvolts, lradc_in_mvolts, 0,
> 
> Only standard attributes please. What is the point anyway ? Voltage is
> reported in mV, period. If you want to report raw values, you'll need 
> to
> write an iio driver. FWIW, that may be more appropriate anyway since
> the other ADC on this chip is already handled with an iio driver.
> 
>>>> >> 1);
>>>> >> +
>>>> >> +static SENSOR_DEVICE_ATTR_2_RO(in1_input, lradc_in, 1, 0);
>>>> >> +static SENSOR_DEVICE_ATTR_2_RO(in1_input_mvolts, lradc_in_mvolts, 1,
>>>> >> 1);
>>>> >> +
>>>> >> +static struct attribute *lradc_attrs[] = {
>>>> >> +    &sensor_dev_attr_in0_input.dev_attr.attr,
>>>> >> +    &sensor_dev_attr_in0_input_mvolts.dev_attr.attr,
>>>> >> +    &sensor_dev_attr_in1_input.dev_attr.attr,
>>>> >> +    &sensor_dev_attr_in1_input_mvolts.dev_attr.attr,
>>>> >> +    NULL
>>>> >> +};
>>>> >> +
>>>> >> +static const struct attribute_group lradc_group = {
>>>> >> +    .attrs = lradc_attrs,
>>>> >> +    .is_visible = lradc_is_visible,
>>>> >> +};
>>>> >> +__ATTRIBUTE_GROUPS(lradc);
>>>> >> +
>>>> >> +
>>>> >> +static irqreturn_t sun4i_lradc_irq(int irq, void *dev_id)
>>>> >> +{
>>>> >> +    struct sun4i_lradc_data *lradc = dev_id;
>>>> >> +    u32 ints;
>>>> >> +
>>>> >> +    ints  = readl(lradc->base + LRADC_INTS);
>>>> >> +
>>>> >> +    if (ints & CHAN0_DATA_IRQ) {
>>>> >> +        lradc->in[0] = readl(lradc->base + LRADC_DATA0) &
>>>> >> +            lradc->variant->bits;
>>>> >> +        lradc->in_mvolts[0] = lradc->in[0] * lradc->variant-
>>>> >> vref /
>>>> >> +            lradc->variant->resolution / 1000;
>>>> >> +    }
>>>> >> +
>>>> >> +    if (ints & CHAN1_DATA_IRQ) {
>>>> >> +        lradc->in[1] = readl(lradc->base + LRADC_DATA1) &
>>>> >> +            lradc->variant->bits;
>>>> >> +        lradc->in_mvolts[1] = lradc->in[1] * lradc->variant-
>>>> >> vref /
>>>> >> +            lradc->variant->resolution / 1000;
>>>> >> +    }
>>>> >> +
>>>> >> +    writel(ints, lradc->base + LRADC_INTS);
>>>> >> +
>>>> >> +    return IRQ_HANDLED;
>>>> >> +}
> 
> This is highly unusual. Why keep reading those values ? hwmon drivers
> report data on request and are not continuously polling values.
> 
> 
>>>> >> +
>>>> >> +static int sun4i_lradc_open(struct device *dev)
>>>> >> +{
>>>> >> +    struct sun4i_lradc_data *lradc = dev_get_drvdata(dev);
>>>> >> +
>>>> >> +    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);
>>>> >> +
>>>> >> +    writel(CHAN0_DATA_IRQ | CHAN1_DATA_IRQ, lradc->base + LRADC_INTC);
>>>> >> +
>>>> >> +    return 0;
>>>> >> +}
>>>> >> +
>>>> >> +static void sun4i_lradc_close(struct device *dev)
>>>> >> +{
>>>> >> +    struct sun4i_lradc_data *lradc = dev_get_drvdata(dev);
>>>> >> +
>>>> >> +    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_resume(struct platform_device *pdev)
>>>> >> +{
>>>> >> +    struct device *dev = &pdev->dev;
>>>> >> +
>>>> >> +    return sun4i_lradc_open(dev);
>>>> >> +}
>>>> >> +
>>>> >> +static int sun4i_lradc_suspend(struct platform_device *pdev,
>>>> >> pm_message_t
>>>> >> state) +{
>>>> >> +    struct device *dev = &pdev->dev;
>>>> >> +
>>>> >> +    sun4i_lradc_close(dev);
>>>> >> +    return 0;
>>>> >> +}
>>>> >> +
>>>> >> +static int sun4i_lradc_remove(struct platform_device *pdev)
>>>> >> +{
>>>> >> +    struct device *dev = &pdev->dev;
>>>> >> +
>>>> >> +    sun4i_lradc_close(dev);
>>>> >> +    return 0;
>>>> >> +}
>>>> >> +
>>>> >> +static int sun4i_lradc_probe(struct platform_device *pdev)
>>>> >> +{
>>>> >> +    struct sun4i_lradc_data *lradc;
>>>> >> +    struct device *dev = &pdev->dev;
>>>> >> +    int 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) {
>>>> >> +        dev_err(&pdev->dev, "Missing '%s' or '%s' variant\n",
>>>> >> +            sun4i_lradc_of_match[0].compatible,
>>>> >> +            sun4i_lradc_of_match[1].compatible);
> 
> Drop the noise. Just return -EINVAL.
> 
>>>> >> +        return -EINVAL;
>>>> >> +    }
>>>> >> +
>>>> >> +    lradc->dev = dev;
>>>> >> +    lradc->hwmon_dev = devm_hwmon_device_register_with_groups(dev,
> 
> hwmon_dev is unnecessary in lradc.
> 
>>>> >
>>>> > "lradc",
> 
> Use devm_hwmon_device_register_with_info()
> 
>>>> >
>>>> >> +
>>>> >
>>>> > lradc, lradc_groups);
>>>> >
>>>> >> +    if (IS_ERR(lradc->hwmon_dev)) {
>>>> >> +        error = PTR_ERR(lradc->hwmon_dev);
>>>> >> +        return error;
>>>> >> +    }
>>>> >> +
>>>> >> +    lradc->base = devm_ioremap_resource(dev,
>>>> >> +            platform_get_resource(pdev, IORESOURCE_MEM,
>>>> >
>>>> > 0));
>>>> >
>>>> >> +    if (IS_ERR(lradc->base))
>>>> >> +        return PTR_ERR(lradc->base);
>>>> >> +
>>>> >> +    error = devm_request_irq(dev, platform_get_irq(pdev, 0),
>>>> >> +            sun4i_lradc_irq, 0, "sun4i-lradc-hwmon",
>>>> >
> This is all completely racy and must be done before registering
> the hwmon device.
> 
>>>> > lradc);
>>>> >
>>>> >> +    if (error)
>>>> >> +        return error;
>>>> >> +
>>>> >> +    error = sun4i_lradc_open(dev);
> 
> What does this function do ? Its name is misleading. There is no 
> "open".
> It seems to me it is really start/stop. If so, name it accordingly.
> 
> The stop function should be handled with devm_add_action_or_reset(),
> and there should be no remove function.
> 
>>>> >> +    if (error)
>>>> >> +        return error;
>>>> >> +
>>>> >> +    return 0;
> 
> This is equivalent of just
> 		return error;
> 
>>>> >> +}
>>>> >> +
>>>> >> +static const struct of_device_id sun4i_lradc_of_match[] = {
>>>> >> +    { .compatible = "allwinner,sun4i-a10-lradc-keys", .data =
>>>> >> &variant_sun4i_a10_lradc}, +    { .compatible =
>>>> >> "allwinner,sun8i-a83t-r-lradc", .data = &variant_sun8i_a83t_r_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),
>>>> >> +    },
>>>> >> +    .probe    = sun4i_lradc_probe,
>>>> >> +    .remove    = sun4i_lradc_remove,
>>>> >> +    .resume    = sun4i_lradc_resume,
> 
> Use .pm driver callback for suspend/resume.
> 
>>>> >> +    .suspend = sun4i_lradc_suspend,
>>>> >> +};
>>>> >> +
>>>> >> +module_platform_driver(sun4i_lradc_driver);
>>>> >> +
>>>> >> +
>>>> >> +
>>>> >> +
>>>> >> +MODULE_DESCRIPTION("Allwinner A13/A20 LRADC hwmon driver");
>>>> >> +MODULE_AUTHOR("Ruslan Zalata <rz@fabmicro.ru>");
>>>> >> +MODULE_LICENSE("GPL");