On 3/2/20 11:41 PM, Vinod Koul wrote:
On 27-02-20, 16:31, Pierre-Louis Bossart wrote:
In the existing SoundWire code, Master Devices are not explicitly
represented - only SoundWire Slave Devices are exposed (the use of
capital letters follows the SoundWire specification conventions).
The SoundWire 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
I dont not see that as a soundwire issue. The external dependencies
should be handled as any device would do in Linux kernel with subsystem
specific things for soundwire mechanisms like wake-up
There is nothing in the SoundWire specification that tells how a
controller/master should interface with the rest of the SoC.
There are two ways of handling wake-ups
a. 'normal' wakes handled internally by the master - that's what the
regular clock stop does.
b. PCI-based wakes. In that case, the wake is not detected at the
SoundWire level - the IP is power-gated -, but comes from the PCI
subsystem based on a level detector and needs to be notified to the
SoundWire master. That's what I did.
Now you can claim that this case b) doesn't belong in the
drivers/soundwire code, in which case my answer will be to move all the
Intel controller code in sound/soc/sof/intel. I have no choice but to
implement what's needed on the hardware I have.
Intel has a big controller with HDA, DSP and Soundwire clubbed together,
I dont think we should burden the susbstem due to hw design
also allow for better sysfs support without the reference count issues
mentioned in the initial reviews.
In this patch, we convert the existing code to use an explicit
sdw_slave_type, then define new objects (sdw_master_device and
sdw_master_driver).
Thanks for sdw_master_device, that is required and fully agreed upon.
What is not agreed is the sdw_master_driver. We do not need that.
I will respectfully ask that you have a conversation with Greg on this
one. These patches were reviewed as 'sane', I am not going to go back on
this without an agreement on directions.
As we have discussed your proposal with Greg and aligned (quoting that
here) on following device model for Intel and ARM:
- For DT cases we will have:
-> soundwire DT device (soundwire DT driver)
-> soundwire master device
-> soundwire slave device (slave drivers)
- For Intel case, you would have:
-> HDA PCI device (SOF driver + soundwire module)
-> soundwire master device
-> soundwire slave device (slave drivers)
But you have gone ahead and kept the sdw_master_driver which does not fit
into rest of the world except Intel.
I followed Greg's guidance. There was nothing in the thread with Greg
that hinted as a required change.
If you don't agree with Greg, please talk with him.
I think I am okay with rest of proposal, except this one, so can you
remove this and we can make progress. This issue is lingering since Oct!
Yes indeed, and we just circled once more.
A parent (such as the Intel audio controller or its equivalent on
Qualcomm devices) would use sdw_master_device_add() to create the
device, passing a driver name as a parameter. The master device would
be released when device_unregister() is invoked by the parent.
We already have a DT driver for soundwire master! We dont need another
layer which does not add value!
Talk with Greg please.
Note that since there is no standard for the Master host-facing
interface, so the bus matching relies on a simple string matching (as
previously done with platform devices).
The 'Master Device' driver exposes callbacks for
probe/startup/shutdown/remove/process_wake. The startup and process
wake need to be called by the parent directly (using wrappers), while
the probe/shutdown/remove are handled by the SoundWire bus core upon
device creation and release.
these are added to handle intel DSP and sequencing issue, rest of the
world does not have these issues and does not needs them!
They are not required, don't use them if you don't need them?
What's wrong with this approach?
Additional callbacks will be added in the future for e.g. autonomous
clock stop modes.
Yes these would be required, these can be added in sdw_master_device
too, I dont see them requiring a dummy driver layer..
Again talk with Greg.
@@ -113,8 +152,6 @@ static int sdw_drv_probe(struct device *dev)
slave->probed = true;
complete(&slave->probe_complete);
- dev_dbg(dev, "probe complete\n");
-
This does not seem to belong to this patch.
+struct device_type sdw_master_type = {
+ .name = "soundwire_master",
+ .release = sdw_master_device_release,
+};
+
+struct sdw_master_device
+*sdw_master_device_add(const char *master_name,
+ struct device *parent,
+ struct fwnode_handle *fwnode,
+ int link_id,
+ void *pdata)
+{
+ struct sdw_master_device *md;
+ int ret;
+
+ md = kzalloc(sizeof(*md), GFP_KERNEL);
+ if (!md)
+ return ERR_PTR(-ENOMEM);
+
+ md->link_id = link_id;
+ md->pdata = pdata;
+ md->master_name = master_name;
should we not allocate the memory here for master_name?
+
+ init_completion(&md->probe_complete);
+
+ md->dev.parent = parent;
+ md->dev.fwnode = fwnode;
+ md->dev.bus = &sdw_bus_type;
+ md->dev.type = &sdw_master_type;
+ md->dev.dma_mask = md->dev.parent->dma_mask;
+ dev_set_name(&md->dev, "sdw-master-%d", md->link_id);
why do we need master_name if we are setting this here?
for the matching needed to find a driver.
+
+ 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);
ENOMEM?
no, this function returns a pointer.
+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 invokes this and when, can you add kernel-doc style documentation to
all APIs exported
ok
+int sdw_master_device_process_wake_event(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->process_wake_event)
+ ret = mdrv->process_wake_event(md);
+
+ return ret;
+}
+EXPORT_SYMBOL_GPL(sdw_master_device_process_wake_event);
Documentation required
ok
+/**
+ * struct sdw_master_device - SoundWire 'Master Device' representation
+ *
+ * @dev: Linux device for this Master
+ * @master_name: Linux driver name
+ * @driver: Linux driver for this Master (set by SoundWire core during probe)
+ * @probe_complete: used by parent if synchronous probe behavior is needed
+ * @link_id: link index as defined by MIPI DisCo specification
+ * @pm_runtime_suspended: flag to restore pm_runtime state after system resume
+ * @pdata: private data typically provided with sdw_master_device_add()
+ */
+
+struct sdw_master_device {
+ struct device dev;
+ const char *master_name;
+ struct sdw_master_driver *driver;
+ struct completion probe_complete;
+ int link_id;
+ bool pm_runtime_suspended;
why not use runtime_pm apis like pm_runtime_suspended()
that's exactly what we do, we store the value on pm_runtime_suspended()
and use it on resume.
+/**
+ * sdw_master_device_add() - create a Linux Master Device representation.
+ *
+ * @master_name: Linux driver name
+ * @parent: the parent Linux device (e.g. a PCI device)
+ * @fwnode: the parent fwnode (e.g. an ACPI companion device to the parent)
+ * @link_id: link index as defined by MIPI DisCo specification
+ * @pdata: private data (e.g. register base, offsets, platform quirks, etc).
+ */
+struct sdw_master_device
+*sdw_master_device_add(const char *master_name,
+ struct device *parent,
+ struct fwnode_handle *fwnode,
+ int link_id,
+ void *pdata);
+
+/**
+ * sdw_master_device_startup() - startup hardware
+ *
+ * @md: Linux Soundwire master device
Please add more useful comments like when this API would be invoked and
what shall be expected outcome
Huh, that's pretty much self-explanatory? device_add() adds a device and
if there's a driver for it the probe will be called. Nothing fancy or
earth-shattering.
+ */
+int sdw_master_device_startup(struct sdw_master_device *md);
+
+/**
+ * sdw_master_device_process_wake_event() - handle external wake
+ * event, e.g. handled at the PCI level
+ *
+ * @md: Linux Soundwire master device
+ */
+int sdw_master_device_process_wake_event(struct sdw_master_device *md);
+
If you look at existing headers the documentation is in C files for
APIs, so can you move them over.
When adding stuff please look at the rest of the code as an example.
isn't it more clear when the prototypes come with the documentation,
rather than the kernel doc stuff be buried in pages of code?
