| Message ID | 20250310-mipi-synaptic-1-v2-1-20ee4397c670@redhat.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [v2] drm/panel/synaptics-r63353: Use _multi variants | expand |
On Mon, Mar 10, 2025 at 04:58:22PM -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 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) { > -... > -} > ...+> Again, you need to provide the full coccinelle script here otherwise it's useless. And I have serious doubts that it's actually the script you used, because ... > @@ -106,53 +107,46 @@ 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); > + mipi_dsi_dcs_soft_reset_multi(&dsi_ctx); > + if (dsi_ctx.accum_err) > goto fail; > - } This changes was definitely not what the script is doing. Maxime
Hi, On Mon, Mar 10, 2025 at 1:58 PM Anusha Srivatsa <asrivats@redhat.com> wrote: > > @@ -70,6 +70,7 @@ static int r63353_panel_power_on(struct r63353_panel *rpanel) > { > struct mipi_dsi_device *dsi = rpanel->dsi; > struct device *dev = &dsi->dev; > + struct mipi_dsi_multi_context dsi_ctx = { .dsi = dsi }; > int ret; > > ret = regulator_enable(rpanel->avdd); > @@ -78,7 +79,7 @@ static int r63353_panel_power_on(struct r63353_panel *rpanel) > return ret; > } > > - usleep_range(15000, 25000); > + mipi_dsi_usleep_range(&dsi_ctx, 15000, 25000); No. None of the conversions in this function are correct. mipi_dsi_usleep_range() is only for use when you're in the middle of a bunch of other "multi" calls and want the sleep to be conditional upon there being no error. Here there is no chance of an error because no _multi() are used. Go back to the normal usleep_range(). > @@ -106,53 +107,46 @@ 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); > + mipi_dsi_dcs_soft_reset_multi(&dsi_ctx); > + if (dsi_ctx.accum_err) > goto fail; > - } This isn't how the _multi() functions are intended to be used. The whole idea is _not_ to have scattered "if" statements everywhere and to just deal with errors at the appropriate places. You just trust that the _multi() functions are no-ops if an error is set and so it doesn't hurt to keep calling them. In this case you'd just have a pile of _multi() functions with no "if" checks and then at the very end of the function you check for the error. If the error is set then you set the reset GPIO and return the error. -Doug
On Tue, Mar 11, 2025 at 11:52 AM Doug Anderson <dianders@chromium.org> wrote: > Hi, > > On Mon, Mar 10, 2025 at 1:58 PM Anusha Srivatsa <asrivats@redhat.com> > wrote: > > > > @@ -70,6 +70,7 @@ static int r63353_panel_power_on(struct r63353_panel > *rpanel) > > { > > struct mipi_dsi_device *dsi = rpanel->dsi; > > struct device *dev = &dsi->dev; > > + struct mipi_dsi_multi_context dsi_ctx = { .dsi = dsi }; > > int ret; > > > > ret = regulator_enable(rpanel->avdd); > > @@ -78,7 +79,7 @@ static int r63353_panel_power_on(struct r63353_panel > *rpanel) > > return ret; > > } > > > > - usleep_range(15000, 25000); > > + mipi_dsi_usleep_range(&dsi_ctx, 15000, 25000); > > No. None of the conversions in this function are correct. > mipi_dsi_usleep_range() is only for use when you're in the middle of a > bunch of other "multi" calls and want the sleep to be conditional upon > there being no error. Here there is no chance of an error because no > _multi() are used. Go back to the normal usleep_range(). > > OK. Then the approach to prefer mipi_dsi_usleep_range() over the previously used usleep_range() everywhere is out the window. Sounds good. Is replacing msleep() with mipi_dsi_msleep() preferable? > @@ -106,53 +107,46 @@ 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); > > + mipi_dsi_dcs_soft_reset_multi(&dsi_ctx); > > + if (dsi_ctx.accum_err) > > goto fail; > > - } > > This isn't how the _multi() functions are intended to be used. The > whole idea is _not_ to have scattered "if" statements everywhere and > to just deal with errors at the appropriate places. You just trust > that the _multi() functions are no-ops if an error is set and so it > doesn't hurt to keep calling them. In this case you'd just have a pile > of _multi() functions with no "if" checks and then at the very end of > the function you check for the error. If the error is set then you set > the reset GPIO and return the error. > > Perfect. Thanks. Anusha > -Doug > >
On Tue, Mar 11, 2025 at 3:31 AM Maxime Ripard <mripard@kernel.org> wrote: > On Mon, Mar 10, 2025 at 04:58:22PM -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 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) { > > -... > > -} > > ...+> > > Again, you need to provide the full coccinelle script here otherwise > it's useless. And I have serious doubts that it's actually the script > you used, because ... > > I had another rule just for msleeps and usleep(). The commit msg is getting too big with just the script. But yes, here you go: @rule_4@ 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 = 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); ) ...+> } > > @@ -106,53 +107,46 @@ 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); > > + mipi_dsi_dcs_soft_reset_multi(&dsi_ctx); > > + if (dsi_ctx.accum_err) > > goto fail; > > - } > > This changes was definitely not what the script is doing. > It isnt. Using coccinelle for the major part of pattern matching and replacing the newer _multi variant API. Some handling (including a newline that it introduces) and the returns depend on a case by case basis, which had to be done manually. Anusha > > Maxime >
Hi, On Wed, Mar 12, 2025 at 7:54 AM Anusha Srivatsa <asrivats@redhat.com> wrote: > >> > @@ -78,7 +79,7 @@ static int r63353_panel_power_on(struct r63353_panel *rpanel) >> > return ret; >> > } >> > >> > - usleep_range(15000, 25000); >> > + mipi_dsi_usleep_range(&dsi_ctx, 15000, 25000); >> >> No. None of the conversions in this function are correct. >> mipi_dsi_usleep_range() is only for use when you're in the middle of a >> bunch of other "multi" calls and want the sleep to be conditional upon >> there being no error. Here there is no chance of an error because no >> _multi() are used. Go back to the normal usleep_range(). >> > > OK. Then the approach to prefer mipi_dsi_usleep_range() over the previously used usleep_range() everywhere is out the window. Sounds good. Is replacing msleep() with mipi_dsi_msleep() preferable? Same rules there. If you're in the middle of a sequence of "multi" commands and only want the sleep if there is no error then use mipi_dsi_msleep(). If you're not then use a regular msleep(). -Doug
Hi, On Wed, Mar 12, 2025 at 8:06 AM Anusha Srivatsa <asrivats@redhat.com> wrote: > >> > @@ -106,53 +107,46 @@ 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); >> > + mipi_dsi_dcs_soft_reset_multi(&dsi_ctx); >> > + if (dsi_ctx.accum_err) >> > goto fail; >> > - } >> >> This changes was definitely not what the script is doing. > > > It isnt. Using coccinelle for the major part of pattern matching and replacing the newer _multi variant API. Some handling (including a newline that it introduces) and the returns depend on a case by case basis, which had to be done manually. ...and now you're getting to see why I didn't think a coccinelle script could fully handle this task. ;-) IMO instead of trying to get a coccinelle script to do the full conversion, the right approach would be to use a coccinelle script (or equivalent) to get the basics done (just so you don't make any typos) and then cleanup the result manually. Spending more time on the coccinelle script than it would take to do the conversion manually is probably not the right approach. If your patch wasn't fully generated by a coccinelle script you should document that in the commit message. Something like "Initial patch was generated by a coccinelle script and the result was cleaned up manually." If the script is too long to fit in the commit message, it's fine to put it somewhere online and provide a link. "Somewhere online" could easily be a mailing list post. -Doug
diff --git a/drivers/gpu/drm/panel/panel-synaptics-r63353.c b/drivers/gpu/drm/panel/panel-synaptics-r63353.c index 17349825543fe6a117bbfd9cb92a564ce433d13a..991723e5545725ac1de8e1f1968e3eea8254e2b2 100644 --- a/drivers/gpu/drm/panel/panel-synaptics-r63353.c +++ b/drivers/gpu/drm/panel/panel-synaptics-r63353.c @@ -70,6 +70,7 @@ static int r63353_panel_power_on(struct r63353_panel *rpanel) { struct mipi_dsi_device *dsi = rpanel->dsi; struct device *dev = &dsi->dev; + struct mipi_dsi_multi_context dsi_ctx = { .dsi = dsi }; int ret; ret = regulator_enable(rpanel->avdd); @@ -78,7 +79,7 @@ static int r63353_panel_power_on(struct r63353_panel *rpanel) return ret; } - usleep_range(15000, 25000); + mipi_dsi_usleep_range(&dsi_ctx, 15000, 25000); ret = regulator_enable(rpanel->dvdd); if (ret) { @@ -87,9 +88,9 @@ static int r63353_panel_power_on(struct r63353_panel *rpanel) return ret; } - usleep_range(300000, 350000); + mipi_dsi_usleep_range(&dsi_ctx, 300000, 350000); gpiod_set_value(rpanel->reset_gpio, 1); - usleep_range(15000, 25000); + mipi_dsi_usleep_range(&dsi_ctx, 15000, 25000); return 0; } @@ -106,53 +107,46 @@ 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); + mipi_dsi_dcs_soft_reset_multi(&dsi_ctx); + if (dsi_ctx.accum_err) goto fail; - } - 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); + mipi_dsi_dcs_enter_sleep_mode_multi(&dsi_ctx); + if (dsi_ctx.accum_err) goto fail; - } 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) + mipi_dsi_dcs_write_buffer_multi(&dsi_ctx, instr->data, + instr->len); + if (dsi_ctx.accum_err) goto fail; } - 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); + mipi_dsi_dcs_exit_sleep_mode_multi(&dsi_ctx); + if (dsi_ctx.accum_err) goto fail; - } - 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); + mipi_dsi_dcs_set_display_on_multi(&dsi_ctx); + if (dsi_ctx.accum_err) goto fail; - } - return 0; + return dsi_ctx.accum_err; fail: 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 +175,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)
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) { -... -} ...+> v2: Do not skip the reset in case of error during panel activate (Dmitry) - Convert all usleep_range() 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 --- 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 | 69 ++++++++++---------------- 1 file changed, 27 insertions(+), 42 deletions(-) --- base-commit: ced7486468ac3b38d59a69fca5d97998499c936b change-id: 20250305-mipi-synaptic-1-a2043543cd3a Best regards,