| Message ID | 1522946904-2089-4-git-send-email-vinod.koul@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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); >
On Thu, Apr 05, 2018 at 06:04:32PM -0500, Pierre-Louis Bossart wrote: > On 4/5/18 11:48 AM, Vinod Koul wrote: > >@@ -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. Not in scope, will add in that series > >+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? it is NOT about stream state, but deconfigure the ports. Well we can make it sdw_master_port_release() if that makes you happy! > > >+ > >+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. Nope it is not, we tried in past and end result looked uglier. Only 4 lines of code are same and previous one iterates over master and here we have slave... > > 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... ok > >+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 ok will remove check for master > >+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. well this is better that no check but yes that can be further improved in future to comprehend DisCo properties and port direction. I will add that to my list
On 4/6/18 12:00 AM, Vinod Koul wrote: > On Thu, Apr 05, 2018 at 06:04:32PM -0500, Pierre-Louis Bossart wrote: >> On 4/5/18 11:48 AM, Vinod Koul wrote: > >>> @@ -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. > > Not in scope, will add in that series > >>> +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? > > it is NOT about stream state, but deconfigure the ports. Well we can make it > sdw_master_port_release() if that makes you happy! yes please. There is no operation called 'deconfigure' > >> >>> + >>> +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. > > Nope it is not, we tried in past and end result looked uglier. > Only 4 lines of code are same and previous one iterates > over master and here we have slave... ok > >>> 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... > > ok > >>> +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 > > ok will remove check for master > >>> +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. > > well this is better that no check but yes that can be further improved in > future to comprehend DisCo properties and port direction. I will add that to > my list ok, I am fine with a TODO.
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 * @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); + } +} + +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); + } + } +} + /** * 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); 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); + 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); + 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);