On 08/09/2018 09:48 AM, Jacek Anaszewski wrote:
> Dan,
>
> On 08/09/2018 03:30 PM, Dan Murphy wrote:
>> Jacek and Pavel
>>
>> On 08/09/2018 07:09 AM, Jacek Anaszewski wrote:
>>> Dan,
>>>
>>> On 08/08/2018 11:45 PM, Dan Murphy wrote:
>>>> Jacek
>>>>
>>>> On 08/08/2018 04:09 PM, Jacek Anaszewski wrote:
>>>>> Hi Dan,
>>>>>
>>>>> On 08/08/2018 11:04 PM, Dan Murphy wrote:
>>>>>> On 08/08/2018 04:02 PM, Pavel Machek wrote:
>>>>>>> Hi!
>>>>>>>
>>>>>>>>>> + - #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.
>
> It is better do add some complexity to the driver than to the
> user configurable settings like DT. Besides - you will only need to
> check if given led-source is already taken by another node.
Yes I know that the driver can check the string but if the same string is
declared by another child then
the driver must exit with -EINVAL. Again a lot of code for little pay off.
I believe we should keep drivers as simple as possible.
I will add the changes.
>
>> 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.
>
> Some description will be needed for sure, but I don't expect it
> to be overwhelmingly lengthy.
>
>> 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.
>
> Your control-bank-cfg seemed like having much room for improvement,
> and it would for sure raise questions on why it was implemented that
> way. Documenting all available combinations of the configuration is
> seldom the best solution. It often obscures the issue.
Unfortunately in either case this high level of documentation will need to be
done.
I believe both solutions will raise questions and concerns.
There does not seem to be a good way to describe this device.
Both solutions are wrought with issues and concerns.
But like I said I will re-write the code with the above suggestion.
>
>> 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.
>
> In your bindings device configuration is scattered among global
> control-bank-cfg property and child node's reg property.
> In my proposal each child node contains all the needed configuration,
> also in the form of two properties - led-sources and reg. IMHO having
> all the LED class device related configuration in one place simplifies
> the analysis.
>
Dan
--
------------------
Dan Murphy
