ctx would be better off treated as a pointer to account for most of its
usage so far, and brackets should be added to account for operator 
precedence for correct evaluation.

Fixes: f79d6d28d8fe7 ("drm/mipi-dsi: wrap more functions for streamline handling") 
Signed-off-by: Tejas Vipin <tejasvipin76@gmail.com>
---
 include/drm/drm_mipi_dsi.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/drm/drm_mipi_dsi.h b/include/drm/drm_mipi_dsi.h
index bd5a0b6d0711..71d121aeef24 100644
--- a/include/drm/drm_mipi_dsi.h
+++ b/include/drm/drm_mipi_dsi.h
@@ -293,7 +293,7 @@ ssize_t mipi_dsi_generic_read(struct mipi_dsi_device *dsi, const void *params,
 
 #define mipi_dsi_msleep(ctx, delay)	\
 	do {				\
-		if (!ctx.accum_err)	\
+		if (!(ctx)->accum_err)	\
 			msleep(delay);	\
 	} while (0)
 
-- 
2.45.2

Hi,

On Wed, Jun 12, 2024 at 6:37 AM Tejas Vipin <tejasvipin76@gmail.com> wrote:
>
> ctx would be better off treated as a pointer to account for most of its
> usage so far, and brackets should be added to account for operator
> precedence for correct evaluation.
>
> Fixes: f79d6d28d8fe7 ("drm/mipi-dsi: wrap more functions for streamline handling")
> Signed-off-by: Tejas Vipin <tejasvipin76@gmail.com>
> ---
>  include/drm/drm_mipi_dsi.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Yeah. Looking closer at the history, it looks like it was always
intended to be a pointer since the first users all used it as a
pointer.

Suggested-by: Douglas Anderson <dianders@chromium.org>
Reviewed-by: Douglas Anderson <dianders@chromium.org>

I've also compile-tested all the panels currently using mipi_dsi_msleep().

Neil: Given that this is a correctness thing, I'd rather see this land
sooner rather than later. If you agree, maybe you can land these two
patches whenever you're comfortable with them?


-Doug

On 12/06/2024 16:21, Doug Anderson wrote:
> Hi,
> 
> On Wed, Jun 12, 2024 at 6:37 AM Tejas Vipin <tejasvipin76@gmail.com> wrote:
>>
>> ctx would be better off treated as a pointer to account for most of its
>> usage so far, and brackets should be added to account for operator
>> precedence for correct evaluation.
>>
>> Fixes: f79d6d28d8fe7 ("drm/mipi-dsi: wrap more functions for streamline handling")
>> Signed-off-by: Tejas Vipin <tejasvipin76@gmail.com>
>> ---
>>   include/drm/drm_mipi_dsi.h | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> Yeah. Looking closer at the history, it looks like it was always
> intended to be a pointer since the first users all used it as a
> pointer.
> 
> Suggested-by: Douglas Anderson <dianders@chromium.org>
> Reviewed-by: Douglas Anderson <dianders@chromium.org>
> 
> I've also compile-tested all the panels currently using mipi_dsi_msleep().
> 
> Neil: Given that this is a correctness thing, I'd rather see this land
> sooner rather than later. If you agree, maybe you can land these two
> patches whenever you're comfortable with them?

Applying them, but inverting them, fix should go first.

Neil

> 
> 
> -Doug

Hi,

On Wed, Jun 12, 2024 at 7:34 AM <neil.armstrong@linaro.org> wrote:
>
> On 12/06/2024 16:21, Doug Anderson wrote:
> > Hi,
> >
> > On Wed, Jun 12, 2024 at 6:37 AM Tejas Vipin <tejasvipin76@gmail.com> wrote:
> >>
> >> ctx would be better off treated as a pointer to account for most of its
> >> usage so far, and brackets should be added to account for operator
> >> precedence for correct evaluation.
> >>
> >> Fixes: f79d6d28d8fe7 ("drm/mipi-dsi: wrap more functions for streamline handling")
> >> Signed-off-by: Tejas Vipin <tejasvipin76@gmail.com>
> >> ---
> >>   include/drm/drm_mipi_dsi.h | 2 +-
> >>   1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > Yeah. Looking closer at the history, it looks like it was always
> > intended to be a pointer since the first users all used it as a
> > pointer.
> >
> > Suggested-by: Douglas Anderson <dianders@chromium.org>
> > Reviewed-by: Douglas Anderson <dianders@chromium.org>
> >
> > I've also compile-tested all the panels currently using mipi_dsi_msleep().
> >
> > Neil: Given that this is a correctness thing, I'd rather see this land
> > sooner rather than later. If you agree, maybe you can land these two
> > patches whenever you're comfortable with them?
>
> Applying them, but inverting them, fix should go first.

Well, they're both fixes, and inverting them means that you get a
compile failure across several panels if you happen to be bisecting
and land on the first commit, but it doesn't really matter. I guess
the compile failure is maybe a benefit given that they were not doing
their delays properly... ;-)

-Doug

On 12/06/2024 16:52, Doug Anderson wrote:
> Hi,
> 
> On Wed, Jun 12, 2024 at 7:34 AM <neil.armstrong@linaro.org> wrote:
>>
>> On 12/06/2024 16:21, Doug Anderson wrote:
>>> Hi,
>>>
>>> On Wed, Jun 12, 2024 at 6:37 AM Tejas Vipin <tejasvipin76@gmail.com> wrote:
>>>>
>>>> ctx would be better off treated as a pointer to account for most of its
>>>> usage so far, and brackets should be added to account for operator
>>>> precedence for correct evaluation.
>>>>
>>>> Fixes: f79d6d28d8fe7 ("drm/mipi-dsi: wrap more functions for streamline handling")
>>>> Signed-off-by: Tejas Vipin <tejasvipin76@gmail.com>
>>>> ---
>>>>    include/drm/drm_mipi_dsi.h | 2 +-
>>>>    1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> Yeah. Looking closer at the history, it looks like it was always
>>> intended to be a pointer since the first users all used it as a
>>> pointer.
>>>
>>> Suggested-by: Douglas Anderson <dianders@chromium.org>
>>> Reviewed-by: Douglas Anderson <dianders@chromium.org>
>>>
>>> I've also compile-tested all the panels currently using mipi_dsi_msleep().
>>>
>>> Neil: Given that this is a correctness thing, I'd rather see this land
>>> sooner rather than later. If you agree, maybe you can land these two
>>> patches whenever you're comfortable with them?
>>
>> Applying them, but inverting them, fix should go first.
> 
> Well, they're both fixes, and inverting them means that you get a
> compile failure across several panels if you happen to be bisecting
> and land on the first commit, but it doesn't really matter. I guess
> the compile failure is maybe a benefit given that they were not doing
> their delays properly... ;-)

Yes, and thanksfully there's a fix for the build failure!

> 
> -Doug