[PATCH v3 1/2] iio: max597x: Add support for max597x

[PATCH v3 1/2] iio: max597x: Add support for max597x

[Naresh Solanki]

From: Patrick Rudolph <patrick.rudolph@9elements.com>

max5970 & max5978 has 10bit ADC for voltage & current
monitoring.
Use iio framework to expose the same in sysfs.

Signed-off-by: Patrick Rudolph <patrick.rudolph@9elements.com>
Signed-off-by: Naresh Solanki <Naresh.Solanki@9elements.com>
...
Changes in V3:
- Use bulk read
- Remove line split
Changes in V2:
- Remove fallthrough
- Use pdev->dev instead of i2c->dev
- Init indio_dev->name based on device type.
---
 drivers/iio/adc/Kconfig       |  15 ++++
 drivers/iio/adc/Makefile      |   1 +
 drivers/iio/adc/max597x-iio.c | 148 ++++++++++++++++++++++++++++++++++
 3 files changed, 164 insertions(+)
 create mode 100644 drivers/iio/adc/max597x-iio.c

diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
index 45af2302be53..69310af5c665 100644
--- a/drivers/iio/adc/Kconfig
+++ b/drivers/iio/adc/Kconfig
@@ -735,6 +735,21 @@ config MAX1363
 	  To compile this driver as a module, choose M here: the module will be
 	  called max1363.
 
+config MAX597X_IIO
+	tristate "Maxim 597x power switch and monitor"
+	depends on I2C && OF
+	select MFD_MAX597X
+	help
+	  This driver enables support for the Maxim 5970 & 5978 smart switch
+	  and voltage/current monitoring interface using the Industrial I/O
+	  (IIO) framework. The Maxim 597x is a power switch and monitor that can
+	  provide voltage and current measurements via the I2C bus. Enabling
+	  this driver will allow user space applications to read the voltage
+	  and current measurements using IIO interfaces.
+
+	  To compile this driver as a module, choose M here: the module will be
+	  called max597x-iio.
+
 config MAX9611
 	tristate "Maxim max9611/max9612 ADC driver"
 	depends on I2C
diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile
index 36c18177322a..7ec0c2cf7bbb 100644
--- a/drivers/iio/adc/Makefile
+++ b/drivers/iio/adc/Makefile
@@ -67,6 +67,7 @@ obj-$(CONFIG_MAX11205) += max11205.o
 obj-$(CONFIG_MAX11410) += max11410.o
 obj-$(CONFIG_MAX1241) += max1241.o
 obj-$(CONFIG_MAX1363) += max1363.o
+obj-$(CONFIG_MAX597X_IIO) += max597x-iio.o
 obj-$(CONFIG_MAX9611) += max9611.o
 obj-$(CONFIG_MCP320X) += mcp320x.o
 obj-$(CONFIG_MCP3422) += mcp3422.o
diff --git a/drivers/iio/adc/max597x-iio.c b/drivers/iio/adc/max597x-iio.c
new file mode 100644
index 000000000000..f158e49b5a56
--- /dev/null
+++ b/drivers/iio/adc/max597x-iio.c
@@ -0,0 +1,148 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Device driver for IIO in MAX5970 and MAX5978 IC
+ *
+ * Copyright (c) 2022 9elements GmbH
+ *
+ * Author: Patrick Rudolph <patrick.rudolph@9elements.com>
+ */
+
+#include <linux/iio/iio.h>
+#include <linux/mfd/max597x.h>
+#include <linux/platform_device.h>
+#include <linux/regmap.h>
+
+struct max597x_iio {
+	struct regmap *regmap;
+	int shunt_micro_ohms[MAX5970_NUM_SWITCHES];
+	unsigned int irng[MAX5970_NUM_SWITCHES];
+	unsigned int mon_rng[MAX5970_NUM_SWITCHES];
+};
+
+#define MAX597X_ADC_CHANNEL(_idx, _type) {			\
+	.type = IIO_ ## _type,					\
+	.indexed = 1,						\
+	.channel = (_idx),					\
+	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |		\
+			      BIT(IIO_CHAN_INFO_SCALE),		\
+	.address = MAX5970_REG_ ## _type ## _L(_idx),		\
+}
+
+static const struct iio_chan_spec max5978_adc_iio_channels[] = {
+	MAX597X_ADC_CHANNEL(0, VOLTAGE),
+	MAX597X_ADC_CHANNEL(0, CURRENT),
+};
+
+static const struct iio_chan_spec max5970_adc_iio_channels[] = {
+	MAX597X_ADC_CHANNEL(0, VOLTAGE),
+	MAX597X_ADC_CHANNEL(0, CURRENT),
+	MAX597X_ADC_CHANNEL(1, VOLTAGE),
+	MAX597X_ADC_CHANNEL(1, CURRENT),
+};
+
+static int max597x_iio_read_raw(struct iio_dev *iio_dev,
+				struct iio_chan_spec const *chan,
+				int *val, int *val2, long info)
+{
+	int ret;
+	struct max597x_iio *data = iio_priv(iio_dev);
+	u16 reg_l, reg_h;
+
+	switch (info) {
+	case IIO_CHAN_INFO_RAW:
+		ret = regmap_bulk_read(data->regmap, chan->address - 1, &reg_l, 2);
+		if (ret < 0)
+			return ret;
+		reg_h = reg_l & 0xff;
+		reg_l = (reg_l >> 8) & 0xff;
+		*val = (reg_h << 2) | (reg_l & 3);
+		return IIO_VAL_INT;
+
+	case IIO_CHAN_INFO_SCALE:
+		switch (chan->address) {
+		case MAX5970_REG_CURRENT_L(0):
+		case MAX5970_REG_CURRENT_L(1):
+			/* in A, convert to mA */
+			*val = data->irng[chan->channel] * 1000;
+			*val2 = data->shunt_micro_ohms[chan->channel] * ADC_MASK;
+			return IIO_VAL_FRACTIONAL;
+
+		case MAX5970_REG_VOLTAGE_L(0):
+		case MAX5970_REG_VOLTAGE_L(1):
+			/* in uV, convert to mV */
+			*val = data->mon_rng[chan->channel];
+			*val2 = ADC_MASK * 1000;
+			return IIO_VAL_FRACTIONAL;
+		}
+
+		break;
+	}
+	return -EINVAL;
+}
+
+static const struct iio_info max597x_adc_iio_info = {
+	.read_raw = &max597x_iio_read_raw,
+};
+
+static int max597x_iio_probe(struct platform_device *pdev)
+{
+	struct max597x_data *max597x = dev_get_drvdata(pdev->dev.parent);
+	struct regmap *regmap = dev_get_regmap(pdev->dev.parent, NULL);
+	struct iio_dev *indio_dev;
+	struct max597x_iio *priv;
+	int ret, i;
+
+	if (!regmap)
+		return -EPROBE_DEFER;
+
+	if (!max597x || !max597x->num_switches)
+		return -EPROBE_DEFER;
+
+	indio_dev = devm_iio_device_alloc(&pdev->dev, sizeof(*priv));
+	if (!indio_dev)
+		return dev_err_probe(&pdev->dev, -ENOMEM,
+				     "failed to allocate iio device\n");
+
+	indio_dev->info = &max597x_adc_iio_info;
+	indio_dev->modes = INDIO_DIRECT_MODE;
+
+	switch (max597x->num_switches) {
+	case MAX597x_TYPE_MAX5970:
+		indio_dev->channels = max5970_adc_iio_channels;
+		indio_dev->num_channels = ARRAY_SIZE(max5970_adc_iio_channels);
+		indio_dev->name = "max5970";
+		break;
+	case MAX597x_TYPE_MAX5978:
+		indio_dev->channels = max5978_adc_iio_channels;
+		indio_dev->num_channels = ARRAY_SIZE(max5978_adc_iio_channels);
+		indio_dev->name = "max5978";
+		break;
+	}
+
+	priv = iio_priv(indio_dev);
+	priv->regmap = regmap;
+	for (i = 0; i < indio_dev->num_channels; i++) {
+		priv->irng[i] = max597x->irng[i];
+		priv->mon_rng[i] = max597x->mon_rng[i];
+		priv->shunt_micro_ohms[i] = max597x->shunt_micro_ohms[i];
+	}
+
+	ret = devm_iio_device_register(&pdev->dev, indio_dev);
+	if (ret)
+		return dev_err_probe(&pdev->dev, ret, "could not register iio device\n");
+
+	return 0;
+}
+
+static struct platform_driver max597x_iio_driver = {
+	.driver = {
+		.name = "max597x-iio",
+	},
+	.probe = max597x_iio_probe,
+};
+
+module_platform_driver(max597x_iio_driver);
+
+MODULE_AUTHOR("Patrick Rudolph <patrick.rudolph@9elements.com>");
+MODULE_DESCRIPTION("MAX5970_hot-swap controller driver");
+MODULE_LICENSE("GPL");

base-commit: 368eb79f738a21e16c2bdbcac2444dfa96b01aaa
-- 
2.39.1

[PATCH v3 2/2] leds: max597x: Add support for max597x

[Naresh Solanki]

From: Patrick Rudolph <patrick.rudolph@9elements.com>

max597x is hot swap controller with indicator LED support.
This driver uses DT property to configure led during boot time &
also provide the LED control in sysfs.

DTS example:
    i2c {
        #address-cells = <1>;
        #size-cells = <0>;
        regulator@3a {
            compatible = "maxim,max5978";
            reg = <0x3a>;
            vss1-supply = <&p3v3>;

            regulators {
                sw0_ref_0: sw0 {
                    shunt-resistor-micro-ohms = <12000>;
                };
            };

            leds {
                #address-cells = <1>;
                #size-cells = <0>;
                led@0 {
                    reg = <0>;
                    label = "led0";
                    default-state = "on";
                };
                led@1 {
                    reg = <1>;
                    label = "led1";
                    default-state = "on";
                };
            };
        };
    };

Signed-off-by: Patrick Rudolph <patrick.rudolph@9elements.com>
Signed-off-by: Naresh Solanki <Naresh.Solanki@9elements.com>
...
Changes in V3:
- Remove of_node_put as its handled by for loop
- Print error if an LED fails to register.
- Update driver name in Kconfig description
- Remove unneeded variable assignment
- Use devm_led_classdev_register to reget led
Changes in V2:
- Fix regmap update
- Remove devm_kfree
- Remove default-state
- Add example dts in commit message
- Fix whitespace in Kconfig
- Fix comment
---
 drivers/leds/Kconfig        |  11 ++++
 drivers/leds/Makefile       |   1 +
 drivers/leds/leds-max597x.c | 112 ++++++++++++++++++++++++++++++++++++
 3 files changed, 124 insertions(+)
 create mode 100644 drivers/leds/leds-max597x.c

diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
index 9dbce09eabac..60004cb8c257 100644
--- a/drivers/leds/Kconfig
+++ b/drivers/leds/Kconfig
@@ -590,6 +590,17 @@ config LEDS_ADP5520
 	  To compile this driver as a module, choose M here: the module will
 	  be called leds-adp5520.
 
+config LEDS_MAX597X
+	tristate "LED Support for Maxim 597x"
+	depends on LEDS_CLASS
+	depends on MFD_MAX597X
+	help
+	  This option enables support for the Maxim MAX5970 & MAX5978 smart
+	  switch indication LEDs via the I2C bus.
+
+	  To compile this driver as a module, choose M here: the module will
+	  be called leds-max597x.
+
 config LEDS_MC13783
 	tristate "LED Support for MC13XXX PMIC"
 	depends on LEDS_CLASS
diff --git a/drivers/leds/Makefile b/drivers/leds/Makefile
index d30395d11fd8..da1192e40268 100644
--- a/drivers/leds/Makefile
+++ b/drivers/leds/Makefile
@@ -53,6 +53,7 @@ obj-$(CONFIG_LEDS_LP8501)		+= leds-lp8501.o
 obj-$(CONFIG_LEDS_LP8788)		+= leds-lp8788.o
 obj-$(CONFIG_LEDS_LP8860)		+= leds-lp8860.o
 obj-$(CONFIG_LEDS_LT3593)		+= leds-lt3593.o
+obj-$(CONFIG_LEDS_MAX597X)		+= leds-max597x.o
 obj-$(CONFIG_LEDS_MAX77650)		+= leds-max77650.o
 obj-$(CONFIG_LEDS_MAX8997)		+= leds-max8997.o
 obj-$(CONFIG_LEDS_MC13783)		+= leds-mc13783.o
diff --git a/drivers/leds/leds-max597x.c b/drivers/leds/leds-max597x.c
new file mode 100644
index 000000000000..83e4dfb617fb
--- /dev/null
+++ b/drivers/leds/leds-max597x.c
@@ -0,0 +1,112 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Device driver for leds in MAX5970 and MAX5978 IC
+ *
+ * Copyright (c) 2022 9elements GmbH
+ *
+ * Author: Patrick Rudolph <patrick.rudolph@9elements.com>
+ */
+
+#include <linux/leds.h>
+#include <linux/mfd/max597x.h>
+#include <linux/of.h>
+#include <linux/platform_device.h>
+#include <linux/regmap.h>
+
+#define ldev_to_maxled(c)       container_of(c, struct max597x_led, led)
+
+struct max597x_led {
+	struct regmap *regmap;
+	struct led_classdev led;
+	unsigned int index;
+};
+
+static int max597x_led_set_brightness(struct led_classdev *cdev,
+				      enum led_brightness brightness)
+{
+	struct max597x_led *led = ldev_to_maxled(cdev);
+	int ret, val = 0;
+
+	if (!led || !led->regmap)
+		return -ENODEV;
+
+	val = !brightness ? BIT(led->index) : 0;
+	ret = regmap_update_bits(led->regmap, MAX5970_REG_LED_FLASH, BIT(led->index), val);
+	if (ret < 0)
+		dev_err(cdev->dev, "failed to set brightness %d\n", ret);
+	return ret;
+}
+
+static int max597x_setup_led(struct device *dev, struct regmap *regmap, struct device_node *nc,
+			     u32 reg)
+{
+	struct max597x_led *led;
+	int ret;
+
+	led = devm_kzalloc(dev, sizeof(struct max597x_led),
+			   GFP_KERNEL);
+	if (!led)
+		return -ENOMEM;
+
+	if (of_property_read_string(nc, "label", &led->led.name))
+		led->led.name = nc->name;
+
+	led->led.max_brightness = 1;
+	led->led.brightness_set_blocking = max597x_led_set_brightness;
+	led->led.default_trigger = "none";
+	led->index = reg;
+	led->regmap = regmap;
+	ret = devm_led_classdev_register(dev, &led->led);
+	if (ret)
+		dev_err(dev, "Error in initializing led %s", led->led.name);
+
+	return ret;
+}
+
+static int max597x_led_probe(struct platform_device *pdev)
+{
+	struct device_node *np = dev_of_node(pdev->dev.parent);
+	struct regmap *regmap = dev_get_regmap(pdev->dev.parent, NULL);
+	struct device_node *led_node;
+	struct device_node *child;
+	int ret = 0;
+
+	if (!regmap)
+		return -EPROBE_DEFER;
+
+	led_node = of_get_child_by_name(np, "leds");
+	if (!led_node)
+		return -ENODEV;
+
+	for_each_available_child_of_node(led_node, child) {
+		u32 reg;
+
+		if (of_property_read_u32(child, "reg", &reg))
+			continue;
+
+		if (reg >= MAX597X_NUM_LEDS) {
+			dev_err(&pdev->dev, "invalid LED (%u >= %d)\n", reg,
+				MAX597X_NUM_LEDS);
+			continue;
+		}
+
+		ret = max597x_setup_led(&pdev->dev, regmap, child, reg);
+		if (ret < 0)
+			dev_err(&pdev->dev, "Failed to initialize LED %u\n", reg);
+	}
+
+	return ret;
+}
+
+static struct platform_driver max597x_led_driver = {
+	.driver = {
+		.name = "max597x-led",
+	},
+	.probe = max597x_led_probe,
+};
+
+module_platform_driver(max597x_led_driver);
+
+MODULE_AUTHOR("Patrick Rudolph <patrick.rudolph@9elements.com>");
+MODULE_DESCRIPTION("MAX5970_hot-swap controller driver");
+MODULE_LICENSE("GPL");
-- 
2.39.1

Re: [PATCH v3 2/2] leds: max597x: Add support for max597x

[Lee Jones]

On Tue, 28 Mar 2023, Naresh Solanki wrote:

> From: Patrick Rudolph <patrick.rudolph@9elements.com>
>
> max597x is hot swap controller with indicator LED support.
> This driver uses DT property to configure led during boot time &
> also provide the LED control in sysfs.
>
> DTS example:
>     i2c {
>         #address-cells = <1>;
>         #size-cells = <0>;
>         regulator@3a {
>             compatible = "maxim,max5978";
>             reg = <0x3a>;
>             vss1-supply = <&p3v3>;
>
>             regulators {
>                 sw0_ref_0: sw0 {
>                     shunt-resistor-micro-ohms = <12000>;
>                 };
>             };
>
>             leds {
>                 #address-cells = <1>;
>                 #size-cells = <0>;
>                 led@0 {
>                     reg = <0>;
>                     label = "led0";
>                     default-state = "on";
>                 };
>                 led@1 {
>                     reg = <1>;
>                     label = "led1";
>                     default-state = "on";
>                 };
>             };
>         };
>     };
>
> Signed-off-by: Patrick Rudolph <patrick.rudolph@9elements.com>
> Signed-off-by: Naresh Solanki <Naresh.Solanki@9elements.com>
> ...
> Changes in V3:
> - Remove of_node_put as its handled by for loop
> - Print error if an LED fails to register.
> - Update driver name in Kconfig description
> - Remove unneeded variable assignment
> - Use devm_led_classdev_register to reget led
> Changes in V2:
> - Fix regmap update
> - Remove devm_kfree
> - Remove default-state
> - Add example dts in commit message
> - Fix whitespace in Kconfig
> - Fix comment
> ---
>  drivers/leds/Kconfig        |  11 ++++
>  drivers/leds/Makefile       |   1 +
>  drivers/leds/leds-max597x.c | 112 ++++++++++++++++++++++++++++++++++++
>  3 files changed, 124 insertions(+)
>  create mode 100644 drivers/leds/leds-max597x.c
>
> diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
> index 9dbce09eabac..60004cb8c257 100644
> --- a/drivers/leds/Kconfig
> +++ b/drivers/leds/Kconfig
> @@ -590,6 +590,17 @@ config LEDS_ADP5520
>  	  To compile this driver as a module, choose M here: the module will
>  	  be called leds-adp5520.
>
> +config LEDS_MAX597X
> +	tristate "LED Support for Maxim 597x"
> +	depends on LEDS_CLASS
> +	depends on MFD_MAX597X
> +	help
> +	  This option enables support for the Maxim MAX5970 & MAX5978 smart
> +	  switch indication LEDs via the I2C bus.
> +
> +	  To compile this driver as a module, choose M here: the module will
> +	  be called leds-max597x.
> +
>  config LEDS_MC13783
>  	tristate "LED Support for MC13XXX PMIC"
>  	depends on LEDS_CLASS
> diff --git a/drivers/leds/Makefile b/drivers/leds/Makefile
> index d30395d11fd8..da1192e40268 100644
> --- a/drivers/leds/Makefile
> +++ b/drivers/leds/Makefile
> @@ -53,6 +53,7 @@ obj-$(CONFIG_LEDS_LP8501)		+= leds-lp8501.o
>  obj-$(CONFIG_LEDS_LP8788)		+= leds-lp8788.o
>  obj-$(CONFIG_LEDS_LP8860)		+= leds-lp8860.o
>  obj-$(CONFIG_LEDS_LT3593)		+= leds-lt3593.o
> +obj-$(CONFIG_LEDS_MAX597X)		+= leds-max597x.o
>  obj-$(CONFIG_LEDS_MAX77650)		+= leds-max77650.o
>  obj-$(CONFIG_LEDS_MAX8997)		+= leds-max8997.o
>  obj-$(CONFIG_LEDS_MC13783)		+= leds-mc13783.o
> diff --git a/drivers/leds/leds-max597x.c b/drivers/leds/leds-max597x.c
> new file mode 100644
> index 000000000000..83e4dfb617fb
> --- /dev/null
> +++ b/drivers/leds/leds-max597x.c
> @@ -0,0 +1,112 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Device driver for leds in MAX5970 and MAX5978 IC
> + *
> + * Copyright (c) 2022 9elements GmbH
> + *
> + * Author: Patrick Rudolph <patrick.rudolph@9elements.com>
> + */
> +
> +#include <linux/leds.h>
> +#include <linux/mfd/max597x.h>
> +#include <linux/of.h>
> +#include <linux/platform_device.h>
> +#include <linux/regmap.h>
> +
> +#define ldev_to_maxled(c)       container_of(c, struct max597x_led, led)
> +
> +struct max597x_led {
> +	struct regmap *regmap;
> +	struct led_classdev led;
> +	unsigned int index;
> +};
> +
> +static int max597x_led_set_brightness(struct led_classdev *cdev,
> +				      enum led_brightness brightness)
> +{
> +	struct max597x_led *led = ldev_to_maxled(cdev);
> +	int ret, val = 0;

Why preinitialise?

> +	if (!led || !led->regmap)
> +		return -ENODEV;

Can !led actually happen?

> +	val = !brightness ? BIT(led->index) : 0;

Perhaps a comment?

> +	ret = regmap_update_bits(led->regmap, MAX5970_REG_LED_FLASH, BIT(led->index), val);
> +	if (ret < 0)
> +		dev_err(cdev->dev, "failed to set brightness %d\n", ret);

'\n'

> +	return ret;
> +}
> +
> +static int max597x_setup_led(struct device *dev, struct regmap *regmap, struct device_node *nc,
> +			     u32 reg)
> +{
> +	struct max597x_led *led;
> +	int ret;
> +
> +	led = devm_kzalloc(dev, sizeof(struct max597x_led),
> +			   GFP_KERNEL);

Consistently break at 100-chars please.

You have lines wayyyy longer than this elsewhere.

> +	if (!led)

'led' is confusing.  Either this or the member 'led' should be changed.

Perhaps ddata here and cdev for the member?

> +		return -ENOMEM;
> +
> +	if (of_property_read_string(nc, "label", &led->led.name))
> +		led->led.name = nc->name;
> +
> +	led->led.max_brightness = 1;
> +	led->led.brightness_set_blocking = max597x_led_set_brightness;
> +	led->led.default_trigger = "none";
> +	led->index = reg;
> +	led->regmap = regmap;
> +	ret = devm_led_classdev_register(dev, &led->led);
> +	if (ret)
> +		dev_err(dev, "Error in initializing led %s", led->led.name);

Drop the "in" and s/led/LED/

> +
> +	return ret;
> +}
> +
> +static int max597x_led_probe(struct platform_device *pdev)
> +{
> +	struct device_node *np = dev_of_node(pdev->dev.parent);

Why not have your own compatible string?

> +	struct regmap *regmap = dev_get_regmap(pdev->dev.parent, NULL);

These "big" API calls are usually done outside of the allocation block.

Please move it to just above the check for !regmap.

> +	struct device_node *led_node;
> +	struct device_node *child;
> +	int ret = 0;

Is it okay for an LED driver to not to register any LEDs?

Perhaps -ENODEV?

> +	if (!regmap)
> +		return -EPROBE_DEFER;
> +
> +	led_node = of_get_child_by_name(np, "leds");
> +	if (!led_node)
> +		return -ENODEV;

Ah, that's better.  So set ret to -ENODEV and use it here.

> +	for_each_available_child_of_node(led_node, child) {
> +		u32 reg;
> +
> +		if (of_property_read_u32(child, "reg", &reg))
> +			continue;
> +
> +		if (reg >= MAX597X_NUM_LEDS) {
> +			dev_err(&pdev->dev, "invalid LED (%u >= %d)\n", reg,
> +				MAX597X_NUM_LEDS);
> +			continue;
> +		}
> +
> +		ret = max597x_setup_led(&pdev->dev, regmap, child, reg);
> +		if (ret < 0)
> +			dev_err(&pdev->dev, "Failed to initialize LED %u\n", reg);

I think you (or I) are missing the point of the previous reviews.  It's
not okay to error out and continue executing.  Either this is okay (you
can warn and carry on) or it's not (return an error).  Your first
submission suggested that this was an error.  In which case you do need
to return.  I think Pavel was suggesting that you should unwind
(de-register) before retuning, rather than leaving things in an odd
half-registered state.  Not that you should blindly carry on as if the
issue never occurred.

> +	}
> +
> +	return ret;
> +}
> +
> +static struct platform_driver max597x_led_driver = {
> +	.driver = {
> +		.name = "max597x-led",
> +	},
> +	.probe = max597x_led_probe,
> +};
> +

Remove this line.
> +module_platform_driver(max597x_led_driver);
> +
> +MODULE_AUTHOR("Patrick Rudolph <patrick.rudolph@9elements.com>");
> +MODULE_DESCRIPTION("MAX5970_hot-swap controller driver");

Odd.  I thought this was a LED driver?

> +MODULE_LICENSE("GPL");
> --
> 2.39.1
>

--
Lee Jones [李琼斯]

Re: [PATCH v3 2/2] leds: max597x: Add support for max597x

[Naresh Solanki]

Hi Lee,

On 05-04-2023 08:37 pm, Lee Jones wrote:
> On Tue, 28 Mar 2023, Naresh Solanki wrote:
> 
>> From: Patrick Rudolph <patrick.rudolph@9elements.com>
>>
>> max597x is hot swap controller with indicator LED support.
>> This driver uses DT property to configure led during boot time &
>> also provide the LED control in sysfs.
>>
>> DTS example:
>>      i2c {
>>          #address-cells = <1>;
>>          #size-cells = <0>;
>>          regulator@3a {
>>              compatible = "maxim,max5978";
>>              reg = <0x3a>;
>>              vss1-supply = <&p3v3>;
>>
>>              regulators {
>>                  sw0_ref_0: sw0 {
>>                      shunt-resistor-micro-ohms = <12000>;
>>                  };
>>              };
>>
>>              leds {
>>                  #address-cells = <1>;
>>                  #size-cells = <0>;
>>                  led@0 {
>>                      reg = <0>;
>>                      label = "led0";
>>                      default-state = "on";
>>                  };
>>                  led@1 {
>>                      reg = <1>;
>>                      label = "led1";
>>                      default-state = "on";
>>                  };
>>              };
>>          };
>>      };
>>
>> Signed-off-by: Patrick Rudolph <patrick.rudolph@9elements.com>
>> Signed-off-by: Naresh Solanki <Naresh.Solanki@9elements.com>
>> ...
>> Changes in V3:
>> - Remove of_node_put as its handled by for loop
>> - Print error if an LED fails to register.
>> - Update driver name in Kconfig description
>> - Remove unneeded variable assignment
>> - Use devm_led_classdev_register to reget led
>> Changes in V2:
>> - Fix regmap update
>> - Remove devm_kfree
>> - Remove default-state
>> - Add example dts in commit message
>> - Fix whitespace in Kconfig
>> - Fix comment
>> ---
>>   drivers/leds/Kconfig        |  11 ++++
>>   drivers/leds/Makefile       |   1 +
>>   drivers/leds/leds-max597x.c | 112 ++++++++++++++++++++++++++++++++++++
>>   3 files changed, 124 insertions(+)
>>   create mode 100644 drivers/leds/leds-max597x.c
>>
>> diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
>> index 9dbce09eabac..60004cb8c257 100644
>> --- a/drivers/leds/Kconfig
>> +++ b/drivers/leds/Kconfig
>> @@ -590,6 +590,17 @@ config LEDS_ADP5520
>>   	  To compile this driver as a module, choose M here: the module will
>>   	  be called leds-adp5520.
>>
>> +config LEDS_MAX597X
>> +	tristate "LED Support for Maxim 597x"
>> +	depends on LEDS_CLASS
>> +	depends on MFD_MAX597X
>> +	help
>> +	  This option enables support for the Maxim MAX5970 & MAX5978 smart
>> +	  switch indication LEDs via the I2C bus.
>> +
>> +	  To compile this driver as a module, choose M here: the module will
>> +	  be called leds-max597x.
>> +
>>   config LEDS_MC13783
>>   	tristate "LED Support for MC13XXX PMIC"
>>   	depends on LEDS_CLASS
>> diff --git a/drivers/leds/Makefile b/drivers/leds/Makefile
>> index d30395d11fd8..da1192e40268 100644
>> --- a/drivers/leds/Makefile
>> +++ b/drivers/leds/Makefile
>> @@ -53,6 +53,7 @@ obj-$(CONFIG_LEDS_LP8501)		+= leds-lp8501.o
>>   obj-$(CONFIG_LEDS_LP8788)		+= leds-lp8788.o
>>   obj-$(CONFIG_LEDS_LP8860)		+= leds-lp8860.o
>>   obj-$(CONFIG_LEDS_LT3593)		+= leds-lt3593.o
>> +obj-$(CONFIG_LEDS_MAX597X)		+= leds-max597x.o
>>   obj-$(CONFIG_LEDS_MAX77650)		+= leds-max77650.o
>>   obj-$(CONFIG_LEDS_MAX8997)		+= leds-max8997.o
>>   obj-$(CONFIG_LEDS_MC13783)		+= leds-mc13783.o
>> diff --git a/drivers/leds/leds-max597x.c b/drivers/leds/leds-max597x.c
>> new file mode 100644
>> index 000000000000..83e4dfb617fb
>> --- /dev/null
>> +++ b/drivers/leds/leds-max597x.c
>> @@ -0,0 +1,112 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * Device driver for leds in MAX5970 and MAX5978 IC
>> + *
>> + * Copyright (c) 2022 9elements GmbH
>> + *
>> + * Author: Patrick Rudolph <patrick.rudolph@9elements.com>
>> + */
>> +
>> +#include <linux/leds.h>
>> +#include <linux/mfd/max597x.h>
>> +#include <linux/of.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/regmap.h>
>> +
>> +#define ldev_to_maxled(c)       container_of(c, struct max597x_led, led)
>> +
>> +struct max597x_led {
>> +	struct regmap *regmap;
>> +	struct led_classdev led;
>> +	unsigned int index;
>> +};
>> +
>> +static int max597x_led_set_brightness(struct led_classdev *cdev,
>> +				      enum led_brightness brightness)
>> +{
>> +	struct max597x_led *led = ldev_to_maxled(cdev);
>> +	int ret, val = 0;
> 
> Why preinitialise?
Ack. Not needed so will remove.
> 
>> +	if (!led || !led->regmap)
>> +		return -ENODEV;
> 
> Can !led actually happen?
Ack. Will remove !led .
> 
>> +	val = !brightness ? BIT(led->index) : 0;
> 
> Perhaps a comment?
Sure. Will add "Set/Clear Led index bit"
> 
>> +	ret = regmap_update_bits(led->regmap, MAX5970_REG_LED_FLASH, BIT(led->index), val);
>> +	if (ret < 0)
>> +		dev_err(cdev->dev, "failed to set brightness %d\n", ret);
> 
> '\n'
Ack
> 
>> +	return ret;
>> +}
>> +
>> +static int max597x_setup_led(struct device *dev, struct regmap *regmap, struct device_node *nc,
>> +			     u32 reg)
>> +{
>> +	struct max597x_led *led;
>> +	int ret;
>> +
>> +	led = devm_kzalloc(dev, sizeof(struct max597x_led),
>> +			   GFP_KERNEL);
> 
> Consistently break at 100-chars please.
> 
> You have lines wayyyy longer than this elsewhere.
Ack. Will align with 100-chars. Thanks
> 
>> +	if (!led)
> 
> 'led' is confusing.  Either this or the member 'led' should be changed.
> 
> Perhaps ddata here and cdev for the member?
Sure.
> 
>> +		return -ENOMEM;
>> +
>> +	if (of_property_read_string(nc, "label", &led->led.name))
>> +		led->led.name = nc->name;
>> +
>> +	led->led.max_brightness = 1;
>> +	led->led.brightness_set_blocking = max597x_led_set_brightness;
>> +	led->led.default_trigger = "none";
>> +	led->index = reg;
>> +	led->regmap = regmap;
>> +	ret = devm_led_classdev_register(dev, &led->led);
>> +	if (ret)
>> +		dev_err(dev, "Error in initializing led %s", led->led.name);
> 
> Drop the "in" and s/led/LED/
Ack
> 
>> +
>> +	return ret;
>> +}
>> +
>> +static int max597x_led_probe(struct platform_device *pdev)
>> +{
>> +	struct device_node *np = dev_of_node(pdev->dev.parent);
> 
> Why not have your own compatible string?
This is leaf driver & MFD driver does has compatible string.
> 
>> +	struct regmap *regmap = dev_get_regmap(pdev->dev.parent, NULL);
> 
> These "big" API calls are usually done outside of the allocation block.
> 
> Please move it to just above the check for !regmap.
> 
>> +	struct device_node *led_node;
>> +	struct device_node *child;
>> +	int ret = 0;
> 
> Is it okay for an LED driver to not to register any LEDs?
Yes. Usage of indication LED on the max5970/5978 is optional.
> 
> Perhaps -ENODEV?
This driver is loaded only if MFD driver is included. remap is setup by 
MFD driver & hence defer probe till MFD driver is loaded.
> 
>> +	if (!regmap)
>> +		return -EPROBE_DEFER;
>> +
>> +	led_node = of_get_child_by_name(np, "leds");
>> +	if (!led_node)
>> +		return -ENODEV;
> 
> Ah, that's better.  So set ret to -ENODEV and use it here.
Yes.
> 
>> +	for_each_available_child_of_node(led_node, child) {
>> +		u32 reg;
>> +
>> +		if (of_property_read_u32(child, "reg", &reg))
>> +			continue;
>> +
>> +		if (reg >= MAX597X_NUM_LEDS) {
>> +			dev_err(&pdev->dev, "invalid LED (%u >= %d)\n", reg,
>> +				MAX597X_NUM_LEDS);
>> +			continue;
>> +		}
>> +
>> +		ret = max597x_setup_led(&pdev->dev, regmap, child, reg);
>> +		if (ret < 0)
>> +			dev_err(&pdev->dev, "Failed to initialize LED %u\n", reg);
> 
> I think you (or I) are missing the point of the previous reviews.  It's
> not okay to error out and continue executing.  Either this is okay (you
> can warn and carry on) or it's not (return an error).  Your first
> submission suggested that this was an error.  In which case you do need
> to return.  I think Pavel was suggesting that you should unwind
> (de-register) before retuning, rather than leaving things in an odd
> half-registered state.  Not that you should blindly carry on as if the
> issue never occurred.
I did refer to other such implementations & some have used return on 
error & some just print on error & continue. I felt that continue 
executing with warning(on error) is better approach.

> 
>> +	}
>> +
>> +	return ret;
>> +}
>> +
>> +static struct platform_driver max597x_led_driver = {
>> +	.driver = {
>> +		.name = "max597x-led",
>> +	},
>> +	.probe = max597x_led_probe,
>> +};
>> +
> 
> Remove this line.
>> +module_platform_driver(max597x_led_driver);
>> +
>> +MODULE_AUTHOR("Patrick Rudolph <patrick.rudolph@9elements.com>");
>> +MODULE_DESCRIPTION("MAX5970_hot-swap controller driver");
> 
> Odd.  I thought this was a LED driver?
This also has LED. will update the strign to:
MAX5970_hot-swap controller LED driver
> 
>> +MODULE_LICENSE("GPL");
>> --
>> 2.39.1
>>
> 
> --
> Lee Jones [李琼斯]
Regards,
Naresh

Re: [PATCH v3 2/2] leds: max597x: Add support for max597x

[Lee Jones]

On Wed, 12 Apr 2023, Naresh Solanki wrote:

> Hi Lee,
>
> On 05-04-2023 08:37 pm, Lee Jones wrote:
> > On Tue, 28 Mar 2023, Naresh Solanki wrote:
> >
> > > From: Patrick Rudolph <patrick.rudolph@9elements.com>
> > >
> > > max597x is hot swap controller with indicator LED support.
> > > This driver uses DT property to configure led during boot time &
> > > also provide the LED control in sysfs.
> > >
> > > DTS example:
> > >      i2c {
> > >          #address-cells = <1>;
> > >          #size-cells = <0>;
> > >          regulator@3a {
> > >              compatible = "maxim,max5978";
> > >              reg = <0x3a>;
> > >              vss1-supply = <&p3v3>;
> > >
> > >              regulators {
> > >                  sw0_ref_0: sw0 {
> > >                      shunt-resistor-micro-ohms = <12000>;
> > >                  };
> > >              };
> > >
> > >              leds {
> > >                  #address-cells = <1>;
> > >                  #size-cells = <0>;
> > >                  led@0 {
> > >                      reg = <0>;
> > >                      label = "led0";
> > >                      default-state = "on";
> > >                  };
> > >                  led@1 {
> > >                      reg = <1>;
> > >                      label = "led1";
> > >                      default-state = "on";
> > >                  };
> > >              };
> > >          };
> > >      };
> > >
> > > Signed-off-by: Patrick Rudolph <patrick.rudolph@9elements.com>
> > > Signed-off-by: Naresh Solanki <Naresh.Solanki@9elements.com>
> > > ...
> > > Changes in V3:
> > > - Remove of_node_put as its handled by for loop
> > > - Print error if an LED fails to register.
> > > - Update driver name in Kconfig description
> > > - Remove unneeded variable assignment
> > > - Use devm_led_classdev_register to reget led
> > > Changes in V2:
> > > - Fix regmap update
> > > - Remove devm_kfree
> > > - Remove default-state
> > > - Add example dts in commit message
> > > - Fix whitespace in Kconfig
> > > - Fix comment
> > > ---
> > >   drivers/leds/Kconfig        |  11 ++++
> > >   drivers/leds/Makefile       |   1 +
> > >   drivers/leds/leds-max597x.c | 112 ++++++++++++++++++++++++++++++++++++
> > >   3 files changed, 124 insertions(+)
> > >   create mode 100644 drivers/leds/leds-max597x.c

[...]

> > > +
> > > +static int max597x_led_probe(struct platform_device *pdev)
> > > +{
> > > +	struct device_node *np = dev_of_node(pdev->dev.parent);
> >
> > Why not have your own compatible string?
> This is leaf driver & MFD driver does has compatible string.

I can see that, but why not give this driver it's own one?

> > > +	struct regmap *regmap = dev_get_regmap(pdev->dev.parent, NULL);
> >
> > These "big" API calls are usually done outside of the allocation block.
> >
> > Please move it to just above the check for !regmap.
> >
> > > +	struct device_node *led_node;
> > > +	struct device_node *child;
> > > +	int ret = 0;
> >
> > Is it okay for an LED driver to not to register any LEDs?
> Yes. Usage of indication LED on the max5970/5978 is optional.
> >
> > Perhaps -ENODEV?
> This driver is loaded only if MFD driver is included. remap is setup by MFD
> driver & hence defer probe till MFD driver is loaded.
> >
> > > +	if (!regmap)
> > > +		return -EPROBE_DEFER;
> > > +
> > > +	led_node = of_get_child_by_name(np, "leds");
> > > +	if (!led_node)
> > > +		return -ENODEV;
> >
> > Ah, that's better.  So set ret to -ENODEV and use it here.
> Yes.
> >
> > > +	for_each_available_child_of_node(led_node, child) {
> > > +		u32 reg;
> > > +
> > > +		if (of_property_read_u32(child, "reg", &reg))
> > > +			continue;
> > > +
> > > +		if (reg >= MAX597X_NUM_LEDS) {
> > > +			dev_err(&pdev->dev, "invalid LED (%u >= %d)\n", reg,
> > > +				MAX597X_NUM_LEDS);
> > > +			continue;
> > > +		}
> > > +
> > > +		ret = max597x_setup_led(&pdev->dev, regmap, child, reg);
> > > +		if (ret < 0)
> > > +			dev_err(&pdev->dev, "Failed to initialize LED %u\n", reg);
> >
> > I think you (or I) are missing the point of the previous reviews.  It's
> > not okay to error out and continue executing.  Either this is okay (you
> > can warn and carry on) or it's not (return an error).  Your first
> > submission suggested that this was an error.  In which case you do need
> > to return.  I think Pavel was suggesting that you should unwind
> > (de-register) before retuning, rather than leaving things in an odd
> > half-registered state.  Not that you should blindly carry on as if the
> > issue never occurred.
> I did refer to other such implementations & some have used return on error &
> some just print on error & continue. I felt that continue executing with
> warning(on error) is better approach.

I think it should fail fast and with certainty.


--
Lee Jones [李琼斯]

Re: [PATCH v3 1/2] iio: max597x: Add support for max597x

[Jonathan Cameron]

On Tue, 28 Mar 2023 11:44:14 +0200
Naresh Solanki <naresh.solanki@9elements.com> wrote:

> From: Patrick Rudolph <patrick.rudolph@9elements.com>
> 
> max5970 & max5978 has 10bit ADC for voltage & current
> monitoring.
> Use iio framework to expose the same in sysfs.
> 
> Signed-off-by: Patrick Rudolph <patrick.rudolph@9elements.com>
> Signed-off-by: Naresh Solanki <Naresh.Solanki@9elements.com>
> ...

--- not ...

As I mentioned in my reply to v2 thread (which crossed with this v3)
I'd like this series to be cc'd to the list and maintainers for Hwmon
with a cover letter explaining the reasoning for it being an IIO driver
+ the restrictions that potentially brings.

A few other comments inline from taking another look.

Thanks,

Jonathan


> Changes in V3:
> - Use bulk read
> - Remove line split
> Changes in V2:
> - Remove fallthrough
> - Use pdev->dev instead of i2c->dev
> - Init indio_dev->name based on device type.
> ---
>  drivers/iio/adc/Kconfig       |  15 ++++
>  drivers/iio/adc/Makefile      |   1 +
>  drivers/iio/adc/max597x-iio.c | 148 ++++++++++++++++++++++++++++++++++
>  3 files changed, 164 insertions(+)
>  create mode 100644 drivers/iio/adc/max597x-iio.c
> 
> diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
> index 45af2302be53..69310af5c665 100644
> --- a/drivers/iio/adc/Kconfig
> +++ b/drivers/iio/adc/Kconfig
> @@ -735,6 +735,21 @@ config MAX1363
>  	  To compile this driver as a module, choose M here: the module will be
>  	  called max1363.
>  
> +config MAX597X_IIO
> +	tristate "Maxim 597x power switch and monitor"
> +	depends on I2C && OF
> +	select MFD_MAX597X
> +	help
> +	  This driver enables support for the Maxim 5970 & 5978 smart switch
> +	  and voltage/current monitoring interface using the Industrial I/O
> +	  (IIO) framework. The Maxim 597x is a power switch and monitor that can
> +	  provide voltage and current measurements via the I2C bus. Enabling
> +	  this driver will allow user space applications to read the voltage
> +	  and current measurements using IIO interfaces.
> +
> +	  To compile this driver as a module, choose M here: the module will be
> +	  called max597x-iio.
> +
>  config MAX9611
>  	tristate "Maxim max9611/max9612 ADC driver"
>  	depends on I2C
> diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile
> index 36c18177322a..7ec0c2cf7bbb 100644
> --- a/drivers/iio/adc/Makefile
> +++ b/drivers/iio/adc/Makefile
> @@ -67,6 +67,7 @@ obj-$(CONFIG_MAX11205) += max11205.o
>  obj-$(CONFIG_MAX11410) += max11410.o
>  obj-$(CONFIG_MAX1241) += max1241.o
>  obj-$(CONFIG_MAX1363) += max1363.o
> +obj-$(CONFIG_MAX597X_IIO) += max597x-iio.o
>  obj-$(CONFIG_MAX9611) += max9611.o
>  obj-$(CONFIG_MCP320X) += mcp320x.o
>  obj-$(CONFIG_MCP3422) += mcp3422.o
> diff --git a/drivers/iio/adc/max597x-iio.c b/drivers/iio/adc/max597x-iio.c
> new file mode 100644
> index 000000000000..f158e49b5a56
> --- /dev/null
> +++ b/drivers/iio/adc/max597x-iio.c
> @@ -0,0 +1,148 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Device driver for IIO in MAX5970 and MAX5978 IC
> + *
> + * Copyright (c) 2022 9elements GmbH
> + *
> + * Author: Patrick Rudolph <patrick.rudolph@9elements.com>
> + */
> +
> +#include <linux/iio/iio.h>
> +#include <linux/mfd/max597x.h>
> +#include <linux/platform_device.h>
> +#include <linux/regmap.h>
> +
> +struct max597x_iio {
> +	struct regmap *regmap;
> +	int shunt_micro_ohms[MAX5970_NUM_SWITCHES];
> +	unsigned int irng[MAX5970_NUM_SWITCHES];
> +	unsigned int mon_rng[MAX5970_NUM_SWITCHES];
> +};
> +
> +#define MAX597X_ADC_CHANNEL(_idx, _type) {			\
> +	.type = IIO_ ## _type,					\
> +	.indexed = 1,						\
> +	.channel = (_idx),					\
> +	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |		\
> +			      BIT(IIO_CHAN_INFO_SCALE),		\
> +	.address = MAX5970_REG_ ## _type ## _L(_idx),		\
> +}
> +
> +static const struct iio_chan_spec max5978_adc_iio_channels[] = {
> +	MAX597X_ADC_CHANNEL(0, VOLTAGE),
> +	MAX597X_ADC_CHANNEL(0, CURRENT),
> +};
> +
> +static const struct iio_chan_spec max5970_adc_iio_channels[] = {
> +	MAX597X_ADC_CHANNEL(0, VOLTAGE),
> +	MAX597X_ADC_CHANNEL(0, CURRENT),
> +	MAX597X_ADC_CHANNEL(1, VOLTAGE),
> +	MAX597X_ADC_CHANNEL(1, CURRENT),
> +};
> +
> +static int max597x_iio_read_raw(struct iio_dev *iio_dev,
> +				struct iio_chan_spec const *chan,
> +				int *val, int *val2, long info)
> +{
> +	int ret;
> +	struct max597x_iio *data = iio_priv(iio_dev);
> +	u16 reg_l, reg_h;
> +
> +	switch (info) {
> +	case IIO_CHAN_INFO_RAW:
> +		ret = regmap_bulk_read(data->regmap, chan->address - 1, &reg_l, 2);

This crossed with my reply to the v2 thread.  If reading two separate registers that
don't directly correspond to a large packed value, read them into an array
of u8.  Then get the relevant parts from that.

The following use is not endian safe (it won't work on a big endian machine)

> +		if (ret < 0)
> +			return ret;
> +		reg_h = reg_l & 0xff;
> +		reg_l = (reg_l >> 8) & 0xff;
> +		*val = (reg_h << 2) | (reg_l & 3);
> +		return IIO_VAL_INT;
> +
> +	case IIO_CHAN_INFO_SCALE:
> +		switch (chan->address) {
> +		case MAX5970_REG_CURRENT_L(0):
> +		case MAX5970_REG_CURRENT_L(1):
> +			/* in A, convert to mA */
> +			*val = data->irng[chan->channel] * 1000;
> +			*val2 = data->shunt_micro_ohms[chan->channel] * ADC_MASK;
> +			return IIO_VAL_FRACTIONAL;
> +
> +		case MAX5970_REG_VOLTAGE_L(0):
> +		case MAX5970_REG_VOLTAGE_L(1):
> +			/* in uV, convert to mV */
> +			*val = data->mon_rng[chan->channel];
> +			*val2 = ADC_MASK * 1000;
> +			return IIO_VAL_FRACTIONAL;
> +		}
> +
> +		break;
> +	}
> +	return -EINVAL;

I'd prefer this pushed up as a default: in each of the two switch statements.
Makes it clear to compilers and readers that only listing some case values is
deliberate.

> +}
> +
> +static const struct iio_info max597x_adc_iio_info = {
> +	.read_raw = &max597x_iio_read_raw,
> +};
> +
> +static int max597x_iio_probe(struct platform_device *pdev)
> +{
> +	struct max597x_data *max597x = dev_get_drvdata(pdev->dev.parent);
> +	struct regmap *regmap = dev_get_regmap(pdev->dev.parent, NULL);
> +	struct iio_dev *indio_dev;
> +	struct max597x_iio *priv;
> +	int ret, i;
> +
> +	if (!regmap)
> +		return -EPROBE_DEFER;
> +
> +	if (!max597x || !max597x->num_switches)
> +		return -EPROBE_DEFER;
> +
> +	indio_dev = devm_iio_device_alloc(&pdev->dev, sizeof(*priv));
> +	if (!indio_dev)
> +		return dev_err_probe(&pdev->dev, -ENOMEM,
> +				     "failed to allocate iio device\n");
> +
> +	indio_dev->info = &max597x_adc_iio_info;
> +	indio_dev->modes = INDIO_DIRECT_MODE;
> +
> +	switch (max597x->num_switches) {

Having a value 'num_switches' that maps to a set of enums called _TYPE_ is unusual.
Perhaps rename it to type.


> +	case MAX597x_TYPE_MAX5970:
> +		indio_dev->channels = max5970_adc_iio_channels;
> +		indio_dev->num_channels = ARRAY_SIZE(max5970_adc_iio_channels);
> +		indio_dev->name = "max5970";
> +		break;
> +	case MAX597x_TYPE_MAX5978:
> +		indio_dev->channels = max5978_adc_iio_channels;
> +		indio_dev->num_channels = ARRAY_SIZE(max5978_adc_iio_channels);
> +		indio_dev->name = "max5978";
> +		break;
> +	}
> +
> +	priv = iio_priv(indio_dev);
> +	priv->regmap = regmap;
> +	for (i = 0; i < indio_dev->num_channels; i++) {
> +		priv->irng[i] = max597x->irng[i];
> +		priv->mon_rng[i] = max597x->mon_rng[i];
> +		priv->shunt_micro_ohms[i] = max597x->shunt_micro_ohms[i];
> +	}
> +
> +	ret = devm_iio_device_register(&pdev->dev, indio_dev);
> +	if (ret)
> +		return dev_err_probe(&pdev->dev, ret, "could not register iio device\n");
> +
> +	return 0;
> +}
> +
> +static struct platform_driver max597x_iio_driver = {
> +	.driver = {
> +		.name = "max597x-iio",
> +	},
> +	.probe = max597x_iio_probe,
> +};
> +
> +module_platform_driver(max597x_iio_driver);
> +
> +MODULE_AUTHOR("Patrick Rudolph <patrick.rudolph@9elements.com>");
> +MODULE_DESCRIPTION("MAX5970_hot-swap controller driver");
> +MODULE_LICENSE("GPL");
> 
> base-commit: 368eb79f738a21e16c2bdbcac2444dfa96b01aaa

Re: [PATCH v3 1/2] iio: max597x: Add support for max597x

[Naresh Solanki]

Hi Jonathan,

On 02-04-2023 10:31 pm, Jonathan Cameron wrote:
> On Tue, 28 Mar 2023 11:44:14 +0200
> Naresh Solanki <naresh.solanki@9elements.com> wrote:
> 
>> From: Patrick Rudolph <patrick.rudolph@9elements.com>
>>
>> max5970 & max5978 has 10bit ADC for voltage & current
>> monitoring.
>> Use iio framework to expose the same in sysfs.
>>
>> Signed-off-by: Patrick Rudolph <patrick.rudolph@9elements.com>
>> Signed-off-by: Naresh Solanki <Naresh.Solanki@9elements.com>
>> ...
> 
> --- not ...
> 
> As I mentioned in my reply to v2 thread (which crossed with this v3)
> I'd like this series to be cc'd to the list and maintainers for Hwmon
> with a cover letter explaining the reasoning for it being an IIO driver
> + the restrictions that potentially brings.
Sure.
> 
> A few other comments inline from taking another look.
> 
> Thanks,
> 
> Jonathan
> 
> 
>> Changes in V3:
>> - Use bulk read
>> - Remove line split
>> Changes in V2:
>> - Remove fallthrough
>> - Use pdev->dev instead of i2c->dev
>> - Init indio_dev->name based on device type.
>> ---
>>   drivers/iio/adc/Kconfig       |  15 ++++
>>   drivers/iio/adc/Makefile      |   1 +
>>   drivers/iio/adc/max597x-iio.c | 148 ++++++++++++++++++++++++++++++++++
>>   3 files changed, 164 insertions(+)
>>   create mode 100644 drivers/iio/adc/max597x-iio.c
>>
>> diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
>> index 45af2302be53..69310af5c665 100644
>> --- a/drivers/iio/adc/Kconfig
>> +++ b/drivers/iio/adc/Kconfig
>> @@ -735,6 +735,21 @@ config MAX1363
>>   	  To compile this driver as a module, choose M here: the module will be
>>   	  called max1363.
>>   
>> +config MAX597X_IIO
>> +	tristate "Maxim 597x power switch and monitor"
>> +	depends on I2C && OF
>> +	select MFD_MAX597X
>> +	help
>> +	  This driver enables support for the Maxim 5970 & 5978 smart switch
>> +	  and voltage/current monitoring interface using the Industrial I/O
>> +	  (IIO) framework. The Maxim 597x is a power switch and monitor that can
>> +	  provide voltage and current measurements via the I2C bus. Enabling
>> +	  this driver will allow user space applications to read the voltage
>> +	  and current measurements using IIO interfaces.
>> +
>> +	  To compile this driver as a module, choose M here: the module will be
>> +	  called max597x-iio.
>> +
>>   config MAX9611
>>   	tristate "Maxim max9611/max9612 ADC driver"
>>   	depends on I2C
>> diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile
>> index 36c18177322a..7ec0c2cf7bbb 100644
>> --- a/drivers/iio/adc/Makefile
>> +++ b/drivers/iio/adc/Makefile
>> @@ -67,6 +67,7 @@ obj-$(CONFIG_MAX11205) += max11205.o
>>   obj-$(CONFIG_MAX11410) += max11410.o
>>   obj-$(CONFIG_MAX1241) += max1241.o
>>   obj-$(CONFIG_MAX1363) += max1363.o
>> +obj-$(CONFIG_MAX597X_IIO) += max597x-iio.o
>>   obj-$(CONFIG_MAX9611) += max9611.o
>>   obj-$(CONFIG_MCP320X) += mcp320x.o
>>   obj-$(CONFIG_MCP3422) += mcp3422.o
>> diff --git a/drivers/iio/adc/max597x-iio.c b/drivers/iio/adc/max597x-iio.c
>> new file mode 100644
>> index 000000000000..f158e49b5a56
>> --- /dev/null
>> +++ b/drivers/iio/adc/max597x-iio.c
>> @@ -0,0 +1,148 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * Device driver for IIO in MAX5970 and MAX5978 IC
>> + *
>> + * Copyright (c) 2022 9elements GmbH
>> + *
>> + * Author: Patrick Rudolph <patrick.rudolph@9elements.com>
>> + */
>> +
>> +#include <linux/iio/iio.h>
>> +#include <linux/mfd/max597x.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/regmap.h>
>> +
>> +struct max597x_iio {
>> +	struct regmap *regmap;
>> +	int shunt_micro_ohms[MAX5970_NUM_SWITCHES];
>> +	unsigned int irng[MAX5970_NUM_SWITCHES];
>> +	unsigned int mon_rng[MAX5970_NUM_SWITCHES];
>> +};
>> +
>> +#define MAX597X_ADC_CHANNEL(_idx, _type) {			\
>> +	.type = IIO_ ## _type,					\
>> +	.indexed = 1,						\
>> +	.channel = (_idx),					\
>> +	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |		\
>> +			      BIT(IIO_CHAN_INFO_SCALE),		\
>> +	.address = MAX5970_REG_ ## _type ## _L(_idx),		\
>> +}
>> +
>> +static const struct iio_chan_spec max5978_adc_iio_channels[] = {
>> +	MAX597X_ADC_CHANNEL(0, VOLTAGE),
>> +	MAX597X_ADC_CHANNEL(0, CURRENT),
>> +};
>> +
>> +static const struct iio_chan_spec max5970_adc_iio_channels[] = {
>> +	MAX597X_ADC_CHANNEL(0, VOLTAGE),
>> +	MAX597X_ADC_CHANNEL(0, CURRENT),
>> +	MAX597X_ADC_CHANNEL(1, VOLTAGE),
>> +	MAX597X_ADC_CHANNEL(1, CURRENT),
>> +};
>> +
>> +static int max597x_iio_read_raw(struct iio_dev *iio_dev,
>> +				struct iio_chan_spec const *chan,
>> +				int *val, int *val2, long info)
>> +{
>> +	int ret;
>> +	struct max597x_iio *data = iio_priv(iio_dev);
>> +	u16 reg_l, reg_h;
>> +
>> +	switch (info) {
>> +	case IIO_CHAN_INFO_RAW:
>> +		ret = regmap_bulk_read(data->regmap, chan->address - 1, &reg_l, 2);
> 
> This crossed with my reply to the v2 thread.  If reading two separate registers that
> don't directly correspond to a large packed value, read them into an array
> of u8.  Then get the relevant parts from that.
Sure
> 
> The following use is not endian safe (it won't work on a big endian machine)
Sure
> 
>> +		if (ret < 0)
>> +			return ret;
>> +		reg_h = reg_l & 0xff;
>> +		reg_l = (reg_l >> 8) & 0xff;
>> +		*val = (reg_h << 2) | (reg_l & 3);
>> +		return IIO_VAL_INT;
>> +
>> +	case IIO_CHAN_INFO_SCALE:
>> +		switch (chan->address) {
>> +		case MAX5970_REG_CURRENT_L(0):
>> +		case MAX5970_REG_CURRENT_L(1):
>> +			/* in A, convert to mA */
>> +			*val = data->irng[chan->channel] * 1000;
>> +			*val2 = data->shunt_micro_ohms[chan->channel] * ADC_MASK;
>> +			return IIO_VAL_FRACTIONAL;
>> +
>> +		case MAX5970_REG_VOLTAGE_L(0):
>> +		case MAX5970_REG_VOLTAGE_L(1):
>> +			/* in uV, convert to mV */
>> +			*val = data->mon_rng[chan->channel];
>> +			*val2 = ADC_MASK * 1000;
>> +			return IIO_VAL_FRACTIONAL;
>> +		}
>> +
>> +		break;
>> +	}
>> +	return -EINVAL;
> 
> I'd prefer this pushed up as a default: in each of the two switch statements.
> Makes it clear to compilers and readers that only listing some case values is
> deliberate.
Sure
> 
>> +}
>> +
>> +static const struct iio_info max597x_adc_iio_info = {
>> +	.read_raw = &max597x_iio_read_raw,
>> +};
>> +
>> +static int max597x_iio_probe(struct platform_device *pdev)
>> +{
>> +	struct max597x_data *max597x = dev_get_drvdata(pdev->dev.parent);
>> +	struct regmap *regmap = dev_get_regmap(pdev->dev.parent, NULL);
>> +	struct iio_dev *indio_dev;
>> +	struct max597x_iio *priv;
>> +	int ret, i;
>> +
>> +	if (!regmap)
>> +		return -EPROBE_DEFER;
>> +
>> +	if (!max597x || !max597x->num_switches)
>> +		return -EPROBE_DEFER;
>> +
>> +	indio_dev = devm_iio_device_alloc(&pdev->dev, sizeof(*priv));
>> +	if (!indio_dev)
>> +		return dev_err_probe(&pdev->dev, -ENOMEM,
>> +				     "failed to allocate iio device\n");
>> +
>> +	indio_dev->info = &max597x_adc_iio_info;
>> +	indio_dev->modes = INDIO_DIRECT_MODE;
>> +
>> +	switch (max597x->num_switches) {
> 
> Having a value 'num_switches' that maps to a set of enums called _TYPE_ is unusual.
> Perhaps rename it to type.
Will add a local variable type to track the same within with driver.

> 
> 
>> +	case MAX597x_TYPE_MAX5970:
>> +		indio_dev->channels = max5970_adc_iio_channels;
>> +		indio_dev->num_channels = ARRAY_SIZE(max5970_adc_iio_channels);
>> +		indio_dev->name = "max5970";
>> +		break;
>> +	case MAX597x_TYPE_MAX5978:
>> +		indio_dev->channels = max5978_adc_iio_channels;
>> +		indio_dev->num_channels = ARRAY_SIZE(max5978_adc_iio_channels);
>> +		indio_dev->name = "max5978";
>> +		break;
>> +	}
>> +
>> +	priv = iio_priv(indio_dev);
>> +	priv->regmap = regmap;
>> +	for (i = 0; i < indio_dev->num_channels; i++) {
>> +		priv->irng[i] = max597x->irng[i];
>> +		priv->mon_rng[i] = max597x->mon_rng[i];
>> +		priv->shunt_micro_ohms[i] = max597x->shunt_micro_ohms[i];
>> +	}
>> +
>> +	ret = devm_iio_device_register(&pdev->dev, indio_dev);
>> +	if (ret)
>> +		return dev_err_probe(&pdev->dev, ret, "could not register iio device\n");
>> +
>> +	return 0;
>> +}
>> +
>> +static struct platform_driver max597x_iio_driver = {
>> +	.driver = {
>> +		.name = "max597x-iio",
>> +	},
>> +	.probe = max597x_iio_probe,
>> +};
>> +
>> +module_platform_driver(max597x_iio_driver);
>> +
>> +MODULE_AUTHOR("Patrick Rudolph <patrick.rudolph@9elements.com>");
>> +MODULE_DESCRIPTION("MAX5970_hot-swap controller driver");
>> +MODULE_LICENSE("GPL");
>>
>> base-commit: 368eb79f738a21e16c2bdbcac2444dfa96b01aaa
> 

Regards,
Naresh

Re: [PATCH v3 1/2] iio: max597x: Add support for max597x

[Jonathan Cameron]

On Tue, 4 Apr 2023 15:37:21 +0530
Naresh Solanki <naresh.solanki@9elements.com> wrote:

> Hi Jonathan,
> 
> On 02-04-2023 10:31 pm, Jonathan Cameron wrote:
> > On Tue, 28 Mar 2023 11:44:14 +0200
> > Naresh Solanki <naresh.solanki@9elements.com> wrote:
> >   
> >> From: Patrick Rudolph <patrick.rudolph@9elements.com>
> >>
> >> max5970 & max5978 has 10bit ADC for voltage & current
> >> monitoring.
> >> Use iio framework to expose the same in sysfs.
> >>
> >> Signed-off-by: Patrick Rudolph <patrick.rudolph@9elements.com>
> >> Signed-off-by: Naresh Solanki <Naresh.Solanki@9elements.com>
> >> ...  
> > 
> > --- not ...
> > 
> > As I mentioned in my reply to v2 thread (which crossed with this v3)
> > I'd like this series to be cc'd to the list and maintainers for Hwmon
> > with a cover letter explaining the reasoning for it being an IIO driver
> > + the restrictions that potentially brings.  
> Sure.

A minor request from me for future replies to make things a little
more efficient. Note I'm not always great at this myself - this
is a case of do as I say rather than as I do!

Don't bother agreeing with stuff - reviewers assume you agree
unless you comment!

Crop out any sections of the email reply that have no new content.

e.g.

...

> >> +static int max597x_iio_probe(struct platform_device *pdev)
> >> +{
> >> +	struct max597x_data *max597x = dev_get_drvdata(pdev->dev.parent);
> >> +	struct regmap *regmap = dev_get_regmap(pdev->dev.parent, NULL);
> >> +	struct iio_dev *indio_dev;
> >> +	struct max597x_iio *priv;
> >> +	int ret, i;
> >> +
> >> +	if (!regmap)
> >> +		return -EPROBE_DEFER;
> >> +
> >> +	if (!max597x || !max597x->num_switches)
> >> +		return -EPROBE_DEFER;
> >> +
> >> +	indio_dev = devm_iio_device_alloc(&pdev->dev, sizeof(*priv));
> >> +	if (!indio_dev)
> >> +		return dev_err_probe(&pdev->dev, -ENOMEM,
> >> +				     "failed to allocate iio device\n");
> >> +
> >> +	indio_dev->info = &max597x_adc_iio_info;
> >> +	indio_dev->modes = INDIO_DIRECT_MODE;
> >> +
> >> +	switch (max597x->num_switches) {  
> > 
> > Having a value 'num_switches' that maps to a set of enums called _TYPE_ is unusual.
> > Perhaps rename it to type.  
> Will add a local variable type to track the same within with driver.

Not totally sure what you mean. I'll look for it in newer versions.

Thanks,

Jonathan