| Message ID | 20190328134134.22479-3-srinivas.kandagatla@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | soundwire: few trival fixes and cleanups. | expand |
On 3/28/19 9:41 AM, Srinivas Kandagatla wrote: > Looks like there is a typo while checking number of messages, which > should be checked in defer pointer rather than message length. > > Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org> > --- > drivers/soundwire/cadence_master.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/soundwire/cadence_master.c b/drivers/soundwire/cadence_master.c > index cb6a331f448a..d41adbc57918 100644 > --- a/drivers/soundwire/cadence_master.c > +++ b/drivers/soundwire/cadence_master.c > @@ -434,7 +434,7 @@ cdns_xfer_msg_defer(struct sdw_bus *bus, > int cmd = 0, ret; > > /* for defer only 1 message is supported */ > - if (msg->len > 1) I am not sure this is a typo. IRRC the hardware only defer e.g. a single write that can perform bank switches and writes at specific times indicated with an SSP. What's more is that the defer structure is actually modified below this code (not shown in the diff) to set defer>length = msg->len, so testing before the value is set looks more suspicious than the current code. Vinod, does this ring a bell? > + if (defer->length > 1) > return -ENOTSUPP; > > ret = cdns_prep_msg(cdns, msg, &cmd); >
On 28/03/2019 14:03, Pierre-Louis Bossart wrote: >> --- >> drivers/soundwire/cadence_master.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/drivers/soundwire/cadence_master.c >> b/drivers/soundwire/cadence_master.c >> index cb6a331f448a..d41adbc57918 100644 >> --- a/drivers/soundwire/cadence_master.c >> +++ b/drivers/soundwire/cadence_master.c >> @@ -434,7 +434,7 @@ cdns_xfer_msg_defer(struct sdw_bus *bus, >> int cmd = 0, ret; >> /* for defer only 1 message is supported */ >> - if (msg->len > 1) > > I am not sure this is a typo. > Am bit confused with defer data-structure itself. I was assuming that defer->length is number of sdw messages in the structure. If its same as message lenght then sdw_msg->len and sdw_defer->length are totally redundant. May be we should get rid of lenght field in defer to avoid confusion? > IRRC the hardware only defer e.g. a single write that can perform bank > switches and writes at specific times indicated with an SSP. What's more okay got it. Does that mean that candence controller can only support 1 byte defer transfers? > is that the defer structure is actually modified below this code (not > shown in the diff) to set defer>length = msg->len, so testing before the > value is set looks more suspicious than the current code. removing the length field in defer should get rid of such instances. --srini > > Vinod, does this ring a bell? > > >> + if (defer->length > 1) >> return -ENOTSUPP; >> ret = cdns_prep_msg(cdns, msg, &cmd);
On Thu, Mar 28, 2019 at 02:58:27PM +0000, Srinivas Kandagatla wrote: > > > On 28/03/2019 14:03, Pierre-Louis Bossart wrote: > > > --- > > > ?? drivers/soundwire/cadence_master.c | 2 +- > > > ?? 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > diff --git a/drivers/soundwire/cadence_master.c > > > b/drivers/soundwire/cadence_master.c > > > index cb6a331f448a..d41adbc57918 100644 > > > --- a/drivers/soundwire/cadence_master.c > > > +++ b/drivers/soundwire/cadence_master.c > > > @@ -434,7 +434,7 @@ cdns_xfer_msg_defer(struct sdw_bus *bus, > > > ?????????? int cmd = 0, ret; > > > ?????????? /* for defer only 1 message is supported */ > > > -?????? if (msg->len > 1) > > > > I am not sure this is a typo. > > > > Am bit confused with defer data-structure itself. > > I was assuming that defer->length is number of sdw messages in the > structure. If its same as message lenght then sdw_msg->len and > sdw_defer->length are totally redundant. May be we should get rid of lenght > field in defer to avoid confusion? > > > IRRC the hardware only defer e.g. a single write that can perform bank > > switches and writes at specific times indicated with an SSP. What's more > okay got it. > Does that mean that candence controller can only support 1 byte defer > transfers? > > > is that the defer structure is actually modified below this code (not > > shown in the diff) to set defer>length = msg->len, so testing before the > > value is set looks more suspicious than the current code. > > removing the length field in defer should get rid of such instances. > defer->length is passed as count in cdns_fill_msg_resp function while processing response of defer message. we can as well use msg->len and remove length from sdw_defer. I dont see any other instance where defer->length is used. > --srini > > > > > Vinod, does this ring a bell? > > > > > > > +?????? if (defer->length > 1) > > > ?????????????????? return -ENOTSUPP; > > > ?????????? ret = cdns_prep_msg(cdns, msg, &cmd);
On 28-03-19, 14:58, Srinivas Kandagatla wrote: > > > On 28/03/2019 14:03, Pierre-Louis Bossart wrote: > > > --- > > > drivers/soundwire/cadence_master.c | 2 +- > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > diff --git a/drivers/soundwire/cadence_master.c > > > b/drivers/soundwire/cadence_master.c > > > index cb6a331f448a..d41adbc57918 100644 > > > --- a/drivers/soundwire/cadence_master.c > > > +++ b/drivers/soundwire/cadence_master.c > > > @@ -434,7 +434,7 @@ cdns_xfer_msg_defer(struct sdw_bus *bus, > > > int cmd = 0, ret; > > > /* for defer only 1 message is supported */ > > > - if (msg->len > 1) > > > > I am not sure this is a typo. > > > > Am bit confused with defer data-structure itself. > > I was assuming that defer->length is number of sdw messages in the > structure. If its same as message lenght then sdw_msg->len and > sdw_defer->length are totally redundant. May be we should get rid of lenght > field in defer to avoid confusion? Sorry Srini, I didnt pay much attention to it last time we discussed. So relooking at the code, we send msg as a message to trf (defered or not) and then copy length few lines down, but I agree this is not most useful and should be cleaned up. > > IRRC the hardware only defer e.g. a single write that can perform bank > > switches and writes at specific times indicated with an SSP. What's more > okay got it. > Does that mean that candence controller can only support 1 byte defer > transfers? I think so, Pierre can confirm. > > is that the defer structure is actually modified below this code (not > > shown in the diff) to set defer>length = msg->len, so testing before the > > value is set looks more suspicious than the current code. > > removing the length field in defer should get rid of such instances. Right!
On 29/03/2019 05:54, Vinod Koul wrote: >>> is that the defer structure is actually modified below this code (not >>> shown in the diff) to set defer>length = msg->len, so testing before the >>> value is set looks more suspicious than the current code. >> removing the length field in defer should get rid of such instances. > Right! I will go ahead and post the cleanup patch to get rid of confusing/redundant length field in defer. Thanks, srini
diff --git a/drivers/soundwire/cadence_master.c b/drivers/soundwire/cadence_master.c index cb6a331f448a..d41adbc57918 100644 --- a/drivers/soundwire/cadence_master.c +++ b/drivers/soundwire/cadence_master.c @@ -434,7 +434,7 @@ cdns_xfer_msg_defer(struct sdw_bus *bus, int cmd = 0, ret; /* for defer only 1 message is supported */ - if (msg->len > 1) + if (defer->length > 1) return -ENOTSUPP; ret = cdns_prep_msg(cdns, msg, &cmd);
Looks like there is a typo while checking number of messages, which should be checked in defer pointer rather than message length. Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org> --- drivers/soundwire/cadence_master.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)