Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751920AbeAPTFC (ORCPT + 1 other); Tue, 16 Jan 2018 14:05:02 -0500 Received: from mail-pf0-f196.google.com ([209.85.192.196]:39795 "EHLO mail-pf0-f196.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751822AbeAPTE6 (ORCPT ); Tue, 16 Jan 2018 14:04:58 -0500 X-Google-Smtp-Source: ACJfBoskleUB1rAbfR81hRwV9myy5fuE2eBuhk1ujx2/NdTIUW83Ai9ZOt9nb8iAYRwl3FjkGYPM/g== Date: Tue, 16 Jan 2018 11:04:55 -0800 From: Brian Norris To: Archit Taneja , Andrzej Hajda , Laurent Pinchart Cc: David Airlie , Yannick Fertre , Philippe Cornu , Vincent Abriou , dri-devel@lists.freedesktop.org, linux-kernel@vger.kernel.org, Sean Paul , Nickey Yang , hl@rock-chips.com, linux-rockchip@lists.infradead.org, mka@chromium.org, hoegsberg@gmail.com, zyw@rock-chips.com, xbl@rock-chips.com Subject: Re: [PATCH v2 1/2] drm/bridge/synopsys: dsi: use common mipi_dsi_create_packet() Message-ID: <20180116190454.GB149565@google.com> References: <20180109203248.139249-1-briannorris@chromium.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20180109203248.139249-1-briannorris@chromium.org> User-Agent: Mutt/1.5.21 (2010-09-15) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Return-Path: Hi all, I believe Philippe's comments about return values have been addressed separately, and this patch was applied to drm-misc-next. But I have one additional thought below. On Tue, Jan 09, 2018 at 12:32:47PM -0800, Brian Norris wrote: > This takes care of 2 TODOs in this driver, by using the common DSI > packet-marshalling code instead of our custom short/long write code. > This both saves us some duplicated code and gets us free support for > command types that weren't already part of our switch block (e.g., > MIPI_DSI_GENERIC_LONG_WRITE). > > The code logic stays mostly intact, except that it becomes unnecessary > to split the short/long write functions, and we have to copy data a bit > more. > > Along the way, I noticed that loop bounds were a little odd: > > while (DIV_ROUND_UP(len, pld_data_bytes)) > > This really was just supposed to be 'len != 0', so I made that more > clear. > > Tested on RK3399 with some pending refactoring patches by Nickey Yang, > to make the Rockchip DSI driver wrap this common driver. > > Signed-off-by: Brian Norris > Reviewed-by: Philippe Cornu > Tested-by: Philippe Cornu ... > diff --git a/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c b/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c > index d9cca4fd66ec..ed91e32ee43a 100644 > --- a/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c > +++ b/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c ... > @@ -419,40 +386,27 @@ static int dw_mipi_dsi_dcs_long_write(struct dw_mipi_dsi *dsi, > } > } > > - return dw_mipi_dsi_gen_pkt_hdr_write(dsi, hdr_val); > + remainder = 0; > + memcpy(&remainder, packet->header, sizeof(packet->header)); > + return dw_mipi_dsi_gen_pkt_hdr_write(dsi, remainder); > } > > static ssize_t dw_mipi_dsi_host_transfer(struct mipi_dsi_host *host, > const struct mipi_dsi_msg *msg) > { > struct dw_mipi_dsi *dsi = host_to_dsi(host); > + struct mipi_dsi_packet packet; > int ret; > > - /* > - * TODO dw drv improvements > - * use mipi_dsi_create_packet() instead of all following > - * functions and code (no switch cases, no > - * dw_mipi_dsi_dcs_short_write(), only the loop in long_write...) > - * and use packet.header... > - */ > - dw_mipi_message_config(dsi, msg); > - > - switch (msg->type) { > - case MIPI_DSI_DCS_SHORT_WRITE: > - case MIPI_DSI_DCS_SHORT_WRITE_PARAM: > - case MIPI_DSI_SET_MAXIMUM_RETURN_PACKET_SIZE: > - ret = dw_mipi_dsi_dcs_short_write(dsi, msg); > - break; > - case MIPI_DSI_DCS_LONG_WRITE: > - ret = dw_mipi_dsi_dcs_long_write(dsi, msg); > - break; > - default: > - dev_err(dsi->dev, "unsupported message type 0x%02x\n", > - msg->type); > - ret = -EINVAL; By removing this default case, I'm hiding the fact that this driver doesn't currently implement RX support at all -- only TX. (Previously, if presented with, e.g., a MIPI_DSI_GENERIC_READ_REQUEST_0_PARAM, the driver would reject it. Now it will silently fail to return it.) I believe some Rockchip folks have preliminary RX support implemented somewhere, so we might consider merging that, or else re-introducing a failure case to catch the unimplemented RX support. Brian > + ret = mipi_dsi_create_packet(&packet, msg); > + if (ret) { > + dev_err(dsi->dev, "failed to create packet: %d\n", ret); > + return ret; > } > > - return ret; > + dw_mipi_message_config(dsi, msg); > + > + return dw_mipi_dsi_write(dsi, &packet); > } > > static const struct mipi_dsi_host_ops dw_mipi_dsi_host_ops = {