Re: [PATCH 2/2] backlight: Add support for Orient Chip OCP8178
From: Daniel Thompson <danielt@kernel.org>
Date: 2026-08-10 11:33:42
Also in:
dri-devel, linux-devicetree, linux-leds, lkml
On Thu, Aug 06, 2026 at 10:15:41PM +0200, Wim de With wrote:
quoted hunk ↗ jump to hunk
diff --git a/drivers/video/backlight/ocp8178_bl.c b/drivers/video/backlight/ocp8178_bl.c
Without repeating Uwe's review...
quoted hunk ↗ jump to hunk
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);
Given this happens after we restore local irqs this should probably be fsleep().
+ 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);Do we really need the dev_dbg() here?
+
+ 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;This is not needed. It will happen inside backlight_get_brightness().
+
+ 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);fsleep()?
+ }
+ 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,
+};I'm with Uwe on the indentation here. No padding needed after the = IMHO. Daniel.