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

[Maxime Ripard]

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 at chromium.org> wrote:
> > On Thu, Mar 6, 2025 at 9:20 AM Maxime Ripard <mripard at 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 at 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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 228 bytes
Desc: not available
URL: <https://lists.freedesktop.org/archives/dri-devel/attachments/20250307/770ad47e/attachment.sig>