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@xxxxxxxxxxxxxxxx>
>
> 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
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
