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

[Laurent Pinchart]

Hi Sebastian,

On Wed, Feb 26, 2020 at 11:46:43PM +0100, Sebastian Reichel wrote:
> 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 at ideasonboard.com>
> 
> Are you fine with keeping the mipi_dsi_packet, since it will be
> removed in a further patch?

Yes, please ignore the comments related to the packet structure, I've
realized when reviewing a patch further in this series that this goes
away (I've mentioned that explicitly in the review of that 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.

-- 
Regards,

Laurent Pinchart