Re: [RFC PATCH 11/13] led: bd71828: Support LED outputs on ROHM BD71828 PMIC

[Vaittinen, Matti]

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@xxxxxxxxxxxxxxxxx>
> > ---
> >   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