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