[v8,02/14] drm: exynos: dsi: Properly name HSA/HBP/HFP/HSE bits
diff mbox series

Message ID 20221110183853.3678209-3-jagan@amarulasolutions.com
State New
Headers show
Series
  • drm: bridge: Add Samsung MIPI DSIM bridge
Related show

Commit Message

Jagan Teki Nov. 10, 2022, 6:38 p.m. UTC 
HSA/HBP/HFP/HSE mode bits in Exynos DSI host specify a naming
conversion as 'disable mode bit' due to its bit definition,
0 = Enable and 1 = Disable.

Fix the naming convention of the mode bits.

Signed-off-by: Jagan Teki <jagan@amarulasolutions.com>
---
 drivers/gpu/drm/exynos/exynos_drm_dsi.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

Comments

Marek Vasut Nov. 13, 2022, 12:25 a.m. UTC | #1 
On 11/10/22 19:38, Jagan Teki wrote:
> HSA/HBP/HFP/HSE mode bits in Exynos DSI host specify a naming
> conversion as 'disable mode bit' due to its bit definition,
> 0 = Enable and 1 = Disable.
> 
> Fix the naming convention of the mode bits.
> 
> Signed-off-by: Jagan Teki <jagan@amarulasolutions.com>
> ---
>   drivers/gpu/drm/exynos/exynos_drm_dsi.c | 16 ++++++++--------
>   1 file changed, 8 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/gpu/drm/exynos/exynos_drm_dsi.c b/drivers/gpu/drm/exynos/exynos_drm_dsi.c
> index b5305b145ddb..fce7f0a7e4ee 100644
> --- a/drivers/gpu/drm/exynos/exynos_drm_dsi.c
> +++ b/drivers/gpu/drm/exynos/exynos_drm_dsi.c
> @@ -75,10 +75,10 @@
>   #define DSIM_MAIN_PIX_FORMAT_RGB565	(0x4 << 12)
>   #define DSIM_SUB_VC			(((x) & 0x3) << 16)
>   #define DSIM_MAIN_VC			(((x) & 0x3) << 18)
> -#define DSIM_HSA_MODE			(1 << 20)
> -#define DSIM_HBP_MODE			(1 << 21)
> -#define DSIM_HFP_MODE			(1 << 22)
> -#define DSIM_HSE_MODE			(1 << 23)
> +#define DSIM_HSA_DISABLE		(1 << 20)
> +#define DSIM_HBP_DISABLE		(1 << 21)
> +#define DSIM_HFP_DISABLE		(1 << 22)
> +#define DSIM_HSE_DISABLE		(1 << 23)

Those four bits are called Hxx_DISABLE_MODE in the MX8M{M,N,P}RM at 
least, so keep both suffixes .

A separate patch which turns those bits to BIT() macro would be nice.
Jagan Teki Nov. 14, 2022, 8:27 a.m. UTC | #2 
On Sun, Nov 13, 2022 at 5:55 AM Marek Vasut <marex@denx.de> wrote:
>
> On 11/10/22 19:38, Jagan Teki wrote:
> > HSA/HBP/HFP/HSE mode bits in Exynos DSI host specify a naming
> > conversion as 'disable mode bit' due to its bit definition,
> > 0 = Enable and 1 = Disable.
> >
> > Fix the naming convention of the mode bits.
> >
> > Signed-off-by: Jagan Teki <jagan@amarulasolutions.com>
> > ---
> >   drivers/gpu/drm/exynos/exynos_drm_dsi.c | 16 ++++++++--------
> >   1 file changed, 8 insertions(+), 8 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/exynos/exynos_drm_dsi.c b/drivers/gpu/drm/exynos/exynos_drm_dsi.c
> > index b5305b145ddb..fce7f0a7e4ee 100644
> > --- a/drivers/gpu/drm/exynos/exynos_drm_dsi.c
> > +++ b/drivers/gpu/drm/exynos/exynos_drm_dsi.c
> > @@ -75,10 +75,10 @@
> >   #define DSIM_MAIN_PIX_FORMAT_RGB565 (0x4 << 12)
> >   #define DSIM_SUB_VC                 (((x) & 0x3) << 16)
> >   #define DSIM_MAIN_VC                        (((x) & 0x3) << 18)
> > -#define DSIM_HSA_MODE                        (1 << 20)
> > -#define DSIM_HBP_MODE                        (1 << 21)
> > -#define DSIM_HFP_MODE                        (1 << 22)
> > -#define DSIM_HSE_MODE                        (1 << 23)
> > +#define DSIM_HSA_DISABLE             (1 << 20)
> > +#define DSIM_HBP_DISABLE             (1 << 21)
> > +#define DSIM_HFP_DISABLE             (1 << 22)
> > +#define DSIM_HSE_DISABLE             (1 << 23)
>
> Those four bits are called Hxx_DISABLE_MODE in the MX8M{M,N,P}RM at
> least, so keep both suffixes .

Okay, I will update the suffixes.

>
> A separate patch which turns those bits to BIT() macro would be nice.

I plan to do this conversion for all bits once this basic patchset
merges. I hope it is okay with it.

Jagan.
Frieder Schrempf Dec. 5, 2022, 11:59 a.m. UTC | #3 
On 10.11.22 19:38, Jagan Teki wrote:
> HSA/HBP/HFP/HSE mode bits in Exynos DSI host specify a naming
> conversion as 'disable mode bit' due to its bit definition,
> 0 = Enable and 1 = Disable.
> 
> Fix the naming convention of the mode bits.
> 
> Signed-off-by: Jagan Teki <jagan@amarulasolutions.com>
> ---
>  drivers/gpu/drm/exynos/exynos_drm_dsi.c | 16 ++++++++--------
>  1 file changed, 8 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/gpu/drm/exynos/exynos_drm_dsi.c b/drivers/gpu/drm/exynos/exynos_drm_dsi.c
> index b5305b145ddb..fce7f0a7e4ee 100644
> --- a/drivers/gpu/drm/exynos/exynos_drm_dsi.c
> +++ b/drivers/gpu/drm/exynos/exynos_drm_dsi.c
> @@ -75,10 +75,10 @@
>  #define DSIM_MAIN_PIX_FORMAT_RGB565	(0x4 << 12)
>  #define DSIM_SUB_VC			(((x) & 0x3) << 16)
>  #define DSIM_MAIN_VC			(((x) & 0x3) << 18)
> -#define DSIM_HSA_MODE			(1 << 20)
> -#define DSIM_HBP_MODE			(1 << 21)
> -#define DSIM_HFP_MODE			(1 << 22)
> -#define DSIM_HSE_MODE			(1 << 23)
> +#define DSIM_HSA_DISABLE		(1 << 20)
> +#define DSIM_HBP_DISABLE		(1 << 21)
> +#define DSIM_HFP_DISABLE		(1 << 22)
> +#define DSIM_HSE_DISABLE		(1 << 23)
>  #define DSIM_AUTO_MODE			(1 << 24)
>  #define DSIM_VIDEO_MODE			(1 << 25)
>  #define DSIM_BURST_MODE			(1 << 26)
> @@ -804,13 +804,13 @@ static int exynos_dsi_init_link(struct exynos_dsi *dsi)
>  		if (dsi->mode_flags & MIPI_DSI_MODE_VIDEO_AUTO_VERT)
>  			reg |= DSIM_AUTO_MODE;
>  		if (dsi->mode_flags & MIPI_DSI_MODE_VIDEO_HSE)
> -			reg |= DSIM_HSE_MODE;
> +			reg |= DSIM_HSE_DISABLE;

Please add a comment to explain that the DSIM_HSE_DISABLE bit as named
in the datasheet actually has inverted logic (set = HSE enabled).

>  		if (dsi->mode_flags & MIPI_DSI_MODE_VIDEO_NO_HFP)
> -			reg |= DSIM_HFP_MODE;
> +			reg |= DSIM_HFP_DISABLE;
>  		if (dsi->mode_flags & MIPI_DSI_MODE_VIDEO_NO_HBP)
> -			reg |= DSIM_HBP_MODE;
> +			reg |= DSIM_HBP_DISABLE;
>  		if (dsi->mode_flags & MIPI_DSI_MODE_VIDEO_NO_HSA)
> -			reg |= DSIM_HSA_MODE;
> +			reg |= DSIM_HSA_DISABLE;
>  	}
>  
>  	if (dsi->mode_flags & MIPI_DSI_MODE_NO_EOT_PACKET)
Jagan Teki Dec. 5, 2022, 12:06 p.m. UTC | #4 
On Mon, Dec 5, 2022 at 5:29 PM Frieder Schrempf
<frieder.schrempf@kontron.de> wrote:
>
> On 10.11.22 19:38, Jagan Teki wrote:
> > HSA/HBP/HFP/HSE mode bits in Exynos DSI host specify a naming
> > conversion as 'disable mode bit' due to its bit definition,
> > 0 = Enable and 1 = Disable.
> >
> > Fix the naming convention of the mode bits.
> >
> > Signed-off-by: Jagan Teki <jagan@amarulasolutions.com>
> > ---
> >  drivers/gpu/drm/exynos/exynos_drm_dsi.c | 16 ++++++++--------
> >  1 file changed, 8 insertions(+), 8 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/exynos/exynos_drm_dsi.c b/drivers/gpu/drm/exynos/exynos_drm_dsi.c
> > index b5305b145ddb..fce7f0a7e4ee 100644
> > --- a/drivers/gpu/drm/exynos/exynos_drm_dsi.c
> > +++ b/drivers/gpu/drm/exynos/exynos_drm_dsi.c
> > @@ -75,10 +75,10 @@
> >  #define DSIM_MAIN_PIX_FORMAT_RGB565  (0x4 << 12)
> >  #define DSIM_SUB_VC                  (((x) & 0x3) << 16)
> >  #define DSIM_MAIN_VC                 (((x) & 0x3) << 18)
> > -#define DSIM_HSA_MODE                        (1 << 20)
> > -#define DSIM_HBP_MODE                        (1 << 21)
> > -#define DSIM_HFP_MODE                        (1 << 22)
> > -#define DSIM_HSE_MODE                        (1 << 23)
> > +#define DSIM_HSA_DISABLE             (1 << 20)
> > +#define DSIM_HBP_DISABLE             (1 << 21)
> > +#define DSIM_HFP_DISABLE             (1 << 22)
> > +#define DSIM_HSE_DISABLE             (1 << 23)
> >  #define DSIM_AUTO_MODE                       (1 << 24)
> >  #define DSIM_VIDEO_MODE                      (1 << 25)
> >  #define DSIM_BURST_MODE                      (1 << 26)
> > @@ -804,13 +804,13 @@ static int exynos_dsi_init_link(struct exynos_dsi *dsi)
> >               if (dsi->mode_flags & MIPI_DSI_MODE_VIDEO_AUTO_VERT)
> >                       reg |= DSIM_AUTO_MODE;
> >               if (dsi->mode_flags & MIPI_DSI_MODE_VIDEO_HSE)
> > -                     reg |= DSIM_HSE_MODE;
> > +                     reg |= DSIM_HSE_DISABLE;
>
> Please add a comment to explain that the DSIM_HSE_DISABLE bit as named
> in the datasheet actually has inverted logic (set = HSE enabled).

Yes, I have the V9 series with all relevant fixes and updates -
waiting for the responses on the host init patch 06/14

Jagan.

Patch
diff mbox series 

diff --git a/drivers/gpu/drm/exynos/exynos_drm_dsi.c b/drivers/gpu/drm/exynos/exynos_drm_dsi.c
index b5305b145ddb..fce7f0a7e4ee 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_dsi.c
+++ b/drivers/gpu/drm/exynos/exynos_drm_dsi.c
@@ -75,10 +75,10 @@ 
 #define DSIM_MAIN_PIX_FORMAT_RGB565	(0x4 << 12)
 #define DSIM_SUB_VC			(((x) & 0x3) << 16)
 #define DSIM_MAIN_VC			(((x) & 0x3) << 18)
-#define DSIM_HSA_MODE			(1 << 20)
-#define DSIM_HBP_MODE			(1 << 21)
-#define DSIM_HFP_MODE			(1 << 22)
-#define DSIM_HSE_MODE			(1 << 23)
+#define DSIM_HSA_DISABLE		(1 << 20)
+#define DSIM_HBP_DISABLE		(1 << 21)
+#define DSIM_HFP_DISABLE		(1 << 22)
+#define DSIM_HSE_DISABLE		(1 << 23)
 #define DSIM_AUTO_MODE			(1 << 24)
 #define DSIM_VIDEO_MODE			(1 << 25)
 #define DSIM_BURST_MODE			(1 << 26)
@@ -804,13 +804,13 @@  static int exynos_dsi_init_link(struct exynos_dsi *dsi)
 		if (dsi->mode_flags & MIPI_DSI_MODE_VIDEO_AUTO_VERT)
 			reg |= DSIM_AUTO_MODE;
 		if (dsi->mode_flags & MIPI_DSI_MODE_VIDEO_HSE)
-			reg |= DSIM_HSE_MODE;
+			reg |= DSIM_HSE_DISABLE;
 		if (dsi->mode_flags & MIPI_DSI_MODE_VIDEO_NO_HFP)
-			reg |= DSIM_HFP_MODE;
+			reg |= DSIM_HFP_DISABLE;
 		if (dsi->mode_flags & MIPI_DSI_MODE_VIDEO_NO_HBP)
-			reg |= DSIM_HBP_MODE;
+			reg |= DSIM_HBP_DISABLE;
 		if (dsi->mode_flags & MIPI_DSI_MODE_VIDEO_NO_HSA)
-			reg |= DSIM_HSA_MODE;
+			reg |= DSIM_HSA_DISABLE;
 	}
 
 	if (dsi->mode_flags & MIPI_DSI_MODE_NO_EOT_PACKET)