[alsa-devel] [PATCH v5 07/13] soundwire: Add stream configuration APIs

[Sanyog Kale]

On Thu, Apr 26, 2018 at 09:58:56AM +0530, Vinod Koul wrote:
> On Thu, Apr 26, 2018 at 09:30:16AM +0530, Sanyog Kale wrote:
> > On Wed, Apr 25, 2018 at 03:09:17PM +0530, Vinod Koul wrote:
> > > From: Sanyog Kale <sanyog.r.kale at intel.com>
> > > 
> > > Add APIs for prepare, enable, disable and de-prepare stream.
> > > 
> > > Signed-off-by: Sanyog Kale <sanyog.r.kale at intel.com>
> > > Signed-off-by: Shreyas NC <shreyas.nc at intel.com>
> > > Signed-off-by: Vinod Koul <vkoul at kernel.org>
> > > ---
> > >  drivers/soundwire/bus.c       |   9 ++
> > >  drivers/soundwire/stream.c    | 246 ++++++++++++++++++++++++++++++++++++++++++
> > >  include/linux/soundwire/sdw.h |   4 +
> > >  3 files changed, 259 insertions(+)
> > > 
> > > +
> > > +static int _sdw_deprepare_stream(struct sdw_stream_runtime *stream)
> > > +{
> > > +	struct sdw_master_runtime *m_rt = stream->m_rt;
> > > +	struct sdw_bus *bus = m_rt->bus;
> > > +	int ret = 0;
> > > +
> > > +	/* De-prepare port(s) */
> > > +	ret = sdw_prep_deprep_ports(m_rt, false);
> > > +	if (ret < 0) {
> > > +		dev_err(bus->dev, "De-prepare port(s) failed: %d", ret);
> > > +		return ret;
> > > +	}
> > > +
> > > +	/* TODO: Update this during Device-Device support */
> > > +	bus->params.bandwidth -= m_rt->stream->params.rate *
> > > +		m_rt->ch_count * m_rt->stream->params.bps;
> > > +
> > 
> > We should have kept zero bandwidth check here because there is no need
> > to perform sdw_program_params when no stream is running on bus.
> 
> Looking at it I think it helps to program the calculated values, we may get
> a new stream even before we get a chance to suspend. And frankly at bus, we
> should not make any assumptions about suspend behaviour they may change with
> platforms and archs :)
>

Whats the use of computing & programming values when there is
no stream running? The computing and programming of values for new stream
will be done in new stream setup flow ie. in prepare, nothing needs to be
done here.

I agree we should not make any suspend behaviour assumption here. In
case of zero bandwidth, we should just change the stream state to
DEPREPARE and return.


> > 
> > > +	/* Program params */
> > > +	ret = sdw_program_params(bus);
> > > +	if (ret < 0) {
> > > +		dev_err(bus->dev, "Program params failed: %d", ret);
> > > +		return ret;
> > > +	}
> > > +
> > > +	return do_bank_switch(stream);
> > 
> > Change the state to DEPREPARE and then return.
> 
> good spot will fix
> 
> > 
> > > +
> > > +	stream->state = SDW_STREAM_DEPREPARED;
> > > +
> > > +	return ret;
> -- 
> ~Vinod

--