On 04-03-20, 17:28, Greg KH wrote:
On Wed, Mar 04, 2020 at 09:17:07AM -0600, Pierre-Louis Bossart wrote:
Were the above lines agreed or not? Do you see driver for master devices or not? Greg was okay with as well as these patches but I am not okay with the driver part for master, so I would like to see that removed.
Different reviewers can have different reasons.. I have given bunch of reasons here, BUT I have not seen a single technical reason why this cannot be done.
With all due respect, I consider Greg as THE reviewer for device/driver questions. Your earlier proposal to use platform devices was rejected by Greg, and we've lost an entire month in the process, so I am somewhat dubious on your proposal not to use a driver.
If you want a technical objection, let me restate what I already mentioned:
If you look at the hierarchy, we have
PCI device -> PCI driver soundwire_master_device0 soundwire_slave(s) -> codec driver ... soundwire_master_deviceN soundwire_slave(s) -> codec driver
You have not explained how I could possibly deal with power management without having a driver for the master_device(s). The pm_ops need to be inserted in a driver structure, which means we need a driver. And if we need a driver, then we might as well have a real driver with .probe .remove support, driver_register(), etc.
To weigh in here, yes, you need such a "device" here as it isn't the PCI device that you can use, you need your own. Just like most other busses have this (USB has host controller drivers as one example, that create the "root bus" device that all other USB devices hang off of.) This "controller device" should hang off of the hardware device be it a platform/PCI/i2c/spi/serial/whatever type of controller. That's why it is needed.
I really don't see what's broken or unnecessary with these patches.
The "wait until something else happens" does seem a bit hacky, odds are that's not really needed if you are using the driver model correctly, but soundwire is "odd" in places so maybe that is necessary, I'll defer to you two on that mess :)
I have no issues with adding the root bus device, so we really need the sdw_master_device. That really was a miss from me. If Pierre remove the driver parts from this set, I would merge the rest in a heartbeat.
But in the mix we dont want to add a new driver which is dummy. The PCI/DT device will have a driver which is something to be used here.
For Qualcomm controller, we get a DT device and driver and that does the job, it doesn't need one more driver and probe routine.
If Pierre had a PCI device for SDW controller, he would not be asking for this, since Intel has a complex device, and he would like to split the functions, the need of a new driver arises. But the whole subsystem should not be burdened for that. It can be managed without that and is really an Intel issue not a subsystem one.