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

[Wim de With <wf@dewith.io>]

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