Hi Philippe, On Tue, Jan 09, 2018 at 10:48:43AM +0000, Philippe CORNU wrote: > Hi Brian, > > And many thanks for implementing these TODOs. And thanks for adding them; it gave me a better option than just adding yet another switch case (MIPI_DSI_GENERIC_LONG_WRITE) ;) > On 01/06/2018 01:38 AM, 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 <briannorris@xxxxxxxxxxxx> > > --- > > Could use an extra look from folks. This looks like the correct trivial > > transformation, but I'm not that familiar with DSI. > > > > drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c | 78 ++++++--------------------- > > 1 file changed, 16 insertions(+), 62 deletions(-) > > > > diff --git a/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c b/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c > > index d9cca4fd66ec..2fed20e44dfe 100644 > > --- a/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c > > +++ b/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c > > @@ -136,10 +136,6 @@ > > GEN_SW_0P_TX_LP) > > > > #define DSI_GEN_HDR 0x6c > > -/* TODO These 2 defines will be reworked thanks to mipi_dsi_create_packet() */ > > -#define GEN_HDATA(data) (((data) & 0xffff) << 8) > > -#define GEN_HTYPE(type) (((type) & 0xff) << 0) > > - > > #define DSI_GEN_PLD_DATA 0x70 > > > > #define DSI_CMD_PKT_STATUS 0x74 > > @@ -359,44 +355,15 @@ static int dw_mipi_dsi_gen_pkt_hdr_write(struct dw_mipi_dsi *dsi, u32 hdr_val) > > return 0; > > } > > > > -static int dw_mipi_dsi_dcs_short_write(struct dw_mipi_dsi *dsi, > > - const struct mipi_dsi_msg *msg) > > -{ > > - const u8 *tx_buf = msg->tx_buf; > > - u16 data = 0; > > - u32 val; > > - > > - if (msg->tx_len > 0) > > - data |= tx_buf[0]; > > - if (msg->tx_len > 1) > > - data |= tx_buf[1] << 8; > > - > > - if (msg->tx_len > 2) { > > - dev_err(dsi->dev, "too long tx buf length %zu for short write\n", > > - msg->tx_len); > > - return -EINVAL; > > - } > > - > > - val = GEN_HDATA(data) | GEN_HTYPE(msg->type); > > - return dw_mipi_dsi_gen_pkt_hdr_write(dsi, val); > > -} > > - > > -static int dw_mipi_dsi_dcs_long_write(struct dw_mipi_dsi *dsi, > > - const struct mipi_dsi_msg *msg) > > +static int dw_mipi_dsi_dcs_write(struct dw_mipi_dsi *dsi, > > + const struct mipi_dsi_packet *packet) > > Both DCS and Generic dsi transfers are managed by drm_mipi_dsi.c > helpers. So maybe dw_mipi_dsi_dcs_write() should be renamed > dw_mipi_dsi_write()... Ah, good point. I really meant to remove the _dcs naming too, but I guess I missed it. Will follow up. > > { > > - const u8 *tx_buf = msg->tx_buf; > > - int len = msg->tx_len, pld_data_bytes = sizeof(u32), ret; > > - u32 hdr_val = GEN_HDATA(msg->tx_len) | GEN_HTYPE(msg->type); > > + const u8 *tx_buf = packet->payload; > > + int len = packet->payload_length, pld_data_bytes = sizeof(u32), ret; > > u32 remainder; > > u32 val; > > > > - if (msg->tx_len < 3) { > > - dev_err(dsi->dev, "wrong tx buf length %zu for long write\n", > > - msg->tx_len); > > - return -EINVAL; > > - } > > - > > - while (DIV_ROUND_UP(len, pld_data_bytes)) { > > + while (len) { > > if (len < pld_data_bytes) { > > remainder = 0; > > memcpy(&remainder, tx_buf, len); > > @@ -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)); By the way: I don't think it's an issue that should block this patch, since if I'm right, this function already is "broken", but isn't this actually a bad way to handle byte-to-word marshalling? Particularly, we're copying bytes into a word in LE ordering, but then we later write them to IO registers with writel() (which does endian swapping). So I think we have an endianness problem on BE systems. One solution would be to write these to IO registers with a non-swapped writel() (e.g., __raw_writel()? but that's not very nice...). Another would be to avoid memcpy, and just read this out a word at a time -- that works fine for the aligned pieces, but not so well for any non-aligned bits ('if (len < pld_data_bytes)' above) I think? WDYT? > > + 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; > > + 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_dcs_write(dsi, &packet); > > } > > > > static const struct mipi_dsi_host_ops dw_mipi_dsi_host_ops = { > > > > I performed some tests tracing all DSI_GEN_HDR & DSI_GEN_PLD_DATA reg > writes with panel/panel-orisetech-otm8009a.c (using long dcs commands) > before and after your patch and this is "100% perfect"! > > So, apart the un-important "dcs" in dw_mipi_dsi_dcs_write() function name: > > Reviewed-by: Philippe Cornu <philippe.cornu@xxxxxx> > Tested-by: Philippe Cornu <philippe.cornu@xxxxxx> > > This clean-up will help a lot to add the dsi read feature in the future. > > Very good patch Brian and big "thank you" ! Thanks for the review and test! I'll likely send a v2 with only the naming change + your tags, and I'll see about what do about the endianness issues I noticed as a follow-up. Brian _______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/dri-devel