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

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:

quoted

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

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

quoted

[...]
+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.

quoted

+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