Re: [PATCH v2 1/4] drm/mediatek: dsi: Use GENMASK() for register mask definitions

[AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>]

Il 26/12/23 11:46, Fei Shao ha scritto:
> Hi Angelo,
> 
> On Wed, Dec 20, 2023 at 9:57 PM AngeloGioacchino Del Regno
> <angelogioacchino.delregno@collabora.com> wrote:
>>
>> Change magic numerical masks with usage of the GENMASK() macro
>> to improve readability.
>>
>> This commit brings no functional changes.
>>
>> Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
>> ---
>>   drivers/gpu/drm/mediatek/mtk_dsi.c | 46 ++++++++++++++++--------------
>>   1 file changed, 24 insertions(+), 22 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/mediatek/mtk_dsi.c b/drivers/gpu/drm/mediatek/mtk_dsi.c
>> index a2fdfc8ddb15..23d2c5be8dbb 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)
> 
> GENMASK(17, 16)

That was intended, and depends on the SoC - I should effectively advertise that
in the commit description and I will.

As per the datasheets:

MT8192 - DSI_PS_SEL[18:16]
MT8195 - DSI_PS_SEL[19:16]

...on SoCs that don't have those additional bits, the higher ones are reserved
(unused), so it is safe to have this as 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,27 @@
>>   #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 DA_HS_SYNC                     GENMASK(15, 8)
> 
> This is new, so please introduce it in a separate patch if intended.
> 

I wouldn't really be comfortable doing so: this would mean that I'd
have to first write a constant and then fix that in a later patch...

P.S.: Sorry for working again on this one whole month after .... :-)

Regards,
Angelo

> The rest looks good to me.
> 
> Regards,
> Fei
> 
> 
>>
>> +#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 +139,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))
>>
>> --
>> 2.43.0
>>
>>