Re: [PATCH v3 2/2] leds: is31f132xx: Add support for is31fl3206

["Flávio Amieiro" <flavio@amieiro.net>]

Lee Jones <lee@kernel.org> writes:

> --- checkpatch.pl (1 issue) ---
> Line 172: WARNING: DT compatible string "issi,is31fl3206" appears un-documented -- check ./Documentation/devicetree/bindings/
>
> This true, or is it documented in this set?
>

It is documented in the first patch in the series[1].


>
> New for() loops should initiate the variable.
>
>   for (int i = 0; ...
>

I will make sure to initialize the variable in the loop for any new code
I add in v4. Some of this code, though, was old code that simply moved
around (and by changing the approach in v4 it might not move anymore).

Should I add a patch to the series to fix this in portions of the code I
move to another place?

If it is helpful, I can also change this in the entire file, just let me
know if this is welcome as a patch in this series or if it better to
send it in a different one.

>
> Spaces around '+' please.
>

This is the same as the for loops: I'll make sure to take this into
consideration for code added in v4.

>
> I detest pointers to functions at the device driver level.
>
> Please find another way to do this.
>

I'll find another approach for this but, since this is my first patch,
maybe it's better to align on it before sending in a new version
unnecessarily to the list.

One option that comes to mind is to use the approach taken in the driver
for the is31fl319x[2]: have a boolean flag named is_is31fl3206 in the
chipdef, which means this is a special case, and we should initialize
the LED Control Register specifically for it. This has the added benefit
of still alowing us to set the current in the future (even if it's not
supported now). Does that sound like a good approach?

There are, of course, a few other ways I could do this if that doesn't
sound good.

Also, if it is preferable to discuss over actual patches, please let me
know and I will implement this and send over a new version of this
series.


[1]: https://lore.kernel.org/linux-leds/20261013-add-is31fl3206-support-v3-0-66a762a3c12f@amieiro.net/T/#meb963e4bcc9943bde08cbe679a59f553c2184bcf
[2]: https://git.kernel.org/pub/scm/linux/kernel/git/lee/leds.git/tree/drivers/leds/leds-is31fl319x.c?h=for-leds-next#n112