Re: [PATCH net-next v5 2/7] leds: is32fl3207: Add controller driver

[Jakub Kicinski <kuba@kernel.org>]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
leds: is32fl3207: Add controller driver

This adds an I2C driver for the Lumissil IS32FL3207 18-channel LED
controller, exposing single and multicolor LEDs via the LED class.
Per-output current limits are programmed into the scaling registers based
on the RISET value and each LED's led-max-microamp property.

> diff --git a/drivers/leds/rgb/leds-is32fl3207.c b/drivers/leds/rgb/leds-is32fl3207.c
> new file mode 100644
> index 0000000000000..6a46f97ba50a5
> --- /dev/null
> +++ b/drivers/leds/rgb/leds-is32fl3207.c
> @@ -0,0 +1,736 @@

[ ... ]

> +static int is32fl3207_configure_channel(struct is32fl3207 *chip,
> +					struct fwnode_handle *fwnode,
> +					unsigned int *channel)
> +{

[ ... ]

> +	ret = fwnode_property_read_u32(fwnode, "led-max-microamp",
> +				       &max_microamp);
> +	if (ret)
> +		return dev_err_probe(chip->dev, ret,
> +				     "failed to read current limit for channel %u\n",
> +				     reg);
> +
> +	if (!max_microamp || max_microamp > chip->output_max_microamp)
> +		return dev_err_probe(chip->dev, -EINVAL,
> +				     "invalid current limit %u uA for channel %u\n",
> +				     max_microamp, reg);
> +
> +	/* GCC is fixed at 0xff, so use each output's scaling register. */
> +	scaling = div_u64((u64)max_microamp * 256 * 256,
> +			  (u64)chip->output_max_microamp *
> +			  IS32FL3207_GLOBAL_CURRENT_MAX);
> +	if (!scaling)
> +		return dev_err_probe(chip->dev, -EINVAL,
> +				     "current limit %u uA is below channel %u resolution\n",
> +				     max_microamp, reg);

[Severity: Medium]
Can this reject device trees that are valid according to the binding?

Documentation/devicetree/bindings/leds/issi,is32fl3207.yaml declares
led-max-microamp with minimum: 1, both for the top-level led@ nodes and for
the multi-led component nodes:

      led-max-microamp:
        minimum: 1
        maximum: 38250
        description: |
          Board-safe peak current for this output. The value must not exceed
          the full-scale current derived from issi,riset-ohms.

In is32fl3207_configure_channel() the scaling quotient truncates to zero for
small currents, and the !scaling case aborts probe with -EINVAL via
is32fl3207_probe() -> is32fl3207_register_led() ->
is32fl3207_register_single()/is32fl3207_register_multicolor() ->
is32fl3207_configure_channel().

With the smallest RISET the binding permits, 2000 ohms,
output_max_microamp is 76500000 / 2000 = 38250, so scaling stays zero for
any led-max-microamp below 149 uA (38250 * 255 / 65536 = 148.8). With the
RISET used in the binding example, 4700 ohms, output_max_microamp is 16276
and the cut-off is 64 uA.

So a device tree that passes dt_binding_check with, say,
led-max-microamp = <50> fails to bind, and the actual lower bound is
board-dependent and not documented anywhere.

Should the binding describe the RISET-dependent lower bound, or should the
driver clamp sub-resolution values to a scaling of 1 instead of failing
probe?

Cross-instance finding from sashiko-gemini (abacfa25c43c61e8a044d405172e276c795bb984d2cf45e93b49076c16419dfc):
[Severity: Medium]
`is32fl3207_clear_retained_scaling` fails to latch the zeroed scaling registers, leading to a brief LED flash during hardware initialization.