2015-08-17 16:51 GMT-03:00 Paulo Zanoni <przanoni@xxxxxxxxx>:
> 2015-08-12 12:44 GMT-03:00 <ville.syrjala@xxxxxxxxxxxxxxx>:
>> From: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
>>
>> Indent the PORTx_HOTPLUG_... defines appropriately, and fix some space
>> vs. tab issues.
>>
>> Signed-off-by: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
>> ---
>> drivers/gpu/drm/i915/i915_reg.h | 72 +++++++++++++++++++++--------------------
>> 1 file changed, 37 insertions(+), 35 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
>> index 6786e94..ed2d150 100644
>> --- a/drivers/gpu/drm/i915/i915_reg.h
>> +++ b/drivers/gpu/drm/i915/i915_reg.h
>> @@ -5364,15 +5364,17 @@ enum skl_disp_power_wells {
>>
>> #define CPU_VGACNTRL 0x41000
>>
>> -#define DIGITAL_PORT_HOTPLUG_CNTRL 0x44030
>
> Maybe add a comment for the fields that are only valid up to IVB?
>
>> -#define DIGITAL_PORTA_HOTPLUG_ENABLE (1 << 4)
>> -#define DIGITAL_PORTA_SHORT_PULSE_2MS (0 << 2)
>> -#define DIGITAL_PORTA_SHORT_PULSE_4_5MS (1 << 2)
>> -#define DIGITAL_PORTA_SHORT_PULSE_6MS (2 << 2)
>> -#define DIGITAL_PORTA_SHORT_PULSE_100MS (3 << 2)
>> -#define DIGITAL_PORTA_NO_DETECT (0 << 0)
>> -#define DIGITAL_PORTA_LONG_PULSE_DETECT_MASK (1 << 1)
>> -#define DIGITAL_PORTA_SHORT_PULSE_DETECT_MASK (1 << 0)
>> +#define DIGITAL_PORT_HOTPLUG_CNTRL 0x44030
>> +#define DIGITAL_PORTA_HOTPLUG_ENABLE (1 << 4)
>> +#define DIGITAL_PORTA_PULSE_DURATION_2ms (0 << 2)
>
> I think I prefer the old SHORT_PULSE_duration names.
>
>> +#define DIGITAL_PORTA_PULSE_DURATION_4_5ms (1 << 2)
>> +#define DIGITAL_PORTA_PULSE_DURATION_6ms (2 << 2)
>> +#define DIGITAL_PORTA_PULSE_DURATION_100ms (3 << 2)
>> +#define DIGITAL_PORTA_PULSE_DURATION_MASK (3 << 2)
>> +#define DIGITAL_PORTA_HOTPLUG_STATUS_MASK (3 << 0)
>> +#define DIGITAL_PORTA_HOTPLUG_NO_DETECT (0 << 0)
>> +#define DIGITAL_PORTA_HOTPLUG_SHORT_DETECT (1 << 0)
>> +#define DIGITAL_PORTA_HOTPLUG_LONG_DETECT (2 << 0)
>>
>> /* refresh rate hardware control */
>> #define RR_HW_CTL 0x45300
>> @@ -6000,45 +6002,45 @@ enum skl_disp_power_wells {
>>
>> /* digital port hotplug */
>> #define PCH_PORT_HOTPLUG 0xc4030 /* SHOTPLUG_CTL */
Another bikeshed: use tabs between the name and the reg address on the
line above?
>> -#define BXT_PORTA_HOTPLUG_ENABLE (1 << 28)
>> -#define BXT_PORTA_HOTPLUG_STATUS_MASK (0x3 << 24)
>> +#define BXT_PORTA_HOTPLUG_ENABLE (1 << 28)
>> +#define BXT_PORTA_HOTPLUG_STATUS_MASK (3 << 24)
>> #define BXT_PORTA_HOTPLUG_NO_DETECT (0 << 24)
>> #define BXT_PORTA_HOTPLUG_SHORT_DETECT (1 << 24)
>> #define BXT_PORTA_HOTPLUG_LONG_DETECT (2 << 24)
>> -#define PORTD_HOTPLUG_ENABLE (1 << 20)
>> -#define PORTD_PULSE_DURATION_2ms (0)
>> -#define PORTD_PULSE_DURATION_4_5ms (1 << 18)
>> -#define PORTD_PULSE_DURATION_6ms (2 << 18)
>> -#define PORTD_PULSE_DURATION_100ms (3 << 18)
>> -#define PORTD_PULSE_DURATION_MASK (3 << 18)
>> -#define PORTD_HOTPLUG_STATUS_MASK (0x3 << 16)
>> +#define PORTD_HOTPLUG_ENABLE (1 << 20)
>> +#define PORTD_PULSE_DURATION_2ms (0 << 18)
>> +#define PORTD_PULSE_DURATION_4_5ms (1 << 18)
>> +#define PORTD_PULSE_DURATION_6ms (2 << 18)
>> +#define PORTD_PULSE_DURATION_100ms (3 << 18)
>> +#define PORTD_PULSE_DURATION_MASK (3 << 18)
>> +#define PORTD_HOTPLUG_STATUS_MASK (3 << 16)
>> #define PORTD_HOTPLUG_NO_DETECT (0 << 16)
>> #define PORTD_HOTPLUG_SHORT_DETECT (1 << 16)
>> #define PORTD_HOTPLUG_LONG_DETECT (2 << 16)
>> -#define PORTC_HOTPLUG_ENABLE (1 << 12)
>> -#define PORTC_PULSE_DURATION_2ms (0)
>> -#define PORTC_PULSE_DURATION_4_5ms (1 << 10)
>> -#define PORTC_PULSE_DURATION_6ms (2 << 10)
>> -#define PORTC_PULSE_DURATION_100ms (3 << 10)
>> -#define PORTC_PULSE_DURATION_MASK (3 << 10)
>> -#define PORTC_HOTPLUG_STATUS_MASK (0x3 << 8)
>> +#define PORTC_HOTPLUG_ENABLE (1 << 12)
>> +#define PORTC_PULSE_DURATION_2ms (0 << 10)
>> +#define PORTC_PULSE_DURATION_4_5ms (1 << 10)
>> +#define PORTC_PULSE_DURATION_6ms (2 << 10)
>> +#define PORTC_PULSE_DURATION_100ms (3 << 10)
>> +#define PORTC_PULSE_DURATION_MASK (3 << 10)
>> +#define PORTC_HOTPLUG_STATUS_MASK (3 << 8)
>> #define PORTC_HOTPLUG_NO_DETECT (0 << 8)
>> #define PORTC_HOTPLUG_SHORT_DETECT (1 << 8)
>> #define PORTC_HOTPLUG_LONG_DETECT (2 << 8)
>> -#define PORTB_HOTPLUG_ENABLE (1 << 4)
>> -#define PORTB_PULSE_DURATION_2ms (0)
>> -#define PORTB_PULSE_DURATION_4_5ms (1 << 2)
>> -#define PORTB_PULSE_DURATION_6ms (2 << 2)
>> -#define PORTB_PULSE_DURATION_100ms (3 << 2)
>> -#define PORTB_PULSE_DURATION_MASK (3 << 2)
>> -#define PORTB_HOTPLUG_STATUS_MASK (0x3 << 0)
>> +#define PORTB_HOTPLUG_ENABLE (1 << 4)
>> +#define PORTB_PULSE_DURATION_2ms (0 << 2)
>> +#define PORTB_PULSE_DURATION_4_5ms (1 << 2)
>> +#define PORTB_PULSE_DURATION_6ms (2 << 2)
>> +#define PORTB_PULSE_DURATION_100ms (3 << 2)
>> +#define PORTB_PULSE_DURATION_MASK (3 << 2)
>> +#define PORTB_HOTPLUG_STATUS_MASK (3 << 0)
>> #define PORTB_HOTPLUG_NO_DETECT (0 << 0)
>> #define PORTB_HOTPLUG_SHORT_DETECT (1 << 0)
>> #define PORTB_HOTPLUG_LONG_DETECT (2 << 0)
>
> Maybe we could make something like "#define
> HOTPLUG_PULSE_DURATION_MASK(port) (2 << (port) + X)".
>
>>
>> -#define PCH_PORT_HOTPLUG2 0xc403C /* SHOTPLUG_CTL2 */
>> -#define PORTE_HOTPLUG_ENABLE (1 << 4)
>> -#define PORTE_HOTPLUG_STATUS_MASK (0x3 << 0)
>> +#define PCH_PORT_HOTPLUG2 0xc403C /* SHOTPLUG_CTL2 SPT+ */
Same for the line above.
>> +#define PORTE_HOTPLUG_ENABLE (1 << 4)
>> +#define PORTE_HOTPLUG_STATUS_MASK (3 << 0)
>
> I was going to give the R-B despite the bikesheds, but this chunk
> doesn't apply. The patch that adds the lines you're fixing here was
> not merged yet. Maybe you could give your review comments to the
> author while it's not merged :)
This now applies to -nightly due to the requirement patch being merged
to drm-intel-next-fixes. So it's a matter of waiting for the
backmerge.
With or without the bikesheds: Reviewed-by: Paulo Zanoni
<paulo.r.zanoni@xxxxxxxxx>
>
>
>> #define PORTE_HOTPLUG_NO_DETECT (0 << 0)
>> #define PORTE_HOTPLUG_SHORT_DETECT (1 << 0)
>> #define PORTE_HOTPLUG_LONG_DETECT (2 << 0)
I wouldn't have complained if you had fixed the GPIO lines immediately
below these :)
>> --
>> 2.4.6
>>
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
>
>
> --
> Paulo Zanoni
--
Paulo Zanoni
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
