Re: [PATCH] hwmon: add initial NXP MC34VR500 PMIC monitoring support

[Mario Kicherer <dev@kicherer.org>]

Sorry, I did not know that there is separate submission guide for HWMON. 
I only
checked the code without the --strict parameter.

I made all changes and will send a v2 in a few seconds.

Thank you both for the review!

Best regards,
Mario

On 2023-01-16 17:48, Guenter Roeck wrote:
> On 1/16/23 07:55, Armin Wolf wrote:
>> Am 16.01.23 um 14:42 schrieb Mario Kicherer:
>> 
>>> This patch adds initial monitoring support for the MC34VR500 PMIC. In 
>>> its
>>> current form, input voltage and temperature events are reported to 
>>> hwmon.
>>> Other interrupts only generate a generic entry in the kernel log so 
>>> far.
>>> 
> 
> This is unacceptable. The driver should neither enable nor log any 
> interrupt
> bits it doesn't support.
> 
>>> The header file is copied from U-Boot and placed next to the C file 
>>> as the
>>> chip is usually only configured by the bootloader and it is unlikely 
>>> that
>>> it will be used by another Linux subsystem. If I should remove unused 
>>> parts
>>> or move the file to another path, let me know.
>>> 
>>> Datasheet:
>>>   - https://www.nxp.com/docs/en/data-sheet/MC34VR500.pdf
>>> 
>>> Signed-off-by: Mario Kicherer <dev@kicherer.org>
>>> ---
>>>   drivers/hwmon/Kconfig          |   7 +
>>>   drivers/hwmon/Makefile         |   1 +
>>>   drivers/hwmon/mc34vr500.c      | 382 
>>> +++++++++++++++++++++++++++++++++
>>>   drivers/hwmon/mc34vr500_pmic.h | 172 +++++++++++++++
> 
> Documentation/hwmon/mc34vr500 is missing.
> 
> Devicetree property documentation is missing (though that doesn't
> really matter since the defined property is unacceptable).
> 
>>>   4 files changed, 562 insertions(+)
>>>   create mode 100644 drivers/hwmon/mc34vr500.c
>>>   create mode 100644 drivers/hwmon/mc34vr500_pmic.h
>>> 
>>> diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
>>> index 3176c33af6c6..b917c2528b62 100644
>>> --- a/drivers/hwmon/Kconfig
>>> +++ b/drivers/hwmon/Kconfig
>>> @@ -2350,6 +2350,13 @@ config SENSORS_INTEL_M10_BMC_HWMON
>>>         sensors monitor various telemetry data of different 
>>> components on the
>>>         card, e.g. board temperature, FPGA core 
>>> temperature/voltage/current.
>>> 
>>> +config SENSORS_MC34VR500
>>> +    tristate "NXP MC34VR500 hardware monitoring driver"
>>> +    depends on I2C
>>> +    help
>>> +      If you say yes here you get support for the temperature and 
>>> input
>>> +      voltage sensors of the NXP MC34VR500.
>>> +
>>>   if ACPI
>>> 
>>>   comment "ACPI drivers"
>>> diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile
>>> index e2e4e87b282f..99e8cd8275c5 100644
>>> --- a/drivers/hwmon/Makefile
>>> +++ b/drivers/hwmon/Makefile
>>> @@ -216,6 +216,7 @@ obj-$(CONFIG_SENSORS_W83L786NG)    += w83l786ng.o
>>>   obj-$(CONFIG_SENSORS_WM831X)    += wm831x-hwmon.o
>>>   obj-$(CONFIG_SENSORS_WM8350)    += wm8350-hwmon.o
>>>   obj-$(CONFIG_SENSORS_XGENE)    += xgene-hwmon.o
>>> +obj-$(CONFIG_SENSORS_MC34VR500)    += mc34vr500.o
>>> 
>>>   obj-$(CONFIG_SENSORS_OCC)    += occ/
>>>   obj-$(CONFIG_SENSORS_PECI)    += peci/
>>> diff --git a/drivers/hwmon/mc34vr500.c b/drivers/hwmon/mc34vr500.c
>>> new file mode 100644
>>> index 000000000000..bddf108d05ae
>>> --- /dev/null
>>> +++ b/drivers/hwmon/mc34vr500.c
>>> @@ -0,0 +1,382 @@
>>> +// SPDX-License-Identifier: GPL-2.0-or-later
>>> +/*
>>> + * An hwmon driver for the NXP MC34VR500
>>> + *
>>> + * Copyright 2022 Mario Kicherer <dev@kicherer.org>
>>> + */
>>> +
>>> +#include <linux/hwmon.h>
>>> +#include <linux/hwmon-sysfs.h>
>>> +#include <linux/i2c.h>
>>> +#include <linux/regmap.h>
>>> +#include "mc34vr500_pmic.h"
>>> +
>>> +#define MC34VR500_DEVICEID_VALUE    0x14
>>> +
>>> +// INTSENSE0
> 
> Please no C++ comments.
> 
>>> +#define ENS_BIT        (1<<0)
>>> +#define LOWVINS_BIT    (1<<1)
>>> +#define THERM110S_BIT    (1<<2)
>>> +#define THERM120S_BIT    (1<<3)
>>> +#define THERM125S_BIT    (1<<4)
>>> +#define THERM130S_BIT    (1<<5)
>>> +
>>> +// INTSENSE1
>>> +#define SW1FAULTS1_BIT    (1<<0)
>>> +#define SW1FAULTS2_BIT    (1<<1)
>>> +#define SW1FAULTS3_BIT    (1<<2)
>>> +#define SW2FAULTS_BIT    (1<<3)
>>> +#define SW3FAULTS1_BIT    (1<<4)
>>> +#define SW3FAULTS2_BIT    (1<<5)
>>> +#define SW4FAULTS_BIT    (1<<6)
>>> +
>>> +// INTSENSE4
>>> +#define LDO1FAULTS_BIT    (1<<1)
>>> +#define LDO2FAULTS_BIT    (1<<2)
>>> +#define LDO3FAULTS_BIT    (1<<3)
>>> +#define LDO4FAULTS_BIT    (1<<4)
>>> +#define LDO5FAULTS_BIT    (1<<5)
>> 
>> Maybe you could use BIT() here?
>> 
> 
> Documentation/hwmon/submitting-patches.rst says:
> 
> * Please run your patch through 'checkpatch --strict'. There should be 
> no
>   errors, no warnings, and few if any check messages. If there are any
>   messages, please be prepared to explain.
> 
> With
> 
> total: 0 errors, 2 warnings, 43 checks, 574 lines checked
> 
> the author will have a lot of explaining to do.
> 
> I don't think the author read that document, though, because various
> other requirements in that document are not followed either.
> Some of the more obvious ones:
> 
> * Limit the number of kernel log messages. In general, your driver 
> should not
>   generate an error message just because a runtime operation failed. 
> Report
>   errors to user space instead, using an appropriate error code. Keep 
> in mind
>   that kernel error log messages not only fill up the kernel log, but 
> also are
>   printed synchronously, most likely with interrupt disabled, often to 
> a serial
>   console. Excessive logging can seriously affect system performance.
> 
> * Use devm_hwmon_device_register_with_info() or, if your driver needs a 
> remove
>   function, hwmon_device_register_with_info() to register your driver 
> with the
>   hwmon subsystem. Try using devm_add_action() instead of a remove 
> function if
>   possible. Do not use hwmon_device_register().
> 
> * Document the driver in Documentation/hwmon/<driver_name>.rst.
> 
> * Add the driver to Kconfig and Makefile in alphabetical order.
> 
> That really makes me wonder if I should create a form letter advising 
> people
> to follow the document, and that I'll only review the driver if there 
> are no
> violations.
> 
>>> +
>>> +struct mc34vr500_data {
>>> +    struct i2c_client    *client;
>>> +    struct regmap *regmap;
>>> +};
>>> +
>>> +static ssize_t mc34vr500_bool_show(struct device *dev,
>>> +                   struct device_attribute *da, char *buf)
>>> +{
>>> +    struct sensor_device_attribute *attr = to_sensor_dev_attr(da);
>>> +    struct mc34vr500_data *data = (struct mc34vr500_data *) 
>>> dev_get_drvdata(dev);
>>> +    unsigned int reg;
>>> +    int ret;
>>> +
>>> +    ret = regmap_read(data->regmap, MC34VR500_INTSENSE0 + 
>>> (attr->index >> 8), &reg);
>>> +    if (ret < 0)
>>> +        return ret;
>>> +
>>> +    reg &= (attr->index & 0xff);
>>> +
>>> +    return sysfs_emit(buf, "%d\n", !!reg);
>>> +}
>>> +
>>> +// INTSENSE0
>>> +static SENSOR_DEVICE_ATTR_RO(in1_alarm, mc34vr500_bool, 0x00 | 
>>> LOWVINS_BIT);
> 
> This should really be either _min_alarm or _lcrit_alarm.
> 
>>> +static SENSOR_DEVICE_ATTR_RO(temp110_alarm, mc34vr500_bool, 0x00 | 
>>> THERM110S_BIT);
>>> +static SENSOR_DEVICE_ATTR_RO(temp120_alarm, mc34vr500_bool, 0x00 | 
>>> THERM120S_BIT);
>>> +static SENSOR_DEVICE_ATTR_RO(temp125_alarm, mc34vr500_bool, 0x00 | 
>>> THERM125S_BIT);
>>> +static SENSOR_DEVICE_ATTR_RO(temp130_alarm, mc34vr500_bool, 0x00 | 
>>> THERM130S_BIT);
>> 
>> Does the chip support up to 130 temperature channels? If no, then 
>> maybe you could
>> name them "temp1_*", ..., "temp5_*" and use tempX_label for the 
>> labeling.
>> 
> 
> No, I checked the datasheet. This reflects temperatures and is an abuse 
> of the hwmon ABI.
> There is only a single temperature. The author will have to decide 
> which of the alarm
> attributes (_max_alarm, _crit_alarm, _emergency_alarm) to use for the 
> four limits.
> 
>>> +
>>> +// INTSENSE1
>>> +static SENSOR_DEVICE_ATTR_RO(curr1_alarm, mc34vr500_bool, 0x300 | 
>>> SW1FAULTS1_BIT);
>>> +static SENSOR_DEVICE_ATTR_RO(curr2_alarm, mc34vr500_bool, 0x300 | 
>>> SW1FAULTS2_BIT);
>>> +static SENSOR_DEVICE_ATTR_RO(curr3_alarm, mc34vr500_bool, 0x300 | 
>>> SW1FAULTS3_BIT);
>>> +static SENSOR_DEVICE_ATTR_RO(curr4_alarm, mc34vr500_bool, 0x300 | 
>>> SW2FAULTS_BIT);
>>> +static SENSOR_DEVICE_ATTR_RO(curr5_alarm, mc34vr500_bool, 0x300 | 
>>> SW3FAULTS1_BIT);
>>> +static SENSOR_DEVICE_ATTR_RO(curr6_alarm, mc34vr500_bool, 0x300 | 
>>> SW3FAULTS2_BIT);
>>> +static SENSOR_DEVICE_ATTR_RO(curr7_alarm, mc34vr500_bool, 0x300 | 
>>> SW4FAULTS_BIT);
>>> +
>>> +// INTSENSE4
>>> +static SENSOR_DEVICE_ATTR_RO(curr8_alarm, mc34vr500_bool, 0x600 | 
>>> LDO1FAULTS_BIT);
>>> +static SENSOR_DEVICE_ATTR_RO(curr9_alarm, mc34vr500_bool, 0x600 | 
>>> LDO2FAULTS_BIT);
>>> +static SENSOR_DEVICE_ATTR_RO(curr10_alarm, mc34vr500_bool, 0x600 | 
>>> LDO3FAULTS_BIT);
>>> +static SENSOR_DEVICE_ATTR_RO(curr11_alarm, mc34vr500_bool, 0x600 | 
>>> LDO4FAULTS_BIT);
>>> +static SENSOR_DEVICE_ATTR_RO(curr12_alarm, mc34vr500_bool, 0x600 | 
>>> LDO5FAULTS_BIT);
>>> +
>>> +static struct attribute *mc34vr500_attrs[] = {
>>> +    &sensor_dev_attr_in1_alarm.dev_attr.attr,
>>> +    &sensor_dev_attr_temp110_alarm.dev_attr.attr,
>>> +    &sensor_dev_attr_temp120_alarm.dev_attr.attr,
>>> +    &sensor_dev_attr_temp125_alarm.dev_attr.attr,
>>> +    &sensor_dev_attr_temp130_alarm.dev_attr.attr,
>>> +
>>> +    &sensor_dev_attr_curr1_alarm.dev_attr.attr,
>>> +    &sensor_dev_attr_curr2_alarm.dev_attr.attr,
>>> +    &sensor_dev_attr_curr3_alarm.dev_attr.attr,
>>> +    &sensor_dev_attr_curr4_alarm.dev_attr.attr,
>>> +    &sensor_dev_attr_curr5_alarm.dev_attr.attr,
>>> +    &sensor_dev_attr_curr6_alarm.dev_attr.attr,
>>> +    &sensor_dev_attr_curr7_alarm.dev_attr.attr,
>>> +
>>> +    &sensor_dev_attr_curr8_alarm.dev_attr.attr,
>>> +    &sensor_dev_attr_curr9_alarm.dev_attr.attr,
>>> +    &sensor_dev_attr_curr10_alarm.dev_attr.attr,
>>> +    &sensor_dev_attr_curr11_alarm.dev_attr.attr,
>>> +    &sensor_dev_attr_curr12_alarm.dev_attr.attr,
> 
> There are only 9 current channels (SW1..SW4 and LDO1..LDO5). Some of 
> them
> consume more than one interrupt status bit, but the documentation 
> doesn't
> say what the two bits exactly refer to. Maybe to multiple phases ?
> Either that (but that would not explain why only two of the four SWx
> channels have two status bits) or this is another abuse of the hwmon 
> ABI.
> 
>>> +    NULL
>>> +};
>>> +ATTRIBUTE_GROUPS(mc34vr500);
>> 
>> You should use the new hwmon_device_register_with_info() API, which 
>> does
>> the sysfs handling for you.
>> 
> 
> should -> must. I won't accept new drivers if they don't use the new 
> API.
> 
>>> +
>>> +static irqreturn_t mc34vr500_process_interrupt(int irq, void 
>>> *userdata)
>>> +{
>>> +    struct mc34vr500_data *data = (struct mc34vr500_data *) 
>>> userdata;
>>> +    struct i2c_client *client = data->client;
>>> +    unsigned int reg;
>>> +    int ret;
>>> +
>>> +    pr_debug("mc34vr500: received interrupt\n");
>>> +
>>> +    ret = regmap_read(data->regmap, MC34VR500_INTSTAT0, &reg);
>>> +    if (ret < 0) {
>>> +        dev_err(&client->dev, "unable to read mc34vr500 intsense0 
>>> register\n");
>> 
>> Maybe use some sort of ratelimiting here? Otherwise it could 
>> potentially fill up
>> the kernel log with redundant messages. Or maybe just omit this and 
>> all the other
>> error messages?
>> 
>>> +        return ret;
>>> +    }
>>> +
>>> +    if (reg) {
>>> +        pr_warn("mc34vr500: interrupt intstat0 %u\n", reg);
>> 
>> Same as above, it could fill up the kernel log is the alarm interrupt 
>> is triggered fast enough.
>> 
> 
> This isn't acceptable anyway. It logs a message with every interrupts.
> All those pr_{warn, notice} messages need to be dropped from the 
> driver.
> 
>>> +
>>> +        if (reg & LOWVINS_BIT) {
>>> +            ret = hwmon_notify_event(&client->dev, hwmon_in,
>>> +                         hwmon_in_alarm, 1);
> 
> Notifications need to be on the hwmon device.
> 
>>> +            if (ret)
>>> +                dev_err(&client->dev,
>>> +                    "mc34vr500: error, hwmon_notify_event() failed: 
>>> %d\n",
>>> +                    ret);
>>> +        }
>>> +
>>> +        if (reg & THERM110S_BIT) {
>>> +            ret = hwmon_notify_event(&client->dev, hwmon_temp,
>>> +                         hwmon_temp_alarm, 110);
>>> +            if (ret)
>>> +                dev_err(&client->dev,
>>> +                    "mc34vr500: error, hwmon_notify_event() failed: 
>>> %d\n",
>>> +                    ret);
>>> +        }
> 
> hwmon_notify_event() only fails if bad data is passed to it, ie if the 
> driver
> code is bad. Checking and logging it is only necessary if the code was 
> never
> tested.
> 
>>> +
>>> +        reg = 0;
>>> +        ret = regmap_write(data->regmap, MC34VR500_INTSTAT0, reg);
>>> +        if (ret) {
>>> +            dev_err(&client->dev, "unable to enable intmask0 
>>> interrupts\n");
>>> +            return ret;
>>> +        }
>>> +    }
>>> +
>>> +    ret = regmap_read(data->regmap, MC34VR500_INTSTAT1, &reg);
>>> +    if (ret < 0) {
>>> +        dev_err(&client->dev, "unable to read mc34vr500 intsense1 
>>> register\n");
>>> +        return ret;
>>> +    }
>>> +
>>> +    if (reg) {
>>> +        pr_warn("mc34vr500: interrupt intstat1 %u\n", reg);
>>> +
>>> +        reg = 0;
>>> +        ret = regmap_write(data->regmap, MC34VR500_INTSTAT1, reg);
>>> +        if (ret) {
>>> +            dev_err(&client->dev, "unable to enable intmask1 
>>> interrupts\n");
>>> +            return ret;
>>> +        }
>>> +    }
>>> +
>>> +    ret = regmap_read(data->regmap, MC34VR500_INTSTAT4, &reg);
>>> +    if (ret < 0) {
>>> +        dev_err(&client->dev, "unable to read mc34vr500 intsense4 
>>> register\n");
>>> +        return ret;
>>> +    }
>>> +
>>> +    if (reg) {
>>> +        pr_warn("mc34vr500: interrupt intstat4 %u\n", reg);
>>> +
>>> +        reg = 0;
>>> +        ret = regmap_write(data->regmap, MC34VR500_INTSTAT4, reg);
>>> +        if (ret) {
>>> +            dev_err(&client->dev, "unable to enable intmask4 
>>> interrupts\n");
>>> +            return ret;
>>> +        }
>>> +    }
>>> +
>>> +    return IRQ_HANDLED;
>>> +}
>>> +
>>> +static const struct regmap_config mc34vr500_regmap_config = {
>>> +    .reg_bits = 8,
>>> +    .val_bits = 8,
>>> +    .max_register = MC34VR500_PWRGD_EN,
>>> +};
>>> +
>>> +static int mc34vr500_probe(struct i2c_client *client)
>>> +{
>>> +    struct device *dev = &client->dev;
>>> +    struct mc34vr500_data *data;
>>> +    struct device *hwmon_dev;
>>> +    int ret;
>>> +    unsigned int reg, revid, fabid;
>>> +    struct regmap *regmap;
>>> +    u32 interrupt_mask;
>>> +
>>> +    regmap = devm_regmap_init_i2c(client, &mc34vr500_regmap_config);
>>> +    if (IS_ERR(regmap)) {
>>> +        dev_err(dev, "failed to allocate register map\n");
>>> +        return PTR_ERR(regmap);
>>> +    }
>>> +
>>> +    data = devm_kzalloc(dev, sizeof(struct mc34vr500_data), 
>>> GFP_KERNEL);
>>> +    if (!data)
>>> +        return -ENOMEM;
>>> +
>>> +    data->regmap = regmap;
>>> +    data->client = client;
>>> +
>>> +    ret = regmap_read(regmap, MC34VR500_DEVICEID, &reg);
>>> +    if (ret < 0) {
>>> +        dev_err(dev, "unable to read config register\n");
>>> +
>>> +        return ret;
>>> +    }
>>> +
>>> +    if (reg != MC34VR500_DEVICEID_VALUE) {
>>> +        dev_err(dev, "invalid config register value 0x%x\n", reg);
>>> +
>>> +        return -ENODEV;
>>> +    }
> 
> This simply reflects that the chip isn't there and should result in 
> -ENODEV
> without logging noise.
> 
>>> +
>>> +    ret = regmap_read(regmap, MC34VR500_SILICONREVID, &revid);
>>> +    if (ret < 0) {
>>> +        dev_err(dev, "unable to read mc34vr500 revid register\n");
>>> +        return ret;
>>> +    }
>>> +
>>> +    ret = regmap_read(regmap, MC34VR500_FABID, &fabid);
>>> +    if (ret < 0) {
>>> +        dev_err(dev, "unable to read mc34vr500 fabid register\n");
>>> +        return ret;
>>> +    }
>>> +
>>> +    pr_notice("mc34vr500: revid 0x%x fabid 0x%x\n", revid, fabid);
>>> +
>>> +
>>> +    ret = of_property_read_u32(dev->of_node,
>>> +                   "interrupt-mask",
>>> +                   &interrupt_mask);
>>> +    if (ret == -EINVAL) {
>>> +        interrupt_mask = 0;
>>> +    } else if (ret) {
>>> +        dev_err(dev, "Error reading interrupt_mask\n");
>>> +        return ret;
>>> +    }
>>> +
> 
> This is inappropriate. The driver needs to enable the interrupt bits it 
> needs,
> and nothing else.
> 
> If there is a desire to enable individual channels, that should be done
> with a per-channel deviretree file and with the {in, current, 
> temp}X_enable
> sysfs attribute.
> 
>>> +    ret = regmap_read(regmap, MC34VR500_INTSTAT0, &reg);
>>> +    if (ret < 0) {
>>> +        dev_err(dev, "unable to read mc34vr500 intstat0 
>>> register\n");
>>> +        return ret;
>>> +    }
>>> +    reg = reg & (~interrupt_mask);
>>> +
>>> +    if (reg)
>>> +        pr_notice("mc34vr500: intstat0: 0x%x\n", reg);
>>> +
>>> +    if (reg & LOWVINS_BIT)
>>> +        pr_notice("mc34vr500: low input voltage detected\n");
>>> +
>>> +    if (reg & THERM130S_BIT)
>>> +        pr_notice("mc34vr500: temperature >= 130°c\n");
>>> +    else if (reg & THERM125S_BIT)
>>> +        pr_notice("mc34vr500: temperature >= 125°c\n");
>>> +    else if (reg & THERM120S_BIT)
>>> +        pr_notice("mc34vr500: temperature >= 120°c\n");
>>> +    else if (reg & THERM110S_BIT)
>>> +        pr_notice("mc34vr500: temperature >= 110°c\n");
>>> +
>>> +    ret = regmap_read(regmap, MC34VR500_INTSTAT1, &reg);
>>> +    if (ret < 0) {
>>> +        dev_err(dev, "unable to read mc34vr500 intstat1 
>>> register\n");
>>> +        return ret;
>>> +    }
>>> +    reg = reg & ((~interrupt_mask) >> 8);
>>> +
>>> +    if (reg)
>>> +        pr_notice("mc34vr500: intstat1: 0x%x\n", reg);
>>> +
>>> +    ret = regmap_read(regmap, MC34VR500_INTSTAT4, &reg);
>>> +    if (ret < 0) {
>>> +        dev_err(dev, "unable to read mc34vr500 intstat4 
>>> register\n");
>>> +        return ret;
>>> +    }
>>> +    reg = reg & ((~interrupt_mask) >> 16);
>>> +
>>> +    if (reg)
>>> +        pr_notice("mc34vr500: intstat4: 0x%x\n", reg);
>>> +
> 
> Most of the above does not belong in the probe function and needs to be 
> dropped.
> It is not the responsibility of the probe function to log interrupt 
> status
> values.
> 
>>> +    hwmon_dev = devm_hwmon_device_register_with_groups(dev, 
>>> client->name,
>>> +                               data,
>>> +                               mc34vr500_groups);
>>> +
>>> +    if (client->irq) {
>>> +        pr_notice("mc34vr500: enabling IRQ...\n");
>>> +
> 
> Example for driver noise. I could try to mark them all but gave up 
> after
> realizing how noisy this driver is. This all needs to go.
> 
>>> +        ret = devm_request_threaded_irq(dev, client->irq, NULL,
>>> +                        mc34vr500_process_interrupt,
>>> +                        IRQF_TRIGGER_RISING |
>>> +                            IRQF_ONESHOT |
>>> +                            IRQF_SHARED,
>>> +                        dev_name(dev),
>>> +                        data);
>>> +        if (ret) {
>>> +            dev_err(dev, "Error requesting irq\n");
>>> +            return ret;
>>> +        }
>>> +
>>> +        /* clear interrupts */
>>> +        reg = 0;
>>> +        ret = regmap_write(regmap, MC34VR500_INTSTAT0, reg);
>>> +        if (ret) {
>>> +            dev_err(dev, "unable to write intstat0 register\n");
>>> +            return ret;
>>> +        }
>>> +
>>> +        /* enable interrupts */
>>> +        ret = regmap_write(regmap, MC34VR500_INTMASK0, 
>>> interrupt_mask & 0xff);
>>> +        if (ret) {
>>> +            dev_err(dev, "unable to enable intmask0 interrupts\n");
>>> +            return ret;
>>> +        }
>>> +
>>> +        reg = 0;
>>> +        ret = regmap_write(regmap, MC34VR500_INTSTAT1, reg);
>>> +        if (ret) {
>>> +            dev_err(dev, "unable to write intstat1 register\n");
>>> +            return ret;
>>> +        }
>>> +
>>> +        ret = regmap_write(regmap, MC34VR500_INTMASK1, 
>>> (interrupt_mask >> 8) & 0xff);
>>> +        if (ret) {
>>> +            dev_err(dev, "unable to enable intmask1 interrupts\n");
>>> +            return ret;
>>> +        }
>>> +
>>> +        reg = 0;
>>> +        ret = regmap_write(regmap, MC34VR500_INTSTAT4, reg);
>>> +        if (ret) {
>>> +            dev_err(dev, "unable to write intstat1 register\n");
>>> +            return ret;
>>> +        }
>>> +
>>> +        ret = regmap_write(regmap, MC34VR500_INTMASK4, 
>>> (interrupt_mask >> 16) & 0xff);
>>> +        if (ret) {
>>> +            dev_err(dev, "unable to enable intmask1 interrupts\n");
>>> +            return ret;
>>> +        }
>>> +    }
>>> +
>>> +    return PTR_ERR_OR_ZERO(hwmon_dev);
>>> +}
>>> +
>>> +static const struct i2c_device_id mc34vr500_id[] = {
>>> +    { "mc34vr500", 0 },
>>> +    {}
>>> +};
>>> +MODULE_DEVICE_TABLE(i2c, mc34vr500_id);
>>> +
>>> +static struct i2c_driver mc34vr500_driver = {
>>> +    .driver = {
>>> +        .name    = "mc34vr500",
>>> +    },
>>> +    .probe_new = mc34vr500_probe,
>>> +    .id_table = mc34vr500_id,
>>> +};
>>> +
>>> +module_i2c_driver(mc34vr500_driver);
>>> +
>>> +MODULE_AUTHOR("Mario Kicherer <dev@kicherer.org>");
>>> +
>>> +MODULE_DESCRIPTION("MC34VR500 driver");
>>> +MODULE_LICENSE("GPL");
>>> diff --git a/drivers/hwmon/mc34vr500_pmic.h 
>>> b/drivers/hwmon/mc34vr500_pmic.h
>>> new file mode 100644
>>> index 000000000000..1543ac692d72
>>> --- /dev/null
>>> +++ b/drivers/hwmon/mc34vr500_pmic.h
>>> @@ -0,0 +1,172 @@
>>> +/* SPDX-License-Identifier: GPL-2.0+ */
>>> +/*
>>> + * Copyright 2016 Freescale Semiconductor, Inc.
>>> + * Hou Zhiqiang <Zhiqiang.Hou@freescale.com>
>>> + */
>>> +
>>> +#ifndef __MC34VR500_H_
>>> +#define __MC34VR500_H_
>>> +
>>> +#define MC34VR500_I2C_ADDR    0x08
>>> +
>>> +/* Drivers name */
>>> +#define MC34VR500_REGULATOR_DRIVER    "mc34vr500_regulator"
>>> +
>>> +/* Register map */
>>> +enum {
>>> +    MC34VR500_DEVICEID        = 0x00,
>>> +
>>> +    MC34VR500_SILICONREVID        = 0x03,
>>> +    MC34VR500_FABID,
>>> +    MC34VR500_INTSTAT0,
>>> +    MC34VR500_INTMASK0,
>>> +    MC34VR500_INTSENSE0,
>>> +    MC34VR500_INTSTAT1,
>>> +    MC34VR500_INTMASK1,
>>> +    MC34VR500_INTSENSE1,
>>> +
>>> +    MC34VR500_INTSTAT4        = 0x11,
>>> +    MC34VR500_INTMASK4,
>>> +    MC34VR500_INTSENSE4,
>>> +
>>> +    MC34VR500_PWRCTL        = 0x1B,
>>> +
>>> +    MC34VR500_SW1VOLT        = 0x2E,
>>> +    MC34VR500_SW1STBY,
>>> +    MC34VR500_SW1OFF,
>>> +    MC34VR500_SW1MODE,
>>> +    MC34VR500_SW1CONF,
>>> +    MC34VR500_SW2VOLT,
>>> +    MC34VR500_SW2STBY,
>>> +    MC34VR500_SW2OFF,
>>> +    MC34VR500_SW2MODE,
>>> +    MC34VR500_SW2CONF,
>>> +
>>> +    MC34VR500_SW3VOLT        = 0x3C,
>>> +    MC34VR500_SW3STBY,
>>> +    MC34VR500_SW3OFF,
>>> +    MC34VR500_SW3MODE,
>>> +    MC34VR500_SW3CONF,
>>> +
>>> +    MC34VR500_SW4VOLT        = 0x4A,
>>> +    MC34VR500_SW4STBY,
>>> +    MC34VR500_SW4OFF,
>>> +    MC34VR500_SW4MODE,
>>> +    MC34VR500_SW4CONF,
>>> +
>>> +    MC34VR500_REFOUTCRTRL        = 0x6A,
>>> +
>>> +    MC34VR500_LDO1CTL        = 0x6D,
>>> +    MC34VR500_LDO2CTL,
>>> +    MC34VR500_LDO3CTL,
>>> +    MC34VR500_LDO4CTL,
>>> +    MC34VR500_LDO5CTL,
>>> +
>>> +    MC34VR500_PAGE_REGISTER        = 0x7F,
>>> +
>>> +    /* Internal RAM */
>>> +    MC34VR500_SW1_VOLT        = 0xA8,
>>> +    MC34VR500_SW1_SEQ,
>>> +    MC34VR500_SW1_CONFIG,
>>> +
>>> +    MC34VR500_SW2_VOLT        = 0xAC,
>>> +    MC34VR500_SW2_SEQ,
>>> +    MC34VR500_SW2_CONFIG,
>>> +
>>> +    MC34VR500_SW3_VOLT        = 0xB0,
>>> +    MC34VR500_SW3_SEQ,
>>> +    MC34VR500_SW3_CONFIG,
>>> +
>>> +    MC34VR500_SW4_VOLT        = 0xB8,
>>> +    MC34VR500_SW4_SEQ,
>>> +    MC34VR500_SW4_CONFIG,
>>> +
>>> +    MC34VR500_REFOUT_SEQ        = 0xC4,
>>> +
>>> +    MC34VR500_LDO1_VOLT        = 0xCC,
>>> +    MC34VR500_LDO1_SEQ,
>>> +
>>> +    MC34VR500_LDO2_VOLT        = 0xD0,
>>> +    MC34VR500_LDO2_SEQ,
>>> +
>>> +    MC34VR500_LDO3_VOLT        = 0xD4,
>>> +    MC34VR500_LDO3_SEQ,
>>> +
>>> +    MC34VR500_LDO4_VOLT        = 0xD8,
>>> +    MC34VR500_LDO4_SEQ,
>>> +
>>> +    MC34VR500_LDO5_VOLT        = 0xDC,
>>> +    MC34VR500_LDO5_SEQ,
>>> +
>>> +    MC34VR500_PU_CONFIG1        = 0xE0,
>>> +
>>> +    MC34VR500_TBB_POR        = 0xE4,
>>> +
>>> +    MC34VR500_PWRGD_EN        = 0xE8,
>>> +
>>> +    MC34VR500_NUM_OF_REGS,
>>> +};
>>> +
>>> +/* Registor offset based on SWxVOLT register */
>>> +#define MC34VR500_VOLT_OFFSET    0
>>> +#define MC34VR500_STBY_OFFSET    1
>>> +#define MC34VR500_OFF_OFFSET    2
>>> +#define MC34VR500_MODE_OFFSET    3
>>> +#define MC34VR500_CONF_OFFSET    4
>>> +
>>> +#define SW_MODE_MASK    0xf
>>> +#define SW_MODE_SHIFT    0
>>> +
>>> +#define LDO_VOL_MASK    0xf
>>> +#define LDO_EN        (1 << 4)
>>> +#define LDO_MODE_SHIFT    4
>>> +#define LDO_MODE_MASK    (1 << 4)
>>> +#define LDO_MODE_OFF    0
>>> +#define LDO_MODE_ON    1
>>> +
>>> +#define REFOUTEN    (1 << 4)
>>> +
>>> +/*
>>> + * Regulator Mode Control
>>> + *
>>> + * OFF: The regulator is switched off and the output voltage is 
>>> discharged.
>>> + * PFM: In this mode, the regulator is always in PFM mode, which is 
>>> useful
>>> + *      at light loads for optimized efficiency.
>>> + * PWM: In this mode, the regulator is always in PWM mode operation
>>> + *    regardless of load conditions.
>>> + * APS: In this mode, the regulator moves automatically between 
>>> pulse
>>> + *    skipping mode and PWM mode depending on load conditions.
>>> + *
>>> + * SWxMODE[3:0]
>>> + * Normal Mode  |  Standby Mode    |      value
>>> + *   OFF        OFF        0x0
>>> + *   PWM        OFF        0x1
>>> + *   PFM        OFF        0x3
>>> + *   APS        OFF        0x4
>>> + *   PWM        PWM        0x5
>>> + *   PWM        APS        0x6
>>> + *   APS        APS        0x8
>>> + *   APS        PFM        0xc
>>> + *   PWM        PFM        0xd
>>> + */
>>> +#define OFF_OFF        0x0
>>> +#define PWM_OFF        0x1
>>> +#define PFM_OFF        0x3
>>> +#define APS_OFF        0x4
>>> +#define PWM_PWM        0x5
>>> +#define PWM_APS        0x6
>>> +#define APS_APS        0x8
>>> +#define APS_PFM        0xc
>>> +#define PWM_PFM        0xd
>>> +
>>> +enum swx {
>>> +    SW1 = 0,
>>> +    SW2,
>>> +    SW3,
>>> +    SW4,
>>> +};
>>> +
>>> +int mc34vr500_get_sw_volt(uint8_t sw);
>>> +int mc34vr500_set_sw_volt(uint8_t sw, int sw_volt);
>>> +int power_mc34vr500_init(unsigned char bus);
>>> +#endif /* __MC34VR500_PMIC_H_ */