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

[Vaittinen, Matti]

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