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);