linux-kernel - Re: [PATCH] drm/panel: sitronix-st7703: transition to mipi

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date: Wed, 26 Jun 2024 14:52:52 +0200
From: Ondřej Jirman <megi@....cz>
To: Guido Günther <agx@...xcpu.org>
Cc: Tejas Vipin <tejasvipin76@...il.com>, kernel@...i.sm, 
	neil.armstrong@...aro.org, quic_jesszhan@...cinc.com, dianders@...omium.org, 
	maarten.lankhorst@...ux.intel.com, mripard@...nel.org, tzimmermann@...e.de, airlied@...il.com, 
	daniel@...ll.ch, dri-devel@...ts.freedesktop.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] drm/panel: sitronix-st7703: transition to mipi_dsi
 wrapped functions

Hi Guido,

On Wed, Jun 26, 2024 at 02:25:16PM GMT, Guido Günther wrote:
> [...]
> > -	ret = ctx->desc->init_sequence(ctx);
> > -	if (ret < 0) {
> > -		dev_err(ctx->dev, "Panel init sequence failed: %d\n", ret);
> > -		return ret;
> > -	}
> > +	ctx->desc->init_sequence(&dsi_ctx);
> 
> Why no return early here in the error case (same for the other cases
> below) giving us an indication which step went wrong?

Return early is hidden in the wrapped *_multi() function calls.

> >  
> > -	ret = mipi_dsi_dcs_exit_sleep_mode(dsi);
> > -	if (ret < 0) {
> > -		dev_err(ctx->dev, "Failed to exit sleep mode: %d\n", ret);
> > -		return ret;
> > -	}
> > +	mipi_dsi_dcs_exit_sleep_mode_multi(&dsi_ctx);
> >  
> >  	/* It takes the controller 120 msec to wake up after sleep. */
> > -	msleep(120);
> > +	mipi_dsi_msleep(&dsi_ctx, 120);
> >  
> > -	ret = mipi_dsi_dcs_set_display_on(dsi);
> > -	if (ret)
> > -		return ret;
> > +	mipi_dsi_dcs_set_display_on_multi(&dsi_ctx);
> >  
> > -	dev_dbg(ctx->dev, "Panel init sequence done\n");
> 
> Would be nice to keep the debug message.
> 
> > +	if (!dsi_ctx.accum_err)
> > +		dev_dbg(ctx->dev, "Panel init sequence done\n");
> >  
> > -	return 0;
> > +	return dsi_ctx.accum_err;
> >  }
> >  
> >  static int st7703_disable(struct drm_panel *panel)
> >  {
> >  	struct st7703 *ctx = panel_to_st7703(panel);
> >  	struct mipi_dsi_device *dsi = to_mipi_dsi_device(ctx->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(ctx->dev, "Failed to turn off the display: %d\n", ret);
> > +	mipi_dsi_dcs_set_display_off_multi(&dsi_ctx);
> 
> Similar to the above: it'd be nice to keep the information which step
> failed.

That's done by the _multi() wrappers.

kind regards,
	o.