Re: [PATCH 01/19] drm/gma500: Unify *_lvds_get_max_backlight()

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@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 sam@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