Re: [PATCH 3/7] leds: pca995x: add support for reset GPIO

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [Low] The commit message description violates the LED subsystem capitalization convention.
- [Low] Using usleep_range() for a microsecond delay under 10us.
- [Low] Use of the legacy <linux/gpio.h> header instead of <linux/gpio/consumer.h>.
- [Low] Missing #include <linux/delay.h> for usleep_range().
--

commit 73fa0526608a4d478003bcf92f25fa0aa33eae7c
Author: Nora Schiffer <nora.schiffer@ew.tq-group.com>

leds: pca995x: add support for reset GPIO

This commit adds support for resetting the PCA995x LED driver during
probe by utilizing a reset GPIO pin if provided in the Device Tree.

> leds: pca995x: add support for reset GPIO

[Severity: Low]
This isn't a bug, but does this violate the LED subsystem commit message
capitalization convention? The guidelines require capitalizing the
description after the subsystem prefix, so "add" would ideally be "Add".

> diff --git a/drivers/leds/leds-pca995x.c b/drivers/leds/leds-pca995x.c
> index fee6216cd1bd0..8eb9847b9709f 100644
> --- a/drivers/leds/leds-pca995x.c
> +++ b/drivers/leds/leds-pca995x.c
> @@ -8,6 +8,7 @@
>   * Copyright 2023 Marek Vasut
>   */
>  
> +#include <linux/gpio.h>

[Severity: Low]
Does this code use the legacy <linux/gpio.h> header instead of
<linux/gpio/consumer.h>? 

The kernel typically forbids including <linux/gpio.h> in consumer drivers.
Since this driver only uses descriptor-based APIs like
devm_gpiod_get_optional() and gpiod_set_value_cansleep(), relying on
<linux/gpio.h> can cause implicit declaration build errors if
CONFIG_GPIOLIB is disabled.

[ ... ]

> @@ -130,6 +132,16 @@ static int pca995x_probe(struct i2c_client *client)
>  	if (!dev_fwnode(dev))
>  		return -ENODEV;
>  
> +	reset_gpio = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_HIGH);
> +	if (IS_ERR(reset_gpio))
> +		return dev_err_probe(dev, PTR_ERR(reset_gpio),
> +				     "failed to request reset GPIO\n");
> +	if (reset_gpio) {
> +		usleep_range(3, 4);

[Severity: Low]
Is usleep_range() appropriate for a microsecond delay under 10us here?

According to the kernel timers-howto, usleep_range() is designed for
delays greater than 10us due to timer setup and context switching overhead.
For delays as short as 3-4us, udelay() is generally recommended to avoid
unnecessary performance inefficiency.

[Severity: Low]
Is there a missing #include <linux/delay.h> for this usleep_range() call?

Relying on implicit includes from other headers (like <linux/i2c.h>) is
fragile and could cause build breakages on certain architectures or
configurations.

> +		gpiod_set_value_cansleep(reset_gpio, 0);
> +		usleep_range(1500, 1600);
> +	}
> +
>  	chip = devm_kzalloc(dev, sizeof(*chip), GFP_KERNEL);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/6d3600f99c77c31166f0b6a5ccfb7f518b53c4c9.1788350618.git.nora.schiffer@ew.tq-group.com?part=3