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
