On 20/03/2020 18:17, Pierre-Louis Bossart wrote:
Thanks for the quick review Srinivas,
This patch in general is missing device tree support for both matching
and uevent so this will not clearly work for Qualcomm controller
unless we do via platform bus, which does not sound right!
see other email, the platform bus is handled by a platform
device/driver. There was no intention to change that, it's by design
rather than an omission/error.
I understand this partly now!
This can be probably made better/clear by:
renaming sdw_master_device_add to sdw_master_alloc and do a
device_initialize() as part of this function in subsequent call to
sdw_add_bus_master() we can do a device_add(). Doing this way will avoid
a bit of unnecessary call to device_unregister by the controller driver,
tbh which is confusing.
If the intended call sequence for controller is this (by keeping the
parent bus type intact):
sdw_master_alloc/sdw_master_device_add()
sdw_add_bus_master()
Then we should also remove sdw_unregister_master_driver() and
module_sdw_master_driver() all together. Having them makes the reader
think that they can use module_sdw_master_driver directly without any
parent bus like platform bus in this case.
+
+/**
+ * sdw_master_device_startup() - startup hardware
+ * @md: Linux Soundwire master device
+ *
+ * This use of this function is optional. It is only needed if the
+ * hardware cannot be started during a driver probe, e.g. due to power
+ * rail dependencies. The implementation is platform-specific but the
+ * bus will typically go through a hardware-reset sequence and devices
+ * will be enumerated once they report as ATTACHED.
+ */
+int sdw_master_device_startup(struct sdw_master_device *md)
+{
+ struct sdw_master_driver *mdrv;
+ struct device *dev;
+ int ret = 0;
+
+ if (IS_ERR_OR_NULL(md))
+ return -EINVAL;
+
+ dev = &md->dev;
+ mdrv = drv_to_sdw_master_driver(dev->driver);
+
+ if (mdrv && mdrv->startup)
+ ret = mdrv->startup(md);
+
+ return ret;
+}
+EXPORT_SYMBOL_GPL(sdw_master_device_startup);
Who would call this function? and How would it get hold of master
device instance ?
sdw_master_device_add() returns a struct_master_device *md, so the
parent has a handle to that device. See the device creation here:
https://github.com/thesofproject/linux/blob/9c7487b33072040ab755d32ca173b75151c0160c/drivers/soundwire/intel_init.c#L238
This startup() would be called by the parent when all
requirements/dependencies are met. Put differently, it allows the probe
to run much earlier and check for platform firmware information (which
links are enabled, what devices are exposed by platform firmware), while
the startup is really when the bus clk/data lines will start toggling.
Why would this even need to be in the SoundWire core layer, if parent
can call its local startup function whenever its necessary?
Unless we have some kinda state-machine in the core which can really
deal with this, i see no point in this callback.
--srini
https://github.com/thesofproject/linux/blob/9c7487b33072040ab755d32ca173b75151c0160c/drivers/soundwire/intel_init.c#L341
and the call from the SOF layer is here:
https://github.com/thesofproject/linux/blob/9c7487b33072040ab755d32ca173b75151c0160c/sound/soc/sof/intel/hda-loader.c#L418
Again, if everything is ready at probe time there's no need to delay the
actual bus startup. This is not needed for Qualcomm platforms where the
Master device is part of a codec. It's actually irrelevant since there
is no driver, so the startup callback does not even exist :-)
How would soundwire core also ensure that we do not actively use this
master if it is not ready. Similar comment for shutdown callback.
That's a fair point, we could add a state variable and a check that the
probe happened before.
In practice the two cases of device creation and startup are different
phases so it'd more of a paranoia check.
Thanks,
-Pierre
