Re: [PATCH v3 13/29] drm/i915/display: Fix style and conventions for DP AUX regs

[Lucas De Marchi <lucas.demarchi@xxxxxxxxx>]

On Tue, Sep 12, 2023 at 08:32:54AM -0700, Matt Roper wrote:
> On Mon, Sep 11, 2023 at 09:48:21PM -0700, Lucas De Marchi wrote:
> > Fix some whitespace issues for register definitions and keep the defines
> > for DP_AUX_CH_CTL and DP_AUX_CH_DATA in the right place: together with
> > the bit definition.
> >
> > While at it add a TODO entry that those defines shouldn't be using an
> > implicit dev_priv.
> >
> > Signed-off-by: Lucas De Marchi <lucas.demarchi@xxxxxxxxx>
>
> In most of our other register headers we try to indent the values to
> column 49.  It looks like a bunch of the definitions here are only
> indented to column 41.  Not sure if you want to change that for

I think I prefer to keep as is. I don't think we have any convention to
align on a certain column. It's rather about aligning to surrounding
definitions and make a best effort between making the line too long or
with too many line breaks.

> consistency or not.  Either way,
>
> Reviewed-by: Matt Roper <matthew.d.roper@xxxxxxxxx>

thanks
Lucas De Marchi

> > ---
> >  .../gpu/drm/i915/display/intel_dp_aux_regs.h  | 72 +++++++++----------
> >  1 file changed, 35 insertions(+), 37 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/display/intel_dp_aux_regs.h b/drivers/gpu/drm/i915/display/intel_dp_aux_regs.h
> > index 5185345277c7..4503d94115d7 100644
> > --- a/drivers/gpu/drm/i915/display/intel_dp_aux_regs.h
> > +++ b/drivers/gpu/drm/i915/display/intel_dp_aux_regs.h
> > @@ -13,48 +13,28 @@
> >   * packet size supported is 20 bytes in each direction, hence the 5 fixed data
> >   * registers
> >   */
> > -#define _DPA_AUX_CH_CTL		(DISPLAY_MMIO_BASE(dev_priv) + 0x64010)
> > -#define _DPA_AUX_CH_DATA1	(DISPLAY_MMIO_BASE(dev_priv) + 0x64014)
> > -
> > -#define _DPB_AUX_CH_CTL		(DISPLAY_MMIO_BASE(dev_priv) + 0x64110)
> > -#define _DPB_AUX_CH_DATA1	(DISPLAY_MMIO_BASE(dev_priv) + 0x64114)
> > -
> > -#define DP_AUX_CH_CTL(aux_ch)	_MMIO_PORT(aux_ch, _DPA_AUX_CH_CTL, _DPB_AUX_CH_CTL)
> > -#define DP_AUX_CH_DATA(aux_ch, i)	_MMIO(_PORT(aux_ch, _DPA_AUX_CH_DATA1, _DPB_AUX_CH_DATA1) + (i) * 4) /* 5 registers */
> > -
> > -#define _XELPDP_USBC1_AUX_CH_CTL	0x16F210
> > -#define _XELPDP_USBC2_AUX_CH_CTL	0x16F410
> > -#define _XELPDP_USBC3_AUX_CH_CTL	0x16F610
> > -#define _XELPDP_USBC4_AUX_CH_CTL	0x16F810
> > -
> > -#define XELPDP_DP_AUX_CH_CTL(aux_ch)		_MMIO(_PICK(aux_ch, \
> > -						       _DPA_AUX_CH_CTL, \
> > -						       _DPB_AUX_CH_CTL, \
> > -						       0, /* port/aux_ch C is non-existent */ \
> > -						       _XELPDP_USBC1_AUX_CH_CTL, \
> > -						       _XELPDP_USBC2_AUX_CH_CTL, \
> > -						       _XELPDP_USBC3_AUX_CH_CTL, \
> > -						       _XELPDP_USBC4_AUX_CH_CTL))
> > -
> > -#define _XELPDP_USBC1_AUX_CH_DATA1      0x16F214
> > -#define _XELPDP_USBC2_AUX_CH_DATA1      0x16F414
> > -#define _XELPDP_USBC3_AUX_CH_DATA1      0x16F614
> > -#define _XELPDP_USBC4_AUX_CH_DATA1      0x16F814
> > -
> > -#define XELPDP_DP_AUX_CH_DATA(aux_ch, i)	_MMIO(_PICK(aux_ch, \
> > -						       _DPA_AUX_CH_DATA1, \
> > -						       _DPB_AUX_CH_DATA1, \
> > -						       0, /* port/aux_ch C is non-existent */ \
> > -						       _XELPDP_USBC1_AUX_CH_DATA1, \
> > -						       _XELPDP_USBC2_AUX_CH_DATA1, \
> > -						       _XELPDP_USBC3_AUX_CH_DATA1, \
> > -						       _XELPDP_USBC4_AUX_CH_DATA1) + (i) * 4)
> >
> > +/* TODO: Remove implicit dev_priv */
> > +#define _DPA_AUX_CH_CTL			(DISPLAY_MMIO_BASE(dev_priv) + 0x64010)
> > +#define _DPB_AUX_CH_CTL			(DISPLAY_MMIO_BASE(dev_priv) + 0x64110)
> > +#define _XELPDP_USBC1_AUX_CH_CTL	0x16f210
> > +#define _XELPDP_USBC2_AUX_CH_CTL	0x16f410
> > +#define _XELPDP_USBC3_AUX_CH_CTL	0x16f610
> > +#define _XELPDP_USBC4_AUX_CH_CTL	0x16f810
> > +#define DP_AUX_CH_CTL(aux_ch)		_MMIO_PORT(aux_ch, _DPA_AUX_CH_CTL,	\
> > +						   _DPB_AUX_CH_CTL)
> > +#define XELPDP_DP_AUX_CH_CTL(aux_ch)	_MMIO(_PICK(aux_ch, \
> > +						    _DPA_AUX_CH_CTL, \
> > +						    _DPB_AUX_CH_CTL, \
> > +						    0, /* port/aux_ch C is non-existent */ \
> > +						    _XELPDP_USBC1_AUX_CH_CTL, \
> > +						    _XELPDP_USBC2_AUX_CH_CTL, \
> > +						    _XELPDP_USBC3_AUX_CH_CTL, \
> > +						    _XELPDP_USBC4_AUX_CH_CTL))
> >  #define   DP_AUX_CH_CTL_SEND_BUSY		REG_BIT(31)
> >  #define   DP_AUX_CH_CTL_DONE			REG_BIT(30)
> >  #define   DP_AUX_CH_CTL_INTERRUPT		REG_BIT(29)
> >  #define   DP_AUX_CH_CTL_TIME_OUT_ERROR		REG_BIT(28)
> > -
> >  #define   DP_AUX_CH_CTL_TIME_OUT_MASK		REG_GENMASK(27, 26)
> >  #define   DP_AUX_CH_CTL_TIME_OUT_400us		REG_FIELD_PREP(DP_AUX_CH_CTL_TIME_OUT_MASK, 0)
> >  #define   DP_AUX_CH_CTL_TIME_OUT_600us		REG_FIELD_PREP(DP_AUX_CH_CTL_TIME_OUT_MASK, 1)
> > @@ -83,4 +63,22 @@
> >  #define   DP_AUX_CH_CTL_SYNC_PULSE_SKL_MASK	REG_GENMASK(4, 0) /* skl+ */
> >  #define   DP_AUX_CH_CTL_SYNC_PULSE_SKL(c)	REG_FIELD_PREP(DP_AUX_CH_CTL_SYNC_PULSE_SKL_MASK, (c) - 1)
> >
> > +/* TODO: Remove implicit dev_priv */
> > +#define _DPA_AUX_CH_DATA1		(DISPLAY_MMIO_BASE(dev_priv) + 0x64014)
> > +#define _DPB_AUX_CH_DATA1		(DISPLAY_MMIO_BASE(dev_priv) + 0x64114)
> > +#define _XELPDP_USBC1_AUX_CH_DATA1	0x16f214
> > +#define _XELPDP_USBC2_AUX_CH_DATA1	0x16f414
> > +#define _XELPDP_USBC3_AUX_CH_DATA1	0x16f614
> > +#define _XELPDP_USBC4_AUX_CH_DATA1	0x16f814
> > +#define DP_AUX_CH_DATA(aux_ch, i)	_MMIO(_PORT(aux_ch, _DPA_AUX_CH_DATA1,	\
> > +						    _DPB_AUX_CH_DATA1) + (i) * 4) /* 5 registers */
> > +#define XELPDP_DP_AUX_CH_DATA(aux_ch, i)	_MMIO(_PICK(aux_ch, \
> > +							    _DPA_AUX_CH_DATA1, \
> > +							    _DPB_AUX_CH_DATA1, \
> > +							    0, /* port/aux_ch C is non-existent */ \
> > +							    _XELPDP_USBC1_AUX_CH_DATA1, \
> > +							    _XELPDP_USBC2_AUX_CH_DATA1, \
> > +							    _XELPDP_USBC3_AUX_CH_DATA1, \
> > +							    _XELPDP_USBC4_AUX_CH_DATA1) + (i) * 4)
> > +
> >  #endif /* __INTEL_DP_AUX_REGS_H__ */
> > --
> > 2.40.1
>
> --
> Matt Roper
> Graphics Software Engineer
> Linux GPU Platform Enablement
> Intel Corporation