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

[AngeloGioacchino Del Regno]

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@xxxxxxxxxxxxx> 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@xxxxxxxxxxxxx>
> > ---
> >   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