[PATCH v2] drm/panel : himax-hx83102: fix incorrect argument to mipi_dsi_msleep

[PATCH v2] drm/panel : himax-hx83102: fix incorrect argument to mipi_dsi_msleep

[Tejas Vipin]

mipi_dsi_msleep expects struct mipi_dsi_multi_context to be passed as a
value and not as a reference.

Fixes: a2ab7cb169da ("drm/panel: himax-hx83102: use wrapped MIPI DCS functions")

Signed-off-by: Tejas Vipin <tejasvipin76@gmail.com>
---

Changes in v2:
    - Add Fixes tag

v1: https://lore.kernel.org/all/d9f4546f-c2f9-456d-ba75-85cc195dd9b8@gmail.com/

---
 drivers/gpu/drm/panel/panel-himax-hx83102.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/panel/panel-himax-hx83102.c b/drivers/gpu/drm/panel/panel-himax-hx83102.c
index 6009a3fe1b8f..ab00fd92cce0 100644
--- a/drivers/gpu/drm/panel/panel-himax-hx83102.c
+++ b/drivers/gpu/drm/panel/panel-himax-hx83102.c
@@ -479,7 +479,7 @@ static int hx83102_disable(struct drm_panel *panel)
 	mipi_dsi_dcs_set_display_off_multi(&dsi_ctx);
 	mipi_dsi_dcs_enter_sleep_mode_multi(&dsi_ctx);
 
-	mipi_dsi_msleep(&dsi_ctx, 150);
+	mipi_dsi_msleep(dsi_ctx, 150);
 
 	return dsi_ctx.accum_err;
 }
-- 
2.45.2

Re: [PATCH v2] drm/panel : himax-hx83102: fix incorrect argument to mipi_dsi_msleep

[Jessica Zhang]

On 6/11/2024 7:05 AM, Tejas Vipin wrote:
> mipi_dsi_msleep expects struct mipi_dsi_multi_context to be passed as a
> value and not as a reference.
> 
> Fixes: a2ab7cb169da ("drm/panel: himax-hx83102: use wrapped MIPI DCS functions")
> 
> Signed-off-by: Tejas Vipin <tejasvipin76@gmail.com>

Hi Tejas,

(for future reference, you don't need an extra newline between the Fixes 
and Signed-off-by tags)

LGTM,

Reviewed-by: Jessica Zhang <quic_jesszhan@quicinc.com>

Thanks,

Jessica Zhang

> ---
> 
> Changes in v2:
>      - Add Fixes tag
> 
> v1: https://lore.kernel.org/all/d9f4546f-c2f9-456d-ba75-85cc195dd9b8@gmail.com/
> 
> ---
>   drivers/gpu/drm/panel/panel-himax-hx83102.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/panel/panel-himax-hx83102.c b/drivers/gpu/drm/panel/panel-himax-hx83102.c
> index 6009a3fe1b8f..ab00fd92cce0 100644
> --- a/drivers/gpu/drm/panel/panel-himax-hx83102.c
> +++ b/drivers/gpu/drm/panel/panel-himax-hx83102.c
> @@ -479,7 +479,7 @@ static int hx83102_disable(struct drm_panel *panel)
>   	mipi_dsi_dcs_set_display_off_multi(&dsi_ctx);
>   	mipi_dsi_dcs_enter_sleep_mode_multi(&dsi_ctx);
>   
> -	mipi_dsi_msleep(&dsi_ctx, 150);
> +	mipi_dsi_msleep(dsi_ctx, 150);
>   
>   	return dsi_ctx.accum_err;
>   }
> -- 
> 2.45.2
> 

Re: [PATCH v2] drm/panel : himax-hx83102: fix incorrect argument to mipi_dsi_msleep

[Doug Anderson]

Hi,

On Tue, Jun 11, 2024 at 7:05 AM Tejas Vipin <tejasvipin76@gmail.com> wrote:
>
> mipi_dsi_msleep expects struct mipi_dsi_multi_context to be passed as a
> value and not as a reference.
>
> Fixes: a2ab7cb169da ("drm/panel: himax-hx83102: use wrapped MIPI DCS functions")
>
> Signed-off-by: Tejas Vipin <tejasvipin76@gmail.com>

Should be no blank line between "Fixes" and "Signed-off-by"

> ---
>
> Changes in v2:
>     - Add Fixes tag
>
> v1: https://lore.kernel.org/all/d9f4546f-c2f9-456d-ba75-85cc195dd9b8@gmail.com/
>
> ---
>  drivers/gpu/drm/panel/panel-himax-hx83102.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

I notice you didn't CC me, even though I authored the broken commit.
Presumably get_maintainer should have suggested you CC me?


> diff --git a/drivers/gpu/drm/panel/panel-himax-hx83102.c b/drivers/gpu/drm/panel/panel-himax-hx83102.c
> index 6009a3fe1b8f..ab00fd92cce0 100644
> --- a/drivers/gpu/drm/panel/panel-himax-hx83102.c
> +++ b/drivers/gpu/drm/panel/panel-himax-hx83102.c
> @@ -479,7 +479,7 @@ static int hx83102_disable(struct drm_panel *panel)
>         mipi_dsi_dcs_set_display_off_multi(&dsi_ctx);
>         mipi_dsi_dcs_enter_sleep_mode_multi(&dsi_ctx);
>
> -       mipi_dsi_msleep(&dsi_ctx, 150);
> +       mipi_dsi_msleep(dsi_ctx, 150);

So while your fix is correct, it's not really enough. I swore that I
compile tested my change and, sure enough, the bad code compile tests
fine. This is because the macro mipi_dsi_msleep() fell into a macro
trap. :( Specifically, we have:

#define mipi_dsi_msleep(ctx, delay)        \
        do {                               \
                if (!ctx.accum_err)        \
                        msleep(delay);     \
        } while (0)

Let's look at "if (!ctx.accum_err)". Before your patch, that translated to:

if (!&dsi_ctx.accum_err)

...adding extra parentheses for order of operations, that is:

 if (!&(dsi_ctx.accum_err))

...in other words it's testing whether the address of the "accum_err"
is NULL. That's not a syntax error, but _really_ not what was meant.

We really need to fix the macro trap by changing it like this:

-               if (!ctx.accum_err)     \
+               if (!(ctx).accum_err)   \

When you do that, though, you find that half the users of the macro
were using it wrong since every other "_multi_" function passes the
address. IMO while fixing the macro trap we should just change this to
pass the address and then fix up all the callers.

This is a serious enough problem (thanks for noticing it) that I'm
happy to send out patches, but also I'm fine if you want to tackle it.
If I don't see anything from you in a day or two I'll send out
patches.

Thanks!

-Doug