Hello Jacek,

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

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.

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

quoted

I see no point in _requiring_ the DT entry to be there.

I'm referring to this later in this message.

quoted

If we like being prepared for the theoretical possibilities - what
if
x86 is used to control this PMIC? I guess we wouldn't have DT there
then (And no - I don't see such use-case).

We have fwnode abstraction for that. You can also check:
Documentation/firmware-guide/acpi/dsd/leds.rst.

quoted

quoted

quoted

But assuming this is Ok to DT-folks and if you insist - I will
add
LED
information to DT for the next patches. Hopefully this extra
complexity
helps in some oddball use-case which I can't foresee =)

Then what comes to the DT format.

Do you think LED subsystem should try to follow the convention
with
other sub-systems and not introduce multiple compatibles for
single
device? MFD can handle instantiating the sub-devices just fine
even
when sub-devices have no own compatible property or of_match.
Maybe
we
should also avoid unnecessary sub-nodes when they are not
really
required.

This is beyond my scope of responsibility. It is MFD subsystem
thing
to
choose the way of LED class driver instantiation. When it comes
to
LED subsystem - it expects single compatible pertaining to a
physical
device.

Sorry but I don't quite follow. What the LED subsystem does with
the
compatible property? How does it expect this?

In case of DT based MFD cell probing you must initialize
of_compatible
property of struct mfd_cell element which will then be matched
with struct platform_driver -> driver -> of_match_table in the LED
class driver. Basing on that a relevant platform_device is passed
to the probe function. Its child struct device's of_node property
comes
already initialized to the pointer to the corresponding child node
in MFD node.

I know. I did this at first with the BD71837 - and I was told to not do
that. The difference when we don't use of_compatible is that the
of_node pointer in sub-device (LEDs) is not populated. But when we have
pure MFD sub-device (like LEDs on BD71828), the sub-device knows it is
instantiated by MFD (parent) and it can get the relevant DT data from
parent's of_node - which kind of makes sense as there really is only
one physical device (the MFD). But I see you like to get opinion from
Lee and/or Rob - let's hope they help us to align our views. (It is
also definitely a good idea to correct my understanding if I have
misunderstood this!)

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

quoted

quoted

And one benefit of having separate nodes per MFD cells is that we
can
easily discern the support for which cells is to be turned on.

We don't want to do DT modifications to drop some sub-device
support
out. The DT is HW description and sub-blocks are still there. We
drop
the support by KConfig. 

How would you describe the purpose of 'status = "disabled"' DT
assignment then?

Anyway, I entirely disagree here - it is perfectly proper approach
to define platform capabilities by modifying dts file alone.
This way you can easily create multiple versions of platform
configurations. It may be often impractical to enable all available
platform features, at least from business point of view. And
recompiling
dts is lightweight operation in comparison to kernel compilation.

I guess we have different backgrounds here =) For quite a long time I
was working with devices that had really long life-span. They received
multiple SW updates - but changing a DT was rare. For some of the
products DT changes were impossible after the product was out of the
factory. For some of the products DT changes were possible - but rare -
and during the update the system often booted up in a state where it
had either new SW but old DT. In SW fall-back scenarios system often
had old SW and new DT. And at times there were systems running new SW
but years old DT - especially for those systems where DT was not
updated after the product left factory...

In that environment all the DT updates were a nightmare.

Not saying that in some cases there are secret keys required for
encrypting kernel images, that may not always be at hand.

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

quoted

quoted

quoted

	pmic: pmic@4b {
		compatible = "rohm,bd71828";
		reg = <0x4b>;
		interrupt-parent = <&gpio1>;
		interrupts = <29 GPIO_ACTIVE_LOW>;
		clocks = <&osc 0>;
		#clock-cells = <0>;
		clock-output-names = "bd71828-32k-out";
		gpio-controller;
		#gpio-cells = <2>;
		ngpios = <4>;
		gpio-reserved-ranges = <0 1 2 1>;
		gpio-line-names = "EPDEN";
		rohm,dvs-vsel-gpios = <&gpio1 12 0>,
				      <&gpio1 13 0>;
		regulators {
			...
		};
		
		chg-led {
			function = LED_FUNCTION_CHARGING;
			color = LED_COLOR_ID_AMBER;
		};

		pwr-led {
			function = LED_FUNCTION_POWER;
			color = LED_COLOR_ID_GREEN;
		};

This way you would probably need to probe LED class driver twice,
instead of letting it behave in a standard way and parse child
LED
nodes.

No. Please note that probing the MFD sub-drivers is _not_ bound to
device-tree nodes. MFD sub-devices can be probed just fine even if
they
have no DT entries. When we add MFD cell for LED driver, the
corresponding LED driver is probed. No DT magic needed for this.

What the LED driver (as other sub-device drivers) is required to do
is
to obtain the pointer to parent device's DT node and find
information
which is relevant for it. Ideally, the subsystem framework can
extract
the properties which are common for whole subsystem (like color and
function in case of LEDs) and driver only parses the DT if it has
some
custom properties. Again, ideally the driver has sane defaults - or
some other 'platform data' mechanism if no DT information is found.
There is architectures which do not support DT.

LED common bindings define that each LED should be described
using child node. And we've enforced sticking to this standard
for last two years strictly.

I am not against that. If DT is used, then it is fine for me to have
properties of one LED in own node. But I don't think the DT should be
compulsory at all for cases where the LED information stays static.

LED core mechanism for LED name composition also relies on this
DT design - it expects single 'color' and 'function' properties to
be present in the passed fwnode.

I am not against this either - although I don't fully understand this
as I said above. I believe that set of well defined LED names is a good
thing. And LED APIs should indeed force the name to follow specific
format. But I don't think that the DT should be only mechanism for
bringing the function and colour. I think we should allow LED name
composition for example by specifying the colour and function in LED
class registration API in cases where fwnode is not needed.

LED class registration function registers single LED and it has been
always LED class driver's responsibility to call it for every LED
connected to the LED controller iouts.

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

quoted

In case of BD71828 LEDs my first idea was to go with only the 'sane
defaults' option as I saw no much configurability. The DT snippet
above
contains LED information as per your suggestion.

What the LED sub driver for BD71828 would now do is calling 
devm_led_classdev_register_ext with the DT information of BD71828
device. Eg, it should use the MFD dt node (because this is the real
device) and not just part of it. devm_led_classdev_register_ext
should
then extract the LED specific information. I have not checked the
implementation of devm_led_classdev_register_ext in details - but
it
should ignore non led properties and just walk through the LED
information and create the sysfs interfaces etc. for all LEDs it
finds.

This function does not work like that, as explained above.
Please first get acquainted with the way how existing LED class
drivers
approach LED registration. Because otherwise we're wasting each
others' time.

Right. I see. So each LED driver must first parse the DT information in
order to find the LED node - or each LED node must be identified by
what-ever mechanism is invoking the LED driver... I see this could be
improved in the future by adding LED framework a mechanism to identify
LED nodes. But that discussion is (probably) out of the scope of this
driver. Thanks for pointing that out.

quoted

(In my example this is the chg-led and pwr-led sub-nodes).
Furthermore,
if no LED information is found from DT I would expect
devm_led_classdev_register_ext to fail with well-defined return
value
so that the driver could do what it now does - Eg, use "sane
defaults"
to register the default class-devices for green and amber LEDs. The
default led class dev naming should of course be same format as it
would be if DT was populated with green and amber led information. 

Please go through 5.4-rc1 patches related to LED naming improvements
You can also refer to Documentation/leds/leds-class.rst,
"LED Device Naming" section for starter.

I will. The naming should be coherent - and names in my current draft
were just pulled off from my hat. Thanks.

quoted

quoted

quoted

	};

How do you see this? Or do you really wish to have this one
extra
node:

	pmic: pmic@4b {
		compatible = "rohm,bd71828";
		
reg = <0x4b>;
		interrupt-parent = <&gpio1>;
		interru
pts = <29 GPIO_ACTIVE_LOW>;
		clocks = <&osc 0>;
		
#clock-cells = <0>;
		clock-output-names = "bd71828-32k-out";
		gpio-controller;
		#gpio-cells = <2>;
	
	ngpios = <4>;
		gpio-reserved-ranges = <0 1 2 1>;
	
	gpio-line-names = "EPDEN";
		rohm,dvs-vsel-gpios =
<&gpio1 12 0>,
				      <&gpio1 13 0>;
		
regulators {
			...
		};
		
		leds-dummy {

Why leds-dummy ?

Because there is no real led controller device in any "MFD bus". It
is
just one MFD device with controls for two LEDs. 

quoted

The convention is to have led-controller@unit-address as the
parent
LED
controller node.

What is the unit address here? 0x4b is the I2C slave address and it
is
the MFD node address. There is no addressing for LED controller as
there is no separate LED controller device. There is only one
device,
the PMIC which is MFD device as it has multiple functions meld in.
One
of these functions is LED control and requires LED driver.

For MFD cell you can have just "led".

quoted

quoted

quoted

			chg-led {

s/chg-led/led0/

quoted

				function =
LED_FUNCTION_CHARGING;
				color = LED_COLOR_ID_AMBER;
			};

			pwr-led {

s/pwr-led/led1/

This is ePAPR requirement that DT node name should describe the
general class of device.

Thanks. I had some problems with these node names as I wanted to
make
them generic (led) but also to include some information what leds
they
are. A bit same idea as I see in node names like "chan1" and
"chan345"
that are used in ti-lmu bindings I checked for the example. But I
am
fine with renaming them in this example! I just don't think we
should
have this extra node as I mentioned.

I wonder what Rob and Lee will say here. I personally would
like to stick to LED common bindings and have this extra node.
We define standards for a reason after all.

I don't understand what makes you think we shouldn't stick LED common
bindings? We definitely want to have common bindings and increase
amount of bindings handled by core instead of handling the bindings in
all of the LED drivers. It was just strange to me that LED subsystem
uses this "extra node" and "extra compatible" inside MFD whereas (I
have understood that) other subsystems seem to be giving up on that.
But maybe I am mistaken on that - wouldn't be first time - let's see :)

quoted

quoted

quoted

				function = LED_FUNCTION_POWER;
				color = LED_COLOR_ID_GREEN;
			};

Common LED bindings say this is the proper way to go. However you
would need compatible to probe LED class driver in DT based way.

No. I don't. MFD will probe the LED class driver as long as the
name of
the driver matches to MFD cell name. 

If you initialize only of_compatible in struct mfd_cell element then
it
will use only that for matching. I bet I was checking that five years
ago while working on leds-max77693 driver.

Yes. It sure uses of_compatible for matching and populating the dt
node. This is different from probing though. Sub-device is probed just
fine even when there is no compatible for in DT - if the name matches.
What changes is that the of_node won't be populated and sub driver
needs to figure it out. So both approaches *work* - which is considered
as "right thing to do"(tm) needs to be figured out. I have no further
insight as to why the compatible should or should not be used for MFD
sub-devices - I was just told to avoid that in the past. But let's see
if we get Rob's or Lee's attention :)

quoted

So we only need MFD driver to be
probed based on the compatible. Rest of the sub-device drivers will
be
probed by MFD. What I am missing is MODULE_ALIAS in LED driver for
loading the module when MFD is searching for it if it is not
modprobed
via scripts or built in-kernel. I have understood this is the
standard
way with MFD nowadays - I am positive Lee will kick me if I am
wrong ;)
(I think I have bullied him that much in the past :/ )

Last sentence confirms my observation that you're strongly inclined
to contest status quo :-)

Let's just say that I have had my moments - both in good and in bad :)
I am probably not the easiest guy to work with but not the worst
either. Actually, problems tend to start when I try to be funny
:rolleyes: I should learn when to stop.

quoted

quoted

If you plan to do it otherwise then it makes no sense to have
DT nodes for LEDs.

That was my point. This is why I did not have LEDs in DT in first
place. But as I said above - as a result of this discussion I have
started thinking that maybe I could check if I can easily add
support
for providing LED information also via DT and fall back to defaults
if
no LED information is found. (to allow color change or to omit one
of
the LEDs as you suggested)

quoted

quoted

quoted

quoted

quoted

quoted

quoted

+
+	bd71828 = dev_get_drvdata(pdev->dev.parent);
+	l = devm_kzalloc(&pdev->dev, sizeof(*l),
GFP_KERNEL);
+	if (!l)
+		return -ENOMEM;
+	l->bd71828 = bd71828;
+	a = &l->amber;
+	g = &l->green;
+	a->id = ID_AMBER_LED;
+	g->id = ID_GREEN_LED;
+	a->force_mask = BD71828_MASK_LED_AMBER;
+	g->force_mask = BD71828_MASK_LED_GREEN;
+
+	a->l.name = ANAME;
+	g->l.name = GNAME;
+	a->l.brightness_set_blocking =
bd71828_led_brightness_set;
+	g->l.brightness_set_blocking =
bd71828_led_brightness_set;
+
+	ret = devm_led_classdev_register(&pdev->dev,
&g->l);
+	if (ret)
+		return ret;
+
+	return devm_led_classdev_register(&pdev->dev,
&a->l);

This way you force users to always register two LED class
devices
whereas they might need only one. Please compare how other
LED
class
drivers handle DT parsing and LED class device registration.

I am not sure if I understand correctly what you mean by using
only
one
class device. As I (hopefully) somewhere said - users can't
control
only one of these LEDs. If they decide to enable one led by SW,
then
they inevitably control also the other. Thus it is better that
user
gets control to both of the LEDs if they take the control for
one.

Or do you mean I could achieve the control for both of these
LEDs
via
only one class device?

AFAIU the LEDs, when in SW mode, can be controlled independently,
right?

Yes and no. Both of the LEDs can be forced on/off individually - as
long as one of them is forced ON. If both LEDs are tried to be
forced
OFF - then both LEDs are controlled by HW. If both are controlled
by HW
and then one is forced ON - the other is also no longer controlled
by
HW and is forced OFF.

Eg, bits 0x80 and 0x40 are conrols for these LEDs. 0x80 for one,
0x40
for the other. Setting bit means LED is on, clearing means LED is
off -
with the HW control twist... If either of the bits is set - then
both
leds are controlled by these bits (SW control). If both bits are
cleared, then LEDs are controlled by HW (likely to be off but not
for
sure).

Thank you for the explanation. So they can be represented by separate
LED class devices. Driver logic will just need to update the state of
the sibling LED if it will be affected.

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.

quoted

quoted

Because if not then there is no point in having separate LED
class
devices.

But if I get it right, then allowing for registering only one LED
class
device is entirely justifiable - think of a situation when the
iout
remains not connected on the board.

Yes. This might be unlikely - but this is the reason why I consider
adding the DT support. I just am not sure if covering this scenario
now
is worth the hassle. I tend to think we should only add the DT
support
if someone actually produces a board where this LED is not
connected.

Could you share what board you're working with?

Unfortunately I can't :( I am working for a component vendor and all
customer related information is usually strictly confidental - even in
cases where it can be guessed...