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
