On Wed, Aug 12, 2026 at 07:34:56PM +0000, [email protected] 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
