Re: [PATCH 2/2] ARM: dts: aspeed: bletchley: Add second source PCA9532 LED controllers

[sashiko-bot@kernel.org]

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

New issues:
- [High] The GPIO polarity flags for the alternative PCA9532 LED controllers are incorrectly copied verbatim from the PCA9552 nodes, resulting in inverted physical LED behavior.

Pre-existing issues:
- [High] The `leds-pca9532` driver ignores I2C write errors during initialization, causing it to successfully probe even if the hardware is completely absent, introducing phantom GPIOs and sysfs collisions.
--

--- Patch [2]: [PATCH 2/2] ARM: dts: aspeed: bletchley: Add second source PCA9532 LED controllers ---
commit be0c2ca182f94bd5b992463cc968b5837bdf819b
Author: Cosmo Chou <chou.cosmo@gmail.com>

ARM: dts: aspeed: bletchley: Add second source PCA9532 LED controllers
    
This patch adds alternative PCA9532 LED controllers at address 0x66 to support
boards populated with the PCA9532 instead of the primary PCA9552 at 0x67.

> diff --git a/arch/arm/boot/dts/aspeed/aspeed-bmc-facebook-bletchley.dts b/arch/arm/boot/dts/aspeed/aspeed-bmc-facebook-bletchley.dts
> index a966be69ff27d..86e2b5aecdf94 100644
> --- a/arch/arm/boot/dts/aspeed/aspeed-bmc-facebook-bletchley.dts
> +++ b/arch/arm/boot/dts/aspeed/aspeed-bmc-facebook-bletchley.dts
[ ... ]
> @@ -113,6 +114,58 @@ led-7 {
>  		};
>  	};
>  
> +	fan_gpio_leds_alt {
> +		compatible = "gpio-leds";
> +		led-0 {
> +			label = "fan0_blue";
> +			retain-state-shutdown;
> +			default-state = "on";
> +			gpios = <&fan_leds_alt 8 GPIO_ACTIVE_HIGH>;

[Severity: High]
Does this GPIO polarity result in inverted physical LED behavior?

In drivers/leds/leds-pca955x.c, pca955x_gpio_set_value() maps val = 1 to
PCA955X_LS_LED_OFF (HI-Z). Conversely, in drivers/leds/leds-pca9532.c,
pca9532_gpio_set_value() maps val = 1 to PCA9532_ON (LOW). 

Since the PCA9532 driver maps the logical value to the opposite physical
state compared to PCA9552, should this be GPIO_ACTIVE_LOW instead?

> +		};
[ ... ]
> +	sled1_gpio_leds_alt {
> +		compatible = "gpio-leds";
> +		led-0 {
> +			label = "sled1_amber";
> +			retain-state-shutdown;
> +			default-state = "keep";
> +			gpios = <&sled1_leds_alt 0 GPIO_ACTIVE_LOW>;

[Severity: High]
Similarly, should this be GPIO_ACTIVE_HIGH? 

Since the physical circuit behavior for the PCA9532 differs from the PCA9552,
using the same polarity here might cause the LEDs to turn off when userspace
expects them to be on.

> +		};
[ ... ]
> @@ -957,6 +1214,38 @@ fan_leds: pca9552@67 {
>  		"","","","",
>  		"","","","";
>  	};
> +
> +	fan_leds_alt: pca9532@66 {
> +		compatible = "nxp,pca9532";
> +		reg = <0x66>;
> +		gpio-controller;

[Severity: High]
This isn't a bug introduced by this patch, but will this cause phantom GPIOs
and sysfs collisions on boards where the PCA9532 is not populated?

The leds-pca9532 driver ignores I2C write errors during initialization:

drivers/leds/leds-pca9532.c:pca9532_configure() {
    ...
		i2c_smbus_write_byte_data(client, PCA9532_REG_PWM(maxleds, i),
			data->pwm[i]);
		i2c_smbus_write_byte_data(client, PCA9532_REG_PSC(maxleds, i),
			data->psc[i]);
    ...
}

Because it doesn't check the return codes, it will successfully probe even if
the hardware is absent. This will register a phantom gpiochip with identical
gpio-line-names on the same I2C bus. 

Subsequent user-space GPIO reads from this phantom chip will resolve to
truncated NAK errors, yielding bogus hardware states for critical sensors like
fan presence and power-good.

> +		#gpio-cells = <2>;
> +
> +		gpio-line-names =
> +		"presence-fan0","presence-fan1",
> +		"presence-fan2","presence-fan3",
> +		"power-fan0-good","power-fan1-good",
> +		"power-fan2-good","power-fan3-good",
> +		"","","","",
> +		"","","","";
> +
[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260714120816.1504058-1-chou.cosmo@gmail.com?part=2