Re: [PATCH v2 2/4] leds: lp8788: convert to dev_err_probe()

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

New issues:
- [Low] The commit message description is not capitalized, violating the LED subsystem guidelines.

Pre-existing issues:
- [High] The driver directly mutates a global static configuration structure during initialization, causing cross-contamination between devices or across probes.
--

--- Patch [2]: [PATCH v2 2/4] leds: lp8788: convert to dev_err_probe() ---
commit 757b3eb36736561a1f3fc533b95dcae04a897dbc
Author: Arunachalam <[email protected]>

leds: lp8788: convert to dev_err_probe()

This patch replaces open-coded dev_err() and return sequences with
dev_err_probe() in the lp8788 driver. It improves error handling and properly
manages deferred probing.

> leds: lp8788: convert to dev_err_probe()

[Severity: Low]
This isn't a bug, but should the word "convert" be capitalized to match
the LED subsystem commit naming conventions?

> diff --git a/drivers/leds/leds-lp8788.c b/drivers/leds/leds-lp8788.c
> index 9b9525ccca157..259ac50e8498f 100644
> --- a/drivers/leds/leds-lp8788.c
> +++ b/drivers/leds/leds-lp8788.c
> @@ -143,16 +143,12 @@ static int lp8788_led_probe(struct platform_device *pdev)
>  	mutex_init(&led->lock);
>  
>  	ret = lp8788_led_init_device(led, led_pdata);

[Severity: High]
This is a pre-existing issue, but does modifying the global configuration 
structure cause cross-contamination between devices?

Looking at lp8788_led_init_device() in drivers/leds/leds-lp8788.c:

static int lp8788_led_init_device(struct lp8788_led *led,
                                  struct lp8788_led_platform_data *pdata)
{
        struct lp8788_led_config *cfg = &default_led_config;
        ...
        if (pdata) {
                cfg->scale = pdata->scale;
                cfg->num = pdata->num;
                cfg->iout = pdata->iout_code;
        }

If there are multiple instances, modifying cfg (which points to the
global default_led_config) might cause subsequent instances without platform
data to inherit the mutated configuration rather than the defaults. Could
we make a local copy of the configuration instead?

> -	if (ret) {
> -		dev_err(dev, "led init device err: %d\n", ret);
> -		return ret;
> -	}
> +	if (ret)
> +		return dev_err_probe(dev, ret, "led init device err\n");

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=2