diff mbox series

[v5] leds: max597x: Add support for max597x

Message ID 20230417094035.998965-1-Naresh.Solanki@9elements.com
State New
Headers
Series [v5] leds: max597x: Add support for max597x |

Commit Message

Naresh Solanki April 17, 2023, 9:40 a.m. UTC 
  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 = "ssd0:green";
                    default-state = "on";
                };
                led@1 {
                    reg = <1>;
                    label = "ssd1:green";
                    default-state = "on";
                };
            };
        };
    };

Signed-off-by: Patrick Rudolph <patrick.rudolph@9elements.com>
Signed-off-by: Naresh Solanki <Naresh.Solanki@9elements.com>
...
Changes in V5:
- Update commit message
- Fix comments
- Add necessary new line
Changes in V4:
- Remove unwanted preinitialise
- Remove unneeded line breaks
- Fix variable name to avoid confusion
- Update module description to mention LED driver.
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 | 115 ++++++++++++++++++++++++++++++++++++
 3 files changed, 127 insertions(+)
 create mode 100644 drivers/leds/leds-max597x.c


base-commit: 9d8d0d98885abba451d7ffc4885236d14ead3c9a

Comments

Lee Jones April 20, 2023, 11:50 a.m. UTC | #1 
On Mon, 17 Apr 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 = "ssd0:green";
>                     default-state = "on";
>                 };
>                 led@1 {
>                     reg = <1>;
>                     label = "ssd1:green";
>                     default-state = "on";
>                 };
>             };
>         };
>     };

Where is the DT binding document for this?

> Signed-off-by: Patrick Rudolph <patrick.rudolph@9elements.com>
> Signed-off-by: Naresh Solanki <Naresh.Solanki@9elements.com>
> ...
> Changes in V5:
> - Update commit message
> - Fix comments
> - Add necessary new line
> Changes in V4:
> - Remove unwanted preinitialise
> - Remove unneeded line breaks
> - Fix variable name to avoid confusion
> - Update module description to mention LED driver.
> 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 | 115 ++++++++++++++++++++++++++++++++++++
>  3 files changed, 127 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..edbd43018822
> --- /dev/null
> +++ b/drivers/leds/leds-max597x.c
> @@ -0,0 +1,115 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Device driver for leds in MAX5970 and MAX5978 IC

"MAX5970 and MAX5978 IC LED support"

> + * 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, cdev)
> +
> +struct max597x_led {
> +	struct regmap *regmap;
> +	struct led_classdev cdev;
> +	unsigned int index;
> +};
> +
> +static int max597x_led_set_brightness(struct led_classdev *cdev,
> +				      enum led_brightness brightness)
> +{
> +	struct max597x_led *ddata = ldev_to_maxled(cdev);
> +	int ret, val;
> +
> +	if (!ddata->regmap)
> +		return -ENODEV;
> +
> +	/* 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 max597x_setup_led(struct device *dev, struct regmap *regmap, struct device_node *nc,
> +			     u32 reg)
> +{
> +	struct max597x_led *ddata;
> +	int ret;
> +
> +	ddata = devm_kzalloc(dev, sizeof(struct max597x_led), GFP_KERNEL);
> +	if (!ddata)
> +		return -ENOMEM;
> +
> +	if (of_property_read_string(nc, "label", &ddata->cdev.name))
> +		ddata->cdev.name = nc->name;
> +
> +	ddata->cdev.max_brightness = 1;
> +	ddata->cdev.brightness_set_blocking = max597x_led_set_brightness;
> +	ddata->cdev.default_trigger = "none";
> +	ddata->index = reg;
> +	ddata->regmap = regmap;
> +
> +	ret = devm_led_classdev_register(dev, &ddata->cdev);
> +	if (ret)
> +		dev_err(dev, "Error initializing LED %s", ddata->cdev.name);
> +
> +	return ret;
> +}
> +
> +static int max597x_led_probe(struct platform_device *pdev)
> +{
> +	struct device_node *np = dev_of_node(pdev->dev.parent);

My previous question about having its own compatible string was ignored.

> +	struct regmap *regmap;
> +	struct device_node *led_node;
> +	struct device_node *child;
> +	int ret = 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 >= 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);

You've ignored my previous review.

> +	}
> +
> +	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 LED driver");
> +MODULE_LICENSE("GPL");
> 
> base-commit: 9d8d0d98885abba451d7ffc4885236d14ead3c9a
> -- 
> 2.39.1
>
Naresh Solanki April 20, 2023, 12:19 p.m. UTC | #2 
Hi Lee,

On 20-04-2023 05:20 pm, Lee Jones wrote:
> On Mon, 17 Apr 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 = "ssd0:green";
>>                      default-state = "on";
>>                  };
>>                  led@1 {
>>                      reg = <1>;
>>                      label = "ssd1:green";
>>                      default-state = "on";
>>                  };
>>              };
>>          };
>>      };
> 
> Where is the DT binding document for this?
> 
>> Signed-off-by: Patrick Rudolph <patrick.rudolph@9elements.com>
>> Signed-off-by: Naresh Solanki <Naresh.Solanki@9elements.com>
>> ...
>> Changes in V5:
>> - Update commit message
>> - Fix comments
>> - Add necessary new line
>> Changes in V4:
>> - Remove unwanted preinitialise
>> - Remove unneeded line breaks
>> - Fix variable name to avoid confusion
>> - Update module description to mention LED driver.
>> 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 | 115 ++++++++++++++++++++++++++++++++++++
>>   3 files changed, 127 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..edbd43018822
>> --- /dev/null
>> +++ b/drivers/leds/leds-max597x.c
>> @@ -0,0 +1,115 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * Device driver for leds in MAX5970 and MAX5978 IC
> 
> "MAX5970 and MAX5978 IC LED support"
> 
>> + * 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, cdev)
>> +
>> +struct max597x_led {
>> +	struct regmap *regmap;
>> +	struct led_classdev cdev;
>> +	unsigned int index;
>> +};
>> +
>> +static int max597x_led_set_brightness(struct led_classdev *cdev,
>> +				      enum led_brightness brightness)
>> +{
>> +	struct max597x_led *ddata = ldev_to_maxled(cdev);
>> +	int ret, val;
>> +
>> +	if (!ddata->regmap)
>> +		return -ENODEV;
>> +
>> +	/* 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 max597x_setup_led(struct device *dev, struct regmap *regmap, struct device_node *nc,
>> +			     u32 reg)
>> +{
>> +	struct max597x_led *ddata;
>> +	int ret;
>> +
>> +	ddata = devm_kzalloc(dev, sizeof(struct max597x_led), GFP_KERNEL);
>> +	if (!ddata)
>> +		return -ENOMEM;
>> +
>> +	if (of_property_read_string(nc, "label", &ddata->cdev.name))
>> +		ddata->cdev.name = nc->name;
>> +
>> +	ddata->cdev.max_brightness = 1;
>> +	ddata->cdev.brightness_set_blocking = max597x_led_set_brightness;
>> +	ddata->cdev.default_trigger = "none";
>> +	ddata->index = reg;
>> +	ddata->regmap = regmap;
>> +
>> +	ret = devm_led_classdev_register(dev, &ddata->cdev);
>> +	if (ret)
>> +		dev_err(dev, "Error initializing LED %s", ddata->cdev.name);
>> +
>> +	return ret;
>> +}
>> +
>> +static int max597x_led_probe(struct platform_device *pdev)
>> +{
>> +	struct device_node *np = dev_of_node(pdev->dev.parent);
> 
> My previous question about having its own compatible string was ignored.
As previously stated, the MFD driver for max597x already has a 
compatible string that serves its purpose, so I opted not to include a 
separate compatible string for the leaf driver.
Prior to implementing this approach, I reviewed other implementations 
within the MFD driver, such as the sy7636 which uses the similar 
approach in leaf driver.

> 
>> +	struct regmap *regmap;
>> +	struct device_node *led_node;
>> +	struct device_node *child;
>> +	int ret = 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 >= 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);
> 
> You've ignored my previous review.
I would like to clarify that I did not intend to overlook your previous 
review comment. Rather, I have taken it into consideration and evaluated 
it in the context of the current approach, which is derived from the 
code in other LED drivers.
To eliminate any potential confusion, it may be best to adopt a 
consistent approach that all LED drivers should adhere to in similar 
circumstances.
> 
>> +	}
>> +
>> +	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 LED driver");
>> +MODULE_LICENSE("GPL");
>>
>> base-commit: 9d8d0d98885abba451d7ffc4885236d14ead3c9a
>> -- 
>> 2.39.1
>>
> 
Regards,
Naresh
Lee Jones April 20, 2023, 1:54 p.m. UTC | #3 
On Thu, 20 Apr 2023, Naresh Solanki wrote:

> Hi Lee,
> 
> On 20-04-2023 05:20 pm, Lee Jones wrote:
> > On Mon, 17 Apr 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 = "ssd0:green";
> > >                      default-state = "on";
> > >                  };
> > >                  led@1 {
> > >                      reg = <1>;
> > >                      label = "ssd1:green";
> > >                      default-state = "on";
> > >                  };
> > >              };
> > >          };
> > >      };
> > 
> > Where is the DT binding document for this?


? <---------------------------------------------------------------------------

^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^


> > > Signed-off-by: Patrick Rudolph <patrick.rudolph@9elements.com>
> > > Signed-off-by: Naresh Solanki <Naresh.Solanki@9elements.com>
> > > ...
> > > Changes in V5:
> > > - Update commit message
> > > - Fix comments
> > > - Add necessary new line
> > > Changes in V4:
> > > - Remove unwanted preinitialise
> > > - Remove unneeded line breaks
> > > - Fix variable name to avoid confusion
> > > - Update module description to mention LED driver.
> > > 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 | 115 ++++++++++++++++++++++++++++++++++++
> > >   3 files changed, 127 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..edbd43018822
> > > --- /dev/null
> > > +++ b/drivers/leds/leds-max597x.c
> > > @@ -0,0 +1,115 @@
> > > +// SPDX-License-Identifier: GPL-2.0
> > > +/*
> > > + * Device driver for leds in MAX5970 and MAX5978 IC
> > 
> > "MAX5970 and MAX5978 IC LED support"
> > 
> > > + * 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, cdev)
> > > +
> > > +struct max597x_led {
> > > +	struct regmap *regmap;
> > > +	struct led_classdev cdev;
> > > +	unsigned int index;
> > > +};
> > > +
> > > +static int max597x_led_set_brightness(struct led_classdev *cdev,
> > > +				      enum led_brightness brightness)
> > > +{
> > > +	struct max597x_led *ddata = ldev_to_maxled(cdev);
> > > +	int ret, val;
> > > +
> > > +	if (!ddata->regmap)
> > > +		return -ENODEV;
> > > +
> > > +	/* 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 max597x_setup_led(struct device *dev, struct regmap *regmap, struct device_node *nc,
> > > +			     u32 reg)
> > > +{
> > > +	struct max597x_led *ddata;
> > > +	int ret;
> > > +
> > > +	ddata = devm_kzalloc(dev, sizeof(struct max597x_led), GFP_KERNEL);
> > > +	if (!ddata)
> > > +		return -ENOMEM;
> > > +
> > > +	if (of_property_read_string(nc, "label", &ddata->cdev.name))
> > > +		ddata->cdev.name = nc->name;
> > > +
> > > +	ddata->cdev.max_brightness = 1;
> > > +	ddata->cdev.brightness_set_blocking = max597x_led_set_brightness;
> > > +	ddata->cdev.default_trigger = "none";
> > > +	ddata->index = reg;
> > > +	ddata->regmap = regmap;
> > > +
> > > +	ret = devm_led_classdev_register(dev, &ddata->cdev);
> > > +	if (ret)
> > > +		dev_err(dev, "Error initializing LED %s", ddata->cdev.name);
> > > +
> > > +	return ret;
> > > +}
> > > +
> > > +static int max597x_led_probe(struct platform_device *pdev)
> > > +{
> > > +	struct device_node *np = dev_of_node(pdev->dev.parent);
> > 
> > My previous question about having its own compatible string was ignored.
> As previously stated, the MFD driver for max597x already has a compatible
> string that serves its purpose, so I opted not to include a separate
> compatible string for the leaf driver.
> Prior to implementing this approach, I reviewed other implementations within
> the MFD driver, such as the sy7636 which uses the similar approach in leaf
> driver.

I'm not saying it's never been done.  However, most of the time
leaf/child devices have their own compatible string and node.

> > > +	struct regmap *regmap;
> > > +	struct device_node *led_node;
> > > +	struct device_node *child;
> > > +	int ret = 0;
> > > +
> > > +	regmap = dev_get_regmap(pdev->dev.parent, NULL);
> > > +	if (!regmap)
> > > +		return -EPROBE_DEFER;

Will other child devices have be using the parent's regmap too?

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

It's odd for a device to be referring to itself as the "child".

> > > +	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);
> > 
> > You've ignored my previous review.
> I would like to clarify that I did not intend to overlook your previous
> review comment. Rather, I have taken it into consideration and evaluated it
> in the context of the current approach, which is derived from the code in
> other LED drivers.
> To eliminate any potential confusion, it may be best to adopt a consistent
> approach that all LED drivers should adhere to in similar circumstances.

I agree.  That's start with this one.

You should not error and continue.  If you encounter an error, please
unwind what you have done before and return the error.

> > > +	}
> > > +
> > > +	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 LED driver");
> > > +MODULE_LICENSE("GPL");
> > > 
> > > base-commit: 9d8d0d98885abba451d7ffc4885236d14ead3c9a
> > > -- 
> > > 2.39.1
> > > 
> > 
> Regards,
> Naresh
Naresh Solanki April 20, 2023, 5:20 p.m. UTC | #4 
Hi Lee,

On 20-04-2023 07:24 pm, Lee Jones wrote:
> On Thu, 20 Apr 2023, Naresh Solanki wrote:
> 
>> Hi Lee,
>>
>> On 20-04-2023 05:20 pm, Lee Jones wrote:
>>> On Mon, 17 Apr 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 = "ssd0:green";
>>>>                       default-state = "on";
>>>>                   };
>>>>                   led@1 {
>>>>                       reg = <1>;
>>>>                       label = "ssd1:green";
>>>>                       default-state = "on";
>>>>                   };
>>>>               };
>>>>           };
>>>>       };
>>>
>>> Where is the DT binding document for this?
https://git.kernel.org/pub/scm/linux/kernel/git/lee/mfd.git/tree/Documentation/devicetree/bindings/mfd/maxim,max5970.yaml?h=for-mfd-next
> 
> 
> ? <---------------------------------------------------------------------------
> 
> ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> 
> 
>>>> Signed-off-by: Patrick Rudolph <patrick.rudolph@9elements.com>
>>>> Signed-off-by: Naresh Solanki <Naresh.Solanki@9elements.com>
>>>> ...
>>>> Changes in V5:
>>>> - Update commit message
>>>> - Fix comments
>>>> - Add necessary new line
>>>> Changes in V4:
>>>> - Remove unwanted preinitialise
>>>> - Remove unneeded line breaks
>>>> - Fix variable name to avoid confusion
>>>> - Update module description to mention LED driver.
>>>> 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 | 115 ++++++++++++++++++++++++++++++++++++
>>>>    3 files changed, 127 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..edbd43018822
>>>> --- /dev/null
>>>> +++ b/drivers/leds/leds-max597x.c
>>>> @@ -0,0 +1,115 @@
>>>> +// SPDX-License-Identifier: GPL-2.0
>>>> +/*
>>>> + * Device driver for leds in MAX5970 and MAX5978 IC
>>>
>>> "MAX5970 and MAX5978 IC LED support"
>>>
>>>> + * 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, cdev)
>>>> +
>>>> +struct max597x_led {
>>>> +	struct regmap *regmap;
>>>> +	struct led_classdev cdev;
>>>> +	unsigned int index;
>>>> +};
>>>> +
>>>> +static int max597x_led_set_brightness(struct led_classdev *cdev,
>>>> +				      enum led_brightness brightness)
>>>> +{
>>>> +	struct max597x_led *ddata = ldev_to_maxled(cdev);
>>>> +	int ret, val;
>>>> +
>>>> +	if (!ddata->regmap)
>>>> +		return -ENODEV;
>>>> +
>>>> +	/* 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 max597x_setup_led(struct device *dev, struct regmap *regmap, struct device_node *nc,
>>>> +			     u32 reg)
>>>> +{
>>>> +	struct max597x_led *ddata;
>>>> +	int ret;
>>>> +
>>>> +	ddata = devm_kzalloc(dev, sizeof(struct max597x_led), GFP_KERNEL);
>>>> +	if (!ddata)
>>>> +		return -ENOMEM;
>>>> +
>>>> +	if (of_property_read_string(nc, "label", &ddata->cdev.name))
>>>> +		ddata->cdev.name = nc->name;
>>>> +
>>>> +	ddata->cdev.max_brightness = 1;
>>>> +	ddata->cdev.brightness_set_blocking = max597x_led_set_brightness;
>>>> +	ddata->cdev.default_trigger = "none";
>>>> +	ddata->index = reg;
>>>> +	ddata->regmap = regmap;
>>>> +
>>>> +	ret = devm_led_classdev_register(dev, &ddata->cdev);
>>>> +	if (ret)
>>>> +		dev_err(dev, "Error initializing LED %s", ddata->cdev.name);
>>>> +
>>>> +	return ret;
>>>> +}
>>>> +
>>>> +static int max597x_led_probe(struct platform_device *pdev)
>>>> +{
>>>> +	struct device_node *np = dev_of_node(pdev->dev.parent);
>>>
>>> My previous question about having its own compatible string was ignored.
>> As previously stated, the MFD driver for max597x already has a compatible
>> string that serves its purpose, so I opted not to include a separate
>> compatible string for the leaf driver.
>> Prior to implementing this approach, I reviewed other implementations within
>> the MFD driver, such as the sy7636 which uses the similar approach in leaf
>> driver.
> 
> I'm not saying it's never been done.  However, most of the time
> leaf/child devices have their own compatible string and node.
True.
> 
>>>> +	struct regmap *regmap;
>>>> +	struct device_node *led_node;
>>>> +	struct device_node *child;
>>>> +	int ret = 0;
>>>> +
>>>> +	regmap = dev_get_regmap(pdev->dev.parent, NULL);
>>>> +	if (!regmap)
>>>> +		return -EPROBE_DEFER;
> 
> Will other child devices have be using the parent's regmap too?
Yes
> 
>>>> +	led_node = of_get_child_by_name(np, "leds");
>>>> +	if (!led_node)
>>>> +		return -ENODEV;
> 
> It's odd for a device to be referring to itself as the "child".
As this is leaf driver, LED specific info is present in "leds" node in DT.
> 
>>>> +	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);
>>>
>>> You've ignored my previous review.
>> I would like to clarify that I did not intend to overlook your previous
>> review comment. Rather, I have taken it into consideration and evaluated it
>> in the context of the current approach, which is derived from the code in
>> other LED drivers.
>> To eliminate any potential confusion, it may be best to adopt a consistent
>> approach that all LED drivers should adhere to in similar circumstances.
> 
> I agree.  That's start with this one.
> 
> You should not error and continue.  If you encounter an error, please
> unwind what you have done before and return the error.
Ack.

> 
>>>> +	}
>>>> +
>>>> +	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 LED driver");
>>>> +MODULE_LICENSE("GPL");
>>>>
>>>> base-commit: 9d8d0d98885abba451d7ffc4885236d14ead3c9a
>>>> -- 
>>>> 2.39.1
>>>>
>>>
>> Regards,
>> Naresh
> 
Regards,
Naresh
Lee Jones April 21, 2023, 7:19 a.m. UTC | #5 
On Thu, 20 Apr 2023, Naresh Solanki wrote:

> Hi Lee,
> 
> On 20-04-2023 07:24 pm, Lee Jones wrote:
> > On Thu, 20 Apr 2023, Naresh Solanki wrote:
> > 
> > > Hi Lee,
> > > 
> > > On 20-04-2023 05:20 pm, Lee Jones wrote:
> > > > On Mon, 17 Apr 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 = "ssd0:green";
> > > > >                       default-state = "on";
> > > > >                   };
> > > > >                   led@1 {
> > > > >                       reg = <1>;
> > > > >                       label = "ssd1:green";
> > > > >                       default-state = "on";
> > > > >                   };
> > > > >               };
> > > > >           };
> > > > >       };
> > > > 
> > > > Where is the DT binding document for this?
> https://git.kernel.org/pub/scm/linux/kernel/git/lee/mfd.git/tree/Documentation/devicetree/bindings/mfd/maxim,max5970.yaml?h=for-mfd-next

You need to update it.  It is different to the one you supplied here.

> > > > > Signed-off-by: Patrick Rudolph <patrick.rudolph@9elements.com>
> > > > > Signed-off-by: Naresh Solanki <Naresh.Solanki@9elements.com>
> > > > > ...
> > > > > Changes in V5:
> > > > > - Update commit message
> > > > > - Fix comments
> > > > > - Add necessary new line
> > > > > Changes in V4:
> > > > > - Remove unwanted preinitialise
> > > > > - Remove unneeded line breaks
> > > > > - Fix variable name to avoid confusion
> > > > > - Update module description to mention LED driver.
> > > > > 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 | 115 ++++++++++++++++++++++++++++++++++++
> > > > >    3 files changed, 127 insertions(+)
> > > > >    create mode 100644 drivers/leds/leds-max597x.c

[...]

> > > > +	led_node = of_get_child_by_name(np, "leds");
> > > > > +	if (!led_node)
> > > > > +		return -ENODEV;
> > 
> > It's odd for a device to be referring to itself as the "child".
> As this is leaf driver, LED specific info is present in "leds" node in DT.

I'm aware of the architecture.

If you give the LEDs driver it's own compatible you don't need to keep
doing this self->parent->child level-jumping craziness to obtain
resources.
Naresh Solanki April 24, 2023, 9:40 a.m. UTC | #6 
Hi,

On 21-04-2023 12:49 pm, Lee Jones wrote:
> On Thu, 20 Apr 2023, Naresh Solanki wrote:
> 
>> Hi Lee,
>>
>> On 20-04-2023 07:24 pm, Lee Jones wrote:
>>> On Thu, 20 Apr 2023, Naresh Solanki wrote:
>>>
>>>> Hi Lee,
>>>>
>>>> On 20-04-2023 05:20 pm, Lee Jones wrote:
>>>>> On Mon, 17 Apr 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 = "ssd0:green";
>>>>>>                        default-state = "on";
>>>>>>                    };
>>>>>>                    led@1 {
>>>>>>                        reg = <1>;
>>>>>>                        label = "ssd1:green";
>>>>>>                        default-state = "on";
>>>>>>                    };
>>>>>>                };
>>>>>>            };
>>>>>>        };
>>>>>
>>>>> Where is the DT binding document for this?
>> https://git.kernel.org/pub/scm/linux/kernel/git/lee/mfd.git/tree/Documentation/devicetree/bindings/mfd/maxim,max5970.yaml?h=for-mfd-next
> 
> You need to update it.  It is different to the one you supplied here.
Ack.
> 
>>>>>> Signed-off-by: Patrick Rudolph <patrick.rudolph@9elements.com>
>>>>>> Signed-off-by: Naresh Solanki <Naresh.Solanki@9elements.com>
>>>>>> ...
>>>>>> Changes in V5:
>>>>>> - Update commit message
>>>>>> - Fix comments
>>>>>> - Add necessary new line
>>>>>> Changes in V4:
>>>>>> - Remove unwanted preinitialise
>>>>>> - Remove unneeded line breaks
>>>>>> - Fix variable name to avoid confusion
>>>>>> - Update module description to mention LED driver.
>>>>>> 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 | 115 ++++++++++++++++++++++++++++++++++++
>>>>>>     3 files changed, 127 insertions(+)
>>>>>>     create mode 100644 drivers/leds/leds-max597x.c
> 
> [...]
> 
>>>>> +	led_node = of_get_child_by_name(np, "leds");
>>>>>> +	if (!led_node)
>>>>>> +		return -ENODEV;
>>>
>>> It's odd for a device to be referring to itself as the "child".
>> As this is leaf driver, LED specific info is present in "leds" node in DT.
> 
> I'm aware of the architecture.
> 
> If you give the LEDs driver it's own compatible you don't need to keep
> doing this self->parent->child level-jumping craziness to obtain
> resources.
 From my understanding, it is preferable to have only one compatible per 
chip(in this case MFD driver), and the leaf driver can leverage it 
unless it is not serving the purpose/breaks something.
I think this is acceptable as long as it doesn't creates any 
issue/breaks anything.
> 


Regards,
Naresh
Naresh Solanki April 26, 2023, 9:21 a.m. UTC | #7 
Hi Lee,

On 24-04-2023 03:10 pm, Naresh Solanki wrote:
> Hi,
> 
> On 21-04-2023 12:49 pm, Lee Jones wrote:
>> On Thu, 20 Apr 2023, Naresh Solanki wrote:
>>
>>> Hi Lee,
>>>
>>> On 20-04-2023 07:24 pm, Lee Jones wrote:
>>>> On Thu, 20 Apr 2023, Naresh Solanki wrote:
>>>>
>>>>> Hi Lee,
>>>>>
>>>>> On 20-04-2023 05:20 pm, Lee Jones wrote:
>>>>>> On Mon, 17 Apr 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 = "ssd0:green";
>>>>>>>                        default-state = "on";
>>>>>>>                    };
>>>>>>>                    led@1 {
>>>>>>>                        reg = <1>;
>>>>>>>                        label = "ssd1:green";
>>>>>>>                        default-state = "on";
>>>>>>>                    };
>>>>>>>                };
>>>>>>>            };
>>>>>>>        };
>>>>>>
>>>>>> Where is the DT binding document for this?
>>> https://git.kernel.org/pub/scm/linux/kernel/git/lee/mfd.git/tree/Documentation/devicetree/bindings/mfd/maxim,max5970.yaml?h=for-mfd-next
>>
>> You need to update it.  It is different to the one you supplied here.
> Ack.
>>
>>>>>>> Signed-off-by: Patrick Rudolph <patrick.rudolph@9elements.com>
>>>>>>> Signed-off-by: Naresh Solanki <Naresh.Solanki@9elements.com>
>>>>>>> ...
>>>>>>> Changes in V5:
>>>>>>> - Update commit message
>>>>>>> - Fix comments
>>>>>>> - Add necessary new line
>>>>>>> Changes in V4:
>>>>>>> - Remove unwanted preinitialise
>>>>>>> - Remove unneeded line breaks
>>>>>>> - Fix variable name to avoid confusion
>>>>>>> - Update module description to mention LED driver.
>>>>>>> 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 | 115 
>>>>>>> ++++++++++++++++++++++++++++++++++++
>>>>>>>     3 files changed, 127 insertions(+)
>>>>>>>     create mode 100644 drivers/leds/leds-max597x.c
>>
>> [...]
>>
>>>>>> +    led_node = of_get_child_by_name(np, "leds");
>>>>>>> +    if (!led_node)
>>>>>>> +        return -ENODEV;
>>>>
>>>> It's odd for a device to be referring to itself as the "child".
>>> As this is leaf driver, LED specific info is present in "leds" node 
>>> in DT.
>>
>> I'm aware of the architecture.
>>
>> If you give the LEDs driver it's own compatible you don't need to keep
>> doing this self->parent->child level-jumping craziness to obtain
>> resources.
>  From my understanding, it is preferable to have only one compatible per 
> chip(in this case MFD driver), and the leaf driver can leverage it 
> unless it is not serving the purpose/breaks something.
> I think this is acceptable as long as it doesn't creates any 
> issue/breaks anything.
I just wanted to kindly follow up and check if we are aligned with the 
approach. And kindly let me know if you have any concerns regarding this.

Thank you very much for your time and I look forward to hearing back 
from you soon :)
>>
> 
> 
> Regards,
> Naresh
diff mbox series

Patch

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..edbd43018822
--- /dev/null
+++ b/drivers/leds/leds-max597x.c
@@ -0,0 +1,115 @@ 
+// 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, cdev)
+
+struct max597x_led {
+	struct regmap *regmap;
+	struct led_classdev cdev;
+	unsigned int index;
+};
+
+static int max597x_led_set_brightness(struct led_classdev *cdev,
+				      enum led_brightness brightness)
+{
+	struct max597x_led *ddata = ldev_to_maxled(cdev);
+	int ret, val;
+
+	if (!ddata->regmap)
+		return -ENODEV;
+
+	/* 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 max597x_setup_led(struct device *dev, struct regmap *regmap, struct device_node *nc,
+			     u32 reg)
+{
+	struct max597x_led *ddata;
+	int ret;
+
+	ddata = devm_kzalloc(dev, sizeof(struct max597x_led), GFP_KERNEL);
+	if (!ddata)
+		return -ENOMEM;
+
+	if (of_property_read_string(nc, "label", &ddata->cdev.name))
+		ddata->cdev.name = nc->name;
+
+	ddata->cdev.max_brightness = 1;
+	ddata->cdev.brightness_set_blocking = max597x_led_set_brightness;
+	ddata->cdev.default_trigger = "none";
+	ddata->index = reg;
+	ddata->regmap = regmap;
+
+	ret = devm_led_classdev_register(dev, &ddata->cdev);
+	if (ret)
+		dev_err(dev, "Error initializing LED %s", ddata->cdev.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;
+	struct device_node *led_node;
+	struct device_node *child;
+	int ret = 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 >= 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 LED driver");
+MODULE_LICENSE("GPL");