Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Tue, 6 Oct 2020 16:41:01 +0200
From:   Marek Behun <kabel@blackhole.sk>
To:     Dan Murphy <dmurphy@ti.com>
Cc:     <ultracoolguy@tutanota.com>, Pavel <pavel@ucw.cz>,
        Linux Leds <linux-leds@vger.kernel.org>,
        Linux Kernel <linux-kernel@vger.kernel.org>,
        Rob Herring <robh+dt@kernel.org>, devicetree@vger.kernel.org
Subject: Re: [PATCH] leds: lm3697: Fix out-of-bound access
Message-ID: <20201006164101.2c3fa0d7@blackhole.sk>
In-Reply-To: <144aa75a-4369-cd81-d7dc-2354a9afd7c5@ti.com>
References: <20201005141334.36d9441a@blackhole.sk>
        <MIt2NiS--3-2@tutanota.com>
        <3c5fce56-8604-a7d5-1017-8a075f67061e@ti.com>
        <MItBqjy--3-2@tutanota.com>
        <966c3f39-1310-dd60-6f33-0d9464ed2ff1@ti.com>
        <MItOR9Z--3-2@tutanota.com>
        <20201005164808.slrtmsvmw4pvwppm@falbala.internal.home.lespocky.de>
        <MItjEho--3-2@tutanota.com>
        <20201005173227.GA6431@duo.ucw.cz>
        <20201006093356.6d25b280@blackhole.sk>
        <MIxm3uX--3-2@tutanota.com>
        <144aa75a-4369-cd81-d7dc-2354a9afd7c5@ti.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7bit
Precedence: bulk

Adding Rob to Cc, Rob, could we have your opinion on this? Mine is below.

On Tue, 6 Oct 2020 07:21:14 -0500
Dan Murphy <dmurphy@ti.com> wrote:

> >> By the way I just realized that the DT binding in this driver seems
> >> incorrect to me.
> >>
> >> The controller logically supports 3 LED strings, each having
> >> configurable control bank.  
> 
> There are two control banks. You can connect the HVLED outputs to either 
> control bank A or B there is no individual control of the LED strings.
> 
> 
> >> But the DT binding supports 2 DT nodes, one for each control bank
> >> (identified by the `reg` property) and then `led-sources` says which
> >> string should be controlled by given bank.
> >>
> >> But taking in mind that DT should describe how devices are connected to
> >> each other, I think the child nodes in the binding should instead
> >> describe the 3 supported LED strings...  
> 
> The outputs in this case are virtual outputs which are the banks (A and B).
> 
> Since the device is bank controlled the actual current sinks are not 
> defined thus making the the banks the actual outputs.
> 
> This is why the 'reg' property defines the control bank either A or B 
> and the led-sources indicates the strings associated with the control bank.
> 
> Dan
> 

Dan, I looked at the datasheet, I understand this.

Nonetheless, device tree should describe how devices are connected to
each other. The chip has 3 pins for 3 LED strings.

If this controller should be able to support 3 LED strings via 3
outputs, the device tree binding nodes should, in my opinion, describe
each pin connected string. The nodes should maybe even be called
'led-string@N' where N is from [0, 1, 2].

The fact that the device is bank controlled and there are only two
banks (and it is configurable by which bank each LED string is
controlled) is more relevant to the driver, not as much to device tree
binding.

But yes, I do realize that if we had 3 child nodes, and the driver
created 3 LEDs, then changing brithrness on one of the 3 LEDs would
change brightness on at least another one, which we do not want.

Maybe this driver could parse these 3 `led-string` nodes, each having
defined bank via `led-sources`, and then register LED classdevs for
each bank that is mentioned. This way the device tree would be more
correct, IMO, and the driver would not have the problem mentioned in
the paragraph above.

Marek