пт, 6 лист. 2026 р. о 15:56 Thierry Reding <[email protected]> пише:
>
> 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 <[email protected]>
> > ---
> > 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.
> > 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
