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

[Thierry Reding <thierry.reding@kernel.org>]

[-- Attachment #1: Type: text/plain, Size: 7357 bytes --]

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.

> 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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]