| Message ID | 20180109203248.139249-2-briannorris@chromium.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 09.01.2018 21:32, Brian Norris wrote: > We're filling the "remainder" word with little-endian data, then writing > it out to IO registers with endian-correcting writel(). That probably > won't work on big-endian systems. > > Let's mark the "remainder" variable as LE32 (since we fill it with > memcpy()) and do the swapping explicitly. > > Some of this function could be done more easily without memcpy(), but > the unaligned "remainder" case is a little hard to do without > potentially overrunning 'tx_buf', so I just applied the same solution in > all cases (memcpy() + le32_to_cpu()). > > Tested only on a little-endian system. > > Signed-off-by: Brian Norris <briannorris@chromium.org> > --- > drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c | 18 +++++++++--------- > 1 file changed, 9 insertions(+), 9 deletions(-) > > diff --git a/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c b/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c > index ed91e32ee43a..90f13df6f106 100644 > --- a/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c > +++ b/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c > @@ -360,18 +360,18 @@ static int dw_mipi_dsi_write(struct dw_mipi_dsi *dsi, > { > const u8 *tx_buf = packet->payload; > int len = packet->payload_length, pld_data_bytes = sizeof(u32), ret; > - u32 remainder; > + __le32 word; > u32 val; > > while (len) { > if (len < pld_data_bytes) { > - remainder = 0; > - memcpy(&remainder, tx_buf, len); > - dsi_write(dsi, DSI_GEN_PLD_DATA, remainder); > + word = 0; > + memcpy(&word, tx_buf, len); > + dsi_write(dsi, DSI_GEN_PLD_DATA, le32_to_cpu(word)); > len = 0; > } else { > - memcpy(&remainder, tx_buf, pld_data_bytes); > - dsi_write(dsi, DSI_GEN_PLD_DATA, remainder); > + memcpy(&word, tx_buf, pld_data_bytes); > + dsi_write(dsi, DSI_GEN_PLD_DATA, le32_to_cpu(word)); > tx_buf += pld_data_bytes; > len -= pld_data_bytes; > } > @@ -386,9 +386,9 @@ static int dw_mipi_dsi_write(struct dw_mipi_dsi *dsi, > } > } > > - remainder = 0; > - memcpy(&remainder, packet->header, sizeof(packet->header)); > - return dw_mipi_dsi_gen_pkt_hdr_write(dsi, remainder); > + word = 0; > + memcpy(&word, packet->header, sizeof(packet->header)); > + return dw_mipi_dsi_gen_pkt_hdr_write(dsi, le32_to_cpu(word)); You could create and use appropriate helper, lets say: u32 le_to_cpup(const u8 *p, int count) { __le32 r = 0; memcpy(&r, p, count); return le32_to_cpu(r); } With or without this change: Reviewed-by: Andrzej Hajda <a.hajda@samsung.com> -- Regards Andrzej > } > > static ssize_t dw_mipi_dsi_host_transfer(struct mipi_dsi_host *host,
On 01/10/2018 08:03 PM, Andrzej Hajda wrote: > On 09.01.2018 21:32, Brian Norris wrote: >> We're filling the "remainder" word with little-endian data, then writing >> it out to IO registers with endian-correcting writel(). That probably >> won't work on big-endian systems. >> >> Let's mark the "remainder" variable as LE32 (since we fill it with >> memcpy()) and do the swapping explicitly. >> >> Some of this function could be done more easily without memcpy(), but >> the unaligned "remainder" case is a little hard to do without >> potentially overrunning 'tx_buf', so I just applied the same solution in >> all cases (memcpy() + le32_to_cpu()). >> >> Tested only on a little-endian system. >> >> Signed-off-by: Brian Norris <briannorris@chromium.org> >> --- >> drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c | 18 +++++++++--------- >> 1 file changed, 9 insertions(+), 9 deletions(-) >> >> diff --git a/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c b/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c >> index ed91e32ee43a..90f13df6f106 100644 >> --- a/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c >> +++ b/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c >> @@ -360,18 +360,18 @@ static int dw_mipi_dsi_write(struct dw_mipi_dsi *dsi, >> { >> const u8 *tx_buf = packet->payload; >> int len = packet->payload_length, pld_data_bytes = sizeof(u32), ret; >> - u32 remainder; >> + __le32 word; >> u32 val; >> >> while (len) { >> if (len < pld_data_bytes) { >> - remainder = 0; >> - memcpy(&remainder, tx_buf, len); >> - dsi_write(dsi, DSI_GEN_PLD_DATA, remainder); >> + word = 0; >> + memcpy(&word, tx_buf, len); >> + dsi_write(dsi, DSI_GEN_PLD_DATA, le32_to_cpu(word)); >> len = 0; >> } else { >> - memcpy(&remainder, tx_buf, pld_data_bytes); >> - dsi_write(dsi, DSI_GEN_PLD_DATA, remainder); >> + memcpy(&word, tx_buf, pld_data_bytes); >> + dsi_write(dsi, DSI_GEN_PLD_DATA, le32_to_cpu(word)); >> tx_buf += pld_data_bytes; >> len -= pld_data_bytes; >> } >> @@ -386,9 +386,9 @@ static int dw_mipi_dsi_write(struct dw_mipi_dsi *dsi, >> } >> } >> >> - remainder = 0; >> - memcpy(&remainder, packet->header, sizeof(packet->header)); >> - return dw_mipi_dsi_gen_pkt_hdr_write(dsi, remainder); >> + word = 0; >> + memcpy(&word, packet->header, sizeof(packet->header)); >> + return dw_mipi_dsi_gen_pkt_hdr_write(dsi, le32_to_cpu(word)); > > You could create and use appropriate helper, lets say: > > u32 le_to_cpup(const u8 *p, int count) > { > __le32 r = 0; > > memcpy(&r, p, count); > return le32_to_cpu(r); > } > > With or without this change: > Reviewed-by: Andrzej Hajda <a.hajda@samsung.com> Queued to drm-misc-next as is. Thanks, Archit > > -- > Regards > Andrzej > > >> } >> >> static ssize_t dw_mipi_dsi_host_transfer(struct mipi_dsi_host *host, > >
On Tue, Jan 16, 2018 at 12:22:52PM +0530, Archit Taneja wrote: > On 01/10/2018 08:03 PM, Andrzej Hajda wrote: > >On 09.01.2018 21:32, Brian Norris wrote: > >>@@ -386,9 +386,9 @@ static int dw_mipi_dsi_write(struct dw_mipi_dsi *dsi, > >> } > >> } > >>- remainder = 0; > >>- memcpy(&remainder, packet->header, sizeof(packet->header)); > >>- return dw_mipi_dsi_gen_pkt_hdr_write(dsi, remainder); > >>+ word = 0; > >>+ memcpy(&word, packet->header, sizeof(packet->header)); > >>+ return dw_mipi_dsi_gen_pkt_hdr_write(dsi, le32_to_cpu(word)); > > > >You could create and use appropriate helper, lets say: > > > >u32 le_to_cpup(const u8 *p, int count) > >{ > > __le32 r = 0; > > > > memcpy(&r, p, count); > > return le32_to_cpu(r); > >} I suppose that could be a small improvement, for future consideration, if this gets too out of hand. > >With or without this change: > >Reviewed-by: Andrzej Hajda <a.hajda@samsung.com> Thanks! > Queued to drm-misc-next as is. Thanks! Brian
diff --git a/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c b/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c index ed91e32ee43a..90f13df6f106 100644 --- a/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c +++ b/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c @@ -360,18 +360,18 @@ static int dw_mipi_dsi_write(struct dw_mipi_dsi *dsi, { const u8 *tx_buf = packet->payload; int len = packet->payload_length, pld_data_bytes = sizeof(u32), ret; - u32 remainder; + __le32 word; u32 val; while (len) { if (len < pld_data_bytes) { - remainder = 0; - memcpy(&remainder, tx_buf, len); - dsi_write(dsi, DSI_GEN_PLD_DATA, remainder); + word = 0; + memcpy(&word, tx_buf, len); + dsi_write(dsi, DSI_GEN_PLD_DATA, le32_to_cpu(word)); len = 0; } else { - memcpy(&remainder, tx_buf, pld_data_bytes); - dsi_write(dsi, DSI_GEN_PLD_DATA, remainder); + memcpy(&word, tx_buf, pld_data_bytes); + dsi_write(dsi, DSI_GEN_PLD_DATA, le32_to_cpu(word)); tx_buf += pld_data_bytes; len -= pld_data_bytes; } @@ -386,9 +386,9 @@ static int dw_mipi_dsi_write(struct dw_mipi_dsi *dsi, } } - remainder = 0; - memcpy(&remainder, packet->header, sizeof(packet->header)); - return dw_mipi_dsi_gen_pkt_hdr_write(dsi, remainder); + word = 0; + memcpy(&word, packet->header, sizeof(packet->header)); + return dw_mipi_dsi_gen_pkt_hdr_write(dsi, le32_to_cpu(word)); } static ssize_t dw_mipi_dsi_host_transfer(struct mipi_dsi_host *host,
We're filling the "remainder" word with little-endian data, then writing it out to IO registers with endian-correcting writel(). That probably won't work on big-endian systems. Let's mark the "remainder" variable as LE32 (since we fill it with memcpy()) and do the swapping explicitly. Some of this function could be done more easily without memcpy(), but the unaligned "remainder" case is a little hard to do without potentially overrunning 'tx_buf', so I just applied the same solution in all cases (memcpy() + le32_to_cpu()). Tested only on a little-endian system. Signed-off-by: Brian Norris <briannorris@chromium.org> --- drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-)