Re: [PATCH 2/4] soundwire: cdns: fix check for number of messages

[Sanyog Kale <sanyog.r.kale@intel.com>]

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);

--