From mboxrd@z Thu Jan 1 00:00:00 1970 From: Pierre-Louis Bossart Subject: Re: [PATCH v2 03/13] soundwire: Add support for port management Date: Fri, 6 Apr 2018 10:26:01 -0500 Message-ID: <8a717662-57fa-b9d9-8cd7-3fb03d693297@linux.intel.com> References: <1522946904-2089-1-git-send-email-vinod.koul@intel.com> <1522946904-2089-4-git-send-email-vinod.koul@intel.com> <20180406050029.GF6014@localhost> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" Content-Transfer-Encoding: 7bit Return-path: Received: from mga03.intel.com (mga03.intel.com [134.134.136.65]) by alsa0.perex.cz (Postfix) with ESMTP id 875A7266BBC for ; Fri, 6 Apr 2018 17:26:04 +0200 (CEST) In-Reply-To: <20180406050029.GF6014@localhost> Content-Language: en-US List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: alsa-devel-bounces@alsa-project.org Sender: alsa-devel-bounces@alsa-project.org To: Vinod Koul Cc: ALSA , tiwai@suse.de, Greg KH , liam.r.girdwood@linux.intel.com, patches.audio@intel.com, broonie@kernel.org, Sanyog Kale List-Id: alsa-devel@alsa-project.org 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.