On Mon, Jun 13, 2022 at 7:08 PM Sam Ravnborg <sam@xxxxxxxxxxxx> wrote:
>
> 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>
Hi Sam,
Thanks for having a look.
I've intentionally tried to change as little as possible from the
version I copied so that any functional change is easy to spot and the
series becomes easy to review. Would it be ok if I do cleanups as a
followup series?
-Patrik
>
> >> +
> > +
> > 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
