Message-ID: <CAD=FV=UGNN68Fu4kJQQ8jO+fYP4qVJYmL0quxa_=Y5GEtS-jMQ@mail.gmail.com>
Date: Fri, 25 Jul 2025 14:17:14 -0700
From: Doug Anderson <dianders@...omium.org>
To: Brigham Campbell <me@...ghamcampbell.com>
Cc: maarten.lankhorst@...ux.intel.com, mripard@...nel.org, tzimmermann@...e.de, 
	airlied@...il.com, simona@...ll.ch, linus.walleij@...aro.org, 
	neil.armstrong@...aro.org, jessica.zhang@....qualcomm.com, sam@...nborg.org, 
	skhan@...uxfoundation.org, linux-kernel-mentees@...ts.linux.dev, 
	dri-devel@...ts.freedesktop.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 2/2] drm/panel: novatek-nt35560: Fix bug and clean up

Hi,

On Thu, Jul 24, 2025 at 1:23 PM Brigham Campbell <me@...ghamcampbell.com> wrote:
>
> Fix bug in nt35560_set_brightness() which causes the function to
> erroneously report an error. mipi_dsi_dcs_write() returns either a
> negative value when an error occurred or a positive number of bytes
> written when no error occurred. The buggy code reports and error under
> either condition.

My personal preference would be to code up the fix itself (without the
multi transition) as patch #1. That will make everyone's lives easier
with stable backports. You'll touch the same code twice in your
series, but it will keep it cleaner...


> Update driver to use the "multi" variants of MIPI functions which
> facilitate improved error handling and cleaner driver code.
>
> Fixes: 7835ed6a9e86 ("drm/panel-sony-acx424akp: Modernize backlight handling")
> Signed-off-by: Brigham Campbell <me@...ghamcampbell.com>
> ---
>
> The usage of the u8 array, mipi_buf_out, in nt35560_set_brightness() may
> be a little curious. It's useful here because pwm_ratio and pwm_div
> aren't constant, therefore we must store them in a buffer at runtime.
>
> Using mipi_dsi_dcs_write_{seq,buffer}_multi() in place of
> mipi_dsi_dcs_write() gives the added benefit that kmalloc() isn't used
> to write mipi commands.

Ah, this makes sense. We've seen this before, but I keep forgetting
about it. Thanks for mentioning it. I wonder if it makes sense to have
variants of mipi_dsi_generic_write_seq_multi() and
mipi_dsi_dcs_write_seq_multi() that take non-const data. The only
difference would be that the array they declare on the stack would be
a "const" array instead of a "static const" array...


> The jdi-lpm102a188a driver's unprepare() function will ignore errors
> reported by mipi_dsi_dcs_{set_display_off,enter_sleep_mode}. This
> driver, however, will fail to unprepare the panel if either function
> returns an error. The behavior of the jdi-lpm102a188a panel makes more
> sense to me, but I strongly prefer not to change the behavior of this
> driver without personally having access to hardware to test.

Makes sense to me.


> @@ -176,62 +173,28 @@ static int nt35560_set_brightness(struct backlight_device *bl)
>
>         /* Set up PWM dutycycle ONE byte (differs from the standard) */
>         dev_dbg(nt->dev, "calculated duty cycle %02x\n", pwm_ratio);
> -       ret = mipi_dsi_dcs_write(dsi, MIPI_DCS_SET_DISPLAY_BRIGHTNESS,
> -                                &pwm_ratio, 1);
> -       if (ret < 0) {
> -               dev_err(nt->dev, "failed to set display PWM ratio (%d)\n", ret);
> -               return ret;
> -       }
>
> -       /*
> -        * Sequence to write PWMDIV:
> -        *      address         data
> -        *      0xF3            0xAA   CMD2 Unlock
> -        *      0x00            0x01   Enter CMD2 page 0
> -        *      0X7D            0x01   No reload MTP of CMD2 P1
> -        *      0x22            PWMDIV
> -        *      0x7F            0xAA   CMD2 page 1 lock
> -        */

The above comment was useful. Can you keep it?


> @@ -278,16 +232,18 @@ static int nt35560_read_id(struct nt35560 *nt)
>         case DISPLAY_SONY_ACX424AKP_ID2:
>         case DISPLAY_SONY_ACX424AKP_ID3:
>         case DISPLAY_SONY_ACX424AKP_ID4:
> -               dev_info(nt->dev, "MTP vendor: %02x, version: %02x, panel: %02x\n",
> +               dev_info(&dev,
> +                        "MTP vendor: %02x, version: %02x, panel: %02x\n",
>                          vendor, version, panel);
>                 break;
>         default:
> -               dev_info(nt->dev, "unknown vendor: %02x, version: %02x, panel: %02x\n",
> +               dev_info(&dev,
> +                        "unknown vendor: %02x, version: %02x, panel: %02x\n",
>                          vendor, version, panel);
>                 break;
>         }
>
> -       return 0;
> +       return;
>  }

nit: no need for explicit return here, right?


> @@ -322,92 +278,56 @@ static void nt35560_power_off(struct nt35560 *nt)
>  static int nt35560_prepare(struct drm_panel *panel)
>  {
>         struct nt35560 *nt = panel_to_nt35560(panel);
> -       struct mipi_dsi_device *dsi = to_mipi_dsi_device(nt->dev);
> -       const u8 mddi = 3;
> +       struct mipi_dsi_multi_context dsi_ctx = {
> +               .dsi = to_mipi_dsi_device(nt->dev)
> +       };
>         int ret;
>
>         ret = nt35560_power_on(nt);
>         if (ret)
>                 return ret;
>
> -       ret = nt35560_read_id(nt);
> -       if (ret) {
> -               dev_err(nt->dev, "failed to read panel ID (%d)\n", ret);
> -               goto err_power_off;
> -       }
> +       nt35560_read_id(&dsi_ctx);
>
> -       /* Enabe tearing mode: send TE (tearing effect) at VBLANK */
> -       ret = mipi_dsi_dcs_set_tear_on(dsi,
> +       /* Enable tearing mode: send TE (tearing effect) at VBLANK */
> +       mipi_dsi_dcs_set_tear_on_multi(&dsi_ctx,
>                                        MIPI_DSI_DCS_TEAR_MODE_VBLANK);
> -       if (ret) {
> -               dev_err(nt->dev, "failed to enable vblank TE (%d)\n", ret);
> -               goto err_power_off;
> -       }
>
>         /*
>          * Set MDDI
>          *
>          * This presumably deactivates the Qualcomm MDDI interface and
>          * selects DSI, similar code is found in other drivers such as the
> -        * Sharp LS043T1LE01 which makes us suspect that this panel may be
> -        * using a Novatek NT35565 or similar display driver chip that shares
> -        * this command. Due to the lack of documentation we cannot know for
> -        * sure.
> +        * Sharp LS043T1LE01

Ah, this is the obsolete comment that you removed and talked about
"after the cut". You could probably include that info in the commit
message itself since someone looking at the commit later would
otherwise not know why this info was removed.

Also, nit: you should end your sentence with a period. :-)


Overall this looks like a nicely done cleanup. Thanks! ...just a few
small nits...


-Doug

Powered by blists - more mailing lists