Re: [alsa-devel] [PATCH v5 08/17] soundwire: add initial definitions for sdw_master_device

[Pierre-Louis Bossart]

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