On Tue, May 17, 2022 at 11:11:37AM +0200, Krzysztof Kozlowski wrote:
> On 13/05/2022 21:04, Kyle Swenson wrote:
> > The Awinic AW21024 LED controller is a 24-channel RGB LED controller.
> > Each LED on the controller can be controlled individually or grouped
> > with other LEDs on the controller to form a multi-color LED. Arbitrary
> > combinations of individual and grouped LED control should be possible.
> >
> > Signed-off-by: Kyle Swenson <kyle.swenson@xxxxxxxx>
>
> Thank you for your patch. There is something to discuss/improve.
>
> > +
> > +static const struct i2c_device_id aw21024_id[] = {
> > + { "aw21024", 0 }, /* 24 Channel */
> > + { }
> > +};
> > +MODULE_DEVICE_TABLE(i2c, aw21024_id);
> > +
> > +static const struct of_device_id of_aw21024_leds_match[] = {
> > + { .compatible = "awinic,aw21024", },
> > + {},
> > +};
> > +MODULE_DEVICE_TABLE(of, of_aw21024_leds_match);
> > +
> > +static struct i2c_driver aw21024_driver = {
> > + .driver = {
> > + .name = "aw21024",
> > + .of_match_table = of_match_ptr(of_aw21024_leds_match),
>
> of_match_ptr causes this being unused. kbuild robot probably pointed
> this out... if not - of_match_ptr goes with maybe_unused. You need both
> or none, depending on intended usage.
>
Ah, yes, the kbuild robot did point this out to me, and I had planned on
fixing by adding 'depends on OF' to the Kconfig. Perhaps that isn't
correct or complete (or even relevant)?
I'll do some investigating and determine if I need to use of_match_ptr
or not and I'll fix it either by removing it or adding maybe_unused in
the next version.
> > + },
> > + .probe_new = aw21024_probe,
> > + .remove = aw21024_remove,
> > + .id_table = aw21024_id,
>
> Why other places are indented but this not?
Sorry, it should be. My editor was configured wrong and this now looks
bad. There are a few other places in the driver that also now look bad
and I'll fix those before submitting v2.
>
>
> > +};
> > +module_i2c_driver(aw21024_driver);
> > +
> > +MODULE_AUTHOR("Kyle Swenson <kyle.swenson@xxxxxxxx>");
> > +MODULE_DESCRIPTION("Awinic AW21024 LED driver");
> > +MODULE_LICENSE("GPL");
>
>
> Best regards,
> Krzysztof
Thanks so much for taking a look at this, I really appreciate your time
and patience.
Thanks,
Kyle
