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 <[email protected]> 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/[email protected]?part=2
