Re: [PATCH v6 04/10] drm/tegra: dc: Refactor and expand header

[Mikko Perttunen <mperttunen@nvidia.com>]

On Friday, November 6, 2026 11:06 PM Svyatoslav Ryhel wrote:
> пт, 6 лист. 2026 р. о 15:56 Thierry Reding <thierry.reding@kernel.org> пише:
> >
> > On Thu, Nov 05, 2026 at 04:05:04PM +0200, Svyatoslav Ryhel wrote:
> > > Refactor and expand existing DC register definitions with additional
> > > fields in preparation for adding the 8-bit CPU interface.
> > >
> > > Signed-off-by: Svyatoslav Ryhel <clamor95@gmail.com>
> > > ---
> > >  drivers/gpu/drm/tegra/dc.c  |  11 +-
> > >  drivers/gpu/drm/tegra/dc.h  | 856 +++++++++++++++++++++---------------
> > >  drivers/gpu/drm/tegra/rgb.c |   8 +-
> > >  3 files changed, 514 insertions(+), 361 deletions(-)
> >
> > This is impossible to review. Please split this up into refactor and
> > expand patches. Each should be a separate change.
> >
> > Also, a lot of the "refactor" is just pointless whitespace changes that
> > are just not necessary. Please keep the changes to a minimum.
> >
> 
> Well, I did what Mikko asked. It is not my problem that this header is
> a hot mess. Initially I just wanted to add fragments needed by dbi.

I found the original a bit difficult to read and thus review because of 
the non-aligned define values, hence asking for some cleanup.

Perhaps it'd be best to just clean up the whitespace for those register 
/ define blocks where the patch is adding or modifying values.

> 
> > > diff --git a/drivers/gpu/drm/tegra/dc.c b/drivers/gpu/drm/tegra/dc.c
> > [...]
> > > @@ -2384,7 +2384,8 @@ static void tegra_crtc_atomic_enable(struct drm_crtc *crtc,
> > >
> > >       if (dc->rgb) {
> > >               /* XXX: parameterize? */
> > > -             value = SC0_H_QUALIFIER_NONE | SC1_H_QUALIFIER_NONE;
> > > +             value = SC0_H_QUALIFIER(SC_H_QUALIFIER_NONE) |
> > > +                     SC1_H_QUALIFIER(SC_H_QUALIFIER_NONE);
> >
> > This type of change seems entirely useless to me. It's neither more
> > readable nor shorter.
> >
> > [...]
> > > diff --git a/drivers/gpu/drm/tegra/dc.h b/drivers/gpu/drm/tegra/dc.h
> > > index 0cb0515968b35..2b87bb675d4cf 100644
> > > --- a/drivers/gpu/drm/tegra/dc.h
> > > +++ b/drivers/gpu/drm/tegra/dc.h
> > > @@ -178,9 +178,11 @@ int tegra_dc_rgb_init(struct drm_device *drm, struct tegra_dc *dc);
> > >  int tegra_dc_rgb_exit(struct tegra_dc *dc);
> > >
> > >  #define DC_CMD_GENERAL_INCR_SYNCPT           0x000
> > > +
> > >  #define DC_CMD_GENERAL_INCR_SYNCPT_CNTRL     0x001
> > > -#define  SYNCPT_CNTRL_NO_STALL   (1 << 8)
> > > -#define  SYNCPT_CNTRL_SOFT_RESET (1 << 0)
> > > +#define  SYNCPT_CNTRL_NO_STALL                       BIT(8)
> > > +#define  SYNCPT_CNTRL_SOFT_RESET             BIT(0)
> >
> > The whitespace changes here are completely arbitrary, it's changing one
> > preference for another. You're actually changing the contents in this
> > case, so it's not that bad. Still unnecessary, though.
> >
> > > -#define DISP_CTRL_MODE_STOP (0 << 5)
> > > -#define DISP_CTRL_MODE_C_DISPLAY (1 << 5)
> > > -#define DISP_CTRL_MODE_NC_DISPLAY (2 << 5)
> > > -#define DISP_CTRL_MODE_MASK (3 << 5)
> > > +#define  DISP_CTRL_MODE_STOP                 (0 << 5)
> > > +#define  DISP_CTRL_MODE_C_DISPLAY            (1 << 5)
> > > +#define  DISP_CTRL_MODE_NC_DISPLAY           (2 << 5)
> > > +#define  DISP_CTRL_MODE_MASK                 (3 << 5)
> >
> > These are purely whitespace changes. Please don't do that.
> >
> 
> This separates register content for register definitions
> 
> > >  #define DC_COM_CRC_CONTROL                   0x300
> > > -#define  DC_COM_CRC_CONTROL_ALWAYS (1 << 3)
> > > -#define  DC_COM_CRC_CONTROL_FULL_FRAME  (0 << 2)
> > > -#define  DC_COM_CRC_CONTROL_ACTIVE_DATA (1 << 2)
> > > -#define  DC_COM_CRC_CONTROL_WAIT (1 << 1)
> > > -#define  DC_COM_CRC_CONTROL_ENABLE (1 << 0)
> > > +#define  DC_COM_CRC_CONTROL_ALWAYS           BIT(3)
> > > +#define  DC_COM_CRC_CONTROL_FULL_FRAME               (0 << 2)
> > > +#define  DC_COM_CRC_CONTROL_ACTIVE_DATA              BIT(2)
> > > +#define  DC_COM_CRC_CONTROL_WAIT             BIT(1)
> > > +#define  DC_COM_CRC_CONTROL_ENABLE           BIT(0)
> >
> > FULL_FRAME and ACTIVE_DATA were previously consistent, now it just looks
> > weird. Either use proper GENMASK() definitions where applicable, along
> > with modifications in the driver to use FIELD_PREP() and FIELD_GET(), or
> > just don't change it at all.
> >
> 
> This one seems legit, I must have missed that.
> 
> > > +
> > >  #define DC_COM_CRC_CHECKSUM                  0x301
> > > -#define DC_COM_PIN_OUTPUT_ENABLE(x) (0x302 + (x))
> > > -#define DC_COM_PIN_OUTPUT_POLARITY(x) (0x306 + (x))
> > > -#define LVS_OUTPUT_POLARITY_LOW (1 << 28)
> > > -#define LHS_OUTPUT_POLARITY_LOW (1 << 30)
> > > -#define DC_COM_PIN_OUTPUT_DATA(x) (0x30a + (x))
> > > -#define DC_COM_PIN_INPUT_ENABLE(x) (0x30e + (x))
> > > -#define DC_COM_PIN_INPUT_DATA(x) (0x312 + (x))
> > > -#define DC_COM_PIN_OUTPUT_SELECT(x) (0x314 + (x))
> > > +
> > > +/* Defines in PIN_OUTPUT/INPUT_* describe pin position, not state */
> >
> > This comment is completely confusing to me. The registers themselves
> > define the state and bit positions within them seem to indicate the pin
> > positions.
> >
> > > +#define DC_COM_PIN_OUTPUT_ENABLE(x)          (0x302 + (x))
> > > +#define DC_COM_PIN_OUTPUT_POLARITY(x)                (0x306 + (x))
> > > +#define DC_COM_PIN_OUTPUT_DATA(x)            (0x30a + (x))
> > > +#define  PIN_UNMASK(x)                               BIT((x) + 1)
> >
> > Given that pin positions are in steps of two for all registers, does
> > that PIN_UNMASK apply to all of them or just PIN_OUTPUT_DATA?
> >
> 
> Check TRM
> 
> > > +#define DC_COM_PIN_INPUT_ENABLE(x)           (0x30e + (x))
> > > +
> > [...]
> > > +/* DC_COM_PIN_OUTPUT/INPUT_*_3 */
> > > +#define PIN_LSCK                             0
> > > +#define PIN_LSDA                             2
> > > +#define PIN_LCSN                             4
> > > +#define PIN_LDC                                      6
> > > +#define PIN_LSPI                             8
> > > +#define PIN_LSDI                             10
> > > +
> > > +#define DC_COM_PIN_INPUT_DATA(x)             (0x312 + (x))
> >
> > It's not clear that PIN_INPUT_DATA_* can take any of those PIN_*
> > defines. Given that there are only two PIN_INPUT_DATA registers that's
> > probably not true, so I think it'd be good to somehow specify how that
> > works here.
> >
> 
> I do not need input data, hence this field is not populated
> 
> > > +#define DC_COM_PIN_OUTPUT_SELECT(x)          (0x314 + (x))
> > > +#define  PIN_OUTPUT_SELECT_MASK(x)           (0x7 << (x))
> > > +#define  PIN_OUTPUT_SELECT_PRIM(x)           (0 << (x))
> > > +#define  PIN_OUTPUT_SELECT_OUT(x)            (1 << (x))
> > > +#define  PIN_OUTPUT_SELECT_F2(x)             (2 << (x))
> > > +#define  PIN_OUTPUT_SELECT_F3(x)             (3 << (x))
> > > +#define  PIN_OUTPUT_SELECT_F4(x)             (4 << (x))
> > > +#define  PIN_OUTPUT_SELECT_F5(x)             (5 << (x))
> > > +#define  PIN_OUTPUT_SELECT_F6(x)             (6 << (x))
> > > +#define  PIN_OUTPUT_SELECT_F7(x)             (7 << (x))
> > > +
> > > +/* DC_COM_PIN_OUTPUT_SELECT0 */
> > > +#define PIN_OUTPUT_SELECT_LD0                        0
> > > +#define PIN_OUTPUT_SELECT_LD1                        4
> > > +#define PIN_OUTPUT_SELECT_LD2                        8
> > > +#define PIN_OUTPUT_SELECT_LD3                        12
> > > +#define PIN_OUTPUT_SELECT_LD4                        16
> > > +#define PIN_OUTPUT_SELECT_LD5                        20
> > > +#define PIN_OUTPUT_SELECT_LD6                        24
> > > +#define PIN_OUTPUT_SELECT_LD7                        28
> > > +
> > > +/* DC_COM_PIN_OUTPUT_SELECT1 */
> > > +#define PIN_OUTPUT_SELECT_LD8                        0
> > > +#define PIN_OUTPUT_SELECT_LD9                        4
> > > +#define PIN_OUTPUT_SELECT_LD10                       8
> > > +#define PIN_OUTPUT_SELECT_LD11                       12
> > > +#define PIN_OUTPUT_SELECT_LD12                       16
> > > +#define PIN_OUTPUT_SELECT_LD13                       20
> > > +#define PIN_OUTPUT_SELECT_LD14                       24
> > > +#define PIN_OUTPUT_SELECT_LD15                       28
> > > +
> > > +/* DC_COM_PIN_OUTPUT_SELECT2 */
> > > +#define PIN_OUTPUT_SELECT_LD16                       0
> > > +#define PIN_OUTPUT_SELECT_LD17                       4
> > > +
> > > +/* DC_COM_PIN_OUTPUT_SELECT3 */
> > > +#define PIN_OUTPUT_SELECT_LPW0                       0
> > > +#define PIN_OUTPUT_SELECT_LPW1                       4
> > > +#define PIN_OUTPUT_SELECT_LPW2                       8
> > > +#define PIN_OUTPUT_SELECT_LSC0                       16
> > > +#define PIN_OUTPUT_SELECT_LSC1                       20
> > > +#define PIN_OUTPUT_SELECT_LVS                        24
> > > +#define PIN_OUTPUT_SELECT_LHS                        28
> > > +
> > > +/* DC_COM_PIN_OUTPUT_SELECT4 */
> > > +#define PIN_OUTPUT_SELECT_LHP0                       0
> > > +#define PIN_OUTPUT_SELECT_LHP1                       4
> > > +#define PIN_OUTPUT_SELECT_LHP2                       8
> > > +#define PIN_OUTPUT_SELECT_LVP0                       16
> > > +#define PIN_OUTPUT_SELECT_LVP1                       20
> > > +
> > > +/* DC_COM_PIN_OUTPUT_SELECT5 */
> > > +#define PIN_OUTPUT_SELECT_LM0                        0
> > > +#define PIN_OUTPUT_SELECT_LM1                        4
> > > +#define PIN_OUTPUT_SELECT_LDI                        8
> > > +#define PIN_OUTPUT_SELECT_LPP                        12
> > > +
> > > +/* DC_COM_PIN_OUTPUT_SELECT6 */
> > > +#define PIN_OUTPUT_SELECT_LSCK                       0
> > > +#define PIN_OUTPUT_SELECT_LSDA                       4
> > > +#define PIN_OUTPUT_SELECT_LCSN                       8
> > > +#define PIN_OUTPUT_SELECT_LDC                        12
> > > +#define PIN_OUTPUT_SELECT_LSPI                       16
> > > +#define PIN_OUTPUT_SELECT_LSDI                       20
> > >
> > >  #define DC_COM_PIN_MISC_CONTROL                      0x31b
> >
> > If there's only one PIN_MISC_CONTROL register, does that take these same
> > PIN_* values as bit indices?
> >
> 
> Check TRM
> 
> > Thierry