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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [High] Missing 1-wire initialization on system resume leaves the device unresponsive or at maximum brightness.
- [High] Software bitbanging timings are too tight and will likely violate protocol requirements due to execution overhead.
- [High] Calling gpiod_set_value() with hardware interrupts disabled causes a sleeping-in-atomic bug on PREEMPT_RT kernels.
- [Medium] Preemption during onewire initialization can cause silent failure.
- [Low] Using dev_err() for expected, retryable preemption events will spam the kernel log.
--

commit 999030427b1d2e4a9b850939beacde4b60c21042
Author: Wim de With <wf@dewith.io>

backlight: Add support for Orient Chip OCP8178

This patch introduces a new backlight driver for the Orient Chip OCP8178.
It implements a vendor-specific onewire protocol over GPIO to set static
brightness levels without requiring a continuous PWM signal.

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

[ ... ]

> +static int ocp8178_bl_enable_onewire(struct ocp8178_bl *ocp8178)
> +{
> +	u64 start, duration;
> +
> +	gpiod_set_value(ocp8178->gpiod, 0);
> +	fsleep(OCP8178_T_OFF_US);
> +
> +	start = ktime_get_ns();
> +
> +	gpiod_set_value(ocp8178->gpiod, 1);
> +	udelay(OCP8178_1W_T_DELAY_US);
> +	gpiod_set_value(ocp8178->gpiod, 0);
> +	udelay(OCP8178_1W_T_DETECT_US);

[Severity: Medium]
Since this precise timing pulse is generated without preemption or IRQ
protection, could an interrupt stretch the 110us pulse?

If it stretches to 300us, the total duration (300 + 270 = 570us) still passes
the 1ms check below, but the malformed timing causes the hardware to reject
the switch to 1-wire mode, leading to a silent failure.

> +	gpiod_set_value(ocp8178->gpiod, 1);
> +
> +	duration = ktime_get_ns() - start;
> +
> +	if (duration >= OCP8178_1W_T_WIN_NS) {
> +		dev_err(ocp8178->dev,
> +			"onewire detection window exceeded (%llu ns)\n",
> +			duration);

[Severity: Low]
Because initialization runs with interrupts enabled, transient preemption will
cause this window to exceed 1ms and return -EAGAIN for an automatic retry.
Will using dev_err() here cause unnecessary log spam on busy systems?

> +		return -EAGAIN;
> +	}
> +
> +	return 0;
> +}
> +
> +static void ocp8178_bl_write_u8(struct ocp8178_bl *ocp8178, u8 value)
> +{
> +	unsigned long flags;
> +
> +	gpiod_set_value(ocp8178->gpiod, 1);
> +	udelay(OCP8178_1W_T_START_US);
> +
> +	local_irq_save(flags);
> +
> +	for (int i = 7; i >= 0; i--) {
> +		if ((value >> i) & 1) {
> +			gpiod_set_value(ocp8178->gpiod, 0);

[Severity: High]
Does calling gpiod_set_value() with hardware interrupts disabled cause a
sleeping-in-atomic bug on PREEMPT_RT kernels?

Memory-mapped GPIO drivers typically use spinlock_t to serialize register
accesses, which becomes a sleepable rt_mutex on PREEMPT_RT. Any userspace
write to the brightness sysfs node will trigger this execution path.

> +			udelay(OCP8178_1W_HIGH_BIT_T_LOW_US);
> +			gpiod_set_value(ocp8178->gpiod, 1);
> +			udelay(OCP8178_1W_HIGH_BIT_T_HIGH_US);
> +		} else {
> +			gpiod_set_value(ocp8178->gpiod, 0);
> +			udelay(OCP8178_1W_LOW_BIT_T_LOW_US);
> +			gpiod_set_value(ocp8178->gpiod, 1);
> +			udelay(OCP8178_1W_LOW_BIT_T_HIGH_US);

[Severity: High]
Are these software bitbanging timings too tight to satisfy the protocol?

The protocol requires T_LOW > 2 * T_HIGH for a low bit. The driver uses
udelay(2) for T_HIGH and udelay(5) for T_LOW. After udelay(2), the loop logic
evaluates conditions and decrements variables before pulling the line LOW again.

This CPU overhead executes while the line remains HIGH, stretching T_HIGH by
1-2us, making 2 * T_HIGH (~6-8us) strictly greater than T_LOW (5us) and breaking
the requirement.

> +		}
> +	}
> +
> +	gpiod_set_value(ocp8178->gpiod, 0);
> +	udelay(OCP8178_1W_T_EOS_US);
> +	gpiod_set_value(ocp8178->gpiod, 1);
> +
> +	local_irq_restore(flags);
> +}

[ ... ]

> +static const struct backlight_ops ocp8178_bl_ops = {
> +	.options = BL_CORE_SUSPENDRESUME,

[Severity: High]
Does using BL_CORE_SUSPENDRESUME leave the device unresponsive after a
system suspend?

If the system is suspended and power is cut to the chip, or the GPIO line
goes low for >2.5ms, the device resets to its default PWM mode. On resume,
the backlight core simply restores the brightness by calling
ocp8178_bl_update_status() without ensuring the hardware is in 1-Wire mode.

Because the driver relies on BL_CORE_SUSPENDRESUME and lacks device PM ops in
ocp8178_bl_driver, it fails to re-transmit the required enable_onewire sequence.

> +	.update_status = ocp8178_bl_update_status,
> +};

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260826-ocp8178-backlight-v4-0-47d7acce882e@dewith.io?part=2