[PATCH] drm/panel/synaptics-r63353: Use _multi variants

[PATCH] drm/panel/synaptics-r63353: Use _multi variants

[Anusha Srivatsa]

Move away from using deprecated API and use _multi
variants if available. Use mipi_dsi_msleep()
and mipi_dsi_usleep_range() instead of msleep()
and usleep_range() respectively.

Used Coccinelle to find the multiple occurences.
SmPl patch:
@rule@
identifier dsi_var;
identifier r;
identifier func;
type t;
position p;
expression dsi_device;
expression list es;
@@
t func(...) {
...
struct mipi_dsi_device *dsi_var = dsi_device;
+struct mipi_dsi_multi_context dsi_ctx = { .dsi = dsi_var };
<+...
(
-mipi_dsi_dcs_write_seq(dsi_var,es)@p;
+mipi_dsi_dcs_write_seq_multi(&dsi_ctx,es);
|
-mipi_dsi_generic_write_seq(dsi_var,es)@p;
+mipi_dsi_generic_write_seq_multi(&dsi_ctx,es);
|
-mipi_dsi_generic_write(dsi_var,es)@p;
+mipi_dsi_generic_write_multi(&dsi_ctx,es);
|
-r = mipi_dsi_dcs_nop(dsi_var)@p;
+mipi_dsi_dcs_nop_multi(&dsi_ctx);
|
....rest of API
..
)
-if(r < 0) {
-...
-}
...+>

Cc: Maxime Ripard <maxime.ripard@bootlin.com>
Cc: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
Cc: Tejas Vipin <tejasvipin76@gmail.com>
Cc: Doug Anderson <dianders@chromium.org>
Signed-off-by: Anusha Srivatsa <asrivats@redhat.com>
---
Previous attempt for this change was addressed in:[1]
The series did not handle returns properly and still
used msleep() and usleep_range() API.
It also collided with an Tejas's similar efforts.

Will be sending the patches per driver instead of
major haul of changes.

Following [2] for reference.

[1] -> https://patchwork.freedesktop.org/series/144824/
[2] -> https://lore.kernel.org/dri-devel/20250220045721.145905-1-tejasvipin76@gmail.com/#iZ31drivers:gpu:drm:panel:panel-sony-td4353-jdi.c
---
 drivers/gpu/drm/panel/panel-synaptics-r63353.c | 64 +++++++-------------------
 1 file changed, 17 insertions(+), 47 deletions(-)

diff --git a/drivers/gpu/drm/panel/panel-synaptics-r63353.c b/drivers/gpu/drm/panel/panel-synaptics-r63353.c
index 17349825543fe6a117bbfd9cb92a564ce433d13a..9cb1fe985a6fde7a475746019e89bd1dcee898ff 100644
--- a/drivers/gpu/drm/panel/panel-synaptics-r63353.c
+++ b/drivers/gpu/drm/panel/panel-synaptics-r63353.c
@@ -106,53 +106,32 @@ static int r63353_panel_power_off(struct r63353_panel *rpanel)
 static int r63353_panel_activate(struct r63353_panel *rpanel)
 {
 	struct mipi_dsi_device *dsi = rpanel->dsi;
-	struct device *dev = &dsi->dev;
-	int i, ret;
+	struct mipi_dsi_multi_context dsi_ctx = { .dsi = dsi };
+	int i;
 
-	ret = mipi_dsi_dcs_soft_reset(dsi);
-	if (ret < 0) {
-		dev_err(dev, "Failed to do Software Reset (%d)\n", ret);
-		goto fail;
-	}
+	mipi_dsi_dcs_soft_reset_multi(&dsi_ctx);
 
-	usleep_range(15000, 17000);
+	mipi_dsi_usleep_range(&dsi_ctx, 15000, 17000);
 
-	ret = mipi_dsi_dcs_enter_sleep_mode(dsi);
-	if (ret < 0) {
-		dev_err(dev, "Failed to enter sleep mode (%d)\n", ret);
-		goto fail;
-	}
+	mipi_dsi_dcs_enter_sleep_mode_multi(&dsi_ctx);
 
 	for (i = 0; i < rpanel->pdata->init_length; i++) {
 		const struct r63353_instr *instr = &rpanel->pdata->init[i];
 
-		ret = mipi_dsi_dcs_write_buffer(dsi, instr->data, instr->len);
-		if (ret < 0)
-			goto fail;
+		mipi_dsi_dcs_write_buffer_multi(&dsi_ctx, instr->data,
+						instr->len);
 	}
 
-	msleep(120);
+	mipi_dsi_msleep(&dsi_ctx, 120);
 
-	ret = mipi_dsi_dcs_exit_sleep_mode(dsi);
-	if (ret < 0) {
-		dev_err(dev, "Failed to exit sleep mode (%d)\n", ret);
-		goto fail;
-	}
+	mipi_dsi_dcs_exit_sleep_mode_multi(&dsi_ctx);
 
-	usleep_range(5000, 10000);
+	mipi_dsi_usleep_range(&dsi_ctx, 5000, 10000);
 
-	ret = mipi_dsi_dcs_set_display_on(dsi);
-	if (ret < 0) {
-		dev_err(dev, "Failed to set display ON (%d)\n", ret);
-		goto fail;
-	}
-
-	return 0;
+	mipi_dsi_dcs_set_display_on_multi(&dsi_ctx);
 
-fail:
-	gpiod_set_value(rpanel->reset_gpio, 0);
+	return dsi_ctx.accum_err;
 
-	return ret;
 }
 
 static int r63353_panel_prepare(struct drm_panel *panel)
@@ -181,24 +160,15 @@ static int r63353_panel_prepare(struct drm_panel *panel)
 static int r63353_panel_deactivate(struct r63353_panel *rpanel)
 {
 	struct mipi_dsi_device *dsi = rpanel->dsi;
-	struct device *dev = &dsi->dev;
-	int ret;
+	struct mipi_dsi_multi_context dsi_ctx = { .dsi = dsi };
 
-	ret = mipi_dsi_dcs_set_display_off(dsi);
-	if (ret < 0) {
-		dev_err(dev, "Failed to set display OFF (%d)\n", ret);
-		return ret;
-	}
+	mipi_dsi_dcs_set_display_off_multi(&dsi_ctx);
 
-	usleep_range(5000, 10000);
+	mipi_dsi_usleep_range(&dsi_ctx, 5000, 10000);
 
-	ret = mipi_dsi_dcs_enter_sleep_mode(dsi);
-	if (ret < 0) {
-		dev_err(dev, "Failed to enter sleep mode (%d)\n", ret);
-		return ret;
-	}
+	mipi_dsi_dcs_enter_sleep_mode_multi(&dsi_ctx);
 
-	return 0;
+	return dsi_ctx.accum_err;
 }
 
 static int r63353_panel_unprepare(struct drm_panel *panel)

---
base-commit: ced7486468ac3b38d59a69fca5d97998499c936b
change-id: 20250305-mipi-synaptic-1-a2043543cd3a

Best regards,
-- 
Anusha Srivatsa <asrivats@redhat.com>

Re: [PATCH] drm/panel/synaptics-r63353: Use _multi variants

[Dmitry Baryshkov]

On Wed, Mar 05, 2025 at 07:01:41PM -0500, Anusha Srivatsa wrote:
> Move away from using deprecated API and use _multi
> variants if available. Use mipi_dsi_msleep()
> and mipi_dsi_usleep_range() instead of msleep()
> and usleep_range() respectively.
> 
> Used Coccinelle to find the multiple occurences.
> SmPl patch:
> @rule@
> identifier dsi_var;
> identifier r;
> identifier func;
> type t;
> position p;
> expression dsi_device;
> expression list es;
> @@
> t func(...) {
> ...
> struct mipi_dsi_device *dsi_var = dsi_device;
> +struct mipi_dsi_multi_context dsi_ctx = { .dsi = dsi_var };
> <+...
> (
> -mipi_dsi_dcs_write_seq(dsi_var,es)@p;
> +mipi_dsi_dcs_write_seq_multi(&dsi_ctx,es);
> |
> -mipi_dsi_generic_write_seq(dsi_var,es)@p;
> +mipi_dsi_generic_write_seq_multi(&dsi_ctx,es);
> |
> -mipi_dsi_generic_write(dsi_var,es)@p;
> +mipi_dsi_generic_write_multi(&dsi_ctx,es);
> |
> -r = mipi_dsi_dcs_nop(dsi_var)@p;
> +mipi_dsi_dcs_nop_multi(&dsi_ctx);
> |
> ....rest of API
> ..
> )
> -if(r < 0) {
> -...
> -}
> ...+>
> 
> Cc: Maxime Ripard <maxime.ripard@bootlin.com>
> Cc: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> Cc: Tejas Vipin <tejasvipin76@gmail.com>
> Cc: Doug Anderson <dianders@chromium.org>
> Signed-off-by: Anusha Srivatsa <asrivats@redhat.com>
> ---
> Previous attempt for this change was addressed in:[1]
> The series did not handle returns properly and still
> used msleep() and usleep_range() API.
> It also collided with an Tejas's similar efforts.
> 
> Will be sending the patches per driver instead of
> major haul of changes.
> 
> Following [2] for reference.
> 
> [1] -> https://patchwork.freedesktop.org/series/144824/
> [2] -> https://lore.kernel.org/dri-devel/20250220045721.145905-1-tejasvipin76@gmail.com/#iZ31drivers:gpu:drm:panel:panel-sony-td4353-jdi.c
> ---
>  drivers/gpu/drm/panel/panel-synaptics-r63353.c | 64 +++++++-------------------
>  1 file changed, 17 insertions(+), 47 deletions(-)
> 
>  
> -	ret = mipi_dsi_dcs_set_display_on(dsi);
> -	if (ret < 0) {
> -		dev_err(dev, "Failed to set display ON (%d)\n", ret);
> -		goto fail;
> -	}
> -
> -	return 0;
> +	mipi_dsi_dcs_set_display_on_multi(&dsi_ctx);
>  
> -fail:
> -	gpiod_set_value(rpanel->reset_gpio, 0);
> +	return dsi_ctx.accum_err;

You've lost panel reset in case of an error. The rest LGTM.

>  
> -	return ret;
>  }
>  
>  static int r63353_panel_prepare(struct drm_panel *panel)

-- 
With best wishes
Dmitry

Re: [PATCH] drm/panel/synaptics-r63353: Use _multi variants

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 1320 bytes --]

Hi Anusha,

On Wed, Mar 05, 2025 at 07:01:41PM -0500, Anusha Srivatsa wrote:
> Move away from using deprecated API and use _multi
> variants if available. Use mipi_dsi_msleep()
> and mipi_dsi_usleep_range() instead of msleep()
> and usleep_range() respectively.
> 
> Used Coccinelle to find the multiple occurences.
> SmPl patch:
> @rule@
> identifier dsi_var;
> identifier r;
> identifier func;
> type t;
> position p;
> expression dsi_device;
> expression list es;
> @@
> t func(...) {
> ...
> struct mipi_dsi_device *dsi_var = dsi_device;
> +struct mipi_dsi_multi_context dsi_ctx = { .dsi = dsi_var };
> <+...
> (
> -mipi_dsi_dcs_write_seq(dsi_var,es)@p;
> +mipi_dsi_dcs_write_seq_multi(&dsi_ctx,es);
> |
> -mipi_dsi_generic_write_seq(dsi_var,es)@p;
> +mipi_dsi_generic_write_seq_multi(&dsi_ctx,es);
> |
> -mipi_dsi_generic_write(dsi_var,es)@p;
> +mipi_dsi_generic_write_multi(&dsi_ctx,es);
> |
> -r = mipi_dsi_dcs_nop(dsi_var)@p;
> +mipi_dsi_dcs_nop_multi(&dsi_ctx);
> |
> ....rest of API
> ..
> )
> -if(r < 0) {
> -...
> -}
> ...+>

The point of sending a single patch was to review the coccinelle script,
so you must put the entire script you used here.

> Cc: Maxime Ripard <maxime.ripard@bootlin.com>

That hasn't been my email address for 6 years :)

Maxime

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 273 bytes --]

Re: [PATCH] drm/panel/synaptics-r63353: Use _multi variants

[Dmitry Baryshkov]

On Thu, 6 Mar 2025 at 17:10, Anusha Srivatsa <asrivats@redhat.com> wrote:
>
>
>
> On Thu, Mar 6, 2025 at 4:31 AM Maxime Ripard <mripard@kernel.org> wrote:
>>
>> Hi Anusha,
>>
>> On Wed, Mar 05, 2025 at 07:01:41PM -0500, Anusha Srivatsa wrote:
>> > Move away from using deprecated API and use _multi
>> > variants if available. Use mipi_dsi_msleep()
>> > and mipi_dsi_usleep_range() instead of msleep()
>> > and usleep_range() respectively.
>> >
>> > Used Coccinelle to find the multiple occurences.
>> > SmPl patch:
>> > @rule@
>> > identifier dsi_var;
>> > identifier r;
>> > identifier func;
>> > type t;
>> > position p;
>> > expression dsi_device;
>> > expression list es;
>> > @@
>> > t func(...) {
>> > ...
>> > struct mipi_dsi_device *dsi_var = dsi_device;
>> > +struct mipi_dsi_multi_context dsi_ctx = { .dsi = dsi_var };
>> > <+...
>> > (
>> > -mipi_dsi_dcs_write_seq(dsi_var,es)@p;
>> > +mipi_dsi_dcs_write_seq_multi(&dsi_ctx,es);
>> > |
>> > -mipi_dsi_generic_write_seq(dsi_var,es)@p;
>> > +mipi_dsi_generic_write_seq_multi(&dsi_ctx,es);
>> > |
>> > -mipi_dsi_generic_write(dsi_var,es)@p;
>> > +mipi_dsi_generic_write_multi(&dsi_ctx,es);
>> > |
>> > -r = mipi_dsi_dcs_nop(dsi_var)@p;
>> > +mipi_dsi_dcs_nop_multi(&dsi_ctx);
>> > |
>> > ....rest of API
>> > ..
>> > )
>> > -if(r < 0) {
>> > -...
>> > -}
>> > ...+>
>>
>> The point of sending a single patch was to review the coccinelle script,
>> so you must put the entire script you used here.
>>
>
> I was actually thinking of sending patches per driver this time around since Tejas also seems to be looking into similar parts....Thoughts?

Have you discussed it with Tejas? What is his next target?

>> > Cc: Maxime Ripard <maxime.ripard@bootlin.com>
>>
>> That hasn't been my email address for 6 years :)
>>
> My bad. Will change this.



-- 
With best wishes
Dmitry

Re: [PATCH] drm/panel/synaptics-r63353: Use _multi variants

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 2153 bytes --]

On Thu, Mar 06, 2025 at 10:08:24AM -0500, Anusha Srivatsa wrote:
> On Thu, Mar 6, 2025 at 4:31 AM Maxime Ripard <mripard@kernel.org> wrote:
> 
> > Hi Anusha,
> >
> > On Wed, Mar 05, 2025 at 07:01:41PM -0500, Anusha Srivatsa wrote:
> > > Move away from using deprecated API and use _multi
> > > variants if available. Use mipi_dsi_msleep()
> > > and mipi_dsi_usleep_range() instead of msleep()
> > > and usleep_range() respectively.
> > >
> > > Used Coccinelle to find the multiple occurences.
> > > SmPl patch:
> > > @rule@
> > > identifier dsi_var;
> > > identifier r;
> > > identifier func;
> > > type t;
> > > position p;
> > > expression dsi_device;
> > > expression list es;
> > > @@
> > > t func(...) {
> > > ...
> > > struct mipi_dsi_device *dsi_var = dsi_device;
> > > +struct mipi_dsi_multi_context dsi_ctx = { .dsi = dsi_var };
> > > <+...
> > > (
> > > -mipi_dsi_dcs_write_seq(dsi_var,es)@p;
> > > +mipi_dsi_dcs_write_seq_multi(&dsi_ctx,es);
> > > |
> > > -mipi_dsi_generic_write_seq(dsi_var,es)@p;
> > > +mipi_dsi_generic_write_seq_multi(&dsi_ctx,es);
> > > |
> > > -mipi_dsi_generic_write(dsi_var,es)@p;
> > > +mipi_dsi_generic_write_multi(&dsi_ctx,es);
> > > |
> > > -r = mipi_dsi_dcs_nop(dsi_var)@p;
> > > +mipi_dsi_dcs_nop_multi(&dsi_ctx);
> > > |
> > > ....rest of API
> > > ..
> > > )
> > > -if(r < 0) {
> > > -...
> > > -}
> > > ...+>
> >
> > The point of sending a single patch was to review the coccinelle script,
> > so you must put the entire script you used here.
>
> I was actually thinking of sending patches per driver this time around
> since Tejas also seems to be looking into similar parts....Thoughts?

Not really?

The point of doing it with one driver was to make sure the coccinelle
script was fine before rolling it to other drivers. And actually, it
doesn't really matter: the whole point of putting the script in the
commit log is to be able to review and document the script you used. If
you're not going to put the one you used, it's kind of pointless.

And yeah, you should absolutely sync with Tejas, but it's unrelated to
coccinelle.

Maxime

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH] drm/panel/synaptics-r63353: Use _multi variants

[Tejas Vipin]

On 3/6/25 9:03 PM, Anusha Srivatsa wrote:
> On Thu, Mar 6, 2025 at 11:29 AM Dmitry Baryshkov <
> dmitry.baryshkov@linaro.org> wrote:
> 
>> On Thu, 6 Mar 2025 at 17:10, Anusha Srivatsa <asrivats@redhat.com> wrote:
>>>
>>>
>>>
>>> On Thu, Mar 6, 2025 at 4:31 AM Maxime Ripard <mripard@kernel.org> wrote:
>>>>
>>>> Hi Anusha,
>>>>
>>>> On Wed, Mar 05, 2025 at 07:01:41PM -0500, Anusha Srivatsa wrote:
>>>>> Move away from using deprecated API and use _multi
>>>>> variants if available. Use mipi_dsi_msleep()
>>>>> and mipi_dsi_usleep_range() instead of msleep()
>>>>> and usleep_range() respectively.
>>>>>
>>>>> Used Coccinelle to find the multiple occurences.
>>>>> SmPl patch:
>>>>> @rule@
>>>>> identifier dsi_var;
>>>>> identifier r;
>>>>> identifier func;
>>>>> type t;
>>>>> position p;
>>>>> expression dsi_device;
>>>>> expression list es;
>>>>> @@
>>>>> t func(...) {
>>>>> ...
>>>>> struct mipi_dsi_device *dsi_var = dsi_device;
>>>>> +struct mipi_dsi_multi_context dsi_ctx = { .dsi = dsi_var };
>>>>> <+...
>>>>> (
>>>>> -mipi_dsi_dcs_write_seq(dsi_var,es)@p;
>>>>> +mipi_dsi_dcs_write_seq_multi(&dsi_ctx,es);
>>>>> |
>>>>> -mipi_dsi_generic_write_seq(dsi_var,es)@p;
>>>>> +mipi_dsi_generic_write_seq_multi(&dsi_ctx,es);
>>>>> |
>>>>> -mipi_dsi_generic_write(dsi_var,es)@p;
>>>>> +mipi_dsi_generic_write_multi(&dsi_ctx,es);
>>>>> |
>>>>> -r = mipi_dsi_dcs_nop(dsi_var)@p;
>>>>> +mipi_dsi_dcs_nop_multi(&dsi_ctx);
>>>>> |
>>>>> ....rest of API
>>>>> ..
>>>>> )
>>>>> -if(r < 0) {
>>>>> -...
>>>>> -}
>>>>> ...+>
>>>>
>>>> The point of sending a single patch was to review the coccinelle script,
>>>> so you must put the entire script you used here.
>>>>
>>>
>>> I was actually thinking of sending patches per driver this time around
>> since Tejas also seems to be looking into similar parts....Thoughts?
>>
>> Have you discussed it with Tejas? What is his next target?
>>
>> I was hoping he will have some feedback on this patch and we could take it
> from there.....
> It *should* be okay for me to send all changes in a single series...
> 
> Anusha
>

There's 5 more panels that use dcs/generic write_seq(). Maybe I could
work on those (himax-hx8394, samsung-sofef00, samsung-s6d7aa0,
boe-bf060y8m-aj0, jdi-lpm102a188a) while you work on transitioning the
rest of the panels (excluding these) that use other functions in the 
old API? When either of us finishes before the other we could have 
another discussion about splitting work if necessary. I'm open to other
suggestions too.

>>>>> Cc: Maxime Ripard <maxime.ripard@bootlin.com>
>>>>
>>>> That hasn't been my email address for 6 years :)
>>>>
>>> My bad. Will change this.
>>
>>
>>
>> --
>> With best wishes
>> Dmitry
>>
>>
> 

-- 
Tejas Vipin

Re: [PATCH] drm/panel/synaptics-r63353: Use _multi variants

[Doug Anderson]

Hi,

On Thu, Mar 6, 2025 at 9:20 AM Maxime Ripard <mripard@kernel.org> wrote:
>
> On Thu, Mar 06, 2025 at 10:08:24AM -0500, Anusha Srivatsa wrote:
> > On Thu, Mar 6, 2025 at 4:31 AM Maxime Ripard <mripard@kernel.org> wrote:
> >
> > > Hi Anusha,
> > >
> > > On Wed, Mar 05, 2025 at 07:01:41PM -0500, Anusha Srivatsa wrote:
> > > > Move away from using deprecated API and use _multi
> > > > variants if available. Use mipi_dsi_msleep()
> > > > and mipi_dsi_usleep_range() instead of msleep()
> > > > and usleep_range() respectively.
> > > >
> > > > Used Coccinelle to find the multiple occurences.
> > > > SmPl patch:
> > > > @rule@
> > > > identifier dsi_var;
> > > > identifier r;
> > > > identifier func;
> > > > type t;
> > > > position p;
> > > > expression dsi_device;
> > > > expression list es;
> > > > @@
> > > > t func(...) {
> > > > ...
> > > > struct mipi_dsi_device *dsi_var = dsi_device;
> > > > +struct mipi_dsi_multi_context dsi_ctx = { .dsi = dsi_var };
> > > > <+...
> > > > (
> > > > -mipi_dsi_dcs_write_seq(dsi_var,es)@p;
> > > > +mipi_dsi_dcs_write_seq_multi(&dsi_ctx,es);
> > > > |
> > > > -mipi_dsi_generic_write_seq(dsi_var,es)@p;
> > > > +mipi_dsi_generic_write_seq_multi(&dsi_ctx,es);
> > > > |
> > > > -mipi_dsi_generic_write(dsi_var,es)@p;
> > > > +mipi_dsi_generic_write_multi(&dsi_ctx,es);
> > > > |
> > > > -r = mipi_dsi_dcs_nop(dsi_var)@p;
> > > > +mipi_dsi_dcs_nop_multi(&dsi_ctx);
> > > > |
> > > > ....rest of API
> > > > ..
> > > > )
> > > > -if(r < 0) {
> > > > -...
> > > > -}
> > > > ...+>
> > >
> > > The point of sending a single patch was to review the coccinelle script,
> > > so you must put the entire script you used here.
> >
> > I was actually thinking of sending patches per driver this time around
> > since Tejas also seems to be looking into similar parts....Thoughts?
>
> Not really?
>
> The point of doing it with one driver was to make sure the coccinelle
> script was fine before rolling it to other drivers. And actually, it
> doesn't really matter: the whole point of putting the script in the
> commit log is to be able to review and document the script you used. If
> you're not going to put the one you used, it's kind of pointless.

Personally, I don't have any interest in reviewing the coccinelle
script so I don't need it and, from my point of view, you could just
remove it from the patch description (or point to it indirectly or
something). I'll review each patch on its own merits. I am a bit
curious if you ended up fully generating this patch with a coccinelle
script or if you used a coccinelle script to start and then had to
manually tweak the patch after. Actually, I guess I'll take it back.
If you manage to fully generate conversions for all the panels with a
single cocinelle script, I would love to take a glance at your full
script just to satisfy my curiosity for how you handled everything
properly. :-P

-Doug

Re: [PATCH] drm/panel/synaptics-r63353: Use _multi variants

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 6869 bytes --]

On Thu, Mar 06, 2025 at 02:12:14PM -0500, Anusha Srivatsa wrote:
> On Thu, Mar 6, 2025 at 12:54 PM Doug Anderson <dianders@chromium.org> wrote:
> > On Thu, Mar 6, 2025 at 9:20 AM Maxime Ripard <mripard@kernel.org> wrote:
> > >
> > > On Thu, Mar 06, 2025 at 10:08:24AM -0500, Anusha Srivatsa wrote:
> > > > On Thu, Mar 6, 2025 at 4:31 AM Maxime Ripard <mripard@kernel.org>
> > wrote:
> > > >
> > > > > Hi Anusha,
> > > > >
> > > > > On Wed, Mar 05, 2025 at 07:01:41PM -0500, Anusha Srivatsa wrote:
> > > > > > Move away from using deprecated API and use _multi
> > > > > > variants if available. Use mipi_dsi_msleep()
> > > > > > and mipi_dsi_usleep_range() instead of msleep()
> > > > > > and usleep_range() respectively.
> > > > > >
> > > > > > Used Coccinelle to find the multiple occurences.
> > > > > > SmPl patch:
> > > > > > @rule@
> > > > > > identifier dsi_var;
> > > > > > identifier r;
> > > > > > identifier func;
> > > > > > type t;
> > > > > > position p;
> > > > > > expression dsi_device;
> > > > > > expression list es;
> > > > > > @@
> > > > > > t func(...) {
> > > > > > ...
> > > > > > struct mipi_dsi_device *dsi_var = dsi_device;
> > > > > > +struct mipi_dsi_multi_context dsi_ctx = { .dsi = dsi_var };
> > > > > > <+...
> > > > > > (
> > > > > > -mipi_dsi_dcs_write_seq(dsi_var,es)@p;
> > > > > > +mipi_dsi_dcs_write_seq_multi(&dsi_ctx,es);
> > > > > > |
> > > > > > -mipi_dsi_generic_write_seq(dsi_var,es)@p;
> > > > > > +mipi_dsi_generic_write_seq_multi(&dsi_ctx,es);
> > > > > > |
> > > > > > -mipi_dsi_generic_write(dsi_var,es)@p;
> > > > > > +mipi_dsi_generic_write_multi(&dsi_ctx,es);
> > > > > > |
> > > > > > -r = mipi_dsi_dcs_nop(dsi_var)@p;
> > > > > > +mipi_dsi_dcs_nop_multi(&dsi_ctx);
> > > > > > |
> > > > > > ....rest of API
> > > > > > ..
> > > > > > )
> > > > > > -if(r < 0) {
> > > > > > -...
> > > > > > -}
> > > > > > ...+>
> > > > >
> > > > > The point of sending a single patch was to review the coccinelle
> > script,
> > > > > so you must put the entire script you used here.
> > > >
> > > > I was actually thinking of sending patches per driver this time around
> > > > since Tejas also seems to be looking into similar parts....Thoughts?
> > >
> > > Not really?
> > >
> > > The point of doing it with one driver was to make sure the coccinelle
> > > script was fine before rolling it to other drivers. And actually, it
> > > doesn't really matter: the whole point of putting the script in the
> > > commit log is to be able to review and document the script you used. If
> > > you're not going to put the one you used, it's kind of pointless.
> >
> > Personally, I don't have any interest in reviewing the coccinelle
> > script so I don't need it and, from my point of view, you could just
> > remove it from the patch description (or point to it indirectly or
> > something). I'll review each patch on its own merits. I am a bit
> > curious if you ended up fully generating this patch with a coccinelle
> > script or if you used a coccinelle script to start and then had to
> > manually tweak the patch after. Actually, I guess I'll take it back.
> > If you manage to fully generate conversions for all the panels with a
> > single cocinelle script, I would love to take a glance at your full
> > script just to satisfy my curiosity for how you handled everything
> > properly. :-P
> >
> 
> managed to get all conversions other than the usleep_range() and mslee()
> because I was trying to replace them with mipi_dsi_* variants only in
> functions that I was  touching and not each and every occurrence. Didnt
> spend enough time to get the script smart enough to do that and did only
> usleep() and msleep change manually. Here goes the script:
> 
> @rule_1@
> identifier dsi_var;
> expression dsi_device;
> expression list es;
> @@
> struct mipi_dsi_device *dsi_var = dsi_device;
> +struct mipi_dsi_multi_context dsi_ctx = { .dsi = dsi_var };
> <+...
> -mipi_dsi_dcs_write_seq(dsi_var,es);
> +mipi_dsi_dcs_write_seq_multi(&dsi_ctx,es);
> ...+>
> 
> //rule_2
> @@
> expression list es;
> identifier jdi;
> @@
> static int jdi_write_dcdc_registers(struct jdi_panel *jdi)
> {
> +struct mipi_dsi_multi_context dsi_ctx1 = { .dsi = jdi->link1 };
> +struct mipi_dsi_multi_context dsi_ctx2 = { .dsi = jdi->link2 };
> <+...
> -mipi_dsi_generic_write_seq(jdi->link1,es);
> +mipi_dsi_generic_write_seq_multi(&dsi_ctx1,es);
> -mipi_dsi_generic_write_seq(jdi->link2,es);
> +mipi_dsi_generic_write_seq_multi(&dsi_ctx2,es);
> ...+>
> }
> @rule_3@
> identifier dsi_var;
> identifier r;
> identifier func;
> type t;
> position p;
> expression dsi_device;
> expression list es;
> @@
> t func(...) {
> ...
> struct mipi_dsi_device *dsi_var = dsi_device;
> +struct mipi_dsi_multi_context dsi_ctx = { .dsi = dsi_var };
> <+...
> (
> -r = mipi_dsi_dcs_nop(dsi_var)@p;
> +mipi_dsi_dcs_nop_multi(&dsi_ctx);
> |
> -r = mipi_dsi_dcs_exit_sleep_mode(dsi_var)@p;
> +mipi_dsi_dcs_exit_sleep_mode_multi(&dsi_ctx);
> |
> -r = mipi_dsi_dcs_enter_sleep_mode(dsi_var)@p;
> +mipi_dsi_dcs_enter_sleep_mode_multi(&dsi_ctx);
> |
> |
> -r = mipi_dsi_dcs_write_buffer(dsi_var,es)@p;
> +mipi_dsi_dcs_write_buffer_multi(&dsi_ctx,es);
> |
> -r = mipi_dsi_dcs_set_display_off(dsi_var,es)@p;
> +mipi_dsi_dcs_set_display_off_multi(&dsi_ctx,es);
> |
> -r = mipi_dsi_compression_mode_ext(dsi_var,es)@p;
> +mipi_dsi_compression_mode_ext_multi(&dsi_ctx,es);
> |
> -r = mipi_dsi_compression_mode(dsi_var,es)@p;
> +mipi_dsi_compression_mode_multi(&dsi_ctx,es);
> |
> -r = mipi_dsi_picture_parameter_set(dsi_var,es)@p;
> +mipi_dsi_picture_parameter_set_multi(&dsi_ctx,es);
> |
> -r = mipi_dsi_dcs_set_display_on(dsi_var,es)@p;
> +mipi_dsi_dcs_set_display_on_multi(&dsi_ctx,es);
> |
> -r = mipi_dsi_dcs_set_tear_on(dsi_var)@p;
> +mipi_dsi_dcs_set_tear_on_multi(&dsi_ctx);
> |
> -r = mipi_dsi_turn_on_peripheral(dsi_var)@p;
> +mipi_dsi_turn_on_peripheral_multi(&dsi_ctx);
> |
> -r = mipi_dsi_dcs_soft_reset(dsi_var)@p;
> +mipi_dsi_dcs_soft_reset_multi(&dsi_ctx);
> |
> -r = mipi_dsi_dcs_set_display_brightness(dsi_var,es)@p;
> +mipi_dsi_dcs_set_display_brightness_multi(&dsi_ctx,es);
> |
> -r = mipi_dsi_dcs_set_pixel_format(dsi_var,es)@p;
> +mipi_dsi_dcs_set_pixel_format_multi(&dsi_ctx,es);
> |
> -r = mipi_dsi_dcs_set_column_address(dsi_var,es)@p;
> +mipi_dsi_dcs_set_column_address_multi(&dsi_ctx,es);
> |
> -r = mipi_dsi_dcs_set_page_address(dsi_var,es)@p;
> +mipi_dsi_dcs_set_page_address_multi(&dsi_ctx,es);
> |
> -r = mipi_dsi_dcs_set_tear_scanline(dsi_var,es)@p;
> +mipi_dsi_dcs_set_tear_scanline_multi(&dsi_ctx,es);
> )

You're not matching on msleep because it doesn't return anything, and
here you're expecting a return value. You need to make 'r =' optional.

Maxime

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

[PATCH] drm/panel/synaptics-r63353: Use _multi variants

[Anusha Srivatsa]

Move away from using deprecated API and use _multi
variants if available. Use mipi_dsi_msleep()
and mipi_dsi_usleep_range() instead of msleep()
and usleep_range() respectively.

Used Coccinelle to find the _multi variant APIs,
replacing mpi_dsi_msleep() where necessary and for returning
dsi_ctx.accum_err in these functions. Manually handled the
reset step before returning from r63353_panel_activate()

v2: Do not skip the reset in case of error during
panel activate (Dmitry)
- Convert all usleep_range()

v3: mipi_dsi_usleep_range() is to be used only when in
between _multi commands(Doug)
- Check for error once in the end while using _multi
variants (Doug)

@rule_1@
identifier dsi_var;
identifier r;
identifier func;
type t;
position p;
expression dsi_device;
expression list es;
@@
t func(...) {
...
struct mipi_dsi_device *dsi_var = dsi_device;
+struct mipi_dsi_multi_context dsi_ctx = { .dsi = dsi_var };
<+...
(
-r = mipi_dsi_dcs_nop(dsi_var)@p;
+mipi_dsi_dcs_nop_multi(&dsi_ctx);
|
-r = mipi_dsi_dcs_exit_sleep_mode(dsi_var)@p;
+mipi_dsi_dcs_exit_sleep_mode_multi(&dsi_ctx);
|
-r = mipi_dsi_dcs_enter_sleep_mode(dsi_var)@p;
+mipi_dsi_dcs_enter_sleep_mode_multi(&dsi_ctx);
|
-r = mipi_dsi_dcs_write_buffer(dsi_var,es)@p;
+mipi_dsi_dcs_write_buffer_multi(&dsi_ctx,es);
|
-r = mipi_dsi_dcs_set_display_off(dsi_var,es)@p;
+mipi_dsi_dcs_set_display_off_multi(&dsi_ctx,es);
|
-r = mipi_dsi_compression_mode_ext(dsi_var,es)@p;
+mipi_dsi_compression_mode_ext_multi(&dsi_ctx,es);
|
-r = mipi_dsi_compression_mode(dsi_var,es)@p;
+mipi_dsi_compression_mode_multi(&dsi_ctx,es);
|
-r = mipi_dsi_picture_parameter_set(dsi_var,es)@p;
+mipi_dsi_picture_parameter_set_multi(&dsi_ctx,es);
|
-r = mipi_dsi_dcs_set_display_on(dsi_var,es)@p;
+mipi_dsi_dcs_set_display_on_multi(&dsi_ctx,es);
|
-r = mipi_dsi_dcs_set_tear_on(dsi_var)@p;
+mipi_dsi_dcs_set_tear_on_multi(&dsi_ctx);
|
-r = mipi_dsi_turn_on_peripheral(dsi_var)@p;
+mipi_dsi_turn_on_peripheral_multi(&dsi_ctx);
|
-r = mipi_dsi_dcs_soft_reset(dsi_var)@p;
+mipi_dsi_dcs_soft_reset_multi(&dsi_ctx);
|
-r = mipi_dsi_dcs_set_display_brightness(dsi_var,es)@p;
+mipi_dsi_dcs_set_display_brightness_multi(&dsi_ctx,es);
|
-r = mipi_dsi_dcs_set_pixel_format(dsi_var,es)@p;
+mipi_dsi_dcs_set_pixel_format_multi(&dsi_ctx,es);
|
-r = mipi_dsi_dcs_set_column_address(dsi_var,es)@p;
+mipi_dsi_dcs_set_column_address_multi(&dsi_ctx,es);
|
-r = mipi_dsi_dcs_set_page_address(dsi_var,es)@p;
+mipi_dsi_dcs_set_page_address_multi(&dsi_ctx,es);
|
-r = mipi_dsi_dcs_set_tear_scanline(dsi_var,es)@p;
+mipi_dsi_dcs_set_tear_scanline_multi(&dsi_ctx,es);
)
-if(r < 0) {
-...
-}
...+>
}

@rule_2@
identifier dsi_var;
identifier r;
identifier func;
type t;
position p;
expression dsi_device;
expression list es;
@@
t func(...) {
...
struct mipi_dsi_multi_context dsi_ctx = { .dsi = dsi_var };
<+...
(
-r = msleep(es)@p;
+r = mipi_dsi_msleep(&dsi_ctx,es);
|
-msleep(es)@p;
+mipi_dsi_msleep(&dsi_ctx,es);
|
-r = usleep_range(es)@p;
+r = mipi_dsi_usleep_range(&dsi_ctx,es);
|
-usleep_range(es)@p;
+mipi_dsi_usleep_range(&dsi_ctx,es);
)
...+>
}

@rule_3@
identifier dsi_var;
identifier func;
type t;
position p;
expression list es;
@@
t func(...) {
...
struct mipi_dsi_multi_context dsi_ctx = { .dsi = dsi_var };
...
-return 0;
+return dsi_ctx.accum_err;
}

Cc: Maxime Ripard <mripard@kernel.org>
Cc: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
Cc: Tejas Vipin <tejasvipin76@gmail.com>
Cc: Doug Anderson <dianders@chromium.org>
Signed-off-by: Anusha Srivatsa <asrivats@redhat.com>
---
Previous attempt for this change was addressed in:[1]
The series did not handle returns properly and still
used msleep() and usleep_range() API.
It also collided with an Tejas's similar efforts.

Will be sending the patches per driver instead of
major haul of changes.

Following [2] for reference.

[1] -> https://patchwork.freedesktop.org/series/144824/
[2] -> https://lore.kernel.org/dri-devel/20250220045721.145905-1-tejasvipin76@gmail.com/#iZ31drivers:gpu:drm:panel:panel-sony-td4353-jdi.c
---
Changes in v3:
- Modify the script to handle returns and msleeps ()
- handle reset in case of error properly

- Link to v2: https://lore.kernel.org/all/20250310-mipi-synaptic-1-v2-1-20ee4397c670@redhat.com/
__
Changes in v2:
- Handle the reset case properly 
- Handle msleep() and  usleep_range()

- Link to v1: https://lore.kernel.org/r/20250305-mipi-synaptic-1-v1-1-92017cd19ef6@redhat.com
---
 drivers/gpu/drm/panel/panel-synaptics-r63353.c | 66 ++++++++------------------
 1 file changed, 19 insertions(+), 47 deletions(-)

diff --git a/drivers/gpu/drm/panel/panel-synaptics-r63353.c b/drivers/gpu/drm/panel/panel-synaptics-r63353.c
index 17349825543fe6a117bbfd9cb92a564ce433d13a..e792d6124183d881d07cbafdd0e2dd1a0d034a6f 100644
--- a/drivers/gpu/drm/panel/panel-synaptics-r63353.c
+++ b/drivers/gpu/drm/panel/panel-synaptics-r63353.c
@@ -106,53 +106,34 @@ static int r63353_panel_power_off(struct r63353_panel *rpanel)
 static int r63353_panel_activate(struct r63353_panel *rpanel)
 {
 	struct mipi_dsi_device *dsi = rpanel->dsi;
-	struct device *dev = &dsi->dev;
-	int i, ret;
+	struct mipi_dsi_multi_context dsi_ctx = { .dsi = dsi };
+	int i;
 
-	ret = mipi_dsi_dcs_soft_reset(dsi);
-	if (ret < 0) {
-		dev_err(dev, "Failed to do Software Reset (%d)\n", ret);
-		goto fail;
-	}
+	mipi_dsi_dcs_soft_reset_multi(&dsi_ctx);
 
-	usleep_range(15000, 17000);
+	mipi_dsi_usleep_range(&dsi_ctx, 15000, 17000);
 
-	ret = mipi_dsi_dcs_enter_sleep_mode(dsi);
-	if (ret < 0) {
-		dev_err(dev, "Failed to enter sleep mode (%d)\n", ret);
-		goto fail;
-	}
+	mipi_dsi_dcs_enter_sleep_mode_multi(&dsi_ctx);
 
 	for (i = 0; i < rpanel->pdata->init_length; i++) {
 		const struct r63353_instr *instr = &rpanel->pdata->init[i];
 
-		ret = mipi_dsi_dcs_write_buffer(dsi, instr->data, instr->len);
-		if (ret < 0)
-			goto fail;
+		mipi_dsi_dcs_write_buffer_multi(&dsi_ctx, instr->data,
+						instr->len);
 	}
 
-	msleep(120);
+	mipi_dsi_msleep(&dsi_ctx, 120);
 
-	ret = mipi_dsi_dcs_exit_sleep_mode(dsi);
-	if (ret < 0) {
-		dev_err(dev, "Failed to exit sleep mode (%d)\n", ret);
-		goto fail;
-	}
+	mipi_dsi_dcs_exit_sleep_mode_multi(&dsi_ctx);
 
-	usleep_range(5000, 10000);
+	mipi_dsi_usleep_range(&dsi_ctx, 5000, 10000);
 
-	ret = mipi_dsi_dcs_set_display_on(dsi);
-	if (ret < 0) {
-		dev_err(dev, "Failed to set display ON (%d)\n", ret);
-		goto fail;
-	}
-
-	return 0;
+	mipi_dsi_dcs_set_display_on_multi(&dsi_ctx);
 
-fail:
-	gpiod_set_value(rpanel->reset_gpio, 0);
+	if (dsi_ctx.accum_err)
+		gpiod_set_value(rpanel->reset_gpio, 0);
 
-	return ret;
+	return dsi_ctx.accum_err;
 }
 
 static int r63353_panel_prepare(struct drm_panel *panel)
@@ -181,24 +162,15 @@ static int r63353_panel_prepare(struct drm_panel *panel)
 static int r63353_panel_deactivate(struct r63353_panel *rpanel)
 {
 	struct mipi_dsi_device *dsi = rpanel->dsi;
-	struct device *dev = &dsi->dev;
-	int ret;
+	struct mipi_dsi_multi_context dsi_ctx = { .dsi = dsi };
 
-	ret = mipi_dsi_dcs_set_display_off(dsi);
-	if (ret < 0) {
-		dev_err(dev, "Failed to set display OFF (%d)\n", ret);
-		return ret;
-	}
+	mipi_dsi_dcs_set_display_off_multi(&dsi_ctx);
 
-	usleep_range(5000, 10000);
+	mipi_dsi_usleep_range(&dsi_ctx, 5000, 10000);
 
-	ret = mipi_dsi_dcs_enter_sleep_mode(dsi);
-	if (ret < 0) {
-		dev_err(dev, "Failed to enter sleep mode (%d)\n", ret);
-		return ret;
-	}
+	mipi_dsi_dcs_enter_sleep_mode_multi(&dsi_ctx);
 
-	return 0;
+	return dsi_ctx.accum_err;
 }
 
 static int r63353_panel_unprepare(struct drm_panel *panel)

---
base-commit: dbe74119ff71c00f2d863a32f72aab2d15e61c39
change-id: 20250314-b4-mipi-synaptic-fb2a90c0f5f0

Best regards,
-- 
Anusha Srivatsa <asrivats@redhat.com>

Re: [PATCH] drm/panel/synaptics-r63353: Use _multi variants

[Doug Anderson]

Hi,

On Thu, Mar 13, 2025 at 9:47 PM Anusha Srivatsa <asrivats@redhat.com> wrote:
>
> @@ -181,24 +162,15 @@ static int r63353_panel_prepare(struct drm_panel *panel)
>  static int r63353_panel_deactivate(struct r63353_panel *rpanel)
>  {
>         struct mipi_dsi_device *dsi = rpanel->dsi;
> -       struct device *dev = &dsi->dev;
> -       int ret;
> +       struct mipi_dsi_multi_context dsi_ctx = { .dsi = dsi };
>
> -       ret = mipi_dsi_dcs_set_display_off(dsi);
> -       if (ret < 0) {
> -               dev_err(dev, "Failed to set display OFF (%d)\n", ret);
> -               return ret;
> -       }
> +       mipi_dsi_dcs_set_display_off_multi(&dsi_ctx);
>
> -       usleep_range(5000, 10000);
> +       mipi_dsi_usleep_range(&dsi_ctx, 5000, 10000);
>
> -       ret = mipi_dsi_dcs_enter_sleep_mode(dsi);
> -       if (ret < 0) {
> -               dev_err(dev, "Failed to enter sleep mode (%d)\n", ret);
> -               return ret;
> -       }
> +       mipi_dsi_dcs_enter_sleep_mode_multi(&dsi_ctx);
>
> -       return 0;
> +       return dsi_ctx.accum_err;

nit: the one caller of r63353_panel_deactivate() doesn't actually look
at the error code, so this could be a function that returns "void".
That was true even before your patch, though. I wouldn't mind a
followup patch that fixed this. ;-)

In any case, the patch looks reasonable to me now.

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

Happy for someone else to apply it if they want. If not, I'll snooze
this for ~a week to give others a chance to comment and then plan to
push to drm-misc-next.


-Doug

Re: [PATCH] drm/panel/synaptics-r63353: Use _multi variants

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 4154 bytes --]

Hi

On Fri, Mar 14, 2025 at 12:47:16AM -0400, Anusha Srivatsa wrote:
> Move away from using deprecated API and use _multi
> variants if available. Use mipi_dsi_msleep()
> and mipi_dsi_usleep_range() instead of msleep()
> and usleep_range() respectively.
> 
> Used Coccinelle to find the _multi variant APIs,
> replacing mpi_dsi_msleep() where necessary and for returning
> dsi_ctx.accum_err in these functions. Manually handled the
> reset step before returning from r63353_panel_activate()
> 
> v2: Do not skip the reset in case of error during
> panel activate (Dmitry)
> - Convert all usleep_range()
> 
> v3: mipi_dsi_usleep_range() is to be used only when in
> between _multi commands(Doug)
> - Check for error once in the end while using _multi
> variants (Doug)
> 
> @rule_1@
> identifier dsi_var;
> identifier r;
> identifier func;
> type t;
> position p;
> expression dsi_device;
> expression list es;
> @@
> t func(...) {
> ...
> struct mipi_dsi_device *dsi_var = dsi_device;
> +struct mipi_dsi_multi_context dsi_ctx = { .dsi = dsi_var };
> <+...
> (
> -r = mipi_dsi_dcs_nop(dsi_var)@p;
> +mipi_dsi_dcs_nop_multi(&dsi_ctx);
> |
> -r = mipi_dsi_dcs_exit_sleep_mode(dsi_var)@p;
> +mipi_dsi_dcs_exit_sleep_mode_multi(&dsi_ctx);
> |
> -r = mipi_dsi_dcs_enter_sleep_mode(dsi_var)@p;
> +mipi_dsi_dcs_enter_sleep_mode_multi(&dsi_ctx);
> |
> -r = mipi_dsi_dcs_write_buffer(dsi_var,es)@p;
> +mipi_dsi_dcs_write_buffer_multi(&dsi_ctx,es);
> |
> -r = mipi_dsi_dcs_set_display_off(dsi_var,es)@p;
> +mipi_dsi_dcs_set_display_off_multi(&dsi_ctx,es);
> |
> -r = mipi_dsi_compression_mode_ext(dsi_var,es)@p;
> +mipi_dsi_compression_mode_ext_multi(&dsi_ctx,es);
> |
> -r = mipi_dsi_compression_mode(dsi_var,es)@p;
> +mipi_dsi_compression_mode_multi(&dsi_ctx,es);
> |
> -r = mipi_dsi_picture_parameter_set(dsi_var,es)@p;
> +mipi_dsi_picture_parameter_set_multi(&dsi_ctx,es);
> |
> -r = mipi_dsi_dcs_set_display_on(dsi_var,es)@p;
> +mipi_dsi_dcs_set_display_on_multi(&dsi_ctx,es);
> |
> -r = mipi_dsi_dcs_set_tear_on(dsi_var)@p;
> +mipi_dsi_dcs_set_tear_on_multi(&dsi_ctx);
> |
> -r = mipi_dsi_turn_on_peripheral(dsi_var)@p;
> +mipi_dsi_turn_on_peripheral_multi(&dsi_ctx);
> |
> -r = mipi_dsi_dcs_soft_reset(dsi_var)@p;
> +mipi_dsi_dcs_soft_reset_multi(&dsi_ctx);
> |
> -r = mipi_dsi_dcs_set_display_brightness(dsi_var,es)@p;
> +mipi_dsi_dcs_set_display_brightness_multi(&dsi_ctx,es);
> |
> -r = mipi_dsi_dcs_set_pixel_format(dsi_var,es)@p;
> +mipi_dsi_dcs_set_pixel_format_multi(&dsi_ctx,es);
> |
> -r = mipi_dsi_dcs_set_column_address(dsi_var,es)@p;
> +mipi_dsi_dcs_set_column_address_multi(&dsi_ctx,es);
> |
> -r = mipi_dsi_dcs_set_page_address(dsi_var,es)@p;
> +mipi_dsi_dcs_set_page_address_multi(&dsi_ctx,es);
> |
> -r = mipi_dsi_dcs_set_tear_scanline(dsi_var,es)@p;
> +mipi_dsi_dcs_set_tear_scanline_multi(&dsi_ctx,es);
> )
> -if(r < 0) {
> -...
> -}
> ...+>
> }
> 
> @rule_2@
> identifier dsi_var;
> identifier r;
> identifier func;
> type t;
> position p;
> expression dsi_device;
> expression list es;
> @@
> t func(...) {
> ...
> struct mipi_dsi_multi_context dsi_ctx = { .dsi = dsi_var };
> <+...
> (
> -r = msleep(es)@p;
> +r = mipi_dsi_msleep(&dsi_ctx,es);
> |
> -msleep(es)@p;
> +mipi_dsi_msleep(&dsi_ctx,es);
> |
> -r = usleep_range(es)@p;
> +r = mipi_dsi_usleep_range(&dsi_ctx,es);
> |
> -usleep_range(es)@p;
> +mipi_dsi_usleep_range(&dsi_ctx,es);
> )
> ...+>
> }
> 
> @rule_3@
> identifier dsi_var;
> identifier func;
> type t;
> position p;
> expression list es;
> @@
> t func(...) {
> ...
> struct mipi_dsi_multi_context dsi_ctx = { .dsi = dsi_var };
> ...
> -return 0;
> +return dsi_ctx.accum_err;
> }
> 
> Cc: Maxime Ripard <mripard@kernel.org>
> Cc: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> Cc: Tejas Vipin <tejasvipin76@gmail.com>
> Cc: Doug Anderson <dianders@chromium.org>
> Signed-off-by: Anusha Srivatsa <asrivats@redhat.com>

It's looking good, but you forgot to set the version. If you're not
using it already, b4 simplifies that a lot for you, you should be using
it.

Maxime

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 273 bytes --]

Re: [PATCH] drm/panel/synaptics-r63353: Use _multi variants

[Neil Armstrong]

On 16/03/2025 18:40, Anusha Srivatsa wrote:
> 
> 
> On Fri, Mar 14, 2025 at 10:20 AM Doug Anderson <dianders@chromium.org <mailto:dianders@chromium.org>> wrote:
> 
>     Hi,
> 
>     On Thu, Mar 13, 2025 at 9:47 PM Anusha Srivatsa <asrivats@redhat.com <mailto:asrivats@redhat.com>> wrote:
>      >
>      > @@ -181,24 +162,15 @@ static int r63353_panel_prepare(struct drm_panel *panel)
>      >  static int r63353_panel_deactivate(struct r63353_panel *rpanel)
>      >  {
>      >         struct mipi_dsi_device *dsi = rpanel->dsi;
>      > -       struct device *dev = &dsi->dev;
>      > -       int ret;
>      > +       struct mipi_dsi_multi_context dsi_ctx = { .dsi = dsi };
>      >
>      > -       ret = mipi_dsi_dcs_set_display_off(dsi);
>      > -       if (ret < 0) {
>      > -               dev_err(dev, "Failed to set display OFF (%d)\n", ret);
>      > -               return ret;
>      > -       }
>      > +       mipi_dsi_dcs_set_display_off_multi(&dsi_ctx);
>      >
>      > -       usleep_range(5000, 10000);
>      > +       mipi_dsi_usleep_range(&dsi_ctx, 5000, 10000);
>      >
>      > -       ret = mipi_dsi_dcs_enter_sleep_mode(dsi);
>      > -       if (ret < 0) {
>      > -               dev_err(dev, "Failed to enter sleep mode (%d)\n", ret);
>      > -               return ret;
>      > -       }
>      > +       mipi_dsi_dcs_enter_sleep_mode_multi(&dsi_ctx);
>      >
>      > -       return 0;
>      > +       return dsi_ctx.accum_err;
> 
>     nit: the one caller of r63353_panel_deactivate() doesn't actually look
>     at the error code, so this could be a function that returns "void".
>     That was true even before your patch, though. I wouldn't mind a
>     followup patch that fixed this. ;-)
> 
> 
> This is anyway not merged, Maybe better to fix right now instead of a follow up patch?

If you can, yeah fix it now !

> 
>     In any case, the patch looks reasonable to me now.
> 
>     Reviewed-by: Douglas Anderson <dianders@chromium.org <mailto:dianders@chromium.org>>
> 
> 
> Thanks :)
> 
> Anusha


Thanks,
Neil
> 
> 
>     Happy for someone else to apply it if they want. If not, I'll snooze
>     this for ~a week to give others a chance to comment and then plan to
>     push to drm-misc-next. 
> 
> 
> 
>     -Doug
>