From: Patrick Rudolph <[email protected]>
Add support for the Maxim Max59x power switch with current/voltage
monitor.
Signed-off-by: Patrick Rudolph <[email protected]>
Signed-off-by: Naresh Solanki <[email protected]>
---
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 <[email protected]>
+ */
+
+#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, ®_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, ®);
+ 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, ®);
+ 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 <[email protected]>");
+MODULE_DESCRIPTION("MAX5970_hot-swap controller driver");
+MODULE_LICENSE("GPL v2");
base-commit: b4c288cfd2f84c44994330c408e14645d45dee5b
--
2.39.1
Le 26/04/2023 à 11:03, Naresh Solanki a écrit :
> From: Patrick Rudolph <[email protected]>
>
> Add support for the Maxim Max59x power switch with current/voltage
> monitor.
>
> Signed-off-by: Patrick Rudolph <[email protected]>
> Signed-off-by: Naresh Solanki <[email protected]>
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 <[email protected]>
> + */
> +
> +#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, ®_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, ®);
> + 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, ®);
> + 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 <[email protected]>");
> +MODULE_DESCRIPTION("MAX5970_hot-swap controller driver");
> +MODULE_LICENSE("GPL v2");
>
> base-commit: b4c288cfd2f84c44994330c408e14645d45dee5b
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/[email protected]/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 <[email protected]>
| Link: https://lore.kernel.org/oe-kbuild-all/[email protected]/
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
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests
On Wed, Apr 26, 2023 at 11:03:55AM +0200, Naresh Solanki wrote:
> From: Patrick Rudolph <[email protected]>
>
> Add support for the Maxim Max59x power switch with current/voltage
> monitor.
>
> Signed-off-by: Patrick Rudolph <[email protected]>
> Signed-off-by: Naresh Solanki <[email protected]>
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 <[email protected]>
> + */
> +
> +#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, ®_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, ®);
> + 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, ®);
> + 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 <[email protected]>");
> +MODULE_DESCRIPTION("MAX5970_hot-swap controller driver");
> +MODULE_LICENSE("GPL v2");
>
> base-commit: b4c288cfd2f84c44994330c408e14645d45dee5b
> --
> 2.39.1
>
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 <[email protected]>
>>
>> Add support for the Maxim Max59x power switch with current/voltage
>> monitor.
>>
>> Signed-off-by: Patrick Rudolph <[email protected]>
>> Signed-off-by: Naresh Solanki <[email protected]>
>
> 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 <[email protected]>
>> + */
>> +
>> +#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, ®_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, ®);
>> + 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, ®);
>> + 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 <[email protected]>");
>> +MODULE_DESCRIPTION("MAX5970_hot-swap controller driver");
>> +MODULE_LICENSE("GPL v2");
>>
>> base-commit: b4c288cfd2f84c44994330c408e14645d45dee5b
>> --
>> 2.39.1
>>
Regards,
Naresh
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 <[email protected]>
>>>
>>> Add support for the Maxim Max59x power switch with current/voltage
>>> monitor.
>>>
>>> Signed-off-by: Patrick Rudolph <[email protected]>
>>> Signed-off-by: Naresh Solanki <[email protected]>
>>
>> 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 <[email protected]>
>>> + */
>>> +
>>> +#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, ®_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, ®);
>>> + 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, ®);
>>> + 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 <[email protected]>");
>>> +MODULE_DESCRIPTION("MAX5970_hot-swap controller driver");
>>> +MODULE_LICENSE("GPL v2");
>>>
>>> base-commit: b4c288cfd2f84c44994330c408e14645d45dee5b
>>> --
>>> 2.39.1
>>>
> Regards,
> Naresh
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
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/[email protected]/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 <[email protected]>
| Link: https://lore.kernel.org/oe-kbuild-all/[email protected]/
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
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests