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

[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 [李琼斯]