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

[Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>]

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.

> +	},
> +	.probe_new		= aw21024_probe,
> +	.remove		= aw21024_remove,
> +	.id_table = aw21024_id,

Why other places are indented but this not?


> +};
> +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