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

[Wim de With <wf@dewith.io>]

On Wed, Aug 12, 2026 at 07:34:56PM +0000, sashiko-bot@kernel.org wrote:
> > +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?
> 

This is the same point that Sashiko raised for v1, but now I tested it
like so:

ocp8178_bl_write_u8(ocp8178, OCP8178_DEVICE_ADDRESS);
msleep(1000);
ocp8178_bl_write_u8(ocp8178, data);

It still works for 90% of the brightness updates, and only misses some
if you spam the updates during the sleep, so I'm inclined to say it is
robust enough.

> > +}
> [ ... ]
> > +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.

This is still an open question for me. I know the ClockworkPi platform
doesn't cut power to the chip during suspend, because the cores it uses
don't really support suspend power states.

However, for mainline inclusion, should I assume that the chip might be
used in situations where suspend does cut power to the chip and plan for
it accordingly?

Regards,
Wim