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 :)
thanks,
greg k-h