Re: [PATCH 1/8] soundwire: bus_type: add master_device/driver support

From: Vinod Koul <vkoul@kernel.org>
To: Greg KH <gregkh@linuxfoundation.org>
Cc: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>,
	alsa-devel@alsa-project.org, tiwai@suse.de,
	linux-kernel@vger.kernel.org,
	Ranjani Sridharan <ranjani.sridharan@linux.intel.com>,
	Hui Wang <hui.wang@canonical.com>,
	broonie@kernel.org, srinivas.kandagatla@linaro.org,
	jank@cadence.com, slawomir.blauciak@intel.com,
	Sanyog Kale <sanyog.r.kale@intel.com>,
	Bard liao <yung-chuan.liao@linux.intel.com>,
	Rander Wang <rander.wang@linux.intel.com>
Subject: Re: [PATCH 1/8] soundwire: bus_type: add master_device/driver support
Date: Thu, 5 Mar 2020 12:16:35 +0530	[thread overview]
Message-ID: <20200305064635.GX4148@vkoul-mobl> (raw)
In-Reply-To: <20200304162837.GA1763256@kroah.com>

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.

-- 
~Vinod