Message ID | af1fb3e010d5f34502d354369b88fa28639f587d.1571302099.git.matti.vaittinen@fi.rohmeurope.com (mailing list archive) |
---|---|
State | Superseded, archived |
Headers | show |
Series | Support ROHM BD71828 PMIC | expand |
Matt On 10/17/19 4:53 AM, Matti Vaittinen wrote: > ROHM BD71828 power management IC has two LED outputs for charge status > and button pressing indications. The LED outputs can also be forced > bs SW so add driver allowing to use these LEDs for other indications s/bs/by > as well. > > Leds are controlled by SW using 'Force ON' bits. Please note the > constrains mentioned in data-sheet: > 1. If one LED is forced ON - then also the other LED is forced. > => You can't use SW control to force ON one LED and allow HW > to control the other. > 2. You can't force both LEDs OFF. If the FORCE bit for both LED's is > zero, then LEDs are controlled by HW and indicate button/charger > states as explained in data-sheet. > > Signed-off-by: Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com> > --- > drivers/leds/Kconfig | 10 ++++ > drivers/leds/Makefile | 1 + > drivers/leds/leds-bd71828.c | 97 +++++++++++++++++++++++++++++++++++++ > 3 files changed, 108 insertions(+) > create mode 100644 drivers/leds/leds-bd71828.c > > diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig > index b0fdeef10bd9..ec59f28bcb39 100644 > --- a/drivers/leds/Kconfig > +++ b/drivers/leds/Kconfig > @@ -529,6 +529,16 @@ config LEDS_BD2802 > This option enables support for BD2802GU RGB LED driver chips > accessed via the I2C bus. > > +config LEDS_BD71828 > + tristate "LED driver for LED pins on ROHM BD71828 PMIC" > + depends on LEDS_CLASS doesn't this have a dependency on MFD_ROHM_BD71828 > + depends on I2C > + help > + This option enables support for LED outputs located on ROHM > + BD71828 power management IC. ROHM BD71828 has two led output pins > + which can be left to indicate HW states or controlled by SW. Say > + yes here if you want to enable SW control for these LEDs. > + Add module statement > config LEDS_INTEL_SS4200 > tristate "LED driver for Intel NAS SS4200 series" > depends on LEDS_CLASS > diff --git a/drivers/leds/Makefile b/drivers/leds/Makefile > index 41fb073a39c1..2a8f6a8e4c7c 100644 > --- a/drivers/leds/Makefile > +++ b/drivers/leds/Makefile > @@ -15,6 +15,7 @@ obj-$(CONFIG_LEDS_AN30259A) += leds-an30259a.o > obj-$(CONFIG_LEDS_BCM6328) += leds-bcm6328.o > obj-$(CONFIG_LEDS_BCM6358) += leds-bcm6358.o > obj-$(CONFIG_LEDS_BD2802) += leds-bd2802.o > +obj-$(CONFIG_LEDS_BD71828) += leds-bd71828.o > obj-$(CONFIG_LEDS_CPCAP) += leds-cpcap.o > obj-$(CONFIG_LEDS_LOCOMO) += leds-locomo.o > obj-$(CONFIG_LEDS_LM3530) += leds-lm3530.o > diff --git a/drivers/leds/leds-bd71828.c b/drivers/leds/leds-bd71828.c > new file mode 100644 > index 000000000000..2427619444f5 > --- /dev/null > +++ b/drivers/leds/leds-bd71828.c > @@ -0,0 +1,97 @@ > +// SPDX-License-Identifier: GPL-2.0 > +// Copyright (C) 2019 ROHM Semiconductors > + > +#include <linux/device.h> > +#include <linux/err.h> > +#include <linux/kernel.h> > +#include <linux/leds.h> > +#include <linux/mfd/rohm-bd71828.h> > +#include <linux/module.h> > +#include <linux/platform_device.h> > +#include <linux/regmap.h> > +#include <linux/slab.h> > + > +#define BD71828_LED_TO_DATA(l) ((l)->id == ID_GREEN_LED ? \ > + container_of((l), struct bd71828_leds, green) : \ > + container_of((l), struct bd71828_leds, amber)) I don't think we should be defining the color as the variable. The outputs can drive any color LED. > + > +enum { > + ID_GREEN_LED, > + ID_AMBER_LED, > + ID_NMBR_OF, > +}; > + Please use the color_id in linux/include/dt-bindings/leds/common.h > +struct bd71828_led { > + int id; > + struct led_classdev l; > + u8 force_mask; > +}; > + > +struct bd71828_leds { > + struct rohm_regmap_dev *bd71828; > + struct bd71828_led green; > + struct bd71828_led amber; > +}; > + > +static int bd71828_led_brightness_set(struct led_classdev *led_cdev, > + enum led_brightness value) > +{ > + struct bd71828_led *l = container_of(led_cdev, struct bd71828_led, l); > + struct bd71828_leds *data; > + unsigned int val = BD71828_LED_OFF; > + > + data = BD71828_LED_TO_DATA(l); > + if (value != LED_OFF) > + val = BD71828_LED_ON; > + > + return regmap_update_bits(data->bd71828->regmap, BD71828_REG_LED_CTRL, > + l->force_mask, val); > +} > + > +static int bd71828_led_probe(struct platform_device *pdev) > +{ > + struct rohm_regmap_dev *bd71828; > + struct bd71828_leds *l; > + struct bd71828_led *g, *a; > + static const char *GNAME = "bd71828-green-led"; > + static const char *ANAME = "bd71828-amber-led"; The LED class creates the name it can get it from the DT. > + int ret; > + > + pr_info("bd71828 LED driver probed\n"); > + > + bd71828 = dev_get_drvdata(pdev->dev.parent); > + l = devm_kzalloc(&pdev->dev, sizeof(*l), GFP_KERNEL); > + if (!l) > + return -ENOMEM; > + l->bd71828 = bd71828; > + a = &l->amber; > + g = &l->green; > + a->id = ID_AMBER_LED; > + g->id = ID_GREEN_LED; > + a->force_mask = BD71828_MASK_LED_AMBER; > + g->force_mask = BD71828_MASK_LED_GREEN; > + > + a->l.name = ANAME; > + g->l.name = GNAME; > + a->l.brightness_set_blocking = bd71828_led_brightness_set; > + g->l.brightness_set_blocking = bd71828_led_brightness_set; > + > + ret = devm_led_classdev_register(&pdev->dev, &g->l); > + if (ret) > + return ret; > + > + return devm_led_classdev_register(&pdev->dev, &a->l); > +} > + This looks different. Not sure why you register both LEDs in this probe. You can use the DT to define both LEDs and then each will be probed and registered separately. This is how it is commonly done. You can reference the LM36274 led driver as this is a MFD device to the ti-lmu.c in the MFD directory. > +static struct platform_driver bd71828_led_driver = { > + .driver = { > + .name = "bd71828-led", > + }, > + .probe = bd71828_led_probe, > +}; > + > +module_platform_driver(bd71828_led_driver); > + > +MODULE_AUTHOR("Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com>"); > +MODULE_DESCRIPTION("ROHM BD71828 LED driver"); > +MODULE_LICENSE("GPL"); GPL v2
On 17/10/2019 09:04:48-0500, Dan Murphy wrote: > Matt > > On 10/17/19 4:53 AM, Matti Vaittinen wrote: > > ROHM BD71828 power management IC has two LED outputs for charge status > > and button pressing indications. The LED outputs can also be forced > > bs SW so add driver allowing to use these LEDs for other indications > s/bs/by > > as well. > > > > Leds are controlled by SW using 'Force ON' bits. Please note the > > constrains mentioned in data-sheet: > > 1. If one LED is forced ON - then also the other LED is forced. > > => You can't use SW control to force ON one LED and allow HW > > to control the other. > > 2. You can't force both LEDs OFF. If the FORCE bit for both LED's is > > zero, then LEDs are controlled by HW and indicate button/charger > > states as explained in data-sheet. > > > > Signed-off-by: Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com> > > --- > > drivers/leds/Kconfig | 10 ++++ > > drivers/leds/Makefile | 1 + > > drivers/leds/leds-bd71828.c | 97 +++++++++++++++++++++++++++++++++++++ > > 3 files changed, 108 insertions(+) > > create mode 100644 drivers/leds/leds-bd71828.c > > > > diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig > > index b0fdeef10bd9..ec59f28bcb39 100644 > > --- a/drivers/leds/Kconfig > > +++ b/drivers/leds/Kconfig > > @@ -529,6 +529,16 @@ config LEDS_BD2802 > > This option enables support for BD2802GU RGB LED driver chips > > accessed via the I2C bus. > > +config LEDS_BD71828 > > + tristate "LED driver for LED pins on ROHM BD71828 PMIC" > > + depends on LEDS_CLASS > doesn't this have a dependency on MFD_ROHM_BD71828 > > + depends on I2C > > + help > > + This option enables support for LED outputs located on ROHM > > + BD71828 power management IC. ROHM BD71828 has two led output pins > > + which can be left to indicate HW states or controlled by SW. Say > > + yes here if you want to enable SW control for these LEDs. > > + > > Add module statement > > > > config LEDS_INTEL_SS4200 > > tristate "LED driver for Intel NAS SS4200 series" > > depends on LEDS_CLASS > > diff --git a/drivers/leds/Makefile b/drivers/leds/Makefile > > index 41fb073a39c1..2a8f6a8e4c7c 100644 > > --- a/drivers/leds/Makefile > > +++ b/drivers/leds/Makefile > > @@ -15,6 +15,7 @@ obj-$(CONFIG_LEDS_AN30259A) += leds-an30259a.o > > obj-$(CONFIG_LEDS_BCM6328) += leds-bcm6328.o > > obj-$(CONFIG_LEDS_BCM6358) += leds-bcm6358.o > > obj-$(CONFIG_LEDS_BD2802) += leds-bd2802.o > > +obj-$(CONFIG_LEDS_BD71828) += leds-bd71828.o > > obj-$(CONFIG_LEDS_CPCAP) += leds-cpcap.o > > obj-$(CONFIG_LEDS_LOCOMO) += leds-locomo.o > > obj-$(CONFIG_LEDS_LM3530) += leds-lm3530.o > > diff --git a/drivers/leds/leds-bd71828.c b/drivers/leds/leds-bd71828.c > > new file mode 100644 > > index 000000000000..2427619444f5 > > --- /dev/null > > +++ b/drivers/leds/leds-bd71828.c > > @@ -0,0 +1,97 @@ > > +// SPDX-License-Identifier: GPL-2.0 > > +// Copyright (C) 2019 ROHM Semiconductors > > + > > +#include <linux/device.h> > > +#include <linux/err.h> > > +#include <linux/kernel.h> > > +#include <linux/leds.h> > > +#include <linux/mfd/rohm-bd71828.h> > > +#include <linux/module.h> > > +#include <linux/platform_device.h> > > +#include <linux/regmap.h> > > +#include <linux/slab.h> > > + > > +#define BD71828_LED_TO_DATA(l) ((l)->id == ID_GREEN_LED ? \ > > + container_of((l), struct bd71828_leds, green) : \ > > + container_of((l), struct bd71828_leds, amber)) > > I don't think we should be defining the color as the variable. The outputs > can drive any color LED. > > > > + > > +enum { > > + ID_GREEN_LED, > > + ID_AMBER_LED, > > + ID_NMBR_OF, > > +}; > > + > > Please use the color_id in linux/include/dt-bindings/leds/common.h > > > > +struct bd71828_led { > > + int id; > > + struct led_classdev l; > > + u8 force_mask; > > +}; > > + > > +struct bd71828_leds { > > + struct rohm_regmap_dev *bd71828; > > + struct bd71828_led green; > > + struct bd71828_led amber; > > +}; > > + > > +static int bd71828_led_brightness_set(struct led_classdev *led_cdev, > > + enum led_brightness value) > > +{ > > + struct bd71828_led *l = container_of(led_cdev, struct bd71828_led, l); > > + struct bd71828_leds *data; > > + unsigned int val = BD71828_LED_OFF; > > + > > + data = BD71828_LED_TO_DATA(l); > > + if (value != LED_OFF) > > + val = BD71828_LED_ON; > > + > > + return regmap_update_bits(data->bd71828->regmap, BD71828_REG_LED_CTRL, > > + l->force_mask, val); > > +} > > + > > +static int bd71828_led_probe(struct platform_device *pdev) > > +{ > > + struct rohm_regmap_dev *bd71828; > > + struct bd71828_leds *l; > > + struct bd71828_led *g, *a; > > + static const char *GNAME = "bd71828-green-led"; > > + static const char *ANAME = "bd71828-amber-led"; > The LED class creates the name it can get it from the DT. > > + int ret; > > + > > + pr_info("bd71828 LED driver probed\n"); > > + > > + bd71828 = dev_get_drvdata(pdev->dev.parent); > > + l = devm_kzalloc(&pdev->dev, sizeof(*l), GFP_KERNEL); > > + if (!l) > > + return -ENOMEM; > > + l->bd71828 = bd71828; > > + a = &l->amber; > > + g = &l->green; > > + a->id = ID_AMBER_LED; > > + g->id = ID_GREEN_LED; > > + a->force_mask = BD71828_MASK_LED_AMBER; > > + g->force_mask = BD71828_MASK_LED_GREEN; > > + > > + a->l.name = ANAME; > > + g->l.name = GNAME; > > + a->l.brightness_set_blocking = bd71828_led_brightness_set; > > + g->l.brightness_set_blocking = bd71828_led_brightness_set; > > + > > + ret = devm_led_classdev_register(&pdev->dev, &g->l); > > + if (ret) > > + return ret; > > + > > + return devm_led_classdev_register(&pdev->dev, &a->l); > > +} > > + > > This looks different. Not sure why you register both LEDs in this probe. > > You can use the DT to define both LEDs and then each will be probed and > registered separately. > > This is how it is commonly done. > > You can reference the LM36274 led driver as this is a MFD device to the > ti-lmu.c in the MFD directory. > > > > +static struct platform_driver bd71828_led_driver = { > > + .driver = { > > + .name = "bd71828-led", > > + }, > > + .probe = bd71828_led_probe, > > +}; > > + > > +module_platform_driver(bd71828_led_driver); > > + > > +MODULE_AUTHOR("Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com>"); > > +MODULE_DESCRIPTION("ROHM BD71828 LED driver"); > > +MODULE_LICENSE("GPL"); > GPL v2 GPL is fine, see bf7fbeeae6db ("module: Cure the MODULE_LICENSE "GPL" vs. "GPL v2" bogosity")
Hello Dan, Thanks for taking the time to check my driver :) I truly appreciate all the help! A "fundamental question" regarding these review comments is whether I should add DT entries for these LEDs or not. I thought I shouldn't but I would like to get a comment from Rob regarding it. On Thu, 2019-10-17 at 09:04 -0500, Dan Murphy wrote: > Matt > > On 10/17/19 4:53 AM, Matti Vaittinen wrote: > > ROHM BD71828 power management IC has two LED outputs for charge > > status > > and button pressing indications. The LED outputs can also be forced > > bs SW so add driver allowing to use these LEDs for other > > indications > s/bs/by > > as well. > > > > Leds are controlled by SW using 'Force ON' bits. Please note the > > constrains mentioned in data-sheet: > > 1. If one LED is forced ON - then also the other LED is forced. > > => You can't use SW control to force ON one LED and allow HW > > to control the other. > > 2. You can't force both LEDs OFF. If the FORCE bit for both LED's > > is > > zero, then LEDs are controlled by HW and indicate > > button/charger > > states as explained in data-sheet. > > > > Signed-off-by: Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com> > > --- > > drivers/leds/Kconfig | 10 ++++ > > drivers/leds/Makefile | 1 + > > drivers/leds/leds-bd71828.c | 97 > > +++++++++++++++++++++++++++++++++++++ > > 3 files changed, 108 insertions(+) > > create mode 100644 drivers/leds/leds-bd71828.c > > > > diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig > > index b0fdeef10bd9..ec59f28bcb39 100644 > > --- a/drivers/leds/Kconfig > > +++ b/drivers/leds/Kconfig > > @@ -529,6 +529,16 @@ config LEDS_BD2802 > > This option enables support for BD2802GU RGB LED driver chips > > accessed via the I2C bus. > > > > +config LEDS_BD71828 > > + tristate "LED driver for LED pins on ROHM BD71828 PMIC" > > + depends on LEDS_CLASS > doesn't this have a dependency on MFD_ROHM_BD71828 > > + depends on I2C > > + help > > + This option enables support for LED outputs located on ROHM > > + BD71828 power management IC. ROHM BD71828 has two led output > > pins > > + which can be left to indicate HW states or controlled by SW. > > Say > > + yes here if you want to enable SW control for these LEDs. > > + > > Add module statement What is the module statement? Do you mean the 'if you compile this as a module it will be called blahblah' or 'choose M to blahblah'? I've never understood why some entries have those statements. 'Choose M' stuff is help for config system - why should each module explain how to use configs? This information should be in more generic documentation. Furthermore, the 'tristate' there already says you can compile this as a module. Module name on the other hand really is module's property but it may well change if one changes the name of the file. That should not require change in KConfig. Furthermore, where do you need the module name? And if you really need the module name you should check the config name from Makefile to be sure - module/file names in comments or docs tend to get outdated. After all this being said - I can add any boilerplate text in KConfig if necessary - I just see zero benefit from this. And if you didn't mean this - can you then please tell me what is the module statement? > > config LEDS_INTEL_SS4200 > > tristate "LED driver for Intel NAS SS4200 series" > > depends on LEDS_CLASS > > diff --git a/drivers/leds/Makefile b/drivers/leds/Makefile > > index 41fb073a39c1..2a8f6a8e4c7c 100644 > > --- a/drivers/leds/Makefile > > +++ b/drivers/leds/Makefile > > @@ -15,6 +15,7 @@ obj-$(CONFIG_LEDS_AN30259A) += > > leds-an30259a.o > > obj-$(CONFIG_LEDS_BCM6328) += leds-bcm6328.o > > obj-$(CONFIG_LEDS_BCM6358) += leds-bcm6358.o > > obj-$(CONFIG_LEDS_BD2802) += leds-bd2802.o > > +obj-$(CONFIG_LEDS_BD71828) += leds-bd71828.o > > obj-$(CONFIG_LEDS_CPCAP) += leds-cpcap.o > > obj-$(CONFIG_LEDS_LOCOMO) += leds-locomo.o > > obj-$(CONFIG_LEDS_LM3530) += leds-lm3530.o > > diff --git a/drivers/leds/leds-bd71828.c b/drivers/leds/leds- > > bd71828.c > > new file mode 100644 > > index 000000000000..2427619444f5 > > --- /dev/null > > +++ b/drivers/leds/leds-bd71828.c > > @@ -0,0 +1,97 @@ > > +// SPDX-License-Identifier: GPL-2.0 > > +// Copyright (C) 2019 ROHM Semiconductors > > + > > +#include <linux/device.h> > > +#include <linux/err.h> > > +#include <linux/kernel.h> > > +#include <linux/leds.h> > > +#include <linux/mfd/rohm-bd71828.h> > > +#include <linux/module.h> > > +#include <linux/platform_device.h> > > +#include <linux/regmap.h> > > +#include <linux/slab.h> > > + > > +#define BD71828_LED_TO_DATA(l) ((l)->id == ID_GREEN_LED ? \ > > + container_of((l), struct bd71828_leds, green) : \ > > + container_of((l), struct bd71828_leds, amber)) > > I don't think we should be defining the color as the variable. The > outputs can drive any color LED. I used the colors mentioned in BD71828 data-sheet. It is true someone might use different LEDs on their board but at least this naming allows one to match the output to one in data-sheet. I can add comment explaining this if you thin it's worth mentioning. > > + > > +enum { > > + ID_GREEN_LED, > > + ID_AMBER_LED, > > + ID_NMBR_OF, > > +}; > > + > > Please use the color_id in linux/include/dt-bindings/leds/common.h Maybe I should not include anything from dt-bindings if I don't use DT for this sub-device? (Please see the comments below). > > +struct bd71828_led { > > + int id; > > + struct led_classdev l; > > + u8 force_mask; > > +}; > > + > > +struct bd71828_leds { > > + struct rohm_regmap_dev *bd71828; > > + struct bd71828_led green; > > + struct bd71828_led amber; > > +}; > > + > > +static int bd71828_led_brightness_set(struct led_classdev > > *led_cdev, > > + enum led_brightness value) > > +{ > > + struct bd71828_led *l = container_of(led_cdev, struct > > bd71828_led, l); > > + struct bd71828_leds *data; > > + unsigned int val = BD71828_LED_OFF; > > + > > + data = BD71828_LED_TO_DATA(l); > > + if (value != LED_OFF) > > + val = BD71828_LED_ON; > > + > > + return regmap_update_bits(data->bd71828->regmap, > > BD71828_REG_LED_CTRL, > > + l->force_mask, val); > > +} > > + > > +static int bd71828_led_probe(struct platform_device *pdev) > > +{ > > + struct rohm_regmap_dev *bd71828; > > + struct bd71828_leds *l; > > + struct bd71828_led *g, *a; > > + static const char *GNAME = "bd71828-green-led"; > > + static const char *ANAME = "bd71828-amber-led"; > The LED class creates the name it can get it from the DT. I did not add DT node for LEDs as I thought this is preferred way when there is not much HW information to bring from DT. I am not sure as previous PMICs I did drivers for didn't have LEDs though. Currently this is a MFD sub-device and gets no data from DT. Actually the driver crashed when I first didn't explicitly give these names. > > + int ret; > > + > > + pr_info("bd71828 LED driver probed\n"); as a comment from myself - this print should be eliminated or by minimum converted to dev_dbg. > > + > > + bd71828 = dev_get_drvdata(pdev->dev.parent); > > + l = devm_kzalloc(&pdev->dev, sizeof(*l), GFP_KERNEL); > > + if (!l) > > + return -ENOMEM; > > + l->bd71828 = bd71828; > > + a = &l->amber; > > + g = &l->green; > > + a->id = ID_AMBER_LED; > > + g->id = ID_GREEN_LED; > > + a->force_mask = BD71828_MASK_LED_AMBER; > > + g->force_mask = BD71828_MASK_LED_GREEN; > > + > > + a->l.name = ANAME; > > + g->l.name = GNAME; > > + a->l.brightness_set_blocking = bd71828_led_brightness_set; > > + g->l.brightness_set_blocking = bd71828_led_brightness_set; > > + > > + ret = devm_led_classdev_register(&pdev->dev, &g->l); > > + if (ret) > > + return ret; > > + > > + return devm_led_classdev_register(&pdev->dev, &a->l); > > +} > > + > > This looks different. Not sure why you register both LEDs in this > probe. > > You can use the DT to define both LEDs and then each will be probed > and > registered separately. As I mentioned above, this driver is currently not using DT at all. Reason why it does not is that I didn't know all the LEDs are usually having own DT entries/drivers. But there is actually reason for bundling the LEDs to same driver. What is not shown in driver is that LEDs can be controlled by PMIC state machine so that they are indicating charger states. Other option is driving LEDs using this driver - but forcing one of the LEDs will cause also the other LED to be under SW control. Eg, one can't control just single LED using SW, its both of them or none. > > This is how it is commonly done. > > You can reference the LM36274 led driver as this is a MFD device to > the > ti-lmu.c in the MFD directory. Thanks for pointing this out. I will gladly see how others have it implemented! I just would like to know if the DT binding is really a must? In this case I am unsure what LED related extra information we could really give from DT (for this specific device). I just checked the lm36274 you mentioned. I see that also lm36274 do parse the dt and set the name itself (so maybe the led_class is not doing this after all?) - although the name setting code in lm36274 is a bit peculiar as it loops through all child nodes and overwrites the old name if it finds more than one "label" properties from nodes (if I read the code correctly). In any case I am unsure what is the benefit from using DT and adding the DT parsing code for this PMIC's LEDs. I could understand DT usage if LED class handled dt parsing and if there was something to configure in BD71828 LEDs - but I see no such benefits in this case. Best Regards Matti Vaittinen
Hi Matti, On 10/21/19 10:00 AM, Vaittinen, Matti wrote: > Hello Dan, > > Thanks for taking the time to check my driver :) I truly appreciate all > the help! > > A "fundamental question" regarding these review comments is whether I > should add DT entries for these LEDs or not. I thought I shouldn't but > I would like to get a comment from Rob regarding it. If the LED controller is a part of MFD device probed from DT then there is no doubt it should have corresponding DT sub-node. We've recently added some standardization of LED naming so please use the new 'function' and 'color' DT properties in the bindings. Please refer to Documentation/devicetree/bindings/leds/common.txt. You should register LED class devices using devm_led_classdev_register_ext() API to benefit from generic support for LED name composition. This support is available in Linus tree starting from 5.4-rc1. > On Thu, 2019-10-17 at 09:04 -0500, Dan Murphy wrote: >> Matt >> >> On 10/17/19 4:53 AM, Matti Vaittinen wrote: >>> ROHM BD71828 power management IC has two LED outputs for charge >>> status >>> and button pressing indications. The LED outputs can also be forced >>> bs SW so add driver allowing to use these LEDs for other >>> indications >> s/bs/by >>> as well. >>> >>> Leds are controlled by SW using 'Force ON' bits. Please note the >>> constrains mentioned in data-sheet: >>> 1. If one LED is forced ON - then also the other LED is forced. >>> => You can't use SW control to force ON one LED and allow HW >>> to control the other. >>> 2. You can't force both LEDs OFF. If the FORCE bit for both LED's >>> is >>> zero, then LEDs are controlled by HW and indicate >>> button/charger >>> states as explained in data-sheet. >>> >>> Signed-off-by: Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com> >>> --- >>> drivers/leds/Kconfig | 10 ++++ >>> drivers/leds/Makefile | 1 + >>> drivers/leds/leds-bd71828.c | 97 >>> +++++++++++++++++++++++++++++++++++++ >>> 3 files changed, 108 insertions(+) >>> create mode 100644 drivers/leds/leds-bd71828.c >>> >>> diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig >>> index b0fdeef10bd9..ec59f28bcb39 100644 >>> --- a/drivers/leds/Kconfig >>> +++ b/drivers/leds/Kconfig >>> @@ -529,6 +529,16 @@ config LEDS_BD2802 >>> This option enables support for BD2802GU RGB LED driver chips >>> accessed via the I2C bus. >>> >>> +config LEDS_BD71828 >>> + tristate "LED driver for LED pins on ROHM BD71828 PMIC" >>> + depends on LEDS_CLASS >> doesn't this have a dependency on MFD_ROHM_BD71828 >>> + depends on I2C >>> + help >>> + This option enables support for LED outputs located on ROHM >>> + BD71828 power management IC. ROHM BD71828 has two led output >>> pins >>> + which can be left to indicate HW states or controlled by SW. >>> Say >>> + yes here if you want to enable SW control for these LEDs. >>> + >> >> Add module statement > > What is the module statement? Do you mean the 'if you compile this as a > module it will be called blahblah' or 'choose M to blahblah'? > > I've never understood why some entries have those statements. 'Choose > M' stuff is help for config system - why should each module explain how > to use configs? This information should be in more generic > documentation. Furthermore, the 'tristate' there already says you can > compile this as a module. Module name on the other hand really is > module's property but it may well change if one changes the name of the > file. That should not require change in KConfig. Furthermore, where do > you need the module name? And if you really need the module name you > should check the config name from Makefile to be sure - module/file > names in comments or docs tend to get outdated. > > After all this being said - I can add any boilerplate text in KConfig > if necessary - I just see zero benefit from this. And if you didn't > mean this - can you then please tell me what is the module statement? Yes, like you noticed, this is boilerplate so please follow the convention. If you'd like to discuss its relevance please submit a message to kernel-janitors@vger.kernel.org. >>> config LEDS_INTEL_SS4200 >>> tristate "LED driver for Intel NAS SS4200 series" >>> depends on LEDS_CLASS >>> diff --git a/drivers/leds/Makefile b/drivers/leds/Makefile >>> index 41fb073a39c1..2a8f6a8e4c7c 100644 >>> --- a/drivers/leds/Makefile >>> +++ b/drivers/leds/Makefile >>> @@ -15,6 +15,7 @@ obj-$(CONFIG_LEDS_AN30259A) += >>> leds-an30259a.o >>> obj-$(CONFIG_LEDS_BCM6328) += leds-bcm6328.o >>> obj-$(CONFIG_LEDS_BCM6358) += leds-bcm6358.o >>> obj-$(CONFIG_LEDS_BD2802) += leds-bd2802.o >>> +obj-$(CONFIG_LEDS_BD71828) += leds-bd71828.o >>> obj-$(CONFIG_LEDS_CPCAP) += leds-cpcap.o >>> obj-$(CONFIG_LEDS_LOCOMO) += leds-locomo.o >>> obj-$(CONFIG_LEDS_LM3530) += leds-lm3530.o >>> diff --git a/drivers/leds/leds-bd71828.c b/drivers/leds/leds- >>> bd71828.c >>> new file mode 100644 >>> index 000000000000..2427619444f5 >>> --- /dev/null >>> +++ b/drivers/leds/leds-bd71828.c >>> @@ -0,0 +1,97 @@ >>> +// SPDX-License-Identifier: GPL-2.0 >>> +// Copyright (C) 2019 ROHM Semiconductors >>> + >>> +#include <linux/device.h> >>> +#include <linux/err.h> >>> +#include <linux/kernel.h> >>> +#include <linux/leds.h> >>> +#include <linux/mfd/rohm-bd71828.h> >>> +#include <linux/module.h> >>> +#include <linux/platform_device.h> >>> +#include <linux/regmap.h> >>> +#include <linux/slab.h> >>> + >>> +#define BD71828_LED_TO_DATA(l) ((l)->id == ID_GREEN_LED ? \ >>> + container_of((l), struct bd71828_leds, green) : \ >>> + container_of((l), struct bd71828_leds, amber)) >> >> I don't think we should be defining the color as the variable. The >> outputs can drive any color LED. > > I used the colors mentioned in BD71828 data-sheet. It is true someone > might use different LEDs on their board but at least this naming allows > one to match the output to one in data-sheet. I can add comment > explaining this if you thin it's worth mentioning. I see you've come up with below definitions in rohm-bd71828.h: #define BD71828_MASK_LED_AMBER 0x80 #define BD71828_MASK_LED_GREEN 0x40 Is this how those bit fields are named in the data sheet? >>> + >>> +enum { >>> + ID_GREEN_LED, >>> + ID_AMBER_LED, >>> + ID_NMBR_OF, >>> +}; >>> + >> >> Please use the color_id in linux/include/dt-bindings/leds/common.h > > Maybe I should not include anything from dt-bindings if I don't use DT > for this sub-device? (Please see the comments below). But you should. >>> +struct bd71828_led { >>> + int id; >>> + struct led_classdev l; >>> + u8 force_mask; >>> +}; >>> + >>> +struct bd71828_leds { >>> + struct rohm_regmap_dev *bd71828; >>> + struct bd71828_led green; >>> + struct bd71828_led amber; >>> +}; >>> + >>> +static int bd71828_led_brightness_set(struct led_classdev >>> *led_cdev, >>> + enum led_brightness value) >>> +{ >>> + struct bd71828_led *l = container_of(led_cdev, struct >>> bd71828_led, l); >>> + struct bd71828_leds *data; >>> + unsigned int val = BD71828_LED_OFF; >>> + >>> + data = BD71828_LED_TO_DATA(l); >>> + if (value != LED_OFF) >>> + val = BD71828_LED_ON; >>> + >>> + return regmap_update_bits(data->bd71828->regmap, >>> BD71828_REG_LED_CTRL, >>> + l->force_mask, val); >>> +} >>> + >>> +static int bd71828_led_probe(struct platform_device *pdev) >>> +{ >>> + struct rohm_regmap_dev *bd71828; >>> + struct bd71828_leds *l; >>> + struct bd71828_led *g, *a; >>> + static const char *GNAME = "bd71828-green-led"; >>> + static const char *ANAME = "bd71828-amber-led"; >> The LED class creates the name it can get it from the DT. > > I did not add DT node for LEDs as I thought this is preferred way when > there is not much HW information to bring from DT. I am not sure as > previous PMICs I did drivers for didn't have LEDs though. Currently > this is a MFD sub-device and gets no data from DT. Actually the driver > crashed when I first didn't explicitly give these names. Please compare below bindings of MFD devices with LED support: Documentation/devicetree/bindings/mfd/max77693.txt Documentation/devicetree/bindings/mfd/ti-lmu.txt Please follow them but use new 'function' and 'color' properties instead of 'label'. >>> + int ret; >>> + >>> + pr_info("bd71828 LED driver probed\n"); > as a comment from myself - this print should be eliminated or by > minimum converted to dev_dbg. > >>> + >>> + bd71828 = dev_get_drvdata(pdev->dev.parent); >>> + l = devm_kzalloc(&pdev->dev, sizeof(*l), GFP_KERNEL); >>> + if (!l) >>> + return -ENOMEM; >>> + l->bd71828 = bd71828; >>> + a = &l->amber; >>> + g = &l->green; >>> + a->id = ID_AMBER_LED; >>> + g->id = ID_GREEN_LED; >>> + a->force_mask = BD71828_MASK_LED_AMBER; >>> + g->force_mask = BD71828_MASK_LED_GREEN; >>> + >>> + a->l.name = ANAME; >>> + g->l.name = GNAME; >>> + a->l.brightness_set_blocking = bd71828_led_brightness_set; >>> + g->l.brightness_set_blocking = bd71828_led_brightness_set; >>> + >>> + ret = devm_led_classdev_register(&pdev->dev, &g->l); >>> + if (ret) >>> + return ret; >>> + >>> + return devm_led_classdev_register(&pdev->dev, &a->l); This way you force users to always register two LED class devices whereas they might need only one. Please compare how other LED class drivers handle DT parsing and LED class device registration. >>> +} >>> + >> >> This looks different. Not sure why you register both LEDs in this >> probe. >> >> You can use the DT to define both LEDs and then each will be probed >> and >> registered separately. > > As I mentioned above, this driver is currently not using DT at all. > Reason why it does not is that I didn't know all the LEDs are usually > having own DT entries/drivers. > > But there is actually reason for bundling the LEDs to same driver. What > is not shown in driver is that LEDs can be controlled by PMIC state > machine so that they are indicating charger states. Other option is This can be handled by the LED trigger that your driver should expose. On activation the trigger would setup the hardware to control the LEDs. But that can be covered later. > driving LEDs using this driver - but forcing one of the LEDs will cause > also the other LED to be under SW control. Eg, one can't control just > single LED using SW, its both of them or none. So this limitation will have to by documented in the trigger ABI. >> This is how it is commonly done. >> >> You can reference the LM36274 led driver as this is a MFD device to >> the >> ti-lmu.c in the MFD directory. > > Thanks for pointing this out. I will gladly see how others have it > implemented! I just would like to know if the DT binding is really a > must? In this case I am unsure what LED related extra information we > could really give from DT (for this specific device). > > I just checked the lm36274 you mentioned. I see that also lm36274 do > parse the dt and set the name itself (so maybe the led_class is not > doing this after all?) - although the name setting code in lm36274 is a > bit peculiar as it loops through all child nodes and overwrites the old > name if it finds more than one "label" properties from nodes (if I read > the code correctly). > > In any case I am unsure what is the benefit from using DT and adding > the DT parsing code for this PMIC's LEDs. I could understand DT usage > if LED class handled dt parsing and if there was something to configure > in BD71828 LEDs - but I see no such benefits in this case. I hope I was able to clarify most of your doubts.
Hello Jacek, Thanks for the clarifications. I think I now understand the LED subsystem a bit better :) On Mon, 2019-10-21 at 21:09 +0200, Jacek Anaszewski wrote: > Hi Matti, > > On 10/21/19 10:00 AM, Vaittinen, Matti wrote: > > Hello Dan, > > > > Thanks for taking the time to check my driver :) I truly appreciate > > all > > the help! > > > > A "fundamental question" regarding these review comments is whether > > I > > should add DT entries for these LEDs or not. I thought I shouldn't > > but > > I would like to get a comment from Rob regarding it. > > If the LED controller is a part of MFD device probed from DT then > there is no doubt it should have corresponding DT sub-node. Sorry but I still see no much benefit from adding this information in DT. Why should it have corresponding DT-node if the LED properties are fixed and if we only wish to allow user-space control and have no dependencies to other devices in DT? In this specific case the information we can provide from DT is supposed to be fixed. No board based variation. Furthermore, there is not much generic driver/led core functionality which would be able to parse and utilize relevant information from DT. I think we can only give the name (function) and colour. And they are supposed to be fixed and thus could be just hard-coded in driver. Hard-coding these would be simpler and less error prone for users (no DT bindings to write) and simpler to create and probably also to maintain (no separate binding documents needed for LEDs). But assuming this is Ok to DT-folks and if you insist - I will add LED information to DT for the next patches. Hopefully this extra complexity helps in some oddball use-case which I can't foresee =) Then what comes to the DT format. Do you think LED subsystem should try to follow the convention with other sub-systems and not introduce multiple compatibles for single device? MFD can handle instantiating the sub-devices just fine even when sub-devices have no own compatible property or of_match. Maybe we should also avoid unnecessary sub-nodes when they are not really required. If we look at clk and GPIOs it is nowadays preferred (as per my understanding after discussions with Stephen, Rob and some others - please correct me if I am wrong) to place the 'provider' information in the MFD node and obtain the relevant properties from parent->of_node in the sub-device driver (or for generic properties, in the core framework) - at least for simple cases. I guess rationale was that it reflects the real hardware better when no artificial sub-nodes are required? I see the example bindings (like max77693 below) you pointed to me don't follow that convention but create own sub nodes with own compatible properties in MFD for all the logical blocks. I am asking this as I was "strongly advieced" (a.k.a told to change my approach if I wish to get driver ion their trees ;]) by Rob and Stephen to not do that with previous PMIC drivers I upstreamed. As example - the relevant bindings for BD71837 clock output were originally: pmic: bd71837@4b { compatible = "rohm,bd71837"; regulators { ... }; /* Clock node */ clk: bd71837-32k-out { compatible = "rohm,bd71837-clk"; #clock-cells = <0>; clock-frequency = <32768>; clock-output-names = "bd71837-32k-out"; }; }; but it was preferred to not have the the clk sub-node and place the provider information directly in pmic node instead. Result was: pmic: bd71837@4b { compatible = "rohm,bd71837"; #clock-cells = <0>; clock-frequency = <32768>; clock-output-names = "bd71837-32k-out"; regulators { ... }; }; clk consumers can then live anywhere in DT and just refer to clock provider via reference. The sub-device driver is instantiated by MFD and it does not need the DT (or compatible) for this. After that the sub-driver can fetch the parent DT node and looks for relevant information - or just passes that node to core if it expects no driver specific properties. The bd70528 is another example. It is a MFD with drivers for regulators, clk-gate, few GPIOs, watchdog, charger and rtc. It still has only one compatible in DT, and only MFD node with regulators sub- node. There is no need for WDG, RTC or charger to be present in DT. (In my books LEDs for BD71828 would fall in the same category but I guess I already mentioned that xD ). pmic: pmic@4b { compatible = "rohm,bd70528"; reg = <0x4b>; interrupt-parent = <&gpio1>; interrupts = <29 GPIO_ACTIVE_LOW>; clocks = <&osc 0>; #clock-cells = <0>; clock-output-names = "bd70528-32k-out"; #gpio-cells = <2>; gpio-controller; interrupt-controller; #interrupt-cells = <2>; regulators { ... }; }; So, to make LED subsystem at least partially follow same principle I suggest I'll do following: Where max77693 does: max77693@66 { compatible = "maxim,max77693"; reg = <0x66>; interrupt-parent = <&gpx1>; interrupts = <5 2>; regulators { esafeout@1 { regulator-compatible = "ESAFEOUT1"; ... }; }; haptic { compatible = "maxim,max77693-haptic"; ... }; charger { compatible = "maxim,max77693-charger"; ... }; led { compatible = "maxim,max77693-led"; ... camera_flash: flash-led { label = "max77693-flash"; ... }; }; }; I would go with: pmic: pmic@4b { compatible = "rohm,bd71828"; reg = <0x4b>; interrupt-parent = <&gpio1>; interrupts = <29 GPIO_ACTIVE_LOW>; clocks = <&osc 0>; #clock-cells = <0>; clock-output-names = "bd71828-32k-out"; gpio-controller; #gpio-cells = <2>; ngpios = <4>; gpio-reserved-ranges = <0 1 2 1>; gpio-line-names = "EPDEN"; rohm,dvs-vsel-gpios = <&gpio1 12 0>, <&gpio1 13 0>; regulators { ... }; chg-led { function = LED_FUNCTION_CHARGING; color = LED_COLOR_ID_AMBER; }; pwr-led { function = LED_FUNCTION_POWER; color = LED_COLOR_ID_GREEN; }; }; How do you see this? Or do you really wish to have this one extra node: pmic: pmic@4b { compatible = "rohm,bd71828"; reg = <0x4b>; interrupt-parent = <&gpio1>; interru pts = <29 GPIO_ACTIVE_LOW>; clocks = <&osc 0>; #clock-cells = <0>; clock-output-names = "bd71828-32k-out"; gpio-controller; #gpio-cells = <2>; ngpios = <4>; gpio-reserved-ranges = <0 1 2 1>; gpio-line-names = "EPDEN"; rohm,dvs-vsel-gpios = <&gpio1 12 0>, <&gpio1 13 0>; regulators { ... }; leds-dummy { chg-led { function = LED_FUNCTION_CHARGING; color = LED_COLOR_ID_AMBER; }; pwr-led { function = LED_FUNCTION_POWER; color = LED_COLOR_ID_GREEN; }; }; }; > We've recently added some standardization of LED naming so please > use the new 'function' and 'color' DT properties in the bindings. > > Please refer to Documentation/devicetree/bindings/leds/common.txt. > > You should register LED class devices using > devm_led_classdev_register_ext() API to benefit from generic support > for LED name composition. This support is available in Linus tree > starting from 5.4-rc1. Thanks for the hint. So this API would make the name out of function and colour given via dt, right? > > On Thu, 2019-10-17 at 09:04 -0500, Dan Murphy wrote: > > > Matt > > > > > > On 10/17/19 4:53 AM, Matti Vaittinen wrote: > > > > ROHM BD71828 power management IC has two LED outputs for charge > > > > status > > > > and button pressing indications. The LED outputs can also be > > > > forced > > > > bs SW so add driver allowing to use these LEDs for other > > > > indications > > > s/bs/by > > > > as well. > > > > > > > > Leds are controlled by SW using 'Force ON' bits. Please note > > > > the > > > > constrains mentioned in data-sheet: > > > > 1. If one LED is forced ON - then also the other LED is forced. > > > > => You can't use SW control to force ON one LED and > > > > allow HW > > > > to control the other. > > > > 2. You can't force both LEDs OFF. If the FORCE bit for both > > > > LED's > > > > is > > > > zero, then LEDs are controlled by HW and indicate > > > > button/charger > > > > states as explained in data-sheet. > > > > > > > > Signed-off-by: Matti Vaittinen < > > > > matti.vaittinen@fi.rohmeurope.com> > > > > --- > > > > drivers/leds/Kconfig | 10 ++++ > > > > drivers/leds/Makefile | 1 + > > > > drivers/leds/leds-bd71828.c | 97 > > > > +++++++++++++++++++++++++++++++++++++ > > > > 3 files changed, 108 insertions(+) > > > > create mode 100644 drivers/leds/leds-bd71828.c > > > > > > > > diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig > > > > index b0fdeef10bd9..ec59f28bcb39 100644 > > > > --- a/drivers/leds/Kconfig > > > > +++ b/drivers/leds/Kconfig > > > > @@ -529,6 +529,16 @@ config LEDS_BD2802 > > > > This option enables support for BD2802GU RGB LED > > > > driver chips > > > > accessed via the I2C bus. > > > > > > > > +config LEDS_BD71828 > > > > + tristate "LED driver for LED pins on ROHM BD71828 PMIC" > > > > + depends on LEDS_CLASS > > > doesn't this have a dependency on MFD_ROHM_BD71828 > > > > + depends on I2C > > > > + help > > > > + This option enables support for LED outputs located > > > > on ROHM > > > > + BD71828 power management IC. ROHM BD71828 has two > > > > led output > > > > pins > > > > + which can be left to indicate HW states or > > > > controlled by SW. > > > > Say > > > > + yes here if you want to enable SW control for these > > > > LEDs. > > > > + > > > > > > Add module statement > > > > What is the module statement? Do you mean the 'if you compile this > > as a > > module it will be called blahblah' or 'choose M to blahblah'? > > > > I've never understood why some entries have those statements. > > 'Choose > > M' stuff is help for config system - why should each module explain > > how > > to use configs? This information should be in more generic > > documentation. Furthermore, the 'tristate' there already says you > > can > > compile this as a module. Module name on the other hand really is > > module's property but it may well change if one changes the name of > > the > > file. That should not require change in KConfig. Furthermore, where > > do > > you need the module name? And if you really need the module name > > you > > should check the config name from Makefile to be sure - module/file > > names in comments or docs tend to get outdated. > > > > After all this being said - I can add any boilerplate text in > > KConfig > > if necessary - I just see zero benefit from this. And if you didn't > > mean this - can you then please tell me what is the module > > statement? > > Yes, like you noticed, this is boilerplate so please follow the > convention. If you'd like to discuss its relevance please submit > a message to kernel-janitors@vger.kernel.org. I did follow the convention. There is 67 tristated LED drivers which do NOT add this module building babbling in description. Then there is 14 drivers which do. So common convention even in LED subsystem is to NOT include meaningless mumbojumbo there. So even regarding convention it is better to have short description to the point. That actually makes the requiring boilerplate even more useless. But as I said, I can put any meaningless letters there. (again, if I can't convince you to reconsider how you like the LED subsystem to appear like). Knowing how hard it is to help people reducing waste - it's may be easier for me than discussing this further :( > > > > > config LEDS_INTEL_SS4200 > > > > tristate "LED driver for Intel NAS SS4200 series" > > > > depends on LEDS_CLASS > > > > diff --git a/drivers/leds/Makefile b/drivers/leds/Makefile > > > > index 41fb073a39c1..2a8f6a8e4c7c 100644 > > > > --- a/drivers/leds/Makefile > > > > +++ b/drivers/leds/Makefile > > > > @@ -15,6 +15,7 @@ obj-$(CONFIG_LEDS_AN30259A) += > > > > leds-an30259a.o > > > > obj-$(CONFIG_LEDS_BCM6328) += leds-bcm6328.o > > > > obj-$(CONFIG_LEDS_BCM6358) += leds-bcm6358.o > > > > obj-$(CONFIG_LEDS_BD2802) += leds-bd2802.o > > > > +obj-$(CONFIG_LEDS_BD71828) += leds-bd71828.o > > > > obj-$(CONFIG_LEDS_CPCAP) += leds-cpcap.o > > > > obj-$(CONFIG_LEDS_LOCOMO) += leds-locomo.o > > > > obj-$(CONFIG_LEDS_LM3530) += leds-lm3530.o > > > > diff --git a/drivers/leds/leds-bd71828.c b/drivers/leds/leds- > > > > bd71828.c > > > > new file mode 100644 > > > > index 000000000000..2427619444f5 > > > > --- /dev/null > > > > +++ b/drivers/leds/leds-bd71828.c > > > > @@ -0,0 +1,97 @@ > > > > +// SPDX-License-Identifier: GPL-2.0 > > > > +// Copyright (C) 2019 ROHM Semiconductors > > > > + > > > > +#include <linux/device.h> > > > > +#include <linux/err.h> > > > > +#include <linux/kernel.h> > > > > +#include <linux/leds.h> > > > > +#include <linux/mfd/rohm-bd71828.h> > > > > +#include <linux/module.h> > > > > +#include <linux/platform_device.h> > > > > +#include <linux/regmap.h> > > > > +#include <linux/slab.h> > > > > + > > > > +#define BD71828_LED_TO_DATA(l) ((l)->id == ID_GREEN_LED ? \ > > > > + container_of((l), struct bd71828_leds, green) : \ > > > > + container_of((l), struct bd71828_leds, amber)) > > > > > > I don't think we should be defining the color as the variable. > > > The > > > outputs can drive any color LED. > > > > I used the colors mentioned in BD71828 data-sheet. It is true > > someone > > might use different LEDs on their board but at least this naming > > allows > > one to match the output to one in data-sheet. I can add comment > > explaining this if you thin it's worth mentioning. > > I see you've come up with below definitions in rohm-bd71828.h: > > #define BD71828_MASK_LED_AMBER 0x80 > #define BD71828_MASK_LED_GREEN 0x40 > > Is this how those bit fields are named in the data sheet? The leds are through the document referred as "GRNLED" and "AMBLED". These specific bits are named "AMBLED_FORCE_ON" and "GRNLED_FORCE_ON". > > > > > + > > > > +enum { > > > > + ID_GREEN_LED, > > > > + ID_AMBER_LED, > > > > + ID_NMBR_OF, > > > > +}; > > > > + > > > > > > Please use the color_id in linux/include/dt- > > > bindings/leds/common.h > > > > Maybe I should not include anything from dt-bindings if I don't use > > DT > > for this sub-device? (Please see the comments below). > > But you should. Is this your final say? You think I can't convince you to reconsider? Anyways, this is LED ID, not color property. Eg, this is not defining the color, this is identifying the LED. Please see the macro: #define BD71828_LED_TO_DATA(l) ((l)->id == ID_GREEN_LED ? \ container_of((l), struct bd71828_leds, green) : \ container_of((l), struct bd71828_leds, amber)) which is used to obtain the pointer to driver data based on pointer to led structure. Eg, we get the correct offset by determining which LED was contolled (based on the LED ID). > > > > > +struct bd71828_led { > > > > + int id; > > > > + struct led_classdev l; > > > > + u8 force_mask; > > > > +}; > > > > + > > > > +struct bd71828_leds { > > > > + struct rohm_regmap_dev *bd71828; > > > > + struct bd71828_led green; > > > > + struct bd71828_led amber; > > > > +}; > > > > + > > > > +static int bd71828_led_brightness_set(struct led_classdev > > > > *led_cdev, > > > > + enum led_brightness > > > > value) > > > > +{ > > > > + struct bd71828_led *l = container_of(led_cdev, struct > > > > bd71828_led, l); > > > > + struct bd71828_leds *data; > > > > + unsigned int val = BD71828_LED_OFF; > > > > + > > > > + data = BD71828_LED_TO_DATA(l); > > > > + if (value != LED_OFF) > > > > + val = BD71828_LED_ON; > > > > + > > > > + return regmap_update_bits(data->bd71828->regmap, > > > > BD71828_REG_LED_CTRL, > > > > + l->force_mask, val); > > > > +} > > > > + > > > > +static int bd71828_led_probe(struct platform_device *pdev) > > > > +{ > > > > + struct rohm_regmap_dev *bd71828; > > > > + struct bd71828_leds *l; > > > > + struct bd71828_led *g, *a; > > > > + static const char *GNAME = "bd71828-green-led"; > > > > + static const char *ANAME = "bd71828-amber-led"; > > > The LED class creates the name it can get it from the DT. > > > > I did not add DT node for LEDs as I thought this is preferred way > > when > > there is not much HW information to bring from DT. I am not sure as > > previous PMICs I did drivers for didn't have LEDs though. Currently > > this is a MFD sub-device and gets no data from DT. Actually the > > driver > > crashed when I first didn't explicitly give these names. > > Please compare below bindings of MFD devices with LED support: > > Documentation/devicetree/bindings/mfd/max77693.txt > Documentation/devicetree/bindings/mfd/ti-lmu.txt > > Please follow them but use new 'function' and 'color' properties > instead of 'label'. > > > > > + int ret; > > > > + > > > > + pr_info("bd71828 LED driver probed\n"); > > as a comment from myself - this print should be eliminated or by > > minimum converted to dev_dbg. > > > > > > + > > > > + bd71828 = dev_get_drvdata(pdev->dev.parent); > > > > + l = devm_kzalloc(&pdev->dev, sizeof(*l), GFP_KERNEL); > > > > + if (!l) > > > > + return -ENOMEM; > > > > + l->bd71828 = bd71828; > > > > + a = &l->amber; > > > > + g = &l->green; > > > > + a->id = ID_AMBER_LED; > > > > + g->id = ID_GREEN_LED; > > > > + a->force_mask = BD71828_MASK_LED_AMBER; > > > > + g->force_mask = BD71828_MASK_LED_GREEN; > > > > + > > > > + a->l.name = ANAME; > > > > + g->l.name = GNAME; > > > > + a->l.brightness_set_blocking = > > > > bd71828_led_brightness_set; > > > > + g->l.brightness_set_blocking = > > > > bd71828_led_brightness_set; > > > > + > > > > + ret = devm_led_classdev_register(&pdev->dev, &g->l); > > > > + if (ret) > > > > + return ret; > > > > + > > > > + return devm_led_classdev_register(&pdev->dev, &a->l); > > This way you force users to always register two LED class devices > whereas they might need only one. Please compare how other LED class > drivers handle DT parsing and LED class device registration. I am not sure if I understand correctly what you mean by using only one class device. As I (hopefully) somewhere said - users can't control only one of these LEDs. If they decide to enable one led by SW, then they inevitably control also the other. Thus it is better that user gets control to both of the LEDs if they take the control for one. Or do you mean I could achieve the control for both of these LEDs via only one class device? > > > > > +} > > > > + > > > > > > This looks different. Not sure why you register both LEDs in > > > this > > > probe. > > > > > > You can use the DT to define both LEDs and then each will be > > > probed > > > and > > > registered separately. > > > > As I mentioned above, this driver is currently not using DT at all. > > Reason why it does not is that I didn't know all the LEDs are > > usually > > having own DT entries/drivers. > > > > But there is actually reason for bundling the LEDs to same driver. > > What > > is not shown in driver is that LEDs can be controlled by PMIC state > > machine so that they are indicating charger states. Other option is > > This can be handled by the LED trigger that your driver should > expose. > On activation the trigger would setup the hardware to control the > LEDs. But that can be covered later. Yet another thing for me to learn =) I looked at the trigger properties in DT. That looked like a way to make the LED framework to "bind" the LED state to some trigger. (For example make the LED framework to toggle specific LED state when USB device is plugged?) If this is the case then it might not be relevant for BD71828. Here the LED is by-default controlled by HW. Eg, when charger starts charging the battery, the PMIC will lit the LED. It will do so also when power button is pressed or certain problems are detected. This reqires no SW interaction. What this driver intends to do is to allow SW to take over this. Eg, if system is designed so that it is preferably to use these LEDs for some other purpose it can be done by loading this LED driver and allowing user-space to control these LEDs via sysfs. > > driving LEDs using this driver - but forcing one of the LEDs will > > cause > > also the other LED to be under SW control. Eg, one can't control > > just > > single LED using SW, its both of them or none. > > So this limitation will have to by documented in the trigger ABI. > > > > This is how it is commonly done. > > > > > > You can reference the LM36274 led driver as this is a MFD device > > > to > > > the > > > ti-lmu.c in the MFD directory. > > > > Thanks for pointing this out. I will gladly see how others have it > > implemented! I just would like to know if the DT binding is really > > a > > must? In this case I am unsure what LED related extra information > > we > > could really give from DT (for this specific device). > > > > I just checked the lm36274 you mentioned. I see that also lm36274 > > do > > parse the dt and set the name itself (so maybe the led_class is not > > doing this after all?) - although the name setting code in lm36274 > > is a > > bit peculiar as it loops through all child nodes and overwrites the > > old > > name if it finds more than one "label" properties from nodes (if I > > read > > the code correctly). > > > > In any case I am unsure what is the benefit from using DT and > > adding > > the DT parsing code for this PMIC's LEDs. I could understand DT > > usage > > if LED class handled dt parsing and if there was something to > > configure > > in BD71828 LEDs - but I see no such benefits in this case. > > I hope I was able to clarify most of your doubts. Thanks again for taking the time to check this out. I appreciate the effort you guys put into this! And what comes to my doubts - yes, I still have a few ;) But I am sure we can work this out. Thanks a bunch for this discussion Jacek! Br, Matti Vaittinen
Matti, On 10/22/19 2:40 PM, Vaittinen, Matti wrote: > Hello Jacek, > > Thanks for the clarifications. I think I now understand the LED > subsystem a bit better :) > > On Mon, 2019-10-21 at 21:09 +0200, Jacek Anaszewski wrote: >> Hi Matti, >> >> On 10/21/19 10:00 AM, Vaittinen, Matti wrote: >>> Hello Dan, >>> >>> Thanks for taking the time to check my driver :) I truly appreciate >>> all >>> the help! >>> >>> A "fundamental question" regarding these review comments is whether >>> I >>> should add DT entries for these LEDs or not. I thought I shouldn't >>> but >>> I would like to get a comment from Rob regarding it. >> >> If the LED controller is a part of MFD device probed from DT then >> there is no doubt it should have corresponding DT sub-node. > > Sorry but I still see no much benefit from adding this information in > DT. Why should it have corresponding DT-node if the LED properties are > fixed and if we only wish to allow user-space control and have no > dependencies to other devices in DT? > > In this specific case the information we can provide from DT is > supposed to be fixed. No board based variation. Furthermore, there is > not much generic driver/led core functionality which would be able to > parse and utilize relevant information from DT. I think we can only > give the name (function) and colour. And they are supposed to be fixed > and thus could be just hard-coded in driver. Hard-coding these would be > simpler and less error prone for users (no DT bindings to write) and > simpler to create and probably also to maintain (no separate binding > documents needed for LEDs). AFAICS it is possible to connect LED of arbitrary color to the iouts of this device. If this is the case then it is justified to have DT node only to allow for LED name customization. > But assuming this is Ok to DT-folks and if you insist - I will add LED > information to DT for the next patches. Hopefully this extra complexity > helps in some oddball use-case which I can't foresee =) > > Then what comes to the DT format. > > Do you think LED subsystem should try to follow the convention with > other sub-systems and not introduce multiple compatibles for single > device? MFD can handle instantiating the sub-devices just fine even > when sub-devices have no own compatible property or of_match. Maybe we > should also avoid unnecessary sub-nodes when they are not really > required. This is beyond my scope of responsibility. It is MFD subsystem thing to choose the way of LED class driver instantiation. When it comes to LED subsystem - it expects single compatible pertaining to a physical device. Nonetheless, so far we used to have separate compatibles for drivers of MFD devices' LED cells. If we are going to change that I'd like to see explicit DT maintainer's statement confirming that. And one benefit of having separate nodes per MFD cells is that we can easily discern the support for which cells is to be turned on. > If we look at clk and GPIOs it is nowadays preferred (as per my > understanding after discussions with Stephen, Rob and some others - > please correct me if I am wrong) to place the 'provider' information in > the MFD node and obtain the relevant properties from parent->of_node in > the sub-device driver (or for generic properties, in the core > framework) - at least for simple cases. I guess rationale was that it > reflects the real hardware better when no artificial sub-nodes are > required? > > I see the example bindings (like max77693 below) you pointed to me > don't follow that convention but create own sub nodes with own > compatible properties in MFD for all the logical blocks. > > I am asking this as I was "strongly advieced" (a.k.a told to change my > approach if I wish to get driver ion their trees ;]) by Rob and Stephen > to not do that with previous PMIC drivers I upstreamed. > > As example - the relevant bindings for BD71837 clock output were > originally: > > pmic: bd71837@4b { > compatible = "rohm,bd71837"; > > regulators { > ... > }; > > /* Clock node */ > clk: bd71837-32k-out { > compatible = "rohm,bd71837-clk"; > #clock-cells = <0>; > clock-frequency = <32768>; > clock-output-names = "bd71837-32k-out"; > }; > }; > > but it was preferred to not have the the clk sub-node and place the > provider information directly in pmic node instead. Result was: > > pmic: bd71837@4b { > compatible = "rohm,bd71837"; > > #clock-cells = <0>; > clock-frequency = <32768>; > clock-output-names = "bd71837-32k-out"; > > regulators { > ... > }; > }; > > clk consumers can then live anywhere in DT and just refer to clock > provider via reference. > > The sub-device driver is instantiated by MFD and it does not need the > DT (or compatible) for this. After that the sub-driver can fetch the > parent DT node and looks for relevant information - or just passes that > node to core if it expects no driver specific properties. > > The bd70528 is another example. It is a MFD with drivers for > regulators, clk-gate, few GPIOs, watchdog, charger and rtc. It still > has only one compatible in DT, and only MFD node with regulators sub- > node. There is no need for WDG, RTC or charger to be present in DT. (In > my books LEDs for BD71828 would fall in the same category but I guess I > already mentioned that xD ). > > pmic: pmic@4b { > compatible = "rohm,bd70528"; > reg = <0x4b>; > interrupt-parent = <&gpio1>; > interrupts = <29 GPIO_ACTIVE_LOW>; > clocks = <&osc 0>; > #clock-cells = <0>; > clock-output-names = "bd70528-32k-out"; > #gpio-cells = <2>; > gpio-controller; > interrupt-controller; > #interrupt-cells = <2>; > > regulators { > ... > }; > }; > > So, to make LED subsystem at least partially follow same principle I > suggest I'll do following: > > Where max77693 does: > max77693@66 { > compatible = "maxim,max77693"; > reg = <0x66>; > interrupt-parent = <&gpx1>; > interrupts = <5 2>; > > regulators { > esafeout@1 { > regulator-compatible = "ESAFEOUT1"; > ... > }; > }; > > haptic { > compatible = "maxim,max77693-haptic"; > ... > }; > > charger { > compatible = "maxim,max77693-charger"; > ... > }; > > led { > compatible = "maxim,max77693-led"; > ... > > camera_flash: flash-led { > label = "max77693-flash"; > ... > }; > }; > }; > > I would go with: > > pmic: pmic@4b { > compatible = "rohm,bd71828"; > reg = <0x4b>; > interrupt-parent = <&gpio1>; > interrupts = <29 GPIO_ACTIVE_LOW>; > clocks = <&osc 0>; > #clock-cells = <0>; > clock-output-names = "bd71828-32k-out"; > gpio-controller; > #gpio-cells = <2>; > ngpios = <4>; > gpio-reserved-ranges = <0 1 2 1>; > gpio-line-names = "EPDEN"; > rohm,dvs-vsel-gpios = <&gpio1 12 0>, > <&gpio1 13 0>; > regulators { > ... > }; > > chg-led { > function = LED_FUNCTION_CHARGING; > color = LED_COLOR_ID_AMBER; > }; > > pwr-led { > function = LED_FUNCTION_POWER; > color = LED_COLOR_ID_GREEN; > }; This way you would probably need to probe LED class driver twice, instead of letting it behave in a standard way and parse child LED nodes. > }; > > How do you see this? Or do you really wish to have this one extra node: > > pmic: pmic@4b { > compatible = "rohm,bd71828"; > > reg = <0x4b>; > interrupt-parent = <&gpio1>; > interru > pts = <29 GPIO_ACTIVE_LOW>; > clocks = <&osc 0>; > > #clock-cells = <0>; > clock-output-names = "bd71828-32k-out"; > gpio-controller; > #gpio-cells = <2>; > > ngpios = <4>; > gpio-reserved-ranges = <0 1 2 1>; > > gpio-line-names = "EPDEN"; > rohm,dvs-vsel-gpios = > <&gpio1 12 0>, > <&gpio1 13 0>; > > regulators { > ... > }; > > leds-dummy { Why leds-dummy ? The convention is to have led-controller@unit-address as the parent LED controller node. > chg-led { s/chg-led/led0/ > function = LED_FUNCTION_CHARGING; > color = LED_COLOR_ID_AMBER; > }; > > pwr-led { s/pwr-led/led1/ This is ePAPR requirement that DT node name should describe the general class of device. > function = LED_FUNCTION_POWER; > color = LED_COLOR_ID_GREEN; > }; Common LED bindings say this is the proper way to go. However you would need compatible to probe LED class driver in DT based way. If you plan to do it otherwise then it makes no sense to have DT nodes for LEDs. > }; > }; > > >> We've recently added some standardization of LED naming so please >> use the new 'function' and 'color' DT properties in the bindings. >> >> Please refer to Documentation/devicetree/bindings/leds/common.txt. >> >> You should register LED class devices using >> devm_led_classdev_register_ext() API to benefit from generic support >> for LED name composition. This support is available in Linus tree >> starting from 5.4-rc1. > > Thanks for the hint. So this API would make the name out of function > and colour given via dt, right? Right. >>> On Thu, 2019-10-17 at 09:04 -0500, Dan Murphy wrote: >>>> Matt >>>> >>>> On 10/17/19 4:53 AM, Matti Vaittinen wrote: >>>>> ROHM BD71828 power management IC has two LED outputs for charge >>>>> status >>>>> and button pressing indications. The LED outputs can also be >>>>> forced >>>>> bs SW so add driver allowing to use these LEDs for other >>>>> indications >>>> s/bs/by >>>>> as well. >>>>> >>>>> Leds are controlled by SW using 'Force ON' bits. Please note >>>>> the >>>>> constrains mentioned in data-sheet: >>>>> 1. If one LED is forced ON - then also the other LED is forced. >>>>> => You can't use SW control to force ON one LED and >>>>> allow HW >>>>> to control the other. >>>>> 2. You can't force both LEDs OFF. If the FORCE bit for both >>>>> LED's >>>>> is >>>>> zero, then LEDs are controlled by HW and indicate >>>>> button/charger >>>>> states as explained in data-sheet. >>>>> >>>>> Signed-off-by: Matti Vaittinen < >>>>> matti.vaittinen@fi.rohmeurope.com> >>>>> --- >>>>> drivers/leds/Kconfig | 10 ++++ >>>>> drivers/leds/Makefile | 1 + >>>>> drivers/leds/leds-bd71828.c | 97 >>>>> +++++++++++++++++++++++++++++++++++++ >>>>> 3 files changed, 108 insertions(+) >>>>> create mode 100644 drivers/leds/leds-bd71828.c >>>>> >>>>> diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig >>>>> index b0fdeef10bd9..ec59f28bcb39 100644 >>>>> --- a/drivers/leds/Kconfig >>>>> +++ b/drivers/leds/Kconfig >>>>> @@ -529,6 +529,16 @@ config LEDS_BD2802 >>>>> This option enables support for BD2802GU RGB LED >>>>> driver chips >>>>> accessed via the I2C bus. >>>>> >>>>> +config LEDS_BD71828 >>>>> + tristate "LED driver for LED pins on ROHM BD71828 PMIC" >>>>> + depends on LEDS_CLASS >>>> doesn't this have a dependency on MFD_ROHM_BD71828 >>>>> + depends on I2C >>>>> + help >>>>> + This option enables support for LED outputs located >>>>> on ROHM >>>>> + BD71828 power management IC. ROHM BD71828 has two >>>>> led output >>>>> pins >>>>> + which can be left to indicate HW states or >>>>> controlled by SW. >>>>> Say >>>>> + yes here if you want to enable SW control for these >>>>> LEDs. >>>>> + >>>> >>>> Add module statement >>> >>> What is the module statement? Do you mean the 'if you compile this >>> as a >>> module it will be called blahblah' or 'choose M to blahblah'? >>> >>> I've never understood why some entries have those statements. >>> 'Choose >>> M' stuff is help for config system - why should each module explain >>> how >>> to use configs? This information should be in more generic >>> documentation. Furthermore, the 'tristate' there already says you >>> can >>> compile this as a module. Module name on the other hand really is >>> module's property but it may well change if one changes the name of >>> the >>> file. That should not require change in KConfig. Furthermore, where >>> do >>> you need the module name? And if you really need the module name >>> you >>> should check the config name from Makefile to be sure - module/file >>> names in comments or docs tend to get outdated. >>> >>> After all this being said - I can add any boilerplate text in >>> KConfig >>> if necessary - I just see zero benefit from this. And if you didn't >>> mean this - can you then please tell me what is the module >>> statement? >> >> Yes, like you noticed, this is boilerplate so please follow the >> convention. If you'd like to discuss its relevance please submit >> a message to kernel-janitors@vger.kernel.org. > > I did follow the convention. There is 67 tristated LED drivers which do > NOT add this module building babbling in description. Then there is 14 > drivers which do. So common convention even in LED subsystem is to NOT > include meaningless mumbojumbo there. > > So even regarding convention it is better to have short description to > the point. That actually makes the requiring boilerplate even more > useless. But as I said, I can put any meaningless letters there. > (again, if I can't convince you to reconsider how you like the LED > subsystem to appear like). Knowing how hard it is to help people > reducing waste - it's may be easier for me than discussing this further > :( I will not insist - it's up to you. Unless Dan who raised the issue sees that differently. >>>>> config LEDS_INTEL_SS4200 >>>>> tristate "LED driver for Intel NAS SS4200 series" >>>>> depends on LEDS_CLASS >>>>> diff --git a/drivers/leds/Makefile b/drivers/leds/Makefile >>>>> index 41fb073a39c1..2a8f6a8e4c7c 100644 >>>>> --- a/drivers/leds/Makefile >>>>> +++ b/drivers/leds/Makefile >>>>> @@ -15,6 +15,7 @@ obj-$(CONFIG_LEDS_AN30259A) += >>>>> leds-an30259a.o >>>>> obj-$(CONFIG_LEDS_BCM6328) += leds-bcm6328.o >>>>> obj-$(CONFIG_LEDS_BCM6358) += leds-bcm6358.o >>>>> obj-$(CONFIG_LEDS_BD2802) += leds-bd2802.o >>>>> +obj-$(CONFIG_LEDS_BD71828) += leds-bd71828.o >>>>> obj-$(CONFIG_LEDS_CPCAP) += leds-cpcap.o >>>>> obj-$(CONFIG_LEDS_LOCOMO) += leds-locomo.o >>>>> obj-$(CONFIG_LEDS_LM3530) += leds-lm3530.o >>>>> diff --git a/drivers/leds/leds-bd71828.c b/drivers/leds/leds- >>>>> bd71828.c >>>>> new file mode 100644 >>>>> index 000000000000..2427619444f5 >>>>> --- /dev/null >>>>> +++ b/drivers/leds/leds-bd71828.c >>>>> @@ -0,0 +1,97 @@ >>>>> +// SPDX-License-Identifier: GPL-2.0 >>>>> +// Copyright (C) 2019 ROHM Semiconductors >>>>> + >>>>> +#include <linux/device.h> >>>>> +#include <linux/err.h> >>>>> +#include <linux/kernel.h> >>>>> +#include <linux/leds.h> >>>>> +#include <linux/mfd/rohm-bd71828.h> >>>>> +#include <linux/module.h> >>>>> +#include <linux/platform_device.h> >>>>> +#include <linux/regmap.h> >>>>> +#include <linux/slab.h> >>>>> + >>>>> +#define BD71828_LED_TO_DATA(l) ((l)->id == ID_GREEN_LED ? \ >>>>> + container_of((l), struct bd71828_leds, green) : \ >>>>> + container_of((l), struct bd71828_leds, amber)) >>>> >>>> I don't think we should be defining the color as the variable. >>>> The >>>> outputs can drive any color LED. >>> >>> I used the colors mentioned in BD71828 data-sheet. It is true >>> someone >>> might use different LEDs on their board but at least this naming >>> allows >>> one to match the output to one in data-sheet. I can add comment >>> explaining this if you thin it's worth mentioning. >> >> I see you've come up with below definitions in rohm-bd71828.h: >> >> #define BD71828_MASK_LED_AMBER 0x80 >> #define BD71828_MASK_LED_GREEN 0x40 >> >> Is this how those bit fields are named in the data sheet? > > The leds are through the document referred as "GRNLED" and "AMBLED". > These specific bits are named "AMBLED_FORCE_ON" and "GRNLED_FORCE_ON". OK, so then it's reasonable to use those names in the driver. I would only add a comment next to the definitions, highlighting that their names don't imply the scope of supported colors as this is entirely irrelevant. >>>>> + >>>>> +enum { >>>>> + ID_GREEN_LED, >>>>> + ID_AMBER_LED, >>>>> + ID_NMBR_OF, >>>>> +}; >>>>> + >>>> >>>> Please use the color_id in linux/include/dt- >>>> bindings/leds/common.h >>> >>> Maybe I should not include anything from dt-bindings if I don't use >>> DT >>> for this sub-device? (Please see the comments below). >> >> But you should. > > Is this your final say? You think I can't convince you to reconsider? > > Anyways, this is LED ID, not color property. Eg, this is not defining > the color, this is identifying the LED. Please see the macro: > > #define BD71828_LED_TO_DATA(l) ((l)->id == ID_GREEN_LED ? \ > container_of((l), struct bd71828_leds, green) : \ > container_of((l), struct bd71828_leds, amber)) > > which is used to obtain the pointer to driver data based on pointer to > led structure. Eg, we get the correct offset by determining which LED > was contolled (based on the LED ID). > >> >>>>> +struct bd71828_led { >>>>> + int id; >>>>> + struct led_classdev l; >>>>> + u8 force_mask; >>>>> +}; >>>>> + >>>>> +struct bd71828_leds { >>>>> + struct rohm_regmap_dev *bd71828; >>>>> + struct bd71828_led green; >>>>> + struct bd71828_led amber; >>>>> +}; >>>>> + >>>>> +static int bd71828_led_brightness_set(struct led_classdev >>>>> *led_cdev, >>>>> + enum led_brightness >>>>> value) >>>>> +{ >>>>> + struct bd71828_led *l = container_of(led_cdev, struct >>>>> bd71828_led, l); >>>>> + struct bd71828_leds *data; >>>>> + unsigned int val = BD71828_LED_OFF; >>>>> + >>>>> + data = BD71828_LED_TO_DATA(l); >>>>> + if (value != LED_OFF) >>>>> + val = BD71828_LED_ON; >>>>> + >>>>> + return regmap_update_bits(data->bd71828->regmap, >>>>> BD71828_REG_LED_CTRL, >>>>> + l->force_mask, val); >>>>> +} >>>>> + >>>>> +static int bd71828_led_probe(struct platform_device *pdev) >>>>> +{ >>>>> + struct rohm_regmap_dev *bd71828; >>>>> + struct bd71828_leds *l; >>>>> + struct bd71828_led *g, *a; >>>>> + static const char *GNAME = "bd71828-green-led"; >>>>> + static const char *ANAME = "bd71828-amber-led"; >>>> The LED class creates the name it can get it from the DT. >>> >>> I did not add DT node for LEDs as I thought this is preferred way >>> when >>> there is not much HW information to bring from DT. I am not sure as >>> previous PMICs I did drivers for didn't have LEDs though. Currently >>> this is a MFD sub-device and gets no data from DT. Actually the >>> driver >>> crashed when I first didn't explicitly give these names. >> >> Please compare below bindings of MFD devices with LED support: >> >> Documentation/devicetree/bindings/mfd/max77693.txt >> Documentation/devicetree/bindings/mfd/ti-lmu.txt >> >> Please follow them but use new 'function' and 'color' properties >> instead of 'label'. >> >>>>> + int ret; >>>>> + >>>>> + pr_info("bd71828 LED driver probed\n"); >>> as a comment from myself - this print should be eliminated or by >>> minimum converted to dev_dbg. >>> >>>>> + >>>>> + bd71828 = dev_get_drvdata(pdev->dev.parent); >>>>> + l = devm_kzalloc(&pdev->dev, sizeof(*l), GFP_KERNEL); >>>>> + if (!l) >>>>> + return -ENOMEM; >>>>> + l->bd71828 = bd71828; >>>>> + a = &l->amber; >>>>> + g = &l->green; >>>>> + a->id = ID_AMBER_LED; >>>>> + g->id = ID_GREEN_LED; >>>>> + a->force_mask = BD71828_MASK_LED_AMBER; >>>>> + g->force_mask = BD71828_MASK_LED_GREEN; >>>>> + >>>>> + a->l.name = ANAME; >>>>> + g->l.name = GNAME; >>>>> + a->l.brightness_set_blocking = >>>>> bd71828_led_brightness_set; >>>>> + g->l.brightness_set_blocking = >>>>> bd71828_led_brightness_set; >>>>> + >>>>> + ret = devm_led_classdev_register(&pdev->dev, &g->l); >>>>> + if (ret) >>>>> + return ret; >>>>> + >>>>> + return devm_led_classdev_register(&pdev->dev, &a->l); >> >> This way you force users to always register two LED class devices >> whereas they might need only one. Please compare how other LED class >> drivers handle DT parsing and LED class device registration. > > I am not sure if I understand correctly what you mean by using only one > class device. As I (hopefully) somewhere said - users can't control > only one of these LEDs. If they decide to enable one led by SW, then > they inevitably control also the other. Thus it is better that user > gets control to both of the LEDs if they take the control for one. > > Or do you mean I could achieve the control for both of these LEDs via > only one class device? AFAIU the LEDs, when in SW mode, can be controlled independently, right? Because if not then there is no point in having separate LED class devices. But if I get it right, then allowing for registering only one LED class device is entirely justifiable - think of a situation when the iout remains not connected on the board. >> >>>>> +} >>>>> + >>>> >>>> This looks different. Not sure why you register both LEDs in >>>> this >>>> probe. >>>> >>>> You can use the DT to define both LEDs and then each will be >>>> probed >>>> and >>>> registered separately. >>> >>> As I mentioned above, this driver is currently not using DT at all. >>> Reason why it does not is that I didn't know all the LEDs are >>> usually >>> having own DT entries/drivers. >>> >>> But there is actually reason for bundling the LEDs to same driver. >>> What >>> is not shown in driver is that LEDs can be controlled by PMIC state >>> machine so that they are indicating charger states. Other option is >> >> This can be handled by the LED trigger that your driver should >> expose. >> On activation the trigger would setup the hardware to control the >> LEDs. But that can be covered later. > > Yet another thing for me to learn =) I looked at the trigger properties > in DT. That looked like a way to make the LED framework to "bind" the > LED state to some trigger. (For example make the LED framework to > toggle specific LED state when USB device is plugged?) > > If this is the case then it might not be relevant for BD71828. Here the > LED is by-default controlled by HW. Eg, when charger starts charging > the battery, the PMIC will lit the LED. It will do so also when power > button is pressed or certain problems are detected. This reqires no SW > interaction. The trigger can be always deactivated from sysfs as well as set up again. > > What this driver intends to do is to allow SW to take over this. Eg, if > system is designed so that it is preferably to use these LEDs for some > other purpose it can be done by loading this LED driver and allowing > user-space to control these LEDs via sysfs. So the LED trigger interface would help to signalize in which state the LED is. If the trigger is set then it means the LED is under hw control. >>> driving LEDs using this driver - but forcing one of the LEDs will >>> cause >>> also the other LED to be under SW control. Eg, one can't control >>> just >>> single LED using SW, its both of them or none. >> >> So this limitation will have to by documented in the trigger ABI. >> >>>> This is how it is commonly done. >>>> >>>> You can reference the LM36274 led driver as this is a MFD device >>>> to >>>> the >>>> ti-lmu.c in the MFD directory. >>> >>> Thanks for pointing this out. I will gladly see how others have it >>> implemented! I just would like to know if the DT binding is really >>> a >>> must? In this case I am unsure what LED related extra information >>> we >>> could really give from DT (for this specific device). >>> >>> I just checked the lm36274 you mentioned. I see that also lm36274 >>> do >>> parse the dt and set the name itself (so maybe the led_class is not >>> doing this after all?) - although the name setting code in lm36274 >>> is a >>> bit peculiar as it loops through all child nodes and overwrites the >>> old >>> name if it finds more than one "label" properties from nodes (if I >>> read >>> the code correctly). >>> >>> In any case I am unsure what is the benefit from using DT and >>> adding >>> the DT parsing code for this PMIC's LEDs. I could understand DT >>> usage >>> if LED class handled dt parsing and if there was something to >>> configure >>> in BD71828 LEDs - but I see no such benefits in this case. >> >> I hope I was able to clarify most of your doubts. > > Thanks again for taking the time to check this out. I appreciate the > effort you guys put into this! And what comes to my doubts - yes, I > still have a few ;) But I am sure we can work this out. Thanks a bunch > for this discussion Jacek! You're welcome!
Morning Jacek, Thanks for the reply again. I did some cleaning to this mail as it was getting lengthy. On Tue, 2019-10-22 at 19:40 +0200, Jacek Anaszewski wrote: > Matti, > > On 10/22/19 2:40 PM, Vaittinen, Matti wrote: > > Hello Jacek, > > > > Thanks for the clarifications. I think I now understand the LED > > subsystem a bit better :) > > > > On Mon, 2019-10-21 at 21:09 +0200, Jacek Anaszewski wrote: > > > Hi Matti, > > > > > > On 10/21/19 10:00 AM, Vaittinen, Matti wrote: > > > > Hello Dan, > > > > > > > > Thanks for taking the time to check my driver :) I truly > > > > appreciate > > > > all > > > > the help! > > > > > > > > A "fundamental question" regarding these review comments is > > > > whether > > > > I > > > > should add DT entries for these LEDs or not. I thought I > > > > shouldn't > > > > but > > > > I would like to get a comment from Rob regarding it. > > > > > > If the LED controller is a part of MFD device probed from DT then > > > there is no doubt it should have corresponding DT sub-node. > > > > Sorry but I still see no much benefit from adding this information > > in > > DT. Why should it have corresponding DT-node if the LED properties > > are > > fixed and if we only wish to allow user-space control and have no > > dependencies to other devices in DT? > > > > In this specific case the information we can provide from DT is > > supposed to be fixed. No board based variation. Furthermore, there > > is > > not much generic driver/led core functionality which would be able > > to > > parse and utilize relevant information from DT. I think we can only > > give the name (function) and colour. And they are supposed to be > > fixed > > and thus could be just hard-coded in driver. Hard-coding these > > would be > > simpler and less error prone for users (no DT bindings to write) > > and > > simpler to create and probably also to maintain (no separate > > binding > > documents needed for LEDs). > > AFAICS it is possible to connect LED of arbitrary color to the iouts > of this device. If this is the case then it is justified to have DT > node only to allow for LED name customization. In theory, yes. In practice (if I understand it correctly) the color in this case is only visible in sysfs path name. I am not at all sure that reflecting the (unlikely) color change in path name is worth the hassle. Besides - if this happens, then the driver and DT can be changed. It is easier to add DT entries than remove them. If you see the color change support as really crucial - then I could even consider defaulting the colours to amber and green if no colour property is present in DT. I see no point in _requiring_ the DT entry to be there. If we like being prepared for the theoretical possibilities - what if x86 is used to control this PMIC? I guess we wouldn't have DT there then (And no - I don't see such use-case). > > But assuming this is Ok to DT-folks and if you insist - I will add > > LED > > information to DT for the next patches. Hopefully this extra > > complexity > > helps in some oddball use-case which I can't foresee =) > > > > Then what comes to the DT format. > > > > Do you think LED subsystem should try to follow the convention with > > other sub-systems and not introduce multiple compatibles for single > > device? MFD can handle instantiating the sub-devices just fine even > > when sub-devices have no own compatible property or of_match. Maybe > > we > > should also avoid unnecessary sub-nodes when they are not really > > required. > > This is beyond my scope of responsibility. It is MFD subsystem thing > to > choose the way of LED class driver instantiation. When it comes to > LED subsystem - it expects single compatible pertaining to a physical > device. Sorry but I don't quite follow. What the LED subsystem does with the compatible property? How does it expect this? > Nonetheless, so far we used to have separate compatibles for drivers > of > MFD devices' LED cells. If we are going to change that I'd like to > see > explicit DT maintainer's statement confirming that. I don't expect that existing DTs would be changed. But as I said, the consensus amongst most of the subsystenm maintainers and DT maintainers seems to be that sub-devices should not have own compatibles. I hope Rob acks this here - but knowing he is a busy guy I add some old discussions from which I have gathered my understanding: BD71837 - first patch where regulators had compatible - Mark (regulator maintainer instructed me to drop it): https://lore.kernel.org/linux-clk/20180524140118.GS4828@sirena.org.uk/ And here Stephen (the clk subsystem maintainer) told me to drop whole clocks sub-node (including the compatible): https://lore.kernel.org/linux-clk/152777867392.144038.18188452389972834689@swboyd.mtv.corp.google.com/ > And one benefit of having separate nodes per MFD cells is that we can > easily discern the support for which cells is to be turned on. We don't want to do DT modifications to drop some sub-device support out. The DT is HW description and sub-blocks are still there. We drop the support by KConfig. Only 'configuration' we could bring from DT is the amount of connected LEDs (as you said). But on the other hand - whether preparing for such unlikely design is reasonable (or needed) is questionable. > > pmic: pmic@4b { > > compatible = "rohm,bd71828"; > > reg = <0x4b>; > > interrupt-parent = <&gpio1>; > > interrupts = <29 GPIO_ACTIVE_LOW>; > > clocks = <&osc 0>; > > #clock-cells = <0>; > > clock-output-names = "bd71828-32k-out"; > > gpio-controller; > > #gpio-cells = <2>; > > ngpios = <4>; > > gpio-reserved-ranges = <0 1 2 1>; > > gpio-line-names = "EPDEN"; > > rohm,dvs-vsel-gpios = <&gpio1 12 0>, > > <&gpio1 13 0>; > > regulators { > > ... > > }; > > > > chg-led { > > function = LED_FUNCTION_CHARGING; > > color = LED_COLOR_ID_AMBER; > > }; > > > > pwr-led { > > function = LED_FUNCTION_POWER; > > color = LED_COLOR_ID_GREEN; > > }; > > This way you would probably need to probe LED class driver twice, > instead of letting it behave in a standard way and parse child LED > nodes. No. Please note that probing the MFD sub-drivers is _not_ bound to device-tree nodes. MFD sub-devices can be probed just fine even if they have no DT entries. When we add MFD cell for LED driver, the corresponding LED driver is probed. No DT magic needed for this. What the LED driver (as other sub-device drivers) is required to do is to obtain the pointer to parent device's DT node and find information which is relevant for it. Ideally, the subsystem framework can extract the properties which are common for whole subsystem (like color and function in case of LEDs) and driver only parses the DT if it has some custom properties. Again, ideally the driver has sane defaults - or some other 'platform data' mechanism if no DT information is found. There is architectures which do not support DT. In case of BD71828 LEDs my first idea was to go with only the 'sane defaults' option as I saw no much configurability. The DT snippet above contains LED information as per your suggestion. What the LED sub driver for BD71828 would now do is calling devm_led_classdev_register_ext with the DT information of BD71828 device. Eg, it should use the MFD dt node (because this is the real device) and not just part of it. devm_led_classdev_register_ext should then extract the LED specific information. I have not checked the implementation of devm_led_classdev_register_ext in details - but it should ignore non led properties and just walk through the LED information and create the sysfs interfaces etc. for all LEDs it finds. (In my example this is the chg-led and pwr-led sub-nodes). Furthermore, if no LED information is found from DT I would expect devm_led_classdev_register_ext to fail with well-defined return value so that the driver could do what it now does - Eg, use "sane defaults" to register the default class-devices for green and amber LEDs. The default led class dev naming should of course be same format as it would be if DT was populated with green and amber led information. > > }; > > > > How do you see this? Or do you really wish to have this one extra > > node: > > > > pmic: pmic@4b { > > compatible = "rohm,bd71828"; > > > > reg = <0x4b>; > > interrupt-parent = <&gpio1>; > > interru > > pts = <29 GPIO_ACTIVE_LOW>; > > clocks = <&osc 0>; > > > > #clock-cells = <0>; > > clock-output-names = "bd71828-32k-out"; > > gpio-controller; > > #gpio-cells = <2>; > > > > ngpios = <4>; > > gpio-reserved-ranges = <0 1 2 1>; > > > > gpio-line-names = "EPDEN"; > > rohm,dvs-vsel-gpios = > > <&gpio1 12 0>, > > <&gpio1 13 0>; > > > > regulators { > > ... > > }; > > > > leds-dummy { > > Why leds-dummy ? Because there is no real led controller device in any "MFD bus". It is just one MFD device with controls for two LEDs. > The convention is to have led-controller@unit-address as the parent > LED > controller node. What is the unit address here? 0x4b is the I2C slave address and it is the MFD node address. There is no addressing for LED controller as there is no separate LED controller device. There is only one device, the PMIC which is MFD device as it has multiple functions meld in. One of these functions is LED control and requires LED driver. > > chg-led { > s/chg-led/led0/ > > > function = LED_FUNCTION_CHARGING; > > color = LED_COLOR_ID_AMBER; > > }; > > > > pwr-led { > > s/pwr-led/led1/ > > This is ePAPR requirement that DT node name should describe the > general class of device. Thanks. I had some problems with these node names as I wanted to make them generic (led) but also to include some information what leds they are. A bit same idea as I see in node names like "chan1" and "chan345" that are used in ti-lmu bindings I checked for the example. But I am fine with renaming them in this example! I just don't think we should have this extra node as I mentioned. > > > function = LED_FUNCTION_POWER; > > color = LED_COLOR_ID_GREEN; > > }; > > Common LED bindings say this is the proper way to go. However you > would need compatible to probe LED class driver in DT based way. No. I don't. MFD will probe the LED class driver as long as the name of the driver matches to MFD cell name. So we only need MFD driver to be probed based on the compatible. Rest of the sub-device drivers will be probed by MFD. What I am missing is MODULE_ALIAS in LED driver for loading the module when MFD is searching for it if it is not modprobed via scripts or built in-kernel. I have understood this is the standard way with MFD nowadays - I am positive Lee will kick me if I am wrong ;) (I think I have bullied him that much in the past :/ ) > If you plan to do it otherwise then it makes no sense to have > DT nodes for LEDs. That was my point. This is why I did not have LEDs in DT in first place. But as I said above - as a result of this discussion I have started thinking that maybe I could check if I can easily add support for providing LED information also via DT and fall back to defaults if no LED information is found. (to allow color change or to omit one of the LEDs as you suggested) > > > > > > diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig > > > > > > index b0fdeef10bd9..ec59f28bcb39 100644 > > > > > > --- a/drivers/leds/Kconfig > > > > > > +++ b/drivers/leds/Kconfig > > > > > > @@ -529,6 +529,16 @@ config LEDS_BD2802 > > > > > > This option enables support for BD2802GU RGB LED > > > > > > driver chips > > > > > > accessed via the I2C bus. > > > > > > > > > > > > +config LEDS_BD71828 > > > > > > + tristate "LED driver for LED pins on ROHM BD71828 PMIC" > > > > > > + depends on LEDS_CLASS > > > > > doesn't this have a dependency on MFD_ROHM_BD71828 > > > > > > + depends on I2C > > > > > > + help > > > > > > + This option enables support for LED outputs located > > > > > > on ROHM > > > > > > + BD71828 power management IC. ROHM BD71828 has two > > > > > > led output > > > > > > pins > > > > > > + which can be left to indicate HW states or > > > > > > controlled by SW. > > > > > > Say > > > > > > + yes here if you want to enable SW control for these > > > > > > LEDs. > > > > > > + > > > > > > > > > > Add module statement > > > > > > > > What is the module statement? Do you mean the 'if you compile > > > > this > > > > as a > > > > module it will be called blahblah' or 'choose M to blahblah'? > > > > > > > > I've never understood why some entries have those statements. > > > > 'Choose > > > > M' stuff is help for config system - why should each module > > > > explain > > > > how > > > > to use configs? This information should be in more generic > > > > documentation. Furthermore, the 'tristate' there already says > > > > you > > > > can > > > > compile this as a module. Module name on the other hand really > > > > is > > > > module's property but it may well change if one changes the > > > > name of > > > > the > > > > file. That should not require change in KConfig. Furthermore, > > > > where > > > > do > > > > you need the module name? And if you really need the module > > > > name > > > > you > > > > should check the config name from Makefile to be sure - > > > > module/file > > > > names in comments or docs tend to get outdated. > > > > > > > > After all this being said - I can add any boilerplate text in > > > > KConfig > > > > if necessary - I just see zero benefit from this. And if you > > > > didn't > > > > mean this - can you then please tell me what is the module > > > > statement? > > > > > > Yes, like you noticed, this is boilerplate so please follow the > > > convention. If you'd like to discuss its relevance please submit > > > a message to kernel-janitors@vger.kernel.org. > > > > I did follow the convention. There is 67 tristated LED drivers > > which do > > NOT add this module building babbling in description. Then there is > > 14 > > drivers which do. So common convention even in LED subsystem is to > > NOT > > include meaningless mumbojumbo there. > > > > So even regarding convention it is better to have short description > > to > > the point. That actually makes the requiring boilerplate even more > > useless. But as I said, I can put any meaningless letters there. > > (again, if I can't convince you to reconsider how you like the LED > > subsystem to appear like). Knowing how hard it is to help people > > reducing waste - it's may be easier for me than discussing this > > further > > :( > > I will not insist - it's up to you. Unless Dan who raised the > issue sees that differently. Thanks :) Dan, I would like to hear your thoughts on this - do you still think this is a fatal issue for you? > > > > > > +#define BD71828_LED_TO_DATA(l) ((l)->id == ID_GREEN_LED ? > > > > > > \ > > > > > > + container_of((l), struct bd71828_leds, green) : \ > > > > > > + container_of((l), struct bd71828_leds, amber)) > > > > > > > > > > I don't think we should be defining the color as the > > > > > variable. > > > > > The > > > > > outputs can drive any color LED. > > > > > > > > I used the colors mentioned in BD71828 data-sheet. It is true > > > > someone > > > > might use different LEDs on their board but at least this > > > > naming > > > > allows > > > > one to match the output to one in data-sheet. I can add comment > > > > explaining this if you thin it's worth mentioning. > > > > > > I see you've come up with below definitions in rohm-bd71828.h: > > > > > > #define BD71828_MASK_LED_AMBER 0x80 > > > #define BD71828_MASK_LED_GREEN 0x40 > > > > > > Is this how those bit fields are named in the data sheet? > > > > The leds are through the document referred as "GRNLED" and > > "AMBLED". > > These specific bits are named "AMBLED_FORCE_ON" and > > "GRNLED_FORCE_ON". > > OK, so then it's reasonable to use those names in the driver. > I would only add a comment next to the definitions, highlighting > that their names don't imply the scope of supported colors as this is > entirely irrelevant. I'll add a note here. > > > > > > > + > > > > > > + bd71828 = dev_get_drvdata(pdev->dev.parent); > > > > > > + l = devm_kzalloc(&pdev->dev, sizeof(*l), GFP_KERNEL); > > > > > > + if (!l) > > > > > > + return -ENOMEM; > > > > > > + l->bd71828 = bd71828; > > > > > > + a = &l->amber; > > > > > > + g = &l->green; > > > > > > + a->id = ID_AMBER_LED; > > > > > > + g->id = ID_GREEN_LED; > > > > > > + a->force_mask = BD71828_MASK_LED_AMBER; > > > > > > + g->force_mask = BD71828_MASK_LED_GREEN; > > > > > > + > > > > > > + a->l.name = ANAME; > > > > > > + g->l.name = GNAME; > > > > > > + a->l.brightness_set_blocking = > > > > > > bd71828_led_brightness_set; > > > > > > + g->l.brightness_set_blocking = > > > > > > bd71828_led_brightness_set; > > > > > > + > > > > > > + ret = devm_led_classdev_register(&pdev->dev, &g->l); > > > > > > + if (ret) > > > > > > + return ret; > > > > > > + > > > > > > + return devm_led_classdev_register(&pdev->dev, &a->l); > > > > > > This way you force users to always register two LED class devices > > > whereas they might need only one. Please compare how other LED > > > class > > > drivers handle DT parsing and LED class device registration. > > > > I am not sure if I understand correctly what you mean by using only > > one > > class device. As I (hopefully) somewhere said - users can't control > > only one of these LEDs. If they decide to enable one led by SW, > > then > > they inevitably control also the other. Thus it is better that user > > gets control to both of the LEDs if they take the control for one. > > > > Or do you mean I could achieve the control for both of these LEDs > > via > > only one class device? > > AFAIU the LEDs, when in SW mode, can be controlled independently, > right? Yes and no. Both of the LEDs can be forced on/off individually - as long as one of them is forced ON. If both LEDs are tried to be forced OFF - then both LEDs are controlled by HW. If both are controlled by HW and then one is forced ON - the other is also no longer controlled by HW and is forced OFF. Eg, bits 0x80 and 0x40 are conrols for these LEDs. 0x80 for one, 0x40 for the other. Setting bit means LED is on, clearing means LED is off - with the HW control twist... If either of the bits is set - then both leds are controlled by these bits (SW control). If both bits are cleared, then LEDs are controlled by HW (likely to be off but not for sure). > Because if not then there is no point in having separate LED class > devices. > > But if I get it right, then allowing for registering only one LED > class > device is entirely justifiable - think of a situation when the iout > remains not connected on the board. Yes. This might be unlikely - but this is the reason why I consider adding the DT support. I just am not sure if covering this scenario now is worth the hassle. I tend to think we should only add the DT support if someone actually produces a board where this LED is not connected. > > Yet another thing for me to learn =) I looked at the trigger > > properties > > in DT. That looked like a way to make the LED framework to "bind" > > the > > LED state to some trigger. (For example make the LED framework to > > toggle specific LED state when USB device is plugged?) > > > > If this is the case then it might not be relevant for BD71828. Here > > the > > LED is by-default controlled by HW. Eg, when charger starts > > charging > > the battery, the PMIC will lit the LED. It will do so also when > > power > > button is pressed or certain problems are detected. This reqires no > > SW > > interaction. > > The trigger can be always deactivated from sysfs as well as set up > again. > > > What this driver intends to do is to allow SW to take over this. > > Eg, if > > system is designed so that it is preferably to use these LEDs for > > some > > other purpose it can be done by loading this LED driver and > > allowing > > user-space to control these LEDs via sysfs. > > So the LED trigger interface would help to signalize in which state > the LED is. If the trigger is set then it means the LED is under hw > control. This might be handy. I need to check the data-sheet because I think there was own control for using one of the LEDs for charge indicator. It might be that by-default the HW control of LEDs means that only the power button presses are signaled via these LEDs. This trigger thing could be handy for enabling/disabling also the charge indication as well as for checking if LEDs are in forced state - although this might be somewhat complicated because the 'turn led on' bit is connected to the 'trigger'. Eg, even if the trigger says that SW is controlling LED, turning off the LED may mean that trigger changes. But well, this is the HW design we are dealing with at this time :/ In any case, I'll leave this as further dev item for now. Again, thanks for all the help! Br, Matti Vaittinen
Hi Matti, On 10/23/19 10:37 AM, Vaittinen, Matti wrote: > Morning Jacek, > > Thanks for the reply again. I did some cleaning to this mail as it was > getting lengthy. > > On Tue, 2019-10-22 at 19:40 +0200, Jacek Anaszewski wrote: >> Matti, >> >> On 10/22/19 2:40 PM, Vaittinen, Matti wrote: >>> Hello Jacek, >>> >>> Thanks for the clarifications. I think I now understand the LED >>> subsystem a bit better :) >>> >>> On Mon, 2019-10-21 at 21:09 +0200, Jacek Anaszewski wrote: >>>> Hi Matti, >>>> >>>> On 10/21/19 10:00 AM, Vaittinen, Matti wrote: >>>>> Hello Dan, >>>>> >>>>> Thanks for taking the time to check my driver :) I truly >>>>> appreciate >>>>> all >>>>> the help! >>>>> >>>>> A "fundamental question" regarding these review comments is >>>>> whether >>>>> I >>>>> should add DT entries for these LEDs or not. I thought I >>>>> shouldn't >>>>> but >>>>> I would like to get a comment from Rob regarding it. >>>> >>>> If the LED controller is a part of MFD device probed from DT then >>>> there is no doubt it should have corresponding DT sub-node. >>> >>> Sorry but I still see no much benefit from adding this information >>> in >>> DT. Why should it have corresponding DT-node if the LED properties >>> are >>> fixed and if we only wish to allow user-space control and have no >>> dependencies to other devices in DT? >>> >>> In this specific case the information we can provide from DT is >>> supposed to be fixed. No board based variation. Furthermore, there >>> is >>> not much generic driver/led core functionality which would be able >>> to >>> parse and utilize relevant information from DT. I think we can only >>> give the name (function) and colour. And they are supposed to be >>> fixed >>> and thus could be just hard-coded in driver. Hard-coding these >>> would be >>> simpler and less error prone for users (no DT bindings to write) >>> and >>> simpler to create and probably also to maintain (no separate >>> binding >>> documents needed for LEDs). >> >> AFAICS it is possible to connect LED of arbitrary color to the iouts >> of this device. If this is the case then it is justified to have DT >> node only to allow for LED name customization. > > In theory, yes. In practice (if I understand it correctly) the color in > this case is only visible in sysfs path name. I am not at all sure that > reflecting the (unlikely) color change in path name is worth the > hassle. Besides - if this happens, then the driver and DT can be > changed. Driver should not be changed. We have DT for conveying board specific parameters. > It is easier to add DT entries than remove them. If you see > the color change support as really crucial - then I could even consider > defaulting the colours to amber and green if no colour property is > present in DT. You don't need to default to anything. The color section will be left empty if the property is not provided. > I see no point in _requiring_ the DT entry to be there. I'm referring to this later in this message. > If we like being prepared for the theoretical possibilities - what if > x86 is used to control this PMIC? I guess we wouldn't have DT there > then (And no - I don't see such use-case). We have fwnode abstraction for that. You can also check: Documentation/firmware-guide/acpi/dsd/leds.rst. >>> But assuming this is Ok to DT-folks and if you insist - I will add >>> LED >>> information to DT for the next patches. Hopefully this extra >>> complexity >>> helps in some oddball use-case which I can't foresee =) >>> >>> Then what comes to the DT format. >>> >>> Do you think LED subsystem should try to follow the convention with >>> other sub-systems and not introduce multiple compatibles for single >>> device? MFD can handle instantiating the sub-devices just fine even >>> when sub-devices have no own compatible property or of_match. Maybe >>> we >>> should also avoid unnecessary sub-nodes when they are not really >>> required. >> >> This is beyond my scope of responsibility. It is MFD subsystem thing >> to >> choose the way of LED class driver instantiation. When it comes to >> LED subsystem - it expects single compatible pertaining to a physical >> device. > > Sorry but I don't quite follow. What the LED subsystem does with the > compatible property? How does it expect this? In case of DT based MFD cell probing you must initialize of_compatible property of struct mfd_cell element which will then be matched with struct platform_driver -> driver -> of_match_table in the LED class driver. Basing on that a relevant platform_device is passed to the probe function. Its child struct device's of_node property comes already initialized to the pointer to the corresponding child node in MFD node. >> Nonetheless, so far we used to have separate compatibles for drivers >> of >> MFD devices' LED cells. If we are going to change that I'd like to >> see >> explicit DT maintainer's statement confirming that. > > I don't expect that existing DTs would be changed. I didn't suggest that. > But as I said, the > consensus amongst most of the subsystenm maintainers and DT maintainers > seems to be that sub-devices should not have own compatibles. I hope > Rob acks this here - but knowing he is a busy guy I add some old > discussions from which I have gathered my understanding: > > BD71837 - first patch where regulators had compatible - Mark (regulator > maintainer instructed me to drop it): > https://lore.kernel.org/linux-clk/20180524140118.GS4828@sirena.org.uk/ > > And here Stephen (the clk subsystem maintainer) told me to drop whole > clocks sub-node (including the compatible): > https://lore.kernel.org/linux-clk/152777867392.144038.18188452389972834689@swboyd.mtv.corp.google.com/ Still, there are MFD drivers using of_compatible for matching cell drivers. I don't follow current trends on MFD subsystem side. You've got to wait for review feedback from Lee Jones anyway to find out how to proceed with MFD bindings. >> And one benefit of having separate nodes per MFD cells is that we can >> easily discern the support for which cells is to be turned on. > > We don't want to do DT modifications to drop some sub-device support > out. The DT is HW description and sub-blocks are still there. We drop > the support by KConfig. How would you describe the purpose of 'status = "disabled"' DT assignment then? Anyway, I entirely disagree here - it is perfectly proper approach to define platform capabilities by modifying dts file alone. This way you can easily create multiple versions of platform configurations. It may be often impractical to enable all available platform features, at least from business point of view. And recompiling dts is lightweight operation in comparison to kernel compilation. Not saying that in some cases there are secret keys required for encrypting kernel images, that may not always be at hand. > Only 'configuration' we could bring from DT is > the amount of connected LEDs (as you said). But on the other hand - > whether preparing for such unlikely design is reasonable (or needed) is > questionable. LED naming related data is vital as well. >>> pmic: pmic@4b { >>> compatible = "rohm,bd71828"; >>> reg = <0x4b>; >>> interrupt-parent = <&gpio1>; >>> interrupts = <29 GPIO_ACTIVE_LOW>; >>> clocks = <&osc 0>; >>> #clock-cells = <0>; >>> clock-output-names = "bd71828-32k-out"; >>> gpio-controller; >>> #gpio-cells = <2>; >>> ngpios = <4>; >>> gpio-reserved-ranges = <0 1 2 1>; >>> gpio-line-names = "EPDEN"; >>> rohm,dvs-vsel-gpios = <&gpio1 12 0>, >>> <&gpio1 13 0>; >>> regulators { >>> ... >>> }; >>> >>> chg-led { >>> function = LED_FUNCTION_CHARGING; >>> color = LED_COLOR_ID_AMBER; >>> }; >>> >>> pwr-led { >>> function = LED_FUNCTION_POWER; >>> color = LED_COLOR_ID_GREEN; >>> }; >> >> This way you would probably need to probe LED class driver twice, >> instead of letting it behave in a standard way and parse child LED >> nodes. > > No. Please note that probing the MFD sub-drivers is _not_ bound to > device-tree nodes. MFD sub-devices can be probed just fine even if they > have no DT entries. When we add MFD cell for LED driver, the > corresponding LED driver is probed. No DT magic needed for this. > > What the LED driver (as other sub-device drivers) is required to do is > to obtain the pointer to parent device's DT node and find information > which is relevant for it. Ideally, the subsystem framework can extract > the properties which are common for whole subsystem (like color and > function in case of LEDs) and driver only parses the DT if it has some > custom properties. Again, ideally the driver has sane defaults - or > some other 'platform data' mechanism if no DT information is found. > There is architectures which do not support DT. LED common bindings define that each LED should be described using child node. And we've enforced sticking to this standard for last two years strictly. LED core mechanism for LED name composition also relies on this DT design - it expects single 'color' and 'function' properties to be present in the passed fwnode. LED class registration function registers single LED and it has been always LED class driver's responsibility to call it for every LED connected to the LED controller iouts. > In case of BD71828 LEDs my first idea was to go with only the 'sane > defaults' option as I saw no much configurability. The DT snippet above > contains LED information as per your suggestion. > > What the LED sub driver for BD71828 would now do is calling > devm_led_classdev_register_ext with the DT information of BD71828 > device. Eg, it should use the MFD dt node (because this is the real > device) and not just part of it. devm_led_classdev_register_ext should > then extract the LED specific information. I have not checked the > implementation of devm_led_classdev_register_ext in details - but it > should ignore non led properties and just walk through the LED > information and create the sysfs interfaces etc. for all LEDs it finds. This function does not work like that, as explained above. Please first get acquainted with the way how existing LED class drivers approach LED registration. Because otherwise we're wasting each others' time. > (In my example this is the chg-led and pwr-led sub-nodes). Furthermore, > if no LED information is found from DT I would expect > devm_led_classdev_register_ext to fail with well-defined return value > so that the driver could do what it now does - Eg, use "sane defaults" > to register the default class-devices for green and amber LEDs. The > default led class dev naming should of course be same format as it > would be if DT was populated with green and amber led information. Please go through 5.4-rc1 patches related to LED naming improvements You can also refer to Documentation/leds/leds-class.rst, "LED Device Naming" section for starter. >>> }; >>> >>> How do you see this? Or do you really wish to have this one extra >>> node: >>> >>> pmic: pmic@4b { >>> compatible = "rohm,bd71828"; >>> >>> reg = <0x4b>; >>> interrupt-parent = <&gpio1>; >>> interru >>> pts = <29 GPIO_ACTIVE_LOW>; >>> clocks = <&osc 0>; >>> >>> #clock-cells = <0>; >>> clock-output-names = "bd71828-32k-out"; >>> gpio-controller; >>> #gpio-cells = <2>; >>> >>> ngpios = <4>; >>> gpio-reserved-ranges = <0 1 2 1>; >>> >>> gpio-line-names = "EPDEN"; >>> rohm,dvs-vsel-gpios = >>> <&gpio1 12 0>, >>> <&gpio1 13 0>; >>> >>> regulators { >>> ... >>> }; >>> >>> leds-dummy { >> >> Why leds-dummy ? > > Because there is no real led controller device in any "MFD bus". It is > just one MFD device with controls for two LEDs. > >> The convention is to have led-controller@unit-address as the parent >> LED >> controller node. > > What is the unit address here? 0x4b is the I2C slave address and it is > the MFD node address. There is no addressing for LED controller as > there is no separate LED controller device. There is only one device, > the PMIC which is MFD device as it has multiple functions meld in. One > of these functions is LED control and requires LED driver. For MFD cell you can have just "led". >>> chg-led { >> s/chg-led/led0/ >> >>> function = LED_FUNCTION_CHARGING; >>> color = LED_COLOR_ID_AMBER; >>> }; >>> >>> pwr-led { >> >> s/pwr-led/led1/ >> >> This is ePAPR requirement that DT node name should describe the >> general class of device. > > Thanks. I had some problems with these node names as I wanted to make > them generic (led) but also to include some information what leds they > are. A bit same idea as I see in node names like "chan1" and "chan345" > that are used in ti-lmu bindings I checked for the example. But I am > fine with renaming them in this example! I just don't think we should > have this extra node as I mentioned. I wonder what Rob and Lee will say here. I personally would like to stick to LED common bindings and have this extra node. We define standards for a reason after all. >>> function = LED_FUNCTION_POWER; >>> color = LED_COLOR_ID_GREEN; >>> }; >> >> Common LED bindings say this is the proper way to go. However you >> would need compatible to probe LED class driver in DT based way. > > No. I don't. MFD will probe the LED class driver as long as the name of > the driver matches to MFD cell name. If you initialize only of_compatible in struct mfd_cell element then it will use only that for matching. I bet I was checking that five years ago while working on leds-max77693 driver. > So we only need MFD driver to be > probed based on the compatible. Rest of the sub-device drivers will be > probed by MFD. What I am missing is MODULE_ALIAS in LED driver for > loading the module when MFD is searching for it if it is not modprobed > via scripts or built in-kernel. I have understood this is the standard > way with MFD nowadays - I am positive Lee will kick me if I am wrong ;) > (I think I have bullied him that much in the past :/ ) Last sentence confirms my observation that you're strongly inclined to contest status quo :-) >> If you plan to do it otherwise then it makes no sense to have >> DT nodes for LEDs. > > That was my point. This is why I did not have LEDs in DT in first > place. But as I said above - as a result of this discussion I have > started thinking that maybe I could check if I can easily add support > for providing LED information also via DT and fall back to defaults if > no LED information is found. (to allow color change or to omit one of > the LEDs as you suggested) > >>>>>>> diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig >>>>>>> index b0fdeef10bd9..ec59f28bcb39 100644 >>>>>>> --- a/drivers/leds/Kconfig >>>>>>> +++ b/drivers/leds/Kconfig >>>>>>> @@ -529,6 +529,16 @@ config LEDS_BD2802 >>>>>>> This option enables support for BD2802GU RGB LED >>>>>>> driver chips >>>>>>> accessed via the I2C bus. >>>>>>> >>>>>>> +config LEDS_BD71828 >>>>>>> + tristate "LED driver for LED pins on ROHM BD71828 PMIC" >>>>>>> + depends on LEDS_CLASS >>>>>> doesn't this have a dependency on MFD_ROHM_BD71828 >>>>>>> + depends on I2C >>>>>>> + help >>>>>>> + This option enables support for LED outputs located >>>>>>> on ROHM >>>>>>> + BD71828 power management IC. ROHM BD71828 has two >>>>>>> led output >>>>>>> pins >>>>>>> + which can be left to indicate HW states or >>>>>>> controlled by SW. >>>>>>> Say >>>>>>> + yes here if you want to enable SW control for these >>>>>>> LEDs. >>>>>>> + >>>>>> >>>>>> Add module statement >>>>> >>>>> What is the module statement? Do you mean the 'if you compile >>>>> this >>>>> as a >>>>> module it will be called blahblah' or 'choose M to blahblah'? >>>>> >>>>> I've never understood why some entries have those statements. >>>>> 'Choose >>>>> M' stuff is help for config system - why should each module >>>>> explain >>>>> how >>>>> to use configs? This information should be in more generic >>>>> documentation. Furthermore, the 'tristate' there already says >>>>> you >>>>> can >>>>> compile this as a module. Module name on the other hand really >>>>> is >>>>> module's property but it may well change if one changes the >>>>> name of >>>>> the >>>>> file. That should not require change in KConfig. Furthermore, >>>>> where >>>>> do >>>>> you need the module name? And if you really need the module >>>>> name >>>>> you >>>>> should check the config name from Makefile to be sure - >>>>> module/file >>>>> names in comments or docs tend to get outdated. >>>>> >>>>> After all this being said - I can add any boilerplate text in >>>>> KConfig >>>>> if necessary - I just see zero benefit from this. And if you >>>>> didn't >>>>> mean this - can you then please tell me what is the module >>>>> statement? >>>> >>>> Yes, like you noticed, this is boilerplate so please follow the >>>> convention. If you'd like to discuss its relevance please submit >>>> a message to kernel-janitors@vger.kernel.org. >>> >>> I did follow the convention. There is 67 tristated LED drivers >>> which do >>> NOT add this module building babbling in description. Then there is >>> 14 >>> drivers which do. So common convention even in LED subsystem is to >>> NOT >>> include meaningless mumbojumbo there. >>> >>> So even regarding convention it is better to have short description >>> to >>> the point. That actually makes the requiring boilerplate even more >>> useless. But as I said, I can put any meaningless letters there. >>> (again, if I can't convince you to reconsider how you like the LED >>> subsystem to appear like). Knowing how hard it is to help people >>> reducing waste - it's may be easier for me than discussing this >>> further >>> :( >> >> I will not insist - it's up to you. Unless Dan who raised the >> issue sees that differently. > > Thanks :) Dan, I would like to hear your thoughts on this - do you > still think this is a fatal issue for you? > >>>>>>> +#define BD71828_LED_TO_DATA(l) ((l)->id == ID_GREEN_LED ? >>>>>>> \ >>>>>>> + container_of((l), struct bd71828_leds, green) : \ >>>>>>> + container_of((l), struct bd71828_leds, amber)) >>>>>> >>>>>> I don't think we should be defining the color as the >>>>>> variable. >>>>>> The >>>>>> outputs can drive any color LED. >>>>> >>>>> I used the colors mentioned in BD71828 data-sheet. It is true >>>>> someone >>>>> might use different LEDs on their board but at least this >>>>> naming >>>>> allows >>>>> one to match the output to one in data-sheet. I can add comment >>>>> explaining this if you thin it's worth mentioning. >>>> >>>> I see you've come up with below definitions in rohm-bd71828.h: >>>> >>>> #define BD71828_MASK_LED_AMBER 0x80 >>>> #define BD71828_MASK_LED_GREEN 0x40 >>>> >>>> Is this how those bit fields are named in the data sheet? >>> >>> The leds are through the document referred as "GRNLED" and >>> "AMBLED". >>> These specific bits are named "AMBLED_FORCE_ON" and >>> "GRNLED_FORCE_ON". >> >> OK, so then it's reasonable to use those names in the driver. >> I would only add a comment next to the definitions, highlighting >> that their names don't imply the scope of supported colors as this is >> entirely irrelevant. > > I'll add a note here. > >> >>>>>>> + >>>>>>> + bd71828 = dev_get_drvdata(pdev->dev.parent); >>>>>>> + l = devm_kzalloc(&pdev->dev, sizeof(*l), GFP_KERNEL); >>>>>>> + if (!l) >>>>>>> + return -ENOMEM; >>>>>>> + l->bd71828 = bd71828; >>>>>>> + a = &l->amber; >>>>>>> + g = &l->green; >>>>>>> + a->id = ID_AMBER_LED; >>>>>>> + g->id = ID_GREEN_LED; >>>>>>> + a->force_mask = BD71828_MASK_LED_AMBER; >>>>>>> + g->force_mask = BD71828_MASK_LED_GREEN; >>>>>>> + >>>>>>> + a->l.name = ANAME; >>>>>>> + g->l.name = GNAME; >>>>>>> + a->l.brightness_set_blocking = >>>>>>> bd71828_led_brightness_set; >>>>>>> + g->l.brightness_set_blocking = >>>>>>> bd71828_led_brightness_set; >>>>>>> + >>>>>>> + ret = devm_led_classdev_register(&pdev->dev, &g->l); >>>>>>> + if (ret) >>>>>>> + return ret; >>>>>>> + >>>>>>> + return devm_led_classdev_register(&pdev->dev, &a->l); >>>> >>>> This way you force users to always register two LED class devices >>>> whereas they might need only one. Please compare how other LED >>>> class >>>> drivers handle DT parsing and LED class device registration. >>> >>> I am not sure if I understand correctly what you mean by using only >>> one >>> class device. As I (hopefully) somewhere said - users can't control >>> only one of these LEDs. If they decide to enable one led by SW, >>> then >>> they inevitably control also the other. Thus it is better that user >>> gets control to both of the LEDs if they take the control for one. >>> >>> Or do you mean I could achieve the control for both of these LEDs >>> via >>> only one class device? >> >> AFAIU the LEDs, when in SW mode, can be controlled independently, >> right? > > Yes and no. Both of the LEDs can be forced on/off individually - as > long as one of them is forced ON. If both LEDs are tried to be forced > OFF - then both LEDs are controlled by HW. If both are controlled by HW > and then one is forced ON - the other is also no longer controlled by > HW and is forced OFF. > > Eg, bits 0x80 and 0x40 are conrols for these LEDs. 0x80 for one, 0x40 > for the other. Setting bit means LED is on, clearing means LED is off - > with the HW control twist... If either of the bits is set - then both > leds are controlled by these bits (SW control). If both bits are > cleared, then LEDs are controlled by HW (likely to be off but not for > sure). Thank you for the explanation. So they can be represented by separate LED class devices. Driver logic will just need to update the state of the sibling LED if it will be affected. >> Because if not then there is no point in having separate LED class >> devices. >> >> But if I get it right, then allowing for registering only one LED >> class >> device is entirely justifiable - think of a situation when the iout >> remains not connected on the board. > > Yes. This might be unlikely - but this is the reason why I consider > adding the DT support. I just am not sure if covering this scenario now > is worth the hassle. I tend to think we should only add the DT support > if someone actually produces a board where this LED is not connected. Could you share what board you're working with? >>> Yet another thing for me to learn =) I looked at the trigger >>> properties >>> in DT. That looked like a way to make the LED framework to "bind" >>> the >>> LED state to some trigger. (For example make the LED framework to >>> toggle specific LED state when USB device is plugged?) >>> >>> If this is the case then it might not be relevant for BD71828. Here >>> the >>> LED is by-default controlled by HW. Eg, when charger starts >>> charging >>> the battery, the PMIC will lit the LED. It will do so also when >>> power >>> button is pressed or certain problems are detected. This reqires no >>> SW >>> interaction. >> >> The trigger can be always deactivated from sysfs as well as set up >> again. >> >>> What this driver intends to do is to allow SW to take over this. >>> Eg, if >>> system is designed so that it is preferably to use these LEDs for >>> some >>> other purpose it can be done by loading this LED driver and >>> allowing >>> user-space to control these LEDs via sysfs. >> >> So the LED trigger interface would help to signalize in which state >> the LED is. If the trigger is set then it means the LED is under hw >> control. > > This might be handy. I need to check the data-sheet because I think > there was own control for using one of the LEDs for charge indicator. > It might be that by-default the HW control of LEDs means that only the > power button presses are signaled via these LEDs. This trigger thing > could be handy for enabling/disabling also the charge indication as > well as for checking if LEDs are in forced state - although this might > be somewhat complicated because the 'turn led on' bit is connected to > the 'trigger'. Eg, even if the trigger says that SW is controlling LED, > turning off the LED may mean that trigger changes. But well, this is > the HW design we are dealing with at this time :/ In any case, I'll > leave this as further dev item for now. > > Again, thanks for all the help! > > Br, > Matti Vaittinen >
Hello Jacek, On Wed, 2019-10-23 at 23:59 +0200, Jacek Anaszewski wrote: > On 10/23/19 10:37 AM, Vaittinen, Matti wrote: > > On Tue, 2019-10-22 at 19:40 +0200, Jacek Anaszewski wrote: > > > On 10/22/19 2:40 PM, Vaittinen, Matti wrote: > > > > On Mon, 2019-10-21 at 21:09 +0200, Jacek Anaszewski wrote: > > > > > On 10/21/19 10:00 AM, Vaittinen, Matti wrote: > > > > > > Hello Dan, > > > > > > > > > > > > Thanks for taking the time to check my driver :) I truly > > > > > > appreciate > > > > > > all > > > > > > the help! > > > > > > > > > > > > A "fundamental question" regarding these review comments is > > > > > > whether > > > > > > I > > > > > > should add DT entries for these LEDs or not. I thought I > > > > > > shouldn't > > > > > > but > > > > > > I would like to get a comment from Rob regarding it. > > > > > > > > > > If the LED controller is a part of MFD device probed from DT > > > > > then > > > > > there is no doubt it should have corresponding DT sub-node. > > > > > > > > Sorry but I still see no much benefit from adding this > > > > information > > > > in > > > > DT. Why should it have corresponding DT-node if the LED > > > > properties > > > > are > > > > fixed and if we only wish to allow user-space control and have > > > > no > > > > dependencies to other devices in DT? > > > > > > > > In this specific case the information we can provide from DT is > > > > supposed to be fixed. No board based variation. Furthermore, > > > > there > > > > is > > > > not much generic driver/led core functionality which would be > > > > able > > > > to > > > > parse and utilize relevant information from DT. I think we can > > > > only > > > > give the name (function) and colour. And they are supposed to > > > > be > > > > fixed > > > > and thus could be just hard-coded in driver. Hard-coding these > > > > would be > > > > simpler and less error prone for users (no DT bindings to > > > > write) > > > > and > > > > simpler to create and probably also to maintain (no separate > > > > binding > > > > documents needed for LEDs). > > > > > > AFAICS it is possible to connect LED of arbitrary color to the > > > iouts > > > of this device. If this is the case then it is justified to have > > > DT > > > node only to allow for LED name customization. > > > > In theory, yes. In practice (if I understand it correctly) the > > color in > > this case is only visible in sysfs path name. I am not at all sure > > that > > reflecting the (unlikely) color change in path name is worth the > > hassle. Besides - if this happens, then the driver and DT can be > > changed. > > Driver should not be changed. We have DT for conveying board specific > parameters. Driver needs to be changed if we add new feature to it. It is waste to develop features that are never needed. DT support here may be such. So I'd suggest we add DT support later if it is needed. > > It is easier to add DT entries than remove them. If you see > > the color change support as really crucial - then I could even > > consider > > defaulting the colours to amber and green if no colour property is > > present in DT. > > You don't need to default to anything. The color section will be left > empty if the property is not provided. Thanks for this info :) It makes sense. And no need to explain me this if you are busy - but I don't really see why we have led colour in sysfs path? I admit I am not too deep in the world of LEDs (so I am sure there is just something I haven't been thinking about) but it intuitively feels terribly wrong. If I was writing application to control - let's say a charger LED - I would definitely prefer to always have the charger led control in same sysfs path - no matter what the color is. If my application was interested in knowing the colour, then I hoped to be able to read / change it via file "colour" which resides in the charger sysfs path. (And yes, if I had bunch of RGB leds, I would _definitely_ want to change the 'abstract' color - not brightnes of red, green and blue LEDs). But all this is off-topic and not related to this discussion - I was just curious as my limited brains don't see the reasoning behind this :) > > I see no point in _requiring_ the DT entry to be there. > > I'm referring to this later in this message. > > > If we like being prepared for the theoretical possibilities - what > > if > > x86 is used to control this PMIC? I guess we wouldn't have DT there > > then (And no - I don't see such use-case). > > We have fwnode abstraction for that. You can also check: > Documentation/firmware-guide/acpi/dsd/leds.rst. > > > > > But assuming this is Ok to DT-folks and if you insist - I will > > > > add > > > > LED > > > > information to DT for the next patches. Hopefully this extra > > > > complexity > > > > helps in some oddball use-case which I can't foresee =) > > > > > > > > Then what comes to the DT format. > > > > > > > > Do you think LED subsystem should try to follow the convention > > > > with > > > > other sub-systems and not introduce multiple compatibles for > > > > single > > > > device? MFD can handle instantiating the sub-devices just fine > > > > even > > > > when sub-devices have no own compatible property or of_match. > > > > Maybe > > > > we > > > > should also avoid unnecessary sub-nodes when they are not > > > > really > > > > required. > > > > > > This is beyond my scope of responsibility. It is MFD subsystem > > > thing > > > to > > > choose the way of LED class driver instantiation. When it comes > > > to > > > LED subsystem - it expects single compatible pertaining to a > > > physical > > > device. > > > > Sorry but I don't quite follow. What the LED subsystem does with > > the > > compatible property? How does it expect this? > > In case of DT based MFD cell probing you must initialize > of_compatible > property of struct mfd_cell element which will then be matched > with struct platform_driver -> driver -> of_match_table in the LED > class driver. Basing on that a relevant platform_device is passed > to the probe function. Its child struct device's of_node property > comes > already initialized to the pointer to the corresponding child node > in MFD node. I know. I did this at first with the BD71837 - and I was told to not do that. The difference when we don't use of_compatible is that the of_node pointer in sub-device (LEDs) is not populated. But when we have pure MFD sub-device (like LEDs on BD71828), the sub-device knows it is instantiated by MFD (parent) and it can get the relevant DT data from parent's of_node - which kind of makes sense as there really is only one physical device (the MFD). But I see you like to get opinion from Lee and/or Rob - let's hope they help us to align our views. (It is also definitely a good idea to correct my understanding if I have misunderstood this!) > > > Nonetheless, so far we used to have separate compatibles for > > > drivers > > > of > > > MFD devices' LED cells. If we are going to change that I'd like > > > to > > > see > > > explicit DT maintainer's statement confirming that. > > > > I don't expect that existing DTs would be changed. > > I didn't suggest that. > > > But as I said, the > > consensus amongst most of the subsystenm maintainers and DT > > maintainers > > seems to be that sub-devices should not have own compatibles. I > > hope > > Rob acks this here - but knowing he is a busy guy I add some old > > discussions from which I have gathered my understanding: > > > > BD71837 - first patch where regulators had compatible - Mark > > (regulator > > maintainer instructed me to drop it): > > https://lore.kernel.org/linux-clk/20180524140118.GS4828@sirena.org.uk/ > > > > And here Stephen (the clk subsystem maintainer) told me to drop > > whole > > clocks sub-node (including the compatible): > > https://lore.kernel.org/linux-clk/152777867392.144038.18188452389972834689@swboyd.mtv.corp.google.com/ > > Still, there are MFD drivers using of_compatible for matching cell > drivers. I don't follow current trends on MFD subsystem side. > You've got to wait for review feedback from Lee Jones anyway > to find out how to proceed with MFD bindings. Sure. And as the subject states, this whole series is still RFC. I am not expecting the regulator run-level control to be accepted as such - I am hoping to get a push to right direction - although the basic regulator control might go in without big changes. So my case does not require super fast decision - but I think that if the general direction should be more towards dropping own compatibles from MFD sub-devices, then it might be good idea to get this sorted sooner than later :) > > > And one benefit of having separate nodes per MFD cells is that we > > > can > > > easily discern the support for which cells is to be turned on. > > > > We don't want to do DT modifications to drop some sub-device > > support > > out. The DT is HW description and sub-blocks are still there. We > > drop > > the support by KConfig. > > How would you describe the purpose of 'status = "disabled"' DT > assignment then? > > Anyway, I entirely disagree here - it is perfectly proper approach > to define platform capabilities by modifying dts file alone. > This way you can easily create multiple versions of platform > configurations. It may be often impractical to enable all available > platform features, at least from business point of view. And > recompiling > dts is lightweight operation in comparison to kernel compilation. I guess we have different backgrounds here =) For quite a long time I was working with devices that had really long life-span. They received multiple SW updates - but changing a DT was rare. For some of the products DT changes were impossible after the product was out of the factory. For some of the products DT changes were possible - but rare - and during the update the system often booted up in a state where it had either new SW but old DT. In SW fall-back scenarios system often had old SW and new DT. And at times there were systems running new SW but years old DT - especially for those systems where DT was not updated after the product left factory... In that environment all the DT updates were a nightmare. > Not saying that in some cases there are secret keys required for > encrypting kernel images, that may not always be at hand. > > > Only 'configuration' we could bring from DT is > > the amount of connected LEDs (as you said). But on the other hand - > > whether preparing for such unlikely design is reasonable (or > > needed) is > > questionable. > > LED naming related data is vital as well. Sure. But I don't think the LED names need to be changed. On the contrary - I expect the user-space to hope the names stay constant. Maybe I just don't understand something here :) > > > > pmic: pmic@4b { > > > > compatible = "rohm,bd71828"; > > > > reg = <0x4b>; > > > > interrupt-parent = <&gpio1>; > > > > interrupts = <29 GPIO_ACTIVE_LOW>; > > > > clocks = <&osc 0>; > > > > #clock-cells = <0>; > > > > clock-output-names = "bd71828-32k-out"; > > > > gpio-controller; > > > > #gpio-cells = <2>; > > > > ngpios = <4>; > > > > gpio-reserved-ranges = <0 1 2 1>; > > > > gpio-line-names = "EPDEN"; > > > > rohm,dvs-vsel-gpios = <&gpio1 12 0>, > > > > <&gpio1 13 0>; > > > > regulators { > > > > ... > > > > }; > > > > > > > > chg-led { > > > > function = LED_FUNCTION_CHARGING; > > > > color = LED_COLOR_ID_AMBER; > > > > }; > > > > > > > > pwr-led { > > > > function = LED_FUNCTION_POWER; > > > > color = LED_COLOR_ID_GREEN; > > > > }; > > > > > > This way you would probably need to probe LED class driver twice, > > > instead of letting it behave in a standard way and parse child > > > LED > > > nodes. > > > > No. Please note that probing the MFD sub-drivers is _not_ bound to > > device-tree nodes. MFD sub-devices can be probed just fine even if > > they > > have no DT entries. When we add MFD cell for LED driver, the > > corresponding LED driver is probed. No DT magic needed for this. > > > > What the LED driver (as other sub-device drivers) is required to do > > is > > to obtain the pointer to parent device's DT node and find > > information > > which is relevant for it. Ideally, the subsystem framework can > > extract > > the properties which are common for whole subsystem (like color and > > function in case of LEDs) and driver only parses the DT if it has > > some > > custom properties. Again, ideally the driver has sane defaults - or > > some other 'platform data' mechanism if no DT information is found. > > There is architectures which do not support DT. > > LED common bindings define that each LED should be described > using child node. And we've enforced sticking to this standard > for last two years strictly. I am not against that. If DT is used, then it is fine for me to have properties of one LED in own node. But I don't think the DT should be compulsory at all for cases where the LED information stays static. > LED core mechanism for LED name composition also relies on this > DT design - it expects single 'color' and 'function' properties to > be present in the passed fwnode. I am not against this either - although I don't fully understand this as I said above. I believe that set of well defined LED names is a good thing. And LED APIs should indeed force the name to follow specific format. But I don't think that the DT should be only mechanism for bringing the function and colour. I think we should allow LED name composition for example by specifying the colour and function in LED class registration API in cases where fwnode is not needed. > LED class registration function registers single LED and it has been > always LED class driver's responsibility to call it for every LED > connected to the LED controller iouts. This is fine for me too (especially when DT is not used). And my driver draft did this, right? But I see that lots of code duplication in drivers could be avoided if the LED framework provided function which could extract all LEDs from a (MFD) device-tree node and did register more than one of them. The typical "for_each_child_of_node" could be in LED core. But this is currently some what irrelevant - let's first see how the "compatible" discussion for sub-devices turns out ;) > > > In case of BD71828 LEDs my first idea was to go with only the 'sane > > defaults' option as I saw no much configurability. The DT snippet > > above > > contains LED information as per your suggestion. > > > > What the LED sub driver for BD71828 would now do is calling > > devm_led_classdev_register_ext with the DT information of BD71828 > > device. Eg, it should use the MFD dt node (because this is the real > > device) and not just part of it. devm_led_classdev_register_ext > > should > > then extract the LED specific information. I have not checked the > > implementation of devm_led_classdev_register_ext in details - but > > it > > should ignore non led properties and just walk through the LED > > information and create the sysfs interfaces etc. for all LEDs it > > finds. > > This function does not work like that, as explained above. > Please first get acquainted with the way how existing LED class > drivers > approach LED registration. Because otherwise we're wasting each > others' time. Right. I see. So each LED driver must first parse the DT information in order to find the LED node - or each LED node must be identified by what-ever mechanism is invoking the LED driver... I see this could be improved in the future by adding LED framework a mechanism to identify LED nodes. But that discussion is (probably) out of the scope of this driver. Thanks for pointing that out. > > (In my example this is the chg-led and pwr-led sub-nodes). > > Furthermore, > > if no LED information is found from DT I would expect > > devm_led_classdev_register_ext to fail with well-defined return > > value > > so that the driver could do what it now does - Eg, use "sane > > defaults" > > to register the default class-devices for green and amber LEDs. The > > default led class dev naming should of course be same format as it > > would be if DT was populated with green and amber led information. > > Please go through 5.4-rc1 patches related to LED naming improvements > You can also refer to Documentation/leds/leds-class.rst, > "LED Device Naming" section for starter. I will. The naming should be coherent - and names in my current draft were just pulled off from my hat. Thanks. > > > > }; > > > > > > > > How do you see this? Or do you really wish to have this one > > > > extra > > > > node: > > > > > > > > pmic: pmic@4b { > > > > compatible = "rohm,bd71828"; > > > > > > > > reg = <0x4b>; > > > > interrupt-parent = <&gpio1>; > > > > interru > > > > pts = <29 GPIO_ACTIVE_LOW>; > > > > clocks = <&osc 0>; > > > > > > > > #clock-cells = <0>; > > > > clock-output-names = "bd71828-32k-out"; > > > > gpio-controller; > > > > #gpio-cells = <2>; > > > > > > > > ngpios = <4>; > > > > gpio-reserved-ranges = <0 1 2 1>; > > > > > > > > gpio-line-names = "EPDEN"; > > > > rohm,dvs-vsel-gpios = > > > > <&gpio1 12 0>, > > > > <&gpio1 13 0>; > > > > > > > > regulators { > > > > ... > > > > }; > > > > > > > > leds-dummy { > > > > > > Why leds-dummy ? > > > > Because there is no real led controller device in any "MFD bus". It > > is > > just one MFD device with controls for two LEDs. > > > > > The convention is to have led-controller@unit-address as the > > > parent > > > LED > > > controller node. > > > > What is the unit address here? 0x4b is the I2C slave address and it > > is > > the MFD node address. There is no addressing for LED controller as > > there is no separate LED controller device. There is only one > > device, > > the PMIC which is MFD device as it has multiple functions meld in. > > One > > of these functions is LED control and requires LED driver. > > For MFD cell you can have just "led". > > > > > chg-led { > > > s/chg-led/led0/ > > > > > > > function = > > > > LED_FUNCTION_CHARGING; > > > > color = LED_COLOR_ID_AMBER; > > > > }; > > > > > > > > pwr-led { > > > > > > s/pwr-led/led1/ > > > > > > This is ePAPR requirement that DT node name should describe the > > > general class of device. > > > > Thanks. I had some problems with these node names as I wanted to > > make > > them generic (led) but also to include some information what leds > > they > > are. A bit same idea as I see in node names like "chan1" and > > "chan345" > > that are used in ti-lmu bindings I checked for the example. But I > > am > > fine with renaming them in this example! I just don't think we > > should > > have this extra node as I mentioned. > > I wonder what Rob and Lee will say here. I personally would > like to stick to LED common bindings and have this extra node. > We define standards for a reason after all. I don't understand what makes you think we shouldn't stick LED common bindings? We definitely want to have common bindings and increase amount of bindings handled by core instead of handling the bindings in all of the LED drivers. It was just strange to me that LED subsystem uses this "extra node" and "extra compatible" inside MFD whereas (I have understood that) other subsystems seem to be giving up on that. But maybe I am mistaken on that - wouldn't be first time - let's see :) > > > > function = LED_FUNCTION_POWER; > > > > color = LED_COLOR_ID_GREEN; > > > > }; > > > > > > Common LED bindings say this is the proper way to go. However you > > > would need compatible to probe LED class driver in DT based way. > > > > No. I don't. MFD will probe the LED class driver as long as the > > name of > > the driver matches to MFD cell name. > > If you initialize only of_compatible in struct mfd_cell element then > it > will use only that for matching. I bet I was checking that five years > ago while working on leds-max77693 driver. Yes. It sure uses of_compatible for matching and populating the dt node. This is different from probing though. Sub-device is probed just fine even when there is no compatible for in DT - if the name matches. What changes is that the of_node won't be populated and sub driver needs to figure it out. So both approaches *work* - which is considered as "right thing to do"(tm) needs to be figured out. I have no further insight as to why the compatible should or should not be used for MFD sub-devices - I was just told to avoid that in the past. But let's see if we get Rob's or Lee's attention :) > > So we only need MFD driver to be > > probed based on the compatible. Rest of the sub-device drivers will > > be > > probed by MFD. What I am missing is MODULE_ALIAS in LED driver for > > loading the module when MFD is searching for it if it is not > > modprobed > > via scripts or built in-kernel. I have understood this is the > > standard > > way with MFD nowadays - I am positive Lee will kick me if I am > > wrong ;) > > (I think I have bullied him that much in the past :/ ) > > Last sentence confirms my observation that you're strongly inclined > to contest status quo :-) Let's just say that I have had my moments - both in good and in bad :) I am probably not the easiest guy to work with but not the worst either. Actually, problems tend to start when I try to be funny :rolleyes: I should learn when to stop. > > > If you plan to do it otherwise then it makes no sense to have > > > DT nodes for LEDs. > > > > That was my point. This is why I did not have LEDs in DT in first > > place. But as I said above - as a result of this discussion I have > > started thinking that maybe I could check if I can easily add > > support > > for providing LED information also via DT and fall back to defaults > > if > > no LED information is found. (to allow color change or to omit one > > of > > the LEDs as you suggested) > > > > > > > > + > > > > > > > > + bd71828 = dev_get_drvdata(pdev->dev.parent); > > > > > > > > + l = devm_kzalloc(&pdev->dev, sizeof(*l), > > > > > > > > GFP_KERNEL); > > > > > > > > + if (!l) > > > > > > > > + return -ENOMEM; > > > > > > > > + l->bd71828 = bd71828; > > > > > > > > + a = &l->amber; > > > > > > > > + g = &l->green; > > > > > > > > + a->id = ID_AMBER_LED; > > > > > > > > + g->id = ID_GREEN_LED; > > > > > > > > + a->force_mask = BD71828_MASK_LED_AMBER; > > > > > > > > + g->force_mask = BD71828_MASK_LED_GREEN; > > > > > > > > + > > > > > > > > + a->l.name = ANAME; > > > > > > > > + g->l.name = GNAME; > > > > > > > > + a->l.brightness_set_blocking = > > > > > > > > bd71828_led_brightness_set; > > > > > > > > + g->l.brightness_set_blocking = > > > > > > > > bd71828_led_brightness_set; > > > > > > > > + > > > > > > > > + ret = devm_led_classdev_register(&pdev->dev, > > > > > > > > &g->l); > > > > > > > > + if (ret) > > > > > > > > + return ret; > > > > > > > > + > > > > > > > > + return devm_led_classdev_register(&pdev->dev, > > > > > > > > &a->l); > > > > > > > > > > This way you force users to always register two LED class > > > > > devices > > > > > whereas they might need only one. Please compare how other > > > > > LED > > > > > class > > > > > drivers handle DT parsing and LED class device registration. > > > > > > > > I am not sure if I understand correctly what you mean by using > > > > only > > > > one > > > > class device. As I (hopefully) somewhere said - users can't > > > > control > > > > only one of these LEDs. If they decide to enable one led by SW, > > > > then > > > > they inevitably control also the other. Thus it is better that > > > > user > > > > gets control to both of the LEDs if they take the control for > > > > one. > > > > > > > > Or do you mean I could achieve the control for both of these > > > > LEDs > > > > via > > > > only one class device? > > > > > > AFAIU the LEDs, when in SW mode, can be controlled independently, > > > right? > > > > Yes and no. Both of the LEDs can be forced on/off individually - as > > long as one of them is forced ON. If both LEDs are tried to be > > forced > > OFF - then both LEDs are controlled by HW. If both are controlled > > by HW > > and then one is forced ON - the other is also no longer controlled > > by > > HW and is forced OFF. > > > > Eg, bits 0x80 and 0x40 are conrols for these LEDs. 0x80 for one, > > 0x40 > > for the other. Setting bit means LED is on, clearing means LED is > > off - > > with the HW control twist... If either of the bits is set - then > > both > > leds are controlled by these bits (SW control). If both bits are > > cleared, then LEDs are controlled by HW (likely to be off but not > > for > > sure). > > Thank you for the explanation. So they can be represented by separate > LED class devices. Driver logic will just need to update the state of > the sibling LED if it will be affected. Right. Or at first it might be enough (and simplest) to assume that if LEDs are used via this driver, then colour for both LEDs is set explicitly by user-space. I wouldn't try guessing if sibling LED state changes to OFF when one LED is turned ON - or if LED states change to ON if both are turned OFF. This would require exporting interfaces from power-supply driver - and it would still be racy. The thing is that when both LEDs are on board they are both either under HW or SW control. So it makes no sense to control only one LED in such case. Thus I think it is Ok if this LED driver is registering both class devices at one probe. No need to instantiate own platform devices for both of the LEDs. > > > Because if not then there is no point in having separate LED > > > class > > > devices. > > > > > > But if I get it right, then allowing for registering only one LED > > > class > > > device is entirely justifiable - think of a situation when the > > > iout > > > remains not connected on the board. > > > > Yes. This might be unlikely - but this is the reason why I consider > > adding the DT support. I just am not sure if covering this scenario > > now > > is worth the hassle. I tend to think we should only add the DT > > support > > if someone actually produces a board where this LED is not > > connected. > > Could you share what board you're working with? Unfortunately I can't :( I am working for a component vendor and all customer related information is usually strictly confidental - even in cases where it can be guessed...
Hi Matti, On 10/24/19 10:15 AM, Vaittinen, Matti wrote: > Hello Jacek, > > On Wed, 2019-10-23 at 23:59 +0200, Jacek Anaszewski wrote: >> On 10/23/19 10:37 AM, Vaittinen, Matti wrote: >>> On Tue, 2019-10-22 at 19:40 +0200, Jacek Anaszewski wrote: >>>> On 10/22/19 2:40 PM, Vaittinen, Matti wrote: >>>>> On Mon, 2019-10-21 at 21:09 +0200, Jacek Anaszewski wrote: >>>>>> On 10/21/19 10:00 AM, Vaittinen, Matti wrote: >>>>>>> Hello Dan, >>>>>>> >>>>>>> Thanks for taking the time to check my driver :) I truly >>>>>>> appreciate >>>>>>> all >>>>>>> the help! >>>>>>> >>>>>>> A "fundamental question" regarding these review comments is >>>>>>> whether >>>>>>> I >>>>>>> should add DT entries for these LEDs or not. I thought I >>>>>>> shouldn't >>>>>>> but >>>>>>> I would like to get a comment from Rob regarding it. >>>>>> >>>>>> If the LED controller is a part of MFD device probed from DT >>>>>> then >>>>>> there is no doubt it should have corresponding DT sub-node. >>>>> >>>>> Sorry but I still see no much benefit from adding this >>>>> information >>>>> in >>>>> DT. Why should it have corresponding DT-node if the LED >>>>> properties >>>>> are >>>>> fixed and if we only wish to allow user-space control and have >>>>> no >>>>> dependencies to other devices in DT? >>>>> >>>>> In this specific case the information we can provide from DT is >>>>> supposed to be fixed. No board based variation. Furthermore, >>>>> there >>>>> is >>>>> not much generic driver/led core functionality which would be >>>>> able >>>>> to >>>>> parse and utilize relevant information from DT. I think we can >>>>> only >>>>> give the name (function) and colour. And they are supposed to >>>>> be >>>>> fixed >>>>> and thus could be just hard-coded in driver. Hard-coding these >>>>> would be >>>>> simpler and less error prone for users (no DT bindings to >>>>> write) >>>>> and >>>>> simpler to create and probably also to maintain (no separate >>>>> binding >>>>> documents needed for LEDs). >>>> >>>> AFAICS it is possible to connect LED of arbitrary color to the >>>> iouts >>>> of this device. If this is the case then it is justified to have >>>> DT >>>> node only to allow for LED name customization. >>> >>> In theory, yes. In practice (if I understand it correctly) the >>> color in >>> this case is only visible in sysfs path name. I am not at all sure >>> that >>> reflecting the (unlikely) color change in path name is worth the >>> hassle. Besides - if this happens, then the driver and DT can be >>> changed. >> >> Driver should not be changed. We have DT for conveying board specific >> parameters. > > Driver needs to be changed if we add new feature to it. It is waste to > develop features that are never needed. DT support here may be such. So > I'd suggest we add DT support later if it is needed. If you add a driver to mainline kernel you have to think of its all possible applications to make any prospective users' life easier. And besides, I was referring to changes of hardcoded color. You mentioned that color change is unlikely. This indicates that you don't take into account other applications for this device than your current board. Such approach is applied for platform drivers where LEDs are controlled e.g. via MMIO. In this case LED names can be initialized in a static way. You should look at drivers in arch directory. I see that some of them include also headers of mfd devices so it may be your area of interest. If your device is really bound to this single platform then this is different discussion. >>> It is easier to add DT entries than remove them. If you see >>> the color change support as really crucial - then I could even >>> consider >>> defaulting the colours to amber and green if no colour property is >>> present in DT. >> >> You don't need to default to anything. The color section will be left >> empty if the property is not provided. > > Thanks for this info :) It makes sense. > > And no need to explain me this if you are busy - but I don't really see > why we have led colour in sysfs path? I admit I am not too deep in the > world of LEDs (so I am sure there is just something I haven't been > thinking about) but it intuitively feels terribly wrong. If I was > writing application to control - let's say a charger LED - I would > definitely prefer to always have the charger led control in same sysfs > path - no matter what the color is. If my application was interested in > knowing the colour, then I hoped to be able to read / change it via > file "colour" which resides in the charger sysfs path. (And yes, if I > had bunch of RGB leds, I would _definitely_ want to change the > 'abstract' color - not brightnes of red, green and blue LEDs). But all > this is off-topic and not related to this discussion - I was just > curious as my limited brains don't see the reasoning behind this :) LED subsystem was founded in 2006 with this naming convention. Here is an excerpt from the docs touching this issue: " There have been calls for LED properties such as color to be exported as individual led class attributes. As a solution which doesn't incur as much overhead, I suggest these become part of the device name. The naming scheme above leaves scope for further attributes should they be needed. If sections of the name don't apply, just leave that section blank. " I was opting for moving color to the sysfs file during LED naming rework but there was no consensus, so we have to live with that. >>> I see no point in _requiring_ the DT entry to be there. >> >> I'm referring to this later in this message. >> >>> If we like being prepared for the theoretical possibilities - what >>> if >>> x86 is used to control this PMIC? I guess we wouldn't have DT there >>> then (And no - I don't see such use-case). >> >> We have fwnode abstraction for that. You can also check: >> Documentation/firmware-guide/acpi/dsd/leds.rst. >> >>>>> But assuming this is Ok to DT-folks and if you insist - I will >>>>> add >>>>> LED >>>>> information to DT for the next patches. Hopefully this extra >>>>> complexity >>>>> helps in some oddball use-case which I can't foresee =) >>>>> >>>>> Then what comes to the DT format. >>>>> >>>>> Do you think LED subsystem should try to follow the convention >>>>> with >>>>> other sub-systems and not introduce multiple compatibles for >>>>> single >>>>> device? MFD can handle instantiating the sub-devices just fine >>>>> even >>>>> when sub-devices have no own compatible property or of_match. >>>>> Maybe >>>>> we >>>>> should also avoid unnecessary sub-nodes when they are not >>>>> really >>>>> required. >>>> >>>> This is beyond my scope of responsibility. It is MFD subsystem >>>> thing >>>> to >>>> choose the way of LED class driver instantiation. When it comes >>>> to >>>> LED subsystem - it expects single compatible pertaining to a >>>> physical >>>> device. >>> >>> Sorry but I don't quite follow. What the LED subsystem does with >>> the >>> compatible property? How does it expect this? >> >> In case of DT based MFD cell probing you must initialize >> of_compatible >> property of struct mfd_cell element which will then be matched >> with struct platform_driver -> driver -> of_match_table in the LED >> class driver. Basing on that a relevant platform_device is passed >> to the probe function. Its child struct device's of_node property >> comes >> already initialized to the pointer to the corresponding child node >> in MFD node. > > I know. I did this at first with the BD71837 - and I was told to not do > that. The difference when we don't use of_compatible is that the > of_node pointer in sub-device (LEDs) is not populated. But when we have > pure MFD sub-device (like LEDs on BD71828), the sub-device knows it is > instantiated by MFD (parent) and it can get the relevant DT data from > parent's of_node - which kind of makes sense as there really is only > one physical device (the MFD). But I see you like to get opinion from > Lee and/or Rob - let's hope they help us to align our views. (It is > also definitely a good idea to correct my understanding if I have > misunderstood this!) > >>>> Nonetheless, so far we used to have separate compatibles for >>>> drivers >>>> of >>>> MFD devices' LED cells. If we are going to change that I'd like >>>> to >>>> see >>>> explicit DT maintainer's statement confirming that. >>> >>> I don't expect that existing DTs would be changed. >> >> I didn't suggest that. >> >>> But as I said, the >>> consensus amongst most of the subsystenm maintainers and DT >>> maintainers >>> seems to be that sub-devices should not have own compatibles. I >>> hope >>> Rob acks this here - but knowing he is a busy guy I add some old >>> discussions from which I have gathered my understanding: >>> >>> BD71837 - first patch where regulators had compatible - Mark >>> (regulator >>> maintainer instructed me to drop it): >>> https://lore.kernel.org/linux-clk/20180524140118.GS4828@sirena.org.uk/ >>> >>> And here Stephen (the clk subsystem maintainer) told me to drop >>> whole >>> clocks sub-node (including the compatible): >>> https://lore.kernel.org/linux-clk/152777867392.144038.18188452389972834689@swboyd.mtv.corp.google.com/ >> >> Still, there are MFD drivers using of_compatible for matching cell >> drivers. I don't follow current trends on MFD subsystem side. >> You've got to wait for review feedback from Lee Jones anyway >> to find out how to proceed with MFD bindings. > > Sure. And as the subject states, this whole series is still RFC. I am > not expecting the regulator run-level control to be accepted as such - > I am hoping to get a push to right direction - although the basic > regulator control might go in without big changes. So my case does not > require super fast decision - but I think that if the general direction > should be more towards dropping own compatibles from MFD sub-devices, > then it might be good idea to get this sorted sooner than later :) All your doubts may stem from the fact that you look at drivers from platform centric POV and you don't think of portability. >>>> And one benefit of having separate nodes per MFD cells is that we >>>> can >>>> easily discern the support for which cells is to be turned on. >>> >>> We don't want to do DT modifications to drop some sub-device >>> support >>> out. The DT is HW description and sub-blocks are still there. We >>> drop >>> the support by KConfig. >> >> How would you describe the purpose of 'status = "disabled"' DT >> assignment then? >> >> Anyway, I entirely disagree here - it is perfectly proper approach >> to define platform capabilities by modifying dts file alone. >> This way you can easily create multiple versions of platform >> configurations. It may be often impractical to enable all available >> platform features, at least from business point of view. And >> recompiling >> dts is lightweight operation in comparison to kernel compilation. > > I guess we have different backgrounds here =) For quite a long time I > was working with devices that had really long life-span. They received > multiple SW updates - but changing a DT was rare. For some of the > products DT changes were impossible after the product was out of the > factory. For some of the products DT changes were possible - but rare - > and during the update the system often booted up in a state where it > had either new SW but old DT. In SW fall-back scenarios system often > had old SW and new DT. And at times there were systems running new SW > but years old DT - especially for those systems where DT was not > updated after the product left factory... > > In that environment all the DT updates were a nightmare. > >> Not saying that in some cases there are secret keys required for >> encrypting kernel images, that may not always be at hand. >> >>> Only 'configuration' we could bring from DT is >>> the amount of connected LEDs (as you said). But on the other hand - >>> whether preparing for such unlikely design is reasonable (or >>> needed) is >>> questionable. >> >> LED naming related data is vital as well. > > Sure. But I don't think the LED names need to be changed. On the > contrary - I expect the user-space to hope the names stay constant. > Maybe I just don't understand something here :) So this is the gist of the problem - you are platform biased. The same LED controller can be mounted on various platforms and may have different functions, which needs to be reflected in sysfs. >>>>> pmic: pmic@4b { >>>>> compatible = "rohm,bd71828"; >>>>> reg = <0x4b>; >>>>> interrupt-parent = <&gpio1>; >>>>> interrupts = <29 GPIO_ACTIVE_LOW>; >>>>> clocks = <&osc 0>; >>>>> #clock-cells = <0>; >>>>> clock-output-names = "bd71828-32k-out"; >>>>> gpio-controller; >>>>> #gpio-cells = <2>; >>>>> ngpios = <4>; >>>>> gpio-reserved-ranges = <0 1 2 1>; >>>>> gpio-line-names = "EPDEN"; >>>>> rohm,dvs-vsel-gpios = <&gpio1 12 0>, >>>>> <&gpio1 13 0>; >>>>> regulators { >>>>> ... >>>>> }; >>>>> >>>>> chg-led { >>>>> function = LED_FUNCTION_CHARGING; >>>>> color = LED_COLOR_ID_AMBER; >>>>> }; >>>>> >>>>> pwr-led { >>>>> function = LED_FUNCTION_POWER; >>>>> color = LED_COLOR_ID_GREEN; >>>>> }; >>>> >>>> This way you would probably need to probe LED class driver twice, >>>> instead of letting it behave in a standard way and parse child >>>> LED >>>> nodes. >>> >>> No. Please note that probing the MFD sub-drivers is _not_ bound to >>> device-tree nodes. MFD sub-devices can be probed just fine even if >>> they >>> have no DT entries. When we add MFD cell for LED driver, the >>> corresponding LED driver is probed. No DT magic needed for this. >>> >>> What the LED driver (as other sub-device drivers) is required to do >>> is >>> to obtain the pointer to parent device's DT node and find >>> information >>> which is relevant for it. Ideally, the subsystem framework can >>> extract >>> the properties which are common for whole subsystem (like color and >>> function in case of LEDs) and driver only parses the DT if it has >>> some >>> custom properties. Again, ideally the driver has sane defaults - or >>> some other 'platform data' mechanism if no DT information is found. >>> There is architectures which do not support DT. >> >> LED common bindings define that each LED should be described >> using child node. And we've enforced sticking to this standard >> for last two years strictly. > > I am not against that. If DT is used, then it is fine for me to have > properties of one LED in own node. But I don't think the DT should be > compulsory at all for cases where the LED information stays static. > >> LED core mechanism for LED name composition also relies on this >> DT design - it expects single 'color' and 'function' properties to >> be present in the passed fwnode. > > I am not against this either - although I don't fully understand this > as I said above. I believe that set of well defined LED names is a good > thing. And LED APIs should indeed force the name to follow specific > format. But I don't think that the DT should be only mechanism for > bringing the function and colour. I think we should allow LED name > composition for example by specifying the colour and function in LED > class registration API in cases where fwnode is not needed. > >> LED class registration function registers single LED and it has been >> always LED class driver's responsibility to call it for every LED >> connected to the LED controller iouts. > > This is fine for me too (especially when DT is not used). And my driver > draft did this, right? But I see that lots of code duplication in > drivers could be avoided if the LED framework provided function which > could extract all LEDs from a (MFD) device-tree node and did register > more than one of them. The typical "for_each_child_of_node" could be in > LED core. But this is currently some what irrelevant - let's first see > how the "compatible" discussion for sub-devices turns out ;) It is not trivial to come up with generic mechanism for registering sub-LEDs due to various possible iout topologies and arrangements. >> >>> In case of BD71828 LEDs my first idea was to go with only the 'sane >>> defaults' option as I saw no much configurability. The DT snippet >>> above >>> contains LED information as per your suggestion. >>> >>> What the LED sub driver for BD71828 would now do is calling >>> devm_led_classdev_register_ext with the DT information of BD71828 >>> device. Eg, it should use the MFD dt node (because this is the real >>> device) and not just part of it. devm_led_classdev_register_ext >>> should >>> then extract the LED specific information. I have not checked the >>> implementation of devm_led_classdev_register_ext in details - but >>> it >>> should ignore non led properties and just walk through the LED >>> information and create the sysfs interfaces etc. for all LEDs it >>> finds. >> >> This function does not work like that, as explained above. >> Please first get acquainted with the way how existing LED class >> drivers >> approach LED registration. Because otherwise we're wasting each >> others' time. > > Right. I see. So each LED driver must first parse the DT information in > order to find the LED node - or each LED node must be identified by > what-ever mechanism is invoking the LED driver... I see this could be > improved in the future by adding LED framework a mechanism to identify > LED nodes. But that discussion is (probably) out of the scope of this > driver. Thanks for pointing that out. > >>> (In my example this is the chg-led and pwr-led sub-nodes). >>> Furthermore, >>> if no LED information is found from DT I would expect >>> devm_led_classdev_register_ext to fail with well-defined return >>> value >>> so that the driver could do what it now does - Eg, use "sane >>> defaults" >>> to register the default class-devices for green and amber LEDs. The >>> default led class dev naming should of course be same format as it >>> would be if DT was populated with green and amber led information. >> >> Please go through 5.4-rc1 patches related to LED naming improvements >> You can also refer to Documentation/leds/leds-class.rst, >> "LED Device Naming" section for starter. > > I will. The naming should be coherent - and names in my current draft > were just pulled off from my hat. Thanks. > >>>>> }; >>>>> >>>>> How do you see this? Or do you really wish to have this one >>>>> extra >>>>> node: >>>>> >>>>> pmic: pmic@4b { >>>>> compatible = "rohm,bd71828"; >>>>> >>>>> reg = <0x4b>; >>>>> interrupt-parent = <&gpio1>; >>>>> interru >>>>> pts = <29 GPIO_ACTIVE_LOW>; >>>>> clocks = <&osc 0>; >>>>> >>>>> #clock-cells = <0>; >>>>> clock-output-names = "bd71828-32k-out"; >>>>> gpio-controller; >>>>> #gpio-cells = <2>; >>>>> >>>>> ngpios = <4>; >>>>> gpio-reserved-ranges = <0 1 2 1>; >>>>> >>>>> gpio-line-names = "EPDEN"; >>>>> rohm,dvs-vsel-gpios = >>>>> <&gpio1 12 0>, >>>>> <&gpio1 13 0>; >>>>> >>>>> regulators { >>>>> ... >>>>> }; >>>>> >>>>> leds-dummy { >>>> >>>> Why leds-dummy ? >>> >>> Because there is no real led controller device in any "MFD bus". It >>> is >>> just one MFD device with controls for two LEDs. >>> >>>> The convention is to have led-controller@unit-address as the >>>> parent >>>> LED >>>> controller node. >>> >>> What is the unit address here? 0x4b is the I2C slave address and it >>> is >>> the MFD node address. There is no addressing for LED controller as >>> there is no separate LED controller device. There is only one >>> device, >>> the PMIC which is MFD device as it has multiple functions meld in. >>> One >>> of these functions is LED control and requires LED driver. >> >> For MFD cell you can have just "led". >> >>>>> chg-led { >>>> s/chg-led/led0/ >>>> >>>>> function = >>>>> LED_FUNCTION_CHARGING; >>>>> color = LED_COLOR_ID_AMBER; >>>>> }; >>>>> >>>>> pwr-led { >>>> >>>> s/pwr-led/led1/ >>>> >>>> This is ePAPR requirement that DT node name should describe the >>>> general class of device. >>> >>> Thanks. I had some problems with these node names as I wanted to >>> make >>> them generic (led) but also to include some information what leds >>> they >>> are. A bit same idea as I see in node names like "chan1" and >>> "chan345" >>> that are used in ti-lmu bindings I checked for the example. But I >>> am >>> fine with renaming them in this example! I just don't think we >>> should >>> have this extra node as I mentioned. >> >> I wonder what Rob and Lee will say here. I personally would >> like to stick to LED common bindings and have this extra node. >> We define standards for a reason after all. > > I don't understand what makes you think we shouldn't stick LED common > bindings? We definitely want to have common bindings and increase > amount of bindings handled by core instead of handling the bindings in > all of the LED drivers. It was just strange to me that LED subsystem > uses this "extra node" and "extra compatible" inside MFD whereas (I > have understood that) other subsystems seem to be giving up on that. > But maybe I am mistaken on that - wouldn't be first time - let's see :) > >>>>> function = LED_FUNCTION_POWER; >>>>> color = LED_COLOR_ID_GREEN; >>>>> }; >>>> >>>> Common LED bindings say this is the proper way to go. However you >>>> would need compatible to probe LED class driver in DT based way. >>> >>> No. I don't. MFD will probe the LED class driver as long as the >>> name of >>> the driver matches to MFD cell name. >> >> If you initialize only of_compatible in struct mfd_cell element then >> it >> will use only that for matching. I bet I was checking that five years >> ago while working on leds-max77693 driver. > > Yes. It sure uses of_compatible for matching and populating the dt > node. This is different from probing though. Sub-device is probed just > fine even when there is no compatible for in DT - if the name matches. > What changes is that the of_node won't be populated and sub driver > needs to figure it out. So both approaches *work* - which is considered > as "right thing to do"(tm) needs to be figured out. I have no further > insight as to why the compatible should or should not be used for MFD > sub-devices - I was just told to avoid that in the past. But let's see > if we get Rob's or Lee's attention :) > >>> So we only need MFD driver to be >>> probed based on the compatible. Rest of the sub-device drivers will >>> be >>> probed by MFD. What I am missing is MODULE_ALIAS in LED driver for >>> loading the module when MFD is searching for it if it is not >>> modprobed >>> via scripts or built in-kernel. I have understood this is the >>> standard >>> way with MFD nowadays - I am positive Lee will kick me if I am >>> wrong ;) >>> (I think I have bullied him that much in the past :/ ) >> >> Last sentence confirms my observation that you're strongly inclined >> to contest status quo :-) > > Let's just say that I have had my moments - both in good and in bad :) > I am probably not the easiest guy to work with but not the worst > either. Actually, problems tend to start when I try to be funny > :rolleyes: I should learn when to stop. > >>>> If you plan to do it otherwise then it makes no sense to have >>>> DT nodes for LEDs. >>> >>> That was my point. This is why I did not have LEDs in DT in first >>> place. But as I said above - as a result of this discussion I have >>> started thinking that maybe I could check if I can easily add >>> support >>> for providing LED information also via DT and fall back to defaults >>> if >>> no LED information is found. (to allow color change or to omit one >>> of >>> the LEDs as you suggested) > >>>>>>>>> + >>>>>>>>> + bd71828 = dev_get_drvdata(pdev->dev.parent); >>>>>>>>> + l = devm_kzalloc(&pdev->dev, sizeof(*l), >>>>>>>>> GFP_KERNEL); >>>>>>>>> + if (!l) >>>>>>>>> + return -ENOMEM; >>>>>>>>> + l->bd71828 = bd71828; >>>>>>>>> + a = &l->amber; >>>>>>>>> + g = &l->green; >>>>>>>>> + a->id = ID_AMBER_LED; >>>>>>>>> + g->id = ID_GREEN_LED; >>>>>>>>> + a->force_mask = BD71828_MASK_LED_AMBER; >>>>>>>>> + g->force_mask = BD71828_MASK_LED_GREEN; >>>>>>>>> + >>>>>>>>> + a->l.name = ANAME; >>>>>>>>> + g->l.name = GNAME; >>>>>>>>> + a->l.brightness_set_blocking = >>>>>>>>> bd71828_led_brightness_set; >>>>>>>>> + g->l.brightness_set_blocking = >>>>>>>>> bd71828_led_brightness_set; >>>>>>>>> + >>>>>>>>> + ret = devm_led_classdev_register(&pdev->dev, >>>>>>>>> &g->l); >>>>>>>>> + if (ret) >>>>>>>>> + return ret; >>>>>>>>> + >>>>>>>>> + return devm_led_classdev_register(&pdev->dev, >>>>>>>>> &a->l); >>>>>> >>>>>> This way you force users to always register two LED class >>>>>> devices >>>>>> whereas they might need only one. Please compare how other >>>>>> LED >>>>>> class >>>>>> drivers handle DT parsing and LED class device registration. >>>>> >>>>> I am not sure if I understand correctly what you mean by using >>>>> only >>>>> one >>>>> class device. As I (hopefully) somewhere said - users can't >>>>> control >>>>> only one of these LEDs. If they decide to enable one led by SW, >>>>> then >>>>> they inevitably control also the other. Thus it is better that >>>>> user >>>>> gets control to both of the LEDs if they take the control for >>>>> one. >>>>> >>>>> Or do you mean I could achieve the control for both of these >>>>> LEDs >>>>> via >>>>> only one class device? >>>> >>>> AFAIU the LEDs, when in SW mode, can be controlled independently, >>>> right? >>> >>> Yes and no. Both of the LEDs can be forced on/off individually - as >>> long as one of them is forced ON. If both LEDs are tried to be >>> forced >>> OFF - then both LEDs are controlled by HW. If both are controlled >>> by HW >>> and then one is forced ON - the other is also no longer controlled >>> by >>> HW and is forced OFF. >>> >>> Eg, bits 0x80 and 0x40 are conrols for these LEDs. 0x80 for one, >>> 0x40 >>> for the other. Setting bit means LED is on, clearing means LED is >>> off - >>> with the HW control twist... If either of the bits is set - then >>> both >>> leds are controlled by these bits (SW control). If both bits are >>> cleared, then LEDs are controlled by HW (likely to be off but not >>> for >>> sure). >> >> Thank you for the explanation. So they can be represented by separate >> LED class devices. Driver logic will just need to update the state of >> the sibling LED if it will be affected. > > Right. Or at first it might be enough (and simplest) to assume that if > LEDs are used via this driver, then colour for both LEDs is set > explicitly by user-space. I wouldn't try guessing if sibling LED state > changes to OFF when one LED is turned ON - or if LED states change to > ON if both are turned OFF. This would require exporting interfaces from > power-supply driver - and it would still be racy. The thing is that > when both LEDs are on board they are both either under HW or SW > control. So it makes no sense to control only one LED in such case. > Thus I think it is Ok if this LED driver is registering both class > devices at one probe. No need to instantiate own platform devices for > both of the LEDs. We always register all LEDs originating from the same device in one probe. >>>> Because if not then there is no point in having separate LED >>>> class >>>> devices. >>>> >>>> But if I get it right, then allowing for registering only one LED >>>> class >>>> device is entirely justifiable - think of a situation when the >>>> iout >>>> remains not connected on the board. >>> >>> Yes. This might be unlikely - but this is the reason why I consider >>> adding the DT support. I just am not sure if covering this scenario >>> now >>> is worth the hassle. I tend to think we should only add the DT >>> support >>> if someone actually produces a board where this LED is not >>> connected. >> >> Could you share what board you're working with? > > Unfortunately I can't :( I am working for a component vendor and all > customer related information is usually strictly confidental - even in > cases where it can be guessed... > > >
Hi Again Jacek, This has been a nice conversation. I guess I have learned something from this all but I think this is no longer going forward too much :) I'll cook up second version - where I add LEDs to DT (even if I don't see the value for it now). I won't add own compatible for the LED (for now) - as it is part of MFD - and I'll add the LEDs also to binding docs. I think that will get the attention from Lee/Rob better than the LED driver discussion. We can continue discussion there. I hope this is Ok to you. (And then just few compulsory notes about my view on your replies - after all, I can't let you to have the final say xD - you can ignore them or respond just as you like) On Fri, 2019-10-25 at 00:04 +0200, Jacek Anaszewski wrote: > Hi Matti, > > On 10/24/19 10:15 AM, Vaittinen, Matti wrote: > > Hello Jacek, > > > > On Wed, 2019-10-23 at 23:59 +0200, Jacek Anaszewski wrote: > > > On 10/23/19 10:37 AM, Vaittinen, Matti wrote: > > > > On Tue, 2019-10-22 at 19:40 +0200, Jacek Anaszewski wrote: > > > > > On 10/22/19 2:40 PM, Vaittinen, Matti wrote: > > > > > > On Mon, 2019-10-21 at 21:09 +0200, Jacek Anaszewski wrote: > > > > > > > On 10/21/19 10:00 AM, Vaittinen, Matti wrote: > > > > > > > > Hello Dan, > > > > > > > > > > > > > > > > Thanks for taking the time to check my driver :) I > > > > > > > > truly > > > > > > > > appreciate > > > > > > > > all > > > > > > > > the help! > > > > > > > > > > > > > > > > A "fundamental question" regarding these review > > > > > > > > comments is > > > > > > > > whether > > > > > > > > I > > > > > > > > should add DT entries for these LEDs or not. I thought > > > > > > > > I > > > > > > > > shouldn't > > > > > > > > but > > > > > > > > I would like to get a comment from Rob regarding it. > > > > > > > > > > > > > > If the LED controller is a part of MFD device probed from > > > > > > > DT > > > > > > > then > > > > > > > there is no doubt it should have corresponding DT sub- > > > > > > > node. > > > > > > > > > > > > Sorry but I still see no much benefit from adding this > > > > > > information > > > > > > in > > > > > > DT. Why should it have corresponding DT-node if the LED > > > > > > properties > > > > > > are > > > > > > fixed and if we only wish to allow user-space control and > > > > > > have > > > > > > no > > > > > > dependencies to other devices in DT? > > > > > > > > > > > > In this specific case the information we can provide from > > > > > > DT is > > > > > > supposed to be fixed. No board based variation. > > > > > > Furthermore, > > > > > > there > > > > > > is > > > > > > not much generic driver/led core functionality which would > > > > > > be > > > > > > able > > > > > > to > > > > > > parse and utilize relevant information from DT. I think we > > > > > > can > > > > > > only > > > > > > give the name (function) and colour. And they are supposed > > > > > > to > > > > > > be > > > > > > fixed > > > > > > and thus could be just hard-coded in driver. Hard-coding > > > > > > these > > > > > > would be > > > > > > simpler and less error prone for users (no DT bindings to > > > > > > write) > > > > > > and > > > > > > simpler to create and probably also to maintain (no > > > > > > separate > > > > > > binding > > > > > > documents needed for LEDs). > > > > > > > > > > AFAICS it is possible to connect LED of arbitrary color to > > > > > the > > > > > iouts > > > > > of this device. If this is the case then it is justified to > > > > > have > > > > > DT > > > > > node only to allow for LED name customization. > > > > > > > > In theory, yes. In practice (if I understand it correctly) the > > > > color in > > > > this case is only visible in sysfs path name. I am not at all > > > > sure > > > > that > > > > reflecting the (unlikely) color change in path name is worth > > > > the > > > > hassle. Besides - if this happens, then the driver and DT can > > > > be > > > > changed. > > > > > > Driver should not be changed. We have DT for conveying board > > > specific > > > parameters. > > > > Driver needs to be changed if we add new feature to it. It is waste > > to > > develop features that are never needed. DT support here may be > > such. So > > I'd suggest we add DT support later if it is needed. > > If you add a driver to mainline kernel you have to think of its all > possible applications to make any prospective users' life easier. Also, when you add stuff to mainline kernel you want to cut out everything unnecessary. Being prepared for unknown future is often waste. We should add code to support what we have NOW - but do it so that it can be later changed. Good thing of open source is that if it does not meet your needs you can improve it. > And besides, I was referring to changes of hardcoded color. You > mentioned that color change is unlikely. This indicates that you > don't > take into account other applications for this device than your > current > board. PMICs tend to be targeted for specific SoCs. There are few generic PMICs but many are quite SoC specific. Still, I of course hope more SoCs would be using this PMIC in the future - but right now we should consider what we have right now. It may be this is never used to power anything else - who knows. See my previous chapter > Such approach is applied for platform drivers where LEDs are > controlled e.g. via MMIO. In this case LED names can be initialized > in a static way. You should look at drivers in arch directory. > I see that some of them include also headers of mfd devices so > it may be your area of interest. This boils down to ruling out other possibilities. Even if I don't see this PMIC being used in other projects I don't want to ensure that by bolting this PMIC in specific arch code - besides I guess it wouldn't be welcomed there :P > If your device is really bound to this single platform then > this is different discussion. At the moment it is - but we want to pretend it is not in order to keep the doors open :] > > > > It is easier to add DT entries than remove them. If you see > > > > the color change support as really crucial - then I could even > > > > consider > > > > defaulting the colours to amber and green if no colour property > > > > is > > > > present in DT. > > > > > > You don't need to default to anything. The color section will be > > > left > > > empty if the property is not provided. > > > > Thanks for this info :) It makes sense. > > > > And no need to explain me this if you are busy - but I don't really > > see > > why we have led colour in sysfs path? I admit I am not too deep in > > the > > world of LEDs (so I am sure there is just something I haven't been > > thinking about) but it intuitively feels terribly wrong. If I was > > writing application to control - let's say a charger LED - I would > > definitely prefer to always have the charger led control in same > > sysfs > > path - no matter what the color is. If my application was > > interested in > > knowing the colour, then I hoped to be able to read / change it via > > file "colour" which resides in the charger sysfs path. (And yes, if > > I > > had bunch of RGB leds, I would _definitely_ want to change the > > 'abstract' color - not brightnes of red, green and blue LEDs). But > > all > > this is off-topic and not related to this discussion - I was just > > curious as my limited brains don't see the reasoning behind this :) > > LED subsystem was founded in 2006 with this naming convention. > Here is an excerpt from the docs touching this issue: > > " > There have been calls for LED properties such as color to be exported > as > individual led class attributes. As a solution which doesn't incur as > much > overhead, I suggest these become part of the device name. The naming > scheme > above leaves scope for further attributes should they be needed. If > sections > of the name don't apply, just leave that section blank. > " > > I was opting for moving color to the sysfs file during LED naming > rework but there was no consensus, so we have to live with that. I understand that choices have been done under different circumstances. They always are. But knowing how much LEDs have been developed - I wonder how long we can stick with old ways. Actually, I wonder how the devices which control larger amount of RGB leds are done... I could think that having fixed colour makes colour control quite horrible. I wouldn't be surprized if we wouldn't have to live with that rest of our careers :) But as I said, this is not relevant here, I was just curious. Thank you for the explanation! I again appreciate you educating me :) > > > > > > > > > > > Nonetheless, so far we used to have separate compatibles for > > > > > drivers > > > > > of > > > > > MFD devices' LED cells. If we are going to change that I'd > > > > > like > > > > > to > > > > > see > > > > > explicit DT maintainer's statement confirming that. > > > > > > > > I don't expect that existing DTs would be changed. > > > > > > I didn't suggest that. > > > > > > > But as I said, the > > > > consensus amongst most of the subsystenm maintainers and DT > > > > maintainers > > > > seems to be that sub-devices should not have own compatibles. I > > > > hope > > > > Rob acks this here - but knowing he is a busy guy I add some > > > > old > > > > discussions from which I have gathered my understanding: > > > > > > > > BD71837 - first patch where regulators had compatible - Mark > > > > (regulator > > > > maintainer instructed me to drop it): > > > > https://lore.kernel.org/linux-clk/20180524140118.GS4828@sirena.org.uk/ > > > > > > > > And here Stephen (the clk subsystem maintainer) told me to drop > > > > whole > > > > clocks sub-node (including the compatible): > > > > https://lore.kernel.org/linux-clk/152777867392.144038.18188452389972834689@swboyd.mtv.corp.google.com/ > > > > > > Still, there are MFD drivers using of_compatible for matching > > > cell > > > drivers. I don't follow current trends on MFD subsystem side. > > > You've got to wait for review feedback from Lee Jones anyway > > > to find out how to proceed with MFD bindings. > > > > Sure. And as the subject states, this whole series is still RFC. I > > am > > not expecting the regulator run-level control to be accepted as > > such - > > I am hoping to get a push to right direction - although the basic > > regulator control might go in without big changes. So my case does > > not > > require super fast decision - but I think that if the general > > direction > > should be more towards dropping own compatibles from MFD sub- > > devices, > > then it might be good idea to get this sorted sooner than later :) > > All your doubts may stem from the fact that you look at drivers > from platform centric POV and you don't think of portability. In this specific case my doubts stem from the fact that I was previously told to drop the sub-device compatibles and meld the properties in MFD node where possible. > > > > Only 'configuration' we could bring from DT is > > > > the amount of connected LEDs (as you said). But on the other > > > > hand - > > > > whether preparing for such unlikely design is reasonable (or > > > > needed) is > > > > questionable. > > > > > > LED naming related data is vital as well. > > > > Sure. But I don't think the LED names need to be changed. On the > > contrary - I expect the user-space to hope the names stay constant. > > Maybe I just don't understand something here :) > > So this is the gist of the problem - you are platform biased. > The same LED controller can be mounted on various platforms > and may have different functions, which needs to be reflected > in sysfs. Right. I understand this need very well for any generic LED controller. But at this time we are not dealing with generic LED controlled but a PMIC. And even in this case I see no problem adding DT support later if it is needed. > > This is fine for me too (especially when DT is not used). And my > > driver > > draft did this, right? But I see that lots of code duplication in > > drivers could be avoided if the LED framework provided function > > which > > could extract all LEDs from a (MFD) device-tree node and did > > register > > more than one of them. The typical "for_each_child_of_node" could > > be in > > LED core. But this is currently some what irrelevant - let's first > > see > > how the "compatible" discussion for sub-devices turns out ;) > > It is not trivial to come up with generic mechanism for registering > sub-LEDs due to various possible iout topologies and arrangements. Not knowing the field in great depths I can't really tell how complicated it would be. For a LED novice like me it just does not sound harder than it is for some other systems which do similar thing. For example the regulator subsystem does this just fine even though the supply chains may be complex. For example the regulator core does parse all the regulator sub-nodes. The simple regulator drivers don't need to look at the DT node at all. Regulator core even allows registering a DT parsing call-back for individual regulators and pass pointer to very specific regulator sub-node to this call-back during regulator registration - if specific handling is required. But this is just babbling until someone attempts on implementing this. Actually, this might be a fun challenge if I found the time :) Please, don't take this as criticism - I know that I don't _know_ unless I try doing it myself :) It's always easy to shout towards the government when you are in the opposition ;) > > Right. Or at first it might be enough (and simplest) to assume that > > if > > LEDs are used via this driver, then colour for both LEDs is set > > explicitly by user-space. I wouldn't try guessing if sibling LED > > state > > changes to OFF when one LED is turned ON - or if LED states change > > to > > ON if both are turned OFF. This would require exporting interfaces > > from > > power-supply driver - and it would still be racy. The thing is that > > when both LEDs are on board they are both either under HW or SW > > control. So it makes no sense to control only one LED in such case. > > Thus I think it is Ok if this LED driver is registering both class > > devices at one probe. No need to instantiate own platform devices > > for > > both of the LEDs. > > We always register all LEDs originating from the same device in one > probe. > Then I see little benefit from of_compatible or leds subnode for MFD devices with many LEDs. The driver or core must in any ways parse the DT in order to find the sub nodes with information for individual LEDs. I don't think that would be much different from just using the MFD node as controller node and walking through the MFD child nodes to locate LED sub nodes (at least for MFDs with simple LED controller). Br, Matti Vaittinen
On Fri, Oct 25, 2019 at 2:07 AM Vaittinen, Matti <Matti.Vaittinen@fi.rohmeurope.com> wrote: > > Hi Again Jacek, > > This has been a nice conversation. I guess I have learned something > from this all but I think this is no longer going forward too much :) > I'll cook up second version - where I add LEDs to DT (even if I don't > see the value for it now). I won't add own compatible for the LED (for > now) - as it is part of MFD - and I'll add the LEDs also to binding > docs. I think that will get the attention from Lee/Rob better than the > LED driver discussion. We can continue discussion there. I hope this is > Ok to you. (And then just few compulsory notes about my view on your > replies - after all, I can't let you to have the final say xD - you can > ignore them or respond just as you like) > > On Fri, 2019-10-25 at 00:04 +0200, Jacek Anaszewski wrote: > > Hi Matti, > > > > On 10/24/19 10:15 AM, Vaittinen, Matti wrote: > > > Hello Jacek, > > > > > > On Wed, 2019-10-23 at 23:59 +0200, Jacek Anaszewski wrote: > > > > On 10/23/19 10:37 AM, Vaittinen, Matti wrote: > > > > > On Tue, 2019-10-22 at 19:40 +0200, Jacek Anaszewski wrote: > > > > > > On 10/22/19 2:40 PM, Vaittinen, Matti wrote: > > > > > > > On Mon, 2019-10-21 at 21:09 +0200, Jacek Anaszewski wrote: > > > > > > > > On 10/21/19 10:00 AM, Vaittinen, Matti wrote: > > > > > > > > > Hello Dan, > > > > > > > > > > > > > > > > > > Thanks for taking the time to check my driver :) I > > > > > > > > > truly > > > > > > > > > appreciate > > > > > > > > > all > > > > > > > > > the help! > > > > > > > > > > > > > > > > > > A "fundamental question" regarding these review > > > > > > > > > comments is > > > > > > > > > whether > > > > > > > > > I > > > > > > > > > should add DT entries for these LEDs or not. I thought > > > > > > > > > I > > > > > > > > > shouldn't > > > > > > > > > but > > > > > > > > > I would like to get a comment from Rob regarding it. > > > > > > > > > > > > > > > > If the LED controller is a part of MFD device probed from > > > > > > > > DT > > > > > > > > then > > > > > > > > there is no doubt it should have corresponding DT sub- > > > > > > > > node. Agreed. [...] > > > Right. Or at first it might be enough (and simplest) to assume that > > > if > > > LEDs are used via this driver, then colour for both LEDs is set > > > explicitly by user-space. I wouldn't try guessing if sibling LED > > > state > > > changes to OFF when one LED is turned ON - or if LED states change > > > to > > > ON if both are turned OFF. This would require exporting interfaces > > > from > > > power-supply driver - and it would still be racy. The thing is that > > > when both LEDs are on board they are both either under HW or SW > > > control. So it makes no sense to control only one LED in such case. > > > Thus I think it is Ok if this LED driver is registering both class > > > devices at one probe. No need to instantiate own platform devices > > > for > > > both of the LEDs. > > > > We always register all LEDs originating from the same device in one > > probe. > > > > Then I see little benefit from of_compatible or leds subnode for MFD > devices with many LEDs. The driver or core must in any ways parse the > DT in order to find the sub nodes with information for individual LEDs. > I don't think that would be much different from just using the MFD node > as controller node and walking through the MFD child nodes to locate > LED sub nodes (at least for MFDs with simple LED controller). The cases for not having child nodes are when you have child nodes with nothing more than a compatible and possibly provider properties (e.g. #gpio-cells for gpio providers). If you have other resource dependencies (e.g. clocks) or data to define (e.g. voltages for regulators), then child nodes absolutely make sense. Once we have child nodes, then generally it is easier for every function to be a child node and not mix the two. I'm sure I have told people incorrectly to not do child nodes because they define incomplete bindings. I would group the led nodes under an led-controller node (with a compatible). The simple reason is each level only has one number/address space and you can't mix different ones. You're not numbering the leds here, but could you (with numbers that correspond to something in the h/w, not just 0..N)? The other reason is modern LED bindings define a controller node for the controller and led nodes for the actual led on the board. While the MFD node could be the controller node, that gets into mixing. Rob
Hello Peeps, On Fri, 2019-10-25 at 08:24 -0500, Rob Herring wrote: > On Fri, Oct 25, 2019 at 2:07 AM Vaittinen, Matti > <Matti.Vaittinen@fi.rohmeurope.com> wrote: > > Hi Again Jacek, > > > > This has been a nice conversation. I guess I have learned something > > from this all but I think this is no longer going forward too much > > :) > > I'll cook up second version - where I add LEDs to DT (even if I > > don't > > see the value for it now). I won't add own compatible for the LED > > (for > > now) - as it is part of MFD - and I'll add the LEDs also to binding > > docs. I think that will get the attention from Lee/Rob better than > > the > > LED driver discussion. We can continue discussion there. I hope > > this is > > Ok to you. (And then just few compulsory notes about my view on > > your > > replies - after all, I can't let you to have the final say xD - you > > can > > ignore them or respond just as you like) > > > > On Fri, 2019-10-25 at 00:04 +0200, Jacek Anaszewski wrote: > > > Hi Matti, > > > > > > On 10/24/19 10:15 AM, Vaittinen, Matti wrote: > > > > Hello Jacek, > > > > > > > > On Wed, 2019-10-23 at 23:59 +0200, Jacek Anaszewski wrote: > > > > > On 10/23/19 10:37 AM, Vaittinen, Matti wrote: > > > > > > On Tue, 2019-10-22 at 19:40 +0200, Jacek Anaszewski wrote: > > > > > > > On 10/22/19 2:40 PM, Vaittinen, Matti wrote: > > > > > > > > On Mon, 2019-10-21 at 21:09 +0200, Jacek Anaszewski > > > > > > > > wrote: > > > > > > > > > On 10/21/19 10:00 AM, Vaittinen, Matti wrote: > > > > > > > > > > Hello Dan, > > > > > > > > > > > > > > > > > > > > Thanks for taking the time to check my driver :) I > > > > > > > > > > truly > > > > > > > > > > appreciate > > > > > > > > > > all > > > > > > > > > > the help! > > > > > > > > > > > > > > > > > > > > A "fundamental question" regarding these review > > > > > > > > > > comments is > > > > > > > > > > whether > > > > > > > > > > I > > > > > > > > > > should add DT entries for these LEDs or not. I > > > > > > > > > > thought > > > > > > > > > > I > > > > > > > > > > shouldn't > > > > > > > > > > but > > > > > > > > > > I would like to get a comment from Rob regarding > > > > > > > > > > it. > > > > > > > > > > > > > > > > > > If the LED controller is a part of MFD device probed > > > > > > > > > from > > > > > > > > > DT > > > > > > > > > then > > > > > > > > > there is no doubt it should have corresponding DT > > > > > > > > > sub- > > > > > > > > > node. > > Agreed. Ouch. That annoying feeling when you notice you have been wrong... > [...] > > > > > Right. Or at first it might be enough (and simplest) to assume > > > > that > > > > if > > > > LEDs are used via this driver, then colour for both LEDs is set > > > > explicitly by user-space. I wouldn't try guessing if sibling > > > > LED > > > > state > > > > changes to OFF when one LED is turned ON - or if LED states > > > > change > > > > to > > > > ON if both are turned OFF. This would require exporting > > > > interfaces > > > > from > > > > power-supply driver - and it would still be racy. The thing is > > > > that > > > > when both LEDs are on board they are both either under HW or SW > > > > control. So it makes no sense to control only one LED in such > > > > case. > > > > Thus I think it is Ok if this LED driver is registering both > > > > class > > > > devices at one probe. No need to instantiate own platform > > > > devices > > > > for > > > > both of the LEDs. > > > > > > We always register all LEDs originating from the same device in > > > one > > > probe. > > > > > > > Then I see little benefit from of_compatible or leds subnode for > > MFD > > devices with many LEDs. The driver or core must in any ways parse > > the > > DT in order to find the sub nodes with information for individual > > LEDs. > > I don't think that would be much different from just using the MFD > > node > > as controller node and walking through the MFD child nodes to > > locate > > LED sub nodes (at least for MFDs with simple LED controller). > > The cases for not having child nodes are when you have child nodes > with nothing more than a compatible and possibly provider properties > (e.g. #gpio-cells for gpio providers). If you have other resource > dependencies (e.g. clocks) or data to define (e.g. voltages for > regulators), then child nodes absolutely make sense. Thanks for telling the reasoning behind. Makes sense. > Once we have > child nodes, then generally it is easier for every function to be a > child node and not mix the two. I'm sure I have told people > incorrectly to not do child nodes because they define incomplete > bindings. Does this mean that if I add LED controlled node with LED nodes inside - then I should actually add sub nodes for clk and GPIO too? I would prefer still having the clk provider information in MFD node as adding a sub-node for clk would probably require changes in the bd718x7_clk driver. (Not big ones but avoidable if clk provider information can still dwell in MFD node). > I would group the led nodes under an led-controller node (with a > compatible). The simple reason is each level only has one > number/address space and you can't mix different ones. You're not > numbering the leds here, but could you (with numbers that correspond > to something in the h/w, not just 0..N)? I don't know what that would be. The LED controller resides in MFD device in I2C bus and has no meaningful numbers I can think of. The actual LEDs (on my board) are dummy devices and I really don't know how to invent meaningfull numbers for them either. > The other reason is modern > LED bindings define a controller node for the controller and led > nodes > for the actual led on the board. While the MFD node could be the > controller node, that gets into mixing. My idea (if we use DT) was to use the MFD as controller - but that really would be "mixing" then. I'll see what I can prepare for v3. Oh, and sorry Jacek for taking the time - I guess it was frustrating for you - I really thought I knew how this should be done. Being wrong is hard job at times, but so must be being right ;) Br, Matti
On Fri, Oct 25, 2019 at 9:37 AM Vaittinen, Matti <Matti.Vaittinen@fi.rohmeurope.com> wrote: > > Hello Peeps, > > On Fri, 2019-10-25 at 08:24 -0500, Rob Herring wrote: > > On Fri, Oct 25, 2019 at 2:07 AM Vaittinen, Matti > > <Matti.Vaittinen@fi.rohmeurope.com> wrote: > > > Hi Again Jacek, > > > > > > This has been a nice conversation. I guess I have learned something > > > from this all but I think this is no longer going forward too much > > > :) > > > I'll cook up second version - where I add LEDs to DT (even if I > > > don't > > > see the value for it now). I won't add own compatible for the LED > > > (for > > > now) - as it is part of MFD - and I'll add the LEDs also to binding > > > docs. I think that will get the attention from Lee/Rob better than > > > the > > > LED driver discussion. We can continue discussion there. I hope > > > this is > > > Ok to you. (And then just few compulsory notes about my view on > > > your > > > replies - after all, I can't let you to have the final say xD - you > > > can > > > ignore them or respond just as you like) > > > > > > On Fri, 2019-10-25 at 00:04 +0200, Jacek Anaszewski wrote: > > > > Hi Matti, > > > > > > > > On 10/24/19 10:15 AM, Vaittinen, Matti wrote: > > > > > Hello Jacek, > > > > > > > > > > On Wed, 2019-10-23 at 23:59 +0200, Jacek Anaszewski wrote: > > > > > > On 10/23/19 10:37 AM, Vaittinen, Matti wrote: > > > > > > > On Tue, 2019-10-22 at 19:40 +0200, Jacek Anaszewski wrote: > > > > > > > > On 10/22/19 2:40 PM, Vaittinen, Matti wrote: > > > > > > > > > On Mon, 2019-10-21 at 21:09 +0200, Jacek Anaszewski > > > > > > > > > wrote: > > > > > > > > > > On 10/21/19 10:00 AM, Vaittinen, Matti wrote: > > > > > > > > > > > Hello Dan, > > > > > > > > > > > > > > > > > > > > > > Thanks for taking the time to check my driver :) I > > > > > > > > > > > truly > > > > > > > > > > > appreciate > > > > > > > > > > > all > > > > > > > > > > > the help! > > > > > > > > > > > > > > > > > > > > > > A "fundamental question" regarding these review > > > > > > > > > > > comments is > > > > > > > > > > > whether > > > > > > > > > > > I > > > > > > > > > > > should add DT entries for these LEDs or not. I > > > > > > > > > > > thought > > > > > > > > > > > I > > > > > > > > > > > shouldn't > > > > > > > > > > > but > > > > > > > > > > > I would like to get a comment from Rob regarding > > > > > > > > > > > it. > > > > > > > > > > > > > > > > > > > > If the LED controller is a part of MFD device probed > > > > > > > > > > from > > > > > > > > > > DT > > > > > > > > > > then > > > > > > > > > > there is no doubt it should have corresponding DT > > > > > > > > > > sub- > > > > > > > > > > node. > > > > Agreed. > > Ouch. That annoying feeling when you notice you have been wrong... > > > [...] > > > > > > > Right. Or at first it might be enough (and simplest) to assume > > > > > that > > > > > if > > > > > LEDs are used via this driver, then colour for both LEDs is set > > > > > explicitly by user-space. I wouldn't try guessing if sibling > > > > > LED > > > > > state > > > > > changes to OFF when one LED is turned ON - or if LED states > > > > > change > > > > > to > > > > > ON if both are turned OFF. This would require exporting > > > > > interfaces > > > > > from > > > > > power-supply driver - and it would still be racy. The thing is > > > > > that > > > > > when both LEDs are on board they are both either under HW or SW > > > > > control. So it makes no sense to control only one LED in such > > > > > case. > > > > > Thus I think it is Ok if this LED driver is registering both > > > > > class > > > > > devices at one probe. No need to instantiate own platform > > > > > devices > > > > > for > > > > > both of the LEDs. > > > > > > > > We always register all LEDs originating from the same device in > > > > one > > > > probe. > > > > > > > > > > Then I see little benefit from of_compatible or leds subnode for > > > MFD > > > devices with many LEDs. The driver or core must in any ways parse > > > the > > > DT in order to find the sub nodes with information for individual > > > LEDs. > > > I don't think that would be much different from just using the MFD > > > node > > > as controller node and walking through the MFD child nodes to > > > locate > > > LED sub nodes (at least for MFDs with simple LED controller). > > > > The cases for not having child nodes are when you have child nodes > > with nothing more than a compatible and possibly provider properties > > (e.g. #gpio-cells for gpio providers). If you have other resource > > dependencies (e.g. clocks) or data to define (e.g. voltages for > > regulators), then child nodes absolutely make sense. > > Thanks for telling the reasoning behind. Makes sense. > > > Once we have > > child nodes, then generally it is easier for every function to be a > > child node and not mix the two. I'm sure I have told people > > incorrectly to not do child nodes because they define incomplete > > bindings. > > Does this mean that if I add LED controlled node with LED nodes inside > - then I should actually add sub nodes for clk and GPIO too? I would > prefer still having the clk provider information in MFD node as adding > a sub-node for clk would probably require changes in the bd718x7_clk > driver. (Not big ones but avoidable if clk provider information can > still dwell in MFD node). Probably not, if there's an existing structure to follow, then continue doing that. > > I would group the led nodes under an led-controller node (with a > > compatible). The simple reason is each level only has one > > number/address space and you can't mix different ones. You're not > > numbering the leds here, but could you (with numbers that correspond > > to something in the h/w, not just 0..N)? > > I don't know what that would be. The LED controller resides in MFD > device in I2C bus and has no meaningful numbers I can think of. The > actual LEDs (on my board) are dummy devices and I really don't know how > to invent meaningfull numbers for them either. If you have something like "led control registers 1, 2, 3" where 1,2,3 is each LED channel, then use that. Or if the LED supplies (or supply pins) have some numbering, use that. If there's none of that, then following standard node names kind of falls apart. '<generic name>-N' is what I've been defining for some schema. Rob
Hello Rob, On Fri, 2019-10-25 at 10:47 -0500, Rob Herring wrote: > On Fri, Oct 25, 2019 at 9:37 AM Vaittinen, Matti > <Matti.Vaittinen@fi.rohmeurope.com> wrote: > > Hello Peeps, > > > > On Fri, 2019-10-25 at 08:24 -0500, Rob Herring wrote: > > > On Fri, Oct 25, 2019 at 2:07 AM Vaittinen, Matti > > > <Matti.Vaittinen@fi.rohmeurope.com> wrote: > > > The cases for not having child nodes are when you have child > > > nodes > > > with nothing more than a compatible and possibly provider > > > properties > > > (e.g. #gpio-cells for gpio providers). If you have other resource > > > dependencies (e.g. clocks) or data to define (e.g. voltages for > > > regulators), then child nodes absolutely make sense. > > > > Thanks for telling the reasoning behind. Makes sense. > > > > > Once we have > > > child nodes, then generally it is easier for every function to be > > > a > > > child node and not mix the two. I'm sure I have told people > > > incorrectly to not do child nodes because they define incomplete > > > bindings. > > > > Does this mean that if I add LED controlled node with LED nodes > > inside > > - then I should actually add sub nodes for clk and GPIO too? I > > would > > prefer still having the clk provider information in MFD node as > > adding > > a sub-node for clk would probably require changes in the > > bd718x7_clk > > driver. (Not big ones but avoidable if clk provider information can > > still dwell in MFD node). > > Probably not, if there's an existing structure to follow, then > continue doing that. Ok, thanks. > > > > I would group the led nodes under an led-controller node (with a > > > compatible). The simple reason is each level only has one > > > number/address space and you can't mix different ones. You're not > > > numbering the leds here, but could you (with numbers that > > > correspond > > > to something in the h/w, not just 0..N)? > > > > I don't know what that would be. The LED controller resides in MFD > > device in I2C bus and has no meaningful numbers I can think of. The > > actual LEDs (on my board) are dummy devices and I really don't know > > how > > to invent meaningfull numbers for them either. > > If you have something like "led control registers 1, 2, 3" where > 1,2,3 > is each LED channel, then use that. Unfortunately, no. LED controls are in same register. > Or if the LED supplies (or supply > pins) have some numbering, use that. I don't know how to format the numbering either. Currently planned PMIC package is so called "UCSP55M3C" meaning the pins are in a matrix - columns having numbers from 1 to 8 and rows having letters from A to J. In this case the LED outputs are F6 and H6. I don't know if different packaging is planned. Only 'constant' I can find is the output pin naming 'GRNLED' and 'AMBLED' :/ > If there's none of that, then > following standard node names kind of falls apart. '<generic name>-N' > is what I've been defining for some schema. So I could use node names led-1 and led-2 in the example, right? Br, Matti Vaittinen
diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig index b0fdeef10bd9..ec59f28bcb39 100644 --- a/drivers/leds/Kconfig +++ b/drivers/leds/Kconfig @@ -529,6 +529,16 @@ config LEDS_BD2802 This option enables support for BD2802GU RGB LED driver chips accessed via the I2C bus. +config LEDS_BD71828 + tristate "LED driver for LED pins on ROHM BD71828 PMIC" + depends on LEDS_CLASS + depends on I2C + help + This option enables support for LED outputs located on ROHM + BD71828 power management IC. ROHM BD71828 has two led output pins + which can be left to indicate HW states or controlled by SW. Say + yes here if you want to enable SW control for these LEDs. + config LEDS_INTEL_SS4200 tristate "LED driver for Intel NAS SS4200 series" depends on LEDS_CLASS diff --git a/drivers/leds/Makefile b/drivers/leds/Makefile index 41fb073a39c1..2a8f6a8e4c7c 100644 --- a/drivers/leds/Makefile +++ b/drivers/leds/Makefile @@ -15,6 +15,7 @@ obj-$(CONFIG_LEDS_AN30259A) += leds-an30259a.o obj-$(CONFIG_LEDS_BCM6328) += leds-bcm6328.o obj-$(CONFIG_LEDS_BCM6358) += leds-bcm6358.o obj-$(CONFIG_LEDS_BD2802) += leds-bd2802.o +obj-$(CONFIG_LEDS_BD71828) += leds-bd71828.o obj-$(CONFIG_LEDS_CPCAP) += leds-cpcap.o obj-$(CONFIG_LEDS_LOCOMO) += leds-locomo.o obj-$(CONFIG_LEDS_LM3530) += leds-lm3530.o diff --git a/drivers/leds/leds-bd71828.c b/drivers/leds/leds-bd71828.c new file mode 100644 index 000000000000..2427619444f5 --- /dev/null +++ b/drivers/leds/leds-bd71828.c @@ -0,0 +1,97 @@ +// SPDX-License-Identifier: GPL-2.0 +// Copyright (C) 2019 ROHM Semiconductors + +#include <linux/device.h> +#include <linux/err.h> +#include <linux/kernel.h> +#include <linux/leds.h> +#include <linux/mfd/rohm-bd71828.h> +#include <linux/module.h> +#include <linux/platform_device.h> +#include <linux/regmap.h> +#include <linux/slab.h> + +#define BD71828_LED_TO_DATA(l) ((l)->id == ID_GREEN_LED ? \ + container_of((l), struct bd71828_leds, green) : \ + container_of((l), struct bd71828_leds, amber)) + +enum { + ID_GREEN_LED, + ID_AMBER_LED, + ID_NMBR_OF, +}; + +struct bd71828_led { + int id; + struct led_classdev l; + u8 force_mask; +}; + +struct bd71828_leds { + struct rohm_regmap_dev *bd71828; + struct bd71828_led green; + struct bd71828_led amber; +}; + +static int bd71828_led_brightness_set(struct led_classdev *led_cdev, + enum led_brightness value) +{ + struct bd71828_led *l = container_of(led_cdev, struct bd71828_led, l); + struct bd71828_leds *data; + unsigned int val = BD71828_LED_OFF; + + data = BD71828_LED_TO_DATA(l); + if (value != LED_OFF) + val = BD71828_LED_ON; + + return regmap_update_bits(data->bd71828->regmap, BD71828_REG_LED_CTRL, + l->force_mask, val); +} + +static int bd71828_led_probe(struct platform_device *pdev) +{ + struct rohm_regmap_dev *bd71828; + struct bd71828_leds *l; + struct bd71828_led *g, *a; + static const char *GNAME = "bd71828-green-led"; + static const char *ANAME = "bd71828-amber-led"; + int ret; + + pr_info("bd71828 LED driver probed\n"); + + bd71828 = dev_get_drvdata(pdev->dev.parent); + l = devm_kzalloc(&pdev->dev, sizeof(*l), GFP_KERNEL); + if (!l) + return -ENOMEM; + l->bd71828 = bd71828; + a = &l->amber; + g = &l->green; + a->id = ID_AMBER_LED; + g->id = ID_GREEN_LED; + a->force_mask = BD71828_MASK_LED_AMBER; + g->force_mask = BD71828_MASK_LED_GREEN; + + a->l.name = ANAME; + g->l.name = GNAME; + a->l.brightness_set_blocking = bd71828_led_brightness_set; + g->l.brightness_set_blocking = bd71828_led_brightness_set; + + ret = devm_led_classdev_register(&pdev->dev, &g->l); + if (ret) + return ret; + + return devm_led_classdev_register(&pdev->dev, &a->l); +} + +static struct platform_driver bd71828_led_driver = { + .driver = { + .name = "bd71828-led", + }, + .probe = bd71828_led_probe, +}; + +module_platform_driver(bd71828_led_driver); + +MODULE_AUTHOR("Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com>"); +MODULE_DESCRIPTION("ROHM BD71828 LED driver"); +MODULE_LICENSE("GPL");
ROHM BD71828 power management IC has two LED outputs for charge status and button pressing indications. The LED outputs can also be forced bs SW so add driver allowing to use these LEDs for other indications as well. Leds are controlled by SW using 'Force ON' bits. Please note the constrains mentioned in data-sheet: 1. If one LED is forced ON - then also the other LED is forced. => You can't use SW control to force ON one LED and allow HW to control the other. 2. You can't force both LEDs OFF. If the FORCE bit for both LED's is zero, then LEDs are controlled by HW and indicate button/charger states as explained in data-sheet. Signed-off-by: Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com> --- drivers/leds/Kconfig | 10 ++++ drivers/leds/Makefile | 1 + drivers/leds/leds-bd71828.c | 97 +++++++++++++++++++++++++++++++++++++ 3 files changed, 108 insertions(+) create mode 100644 drivers/leds/leds-bd71828.c