[PATCH 00/20] drm/panel: Move to using mipi_dsi_*_multi() variants when available

Anusha Srivatsa posted 20 patches 10 months, 1 week ago
Documentation/gpu/todo.rst                         |   19 -
drivers/gpu/drm/drm_mipi_dsi.c                     |   56 -
.../gpu/drm/panel/panel-asus-z00t-tm5p5-n35596.c   |    6 +-
drivers/gpu/drm/panel/panel-boe-bf060y8m-aj0.c     |   75 +-
drivers/gpu/drm/panel/panel-dsi-cm.c               |   44 +-
drivers/gpu/drm/panel/panel-ebbg-ft8719.c          |   45 +-
drivers/gpu/drm/panel/panel-himax-hx8394.c         |  389 +++--
drivers/gpu/drm/panel/panel-jdi-lpm102a188a.c      |  141 +-
drivers/gpu/drm/panel/panel-jdi-lt070me05000.c     |   81 +-
drivers/gpu/drm/panel/panel-novatek-nt36523.c      | 1678 ++++++++++----------
drivers/gpu/drm/panel/panel-raydium-rm67191.c      |   60 +-
drivers/gpu/drm/panel/panel-samsung-s6d7aa0.c      |  101 +-
.../drm/panel/panel-samsung-s6e88a0-ams452ef01.c   |   63 +-
drivers/gpu/drm/panel/panel-samsung-sofef00.c      |   54 +-
drivers/gpu/drm/panel/panel-sharp-ls043t1le01.c    |   28 +-
drivers/gpu/drm/panel/panel-sharp-ls060t1sx01.c    |   34 +-
drivers/gpu/drm/panel/panel-sony-td4353-jdi.c      |   71 +-
.../gpu/drm/panel/panel-sony-tulip-truly-nt35521.c |    6 +-
drivers/gpu/drm/panel/panel-visionox-r66451.c      |  156 +-
drivers/gpu/drm/panel/panel-xinpeng-xpp055c272.c   |  138 +-
include/drm/drm_mipi_dsi.h                         |   47 -
21 files changed, 1432 insertions(+), 1860 deletions(-)
[PATCH 00/20] drm/panel: Move to using mipi_dsi_*_multi() variants when available
Posted by Anusha Srivatsa 10 months, 1 week ago
A lot of mipi API are deprecated and have a _multi() variant
which is the preferred way forward. This covers  TODO in the
gpu Documentation:[1]

An incomplete effort was made in the previous version
to address this[2]. It removed on the mipi_dsi_dcs_write_seq()
and mipi_dsi_generic_write_seq_multi() with the respective
replacemts and not the rest of the API.

The Coccinelle patch used to mkae changes:
@rule_1@
expression dsi_var;
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_2@
expression dsi_var;
expression list es;
identifier jdi;
@@
struct mipi_dsi_device *dsi0 = pinfo->dsi[0];
struct mipi_dsi_device *dsi1 = pinfo->dsi[1];
+struct mipi_dsi_multi_context dsi_ctx0 = { .dsi = dsi0 };
+struct mipi_dsi_multi_context dsi_ctx1 = { .dsi = dsi1 };
<+...
-mipi_dsi_dual_dcs_write_seq(dsi0, dsi1, es);
+mipi_dsi_dual_dcs_write_seq(dsi_ctx0, dsi_ctx1, es);
...+>

@rule_3@
identifier func;
identifier r;
type t;
expression list es;
position p;
@@
t func(...) {
<+...
(
-mipi_dsi_dcs_write_seq(dsi_var,es);
+mipi_dsi_dcs_write_seq_multi(&dsi_ctx,es);
|
-r = mipi_dsi_dcs_exit_sleep_mode(jdi->link1)@p;
+mipi_dsi_dcs_exit_sleep_mode_multi(&dsi_ctx1);
|
-r = mipi_dsi_dcs_enter_sleep_mode(jdi->link1)@p;
+mipi_dsi_dcs_enter_sleep_mode_multi(&dsi_ctx1);
|
-r = mipi_dsi_dcs_set_display_off(jdi->link1)@p;
+mipi_dsi_dcs_set_display_off_multi(&dsi_ctx1);
|
.....//rest of the mipi APIs with _multi variant
)
<+...
-if(r < 0) {
-...
-}
...+>

[1] -> https://docs.kernel.org/gpu/todo.html#transition-away-from-using-mipi-dsi-write-seq
[2] -> https://patchwork.freedesktop.org/series/144446/

Signed-off-by: Anusha Srivatsa <asrivats@redhat.com>
---
Anusha Srivatsa (20):
      drm/panel/xpp055c272: Move to using mipi_dsi_*_multi() variants
      drm/panel/visionox-r66451: Move to using mipi_dsi_*_multi() variants
      drm/panel/asus-tm5p5-n35596: Move to using mipi_dsi_*_multi() variants
      drm/panel/boe-bf060y8m-aj0: Move to using mipi_dsi_*_multi() variants
      drm/panel/dsi-cm: Move to using mipi_dsi_*_multi() variants
      drm/panel/sony-nt35521: Move to using mipi_dsi_*_multi() variants
      drm/panel/ebbg-ft8719: Move to using mipi_dsi_*_multi() variants
      drm/panel/himax-hx8394: Move to using mipi_dsi_*_multi() variants
      drm/panel/jdi-lpm102a188a: Move to using mipi_dsi_*_multi() variants
      drm/panel/jdi-lt070me05000: Move to using mipi_dsi_*_multi() variants
      drm/panel/novatek-nt36523: Move to using mipi_dsi_*_multi() variants
      drm/panel/raydium-rm67191: Move to using mipi_dsi_*_multi() variants
      drm/panel/samsung-s6d7aa0:Move to using mipi_dsi_*_multi() variants
      drm/panel/s6e88a0-ams452ef01: Move to using mipi_dsi_*_multi() variants
      drm/panel/samsung-sofef00: Move to using mipi_dsi_*_multi() variants
      drm/panel/ls043t1le01: Move to using mipi_dsi_*_multi() variants
      drm/panel/ls060t1sx01: Move to using mipi_dsi_*_multi() variants
      drm/panel/sony-td4353-jdi: Move to using mipi_dsi_*_multi() variants
      drm/panel: Remove deprecated functions
      Documentation: Update the documentation

 Documentation/gpu/todo.rst                         |   19 -
 drivers/gpu/drm/drm_mipi_dsi.c                     |   56 -
 .../gpu/drm/panel/panel-asus-z00t-tm5p5-n35596.c   |    6 +-
 drivers/gpu/drm/panel/panel-boe-bf060y8m-aj0.c     |   75 +-
 drivers/gpu/drm/panel/panel-dsi-cm.c               |   44 +-
 drivers/gpu/drm/panel/panel-ebbg-ft8719.c          |   45 +-
 drivers/gpu/drm/panel/panel-himax-hx8394.c         |  389 +++--
 drivers/gpu/drm/panel/panel-jdi-lpm102a188a.c      |  141 +-
 drivers/gpu/drm/panel/panel-jdi-lt070me05000.c     |   81 +-
 drivers/gpu/drm/panel/panel-novatek-nt36523.c      | 1678 ++++++++++----------
 drivers/gpu/drm/panel/panel-raydium-rm67191.c      |   60 +-
 drivers/gpu/drm/panel/panel-samsung-s6d7aa0.c      |  101 +-
 .../drm/panel/panel-samsung-s6e88a0-ams452ef01.c   |   63 +-
 drivers/gpu/drm/panel/panel-samsung-sofef00.c      |   54 +-
 drivers/gpu/drm/panel/panel-sharp-ls043t1le01.c    |   28 +-
 drivers/gpu/drm/panel/panel-sharp-ls060t1sx01.c    |   34 +-
 drivers/gpu/drm/panel/panel-sony-td4353-jdi.c      |   71 +-
 .../gpu/drm/panel/panel-sony-tulip-truly-nt35521.c |    6 +-
 drivers/gpu/drm/panel/panel-visionox-r66451.c      |  156 +-
 drivers/gpu/drm/panel/panel-xinpeng-xpp055c272.c   |  138 +-
 include/drm/drm_mipi_dsi.h                         |   47 -
 21 files changed, 1432 insertions(+), 1860 deletions(-)
---
base-commit: febbc555cf0fff895546ddb8ba2c9a523692fb55
change-id: 20250211-mipi_cocci_multi-440af15236c2

Best regards,
-- 
Anusha Srivatsa <asrivats@redhat.com>
Re: [PATCH 00/20] drm/panel: Move to using mipi_dsi_*_multi() variants when available
Posted by Doug Anderson 10 months, 1 week ago
Hi,

On Thu, Feb 13, 2025 at 12:44 PM Anusha Srivatsa <asrivats@redhat.com> wrote:
>
> A lot of mipi API are deprecated and have a _multi() variant
> which is the preferred way forward. This covers  TODO in the
> gpu Documentation:[1]
>
> An incomplete effort was made in the previous version
> to address this[2]. It removed on the mipi_dsi_dcs_write_seq()
> and mipi_dsi_generic_write_seq_multi() with the respective
> replacemts and not the rest of the API.

You didn't seem to take most of the suggestions I gave in response to
your v1 [3]. Specifically:

a) I asked that you CC Tejas. I've added him again.

b) I asked that you CC me on the whole patch series, which you didn't
do. I can find them, but I'd find it convenient in this case for them
to be in my Inbox.

The first patch conflicts with what Tejas already landed in
drm-misc-next. See commit 8025f23728e9 ("drm/panel:
xinpeng-xpp055c272: transition to mipi_dsi wrapped functions"). The
second patch _also_ conflicts with what Tejas already landed. See
commit f4dd4cb79f9e ("drm/panel: visionox-r66451: transition to
mipi_dsi wrapped functions"). Later patches also also conflict. See
commit 0d6c9edf9e5b ("drm/panel: ebbg-ft8719: transition to mipi_dsi
wrapped functions"), commit ce8c69ec90ca ("drm/panel:
samsung-s6e88a0-ams452ef01: transition to mipi_dsi wrapped
functions"), and commit 7e3bf00047cd ("drm/panel: sharp-ls060t1sx01:
transition to mipi_dsi wrapped functions"). Maybe you should sync up
with drm-misc-next before submitting.

I also questioned whether this really made sense to try to do with a
Coccinelle script and I still don't think so. It looks like Dmitry has
already reviewed the first few of your patches and has repeated my
advice. If you want to help with the effort of addressing this TODO
item then that's great, but I'll stop reviewing (and start silently
deleting) any future submissions of yours that say that they're done
entirely with a Coccinelle script unless you address this point and
convince me that your Coccinelle script is really smart enough to
handle all the corner cases. I'll also assert that you should review
Tejas's submissions to see how these conversions are expected to go.

[3] https://lore.kernel.org/r/CAD=FV=WkPefg00R_TAQQA6waRqGdD+3e84JXfPLk2i9BRzW6Yg@mail.gmail.com
Re: [PATCH 00/20] drm/panel: Move to using mipi_dsi_*_multi() variants when available
Posted by Maxime Ripard 10 months ago
On Fri, Feb 14, 2025 at 08:26:02AM -0800, Doug Anderson wrote:
> Hi,
> 
> On Thu, Feb 13, 2025 at 12:44 PM Anusha Srivatsa <asrivats@redhat.com> wrote:
> >
> > A lot of mipi API are deprecated and have a _multi() variant
> > which is the preferred way forward. This covers  TODO in the
> > gpu Documentation:[1]
> >
> > An incomplete effort was made in the previous version
> > to address this[2]. It removed on the mipi_dsi_dcs_write_seq()
> > and mipi_dsi_generic_write_seq_multi() with the respective
> > replacemts and not the rest of the API.
> 
> You didn't seem to take most of the suggestions I gave in response to
> your v1 [3]. Specifically:
> 
> a) I asked that you CC Tejas. I've added him again.
> 
> b) I asked that you CC me on the whole patch series, which you didn't
> do. I can find them, but I'd find it convenient in this case for them
> to be in my Inbox.
> 
> The first patch conflicts with what Tejas already landed in
> drm-misc-next. See commit 8025f23728e9 ("drm/panel:
> xinpeng-xpp055c272: transition to mipi_dsi wrapped functions"). The
> second patch _also_ conflicts with what Tejas already landed. See
> commit f4dd4cb79f9e ("drm/panel: visionox-r66451: transition to
> mipi_dsi wrapped functions"). Later patches also also conflict. See
> commit 0d6c9edf9e5b ("drm/panel: ebbg-ft8719: transition to mipi_dsi
> wrapped functions"), commit ce8c69ec90ca ("drm/panel:
> samsung-s6e88a0-ams452ef01: transition to mipi_dsi wrapped
> functions"), and commit 7e3bf00047cd ("drm/panel: sharp-ls060t1sx01:
> transition to mipi_dsi wrapped functions"). Maybe you should sync up
> with drm-misc-next before submitting.

Yes, you should definitely work from drm-misc-next there, and sync with
Tejas.

> I also questioned whether this really made sense to try to do with a
> Coccinelle script and I still don't think so. It looks like Dmitry has
> already reviewed the first few of your patches and has repeated my
> advice. If you want to help with the effort of addressing this TODO
> item then that's great, but I'll stop reviewing (and start silently
> deleting) any future submissions of yours that say that they're done
> entirely with a Coccinelle script unless you address this point and
> convince me that your Coccinelle script is really smart enough to
> handle all the corner cases. I'll also assert that you should review
> Tejas's submissions to see how these conversions are expected to go.

I couldn't find that in your first answer though. What corner cases do
you have in mind, and why do you think coccinelle can't handle them?

Also, why do you think ignoring a contributor after a second mistake is
a reasonable reaction?

Anusha, most of those comments aren't the end of the discussion though.
If you feel like something's not clear enough or ambiguous, feel free to
ask for more details and keep the discussion going.

Maxime
Re: [PATCH 00/20] drm/panel: Move to using mipi_dsi_*_multi() variants when available
Posted by Doug Anderson 9 months, 3 weeks ago
Hi,

On Tue, Feb 18, 2025 at 1:55 AM Maxime Ripard <mripard@kernel.org> wrote:
>
> > I also questioned whether this really made sense to try to do with a
> > Coccinelle script and I still don't think so. It looks like Dmitry has
> > already reviewed the first few of your patches and has repeated my
> > advice. If you want to help with the effort of addressing this TODO
> > item then that's great, but I'll stop reviewing (and start silently
> > deleting) any future submissions of yours that say that they're done
> > entirely with a Coccinelle script unless you address this point and
> > convince me that your Coccinelle script is really smart enough to
> > handle all the corner cases. I'll also assert that you should review
> > Tejas's submissions to see how these conversions are expected to go.
>
> I couldn't find that in your first answer though. What corner cases do
> you have in mind, and why do you think coccinelle can't handle them?

My gut says that it is a difficult problem to make this kind of change
purely with Coccinelle. That's not to say I couldn't be convinced if
someone gave some good evidence showing some amazing patches generated
by a Cocinelle script. To show this, I would expect someone to
understand what Tejas has been doing and then compare that to what the
script can produce. In theory, you could even run the script on an old
version of panels (before Tejas's fixes) and compare what the script
does. If you can make the results nearly the same then that's amazing.


> Also, why do you think ignoring a contributor after a second mistake is
> a reasonable reaction?

Just to be clear, I said that "unless you address this point and
convince me...". My problem was that I brought up the questions of the
suitability of Cocinelle for this problem in response to v1. Then I
saw v2 posted without any reply to my concerns and with the same types
of problems. I was simply saying that if a v3 was posted in the same
vein then I would ignore it. I'm more than happy to have a
conversation, but if I start getting new versions that ignore previous
feedback and no response saying why feedback was ignored then I will
start ignoring new versions. That feels pretty reasonable to me.
Re: [PATCH 00/20] drm/panel: Move to using mipi_dsi_*_multi() variants when available
Posted by Dmitry Baryshkov 10 months ago
On Tue, Feb 18, 2025 at 10:55:49AM +0100, Maxime Ripard wrote:
> On Fri, Feb 14, 2025 at 08:26:02AM -0800, Doug Anderson wrote:
> > Hi,
> > 
> > On Thu, Feb 13, 2025 at 12:44 PM Anusha Srivatsa <asrivats@redhat.com> wrote:
> > >
> > > A lot of mipi API are deprecated and have a _multi() variant
> > > which is the preferred way forward. This covers  TODO in the
> > > gpu Documentation:[1]
> > >
> > > An incomplete effort was made in the previous version
> > > to address this[2]. It removed on the mipi_dsi_dcs_write_seq()
> > > and mipi_dsi_generic_write_seq_multi() with the respective
> > > replacemts and not the rest of the API.
> > 
> > You didn't seem to take most of the suggestions I gave in response to
> > your v1 [3]. Specifically:
> > 
> > a) I asked that you CC Tejas. I've added him again.
> > 
> > b) I asked that you CC me on the whole patch series, which you didn't
> > do. I can find them, but I'd find it convenient in this case for them
> > to be in my Inbox.
> > 
> > The first patch conflicts with what Tejas already landed in
> > drm-misc-next. See commit 8025f23728e9 ("drm/panel:
> > xinpeng-xpp055c272: transition to mipi_dsi wrapped functions"). The
> > second patch _also_ conflicts with what Tejas already landed. See
> > commit f4dd4cb79f9e ("drm/panel: visionox-r66451: transition to
> > mipi_dsi wrapped functions"). Later patches also also conflict. See
> > commit 0d6c9edf9e5b ("drm/panel: ebbg-ft8719: transition to mipi_dsi
> > wrapped functions"), commit ce8c69ec90ca ("drm/panel:
> > samsung-s6e88a0-ams452ef01: transition to mipi_dsi wrapped
> > functions"), and commit 7e3bf00047cd ("drm/panel: sharp-ls060t1sx01:
> > transition to mipi_dsi wrapped functions"). Maybe you should sync up
> > with drm-misc-next before submitting.
> 
> Yes, you should definitely work from drm-misc-next there, and sync with
> Tejas.
> 
> > I also questioned whether this really made sense to try to do with a
> > Coccinelle script and I still don't think so. It looks like Dmitry has
> > already reviewed the first few of your patches and has repeated my
> > advice. If you want to help with the effort of addressing this TODO
> > item then that's great, but I'll stop reviewing (and start silently
> > deleting) any future submissions of yours that say that they're done
> > entirely with a Coccinelle script unless you address this point and
> > convince me that your Coccinelle script is really smart enough to
> > handle all the corner cases. I'll also assert that you should review
> > Tejas's submissions to see how these conversions are expected to go.
> 
> I couldn't find that in your first answer though. What corner cases do
> you have in mind, and why do you think coccinelle can't handle them?

As can be seen from the reviews:

- sleeps between DSI calls
- properly propagating the error at the end of the function
- making decision whether to create the context at the caller or the
  callee side. E.g. in patch 8 it is better to allocate context in
  hx8394_enable() and pass it to .init_sequence() instead of keeping
  some of error handling.

> Also, why do you think ignoring a contributor after a second mistake is
> a reasonable reaction?
> 
> Anusha, most of those comments aren't the end of the discussion though.
> If you feel like something's not clear enough or ambiguous, feel free to
> ask for more details and keep the discussion going.

From my side: feel free to ask for the details if any of the emails is
not clear enough. At the same time, please review your patches before
sending them. Returning 0 in case there was an error is an obvious
issue.

-- 
With best wishes
Dmitry
Re: [PATCH 00/20] drm/panel: Move to using mipi_dsi_*_multi() variants when available
Posted by Maxime Ripard 10 months ago
On Tue, Feb 18, 2025 at 02:14:43PM +0200, Dmitry Baryshkov wrote:
> On Tue, Feb 18, 2025 at 10:55:49AM +0100, Maxime Ripard wrote:
> > On Fri, Feb 14, 2025 at 08:26:02AM -0800, Doug Anderson wrote:
> > > Hi,
> > > 
> > > On Thu, Feb 13, 2025 at 12:44 PM Anusha Srivatsa <asrivats@redhat.com> wrote:
> > > >
> > > > A lot of mipi API are deprecated and have a _multi() variant
> > > > which is the preferred way forward. This covers  TODO in the
> > > > gpu Documentation:[1]
> > > >
> > > > An incomplete effort was made in the previous version
> > > > to address this[2]. It removed on the mipi_dsi_dcs_write_seq()
> > > > and mipi_dsi_generic_write_seq_multi() with the respective
> > > > replacemts and not the rest of the API.
> > > 
> > > You didn't seem to take most of the suggestions I gave in response to
> > > your v1 [3]. Specifically:
> > > 
> > > a) I asked that you CC Tejas. I've added him again.
> > > 
> > > b) I asked that you CC me on the whole patch series, which you didn't
> > > do. I can find them, but I'd find it convenient in this case for them
> > > to be in my Inbox.
> > > 
> > > The first patch conflicts with what Tejas already landed in
> > > drm-misc-next. See commit 8025f23728e9 ("drm/panel:
> > > xinpeng-xpp055c272: transition to mipi_dsi wrapped functions"). The
> > > second patch _also_ conflicts with what Tejas already landed. See
> > > commit f4dd4cb79f9e ("drm/panel: visionox-r66451: transition to
> > > mipi_dsi wrapped functions"). Later patches also also conflict. See
> > > commit 0d6c9edf9e5b ("drm/panel: ebbg-ft8719: transition to mipi_dsi
> > > wrapped functions"), commit ce8c69ec90ca ("drm/panel:
> > > samsung-s6e88a0-ams452ef01: transition to mipi_dsi wrapped
> > > functions"), and commit 7e3bf00047cd ("drm/panel: sharp-ls060t1sx01:
> > > transition to mipi_dsi wrapped functions"). Maybe you should sync up
> > > with drm-misc-next before submitting.
> > 
> > Yes, you should definitely work from drm-misc-next there, and sync with
> > Tejas.
> > 
> > > I also questioned whether this really made sense to try to do with a
> > > Coccinelle script and I still don't think so. It looks like Dmitry has
> > > already reviewed the first few of your patches and has repeated my
> > > advice. If you want to help with the effort of addressing this TODO
> > > item then that's great, but I'll stop reviewing (and start silently
> > > deleting) any future submissions of yours that say that they're done
> > > entirely with a Coccinelle script unless you address this point and
> > > convince me that your Coccinelle script is really smart enough to
> > > handle all the corner cases. I'll also assert that you should review
> > > Tejas's submissions to see how these conversions are expected to go.
> > 
> > I couldn't find that in your first answer though. What corner cases do
> > you have in mind, and why do you think coccinelle can't handle them?
> 
> As can be seen from the reviews:
> 
> - sleeps between DSI calls
> - properly propagating the error at the end of the function

These two are legitimate feedback, but I don't see how coccinelle cannot
deal with them.

> - making decision whether to create the context at the caller or the
>   callee side. E.g. in patch 8 it is better to allocate context in
>   hx8394_enable() and pass it to .init_sequence() instead of keeping
>   some of error handling.

Yeah, that one is definitely subjective, and is going to need manual
review.

Maxime
Re: [PATCH 00/20] drm/panel: Move to using mipi_dsi_*_multi() variants when available
Posted by Dmitry Baryshkov 10 months ago
On Tue, Feb 18, 2025 at 04:52:53PM +0100, Maxime Ripard wrote:
> On Tue, Feb 18, 2025 at 02:14:43PM +0200, Dmitry Baryshkov wrote:
> > On Tue, Feb 18, 2025 at 10:55:49AM +0100, Maxime Ripard wrote:
> > > On Fri, Feb 14, 2025 at 08:26:02AM -0800, Doug Anderson wrote:
> > > > Hi,
> > > > 
> > > > On Thu, Feb 13, 2025 at 12:44 PM Anusha Srivatsa <asrivats@redhat.com> wrote:
> > > > >
> > > > > A lot of mipi API are deprecated and have a _multi() variant
> > > > > which is the preferred way forward. This covers  TODO in the
> > > > > gpu Documentation:[1]
> > > > >
> > > > > An incomplete effort was made in the previous version
> > > > > to address this[2]. It removed on the mipi_dsi_dcs_write_seq()
> > > > > and mipi_dsi_generic_write_seq_multi() with the respective
> > > > > replacemts and not the rest of the API.
> > > > 
> > > > You didn't seem to take most of the suggestions I gave in response to
> > > > your v1 [3]. Specifically:
> > > > 
> > > > a) I asked that you CC Tejas. I've added him again.
> > > > 
> > > > b) I asked that you CC me on the whole patch series, which you didn't
> > > > do. I can find them, but I'd find it convenient in this case for them
> > > > to be in my Inbox.
> > > > 
> > > > The first patch conflicts with what Tejas already landed in
> > > > drm-misc-next. See commit 8025f23728e9 ("drm/panel:
> > > > xinpeng-xpp055c272: transition to mipi_dsi wrapped functions"). The
> > > > second patch _also_ conflicts with what Tejas already landed. See
> > > > commit f4dd4cb79f9e ("drm/panel: visionox-r66451: transition to
> > > > mipi_dsi wrapped functions"). Later patches also also conflict. See
> > > > commit 0d6c9edf9e5b ("drm/panel: ebbg-ft8719: transition to mipi_dsi
> > > > wrapped functions"), commit ce8c69ec90ca ("drm/panel:
> > > > samsung-s6e88a0-ams452ef01: transition to mipi_dsi wrapped
> > > > functions"), and commit 7e3bf00047cd ("drm/panel: sharp-ls060t1sx01:
> > > > transition to mipi_dsi wrapped functions"). Maybe you should sync up
> > > > with drm-misc-next before submitting.
> > > 
> > > Yes, you should definitely work from drm-misc-next there, and sync with
> > > Tejas.
> > > 
> > > > I also questioned whether this really made sense to try to do with a
> > > > Coccinelle script and I still don't think so. It looks like Dmitry has
> > > > already reviewed the first few of your patches and has repeated my
> > > > advice. If you want to help with the effort of addressing this TODO
> > > > item then that's great, but I'll stop reviewing (and start silently
> > > > deleting) any future submissions of yours that say that they're done
> > > > entirely with a Coccinelle script unless you address this point and
> > > > convince me that your Coccinelle script is really smart enough to
> > > > handle all the corner cases. I'll also assert that you should review
> > > > Tejas's submissions to see how these conversions are expected to go.
> > > 
> > > I couldn't find that in your first answer though. What corner cases do
> > > you have in mind, and why do you think coccinelle can't handle them?
> > 
> > As can be seen from the reviews:
> > 
> > - sleeps between DSI calls
> > - properly propagating the error at the end of the function
> 
> These two are legitimate feedback, but I don't see how coccinelle cannot
> deal with them.

Maybe it can. both issues were pointed out during review of the first
series, there was no improvement here. I'd really ask to perform
conversion of a single driver, so that the script (or post-script
fixups) can be improved. I'd still expect that Anusha actually reviews
the changed driver before posting it and verifies that there is no
regression in the logic / error reporting.

> 
> > - making decision whether to create the context at the caller or the
> >   callee side. E.g. in patch 8 it is better to allocate context in
> >   hx8394_enable() and pass it to .init_sequence() instead of keeping
> >   some of error handling.
> 
> Yeah, that one is definitely subjective, and is going to need manual
> review.

-- 
With best wishes
Dmitry
Re: [PATCH 00/20] drm/panel: Move to using mipi_dsi_*_multi() variants when available
Posted by Maxime Ripard 10 months ago
On Wed, Feb 19, 2025 at 11:11:33AM +0200, Dmitry Baryshkov wrote:
> On Tue, Feb 18, 2025 at 04:52:53PM +0100, Maxime Ripard wrote:
> > On Tue, Feb 18, 2025 at 02:14:43PM +0200, Dmitry Baryshkov wrote:
> > > On Tue, Feb 18, 2025 at 10:55:49AM +0100, Maxime Ripard wrote:
> > > > On Fri, Feb 14, 2025 at 08:26:02AM -0800, Doug Anderson wrote:
> > > > > Hi,
> > > > > 
> > > > > On Thu, Feb 13, 2025 at 12:44 PM Anusha Srivatsa <asrivats@redhat.com> wrote:
> > > > > >
> > > > > > A lot of mipi API are deprecated and have a _multi() variant
> > > > > > which is the preferred way forward. This covers  TODO in the
> > > > > > gpu Documentation:[1]
> > > > > >
> > > > > > An incomplete effort was made in the previous version
> > > > > > to address this[2]. It removed on the mipi_dsi_dcs_write_seq()
> > > > > > and mipi_dsi_generic_write_seq_multi() with the respective
> > > > > > replacemts and not the rest of the API.
> > > > > 
> > > > > You didn't seem to take most of the suggestions I gave in response to
> > > > > your v1 [3]. Specifically:
> > > > > 
> > > > > a) I asked that you CC Tejas. I've added him again.
> > > > > 
> > > > > b) I asked that you CC me on the whole patch series, which you didn't
> > > > > do. I can find them, but I'd find it convenient in this case for them
> > > > > to be in my Inbox.
> > > > > 
> > > > > The first patch conflicts with what Tejas already landed in
> > > > > drm-misc-next. See commit 8025f23728e9 ("drm/panel:
> > > > > xinpeng-xpp055c272: transition to mipi_dsi wrapped functions"). The
> > > > > second patch _also_ conflicts with what Tejas already landed. See
> > > > > commit f4dd4cb79f9e ("drm/panel: visionox-r66451: transition to
> > > > > mipi_dsi wrapped functions"). Later patches also also conflict. See
> > > > > commit 0d6c9edf9e5b ("drm/panel: ebbg-ft8719: transition to mipi_dsi
> > > > > wrapped functions"), commit ce8c69ec90ca ("drm/panel:
> > > > > samsung-s6e88a0-ams452ef01: transition to mipi_dsi wrapped
> > > > > functions"), and commit 7e3bf00047cd ("drm/panel: sharp-ls060t1sx01:
> > > > > transition to mipi_dsi wrapped functions"). Maybe you should sync up
> > > > > with drm-misc-next before submitting.
> > > > 
> > > > Yes, you should definitely work from drm-misc-next there, and sync with
> > > > Tejas.
> > > > 
> > > > > I also questioned whether this really made sense to try to do with a
> > > > > Coccinelle script and I still don't think so. It looks like Dmitry has
> > > > > already reviewed the first few of your patches and has repeated my
> > > > > advice. If you want to help with the effort of addressing this TODO
> > > > > item then that's great, but I'll stop reviewing (and start silently
> > > > > deleting) any future submissions of yours that say that they're done
> > > > > entirely with a Coccinelle script unless you address this point and
> > > > > convince me that your Coccinelle script is really smart enough to
> > > > > handle all the corner cases. I'll also assert that you should review
> > > > > Tejas's submissions to see how these conversions are expected to go.
> > > > 
> > > > I couldn't find that in your first answer though. What corner cases do
> > > > you have in mind, and why do you think coccinelle can't handle them?
> > > 
> > > As can be seen from the reviews:
> > > 
> > > - sleeps between DSI calls
> > > - properly propagating the error at the end of the function
> > 
> > These two are legitimate feedback, but I don't see how coccinelle cannot
> > deal with them.
> 
> Maybe it can. both issues were pointed out during review of the first
> series, there was no improvement here. I'd really ask to perform
> conversion of a single driver, so that the script (or post-script
> fixups) can be improved. I'd still expect that Anusha actually reviews
> the changed driver before posting it and verifies that there is no
> regression in the logic / error reporting.

Yeah, it makes sense, thanks!
Maxime