Re: [PATCH v4 08/15] soundwire: add initial definitions for sdw_master_device

[Pierre-Louis Bossart]

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.