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 <[email protected]>
Reviewed-by: Philippe Cornu <[email protected]>
Tested-by: Philippe Cornu <[email protected]>
---
v2:
* remove "dcs" naming, since these commands handle generic DSI too, not
just DCS (thanks Philippe)
* add Philippe's {Tested,Reviewed}-by
---
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..ed91e32ee43a 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_write(struct dw_mipi_dsi *dsi,
+ const struct mipi_dsi_packet *packet)
{
- 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));
+ 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_write(dsi, &packet);
}
static const struct mipi_dsi_host_ops dw_mipi_dsi_host_ops = {
--
2.16.0.rc1.238.g530d649a79-goog
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 <[email protected]>
---
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));
}
static ssize_t dw_mipi_dsi_host_transfer(struct mipi_dsi_host *host,
--
2.16.0.rc1.238.g530d649a79-goog
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 <[email protected]>
> ---
> 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 <[email protected]>
--
Regards
Andrzej
> }
>
> static ssize_t dw_mipi_dsi_host_transfer(struct mipi_dsi_host *host,
Hi Brian & All *DSI DRM experts*,
1) Re-reading this patch, I realize that the returned value of
dw_mipi_dsi_host_transfer() is not correct: we should return the number
of transfered/received bytes...
so I think there are two solutions: fix this in this serie or add a TODO
for later (both solutions are fine to me :-)
2) Digging more into the drm code, the function
mipi_dsi_device_transfer() in drm_mipi_dsi.c is called in the same file
by the 3 following functions: mipi_dsi_shutdown_peripheral(),
mipi_dsi_turn_on_peripheral() &
mipi_dsi_set_maximum_return_packet_size(). All these 3 functions are
expecting "Return: 0 on success or a negative error code on failure."
that is not in line with the transfer function.
So then, we can change the documentation in this file and have instead
"* Return: The number of bytes transmitted on success or a negative
error code on failure." as for mipi_dsi_generic_write()...
Or we can change the source code of these 3 functions to match with the
documentation "Return: 0 on success...".
note: Hopefully, "users" of these 3 functions test the sign of the
return value (or do not use it).
Does anyone have a preferred solutions?
Many thanks
Philippe :-)
On 01/09/2018 09:32 PM, 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 <[email protected]>
> Reviewed-by: Philippe Cornu <[email protected]>
> Tested-by: Philippe Cornu <[email protected]>
> ---
> v2:
> * remove "dcs" naming, since these commands handle generic DSI too, not
> just DCS (thanks Philippe)
> * add Philippe's {Tested,Reviewed}-by
> ---
> 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..ed91e32ee43a 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_write(struct dw_mipi_dsi *dsi,
> + const struct mipi_dsi_packet *packet)
> {
> - 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));
> + 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_write(dsi, &packet);
> }
>
> static const struct mipi_dsi_host_ops dw_mipi_dsi_host_ops = {
>
On 11.01.2018 14:51, Philippe CORNU wrote:
> Hi Brian & All *DSI DRM experts*,
>
> 1) Re-reading this patch, I realize that the returned value of
> dw_mipi_dsi_host_transfer() is not correct: we should return the number
> of transfered/received bytes...
>
> so I think there are two solutions: fix this in this serie or add a TODO
> for later (both solutions are fine to me :-)
>
>
> 2) Digging more into the drm code, the function
> mipi_dsi_device_transfer() in drm_mipi_dsi.c is called in the same file
> by the 3 following functions: mipi_dsi_shutdown_peripheral(),
> mipi_dsi_turn_on_peripheral() &
> mipi_dsi_set_maximum_return_packet_size(). All these 3 functions are
> expecting "Return: 0 on success or a negative error code on failure."
> that is not in line with the transfer function.
>
> So then, we can change the documentation in this file and have instead
> "* Return: The number of bytes transmitted on success or a negative
> error code on failure." as for mipi_dsi_generic_write()...
> Or we can change the source code of these 3 functions to match with the
> documentation "Return: 0 on success...".
>
> note: Hopefully, "users" of these 3 functions test the sign of the
> return value (or do not use it).
>
> Does anyone have a preferred solutions?
All three functions performs single operation which can succeed only in
one way, nobody is interested in the number of bytes send to achieve the
result. So IMO the result should be 0 or error.
And mipi_dsi_device_transfer() is a different beast, it returns number
of written/read bytes, which can vary (more specifically, only number of
read bytes can vary :) ).
Regards
Andrzej
>
> Many thanks
> Philippe :-)
>
> On 01/09/2018 09:32 PM, 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 <[email protected]>
>> Reviewed-by: Philippe Cornu <[email protected]>
>> Tested-by: Philippe Cornu <[email protected]>
>> ---
>> v2:
>> * remove "dcs" naming, since these commands handle generic DSI too, not
>> just DCS (thanks Philippe)
>> * add Philippe's {Tested,Reviewed}-by
>> ---
>> 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..ed91e32ee43a 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_write(struct dw_mipi_dsi *dsi,
>> + const struct mipi_dsi_packet *packet)
>> {
>> - 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));
>> + 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_write(dsi, &packet);
>> }
>>
>> static const struct mipi_dsi_host_ops dw_mipi_dsi_host_ops = {
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 <[email protected]>
>> ---
>> 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 <[email protected]>
Queued to drm-misc-next as is.
Thanks,
Archit
>
> --
> Regards
> Andrzej
>
>
>> }
>>
>> static ssize_t dw_mipi_dsi_host_transfer(struct mipi_dsi_host *host,
>
>
--
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
On 01/10/2018 02:02 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.
Queued to drm-misc-next. Philippe's point about the return value of
mipi_dsi_device_transfer is addressed in a different patch.
Thanks,
Archit
>
> Signed-off-by: Brian Norris <[email protected]>
> Reviewed-by: Philippe Cornu <[email protected]>
> Tested-by: Philippe Cornu <[email protected]>
> ---
> v2:
> * remove "dcs" naming, since these commands handle generic DSI too, not
> just DCS (thanks Philippe)
> * add Philippe's {Tested,Reviewed}-by
> ---
> 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..ed91e32ee43a 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_write(struct dw_mipi_dsi *dsi,
> + const struct mipi_dsi_packet *packet)
> {
> - 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));
> + 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_write(dsi, &packet);
> }
>
> static const struct mipi_dsi_host_ops dw_mipi_dsi_host_ops = {
>
--
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
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 <[email protected]>
Thanks!
> Queued to drm-misc-next as is.
Thanks!
Brian
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 <[email protected]>
> Reviewed-by: Philippe Cornu <[email protected]>
> Tested-by: Philippe Cornu <[email protected]>
...
> 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 = {