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.
> 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.
> #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.
> +
> #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?
> +#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.
> +#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?
Thierry
signature.asc
Description: PGP signature
