[PATCH 1/2] drm/i915: Abstract backlight registers a bit

[PATCH 1/2] drm/i915: Abstract backlight registers a bit

[Ben Widawsky]

Many GENs generally perform the same actions just on different
registers. This is true going forward as well. To ease the transition a
bit, extract the common code where possible.

Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
---
 drivers/gpu/drm/i915/intel_panel.c | 25 +++++++++++++------------
 1 file changed, 13 insertions(+), 12 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_panel.c b/drivers/gpu/drm/i915/intel_panel.c
index 1f29960..83e174b 100644
--- a/drivers/gpu/drm/i915/intel_panel.c
+++ b/drivers/gpu/drm/i915/intel_panel.c
@@ -430,24 +430,25 @@ static u32 intel_panel_compute_brightness(struct drm_device *dev, u32 val)
 static u32 intel_panel_get_backlight(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
-	u32 val;
+	u32 reg, val;
 	unsigned long flags;
 
 	spin_lock_irqsave(&dev_priv->backlight.lock, flags);
 
-	if (HAS_PCH_SPLIT(dev)) {
-		val = I915_READ(BLC_PWM_CPU_CTL) & BACKLIGHT_DUTY_CYCLE_MASK;
-	} else {
-		val = I915_READ(BLC_PWM_CTL) & BACKLIGHT_DUTY_CYCLE_MASK;
-		if (INTEL_INFO(dev)->gen < 4)
-			val >>= 1;
+	if (HAS_PCH_SPLIT(dev))
+		reg = BLC_PWM_CPU_CTL;
+	else
+		reg = BLC_PWM_CTL;
 
-		if (is_backlight_combination_mode(dev)) {
-			u8 lbpc;
+	val = I915_READ(reg) & BACKLIGHT_DUTY_CYCLE_MASK;
+	if (INTEL_INFO(dev)->gen < 4)
+		val >>= 1;
 
-			pci_read_config_byte(dev->pdev, PCI_LBPC, &lbpc);
-			val *= lbpc;
-		}
+	if (is_backlight_combination_mode(dev)) {
+		u8 lbpc;
+
+		pci_read_config_byte(dev->pdev, PCI_LBPC, &lbpc);
+		val *= lbpc;
 	}
 
 	val = intel_panel_compute_brightness(dev, val);
-- 
1.8.4.1

Re: [PATCH 1/2] drm/i915: Abstract backlight registers a bit

[Daniel Vetter]

On Mon, Oct 21, 2013 at 7:35 PM, Ben Widawsky
<benjamin.widawsky@intel.com> wrote:
> Many GENs generally perform the same actions just on different
> registers. This is true going forward as well. To ease the transition a
> bit, extract the common code where possible.
>
> Signed-off-by: Ben Widawsky <ben@bwidawsk.net>

Imo we should just switch to vtables or something like that, since
it's not just the register that changes, but also the meaning of the
different bits in the registers themselves and the logic to drive
stuff. We've often had regression-galore in this code and strictly
separating different platforms should help. I hope.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH 1/2] drm/i915: Abstract backlight registers a bit

[Ben Widawsky]

On Tue, Oct 22, 2013 at 12:30:11AM +0200, Daniel Vetter wrote:
> On Mon, Oct 21, 2013 at 7:35 PM, Ben Widawsky
> <benjamin.widawsky@intel.com> wrote:
> > Many GENs generally perform the same actions just on different
> > registers. This is true going forward as well. To ease the transition a
> > bit, extract the common code where possible.
> >
> > Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
> 
> Imo we should just switch to vtables or something like that, since
> it's not just the register that changes, but also the meaning of the
> different bits in the registers themselves and the logic to drive
> stuff. We've often had regression-galore in this code and strictly
> separating different platforms should help. I hope.
> -Daniel

I do not disagree. I have neither the skill, time, not inclination to do
that properly, and I really only care about patch 2. The cleanup here
makes the next patch slightly nicer, but I can live with whatever.

-- 
Ben Widawsky, Intel Open Source Technology Center

Re: [PATCH 1/2] drm/i915: Abstract backlight registers a bit

[Daniel Vetter]

On Tue, Oct 22, 2013 at 12:48 AM, Ben Widawsky <ben@bwidawsk.net> wrote:
> On Tue, Oct 22, 2013 at 12:30:11AM +0200, Daniel Vetter wrote:
>> On Mon, Oct 21, 2013 at 7:35 PM, Ben Widawsky
>> <benjamin.widawsky@intel.com> wrote:
>> > Many GENs generally perform the same actions just on different
>> > registers. This is true going forward as well. To ease the transition a
>> > bit, extract the common code where possible.
>> >
>> > Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
>>
>> Imo we should just switch to vtables or something like that, since
>> it's not just the register that changes, but also the meaning of the
>> different bits in the registers themselves and the logic to drive
>> stuff. We've often had regression-galore in this code and strictly
>> separating different platforms should help. I hope.
>> -Daniel
>
> I do not disagree. I have neither the skill, time, not inclination to do
> that properly, and I really only care about patch 2. The cleanup here
> makes the next patch slightly nicer, but I can live with whatever.

Personally I vote to keep the ugliness here until someone is fed up
and cleans it up for good. Jesse's looking ripe for it, especially
since vlv has this new need for a pipe parameter added to the
backlight code. So for now I'd just add another if clause all over and
copy-paste the existing code for the 2nd patch for -internal.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch