Re: [PATCH v3 5/6] soundwire: Add support for multi link bank switch

[Shreyas NC <shreyas.nc@intel.com>]

On Thu, Jun 14, 2018 at 01:34:13PM -0500, Pierre-Louis Bossart wrote:
> 
> >diff --git a/drivers/soundwire/stream.c b/drivers/soundwire/stream.c
> >index 0e8a2eb5..771c963 100644
> >--- a/drivers/soundwire/stream.c
> >+++ b/drivers/soundwire/stream.c
> >@@ -638,6 +638,8 @@ static int sdw_bank_switch(struct sdw_bus *bus)
> >  	if (!wr_msg)
> >  		return -ENOMEM;
> >+	bus->defer_msg.msg = wr_msg;
> >+
> >  	wbuf = kzalloc(sizeof(*wbuf), GFP_KERNEL);
> >  	if (!wbuf) {
> >  		ret = -ENOMEM;
> >@@ -658,17 +660,23 @@ static int sdw_bank_switch(struct sdw_bus *bus)
> >  					SDW_MSG_FLAG_WRITE, wbuf);
> >  	wr_msg->ssp_sync = true;
> >-	ret = sdw_transfer(bus, wr_msg);
> >+	if (bus->multi_link)
> >+		ret = sdw_transfer_defer(bus, wr_msg, &bus->defer_msg);
> >+	else
> >+		ret = sdw_transfer(bus, wr_msg);
> >+
> 
> Re-reading this v3, I wonder if there is a small logic flaw here or an
> intended behavior.
> 
> the bus->multi_link field is a hardware capability, it provides information
> on whether the hardware can support synchronized bank switches across
> multiple Master interfaces. This is independent of the streams being
> handled, it's a bus property - not a stream one.
> 
> However the default/typical case is a stream handled by a single master, so
> when the hardware is capable of handling multiple masters this does not mean
> you should use the deferred transfer, the regular case would work just fine
> for a regular stream. If your SSP rate is faster than the common sync signal
> it might even be beneficial not to use the deferred transfer.
> 
> in other words, should the code be written with something like
> if (bus->multi_link && master_rt_count > 1)
> with master_rt_count being determined based on the stream properties.
> 
> Or was the intent that you always rely on the deferred mechanism, even if
> the stream is not handled by multiple masters? In that case, do we even need
> the regular bank switch?

After discussing with folks here, I understand that intent was to have an
unified flow for both Multi and Single instance cases. Yes, we can always
use the sdw_transfer_defer() and remove the references to bus->multi_link
possibly. So, I will fix this up and post v4.

Thanks for the catch :)


--Shreyas

--