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

[Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>]

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.