Hi Sebastian,
Thank you for the patch.
On Tue, Feb 25, 2020 at 12:20:38AM +0100, Sebastian Reichel wrote:
> This converts the panel-dsi-cm driver to use the transfer
> API instead of specific functions, so that the specific
> functions can be unexported and squashed into the generic
> transfer function.
>
> Signed-off-by: Sebastian Reichel <sebastian.reichel@xxxxxxxxxxxxx>
> ---
> .../gpu/drm/omapdrm/displays/panel-dsi-cm.c | 133 +++++++++++++-----
> 1 file changed, 96 insertions(+), 37 deletions(-)
>
> diff --git a/drivers/gpu/drm/omapdrm/displays/panel-dsi-cm.c b/drivers/gpu/drm/omapdrm/displays/panel-dsi-cm.c
> index e6ebfc35243e..92f510a771fe 100644
> --- a/drivers/gpu/drm/omapdrm/displays/panel-dsi-cm.c
> +++ b/drivers/gpu/drm/omapdrm/displays/panel-dsi-cm.c
> @@ -140,45 +140,61 @@ static void hw_guard_wait(struct panel_drv_data *ddata)
> static int dsicm_dcs_read_1(struct panel_drv_data *ddata, u8 dcs_cmd, u8 *data)
> {
> struct omap_dss_device *src = ddata->src;
> - int r;
> - u8 buf[1];
> -
> - r = src->ops->dsi.dcs_read(src, ddata->channel, dcs_cmd, buf, 1);
> -
> - if (r < 0)
> - return r;
> -
> - *data = buf[0];
> + const struct mipi_dsi_msg msg = {
> + .channel = ddata->channel,
> + .type = MIPI_DSI_DCS_READ,
> + .tx_len = 1,
> + .tx_buf = &dcs_cmd,
> + .rx_len = 1,
> + .rx_buf = data
> + };
>
> - return 0;
> + return src->ops->dsi.transfer(src, &msg);
> }
>
> static int dsicm_dcs_write_0(struct panel_drv_data *ddata, u8 dcs_cmd)
> {
> struct omap_dss_device *src = ddata->src;
> + const struct mipi_dsi_msg msg = {
> + .channel = ddata->channel,
> + .type = MIPI_DSI_DCS_SHORT_WRITE,
> + .tx_buf = &dcs_cmd,
> + .tx_len = 1,
> + };
>
> - return src->ops->dsi.dcs_write(src, ddata->channel, &dcs_cmd, 1);
> + return src->ops->dsi.transfer(src, &msg);
> }
>
> static int dsicm_dcs_write_1(struct panel_drv_data *ddata, u8 dcs_cmd, u8 param)
> {
> struct omap_dss_device *src = ddata->src;
> - u8 buf[2] = { dcs_cmd, param };
> + const u8 buf[] = { dcs_cmd, param };
> + const struct mipi_dsi_msg msg = {
> + .channel = ddata->channel,
> + .type = MIPI_DSI_DCS_SHORT_WRITE_PARAM,
> + .tx_buf = &buf,
> + .tx_len = 2,
> + };
>
> - return src->ops->dsi.dcs_write(src, ddata->channel, buf, 2);
> + return src->ops->dsi.transfer(src, &msg);
> }
>
> static int dsicm_sleep_in(struct panel_drv_data *ddata)
>
> {
> struct omap_dss_device *src = ddata->src;
> - u8 cmd;
> int r;
> + const u8 cmd = MIPI_DCS_ENTER_SLEEP_MODE;
> + const struct mipi_dsi_msg msg = {
> + .channel = ddata->channel,
> + .type = MIPI_DSI_DCS_SHORT_WRITE,
> + .tx_buf = &cmd,
> + .tx_len = 1,
> + };
>
> hw_guard_wait(ddata);
>
> - cmd = MIPI_DCS_ENTER_SLEEP_MODE;
> - r = src->ops->dsi.dcs_write_nosync(src, ddata->channel, &cmd, 1);
> + r = src->ops->dsi.transfer(src, &msg);
Should you call dsicm_dcs_write_0(ddata, MIPI_DCS_ENTER_SLEEP_MODE)
instead ? This uses the _nosync variant though, is it an issue ?
> if (r)
> return r;
>
> @@ -233,28 +249,44 @@ static int dsicm_set_update_window(struct panel_drv_data *ddata,
> u16 y1 = y;
> u16 y2 = y + h - 1;
>
> - u8 buf[5];
> - buf[0] = MIPI_DCS_SET_COLUMN_ADDRESS;
> - buf[1] = (x1 >> 8) & 0xff;
> - buf[2] = (x1 >> 0) & 0xff;
> - buf[3] = (x2 >> 8) & 0xff;
> - buf[4] = (x2 >> 0) & 0xff;
> + const u8 paramX[] = {
> + MIPI_DCS_SET_COLUMN_ADDRESS,
> + (x1 >> 8) & 0xff,
> + (x1 >> 0) & 0xff,
> + (x2 >> 8) & 0xff,
> + (x2 >> 0) & 0xff,
> + };
>
> - r = src->ops->dsi.dcs_write_nosync(src, ddata->channel, buf, sizeof(buf));
> - if (r)
> - return r;
> + const struct mipi_dsi_msg msgX = {
> + .channel = ddata->channel,
> + .type = MIPI_DSI_GENERIC_LONG_WRITE,
> + .tx_buf = paramX,
> + .tx_len = 5,
> + };
>
> - buf[0] = MIPI_DCS_SET_PAGE_ADDRESS;
> - buf[1] = (y1 >> 8) & 0xff;
> - buf[2] = (y1 >> 0) & 0xff;
> - buf[3] = (y2 >> 8) & 0xff;
> - buf[4] = (y2 >> 0) & 0xff;
> + const u8 paramY[] = {
> + MIPI_DCS_SET_PAGE_ADDRESS,
> + (y1 >> 8) & 0xff,
> + (y1 >> 0) & 0xff,
> + (y2 >> 8) & 0xff,
> + (y2 >> 0) & 0xff,
> + };
>
> - r = src->ops->dsi.dcs_write_nosync(src, ddata->channel, buf, sizeof(buf));
Also replacing a _nosync variant here.
> + const struct mipi_dsi_msg msgY = {
> + .channel = ddata->channel,
> + .type = MIPI_DSI_GENERIC_LONG_WRITE,
> + .tx_buf = paramY,
> + .tx_len = 5,
> + };
> +
> +
A single blank line is enough.
> + r = src->ops->dsi.transfer(src, &msgX);
> if (r)
> return r;
>
> - src->ops->dsi.bta_sync(src, ddata->channel);
> + r = src->ops->dsi.transfer(src, &msgY);
And here, you're replacing bta_sync. If I understand the code correctly,
you're essentially removing an optimization, as each write will sync,
right ? I'm fine with this change if we add the functionality back later
in this series.
> + if (r)
> + return r;
>
> return r;
> }
> @@ -991,6 +1023,27 @@ static int dsicm_get_te(struct omap_dss_device *dssdev)
> return r;
> }
>
> +static int dsicm_set_max_rx_packet_size(struct omap_dss_device *dssdev,
> + u16 size)
Please use tabs instead of spaces for indentation.
> +{
> + struct panel_drv_data *ddata = to_panel_data(dssdev);
> + struct omap_dss_device *src = ddata->src;
> +
> + const u8 buf[] = {
> + size & 0xff,
> + size >> 8 & 0xff,
> + };
> +
> + const struct mipi_dsi_msg msg = {
> + .channel = ddata->channel,
> + .type = MIPI_DSI_SET_MAXIMUM_RETURN_PACKET_SIZE,
> + .tx_buf = buf,
> + .tx_len = 2,
> + };
> +
> + return src->ops->dsi.transfer(src, &msg);
> +}
> +
> static int dsicm_memory_read(struct omap_dss_device *dssdev,
> void *buf, size_t size,
> u16 x, u16 y, u16 w, u16 h)
> @@ -1031,17 +1084,23 @@ static int dsicm_memory_read(struct omap_dss_device *dssdev,
>
> dsicm_set_update_window(ddata, x, y, w, h);
>
> - r = src->ops->dsi.set_max_rx_packet_size(src, ddata->channel, plen);
> + r = dsicm_set_max_rx_packet_size(dssdev, plen);
> if (r)
> goto err2;
>
> while (buf_used < size) {
> u8 dcs_cmd = first ? 0x2e : 0x3e;
> + const struct mipi_dsi_msg msg = {
> + .channel = ddata->channel,
> + .type = MIPI_DSI_DCS_READ,
> + .tx_buf = &dcs_cmd,
> + .tx_len = 1,
> + .rx_buf = buf + buf_used,
> + .rx_len = size - buf_used,
> + };
> first = 0;
>
> - r = src->ops->dsi.dcs_read(src, ddata->channel, dcs_cmd,
> - buf + buf_used, size - buf_used);
> -
> + r = src->ops->dsi.transfer(src, &msg);
> if (r < 0) {
> dev_err(dssdev->dev, "read error\n");
> goto err3;
> @@ -1065,7 +1124,7 @@ static int dsicm_memory_read(struct omap_dss_device *dssdev,
> r = buf_used;
>
> err3:
> - src->ops->dsi.set_max_rx_packet_size(src, ddata->channel, 1);
> + dsicm_set_max_rx_packet_size(dssdev, 1);
> err2:
> src->ops->dsi.bus_unlock(src);
> err1:
--
Regards,
Laurent Pinchart
