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

[Vinod Koul <vinod.koul@intel.com>]

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

-- 
~Vinod