Re: [PATCH v2 2/2] backlight: Add support for Orient Chip OCP8178

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] The driver fails to re-initialize the 1-Wire mode after a power loss (e.g., during suspend), leaving the device stuck in PWM mode.
- [Medium] Lack of atomicity and timing validation across the 2-byte 1-Wire transaction can cause silent brightness update failures due to preemption.
--

--- Patch [2]: [PATCH v2 2/2] backlight: Add support for Orient Chip OCP8178 ---
commit 11ca2bd66a2c85f8bb56c8b3475708379c8ddf5e
Author: Wim de With <[email protected]>

backlight: Add support for Orient Chip OCP8178

Add a driver for the Orient Chip OCP8178 backlight controller.
The OCP8178 supports either PWM or a vendor-specific onewire protocol
over GPIO.

> diff --git a/drivers/video/backlight/ocp8178_bl.c b/drivers/video/backlight/ocp8178_bl.c
> new file mode 100644
> index 0000000000000..bc48636ef64e4
> --- /dev/null
> +++ b/drivers/video/backlight/ocp8178_bl.c
[ ... ]
> +static void ocp8178_bl_set_brightness(struct ocp8178_bl *ocp8178, u8 brightness)
> +{
> +	u8 data = 0;
> +
> +	/*
> +	 * We cannot set RFA (request for acknowledge) unless the GPIO pin is
> +	 * configured as open drain.
> +	 */
> +	data |= FIELD_PREP(OCP8178_DATA_RFA, 0);
> +	data |= FIELD_PREP(OCP8178_DATA_ADDR, 0);
> +	data |= FIELD_PREP(OCP8178_DATA_VALUE, brightness);
> +
> +	ocp8178_bl_write_u8(ocp8178, OCP8178_DEVICE_ADDRESS);
> +	ocp8178_bl_write_u8(ocp8178, data);

[Severity: Medium]
Can a preemption or interrupt between these two byte writes delay the second
byte and violate the 1-Wire protocol timing?

The initialization sequence ocp8178_bl_enable_onewire() explicitly checks
duration >= OCP8178_1W_T_WIN_NS to verify that timing wasn't ruined by
preemption. However, in ocp8178_bl_set_brightness(), if the thread is preempted
between the two ocp8178_bl_write_u8() calls, the GPIO line is left idle HIGH.

Would this cause the hardware's 1-Wire state machine to time out and treat the
subsequent data byte as a new command, resulting in a silently lost brightness
update since the driver doesn't read an ACK?

> +}
[ ... ]
> +static const struct backlight_ops ocp8178_bl_ops = {
> +	.options = BL_CORE_SUSPENDRESUME,
> +	.update_status = ocp8178_bl_update_status,
> +};
[ ... ]
> +static struct platform_driver ocp8178_bl_driver = {
> +	.driver = {
> +		.name = "ocp8178-bl",
> +		.of_match_table = ocp8178_bl_of_match,
> +	},
> +	.probe = ocp8178_bl_probe,
> +};

[Severity: High]
Does this driver need a resume PM hook to re-initialize the 1-Wire mode?

The BL_CORE_SUSPENDRESUME option causes the backlight core to call
update_status() on resume. Since power to the chip may be cut during suspend,
the device defaults to PWM mode upon power-on.

Without a resume hook in the platform driver to call
ocp8178_bl_enable_onewire(), the chip will misinterpret the 1-Wire brightness
data as a PWM signal. This leaves the GPIO HIGH at the end of the transaction,
which translates to 100% brightness in PWM mode.

> +module_platform_driver(ocp8178_bl_driver);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=2