| Message ID | 20190808144504.24823-3-srinivas.kandagatla@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | ASoC: codecs: Add WSA881x Smart Speaker amplifier support | expand |
> @@ -35,6 +36,7 @@ static int sdw_slave_add(struct sdw_bus *bus, > > slave->dev.release = sdw_slave_release; > slave->dev.bus = &sdw_bus_type; > + slave->dev.of_node = of_node_get(to_of_node(fwnode)); shouldn't this protected by #if IS_ENABLED(CONFIG_OF) ? > slave->bus = bus; > slave->status = SDW_SLAVE_UNATTACHED; > slave->dev_num = 0; > @@ -112,3 +114,48 @@ int sdw_acpi_find_slaves(struct sdw_bus *bus) > } > > #endif > + > +/* > + * sdw_of_find_slaves() - Find Slave devices in master device tree node > + * @bus: SDW bus instance > + * > + * Scans Master DT node for SDW child Slave devices and registers it. > + */ > +int sdw_of_find_slaves(struct sdw_bus *bus) > +{ > + struct device *dev = bus->dev; > + struct device_node *node; > + > + for_each_child_of_node(bus->dev->of_node, node) { > + struct sdw_slave_id id; > + const char *compat = NULL; > + int unique_id, ret; > + int ver, mfg_id, part_id, class_id; > + > + compat = of_get_property(node, "compatible", NULL); > + if (!compat) > + continue; > + > + ret = sscanf(compat, "sdw%x,%x,%x,%x", > + &ver, &mfg_id, &part_id, &class_id); > + if (ret != 4) { > + dev_err(dev, "Manf ID & Product code not found %s\n", > + compat); > + continue; > + } > + > + ret = of_property_read_u32(node, "sdw-instance-id", &unique_id); > + if (ret) { > + dev_err(dev, "Instance id not found:%d\n", ret); > + continue; I am confused here. If you have two identical devices on the same link, isn't this property required and that should be a real error instead of a continue? > + } > + > + id.sdw_version = ver - 0xF; maybe a comment in the code would help to make the encoding self-explanatory, as you did in the DT bindings Version number '0x10' represents SoundWire 1.0 Version number '0x11' represents SoundWire 1.1 > + id.unique_id = unique_id; > + id.mfg_id = mfg_id; > + id.part_id = part_id; > + id.class_id = class_id; > + sdw_slave_add(bus, &id, of_fwnode_handle(node)); > + } > + return 0; > +} >
Thanks for taking time to review. On 08/08/2019 16:00, Pierre-Louis Bossart wrote: > >> @@ -35,6 +36,7 @@ static int sdw_slave_add(struct sdw_bus *bus, >> slave->dev.release = sdw_slave_release; >> slave->dev.bus = &sdw_bus_type; >> + slave->dev.of_node = of_node_get(to_of_node(fwnode)); > > shouldn't this protected by > #if IS_ENABLED(CONFIG_OF) ? > These macros and functions have dummy entries, so it should not be an issue. I did build soundwire with i386_defconfig with no issues. >> slave->bus = bus; >> slave->status = SDW_SLAVE_UNATTACHED; >> slave->dev_num = 0; >> @@ -112,3 +114,48 @@ int sdw_acpi_find_slaves(struct sdw_bus *bus) >> } >> #endif >> + >> +/* >> + * sdw_of_find_slaves() - Find Slave devices in master device tree node >> + * @bus: SDW bus instance >> + * >> + * Scans Master DT node for SDW child Slave devices and registers it. >> + */ >> +int sdw_of_find_slaves(struct sdw_bus *bus) >> +{ >> + struct device *dev = bus->dev; >> + struct device_node *node; >> + >> + for_each_child_of_node(bus->dev->of_node, node) { >> + struct sdw_slave_id id; >> + const char *compat = NULL; >> + int unique_id, ret; >> + int ver, mfg_id, part_id, class_id; >> + >> + compat = of_get_property(node, "compatible", NULL); >> + if (!compat) >> + continue; >> + >> + ret = sscanf(compat, "sdw%x,%x,%x,%x", >> + &ver, &mfg_id, &part_id, &class_id); >> + if (ret != 4) { >> + dev_err(dev, "Manf ID & Product code not found %s\n", >> + compat); >> + continue; >> + } >> + >> + ret = of_property_read_u32(node, "sdw-instance-id", &unique_id); >> + if (ret) { >> + dev_err(dev, "Instance id not found:%d\n", ret); >> + continue; > > I am confused here. > If you have two identical devices on the same link, isn't this property > required and that should be a real error instead of a continue? Yes, I agree it will be mandatory in such cases. Am okay either way, I dont mind changing it to returning EINVAL in all the cases. > >> + } >> + >> + id.sdw_version = ver - 0xF; > > maybe a comment in the code would help to make the encoding > self-explanatory, as you did in the DT bindings > > Version number '0x10' represents SoundWire 1.0 > Version number '0x11' represents SoundWire 1.1 Makes sense, will fix this in next version. This info is also available in bindings. --srini > >> + id.unique_id = unique_id; >> + id.mfg_id = mfg_id; >> + id.part_id = part_id; >> + id.class_id = class_id; >> + sdw_slave_add(bus, &id, of_fwnode_handle(node)); >> + } >> + return 0; >> +} >>
On 08-08-19, 15:45, Srinivas Kandagatla wrote: > This patch adds support to parsing device tree based > SoundWire slave devices. > > Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org> > --- > drivers/soundwire/bus.c | 2 ++ > drivers/soundwire/bus.h | 1 + > drivers/soundwire/slave.c | 47 +++++++++++++++++++++++++++++++++++++++ > 3 files changed, 50 insertions(+) > > diff --git a/drivers/soundwire/bus.c b/drivers/soundwire/bus.c > index fe745830a261..324c54dc52fb 100644 > --- a/drivers/soundwire/bus.c > +++ b/drivers/soundwire/bus.c > @@ -77,6 +77,8 @@ int sdw_add_bus_master(struct sdw_bus *bus) > */ > if (IS_ENABLED(CONFIG_ACPI) && ACPI_HANDLE(bus->dev)) > ret = sdw_acpi_find_slaves(bus); > + else if (IS_ENABLED(CONFIG_OF) && bus->dev->of_node) > + ret = sdw_of_find_slaves(bus); > else > ret = -ENOTSUPP; /* No ACPI/DT so error out */ > > diff --git a/drivers/soundwire/bus.h b/drivers/soundwire/bus.h > index 3048ca153f22..ee46befedbd1 100644 > --- a/drivers/soundwire/bus.h > +++ b/drivers/soundwire/bus.h > @@ -15,6 +15,7 @@ static inline int sdw_acpi_find_slaves(struct sdw_bus *bus) > } > #endif > > +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); > > diff --git a/drivers/soundwire/slave.c b/drivers/soundwire/slave.c > index f39a5815e25d..8ab76f5d5a56 100644 > --- a/drivers/soundwire/slave.c > +++ b/drivers/soundwire/slave.c > @@ -2,6 +2,7 @@ > // Copyright(c) 2015-17 Intel Corporation. > > #include <linux/acpi.h> > +#include <linux/of.h> > #include <linux/soundwire/sdw.h> > #include <linux/soundwire/sdw_type.h> > #include "bus.h" > @@ -35,6 +36,7 @@ static int sdw_slave_add(struct sdw_bus *bus, > > slave->dev.release = sdw_slave_release; > slave->dev.bus = &sdw_bus_type; > + slave->dev.of_node = of_node_get(to_of_node(fwnode)); > slave->bus = bus; > slave->status = SDW_SLAVE_UNATTACHED; > slave->dev_num = 0; > @@ -112,3 +114,48 @@ int sdw_acpi_find_slaves(struct sdw_bus *bus) > } > > #endif > + > +/* > + * sdw_of_find_slaves() - Find Slave devices in master device tree node > + * @bus: SDW bus instance > + * > + * Scans Master DT node for SDW child Slave devices and registers it. > + */ > +int sdw_of_find_slaves(struct sdw_bus *bus) > +{ > + struct device *dev = bus->dev; > + struct device_node *node; > + > + for_each_child_of_node(bus->dev->of_node, node) { > + struct sdw_slave_id id; > + const char *compat = NULL; > + int unique_id, ret; > + int ver, mfg_id, part_id, class_id; > + > + compat = of_get_property(node, "compatible", NULL); > + if (!compat) > + continue; Why not use of_find_compatible_node() that will return the node which is sdw* and we dont need to checks on that.. > + > + ret = sscanf(compat, "sdw%x,%x,%x,%x", > + &ver, &mfg_id, &part_id, &class_id); > + if (ret != 4) { > + dev_err(dev, "Manf ID & Product code not found %s\n", > + compat); > + continue; > + } > + > + ret = of_property_read_u32(node, "sdw-instance-id", &unique_id); > + if (ret) { > + dev_err(dev, "Instance id not found:%d\n", ret); > + continue; > + } > + > + id.sdw_version = ver - 0xF; > + id.unique_id = unique_id; > + id.mfg_id = mfg_id; > + id.part_id = part_id; > + id.class_id = class_id; empty line here please > + sdw_slave_add(bus, &id, of_fwnode_handle(node)); > + } and here as well > + return 0; > +} > -- > 2.21.0
On 08-08-19, 16:17, Srinivas Kandagatla wrote: > Thanks for taking time to review. > > On 08/08/2019 16:00, Pierre-Louis Bossart wrote: > > > > > @@ -35,6 +36,7 @@ static int sdw_slave_add(struct sdw_bus *bus, > > > slave->dev.release = sdw_slave_release; > > > slave->dev.bus = &sdw_bus_type; > > > + slave->dev.of_node = of_node_get(to_of_node(fwnode)); > > > > shouldn't this protected by > > #if IS_ENABLED(CONFIG_OF) ? > > > These macros and functions have dummy entries, so it should not be an issue. > I did build soundwire with i386_defconfig with no issues. That means this function was compiled without errors, that is not strange nowadays given the ARM compiles ACPI and x86 OF, so check with OF being disable just to be safe :) I think dummy entries are helping > > > > slave->bus = bus; > > > slave->status = SDW_SLAVE_UNATTACHED; > > > slave->dev_num = 0; > > > @@ -112,3 +114,48 @@ int sdw_acpi_find_slaves(struct sdw_bus *bus) > > > } > > > #endif > > > + > > > +/* > > > + * sdw_of_find_slaves() - Find Slave devices in master device tree node > > > + * @bus: SDW bus instance > > > + * > > > + * Scans Master DT node for SDW child Slave devices and registers it. > > > + */ > > > +int sdw_of_find_slaves(struct sdw_bus *bus) > > > +{ > > > + struct device *dev = bus->dev; > > > + struct device_node *node; > > > + > > > + for_each_child_of_node(bus->dev->of_node, node) { > > > + struct sdw_slave_id id; > > > + const char *compat = NULL; > > > + int unique_id, ret; > > > + int ver, mfg_id, part_id, class_id; > > > + > > > + compat = of_get_property(node, "compatible", NULL); > > > + if (!compat) > > > + continue; > > > + > > > + ret = sscanf(compat, "sdw%x,%x,%x,%x", > > > + &ver, &mfg_id, &part_id, &class_id); > > > + if (ret != 4) { > > > + dev_err(dev, "Manf ID & Product code not found %s\n", > > > + compat); > > > + continue; > > > + } > > > + > > > + ret = of_property_read_u32(node, "sdw-instance-id", &unique_id); > > > + if (ret) { > > > + dev_err(dev, "Instance id not found:%d\n", ret); > > > + continue; > > > > I am confused here. > > If you have two identical devices on the same link, isn't this property > > required and that should be a real error instead of a continue? > > Yes, I agree it will be mandatory in such cases. > > Am okay either way, I dont mind changing it to returning EINVAL in all the > cases. Do we want to abort? We are in loop scanning for devices so makes sense if we do not do that and continue to check next one..
On 09/08/2019 06:46, Vinod Koul wrote: >>>> +int sdw_of_find_slaves(struct sdw_bus *bus) >>>> +{ >>>> + struct device *dev = bus->dev; >>>> + struct device_node *node; >>>> + >>>> + for_each_child_of_node(bus->dev->of_node, node) { >>>> + struct sdw_slave_id id; >>>> + const char *compat = NULL; >>>> + int unique_id, ret; >>>> + int ver, mfg_id, part_id, class_id; >>>> + >>>> + compat = of_get_property(node, "compatible", NULL); >>>> + if (!compat) >>>> + continue; >>>> + >>>> + ret = sscanf(compat, "sdw%x,%x,%x,%x", >>>> + &ver, &mfg_id, &part_id, &class_id); >>>> + if (ret != 4) { >>>> + dev_err(dev, "Manf ID & Product code not found %s\n", >>>> + compat); >>>> + continue; >>>> + } >>>> + >>>> + ret = of_property_read_u32(node, "sdw-instance-id", &unique_id); >>>> + if (ret) { >>>> + dev_err(dev, "Instance id not found:%d\n", ret); >>>> + continue; >>> I am confused here. >>> If you have two identical devices on the same link, isn't this property >>> required and that should be a real error instead of a continue? >> Yes, I agree it will be mandatory in such cases. >> >> Am okay either way, I dont mind changing it to returning EINVAL in all the >> cases. > Do we want to abort? We are in loop scanning for devices so makes sense > if we do not do that and continue to check next one.. That was my inital plan. Pierre suggested a better compatible to include instance ID and LinkID so this check would be part of the check one before this line. --srini
On 09/08/2019 06:07, Vinod Koul wrote: > On 08-08-19, 15:45, Srinivas Kandagatla wrote: >> This patch adds support to parsing device tree based >> SoundWire slave devices. >> >> Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org> >> --- >> drivers/soundwire/bus.c | 2 ++ >> drivers/soundwire/bus.h | 1 + >> drivers/soundwire/slave.c | 47 +++++++++++++++++++++++++++++++++++++++ >> 3 files changed, 50 insertions(+) >> >> diff --git a/drivers/soundwire/bus.c b/drivers/soundwire/bus.c >> index fe745830a261..324c54dc52fb 100644 >> --- a/drivers/soundwire/bus.c >> +++ b/drivers/soundwire/bus.c >> @@ -77,6 +77,8 @@ int sdw_add_bus_master(struct sdw_bus *bus) >> */ >> if (IS_ENABLED(CONFIG_ACPI) && ACPI_HANDLE(bus->dev)) >> ret = sdw_acpi_find_slaves(bus); >> + else if (IS_ENABLED(CONFIG_OF) && bus->dev->of_node) >> + ret = sdw_of_find_slaves(bus); >> else >> ret = -ENOTSUPP; /* No ACPI/DT so error out */ >> >> diff --git a/drivers/soundwire/bus.h b/drivers/soundwire/bus.h >> index 3048ca153f22..ee46befedbd1 100644 >> --- a/drivers/soundwire/bus.h >> +++ b/drivers/soundwire/bus.h >> @@ -15,6 +15,7 @@ static inline int sdw_acpi_find_slaves(struct sdw_bus *bus) >> } >> #endif >> >> +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); >> >> diff --git a/drivers/soundwire/slave.c b/drivers/soundwire/slave.c >> index f39a5815e25d..8ab76f5d5a56 100644 >> --- a/drivers/soundwire/slave.c >> +++ b/drivers/soundwire/slave.c >> @@ -2,6 +2,7 @@ >> // Copyright(c) 2015-17 Intel Corporation. >> >> #include <linux/acpi.h> >> +#include <linux/of.h> >> #include <linux/soundwire/sdw.h> >> #include <linux/soundwire/sdw_type.h> >> #include "bus.h" >> @@ -35,6 +36,7 @@ static int sdw_slave_add(struct sdw_bus *bus, >> >> slave->dev.release = sdw_slave_release; >> slave->dev.bus = &sdw_bus_type; >> + slave->dev.of_node = of_node_get(to_of_node(fwnode)); >> slave->bus = bus; >> slave->status = SDW_SLAVE_UNATTACHED; >> slave->dev_num = 0; >> @@ -112,3 +114,48 @@ int sdw_acpi_find_slaves(struct sdw_bus *bus) >> } >> >> #endif >> + >> +/* >> + * sdw_of_find_slaves() - Find Slave devices in master device tree node >> + * @bus: SDW bus instance >> + * >> + * Scans Master DT node for SDW child Slave devices and registers it. >> + */ >> +int sdw_of_find_slaves(struct sdw_bus *bus) >> +{ >> + struct device *dev = bus->dev; >> + struct device_node *node; >> + >> + for_each_child_of_node(bus->dev->of_node, node) { >> + struct sdw_slave_id id; >> + const char *compat = NULL; >> + int unique_id, ret; >> + int ver, mfg_id, part_id, class_id; >> + >> + compat = of_get_property(node, "compatible", NULL); >> + if (!compat) >> + continue; > > Why not use of_find_compatible_node() that will return the node which is > sdw* and we dont need to checks on that.. Am not sure if wild characters are really supported in this api. Also AFIU, it would not very optimal to use this api and it would complicate the code without much gain. > >> + >> + ret = sscanf(compat, "sdw%x,%x,%x,%x", >> + &ver, &mfg_id, &part_id, &class_id); >> + if (ret != 4) { >> + dev_err(dev, "Manf ID & Product code not found %s\n", >> + compat); >> + continue; >> + } >> + >> + ret = of_property_read_u32(node, "sdw-instance-id", &unique_id); >> + if (ret) { >> + dev_err(dev, "Instance id not found:%d\n", ret); >> + continue; >> + } >> + >> + id.sdw_version = ver - 0xF; >> + id.unique_id = unique_id; >> + id.mfg_id = mfg_id; >> + id.part_id = part_id; >> + id.class_id = class_id; > > empty line here please yep, will add that. thanks, srini > >> + sdw_slave_add(bus, &id, of_fwnode_handle(node)); >> + } > > and here as well > >> + return 0; >> +} >> -- >> 2.21.0 >
diff --git a/drivers/soundwire/bus.c b/drivers/soundwire/bus.c index fe745830a261..324c54dc52fb 100644 --- a/drivers/soundwire/bus.c +++ b/drivers/soundwire/bus.c @@ -77,6 +77,8 @@ int sdw_add_bus_master(struct sdw_bus *bus) */ if (IS_ENABLED(CONFIG_ACPI) && ACPI_HANDLE(bus->dev)) ret = sdw_acpi_find_slaves(bus); + else if (IS_ENABLED(CONFIG_OF) && bus->dev->of_node) + ret = sdw_of_find_slaves(bus); else ret = -ENOTSUPP; /* No ACPI/DT so error out */ diff --git a/drivers/soundwire/bus.h b/drivers/soundwire/bus.h index 3048ca153f22..ee46befedbd1 100644 --- a/drivers/soundwire/bus.h +++ b/drivers/soundwire/bus.h @@ -15,6 +15,7 @@ static inline int sdw_acpi_find_slaves(struct sdw_bus *bus) } #endif +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); diff --git a/drivers/soundwire/slave.c b/drivers/soundwire/slave.c index f39a5815e25d..8ab76f5d5a56 100644 --- a/drivers/soundwire/slave.c +++ b/drivers/soundwire/slave.c @@ -2,6 +2,7 @@ // Copyright(c) 2015-17 Intel Corporation. #include <linux/acpi.h> +#include <linux/of.h> #include <linux/soundwire/sdw.h> #include <linux/soundwire/sdw_type.h> #include "bus.h" @@ -35,6 +36,7 @@ static int sdw_slave_add(struct sdw_bus *bus, slave->dev.release = sdw_slave_release; slave->dev.bus = &sdw_bus_type; + slave->dev.of_node = of_node_get(to_of_node(fwnode)); slave->bus = bus; slave->status = SDW_SLAVE_UNATTACHED; slave->dev_num = 0; @@ -112,3 +114,48 @@ int sdw_acpi_find_slaves(struct sdw_bus *bus) } #endif + +/* + * sdw_of_find_slaves() - Find Slave devices in master device tree node + * @bus: SDW bus instance + * + * Scans Master DT node for SDW child Slave devices and registers it. + */ +int sdw_of_find_slaves(struct sdw_bus *bus) +{ + struct device *dev = bus->dev; + struct device_node *node; + + for_each_child_of_node(bus->dev->of_node, node) { + struct sdw_slave_id id; + const char *compat = NULL; + int unique_id, ret; + int ver, mfg_id, part_id, class_id; + + compat = of_get_property(node, "compatible", NULL); + if (!compat) + continue; + + ret = sscanf(compat, "sdw%x,%x,%x,%x", + &ver, &mfg_id, &part_id, &class_id); + if (ret != 4) { + dev_err(dev, "Manf ID & Product code not found %s\n", + compat); + continue; + } + + ret = of_property_read_u32(node, "sdw-instance-id", &unique_id); + if (ret) { + dev_err(dev, "Instance id not found:%d\n", ret); + continue; + } + + id.sdw_version = ver - 0xF; + id.unique_id = unique_id; + id.mfg_id = mfg_id; + id.part_id = part_id; + id.class_id = class_id; + sdw_slave_add(bus, &id, of_fwnode_handle(node)); + } + return 0; +}
This patch adds support to parsing device tree based SoundWire slave devices. Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org> --- drivers/soundwire/bus.c | 2 ++ drivers/soundwire/bus.h | 1 + drivers/soundwire/slave.c | 47 +++++++++++++++++++++++++++++++++++++++ 3 files changed, 50 insertions(+)