On Fri, Oct 25, 2019 at 9:37 AM Vaittinen, Matti
[off-list ref] wrote:

Hello Peeps,

On Fri, 2019-10-25 at 08:24 -0500, Rob Herring wrote:

quoted

On Fri, Oct 25, 2019 at 2:07 AM Vaittinen, Matti
[off-list ref] wrote:

quoted

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:

quoted

Hi Matti,

On 10/24/19 10:15 AM, Vaittinen, Matti wrote:

quoted

Hello Jacek,

On Wed, 2019-10-23 at 23:59 +0200, Jacek Anaszewski wrote:

quoted

On 10/23/19 10:37 AM, Vaittinen, Matti wrote:

quoted

On Tue, 2019-10-22 at 19:40 +0200, Jacek Anaszewski wrote:

quoted

On 10/22/19 2:40 PM, Vaittinen, Matti wrote:

quoted

On Mon, 2019-10-21 at 21:09 +0200, Jacek Anaszewski
wrote:

quoted

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.

Agreed.

Ouch. That annoying feeling when you notice you have been wrong...

quoted

[...]

quoted

quoted

quoted

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.

quoted

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

Probably not, if there's an existing structure to follow, then
continue doing that.

quoted

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.

If you have something like "led control registers 1, 2, 3" where 1,2,3
is each LED channel, then use that. Or if the LED supplies (or supply
pins) have some numbering, use that. If there's none of that, then
following standard node names kind of falls apart. '<generic name>-N'
is what I've been defining for some schema.

Rob