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

["Vaittinen, Matti" <Matti.Vaittinen@xxxxxxxxxxxxxxxxx>]

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@xxxxxxxxxxxxx/
> > > > 
> > > > 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@xxxxxxxxxxxxxxxxxxxxxxxxxx/
> > > 
> > > 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