| Message ID | 20191217210314.20410-9-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: > Since we want an explicit support for the SoundWire Master device, add > the definitions, following the Greybus example of a 'Host Device'. > > A parent (such as the Intel audio controller) would use sdw_md_add() > to create the device, passing a driver as a parameter. The > sdw_md_release() would be called when put_device() is invoked by the > parent. We use the shortcut 'md' for 'master device' to avoid very > long function names. I agree we should not have long name :) but md does not sound great. Can we drop the device and use sdw_slave and sdw_master for devices and append _driver when we are talking about drivers... we dont use sd for slave and imo this would gel well with existing names > --- a/drivers/soundwire/bus_type.c > +++ b/drivers/soundwire/bus_type.c > @@ -66,7 +66,10 @@ int sdw_uevent(struct device *dev, struct kobj_uevent_env *env) > * 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"); > + if (is_sdw_master_device(dev)) > + dev_err(dev, "uevent for SoundWire Master type\n"); see below [1]: > +static void sdw_md_release(struct device *dev) sdw_master_release() won't be too long! > +{ > + struct sdw_master_device *md = to_sdw_master_device(dev); > + > + kfree(md); > +} > + > +struct device_type sdw_md_type = { sdw_master_type and so on :) > + .name = "soundwire_master", > + .release = sdw_md_release, [1]: There is no uevent added here, so sdw_uevent() will never be called for this, can you check again if you see the print you added? > > +struct sdw_master_device { we have sdw_slave, so would be better if we call this sdw_master > + struct device dev; > + int link_id; > + struct sdw_md_driver *driver; > + void *pdata; no sdw_bus pointer and I dont see bus adding this object.. Is there no link between bus and master device objects?
On 12/27/19 1:14 AM, Vinod Koul wrote: > On 17-12-19, 15:03, Pierre-Louis Bossart wrote: >> Since we want an explicit support for the SoundWire Master device, add >> the definitions, following the Greybus example of a 'Host Device'. >> >> A parent (such as the Intel audio controller) would use sdw_md_add() >> to create the device, passing a driver as a parameter. The >> sdw_md_release() would be called when put_device() is invoked by the >> parent. We use the shortcut 'md' for 'master device' to avoid very >> long function names. > > I agree we should not have long name :) but md does not sound great. Can > we drop the device and use sdw_slave and sdw_master for devices and > append _driver when we are talking about drivers... > > we dont use sd for slave and imo this would gel well with existing names In SoundWire parlance, both 'Slave' and 'Master' are 'Devices', so yes we do in the standard talk about 'Slave Devices' and 'Master Devices'. Then we have Linux 'Devices' which can be used for both. If we use 'sdw_slave', would we be referring to the actual physical part or the Linux device? FWIW the Greybus example used 'Host Device' and 'hd' as shortcut. > >> --- a/drivers/soundwire/bus_type.c >> +++ b/drivers/soundwire/bus_type.c >> @@ -66,7 +66,10 @@ int sdw_uevent(struct device *dev, struct kobj_uevent_env *env) >> * 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"); >> + if (is_sdw_master_device(dev)) >> + dev_err(dev, "uevent for SoundWire Master type\n"); > > see below [1]: > >> +static void sdw_md_release(struct device *dev) > > sdw_master_release() won't be too long! yes, but there is no such operation as 'Master Release' in SoundWire. At least the 'md' shortcut conveys the implicit convention that this is a Linux device only. I really would like to avoid overloading the standard definitions with the Linux ones... > >> +{ >> + struct sdw_master_device *md = to_sdw_master_device(dev); >> + >> + kfree(md); >> +} >> + >> +struct device_type sdw_md_type = { > > sdw_master_type and so on :) > >> + .name = "soundwire_master", >> + .release = sdw_md_release, > > [1]: > There is no uevent added here, so sdw_uevent() will never be called for > this, can you check again if you see the print you added? as explained this is to avoid errors at a later point. I don't see any harm in providing error checks for a routine that can only be used for 1 of the 3 devices defined in the standard? >> >> +struct sdw_master_device { > > we have sdw_slave, so would be better if we call this sdw_master > >> + struct device dev; >> + int link_id; >> + struct sdw_md_driver *driver; >> + void *pdata; > > no sdw_bus pointer and I dont see bus adding this object.. Is there no > link between bus and master device objects? There is currently no bus device object, see the structure definition it's a pointer to a device (which leads to all sorts of issues because we can't use container_of). when the master device gets added, that's where the Linux device is created and the pointer saved in the bus structure, with IIRC sdw_add_bus_master(). ret = sdw_add_bus_master(&sdw->cdns.bus);
On 27-12-19, 17:38, Pierre-Louis Bossart wrote: > > > On 12/27/19 1:14 AM, Vinod Koul wrote: > > On 17-12-19, 15:03, Pierre-Louis Bossart wrote: > > > Since we want an explicit support for the SoundWire Master device, add > > > the definitions, following the Greybus example of a 'Host Device'. > > > > > > A parent (such as the Intel audio controller) would use sdw_md_add() > > > to create the device, passing a driver as a parameter. The > > > sdw_md_release() would be called when put_device() is invoked by the > > > parent. We use the shortcut 'md' for 'master device' to avoid very > > > long function names. > > > > I agree we should not have long name :) but md does not sound great. Can > > we drop the device and use sdw_slave and sdw_master for devices and > > append _driver when we are talking about drivers... > > > > we dont use sd for slave and imo this would gel well with existing names > > In SoundWire parlance, both 'Slave' and 'Master' are 'Devices', so yes we do > in the standard talk about 'Slave Devices' and 'Master Devices'. > > Then we have Linux 'Devices' which can be used for both. > > If we use 'sdw_slave', would we be referring to the actual physical part or > the Linux device? > > FWIW the Greybus example used 'Host Device' and 'hd' as shortcut. But this messes up consistency in the naming of sdw objects. I am all for it, if we do sd for slave and name all structs and APIs accordingly. The key is consistency! So it needs to be sd/md and so on or sdw_slave and sdw_master and so on... not a mix of both > > > --- a/drivers/soundwire/bus_type.c > > > +++ b/drivers/soundwire/bus_type.c > > > @@ -66,7 +66,10 @@ int sdw_uevent(struct device *dev, struct kobj_uevent_env *env) > > > * 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"); > > > + if (is_sdw_master_device(dev)) > > > + dev_err(dev, "uevent for SoundWire Master type\n"); > > > > see below [1]: > > > > > +static void sdw_md_release(struct device *dev) > > > > sdw_master_release() won't be too long! > > yes, but there is no such operation as 'Master Release' in SoundWire. > At least the 'md' shortcut conveys the implicit convention that this is a > Linux device only. > > I really would like to avoid overloading the standard definitions with the > Linux ones... I agree with you on not overloading but from a linux pov, we need names which are consistent with each other... > > > +{ > > > + struct sdw_master_device *md = to_sdw_master_device(dev); > > > + > > > + kfree(md); > > > +} > > > + > > > +struct device_type sdw_md_type = { > > > > sdw_master_type and so on :) > > > > > + .name = "soundwire_master", > > > + .release = sdw_md_release, > > > > [1]: > > There is no uevent added here, so sdw_uevent() will never be called for > > this, can you check again if you see the print you added? > > as explained this is to avoid errors at a later point. I don't see any harm > in providing error checks for a routine that can only be used for 1 of the 3 > devices defined in the standard? > > > > +struct sdw_master_device { > > > > we have sdw_slave, so would be better if we call this sdw_master > > > > > + struct device dev; > > > + int link_id; > > > + struct sdw_md_driver *driver; > > > + void *pdata; > > > > no sdw_bus pointer and I dont see bus adding this object.. Is there no > > link between bus and master device objects? > > There is currently no bus device object, see the structure definition it's a > pointer to a device (which leads to all sorts of issues because we can't use > container_of). > > when the master device gets added, that's where the Linux device is created > and the pointer saved in the bus structure, with IIRC sdw_add_bus_master(). > > > ret = sdw_add_bus_master(&sdw->cdns.bus);
>>>> A parent (such as the Intel audio controller) would use sdw_md_add() >>>> to create the device, passing a driver as a parameter. The >>>> sdw_md_release() would be called when put_device() is invoked by the >>>> parent. We use the shortcut 'md' for 'master device' to avoid very >>>> long function names. >>> >>> I agree we should not have long name :) but md does not sound great. Can >>> we drop the device and use sdw_slave and sdw_master for devices and >>> append _driver when we are talking about drivers... >>> >>> we dont use sd for slave and imo this would gel well with existing names >> >> In SoundWire parlance, both 'Slave' and 'Master' are 'Devices', so yes we do >> in the standard talk about 'Slave Devices' and 'Master Devices'. >> >> Then we have Linux 'Devices' which can be used for both. >> >> If we use 'sdw_slave', would we be referring to the actual physical part or >> the Linux device? >> >> FWIW the Greybus example used 'Host Device' and 'hd' as shortcut. > > But this messes up consistency in the naming of sdw objects. I am all for > it, if we do sd for slave and name all structs and APIs accordingly. The key > is consistency! > > So it needs to be sd/md and so on or sdw_slave and sdw_master and so > on... not a mix of both Well the problem is that the existing code took a shortcut and only modeled the slave part, e.g. struct sdw_slave *slave = dev_to_sdw_dev(dev); so now it's difficult to add 'sdw_slave_device' and 'sdw_master_device' without quite a few changes. Would this work for you if we used the following names: sdw_slave (legacy shortcut for sdw_slave_device, which could be removed in a a future cleanup if desired). sdw_slave_driver sdw_master_device sdw_master_driver and all the 'md' replaced by the full 'master_device'.
On 02-01-20, 11:36, Pierre-Louis Bossart wrote: > Would this work for you if we used the following names: > > sdw_slave (legacy shortcut for sdw_slave_device, which could be removed in a > a future cleanup if desired). > sdw_slave_driver > sdw_master_device > sdw_master_driver > > and all the 'md' replaced by the full 'master_device'. That does sound nicer, thanks
diff --git a/drivers/soundwire/Makefile b/drivers/soundwire/Makefile index 563894e5ecaf..89b29819dd3a 100644 --- a/drivers/soundwire/Makefile +++ b/drivers/soundwire/Makefile @@ -4,7 +4,7 @@ # #Bus Objs -soundwire-bus-objs := bus_type.o bus.o slave.o mipi_disco.o stream.o +soundwire-bus-objs := bus_type.o bus.o master.o slave.o mipi_disco.o stream.o obj-$(CONFIG_SOUNDWIRE) += soundwire-bus.o ifdef CONFIG_DEBUG_FS diff --git a/drivers/soundwire/bus_type.c b/drivers/soundwire/bus_type.c index 605bc7ae57a8..2e4e8c2e629f 100644 --- a/drivers/soundwire/bus_type.c +++ b/drivers/soundwire/bus_type.c @@ -66,7 +66,10 @@ int sdw_uevent(struct device *dev, struct kobj_uevent_env *env) * 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"); + if (is_sdw_master_device(dev)) + dev_err(dev, "uevent for SoundWire Master type\n"); + else + dev_err(dev, "uevent for unknown Soundwire type\n"); return -EINVAL; } diff --git a/drivers/soundwire/master.c b/drivers/soundwire/master.c new file mode 100644 index 000000000000..b018c561708e --- /dev/null +++ b/drivers/soundwire/master.c @@ -0,0 +1,63 @@ +// SPDX-License-Identifier: (GPL-2.0) +// Copyright(c) 2019 Intel Corporation. + +#include <linux/device.h> +#include <linux/acpi.h> +#include <linux/soundwire/sdw.h> +#include <linux/soundwire/sdw_type.h> +#include "bus.h" + +static void sdw_md_release(struct device *dev) +{ + struct sdw_master_device *md = to_sdw_master_device(dev); + + kfree(md); +} + +struct device_type sdw_md_type = { + .name = "soundwire_master", + .release = sdw_md_release, +}; + +struct sdw_master_device *sdw_md_add(struct sdw_md_driver *driver, + struct device *parent, + struct fwnode_handle *fwnode, + int link_id) +{ + struct sdw_master_device *md; + int ret; + + if (!driver->probe) { + dev_err(parent, "mandatory probe callback missing\n"); + return ERR_PTR(-EINVAL); + } + + md = kzalloc(sizeof(*md), GFP_KERNEL); + if (!md) + return ERR_PTR(-ENOMEM); + + md->link_id = link_id; + + md->driver = driver; + + md->dev.parent = parent; + md->dev.fwnode = fwnode; + md->dev.bus = &sdw_bus_type; + md->dev.type = &sdw_md_type; + md->dev.dma_mask = md->dev.parent->dma_mask; + dev_set_name(&md->dev, "sdw-master-%d", md->link_id); + + ret = device_register(&md->dev); + if (ret) { + dev_err(parent, "Failed to add master: ret %d\n", ret); + /* + * On err, don't free but drop ref as this will be freed + * when release method is invoked. + */ + put_device(&md->dev); + return ERR_PTR(-ENOMEM); + } + + return md; +} +EXPORT_SYMBOL_GPL(sdw_md_add); diff --git a/include/linux/soundwire/sdw.h b/include/linux/soundwire/sdw.h index 5b1180f1e6b5..f73c355de5e5 100644 --- a/include/linux/soundwire/sdw.h +++ b/include/linux/soundwire/sdw.h @@ -585,6 +585,16 @@ struct sdw_slave { #define to_sdw_slave_device(d) \ container_of(d, struct sdw_slave, dev) +struct sdw_master_device { + struct device dev; + int link_id; + struct sdw_md_driver *driver; + void *pdata; +}; + +#define to_sdw_master_device(d) \ + container_of(d, struct sdw_master_device, dev) + struct sdw_driver { const char *name; @@ -599,6 +609,29 @@ struct sdw_driver { struct device_driver driver; }; +/** + * struct sdw_md_driver - SoundWire 'Master Device' driver + * + * @probe: initializations and allocation (hardware may not be enabled yet) + * @startup: initialization handled after the hardware is enabled, all + * clock/power dependencies are available + * @shutdown: cleanups before hardware is disabled (optional) + * @free: free all remaining resources + * @autonomous_clock_stop_enable: enable/disable driver control while + * in clock-stop mode, typically in always-on/D0ix modes. When the driver + * yields control, another entity in the system (typically firmware + * running on an always-on microprocessor) is responsible to tracking + * Slave-initiated wakes + */ +struct sdw_md_driver { + int (*probe)(struct sdw_master_device *md, void *link_ctx); + int (*startup)(struct sdw_master_device *md); + int (*shutdown)(struct sdw_master_device *md); + int (*remove)(struct sdw_master_device *md); + int (*autonomous_clock_stop_enable)(struct sdw_master_device *md, + bool state); +}; + #define SDW_SLAVE_ENTRY(_mfg_id, _part_id, _drv_data) \ { .mfg_id = (_mfg_id), .part_id = (_part_id), \ .driver_data = (unsigned long)(_drv_data) } @@ -788,6 +821,11 @@ struct sdw_bus { int sdw_add_bus_master(struct sdw_bus *bus); void sdw_delete_bus_master(struct sdw_bus *bus); +struct sdw_master_device *sdw_md_add(struct sdw_md_driver *driver, + struct device *parent, + struct fwnode_handle *fwnode, + int link_id); + /** * sdw_port_config: Master or Slave Port configuration * diff --git a/include/linux/soundwire/sdw_type.h b/include/linux/soundwire/sdw_type.h index c681b3426478..a2a5ea7843a6 100644 --- a/include/linux/soundwire/sdw_type.h +++ b/include/linux/soundwire/sdw_type.h @@ -6,15 +6,24 @@ extern struct bus_type sdw_bus_type; extern struct device_type sdw_slave_type; +extern struct device_type sdw_md_type; static inline int is_sdw_slave(const struct device *dev) { return dev->type == &sdw_slave_type; } +static inline int is_sdw_master_device(const struct device *dev) +{ + return dev->type == &sdw_md_type; +} + #define to_sdw_slave_driver(_drv) \ container_of(_drv, struct sdw_driver, driver) +#define to_sdw_md_driver(_drv) \ + container_of(_drv, struct sdw_md_driver, driver) + #define sdw_register_slave_driver(drv) \ __sdw_register_slave_driver(drv, THIS_MODULE)
Since we want an explicit support for the SoundWire Master device, add the definitions, following the Greybus example of a 'Host Device'. A parent (such as the Intel audio controller) would use sdw_md_add() to create the device, passing a driver as a parameter. The sdw_md_release() would be called when put_device() is invoked by the parent. We use the shortcut 'md' for 'master device' to avoid very long function names. The 'Master Device' driver exposes interfaces for probe/startup/shutdown/remove and a placeholder for autonomous clock stop support (when the bus control is handed over to a lower-power solution, typically in D0ix modes). Module namespace support is added in a separate patch. Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com> --- drivers/soundwire/Makefile | 2 +- drivers/soundwire/bus_type.c | 5 ++- drivers/soundwire/master.c | 63 ++++++++++++++++++++++++++++++ include/linux/soundwire/sdw.h | 38 ++++++++++++++++++ include/linux/soundwire/sdw_type.h | 9 +++++ 5 files changed, 115 insertions(+), 2 deletions(-) create mode 100644 drivers/soundwire/master.c