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

["Vaittinen, Matti" <Matti.Vaittinen@fi.rohmeurope.com>]

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