On Mon, Aug 10, 2026 at 11:57:06AM +0200, Neil Armstrong wrote:
> > +   struct mipi_dsi_multi_context dsi_ctx = { .dsi = 
> > to_mipi_dsi_device(s6->dev) };
> 
> Ok why not keeping the original:
> 
>       struct mipi_dsi_device *dsi = to_mipi_dsi_device(s6->dev);
>       struct mipi_dsi_multi_context dsi_ctx = { .dsi = dsi };
> 
> 
> It's much easier to read, for me at least, but I won't block it...
> 
> >     /* Enter sleep mode */
> > -   ret = mipi_dsi_dcs_enter_sleep_mode(dsi);
> > -   if (ret) {
> > -           dev_err(s6->dev, "failed to enter sleep mode (%d)\n", ret);
> > -           return ret;
> > -   }
> > +   mipi_dsi_dcs_enter_sleep_mode_multi(&dsi_ctx);
> 
> Add a comment you ignore the the dsi_ctx.accum_err and it's fine
 
> You could write:
> 
>       if (dsi_ctx.accum_err) {
>               gpiod_set_value_cansleep(s6->reset_gpio, 1);
>               regulator_disable(s6->supply);
>       }
> 
>       return dsi_ctx.accum_err;

Thanks for the review, Neil. Will address all three in v2.

On the intermediate dsi variable, I prefer inlining it to keep one fewer
variable in scope, but I see the readability argument. Will revert it.
-- 
Thanks,
Akash

Reply via email to