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@xxxxxxxxxx>
>> ---
>
>> +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
>> }
>
