Hi Hans,
just a few minor things. See comments.
I like the diff - removes much more than it adds.
Sam
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 <hdego...@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.
> 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..
> + 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
> }
