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