On 12/13/19 1:28 AM, Greg KH wrote:
On Thu, Dec 12, 2019 at 11:04:02PM -0600, Pierre-Louis Bossart wrote:
Since we want an explicit support for the SoundWire Master device, add the definitions, following the Grey Bus example.
"Greybus" All one word please.
Ack, will fix.
@@ -59,9 +59,12 @@ int sdw_uevent(struct device *dev, struct kobj_uevent_env *env)
if (add_uevent_var(env, "MODALIAS=%s", modalias)) return -ENOMEM;
- } else if (is_sdw_md(dev)) {
Ok, "is_sdw_md()" is a horrid function name. Spell it out please, this ends up in the global namespace.
ok, will use is_sdw_master_device.
Actually, why are you not using module namespaces here for this new code? That would help you out a lot.
I must admit I don't understand the question. This is literally modeled after is_gb_host_device(), did I miss something in the Greybus implementation?
/* this should not happen but throw an error */dev_warn(dev, "uevent for Master device, unsupported\n");Um, what? This is supported as it will happen when you create such a device. It's an issue of "I didn't write the code yet", not that it is not "supported".
I will remove, it cannot happen.
diff --git a/drivers/soundwire/master.c b/drivers/soundwire/master.c new file mode 100644 index 000000000000..6210098c892b --- /dev/null +++ b/drivers/soundwire/master.c @@ -0,0 +1,62 @@ +// SPDX-License-Identifier: (GPL-2.0 OR BSD-3-Clause)
Still with the crazy dual license? I thought we went over this all before.
You can not do this for code that touches driver core stuff, like this. Please stop and just make all of this GPLv2 like we discussed months ago.
I don't recall this was the guidance but fine.
+// 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,
Bad function names, please spell things out, you have plenty of characters to go around.
This was modeled after gb_hd_create ;-)
sdw_master_device_add starts to be on the long side, no?
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");The callback is missing for the driver you passed in, not for the parent, right?
yes, this function is called as part of the parent probe.
- 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);But you still return a valid pointer? Why????
Ah, yes, this is clearly wrong, thanks for pointing this out.
What's the recommended error code for this? Greybus uses:
return ERR_PTR(-ENOMEM);
+EXPORT_SYMBOL(sdw_md_add);
EXPORT_SYMBOL_GPL()?
yes, will fix
diff --git a/include/linux/soundwire/sdw.h b/include/linux/soundwire/sdw.h index 5b1180f1e6b5..af0a72e7afdf 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; /* core does not touch */
Core of what?
SoundWire bus driver. This is a copy/paste from the SOF code I am afraid, will fix.
+};
+#define to_sdw_master_device(d) \
- container_of(d, struct sdw_master_device, dev)
- struct sdw_driver { const char *name;
@@ -599,6 +609,26 @@ struct sdw_driver { struct device_driver driver; };
+struct sdw_md_driver {
- /* initializations and allocations */
- int (*probe)(struct sdw_master_device *md, void *link_ctx);
- /* hardware enablement, all clock/power dependencies are available */
- int (*startup)(struct sdw_master_device *md);
- /* hardware disabled */
- int (*shutdown)(struct sdw_master_device *md);
- /* free all resources */
- int (*remove)(struct sdw_master_device *md);
- /*
* 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*/- int (*autonomous_clock_stop_enable)(struct sdw_master_device *md,
bool state);+};
Use kerneldoc comments for this to make it easier to understand and for others to read?
yes, I used kerneldoc everywhere except here, will fix.