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
