[1/2] hwmon: (isl28022) new driver for ISL28022 power monitor

Commit Message

Carsten Spieß July 26, 2023, 3:22 p.m. UTC

Driver for Renesas ISL28022 power monitor with I2C interface.
The device monitors voltage, current via shunt resistor
and calculated power.

Signed-off-by: Carsten Spieß <[e-mail]>
---
 Documentation/hwmon/index.rst    |   1 +
 Documentation/hwmon/isl28022.rst |  56 +++++
 MAINTAINERS                      |   7 +
 drivers/hwmon/Kconfig            |  11 +
 drivers/hwmon/Makefile           |   1 +
 drivers/hwmon/isl28022.c         | 405 +++++++++++++++++++++++++++++++
 6 files changed, 481 insertions(+)
 create mode 100644 Documentation/hwmon/isl28022.rst
 create mode 100644 drivers/hwmon/isl28022.c

Comments

Guenter Roeck July 26, 2023, 4:14 p.m. UTC | #1

On 7/26/23 08:22, Carsten Spieß wrote:
> Driver for Renesas ISL28022 power monitor with I2C interface.
> The device monitors voltage, current via shunt resistor
> and calculated power.
> 
> Signed-off-by: Carsten Spieß <[e-mail]>
> ---
>   Documentation/hwmon/index.rst    |   1 +
>   Documentation/hwmon/isl28022.rst |  56 +++++
>   MAINTAINERS                      |   7 +
>   drivers/hwmon/Kconfig            |  11 +
>   drivers/hwmon/Makefile           |   1 +
>   drivers/hwmon/isl28022.c         | 405 +++++++++++++++++++++++++++++++
>   6 files changed, 481 insertions(+)
>   create mode 100644 Documentation/hwmon/isl28022.rst
>   create mode 100644 drivers/hwmon/isl28022.c
> 
> diff --git a/Documentation/hwmon/index.rst b/Documentation/hwmon/index.rst
> index d11924667f76..c9548fc5c40e 100644
> --- a/Documentation/hwmon/index.rst
> +++ b/Documentation/hwmon/index.rst
> @@ -90,6 +90,7 @@ Hardware Monitoring Kernel Drivers
>      ir35221
>      ir38064
>      ir36021
> +   isl28022
>      isl68137
>      it87
>      jc42
> diff --git a/Documentation/hwmon/isl28022.rst b/Documentation/hwmon/isl28022.rst
> new file mode 100644
> index 000000000000..84c27ddcd33e
> --- /dev/null
> +++ b/Documentation/hwmon/isl28022.rst
> @@ -0,0 +1,56 @@
> +.. SPDX-License-Identifier: GPL-2.0-or-later
> +
> +Kernel driver isl28022
> +======================
> +
> +Supported chips:
> +
> +  * Renesas ISL28022
> +
> +    Prefix: 'isl28022'
> +
> +    Addresses scanned: none
> +
> +    Datasheet: Publicly available at the Renesas website
> +
> +	       https://www.renesas.com/us/en/www/doc/datasheet/isl28022.pdf
> +
> +Author:
> +    Carsten Spieß <[e-mail]>
> +
> +Description
> +-----------
> +
> +The ISL28022 is a power monitor with I2C interface. The device monitors
> +voltage, current via shunt resistor and calculated power.
> +
> +Usage Notes
> +-----------
> +
> +This driver does not auto-detect devices. You will have to instantiate the
> +device explicitly. Please see Documentation/i2c/instantiating-devices.rst for
> +details.
> +
> +The shunt value in micro-ohms, shunt gain and averaging can be set
> +via device tree at compile-time.

At _compile-time_ ?

> +Please refer to the Documentation/devicetree/bindings/hwmon/isl,isl28022.yaml
> +for bindings if the device tree is used.
> +
> +Sysfs entries
> +-------------
> +
> +The following attributes are supported. All attributes are read-only.
> +
> +======================= =======================================================
> +in0_input		shunt voltage (micro Volt)

No. You must not change the ABI like that.

> +in1_input		bus voltage (milli Volt)
> +
> +curr1_input		current (milli Ampere)
> +power1_input		power (micro Watt)
> +
> +note			current and power attributes are supported only when
> +			shunt value is configured via device tree

No. Use a reasonable default if there are no devicetree properties.
> +
> +			shunt voltage is in micro Volt, not milli Volt,
> +			to get useful values

I'd argue that shunt voltage is all but useless, but if you want to have it supported
it _has_ to be in mV.

Why not support limit attributes ?

> +======================= =======================================================
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 7abb5710e1bb..c61aa688cd11 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -11072,6 +11072,13 @@ S:	Maintained
>   F:	Documentation/filesystems/isofs.rst
>   F:	fs/isofs/
>   
> +ISL28022 HARDWARE MONITORING DRIVER
> +M:	Carsten Spieß <[e-mail]>
> +L:	[e-mail]
> +S:	Maintained
> +F:	Documentation/hwmon/isl28022.rst
> +F:	drivers/hwmon/isl28022.c
> +
>   IT87 HARDWARE MONITORING DRIVER
>   M:	Jean Delvare <[e-mail]>
>   L:	[e-mail]
> diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
> index 2913299c2c9e..3049c519e6ac 100644
> --- a/drivers/hwmon/Kconfig
> +++ b/drivers/hwmon/Kconfig
> @@ -800,6 +800,17 @@ config SENSORS_CORETEMP
>   	  sensor inside your CPU. Most of the family 6 CPUs
>   	  are supported. Check Documentation/hwmon/coretemp.rst for details.
>   
> +config SENSORS_ISL28022
> +	tristate "Renesas ISL28022"
> +	depends on I2C
> +	select REGMAP_I2C
> +	help
> +	  If you say yes here you get support for ISL28022 power monitor.
> +	  Check Documentation/hwmon/isl28022.rst for details.
> +
> +	  This driver can also be built as a module. If so, the module
> +	  will be called isl28022.
> +
>   config SENSORS_IT87
>   	tristate "ITE IT87xx and compatibles"
>   	depends on !PPC
> diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile
> index ff6bfd109c72..4046fed7f48d 100644
> --- a/drivers/hwmon/Makefile
> +++ b/drivers/hwmon/Makefile
> @@ -98,6 +98,7 @@ obj-$(CONFIG_SENSORS_INA2XX)	+= ina2xx.o
>   obj-$(CONFIG_SENSORS_INA238)	+= ina238.o
>   obj-$(CONFIG_SENSORS_INA3221)	+= ina3221.o
>   obj-$(CONFIG_SENSORS_INTEL_M10_BMC_HWMON) += intel-m10-bmc-hwmon.o
> +obj-$(CONFIG_SENSORS_ISL28022)	+= isl28022.o
>   obj-$(CONFIG_SENSORS_IT87)	+= it87.o
>   obj-$(CONFIG_SENSORS_JC42)	+= jc42.o
>   obj-$(CONFIG_SENSORS_K8TEMP)	+= k8temp.o
> diff --git a/drivers/hwmon/isl28022.c b/drivers/hwmon/isl28022.c
> new file mode 100644
> index 000000000000..67cf47c31600
> --- /dev/null
> +++ b/drivers/hwmon/isl28022.c
> @@ -0,0 +1,405 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * isl28022.c - driver for Renesas ISL28022 power monitor chip
> + *	 monitoring
> + * Copyright (c) 2023 Carsten Spieß <[e-mail]>
> + */
> +
> +#include <linux/module.h>
> +#include <linux/init.h>
> +#include <linux/slab.h>
> +#include <linux/i2c.h>
> +#include <linux/hwmon.h>
> +#include <linux/hwmon-sysfs.h>
> +#include <linux/err.h>
> +#include <linux/of_device.h>
> +#include <linux/of.h>
> +#include <linux/regmap.h>
> +#include <linux/util_macros.h>
> +#include <linux/regulator/consumer.h>
> +
> +/* ISL28022 registers */
> +#define ISL28022_REG_CONFIG	0x00
> +#define ISL28022_REG_SHUNT	0x01
> +#define ISL28022_REG_BUS	0x02
> +#define ISL28022_REG_POWER	0x03
> +#define ISL28022_REG_CURRENT	0x04
> +#define ISL28022_REG_CALIB	0x05
> +#define ISL28022_REG_SHUNT_THR	0x06
> +#define ISL28022_REG_BUS_THR	0x07
> +#define ISL28022_REG_INT	0x08
> +#define ISL28022_REG_AUX	0x09
> +#define ISL28022_REG_MAX	ISL28022_REG_AUX
> +
> +/* ISL28022 config flags */
> +/* mode flags */
> +#define ISL28022_MODE_SHIFT	0
> +#define ISL28022_MODE_MASK	0x0007
> +
> +#define ISL28022_MODE_PWR_DOWN	0x0
> +#define ISL28022_MODE_TRG_S	0x1
> +#define ISL28022_MODE_TRG_B	0x2
> +#define ISL28022_MODE_TRG_SB	0x3
> +#define ISL28022_MODE_ADC_OFF	0x4
> +#define ISL28022_MODE_CONT_S	0x5
> +#define ISL28022_MODE_CONT_B	0x6
> +#define ISL28022_MODE_CONT_SB	0x7
> +
> +/* shunt ADC settings */
> +#define ISL28022_SADC_SHIFT	3
> +#define ISL28022_SADC_MASK	0x0078
> +
> +#define ISL28022_BADC_SHIFT	7
> +#define ISL28022_BADC_MASK	0x0780
> +
> +#define ISL28022_ADC_12		0x0	/* 12 bit ADC */
> +#define ISL28022_ADC_13		0x1	/* 13 bit ADC */
> +#define ISL28022_ADC_14		0x2	/* 14 bit ADC */
> +#define ISL28022_ADC_15		0x3	/* 15 bit ADC */
> +#define ISL28022_ADC_15_1	0x8	/* 15 bit ADC, 1 sample */
> +#define ISL28022_ADC_15_2	0x9	/* 15 bit ADC, 2 samples */
> +#define ISL28022_ADC_15_4	0xA	/* 15 bit ADC, 4 samples */
> +#define ISL28022_ADC_15_8	0xB	/* 15 bit ADC, 8 samples */
> +#define ISL28022_ADC_15_16	0xC	/* 15 bit ADC, 16 samples */
> +#define ISL28022_ADC_15_32	0xD	/* 15 bit ADC, 32 samples */
> +#define ISL28022_ADC_15_64	0xE	/* 15 bit ADC, 64 samples */
> +#define ISL28022_ADC_15_128	0xF	/* 15 bit ADC, 128 samples */
> +
> +/* shunt voltage range */
> +#define ISL28022_PG_SHIFT	11
> +#define ISL28022_PG_MASK	0x1800
> +
> +#define ISL28022_PG_40		0x0	/* +/-40 mV */
> +#define ISL28022_PG_80		0x1	/* +/-80 mV */
> +#define ISL28022_PG_160		0x2	/* +/-160 mV */
> +#define ISL28022_PG_320		0x3	/* +/-3200 mV */
> +
> +/* bus voltage range */
> +#define ISL28022_BRNG_SHIFT	13
> +#define ISL28022_BRNG_MASK	0x6000
> +
> +#define ISL28022_BRNG_16	0x0	/* 16 V */
> +#define ISL28022_BRNG_32	0x1	/* 32 V */
> +#define ISL28022_BRNG_60	0x3	/* 60 V */
> +
> +/* reset */
> +#define ISL28022_RESET		0x8000
> +
> +struct isl28022_data {
> +	struct i2c_client	*client;
> +	struct regmap		*regmap;
> +	u32			shunt;
> +	u32			gain;
> +	u32			average;
> +	u16			config;
> +	u16			calib;
> +};
> +
> +static int isl28022_read(struct device *dev, enum hwmon_sensor_types type,
> +			 u32 attr, int channel, long *val)
> +{
> +	struct isl28022_data *data = dev_get_drvdata(dev);
> +	unsigned int regval;
> +	int err;
> +
> +	switch (type) {
> +	case hwmon_in:
> +		switch (attr) {
> +		case hwmon_in_input:
> +			err = regmap_read(data->regmap,
> +					  ISL28022_REG_SHUNT + channel, &regval);

That never reads REG_BUS.

> +			if (err < 0)
> +				return err;
> +			*val = (channel == 0) ?
> +					(long)((s16)((u16)regval)) * 10 :
> +					(long)(((u16)regval) & 0xFFFC);

I don't think the sign extensions are correct based on the datasheet.
This will have to use sign_extend.

> +			break;
> +		default:
> +			return -EINVAL;
> +		}
> +		break;
> +	case hwmon_curr:
> +		switch (attr) {
> +		case hwmon_curr_input:
> +			err = regmap_read(data->regmap,
> +					  ISL28022_REG_CURRENT, &regval);
> +			if (err < 0)
> +				return err;
> +			if (!data->shunt)
> +				return -EINVAL;

Getting an error message each time the "sensors" command is executed ?
Unacceptable.

> +			*val = ((long)regval * 10000L * (long)data->gain) /
> +				(long)(8 * data->shunt);
> +			break;
> +		default:
> +			return -EINVAL;
> +		}
> +		break;
> +	case hwmon_power:
> +		switch (attr) {
> +		case hwmon_power_input:
> +			err = regmap_read(data->regmap,
> +					  ISL28022_REG_POWER, &regval);
> +			if (err < 0)
> +				return err;
> +			if (!data->shunt)
> +				return -EINVAL;

Unacceptable.

> +			*val = ((long)regval * 409600000L * (long)data->gain) /
> +				(long)(8 * data->shunt);

I don't think this was checked for overflows.

> +			break;
> +		default:
> +			return -EINVAL;
> +		}
> +		break;
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	return 0;
> +}
> +
> +static umode_t isl28022_is_visible(const void *data, enum hwmon_sensor_types type,
> +				   u32 attr, int channel)
> +{
> +	switch (type) {
> +	case hwmon_in:
> +		switch (attr) {
> +		case hwmon_in_input:
> +			return 0444;
> +		}
> +		break;
> +	case hwmon_curr:
> +		switch (attr) {
> +		case hwmon_curr_input:
> +			return 0444;
> +		}
> +		break;
> +	case hwmon_power:
> +		switch (attr) {
> +		case hwmon_power_input:
> +			return 0444;
> +		}
> +		break;
> +	default:
> +		break;
> +	}
> +	return 0;
> +}
> +
> +static const struct hwmon_channel_info *isl28022_info[] = {
> +	HWMON_CHANNEL_INFO(in,
> +			   HWMON_I_INPUT,	/* channel 0: shunt voltage (µV) */
> +			   HWMON_I_INPUT),	/* channel 1: bus voltage (mV) */
> +	NULL
> +};
> +
> +static const struct hwmon_channel_info *isl28022_info_shunt[] = {
> +	HWMON_CHANNEL_INFO(in,
> +			   HWMON_I_INPUT,	/* channel 0: shunt voltage (µV) */
> +			   HWMON_I_INPUT),	/* channel 1: bus voltage (mV) */
> +	HWMON_CHANNEL_INFO(curr,
> +			   HWMON_C_INPUT), /* channel 1: current (mA) */
> +	HWMON_CHANNEL_INFO(power,
> +			   HWMON_P_INPUT),	/* channel 1: power (µW) */
> +	NULL
> +};
> +
> +static const struct hwmon_ops isl28022_hwmon_ops = {
> +	.is_visible = isl28022_is_visible,
> +	.read = isl28022_read,
> +};
> +
> +static const struct hwmon_chip_info isl28022_chip_info = {
> +	.ops = &isl28022_hwmon_ops,
> +	.info = isl28022_info,
> +};
> +
> +static const struct hwmon_chip_info isl28022_chip_info_shunt = {
> +	.ops = &isl28022_hwmon_ops,
> +	.info = isl28022_info_shunt,
> +};
> +
> +static bool isl28022_is_writeable_reg(struct device *dev, unsigned int reg)
> +{
> +	switch (reg) {
> +	case ISL28022_REG_CONFIG:
> +	case ISL28022_REG_CALIB:
> +	case ISL28022_REG_SHUNT_THR:
> +	case ISL28022_REG_BUS_THR:
> +	case ISL28022_REG_INT:
> +	case ISL28022_REG_AUX:
> +		return true;
> +	}
> +
> +	return false;
> +}
> +
> +static bool isl28022_is_volatile_reg(struct device *dev, unsigned int reg)
> +{
> +	switch (reg) {
> +	case ISL28022_REG_CONFIG:
> +	case ISL28022_REG_SHUNT:
> +	case ISL28022_REG_BUS:
> +	case ISL28022_REG_POWER:
> +	case ISL28022_REG_CURRENT:
> +	case ISL28022_REG_INT:
> +	case ISL28022_REG_AUX:
> +		return true;
> +	}
> +	return true;
> +}
> +
> +static const struct regmap_config isl28022_regmap_config = {
> +	.reg_bits = 8,
> +	.val_bits = 16,
> +	.max_register = ISL28022_REG_MAX,
> +	.writeable_reg = isl28022_is_writeable_reg,
> +	.volatile_reg = isl28022_is_volatile_reg,
> +	.val_format_endian = REGMAP_ENDIAN_BIG,
> +	.cache_type = REGCACHE_RBTREE,
> +	.use_single_read = true,
> +	.use_single_write = true,
> +};
> +
> +static const struct i2c_device_id isl28022_ids[] = {
> +	{ "isl28022", 0},
> +	{ /* LIST END */ }
> +};
> +MODULE_DEVICE_TABLE(i2c, isl28022_ids);
> +
> +static int isl28022_config(struct device *dev)
> +{
> +	struct isl28022_data *data = dev_get_drvdata(dev);
> +
> +	if (!dev || !data)
> +		return -EINVAL;

How would this ever happen ?

> +				;
> +	data->config = (ISL28022_MODE_CONT_SB << ISL28022_MODE_SHIFT) |
> +			(ISL28022_BRNG_60 << ISL28022_BRNG_SHIFT);
> +
> +	switch (data->gain) {
> +	case 1:
> +		data->config |= (ISL28022_PG_40 << ISL28022_PG_SHIFT);
> +		break;
> +	case 2:
> +		data->config |= (ISL28022_PG_80 << ISL28022_PG_SHIFT);
> +		break;
> +	case 4:
> +		data->config |= (ISL28022_PG_160 << ISL28022_PG_SHIFT);
> +		break;
> +	default:
> +		data->config |= (ISL28022_PG_320 << ISL28022_PG_SHIFT);
> +		data->gain = 8;
> +		break;
> +	}
> +
> +	data->calib = data->shunt ? 0x8000 / data->gain : 0;
> +
> +	switch (data->average) {
> +	case 128:
> +		data->config |= (ISL28022_ADC_15_128 << ISL28022_SADC_SHIFT) |
> +				(ISL28022_ADC_15_128 << ISL28022_BADC_SHIFT);
> +		break;
> +	case 64:
> +		data->config |= (ISL28022_ADC_15_64 << ISL28022_SADC_SHIFT) |
> +				(ISL28022_ADC_15_64 << ISL28022_BADC_SHIFT);
> +		break;
> +	case 32:
> +		data->config |= (ISL28022_ADC_15_32 << ISL28022_SADC_SHIFT) |
> +				(ISL28022_ADC_15_32 << ISL28022_BADC_SHIFT);
> +		break;
> +	case 16:
> +		data->config |= (ISL28022_ADC_15_16 << ISL28022_SADC_SHIFT) |
> +				(ISL28022_ADC_15_16 << ISL28022_BADC_SHIFT);
> +		break;
> +	case 8:
> +		data->config |= (ISL28022_ADC_15_8 << ISL28022_SADC_SHIFT) |
> +				(ISL28022_ADC_15_8 << ISL28022_BADC_SHIFT);
> +		break;
> +	case 4:
> +		data->config |= (ISL28022_ADC_15_4 << ISL28022_SADC_SHIFT) |
> +				(ISL28022_ADC_15_4 << ISL28022_BADC_SHIFT);
> +		break;
> +	case 2:
> +		data->config |= (ISL28022_ADC_15_2 << ISL28022_SADC_SHIFT) |
> +				(ISL28022_ADC_15_2 << ISL28022_BADC_SHIFT);
> +		break;
> +	case 1:
> +		data->config |= (ISL28022_ADC_15_1 << ISL28022_SADC_SHIFT) |
> +				(ISL28022_ADC_15_1 << ISL28022_BADC_SHIFT);
> +		break;
> +	default:
> +		data->config |= (ISL28022_ADC_15 << ISL28022_SADC_SHIFT) |
> +				(ISL28022_ADC_15 << ISL28022_BADC_SHIFT);
> +		data->average = 0;
> +		break;
> +	}
> +
> +	regmap_write(data->regmap, ISL28022_REG_CONFIG, data->config);
> +	regmap_write(data->regmap, ISL28022_REG_CALIB, data->calib);

Error checking needed.

> +
> +	return 0;
> +}
> +
> +static int isl28022_probe(struct i2c_client *client)
> +{
> +	struct device *dev = &client->dev;
> +	struct device *hwmon_dev;
> +	struct isl28022_data *data;
> +	int status;
> +
> +	if (!i2c_check_functionality(client->adapter,
> +				     I2C_FUNC_SMBUS_BYTE_DATA |
> +				     I2C_FUNC_SMBUS_WORD_DATA))
> +		return -EIO;

This is not an IO error. Return -ENODEV as most other drivers do.

> +
> +	data = devm_kzalloc(dev, sizeof(struct isl28022_data), GFP_KERNEL);
> +	if (!data)
> +		return -ENOMEM;
> +
> +	data->client = client;
> +
> +	of_property_read_u32(dev->of_node, "shunt-resistor-micro-ohms", &data->shunt);
> +	of_property_read_u32(dev->of_node, "shunt-gain", &data->gain);
> +	of_property_read_u32(dev->of_node, "average", &data->average);

Check for errors and provide defaults if properties are not set.
Also please use device_property_read_u32() to enable use from non-devicetree
systems.

> +
> +	data->regmap = devm_regmap_init_i2c(client, &isl28022_regmap_config);
> +	if (IS_ERR(data->regmap))
> +		return PTR_ERR(data->regmap);
> +
> +	hwmon_dev = devm_hwmon_device_register_with_info(dev, "isl28022_hwmon",
> +							 data, data->shunt ?
> +							 &isl28022_chip_info_shunt :
> +							 &isl28022_chip_info, NULL);
> +	if (IS_ERR(hwmon_dev))
> +		return PTR_ERR(hwmon_dev);
> +
> +	status = isl28022_config(hwmon_dev);
> +	if (status)
> +		return status;

That has to happen before the call to devm_hwmon_device_register_with_info()
to avoid race conditions.

> +
> +	dev_info(dev, "%s: sensor '%s'\n", dev_name(hwmon_dev), client->name);
> +
> +	return 0;
> +}
> +
> +static const struct of_device_id __maybe_unused isl28022_of_match[] = {
> +	{ .compatible = "renesas,isl28022"},
> +	{ /* LIST END */ }
> +};
> +MODULE_DEVICE_TABLE(of, isl28022_of_match);
> +
> +static struct i2c_driver isl28022_driver = {
> +	.class		= I2C_CLASS_HWMON,
> +	.driver = {
> +		.name	= "isl28022",
> +		.of_match_table = of_match_ptr(isl28022_of_match),

Drop of_match_ptr()

> +	},
> +	.probe_new	= isl28022_probe,
> +	.id_table	= isl28022_ids,
> +};
> +
> +module_i2c_driver(isl28022_driver);
> +
> +MODULE_AUTHOR("Carsten Spiess <[e-mail]>");
> +MODULE_DESCRIPTION("ISL28022 driver");
> +MODULE_LICENSE("GPL");

Guenter Roeck July 26, 2023, 4:19 p.m. UTC | #2

On 7/26/23 08:22, Carsten Spieß wrote:
> Driver for Renesas ISL28022 power monitor with I2C interface.
> The device monitors voltage, current via shunt resistor
> and calculated power.
> 
> Signed-off-by: Carsten Spieß <[e-mail]>

Please provide a register dump (using i2cdump) for this chip. I strongly suspect
that the conversions will result in overflows and that they are not always correct.
I'll want to write unit test code before accepting the driver.

Guenter

> ---
>   Documentation/hwmon/index.rst    |   1 +
>   Documentation/hwmon/isl28022.rst |  56 +++++
>   MAINTAINERS                      |   7 +
>   drivers/hwmon/Kconfig            |  11 +
>   drivers/hwmon/Makefile           |   1 +
>   drivers/hwmon/isl28022.c         | 405 +++++++++++++++++++++++++++++++
>   6 files changed, 481 insertions(+)
>   create mode 100644 Documentation/hwmon/isl28022.rst
>   create mode 100644 drivers/hwmon/isl28022.c
> 
> diff --git a/Documentation/hwmon/index.rst b/Documentation/hwmon/index.rst
> index d11924667f76..c9548fc5c40e 100644
> --- a/Documentation/hwmon/index.rst
> +++ b/Documentation/hwmon/index.rst
> @@ -90,6 +90,7 @@ Hardware Monitoring Kernel Drivers
>      ir35221
>      ir38064
>      ir36021
> +   isl28022
>      isl68137
>      it87
>      jc42
> diff --git a/Documentation/hwmon/isl28022.rst b/Documentation/hwmon/isl28022.rst
> new file mode 100644
> index 000000000000..84c27ddcd33e
> --- /dev/null
> +++ b/Documentation/hwmon/isl28022.rst
> @@ -0,0 +1,56 @@
> +.. SPDX-License-Identifier: GPL-2.0-or-later
> +
> +Kernel driver isl28022
> +======================
> +
> +Supported chips:
> +
> +  * Renesas ISL28022
> +
> +    Prefix: 'isl28022'
> +
> +    Addresses scanned: none
> +
> +    Datasheet: Publicly available at the Renesas website
> +
> +	       https://www.renesas.com/us/en/www/doc/datasheet/isl28022.pdf
> +
> +Author:
> +    Carsten Spieß <[e-mail]>
> +
> +Description
> +-----------
> +
> +The ISL28022 is a power monitor with I2C interface. The device monitors
> +voltage, current via shunt resistor and calculated power.
> +
> +Usage Notes
> +-----------
> +
> +This driver does not auto-detect devices. You will have to instantiate the
> +device explicitly. Please see Documentation/i2c/instantiating-devices.rst for
> +details.
> +
> +The shunt value in micro-ohms, shunt gain and averaging can be set
> +via device tree at compile-time.
> +Please refer to the Documentation/devicetree/bindings/hwmon/isl,isl28022.yaml
> +for bindings if the device tree is used.
> +
> +Sysfs entries
> +-------------
> +
> +The following attributes are supported. All attributes are read-only.
> +
> +======================= =======================================================
> +in0_input		shunt voltage (micro Volt)
> +in1_input		bus voltage (milli Volt)
> +
> +curr1_input		current (milli Ampere)
> +power1_input		power (micro Watt)
> +
> +note			current and power attributes are supported only when
> +			shunt value is configured via device tree
> +
> +			shunt voltage is in micro Volt, not milli Volt,
> +			to get useful values
> +======================= =======================================================
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 7abb5710e1bb..c61aa688cd11 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -11072,6 +11072,13 @@ S:	Maintained
>   F:	Documentation/filesystems/isofs.rst
>   F:	fs/isofs/
>   
> +ISL28022 HARDWARE MONITORING DRIVER
> +M:	Carsten Spieß <[e-mail]>
> +L:	[e-mail]
> +S:	Maintained
> +F:	Documentation/hwmon/isl28022.rst
> +F:	drivers/hwmon/isl28022.c
> +
>   IT87 HARDWARE MONITORING DRIVER
>   M:	Jean Delvare <[e-mail]>
>   L:	[e-mail]
> diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
> index 2913299c2c9e..3049c519e6ac 100644
> --- a/drivers/hwmon/Kconfig
> +++ b/drivers/hwmon/Kconfig
> @@ -800,6 +800,17 @@ config SENSORS_CORETEMP
>   	  sensor inside your CPU. Most of the family 6 CPUs
>   	  are supported. Check Documentation/hwmon/coretemp.rst for details.
>   
> +config SENSORS_ISL28022
> +	tristate "Renesas ISL28022"
> +	depends on I2C
> +	select REGMAP_I2C
> +	help
> +	  If you say yes here you get support for ISL28022 power monitor.
> +	  Check Documentation/hwmon/isl28022.rst for details.
> +
> +	  This driver can also be built as a module. If so, the module
> +	  will be called isl28022.
> +
>   config SENSORS_IT87
>   	tristate "ITE IT87xx and compatibles"
>   	depends on !PPC
> diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile
> index ff6bfd109c72..4046fed7f48d 100644
> --- a/drivers/hwmon/Makefile
> +++ b/drivers/hwmon/Makefile
> @@ -98,6 +98,7 @@ obj-$(CONFIG_SENSORS_INA2XX)	+= ina2xx.o
>   obj-$(CONFIG_SENSORS_INA238)	+= ina238.o
>   obj-$(CONFIG_SENSORS_INA3221)	+= ina3221.o
>   obj-$(CONFIG_SENSORS_INTEL_M10_BMC_HWMON) += intel-m10-bmc-hwmon.o
> +obj-$(CONFIG_SENSORS_ISL28022)	+= isl28022.o
>   obj-$(CONFIG_SENSORS_IT87)	+= it87.o
>   obj-$(CONFIG_SENSORS_JC42)	+= jc42.o
>   obj-$(CONFIG_SENSORS_K8TEMP)	+= k8temp.o
> diff --git a/drivers/hwmon/isl28022.c b/drivers/hwmon/isl28022.c
> new file mode 100644
> index 000000000000..67cf47c31600
> --- /dev/null
> +++ b/drivers/hwmon/isl28022.c
> @@ -0,0 +1,405 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * isl28022.c - driver for Renesas ISL28022 power monitor chip
> + *	 monitoring
> + * Copyright (c) 2023 Carsten Spieß <[e-mail]>
> + */
> +
> +#include <linux/module.h>
> +#include <linux/init.h>
> +#include <linux/slab.h>
> +#include <linux/i2c.h>
> +#include <linux/hwmon.h>
> +#include <linux/hwmon-sysfs.h>
> +#include <linux/err.h>
> +#include <linux/of_device.h>
> +#include <linux/of.h>
> +#include <linux/regmap.h>
> +#include <linux/util_macros.h>
> +#include <linux/regulator/consumer.h>
> +
> +/* ISL28022 registers */
> +#define ISL28022_REG_CONFIG	0x00
> +#define ISL28022_REG_SHUNT	0x01
> +#define ISL28022_REG_BUS	0x02
> +#define ISL28022_REG_POWER	0x03
> +#define ISL28022_REG_CURRENT	0x04
> +#define ISL28022_REG_CALIB	0x05
> +#define ISL28022_REG_SHUNT_THR	0x06
> +#define ISL28022_REG_BUS_THR	0x07
> +#define ISL28022_REG_INT	0x08
> +#define ISL28022_REG_AUX	0x09
> +#define ISL28022_REG_MAX	ISL28022_REG_AUX
> +
> +/* ISL28022 config flags */
> +/* mode flags */
> +#define ISL28022_MODE_SHIFT	0
> +#define ISL28022_MODE_MASK	0x0007
> +
> +#define ISL28022_MODE_PWR_DOWN	0x0
> +#define ISL28022_MODE_TRG_S	0x1
> +#define ISL28022_MODE_TRG_B	0x2
> +#define ISL28022_MODE_TRG_SB	0x3
> +#define ISL28022_MODE_ADC_OFF	0x4
> +#define ISL28022_MODE_CONT_S	0x5
> +#define ISL28022_MODE_CONT_B	0x6
> +#define ISL28022_MODE_CONT_SB	0x7
> +
> +/* shunt ADC settings */
> +#define ISL28022_SADC_SHIFT	3
> +#define ISL28022_SADC_MASK	0x0078
> +
> +#define ISL28022_BADC_SHIFT	7
> +#define ISL28022_BADC_MASK	0x0780
> +
> +#define ISL28022_ADC_12		0x0	/* 12 bit ADC */
> +#define ISL28022_ADC_13		0x1	/* 13 bit ADC */
> +#define ISL28022_ADC_14		0x2	/* 14 bit ADC */
> +#define ISL28022_ADC_15		0x3	/* 15 bit ADC */
> +#define ISL28022_ADC_15_1	0x8	/* 15 bit ADC, 1 sample */
> +#define ISL28022_ADC_15_2	0x9	/* 15 bit ADC, 2 samples */
> +#define ISL28022_ADC_15_4	0xA	/* 15 bit ADC, 4 samples */
> +#define ISL28022_ADC_15_8	0xB	/* 15 bit ADC, 8 samples */
> +#define ISL28022_ADC_15_16	0xC	/* 15 bit ADC, 16 samples */
> +#define ISL28022_ADC_15_32	0xD	/* 15 bit ADC, 32 samples */
> +#define ISL28022_ADC_15_64	0xE	/* 15 bit ADC, 64 samples */
> +#define ISL28022_ADC_15_128	0xF	/* 15 bit ADC, 128 samples */
> +
> +/* shunt voltage range */
> +#define ISL28022_PG_SHIFT	11
> +#define ISL28022_PG_MASK	0x1800
> +
> +#define ISL28022_PG_40		0x0	/* +/-40 mV */
> +#define ISL28022_PG_80		0x1	/* +/-80 mV */
> +#define ISL28022_PG_160		0x2	/* +/-160 mV */
> +#define ISL28022_PG_320		0x3	/* +/-3200 mV */
> +
> +/* bus voltage range */
> +#define ISL28022_BRNG_SHIFT	13
> +#define ISL28022_BRNG_MASK	0x6000
> +
> +#define ISL28022_BRNG_16	0x0	/* 16 V */
> +#define ISL28022_BRNG_32	0x1	/* 32 V */
> +#define ISL28022_BRNG_60	0x3	/* 60 V */
> +
> +/* reset */
> +#define ISL28022_RESET		0x8000
> +
> +struct isl28022_data {
> +	struct i2c_client	*client;
> +	struct regmap		*regmap;
> +	u32			shunt;
> +	u32			gain;
> +	u32			average;
> +	u16			config;
> +	u16			calib;
> +};
> +
> +static int isl28022_read(struct device *dev, enum hwmon_sensor_types type,
> +			 u32 attr, int channel, long *val)
> +{
> +	struct isl28022_data *data = dev_get_drvdata(dev);
> +	unsigned int regval;
> +	int err;
> +
> +	switch (type) {
> +	case hwmon_in:
> +		switch (attr) {
> +		case hwmon_in_input:
> +			err = regmap_read(data->regmap,
> +					  ISL28022_REG_SHUNT + channel, &regval);
> +			if (err < 0)
> +				return err;
> +			*val = (channel == 0) ?
> +					(long)((s16)((u16)regval)) * 10 :
> +					(long)(((u16)regval) & 0xFFFC);
> +			break;
> +		default:
> +			return -EINVAL;
> +		}
> +		break;
> +	case hwmon_curr:
> +		switch (attr) {
> +		case hwmon_curr_input:
> +			err = regmap_read(data->regmap,
> +					  ISL28022_REG_CURRENT, &regval);
> +			if (err < 0)
> +				return err;
> +			if (!data->shunt)
> +				return -EINVAL;
> +			*val = ((long)regval * 10000L * (long)data->gain) /
> +				(long)(8 * data->shunt);
> +			break;
> +		default:
> +			return -EINVAL;
> +		}
> +		break;
> +	case hwmon_power:
> +		switch (attr) {
> +		case hwmon_power_input:
> +			err = regmap_read(data->regmap,
> +					  ISL28022_REG_POWER, &regval);
> +			if (err < 0)
> +				return err;
> +			if (!data->shunt)
> +				return -EINVAL;
> +			*val = ((long)regval * 409600000L * (long)data->gain) /
> +				(long)(8 * data->shunt);
> +			break;
> +		default:
> +			return -EINVAL;
> +		}
> +		break;
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	return 0;
> +}
> +
> +static umode_t isl28022_is_visible(const void *data, enum hwmon_sensor_types type,
> +				   u32 attr, int channel)
> +{
> +	switch (type) {
> +	case hwmon_in:
> +		switch (attr) {
> +		case hwmon_in_input:
> +			return 0444;
> +		}
> +		break;
> +	case hwmon_curr:
> +		switch (attr) {
> +		case hwmon_curr_input:
> +			return 0444;
> +		}
> +		break;
> +	case hwmon_power:
> +		switch (attr) {
> +		case hwmon_power_input:
> +			return 0444;
> +		}
> +		break;
> +	default:
> +		break;
> +	}
> +	return 0;
> +}
> +
> +static const struct hwmon_channel_info *isl28022_info[] = {
> +	HWMON_CHANNEL_INFO(in,
> +			   HWMON_I_INPUT,	/* channel 0: shunt voltage (µV) */
> +			   HWMON_I_INPUT),	/* channel 1: bus voltage (mV) */
> +	NULL
> +};
> +
> +static const struct hwmon_channel_info *isl28022_info_shunt[] = {
> +	HWMON_CHANNEL_INFO(in,
> +			   HWMON_I_INPUT,	/* channel 0: shunt voltage (µV) */
> +			   HWMON_I_INPUT),	/* channel 1: bus voltage (mV) */
> +	HWMON_CHANNEL_INFO(curr,
> +			   HWMON_C_INPUT), /* channel 1: current (mA) */
> +	HWMON_CHANNEL_INFO(power,
> +			   HWMON_P_INPUT),	/* channel 1: power (µW) */
> +	NULL
> +};
> +
> +static const struct hwmon_ops isl28022_hwmon_ops = {
> +	.is_visible = isl28022_is_visible,
> +	.read = isl28022_read,
> +};
> +
> +static const struct hwmon_chip_info isl28022_chip_info = {
> +	.ops = &isl28022_hwmon_ops,
> +	.info = isl28022_info,
> +};
> +
> +static const struct hwmon_chip_info isl28022_chip_info_shunt = {
> +	.ops = &isl28022_hwmon_ops,
> +	.info = isl28022_info_shunt,
> +};
> +
> +static bool isl28022_is_writeable_reg(struct device *dev, unsigned int reg)
> +{
> +	switch (reg) {
> +	case ISL28022_REG_CONFIG:
> +	case ISL28022_REG_CALIB:
> +	case ISL28022_REG_SHUNT_THR:
> +	case ISL28022_REG_BUS_THR:
> +	case ISL28022_REG_INT:
> +	case ISL28022_REG_AUX:
> +		return true;
> +	}
> +
> +	return false;
> +}
> +
> +static bool isl28022_is_volatile_reg(struct device *dev, unsigned int reg)
> +{
> +	switch (reg) {
> +	case ISL28022_REG_CONFIG:
> +	case ISL28022_REG_SHUNT:
> +	case ISL28022_REG_BUS:
> +	case ISL28022_REG_POWER:
> +	case ISL28022_REG_CURRENT:
> +	case ISL28022_REG_INT:
> +	case ISL28022_REG_AUX:
> +		return true;
> +	}
> +	return true;
> +}
> +
> +static const struct regmap_config isl28022_regmap_config = {
> +	.reg_bits = 8,
> +	.val_bits = 16,
> +	.max_register = ISL28022_REG_MAX,
> +	.writeable_reg = isl28022_is_writeable_reg,
> +	.volatile_reg = isl28022_is_volatile_reg,
> +	.val_format_endian = REGMAP_ENDIAN_BIG,
> +	.cache_type = REGCACHE_RBTREE,
> +	.use_single_read = true,
> +	.use_single_write = true,
> +};
> +
> +static const struct i2c_device_id isl28022_ids[] = {
> +	{ "isl28022", 0},
> +	{ /* LIST END */ }
> +};
> +MODULE_DEVICE_TABLE(i2c, isl28022_ids);
> +
> +static int isl28022_config(struct device *dev)
> +{
> +	struct isl28022_data *data = dev_get_drvdata(dev);
> +
> +	if (!dev || !data)
> +		return -EINVAL;
> +				;
> +	data->config = (ISL28022_MODE_CONT_SB << ISL28022_MODE_SHIFT) |
> +			(ISL28022_BRNG_60 << ISL28022_BRNG_SHIFT);
> +
> +	switch (data->gain) {
> +	case 1:
> +		data->config |= (ISL28022_PG_40 << ISL28022_PG_SHIFT);
> +		break;
> +	case 2:
> +		data->config |= (ISL28022_PG_80 << ISL28022_PG_SHIFT);
> +		break;
> +	case 4:
> +		data->config |= (ISL28022_PG_160 << ISL28022_PG_SHIFT);
> +		break;
> +	default:
> +		data->config |= (ISL28022_PG_320 << ISL28022_PG_SHIFT);
> +		data->gain = 8;
> +		break;
> +	}
> +
> +	data->calib = data->shunt ? 0x8000 / data->gain : 0;
> +
> +	switch (data->average) {
> +	case 128:
> +		data->config |= (ISL28022_ADC_15_128 << ISL28022_SADC_SHIFT) |
> +				(ISL28022_ADC_15_128 << ISL28022_BADC_SHIFT);
> +		break;
> +	case 64:
> +		data->config |= (ISL28022_ADC_15_64 << ISL28022_SADC_SHIFT) |
> +				(ISL28022_ADC_15_64 << ISL28022_BADC_SHIFT);
> +		break;
> +	case 32:
> +		data->config |= (ISL28022_ADC_15_32 << ISL28022_SADC_SHIFT) |
> +				(ISL28022_ADC_15_32 << ISL28022_BADC_SHIFT);
> +		break;
> +	case 16:
> +		data->config |= (ISL28022_ADC_15_16 << ISL28022_SADC_SHIFT) |
> +				(ISL28022_ADC_15_16 << ISL28022_BADC_SHIFT);
> +		break;
> +	case 8:
> +		data->config |= (ISL28022_ADC_15_8 << ISL28022_SADC_SHIFT) |
> +				(ISL28022_ADC_15_8 << ISL28022_BADC_SHIFT);
> +		break;
> +	case 4:
> +		data->config |= (ISL28022_ADC_15_4 << ISL28022_SADC_SHIFT) |
> +				(ISL28022_ADC_15_4 << ISL28022_BADC_SHIFT);
> +		break;
> +	case 2:
> +		data->config |= (ISL28022_ADC_15_2 << ISL28022_SADC_SHIFT) |
> +				(ISL28022_ADC_15_2 << ISL28022_BADC_SHIFT);
> +		break;
> +	case 1:
> +		data->config |= (ISL28022_ADC_15_1 << ISL28022_SADC_SHIFT) |
> +				(ISL28022_ADC_15_1 << ISL28022_BADC_SHIFT);
> +		break;
> +	default:
> +		data->config |= (ISL28022_ADC_15 << ISL28022_SADC_SHIFT) |
> +				(ISL28022_ADC_15 << ISL28022_BADC_SHIFT);
> +		data->average = 0;
> +		break;
> +	}
> +
> +	regmap_write(data->regmap, ISL28022_REG_CONFIG, data->config);
> +	regmap_write(data->regmap, ISL28022_REG_CALIB, data->calib);
> +
> +	return 0;
> +}
> +
> +static int isl28022_probe(struct i2c_client *client)
> +{
> +	struct device *dev = &client->dev;
> +	struct device *hwmon_dev;
> +	struct isl28022_data *data;
> +	int status;
> +
> +	if (!i2c_check_functionality(client->adapter,
> +				     I2C_FUNC_SMBUS_BYTE_DATA |
> +				     I2C_FUNC_SMBUS_WORD_DATA))
> +		return -EIO;
> +
> +	data = devm_kzalloc(dev, sizeof(struct isl28022_data), GFP_KERNEL);
> +	if (!data)
> +		return -ENOMEM;
> +
> +	data->client = client;
> +
> +	of_property_read_u32(dev->of_node, "shunt-resistor-micro-ohms", &data->shunt);
> +	of_property_read_u32(dev->of_node, "shunt-gain", &data->gain);
> +	of_property_read_u32(dev->of_node, "average", &data->average);
> +
> +	data->regmap = devm_regmap_init_i2c(client, &isl28022_regmap_config);
> +	if (IS_ERR(data->regmap))
> +		return PTR_ERR(data->regmap);
> +
> +	hwmon_dev = devm_hwmon_device_register_with_info(dev, "isl28022_hwmon",
> +							 data, data->shunt ?
> +							 &isl28022_chip_info_shunt :
> +							 &isl28022_chip_info, NULL);
> +	if (IS_ERR(hwmon_dev))
> +		return PTR_ERR(hwmon_dev);
> +
> +	status = isl28022_config(hwmon_dev);
> +	if (status)
> +		return status;
> +
> +	dev_info(dev, "%s: sensor '%s'\n", dev_name(hwmon_dev), client->name);
> +
> +	return 0;
> +}
> +
> +static const struct of_device_id __maybe_unused isl28022_of_match[] = {
> +	{ .compatible = "renesas,isl28022"},
> +	{ /* LIST END */ }
> +};
> +MODULE_DEVICE_TABLE(of, isl28022_of_match);
> +
> +static struct i2c_driver isl28022_driver = {
> +	.class		= I2C_CLASS_HWMON,
> +	.driver = {
> +		.name	= "isl28022",
> +		.of_match_table = of_match_ptr(isl28022_of_match),
> +	},
> +	.probe_new	= isl28022_probe,
> +	.id_table	= isl28022_ids,
> +};
> +
> +module_i2c_driver(isl28022_driver);
> +
> +MODULE_AUTHOR("Carsten Spiess <[e-mail]>");
> +MODULE_DESCRIPTION("ISL28022 driver");
> +MODULE_LICENSE("GPL");

Randy Dunlap July 26, 2023, 8:16 p.m. UTC | #3

On 7/26/23 08:22, Carsten Spieß wrote:
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 7abb5710e1bb..c61aa688cd11 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -11072,6 +11072,13 @@ S:	Maintained
>  F:	Documentation/filesystems/isofs.rst
>  F:	fs/isofs/
>  

New entry is not quite in the correct place for alphabetical order.
It should be just before the ISOFS entry, not just after it.

> +ISL28022 HARDWARE MONITORING DRIVER
> +M:	Carsten Spieß <[e-mail]>
> +L:	[e-mail]
> +S:	Maintained
> +F:	Documentation/hwmon/isl28022.rst
> +F:	drivers/hwmon/isl28022.c
> +
>  IT87 HARDWARE MONITORING DRIVER
>  M:	Jean Delvare <[e-mail]>
>  L:	[e-mail]

Carsten Spieß July 27, 2023, 7:35 a.m. UTC | #4

On 7/26/23 18:14, Gueter Roeck wrote:
> On 7/26/23 08:22, Carsten Spieß wrote:
> > +The shunt value in micro-ohms, shunt gain and averaging can be set
> > +via device tree at compile-time.  
> 
> At _compile-time_ ?
How to explain better that it isn't set at runtime?
Other drivers use the same term.

> I'd argue that shunt voltage is all but useless, but if you want to have it supported
> it _has_ to be in mV.
That's a problem.

In my case the ER-6P has a 8 milli Ohm (or 8000 micro Ohm) shunt and a powersupply with 
max. 2.5 A. This gives a max shunt voltage of 20 mV at 2.5 A.
The device normaly consumes between 200 and 500 mA. (typ ~250 mA).
This results in shunt voltage of 1.6 to 4.0 mV (typ ~2mV).
Having no fractions will make it useless. 

Unfortunately there is no possibility to give a scaling factor.
Or returning float values (i know, this can't and shouldn't be changed)

> Why not support limit attributes ?
Due to limited time, left for later enhancement.


> > +#define ISL28022_REG_SHUNT	0x01
> > +#define ISL28022_REG_BUS	0x02


> > +	switch (type) {
> > +	case hwmon_in:
> > +		switch (attr) {
> > +		case hwmon_in_input:
> > +			err = regmap_read(data->regmap,
> > +					  ISL28022_REG_SHUNT + channel, &regval);  
> 
> That never reads REG_BUS.
Hmm, 
channel 0: ISL28022_REG_SHUNT + 0 == 0x01
channel 1: ISL28022_REG_SHUNT + 1 == 0x02 == ISL28022_REG_BUS
or do i miss something?

> > +			if (err < 0)
> > +				return err;
> > +			*val = (channel == 0) ?
> > +					(long)((s16)((u16)regval)) * 10 :
> > +					(long)(((u16)regval) & 0xFFFC);  
> 
> I don't think the sign extensions are correct based on the datasheet.
> This will have to use sign_extend.
From my understading (see table 11 on page 16 of the ISL28022 datasheet)
shunt value is already sign extended, (D15-D12 is sign)
bus value (table 12 on page 16) is unsigned.

> > +			err = regmap_read(data->regmap,
> > +					  ISL28022_REG_CURRENT, &regval);
> > +			if (err < 0)
> > +				return err;
> > +			if (!data->shunt)
> > +				return -EINVAL;  
> 
> Getting an error message each time the "sensors" command is executed ?
> Unacceptable.
o.k., will change to set *val = 0;

> > +			err = regmap_read(data->regmap,
> > +					  ISL28022_REG_POWER, &regval);
> > +			if (err < 0)
> > +				return err;
> > +			if (!data->shunt)
> > +				return -EINVAL;  
> 
> Unacceptable.
o.k., as above

> > +			*val = ((long)regval * 409600000L * (long)data->gain) /
> > +				(long)(8 * data->shunt);  
> 
> I don't think this was checked for overflows.
Yes, i must first divide then multiply.
I have to think about how to keep accuracy on high shunt resistor values.

> > +static int isl28022_config(struct device *dev)
> > +{
> > +	struct isl28022_data *data = dev_get_drvdata(dev);
> > +
> > +	if (!dev || !data)
> > +		return -EINVAL;  
> 
> How would this ever happen ?
Shouldn't, but i'm carefully (i had it once during development due to an error
(using dev instead of hwmon_dev) on calling this function
 
> > +	regmap_write(data->regmap, ISL28022_REG_CONFIG, data->config);
> > +	regmap_write(data->regmap, ISL28022_REG_CALIB, data->calib);  
> 
> Error checking needed.
o.k. will add.

> > +static int isl28022_probe(struct i2c_client *client)
> > +{

> > +	if (!i2c_check_functionality(client->adapter,
> > +				     I2C_FUNC_SMBUS_BYTE_DATA |
> > +				     I2C_FUNC_SMBUS_WORD_DATA))
> > +		return -EIO;  
> 
> This is not an IO error. Return -ENODEV as most other drivers do.
o.k.

> > +	of_property_read_u32(dev->of_node, "shunt-gain", &data->gain);
> > +	of_property_read_u32(dev->of_node, "average", &data->average);  
> 
> Check for errors and provide defaults if properties are not set.
o.k.

> Also please use device_property_read_u32() to enable use from non-devicetree
> systems.
o.k. Never used this, have to look for an example on using it.
Many (old) drivers are using the of_property_* functions, is it intended to replace it.
What about backporting this driver to e.g. 5.15, will it affect it?

> > +	status = isl28022_config(hwmon_dev);
> > +	if (status)
> > +		return status;  
> 
> That has to happen before the call to devm_hwmon_device_register_with_info()
> to avoid race conditions.
o.k.

> > +static struct i2c_driver isl28022_driver = {
> > +	.class		= I2C_CLASS_HWMON,
> > +	.driver = {
> > +		.name	= "isl28022",
> > +		.of_match_table = of_match_ptr(isl28022_of_match),  
> 
> Drop of_match_ptr()
Most drivers have this, why drop?

Regards, Carsten

Carsten Spieß July 27, 2023, 7:35 a.m. UTC | #5

On 7/26/23 18:19, Guenter Roeck wrote:
> Please provide a register dump (using i2cdump) for this chip. 

# i2cdump -y -r 0-9 1 0x40 w
     0,8  1,9  2,a  3,b  4,c  5,d  6,e  7,f
00: ff67 ca00 a25d c803 5006 0080 817f 00ff 
08: 0000 0000  

Please note that due to big vs.- little endian bytes are swapped,
should be read as:
00: 67ff 00ca 5da2 03c8 0650 8000 7f81 ff00
08: 0000 0000  

corresponding sensor values are about (not read at same time):
in0:           1.99 V  
in1:          23.97 V  
power1:        6.10 W  
curr1:       248.00 mA 

in0 should be read as mV

Regards, Carsten

Guenter Roeck July 27, 2023, 2:30 p.m. UTC | #6

On 7/27/23 00:35, Carsten Spieß wrote:
> 
> On 7/26/23 18:14, Gueter Roeck wrote:
>> On 7/26/23 08:22, Carsten Spieß wrote:
>>> +The shunt value in micro-ohms, shunt gain and averaging can be set
>>> +via device tree at compile-time.
>>
>> At _compile-time_ ?
> How to explain better that it isn't set at runtime?
> Other drivers use the same term.

You mean it is ok to exceed the speed limit because others do it
as well ?

[ Sorry, I have heard the "Other drivers do it" pseudo-argument too many times.
   That doesn't mean it is the right thing to do. ]

Why not just leave it off ? What value does it add ? Besides,
even "via devicetree" isn't really correct because it can also
be set via ACPI or by a platform driver when using device_ properties.
I would suggest "can be set with device properties".

> 
>> I'd argue that shunt voltage is all but useless, but if you want to have it supported
>> it _has_ to be in mV.
> That's a problem.
> 
> In my case the ER-6P has a 8 milli Ohm (or 8000 micro Ohm) shunt and a powersupply with
> max. 2.5 A. This gives a max shunt voltage of 20 mV at 2.5 A.
> The device normaly consumes between 200 and 500 mA. (typ ~250 mA).
> This results in shunt voltage of 1.6 to 4.0 mV (typ ~2mV).
> Having no fractions will make it useless.
> 
> Unfortunately there is no possibility to give a scaling factor.
> Or returning float values (i know, this can't and shouldn't be changed)
> 

Just like the ABI must not be changed. The sensors command would display your
4mV shunt voltage as 4V, which is just as useless.

In practice, the shunt voltage _is_ useless for hardware monitoring purpose
because it can be calculated from current and shunt resistor value.
I'd say if you really want it, provide it as debugfs attribute. As hwmon
attribute it has to be in mV.

>> Why not support limit attributes ?
> Due to limited time, left for later enhancement.
> 
> 
>>> +#define ISL28022_REG_SHUNT	0x01
>>> +#define ISL28022_REG_BUS	0x02
> 
> 
>>> +	switch (type) {
>>> +	case hwmon_in:
>>> +		switch (attr) {
>>> +		case hwmon_in_input:
>>> +			err = regmap_read(data->regmap,
>>> +					  ISL28022_REG_SHUNT + channel, &regval);
>>
>> That never reads REG_BUS.
> Hmm,
> channel 0: ISL28022_REG_SHUNT + 0 == 0x01
> channel 1: ISL28022_REG_SHUNT + 1 == 0x02 == ISL28022_REG_BUS
> or do i miss something?
> 

No, I missed the "+ channel".

>>> +			if (err < 0)
>>> +				return err;
>>> +			*val = (channel == 0) ?
>>> +					(long)((s16)((u16)regval)) * 10 :
>>> +					(long)(((u16)regval) & 0xFFFC);
>>
>> I don't think the sign extensions are correct based on the datasheet.
>> This will have to use sign_extend.
>  From my understading (see table 11 on page 16 of the ISL28022 datasheet)
> shunt value is already sign extended, (D15-D12 is sign)
> bus value (table 12 on page 16) is unsigned.
> 

Not really. For the shunt voltage, 0xf000 has different meanings depending on scale
and range settings. LSB for bus voltage is 4 mV and starts at bit 2 or 3 depending
on BRNG. The above just happens to be correct if BRNG = 10 OR 11 per datasheet.
If that is intentional, it needs to get a comment.

>>> +			err = regmap_read(data->regmap,
>>> +					  ISL28022_REG_CURRENT, &regval);
>>> +			if (err < 0)
>>> +				return err;
>>> +			if (!data->shunt)
>>> +				return -EINVAL;
>>
>> Getting an error message each time the "sensors" command is executed ?
>> Unacceptable.
> o.k., will change to set *val = 0;
> 
Still unacceptable.

>>> +			err = regmap_read(data->regmap,
>>> +					  ISL28022_REG_POWER, &regval);
>>> +			if (err < 0)
>>> +				return err;
>>> +			if (!data->shunt)
>>> +				return -EINVAL;
>>
>> Unacceptable.
> o.k., as above
> 
>>> +			*val = ((long)regval * 409600000L * (long)data->gain) /
>>> +				(long)(8 * data->shunt);
>>
>> I don't think this was checked for overflows.
> Yes, i must first divide then multiply.
> I have to think about how to keep accuracy on high shunt resistor values.
> 
>>> +static int isl28022_config(struct device *dev)
>>> +{
>>> +	struct isl28022_data *data = dev_get_drvdata(dev);
>>> +
>>> +	if (!dev || !data)
>>> +		return -EINVAL;
>>
>> How would this ever happen ?
> Shouldn't, but i'm carefully (i had it once during development due to an error
> (using dev instead of hwmon_dev) on calling this function
>   

Parameter checks are only acceptable on API functions. This is not an API function.
Local functions are expected to be consistent. If this function is called with
a bad argument, that needs to be fixed during development.

>>> +	regmap_write(data->regmap, ISL28022_REG_CONFIG, data->config);
>>> +	regmap_write(data->regmap, ISL28022_REG_CALIB, data->calib);
>>
>> Error checking needed.
> o.k. will add.
> 
>>> +static int isl28022_probe(struct i2c_client *client)
>>> +{
> 
>>> +	if (!i2c_check_functionality(client->adapter,
>>> +				     I2C_FUNC_SMBUS_BYTE_DATA |
>>> +				     I2C_FUNC_SMBUS_WORD_DATA))
>>> +		return -EIO;
>>
>> This is not an IO error. Return -ENODEV as most other drivers do.
> o.k.
> 
>>> +	of_property_read_u32(dev->of_node, "shunt-gain", &data->gain);
>>> +	of_property_read_u32(dev->of_node, "average", &data->average);
>>
>> Check for errors and provide defaults if properties are not set.
> o.k.
> 
>> Also please use device_property_read_u32() to enable use from non-devicetree
>> systems.
> o.k. Never used this, have to look for an example on using it.
> Many (old) drivers are using the of_property_* functions, is it intended to replace it.

This is not an old driver. It is more generic than of_ functions and
should be used in new drivers.

> What about backporting this driver to e.g. 5.15, will it affect it?
> 

Device property callback functions existed for a long time. The function
exists in v4.14.y, which is the oldest supported kernel. Either case,
lack of support in an older kernel version is not an argument for avoiding
a new API. Anyone who backports a driver to an older kernel is responsible
for handling the backport, which may include new API functions.

Specific example: Your driver will have to use the .probe callback.
That has one argument in the latest kernel. In v5.15.y, it has two arguments.
You'll have to use the .probe_new callback there. Yes, technically you could
try sneaking in the use of .probe_new in your driver, but that callback will
be removed in v6.6. So you'll _have_ to do some backport, if you want it or not.

>>> +	status = isl28022_config(hwmon_dev);
>>> +	if (status)
>>> +		return status;
>>
>> That has to happen before the call to devm_hwmon_device_register_with_info()
>> to avoid race conditions.
> o.k.
> 
>>> +static struct i2c_driver isl28022_driver = {
>>> +	.class		= I2C_CLASS_HWMON,
>>> +	.driver = {
>>> +		.name	= "isl28022",
>>> +		.of_match_table = of_match_ptr(isl28022_of_match),
>>
>> Drop of_match_ptr()
> Most drivers have this, why drop?
> 

It is needed for device_property_read_u32() to work. And, again, "other drivers
do it" is not a valid argument.

Guenter

Carsten Spieß July 27, 2023, 3:13 p.m. UTC | #7

On 7/27/23 16:30, Gueter Roeck wrote:  
> >> On 7/26/23 08:22, Carsten Spieß wrote:  
> >> At _compile-time_ ?  
> > How to explain better that it isn't set at runtime?
> I would suggest "can be set with device properties".
Yeah, i like that.

> >> I'd argue that shunt voltage is all but useless, but if you want to have it supported
> >> it _has_ to be in mV.  
> > That's a problem.
> > 
> > In my case the ER-6P has a 8 milli Ohm (or 8000 micro Ohm) shunt and a powersupply with
> > max. 2.5 A. This gives a max shunt voltage of 20 mV at 2.5 A.
> > The device normaly consumes between 200 and 500 mA. (typ ~250 mA).
> > This results in shunt voltage of 1.6 to 4.0 mV (typ ~2mV).
> > Having no fractions will make it useless.
> > 
> > Unfortunately there is no possibility to give a scaling factor.
> > Or returning float values (i know, this can't and shouldn't be changed)
> >   
> 
> Just like the ABI must not be changed. The sensors command would display your
> 4mV shunt voltage as 4V, which is just as useless.
> 
> In practice, the shunt voltage _is_ useless for hardware monitoring purpose
> because it can be calculated from current and shunt resistor value.
> I'd say if you really want it, provide it as debugfs attribute. As hwmon
> attribute it has to be in mV.
O.k. will move to debugfs.

> >> I don't think the sign extensions are correct based on the datasheet.
> >> This will have to use sign_extend.  
> >  From my understading (see table 11 on page 16 of the ISL28022 datasheet)
> > shunt value is already sign extended, (D15-D12 is sign)
> > bus value (table 12 on page 16) is unsigned.
> >   
> 
> Not really. For the shunt voltage, 0xf000 has different meanings depending on scale
> and range settings. 
Sorry, i don't agree, 0xf000 is -40.96 mV on all scale settings.

> LSB for bus voltage is 4 mV and starts at bit 2 or 3 depending
> on BRNG. The above just happens to be correct if BRNG = 10 OR 11 per datasheet.
> If that is intentional, it needs to get a comment.
Yes, will add comment.

> >> Getting an error message each time the "sensors" command is executed ?
> >> Unacceptable.  
> > o.k., will change to set *val = 0;
> >   
> Still unacceptable.
O.k. i will limit shunt-resistor-milli-ohms to a minimal value > 0 and drop check here.

> >>> +	if (!dev || !data)
> >>> +		return -EINVAL;  
> >>
> >> How would this ever happen ?  
> > Shouldn't, but i'm carefully (i had it once during development due to an error
> > (using dev instead of hwmon_dev) on calling this function
> >     
> 
> Parameter checks are only acceptable on API functions. This is not an API function.
> Local functions are expected to be consistent. If this function is called with
> a bad argument, that needs to be fixed during development.
O.k., removed.

> >>> +static struct i2c_driver isl28022_driver = {
> >>> +	.class		= I2C_CLASS_HWMON,
> >>> +	.driver = {
> >>> +		.name	= "isl28022",
> >>> +		.of_match_table = of_match_ptr(isl28022_of_match),  
> >>
> >> Drop of_match_ptr()  
> > Most drivers have this, why drop?
> >   
> 
> It is needed for device_property_read_u32() to work. 
O.k. dropped, i wasn't familiar with device_property_read functions.

Regards Carsten

Carsten Spieß July 27, 2023, 3:28 p.m. UTC | #8

> On 7/26/23 18:19, Guenter Roeck wrote:
> I strongly suspect that the conversions will result in overflows and that they are not always correct.
> I'll want to write unit test code before accepting the driver.
I changed the conversion for current to 
				*val = ((long)regval * 1250L * (long)data->gain) /
					(long)data->shunt;

The term (1250 * gain) will be 10000 max, fits to 14 bit.
So no risk for 32bit overflow when multiply with 16bit value.

And changed the conversion for power to 
				*val = ((51200000L * ((long)data->gain)) /
					(long)data->shunt) * (long)regval;

The first term (51200000 * gain / shunt) will be larger than 16bit when 
(shunt/gain) is less than ~800. So min values for shunt are 
- 6400 µOhm for 320 mV range
- 3200 µOhm for 160 mV range
- 1600 µOhm for 80 mV range
- 800 µOhm for 40 mV range
Can i set this conditionally in the .yaml file?

Regards Carsten

Patch

diff --git a/Documentation/hwmon/index.rst b/Documentation/hwmon/index.rst
index d11924667f76..c9548fc5c40e 100644
--- a/Documentation/hwmon/index.rst
+++ b/Documentation/hwmon/index.rst
@@ -90,6 +90,7 @@  Hardware Monitoring Kernel Drivers
    ir35221
    ir38064
    ir36021
+   isl28022
    isl68137
    it87
    jc42
diff --git a/Documentation/hwmon/isl28022.rst b/Documentation/hwmon/isl28022.rst
new file mode 100644
index 000000000000..84c27ddcd33e
--- /dev/null
+++ b/Documentation/hwmon/isl28022.rst
@@ -0,0 +1,56 @@ 
+.. SPDX-License-Identifier: GPL-2.0-or-later
+
+Kernel driver isl28022
+======================
+
+Supported chips:
+
+  * Renesas ISL28022
+
+    Prefix: 'isl28022'
+
+    Addresses scanned: none
+
+    Datasheet: Publicly available at the Renesas website
+
+	       https://www.renesas.com/us/en/www/doc/datasheet/isl28022.pdf
+
+Author:
+    Carsten Spieß <[e-mail]>
+
+Description
+-----------
+
+The ISL28022 is a power monitor with I2C interface. The device monitors
+voltage, current via shunt resistor and calculated power.
+
+Usage Notes
+-----------
+
+This driver does not auto-detect devices. You will have to instantiate the
+device explicitly. Please see Documentation/i2c/instantiating-devices.rst for
+details.
+
+The shunt value in micro-ohms, shunt gain and averaging can be set
+via device tree at compile-time.
+Please refer to the Documentation/devicetree/bindings/hwmon/isl,isl28022.yaml
+for bindings if the device tree is used.
+
+Sysfs entries
+-------------
+
+The following attributes are supported. All attributes are read-only.
+
+======================= =======================================================
+in0_input		shunt voltage (micro Volt)
+in1_input		bus voltage (milli Volt)
+
+curr1_input		current (milli Ampere)
+power1_input		power (micro Watt)
+
+note			current and power attributes are supported only when
+			shunt value is configured via device tree
+
+			shunt voltage is in micro Volt, not milli Volt,
+			to get useful values
+======================= =======================================================
diff --git a/MAINTAINERS b/MAINTAINERS
index 7abb5710e1bb..c61aa688cd11 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -11072,6 +11072,13 @@  S:	Maintained
 F:	Documentation/filesystems/isofs.rst
 F:	fs/isofs/
 
+ISL28022 HARDWARE MONITORING DRIVER
+M:	Carsten Spieß <[e-mail]>
+L:	[e-mail]
+S:	Maintained
+F:	Documentation/hwmon/isl28022.rst
+F:	drivers/hwmon/isl28022.c
+
 IT87 HARDWARE MONITORING DRIVER
 M:	Jean Delvare <[e-mail]>
 L:	[e-mail]
diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
index 2913299c2c9e..3049c519e6ac 100644
--- a/drivers/hwmon/Kconfig
+++ b/drivers/hwmon/Kconfig
@@ -800,6 +800,17 @@  config SENSORS_CORETEMP
 	  sensor inside your CPU. Most of the family 6 CPUs
 	  are supported. Check Documentation/hwmon/coretemp.rst for details.
 
+config SENSORS_ISL28022
+	tristate "Renesas ISL28022"
+	depends on I2C
+	select REGMAP_I2C
+	help
+	  If you say yes here you get support for ISL28022 power monitor.
+	  Check Documentation/hwmon/isl28022.rst for details.
+
+	  This driver can also be built as a module. If so, the module
+	  will be called isl28022.
+
 config SENSORS_IT87
 	tristate "ITE IT87xx and compatibles"
 	depends on !PPC
diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile
index ff6bfd109c72..4046fed7f48d 100644
--- a/drivers/hwmon/Makefile
+++ b/drivers/hwmon/Makefile
@@ -98,6 +98,7 @@  obj-$(CONFIG_SENSORS_INA2XX)	+= ina2xx.o
 obj-$(CONFIG_SENSORS_INA238)	+= ina238.o
 obj-$(CONFIG_SENSORS_INA3221)	+= ina3221.o
 obj-$(CONFIG_SENSORS_INTEL_M10_BMC_HWMON) += intel-m10-bmc-hwmon.o
+obj-$(CONFIG_SENSORS_ISL28022)	+= isl28022.o
 obj-$(CONFIG_SENSORS_IT87)	+= it87.o
 obj-$(CONFIG_SENSORS_JC42)	+= jc42.o
 obj-$(CONFIG_SENSORS_K8TEMP)	+= k8temp.o
diff --git a/drivers/hwmon/isl28022.c b/drivers/hwmon/isl28022.c
new file mode 100644
index 000000000000..67cf47c31600
--- /dev/null
+++ b/drivers/hwmon/isl28022.c
@@ -0,0 +1,405 @@ 
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * isl28022.c - driver for Renesas ISL28022 power monitor chip
+ *	 monitoring
+ * Copyright (c) 2023 Carsten Spieß <[e-mail]>
+ */
+
+#include <linux/module.h>
+#include <linux/init.h>
+#include <linux/slab.h>
+#include <linux/i2c.h>
+#include <linux/hwmon.h>
+#include <linux/hwmon-sysfs.h>
+#include <linux/err.h>
+#include <linux/of_device.h>
+#include <linux/of.h>
+#include <linux/regmap.h>
+#include <linux/util_macros.h>
+#include <linux/regulator/consumer.h>
+
+/* ISL28022 registers */
+#define ISL28022_REG_CONFIG	0x00
+#define ISL28022_REG_SHUNT	0x01
+#define ISL28022_REG_BUS	0x02
+#define ISL28022_REG_POWER	0x03
+#define ISL28022_REG_CURRENT	0x04
+#define ISL28022_REG_CALIB	0x05
+#define ISL28022_REG_SHUNT_THR	0x06
+#define ISL28022_REG_BUS_THR	0x07
+#define ISL28022_REG_INT	0x08
+#define ISL28022_REG_AUX	0x09
+#define ISL28022_REG_MAX	ISL28022_REG_AUX
+
+/* ISL28022 config flags */
+/* mode flags */
+#define ISL28022_MODE_SHIFT	0
+#define ISL28022_MODE_MASK	0x0007
+
+#define ISL28022_MODE_PWR_DOWN	0x0
+#define ISL28022_MODE_TRG_S	0x1
+#define ISL28022_MODE_TRG_B	0x2
+#define ISL28022_MODE_TRG_SB	0x3
+#define ISL28022_MODE_ADC_OFF	0x4
+#define ISL28022_MODE_CONT_S	0x5
+#define ISL28022_MODE_CONT_B	0x6
+#define ISL28022_MODE_CONT_SB	0x7
+
+/* shunt ADC settings */
+#define ISL28022_SADC_SHIFT	3
+#define ISL28022_SADC_MASK	0x0078
+
+#define ISL28022_BADC_SHIFT	7
+#define ISL28022_BADC_MASK	0x0780
+
+#define ISL28022_ADC_12		0x0	/* 12 bit ADC */
+#define ISL28022_ADC_13		0x1	/* 13 bit ADC */
+#define ISL28022_ADC_14		0x2	/* 14 bit ADC */
+#define ISL28022_ADC_15		0x3	/* 15 bit ADC */
+#define ISL28022_ADC_15_1	0x8	/* 15 bit ADC, 1 sample */
+#define ISL28022_ADC_15_2	0x9	/* 15 bit ADC, 2 samples */
+#define ISL28022_ADC_15_4	0xA	/* 15 bit ADC, 4 samples */
+#define ISL28022_ADC_15_8	0xB	/* 15 bit ADC, 8 samples */
+#define ISL28022_ADC_15_16	0xC	/* 15 bit ADC, 16 samples */
+#define ISL28022_ADC_15_32	0xD	/* 15 bit ADC, 32 samples */
+#define ISL28022_ADC_15_64	0xE	/* 15 bit ADC, 64 samples */
+#define ISL28022_ADC_15_128	0xF	/* 15 bit ADC, 128 samples */
+
+/* shunt voltage range */
+#define ISL28022_PG_SHIFT	11
+#define ISL28022_PG_MASK	0x1800
+
+#define ISL28022_PG_40		0x0	/* +/-40 mV */
+#define ISL28022_PG_80		0x1	/* +/-80 mV */
+#define ISL28022_PG_160		0x2	/* +/-160 mV */
+#define ISL28022_PG_320		0x3	/* +/-3200 mV */
+
+/* bus voltage range */
+#define ISL28022_BRNG_SHIFT	13
+#define ISL28022_BRNG_MASK	0x6000
+
+#define ISL28022_BRNG_16	0x0	/* 16 V */
+#define ISL28022_BRNG_32	0x1	/* 32 V */
+#define ISL28022_BRNG_60	0x3	/* 60 V */
+
+/* reset */
+#define ISL28022_RESET		0x8000
+
+struct isl28022_data {
+	struct i2c_client	*client;
+	struct regmap		*regmap;
+	u32			shunt;
+	u32			gain;
+	u32			average;
+	u16			config;
+	u16			calib;
+};
+
+static int isl28022_read(struct device *dev, enum hwmon_sensor_types type,
+			 u32 attr, int channel, long *val)
+{
+	struct isl28022_data *data = dev_get_drvdata(dev);
+	unsigned int regval;
+	int err;
+
+	switch (type) {
+	case hwmon_in:
+		switch (attr) {
+		case hwmon_in_input:
+			err = regmap_read(data->regmap,
+					  ISL28022_REG_SHUNT + channel, &regval);
+			if (err < 0)
+				return err;
+			*val = (channel == 0) ?
+					(long)((s16)((u16)regval)) * 10 :
+					(long)(((u16)regval) & 0xFFFC);
+			break;
+		default:
+			return -EINVAL;
+		}
+		break;
+	case hwmon_curr:
+		switch (attr) {
+		case hwmon_curr_input:
+			err = regmap_read(data->regmap,
+					  ISL28022_REG_CURRENT, &regval);
+			if (err < 0)
+				return err;
+			if (!data->shunt)
+				return -EINVAL;
+			*val = ((long)regval * 10000L * (long)data->gain) /
+				(long)(8 * data->shunt);
+			break;
+		default:
+			return -EINVAL;
+		}
+		break;
+	case hwmon_power:
+		switch (attr) {
+		case hwmon_power_input:
+			err = regmap_read(data->regmap,
+					  ISL28022_REG_POWER, &regval);
+			if (err < 0)
+				return err;
+			if (!data->shunt)
+				return -EINVAL;
+			*val = ((long)regval * 409600000L * (long)data->gain) /
+				(long)(8 * data->shunt);
+			break;
+		default:
+			return -EINVAL;
+		}
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
+static umode_t isl28022_is_visible(const void *data, enum hwmon_sensor_types type,
+				   u32 attr, int channel)
+{
+	switch (type) {
+	case hwmon_in:
+		switch (attr) {
+		case hwmon_in_input:
+			return 0444;
+		}
+		break;
+	case hwmon_curr:
+		switch (attr) {
+		case hwmon_curr_input:
+			return 0444;
+		}
+		break;
+	case hwmon_power:
+		switch (attr) {
+		case hwmon_power_input:
+			return 0444;
+		}
+		break;
+	default:
+		break;
+	}
+	return 0;
+}
+
+static const struct hwmon_channel_info *isl28022_info[] = {
+	HWMON_CHANNEL_INFO(in,
+			   HWMON_I_INPUT,	/* channel 0: shunt voltage (µV) */
+			   HWMON_I_INPUT),	/* channel 1: bus voltage (mV) */
+	NULL
+};
+
+static const struct hwmon_channel_info *isl28022_info_shunt[] = {
+	HWMON_CHANNEL_INFO(in,
+			   HWMON_I_INPUT,	/* channel 0: shunt voltage (µV) */
+			   HWMON_I_INPUT),	/* channel 1: bus voltage (mV) */
+	HWMON_CHANNEL_INFO(curr,
+			   HWMON_C_INPUT), /* channel 1: current (mA) */
+	HWMON_CHANNEL_INFO(power,
+			   HWMON_P_INPUT),	/* channel 1: power (µW) */
+	NULL
+};
+
+static const struct hwmon_ops isl28022_hwmon_ops = {
+	.is_visible = isl28022_is_visible,
+	.read = isl28022_read,
+};
+
+static const struct hwmon_chip_info isl28022_chip_info = {
+	.ops = &isl28022_hwmon_ops,
+	.info = isl28022_info,
+};
+
+static const struct hwmon_chip_info isl28022_chip_info_shunt = {
+	.ops = &isl28022_hwmon_ops,
+	.info = isl28022_info_shunt,
+};
+
+static bool isl28022_is_writeable_reg(struct device *dev, unsigned int reg)
+{
+	switch (reg) {
+	case ISL28022_REG_CONFIG:
+	case ISL28022_REG_CALIB:
+	case ISL28022_REG_SHUNT_THR:
+	case ISL28022_REG_BUS_THR:
+	case ISL28022_REG_INT:
+	case ISL28022_REG_AUX:
+		return true;
+	}
+
+	return false;
+}
+
+static bool isl28022_is_volatile_reg(struct device *dev, unsigned int reg)
+{
+	switch (reg) {
+	case ISL28022_REG_CONFIG:
+	case ISL28022_REG_SHUNT:
+	case ISL28022_REG_BUS:
+	case ISL28022_REG_POWER:
+	case ISL28022_REG_CURRENT:
+	case ISL28022_REG_INT:
+	case ISL28022_REG_AUX:
+		return true;
+	}
+	return true;
+}
+
+static const struct regmap_config isl28022_regmap_config = {
+	.reg_bits = 8,
+	.val_bits = 16,
+	.max_register = ISL28022_REG_MAX,
+	.writeable_reg = isl28022_is_writeable_reg,
+	.volatile_reg = isl28022_is_volatile_reg,
+	.val_format_endian = REGMAP_ENDIAN_BIG,
+	.cache_type = REGCACHE_RBTREE,
+	.use_single_read = true,
+	.use_single_write = true,
+};
+
+static const struct i2c_device_id isl28022_ids[] = {
+	{ "isl28022", 0},
+	{ /* LIST END */ }
+};
+MODULE_DEVICE_TABLE(i2c, isl28022_ids);
+
+static int isl28022_config(struct device *dev)
+{
+	struct isl28022_data *data = dev_get_drvdata(dev);
+
+	if (!dev || !data)
+		return -EINVAL;
+				;
+	data->config = (ISL28022_MODE_CONT_SB << ISL28022_MODE_SHIFT) |
+			(ISL28022_BRNG_60 << ISL28022_BRNG_SHIFT);
+
+	switch (data->gain) {
+	case 1:
+		data->config |= (ISL28022_PG_40 << ISL28022_PG_SHIFT);
+		break;
+	case 2:
+		data->config |= (ISL28022_PG_80 << ISL28022_PG_SHIFT);
+		break;
+	case 4:
+		data->config |= (ISL28022_PG_160 << ISL28022_PG_SHIFT);
+		break;
+	default:
+		data->config |= (ISL28022_PG_320 << ISL28022_PG_SHIFT);
+		data->gain = 8;
+		break;
+	}
+
+	data->calib = data->shunt ? 0x8000 / data->gain : 0;
+
+	switch (data->average) {
+	case 128:
+		data->config |= (ISL28022_ADC_15_128 << ISL28022_SADC_SHIFT) |
+				(ISL28022_ADC_15_128 << ISL28022_BADC_SHIFT);
+		break;
+	case 64:
+		data->config |= (ISL28022_ADC_15_64 << ISL28022_SADC_SHIFT) |
+				(ISL28022_ADC_15_64 << ISL28022_BADC_SHIFT);
+		break;
+	case 32:
+		data->config |= (ISL28022_ADC_15_32 << ISL28022_SADC_SHIFT) |
+				(ISL28022_ADC_15_32 << ISL28022_BADC_SHIFT);
+		break;
+	case 16:
+		data->config |= (ISL28022_ADC_15_16 << ISL28022_SADC_SHIFT) |
+				(ISL28022_ADC_15_16 << ISL28022_BADC_SHIFT);
+		break;
+	case 8:
+		data->config |= (ISL28022_ADC_15_8 << ISL28022_SADC_SHIFT) |
+				(ISL28022_ADC_15_8 << ISL28022_BADC_SHIFT);
+		break;
+	case 4:
+		data->config |= (ISL28022_ADC_15_4 << ISL28022_SADC_SHIFT) |
+				(ISL28022_ADC_15_4 << ISL28022_BADC_SHIFT);
+		break;
+	case 2:
+		data->config |= (ISL28022_ADC_15_2 << ISL28022_SADC_SHIFT) |
+				(ISL28022_ADC_15_2 << ISL28022_BADC_SHIFT);
+		break;
+	case 1:
+		data->config |= (ISL28022_ADC_15_1 << ISL28022_SADC_SHIFT) |
+				(ISL28022_ADC_15_1 << ISL28022_BADC_SHIFT);
+		break;
+	default:
+		data->config |= (ISL28022_ADC_15 << ISL28022_SADC_SHIFT) |
+				(ISL28022_ADC_15 << ISL28022_BADC_SHIFT);
+		data->average = 0;
+		break;
+	}
+
+	regmap_write(data->regmap, ISL28022_REG_CONFIG, data->config);
+	regmap_write(data->regmap, ISL28022_REG_CALIB, data->calib);
+
+	return 0;
+}
+
+static int isl28022_probe(struct i2c_client *client)
+{
+	struct device *dev = &client->dev;
+	struct device *hwmon_dev;
+	struct isl28022_data *data;
+	int status;
+
+	if (!i2c_check_functionality(client->adapter,
+				     I2C_FUNC_SMBUS_BYTE_DATA |
+				     I2C_FUNC_SMBUS_WORD_DATA))
+		return -EIO;
+
+	data = devm_kzalloc(dev, sizeof(struct isl28022_data), GFP_KERNEL);
+	if (!data)
+		return -ENOMEM;
+
+	data->client = client;
+
+	of_property_read_u32(dev->of_node, "shunt-resistor-micro-ohms", &data->shunt);
+	of_property_read_u32(dev->of_node, "shunt-gain", &data->gain);
+	of_property_read_u32(dev->of_node, "average", &data->average);
+
+	data->regmap = devm_regmap_init_i2c(client, &isl28022_regmap_config);
+	if (IS_ERR(data->regmap))
+		return PTR_ERR(data->regmap);
+
+	hwmon_dev = devm_hwmon_device_register_with_info(dev, "isl28022_hwmon",
+							 data, data->shunt ?
+							 &isl28022_chip_info_shunt :
+							 &isl28022_chip_info, NULL);
+	if (IS_ERR(hwmon_dev))
+		return PTR_ERR(hwmon_dev);
+
+	status = isl28022_config(hwmon_dev);
+	if (status)
+		return status;
+
+	dev_info(dev, "%s: sensor '%s'\n", dev_name(hwmon_dev), client->name);
+
+	return 0;
+}
+
+static const struct of_device_id __maybe_unused isl28022_of_match[] = {
+	{ .compatible = "renesas,isl28022"},
+	{ /* LIST END */ }
+};
+MODULE_DEVICE_TABLE(of, isl28022_of_match);
+
+static struct i2c_driver isl28022_driver = {
+	.class		= I2C_CLASS_HWMON,
+	.driver = {
+		.name	= "isl28022",
+		.of_match_table = of_match_ptr(isl28022_of_match),
+	},
+	.probe_new	= isl28022_probe,
+	.id_table	= isl28022_ids,
+};
+
+module_i2c_driver(isl28022_driver);
+
+MODULE_AUTHOR("Carsten Spiess <[e-mail]>");
+MODULE_DESCRIPTION("ISL28022 driver");
+MODULE_LICENSE("GPL");