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

[Hans de Goede]

Hi Sam,

On 9/11/22 13:48, Sam Ravnborg wrote:
> Hi Hans,
> 
> just a few minor things. See comments.
> I like the diff - removes much more than it adds.

I'm glad you like it and thank you for the review.

> On Sat, Sep 10, 2022 at 10:50:58PM +0200, Hans de Goede wrote:
>> Refactor backlight support so that the gma_backlight_enable() /
>> gma_backlight_disable() / gma_backlight_set() functions used by
>> the Opregion handle will also work if no backlight_device gets
>> registered.
>>
>> This is a preparation patch for not registering the gma500's own backlight
>> device when acpi_video should be used, since registering 2 backlight
>> devices for a single display really is undesirable.
>>
>> Since the acpi-video interface often uses the OpRegion we need to keep
>> the OpRegion functional even when dev_priv->backlight_device is NULL.
>>
>> As a result of this refactor the actual backlight_device_register()
>> call is moved to the shared backlight.c code and all #ifdefs related to
>> CONFIG_BACKLIGHT_CLASS_DEVICE are now also limited to backlight.c .
>>
>> No functional changes intended.
>>
>> This has been tested on a Packard Bell Dot SC (Intel Atom N2600, cedarview)
>> and a Sony Vaio vpc-x11s1e (Intel N540, poulsbo) laptop.
>>
>> Signed-off-by: Hans de Goede <hdegoede at redhat.com>
>> ---
> 
>> +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 ?

> 
> 
>>  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.

Regards,

Hans





> 
>> +	if (IS_ERR(dev_priv->backlight_device))
>> +		return PTR_ERR(dev_priv->backlight_device);
>>  #endif
>> +
>> +	return 0;
>>  }
>>  
>>  void gma_backlight_exit(struct drm_device *dev)
>>  {
>>  #ifdef CONFIG_BACKLIGHT_CLASS_DEVICE
>>  	struct drm_psb_private *dev_priv = to_drm_psb_private(dev);
>> -	if (dev_priv->backlight_device) {
>> -		dev_priv->backlight_device->props.brightness = 0;
>> -		backlight_update_status(dev_priv->backlight_device);
>> +
>> +	if (dev_priv->backlight_device)
>>  		backlight_device_unregister(dev_priv->backlight_device);
>> -	}
>>  #endif
>>  }
>