Hi Hans, > >> +static int gma_backlight_update_status(struct backlight_device *bd) > >> +{ > >> + struct drm_device *dev = bl_get_data(bd); > >> + int level = bd->props.brightness; > > > > Here backlight_get_brightness(bd) should be used. > > Ack, but the old set methods all 3 used: > > int level = bd->props.brightness; > > So that would be a small functional / behavior change. > > As such I would prefer to split using backlight_get_brightness(bd) > out into a separate patch for version 2 of the series. > Like how I also made the change from type = BACKLIGHT_PLATFORM -> > type = BACKLIGHT_RAW a separate change. > > Would that be ok with you ? That would be perfect!
> > > > > >> int gma_backlight_init(struct drm_device *dev) > >> { > >> -#ifdef CONFIG_BACKLIGHT_CLASS_DEVICE > >> struct drm_psb_private *dev_priv = to_drm_psb_private(dev); > >> + struct backlight_properties props = {}; > >> + int ret; > >> + > >> dev_priv->backlight_enabled = true; > >> - return dev_priv->ops->backlight_init(dev); > >> -#else > >> - return 0; > >> + dev_priv->backlight_level = 100; > >> + > >> + ret = dev_priv->ops->backlight_init(dev); > >> + if (ret) > >> + return ret; > >> + > >> +#ifdef CONFIG_BACKLIGHT_CLASS_DEVICE > >> + props.brightness = dev_priv->backlight_level; > >> + props.max_brightness = PSB_MAX_BRIGHTNESS; > >> + props.type = BACKLIGHT_PLATFORM; > >> + > >> + dev_priv->backlight_device = > >> + backlight_device_register(dev_priv->ops->backlight_name, > >> + dev->dev, dev, > >> + &gma_backlight_ops, &props); > > > > Consider using the devm_backlight_device_register() variant. > > Then you can drop gma_backlight_exit() - I think.. > > The problem is the rest of the driver does not use devm_foo functions, > so then psb_driver_unload() which runs before the devm cleanup functions > will already release various iommap-s before the backlight is unregistered. > > This leaves a race where the backlight device could be poked and then try > to use no longer valid pointers in the main driver struct to write to the hw. Thanks for the explanation. When someone update the driver to devn_ then they surely will include backlight too. (I do not try to persuade you to do it, your time is better spent on the bigger backlight picture). Sam