Jacek and Pavel

On 08/09/2018 07:09 AM, Jacek Anaszewski wrote:

Dan,

On 08/08/2018 11:45 PM, Dan Murphy wrote:

quoted

Jacek

On 08/08/2018 04:09 PM, Jacek Anaszewski wrote:

quoted

Hi Dan,

On 08/08/2018 11:04 PM, Dan Murphy wrote:

quoted

On 08/08/2018 04:02 PM, Pavel Machek wrote:

quoted

Hi!

quoted

quoted

quoted

+	- #size-cells : 0
+	- control-bank-cfg - : Indicates which sink is connected to which control bank
+		0 - All HVLED outputs are controlled by bank A
+		1 - HVLED1 is controlled bank B, HVLED2/3 are controlled by bank A
+		2 - HVLED2 is controlled bank B, HVLED1/3 are controlled by bank A
+		3 - HVLED1/2 are controlled by bank B, HVLED3 is controlled by bank A
+		4 - HVLED3 is controlled by bank B, HVLED1/2 are controlled by bank A
+		5 - HVLED1/3 is controlled by bank B, HVLED2 is controlled by bank A
+		6 - (default) HVLED1 is controlled by bank A, HVLED2/3 are controlled by bank B
+		7 - All HVLED outputs are controlled by bank B

This is quite long way to describe a bitmask, no? Could we make
it so that control-bank-cfg is not needed?

The problem we have here is there is a potential to control
3 different LED string but only 2 sinks.  So control bank A can control 2 LED strings and control
bank b can control 1 LED string.  

Can we forget about the LED strings, and just expose the sinks as
Linux LED devices?

2 sinks 3 LED strings.  How do you know which LED string is which and what bank it belongs
to when setting the brightness.  Each Bank has a separate register for brightness control.

Just a blind shot, without going into details - could you please check
if led-sources property documented in the common LED bindings couldn't
help here?

I could change the name to led-sources.  But this part does not really follow the 1 output to a
1 LED string topology.

led-sources was designed for describing the topology where one LED can
be connected to more then one output, see bindings of
max77693-led (in Documentation/devicetree/bindings/mfd/max77693.txt).

Here the topology is a bit different - more than one LED (string) can be
connected to a single bank, but this is accomplished inside the chip.
Logically LEDs configured that way can be treated as a single LED
(string) connected to two outputs, and what follows they should be
described by a single DT child node.

led-sources will fit very well for this purpose. You could do
the following mapping:

0 - HVLED1
1 - HVLED2
2 - HVLED3

Then, in the child DT nodes you would use these identifiers to describe
the topology:

Following node would describe strings connected to the outputs
HVLED1 and HVLED2 controlled by bank A.

led@0 {
	reg = <0>;
	led-sources = <0>. <1>;
	label = "white:first_backlight_cluster";
	linux,default-trigger = "backlight";
};


IOW I agree with Pavel, but I propose to use already documented common
DT LED property.

I agree to use the led-sources but I still believe this approach may be confusing to other sw devs
and will lead to configuration issues by users.

This implementation requires the sw dev to know which strings are controlled by which bank.
And this method may produce a misconfiguration like something below where HVLED2 is declared in
both bank A and bank B

led@0 {
	reg = <0>;
	led-sources = <0>. <1>;
	label = "white:first_backlight_cluster";
	linux,default-trigger = "backlight";
};

led@1 {
	reg = <1>;
	led-sources = <1>. <2>;
	label = "white:keypad_cluster";
	linux,default-trigger = "backlight";
};

The driver will need to be intelligent and declare a miss configuration on the above.
Not saying this cannot be done but I am not sure why we want to add all of these extra LoC and intelligence
in the kernel driver.
The driver cannot make assumptions on the intention.  And the device tree documentation will need to
pretty much need a lengthy explanation on how to configure the child nodes.

The implementation I suggested removes that ambiguity.  It is a simple integer that is written to the device
as part of the device configuration, which the config is a setting for the device.

The child nodes denote which bank the exposed LED node will control.  Removing any need
for the sw developers new or old to know the specific device configurations.


Dan
-- 
------------------
Dan Murphy