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:

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.

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 :]

quoted

quoted

quoted

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

quoted

quoted

quoted

quoted

quoted

quoted

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.

quoted

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/ (local)

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/ (local)

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.

quoted

quoted

quoted

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.

quoted

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

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


Br,
	Matti Vaittinen