Re: [PATCH] soundwire: bus: add enumerated slave to device list

[Srinivas Kandagatla <srinivas.kandagatla@linaro.org>]

On 09/09/2020 14:37, Pierre-Louis Bossart wrote:
> 
> 
> On 9/9/20 3:27 AM, Srinivas Kandagatla wrote:
>> Currently slave devices are only added either from device tree or acpi
>> entries. However lets say, there is wrong or no entry of a slave device
>> in DT that is enumerated, then there is no way for user to know all
>> the enumerated devices on the bus.
> 
> Sorry Srinivas, I don't understand your point.
> 
> The sysfs entries will include all devices that are described in 
> platform firmware (be it DT or ACPI).

yes that is true, but it will not include all the enumerated devices on 
the bus!

In my case on a new board I was trying to figure out what devices are on 
the bus even before even adding any device tree entries!

In second case I had a typo in the device tree entry and sysfs displayed 
devices with that typo rather than actual enumerated device id.

> 
> If you add to sysfs entries unknown devices which happen to be present 
> on the bus, then what? How would you identify them from the devices that 
> are described in firmware?

Both of them should be displayed in sysfs, core should be able to 
differentiate this based on the presence of fw_node or of_node and not bind!

> 
> Also the sysfs entries describe properties, but if you haven't bound a 
> driver then how would this work?

This is would be informative, atleast in cases like me!

All I want to know is the list of enumerated devices on the bus, If 
doing this way is not the right thing, then am happy to try any suggestion!

For now I have managed to figure out enumerated device ids on the bus 
with this patch, I was hoping that other people would also hit such 
issue, so I sent this patch!


thanks,
srini
> 
> I really feel this deserves more explanations on the problem statement 
> and what you are hoping to achieve in this case.
> 
> Thanks!
> 
>>
>> To fix this add slave device by default if there is no matching dt or
>> acpi entry, so that we can see this in sysfs entry.
>>
>> In my case I had a wrong address entry in DT, However I had no way to
>> know what devices are actually enumerated on the bus!
>>
>> Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
>> ---
>>   drivers/soundwire/bus.c      | 1 +
>>   drivers/soundwire/bus.h      | 2 ++
>>   drivers/soundwire/bus_type.c | 6 ++++++
>>   drivers/soundwire/slave.c    | 4 ++--
>>   4 files changed, 11 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/soundwire/bus.c b/drivers/soundwire/bus.c
>> index e6e0fb9a81b4..55d9c22c4ec5 100644
>> --- a/drivers/soundwire/bus.c
>> +++ b/drivers/soundwire/bus.c
>> @@ -699,6 +699,7 @@ static int sdw_program_device_num(struct sdw_bus 
>> *bus)
>>           if (!found) {
>>               /* TODO: Park this device in Group 13 */
>> +            sdw_slave_add(bus, &id, NULL);
>>               dev_err(bus->dev, "Slave Entry not found\n");
>>           }
>> diff --git a/drivers/soundwire/bus.h b/drivers/soundwire/bus.h
>> index 82484f741168..1517d6789dff 100644
>> --- a/drivers/soundwire/bus.h
>> +++ b/drivers/soundwire/bus.h
>> @@ -19,6 +19,8 @@ static inline int sdw_acpi_find_slaves(struct 
>> sdw_bus *bus)
>>   int sdw_of_find_slaves(struct sdw_bus *bus);
>>   void sdw_extract_slave_id(struct sdw_bus *bus,
>>                 u64 addr, struct sdw_slave_id *id);
>> +int sdw_slave_add(struct sdw_bus *bus, struct sdw_slave_id *id,
>> +          struct fwnode_handle *fwnode);
>>   int sdw_master_device_add(struct sdw_bus *bus, struct device *parent,
>>                 struct fwnode_handle *fwnode);
>>   int sdw_master_device_del(struct sdw_bus *bus);
>> diff --git a/drivers/soundwire/bus_type.c b/drivers/soundwire/bus_type.c
>> index 6fba55898cf0..ac036223046f 100644
>> --- a/drivers/soundwire/bus_type.c
>> +++ b/drivers/soundwire/bus_type.c
>> @@ -84,6 +84,12 @@ static int sdw_drv_probe(struct device *dev)
>>       const struct sdw_device_id *id;
>>       int ret;
>> +    /*
>> +     * fw description is mandatory to bind
>> +     */
>> +    if (!dev->fwnode || !dev->of_node)
>> +        return -ENODEV;
>> +
>>       id = sdw_get_device_id(slave, drv);
>>       if (!id)
>>           return -ENODEV;
>> diff --git a/drivers/soundwire/slave.c b/drivers/soundwire/slave.c
>> index 0839445ee07b..24a16ebf9ae2 100644
>> --- a/drivers/soundwire/slave.c
>> +++ b/drivers/soundwire/slave.c
>> @@ -20,8 +20,8 @@ struct device_type sdw_slave_type = {
>>       .uevent =    sdw_slave_uevent,
>>   };
>> -static int sdw_slave_add(struct sdw_bus *bus,
>> -             struct sdw_slave_id *id, struct fwnode_handle *fwnode)
>> +int sdw_slave_add(struct sdw_bus *bus,
>> +          struct sdw_slave_id *id, struct fwnode_handle *fwnode)
>>   {
>>       struct sdw_slave *slave;
>>       int ret;
>>