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

On 2021-10-05 15:03:49, Daniel Thompson wrote:
[..]

quoted

I much prefer doing that instead of trying to wrangle enumeration
parsing around integer values that are supposed to be used as-is.  After
all this variable is already named to set the `+ 1` override currently,
and `qcom,enabled_strings` has "custom" handling as well.  I'll extend
the validation to ensure num_strings>=1 too.

Great.

quoted

In addition, and this needs some investigation on the dt-bindings side
too, it might be beneficial to make both properties mutually exclusive.
When specifying qcom,enabled_strings it makes little sense to also
provide qcom,num_strings and we want the former to take precedence.

If we are designing a "fix" for that then my view is that if both are
passed then num-strings should take precedence because it is an
explicit statement about the number of strings where enabled_strings
is implicit. In other words, if num-strings <= len(enabled_strings) then
we should do what we are told, otherwise report error.

IMO both should be identical (num-strings == len(enabled-strings)) to
avoid ambiguity, but do read on.

quoted

At that point one might ask why qcom,num_strings remains at all when
DT can use qcom,enabled_strings instead.  We will supposedly have to
keep backwards compatibility with DTs in mind so none of this can be
removed or made mutually exclusive from a driver standpoint, that all
has to be done in dt-bindings yaml to be enforced on checked-in DTs.

So... perhaps I made a make offering a Reviewed-by: to a patch
that allows len(enabled-strings) to have precedence. If anything
currently uses enabled-strings then it *will* be 4 cells long and
is relying on num-strings to ensure the right things happens ;-) .

Unfortunately Konrad (one of my team members) landed such a patch at the
beginning of this year because I failed to submit this patchset in time
while it has been sitting in my queue since 2019 after being used in a
downstream project.  This is in pmi8994 which doesn't have anything
widely used / production ready yet, so I'd prefer to fix the DT instead
and remove / fix his comment:

    /* Yes, all four strings *have to* be defined or things won't work. */

But this is mostly because, prior to this patchset, no default was set
for WLED4 so the 0'th string would get enabled num-strings (3 in
pmi8994's case) times.

Aside that there's only one more PMIC (also being worked on by
SoMainline) that sets qcom,enabled-strings: this is pm660l, pulled from
our local tree, and it actually has enabled-strings of length 2 which is
broken in its current form, exactly because of relying on this patchset.

Finally, we already discussed this inside SoMainline and the
number/enabled leds should most likely be moved out of the PMIC dtsi's
as they're probably panel, hence board or even device dependent.

We'd like that case to keep working so we must allow num-strings to have
precedence. In other words, when you add the new code, please put it at
the end of the function!

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?

PS. In -next pmi8994_wled is only enabled for sony-xperia-tone, and
pm660l_wled has yet to be enabled by anything.

- Marijn