[RESEND PATCH v3] leds: max5970: Add support for max5970

[RESEND PATCH v3] leds: max5970: Add support for max5970

[Naresh Solanki]

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

The MAX5970 is hot swap controller and has 4 indication LED.

Signed-off-by: Patrick Rudolph <patrick.rudolph@9elements.com>
Signed-off-by: Naresh Solanki <Naresh.Solanki@9elements.com>
---
Changes in V3:
- Drop array for ddata variable.
Changes in V2:
- Add of_node_put before return.
- Code cleanup
- Refactor code & remove max5970_setup_led function.
---
 drivers/leds/Kconfig        |  11 ++++
 drivers/leds/Makefile       |   1 +
 drivers/leds/leds-max5970.c | 110 ++++++++++++++++++++++++++++++++++++
 3 files changed, 122 insertions(+)
 create mode 100644 drivers/leds/leds-max5970.c

diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
index b92208eccdea..03ef527cc545 100644
--- a/drivers/leds/Kconfig
+++ b/drivers/leds/Kconfig
@@ -637,6 +637,17 @@ config LEDS_ADP5520
 	  To compile this driver as a module, choose M here: the module will
 	  be called leds-adp5520.
 
+config LEDS_MAX5970
+	tristate "LED Support for Maxim 5970"
+	depends on LEDS_CLASS
+	depends on MFD_MAX5970
+	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-max5970.
+
 config LEDS_MC13783
 	tristate "LED Support for MC13XXX PMIC"
 	depends on LEDS_CLASS
diff --git a/drivers/leds/Makefile b/drivers/leds/Makefile
index d7348e8bc019..6eaee0a753c6 100644
--- a/drivers/leds/Makefile
+++ b/drivers/leds/Makefile
@@ -56,6 +56,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_MAX5970)		+= leds-max5970.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-max5970.c b/drivers/leds/leds-max5970.c
new file mode 100644
index 000000000000..c9685990e26e
--- /dev/null
+++ b/drivers/leds/leds-max5970.c
@@ -0,0 +1,110 @@
+// 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/max5970.h>
+#include <linux/of.h>
+#include <linux/platform_device.h>
+#include <linux/regmap.h>
+
+#define ldev_to_maxled(c)       container_of(c, struct max5970_led, cdev)
+
+struct max5970_led {
+	struct device *dev;
+	struct regmap *regmap;
+	struct led_classdev cdev;
+	unsigned int index;
+};
+
+static int max5970_led_set_brightness(struct led_classdev *cdev,
+				      enum led_brightness brightness)
+{
+	struct max5970_led *ddata = ldev_to_maxled(cdev);
+	int ret, val;
+
+	/* Set/clear corresponding bit for given led index */
+	val = !brightness ? BIT(ddata->index) : 0;
+
+	ret = regmap_update_bits(ddata->regmap, MAX5970_REG_LED_FLASH, BIT(ddata->index), val);
+	if (ret < 0)
+		dev_err(cdev->dev, "failed to set brightness %d", ret);
+
+	return ret;
+}
+
+static int max5970_led_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct device_node *np = dev_of_node(dev->parent);
+	struct regmap *regmap;
+	struct device_node *led_node;
+	struct device_node *child;
+	struct max5970_led *ddata;
+	int ret = -ENODEV, num_leds = 0;
+
+	regmap = dev_get_regmap(pdev->dev.parent, NULL);
+	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 >= MAX5970_NUM_LEDS) {
+			dev_err(dev, "invalid LED (%u >= %d)\n", reg, MAX5970_NUM_LEDS);
+			continue;
+		}
+
+		ddata = devm_kzalloc(dev, sizeof(struct max5970_led), GFP_KERNEL);
+		if (!ddata) {
+			of_node_put(child);
+			return -ENOMEM;
+		}
+
+		ddata->index = reg;
+		ddata->regmap = regmap;
+		ddata->dev = dev;
+
+		if (of_property_read_string(child, "label", &ddata->cdev.name))
+			ddata->cdev.name = child->name;
+
+		ddata->cdev.max_brightness = 1;
+		ddata->cdev.brightness_set_blocking = max5970_led_set_brightness;
+		ddata->cdev.default_trigger = "none";
+
+		ret = devm_led_classdev_register(ddata->dev, &ddata->cdev);
+		if (ret < 0) {
+			of_node_put(child);
+			dev_err(dev, "Failed to initialize LED %u\n", reg);
+			return ret;
+		}
+		num_leds++;
+	}
+
+	return ret;
+}
+
+static struct platform_driver max5970_led_driver = {
+	.driver = {
+		.name = "max5970-led",
+	},
+	.probe = max5970_led_probe,
+};
+
+module_platform_driver(max5970_led_driver);
+MODULE_AUTHOR("Patrick Rudolph <patrick.rudolph@9elements.com>");
+MODULE_AUTHOR("Naresh Solanki <Naresh.Solanki@9elements.com>");
+MODULE_DESCRIPTION("MAX5970_hot-swap controller LED driver");
+MODULE_LICENSE("GPL");

base-commit: baca986e1f2c31f8e4b2a6d99d47c3bc844033e8
-- 
2.41.0

[RESEND PATCH v3] leds: max5970: Add support for max5970

[Naresh Solanki]

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

The MAX5970 is hot swap controller and has 4 indication LED.

Signed-off-by: Patrick Rudolph <patrick.rudolph@9elements.com>
Signed-off-by: Naresh Solanki <Naresh.Solanki@9elements.com>
---
Changes in V3:
- Drop array for ddata variable.
Changes in V2:
- Add of_node_put before return.
- Code cleanup
- Refactor code & remove max5970_setup_led function.
---
 drivers/leds/Kconfig        |  11 ++++
 drivers/leds/Makefile       |   1 +
 drivers/leds/leds-max5970.c | 110 ++++++++++++++++++++++++++++++++++++
 3 files changed, 122 insertions(+)
 create mode 100644 drivers/leds/leds-max5970.c

diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
index b92208eccdea..03ef527cc545 100644
--- a/drivers/leds/Kconfig
+++ b/drivers/leds/Kconfig
@@ -637,6 +637,17 @@ config LEDS_ADP5520
 	  To compile this driver as a module, choose M here: the module will
 	  be called leds-adp5520.
 
+config LEDS_MAX5970
+	tristate "LED Support for Maxim 5970"
+	depends on LEDS_CLASS
+	depends on MFD_MAX5970
+	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-max5970.
+
 config LEDS_MC13783
 	tristate "LED Support for MC13XXX PMIC"
 	depends on LEDS_CLASS
diff --git a/drivers/leds/Makefile b/drivers/leds/Makefile
index d7348e8bc019..6eaee0a753c6 100644
--- a/drivers/leds/Makefile
+++ b/drivers/leds/Makefile
@@ -56,6 +56,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_MAX5970)		+= leds-max5970.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-max5970.c b/drivers/leds/leds-max5970.c
new file mode 100644
index 000000000000..c9685990e26e
--- /dev/null
+++ b/drivers/leds/leds-max5970.c
@@ -0,0 +1,110 @@
+// 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/max5970.h>
+#include <linux/of.h>
+#include <linux/platform_device.h>
+#include <linux/regmap.h>
+
+#define ldev_to_maxled(c)       container_of(c, struct max5970_led, cdev)
+
+struct max5970_led {
+	struct device *dev;
+	struct regmap *regmap;
+	struct led_classdev cdev;
+	unsigned int index;
+};
+
+static int max5970_led_set_brightness(struct led_classdev *cdev,
+				      enum led_brightness brightness)
+{
+	struct max5970_led *ddata = ldev_to_maxled(cdev);
+	int ret, val;
+
+	/* Set/clear corresponding bit for given led index */
+	val = !brightness ? BIT(ddata->index) : 0;
+
+	ret = regmap_update_bits(ddata->regmap, MAX5970_REG_LED_FLASH, BIT(ddata->index), val);
+	if (ret < 0)
+		dev_err(cdev->dev, "failed to set brightness %d", ret);
+
+	return ret;
+}
+
+static int max5970_led_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct device_node *np = dev_of_node(dev->parent);
+	struct regmap *regmap;
+	struct device_node *led_node;
+	struct device_node *child;
+	struct max5970_led *ddata;
+	int ret = -ENODEV, num_leds = 0;
+
+	regmap = dev_get_regmap(pdev->dev.parent, NULL);
+	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 >= MAX5970_NUM_LEDS) {
+			dev_err(dev, "invalid LED (%u >= %d)\n", reg, MAX5970_NUM_LEDS);
+			continue;
+		}
+
+		ddata = devm_kzalloc(dev, sizeof(struct max5970_led), GFP_KERNEL);
+		if (!ddata) {
+			of_node_put(child);
+			return -ENOMEM;
+		}
+
+		ddata->index = reg;
+		ddata->regmap = regmap;
+		ddata->dev = dev;
+
+		if (of_property_read_string(child, "label", &ddata->cdev.name))
+			ddata->cdev.name = child->name;
+
+		ddata->cdev.max_brightness = 1;
+		ddata->cdev.brightness_set_blocking = max5970_led_set_brightness;
+		ddata->cdev.default_trigger = "none";
+
+		ret = devm_led_classdev_register(ddata->dev, &ddata->cdev);
+		if (ret < 0) {
+			of_node_put(child);
+			dev_err(dev, "Failed to initialize LED %u\n", reg);
+			return ret;
+		}
+		num_leds++;
+	}
+
+	return ret;
+}
+
+static struct platform_driver max5970_led_driver = {
+	.driver = {
+		.name = "max5970-led",
+	},
+	.probe = max5970_led_probe,
+};
+
+module_platform_driver(max5970_led_driver);
+MODULE_AUTHOR("Patrick Rudolph <patrick.rudolph@9elements.com>");
+MODULE_AUTHOR("Naresh Solanki <Naresh.Solanki@9elements.com>");
+MODULE_DESCRIPTION("MAX5970_hot-swap controller LED driver");
+MODULE_LICENSE("GPL");

base-commit: baca986e1f2c31f8e4b2a6d99d47c3bc844033e8
-- 
2.41.0

Re: [RESEND PATCH v3] leds: max5970: Add support for max5970

[Lee Jones]

On Thu, 14 Sep 2023, Naresh Solanki wrote:

> From: Patrick Rudolph <patrick.rudolph@9elements.com>
> 
> The MAX5970 is hot swap controller and has 4 indication LED.
> 
> Signed-off-by: Patrick Rudolph <patrick.rudolph@9elements.com>
> Signed-off-by: Naresh Solanki <Naresh.Solanki@9elements.com>
> ---
> Changes in V3:
> - Drop array for ddata variable.
> Changes in V2:
> - Add of_node_put before return.
> - Code cleanup
> - Refactor code & remove max5970_setup_led function.
> ---
>  drivers/leds/Kconfig        |  11 ++++
>  drivers/leds/Makefile       |   1 +
>  drivers/leds/leds-max5970.c | 110 ++++++++++++++++++++++++++++++++++++
>  3 files changed, 122 insertions(+)
>  create mode 100644 drivers/leds/leds-max5970.c

Couple of nits and you're good to go.

Once fixed please resubmit with my:

  Reviewed-by: Lee Jones <lee@kernel.org>

> diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
> index b92208eccdea..03ef527cc545 100644
> --- a/drivers/leds/Kconfig
> +++ b/drivers/leds/Kconfig
> @@ -637,6 +637,17 @@ config LEDS_ADP5520
>  	  To compile this driver as a module, choose M here: the module will
>  	  be called leds-adp5520.
>  
> +config LEDS_MAX5970
> +	tristate "LED Support for Maxim 5970"
> +	depends on LEDS_CLASS
> +	depends on MFD_MAX5970
> +	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-max5970.
> +
>  config LEDS_MC13783
>  	tristate "LED Support for MC13XXX PMIC"
>  	depends on LEDS_CLASS
> diff --git a/drivers/leds/Makefile b/drivers/leds/Makefile
> index d7348e8bc019..6eaee0a753c6 100644
> --- a/drivers/leds/Makefile
> +++ b/drivers/leds/Makefile
> @@ -56,6 +56,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_MAX5970)		+= leds-max5970.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-max5970.c b/drivers/leds/leds-max5970.c
> new file mode 100644
> index 000000000000..c9685990e26e
> --- /dev/null
> +++ b/drivers/leds/leds-max5970.c
> @@ -0,0 +1,110 @@
> +// 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/max5970.h>
> +#include <linux/of.h>
> +#include <linux/platform_device.h>
> +#include <linux/regmap.h>
> +
> +#define ldev_to_maxled(c)       container_of(c, struct max5970_led, cdev)
> +
> +struct max5970_led {
> +	struct device *dev;
> +	struct regmap *regmap;
> +	struct led_classdev cdev;
> +	unsigned int index;
> +};
> +
> +static int max5970_led_set_brightness(struct led_classdev *cdev,
> +				      enum led_brightness brightness)
> +{
> +	struct max5970_led *ddata = ldev_to_maxled(cdev);
> +	int ret, val;
> +
> +	/* Set/clear corresponding bit for given led index */
> +	val = !brightness ? BIT(ddata->index) : 0;
> +
> +	ret = regmap_update_bits(ddata->regmap, MAX5970_REG_LED_FLASH, BIT(ddata->index), val);
> +	if (ret < 0)
> +		dev_err(cdev->dev, "failed to set brightness %d", ret);
> +
> +	return ret;
> +}
> +
> +static int max5970_led_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct device_node *np = dev_of_node(dev->parent);
> +	struct regmap *regmap;
> +	struct device_node *led_node;
> +	struct device_node *child;

Nit: You can place these on the same line.

> +	struct max5970_led *ddata;
> +	int ret = -ENODEV, num_leds = 0;
> +
> +	regmap = dev_get_regmap(pdev->dev.parent, NULL);
> +	if (!regmap)
> +		return -EPROBE_DEFER;

Why are you deferring here?

> +	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 >= MAX5970_NUM_LEDS) {
> +			dev_err(dev, "invalid LED (%u >= %d)\n", reg, MAX5970_NUM_LEDS);
> +			continue;
> +		}
> +
> +		ddata = devm_kzalloc(dev, sizeof(struct max5970_led), GFP_KERNEL);

Nit: sizeof(*ddata)

> +		if (!ddata) {
> +			of_node_put(child);
> +			return -ENOMEM;
> +		}
> +
> +		ddata->index = reg;
> +		ddata->regmap = regmap;
> +		ddata->dev = dev;
> +
> +		if (of_property_read_string(child, "label", &ddata->cdev.name))
> +			ddata->cdev.name = child->name;
> +
> +		ddata->cdev.max_brightness = 1;
> +		ddata->cdev.brightness_set_blocking = max5970_led_set_brightness;
> +		ddata->cdev.default_trigger = "none";
> +
> +		ret = devm_led_classdev_register(ddata->dev, &ddata->cdev);

Nit: Use the shorter 'dev' version whilst it's available.

> +		if (ret < 0) {
> +			of_node_put(child);
> +			dev_err(dev, "Failed to initialize LED %u\n", reg);
> +			return ret;
> +		}
> +		num_leds++;
> +	}
> +
> +	return ret;
> +}
> +
> +static struct platform_driver max5970_led_driver = {
> +	.driver = {
> +		.name = "max5970-led",
> +	},
> +	.probe = max5970_led_probe,
> +};
> +
> +module_platform_driver(max5970_led_driver);
> +MODULE_AUTHOR("Patrick Rudolph <patrick.rudolph@9elements.com>");
> +MODULE_AUTHOR("Naresh Solanki <Naresh.Solanki@9elements.com>");
> +MODULE_DESCRIPTION("MAX5970_hot-swap controller LED driver");
> +MODULE_LICENSE("GPL");
> 
> base-commit: baca986e1f2c31f8e4b2a6d99d47c3bc844033e8
> -- 
> 2.41.0
> 

-- 
Lee Jones [李琼斯]

Re: [RESEND PATCH v3] leds: max5970: Add support for max5970

[Naresh Solanki]

Hi


On Wed, 20 Sept 2023 at 18:35, Lee Jones <lee@kernel.org> wrote:
>
> On Thu, 14 Sep 2023, Naresh Solanki wrote:
>
> > From: Patrick Rudolph <patrick.rudolph@9elements.com>
> >
> > The MAX5970 is hot swap controller and has 4 indication LED.
> >
> > Signed-off-by: Patrick Rudolph <patrick.rudolph@9elements.com>
> > Signed-off-by: Naresh Solanki <Naresh.Solanki@9elements.com>
> > ---
> > Changes in V3:
> > - Drop array for ddata variable.
> > Changes in V2:
> > - Add of_node_put before return.
> > - Code cleanup
> > - Refactor code & remove max5970_setup_led function.
> > ---
> >  drivers/leds/Kconfig        |  11 ++++
> >  drivers/leds/Makefile       |   1 +
> >  drivers/leds/leds-max5970.c | 110 ++++++++++++++++++++++++++++++++++++
> >  3 files changed, 122 insertions(+)
> >  create mode 100644 drivers/leds/leds-max5970.c
>
> Couple of nits and you're good to go.
>
> Once fixed please resubmit with my:
>
>   Reviewed-by: Lee Jones <lee@kernel.org>
>
> > diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
> > index b92208eccdea..03ef527cc545 100644
> > --- a/drivers/leds/Kconfig
> > +++ b/drivers/leds/Kconfig
> > @@ -637,6 +637,17 @@ config LEDS_ADP5520
> >         To compile this driver as a module, choose M here: the module will
> >         be called leds-adp5520.
> >
> > +config LEDS_MAX5970
> > +     tristate "LED Support for Maxim 5970"
> > +     depends on LEDS_CLASS
> > +     depends on MFD_MAX5970
> > +     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-max5970.
> > +
> >  config LEDS_MC13783
> >       tristate "LED Support for MC13XXX PMIC"
> >       depends on LEDS_CLASS
> > diff --git a/drivers/leds/Makefile b/drivers/leds/Makefile
> > index d7348e8bc019..6eaee0a753c6 100644
> > --- a/drivers/leds/Makefile
> > +++ b/drivers/leds/Makefile
> > @@ -56,6 +56,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_MAX5970)           += leds-max5970.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-max5970.c b/drivers/leds/leds-max5970.c
> > new file mode 100644
> > index 000000000000..c9685990e26e
> > --- /dev/null
> > +++ b/drivers/leds/leds-max5970.c
> > @@ -0,0 +1,110 @@
> > +// 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/max5970.h>
> > +#include <linux/of.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/regmap.h>
> > +
> > +#define ldev_to_maxled(c)       container_of(c, struct max5970_led, cdev)
> > +
> > +struct max5970_led {
> > +     struct device *dev;
> > +     struct regmap *regmap;
> > +     struct led_classdev cdev;
> > +     unsigned int index;
> > +};
> > +
> > +static int max5970_led_set_brightness(struct led_classdev *cdev,
> > +                                   enum led_brightness brightness)
> > +{
> > +     struct max5970_led *ddata = ldev_to_maxled(cdev);
> > +     int ret, val;
> > +
> > +     /* Set/clear corresponding bit for given led index */
> > +     val = !brightness ? BIT(ddata->index) : 0;
> > +
> > +     ret = regmap_update_bits(ddata->regmap, MAX5970_REG_LED_FLASH, BIT(ddata->index), val);
> > +     if (ret < 0)
> > +             dev_err(cdev->dev, "failed to set brightness %d", ret);
> > +
> > +     return ret;
> > +}
> > +
> > +static int max5970_led_probe(struct platform_device *pdev)
> > +{
> > +     struct device *dev = &pdev->dev;
> > +     struct device_node *np = dev_of_node(dev->parent);
> > +     struct regmap *regmap;
> > +     struct device_node *led_node;
> > +     struct device_node *child;
>
> Nit: You can place these on the same line.
Ack
>
> > +     struct max5970_led *ddata;
> > +     int ret = -ENODEV, num_leds = 0;
> > +
> > +     regmap = dev_get_regmap(pdev->dev.parent, NULL);
> > +     if (!regmap)
> > +             return -EPROBE_DEFER;
>
> Why are you deferring here?
This is a Leaf driver. Making sure the parent driver has initialized regmap.
>
> > +     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 >= MAX5970_NUM_LEDS) {
> > +                     dev_err(dev, "invalid LED (%u >= %d)\n", reg, MAX5970_NUM_LEDS);
> > +                     continue;
> > +             }
> > +
> > +             ddata = devm_kzalloc(dev, sizeof(struct max5970_led), GFP_KERNEL);
>
> Nit: sizeof(*ddata)
Ack
>
> > +             if (!ddata) {
> > +                     of_node_put(child);
> > +                     return -ENOMEM;
> > +             }
> > +
> > +             ddata->index = reg;
> > +             ddata->regmap = regmap;
> > +             ddata->dev = dev;
> > +
> > +             if (of_property_read_string(child, "label", &ddata->cdev.name))
> > +                     ddata->cdev.name = child->name;
> > +
> > +             ddata->cdev.max_brightness = 1;
> > +             ddata->cdev.brightness_set_blocking = max5970_led_set_brightness;
> > +             ddata->cdev.default_trigger = "none";
> > +
> > +             ret = devm_led_classdev_register(ddata->dev, &ddata->cdev);
>
> Nit: Use the shorter 'dev' version whilst it's available.
Ack
>
> > +             if (ret < 0) {
> > +                     of_node_put(child);
> > +                     dev_err(dev, "Failed to initialize LED %u\n", reg);
> > +                     return ret;
> > +             }
> > +             num_leds++;
> > +     }
> > +
> > +     return ret;
> > +}
> > +
> > +static struct platform_driver max5970_led_driver = {
> > +     .driver = {
> > +             .name = "max5970-led",
> > +     },
> > +     .probe = max5970_led_probe,
> > +};
> > +
> > +module_platform_driver(max5970_led_driver);
> > +MODULE_AUTHOR("Patrick Rudolph <patrick.rudolph@9elements.com>");
> > +MODULE_AUTHOR("Naresh Solanki <Naresh.Solanki@9elements.com>");
> > +MODULE_DESCRIPTION("MAX5970_hot-swap controller LED driver");
> > +MODULE_LICENSE("GPL");
> >
> > base-commit: baca986e1f2c31f8e4b2a6d99d47c3bc844033e8
> > --
> > 2.41.0
> >
>
> --
> Lee Jones [李琼斯]

Re: [RESEND PATCH v3] leds: max5970: Add support for max5970

[Lee Jones]

On Thu, 21 Sep 2023, Naresh Solanki wrote:

> Hi
> 
> 
> On Wed, 20 Sept 2023 at 18:35, Lee Jones <lee@kernel.org> wrote:
> >
> > On Thu, 14 Sep 2023, Naresh Solanki wrote:
> >
> > > From: Patrick Rudolph <patrick.rudolph@9elements.com>
> > >
> > > The MAX5970 is hot swap controller and has 4 indication LED.
> > >
> > > Signed-off-by: Patrick Rudolph <patrick.rudolph@9elements.com>
> > > Signed-off-by: Naresh Solanki <Naresh.Solanki@9elements.com>
> > > ---
> > > Changes in V3:
> > > - Drop array for ddata variable.
> > > Changes in V2:
> > > - Add of_node_put before return.
> > > - Code cleanup
> > > - Refactor code & remove max5970_setup_led function.
> > > ---
> > >  drivers/leds/Kconfig        |  11 ++++
> > >  drivers/leds/Makefile       |   1 +
> > >  drivers/leds/leds-max5970.c | 110 ++++++++++++++++++++++++++++++++++++
> > >  3 files changed, 122 insertions(+)
> > >  create mode 100644 drivers/leds/leds-max5970.c
> >
> > Couple of nits and you're good to go.
> >
> > Once fixed please resubmit with my:
> >
> >   Reviewed-by: Lee Jones <lee@kernel.org>
> >
> > > diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
> > > index b92208eccdea..03ef527cc545 100644
> > > --- a/drivers/leds/Kconfig
> > > +++ b/drivers/leds/Kconfig
> > > @@ -637,6 +637,17 @@ config LEDS_ADP5520
> > >         To compile this driver as a module, choose M here: the module will
> > >         be called leds-adp5520.
> > >
> > > +config LEDS_MAX5970
> > > +     tristate "LED Support for Maxim 5970"
> > > +     depends on LEDS_CLASS
> > > +     depends on MFD_MAX5970
> > > +     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-max5970.
> > > +
> > >  config LEDS_MC13783
> > >       tristate "LED Support for MC13XXX PMIC"
> > >       depends on LEDS_CLASS
> > > diff --git a/drivers/leds/Makefile b/drivers/leds/Makefile
> > > index d7348e8bc019..6eaee0a753c6 100644
> > > --- a/drivers/leds/Makefile
> > > +++ b/drivers/leds/Makefile
> > > @@ -56,6 +56,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_MAX5970)           += leds-max5970.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-max5970.c b/drivers/leds/leds-max5970.c
> > > new file mode 100644
> > > index 000000000000..c9685990e26e
> > > --- /dev/null
> > > +++ b/drivers/leds/leds-max5970.c
> > > @@ -0,0 +1,110 @@
> > > +// 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/max5970.h>
> > > +#include <linux/of.h>
> > > +#include <linux/platform_device.h>
> > > +#include <linux/regmap.h>
> > > +
> > > +#define ldev_to_maxled(c)       container_of(c, struct max5970_led, cdev)
> > > +
> > > +struct max5970_led {
> > > +     struct device *dev;
> > > +     struct regmap *regmap;
> > > +     struct led_classdev cdev;
> > > +     unsigned int index;
> > > +};
> > > +
> > > +static int max5970_led_set_brightness(struct led_classdev *cdev,
> > > +                                   enum led_brightness brightness)
> > > +{
> > > +     struct max5970_led *ddata = ldev_to_maxled(cdev);
> > > +     int ret, val;
> > > +
> > > +     /* Set/clear corresponding bit for given led index */
> > > +     val = !brightness ? BIT(ddata->index) : 0;
> > > +
> > > +     ret = regmap_update_bits(ddata->regmap, MAX5970_REG_LED_FLASH, BIT(ddata->index), val);
> > > +     if (ret < 0)
> > > +             dev_err(cdev->dev, "failed to set brightness %d", ret);
> > > +
> > > +     return ret;
> > > +}
> > > +
> > > +static int max5970_led_probe(struct platform_device *pdev)
> > > +{
> > > +     struct device *dev = &pdev->dev;
> > > +     struct device_node *np = dev_of_node(dev->parent);
> > > +     struct regmap *regmap;
> > > +     struct device_node *led_node;
> > > +     struct device_node *child;
> >
> > Nit: You can place these on the same line.
> Ack
> >
> > > +     struct max5970_led *ddata;
> > > +     int ret = -ENODEV, num_leds = 0;
> > > +
> > > +     regmap = dev_get_regmap(pdev->dev.parent, NULL);
> > > +     if (!regmap)
> > > +             return -EPROBE_DEFER;
> >
> > Why are you deferring here?
> This is a Leaf driver. Making sure the parent driver has initialized regmap.

How can this driver initialise before the parent driver?

-- 
Lee Jones [李琼斯]

Re: [RESEND PATCH v3] leds: max5970: Add support for max5970

[Naresh Solanki]

Hi,

On Thu, 21 Sept 2023 at 16:02, Lee Jones <lee@kernel.org> wrote:
>
> On Thu, 21 Sep 2023, Naresh Solanki wrote:
>
> > Hi
> >
> >
> > On Wed, 20 Sept 2023 at 18:35, Lee Jones <lee@kernel.org> wrote:
> > >
> > > On Thu, 14 Sep 2023, Naresh Solanki wrote:
> > >
> > > > From: Patrick Rudolph <patrick.rudolph@9elements.com>
> > > >
> > > > The MAX5970 is hot swap controller and has 4 indication LED.
> > > >
> > > > Signed-off-by: Patrick Rudolph <patrick.rudolph@9elements.com>
> > > > Signed-off-by: Naresh Solanki <Naresh.Solanki@9elements.com>
> > > > ---
> > > > Changes in V3:
> > > > - Drop array for ddata variable.
> > > > Changes in V2:
> > > > - Add of_node_put before return.
> > > > - Code cleanup
> > > > - Refactor code & remove max5970_setup_led function.
> > > > ---
> > > >  drivers/leds/Kconfig        |  11 ++++
> > > >  drivers/leds/Makefile       |   1 +
> > > >  drivers/leds/leds-max5970.c | 110 ++++++++++++++++++++++++++++++++++++
> > > >  3 files changed, 122 insertions(+)
> > > >  create mode 100644 drivers/leds/leds-max5970.c
> > >
> > > Couple of nits and you're good to go.
> > >
> > > Once fixed please resubmit with my:
> > >
> > >   Reviewed-by: Lee Jones <lee@kernel.org>
> > >
> > > > diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
> > > > index b92208eccdea..03ef527cc545 100644
> > > > --- a/drivers/leds/Kconfig
> > > > +++ b/drivers/leds/Kconfig
> > > > @@ -637,6 +637,17 @@ config LEDS_ADP5520
> > > >         To compile this driver as a module, choose M here: the module will
> > > >         be called leds-adp5520.
> > > >
> > > > +config LEDS_MAX5970
> > > > +     tristate "LED Support for Maxim 5970"
> > > > +     depends on LEDS_CLASS
> > > > +     depends on MFD_MAX5970
> > > > +     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-max5970.
> > > > +
> > > >  config LEDS_MC13783
> > > >       tristate "LED Support for MC13XXX PMIC"
> > > >       depends on LEDS_CLASS
> > > > diff --git a/drivers/leds/Makefile b/drivers/leds/Makefile
> > > > index d7348e8bc019..6eaee0a753c6 100644
> > > > --- a/drivers/leds/Makefile
> > > > +++ b/drivers/leds/Makefile
> > > > @@ -56,6 +56,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_MAX5970)           += leds-max5970.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-max5970.c b/drivers/leds/leds-max5970.c
> > > > new file mode 100644
> > > > index 000000000000..c9685990e26e
> > > > --- /dev/null
> > > > +++ b/drivers/leds/leds-max5970.c
> > > > @@ -0,0 +1,110 @@
> > > > +// 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/max5970.h>
> > > > +#include <linux/of.h>
> > > > +#include <linux/platform_device.h>
> > > > +#include <linux/regmap.h>
> > > > +
> > > > +#define ldev_to_maxled(c)       container_of(c, struct max5970_led, cdev)
> > > > +
> > > > +struct max5970_led {
> > > > +     struct device *dev;
> > > > +     struct regmap *regmap;
> > > > +     struct led_classdev cdev;
> > > > +     unsigned int index;
> > > > +};
> > > > +
> > > > +static int max5970_led_set_brightness(struct led_classdev *cdev,
> > > > +                                   enum led_brightness brightness)
> > > > +{
> > > > +     struct max5970_led *ddata = ldev_to_maxled(cdev);
> > > > +     int ret, val;
> > > > +
> > > > +     /* Set/clear corresponding bit for given led index */
> > > > +     val = !brightness ? BIT(ddata->index) : 0;
> > > > +
> > > > +     ret = regmap_update_bits(ddata->regmap, MAX5970_REG_LED_FLASH, BIT(ddata->index), val);
> > > > +     if (ret < 0)
> > > > +             dev_err(cdev->dev, "failed to set brightness %d", ret);
> > > > +
> > > > +     return ret;
> > > > +}
> > > > +
> > > > +static int max5970_led_probe(struct platform_device *pdev)
> > > > +{
> > > > +     struct device *dev = &pdev->dev;
> > > > +     struct device_node *np = dev_of_node(dev->parent);
> > > > +     struct regmap *regmap;
> > > > +     struct device_node *led_node;
> > > > +     struct device_node *child;
> > >
> > > Nit: You can place these on the same line.
> > Ack
> > >
> > > > +     struct max5970_led *ddata;
> > > > +     int ret = -ENODEV, num_leds = 0;
> > > > +
> > > > +     regmap = dev_get_regmap(pdev->dev.parent, NULL);
> > > > +     if (!regmap)
> > > > +             return -EPROBE_DEFER;
> > >
> > > Why are you deferring here?
> > This is a Leaf driver. Making sure the parent driver has initialized regmap.
>
> How can this driver initialise before the parent driver?
The parent driver in this case is simple_i2c_mfd.
Based on reference from other similar implementations, the regmap
check was adapted.
As you mentioned, your right that leaf driver will not start before parent
driver is loaded successfully so probably the DEFER might not be needed
here.

Thanks,
Naresh
>
> --
> Lee Jones [李琼斯]

Re: [RESEND PATCH v3] leds: max5970: Add support for max5970

[Naresh Solanki]

Hey Lee,

Is there anything specific you'd suggest changing in the current
patchset, or are we good to proceed?

Thanks,
Naresh Solanki

On Mon, 30 Oct 2023 at 14:22, Naresh Solanki
<naresh.solanki@9elements.com> wrote:
>
> Hi,
>
> On Thu, 21 Sept 2023 at 16:02, Lee Jones <lee@kernel.org> wrote:
> >
> > On Thu, 21 Sep 2023, Naresh Solanki wrote:
> >
> > > Hi
> > >
> > >
> > > On Wed, 20 Sept 2023 at 18:35, Lee Jones <lee@kernel.org> wrote:
> > > >
> > > > On Thu, 14 Sep 2023, Naresh Solanki wrote:
> > > >
> > > > > From: Patrick Rudolph <patrick.rudolph@9elements.com>
> > > > >
> > > > > The MAX5970 is hot swap controller and has 4 indication LED.
> > > > >
> > > > > Signed-off-by: Patrick Rudolph <patrick.rudolph@9elements.com>
> > > > > Signed-off-by: Naresh Solanki <Naresh.Solanki@9elements.com>
> > > > > ---
> > > > > Changes in V3:
> > > > > - Drop array for ddata variable.
> > > > > Changes in V2:
> > > > > - Add of_node_put before return.
> > > > > - Code cleanup
> > > > > - Refactor code & remove max5970_setup_led function.
> > > > > ---
> > > > >  drivers/leds/Kconfig        |  11 ++++
> > > > >  drivers/leds/Makefile       |   1 +
> > > > >  drivers/leds/leds-max5970.c | 110 ++++++++++++++++++++++++++++++++++++
> > > > >  3 files changed, 122 insertions(+)
> > > > >  create mode 100644 drivers/leds/leds-max5970.c
> > > >
> > > > Couple of nits and you're good to go.
> > > >
> > > > Once fixed please resubmit with my:
> > > >
> > > >   Reviewed-by: Lee Jones <lee@kernel.org>
> > > >
> > > > > diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
> > > > > index b92208eccdea..03ef527cc545 100644
> > > > > --- a/drivers/leds/Kconfig
> > > > > +++ b/drivers/leds/Kconfig
> > > > > @@ -637,6 +637,17 @@ config LEDS_ADP5520
> > > > >         To compile this driver as a module, choose M here: the module will
> > > > >         be called leds-adp5520.
> > > > >
> > > > > +config LEDS_MAX5970
> > > > > +     tristate "LED Support for Maxim 5970"
> > > > > +     depends on LEDS_CLASS
> > > > > +     depends on MFD_MAX5970
> > > > > +     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-max5970.
> > > > > +
> > > > >  config LEDS_MC13783
> > > > >       tristate "LED Support for MC13XXX PMIC"
> > > > >       depends on LEDS_CLASS
> > > > > diff --git a/drivers/leds/Makefile b/drivers/leds/Makefile
> > > > > index d7348e8bc019..6eaee0a753c6 100644
> > > > > --- a/drivers/leds/Makefile
> > > > > +++ b/drivers/leds/Makefile
> > > > > @@ -56,6 +56,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_MAX5970)           += leds-max5970.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-max5970.c b/drivers/leds/leds-max5970.c
> > > > > new file mode 100644
> > > > > index 000000000000..c9685990e26e
> > > > > --- /dev/null
> > > > > +++ b/drivers/leds/leds-max5970.c
> > > > > @@ -0,0 +1,110 @@
> > > > > +// 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/max5970.h>
> > > > > +#include <linux/of.h>
> > > > > +#include <linux/platform_device.h>
> > > > > +#include <linux/regmap.h>
> > > > > +
> > > > > +#define ldev_to_maxled(c)       container_of(c, struct max5970_led, cdev)
> > > > > +
> > > > > +struct max5970_led {
> > > > > +     struct device *dev;
> > > > > +     struct regmap *regmap;
> > > > > +     struct led_classdev cdev;
> > > > > +     unsigned int index;
> > > > > +};
> > > > > +
> > > > > +static int max5970_led_set_brightness(struct led_classdev *cdev,
> > > > > +                                   enum led_brightness brightness)
> > > > > +{
> > > > > +     struct max5970_led *ddata = ldev_to_maxled(cdev);
> > > > > +     int ret, val;
> > > > > +
> > > > > +     /* Set/clear corresponding bit for given led index */
> > > > > +     val = !brightness ? BIT(ddata->index) : 0;
> > > > > +
> > > > > +     ret = regmap_update_bits(ddata->regmap, MAX5970_REG_LED_FLASH, BIT(ddata->index), val);
> > > > > +     if (ret < 0)
> > > > > +             dev_err(cdev->dev, "failed to set brightness %d", ret);
> > > > > +
> > > > > +     return ret;
> > > > > +}
> > > > > +
> > > > > +static int max5970_led_probe(struct platform_device *pdev)
> > > > > +{
> > > > > +     struct device *dev = &pdev->dev;
> > > > > +     struct device_node *np = dev_of_node(dev->parent);
> > > > > +     struct regmap *regmap;
> > > > > +     struct device_node *led_node;
> > > > > +     struct device_node *child;
> > > >
> > > > Nit: You can place these on the same line.
> > > Ack
> > > >
> > > > > +     struct max5970_led *ddata;
> > > > > +     int ret = -ENODEV, num_leds = 0;
> > > > > +
> > > > > +     regmap = dev_get_regmap(pdev->dev.parent, NULL);
> > > > > +     if (!regmap)
> > > > > +             return -EPROBE_DEFER;
> > > >
> > > > Why are you deferring here?
> > > This is a Leaf driver. Making sure the parent driver has initialized regmap.
> >
> > How can this driver initialise before the parent driver?
> The parent driver in this case is simple_i2c_mfd.
> Based on reference from other similar implementations, the regmap
> check was adapted.
> As you mentioned, your right that leaf driver will not start before parent
> driver is loaded successfully so probably the DEFER might not be needed
> here.
>
> Thanks,
> Naresh
> >
> > --
> > Lee Jones [李琼斯]

Re: [RESEND PATCH v3] leds: max5970: Add support for max5970

[Lee Jones]

On Thu, 09 Nov 2023, Naresh Solanki wrote:

> Hey Lee,
> 
> Is there anything specific you'd suggest changing in the current
> patchset, or are we good to proceed?

What do you mean by proceed?

You are good to make changes and submit a subsequent version.

Not entirely sure what you're asking.

> On Mon, 30 Oct 2023 at 14:22, Naresh Solanki
> <naresh.solanki@9elements.com> wrote:
> >
> > Hi,
> >
> > On Thu, 21 Sept 2023 at 16:02, Lee Jones <lee@kernel.org> wrote:
> > >
> > > On Thu, 21 Sep 2023, Naresh Solanki wrote:
> > >
> > > > Hi
> > > >
> > > >
> > > > On Wed, 20 Sept 2023 at 18:35, Lee Jones <lee@kernel.org> wrote:
> > > > >
> > > > > On Thu, 14 Sep 2023, Naresh Solanki wrote:
> > > > >
> > > > > > From: Patrick Rudolph <patrick.rudolph@9elements.com>
> > > > > >
> > > > > > The MAX5970 is hot swap controller and has 4 indication LED.
> > > > > >
> > > > > > Signed-off-by: Patrick Rudolph <patrick.rudolph@9elements.com>
> > > > > > Signed-off-by: Naresh Solanki <Naresh.Solanki@9elements.com>
> > > > > > ---
> > > > > > Changes in V3:
> > > > > > - Drop array for ddata variable.
> > > > > > Changes in V2:
> > > > > > - Add of_node_put before return.
> > > > > > - Code cleanup
> > > > > > - Refactor code & remove max5970_setup_led function.
> > > > > > ---
> > > > > >  drivers/leds/Kconfig        |  11 ++++
> > > > > >  drivers/leds/Makefile       |   1 +
> > > > > >  drivers/leds/leds-max5970.c | 110 ++++++++++++++++++++++++++++++++++++
> > > > > >  3 files changed, 122 insertions(+)
> > > > > >  create mode 100644 drivers/leds/leds-max5970.c
> > > > >
> > > > > Couple of nits and you're good to go.
> > > > >
> > > > > Once fixed please resubmit with my:
> > > > >
> > > > >   Reviewed-by: Lee Jones <lee@kernel.org>
> > > > >
> > > > > > diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
> > > > > > index b92208eccdea..03ef527cc545 100644
> > > > > > --- a/drivers/leds/Kconfig
> > > > > > +++ b/drivers/leds/Kconfig
> > > > > > @@ -637,6 +637,17 @@ config LEDS_ADP5520
> > > > > >         To compile this driver as a module, choose M here: the module will
> > > > > >         be called leds-adp5520.
> > > > > >
> > > > > > +config LEDS_MAX5970
> > > > > > +     tristate "LED Support for Maxim 5970"
> > > > > > +     depends on LEDS_CLASS
> > > > > > +     depends on MFD_MAX5970
> > > > > > +     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-max5970.
> > > > > > +
> > > > > >  config LEDS_MC13783
> > > > > >       tristate "LED Support for MC13XXX PMIC"
> > > > > >       depends on LEDS_CLASS
> > > > > > diff --git a/drivers/leds/Makefile b/drivers/leds/Makefile
> > > > > > index d7348e8bc019..6eaee0a753c6 100644
> > > > > > --- a/drivers/leds/Makefile
> > > > > > +++ b/drivers/leds/Makefile
> > > > > > @@ -56,6 +56,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_MAX5970)           += leds-max5970.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-max5970.c b/drivers/leds/leds-max5970.c
> > > > > > new file mode 100644
> > > > > > index 000000000000..c9685990e26e
> > > > > > --- /dev/null
> > > > > > +++ b/drivers/leds/leds-max5970.c
> > > > > > @@ -0,0 +1,110 @@
> > > > > > +// 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/max5970.h>
> > > > > > +#include <linux/of.h>
> > > > > > +#include <linux/platform_device.h>
> > > > > > +#include <linux/regmap.h>
> > > > > > +
> > > > > > +#define ldev_to_maxled(c)       container_of(c, struct max5970_led, cdev)
> > > > > > +
> > > > > > +struct max5970_led {
> > > > > > +     struct device *dev;
> > > > > > +     struct regmap *regmap;
> > > > > > +     struct led_classdev cdev;
> > > > > > +     unsigned int index;
> > > > > > +};
> > > > > > +
> > > > > > +static int max5970_led_set_brightness(struct led_classdev *cdev,
> > > > > > +                                   enum led_brightness brightness)
> > > > > > +{
> > > > > > +     struct max5970_led *ddata = ldev_to_maxled(cdev);
> > > > > > +     int ret, val;
> > > > > > +
> > > > > > +     /* Set/clear corresponding bit for given led index */
> > > > > > +     val = !brightness ? BIT(ddata->index) : 0;
> > > > > > +
> > > > > > +     ret = regmap_update_bits(ddata->regmap, MAX5970_REG_LED_FLASH, BIT(ddata->index), val);
> > > > > > +     if (ret < 0)
> > > > > > +             dev_err(cdev->dev, "failed to set brightness %d", ret);
> > > > > > +
> > > > > > +     return ret;
> > > > > > +}
> > > > > > +
> > > > > > +static int max5970_led_probe(struct platform_device *pdev)
> > > > > > +{
> > > > > > +     struct device *dev = &pdev->dev;
> > > > > > +     struct device_node *np = dev_of_node(dev->parent);
> > > > > > +     struct regmap *regmap;
> > > > > > +     struct device_node *led_node;
> > > > > > +     struct device_node *child;
> > > > >
> > > > > Nit: You can place these on the same line.
> > > > Ack
> > > > >
> > > > > > +     struct max5970_led *ddata;
> > > > > > +     int ret = -ENODEV, num_leds = 0;
> > > > > > +
> > > > > > +     regmap = dev_get_regmap(pdev->dev.parent, NULL);
> > > > > > +     if (!regmap)
> > > > > > +             return -EPROBE_DEFER;
> > > > >
> > > > > Why are you deferring here?
> > > > This is a Leaf driver. Making sure the parent driver has initialized regmap.
> > >
> > > How can this driver initialise before the parent driver?
> > The parent driver in this case is simple_i2c_mfd.
> > Based on reference from other similar implementations, the regmap
> > check was adapted.
> > As you mentioned, your right that leaf driver will not start before parent
> > driver is loaded successfully so probably the DEFER might not be needed
> > here.
> >
> > Thanks,
> > Naresh
> > >
> > > --
> > > Lee Jones [李琼斯]

-- 
Lee Jones [李琼斯]

Re: [RESEND PATCH v3] leds: max5970: Add support for max5970

[Naresh Solanki]

Hi

On Fri, 17 Nov 2023 at 17:45, Lee Jones <lee@kernel.org> wrote:
>
> On Thu, 09 Nov 2023, Naresh Solanki wrote:
>
> > Hey Lee,
> >
> > Is there anything specific you'd suggest changing in the current
> > patchset, or are we good to proceed?
>
> What do you mean by proceed?
>
> You are good to make changes and submit a subsequent version.
>
> Not entirely sure what you're asking.

As a follow up on previous discussion regarding use of DEFER on probe
if regmap isn't initialized, the implementation was based on other similar
drivers & hence it was retained although its not needed due to dependencies.

I'm not entirely sure to keep the regmap check or make another
patch revision with regmap check removed ?


Regards,
Naresh
>
> > On Mon, 30 Oct 2023 at 14:22, Naresh Solanki
> > <naresh.solanki@9elements.com> wrote:
> > >
> > > Hi,
> > >
> > > On Thu, 21 Sept 2023 at 16:02, Lee Jones <lee@kernel.org> wrote:
> > > >
> > > > On Thu, 21 Sep 2023, Naresh Solanki wrote:
> > > >
> > > > > Hi
> > > > >
> > > > >
> > > > > On Wed, 20 Sept 2023 at 18:35, Lee Jones <lee@kernel.org> wrote:
> > > > > >
> > > > > > On Thu, 14 Sep 2023, Naresh Solanki wrote:
> > > > > >
> > > > > > > From: Patrick Rudolph <patrick.rudolph@9elements.com>
> > > > > > >
> > > > > > > The MAX5970 is hot swap controller and has 4 indication LED.
> > > > > > >
> > > > > > > Signed-off-by: Patrick Rudolph <patrick.rudolph@9elements.com>
> > > > > > > Signed-off-by: Naresh Solanki <Naresh.Solanki@9elements.com>
> > > > > > > ---
> > > > > > > Changes in V3:
> > > > > > > - Drop array for ddata variable.
> > > > > > > Changes in V2:
> > > > > > > - Add of_node_put before return.
> > > > > > > - Code cleanup
> > > > > > > - Refactor code & remove max5970_setup_led function.
> > > > > > > ---
> > > > > > >  drivers/leds/Kconfig        |  11 ++++
> > > > > > >  drivers/leds/Makefile       |   1 +
> > > > > > >  drivers/leds/leds-max5970.c | 110 ++++++++++++++++++++++++++++++++++++
> > > > > > >  3 files changed, 122 insertions(+)
> > > > > > >  create mode 100644 drivers/leds/leds-max5970.c
> > > > > >
> > > > > > Couple of nits and you're good to go.
> > > > > >
> > > > > > Once fixed please resubmit with my:
> > > > > >
> > > > > >   Reviewed-by: Lee Jones <lee@kernel.org>
> > > > > >
> > > > > > > diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
> > > > > > > index b92208eccdea..03ef527cc545 100644
> > > > > > > --- a/drivers/leds/Kconfig
> > > > > > > +++ b/drivers/leds/Kconfig
> > > > > > > @@ -637,6 +637,17 @@ config LEDS_ADP5520
> > > > > > >         To compile this driver as a module, choose M here: the module will
> > > > > > >         be called leds-adp5520.
> > > > > > >
> > > > > > > +config LEDS_MAX5970
> > > > > > > +     tristate "LED Support for Maxim 5970"
> > > > > > > +     depends on LEDS_CLASS
> > > > > > > +     depends on MFD_MAX5970
> > > > > > > +     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-max5970.
> > > > > > > +
> > > > > > >  config LEDS_MC13783
> > > > > > >       tristate "LED Support for MC13XXX PMIC"
> > > > > > >       depends on LEDS_CLASS
> > > > > > > diff --git a/drivers/leds/Makefile b/drivers/leds/Makefile
> > > > > > > index d7348e8bc019..6eaee0a753c6 100644
> > > > > > > --- a/drivers/leds/Makefile
> > > > > > > +++ b/drivers/leds/Makefile
> > > > > > > @@ -56,6 +56,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_MAX5970)           += leds-max5970.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-max5970.c b/drivers/leds/leds-max5970.c
> > > > > > > new file mode 100644
> > > > > > > index 000000000000..c9685990e26e
> > > > > > > --- /dev/null
> > > > > > > +++ b/drivers/leds/leds-max5970.c
> > > > > > > @@ -0,0 +1,110 @@
> > > > > > > +// 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/max5970.h>
> > > > > > > +#include <linux/of.h>
> > > > > > > +#include <linux/platform_device.h>
> > > > > > > +#include <linux/regmap.h>
> > > > > > > +
> > > > > > > +#define ldev_to_maxled(c)       container_of(c, struct max5970_led, cdev)
> > > > > > > +
> > > > > > > +struct max5970_led {
> > > > > > > +     struct device *dev;
> > > > > > > +     struct regmap *regmap;
> > > > > > > +     struct led_classdev cdev;
> > > > > > > +     unsigned int index;
> > > > > > > +};
> > > > > > > +
> > > > > > > +static int max5970_led_set_brightness(struct led_classdev *cdev,
> > > > > > > +                                   enum led_brightness brightness)
> > > > > > > +{
> > > > > > > +     struct max5970_led *ddata = ldev_to_maxled(cdev);
> > > > > > > +     int ret, val;
> > > > > > > +
> > > > > > > +     /* Set/clear corresponding bit for given led index */
> > > > > > > +     val = !brightness ? BIT(ddata->index) : 0;
> > > > > > > +
> > > > > > > +     ret = regmap_update_bits(ddata->regmap, MAX5970_REG_LED_FLASH, BIT(ddata->index), val);
> > > > > > > +     if (ret < 0)
> > > > > > > +             dev_err(cdev->dev, "failed to set brightness %d", ret);
> > > > > > > +
> > > > > > > +     return ret;
> > > > > > > +}
> > > > > > > +
> > > > > > > +static int max5970_led_probe(struct platform_device *pdev)
> > > > > > > +{
> > > > > > > +     struct device *dev = &pdev->dev;
> > > > > > > +     struct device_node *np = dev_of_node(dev->parent);
> > > > > > > +     struct regmap *regmap;
> > > > > > > +     struct device_node *led_node;
> > > > > > > +     struct device_node *child;
> > > > > >
> > > > > > Nit: You can place these on the same line.
> > > > > Ack
> > > > > >
> > > > > > > +     struct max5970_led *ddata;
> > > > > > > +     int ret = -ENODEV, num_leds = 0;
> > > > > > > +
> > > > > > > +     regmap = dev_get_regmap(pdev->dev.parent, NULL);
> > > > > > > +     if (!regmap)
> > > > > > > +             return -EPROBE_DEFER;
> > > > > >
> > > > > > Why are you deferring here?
> > > > > This is a Leaf driver. Making sure the parent driver has initialized regmap.
> > > >
> > > > How can this driver initialise before the parent driver?
> > > The parent driver in this case is simple_i2c_mfd.
> > > Based on reference from other similar implementations, the regmap
> > > check was adapted.
> > > As you mentioned, your right that leaf driver will not start before parent
> > > driver is loaded successfully so probably the DEFER might not be needed
> > > here.
> > >
> > > Thanks,
> > > Naresh
> > > >
> > > > --
> > > > Lee Jones [李琼斯]
>
> --
> Lee Jones [李琼斯]

Re: [RESEND PATCH v3] leds: max5970: Add support for max5970

[Lee Jones]

On Mon, 20 Nov 2023, Naresh Solanki wrote:

> Hi
> 
> On Fri, 17 Nov 2023 at 17:45, Lee Jones <lee@kernel.org> wrote:
> >
> > On Thu, 09 Nov 2023, Naresh Solanki wrote:
> >
> > > Hey Lee,
> > >
> > > Is there anything specific you'd suggest changing in the current
> > > patchset, or are we good to proceed?
> >
> > What do you mean by proceed?
> >
> > You are good to make changes and submit a subsequent version.
> >
> > Not entirely sure what you're asking.
> 
> As a follow up on previous discussion regarding use of DEFER on probe
> if regmap isn't initialized, the implementation was based on other similar
> drivers & hence it was retained although its not needed due to dependencies.
> 
> I'm not entirely sure to keep the regmap check or make another
> patch revision with regmap check removed ?

You tell me.

You should understand the device you're attempting to support along with
the code you're authoring and its subsequent implications.  If you don't
know what a section of code does or whether/why it's required, why did
you write it?

-- 
Lee Jones [李琼斯]

Re: [RESEND PATCH v3] leds: max5970: Add support for max5970

[Naresh Solanki]

Hi Lee,

Thank you for your insights. I appreciate your guidance on the matter.
Yes will rewrite the change as below:

        regmap = dev_get_regmap(pdev->dev.parent, NULL);
        if (!regmap)
                return -ENODEV;

I believe this modification aligns with your suggestion. Please let me
know if this meets the requirements or if you have any further
suggestions or adjustments

Regards,
Naresh


On Tue, 21 Nov 2023 at 21:03, Lee Jones <lee@kernel.org> wrote:
>
> On Mon, 20 Nov 2023, Naresh Solanki wrote:
>
> > Hi
> >
> > On Fri, 17 Nov 2023 at 17:45, Lee Jones <lee@kernel.org> wrote:
> > >
> > > On Thu, 09 Nov 2023, Naresh Solanki wrote:
> > >
> > > > Hey Lee,
> > > >
> > > > Is there anything specific you'd suggest changing in the current
> > > > patchset, or are we good to proceed?
> > >
> > > What do you mean by proceed?
> > >
> > > You are good to make changes and submit a subsequent version.
> > >
> > > Not entirely sure what you're asking.
> >
> > As a follow up on previous discussion regarding use of DEFER on probe
> > if regmap isn't initialized, the implementation was based on other similar
> > drivers & hence it was retained although its not needed due to dependencies.
> >
> > I'm not entirely sure to keep the regmap check or make another
> > patch revision with regmap check removed ?
>
> You tell me.
>
> You should understand the device you're attempting to support along with
> the code you're authoring and its subsequent implications.  If you don't
> know what a section of code does or whether/why it's required, why did
> you write it?
>
> --
> Lee Jones [李琼斯]

Re: [RESEND PATCH v3] leds: max5970: Add support for max5970

[Lee Jones]

Please read this:

  https://subspace.kernel.org/etiquette.html#do-not-top-post-when-replying

On Tue, 21 Nov 2023, Naresh Solanki wrote:

> Hi Lee,
> 
> Thank you for your insights. I appreciate your guidance on the matter.
> Yes will rewrite the change as below:
> 
>         regmap = dev_get_regmap(pdev->dev.parent, NULL);
>         if (!regmap)
>                 return -ENODEV;
> 
> I believe this modification aligns with your suggestion. Please let me
> know if this meets the requirements or if you have any further
> suggestions or adjustments

Please submit the next revision.

> On Tue, 21 Nov 2023 at 21:03, Lee Jones <lee@kernel.org> wrote:
> >
> > On Mon, 20 Nov 2023, Naresh Solanki wrote:
> >
> > > Hi
> > >
> > > On Fri, 17 Nov 2023 at 17:45, Lee Jones <lee@kernel.org> wrote:
> > > >
> > > > On Thu, 09 Nov 2023, Naresh Solanki wrote:
> > > >
> > > > > Hey Lee,
> > > > >
> > > > > Is there anything specific you'd suggest changing in the current
> > > > > patchset, or are we good to proceed?
> > > >
> > > > What do you mean by proceed?
> > > >
> > > > You are good to make changes and submit a subsequent version.
> > > >
> > > > Not entirely sure what you're asking.
> > >
> > > As a follow up on previous discussion regarding use of DEFER on probe
> > > if regmap isn't initialized, the implementation was based on other similar
> > > drivers & hence it was retained although its not needed due to dependencies.
> > >
> > > I'm not entirely sure to keep the regmap check or make another
> > > patch revision with regmap check removed ?
> >
> > You tell me.
> >
> > You should understand the device you're attempting to support along with
> > the code you're authoring and its subsequent implications.  If you don't
> > know what a section of code does or whether/why it's required, why did
> > you write it?
> >
> > --
> > Lee Jones [李琼斯]

-- 
Lee Jones [李琼斯]

Re: [RESEND PATCH v3] leds: max5970: Add support for max5970

[Naresh Solanki]

Hi Lee

On Wed, 22 Nov 2023 at 17:20, Lee Jones <lee@kernel.org> wrote:
>
> Please read this:
>
>   https://subspace.kernel.org/etiquette.html#do-not-top-post-when-replying
Ack
>
> On Tue, 21 Nov 2023, Naresh Solanki wrote:
>
> > Hi Lee,
> >
> > Thank you for your insights. I appreciate your guidance on the matter.
> > Yes will rewrite the change as below:
> >
> >         regmap = dev_get_regmap(pdev->dev.parent, NULL);
> >         if (!regmap)
> >                 return -ENODEV;
> >
> > I believe this modification aligns with your suggestion. Please let me
> > know if this meets the requirements or if you have any further
> > suggestions or adjustments
>
> Please submit the next revision.
Ack

Regards,
Naresh
>
> > On Tue, 21 Nov 2023 at 21:03, Lee Jones <lee@kernel.org> wrote:
> > >
> > > On Mon, 20 Nov 2023, Naresh Solanki wrote:
> > >
> > > > Hi
> > > >
> > > > On Fri, 17 Nov 2023 at 17:45, Lee Jones <lee@kernel.org> wrote:
> > > > >
> > > > > On Thu, 09 Nov 2023, Naresh Solanki wrote:
> > > > >
> > > > > > Hey Lee,
> > > > > >
> > > > > > Is there anything specific you'd suggest changing in the current
> > > > > > patchset, or are we good to proceed?
> > > > >
> > > > > What do you mean by proceed?
> > > > >
> > > > > You are good to make changes and submit a subsequent version.
> > > > >
> > > > > Not entirely sure what you're asking.
> > > >
> > > > As a follow up on previous discussion regarding use of DEFER on probe
> > > > if regmap isn't initialized, the implementation was based on other similar
> > > > drivers & hence it was retained although its not needed due to dependencies.
> > > >
> > > > I'm not entirely sure to keep the regmap check or make another
> > > > patch revision with regmap check removed ?
> > >
> > > You tell me.
> > >
> > > You should understand the device you're attempting to support along with
> > > the code you're authoring and its subsequent implications.  If you don't
> > > know what a section of code does or whether/why it's required, why did
> > > you write it?
> > >
> > > --
> > > Lee Jones [李琼斯]
>
> --
> Lee Jones [李琼斯]