Re: [PATCH v2 1/2] dt-bindings: power: supply: ab8500: Add AB8505 charger

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] power: supply: ab8500: driver probe fails on AB8505 due to 9-interrupt schema constraint
- [Medium] dt-bindings: power: supply: ab8500: missing io-channels variant constraints
- [Medium] dt-bindings: power: supply: ab8500: autopower_cfg missing variant restriction
--

commit 1fff99ca786343bf578c093a179ec7f5838e1bb0
Author: Linus Walleij <linusw@kernel.org>

dt-bindings: power: supply: ab8500: Add AB8505 charger

AB8500 and AB8505 expose different charger interrupt sets. Add a unique
AB8505 charger compatible with the AB8500 compatible as its fallback.
Use the variant-specific string to require the fourteen main and USB
charger interrupts on AB8500 and the nine USB charger interrupts on
AB8505.

> diff --git a/Documentation/devicetree/bindings/power/supply/stericsson,ab8500-charger.yaml b/Documentation/devicetree/bindings/power/supply/stericsson,ab8500-charger.yaml
> index 4f19744844e9c..73f5df2d7a36b 100644
> --- a/Documentation/devicetree/bindings/power/supply/stericsson,ab8500-charger.yaml
> +++ b/Documentation/devicetree/bindings/power/supply/stericsson,ab8500-charger.yaml
> @@ -12,10 +12,58 @@ maintainers:
>  
>  allOf:
>    - $ref: power-supply.yaml#
> +  - if:
> +      properties:
> +        compatible:
> +          contains:
> +            const: stericsson,ab8505-charger
> +    then:
> +      properties:

[Severity: Medium]
Should there be an io-channels variant constraint for AB8505 here?

A device tree could specify the stericsson,ab8500-charger compatible but only
provide 2 IO channels (vbus_v and usb_charger_c).

This would pass the global schema check since io-channels allows between 2 and
4, but the driver strictly requires 4 channels (including main_charger_v) for
AB8500 hardware:

drivers/power/supply/ab8500_charger.c:ab8500_charger_probe() {
	if (!is_ab8505(di->parent)) {
		di->adc_main_charger_v = devm_iio_channel_get(dev, "main_charger_v");
		if (IS_ERR(di->adc_main_charger_v)) {
            ...
}

Should the schema enforce minItems: 4 for AB8500 and maxItems: 2 for AB8505
within these new variant conditional blocks to prevent invalid configurations
from passing schema validation but failing during probe?


[Severity: Medium]
Does the autopower_cfg property need to be restricted to false in this AB8505
conditional block?

If a device tree for an AB8505 system incorrectly includes this property, the
schema allows it because it's defined globally but not restricted here.

The driver would then read it and later call
ab8500_enable_disable_sw_fallback(), writing to bit 0 of register 0x03
(AB8500_SW_CONTROL_FALLBACK).

Since the commit message explicitly states this bit is reserved on AB8505,
could writing to it cause undefined behavior?

> +        interrupts:
> +          minItems: 9
> +          maxItems: 9

[Severity: High]
Does this break the probe of the ab8500-charger driver on AB8505 hardware?

A device tree for an AB8505 system conforming to this newly introduced schema
would provide only 9 USB charger interrupts.

During driver probe, the driver unconditionally iterates over its hardcoded
ab8500_charger_irq array of 14 interrupts:

drivers/power/supply/ab8500_charger.c:ab8500_charger_probe() {
	for (i = 0; i < ARRAY_SIZE(ab8500_charger_irq); i++) {
		irq = platform_get_irq_byname(pdev, ab8500_charger_irq[i].name);
		if (irq < 0)
			return irq;
    ...
}

When it calls platform_get_irq_byname() for the missing main charger
interrupts, won't it receive an error code (e.g., -ENXIO) and immediately
return it, aborting the probe?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260918-ab8500-charger-dt-v2-0-5f17c332a2a7@kernel.org?part=1