Re: [alsa-devel] [PATCH v2 03/13] soundwire: Add support for port management

On 4/5/18 11:48 AM, Vinod Koul wrote:

From: Sanyog Kale sanyog.r.kale@intel.com

Add Soundwire port data structures and APIS for initialization and release of ports.

Signed-off-by: Sanyog Kale sanyog.r.kale@intel.com Signed-off-by: Shreyas NC shreyas.nc@intel.com Signed-off-by: Vinod Koul vinod.koul@intel.com

---

drivers/soundwire/bus.h | 25 +++++++ drivers/soundwire/stream.c | 151 +++++++++++++++++++++++++++++++++++++++++- include/linux/soundwire/sdw.h | 67 +++++++++++++++++++ 3 files changed, 241 insertions(+), 2 deletions(-)

diff --git a/drivers/soundwire/bus.h b/drivers/soundwire/bus.h index 3c66b6aecc14..2e834a8038ed 100644 --- a/drivers/soundwire/bus.h +++ b/drivers/soundwire/bus.h @@ -46,6 +46,27 @@ struct sdw_msg { };

/**

• • sdw_port_runtime: Runtime port parameters for Master or Slave
• • @num: Port number. For audio streams, valid port number ranges from
• • [1,14]
• • @ch_mask: Channel mask
• • @transport_params: Transport parameters
• • @port_params: Port parameters
• • @port_node: List node for Master or Slave port_list
• • SoundWire spec has no mention of ports for Master interface but the
• • concept is logically extended.
• */

+struct sdw_port_runtime {

• int num;
• int ch_mask;
• struct sdw_transport_params transport_params;
• struct sdw_port_params port_params;
• struct list_head port_node;

+};

+/**

• sdw_slave_runtime: Runtime Stream parameters for Slave
• @slave: Slave handle

@@ -53,12 +74,14 @@ struct sdw_msg {

• @ch_count: Number of channels handled by the Slave for
• this stream
• @m_rt_node: sdw_master_runtime list node

• • @port_list: List of Slave Ports configured for this stream

  */ struct sdw_slave_runtime { struct sdw_slave *slave; enum sdw_data_direction direction; unsigned int ch_count; struct list_head m_rt_node;

• struct list_head port_list; };

  /**

@@ -70,6 +93,7 @@ struct sdw_slave_runtime {

• @ch_count: Number of channels handled by the Master for
• this stream
• @slave_rt_list: Slave runtime list

• • @port_list: List of Master Ports configured for this stream

possibly empty for device to device communication.

• @bus_node: sdw_bus m_rt_list node

*/ struct sdw_master_runtime { @@ -78,6 +102,7 @@ struct sdw_master_runtime { enum sdw_data_direction direction; unsigned int ch_count; struct list_head slave_rt_list;

• struct list_head port_list; struct list_head bus_node; };

diff --git a/drivers/soundwire/stream.c b/drivers/soundwire/stream.c index 5c34177e4954..4b10f07b3e9e 100644 --- a/drivers/soundwire/stream.c +++ b/drivers/soundwire/stream.c @@ -75,6 +75,7 @@ static struct sdw_master_runtime return NULL;

/* Initialization of Master runtime handle */

• INIT_LIST_HEAD(&m_rt->port_list); INIT_LIST_HEAD(&m_rt->slave_rt_list); stream->m_rt = m_rt;

@@ -109,6 +110,7 @@ static struct sdw_slave_runtime if (!s_rt) return NULL;

• INIT_LIST_HEAD(&s_rt->port_list);

  s_rt->ch_count = stream_config->ch_count; s_rt->direction = stream_config->direction;

@@ -117,6 +119,41 @@ static struct sdw_slave_runtime return s_rt; }

+static void sdw_master_port_deconfig(struct sdw_bus *bus,

• 	struct sdw_master_runtime *m_rt)
  

+{

• struct sdw_port_runtime *p_rt, *_p_rt;
• list_for_each_entry_safe(p_rt, _p_rt,

• 	&m_rt->port_list, port_node) {
  

• list_del(&p_rt->port_node);
  

• kfree(p_rt);
  

• }

+}

I still don't get the naming conventions. There is no DECONFIGURED state, why not call it release? In which state is this called?

+static void sdw_slave_port_deconfig(struct sdw_bus *bus,

• struct sdw_slave *slave,
  

• struct sdw_stream_runtime *stream)
  

+{

• struct sdw_port_runtime *p_rt, *_p_rt;
• struct sdw_master_runtime *m_rt = stream->m_rt;
• struct sdw_slave_runtime *s_rt;
• list_for_each_entry(s_rt, &m_rt->slave_rt_list, m_rt_node) {

• if (s_rt->slave != slave)
  

• 	continue;
  

• list_for_each_entry_safe(p_rt, _p_rt,
  

• 		&s_rt->port_list, port_node) {
  

• 	list_del(&p_rt->port_node);
  

• 	kfree(p_rt);
  

• }
  

can we use common code between master and slaves? This looks virtually identical.

• }

+}

• /**
  • sdw_release_slave_stream: Free Slave(s) runtime handle

@@ -161,7 +198,7 @@ static void sdw_release_master_stream(struct sdw_stream_runtime *stream)

• @bus: SDW Bus instance
• @stream: Soundwire stream

• • This removes and frees master_rt from a stream

• • This removes and frees port_rt and master_rt from a stream
  */ int sdw_stream_remove_master(struct sdw_bus *bus, struct sdw_stream_runtime *stream)

@@ -169,6 +206,7 @@ int sdw_stream_remove_master(struct sdw_bus *bus, mutex_lock(&bus->bus_lock);

sdw_release_master_stream(stream);

• sdw_master_port_deconfig(bus, stream->m_rt); stream->state = SDW_STREAM_RELEASED; kfree(stream->m_rt); stream->m_rt = NULL;

@@ -185,13 +223,14 @@ EXPORT_SYMBOL(sdw_stream_remove_master);

• @slave: SDW Slave instance
• @stream: Soundwire stream

• • This removes and frees slave_rt from a stream

• • This removes and frees port_rt and 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_slave_port_deconfig(slave->bus, slave, stream);

then call it sdw_slave_port_release as I mentioned above...

sdw_release_slave_stream(slave, stream);

mutex_unlock(&slave->bus->bus_lock); @@ -241,15 +280,111 @@ static int sdw_config_stream(struct device *dev, return 0; }

+static int sdw_is_valid_port_range(struct device *dev,

• 		struct sdw_port_runtime *p_rt)
  

+{

• if (!SDW_VALID_PORT_RANGE(p_rt->num)) {

• dev_err(dev,
  

• 	"SoundWire: Invalid port number :%d", p_rt->num);
  

• return -EINVAL;
  

• }
• return 0;

+}

+static struct sdw_port_runtime *sdw_port_alloc(struct device *dev,

• 		struct sdw_port_config *port_config,
  

• 		int port_index)
  

+{

• struct sdw_port_runtime *p_rt;
• p_rt = kzalloc(sizeof(*p_rt), GFP_KERNEL);
• if (!p_rt)

• return NULL;
  

• p_rt->ch_mask = port_config[port_index].ch_mask;
• p_rt->num = port_config[port_index].num;
• return p_rt;

+}

+static int sdw_master_port_config(struct sdw_bus *bus,

• 	struct sdw_master_runtime *m_rt,
  

• 	struct sdw_port_config *port_config,
  

• 	unsigned int num_ports)
  

+{

• struct sdw_port_runtime *p_rt;
• int i, ret;
• /* Iterate for number of ports to perform initialization */
• for (i = 0; i < num_ports; i++) {

• p_rt = sdw_port_alloc(bus->dev, port_config, i);
  

• if (!p_rt)
  

• 	return -ENOMEM; > +
  

• ret = sdw_is_valid_port_range(bus->dev, p_rt);
  

a master has no definition of ports. You could have more than 14 ports. Even if you have a description of those ports, it has to be checking not for the standard definition but what the hardware can support

• if (ret < 0) {
  

• 	kfree(p_rt);
  

• 	return ret;
  

• }
  

• /*
  

•  * TODO: Check port capabilities for requested
  

•  * configuration (audio mode support)
  

•  */
  

• list_add_tail(&p_rt->port_node, &m_rt->port_list);
  

• }
• return 0;

+}

+static int sdw_slave_port_config(struct sdw_slave *slave,

• 	struct sdw_slave_runtime *s_rt,
  

• 	struct sdw_port_config *port_config,
  

• 	unsigned int num_config)
  

+{

• struct sdw_port_runtime *p_rt;
• int i, ret;
• /* Iterate for number of ports to perform initialization */
• for (i = 0; i < num_config; i++) {

• p_rt = sdw_port_alloc(&slave->dev, port_config, i);
  

• if (!p_rt)
  

• 	return -ENOMEM;
  

• ret = sdw_is_valid_port_range(&slave->dev, p_rt);
  

this is optimistic. You should check the actual port range (as defined in DisCo properties or driver), not just the worst case allowed by the standard. This should include a check that the bi-dir ports are configured for the right role and that the direction is compatible for regular fixed-direction ports.

• if (ret < 0) {
  

• 	kfree(p_rt);
  

• 	return ret;
  

• }
  

• /*
  

•  * TODO: Check port capabilities for requested
  

•  * configuration (audio mode support)
  

•  */
  

• list_add_tail(&p_rt->port_node, &s_rt->port_list);
  

• }
• 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
• • @port_config: Port configuration for audio stream
• • @num_ports: Number of ports
  • @stream: Soundwire stream
  */ int sdw_stream_add_master(struct sdw_bus *bus, struct sdw_stream_config *stream_config,

• struct sdw_port_config *port_config,
  

• unsigned int num_ports,
  

  struct sdw_stream_runtime *stream) { struct sdw_master_runtime *m_rt = NULL;

@@ -277,6 +412,10 @@ int sdw_stream_add_master(struct sdw_bus *bus, if (ret) goto stream_error;

• ret = sdw_master_port_config(bus, m_rt, port_config, num_ports);
• if (ret)

• goto stream_error;
  

• if (!list_empty(&m_rt->slave_rt_list) && stream->state == SDW_STREAM_ALLOCATED) stream->state = SDW_STREAM_CONFIGURED;

@@ -295,10 +434,14 @@ EXPORT_SYMBOL(sdw_stream_add_master);

• @slave: SDW Slave instance
• @stream_config: Stream configuration for audio stream

• • @port_config: Port configuration for audio stream
• • @num_ports: Number of ports
  • @stream: Soundwire stream
  */ int sdw_stream_add_slave(struct sdw_slave *slave, struct sdw_stream_config *stream_config,

• struct sdw_port_config *port_config,
  

• unsigned int num_ports,
  

  struct sdw_stream_runtime *stream) { struct sdw_slave_runtime *s_rt;

@@ -342,6 +485,10 @@ int sdw_stream_add_slave(struct sdw_slave *slave,

list_add_tail(&s_rt->m_rt_node, &m_rt->slave_rt_list);

• ret = sdw_slave_port_config(slave, s_rt, port_config, num_ports);
• if (ret)

• goto stream_error;
  

• stream->state = SDW_STREAM_CONFIGURED; goto error;

diff --git a/include/linux/soundwire/sdw.h b/include/linux/soundwire/sdw.h index 893e1b6b4914..228fdbf506fe 100644 --- a/include/linux/soundwire/sdw.h +++ b/include/linux/soundwire/sdw.h @@ -26,6 +26,8 @@ struct sdw_slave;

#define SDW_MAX_DEVICES 11

+#define SDW_VALID_PORT_RANGE(n) (n <= 14 && n >= 1)

• /**
  • enum sdw_slave_status - Slave status
  • @SDW_SLAVE_UNATTACHED: Slave is not attached with the bus.

@@ -430,6 +432,56 @@ int sdw_handle_slave_status(struct sdw_bus *bus,

• SDW master structures and APIs

*/

+/**

• • struct sdw_port_params: Data Port parameters
• • @num: Port number
• • @bps: Word length of the Port
• • @flow_mode: Port Data flow mode
• • @data_mode: Test modes or normal mode
• • This is used to program the Data Port based on Data Port stream
• • parameters.
• */

+struct sdw_port_params {

• unsigned int num;
• unsigned int bps;
• unsigned int flow_mode;
• unsigned int data_mode;

+};

+/**

• • struct sdw_transport_params: Data Port Transport Parameters
• • @blk_grp_ctrl_valid: Port implements block group control
• • @num: Port number
• • @blk_grp_ctrl: Block group control value
• • @sample_interval: Sample interval
• • @offset1: Blockoffset of the payload data
• • @offset2: Blockoffset of the payload data
• • @hstart: Horizontal start of the payload data
• • @hstop: Horizontal stop of the payload data
• • @blk_pkg_mode: Block per channel or block per port
• • @lane_ctrl: Data lane Port uses for Data transfer. Currently only single
• • data lane is supported in bus
• • This is used to program the Data Port based on Data Port transport
• • parameters. All these parameters are banked and can be modified
• • during a bank switch without any artifacts in audio stream.
• */

+struct sdw_transport_params {

• bool blk_grp_ctrl_valid;
• unsigned int port_num;
• unsigned int blk_grp_ctrl;
• unsigned int sample_interval;
• unsigned int offset1;
• unsigned int offset2;
• unsigned int hstart;
• unsigned int hstop;
• unsigned int blk_pkg_mode;
• unsigned int lane_ctrl;

+};

• struct sdw_msg;

  /**

@@ -498,6 +550,17 @@ int sdw_add_bus_master(struct sdw_bus *bus); void sdw_delete_bus_master(struct sdw_bus *bus);

/**

• • sdw_port_config: Master or Slave Port configuration
• • @num: Port number
• • @ch_mask: channels mask for port
• */

+struct sdw_port_config {

• unsigned int num;
• unsigned int ch_mask;

+};

+/**

• sdw_stream_config: Master or Slave stream configuration
• @frame_rate: Audio frame rate of the stream, in Hz

@@ -569,9 +632,13 @@ 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_port_config *port_config,
  

• unsigned int num_ports,
  

  struct sdw_stream_runtime *stream); int sdw_stream_add_slave(struct sdw_slave *slave, struct sdw_stream_config *stream_config,

• struct sdw_port_config *port_config,
  

• unsigned int num_ports,
  

  struct sdw_stream_runtime *stream); int sdw_stream_remove_master(struct sdw_bus *bus, struct sdw_stream_runtime *stream);