Re: [PATCHv2 11/56] drm/omap: dsi: simplify write function

[Sebastian Reichel <sre@kernel.org>]

[-- Attachment #1: Type: text/plain, Size: 2868 bytes --]

Hi Laurent,

On Tue, Feb 25, 2020 at 05:31:05PM +0200, Laurent Pinchart wrote:
> > +	if (mipi_dsi_packet_format_is_short(msg->type)) {
> > +		u16 data = packet.header[1] | (packet.header[2] << 8);
> > +		r = dsi_vc_send_short(dsi, msg->channel, msg->type, data, 0);
> 
> You use the packet for this case only, I think you could simply write
> 
> 		u16 data = ((msg->tx_len > 0) ? tx[0] : 0)
> 			 | (((msg->tx_len > 1) ? tx[1] : 0) << 8);
> 		r = dsi_vc_send_short(dsi, msg->channel, msg->type, data, 0);

That probably works with 's/tx[/((u8*) msg->tx_buf)[', which looks
really ugly :) This code is further simplified by a further patch,
which forwards the complete message into dsi_vc_send_short().

> >  	} else {
> > -		r = dsi_vc_send_long(dsi, channel,
> > -				type == DSS_DSI_CONTENT_GENERIC ?
> > -				MIPI_DSI_GENERIC_LONG_WRITE :
> > -				MIPI_DSI_DCS_LONG_WRITE, data, len, 0);
> > +		r = dsi_vc_send_long(dsi, msg->channel, msg->type,
> > +						   msg->tx_buf, msg->tx_len, 0);
> 
> Indentation.

Ok.

> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

Are you fine with keeping the mipi_dsi_packet, since it will be
removed in a further patch?

> >  	}
> >  
> > -	return r;
> > -}
> > -
> > -static int dsi_vc_dcs_write_nosync(struct omap_dss_device *dssdev, int channel,
> > -		const u8 *data, int len)
> > -{
> > -	struct dsi_data *dsi = to_dsi_data(dssdev);
> > -
> > -	return dsi_vc_write_nosync_common(dsi, channel, data, len,
> > -			DSS_DSI_CONTENT_DCS);
> > -}
> > -
> > -static int dsi_vc_generic_write_nosync(struct omap_dss_device *dssdev, int channel,
> > -		const u8 *data, int len)
> > -{
> > -	struct dsi_data *dsi = to_dsi_data(dssdev);
> > -
> > -	return dsi_vc_write_nosync_common(dsi, channel, data, len,
> > -			DSS_DSI_CONTENT_GENERIC);
> > -}
> > -
> > -static int dsi_vc_write_common(struct omap_dss_device *dssdev,
> > -			       int channel, const u8 *data, int len,
> > -			       enum dss_dsi_content_type type)
> > -{
> > -	struct dsi_data *dsi = to_dsi_data(dssdev);
> > -	int r;
> > +	if (r < 0)
> > +		return r;
> >  
> > -	r = dsi_vc_write_nosync_common(dsi, channel, data, len, type);
> > -	if (r)
> > -		goto err;
> > +	/*
> > +	 * we do not always have to do the BTA sync, for example we can
> > +	 * improve performance by setting the update window information
> > +	 * without sending BTA sync between the commands. In that case
> > +	 * we can return earily.
> 
> s/earily/early/
> 
> Do I understand correctly that this isn't implemented yet ? You should
> make it clear in the comment that it's a candidate for a future
> optimization.

Yes. I forgot the TODO keyword for some reason. Has been quite some
time since I wrote this patch :) I fixed the earily and prefixed the
message with TODO.

-- Sebastian

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]