linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Dan Murphy <dmurphy@ti.com>
To: Jacek Anaszewski <jacek.anaszewski@gmail.com>,
	Pavel Machek <pavel@ucw.cz>
Cc: <robh+dt@kernel.org>, <devicetree@vger.kernel.org>,
	<linux-kernel@vger.kernel.org>, <linux-leds@vger.kernel.org>
Subject: Re: [PATCH v2 1/2] dt: bindings: lm3697: Add bindings for lm3697 driver
Date: Thu, 9 Aug 2018 10:01:02 -0500	[thread overview]
Message-ID: <0852bdeb-a16e-a635-4f10-34be283f9073@ti.com> (raw)
In-Reply-To: <04c37241-78c6-529f-4c07-db6f16b4f92a@gmail.com>

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

  reply	other threads:[~2018-08-09 15:01 UTC|newest]

Thread overview: 24+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-08-07 16:04 [PATCH v2 1/2] dt: bindings: lm3697: Add bindings for lm3697 driver Dan Murphy
2018-08-07 16:04 ` [PATCH v2 2/2] leds: lm3697: Introduce the " Dan Murphy
2018-08-08 19:59   ` Pavel Machek
2018-08-14 13:54     ` Dan Murphy
2018-08-08  7:56 ` [PATCH v2 1/2] dt: bindings: lm3697: Add bindings for " Michal Vokáč
2018-08-08  9:52   ` Jacek Anaszewski
2018-08-08 19:59 ` Pavel Machek
2018-08-08 20:42   ` Dan Murphy
2018-08-08 21:02     ` Pavel Machek
2018-08-08 21:04       ` Dan Murphy
2018-08-08 21:09         ` Pavel Machek
2018-08-08 21:41           ` Dan Murphy
2018-08-08 21:45             ` Pavel Machek
2018-08-08 21:50               ` Dan Murphy
2018-08-08 21:58                 ` Pavel Machek
2018-08-08 21:09         ` Jacek Anaszewski
2018-08-08 21:45           ` Dan Murphy
2018-08-09 12:09             ` Jacek Anaszewski
2018-08-09 13:24               ` Pavel Machek
2018-08-09 13:30               ` Dan Murphy
2018-08-09 14:48                 ` Jacek Anaszewski
2018-08-09 15:01                   ` Dan Murphy [this message]
2018-08-09 21:59                 ` Pavel Machek
2018-08-08 22:00 ` Pavel Machek

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=0852bdeb-a16e-a635-4f10-34be283f9073@ti.com \
    --to=dmurphy@ti.com \
    --cc=devicetree@vger.kernel.org \
    --cc=jacek.anaszewski@gmail.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-leds@vger.kernel.org \
    --cc=pavel@ucw.cz \
    --cc=robh+dt@kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).