diff mbox series

[v2] drm/panel: nt35510: Make new commands optional

Message ID 20240908-fix-nt35510-v2-1-d4834b9cdb9b@linaro.org (mailing list archive)
State New, archived
Headers show
Series [v2] drm/panel: nt35510: Make new commands optional | expand

Commit Message

Linus Walleij Sept. 8, 2024, 9:50 p.m. UTC 
The commit introducing the Frida display started to write the
SETVCMOFF registers unconditionally, and some (not all!) Hydis
display seem to be affected by ghosting after the commit.

Make SETVCMOFF optional and only send these commands on the
Frida display for now.

Reported-by: Stefan Hansson <newbyte@postmarketos.org>
Fixes: 219a1f49094f ("drm/panel: nt35510: support FRIDA FRD400B25025-A-CTK")
Acked-by: Jessica Zhang <quic_jesszhan@quicinc.com>
Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
Changes in v2:
- After Stefan's testing conclude that we only need to make
  SETVCMOFF optional.
- Link to v1: https://lore.kernel.org/r/20240906-fix-nt35510-v1-1-1971f3af7dda@linaro.org
---
 drivers/gpu/drm/panel/panel-novatek-nt35510.c | 18 ++++++++++++------
 1 file changed, 12 insertions(+), 6 deletions(-)


---
base-commit: 8400291e289ee6b2bf9779ff1c83a291501f017b
change-id: 20240906-fix-nt35510-a8ec6e47e036

Best regards,

Comments

Stefan Hansson Sept. 9, 2024, 7:31 a.m. UTC | #1 
Hi again!

On 2024-09-08 23:50, Linus Walleij wrote:
> The commit introducing the Frida display started to write the
> SETVCMOFF registers unconditionally, and some (not all!) Hydis
> display seem to be affected by ghosting after the commit.
> 
> Make SETVCMOFF optional and only send these commands on the
> Frida display for now.
> 
> Reported-by: Stefan Hansson <newbyte@postmarketos.org>
> Fixes: 219a1f49094f ("drm/panel: nt35510: support FRIDA FRD400B25025-A-CTK")
> Acked-by: Jessica Zhang <quic_jesszhan@quicinc.com>
> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
> ---
> Changes in v2:
> - After Stefan's testing conclude that we only need to make
>    SETVCMOFF optional.
> - Link to v1: https://lore.kernel.org/r/20240906-fix-nt35510-v1-1-1971f3af7dda@linaro.org
> ---
>   drivers/gpu/drm/panel/panel-novatek-nt35510.c | 18 ++++++++++++------
>   1 file changed, 12 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/gpu/drm/panel/panel-novatek-nt35510.c b/drivers/gpu/drm/panel/panel-novatek-nt35510.c
> index d3bfdfc9cff6..a3460ed38cc4 100644
> --- a/drivers/gpu/drm/panel/panel-novatek-nt35510.c
> +++ b/drivers/gpu/drm/panel/panel-novatek-nt35510.c
> @@ -38,6 +38,7 @@
>   
>   #define NT35510_CMD_CORRECT_GAMMA BIT(0)
>   #define NT35510_CMD_CONTROL_DISPLAY BIT(1)
> +#define NT35510_CMD_SETVCMOFF BIT(2)
>   
>   #define MCS_CMD_MAUCCTR		0xF0 /* Manufacturer command enable */
>   #define MCS_CMD_READ_ID1	0xDA
> @@ -675,16 +676,19 @@ static int nt35510_setup_power(struct nt35510 *nt)
>   				nt->conf->bt2ctr);
>   	if (ret)
>   		return ret;
> +
>   	ret = nt35510_send_long(nt, dsi, NT35510_P1_SETVCL,
>   				NT35510_P1_VCL_LEN,
>   				nt->conf->vcl);
>   	if (ret)
>   		return ret;
> +
>   	ret = nt35510_send_long(nt, dsi, NT35510_P1_BT3CTR,
>   				NT35510_P1_BT3CTR_LEN,
>   				nt->conf->bt3ctr);
>   	if (ret)
>   		return ret;
> +
>   	ret = nt35510_send_long(nt, dsi, NT35510_P1_SETVGH,
>   				NT35510_P1_VGH_LEN,
>   				nt->conf->vgh);
> @@ -721,11 +725,13 @@ static int nt35510_setup_power(struct nt35510 *nt)
>   	if (ret)
>   		return ret;
>   
> -	ret = nt35510_send_long(nt, dsi, NT35510_P1_SETVCMOFF,
> -				NT35510_P1_VCMOFF_LEN,
> -				nt->conf->vcmoff);
> -	if (ret)
> -		return ret;
> +	if (nt->conf->cmds & NT35510_CMD_SETVCMOFF) {
> +		ret = nt35510_send_long(nt, dsi, NT35510_P1_SETVCMOFF,
> +					NT35510_P1_VCMOFF_LEN,
> +					nt->conf->vcmoff);
> +		if (ret)
> +			return ret;
> +	}
>   
>   	/* Typically 10 ms */
>   	usleep_range(10000, 20000);
> @@ -1319,7 +1325,7 @@ static const struct nt35510_config nt35510_frida_frd400b25025 = {
>   	},
>   	.mode_flags = MIPI_DSI_MODE_VIDEO | MIPI_DSI_MODE_VIDEO_BURST |
>   			MIPI_DSI_MODE_LPM,
> -	.cmds = NT35510_CMD_CONTROL_DISPLAY,
> +	.cmds = NT35510_CMD_CONTROL_DISPLAY | NT35510_CMD_SETVCMOFF,
>   	/* 0x03: AVDD = 6.2V */
>   	.avdd = { 0x03, 0x03, 0x03 },
>   	/* 0x46: PCK = 2 x Hsync, BTP = 2.5 x VDDB */
> 
> ---
> base-commit: 8400291e289ee6b2bf9779ff1c83a291501f017b
> change-id: 20240906-fix-nt35510-a8ec6e47e036
> 
> Best regards,

It is

Tested-by: Stefan Hansson <newbyte@postmarketos.org>
Dmitry Baryshkov Sept. 22, 2024, 2:47 p.m. UTC | #2 
On Sun, Sep 08, 2024 at 11:50:30PM GMT, Linus Walleij wrote:
> The commit introducing the Frida display started to write the
> SETVCMOFF registers unconditionally, and some (not all!) Hydis
> display seem to be affected by ghosting after the commit.
> 
> Make SETVCMOFF optional and only send these commands on the
> Frida display for now.
> 
> Reported-by: Stefan Hansson <newbyte@postmarketos.org>
> Fixes: 219a1f49094f ("drm/panel: nt35510: support FRIDA FRD400B25025-A-CTK")
> Acked-by: Jessica Zhang <quic_jesszhan@quicinc.com>
> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
> ---
> Changes in v2:
> - After Stefan's testing conclude that we only need to make
>   SETVCMOFF optional.
> - Link to v1: https://lore.kernel.org/r/20240906-fix-nt35510-v1-1-1971f3af7dda@linaro.org
> ---
>  drivers/gpu/drm/panel/panel-novatek-nt35510.c | 18 ++++++++++++------
>  1 file changed, 12 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/gpu/drm/panel/panel-novatek-nt35510.c b/drivers/gpu/drm/panel/panel-novatek-nt35510.c
> index d3bfdfc9cff6..a3460ed38cc4 100644
> --- a/drivers/gpu/drm/panel/panel-novatek-nt35510.c
> +++ b/drivers/gpu/drm/panel/panel-novatek-nt35510.c
> @@ -38,6 +38,7 @@
>  
>  #define NT35510_CMD_CORRECT_GAMMA BIT(0)
>  #define NT35510_CMD_CONTROL_DISPLAY BIT(1)
> +#define NT35510_CMD_SETVCMOFF BIT(2)
>  
>  #define MCS_CMD_MAUCCTR		0xF0 /* Manufacturer command enable */
>  #define MCS_CMD_READ_ID1	0xDA
> @@ -675,16 +676,19 @@ static int nt35510_setup_power(struct nt35510 *nt)
>  				nt->conf->bt2ctr);
>  	if (ret)
>  		return ret;
> +
>  	ret = nt35510_send_long(nt, dsi, NT35510_P1_SETVCL,
>  				NT35510_P1_VCL_LEN,
>  				nt->conf->vcl);
>  	if (ret)
>  		return ret;
> +
>  	ret = nt35510_send_long(nt, dsi, NT35510_P1_BT3CTR,
>  				NT35510_P1_BT3CTR_LEN,
>  				nt->conf->bt3ctr);
>  	if (ret)
>  		return ret;
> +
>  	ret = nt35510_send_long(nt, dsi, NT35510_P1_SETVGH,
>  				NT35510_P1_VGH_LEN,
>  				nt->conf->vgh);

I think this part is unrelted and should go in as a separate commit.
Other than that LGTM.

> @@ -721,11 +725,13 @@ static int nt35510_setup_power(struct nt35510 *nt)
>  	if (ret)
>  		return ret;
>  
> -	ret = nt35510_send_long(nt, dsi, NT35510_P1_SETVCMOFF,
> -				NT35510_P1_VCMOFF_LEN,
> -				nt->conf->vcmoff);
> -	if (ret)
> -		return ret;
> +	if (nt->conf->cmds & NT35510_CMD_SETVCMOFF) {
> +		ret = nt35510_send_long(nt, dsi, NT35510_P1_SETVCMOFF,
> +					NT35510_P1_VCMOFF_LEN,
> +					nt->conf->vcmoff);
> +		if (ret)
> +			return ret;
> +	}
>  
>  	/* Typically 10 ms */
>  	usleep_range(10000, 20000);
> @@ -1319,7 +1325,7 @@ static const struct nt35510_config nt35510_frida_frd400b25025 = {
>  	},
>  	.mode_flags = MIPI_DSI_MODE_VIDEO | MIPI_DSI_MODE_VIDEO_BURST |
>  			MIPI_DSI_MODE_LPM,
> -	.cmds = NT35510_CMD_CONTROL_DISPLAY,
> +	.cmds = NT35510_CMD_CONTROL_DISPLAY | NT35510_CMD_SETVCMOFF,
>  	/* 0x03: AVDD = 6.2V */
>  	.avdd = { 0x03, 0x03, 0x03 },
>  	/* 0x46: PCK = 2 x Hsync, BTP = 2.5 x VDDB */
> 
> ---
> base-commit: 8400291e289ee6b2bf9779ff1c83a291501f017b
> change-id: 20240906-fix-nt35510-a8ec6e47e036
> 
> Best regards,
> -- 
> Linus Walleij <linus.walleij@linaro.org>
>
Linus Walleij Sept. 22, 2024, 8:18 p.m. UTC | #3 
On Sun, Sep 22, 2024 at 4:47 PM Dmitry Baryshkov
<dmitry.baryshkov@linaro.org> wrote:

> >       if (ret)
> >               return ret;
> > +
> >       ret = nt35510_send_long(nt, dsi, NT35510_P1_SETVCL,
> >                               NT35510_P1_VCL_LEN,
> >                               nt->conf->vcl);
> >       if (ret)
> >               return ret;
> > +
> >       ret = nt35510_send_long(nt, dsi, NT35510_P1_BT3CTR,
> >                               NT35510_P1_BT3CTR_LEN,
> >                               nt->conf->bt3ctr);
> >       if (ret)
> >               return ret;
> > +
> >       ret = nt35510_send_long(nt, dsi, NT35510_P1_SETVGH,
> >                               NT35510_P1_VGH_LEN,
> >                               nt->conf->vgh);
>
> I think this part is unrelted and should go in as a separate commit.
> Other than that LGTM.

Ooops residuals from an earlier patch iteration.

I fixed it up while applying the patch.

Thanks!
Linus Walleij
diff mbox series

Patch

diff --git a/drivers/gpu/drm/panel/panel-novatek-nt35510.c b/drivers/gpu/drm/panel/panel-novatek-nt35510.c
index d3bfdfc9cff6..a3460ed38cc4 100644
--- a/drivers/gpu/drm/panel/panel-novatek-nt35510.c
+++ b/drivers/gpu/drm/panel/panel-novatek-nt35510.c
@@ -38,6 +38,7 @@ 
 
 #define NT35510_CMD_CORRECT_GAMMA BIT(0)
 #define NT35510_CMD_CONTROL_DISPLAY BIT(1)
+#define NT35510_CMD_SETVCMOFF BIT(2)
 
 #define MCS_CMD_MAUCCTR		0xF0 /* Manufacturer command enable */
 #define MCS_CMD_READ_ID1	0xDA
@@ -675,16 +676,19 @@  static int nt35510_setup_power(struct nt35510 *nt)
 				nt->conf->bt2ctr);
 	if (ret)
 		return ret;
+
 	ret = nt35510_send_long(nt, dsi, NT35510_P1_SETVCL,
 				NT35510_P1_VCL_LEN,
 				nt->conf->vcl);
 	if (ret)
 		return ret;
+
 	ret = nt35510_send_long(nt, dsi, NT35510_P1_BT3CTR,
 				NT35510_P1_BT3CTR_LEN,
 				nt->conf->bt3ctr);
 	if (ret)
 		return ret;
+
 	ret = nt35510_send_long(nt, dsi, NT35510_P1_SETVGH,
 				NT35510_P1_VGH_LEN,
 				nt->conf->vgh);
@@ -721,11 +725,13 @@  static int nt35510_setup_power(struct nt35510 *nt)
 	if (ret)
 		return ret;
 
-	ret = nt35510_send_long(nt, dsi, NT35510_P1_SETVCMOFF,
-				NT35510_P1_VCMOFF_LEN,
-				nt->conf->vcmoff);
-	if (ret)
-		return ret;
+	if (nt->conf->cmds & NT35510_CMD_SETVCMOFF) {
+		ret = nt35510_send_long(nt, dsi, NT35510_P1_SETVCMOFF,
+					NT35510_P1_VCMOFF_LEN,
+					nt->conf->vcmoff);
+		if (ret)
+			return ret;
+	}
 
 	/* Typically 10 ms */
 	usleep_range(10000, 20000);
@@ -1319,7 +1325,7 @@  static const struct nt35510_config nt35510_frida_frd400b25025 = {
 	},
 	.mode_flags = MIPI_DSI_MODE_VIDEO | MIPI_DSI_MODE_VIDEO_BURST |
 			MIPI_DSI_MODE_LPM,
-	.cmds = NT35510_CMD_CONTROL_DISPLAY,
+	.cmds = NT35510_CMD_CONTROL_DISPLAY | NT35510_CMD_SETVCMOFF,
 	/* 0x03: AVDD = 6.2V */
 	.avdd = { 0x03, 0x03, 0x03 },
 	/* 0x46: PCK = 2 x Hsync, BTP = 2.5 x VDDB */