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

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

[Wim de With]

The ClockworkPi uConsole[1] is a modular handheld console manufactured
by Clockwork Tech that runs Linux. It consists of a mainboard which
connects a number of peripherals, such as the screen, batteries and
keyboard. This mainboard hosts a system on a module (SoM), such as the
Raspberry Pi Compute Module as a so-called core to provide the compute
for the device. The device supports different kinds of SoM as cores. The
mainboard is shared between different devices, such as the DevTerm[2].

Currently, the uConsole requires a number of out-of-tree drivers to
support the hardware. In addition, most (if not all) supported SoMs
require vendor kernels. In practice, this means that kernel support for
the device is scattered across various repositories in the form of
patches with dubious quality.

This patch series is the first in an attempt to upstream as many of the
needed drivers as possible. The goal is to support at least the
mainboard and the peripherals of the uConsole in the upstream kernel.
These drivers should find their way downstream to the SoM vendor kernels
eventually.

This series adds the driver for the backlight controller, the Orient
Chip OCP8178. I wrote the driver from scratch using the datasheet, to
avoid any copyright or attribution problems.

[1]: https://www.clockworkpi.com/home-uconsole
[2]: https://www.clockworkpi.com/home-devterm

Wim de With (2):
  dt-bindings: backlight: Add Orient Chip OCP8178 bindings
  backlight: Add support for Orient Chip OCP8178

 .../bindings/leds/backlight/ocs,ocp8178.yaml  |  46 ++++
 MAINTAINERS                                   |   6 +
 drivers/video/backlight/Kconfig               |  10 +
 drivers/video/backlight/Makefile              |   1 +
 drivers/video/backlight/ocp8178_bl.c          | 243 ++++++++++++++++++
 5 files changed, 306 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/leds/backlight/ocs,ocp8178.yaml
 create mode 100644 drivers/video/backlight/ocp8178_bl.c

-- 
2.55.0

[PATCH 1/2] dt-bindings: backlight: Add Orient Chip OCP8178

[Wim de With]

Add device tree bindings for the Orient Chip OCP8178 LED backlight
driver.

Signed-off-by: Wim de With <wf@dewith.io>
---
 .../bindings/leds/backlight/ocs,ocp8178.yaml  | 46 +++++++++++++++++++
 MAINTAINERS                                   |  5 ++
 2 files changed, 51 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/leds/backlight/ocs,ocp8178.yaml

diff --git a/Documentation/devicetree/bindings/leds/backlight/ocs,ocp8178.yaml b/Documentation/devicetree/bindings/leds/backlight/ocs,ocp8178.yaml
new file mode 100644
index 0000000000000..9a4df1ef91edd
--- /dev/null
+++ b/Documentation/devicetree/bindings/leds/backlight/ocs,ocp8178.yaml
@@ -0,0 +1,46 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/leds/backlight/ocs,ocp8178.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Orient Chip OCP8178 backlight
+
+maintainers:
+  - Wim de With <wf@dewith.io>
+
+description: |
+  The Orient Chip OCP8178 is a white LED backlight driver controlled by a single
+  GPIO line with a vendor-specific onewire protocol.
+
+allOf:
+  - $ref: common.yaml#
+
+properties:
+  compatible:
+    const: ocs,ocp8178
+
+  enable-gpios:
+    description: GPIO to use to enable/disable and dim the backlight.
+    maxItems: 1
+
+  default-brightness:
+    maximum: 31
+  max-brightness:
+    maximum: 31
+
+required:
+  - compatible
+  - enable-gpios
+
+additionalProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/gpio/gpio.h>
+    backlight {
+        compatible = "ocs,ocp8178";
+        enable-gpios = <&gpio2 5 GPIO_ACTIVE_HIGH>;
+        default-brightness = <13>;
+        max-brightness = <31>;
+    };
diff --git a/MAINTAINERS b/MAINTAINERS
index cfee5d47b214d..3ca99d9c32ef1 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -19678,6 +19678,11 @@ F:	drivers/mfd/ocelot*
 F:	drivers/net/dsa/ocelot/ocelot_ext.c
 F:	include/linux/mfd/ocelot.h
 
+OCP8178 BACKLIGHT DRIVER
+M:	Wim de With <wf@dewith.io>
+S:	Maintained
+F:	Documentation/devicetree/bindings/leds/backlight/ocs,ocp8178.yaml
+
 OCXL (Open Coherent Accelerator Processor Interface OpenCAPI) DRIVER
 M:	Mahesh J Salgaonkar <mahesh@linux.ibm.com>
 R:	Andrew Donnellan <andrew+kernel@donnellan.id.au>
-- 
2.55.0

Re: [PATCH 1/2] dt-bindings: backlight: Add Orient Chip OCP8178

[sashiko-bot]

> Add device tree bindings for the Orient Chip OCP8178 LED backlight
> driver.
> 
> Signed-off-by: Wim de With <wf@dewith.io>

Sashiko has reviewed this patch and found no issues. It looks great!

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260806201541.101304-1-wf@dewith.io?part=1

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

[Wim de With]

Add a driver for the Orient Chip OCP8178 backlight controller.

The OCP8178 supports either PWM or a vendor-specific onewire protocol
over GPIO. This driver implements the single-wire protocol, which
enables setting static brightness levels without requiring a continuous
PWM signal from the host CPU.

Signed-off-by: Wim de With <wf@dewith.io>
---
 MAINTAINERS                          |   1 +
 drivers/video/backlight/Kconfig      |  10 ++
 drivers/video/backlight/Makefile     |   1 +
 drivers/video/backlight/ocp8178_bl.c | 243 +++++++++++++++++++++++++++
 4 files changed, 255 insertions(+)
 create mode 100644 drivers/video/backlight/ocp8178_bl.c

diff --git a/MAINTAINERS b/MAINTAINERS
index 3ca99d9c32ef1..7146e4dea8d78 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -19682,6 +19682,7 @@ OCP8178 BACKLIGHT DRIVER
 M:	Wim de With <wf@dewith.io>
 S:	Maintained
 F:	Documentation/devicetree/bindings/leds/backlight/ocs,ocp8178.yaml
+F:	drivers/video/backlight/ocp8178_bl.c
 
 OCXL (Open Coherent Accelerator Processor Interface OpenCAPI) DRIVER
 M:	Mahesh J Salgaonkar <mahesh@linux.ibm.com>
diff --git a/drivers/video/backlight/Kconfig b/drivers/video/backlight/Kconfig
index 7aa1c4b21111f..aa845230c5f58 100644
--- a/drivers/video/backlight/Kconfig
+++ b/drivers/video/backlight/Kconfig
@@ -226,6 +226,16 @@ config BACKLIGHT_LOCOMO
 	  If you have a Sharp Zaurus SL-5500 (Collie) or SL-5600 (Poodle) say y to
 	  enable the LCD/backlight driver.
 
+config BACKLIGHT_OCP8178
+	tristate "OCP8178 Backlight Driver"
+	depends on GPIOLIB
+	help
+	  If you have an Orient Chip OCP8178, say Y to enable the backlight
+	  driver.
+
+	  To compile this driver as a module, choose M here: the module will
+	  be called ocp8178_bl.
+
 config BACKLIGHT_OMAP1
 	tristate "OMAP1 PWL-based LCD Backlight"
 	depends on ARCH_OMAP1 || COMPILE_TEST
diff --git a/drivers/video/backlight/Makefile b/drivers/video/backlight/Makefile
index 21c8313cfb121..c4e87b708ae21 100644
--- a/drivers/video/backlight/Makefile
+++ b/drivers/video/backlight/Makefile
@@ -51,6 +51,7 @@ obj-$(CONFIG_BACKLIGHT_MAX25014)	+= max25014.o
 obj-$(CONFIG_BACKLIGHT_MAX8925)		+= max8925_bl.o
 obj-$(CONFIG_BACKLIGHT_MP3309C)		+= mp3309c.o
 obj-$(CONFIG_BACKLIGHT_MT6370)		+= mt6370-backlight.o
+obj-$(CONFIG_BACKLIGHT_OCP8178)		+= ocp8178_bl.o
 obj-$(CONFIG_BACKLIGHT_OMAP1)		+= omap1_bl.o
 obj-$(CONFIG_BACKLIGHT_PANDORA)		+= pandora_bl.o
 obj-$(CONFIG_BACKLIGHT_PWM)		+= pwm_bl.o
diff --git a/drivers/video/backlight/ocp8178_bl.c b/drivers/video/backlight/ocp8178_bl.c
new file mode 100644
index 0000000000000..4500c9d23f7ef
--- /dev/null
+++ b/drivers/video/backlight/ocp8178_bl.c
@@ -0,0 +1,243 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Orient Chip OCP8178 Backlight Driver
+ *
+ * Copyright (C) 2026 Wim de With
+ *
+ * Author: Wim de With <wf@dewith.io>
+ */
+
+#include <linux/backlight.h>
+#include <linux/bitfield.h>
+#include <linux/bits.h>
+#include <linux/delay.h>
+#include <linux/device.h>
+#include <linux/gpio/consumer.h>
+#include <linux/irqflags.h>
+#include <linux/mod_devicetable.h>
+#include <linux/platform_device.h>
+#include <linux/property.h>
+
+#define OCP8178_MAX_BRIGHTNESS 0x1F /* 5 bits */
+
+#define OCP8178_DEVICE_ADDRESS 0x72
+
+/*
+ * We cannot set RFA (request for acknowledge) unless the GPIO pin is
+ * configured as open drain.
+ */
+#define OCP8178_DATA_RFA BIT(7)
+#define OCP8178_DATA_ADDR GENMASK(6, 5)
+#define OCP8178_DATA_VALUE GENMASK(4, 0)
+
+#define OCP8178_1W_INIT_MAX_RETRIES 5
+#define OCP8178_1W_INIT_SLEEP_MS 50
+
+#define OCP8178_T_OFF_MS 3 /* datasheet specifies at least 2.5 ms */
+#define OCP8178_1W_T_DELAY_US (100 + 10) /* 10 us as safety factor */
+#define OCP8178_1W_T_DETECT_US (260 + 10) /* 10 us as safety factor */
+#define OCP8178_1W_T_START_US 2
+#define OCP8178_1W_T_EOS_US 2
+#define OCP8178_1W_T_WIN_NS (1000 * 1000)
+
+/*
+ * The datasheet specifies 1.7 Kbps to 160 Kbps.
+ * 1 / (160 Kbps) is about 6.67 us, so using 7 us per bit should be fine.
+ * T_HIGH + T_LOW = 7 us
+ * T_HIGH > 2 * T_LOW for high bits
+ * T_LOW > 2 * T_HIGH for low bits
+ */
+#define OCP8178_1W_HIGH_BIT_T_LOW_US 2
+#define OCP8178_1W_HIGH_BIT_T_HIGH_US 5
+#define OCP8178_1W_LOW_BIT_T_LOW_US 5
+#define OCP8178_1W_LOW_BIT_T_HIGH_US 2
+
+struct ocp8178_bl {
+	struct device *dev;
+	struct gpio_desc *gpiod;
+};
+
+static int ocp8178_bl_enable_onewire(struct ocp8178_bl *ocp8178)
+{
+	u64 start, duration;
+
+	dev_dbg(ocp8178->dev, "enabling onewire protocol\n");
+
+	gpiod_set_value_cansleep(ocp8178->gpiod, 0);
+	msleep(OCP8178_T_OFF_MS);
+
+	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);
+	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);
+		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);
+			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);
+		}
+	}
+
+	gpiod_set_value(ocp8178->gpiod, 0);
+
+	local_irq_restore(flags);
+
+	udelay(OCP8178_1W_T_EOS_US);
+	gpiod_set_value(ocp8178->gpiod, 1);
+}
+
+static void ocp8178_bl_set_brightness(struct ocp8178_bl *ocp8178, u8 brightness)
+{
+	u8 data = 0;
+
+	dev_dbg(ocp8178->dev, "setting brightness to %u\n", brightness);
+
+	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);
+}
+
+static int ocp8178_bl_update_status(struct backlight_device *bl)
+{
+	struct ocp8178_bl *ocp8178 = bl_get_data(bl);
+	u8 brightness = backlight_get_brightness(bl);
+
+	/*
+	 * Setting brightness to 0 turns the backlight off but retains the
+	 * onewire mode. If we disable the controller, we would need to enable
+	 * the onewire mode again.
+	 */
+	if (backlight_is_blank(bl))
+		brightness = 0;
+
+	ocp8178_bl_set_brightness(ocp8178, brightness);
+	return 0;
+}
+
+static const struct backlight_ops ocp8178_bl_ops = {
+	.options	= BL_CORE_SUSPENDRESUME,
+	.update_status	= ocp8178_bl_update_status,
+};
+
+static int ocp8178_bl_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct backlight_device *bl;
+	struct backlight_properties props;
+	struct ocp8178_bl *ocp8178;
+	u32 max_brightness, brightness;
+	int ret, retries;
+
+	ocp8178 = devm_kzalloc(dev, sizeof(*ocp8178), GFP_KERNEL);
+	if (!ocp8178)
+		return -ENOMEM;
+
+	ocp8178->dev = dev;
+
+	ret = device_property_read_u32(dev, "max-brightness", &max_brightness);
+	if (ret)
+		max_brightness = OCP8178_MAX_BRIGHTNESS;
+	if (max_brightness > OCP8178_MAX_BRIGHTNESS) {
+		dev_warn(dev, "max brightness exceeds hardware limit\n");
+		max_brightness = OCP8178_MAX_BRIGHTNESS;
+	}
+
+	ret = device_property_read_u32(dev, "default-brightness", &brightness);
+	if (ret)
+		brightness = max_brightness;
+	if (brightness > max_brightness) {
+		dev_warn(dev, "default brightness exceeds max brightness\n");
+		brightness = max_brightness;
+	}
+
+	ocp8178->gpiod = devm_gpiod_get(dev, "enable", GPIOD_OUT_LOW);
+	if (IS_ERR(ocp8178->gpiod))
+		return dev_err_probe(dev, PTR_ERR(ocp8178->gpiod),
+				     "gpio missing or invalid\n");
+	gpiod_set_consumer_name(ocp8178->gpiod, dev_name(dev));
+
+	for (retries = 0; retries < OCP8178_1W_INIT_MAX_RETRIES; retries++) {
+		ret = ocp8178_bl_enable_onewire(ocp8178);
+		if (!ret)
+			break;
+		if (ret != -EAGAIN)
+			return ret;
+		msleep(OCP8178_1W_INIT_SLEEP_MS);
+	}
+	if (retries >= OCP8178_1W_INIT_MAX_RETRIES)
+		return dev_err_probe(dev, -ETIMEDOUT,
+				     "failed to initialize onewire protocol");
+
+	props = (typeof(props)){
+		.type		= BACKLIGHT_RAW,
+		.brightness	= brightness,
+		.max_brightness = max_brightness,
+		.power		= BACKLIGHT_POWER_ON,
+		.scale		= BACKLIGHT_SCALE_NON_LINEAR,
+	};
+
+	bl = devm_backlight_device_register(dev, dev_name(dev), dev, ocp8178,
+					    &ocp8178_bl_ops, &props);
+	if (IS_ERR(bl))
+		return dev_err_probe(dev, PTR_ERR(bl),
+				     "failed to register backlight\n");
+
+	platform_set_drvdata(pdev, bl);
+	backlight_update_status(bl);
+
+	dev_info(dev, "probed, brightness=%u/%u\n", brightness, max_brightness);
+
+	return 0;
+}
+
+static const struct of_device_id ocp8178_bl_of_match[] = {
+	{ .compatible = "ocs,ocp8178" },
+	{ /* sentinel */ }
+};
+MODULE_DEVICE_TABLE(of, ocp8178_bl_of_match);
+
+static struct platform_driver ocp8178_bl_driver = {
+	.driver		= {
+		.name		= "ocp8178-bl",
+		.of_match_table	= ocp8178_bl_of_match,
+	},
+	.probe		= ocp8178_bl_probe,
+};
+module_platform_driver(ocp8178_bl_driver);
+
+MODULE_AUTHOR("Wim de With <wf@dewith.io>");
+MODULE_DESCRIPTION("Orient Chip OCP8178 Backlight Driver");
+MODULE_LICENSE("GPL");
-- 
2.55.0

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

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] Restoring interrupts while the GPIO is driven low exposes the End-Of-Sequence pulse to arbitrary timing extension.
- [High] The driver calls `gpiod_set_value()` inside an atomic section without verifying if the GPIO can sleep.
- [Medium] Missing onewire protocol re-initialization on system resume.
- [Medium] The protocol transaction is split across multiple atomic sections, exposing the inter-byte gap to interrupt latency.
--

--- Patch [2]: [PATCH 2/2] backlight: Add support for Orient Chip OCP8178 ---
commit a17d436f493c13ff679b7f63c64445e13b71e314
Author: Wim de With <wf@dewith.io>

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. This driver implements the single-wire protocol, which
enables setting static brightness levels without requiring a continuous
PWM signal from the host CPU.

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

[ ... ]

> +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]
Could this trigger a scheduling while atomic bug if the GPIO controller is
behind an I2C or SPI expander?

Since this bit-banging occurs inside a local_irq_save() block where hardware
interrupts are disabled, any GPIO accesses must not sleep. If the driver
is instantiated via Device Tree with a sleepable GPIO, gpiod_set_value()
will attempt to sleep in an atomic context.

Should probe() explicitly reject sleepable GPIOs by checking
gpiod_cansleep()?

> +			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);
> +		}
> +	}
> +
> +	gpiod_set_value(ocp8178->gpiod, 0);
> +
> +	local_irq_restore(flags);
> +
> +	udelay(OCP8178_1W_T_EOS_US);
> +	gpiod_set_value(ocp8178->gpiod, 1);

[Severity: High]
Is there a risk that the end-of-sequence pulse gets extended arbitrarily here?

By calling local_irq_restore() while the GPIO is driven low, any pending
interrupts will run immediately. If system interrupt processing or softirqs
delay execution by more than 2.5ms (OCP8178_T_OFF_MS), the hardware will
interpret this extended low pulse as a command to disable the onewire mode.

This could cause the controller to unexpectedly power off and revert to PWM
mode, breaking backlight control.

Would it be safer to restore interrupts after the final gpiod_set_value()
call?

> +}
> +
> +static void ocp8178_bl_set_brightness(struct ocp8178_bl *ocp8178, u8 brightness)
> +{
> +	u8 data = 0;
> +
> +	dev_dbg(ocp8178->dev, "setting brightness to %u\n", brightness);
> +
> +	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]
Does the controller require strict timing between the address and data bytes?

Since ocp8178_bl_write_u8() only disables interrupts for the duration of a
single byte, an interrupt could fire between these two calls. If the 1-wire
hardware state machine has a maximum idle time, arbitrary interrupt latency
in the inter-byte gap might cause the controller to reset and drop the
brightness update.

> +}

[ ... ]

> +static const struct backlight_ops ocp8178_bl_ops = {
> +	.options	= BL_CORE_SUSPENDRESUME,
> +	.update_status	= ocp8178_bl_update_status,
> +};

[Severity: Medium]
Is BL_CORE_SUSPENDRESUME sufficient for restoring this hardware after suspend?

If the platform cuts power to the backlight controller during system suspend,
the hardware will wake up in its default PWM mode. The backlight core will
simply call update_status() on resume to restore brightness, but the onewire
brightness commands will be ignored because the onewire mode hasn't been
re-enabled.

Should the platform driver provide pm operations to re-execute
ocp8178_bl_enable_onewire() on system resume?

[ ... ]

> +	ocp8178->gpiod = devm_gpiod_get(dev, "enable", GPIOD_OUT_LOW);
> +	if (IS_ERR(ocp8178->gpiod))
> +		return dev_err_probe(dev, PTR_ERR(ocp8178->gpiod),
> +				     "gpio missing or invalid\n");

[Severity: High]
As mentioned above, since the driver requires microsecond precision
bit-banging in an atomic context, should we check gpiod_cansleep() here
and fail probe if the GPIO requires sleeping?

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260806201541.101304-1-wf@dewith.io?part=2

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

[Wim de With]

On Thu, Aug 06, 2026 at 08:36:53PM +0000, sashiko-bot@kernel.org wrote:
> > +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]
> Could this trigger a scheduling while atomic bug if the GPIO controller is
> behind an I2C or SPI expander?
> 
> Since this bit-banging occurs inside a local_irq_save() block where hardware
> interrupts are disabled, any GPIO accesses must not sleep. If the driver
> is instantiated via Device Tree with a sleepable GPIO, gpiod_set_value()
> will attempt to sleep in an atomic context.

> Should probe() explicitly reject sleepable GPIOs by checking
> gpiod_cansleep()?

I didn't realize that the spinlock safety is a property of the GPIO
itself. Applying this suggestion means that we don't need to use
gpiod_set_value_cansleep() at all in this driver.

I took inspiration from the ktd253 driver, which also has tight timing
requirements on GPIO bit-banging. This driver does not check
gpiod_cansleep() either and mixes gpiod_set_value() and
gpiod_set_value_cansleep(), which is incorrect.

> > + 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);
> > + }
> > + }
> > +
> > + gpiod_set_value(ocp8178->gpiod, 0);
> > +
> > + local_irq_restore(flags);
> > +
> > + udelay(OCP8178_1W_T_EOS_US);
> > + gpiod_set_value(ocp8178->gpiod, 1);
> 
> [Severity: High]
> Is there a risk that the end-of-sequence pulse gets extended arbitrarily here?
> 
> By calling local_irq_restore() while the GPIO is driven low, any pending
> interrupts will run immediately. If system interrupt processing or softirqs
> delay execution by more than 2.5ms (OCP8178_T_OFF_MS), the hardware will
> interpret this extended low pulse as a command to disable the onewire mode.
> 
> This could cause the controller to unexpectedly power off and revert to PWM
> mode, breaking backlight control.
> 
> Would it be safer to restore interrupts after the final gpiod_set_value()
> call?

This is indeed a problem and I'll fix it in v2. The datasheet states
that T_EOS must be between 2 and 360 us. Interrupt handling may
exceed this value.

> > +static void ocp8178_bl_set_brightness(struct ocp8178_bl *ocp8178, u8 brightness)
> > +{
> > + u8 data = 0;
> > +
> > + dev_dbg(ocp8178->dev, "setting brightness to %u\n", brightness);
> > +
> > + 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]
> Does the controller require strict timing between the address and data bytes?
> 
> Since ocp8178_bl_write_u8() only disables interrupts for the duration of a
> single byte, an interrupt could fire between these two calls. If the 1-wire
> hardware state machine has a maximum idle time, arbitrary interrupt latency
> in the inter-byte gap might cause the controller to reset and drop the
> brightness update.

For sending a single byte, we need to disable interrupts for about 58
us. The datasheet doesn't put a maximum on T_START (which is the start
of sequence period) so I would need to test the practical limitation.
Regardless though, disabling interrupts for both writes would take 118
us and I'm not sure if that is preferable over failing a brightness
update in rare situations.

> > +static const struct backlight_ops ocp8178_bl_ops = {
> > + .options = BL_CORE_SUSPENDRESUME,
> > + .update_status = ocp8178_bl_update_status,
> > +};
> 
> [Severity: Medium]
> Is BL_CORE_SUSPENDRESUME sufficient for restoring this hardware after suspend?
> 
> If the platform cuts power to the backlight controller during system suspend,
> the hardware will wake up in its default PWM mode. The backlight core will
> simply call update_status() on resume to restore brightness, but the onewire
> brightness commands will be ignored because the onewire mode hasn't been
> re-enabled.
> 
> Should the platform driver provide pm operations to re-execute
> ocp8178_bl_enable_onewire() on system resume?

I'm not sure if this is just LLM-hallucination or actually relevant.
Should I account for the power cutting to the controller if the driver
itself doesn't implement the power cutting?

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

[Uwe Kleine-König]

[-- Attachment #1: Type: text/plain, Size: 2384 bytes --]

On Thu, Aug 06, 2026 at 10:15:41PM +0200, Wim de With wrote:
> +#include <linux/mod_devicetable.h>
> +#include <linux/platform_device.h>

Please don't use <linux/mod_devicetable.h> in new code.
<linux/platform_device.h> already provides struct of_device_id, so you
should be able to just drop the include for <linux/mod_devicetable.h>.

> [...]
> +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);
> +			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);
> +		}
> +	}
> +
> +	gpiod_set_value(ocp8178->gpiod, 0);
> +
> +	local_irq_restore(flags);
> +
> +	udelay(OCP8178_1W_T_EOS_US);
> +	gpiod_set_value(ocp8178->gpiod, 1);
> +}

Is this function open-coding stuff that already exists in drivers/w1?
(Just asking because you call that onewire).

> [...]
> +static int ocp8178_bl_probe(struct platform_device *pdev)
> +{
> +	[...]
> +
> +	dev_info(dev, "probed, brightness=%u/%u\n", brightness, max_brightness);

IMHO this is just noise once the code hits mainline. The amount of log
lines like these during boot is just annoying and makes it hard to
identify the relevant lines. So if you're confident that your driver
works, users are probably not interested in that line and you can drop
it (or degrade to dev_dbg).

> +static const struct of_device_id ocp8178_bl_of_match[] = {
> +	{ .compatible = "ocs,ocp8178" },
> +	{ /* sentinel */ }
> +};
> +MODULE_DEVICE_TABLE(of, ocp8178_bl_of_match);
> +
> +static struct platform_driver ocp8178_bl_driver = {
> +	.driver		= {
> +		.name		= "ocp8178-bl",
> +		.of_match_table	= ocp8178_bl_of_match,
> +	},
> +	.probe		= ocp8178_bl_probe,

I'm not a fan of aligning the = chars. But opinions differ.

> +};
> +module_platform_driver(ocp8178_bl_driver);
> +
> +MODULE_AUTHOR("Wim de With <wf@dewith.io>");
> +MODULE_DESCRIPTION("Orient Chip OCP8178 Backlight Driver");
> +MODULE_LICENSE("GPL");

Best regards
Uwe

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

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

[Wim de With]

On Fri, Aug 07, 2026 at 08:46:07AM +0200, Uwe Kleine-König wrote:
> On Thu, Aug 06, 2026 at 10:15:41PM +0200, Wim de With wrote:
> > +#include <linux/mod_devicetable.h>
> > +#include <linux/platform_device.h>
> 
> Please don't use <linux/mod_devicetable.h> in new code.
> <linux/platform_device.h> already provides struct of_device_id, so you
> should be able to just drop the include for <linux/mod_devicetable.h>.

Sure, will do.

> > +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);
> > +			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);
> > +		}
> > +	}
> > +
> > +	gpiod_set_value(ocp8178->gpiod, 0);
> > +
> > +	local_irq_restore(flags);
> > +
> > +	udelay(OCP8178_1W_T_EOS_US);
> > +	gpiod_set_value(ocp8178->gpiod, 1);
> > +}
> 
> Is this function open-coding stuff that already exists in drivers/w1?
> (Just asking because you call that onewire).

The datasheet calls this 1-Wire, but it is a proprietary protocol, not
the 1-Wire protocol from Dallas Semiconductor that is implemented in
drivers/w1.

> > [...]
> > +static int ocp8178_bl_probe(struct platform_device *pdev)
> > +{
> > +	[...]
> > +
> > +	dev_info(dev, "probed, brightness=%u/%u\n", brightness, max_brightness);
> 
> IMHO this is just noise once the code hits mainline. The amount of log
> lines like these during boot is just annoying and makes it hard to
> identify the relevant lines. So if you're confident that your driver
> works, users are probably not interested in that line and you can drop
> it (or degrade to dev_dbg).

I'm confident that when the driver fails to load, a message is logged,
so I'll downgrade it to dev_dbg.

> > +static const struct of_device_id ocp8178_bl_of_match[] = {
> > +	{ .compatible = "ocs,ocp8178" },
> > +	{ /* sentinel */ }
> > +};
> > +MODULE_DEVICE_TABLE(of, ocp8178_bl_of_match);
> > +
> > +static struct platform_driver ocp8178_bl_driver = {
> > +	.driver		= {
> > +		.name		= "ocp8178-bl",
> > +		.of_match_table	= ocp8178_bl_of_match,
> > +	},
> > +	.probe		= ocp8178_bl_probe,
> 
> I'm not a fan of aligning the = chars. But opinions differ.

I have no strong opinions on this, so I'll go along with what the
maintainer wants.

Regards,
Wim