diff mbox series

[v3,1/7] drm/mediatek: dsi: Use GENMASK() for register mask definitions

Message ID 20240131113434.241929-2-angelogioacchino.delregno@collabora.com
State New
Headers
Series MediaTek DRM - DSI driver cleanups |

Commit Message

AngeloGioacchino Del Regno Jan. 31, 2024, 11:34 a.m. UTC 
  Change magic numerical masks with usage of the GENMASK() macro
to improve readability.

While at it, also fix the DSI_PS_SEL mask to include all bits instead
of just a subset of them.

This commit brings no functional changes.

Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
---
 drivers/gpu/drm/mediatek/mtk_dsi.c | 45 +++++++++++++++---------------
 1 file changed, 23 insertions(+), 22 deletions(-)

Comments

CK Hu (胡俊光) Feb. 6, 2024, 8:57 a.m. UTC | #1 
Hi, Angelo:

On Wed, 2024-01-31 at 12:34 +0100, AngeloGioacchino Del Regno wrote:
> Change magic numerical masks with usage of the GENMASK() macro
> to improve readability.
> 
> While at it, also fix the DSI_PS_SEL mask to include all bits instead
> of just a subset of them.
> 
> This commit brings no functional changes.
> 
> Signed-off-by: AngeloGioacchino Del Regno <
> angelogioacchino.delregno@collabora.com>
> ---
>  drivers/gpu/drm/mediatek/mtk_dsi.c | 45 +++++++++++++++-------------
> --
>  1 file changed, 23 insertions(+), 22 deletions(-)
> 
> diff --git a/drivers/gpu/drm/mediatek/mtk_dsi.c
> b/drivers/gpu/drm/mediatek/mtk_dsi.c
> index a2fdfc8ddb15..3b7392c03b4d 100644
> --- a/drivers/gpu/drm/mediatek/mtk_dsi.c
> +++ b/drivers/gpu/drm/mediatek/mtk_dsi.c
> @@ -58,18 +58,18 @@
>  
>  #define DSI_TXRX_CTRL		0x18
>  #define VC_NUM				BIT(1)
> -#define LANE_NUM			(0xf << 2)
> +#define LANE_NUM			GENMASK(5, 2)
>  #define DIS_EOT				BIT(6)
>  #define NULL_EN				BIT(7)
>  #define TE_FREERUN			BIT(8)
>  #define EXT_TE_EN			BIT(9)
>  #define EXT_TE_EDGE			BIT(10)
> -#define MAX_RTN_SIZE			(0xf << 12)
> +#define MAX_RTN_SIZE			GENMASK(15, 12)
>  #define HSTX_CKLP_EN			BIT(16)
>  
>  #define DSI_PSCTRL		0x1c
> -#define DSI_PS_WC			0x3fff
> -#define DSI_PS_SEL			(3 << 16)
> +#define DSI_PS_WC			GENMASK(14, 0)
> +#define DSI_PS_SEL			GENMASK(19, 16)

The original definition of DSI_PS_WC/DSI_PS_SEL is correct in MT8173.
So both need two definition and let each SoC select its own definition.

Regards,
CK

>  #define PACKED_PS_16BIT_RGB565		(0 << 16)
>  #define LOOSELY_PS_18BIT_RGB666		(1 << 16)
>  #define PACKED_PS_18BIT_RGB666		(2 << 16)
> @@ -109,26 +109,26 @@
>  #define LD0_WAKEUP_EN			BIT(2)
>  
>  #define DSI_PHY_TIMECON0	0x110
> -#define LPX				(0xff << 0)
> -#define HS_PREP				(0xff << 8)
> -#define HS_ZERO				(0xff << 16)
> -#define HS_TRAIL			(0xff << 24)
> +#define LPX				GENMASK(7, 0)
> +#define HS_PREP				GENMASK(15, 8)
> +#define HS_ZERO				GENMASK(23, 16)
> +#define HS_TRAIL			GENMASK(31, 24)
>  
>  #define DSI_PHY_TIMECON1	0x114
> -#define TA_GO				(0xff << 0)
> -#define TA_SURE				(0xff << 8)
> -#define TA_GET				(0xff << 16)
> -#define DA_HS_EXIT			(0xff << 24)
> +#define TA_GO				GENMASK(7, 0)
> +#define TA_SURE				GENMASK(15, 8)
> +#define TA_GET				GENMASK(23, 16)
> +#define DA_HS_EXIT			GENMASK(31, 24)
>  
>  #define DSI_PHY_TIMECON2	0x118
> -#define CONT_DET			(0xff << 0)
> -#define CLK_ZERO			(0xff << 16)
> -#define CLK_TRAIL			(0xff << 24)
> +#define CONT_DET			GENMASK(7, 0)
> +#define CLK_ZERO			GENMASK(23, 16)
> +#define CLK_TRAIL			GENMASK(31, 24)
>  
>  #define DSI_PHY_TIMECON3	0x11c
> -#define CLK_HS_PREP			(0xff << 0)
> -#define CLK_HS_POST			(0xff << 8)
> -#define CLK_HS_EXIT			(0xff << 16)
> +#define CLK_HS_PREP			GENMASK(7, 0)
> +#define CLK_HS_POST			GENMASK(15, 8)
> +#define CLK_HS_EXIT			GENMASK(23, 16)
>  
>  #define DSI_VM_CMD_CON		0x130
>  #define VM_CMD_EN			BIT(0)
> @@ -138,13 +138,14 @@
>  #define FORCE_COMMIT			BIT(0)
>  #define BYPASS_SHADOW			BIT(1)
>  
> -#define CONFIG				(0xff << 0)
> +/* CMDQ related bits */
> +#define CONFIG				GENMASK(7, 0)
>  #define SHORT_PACKET			0
>  #define LONG_PACKET			2
>  #define BTA				BIT(2)
> -#define DATA_ID				(0xff << 8)
> -#define DATA_0				(0xff << 16)
> -#define DATA_1				(0xff << 24)
> +#define DATA_ID				GENMASK(15, 8)
> +#define DATA_0				GENMASK(23, 16)
> +#define DATA_1				GENMASK(31, 24)
>  
>  #define NS_TO_CYCLE(n, c)    ((n) / (c) + (((n) % (c)) ? 1 : 0))
>
AngeloGioacchino Del Regno Feb. 6, 2024, 1:27 p.m. UTC | #2 
Il 06/02/24 09:57, CK Hu (胡俊光) ha scritto:
> Hi, Angelo:
> 
> On Wed, 2024-01-31 at 12:34 +0100, AngeloGioacchino Del Regno wrote:
>> Change magic numerical masks with usage of the GENMASK() macro
>> to improve readability.
>>
>> While at it, also fix the DSI_PS_SEL mask to include all bits instead
>> of just a subset of them.
>>
>> This commit brings no functional changes.
>>
>> Signed-off-by: AngeloGioacchino Del Regno <
>> angelogioacchino.delregno@collabora.com>
>> ---
>>   drivers/gpu/drm/mediatek/mtk_dsi.c | 45 +++++++++++++++-------------
>> --
>>   1 file changed, 23 insertions(+), 22 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/mediatek/mtk_dsi.c
>> b/drivers/gpu/drm/mediatek/mtk_dsi.c
>> index a2fdfc8ddb15..3b7392c03b4d 100644
>> --- a/drivers/gpu/drm/mediatek/mtk_dsi.c
>> +++ b/drivers/gpu/drm/mediatek/mtk_dsi.c
>> @@ -58,18 +58,18 @@
>>   
>>   #define DSI_TXRX_CTRL		0x18
>>   #define VC_NUM				BIT(1)
>> -#define LANE_NUM			(0xf << 2)
>> +#define LANE_NUM			GENMASK(5, 2)
>>   #define DIS_EOT				BIT(6)
>>   #define NULL_EN				BIT(7)
>>   #define TE_FREERUN			BIT(8)
>>   #define EXT_TE_EN			BIT(9)
>>   #define EXT_TE_EDGE			BIT(10)
>> -#define MAX_RTN_SIZE			(0xf << 12)
>> +#define MAX_RTN_SIZE			GENMASK(15, 12)
>>   #define HSTX_CKLP_EN			BIT(16)
>>   
>>   #define DSI_PSCTRL		0x1c
>> -#define DSI_PS_WC			0x3fff
>> -#define DSI_PS_SEL			(3 << 16)
>> +#define DSI_PS_WC			GENMASK(14, 0)
>> +#define DSI_PS_SEL			GENMASK(19, 16)
> 
> The original definition of DSI_PS_WC/DSI_PS_SEL is correct in MT8173.
> So both need two definition and let each SoC select its own definition.
> 

The additional bits are unused on older SoCs and, if set, will be simply ignored;
if we want to prevent setting bits that don't exist on the old ones, that should
be done as a later commit introducing SoC capabilities for those and when the new
capabilities for the new SoCs are introduced anyway.

As of now, this doesn't break anything.

Regards,
Angelo
CK Hu (胡俊光) Feb. 7, 2024, 8:21 a.m. UTC | #3 
Hi, Angelo:

On Tue, 2024-02-06 at 14:27 +0100, AngeloGioacchino Del Regno wrote:
> Il 06/02/24 09:57, CK Hu (胡俊光) ha scritto:
> > Hi, Angelo:
> > 
> > On Wed, 2024-01-31 at 12:34 +0100, AngeloGioacchino Del Regno
> > wrote:
> > > Change magic numerical masks with usage of the GENMASK() macro
> > > to improve readability.
> > > 
> > > While at it, also fix the DSI_PS_SEL mask to include all bits
> > > instead
> > > of just a subset of them.
> > > 
> > > This commit brings no functional changes.
> > > 
> > > Signed-off-by: AngeloGioacchino Del Regno <
> > > angelogioacchino.delregno@collabora.com>
> > > ---
> > >   drivers/gpu/drm/mediatek/mtk_dsi.c | 45 +++++++++++++++------
> > > -------
> > > --
> > >   1 file changed, 23 insertions(+), 22 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/mediatek/mtk_dsi.c
> > > b/drivers/gpu/drm/mediatek/mtk_dsi.c
> > > index a2fdfc8ddb15..3b7392c03b4d 100644
> > > --- a/drivers/gpu/drm/mediatek/mtk_dsi.c
> > > +++ b/drivers/gpu/drm/mediatek/mtk_dsi.c
> > > @@ -58,18 +58,18 @@
> > >   
> > >   #define DSI_TXRX_CTRL		0x18
> > >   #define VC_NUM				BIT(1)
> > > -#define LANE_NUM			(0xf << 2)
> > > +#define LANE_NUM			GENMASK(5, 2)
> > >   #define DIS_EOT				BIT(6)
> > >   #define NULL_EN				BIT(7)
> > >   #define TE_FREERUN			BIT(8)
> > >   #define EXT_TE_EN			BIT(9)
> > >   #define EXT_TE_EDGE			BIT(10)
> > > -#define MAX_RTN_SIZE			(0xf << 12)
> > > +#define MAX_RTN_SIZE			GENMASK(15, 12)
> > >   #define HSTX_CKLP_EN			BIT(16)
> > >   
> > >   #define DSI_PSCTRL		0x1c
> > > -#define DSI_PS_WC			0x3fff
> > > -#define DSI_PS_SEL			(3 << 16)
> > > +#define DSI_PS_WC			GENMASK(14, 0)
> > > +#define DSI_PS_SEL			GENMASK(19, 16)
> > 
> > The original definition of DSI_PS_WC/DSI_PS_SEL is correct in
> > MT8173.
> > So both need two definition and let each SoC select its own
> > definition.
> > 
> 
> The additional bits are unused on older SoCs and, if set, will be
> simply ignored;
> if we want to prevent setting bits that don't exist on the old ones,
> that should
> be done as a later commit introducing SoC capabilities for those and
> when the new
> capabilities for the new SoCs are introduced anyway.
> 
> As of now, this doesn't break anything.

The title of this patch is only to use GENMASK(), but here does more
things. I agree this does not break anything, but I would like to
separate this to an independent patch just for new bits. In your later
patch, DSI_PS_WC is not used any more. So maybe after that patch, you
could define as:

#define DSI_PS_WC_MT8173 GENMASK(13, 0)
#define DSI_PS_WC_MT8xxx GENMASK(14, 0)

DSI_PS_SEL is not used now, so it could also define as:

#define DSI_PS_SEL_MT8137 GENMASK(17, 16)
#define DSI_PS_SEL_MT8xxx GENMASK(19, 16)

And add definition of value 4 ~ 15.

Regards,
CK

> 
> Regards,
> Angelo
> 
>
diff mbox series

Patch

diff --git a/drivers/gpu/drm/mediatek/mtk_dsi.c b/drivers/gpu/drm/mediatek/mtk_dsi.c
index a2fdfc8ddb15..3b7392c03b4d 100644
--- a/drivers/gpu/drm/mediatek/mtk_dsi.c
+++ b/drivers/gpu/drm/mediatek/mtk_dsi.c
@@ -58,18 +58,18 @@ 
 
 #define DSI_TXRX_CTRL		0x18
 #define VC_NUM				BIT(1)
-#define LANE_NUM			(0xf << 2)
+#define LANE_NUM			GENMASK(5, 2)
 #define DIS_EOT				BIT(6)
 #define NULL_EN				BIT(7)
 #define TE_FREERUN			BIT(8)
 #define EXT_TE_EN			BIT(9)
 #define EXT_TE_EDGE			BIT(10)
-#define MAX_RTN_SIZE			(0xf << 12)
+#define MAX_RTN_SIZE			GENMASK(15, 12)
 #define HSTX_CKLP_EN			BIT(16)
 
 #define DSI_PSCTRL		0x1c
-#define DSI_PS_WC			0x3fff
-#define DSI_PS_SEL			(3 << 16)
+#define DSI_PS_WC			GENMASK(14, 0)
+#define DSI_PS_SEL			GENMASK(19, 16)
 #define PACKED_PS_16BIT_RGB565		(0 << 16)
 #define LOOSELY_PS_18BIT_RGB666		(1 << 16)
 #define PACKED_PS_18BIT_RGB666		(2 << 16)
@@ -109,26 +109,26 @@ 
 #define LD0_WAKEUP_EN			BIT(2)
 
 #define DSI_PHY_TIMECON0	0x110
-#define LPX				(0xff << 0)
-#define HS_PREP				(0xff << 8)
-#define HS_ZERO				(0xff << 16)
-#define HS_TRAIL			(0xff << 24)
+#define LPX				GENMASK(7, 0)
+#define HS_PREP				GENMASK(15, 8)
+#define HS_ZERO				GENMASK(23, 16)
+#define HS_TRAIL			GENMASK(31, 24)
 
 #define DSI_PHY_TIMECON1	0x114
-#define TA_GO				(0xff << 0)
-#define TA_SURE				(0xff << 8)
-#define TA_GET				(0xff << 16)
-#define DA_HS_EXIT			(0xff << 24)
+#define TA_GO				GENMASK(7, 0)
+#define TA_SURE				GENMASK(15, 8)
+#define TA_GET				GENMASK(23, 16)
+#define DA_HS_EXIT			GENMASK(31, 24)
 
 #define DSI_PHY_TIMECON2	0x118
-#define CONT_DET			(0xff << 0)
-#define CLK_ZERO			(0xff << 16)
-#define CLK_TRAIL			(0xff << 24)
+#define CONT_DET			GENMASK(7, 0)
+#define CLK_ZERO			GENMASK(23, 16)
+#define CLK_TRAIL			GENMASK(31, 24)
 
 #define DSI_PHY_TIMECON3	0x11c
-#define CLK_HS_PREP			(0xff << 0)
-#define CLK_HS_POST			(0xff << 8)
-#define CLK_HS_EXIT			(0xff << 16)
+#define CLK_HS_PREP			GENMASK(7, 0)
+#define CLK_HS_POST			GENMASK(15, 8)
+#define CLK_HS_EXIT			GENMASK(23, 16)
 
 #define DSI_VM_CMD_CON		0x130
 #define VM_CMD_EN			BIT(0)
@@ -138,13 +138,14 @@ 
 #define FORCE_COMMIT			BIT(0)
 #define BYPASS_SHADOW			BIT(1)
 
-#define CONFIG				(0xff << 0)
+/* CMDQ related bits */
+#define CONFIG				GENMASK(7, 0)
 #define SHORT_PACKET			0
 #define LONG_PACKET			2
 #define BTA				BIT(2)
-#define DATA_ID				(0xff << 8)
-#define DATA_0				(0xff << 16)
-#define DATA_1				(0xff << 24)
+#define DATA_ID				GENMASK(15, 8)
+#define DATA_0				GENMASK(23, 16)
+#define DATA_1				GENMASK(31, 24)
 
 #define NS_TO_CYCLE(n, c)    ((n) / (c) + (((n) % (c)) ? 1 : 0))