[PATCH 1/4] drm/gma500: Refactor backlight support

[Sam Ravnborg]

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