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

Thanks for the review.

On Mon, Aug 10, 2026 at 12:33:33PM +0100, Daniel Thompson wrote:

On Thu, Aug 06, 2026 at 10:15:41PM +0200, Wim de With wrote:

quoted

+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().

 
This one is wrong, the IRQs should be restored after this delay and
following GPIO set, as noted by Sashiko.

quoted

+	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?

We don't, it adds no value. I'll remove it in v2.

quoted

+
+	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().

Will remove in v2.

quoted

+
+	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()?

Sure, and I'll make the udelay() for the ~100 us in the initialization
use usleep_range because those timings aren't that precise.

quoted

+	}
+	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.

Will fix in v2.

Daniel.

Regards,
Wim