Re: [PATCH 05/10] backlight: qcom-wled: Fix off-by-one maximum with default num_strings

On 2021-10-06 15:44:44, Daniel Thompson wrote:

On Tue, Oct 05, 2021 at 07:34:00PM +0200, Marijn Suijten wrote:

quoted

On 2021-10-05 17:24:53, Daniel Thompson wrote:

quoted

On Tue, Oct 05, 2021 at 05:23:26PM +0200, Marijn Suijten wrote:

quoted

Since there don't seem to be any substantial platforms/PMICs using this
functionality in a working manner, can I talk you into agreeing with
fixing the DT instead?

I've no objections to seeing the DT updated. However I don't really see
what benefit we get from breaking existing DTs in order to do so.

"Cleaning up annoying legacy" is seldom a good reason to break existing
DTs since, if we could break DTs whenever we choose, there would never
be any annoying legacy to worry about. When conflicting properties
result in uninterpretable DTs then a break may be justified but that is
not the case here.

As mentioned in my message and repeated by Konrad, the only "existing
DT" that could possibly be broken is a platform that's brought up by us
(SoMainline) and we're more than happy to improve the driver and leave
legacy DT behind us, unless there's more DT in circulation that hasn't
landed in Linux mainline but should be taken into account?

Devicetrees are supposed to be the domain of firmware (e.g. not part of
the kernel).

I'm therefore reluctant to adopt an "it only exists if it is upstream"
approach for documented DT bindings. Doubly so when it is our bugs that
causes DTs to be written in a manner which we then retrospectively
declare to be wrong.

I'm aware that DT is considered firmware and is ""intended"" to be
shipped separately (and probably only once out of the factory) but it
seems so far there's an advantage in updating DT in parallel with the
kernel.  However this is the first time hearing that having dt-bindings
documentation available contributes to considering the DT contract
(more) stable.  Either way I'd expect these bindings to have been fixed
much sooner if it was really actively used.

quoted

Anyway the plan is to leave qcom,num-strings in place so that the
default enabled_strings list in this driver actually serves a purpose.
Then, if num-strings and enabled-strings is provided the former has
precedence (assuming it doesn't exceed the size of the latter) but
we'll print a warning about this (now unnecessary) ambiguity, and if
possible at all - haven't found an example yet - make the properties
mutually exclusive in dt-bindings.

Disallowing both cases would only simplify the code in the end but we
can spend a few lines to support the desired legacy.

Yes, warning is OK for me.

Great, sending v2 shortly.

- Marijn