Re: [PATCH 1/2] leds: aw21024: Add support for Awinic's AW21024

[Kyle Swenson <kyle.swenson@est.tech>]

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@est.tech>
> 
> 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@est.tech>");
> > +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