On Fri, Mar 7, 2025 at 12:47 PM Maxime Ripard <mrip...@kernel.org> wrote:
> On Thu, Mar 06, 2025 at 02:12:14PM -0500, Anusha Srivatsa wrote:
> > On Thu, Mar 6, 2025 at 12:54 PM Doug Anderson <diand...@chromium.org>
> wrote:
> > > On Thu, Mar 6, 2025 at 9:20 AM Maxime Ripard <mrip...@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 <mrip...@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.
>
> Yup! works now :)
Anusha
> Maxime
>
