Re: [PATCH v6 2/3] leds: aw2013: Add AW2027 support

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential
issues to consider:
- [High] leds: aw2013: unpowered I2C access in aw2013_probe_dt() via
  aw2027_set_global_imax()
- [Low] leds: aw2013: pass-by-value of struct aw2013_led in
  aw2027_set_local_imax()
--

commit a83734f3f7807e1e9702a5495a99e5cba9b2c327
Author: Paul Sajna <sajattack@postmarketos.org>

leds: aw2013: Add AW2027 support

This commit adds support for the Awinic AW2027 3-channel LED controller to
the aw2013 driver, introducing chip definition structures and configuring
both global and per-channel maximum current registers.

> diff --git a/drivers/leds/leds-aw2013.c b/drivers/leds/leds-aw2013.c
> index cf20e94920fd6..5ff9887086a61 100644
> --- a/drivers/leds/leds-aw2013.c
> +++ b/drivers/leds/leds-aw2013.c
[ ... ]
> @@ -60,19 +72,99 @@ struct aw2013_led {
[ ... ]
> +static int aw2027_set_local_imax(struct aw2013 *chip, struct aw2013_led led, u32 imax)
> +{
> +	int ret;
> +
> +	ret = regmap_update_bits(chip->regmap,
> +				 AW2013_LCFG(led.num),
> +				 AW2027_LOCAL_IMAX_MASK,
> +				 imax);
> +
> +	return ret;
> +}

[Severity: Low]
Does aw2027_set_local_imax() need to take struct aw2013_led by value?
struct aw2013_led embeds struct led_classdev, so each call copies the entire
structure onto the stack just to read led.num, while also passing led.imax
as a separate argument.  Passing a const struct aw2013_led *led pointer (or
unifying the AW2013_LCFG update using a mask from struct aw2013_chipdef)
would avoid the stack copy and duplicate loops.

[ ... ]

> @@ -271,6 +388,16 @@ static int aw2013_probe_dt(struct aw2013 *chip)
>  	if (!count || count > AW2013_MAX_LEDS)
>  		return -EINVAL;
>  
> +	if (chip->cdef->gcr2_imax) {
> +		mutex_lock(&chip->mutex);
> +		ret = aw2027_set_global_imax(chip);
> +		if (ret) {
> +			mutex_unlock(&chip->mutex);
> +			return ret;
> +		}
> +		mutex_unlock(&chip->mutex);
> +	}

[Severity: High]
Does aw2027_set_global_imax() perform an I2C register access here while the
chip is unpowered?

aw2013_probe() resets the chip and disables the vcc and vio regulators via
regulator_bulk_disable() right before calling aw2013_probe_dt(), with
chip->enabled still false:

drivers/leds/leds-aw2013.c:aw2013_probe() {
    ...
	ret = regulator_bulk_disable(ARRAY_SIZE(chip->regulators),
				     chip->regulators);
	if (ret) {
		dev_err(&client->dev,
			"Failed to disable regulators: %d\n", ret);
		goto error;
	}

	mutex_unlock(&chip->mutex);

	ret = devm_add_action(&client->dev, aw2013_chip_disable_action, chip);
	if (ret)
		return ret;

	return aw2013_probe_dt(chip);
    ...
}

Because aw2027_regmap_config has no register cache, calling
aw2027_set_global_imax() at the start of aw2013_probe_dt() issues an I2C
transaction to AW20XX_GCR2 while vcc and vio are disabled, causing probe to
fail when those supplies are gated.  Since aw2013_chip_init() already calls
aw2027_set_global_imax() once aw2013_chip_enable() powers on the chip, can
this block in aw2013_probe_dt() be dropped?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261112-aw2013-aw20xx-rename-v6-0-251eca340abc@postmarketos.org?part=2