diff mbox series

hwmon: (max597x) Add Maxim Max597x

Message ID 20230426090356.745979-1-Naresh.Solanki@9elements.com (mailing list archive)
State Changes Requested
Headers show
Series hwmon: (max597x) Add Maxim Max597x | expand

Commit Message

Naresh Solanki April 26, 2023, 9:03 a.m. UTC 
From: Patrick Rudolph <patrick.rudolph@9elements.com>

Add support for the Maxim Max59x power switch with current/voltage
monitor.

Signed-off-by: Patrick Rudolph <patrick.rudolph@9elements.com>
Signed-off-by: Naresh Solanki <Naresh.Solanki@9elements.com>
---
 drivers/hwmon/Kconfig   |   9 ++
 drivers/hwmon/Makefile  |   1 +
 drivers/hwmon/max597x.c | 212 ++++++++++++++++++++++++++++++++++++++++
 3 files changed, 222 insertions(+)
 create mode 100644 drivers/hwmon/max597x.c


base-commit: b4c288cfd2f84c44994330c408e14645d45dee5b

Comments

Christophe JAILLET April 26, 2023, 11:01 a.m. UTC | #1 
Le 26/04/2023 à 11:03, Naresh Solanki a écrit :
> From: Patrick Rudolph <patrick.rudolph@9elements.com>
> 
> Add support for the Maxim Max59x power switch with current/voltage
> monitor.
> 
> Signed-off-by: Patrick Rudolph <patrick.rudolph@9elements.com>
> Signed-off-by: Naresh Solanki <Naresh.Solanki@9elements.com>

Hi,

a few nit below, should there be a v2.

CJ

> ---
>   drivers/hwmon/Kconfig   |   9 ++
>   drivers/hwmon/Makefile  |   1 +
>   drivers/hwmon/max597x.c | 212 ++++++++++++++++++++++++++++++++++++++++
>   3 files changed, 222 insertions(+)
>   create mode 100644 drivers/hwmon/max597x.c
> 
> diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
> index 5b3b76477b0e..164d980d9de2 100644
> --- a/drivers/hwmon/Kconfig
> +++ b/drivers/hwmon/Kconfig
> @@ -1097,6 +1097,15 @@ config SENSORS_MAX31760
>   	  This driver can also be built as a module. If so, the module
>   	  will be called max31760.
>   
> +config SENSORS_MAX597X
> +	tristate "Maxim 597x power switch and monitor"
> +	depends on I2C
> +	depends on OF
> +	select MFD_MAX597X
> +	help
> +	  This driver exposes Maxim 5970/5978 voltage/current monitoring
> +	  interface.
> +
>   config SENSORS_MAX6620
>   	tristate "Maxim MAX6620 fan controller"
>   	depends on I2C
> diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile
> index 88712b5031c8..720eb7d5fe46 100644
> --- a/drivers/hwmon/Makefile
> +++ b/drivers/hwmon/Makefile
> @@ -142,6 +142,7 @@ obj-$(CONFIG_SENSORS_MAX197)	+= max197.o
>   obj-$(CONFIG_SENSORS_MAX31722)	+= max31722.o
>   obj-$(CONFIG_SENSORS_MAX31730)	+= max31730.o
>   obj-$(CONFIG_SENSORS_MAX31760)  += max31760.o
> +obj-$(CONFIG_SENSORS_MAX597X)	+= max597x.o
>   obj-$(CONFIG_SENSORS_MAX6620)	+= max6620.o
>   obj-$(CONFIG_SENSORS_MAX6621)	+= max6621.o
>   obj-$(CONFIG_SENSORS_MAX6639)	+= max6639.o
> diff --git a/drivers/hwmon/max597x.c b/drivers/hwmon/max597x.c
> new file mode 100644
> index 000000000000..d4d8c2faf55c
> --- /dev/null
> +++ b/drivers/hwmon/max597x.c
> @@ -0,0 +1,212 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Device driver for regulators in MAX5970 and MAX5978 IC
> + *
> + * Copyright (c) 2022 9elements GmbH
> + *
> + * Author: Patrick Rudolph <patrick.rudolph@9elements.com>
> + */
> +
> +#include <linux/hwmon.h>
> +#include <linux/i2c.h>
> +#include <linux/mfd/max597x.h>
> +#include <linux/platform_device.h>
> +#include <linux/regmap.h>
> +
> +struct max597x_hwmon {
> +	int num_switches, irng[MAX5970_NUM_SWITCHES], mon_rng[MAX5970_NUM_SWITCHES];

Having 1 item per line is much more usual.

> +	struct regmap *regmap;
> +};
> +
> +static int max597x_read_reg(struct max597x_hwmon *ddata, int reg, int range, long *val)
> +{
> +	u8 reg_data[2];
> +	int ret;
> +
> +	ret = regmap_bulk_read(ddata->regmap, reg, &reg_data[0], 2);
> +	if (ret < 0)
> +		return ret;
> +
> +	*val = (reg_data[0] << 2) | (reg_data[1] & 3);
> +	*val = *val * range;
> +	/*
> +	 * From datasheet, the range is fractionally less.
> +	 * To compensate that, divide with 1033 number.
> +	 */
> +	*val = *val / 1033;
> +
> +	return 0;
> +}
> +
> +static int max597x_read(struct device *dev, enum hwmon_sensor_types type,
> +			u32 attr, int channel, long *val)
> +{
> +	struct max597x_hwmon *ddata = dev_get_drvdata(dev);
> +	int ret;
> +
> +	switch (type) {
> +	case hwmon_curr:
> +		switch (attr) {
> +		case hwmon_curr_input:
> +			ret = max597x_read_reg(ddata, MAX5970_REG_CURRENT_H(channel),
> +					       ddata->irng[channel], val);
> +			if (ret < 0)
> +				return ret;
> +
> +			return 0;
> +		default:
> +			return -EOPNOTSUPP;
> +		}
> +
> +	case hwmon_in:
> +		switch (attr) {
> +		case hwmon_in_input:
> +			ret = max597x_read_reg(ddata, MAX5970_REG_VOLTAGE_H(channel),
> +					       ddata->mon_rng[channel], val);
> +			if (ret < 0)
> +				return ret;
> +			return 0;
> +		default:
> +			return -EOPNOTSUPP;
> +		}
> +	}
> +	return -EOPNOTSUPP;
> +}
> +
> +static umode_t max597x_is_visible(const void *data,
> +				  enum hwmon_sensor_types type,
> +				  u32 attr, int channel)
> +{
> +	struct max597x_hwmon *ddata = (struct max597x_hwmon *)data;
> +
> +	if (channel >= ddata->num_switches)
> +		return 0;
> +
> +	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;
> +	default:
> +		break;
> +	}
> +	return 0;
> +}
> +
> +static const struct hwmon_ops max597x_hwmon_ops = {
> +	.is_visible = max597x_is_visible,
> +	.read = max597x_read,
> +};
> +
> +#define HWMON_CURRENT	HWMON_C_INPUT
> +#define HWMON_VOLTAGE	HWMON_I_INPUT
> +
> +static const struct hwmon_channel_info *max597x_info[] = {
> +	HWMON_CHANNEL_INFO(in, HWMON_VOLTAGE, HWMON_VOLTAGE),
> +	HWMON_CHANNEL_INFO(curr, HWMON_CURRENT, HWMON_CURRENT),
> +	NULL
> +};
> +
> +static const struct hwmon_chip_info max597x_chip_info = {
> +	.ops = &max597x_hwmon_ops,
> +	.info = max597x_info,
> +};
> +
> +static int max597x_adc_range(struct regmap *regmap, const int ch,
> +			     u32 *irng, u32 *mon_rng)
> +{
> +	unsigned int reg;
> +	int ret;
> +
> +	/* Decode current ADC range */
> +	ret = regmap_read(regmap, MAX5970_REG_STATUS2, &reg);
> +	if (ret)
> +		return ret;
> +	switch (MAX5970_IRNG(reg, ch)) {
> +	case 0:
> +		*irng = 100000;	/* 100 mV */
> +		break;
> +	case 1:
> +		*irng = 50000;	/* 50 mV */
> +		break;
> +	case 2:
> +		*irng = 25000;	/* 25 mV */
> +		break;
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	/* Decode current voltage monitor range */
> +	ret = regmap_read(regmap, MAX5970_REG_MON_RANGE, &reg);
> +	if (ret)
> +		return ret;
> +
> +	*mon_rng = MAX5970_MON_MAX_RANGE_UV >> MAX5970_MON(reg, ch);
> +	*mon_rng /= 1000; /* uV to mV */
> +
> +	return 0;
> +}
> +
> +static int max597x_sensor_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct i2c_client *i2c = to_i2c_client(pdev->dev.parent);
> +	struct max597x_hwmon *ddata;
> +	struct regmap *regmap = dev_get_regmap(pdev->dev.parent, NULL);
> +	struct device *hwmon_dev;
> +	int err;
> +
> +	if (!regmap)
> +		return -EPROBE_DEFER;
> +
> +	ddata = devm_kzalloc(dev, sizeof(struct max597x_hwmon), GFP_KERNEL);
> +	if (!ddata)
> +		return -ENOMEM;
> +
> +	if (of_device_is_compatible(i2c->dev.of_node, "maxim,max5978"))
> +		ddata->num_switches = MAX597x_TYPE_MAX5978;
> +	else if (of_device_is_compatible(i2c->dev.of_node, "maxim,max5970"))
> +		ddata->num_switches = MAX597x_TYPE_MAX5970;
> +	else
> +		return -ENODEV;
> +
> +	ddata->regmap = regmap;
> +
> +	for (int i = 0; i < ddata->num_switches; i++) {

This is unusual to define 'i' within a 'for' loop.

> +		err = max597x_adc_range(regmap, i, &ddata->irng[i], &ddata->mon_rng[i]);
> +		if (err < 0)
> +			return err;
> +	}
> +
> +	hwmon_dev = devm_hwmon_device_register_with_info(dev,
> +							 "max597x_hwmon", ddata,
> +							 &max597x_chip_info, NULL);
> +

Unneeded empty line.

> +	if (IS_ERR(hwmon_dev)) {
> +		err = PTR_ERR(hwmon_dev);
> +		dev_err(dev, "Unable to register hwmon device, returned %d\n", err);
> +		return err;

return dev_err_probe()?

CJ

> +	}
> +
> +	return 0;
> +}
> +
> +static struct platform_driver max597x_sensor_driver = {
> +	.probe = max597x_sensor_probe,
> +	.driver = {
> +		.name = "max597x-hwmon",
> +	},
> +};
> +module_platform_driver(max597x_sensor_driver);
> +
> +MODULE_AUTHOR("Patrick Rudolph <patrick.rudolph@9elements.com>");
> +MODULE_DESCRIPTION("MAX5970_hot-swap controller driver");
> +MODULE_LICENSE("GPL v2");
> 
> base-commit: b4c288cfd2f84c44994330c408e14645d45dee5b
kernel test robot April 26, 2023, 12:35 p.m. UTC | #2 
Hi Naresh,

kernel test robot noticed the following build warnings:

[auto build test WARNING on b4c288cfd2f84c44994330c408e14645d45dee5b]

url:    https://github.com/intel-lab-lkp/linux/commits/Naresh-Solanki/hwmon-max597x-Add-Maxim-Max597x/20230426-170556
base:   b4c288cfd2f84c44994330c408e14645d45dee5b
patch link:    https://lore.kernel.org/r/20230426090356.745979-1-Naresh.Solanki%409elements.com
patch subject: [PATCH] hwmon: (max597x) Add Maxim Max597x
config: sparc-allyesconfig (https://download.01.org/0day-ci/archive/20230426/202304262021.nGJgsK76-lkp@intel.com/config)
compiler: sparc64-linux-gcc (GCC) 12.1.0
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/intel-lab-lkp/linux/commit/3d6f729d86a79adf0603717c79bc389a5dcc4444
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review Naresh-Solanki/hwmon-max597x-Add-Maxim-Max597x/20230426-170556
        git checkout 3d6f729d86a79adf0603717c79bc389a5dcc4444
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=sparc olddefconfig
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=sparc SHELL=/bin/bash drivers/

If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <lkp@intel.com>
| Link: https://lore.kernel.org/oe-kbuild-all/202304262021.nGJgsK76-lkp@intel.com/

All warnings (new ones prefixed by >>):

   drivers/hwmon/max597x.c: In function 'max597x_read':
>> drivers/hwmon/max597x.c:47:9: warning: enumeration value 'hwmon_chip' not handled in switch [-Wswitch]
      47 |         switch (type) {
         |         ^~~~~~
>> drivers/hwmon/max597x.c:47:9: warning: enumeration value 'hwmon_temp' not handled in switch [-Wswitch]
>> drivers/hwmon/max597x.c:47:9: warning: enumeration value 'hwmon_power' not handled in switch [-Wswitch]
>> drivers/hwmon/max597x.c:47:9: warning: enumeration value 'hwmon_energy' not handled in switch [-Wswitch]
>> drivers/hwmon/max597x.c:47:9: warning: enumeration value 'hwmon_humidity' not handled in switch [-Wswitch]
>> drivers/hwmon/max597x.c:47:9: warning: enumeration value 'hwmon_fan' not handled in switch [-Wswitch]
>> drivers/hwmon/max597x.c:47:9: warning: enumeration value 'hwmon_pwm' not handled in switch [-Wswitch]
>> drivers/hwmon/max597x.c:47:9: warning: enumeration value 'hwmon_intrusion' not handled in switch [-Wswitch]
>> drivers/hwmon/max597x.c:47:9: warning: enumeration value 'hwmon_max' not handled in switch [-Wswitch]


vim +/hwmon_chip +47 drivers/hwmon/max597x.c

    40	
    41	static int max597x_read(struct device *dev, enum hwmon_sensor_types type,
    42				u32 attr, int channel, long *val)
    43	{
    44		struct max597x_hwmon *ddata = dev_get_drvdata(dev);
    45		int ret;
    46	
  > 47		switch (type) {
    48		case hwmon_curr:
    49			switch (attr) {
    50			case hwmon_curr_input:
    51				ret = max597x_read_reg(ddata, MAX5970_REG_CURRENT_H(channel),
    52						       ddata->irng[channel], val);
    53				if (ret < 0)
    54					return ret;
    55	
    56				return 0;
    57			default:
    58				return -EOPNOTSUPP;
    59			}
    60	
    61		case hwmon_in:
    62			switch (attr) {
    63			case hwmon_in_input:
    64				ret = max597x_read_reg(ddata, MAX5970_REG_VOLTAGE_H(channel),
    65						       ddata->mon_rng[channel], val);
    66				if (ret < 0)
    67					return ret;
    68				return 0;
    69			default:
    70				return -EOPNOTSUPP;
    71			}
    72		}
    73		return -EOPNOTSUPP;
    74	}
    75
Guenter Roeck April 26, 2023, 2:35 p.m. UTC | #3 
On Wed, Apr 26, 2023 at 11:03:55AM +0200, Naresh Solanki wrote:
> From: Patrick Rudolph <patrick.rudolph@9elements.com>
> 
> Add support for the Maxim Max59x power switch with current/voltage
> monitor.
> 
> Signed-off-by: Patrick Rudolph <patrick.rudolph@9elements.com>
> Signed-off-by: Naresh Solanki <Naresh.Solanki@9elements.com>

Please no wildcards in file names or descriptions for hwmon drivers,
much less dual-digit wildcards.

This patch does not add support for MAX597{0..9} (and much less for
MAX59{00..99}), only for MAX5970 and MAX5978. For example, it does not
and never will support MAX5977 because that chip does not have an I2C
interface, or MAX5974 which happens to be a PWM controller.

Pick one of {5970,5978} for the file name, and mention both in
descriptions. Yes, I understand, this was accepted with wildcard
in mfd and regulator subsystems, but that doesn't make it acceptable
here.

Either case, what does this driver provide that isn't already
available through drivers/regulator/max597x-regulator.c ?
As written, the driver doesn't support any of the limit or alarm
registers, and there seems to be quite some overlap with the regulator
driver in terms of functionality. Please explore if that is acceptable
for the regulator subsystem. If it is not, come back and we can continue
discussing feasibility as separate hwmon driver. 

> ---
>  drivers/hwmon/Kconfig   |   9 ++
>  drivers/hwmon/Makefile  |   1 +
>  drivers/hwmon/max597x.c | 212 ++++++++++++++++++++++++++++++++++++++++

Documentation missing.

>  3 files changed, 222 insertions(+)
>  create mode 100644 drivers/hwmon/max597x.c
> 
> diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
> index 5b3b76477b0e..164d980d9de2 100644
> --- a/drivers/hwmon/Kconfig
> +++ b/drivers/hwmon/Kconfig
> @@ -1097,6 +1097,15 @@ config SENSORS_MAX31760
>  	  This driver can also be built as a module. If so, the module
>  	  will be called max31760.
>  
> +config SENSORS_MAX597X
> +	tristate "Maxim 597x power switch and monitor"
> +	depends on I2C
> +	depends on OF
> +	select MFD_MAX597X

That should be "depends on".

> +	help
> +	  This driver exposes Maxim 5970/5978 voltage/current monitoring
> +	  interface.
> +
>  config SENSORS_MAX6620
>  	tristate "Maxim MAX6620 fan controller"
>  	depends on I2C
> diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile
> index 88712b5031c8..720eb7d5fe46 100644
> --- a/drivers/hwmon/Makefile
> +++ b/drivers/hwmon/Makefile
> @@ -142,6 +142,7 @@ obj-$(CONFIG_SENSORS_MAX197)	+= max197.o
>  obj-$(CONFIG_SENSORS_MAX31722)	+= max31722.o
>  obj-$(CONFIG_SENSORS_MAX31730)	+= max31730.o
>  obj-$(CONFIG_SENSORS_MAX31760)  += max31760.o
> +obj-$(CONFIG_SENSORS_MAX597X)	+= max597x.o
>  obj-$(CONFIG_SENSORS_MAX6620)	+= max6620.o
>  obj-$(CONFIG_SENSORS_MAX6621)	+= max6621.o
>  obj-$(CONFIG_SENSORS_MAX6639)	+= max6639.o
> diff --git a/drivers/hwmon/max597x.c b/drivers/hwmon/max597x.c
> new file mode 100644
> index 000000000000..d4d8c2faf55c
> --- /dev/null
> +++ b/drivers/hwmon/max597x.c
> @@ -0,0 +1,212 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Device driver for regulators in MAX5970 and MAX5978 IC
> + *
> + * Copyright (c) 2022 9elements GmbH
> + *
> + * Author: Patrick Rudolph <patrick.rudolph@9elements.com>
> + */
> +
> +#include <linux/hwmon.h>
> +#include <linux/i2c.h>
> +#include <linux/mfd/max597x.h>
> +#include <linux/platform_device.h>
> +#include <linux/regmap.h>
> +
> +struct max597x_hwmon {
> +	int num_switches, irng[MAX5970_NUM_SWITCHES], mon_rng[MAX5970_NUM_SWITCHES];
> +	struct regmap *regmap;
> +};
> +
> +static int max597x_read_reg(struct max597x_hwmon *ddata, int reg, int range, long *val)
> +{
> +	u8 reg_data[2];
> +	int ret;
> +
> +	ret = regmap_bulk_read(ddata->regmap, reg, &reg_data[0], 2);
> +	if (ret < 0)
> +		return ret;
> +
> +	*val = (reg_data[0] << 2) | (reg_data[1] & 3);
> +	*val = *val * range;
> +	/*
> +	 * From datasheet, the range is fractionally less.
> +	 * To compensate that, divide with 1033 number.
> +	 */
> +	*val = *val / 1033;

Where in the datasheet, and why is this conversion not needed in the
regulator driver ?

> +
> +	return 0;
> +}
> +
> +static int max597x_read(struct device *dev, enum hwmon_sensor_types type,
> +			u32 attr, int channel, long *val)
> +{
> +	struct max597x_hwmon *ddata = dev_get_drvdata(dev);
> +	int ret;
> +
> +	switch (type) {
> +	case hwmon_curr:
> +		switch (attr) {
> +		case hwmon_curr_input:
> +			ret = max597x_read_reg(ddata, MAX5970_REG_CURRENT_H(channel),
> +					       ddata->irng[channel], val);
> +			if (ret < 0)
> +				return ret;
> +
> +			return 0;

max597x_read_reg() returns a negative value or 0. Why not just return it ?

The regulator driver takes shunt resistor values into account.

Again, I think it would make much more sense to add hwmon support
to the regulator driver than having a separate driver since there
is lots of overlap. For example, the regulator driver already
sets and monitors limits.

> +		default:
> +			return -EOPNOTSUPP;
> +		}
> +
> +	case hwmon_in:
> +		switch (attr) {
> +		case hwmon_in_input:
> +			ret = max597x_read_reg(ddata, MAX5970_REG_VOLTAGE_H(channel),
> +					       ddata->mon_rng[channel], val);
> +			if (ret < 0)
> +				return ret;
> +			return 0;
> +		default:
> +			return -EOPNOTSUPP;
> +		}
> +	}

Please add missing default: cases as reported by 0-day.

> +	return -EOPNOTSUPP;
> +}
> +
> +static umode_t max597x_is_visible(const void *data,
> +				  enum hwmon_sensor_types type,
> +				  u32 attr, int channel)
> +{
> +	struct max597x_hwmon *ddata = (struct max597x_hwmon *)data;
> +
> +	if (channel >= ddata->num_switches)
> +		return 0;
> +
> +	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;
> +	default:
> +		break;
> +	}
> +	return 0;
> +}
> +
> +static const struct hwmon_ops max597x_hwmon_ops = {
> +	.is_visible = max597x_is_visible,
> +	.read = max597x_read,
> +};
> +
> +#define HWMON_CURRENT	HWMON_C_INPUT
> +#define HWMON_VOLTAGE	HWMON_I_INPUT

Please drop and use HWMON_C_INPUT as well as HWMON_I_INPUT
directly.

> +
> +static const struct hwmon_channel_info *max597x_info[] = {
> +	HWMON_CHANNEL_INFO(in, HWMON_VOLTAGE, HWMON_VOLTAGE),
> +	HWMON_CHANNEL_INFO(curr, HWMON_CURRENT, HWMON_CURRENT),
> +	NULL
> +};
> +
> +static const struct hwmon_chip_info max597x_chip_info = {
> +	.ops = &max597x_hwmon_ops,
> +	.info = max597x_info,
> +};
> +
> +static int max597x_adc_range(struct regmap *regmap, const int ch,
> +			     u32 *irng, u32 *mon_rng)
> +{

That function also exists on the regulator driver.

> +	unsigned int reg;
> +	int ret;
> +
> +	/* Decode current ADC range */
> +	ret = regmap_read(regmap, MAX5970_REG_STATUS2, &reg);
> +	if (ret)
> +		return ret;
> +	switch (MAX5970_IRNG(reg, ch)) {
> +	case 0:
> +		*irng = 100000;	/* 100 mV */
> +		break;
> +	case 1:
> +		*irng = 50000;	/* 50 mV */
> +		break;
> +	case 2:
> +		*irng = 25000;	/* 25 mV */
> +		break;
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	/* Decode current voltage monitor range */
> +	ret = regmap_read(regmap, MAX5970_REG_MON_RANGE, &reg);
> +	if (ret)
> +		return ret;
> +
> +	*mon_rng = MAX5970_MON_MAX_RANGE_UV >> MAX5970_MON(reg, ch);
> +	*mon_rng /= 1000; /* uV to mV */
> +
> +	return 0;
> +}
> +
> +static int max597x_sensor_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct i2c_client *i2c = to_i2c_client(pdev->dev.parent);
> +	struct max597x_hwmon *ddata;
> +	struct regmap *regmap = dev_get_regmap(pdev->dev.parent, NULL);
> +	struct device *hwmon_dev;
> +	int err;
> +
> +	if (!regmap)
> +		return -EPROBE_DEFER;

Since the parent driver is a mfd driver, I'd assume that this driver
should be instantiated from there, and I don't see why probe would ever
have to be deferred. Please explain how that can happen.

> +
> +	ddata = devm_kzalloc(dev, sizeof(struct max597x_hwmon), GFP_KERNEL);
> +	if (!ddata)
> +		return -ENOMEM;
> +
> +	if (of_device_is_compatible(i2c->dev.of_node, "maxim,max5978"))
> +		ddata->num_switches = MAX597x_TYPE_MAX5978;
> +	else if (of_device_is_compatible(i2c->dev.of_node, "maxim,max5970"))
> +		ddata->num_switches = MAX597x_TYPE_MAX5970;

Uuh, no. That is just accidentally correct, based on

enum max597x_chip_type {
        MAX597x_TYPE_MAX5978 = 1,
        MAX597x_TYPE_MAX5970,
};

The assumption that the numeric chip type matches the number of channels
it supports is as wrong as it can be. There are defines for the number
of channels. I am pretty much completely at loss why they are not used.
Tha same applies for the regulator driver, of course.

> +	else
> +		return -ENODEV;
> +
> +	ddata->regmap = regmap;
> +
> +	for (int i = 0; i < ddata->num_switches; i++) {
> +		err = max597x_adc_range(regmap, i, &ddata->irng[i], &ddata->mon_rng[i]);
> +		if (err < 0)
> +			return err;

Why no error message here but one below ? I am not in favor of error
messages in the probe function, but if you use them please be consistent.

> +	}
> +
> +	hwmon_dev = devm_hwmon_device_register_with_info(dev,
> +							 "max597x_hwmon", ddata,
> +							 &max597x_chip_info, NULL);
> +
> +	if (IS_ERR(hwmon_dev)) {
> +		err = PTR_ERR(hwmon_dev);
> +		dev_err(dev, "Unable to register hwmon device, returned %d\n", err);

Use dev_err_probe()

> +		return err;
> +	}
> +
> +	return 0;
> +}
> +
> +static struct platform_driver max597x_sensor_driver = {
> +	.probe = max597x_sensor_probe,
> +	.driver = {
> +		.name = "max597x-hwmon",
> +	},
> +};
> +module_platform_driver(max597x_sensor_driver);
> +
> +MODULE_AUTHOR("Patrick Rudolph <patrick.rudolph@9elements.com>");
> +MODULE_DESCRIPTION("MAX5970_hot-swap controller driver");
> +MODULE_LICENSE("GPL v2");
> 
> base-commit: b4c288cfd2f84c44994330c408e14645d45dee5b
> -- 
> 2.39.1
>
Naresh Solanki April 27, 2023, 9:50 a.m. UTC | #4 
Hi Guenter,

On 26-04-2023 08:05 pm, Guenter Roeck wrote:
> On Wed, Apr 26, 2023 at 11:03:55AM +0200, Naresh Solanki wrote:
>> From: Patrick Rudolph <patrick.rudolph@9elements.com>
>>
>> Add support for the Maxim Max59x power switch with current/voltage
>> monitor.
>>
>> Signed-off-by: Patrick Rudolph <patrick.rudolph@9elements.com>
>> Signed-off-by: Naresh Solanki <Naresh.Solanki@9elements.com>
> 
> Please no wildcards in file names or descriptions for hwmon drivers,
> much less dual-digit wildcards.
> 
> This patch does not add support for MAX597{0..9} (and much less for
> MAX59{00..99}), only for MAX5970 and MAX5978. For example, it does not
> and never will support MAX5977 because that chip does not have an I2C
> interface, or MAX5974 which happens to be a PWM controller.
> 
> Pick one of {5970,5978} for the file name, and mention both in
> descriptions. Yes, I understand, this was accepted with wildcard
> in mfd and regulator subsystems, but that doesn't make it acceptable
> here.
> 
> Either case, what does this driver provide that isn't already
> available through drivers/regulator/max597x-regulator.c ?
> As written, the driver doesn't support any of the limit or alarm
> registers, and there seems to be quite some overlap with the regulator
> driver in terms of functionality. Please explore if that is acceptable
> for the regulator subsystem. If it is not, come back and we can continue
> discussing feasibility as separate hwmon driver.Thanks for your feedback.
I agree that file name wildcard doesn't makes sense here & there are 
other chips that aren't related to this driver. Hence will work on it to 
get it fixed for sure.
Will check with regulator subsystem maintainers & based on feedback will 
proceed.
Thanks :)
> 
>> ---
>>   drivers/hwmon/Kconfig   |   9 ++
>>   drivers/hwmon/Makefile  |   1 +
>>   drivers/hwmon/max597x.c | 212 ++++++++++++++++++++++++++++++++++++++++
> 
> Documentation missing.
> 
>>   3 files changed, 222 insertions(+)
>>   create mode 100644 drivers/hwmon/max597x.c
>>
>> diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
>> index 5b3b76477b0e..164d980d9de2 100644
>> --- a/drivers/hwmon/Kconfig
>> +++ b/drivers/hwmon/Kconfig
>> @@ -1097,6 +1097,15 @@ config SENSORS_MAX31760
>>   	  This driver can also be built as a module. If so, the module
>>   	  will be called max31760.
>>   
>> +config SENSORS_MAX597X
>> +	tristate "Maxim 597x power switch and monitor"
>> +	depends on I2C
>> +	depends on OF
>> +	select MFD_MAX597X
> 
> That should be "depends on".
Ack
> 
>> +	help
>> +	  This driver exposes Maxim 5970/5978 voltage/current monitoring
>> +	  interface.
>> +
>>   config SENSORS_MAX6620
>>   	tristate "Maxim MAX6620 fan controller"
>>   	depends on I2C
>> diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile
>> index 88712b5031c8..720eb7d5fe46 100644
>> --- a/drivers/hwmon/Makefile
>> +++ b/drivers/hwmon/Makefile
>> @@ -142,6 +142,7 @@ obj-$(CONFIG_SENSORS_MAX197)	+= max197.o
>>   obj-$(CONFIG_SENSORS_MAX31722)	+= max31722.o
>>   obj-$(CONFIG_SENSORS_MAX31730)	+= max31730.o
>>   obj-$(CONFIG_SENSORS_MAX31760)  += max31760.o
>> +obj-$(CONFIG_SENSORS_MAX597X)	+= max597x.o
>>   obj-$(CONFIG_SENSORS_MAX6620)	+= max6620.o
>>   obj-$(CONFIG_SENSORS_MAX6621)	+= max6621.o
>>   obj-$(CONFIG_SENSORS_MAX6639)	+= max6639.o
>> diff --git a/drivers/hwmon/max597x.c b/drivers/hwmon/max597x.c
>> new file mode 100644
>> index 000000000000..d4d8c2faf55c
>> --- /dev/null
>> +++ b/drivers/hwmon/max597x.c
>> @@ -0,0 +1,212 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * Device driver for regulators in MAX5970 and MAX5978 IC
>> + *
>> + * Copyright (c) 2022 9elements GmbH
>> + *
>> + * Author: Patrick Rudolph <patrick.rudolph@9elements.com>
>> + */
>> +
>> +#include <linux/hwmon.h>
>> +#include <linux/i2c.h>
>> +#include <linux/mfd/max597x.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/regmap.h>
>> +
>> +struct max597x_hwmon {
>> +	int num_switches, irng[MAX5970_NUM_SWITCHES], mon_rng[MAX5970_NUM_SWITCHES];
>> +	struct regmap *regmap;
>> +};
>> +
>> +static int max597x_read_reg(struct max597x_hwmon *ddata, int reg, int range, long *val)
>> +{
>> +	u8 reg_data[2];
>> +	int ret;
>> +
>> +	ret = regmap_bulk_read(ddata->regmap, reg, &reg_data[0], 2);
>> +	if (ret < 0)
>> +		return ret;
>> +
>> +	*val = (reg_data[0] << 2) | (reg_data[1] & 3);
>> +	*val = *val * range;
>> +	/*
>> +	 * From datasheet, the range is fractionally less.
>> +	 * To compensate that, divide with 1033 number.
>> +	 */
>> +	*val = *val / 1033;
> 
> Where in the datasheet, and why is this conversion not needed in the
> regulator driver ?
This is taken from the typical value mentioned in table containing MON_ 
LSB Voltage in datasheet. i.e., divide typical value with corresponding 
range.

This is applicable for use in hwmon driver to accurate report readings 
from device.
> 
>> +
>> +	return 0;
>> +}
>> +
>> +static int max597x_read(struct device *dev, enum hwmon_sensor_types type,
>> +			u32 attr, int channel, long *val)
>> +{
>> +	struct max597x_hwmon *ddata = dev_get_drvdata(dev);
>> +	int ret;
>> +
>> +	switch (type) {
>> +	case hwmon_curr:
>> +		switch (attr) {
>> +		case hwmon_curr_input:
>> +			ret = max597x_read_reg(ddata, MAX5970_REG_CURRENT_H(channel),
>> +					       ddata->irng[channel], val);
>> +			if (ret < 0)
>> +				return ret;
>> +
>> +			return 0;
> 
> max597x_read_reg() returns a negative value or 0. Why not just return it ?
Ack.
> 
> The regulator driver takes shunt resistor values into account.
Agree. The shunt resistor value should be accounted for correct current 
measurement. This driver lacks that. This needs to be fixed. Will post 
new patch revision to address that.
> 
> Again, I think it would make much more sense to add hwmon support
> to the regulator driver than having a separate driver since there
> is lots of overlap. For example, the regulator driver already
> sets and monitors limits.
I agree. But the chip also has led functionality. I got review feedback 
to make it MFD. So when rewriting as MFD driver made separate driver 
based on subsystem. I'm not sure if it is ok to use hwmon subsytem in 
regulator driver. Will once check with maintainer on this.
> 
>> +		default:
>> +			return -EOPNOTSUPP;
>> +		}
>> +
>> +	case hwmon_in:
>> +		switch (attr) {
>> +		case hwmon_in_input:
>> +			ret = max597x_read_reg(ddata, MAX5970_REG_VOLTAGE_H(channel),
>> +					       ddata->mon_rng[channel], val);
>> +			if (ret < 0)
>> +				return ret;
>> +			return 0;
>> +		default:
>> +			return -EOPNOTSUPP;
>> +		}
>> +	}
> 
> Please add missing default: cases as reported by 0-day.
Ack
> 
>> +	return -EOPNOTSUPP;
>> +}
>> +
>> +static umode_t max597x_is_visible(const void *data,
>> +				  enum hwmon_sensor_types type,
>> +				  u32 attr, int channel)
>> +{
>> +	struct max597x_hwmon *ddata = (struct max597x_hwmon *)data;
>> +
>> +	if (channel >= ddata->num_switches)
>> +		return 0;
>> +
>> +	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;
>> +	default:
>> +		break;
>> +	}
>> +	return 0;
>> +}
>> +
>> +static const struct hwmon_ops max597x_hwmon_ops = {
>> +	.is_visible = max597x_is_visible,
>> +	.read = max597x_read,
>> +};
>> +
>> +#define HWMON_CURRENT	HWMON_C_INPUT
>> +#define HWMON_VOLTAGE	HWMON_I_INPUT
> 
> Please drop and use HWMON_C_INPUT as well as HWMON_I_INPUT
> directly.
Ack.
> 
>> +
>> +static const struct hwmon_channel_info *max597x_info[] = {
>> +	HWMON_CHANNEL_INFO(in, HWMON_VOLTAGE, HWMON_VOLTAGE),
>> +	HWMON_CHANNEL_INFO(curr, HWMON_CURRENT, HWMON_CURRENT),
>> +	NULL
>> +};
>> +
>> +static const struct hwmon_chip_info max597x_chip_info = {
>> +	.ops = &max597x_hwmon_ops,
>> +	.info = max597x_info,
>> +};
>> +
>> +static int max597x_adc_range(struct regmap *regmap, const int ch,
>> +			     u32 *irng, u32 *mon_rng)
>> +{
> 
> That function also exists on the regulator driver.
Yes. This is to read setting from chip. Added this to avoid dependency & 
race condition with regulator driver.
> 
>> +	unsigned int reg;
>> +	int ret;
>> +
>> +	/* Decode current ADC range */
>> +	ret = regmap_read(regmap, MAX5970_REG_STATUS2, &reg);
>> +	if (ret)
>> +		return ret;
>> +	switch (MAX5970_IRNG(reg, ch)) {
>> +	case 0:
>> +		*irng = 100000;	/* 100 mV */
>> +		break;
>> +	case 1:
>> +		*irng = 50000;	/* 50 mV */
>> +		break;
>> +	case 2:
>> +		*irng = 25000;	/* 25 mV */
>> +		break;
>> +	default:
>> +		return -EINVAL;
>> +	}
>> +
>> +	/* Decode current voltage monitor range */
>> +	ret = regmap_read(regmap, MAX5970_REG_MON_RANGE, &reg);
>> +	if (ret)
>> +		return ret;
>> +
>> +	*mon_rng = MAX5970_MON_MAX_RANGE_UV >> MAX5970_MON(reg, ch);
>> +	*mon_rng /= 1000; /* uV to mV */
>> +
>> +	return 0;
>> +}
>> +
>> +static int max597x_sensor_probe(struct platform_device *pdev)
>> +{
>> +	struct device *dev = &pdev->dev;
>> +	struct i2c_client *i2c = to_i2c_client(pdev->dev.parent);
>> +	struct max597x_hwmon *ddata;
>> +	struct regmap *regmap = dev_get_regmap(pdev->dev.parent, NULL);
>> +	struct device *hwmon_dev;
>> +	int err;
>> +
>> +	if (!regmap)
>> +		return -EPROBE_DEFER;
> 
> Since the parent driver is a mfd driver, I'd assume that this driver
> should be instantiated from there, and I don't see why probe would ever
> have to be deferred. Please explain how that can happen.
I agree with you but this implementation is referenced from other MFD 
leaf driver.
Example: "drivers/hwmon/sy7636a-hwmon.c"
> 
>> +
>> +	ddata = devm_kzalloc(dev, sizeof(struct max597x_hwmon), GFP_KERNEL);
>> +	if (!ddata)
>> +		return -ENOMEM;
>> +
>> +	if (of_device_is_compatible(i2c->dev.of_node, "maxim,max5978"))
>> +		ddata->num_switches = MAX597x_TYPE_MAX5978;
>> +	else if (of_device_is_compatible(i2c->dev.of_node, "maxim,max5970"))
>> +		ddata->num_switches = MAX597x_TYPE_MAX5970;
> 
> Uuh, no. That is just accidentally correct, based on
> 
> enum max597x_chip_type {
>          MAX597x_TYPE_MAX5978 = 1,
>          MAX597x_TYPE_MAX5970,
> };
> 
> The assumption that the numeric chip type matches the number of channels
> it supports is as wrong as it can be. There are defines for the number
> of channels. I am pretty much completely at loss why they are not used.
> Tha same applies for the regulator driver, of course.
Ack. Will use defines.
> 
>> +	else
>> +		return -ENODEV;
>> +
>> +	ddata->regmap = regmap;
>> +
>> +	for (int i = 0; i < ddata->num_switches; i++) {
>> +		err = max597x_adc_range(regmap, i, &ddata->irng[i], &ddata->mon_rng[i]);
>> +		if (err < 0)
>> +			return err;
> 
> Why no error message here but one below ? I am not in favor of error
> messages in the probe function, but if you use them please be consistent.
Ack.
> 
>> +	}
>> +
>> +	hwmon_dev = devm_hwmon_device_register_with_info(dev,
>> +							 "max597x_hwmon", ddata,
>> +							 &max597x_chip_info, NULL);
>> +
>> +	if (IS_ERR(hwmon_dev)) {
>> +		err = PTR_ERR(hwmon_dev);
>> +		dev_err(dev, "Unable to register hwmon device, returned %d\n", err);
> 
> Use dev_err_probe()
Ack
> 
>> +		return err;
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +static struct platform_driver max597x_sensor_driver = {
>> +	.probe = max597x_sensor_probe,
>> +	.driver = {
>> +		.name = "max597x-hwmon",
>> +	},
>> +};
>> +module_platform_driver(max597x_sensor_driver);
>> +
>> +MODULE_AUTHOR("Patrick Rudolph <patrick.rudolph@9elements.com>");
>> +MODULE_DESCRIPTION("MAX5970_hot-swap controller driver");
>> +MODULE_LICENSE("GPL v2");
>>
>> base-commit: b4c288cfd2f84c44994330c408e14645d45dee5b
>> -- 
>> 2.39.1
>>
Regards,
Naresh
Guenter Roeck April 27, 2023, 10:04 a.m. UTC | #5 
On 4/27/23 02:50, Naresh Solanki wrote:
> Hi Guenter,
> 
> On 26-04-2023 08:05 pm, Guenter Roeck wrote:
>> On Wed, Apr 26, 2023 at 11:03:55AM +0200, Naresh Solanki wrote:
>>> From: Patrick Rudolph <patrick.rudolph@9elements.com>
>>>
>>> Add support for the Maxim Max59x power switch with current/voltage
>>> monitor.
>>>
>>> Signed-off-by: Patrick Rudolph <patrick.rudolph@9elements.com>
>>> Signed-off-by: Naresh Solanki <Naresh.Solanki@9elements.com>
>>
>> Please no wildcards in file names or descriptions for hwmon drivers,
>> much less dual-digit wildcards.
>>
>> This patch does not add support for MAX597{0..9} (and much less for
>> MAX59{00..99}), only for MAX5970 and MAX5978. For example, it does not
>> and never will support MAX5977 because that chip does not have an I2C
>> interface, or MAX5974 which happens to be a PWM controller.
>>
>> Pick one of {5970,5978} for the file name, and mention both in
>> descriptions. Yes, I understand, this was accepted with wildcard
>> in mfd and regulator subsystems, but that doesn't make it acceptable
>> here.
>>
>> Either case, what does this driver provide that isn't already
>> available through drivers/regulator/max597x-regulator.c ?
>> As written, the driver doesn't support any of the limit or alarm
>> registers, and there seems to be quite some overlap with the regulator
>> driver in terms of functionality. Please explore if that is acceptable
>> for the regulator subsystem. If it is not, come back and we can continue
>> discussing feasibility as separate hwmon driver.Thanks for your feedback.
> I agree that file name wildcard doesn't makes sense here & there are other chips that aren't related to this driver. Hence will work on it to get it fixed for sure.
> Will check with regulator subsystem maintainers & based on feedback will proceed.
> Thanks :)
>>
>>> ---
>>>   drivers/hwmon/Kconfig   |   9 ++
>>>   drivers/hwmon/Makefile  |   1 +
>>>   drivers/hwmon/max597x.c | 212 ++++++++++++++++++++++++++++++++++++++++
>>
>> Documentation missing.
>>
>>>   3 files changed, 222 insertions(+)
>>>   create mode 100644 drivers/hwmon/max597x.c
>>>
>>> diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
>>> index 5b3b76477b0e..164d980d9de2 100644
>>> --- a/drivers/hwmon/Kconfig
>>> +++ b/drivers/hwmon/Kconfig
>>> @@ -1097,6 +1097,15 @@ config SENSORS_MAX31760
>>>         This driver can also be built as a module. If so, the module
>>>         will be called max31760.
>>> +config SENSORS_MAX597X
>>> +    tristate "Maxim 597x power switch and monitor"
>>> +    depends on I2C
>>> +    depends on OF
>>> +    select MFD_MAX597X
>>
>> That should be "depends on".
> Ack
>>
>>> +    help
>>> +      This driver exposes Maxim 5970/5978 voltage/current monitoring
>>> +      interface.
>>> +
>>>   config SENSORS_MAX6620
>>>       tristate "Maxim MAX6620 fan controller"
>>>       depends on I2C
>>> diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile
>>> index 88712b5031c8..720eb7d5fe46 100644
>>> --- a/drivers/hwmon/Makefile
>>> +++ b/drivers/hwmon/Makefile
>>> @@ -142,6 +142,7 @@ obj-$(CONFIG_SENSORS_MAX197)    += max197.o
>>>   obj-$(CONFIG_SENSORS_MAX31722)    += max31722.o
>>>   obj-$(CONFIG_SENSORS_MAX31730)    += max31730.o
>>>   obj-$(CONFIG_SENSORS_MAX31760)  += max31760.o
>>> +obj-$(CONFIG_SENSORS_MAX597X)    += max597x.o
>>>   obj-$(CONFIG_SENSORS_MAX6620)    += max6620.o
>>>   obj-$(CONFIG_SENSORS_MAX6621)    += max6621.o
>>>   obj-$(CONFIG_SENSORS_MAX6639)    += max6639.o
>>> diff --git a/drivers/hwmon/max597x.c b/drivers/hwmon/max597x.c
>>> new file mode 100644
>>> index 000000000000..d4d8c2faf55c
>>> --- /dev/null
>>> +++ b/drivers/hwmon/max597x.c
>>> @@ -0,0 +1,212 @@
>>> +// SPDX-License-Identifier: GPL-2.0
>>> +/*
>>> + * Device driver for regulators in MAX5970 and MAX5978 IC
>>> + *
>>> + * Copyright (c) 2022 9elements GmbH
>>> + *
>>> + * Author: Patrick Rudolph <patrick.rudolph@9elements.com>
>>> + */
>>> +
>>> +#include <linux/hwmon.h>
>>> +#include <linux/i2c.h>
>>> +#include <linux/mfd/max597x.h>
>>> +#include <linux/platform_device.h>
>>> +#include <linux/regmap.h>
>>> +
>>> +struct max597x_hwmon {
>>> +    int num_switches, irng[MAX5970_NUM_SWITCHES], mon_rng[MAX5970_NUM_SWITCHES];
>>> +    struct regmap *regmap;
>>> +};
>>> +
>>> +static int max597x_read_reg(struct max597x_hwmon *ddata, int reg, int range, long *val)
>>> +{
>>> +    u8 reg_data[2];
>>> +    int ret;
>>> +
>>> +    ret = regmap_bulk_read(ddata->regmap, reg, &reg_data[0], 2);
>>> +    if (ret < 0)
>>> +        return ret;
>>> +
>>> +    *val = (reg_data[0] << 2) | (reg_data[1] & 3);
>>> +    *val = *val * range;
>>> +    /*
>>> +     * From datasheet, the range is fractionally less.
>>> +     * To compensate that, divide with 1033 number.
>>> +     */
>>> +    *val = *val / 1033;
>>
>> Where in the datasheet, and why is this conversion not needed in the
>> regulator driver ?
> This is taken from the typical value mentioned in table containing MON_ LSB Voltage in datasheet. i.e., divide typical value with corresponding range.
> 
> This is applicable for use in hwmon driver to accurate report readings from device.

Please add respective comment.

>>
>>> +
>>> +    return 0;
>>> +}
>>> +
>>> +static int max597x_read(struct device *dev, enum hwmon_sensor_types type,
>>> +            u32 attr, int channel, long *val)
>>> +{
>>> +    struct max597x_hwmon *ddata = dev_get_drvdata(dev);
>>> +    int ret;
>>> +
>>> +    switch (type) {
>>> +    case hwmon_curr:
>>> +        switch (attr) {
>>> +        case hwmon_curr_input:
>>> +            ret = max597x_read_reg(ddata, MAX5970_REG_CURRENT_H(channel),
>>> +                           ddata->irng[channel], val);
>>> +            if (ret < 0)
>>> +                return ret;
>>> +
>>> +            return 0;
>>
>> max597x_read_reg() returns a negative value or 0. Why not just return it ?
> Ack.
>>
>> The regulator driver takes shunt resistor values into account.
> Agree. The shunt resistor value should be accounted for correct current measurement. This driver lacks that. This needs to be fixed. Will post new patch revision to address that.
>>
>> Again, I think it would make much more sense to add hwmon support
>> to the regulator driver than having a separate driver since there
>> is lots of overlap. For example, the regulator driver already
>> sets and monitors limits.
> I agree. But the chip also has led functionality. I got review feedback to make it MFD. So when rewriting as MFD driver made separate driver based on subsystem. I'm not sure if it is ok to use hwmon subsytem in regulator driver. Will once check with maintainer on this.
>>
>>> +        default:
>>> +            return -EOPNOTSUPP;
>>> +        }
>>> +
>>> +    case hwmon_in:
>>> +        switch (attr) {
>>> +        case hwmon_in_input:
>>> +            ret = max597x_read_reg(ddata, MAX5970_REG_VOLTAGE_H(channel),
>>> +                           ddata->mon_rng[channel], val);
>>> +            if (ret < 0)
>>> +                return ret;
>>> +            return 0;
>>> +        default:
>>> +            return -EOPNOTSUPP;
>>> +        }
>>> +    }
>>
>> Please add missing default: cases as reported by 0-day.
> Ack
>>
>>> +    return -EOPNOTSUPP;
>>> +}
>>> +
>>> +static umode_t max597x_is_visible(const void *data,
>>> +                  enum hwmon_sensor_types type,
>>> +                  u32 attr, int channel)
>>> +{
>>> +    struct max597x_hwmon *ddata = (struct max597x_hwmon *)data;
>>> +
>>> +    if (channel >= ddata->num_switches)
>>> +        return 0;
>>> +
>>> +    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;
>>> +    default:
>>> +        break;
>>> +    }
>>> +    return 0;
>>> +}
>>> +
>>> +static const struct hwmon_ops max597x_hwmon_ops = {
>>> +    .is_visible = max597x_is_visible,
>>> +    .read = max597x_read,
>>> +};
>>> +
>>> +#define HWMON_CURRENT    HWMON_C_INPUT
>>> +#define HWMON_VOLTAGE    HWMON_I_INPUT
>>
>> Please drop and use HWMON_C_INPUT as well as HWMON_I_INPUT
>> directly.
> Ack.
>>
>>> +
>>> +static const struct hwmon_channel_info *max597x_info[] = {
>>> +    HWMON_CHANNEL_INFO(in, HWMON_VOLTAGE, HWMON_VOLTAGE),
>>> +    HWMON_CHANNEL_INFO(curr, HWMON_CURRENT, HWMON_CURRENT),
>>> +    NULL
>>> +};
>>> +
>>> +static const struct hwmon_chip_info max597x_chip_info = {
>>> +    .ops = &max597x_hwmon_ops,
>>> +    .info = max597x_info,
>>> +};
>>> +
>>> +static int max597x_adc_range(struct regmap *regmap, const int ch,
>>> +                 u32 *irng, u32 *mon_rng)
>>> +{
>>
>> That function also exists on the regulator driver.
> Yes. This is to read setting from chip. Added this to avoid dependency & race condition with regulator driver.
>>
>>> +    unsigned int reg;
>>> +    int ret;
>>> +
>>> +    /* Decode current ADC range */
>>> +    ret = regmap_read(regmap, MAX5970_REG_STATUS2, &reg);
>>> +    if (ret)
>>> +        return ret;
>>> +    switch (MAX5970_IRNG(reg, ch)) {
>>> +    case 0:
>>> +        *irng = 100000;    /* 100 mV */
>>> +        break;
>>> +    case 1:
>>> +        *irng = 50000;    /* 50 mV */
>>> +        break;
>>> +    case 2:
>>> +        *irng = 25000;    /* 25 mV */
>>> +        break;
>>> +    default:
>>> +        return -EINVAL;
>>> +    }
>>> +
>>> +    /* Decode current voltage monitor range */
>>> +    ret = regmap_read(regmap, MAX5970_REG_MON_RANGE, &reg);
>>> +    if (ret)
>>> +        return ret;
>>> +
>>> +    *mon_rng = MAX5970_MON_MAX_RANGE_UV >> MAX5970_MON(reg, ch);
>>> +    *mon_rng /= 1000; /* uV to mV */
>>> +
>>> +    return 0;
>>> +}
>>> +
>>> +static int max597x_sensor_probe(struct platform_device *pdev)
>>> +{
>>> +    struct device *dev = &pdev->dev;
>>> +    struct i2c_client *i2c = to_i2c_client(pdev->dev.parent);
>>> +    struct max597x_hwmon *ddata;
>>> +    struct regmap *regmap = dev_get_regmap(pdev->dev.parent, NULL);
>>> +    struct device *hwmon_dev;
>>> +    int err;
>>> +
>>> +    if (!regmap)
>>> +        return -EPROBE_DEFER;
>>
>> Since the parent driver is a mfd driver, I'd assume that this driver
>> should be instantiated from there, and I don't see why probe would ever
>> have to be deferred. Please explain how that can happen.
> I agree with you but this implementation is referenced from other MFD leaf driver.
> Example: "drivers/hwmon/sy7636a-hwmon.c"

That doesn't make it better, unless driving above the speed limit is ok
because everyone is doing it.

>>
>>> +
>>> +    ddata = devm_kzalloc(dev, sizeof(struct max597x_hwmon), GFP_KERNEL);
>>> +    if (!ddata)
>>> +        return -ENOMEM;
>>> +
>>> +    if (of_device_is_compatible(i2c->dev.of_node, "maxim,max5978"))
>>> +        ddata->num_switches = MAX597x_TYPE_MAX5978;
>>> +    else if (of_device_is_compatible(i2c->dev.of_node, "maxim,max5970"))
>>> +        ddata->num_switches = MAX597x_TYPE_MAX5970;
>>
>> Uuh, no. That is just accidentally correct, based on
>>
>> enum max597x_chip_type {
>>          MAX597x_TYPE_MAX5978 = 1,
>>          MAX597x_TYPE_MAX5970,
>> };
>>
>> The assumption that the numeric chip type matches the number of channels
>> it supports is as wrong as it can be. There are defines for the number
>> of channels. I am pretty much completely at loss why they are not used.
>> Tha same applies for the regulator driver, of course.
> Ack. Will use defines.
>>
>>> +    else
>>> +        return -ENODEV;
>>> +
>>> +    ddata->regmap = regmap;
>>> +
>>> +    for (int i = 0; i < ddata->num_switches; i++) {
>>> +        err = max597x_adc_range(regmap, i, &ddata->irng[i], &ddata->mon_rng[i]);
>>> +        if (err < 0)
>>> +            return err;
>>
>> Why no error message here but one below ? I am not in favor of error
>> messages in the probe function, but if you use them please be consistent.
> Ack.
>>
>>> +    }
>>> +
>>> +    hwmon_dev = devm_hwmon_device_register_with_info(dev,
>>> +                             "max597x_hwmon", ddata,
>>> +                             &max597x_chip_info, NULL);
>>> +
>>> +    if (IS_ERR(hwmon_dev)) {
>>> +        err = PTR_ERR(hwmon_dev);
>>> +        dev_err(dev, "Unable to register hwmon device, returned %d\n", err);
>>
>> Use dev_err_probe()
> Ack
>>
>>> +        return err;
>>> +    }
>>> +
>>> +    return 0;
>>> +}
>>> +
>>> +static struct platform_driver max597x_sensor_driver = {
>>> +    .probe = max597x_sensor_probe,
>>> +    .driver = {
>>> +        .name = "max597x-hwmon",
>>> +    },
>>> +};
>>> +module_platform_driver(max597x_sensor_driver);
>>> +
>>> +MODULE_AUTHOR("Patrick Rudolph <patrick.rudolph@9elements.com>");
>>> +MODULE_DESCRIPTION("MAX5970_hot-swap controller driver");
>>> +MODULE_LICENSE("GPL v2");
>>>
>>> base-commit: b4c288cfd2f84c44994330c408e14645d45dee5b
>>> -- 
>>> 2.39.1
>>>
> Regards,
> Naresh
Guenter Roeck April 27, 2023, 10:32 a.m. UTC | #6 
On 4/27/23 02:50, Naresh Solanki wrote:
> Hi Guenter,
> 
[ ... ]

>> Again, I think it would make much more sense to add hwmon support
>> to the regulator driver than having a separate driver since there
>> is lots of overlap. For example, the regulator driver already
>> sets and monitors limits.
> I agree. But the chip also has led functionality. I got review feedback to make it MFD. So when rewriting as MFD driver made separate driver based on subsystem. I'm not sure if it is ok to use hwmon subsytem in regulator driver. Will once check with maintainer on this.

LED has completely different functionality. Overlap between regulator and
hwmon is substantial because the regulator code handles limits and alarms.
Sure, you kind of bypassed (ignored) that by not implementing limit and alarm
support in the hwmon driver, but that is really just avoiding the problem,
not solving it.

Guenter
kernel test robot May 3, 2023, 8:29 a.m. UTC | #7 
Hi Naresh,

kernel test robot noticed the following build warnings:

[auto build test WARNING on b4c288cfd2f84c44994330c408e14645d45dee5b]

url:    https://github.com/intel-lab-lkp/linux/commits/Naresh-Solanki/hwmon-max597x-Add-Maxim-Max597x/20230426-170556
base:   b4c288cfd2f84c44994330c408e14645d45dee5b
patch link:    https://lore.kernel.org/r/20230426090356.745979-1-Naresh.Solanki%409elements.com
patch subject: [PATCH] hwmon: (max597x) Add Maxim Max597x
config: i386-allmodconfig (https://download.01.org/0day-ci/archive/20230503/202305031640.SL6o9vFH-lkp@intel.com/config)
compiler: clang version 14.0.6 (https://github.com/llvm/llvm-project f28c006a5895fc0e329fe15fead81e37457cb1d1)
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/intel-lab-lkp/linux/commit/3d6f729d86a79adf0603717c79bc389a5dcc4444
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review Naresh-Solanki/hwmon-max597x-Add-Maxim-Max597x/20230426-170556
        git checkout 3d6f729d86a79adf0603717c79bc389a5dcc4444
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=i386 olddefconfig
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=i386 SHELL=/bin/bash drivers/hwmon/

If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <lkp@intel.com>
| Link: https://lore.kernel.org/oe-kbuild-all/202305031640.SL6o9vFH-lkp@intel.com/

All warnings (new ones prefixed by >>):

>> drivers/hwmon/max597x.c:47:10: warning: 9 enumeration values not handled in switch: 'hwmon_chip', 'hwmon_temp', 'hwmon_power'... [-Wswitch]
           switch (type) {
                   ^~~~
   1 warning generated.


vim +47 drivers/hwmon/max597x.c

    40	
    41	static int max597x_read(struct device *dev, enum hwmon_sensor_types type,
    42				u32 attr, int channel, long *val)
    43	{
    44		struct max597x_hwmon *ddata = dev_get_drvdata(dev);
    45		int ret;
    46	
  > 47		switch (type) {
    48		case hwmon_curr:
    49			switch (attr) {
    50			case hwmon_curr_input:
    51				ret = max597x_read_reg(ddata, MAX5970_REG_CURRENT_H(channel),
    52						       ddata->irng[channel], val);
    53				if (ret < 0)
    54					return ret;
    55	
    56				return 0;
    57			default:
    58				return -EOPNOTSUPP;
    59			}
    60	
    61		case hwmon_in:
    62			switch (attr) {
    63			case hwmon_in_input:
    64				ret = max597x_read_reg(ddata, MAX5970_REG_VOLTAGE_H(channel),
    65						       ddata->mon_rng[channel], val);
    66				if (ret < 0)
    67					return ret;
    68				return 0;
    69			default:
    70				return -EOPNOTSUPP;
    71			}
    72		}
    73		return -EOPNOTSUPP;
    74	}
    75
diff mbox series

Patch

diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
index 5b3b76477b0e..164d980d9de2 100644
--- a/drivers/hwmon/Kconfig
+++ b/drivers/hwmon/Kconfig
@@ -1097,6 +1097,15 @@  config SENSORS_MAX31760
 	  This driver can also be built as a module. If so, the module
 	  will be called max31760.
 
+config SENSORS_MAX597X
+	tristate "Maxim 597x power switch and monitor"
+	depends on I2C
+	depends on OF
+	select MFD_MAX597X
+	help
+	  This driver exposes Maxim 5970/5978 voltage/current monitoring
+	  interface.
+
 config SENSORS_MAX6620
 	tristate "Maxim MAX6620 fan controller"
 	depends on I2C
diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile
index 88712b5031c8..720eb7d5fe46 100644
--- a/drivers/hwmon/Makefile
+++ b/drivers/hwmon/Makefile
@@ -142,6 +142,7 @@  obj-$(CONFIG_SENSORS_MAX197)	+= max197.o
 obj-$(CONFIG_SENSORS_MAX31722)	+= max31722.o
 obj-$(CONFIG_SENSORS_MAX31730)	+= max31730.o
 obj-$(CONFIG_SENSORS_MAX31760)  += max31760.o
+obj-$(CONFIG_SENSORS_MAX597X)	+= max597x.o
 obj-$(CONFIG_SENSORS_MAX6620)	+= max6620.o
 obj-$(CONFIG_SENSORS_MAX6621)	+= max6621.o
 obj-$(CONFIG_SENSORS_MAX6639)	+= max6639.o
diff --git a/drivers/hwmon/max597x.c b/drivers/hwmon/max597x.c
new file mode 100644
index 000000000000..d4d8c2faf55c
--- /dev/null
+++ b/drivers/hwmon/max597x.c
@@ -0,0 +1,212 @@ 
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Device driver for regulators in MAX5970 and MAX5978 IC
+ *
+ * Copyright (c) 2022 9elements GmbH
+ *
+ * Author: Patrick Rudolph <patrick.rudolph@9elements.com>
+ */
+
+#include <linux/hwmon.h>
+#include <linux/i2c.h>
+#include <linux/mfd/max597x.h>
+#include <linux/platform_device.h>
+#include <linux/regmap.h>
+
+struct max597x_hwmon {
+	int num_switches, irng[MAX5970_NUM_SWITCHES], mon_rng[MAX5970_NUM_SWITCHES];
+	struct regmap *regmap;
+};
+
+static int max597x_read_reg(struct max597x_hwmon *ddata, int reg, int range, long *val)
+{
+	u8 reg_data[2];
+	int ret;
+
+	ret = regmap_bulk_read(ddata->regmap, reg, &reg_data[0], 2);
+	if (ret < 0)
+		return ret;
+
+	*val = (reg_data[0] << 2) | (reg_data[1] & 3);
+	*val = *val * range;
+	/*
+	 * From datasheet, the range is fractionally less.
+	 * To compensate that, divide with 1033 number.
+	 */
+	*val = *val / 1033;
+
+	return 0;
+}
+
+static int max597x_read(struct device *dev, enum hwmon_sensor_types type,
+			u32 attr, int channel, long *val)
+{
+	struct max597x_hwmon *ddata = dev_get_drvdata(dev);
+	int ret;
+
+	switch (type) {
+	case hwmon_curr:
+		switch (attr) {
+		case hwmon_curr_input:
+			ret = max597x_read_reg(ddata, MAX5970_REG_CURRENT_H(channel),
+					       ddata->irng[channel], val);
+			if (ret < 0)
+				return ret;
+
+			return 0;
+		default:
+			return -EOPNOTSUPP;
+		}
+
+	case hwmon_in:
+		switch (attr) {
+		case hwmon_in_input:
+			ret = max597x_read_reg(ddata, MAX5970_REG_VOLTAGE_H(channel),
+					       ddata->mon_rng[channel], val);
+			if (ret < 0)
+				return ret;
+			return 0;
+		default:
+			return -EOPNOTSUPP;
+		}
+	}
+	return -EOPNOTSUPP;
+}
+
+static umode_t max597x_is_visible(const void *data,
+				  enum hwmon_sensor_types type,
+				  u32 attr, int channel)
+{
+	struct max597x_hwmon *ddata = (struct max597x_hwmon *)data;
+
+	if (channel >= ddata->num_switches)
+		return 0;
+
+	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;
+	default:
+		break;
+	}
+	return 0;
+}
+
+static const struct hwmon_ops max597x_hwmon_ops = {
+	.is_visible = max597x_is_visible,
+	.read = max597x_read,
+};
+
+#define HWMON_CURRENT	HWMON_C_INPUT
+#define HWMON_VOLTAGE	HWMON_I_INPUT
+
+static const struct hwmon_channel_info *max597x_info[] = {
+	HWMON_CHANNEL_INFO(in, HWMON_VOLTAGE, HWMON_VOLTAGE),
+	HWMON_CHANNEL_INFO(curr, HWMON_CURRENT, HWMON_CURRENT),
+	NULL
+};
+
+static const struct hwmon_chip_info max597x_chip_info = {
+	.ops = &max597x_hwmon_ops,
+	.info = max597x_info,
+};
+
+static int max597x_adc_range(struct regmap *regmap, const int ch,
+			     u32 *irng, u32 *mon_rng)
+{
+	unsigned int reg;
+	int ret;
+
+	/* Decode current ADC range */
+	ret = regmap_read(regmap, MAX5970_REG_STATUS2, &reg);
+	if (ret)
+		return ret;
+	switch (MAX5970_IRNG(reg, ch)) {
+	case 0:
+		*irng = 100000;	/* 100 mV */
+		break;
+	case 1:
+		*irng = 50000;	/* 50 mV */
+		break;
+	case 2:
+		*irng = 25000;	/* 25 mV */
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	/* Decode current voltage monitor range */
+	ret = regmap_read(regmap, MAX5970_REG_MON_RANGE, &reg);
+	if (ret)
+		return ret;
+
+	*mon_rng = MAX5970_MON_MAX_RANGE_UV >> MAX5970_MON(reg, ch);
+	*mon_rng /= 1000; /* uV to mV */
+
+	return 0;
+}
+
+static int max597x_sensor_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct i2c_client *i2c = to_i2c_client(pdev->dev.parent);
+	struct max597x_hwmon *ddata;
+	struct regmap *regmap = dev_get_regmap(pdev->dev.parent, NULL);
+	struct device *hwmon_dev;
+	int err;
+
+	if (!regmap)
+		return -EPROBE_DEFER;
+
+	ddata = devm_kzalloc(dev, sizeof(struct max597x_hwmon), GFP_KERNEL);
+	if (!ddata)
+		return -ENOMEM;
+
+	if (of_device_is_compatible(i2c->dev.of_node, "maxim,max5978"))
+		ddata->num_switches = MAX597x_TYPE_MAX5978;
+	else if (of_device_is_compatible(i2c->dev.of_node, "maxim,max5970"))
+		ddata->num_switches = MAX597x_TYPE_MAX5970;
+	else
+		return -ENODEV;
+
+	ddata->regmap = regmap;
+
+	for (int i = 0; i < ddata->num_switches; i++) {
+		err = max597x_adc_range(regmap, i, &ddata->irng[i], &ddata->mon_rng[i]);
+		if (err < 0)
+			return err;
+	}
+
+	hwmon_dev = devm_hwmon_device_register_with_info(dev,
+							 "max597x_hwmon", ddata,
+							 &max597x_chip_info, NULL);
+
+	if (IS_ERR(hwmon_dev)) {
+		err = PTR_ERR(hwmon_dev);
+		dev_err(dev, "Unable to register hwmon device, returned %d\n", err);
+		return err;
+	}
+
+	return 0;
+}
+
+static struct platform_driver max597x_sensor_driver = {
+	.probe = max597x_sensor_probe,
+	.driver = {
+		.name = "max597x-hwmon",
+	},
+};
+module_platform_driver(max597x_sensor_driver);
+
+MODULE_AUTHOR("Patrick Rudolph <patrick.rudolph@9elements.com>");
+MODULE_DESCRIPTION("MAX5970_hot-swap controller driver");
+MODULE_LICENSE("GPL v2");