Re: [PATCH] drm/panel: samsung-s6d16d0: Use mipi_dsi_*_multi(); fix minor bugs

Akash Sukhavasi <[email protected]>
Newsgroups org.freedesktop.lists.dri-devel,org.kernel.vger.linux-kernel
Message-ID <[email protected]>
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
lmpx.com only provides a reader for public news (NNTP) servers. It is not affiliated with the servers or forums shown here and is not responsible for the content of articles, which is written by their respective authors.