| Message ID | 20191217210314.20410-7-pierre-louis.bossart@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | soundwire: intel: implement new ASoC interfaces | expand |
On 17-12-19, 15:03, Pierre-Louis Bossart wrote: > In the existing SoundWire code, the bus does not have any explicit > representation for Master Devices - only SoundWire Slaves are exposed. > > In SoundWire, the Master Device provides the clock, synchronization > information and command/control channels. When multiple links are > supported, a Controller may expose more than one Master Device; they > are typically embedded inside a larger audio cluster (be it in an > SOC/chipset or an external audio codec), and we need to describe it > using the Linux device and driver model. This will allow for > configuration functions to account for external dependencies such as > power rails, clock sources or wake-up mechanisms. This transition will > also allow for better sysfs support without the reference count issues > mentioned in the initial reviews. > > In this patch, we first convert the existing code to use an explicit > sdw_slave_type and add error checks if this type is not set. > > In follow-up patches we can add support for the sdw_master_type. > > Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com> > --- > drivers/soundwire/bus_type.c | 23 ++++++++++++++++++----- > drivers/soundwire/slave.c | 7 ++++++- > include/linux/soundwire/sdw_type.h | 6 ++++++ > 3 files changed, 30 insertions(+), 6 deletions(-) > > diff --git a/drivers/soundwire/bus_type.c b/drivers/soundwire/bus_type.c > index 9a0fd3ee1014..9719680a1e48 100644 > --- a/drivers/soundwire/bus_type.c > +++ b/drivers/soundwire/bus_type.c > @@ -49,13 +49,26 @@ int sdw_slave_modalias(const struct sdw_slave *slave, char *buf, size_t size) > > static int sdw_uevent(struct device *dev, struct kobj_uevent_env *env) > { > - struct sdw_slave *slave = to_sdw_slave_device(dev); > + struct sdw_slave *slave; > char modalias[32]; > > - sdw_slave_modalias(slave, modalias, sizeof(modalias)); > - > - if (add_uevent_var(env, "MODALIAS=%s", modalias)) > - return -ENOMEM; > + if (is_sdw_slave(dev)) { > + slave = to_sdw_slave_device(dev); > + > + sdw_slave_modalias(slave, modalias, sizeof(modalias)); > + > + if (add_uevent_var(env, "MODALIAS=%s", modalias)) > + return -ENOMEM; > + } else { > + /* > + * We only need to handle uevents for the Slave device > + * type. This error cannot happen unless the .uevent > + * callback is set to use this function for a > + * different device type (e.g. Master or Monitor) > + */ > + dev_err(dev, "uevent for unknown Soundwire type\n"); > + return -EINVAL; At this point and after next patch, the above code would be a no-op, do we want this here, if so why?
>> static int sdw_uevent(struct device *dev, struct kobj_uevent_env *env) >> { >> - struct sdw_slave *slave = to_sdw_slave_device(dev); >> + struct sdw_slave *slave; >> char modalias[32]; >> >> - sdw_slave_modalias(slave, modalias, sizeof(modalias)); >> - >> - if (add_uevent_var(env, "MODALIAS=%s", modalias)) >> - return -ENOMEM; >> + if (is_sdw_slave(dev)) { >> + slave = to_sdw_slave_device(dev); >> + >> + sdw_slave_modalias(slave, modalias, sizeof(modalias)); >> + >> + if (add_uevent_var(env, "MODALIAS=%s", modalias)) >> + return -ENOMEM; >> + } else { >> + /* >> + * We only need to handle uevents for the Slave device >> + * type. This error cannot happen unless the .uevent >> + * callback is set to use this function for a >> + * different device type (e.g. Master or Monitor) >> + */ >> + dev_err(dev, "uevent for unknown Soundwire type\n"); >> + return -EINVAL; > > At this point and after next patch, the above code would be a no-op, do > we want this here, if so why? to be future proof if someone wants to add support for a monitor, as explained above. I can remove this if you don't want it.
On 27-12-19, 17:26, Pierre-Louis Bossart wrote: > > > > static int sdw_uevent(struct device *dev, struct kobj_uevent_env *env) > > > { > > > - struct sdw_slave *slave = to_sdw_slave_device(dev); > > > + struct sdw_slave *slave; > > > char modalias[32]; > > > - sdw_slave_modalias(slave, modalias, sizeof(modalias)); > > > - > > > - if (add_uevent_var(env, "MODALIAS=%s", modalias)) > > > - return -ENOMEM; > > > + if (is_sdw_slave(dev)) { > > > + slave = to_sdw_slave_device(dev); > > > + > > > + sdw_slave_modalias(slave, modalias, sizeof(modalias)); > > > + > > > + if (add_uevent_var(env, "MODALIAS=%s", modalias)) > > > + return -ENOMEM; > > > + } else { > > > + /* > > > + * We only need to handle uevents for the Slave device > > > + * type. This error cannot happen unless the .uevent > > > + * callback is set to use this function for a > > > + * different device type (e.g. Master or Monitor) > > > + */ > > > + dev_err(dev, "uevent for unknown Soundwire type\n"); > > > + return -EINVAL; > > > > At this point and after next patch, the above code would be a no-op, do > > we want this here, if so why? > > to be future proof if someone wants to add support for a monitor, as > explained above. > I can remove this if you don't want it. It can be added with monitor support whenever that comes. We dont like dead code in kernel, the piece can come when future arrives :)
diff --git a/drivers/soundwire/bus_type.c b/drivers/soundwire/bus_type.c index 9a0fd3ee1014..9719680a1e48 100644 --- a/drivers/soundwire/bus_type.c +++ b/drivers/soundwire/bus_type.c @@ -49,13 +49,26 @@ int sdw_slave_modalias(const struct sdw_slave *slave, char *buf, size_t size) static int sdw_uevent(struct device *dev, struct kobj_uevent_env *env) { - struct sdw_slave *slave = to_sdw_slave_device(dev); + struct sdw_slave *slave; char modalias[32]; - sdw_slave_modalias(slave, modalias, sizeof(modalias)); - - if (add_uevent_var(env, "MODALIAS=%s", modalias)) - return -ENOMEM; + if (is_sdw_slave(dev)) { + slave = to_sdw_slave_device(dev); + + sdw_slave_modalias(slave, modalias, sizeof(modalias)); + + if (add_uevent_var(env, "MODALIAS=%s", modalias)) + return -ENOMEM; + } else { + /* + * We only need to handle uevents for the Slave device + * type. This error cannot happen unless the .uevent + * callback is set to use this function for a + * different device type (e.g. Master or Monitor) + */ + dev_err(dev, "uevent for unknown Soundwire type\n"); + return -EINVAL; + } return 0; } diff --git a/drivers/soundwire/slave.c b/drivers/soundwire/slave.c index 48a513680db6..c87267f12a3b 100644 --- a/drivers/soundwire/slave.c +++ b/drivers/soundwire/slave.c @@ -14,6 +14,11 @@ static void sdw_slave_release(struct device *dev) kfree(slave); } +struct device_type sdw_slave_type = { + .name = "sdw_slave", + .release = sdw_slave_release, +}; + static int sdw_slave_add(struct sdw_bus *bus, struct sdw_slave_id *id, struct fwnode_handle *fwnode) { @@ -41,9 +46,9 @@ static int sdw_slave_add(struct sdw_bus *bus, id->class_id, id->unique_id); } - slave->dev.release = sdw_slave_release; slave->dev.bus = &sdw_bus_type; slave->dev.of_node = of_node_get(to_of_node(fwnode)); + slave->dev.type = &sdw_slave_type; slave->bus = bus; slave->status = SDW_SLAVE_UNATTACHED; slave->dev_num = 0; diff --git a/include/linux/soundwire/sdw_type.h b/include/linux/soundwire/sdw_type.h index 7d4bc6a979bf..c681b3426478 100644 --- a/include/linux/soundwire/sdw_type.h +++ b/include/linux/soundwire/sdw_type.h @@ -5,6 +5,12 @@ #define __SOUNDWIRE_TYPES_H extern struct bus_type sdw_bus_type; +extern struct device_type sdw_slave_type; + +static inline int is_sdw_slave(const struct device *dev) +{ + return dev->type == &sdw_slave_type; +} #define to_sdw_slave_driver(_drv) \ container_of(_drv, struct sdw_driver, driver)
In the existing SoundWire code, the bus does not have any explicit representation for Master Devices - only SoundWire Slaves are exposed. In SoundWire, the Master Device provides the clock, synchronization information and command/control channels. When multiple links are supported, a Controller may expose more than one Master Device; they are typically embedded inside a larger audio cluster (be it in an SOC/chipset or an external audio codec), and we need to describe it using the Linux device and driver model. This will allow for configuration functions to account for external dependencies such as power rails, clock sources or wake-up mechanisms. This transition will also allow for better sysfs support without the reference count issues mentioned in the initial reviews. In this patch, we first convert the existing code to use an explicit sdw_slave_type and add error checks if this type is not set. In follow-up patches we can add support for the sdw_master_type. Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com> --- drivers/soundwire/bus_type.c | 23 ++++++++++++++++++----- drivers/soundwire/slave.c | 7 ++++++- include/linux/soundwire/sdw_type.h | 6 ++++++ 3 files changed, 30 insertions(+), 6 deletions(-)