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

[Srinivas Kandagatla]

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@xxxxxxxxxx>
> > ---
> >   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;