[alsa-devel] [PATCH v2 02/13] soundwire: Add support for SoundWire stream management
Pierre-Louis Bossart
pierre-louis.bossart at linux.intel.com
Fri Apr 6 00:34:18 CEST 2018
On 4/5/18 11:48 AM, Vinod Koul wrote:
> From: Sanyog Kale <sanyog.r.kale at intel.com>
>
> This patch adds APIs and relevant stream data structures
> for initialization and release of stream.
>
> Signed-off-by: Hardik T Shah <hardik.t.shah at intel.com>
> 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 <vinod.koul at intel.com>
> ---
> drivers/soundwire/Makefile | 2 +-
> drivers/soundwire/bus.c | 1 +
> drivers/soundwire/bus.h | 36 +++++
> drivers/soundwire/stream.c | 354 ++++++++++++++++++++++++++++++++++++++++++
> include/linux/soundwire/sdw.h | 109 +++++++++++++
> 5 files changed, 501 insertions(+), 1 deletion(-)
> create mode 100644 drivers/soundwire/stream.c
>
> diff --git a/drivers/soundwire/Makefile b/drivers/soundwire/Makefile
> index e1a74c5692aa..5817beaca0e1 100644
> --- a/drivers/soundwire/Makefile
> +++ b/drivers/soundwire/Makefile
> @@ -3,7 +3,7 @@
> #
>
> #Bus Objs
> -soundwire-bus-objs := bus_type.o bus.o slave.o mipi_disco.o
> +soundwire-bus-objs := bus_type.o bus.o slave.o mipi_disco.o stream.o
> obj-$(CONFIG_SOUNDWIRE_BUS) += soundwire-bus.o
>
> #Cadence Objs
> diff --git a/drivers/soundwire/bus.c b/drivers/soundwire/bus.c
> index d6dc8e7a8614..abf046f6b188 100644
> --- a/drivers/soundwire/bus.c
> +++ b/drivers/soundwire/bus.c
> @@ -32,6 +32,7 @@ int sdw_add_bus_master(struct sdw_bus *bus)
> mutex_init(&bus->msg_lock);
> mutex_init(&bus->bus_lock);
> INIT_LIST_HEAD(&bus->slaves);
> + INIT_LIST_HEAD(&bus->m_rt_list);
>
> if (bus->ops->read_prop) {
> ret = bus->ops->read_prop(bus);
> diff --git a/drivers/soundwire/bus.h b/drivers/soundwire/bus.h
> index 345c34d697e9..3c66b6aecc14 100644
> --- a/drivers/soundwire/bus.h
> +++ b/drivers/soundwire/bus.h
> @@ -45,6 +45,42 @@ struct sdw_msg {
> bool page;
> };
>
> +/**
> + * sdw_slave_runtime: Runtime Stream parameters for Slave
> + *
> + * @slave: Slave handle
> + * @direction: Data direction for Slave
> + * @ch_count: Number of channels handled by the Slave for
> + * this stream
> + * @m_rt_node: sdw_master_runtime list node
> + */
> +struct sdw_slave_runtime {
> + struct sdw_slave *slave;
> + enum sdw_data_direction direction;
> + unsigned int ch_count;
> + struct list_head m_rt_node;
> +};
> +
> +/**
> + * sdw_master_runtime: Runtime stream parameters for Master
> + *
> + * @bus: Bus handle
> + * @stream: Stream runtime handle
> + * @direction: Data direction for Master
> + * @ch_count: Number of channels handled by the Master for
> + * this stream
Didn't we say that this count could be zero for device-to-device
communication? 'handled by the Master' is not really correct.
> + * @slave_rt_list: Slave runtime list
> + * @bus_node: sdw_bus m_rt_list node
I think the intention was to progress in steps, but by omitting
references to the multi-master case in this series or comments
explaining the next steps the structure and code are not easy to follow.
There should be a clear explanation that
- a stream can be connected a least one Slave Device with one or more ports.
- a stream may be connected to zero or more Master Devices. In the
former case one of the Slave devices has provide TX ports.
- the slave runtime keeps track of all the masters the stream is
connected to
- the master runtime keeps track of
1. all the Slaves it is physically connected to which support a stream.
2. all bus instances (and related masters) that support this stream.
> + */
> +struct sdw_master_runtime {
> + struct sdw_bus *bus;
> + struct sdw_stream_runtime *stream;
> + enum sdw_data_direction direction;
> + unsigned int ch_count;
> + struct list_head slave_rt_list;
> + struct list_head bus_node;
> +};
> +
> int sdw_transfer(struct sdw_bus *bus, struct sdw_msg *msg);
> int sdw_transfer_defer(struct sdw_bus *bus, struct sdw_msg *msg,
> struct sdw_defer *defer);
> diff --git a/drivers/soundwire/stream.c b/drivers/soundwire/stream.c
> new file mode 100644
> index 000000000000..5c34177e4954
> --- /dev/null
> +++ b/drivers/soundwire/stream.c
> @@ -0,0 +1,354 @@
> +// SPDX-License-Identifier: (GPL-2.0 OR BSD-3-Clause)
> +// Copyright(c) 2015-18 Intel Corporation.
> +
> +/*
> + * stream.c - SoundWire Bus stream operations.
> + */
> +
> +#include <linux/delay.h>
> +#include <linux/device.h>
> +#include <linux/init.h>
> +#include <linux/module.h>
> +#include <linux/mod_devicetable.h>
> +#include <linux/slab.h>
> +#include <linux/soundwire/sdw.h>
> +#include "bus.h"
> +
> +/**
> + * sdw_release_stream: Free the assigned stream runtime
> + *
> + * @stream: SoundWire stream runtime
> + *
> + * sdw_release_stream should be called only once per stream
> + */
> +void sdw_release_stream(struct sdw_stream_runtime *stream)
> +{
> + kfree(stream);
> +}
> +EXPORT_SYMBOL(sdw_release_stream);
> +
> +/**
> + * sdw_alloc_stream: Allocate and return stream runtime
> + *
> + * @stream_name: SoundWire stream name
> + *
> + * Allocates a SoundWire stream runtime instance.
> + * sdw_alloc_stream should be called only once per stream
You should explain who the caller might be, and probably check if a
stream with the same name was not already allocated.
> + */
> +struct sdw_stream_runtime *sdw_alloc_stream(char *stream_name)
> +{
> + struct sdw_stream_runtime *stream;
> +
> + stream = kzalloc(sizeof(*stream), GFP_KERNEL);
> + if (!stream)
> + return NULL;
> +
> + stream->name = stream_name;
> + stream->state = SDW_STREAM_ALLOCATED;
> +
> + return stream;
> +}
> +EXPORT_SYMBOL(sdw_alloc_stream);
> +
> +/**
> + * sdw_alloc_master_rt: Allocates and initialize Master runtime handle
> + *
> + * @bus: SDW bus instance
> + * @stream_config: Stream configuration
> + * @stream: Stream runtime handle.
> + *
> + * This function is to be called with bus_lock held.
> + */
> +static struct sdw_master_runtime
> +*sdw_alloc_master_rt(struct sdw_bus *bus,
> + struct sdw_stream_config *stream_config,
> + struct sdw_stream_runtime *stream)
> +{
> + struct sdw_master_runtime *m_rt;
> +
> + m_rt = stream->m_rt;
> + if (m_rt)
> + goto stream_config;
I know there is a reason for this code pattern but I just can't remember
it and there is no comment indicated why this is valid.
> +
> + m_rt = kzalloc(sizeof(*m_rt), GFP_KERNEL);
> + if (!m_rt)
> + return NULL;
> +
> + /* Initialization of Master runtime handle */
> + INIT_LIST_HEAD(&m_rt->slave_rt_list);
> + stream->m_rt = m_rt;
> +
> + list_add_tail(&m_rt->bus_node, &bus->m_rt_list);
> +
> +stream_config:
> + m_rt->ch_count = stream_config->ch_count;
> + m_rt->bus = bus;
> + m_rt->stream = stream;
> + m_rt->direction = stream_config->direction;
> +
> + return m_rt;
> +}
> +
> +/**
> + * sdw_alloc_slave_rt: Allocate and initialize Slave runtime handle.
> + *
> + * @slave: Slave handle
> + * @stream_config: Stream configuration
> + * @stream: Stream runtime handle
> + *
> + * This function is to be called with bus_lock held.
> + */
> +static struct sdw_slave_runtime
> +*sdw_alloc_slave_rt(struct sdw_slave *slave,
> + struct sdw_stream_config *stream_config,
> + struct sdw_stream_runtime *stream)
> +{
> + struct sdw_slave_runtime *s_rt = NULL;
> +
> + s_rt = kzalloc(sizeof(*s_rt), GFP_KERNEL);
> + if (!s_rt)
> + return NULL;
> +
> +
> + s_rt->ch_count = stream_config->ch_count;
> + s_rt->direction = stream_config->direction;
> + s_rt->slave = slave;
> +
> + return s_rt;
> +}
> +
> +/**
> + * sdw_release_slave_stream: Free Slave(s) runtime handle
> + *
> + * @slave: Slave handle.
> + * @stream: Stream runtime handle.
> + *
> + * This function is to be called with bus_lock held.
> + */
> +static void sdw_release_slave_stream(struct sdw_slave *slave,
> + struct sdw_stream_runtime *stream)
> +{
> + struct sdw_slave_runtime *s_rt, *_s_rt;
> + struct sdw_master_runtime *m_rt = stream->m_rt;
> +
> + /* Retrieve Slave runtime handle */
> + list_for_each_entry_safe(s_rt, _s_rt,
> + &m_rt->slave_rt_list, m_rt_node) {
> +
> + if (s_rt->slave == slave) {
> + list_del(&s_rt->m_rt_node);
> + kfree(s_rt);
> + return;
> + }
> + }
> +}
> +
> +static void sdw_release_master_stream(struct sdw_stream_runtime *stream)
> +{
> + struct sdw_master_runtime *m_rt = stream->m_rt;
> + struct sdw_slave_runtime *s_rt, *_s_rt;
> +
> + list_for_each_entry_safe(s_rt, _s_rt,
> + &m_rt->slave_rt_list, m_rt_node)
> + sdw_release_slave_stream(s_rt->slave, stream);
> +
> + list_del(&m_rt->bus_node);
> +}
> +
> +/**
> + * sdw_stream_remove_master: Remove master from sdw_stream
> + *
> + * @bus: SDW Bus instance
> + * @stream: Soundwire stream
> + *
> + * This removes and frees master_rt from a stream
> + */
> +int sdw_stream_remove_master(struct sdw_bus *bus,
> + struct sdw_stream_runtime *stream)
> +{
> + mutex_lock(&bus->bus_lock);
> +
> + sdw_release_master_stream(stream);
> + stream->state = SDW_STREAM_RELEASED;
> + kfree(stream->m_rt);
> + stream->m_rt = NULL;
> +
> + mutex_unlock(&bus->bus_lock);
> +
> + return 0;
> +}
> +EXPORT_SYMBOL(sdw_stream_remove_master);
> +
> +/**
> + * sdw_stream_remove_slave: Remove slave from sdw_stream
> + *
> + * @slave: SDW Slave instance
> + * @stream: Soundwire stream
> + *
> + * This removes and frees slave_rt from a stream
> + */
> +int sdw_stream_remove_slave(struct sdw_slave *slave,
> + struct sdw_stream_runtime *stream)
> +{
> + mutex_lock(&slave->bus->bus_lock);
> +
> + sdw_release_slave_stream(slave, stream);
> +
> + mutex_unlock(&slave->bus->bus_lock);
> +
> + return 0;
> +}
> +EXPORT_SYMBOL(sdw_stream_remove_slave);
> +
> +/**
> + * sdw_config_stream: Configure the allocated stream
> + *
> + * @dev: SDW device
> + * @stream: Soundwire stream
> + * @stream_config: Stream configuration for audio stream
> + * @is_slave: is API called from Slave or Master
> + *
> + * This function is to be called with bus_lock held.
> + */
> +static int sdw_config_stream(struct device *dev,
> + struct sdw_stream_runtime *stream,
> + struct sdw_stream_config *stream_config, bool is_slave)
> +{
> +
> + /*
> + * Update the stream rate, channel and bps based on data
> + * source. For more than one data source (multilink),
> + * match the rate, bps, stream type and increment number of channels.
> + */
> + if ((stream->params.rate) &&
> + (stream->params.rate != stream_config->frame_rate)) {
> + dev_err(dev, "rate not matching, stream:%s", stream->name);
> + return -EINVAL;
> + }
> +
> + if ((stream->params.bps) &&
> + (stream->params.bps != stream_config->bps)) {
> + dev_err(dev, "bps not matching, stream:%s", stream->name);
> + return -EINVAL;
> + }
> +
> + stream->type = stream_config->type;
> + stream->params.rate = stream_config->frame_rate;
> + stream->params.bps = stream_config->bps;
> + if (is_slave)
> + stream->params.ch_count += stream_config->ch_count;
> +
> + return 0;
> +}
> +
> +/**
> + * sdw_stream_add_master: Allocate and add master runtime to a stream
> + *
> + * @bus: SDW Bus instance
> + * @stream_config: Stream configuration for audio stream
> + * @stream: Soundwire stream
> + */
> +int sdw_stream_add_master(struct sdw_bus *bus,
> + struct sdw_stream_config *stream_config,
> + struct sdw_stream_runtime *stream)
> +{
> + struct sdw_master_runtime *m_rt = NULL;
> + int ret;
> +
> + if (stream->state != SDW_STREAM_ALLOCATED &&
> + stream->state != SDW_STREAM_CONFIGURED) {
> + dev_err(bus->dev,
> + "Invalid stream state %d", stream->state);
> + return -EINVAL;
> + }
Humm, this check looks like new code but I don't get it. Why would one
add a master in either of those two states? Why not ALLOCATED only?
The documentation states that:
+SDW_STREAM_CONFIGURED
+~~~~~~~~~~~~~~~~~~~~~
+
+Configuration state of stream. Operations performed before entering in
+this state:
+
+ (1) The resources allocated for stream information in
SDW_STREAM_ALLOCATED
+ state are updated here. This includes stream parameters, Master(s)
+ and Slave(s) runtime information associated with current stream.
+
+ (2) All the Master(s) and Slave(s) associated with current stream provide
+ the port information to Bus which includes port numbers allocated by
+ Master(s) and Slave(s) for current stream and their channel mask.
so how can Master ports be configured if it is added to a stream *after*
the CONFIGURED state?
> +
> + mutex_lock(&bus->bus_lock);
> +
> + m_rt = sdw_alloc_master_rt(bus, stream_config, stream);
> + if (!m_rt) {
> + dev_err(bus->dev,
> + "Master runtime config failed for stream:%s",
> + stream->name);
> + ret = -ENOMEM;
> + goto error;
> + }
> +
> + ret = sdw_config_stream(bus->dev, stream, stream_config, false);
> + if (ret)
> + goto stream_error;
> +
> + if (!list_empty(&m_rt->slave_rt_list) &&
> + stream->state == SDW_STREAM_ALLOCATED)
> + stream->state = SDW_STREAM_CONFIGURED;
this looks also like new code, what is the idea about this configuration
business?
To me this is inconsistent with the documentation which says
sdw_stream_add_master is called in ALLOCATED state. Under what
circumstances would this routing be called from two different states?
This may be ok but the intent is clear as mud.
> +
> +stream_error:
> + sdw_release_master_stream(stream);
> +error:
> + mutex_unlock(&bus->bus_lock);
> + return ret;
> +
> +}
> +EXPORT_SYMBOL(sdw_stream_add_master);
> +
> +/**
> + * sdw_stream_add_slave: Allocate and add master/slave runtime to a stream
> + *
> + * @slave: SDW Slave instance
> + * @stream_config: Stream configuration for audio stream
> + * @stream: Soundwire stream
> + */
> +int sdw_stream_add_slave(struct sdw_slave *slave,
> + struct sdw_stream_config *stream_config,
> + struct sdw_stream_runtime *stream)
> +{
> + struct sdw_slave_runtime *s_rt;
> + struct sdw_master_runtime *m_rt;
> + int ret;
> +
> + if (stream->state != SDW_STREAM_ALLOCATED &&
> + stream->state != SDW_STREAM_CONFIGURED) {
> + dev_err(&slave->dev,
> + "Invalid stream state %d", stream->state);
> + return -EINVAL;
> + }
same as for the master, why would one call this routine from two
separate states?
> +
> + mutex_lock(&slave->bus->bus_lock);
> +
> + /*
> + * If this API is invoked by slave first then m_rt is not valid.
> + * So, allocate that and add the slave to it.
> + */
> + m_rt = sdw_alloc_master_rt(slave->bus, stream_config, stream);
> + if (!m_rt) {
> + dev_err(&slave->dev,
> + "alloc master runtime failed for stream:%s",
> + stream->name);
> + ret = -ENOMEM;
> + goto error;
> + }
> +
> + s_rt = sdw_alloc_slave_rt(slave, stream_config, stream);
> + if (!s_rt) {
> + dev_err(&slave->dev,
> + "Slave runtime config failed for stream:%s",
> + stream->name);
> + ret = -ENOMEM;
> + goto stream_error;
> + }
> +
> + ret = sdw_config_stream(&slave->dev, stream, stream_config, true);
> + if (ret)
> + goto stream_error;
> +
> + list_add_tail(&s_rt->m_rt_node, &m_rt->slave_rt_list);
so you have a potential to add the same node twice from ALLOCATED and
CONFIGURED states? I am completely lost.
> +
> + stream->state = SDW_STREAM_CONFIGURED;
> + goto error;
> +
> +stream_error:
> + sdw_release_master_stream(stream);
maybe add a comment that this will also release all previously allocated
slave_rt nodes for that stream?
> +error:
> + mutex_unlock(&slave->bus->bus_lock);
> + return ret;
> +}
> +EXPORT_SYMBOL(sdw_stream_add_slave);
> diff --git a/include/linux/soundwire/sdw.h b/include/linux/soundwire/sdw.h
> index e91fdcf41049..893e1b6b4914 100644
> --- a/include/linux/soundwire/sdw.h
> +++ b/include/linux/soundwire/sdw.h
> @@ -61,6 +61,30 @@ enum sdw_command_response {
> SDW_CMD_FAIL_OTHER = 4,
> };
>
> +/**
> + * enum sdw_stream_type: data stream type
> + *
> + * @SDW_STREAM_PCM: PCM data stream
> + * @SDW_STREAM_PDM: PDM data stream
> + *
> + * spec doesn't define this, but is used in implementation
> + */
> +enum sdw_stream_type {
> + SDW_STREAM_PCM = 0,
> + SDW_STREAM_PDM = 1,
> +};
> +
> +/**
> + * enum sdw_data_direction: Data direction
> + *
> + * @SDW_DATA_DIR_RX: Data into Port
> + * @SDW_DATA_DIR_TX: Data out of Port
> + */
> +enum sdw_data_direction {
> + SDW_DATA_DIR_RX = 0,
> + SDW_DATA_DIR_TX = 1,
> +};
> +
> /*
> * SDW properties, defined in MIPI DisCo spec v1.0
> */
> @@ -450,6 +474,9 @@ struct sdw_master_ops {
> * @msg_lock: message lock
> * @ops: Master callback ops
> * @prop: Master properties
> + * @m_rt_list: List of Master instance of all stream(s) running on Bus. This
> + * is used to compute and program bus bandwidth, clock, frame shape,
> + * transport and port parameters
> * @defer_msg: Defer message
> * @clk_stop_timeout: Clock stop timeout computed
> */
> @@ -462,6 +489,7 @@ struct sdw_bus {
> struct mutex msg_lock;
> const struct sdw_master_ops *ops;
> struct sdw_master_prop prop;
> + struct list_head m_rt_list;
> struct sdw_defer defer_msg;
> unsigned int clk_stop_timeout;
> };
> @@ -469,6 +497,87 @@ struct sdw_bus {
> int sdw_add_bus_master(struct sdw_bus *bus);
> void sdw_delete_bus_master(struct sdw_bus *bus);
>
> +/**
> + * sdw_stream_config: Master or Slave stream configuration
> + *
> + * @frame_rate: Audio frame rate of the stream, in Hz
> + * @ch_count: Channel count of the stream
> + * @bps: Number of bits per audio sample
> + * @direction: Data direction
> + * @type: Stream type PCM or PDM
> + */
> +struct sdw_stream_config {
> + unsigned int frame_rate;
> + unsigned int ch_count;
> + unsigned int bps;
> + enum sdw_data_direction direction;
> + enum sdw_stream_type type;
> +};
> +
> +/**
> + * sdw_stream_state: Stream states
> + *
> + * @SDW_STREAM_RELEASED: Stream released
> + * @SDW_STREAM_ALLOCATED: New stream allocated.
> + * @SDW_STREAM_CONFIGURED: Stream configured
> + * @SDW_STREAM_PREPARED: Stream prepared
> + * @SDW_STREAM_ENABLED: Stream enabled
> + * @SDW_STREAM_DISABLED: Stream disabled
> + * @SDW_STREAM_DEPREPARED: Stream de-prepared
> + */
> +enum sdw_stream_state {
> + SDW_STREAM_RELEASED = 0,
> + SDW_STREAM_ALLOCATED = 1,
> + SDW_STREAM_CONFIGURED = 2,
> + SDW_STREAM_PREPARED = 3,
> + SDW_STREAM_ENABLED = 4,
> + SDW_STREAM_DISABLED = 5,
> + SDW_STREAM_DEPREPARED = 6,
> +};
> +
> +/**
> + * sdw_stream_params: Stream parameters
> + *
> + * @rate: Sampling frequency, in Hz
> + * @ch_count: Number of channels
> + * @bps: bits per channel sample
> + */
> +struct sdw_stream_params {
> + unsigned int rate;
> + unsigned int ch_count;
> + unsigned int bps;
> +};
> +
> +/**
> + * sdw_stream_runtime: Runtime stream parameters
> + *
> + * @name: SoundWire stream name
> + * @params: Stream parameters
> + * @state: Current state of the stream
> + * @type: Stream type PCM or PDM
> + * @m_rt: Master runtime
> + */
> +struct sdw_stream_runtime {
> + char *name;
> + struct sdw_stream_params params;
> + enum sdw_stream_state state;
> + enum sdw_stream_type type;
> + struct sdw_master_runtime *m_rt;
> +};
> +
> +struct sdw_stream_runtime *sdw_alloc_stream(char *stream_name);
> +void sdw_release_stream(struct sdw_stream_runtime *stream);
> +int sdw_stream_add_master(struct sdw_bus *bus,
> + struct sdw_stream_config *stream_config,
> + struct sdw_stream_runtime *stream);
> +int sdw_stream_add_slave(struct sdw_slave *slave,
> + struct sdw_stream_config *stream_config,
> + struct sdw_stream_runtime *stream);
> +int sdw_stream_remove_master(struct sdw_bus *bus,
> + struct sdw_stream_runtime *stream);
> +int sdw_stream_remove_slave(struct sdw_slave *slave,
> + struct sdw_stream_runtime *stream);
> +
> /* messaging and data APIs */
>
> int sdw_read(struct sdw_slave *slave, u32 addr);
>
More information about the Alsa-devel
mailing list