On 06-01-20, 08:51, Pierre-Louis Bossart wrote:
/* let the SoundWire master driver to its probe */md->driver->probe(md, link);So you are invoking driver probe here.. That is typically role of driver core to do that.. If we need that, make driver core do that for you!
That reminds me I am missing match code for master driver...
There is no match for the master because it doesn't have an existence in ACPI. There are no _ADR or HID that can be used, the only thing that exists is the Controller which has 4 sublinks. Each master must be added by hand.
Also the SoundWire master cannot be enumerated or matched against a SoundWire bus, since it controls the bus itself (that would be a chicken and egg problem). The SoundWire master would need to be matched on a parent bus (which does not exist for Intel) since the hardware is embedded in a larger audio cluster that's visible on PCI only.
Currently for Intel platforms, the SoundWire master device is created by the SOF driver (via the abstraction in intel_init.c).
That is okay for me, the thing that is bit confusing is having a probe etc and no match.. (more below)..
So we seem to be somewhere is middle wrt driver probing here! IIUC this is not a full master driver, thats okay, but then it is not completely transparent either...
I was somehow thinking that the driver will continue to be 'platform/acpi/of' driver and master device abstraction will be handled in the core (for example see how the busses like i2c handle this). The master device is created and used to represent but driver probing etc is not done
I2C controllers are typically PCI devices or have some sort of ACPI description. This is not the case for SoundWire masters on Intel platforms,
Well the world is not PCI/ACPI... We have controllers which are DT described and work in same manner as a PCI device.
Both DT and PCI would use a DIFFERENT matching on the parent bus, not a matching provided by the SoundWire subsystem itself.
so even if I wanted to I would have no ability to implement any matching or parent bus registration.
Also the notion of 'probe' does not necessarily mean that the device is attached to a bus, we use DAI 'drivers' in ASoC and still have probe/remove callbacks.
The "big" difference is that probe is called by core (asoc) and not by driver onto themselves.. IMO that needs to go away.
What I did is not different from what existed already with platform devices. They were manually created, weren't they?
Manual creation of device based on a requirement is different, did I ask you why you are creating device :)
I am simple asking you not to call probe in the driver. If you need that, move it to core! We do not want these kind of things in the drivers...
And if you look at the definitions, we added additional callbacks since probe/remove are not enough to deal with hardware restrictions:
For Intel platforms, we have a startup() callback which is only invoked once the DSP is powered and the rails stable. Likewise we added an 'autonomous_clock_stop()' callback which will be needed when the Linux driver hands-over control of the hardware to the DSP firmware, e.g. to deal with in-band wakes in D0i3.
FWIW, the implementation here follows what was suggested for Greybus 'Host Devices' [1] [2], so it's not like I am creating any sort of dangerous precedent.
[1] https://elixir.bootlin.com/linux/latest/source/drivers/greybus/es2.c#L1275 [2] https://elixir.bootlin.com/linux/latest/source/drivers/greybus/hd.c#L124
And if you look closely all this work is done by core not by drivers! Drivers _should_ never do all this, it is the job of core to do that for you.
Please look at the code again, you have a USB probe that will manually call the GreyBus device creation.
static int ap_probe(struct usb_interface *interface, const struct usb_device_id *id) { hd = gb_hd_create(&es2_driver, &udev->dev,
static struct usb_driver es2_ap_driver = { .name = "es2_ap_driver", .probe = ap_probe, <<< code above .disconnect = ap_disconnect, .id_table = id_table, .soft_unbind = 1, };
Look closely the driver es2 calls into greybus core hd.c and gets the work done, subtle but a big differances in the approaches..
The master device probe suggested here is also called as part of the parent SOF PCI device probe, same as this USB example. I really don't see what your objection is, given that there is no way to deal with the SoundWire controller as a independent entity for Intel platforms.