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
