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.jakobsson@xxxxxxxxx>
> ---
> +/*
> + * 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 <sam@xxxxxxxxxxxx>
>> +
> +
> 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
