* [PATCH v1 0/2] drm/bridge/synopsys: dsi: Add fix & warning in dsi_host_transfer() @ 2018-01-23 14:26 Philippe Cornu 2018-01-23 14:26 ` [PATCH v1 1/2] drm/bridge/synopsys: dsi: Fix dsi_host_transfer() return value Philippe Cornu 2018-01-23 14:26 ` [PATCH v1 2/2] drm/bridge/synopsys: dsi: Add a warning msg on dsi read operations Philippe Cornu 0 siblings, 2 replies; 11+ messages in thread From: Philippe Cornu @ 2018-01-23 14:26 UTC (permalink / raw) To: Archit Taneja, Andrzej Hajda, Laurent Pinchart, David Airlie, Philippe Cornu, Brian Norris, Benjamin Gaignard, Bhumika Goyal, dri-devel, linux-kernel, Sandy Huang, Heiko Stubner, linux-arm-kernel, linux-rockchip Cc: Yannick Fertre, Vincent Abriou, Alexandre Torgue, Maxime Coquelin, Ludovic Barre, Mickael Reulier Add a fix & a warning in the dsi_host_transfer(). Philippe Cornu (2): drm/bridge/synopsys: dsi: Fix dsi_host_transfer() return value drm/bridge/synopsys: dsi: Add a warning msg on dsi read operations drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c | 17 +++++++++++++++-- 1 file changed, 15 insertions(+), 2 deletions(-) -- 2.15.1 ^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH v1 1/2] drm/bridge/synopsys: dsi: Fix dsi_host_transfer() return value 2018-01-23 14:26 [PATCH v1 0/2] drm/bridge/synopsys: dsi: Add fix & warning in dsi_host_transfer() Philippe Cornu @ 2018-01-23 14:26 ` Philippe Cornu 2018-01-23 21:38 ` Brian Norris 2018-01-23 14:26 ` [PATCH v1 2/2] drm/bridge/synopsys: dsi: Add a warning msg on dsi read operations Philippe Cornu 1 sibling, 1 reply; 11+ messages in thread From: Philippe Cornu @ 2018-01-23 14:26 UTC (permalink / raw) To: Archit Taneja, Andrzej Hajda, Laurent Pinchart, David Airlie, Philippe Cornu, Brian Norris, Benjamin Gaignard, Bhumika Goyal, dri-devel, linux-kernel, Sandy Huang, Heiko Stubner, linux-arm-kernel, linux-rockchip Cc: Yannick Fertre, Vincent Abriou, Alexandre Torgue, Maxime Coquelin, Ludovic Barre, Mickael Reulier The dw_mipi_dsi_host_transfer() must return the number of bytes transmitted/received on success instead of 0. Note: nb_bytes is introduced in this patch as it will be re-used with the future dcs/generic dsi read feature. Signed-off-by: Philippe Cornu <philippe.cornu@st.com> --- drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c b/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c index f458798af788..096cf5e5bb30 100644 --- a/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c +++ b/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c @@ -403,7 +403,7 @@ static ssize_t dw_mipi_dsi_host_transfer(struct mipi_dsi_host *host, { struct dw_mipi_dsi *dsi = host_to_dsi(host); struct mipi_dsi_packet packet; - int ret; + int ret, nb_bytes; ret = mipi_dsi_create_packet(&packet, msg); if (ret) { @@ -413,7 +413,13 @@ static ssize_t dw_mipi_dsi_host_transfer(struct mipi_dsi_host *host, dw_mipi_message_config(dsi, msg); - return dw_mipi_dsi_write(dsi, &packet); + ret = dw_mipi_dsi_write(dsi, &packet); + if (ret) + return ret; + + nb_bytes = packet.size; + + return nb_bytes; } static const struct mipi_dsi_host_ops dw_mipi_dsi_host_ops = { -- 2.15.1 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH v1 1/2] drm/bridge/synopsys: dsi: Fix dsi_host_transfer() return value 2018-01-23 14:26 ` [PATCH v1 1/2] drm/bridge/synopsys: dsi: Fix dsi_host_transfer() return value Philippe Cornu @ 2018-01-23 21:38 ` Brian Norris 2018-01-24 13:33 ` Philippe CORNU 0 siblings, 1 reply; 11+ messages in thread From: Brian Norris @ 2018-01-23 21:38 UTC (permalink / raw) To: Philippe Cornu Cc: Archit Taneja, Andrzej Hajda, Laurent Pinchart, David Airlie, Benjamin Gaignard, Bhumika Goyal, dri-devel, Linux Kernel, Sandy Huang, Heiko Stubner, linux-arm-kernel, open list:ARM/Rockchip SoC..., Yannick Fertre, Vincent Abriou, Alexandre Torgue, Maxime Coquelin, Ludovic Barre, Mickael Reulier Hi Philippe, On Tue, Jan 23, 2018 at 6:26 AM, Philippe Cornu <philippe.cornu@st.com> wrote: > The dw_mipi_dsi_host_transfer() must return the number of > bytes transmitted/received on success instead of 0. I'm a little confused. As of the latest drm-misc-next I'm looking at, this still has conflicting documentation. For ->transfer(): On success it shall return the number of bytes * transmitted for write packets or the number of bytes received for read * packets. While mipi_dsi_generic_read() says: * Return: The number of bytes successfully read or a negative error code on * failure. But it just returns the value that ->transfer() returns. So I'm not sure whether the documentation is still wrong, or if the implementation is. Anyway, I guess maybe that isn't super-critical to *this* patch, since we don't have RX support yet... > Note: nb_bytes is introduced in this patch as it will be > re-used with the future dcs/generic dsi read feature. It feels like you could just wait to add that when you need it? It really feels trivial and useless right now :) Brian > > Signed-off-by: Philippe Cornu <philippe.cornu@st.com> > --- > drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c | 10 ++++++++-- > 1 file changed, 8 insertions(+), 2 deletions(-) > > diff --git a/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c b/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c > index f458798af788..096cf5e5bb30 100644 > --- a/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c > +++ b/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c > @@ -403,7 +403,7 @@ static ssize_t dw_mipi_dsi_host_transfer(struct mipi_dsi_host *host, > { > struct dw_mipi_dsi *dsi = host_to_dsi(host); > struct mipi_dsi_packet packet; > - int ret; > + int ret, nb_bytes; > > ret = mipi_dsi_create_packet(&packet, msg); > if (ret) { > @@ -413,7 +413,13 @@ static ssize_t dw_mipi_dsi_host_transfer(struct mipi_dsi_host *host, > > dw_mipi_message_config(dsi, msg); > > - return dw_mipi_dsi_write(dsi, &packet); > + ret = dw_mipi_dsi_write(dsi, &packet); > + if (ret) > + return ret; > + > + nb_bytes = packet.size; > + > + return nb_bytes; > } > > static const struct mipi_dsi_host_ops dw_mipi_dsi_host_ops = { > -- > 2.15.1 > ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v1 1/2] drm/bridge/synopsys: dsi: Fix dsi_host_transfer() return value 2018-01-23 21:38 ` Brian Norris @ 2018-01-24 13:33 ` Philippe CORNU 2018-01-24 18:37 ` Brian Norris 0 siblings, 1 reply; 11+ messages in thread From: Philippe CORNU @ 2018-01-24 13:33 UTC (permalink / raw) To: Brian Norris Cc: linux-arm-kernel, Maxime Coquelin, open list:ARM/Rockchip SoC..., David Airlie, Linux Kernel, dri-devel, Yannick FERTRE, Laurent Pinchart, Ludovic BARRE, Mickael REULIER, Vincent ABRIOU, Bhumika Goyal, Alexandre TORGUE Hi Brian, And many thanks for your review. On 01/23/2018 10:38 PM, Brian Norris wrote: > Hi Philippe, > > On Tue, Jan 23, 2018 at 6:26 AM, Philippe Cornu <philippe.cornu@st.com> wrote: >> The dw_mipi_dsi_host_transfer() must return the number of >> bytes transmitted/received on success instead of 0. > > I'm a little confused. As of the latest drm-misc-next I'm looking at, > this still has conflicting documentation. > > For ->transfer(): > > On success it shall return the number of bytes > * transmitted for write packets or the number of bytes received for read > * packets. > > While mipi_dsi_generic_read() says: > > * Return: The number of bytes successfully read or a negative error code on > * failure. > > But it just returns the value that ->transfer() returns. > Not sure to follow you here: mipi_dsi_generic_read() will trig a dsi generic read so it has to return "the number of bytes received for read packets" as explained for the ->transfer() function... so it looks "coherent"... But maybe you want to point out something different? > So I'm not sure whether the documentation is still wrong, or if the > implementation is. > > Anyway, I guess maybe that isn't super-critical to *this* patch, since > we don't have RX support yet... > The main reason why I want to "fix" this is because I do not want to explain to our customers (writing dsi panel drivers) why we have a different returned value compare to other platforms : ) >> Note: nb_bytes is introduced in this patch as it will be >> re-used with the future dcs/generic dsi read feature. > > It feels like you could just wait to add that when you need it? It > really feels trivial and useless right now :) > > Brian > Thanks, I agree, I will write & send a simpler version. Philippe :-) >> >> Signed-off-by: Philippe Cornu <philippe.cornu@st.com> >> --- >> drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c | 10 ++++++++-- >> 1 file changed, 8 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c b/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c >> index f458798af788..096cf5e5bb30 100644 >> --- a/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c >> +++ b/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c >> @@ -403,7 +403,7 @@ static ssize_t dw_mipi_dsi_host_transfer(struct mipi_dsi_host *host, >> { >> struct dw_mipi_dsi *dsi = host_to_dsi(host); >> struct mipi_dsi_packet packet; >> - int ret; >> + int ret, nb_bytes; >> >> ret = mipi_dsi_create_packet(&packet, msg); >> if (ret) { >> @@ -413,7 +413,13 @@ static ssize_t dw_mipi_dsi_host_transfer(struct mipi_dsi_host *host, >> >> dw_mipi_message_config(dsi, msg); >> >> - return dw_mipi_dsi_write(dsi, &packet); >> + ret = dw_mipi_dsi_write(dsi, &packet); >> + if (ret) >> + return ret; >> + >> + nb_bytes = packet.size; >> + >> + return nb_bytes; >> } >> >> static const struct mipi_dsi_host_ops dw_mipi_dsi_host_ops = { >> -- >> 2.15.1 >> > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel > ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v1 1/2] drm/bridge/synopsys: dsi: Fix dsi_host_transfer() return value 2018-01-24 13:33 ` Philippe CORNU @ 2018-01-24 18:37 ` Brian Norris 2018-01-25 12:16 ` Andrzej Hajda 0 siblings, 1 reply; 11+ messages in thread From: Brian Norris @ 2018-01-24 18:37 UTC (permalink / raw) To: Philippe CORNU Cc: linux-arm-kernel, Maxime Coquelin, open list:ARM/Rockchip SoC..., David Airlie, Linux Kernel, dri-devel, Yannick FERTRE, Laurent Pinchart, Ludovic BARRE, Mickael REULIER, Vincent ABRIOU, Bhumika Goyal, Alexandre TORGUE Hi Philippe, On Wed, Jan 24, 2018 at 01:33:54PM +0000, Philippe CORNU wrote: > On 01/23/2018 10:38 PM, Brian Norris wrote: > > Hi Philippe, > > > > On Tue, Jan 23, 2018 at 6:26 AM, Philippe Cornu <philippe.cornu@st.com> wrote: > >> The dw_mipi_dsi_host_transfer() must return the number of > >> bytes transmitted/received on success instead of 0. > > > > I'm a little confused. As of the latest drm-misc-next I'm looking at, > > this still has conflicting documentation. > > > > For ->transfer(): > > > > On success it shall return the number of bytes > > * transmitted for write packets or the number of bytes received for read > > * packets. > > > > While mipi_dsi_generic_read() says: > > > > * Return: The number of bytes successfully read or a negative error code on > > * failure. > > > > But it just returns the value that ->transfer() returns. > > > > Not sure to follow you here: mipi_dsi_generic_read() will trig a dsi > generic read so it has to return "the number of bytes received for read > packets" as explained for the ->transfer() function... so it looks > "coherent"... > > But maybe you want to point out something different? Actually, reading back what I wrote, I'm not sure it made sense. I think *I* was confusing "supporting TX only" with "supporting TX and RX". I believe the documentation isn't conflicting, but your current patch is a little misleading. With your current patch, you're returning the 'mipi_dsi_packet::size', which is the sum of both TX and RX. Since we only support TX right now, I suppose that actually is fine (because 'rx_len == 0'). But if we start supporting RX too, then this field is not the right one to return. Anyway, maybe this patch was fine as it was. But when you get RX support, this will have to be something like: if (msg->rx_len) return msg->rx_len; else return packet.size; BTW, does anyone actually care about seeing the number of TX bytes returned? That seems weird, because I wouldn't expect you'd get a good result from a partial TX (dunno about partial RX). And I also see that there are other drivers that get this all wrong too. See mtk_dsi_host_transfer(), which only returns 0 for TX and 'recv_cnt' for RX. So all-in-all, maybe my problem isn't that the documentation is conflicting, exactly, but that the requirements are somewhat odd, such that either implementations get it wrong (2 of 3 that I've looked at!), or they have to write somewhat odd special-casing. > > So I'm not sure whether the documentation is still wrong, or if the > > implementation is. > > > > Anyway, I guess maybe that isn't super-critical to *this* patch, since > > we don't have RX support yet... > > > > The main reason why I want to "fix" this is because I do not want to > explain to our customers (writing dsi panel drivers) why we have a > different returned value compare to other platforms : ) Brian ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v1 1/2] drm/bridge/synopsys: dsi: Fix dsi_host_transfer() return value 2018-01-24 18:37 ` Brian Norris @ 2018-01-25 12:16 ` Andrzej Hajda 2018-01-25 22:51 ` Brian Norris 0 siblings, 1 reply; 11+ messages in thread From: Andrzej Hajda @ 2018-01-25 12:16 UTC (permalink / raw) To: Brian Norris, Philippe CORNU Cc: Bhumika Goyal, Alexandre TORGUE, David Airlie, Linux Kernel, dri-devel, Yannick FERTRE, open list:ARM/Rockchip SoC..., Laurent Pinchart, Maxime Coquelin, Ludovic BARRE, Mickael REULIER, Vincent ABRIOU, linux-arm-kernel, Thierry Reding On 24.01.2018 19:37, Brian Norris wrote: > Hi Philippe, > > On Wed, Jan 24, 2018 at 01:33:54PM +0000, Philippe CORNU wrote: >> On 01/23/2018 10:38 PM, Brian Norris wrote: >>> Hi Philippe, >>> >>> On Tue, Jan 23, 2018 at 6:26 AM, Philippe Cornu <philippe.cornu@st.com> wrote: >>>> The dw_mipi_dsi_host_transfer() must return the number of >>>> bytes transmitted/received on success instead of 0. >>> I'm a little confused. As of the latest drm-misc-next I'm looking at, >>> this still has conflicting documentation. >>> >>> For ->transfer(): >>> >>> On success it shall return the number of bytes >>> * transmitted for write packets or the number of bytes received for read >>> * packets. >>> >>> While mipi_dsi_generic_read() says: >>> >>> * Return: The number of bytes successfully read or a negative error code on >>> * failure. >>> >>> But it just returns the value that ->transfer() returns. >>> >> Not sure to follow you here: mipi_dsi_generic_read() will trig a dsi >> generic read so it has to return "the number of bytes received for read >> packets" as explained for the ->transfer() function... so it looks >> "coherent"... >> >> But maybe you want to point out something different? > Actually, reading back what I wrote, I'm not sure it made sense. I think > *I* was confusing "supporting TX only" with "supporting TX and RX". I > believe the documentation isn't conflicting, but your current patch is a > little misleading. > > With your current patch, you're returning the 'mipi_dsi_packet::size', > which is the sum of both TX and RX. I did not found docs saying mipi_dsi_packet::size is a sum of tx and rx. tx and rx packets are two different packets, so they do not sum up. But thanks for bringing it up, it shows docs are incomplete/misleading. > Since we only support TX right now, > I suppose that actually is fine (because 'rx_len == 0'). But if we start > supporting RX too, then this field is not the right one to return. > > Anyway, maybe this patch was fine as it was. But when you get RX > support, this will have to be something like: > > if (msg->rx_len) > return msg->rx_len; > else > return packet.size; > > BTW, does anyone actually care about seeing the number of TX bytes > returned? That seems weird, because I wouldn't expect you'd get a good > result from a partial TX (dunno about partial RX). And I also see that > there are other drivers that get this all wrong too. See > mtk_dsi_host_transfer(), which only returns 0 for TX and 'recv_cnt' for > RX. As far as I remember MIPI DSI standard does not allow partial TX, it is all-or-nothing operation. > > So all-in-all, maybe my problem isn't that the documentation is > conflicting, exactly, but that the requirements are somewhat odd, such > that either implementations get it wrong (2 of 3 that I've looked at!), > or they have to write somewhat odd special-casing. mipi_dsi_host_ops::transfer in case of write sends only tx packet, in case of read it sends tx packets and receives rx packet, so it can be confusing what it should return in case of read. IMO changing mipi_dsi_host_ops::transfer to always return number of bytes RECEIVED or error should make it clearer and simpler. +CC Thierry Regards Andrzej > >>> So I'm not sure whether the documentation is still wrong, or if the >>> implementation is. >>> >>> Anyway, I guess maybe that isn't super-critical to *this* patch, since >>> we don't have RX support yet... >>> >> The main reason why I want to "fix" this is because I do not want to >> explain to our customers (writing dsi panel drivers) why we have a >> different returned value compare to other platforms : ) > Brian > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v1 1/2] drm/bridge/synopsys: dsi: Fix dsi_host_transfer() return value 2018-01-25 12:16 ` Andrzej Hajda @ 2018-01-25 22:51 ` Brian Norris 0 siblings, 0 replies; 11+ messages in thread From: Brian Norris @ 2018-01-25 22:51 UTC (permalink / raw) To: Andrzej Hajda Cc: Philippe CORNU, Bhumika Goyal, Alexandre TORGUE, David Airlie, Linux Kernel, dri-devel, Yannick FERTRE, open list:ARM/Rockchip SoC..., Laurent Pinchart, Maxime Coquelin, Ludovic BARRE, Mickael REULIER, Vincent ABRIOU, linux-arm-kernel, Thierry Reding On Thu, Jan 25, 2018 at 4:16 AM, Andrzej Hajda <a.hajda@samsung.com> wrote: > On 24.01.2018 19:37, Brian Norris wrote: >> With your current patch, you're returning the 'mipi_dsi_packet::size', >> which is the sum of both TX and RX. > > I did not found docs saying mipi_dsi_packet::size is a sum of tx and rx. > tx and rx packets are two different packets, so they do not sum up. > But thanks for bringing it up, it shows docs are incomplete/misleading. Ugh, I misread that again. No, mipi_dsi_packet::size is NOT claimed to contain both TX and RX. It just says "size of the packet", and packet clearly does not hold the RX data anyway. I don't know what's happening to my reading comprehension... But the mismatch on whether drivers implement these correctly and whether any callers actually care about the documented semantics still stands. Brian ^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH v1 2/2] drm/bridge/synopsys: dsi: Add a warning msg on dsi read operations 2018-01-23 14:26 [PATCH v1 0/2] drm/bridge/synopsys: dsi: Add fix & warning in dsi_host_transfer() Philippe Cornu 2018-01-23 14:26 ` [PATCH v1 1/2] drm/bridge/synopsys: dsi: Fix dsi_host_transfer() return value Philippe Cornu @ 2018-01-23 14:26 ` Philippe Cornu 2018-01-23 21:28 ` Brian Norris 1 sibling, 1 reply; 11+ messages in thread From: Philippe Cornu @ 2018-01-23 14:26 UTC (permalink / raw) To: Archit Taneja, Andrzej Hajda, Laurent Pinchart, David Airlie, Philippe Cornu, Brian Norris, Benjamin Gaignard, Bhumika Goyal, dri-devel, linux-kernel, Sandy Huang, Heiko Stubner, linux-arm-kernel, linux-rockchip Cc: Yannick Fertre, Vincent Abriou, Alexandre Torgue, Maxime Coquelin, Ludovic Barre, Mickael Reulier The DCS/GENERIC DSI read feature is not yet implemented so it is important to warn the host_transfer() caller in case of read operation requests. Signed-off-by: Philippe Cornu <philippe.cornu@st.com> --- drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c b/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c index 096cf5e5bb30..e46ddff8601c 100644 --- a/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c +++ b/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c @@ -417,7 +417,14 @@ static ssize_t dw_mipi_dsi_host_transfer(struct mipi_dsi_host *host, if (ret) return ret; - nb_bytes = packet.size; + if (msg->rx_buf && msg->rx_len > 0) { + /* TODO dw drv improvements: implement read feature */ + dev_warn(dsi->dev, "read operations not yet implemented\n"); + return -EPERM; + + } else { + nb_bytes = packet.size; + } return nb_bytes; } -- 2.15.1 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH v1 2/2] drm/bridge/synopsys: dsi: Add a warning msg on dsi read operations 2018-01-23 14:26 ` [PATCH v1 2/2] drm/bridge/synopsys: dsi: Add a warning msg on dsi read operations Philippe Cornu @ 2018-01-23 21:28 ` Brian Norris 2018-01-24 13:22 ` Philippe CORNU 0 siblings, 1 reply; 11+ messages in thread From: Brian Norris @ 2018-01-23 21:28 UTC (permalink / raw) To: Philippe Cornu Cc: Archit Taneja, Andrzej Hajda, Laurent Pinchart, David Airlie, Benjamin Gaignard, Bhumika Goyal, dri-devel, Linux Kernel, Sandy Huang, Heiko Stubner, linux-arm-kernel, open list:ARM/Rockchip SoC..., Yannick Fertre, Vincent Abriou, Alexandre Torgue, Maxime Coquelin, Ludovic Barre, Mickael Reulier Hi Philippe, I see you sent this out already today, while I only just responded (late) to your questions about it... oh well :) On Tue, Jan 23, 2018 at 6:26 AM, Philippe Cornu <philippe.cornu@st.com> wrote: > The DCS/GENERIC DSI read feature is not yet implemented so it > is important to warn the host_transfer() caller in case of > read operation requests. > > Signed-off-by: Philippe Cornu <philippe.cornu@st.com> > --- > drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c | 9 ++++++++- > 1 file changed, 8 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c b/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c > index 096cf5e5bb30..e46ddff8601c 100644 > --- a/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c > +++ b/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c > @@ -417,7 +417,14 @@ static ssize_t dw_mipi_dsi_host_transfer(struct mipi_dsi_host *host, > if (ret) > return ret; > > - nb_bytes = packet.size; > + if (msg->rx_buf && msg->rx_len > 0) { It feels like you should do this check *before* you start writing anything. It's possible to have a combination TX/RX command, and it would be counterintuitive to only do half the operation then return with an argument error. > + /* TODO dw drv improvements: implement read feature */ > + dev_warn(dsi->dev, "read operations not yet implemented\n"); > + return -EPERM; I'm not sure -EPERM is right. Feels like -EINVAL, -ENOSYS, or -EOPNOTSUPP. I think -ENOSYS actually has been abused somewhat, so maybe one of the other two. > + Spurious blank line? > + } else { > + nb_bytes = packet.size; > + } You don't actually need to put this sort of thing in the 'else' case. The other branch is an error-handling case, which definitely 'return's early, and it's pretty standard coding style to avoid indenting the "good" path like this. Brian > > return nb_bytes; > } > -- > 2.15.1 > ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v1 2/2] drm/bridge/synopsys: dsi: Add a warning msg on dsi read operations 2018-01-23 21:28 ` Brian Norris @ 2018-01-24 13:22 ` Philippe CORNU 2018-01-24 18:14 ` Brian Norris 0 siblings, 1 reply; 11+ messages in thread From: Philippe CORNU @ 2018-01-24 13:22 UTC (permalink / raw) To: Brian Norris Cc: Archit Taneja, Andrzej Hajda, Laurent Pinchart, David Airlie, Benjamin Gaignard, Bhumika Goyal, dri-devel, Linux Kernel, Sandy Huang, Heiko Stubner, linux-arm-kernel, open list:ARM/Rockchip SoC..., Yannick FERTRE, Vincent ABRIOU, Alexandre TORGUE, Maxime Coquelin, Ludovic BARRE, Mickael REULIER Hi Brian, On 01/23/2018 10:28 PM, Brian Norris wrote: > Hi Philippe, > > I see you sent this out already today, while I only just responded > (late) to your questions about it... oh well :) > I got a short period to clean-up and adds features to this driver (1.31 ip version + maybe the read feature), sorry to have not wait a single day more. > On Tue, Jan 23, 2018 at 6:26 AM, Philippe Cornu <philippe.cornu@st.com> wrote: >> The DCS/GENERIC DSI read feature is not yet implemented so it >> is important to warn the host_transfer() caller in case of >> read operation requests. >> >> Signed-off-by: Philippe Cornu <philippe.cornu@st.com> >> --- >> drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c | 9 ++++++++- >> 1 file changed, 8 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c b/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c >> index 096cf5e5bb30..e46ddff8601c 100644 >> --- a/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c >> +++ b/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c >> @@ -417,7 +417,14 @@ static ssize_t dw_mipi_dsi_host_transfer(struct mipi_dsi_host *host, >> if (ret) >> return ret; >> >> - nb_bytes = packet.size; >> + if (msg->rx_buf && msg->rx_len > 0) { > > It feels like you should do this check *before* you start writing > anything. It's possible to have a combination TX/RX command, and it > would be counterintuitive to only do half the operation then return > with an argument error. > Many thanks for your review. I agree with your comments. Well, my patch is not good at all because it contains a small part of the read feature I am writing... but it is not the purpose of this patch. No excuse, sorry guys for making you waste time. I will re-write a new patch 100% decorrelated from a possible future read feature. I could also wait until I have a working read feature but as it could take some times, I prefer warning users asap. >> + /* TODO dw drv improvements: implement read feature */ >> + dev_warn(dsi->dev, "read operations not yet implemented\n"); >> + return -EPERM; > > I'm not sure -EPERM is right. Feels like -EINVAL, -ENOSYS, or > -EOPNOTSUPP. I think -ENOSYS actually has been abused somewhat, so > maybe one of the other two. > not easy to pick the right one. I will use -EINVAL. >> + > > Spurious blank line? > thanks >> + } else { >> + nb_bytes = packet.size; >> + } > > You don't actually need to put this sort of thing in the 'else' case. > The other branch is an error-handling case, which definitely 'return's > early, and it's pretty standard coding style to avoid indenting the > "good" path like this. > > Brian > The else part is linked to my "read feature" too, sorry for that. I will do it simpler in next version. Thank you, Philippe :-) >> >> return nb_bytes; >> } >> -- >> 2.15.1 >> ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v1 2/2] drm/bridge/synopsys: dsi: Add a warning msg on dsi read operations 2018-01-24 13:22 ` Philippe CORNU @ 2018-01-24 18:14 ` Brian Norris 0 siblings, 0 replies; 11+ messages in thread From: Brian Norris @ 2018-01-24 18:14 UTC (permalink / raw) To: Philippe CORNU Cc: Archit Taneja, Andrzej Hajda, Laurent Pinchart, David Airlie, Benjamin Gaignard, Bhumika Goyal, dri-devel, Linux Kernel, Sandy Huang, Heiko Stubner, linux-arm-kernel, open list:ARM/Rockchip SoC..., Yannick FERTRE, Vincent ABRIOU, Alexandre TORGUE, Maxime Coquelin, Ludovic BARRE, Mickael REULIER Hi Philippe, On Wed, Jan 24, 2018 at 01:22:04PM +0000, Philippe CORNU wrote: > On 01/23/2018 10:28 PM, Brian Norris wrote: > > I see you sent this out already today, while I only just responded > > (late) to your questions about it... oh well :) > > > > I got a short period to clean-up and adds features to this driver (1.31 > ip version + maybe the read feature), sorry to have not wait a single > day more. No problem. The key word was "late"; my mail was buried enough I just missed responding. Not your fault! > > On Tue, Jan 23, 2018 at 6:26 AM, Philippe Cornu <philippe.cornu@st.com> wrote: > >> The DCS/GENERIC DSI read feature is not yet implemented so it > >> is important to warn the host_transfer() caller in case of > >> read operation requests. > >> > >> Signed-off-by: Philippe Cornu <philippe.cornu@st.com> > >> --- > >> drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c | 9 ++++++++- > >> 1 file changed, 8 insertions(+), 1 deletion(-) > >> > >> diff --git a/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c b/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c > >> index 096cf5e5bb30..e46ddff8601c 100644 > >> --- a/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c > >> +++ b/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c > >> @@ -417,7 +417,14 @@ static ssize_t dw_mipi_dsi_host_transfer(struct mipi_dsi_host *host, > >> if (ret) > >> return ret; > >> > >> - nb_bytes = packet.size; > >> + if (msg->rx_buf && msg->rx_len > 0) { > > > > It feels like you should do this check *before* you start writing > > anything. It's possible to have a combination TX/RX command, and it > > would be counterintuitive to only do half the operation then return > > with an argument error. > > > > Many thanks for your review. > > I agree with your comments. > > Well, my patch is not good at all because it contains a small part of > the read feature I am writing... but it is not the purpose of this patch. > > No excuse, sorry guys for making you waste time. No worries. These weren't that bad anyway, just a little suboptimal :) > I will re-write a new patch 100% decorrelated from a possible future > read feature. Yeah, that would probably work best. It's hard to write and review good "intermediate" code; we should write it as if the code will last as-is. > I could also wait until I have a working read feature but as it could > take some times, I prefer warning users asap. Sounds good. [snip] Thanks, Brian ^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2018-01-25 22:51 UTC | newest] Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2018-01-23 14:26 [PATCH v1 0/2] drm/bridge/synopsys: dsi: Add fix & warning in dsi_host_transfer() Philippe Cornu 2018-01-23 14:26 ` [PATCH v1 1/2] drm/bridge/synopsys: dsi: Fix dsi_host_transfer() return value Philippe Cornu 2018-01-23 21:38 ` Brian Norris 2018-01-24 13:33 ` Philippe CORNU 2018-01-24 18:37 ` Brian Norris 2018-01-25 12:16 ` Andrzej Hajda 2018-01-25 22:51 ` Brian Norris 2018-01-23 14:26 ` [PATCH v1 2/2] drm/bridge/synopsys: dsi: Add a warning msg on dsi read operations Philippe Cornu 2018-01-23 21:28 ` Brian Norris 2018-01-24 13:22 ` Philippe CORNU 2018-01-24 18:14 ` Brian Norris
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).