Re: [PATCH v2 5/8] Add Advantech EIO Backlight driver

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [High] Partial overwrite of multi-byte integers via 1-byte pointer casts corrupts state logic on different architectures.
- [High] Global module parameters are improperly mutated during the device probe loop, causing cross-device state contamination.
- [High] The probe function arbitrarily fails if the last evaluated backlight is unavailable, tearing down successfully initialized devices.
- [High] The backlight enable/disable logic and state initialization are fundamentally inverted, causing the screen to turn on when blanked and turn off when activated.
- [Medium] A tautological bounds check unconditionally overwrites the hardware's internal default for the enable-invert pin.
--

--- Patch [5]: [PATCH v2 5/8] Add Advantech EIO Backlight driver ---
commit 7087ac6577d7b4e152b12ed2ee29e0940be894fd
Author: Ramiro Oliveira <ramiro.oliveira@advantech.com>

Add Advantech EIO Backlight driver

This commit introduces a new backlight driver for the Advantech EIO Embedded
controller. It supports up to two backlight devices through PMC read and
write operations. Module parameters are also provided for setting PWM
frequency, polarity, and enable pin polarity.

> diff --git a/drivers/video/backlight/eio_bl.c b/drivers/video/backlight/eio_bl.c
> new file mode 100644
> index 0000000000000..c2826400de45b
> --- /dev/null
> +++ b/drivers/video/backlight/eio_bl.c

[ ... ]

> +static int pmc_read(struct device *mfd, u8 ctrl, u8 dev_id, void *data)
> +{
> +	struct pmc_op op = {
> +		.cmd       = PMC_BL_READ,
> +		.control   = ctrl,
> +		.device_id = dev_id,
> +		.payload   = (u8 *)data,
> +		.size      = (ctrl == BL_CTRL_FREQ) ? 4 : 1,
> +	};
> +
> +	return eio_core_pmc_operation(mfd, &op);
> +}

[Severity: High]
Does this cast of the data pointer to a single byte payload cause issues
when reading into 32-bit integer variables? Since variables like bri_invert
and bl_power_invert are initialized to -1 (0xFFFFFFFF), writing only a single
byte to them via this cast will leave the upper bytes intact, potentially
resulting in corrupted values like -256 on little-endian systems.

> +static int bl_update_status(struct backlight_device *bl)
> +{
> +	struct eio_bl_dev *eio_bl = bl_get_data(bl);
> +	u8 duty = clamp_val(backlight_get_brightness(bl), 0, THERMAL_MAX);
> +	u8 sw = backlight_is_blank(bl);
> +	int ret;
> +
> +	/* Setup PWM duty */
> +	ret = pmc_write(eio_bl->mfd, BL_CTRL_DUTY, eio_bl->id, &duty);
> +	if (ret)
> +		return ret;
> +
> +	/* Setup backlight enable pin */
> +	return pmc_write(eio_bl->mfd, BL_CTRL_ENABLE, eio_bl->id, &sw);
> +}

[Severity: High]
Is the logic here backwards for enabling the backlight? The function
backlight_is_blank() returns true (1) when the display should be blanked,
but passing this directly to BL_CTRL_ENABLE would turn the screen on
(since BL_ENABLE_ON is 0x01).

[ ... ]

> +static int bl_init(struct device *dev, int id,
> +		   struct backlight_properties *props)
> +{
> +	int ret;
> +	u8 enabled = 0;
> +	u8 status = 0;

[ ... ]

> +	/* Invert PWM */
> +	if (bri_invert > USE_DEFAULT) {
> +		ret = pmc_write(dev, BL_CTRL_INVERT, id, &bri_invert);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	ret = pmc_read(dev, BL_CTRL_INVERT, id, &bri_invert);

[Severity: High]
Could reading the hardware state directly into the global module parameter
bri_invert cause problems for systems with multiple backlight devices?
When bl_probe() loops over multiple devices, the hardware state from the
first device permanently overwrites the global parameter, which would then
contaminate the configuration for the second device.

[ ... ]

> +	if (bl_power_invert >= USE_DEFAULT) {
> +		ret = pmc_write(dev, BL_CTRL_ENABLE_INVERT, id, &bl_power_invert);
> +		if (ret)
> +			return ret;
> +	}

[Severity: Medium]
Since bl_power_invert is initialized to USE_DEFAULT, does this >= condition
always evaluate to true (-1 >= -1)? This seems to cause an unconditional
pmc_write() that could overwrite the hardware's internal default state for
the enable-invert pin.

> +	ret = pmc_read(dev, BL_CTRL_ENABLE_INVERT, id, &bl_power_invert);
> +	if (ret)
> +		return ret;
> +
> +	/* Read power state */
> +	ret = pmc_read(dev, BL_CTRL_ENABLE, id, &enabled);
> +	if (ret)
> +		return ret;
> +
> +	props->power = enabled ? BACKLIGHT_POWER_OFF : BACKLIGHT_POWER_ON;

[Severity: High]
Does this initialization map the power state inversely? If enabled is true,
assigning BACKLIGHT_POWER_OFF seems to indicate the backlight is off when
the hardware is actually enabled.

[ ... ]

> +static int bl_probe(struct platform_device *pdev)
> +{
> +	u8 id;
> +	int ret;
> +	struct device *dev = &pdev->dev;
> +	struct eio_dev *eio_dev = dev_get_drvdata(dev->parent);

[ ... ]

> +	for (id = 0; id < BL_MAX; id++) {
> +		char name[32];
> +		struct backlight_properties props;
> +		struct eio_bl_dev *eio_bl;
> +		struct backlight_device *bl;
> +
> +		memset(&props, 0, sizeof(props));
> +		props.type           = BACKLIGHT_RAW;
> +		props.max_brightness = THERMAL_MAX;
> +		props.power          = BACKLIGHT_POWER_OFF;
> +		props.brightness     = props.max_brightness;
> +		props.scale	     = BACKLIGHT_SCALE_NON_LINEAR;
> +
> +		eio_bl = devm_kzalloc(dev, sizeof(*eio_bl), GFP_KERNEL);
> +		if (!eio_bl)
> +			return -ENOMEM;
> +
> +		eio_bl->mfd = dev->parent;
> +		eio_bl->id  = id;
> +
> +		ret = bl_init(eio_bl->mfd, id, &props);
> +		if (ret) {
> +			dev_info(dev, "%d No Backlight %u enabled!\n", ret, id);
> +			continue;
> +		}
> +
> +		snprintf(name, sizeof(name), "%s%u", pdev->name, id);
> +
> +		bl = devm_backlight_device_register(dev, name, dev, eio_bl,
> +						    &bl_ops, &props);
> +
> +		if (IS_ERR(bl)) {
> +			ret = PTR_ERR(bl);
> +			if (ret == -EPROBE_DEFER)
> +				return ret;
> +
> +			dev_err(dev, "register %s failed: %d\n", name, ret);
> +			continue;
> +		}
> +	}
> +	return ret;
> +}

[Severity: High]
If the first device initializes successfully but a subsequent iteration
fails bl_init() (such as when a port is unpopulated), ret is updated and
the loop continues. Does this result in bl_probe() returning an error code
at the end, causing the successfully initialized first device to be torn
down by devres?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260714-upstream-v2-v2-0-76e5e41026db@advantech.com?part=5