Hi Patrick.

On Mon, Jun 13, 2022 at 02:34:18PM +0200, Patrik Jakobsson wrote:
> These functions mostly do the same thing so unify them into one. All
> unified lvds code will live in gma_lvds.c.
> 
> Signed-off-by: Patrik Jakobsson <patrik.r.jakobs...@gmail.com>
> ---
> +/*
> + * Returns the maximum level of the backlight duty cycle field.
> + */

I find this function quite un-readable.

> +u32 gma_lvds_get_max_backlight(struct drm_device *dev)
> +{
> +     struct drm_psb_private *dev_priv = to_drm_psb_private(dev);
> +     u32 retval;
> +
> +     if (gma_power_begin(dev, false)) {
> +             retval = ((REG_READ(BLC_PWM_CTL) &
> +                       BACKLIGHT_MODULATION_FREQ_MASK) >>
> +                       BACKLIGHT_MODULATION_FREQ_SHIFT) * 2;
> +
> +             gma_power_end(dev);
> +     } else
> +             retval = ((dev_priv->regs.saveBLC_PWM_CTL &
> +                       BACKLIGHT_MODULATION_FREQ_MASK) >>
> +                       BACKLIGHT_MODULATION_FREQ_SHIFT) * 2;
> +
> +     return retval;
> +}

Maybe like this - which should be the same functionality:

u32 gma_lvds_get_max_backlight(struct drm_device *dev)
{
        struct drm_psb_private *dev_priv = to_drm_psb_private(dev);
        u32 pwmctl;

        if (gma_power_begin(dev, false)) {
                pwmctl = REG_READ(BLC_PWM_CTL);
                gma_power_end(dev);
        } else {
                pwmctl = dev_priv->regs.saveBLC_PWM_CTL;
        }
        
        pwmctl = pwmctl & BACKLIGHT_MODULATION_FREQ_MASK;
        return  (pwmctl >> BACKLIGHT_MODULATION_FREQ_SHIFT) * 2;
}

this is more or less the same as in psb_intel_lvds_get_max_backlight().

With or without this change the patch is:
Reviewed-by: Sam Ravnborg <s...@ravnborg.org>

>> +
> +
> diff --git a/drivers/gpu/drm/gma500/gma_lvds.h 
> b/drivers/gpu/drm/gma500/gma_lvds.h
> new file mode 100644
> index 000000000000..2a9ce8ee3fa7
> --- /dev/null
> +++ b/drivers/gpu/drm/gma500/gma_lvds.h
> @@ -0,0 +1,12 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +
> +/*
> + * Copyright © 2006-2011 Intel Corporation
> + */
> +
> +#ifndef _GMA_LVDS_H
> +#define _GMA_LVDS_H
> +
> +u32 gma_lvds_get_max_backlight(struct drm_device *dev);
> +
> +#endif
> diff --git a/drivers/gpu/drm/gma500/oaktrail_lvds.c 
> b/drivers/gpu/drm/gma500/oaktrail_lvds.c
> index 9c9ebf8e29c4..4913baca7ae2 100644
> --- a/drivers/gpu/drm/gma500/oaktrail_lvds.c
> +++ b/drivers/gpu/drm/gma500/oaktrail_lvds.c
> @@ -20,6 +20,7 @@
>  #include "psb_drv.h"
>  #include "psb_intel_drv.h"
>  #include "psb_intel_reg.h"
> +#include "gma_lvds.h"
>  
>  /* The max/min PWM frequency in BPCR[31:17] - */
>  /* The smallest number is 1 (not 0) that can fit in the
> @@ -170,25 +171,6 @@ static void oaktrail_lvds_prepare(struct drm_encoder 
> *encoder)
>       gma_power_end(dev);
>  }
>  
> -static u32 oaktrail_lvds_get_max_backlight(struct drm_device *dev)
> -{
> -     struct drm_psb_private *dev_priv = to_drm_psb_private(dev);
> -     u32 ret;
> -
> -     if (gma_power_begin(dev, false)) {
> -             ret = ((REG_READ(BLC_PWM_CTL) &
> -                       BACKLIGHT_MODULATION_FREQ_MASK) >>
> -                       BACKLIGHT_MODULATION_FREQ_SHIFT) * 2;
> -
> -             gma_power_end(dev);
> -     } else
> -             ret = ((dev_priv->regs.saveBLC_PWM_CTL &
> -                       BACKLIGHT_MODULATION_FREQ_MASK) >>
> -                       BACKLIGHT_MODULATION_FREQ_SHIFT) * 2;
> -
> -     return ret;
> -}
> -
>  static void oaktrail_lvds_commit(struct drm_encoder *encoder)
>  {
>       struct drm_device *dev = encoder->dev;
> @@ -197,8 +179,7 @@ static void oaktrail_lvds_commit(struct drm_encoder 
> *encoder)
>       struct psb_intel_mode_device *mode_dev = &dev_priv->mode_dev;
>  
>       if (mode_dev->backlight_duty_cycle == 0)
> -             mode_dev->backlight_duty_cycle =
> -                                     oaktrail_lvds_get_max_backlight(dev);
> +             mode_dev->backlight_duty_cycle = 
> gma_lvds_get_max_backlight(dev);
>       oaktrail_lvds_set_power(dev, gma_encoder, true);
>  }
>  
> diff --git a/drivers/gpu/drm/gma500/psb_intel_lvds.c 
> b/drivers/gpu/drm/gma500/psb_intel_lvds.c
> index 7ee6c8ce103b..371c202a15ce 100644
> --- a/drivers/gpu/drm/gma500/psb_intel_lvds.c
> +++ b/drivers/gpu/drm/gma500/psb_intel_lvds.c
> @@ -18,6 +18,7 @@
>  #include "psb_drv.h"
>  #include "psb_intel_drv.h"
>  #include "psb_intel_reg.h"
> +#include "gma_lvds.h"
>  
>  /*
>   * LVDS I2C backlight control macros
> @@ -52,32 +53,6 @@ struct psb_intel_lvds_priv {
>       struct gma_i2c_chan *i2c_bus;
>  };
>  
> -
> -/*
> - * Returns the maximum level of the backlight duty cycle field.
> - */
> -static u32 psb_intel_lvds_get_max_backlight(struct drm_device *dev)
> -{
> -     struct drm_psb_private *dev_priv = to_drm_psb_private(dev);
> -     u32 ret;
> -
> -     if (gma_power_begin(dev, false)) {
> -             ret = REG_READ(BLC_PWM_CTL);
> -             gma_power_end(dev);
> -     } else /* Powered off, use the saved value */
> -             ret = dev_priv->regs.saveBLC_PWM_CTL;
> -
> -     /* Top 15bits hold the frequency mask */
> -     ret = (ret &  BACKLIGHT_MODULATION_FREQ_MASK) >>
> -                                     BACKLIGHT_MODULATION_FREQ_SHIFT;
> -
> -        ret *= 2;    /* Return a 16bit range as needed for setting */
> -        if (ret == 0)
> -                dev_err(dev->dev, "BL bug: Reg %08x save %08X\n",
> -                        REG_READ(BLC_PWM_CTL), 
> dev_priv->regs.saveBLC_PWM_CTL);
> -     return ret;
> -}
> -
>  /*
>   * Set LVDS backlight level by I2C command
>   *
> @@ -131,7 +106,7 @@ static int psb_lvds_pwm_set_brightness(struct drm_device 
> *dev, int level)
>       u32 max_pwm_blc;
>       u32 blc_pwm_duty_cycle;
>  
> -     max_pwm_blc = psb_intel_lvds_get_max_backlight(dev);
> +     max_pwm_blc = gma_lvds_get_max_backlight(dev);
>  
>       /*BLC_PWM_CTL Should be initiated while backlight device init*/
>       BUG_ON(max_pwm_blc == 0);
> @@ -176,7 +151,7 @@ void psb_intel_lvds_set_brightness(struct drm_device 
> *dev, int level)
>  /*
>   * Sets the backlight level.
>   *
> - * level: backlight level, from 0 to psb_intel_lvds_get_max_backlight().
> + * level: backlight level, from 0 to gma_lvds_get_max_backlight().
>   */
>  static void psb_intel_lvds_set_backlight(struct drm_device *dev, int level)
>  {
> @@ -275,8 +250,7 @@ static void psb_intel_lvds_save(struct drm_connector 
> *connector)
>        * just make it full brightness
>        */
>       if (dev_priv->backlight_duty_cycle == 0)
> -             dev_priv->backlight_duty_cycle =
> -             psb_intel_lvds_get_max_backlight(dev);
> +             dev_priv->backlight_duty_cycle = 
> gma_lvds_get_max_backlight(dev);
>  
>       dev_dbg(dev->dev, "(0x%x, 0x%x, 0x%x, 0x%x, 0x%x, 0x%x)\n",
>                       lvds_priv->savePP_ON,
> @@ -445,7 +419,7 @@ static void psb_intel_lvds_commit(struct drm_encoder 
> *encoder)
>  
>       if (mode_dev->backlight_duty_cycle == 0)
>               mode_dev->backlight_duty_cycle =
> -                 psb_intel_lvds_get_max_backlight(dev);
> +                 gma_lvds_get_max_backlight(dev);
>  
>       psb_intel_lvds_set_power(dev, true);
>  }
> -- 
> 2.36.1

Reply via email to