Re: [PATCH v4 2/3] leds: Add LED1202 I2C driver

[Michal Vokáč]

On 02. 11. 24 12:16, Vicentiu Galanopulo wrote:
> > Alphabetical.
> Done
> > > +#define ST1202_BUF_SIZE                    16
> >
> > This appears to be unused.
> Done
> > Please make sure all of these defines are used or removed.
> >
> >
> > I'm usually all for defines, but this one is a bit over the top.
> Removed
> > Why have you broken the line here and not 2 lines down?
> Checkpatch was complaining
>   
> > Do this during declaration.
> Moved it inside for_each_available_child_of_node_scoped
>
> > Lee Jones [李琼斯]
> Thank you for the review. I think corrected everything.

Hi Vicentiu,

Once a while I browse through the patches in various mailing lists to keep myself informed.
So I came across your patch set pretty randomly.

I have few tips for you to make your life easier before you get to some serious troubles
with the maintainers ;)

1. Always send all the patches in the series to the same recipients list.

That is, do not send dt-bindings to just Rob, Krzysztof etc. and LED driver
patches to Lee et al. We all need to see the whole thing.

If you run the scripts/get_maintainer.pl script on the series, you get a complete list.
This is what Krzysztof requested you to do in his comments to v3.

2. Use git format-patch and git send-email tools to submit patches.

If you use these tools you will avoid issues with wrong threading of the messages.

3. The following text should not be here.

You are supposed to just reply in-place to the review messages to acknowledge
that you read the comments and you understand what the reviewers want to
change. Then you send a next version of the series as a new message to all
the recipients. Definitely not as a in-reply-to to the previous version.

> [PATCH v5 2/3] leds: Add LED1202 I2C driver
>
> The output current can be adjusted separately for each channel by 8-bit
> analog (current sink input) and 12-bit digital (PWM) dimming control. The
> LED1202 implements 12 low-side current generators with independent dimming
> control. Internal volatile memory allows the user to store up to 8 different
> patterns, each pattern is a particular output configuration in terms of PWM
> duty-cycle (on 4096 steps). Analog dimming (on 256 steps) is per channel but
> common to all patterns. Each device tree LED node will have a corresponding
> entry in /sys/class/leds with the label name. The brightness property
> corresponds to the per channel analog dimming, while the patterns[1-8] to the
> PWM dimming control.
>
> Signed-off-by: Vicentiu Galanopulo <vicentiu.galanopulo@xxxxxxxxxxxxxxxxx>
> ---
> Changes in v5:
>    - remove unused macros
>    - switch to using devm_led_classdev_register_ext (struct st1202_led update)
>    - add prescalar_to_milliseconds (convert [22..5660]ms to [0..255] reg value)
>    - remove register range check in dt_init (range protected by yaml)
>    - address all review comments in v4
> Changes in v4:
>    - Remove attributes/extended attributes implementation
>    - Use /sys/class/leds/<led>/hw_pattern (Pavel suggestion)
>    - Implement review findings of Christophe JAILLET
> Changes in v3:
>    - Rename all ll1202 to st1202, including driver file name
>    - Convert all magic numbers to defines
>    - Refactor the show/store callbacks as per Lee's and Thomas's review
>    - Remove ll1202_get_channel and use dev_ext_attributes instead
>    - Log all error values for all the functions
>    - Use sysfs_emit for show callbacks
> Changes in v2:
>    - Fix build error for device_attribute modes

I hope you will get this sorted, I know the beginnings are difficult.

Best regards,
Michal