From: Vinod Koul <vkoul@kernel.org>
To: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
Cc: alsa-devel@alsa-project.org, tiwai@suse.de,
gregkh@linuxfoundation.org, linux-kernel@vger.kernel.org,
Ranjani Sridharan <ranjani.sridharan@linux.intel.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: [alsa-devel] [PATCH v5 09/17] soundwire: intel: remove platform devices and use 'Master Devices' instead
Date: Mon, 6 Jan 2020 11:12:21 +0530 [thread overview]
Message-ID: <20200106054221.GN2818@vkoul-mobl> (raw)
In-Reply-To: <5be4d9df-0f46-d36f-471c-aae9e1f55cc0@linux.intel.com>
On 27-12-19, 18:13, Pierre-Louis Bossart wrote:
>
>
> > > +extern struct sdw_md_driver intel_sdw_driver;
> >
> > who uses this intel_sdw_driver? I would assumed someone would register
> > this with the core...
>
> this is a structure used by intel_init(), see the following code.
>
> + md = sdw_md_add(&intel_sdw_driver,
> + res->parent,
> + acpi_fwnode_handle(adev),
> + i);
>
> that will in turn call intel_master_probe() as defined below:
>
> +struct sdw_md_driver intel_sdw_driver = {
> + .probe = intel_master_probe,
> + .startup = intel_master_startup,
> +
>
> > > - link->pdev = pdev;
> > > - link++;
> > > + /* 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.
> 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.
> 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.
--
~Vinod
next prev parent reply other threads:[~2020-01-06 5:42 UTC|newest]
Thread overview: 47+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-12-17 21:02 [PATCH v5 00/17] soundwire: intel: implement new ASoC interfaces Pierre-Louis Bossart
2019-12-17 21:02 ` [PATCH v5 01/17] soundwire: renames to prepare support for master drivers/devices Pierre-Louis Bossart
2019-12-17 21:02 ` [PATCH v5 02/17] soundwire: rename dev_to_sdw_dev macro Pierre-Louis Bossart
2019-12-27 6:54 ` Vinod Koul
2019-12-17 21:03 ` [PATCH v5 03/17] soundwire: rename drv_to_sdw_slave_driver macro Pierre-Louis Bossart
2019-12-27 7:00 ` Vinod Koul
2019-12-27 23:23 ` [alsa-devel] " Pierre-Louis Bossart
2019-12-28 12:03 ` Vinod Koul
2019-12-17 21:03 ` [PATCH v5 04/17] soundwire: bus_type: rename sdw_drv_ to sdw_slave_drv Pierre-Louis Bossart
2019-12-17 21:03 ` [PATCH v5 05/17] soundwire: intel: rename res field as link_res Pierre-Louis Bossart
2019-12-17 21:03 ` [PATCH v5 06/17] soundwire: add support for sdw_slave_type Pierre-Louis Bossart
2019-12-27 7:03 ` Vinod Koul
2019-12-27 23:26 ` [alsa-devel] " Pierre-Louis Bossart
2019-12-28 12:05 ` Vinod Koul
2019-12-17 21:03 ` [PATCH v5 07/17] soundwire: slave: move uevent handling to slave device level Pierre-Louis Bossart
2019-12-17 21:03 ` [PATCH v5 08/17] soundwire: add initial definitions for sdw_master_device Pierre-Louis Bossart
2019-12-27 7:14 ` Vinod Koul
2019-12-27 23:38 ` [alsa-devel] " Pierre-Louis Bossart
2019-12-28 12:09 ` Vinod Koul
2020-01-02 17:36 ` Pierre-Louis Bossart
2020-01-06 5:32 ` Vinod Koul
2019-12-17 21:03 ` [PATCH v5 09/17] soundwire: intel: remove platform devices and use 'Master Devices' instead Pierre-Louis Bossart
2019-12-27 9:08 ` Vinod Koul
2019-12-28 0:13 ` [alsa-devel] " Pierre-Louis Bossart
2020-01-06 5:42 ` Vinod Koul [this message]
2020-01-06 14:51 ` Pierre-Louis Bossart
2020-01-10 6:43 ` Vinod Koul
2020-01-10 16:08 ` Pierre-Louis Bossart
2020-01-13 5:18 ` Vinod Koul
2020-01-13 15:22 ` Pierre-Louis Bossart
2020-01-14 6:09 ` Vinod Koul
2020-01-14 16:01 ` [alsa-devel] " Pierre-Louis Bossart
2020-01-18 7:12 ` Vinod Koul
2020-01-21 17:31 ` Pierre-Louis Bossart
2020-01-28 10:50 ` Vinod Koul
2020-01-28 16:02 ` Pierre-Louis Bossart
2020-01-29 5:08 ` Vinod Koul
2020-01-29 14:59 ` Pierre-Louis Bossart
2020-02-03 12:02 ` Vinod Koul
2019-12-17 21:03 ` [PATCH v5 10/17] soundwire: register master device driver Pierre-Louis Bossart
2019-12-17 21:03 ` [PATCH v5 11/17] soundwire: intel: add prepare support in sdw dai driver Pierre-Louis Bossart
2019-12-17 21:03 ` [PATCH v5 12/17] soundwire: intel: add trigger " Pierre-Louis Bossart
2019-12-17 21:03 ` [PATCH v5 13/17] soundwire: intel: add sdw_stream_setup helper for .startup callback Pierre-Louis Bossart
2019-12-17 21:03 ` [PATCH v5 14/17] soundwire: intel: free all resources on hw_free() Pierre-Louis Bossart
2019-12-17 21:03 ` [PATCH v5 15/17] soundwire: intel_init: add implementation of sdw_intel_enable_irq() Pierre-Louis Bossart
2019-12-17 21:03 ` [PATCH v5 16/17] soundwire: intel_init: use EXPORT_SYMBOL_NS Pierre-Louis Bossart
2019-12-17 21:03 ` [PATCH v5 17/17] soundwire: intel: " Pierre-Louis Bossart
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20200106054221.GN2818@vkoul-mobl \
--to=vkoul@kernel.org \
--cc=alsa-devel@alsa-project.org \
--cc=broonie@kernel.org \
--cc=gregkh@linuxfoundation.org \
--cc=jank@cadence.com \
--cc=linux-kernel@vger.kernel.org \
--cc=pierre-louis.bossart@linux.intel.com \
--cc=rander.wang@linux.intel.com \
--cc=ranjani.sridharan@linux.intel.com \
--cc=sanyog.r.kale@intel.com \
--cc=slawomir.blauciak@intel.com \
--cc=srinivas.kandagatla@linaro.org \
--cc=tiwai@suse.de \
--cc=yung-chuan.liao@linux.intel.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).