Thread (13 messages) 13 messages, 3 authors, 2019-10-29

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

From: Vaittinen, Matti <hidden>
Date: 2019-10-22 12:40:44
Also in: linux-clk, linux-gpio, linux-leds, linux-rtc, lkml 

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:
quoted
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?
quoted
On Thu, 2019-10-17 at 09:04 -0500, Dan Murphy wrote:
quoted
Matt

On 10/17/19 4:53 AM, Matti Vaittinen wrote:
quoted
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
quoted
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
quoted
+	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
:(

quoted
quoted
quoted
  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".

quoted
quoted
quoted
+
+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).

quoted
quoted
quoted
+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'.

quoted
quoted
quoted
+	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.

quoted
quoted
+
+	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?

quoted
quoted
quoted
+}
+
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.

quoted
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.

quoted
quoted
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



  




  

    Related discussions
    
  



    
  

  
    

      

        Keyboard shortcuts
      

      

        
          
hback out one level

          
jnext message in thread

          
kprevious message in thread

          
ldrill in

          
Escclose help / fold thread tree

          
?toggle this help