RE: [PATCH] soundwire: add slave device to linked list after device_register()
> -Original Message-
> From: Pierre-Louis Bossart
> Sent: Wednesday, March 24, 2021 2:31 AM
> To: Vinod Koul ; Bard Liao chuan.l...@linux.intel.com>
> Cc: alsa-de...@alsa-project.org; gre...@linuxfoundation.org; linux-
> ker...@vger.kernel.org; hui.w...@canonical.com;
> srinivas.kandaga...@linaro.org; Kale, Sanyog R ;
> rander.w...@linux.intel.com; Liao, Bard
> Subject: Re: [PATCH] soundwire: add slave device to linked list after
> device_register()
>
> Hi Vinod,
>
> >> We currently add the slave device to a linked list before
> >> device_register(), and then remove it if device_register() fails.
> >>
> >> It's not clear why this sequence was necessary, this patch moves the
> >> linked list management to after the device_register().
> >
> > Maybe add a comment :-)
> >
> > The reason here is that before calling device_register() we need to be
> > ready and completely initialized. device_register is absolutely the
> > last step in the flow, always.
> >
> > The probe of the device can happen and before you get a chance to add
> > to list, many number of things can happen.. So adding after is not a
> > very good idea :)
>
> Can you describe that 'many number of things' in the SoundWire context?
>
> While you are correct in general on the use of device_register(), in this
> specific
> case the device registration and the possible Slave driver probe if there's a
> match doesn't seem to use this linked list.
>
> This sdw_slave_add() routine is called while parsing ACPI/DT tables and at
> this
> point the bus isn't functional. This sequence only deals with device
> registration
> and driver probe, the actual activation and enumeration happen much later.
> What does matter is that by the time all ACPI/DT devices have been scanned all
> initialization is complete. The last part of the flow is not the
> device_register() at
> the individual peripheral level.
>
> Even for the Qualcomm case, the steps are different:
>
> ret = sdw_bus_master_add(>bus, dev, dev->fwnode);
> if (ret) {
> dev_err(dev, "Failed to register Soundwire controller (%d)\n",
> ret);
> goto err_clk;
> }
>
> qcom_swrm_init(ctrl); <<< that's where the bus is functional
>
> Note that we are not going to lay on the tracks for this, this sequence was
> tagged by static analysis tools who don't understand that
> put_device() actually frees memory by way of the .release callback, but that
> led
> us to ask ourselves whether this sequence was actually necessary.
Hi Vinod,
Do you have any comment or objection after Pierre's explanation?
Regards,
Bard
RE: [PATCH v2 3/5] ASoC/SoundWire: rt715-sdca: First version of rt715 sdw sdca codec driver
> -Original Message- > From: Mark Brown > Sent: Thursday, December 3, 2020 12:08 AM > To: Bard Liao > Cc: alsa-de...@alsa-project.org; vk...@kernel.org; vinod.k...@linaro.org; > linux-kernel@vger.kernel.org; ti...@suse.de; gre...@linuxfoundation.org; > j...@cadence.com; srinivas.kandaga...@linaro.org; > rander.w...@linux.intel.com; ranjani.sridha...@linux.intel.com; > hui.w...@canonical.com; pierre-louis.boss...@linux.intel.com; Kale, Sanyog > R ; Liao, Bard > Subject: Re: [PATCH v2 3/5] ASoC/SoundWire: rt715-sdca: First version of > rt715 sdw sdca codec driver > > On Mon, Nov 30, 2020 at 10:40:18PM +0800, Bard Liao wrote: > > From: Jack Yu > > > > First version of rt715 sdw sdca codec driver. > > This doesn't apply against the ASoC tree, please check and resend. Looks like the previous version is already in ASoC tree. I will resend the remaining patches on top of it.
RE: [PATCH v2 0/5] regmap/SoundWire/ASoC: Add SoundWire SDCA support
> -Original Message- > From: Bard Liao > Sent: Monday, November 30, 2020 10:40 PM > To: alsa-de...@alsa-project.org; vk...@kernel.org > Cc: vinod.k...@linaro.org; linux-kernel@vger.kernel.org; ti...@suse.de; > broo...@kernel.org; gre...@linuxfoundation.org; j...@cadence.com; > srinivas.kandaga...@linaro.org; rander.w...@linux.intel.com; > ranjani.sridha...@linux.intel.com; hui.w...@canonical.com; pierre- > louis.boss...@linux.intel.com; Kale, Sanyog R ; > Liao, Bard > Subject: [PATCH v2 0/5] regmap/SoundWire/ASoC: Add SoundWire SDCA > support > > The MIPI SoundWire Device Class standard will define audio functionality > beyond the scope of the existing SoundWire 1.2 standard, which is limited to > the bus and interface. > > The description is inspired by the USB Audio Class, with "functions", > "entities", "control selectors", "audio clusters". The main difference with > the > USB Audio class is that the devices are typically on a motherboard and > descriptors stored in platform firmware instead of being retrieved from the > device. > > The current set of devices managed in this patchset are conformant with the > SDCA 0.6 specification and require dedicated drivers since the descriptors > and platform firmware specification is not complete at this time. They do > however rely on the hierarchical addressing required by the SDCA standard. > Future devices conformant with SDCA 1.0 should rely on a class driver. > > This series adds support for the hierarchical SDCA addressing and extends > regmap. It then provides 3 codecs for RT711-sdca headset codec, RT1316 > amplifier and RT715-scda microphone codec. > > Note that the release of this code before the formal adoption of the SDCA > 1.0 specification was formally endorsed by the MIPI Board to make sure > there is no delay for Linux-based support of this specification. > > v2: > - rt715-sdca: Use rt715_sdca prefix to avoid compiling issue. > - rt715-sdca: Merge multiple mute/volume operation into single > mute/volume > operation > - rt711-sdca: Initial ret = 0 as it could be used uninitialized. > > Jack Yu (1): > ASoC/SoundWire: rt715-sdca: First version of rt715 sdw sdca codec > driver > > Pierre-Louis Bossart (2): > soundwire: SDCA: add helper macro to access controls > regmap/SoundWire: sdw: add support for SoundWire 1.2 MBQ > > Shuming Fan (2): > ASoC/SoundWire: rt1316: Add RT1316 SDCA vendor-specific driver > ASoC/SoundWire: rt711-sdca: Add RT711 SDCA vendor-specific driver > Hi Vinod/Mark, Could we take this series into Vinod's tree with Mark's Acked-by? It failed to build on Mark's tree. > > -- > 2.17.1
RE: [PATCH] regmap: sdw: add required header files
> -Original Message- > From: Greg KH > Sent: Thursday, November 26, 2020 1:35 PM > To: Bard Liao > Cc: alsa-de...@alsa-project.org; vk...@kernel.org; vinod.k...@linaro.org; > linux-kernel@vger.kernel.org; ti...@suse.de; broo...@kernel.org; > j...@cadence.com; srinivas.kandaga...@linaro.org; > rander.w...@linux.intel.com; ranjani.sridha...@linux.intel.com; > hui.w...@canonical.com; pierre-louis.boss...@linux.intel.com; Kale, Sanyog > R ; Liao, Bard > Subject: Re: [PATCH] regmap: sdw: add required header files > > On Wed, Nov 25, 2020 at 09:01:28PM +0800, Bard Liao wrote: > > From: Pierre-Louis Bossart > > > > Explicitly add header files used by regmap SoundWire support. > > What is failing to build without this? I didn't see build errors. But, Documentation/process/submit-checklist.rst says. 1) If you use a facility then #include the file that defines/declares that facility. Don't depend on other header files pulling in ones that you use. > > thanks, > > greg k-h
RE: [PATCH] soundwire: master: use pm_runtime_set_active() on add
> -Original Message-
> From: Vinod Koul
> Sent: Wednesday, November 25, 2020 1:05 PM
> To: Bard Liao
> Cc: alsa-de...@alsa-project.org; linux-kernel@vger.kernel.org;
> gre...@linuxfoundation.org; j...@cadence.com;
> srinivas.kandaga...@linaro.org; rander.w...@linux.intel.com;
> ranjani.sridha...@linux.intel.com; hui.w...@canonical.com; pierre-
> louis.boss...@linux.intel.com; Kale, Sanyog R ; Lin,
> Mengdong ; Liao, Bard
> Subject: Re: [PATCH] soundwire: master: use pm_runtime_set_active() on
> add
>
> On 24-11-20, 21:07, Bard Liao wrote:
> > From: Pierre-Louis Bossart
> >
> > The 'master' device acts as a glue layer used during bus
> > initialization only, and it needs to be 'transparent' for pm_runtime
> > management. Its behavior should be that it becomes active when one of
> > its children becomes active, and suspends when all of its children are
> > suspended.
> >
> > In our tests on Intel platforms, we routinely see these sort of
> > warnings on the initial boot:
> >
> > [ 21.447345] rt715 sdw:3:25d:715:0: runtime PM trying to activate
> > child device sdw:3:25d:715:0 but parent (sdw-master-3) is not active
> >
> > This is root-caused to a missing setup to make the device 'active' on
> > probe. Since we don't want the device to remain active forever after
> > the probe, the autosuspend configuration is also enabled at the end of
> > the probe - the device will actually autosuspend only in the case
> > where there are no devices physically attached. In practice, the
> > master device will suspend when all its children are no longer active.
> >
> > Fixes: bd84256e86ecf ('soundwire: master: enable pm runtime')
> > Signed-off-by: Pierre-Louis Bossart
> >
> > Reviewed-by: Rander Wang
> > Signed-off-by: Bard Liao
> > ---
> > drivers/soundwire/master.c | 14 ++
> > 1 file changed, 14 insertions(+)
> >
> > diff --git a/drivers/soundwire/master.c b/drivers/soundwire/master.c
> > index 3488bb824e84..9b05c9e25ebe 100644
> > --- a/drivers/soundwire/master.c
> > +++ b/drivers/soundwire/master.c
> > @@ -8,6 +8,15 @@
> > #include #include "bus.h"
> >
> > +/*
> > + * The 3s value for autosuspend will only be used if there are no
> > + * devices physically attached on a bus segment. In practice enabling
> > + * the bus operation will result in children devices become active
> > +and
> > + * the master device will only suspend when all its children are no
> > + * longer active.
> > + */
> > +#define SDW_MASTER_SUSPEND_DELAY_MS 3000
> > +
> > /*
> > * The sysfs for properties reflects the MIPI description as given
> > * in the MIPI DisCo spec
> > @@ -154,7 +163,12 @@ int sdw_master_device_add(struct sdw_bus *bus,
> struct device *parent,
> > bus->dev = >dev;
> > bus->md = md;
> >
> > + pm_runtime_set_autosuspend_delay(>md->dev,
> SDW_MASTER_SUSPEND_DELAY_MS);
> > + pm_runtime_use_autosuspend(>md->dev);
> > + pm_runtime_mark_last_busy(>md->dev);
> > + pm_runtime_set_active(>md->dev);
> > pm_runtime_enable(>md->dev);
> > + pm_runtime_idle(>md->dev);
>
> I understand that this needs to be done somewhere but is the core the right
> place. In intel case it maybe a dummy device but other controllers are real
> devices and may not support pm.
>
> I think better idea would be to do this in respective driver.. that way it
> would be an opt-in for device supporting pm.
Should it be put in the same place as pm_runtime_enable?
IMHO, pm_runtime_enable is in the core already and it seems to be harmless
for devices which don't support pm. And pm can still be optional on md's
parent device.
>
> Thanks
> --
> ~Vinod
RE: [PATCH v3] soundwire: SDCA: add helper macro to access controls
> -Original Message- > From: Greg KH > Sent: Friday, October 30, 2020 5:37 PM > To: Bard Liao > Cc: alsa-de...@alsa-project.org; vk...@kernel.org; vinod.k...@linaro.org; > linux-kernel@vger.kernel.org; j...@cadence.com; > srinivas.kandaga...@linaro.org; rander.w...@linux.intel.com; > ranjani.sridha...@linux.intel.com; hui.w...@canonical.com; pierre- > louis.boss...@linux.intel.com; Kale, Sanyog R ; Lin, > Mengdong ; Liao, Bard > Subject: Re: [PATCH v3] soundwire: SDCA: add helper macro to access > controls > > On Fri, Oct 30, 2020 at 04:49:55AM +0800, Bard Liao wrote: > > From: Pierre-Louis Bossart > > > > The upcoming SDCA (SoundWire Device Class Audio) specification defines > > a hierarchical encoding to interface with Class-defined capabilities. > > > > The specification is not yet accessible to the general public but this > > information is released with explicit permission from the MIPI Board > > to avoid delays with SDCA support on Linux platforms. > > > > A block of 64 MBytes of register addresses are allocated to SDCA > > controls, starting at address 0x4000. The 26 LSBs which identify > > individual controls are set based on the following variables: > > > > - Function Number. An SCDA device can be split in up to 8 independent > > Functions. Each of these Functions is described in the SDCA > > specification, e.g. Smart Amplifier, Smart Microphone, Simple > > Microphone, Jack codec, HID, etc. > > > > - Entity Number. Within each Function, an Entity is an identifiable > > block. Up to 127 Entities are connected in a pre-defined > > graph (similar to USB), with Entity0 reserved for Function-level > > configurations. In contrast to USB, the SDCA spec pre-defines > > Function Types, topologies, and allowed options, i.e. the degree of > > freedom is not unlimited to limit the possibility of errors in > > descriptors leading to software quirks. > > > > - Control Selector. Within each Entity, the SDCA specification defines > > 48 controls such as Mute, Gain, AGC, etc, and 16 implementation > > defined ones. Some Control Selectors might be used for low-level > > platform setup, and other exposed to applications and users. Note > > that the same Control Selector capability, e.g. Latency control, > > might be located at different offsets in different entities, the > > Control Selector mapping is Entity-specific. > > > > - Control Number. Some Control Selectors allow channel-specific values > > to be set, with up to 64 channels allowed. This is mostly used for > > volume control. > > > > - Current/Next values. Some Control Selectors are > > 'Dual-Ranked'. Software may either update the Current value directly > > for immediate effect. Alternatively, software may write into the > > 'Next' values and update the SoundWire 1.2 'Commit Groups' register > > to copy 'Next' values into 'Current' ones in a synchronized > > manner. This is different from bank switching which is typically > > used to change the bus configuration only. > > > > - MBQ. the Multi-Byte Quantity bit is used to provide atomic updates > > when accessing more that one byte, for example a 16-bit volume > > control would be updated consistently, the intermediate values > > mixing old MSB with new LSB are not applied. > > > > These 6 parameters are used to build a 32-bit address to access the > > desired Controls. Because of address range, paging is required, but > > the most often used parameter values are placed in the lower 16 bits > > of the address. This helps to keep the paging registers constant while > > updating Controls for a specific Device/Function. > > > > Reviewed-by: Rander Wang > > Reviewed-by: Guennadi Liakhovetski > > > > Reviewed-by: Kai Vehmanen > > Signed-off-by: Pierre-Louis Bossart > > > > Signed-off-by: Bard Liao > > --- > > Changelog: > > > > v2: > > - add SDW_SDCA_MBQ_CTL > > > > v3: > > - add SDW_SDCA_NEXT_CTL > > > > --- > > include/linux/soundwire/sdw_registers.h | 32 > > + > > 1 file changed, 32 insertions(+) > > > > diff --git a/include/linux/soundwire/sdw_registers.h > > b/include/linux/soundwire/sdw_registers.h > > index f420e8059779..e14dff9a9c7f 100644 > > --- a/include/linux/soundwire/sdw_registers.h > > +++ b/include/linux/soundwire/sdw_registers.h > > @@ -298,4 +298,36 @@ > > #define SDW_CASC_PORT_MASK_INTSTAT3
RE: [PATCH 0/7] ASoC: soundwire: Move sdw stream operations to
> -Original Message- > From: Vinod Koul > Sent: Friday, September 4, 2020 1:11 PM > To: Pierre-Louis Bossart > Cc: Bard Liao ; alsa-devel@alsa- > project.org; ti...@suse.de; gre...@linuxfoundation.org; linux- > ker...@vger.kernel.org; ranjani.sridha...@linux.intel.com; > hui.w...@canonical.com; broo...@kernel.org; > srinivas.kandaga...@linaro.org; j...@cadence.com; Lin, Mengdong > ; Kale, Sanyog R ; > rander.w...@linux.intel.com; Liao, Bard > Subject: Re: [PATCH 0/7] ASoC: soundwire: Move sdw stream operations to > > On 03-09-20, 09:05, Pierre-Louis Bossart wrote: > > > > > > On 9/3/20 5:42 AM, Vinod Koul wrote: > > > On 01-09-20, 23:02, Bard Liao wrote: > > > > sdw stream operation APIs can be called once per stream. dailink > > > > callbacks are good places to call these APIs. > > > > > > Again, please mention here if this is to be merged thru sdw tree or > > > ASoC tree > > > > Good point, I thought it wouldn't matter but it does. I just gave it a > > try and there seems to be a conflict on Mark's tree w/ > > drivers/soundwire/intel.c (likely due to missing patches already added to > Vinod's tree). > > > > So this should go to Vinod's tree with Mark's Acked-by tag on the ASoC > > changes. > > > > Alternatively we can also split this in two, with ASoC-only and > > SoundWire-only patches in separate series if it's easier for > > maintainers. We would lose the rationale for the changes but that's not > essential. > > If there are no dependencies on each other, that is best preferred option. > One should mention in cover-letter about the linked series though. Sent as v2
RE: [PATCH v2 3/3] ASoC: codecs: fix port_ready[] dynamic allocation in ASoC codecs
Hi Mark,
> -Original Message-
> From: Bard Liao
> Sent: Friday, August 21, 2020 2:27 AM
> To: alsa-de...@alsa-project.org; vk...@kernel.org
> Cc: vinod.k...@linaro.org; linux-kernel@vger.kernel.org; ti...@suse.de;
> broo...@kernel.org; gre...@linuxfoundation.org; j...@cadence.com;
> srinivas.kandaga...@linaro.org; rander.w...@linux.intel.com;
> ranjani.sridha...@linux.intel.com; hui.w...@canonical.com; pierre-
> louis.boss...@linux.intel.com; Kale, Sanyog R ; Lin,
> Mengdong ; Liao, Bard
> Subject: [PATCH v2 3/3] ASoC: codecs: fix port_ready[] dynamic allocation in
> ASoC codecs
>
> From: Pierre-Louis Bossart
>
> As port_ready is now changed to a fixed array in sdw.h, we can't dynamic
> allocate it in codec drivers.
>
> Signed-off-by: Pierre-Louis Bossart
> Reviewed-by: Rander Wang
> Reviewed-by: Guennadi Liakhovetski
> Signed-off-by: Bard Liao
> ---
> sound/soc/codecs/max98373-sdw.c | 15 +--
> sound/soc/codecs/rt1308-sdw.c | 14 +-
> sound/soc/codecs/rt5682-sdw.c | 15 +--
> sound/soc/codecs/rt700-sdw.c| 15 +--
> sound/soc/codecs/rt711-sdw.c| 15 +--
> sound/soc/codecs/rt715-sdw.c| 33 +
> 6 files changed, 6 insertions(+), 101 deletions(-)
Sorry to ping you, but the patch is really easy to be ignored since I
forget to add ASoC prefix in the cover letter.
Could you Ack it if it looks good to you, please?
>
> diff --git a/sound/soc/codecs/max98373-sdw.c
> b/sound/soc/codecs/max98373-sdw.c index 5fe724728e84..a3ec92775ea7
> 100644
> --- a/sound/soc/codecs/max98373-sdw.c
> +++ b/sound/soc/codecs/max98373-sdw.c
> @@ -282,7 +282,7 @@ static const struct dev_pm_ops max98373_pm =
> { static int max98373_read_prop(struct sdw_slave *slave) {
> struct sdw_slave_prop *prop = >prop;
> - int nval, i, num_of_ports;
> + int nval, i;
> u32 bit;
> unsigned long addr;
> struct sdw_dpn_prop *dpn;
> @@ -295,7 +295,6 @@ static int max98373_read_prop(struct sdw_slave
> *slave)
> prop->clk_stop_timeout = 20;
>
> nval = hweight32(prop->source_ports);
> - num_of_ports = nval;
> prop->src_dpn_prop = devm_kcalloc(>dev, nval,
> sizeof(*prop->src_dpn_prop),
> GFP_KERNEL);
> @@ -315,7 +314,6 @@ static int max98373_read_prop(struct sdw_slave
> *slave)
>
> /* do this again for sink now */
> nval = hweight32(prop->sink_ports);
> - num_of_ports += nval;
> prop->sink_dpn_prop = devm_kcalloc(>dev, nval,
> sizeof(*prop->sink_dpn_prop),
> GFP_KERNEL);
> @@ -333,17 +331,6 @@ static int max98373_read_prop(struct sdw_slave
> *slave)
> i++;
> }
>
> - /* Allocate port_ready based on num_of_ports */
> - slave->port_ready = devm_kcalloc(>dev, num_of_ports,
> - sizeof(*slave->port_ready),
> - GFP_KERNEL);
> - if (!slave->port_ready)
> - return -ENOMEM;
> -
> - /* Initialize completion */
> - for (i = 0; i < num_of_ports; i++)
> - init_completion(>port_ready[i]);
> -
> /* set the timeout values */
> prop->clk_stop_timeout = 20;
>
> diff --git a/sound/soc/codecs/rt1308-sdw.c b/sound/soc/codecs/rt1308-sdw.c
> index b0ba0d2acbdd..09c69dbab12a 100644
> --- a/sound/soc/codecs/rt1308-sdw.c
> +++ b/sound/soc/codecs/rt1308-sdw.c
> @@ -118,7 +118,7 @@ static int rt1308_clock_config(struct device *dev)
> static int rt1308_read_prop(struct sdw_slave *slave) {
> struct sdw_slave_prop *prop = >prop;
> - int nval, i, num_of_ports = 1;
> + int nval, i;
> u32 bit;
> unsigned long addr;
> struct sdw_dpn_prop *dpn;
> @@ -131,7 +131,6 @@ static int rt1308_read_prop(struct sdw_slave *slave)
>
> /* for sink */
> nval = hweight32(prop->sink_ports);
> - num_of_ports += nval;
> prop->sink_dpn_prop = devm_kcalloc(>dev, nval,
> sizeof(*prop-
> >sink_dpn_prop),
> GFP_KERNEL);
> @@ -149,17 +148,6 @@ static int rt1308_read_prop(struct sdw_slave *slave)
> i++;
> }
>
> - /* Allocate port_ready based on num_of_ports */
> - slave->port_ready = devm_kcalloc(>dev, num_of_ports,
> - sizeof(*slave->port_ready),
> - GFP
RE: [PATCH 07/11] soundwire: intel: Only call sdw stream APIs for the first cpu_dai
> -Original Message-
> From: Vinod Koul
> Sent: Wednesday, August 26, 2020 5:47 PM
> To: Bard Liao
> Cc: alsa-de...@alsa-project.org; linux-kernel@vger.kernel.org; ti...@suse.de;
> broo...@kernel.org; gre...@linuxfoundation.org; j...@cadence.com;
> srinivas.kandaga...@linaro.org; rander.w...@linux.intel.com;
> ranjani.sridha...@linux.intel.com; hui.w...@canonical.com; pierre-
> louis.boss...@linux.intel.com; Kale, Sanyog R ; Lin,
> Mengdong ; Liao, Bard
> Subject: Re: [PATCH 07/11] soundwire: intel: Only call sdw stream APIs for
> the first cpu_dai
>
> On 18-08-20, 10:41, Bard Liao wrote:
> > We should call these APIs once per stream. So we can only call it when
> > the dai ops is invoked for the first cpu dai.
> >
> > Signed-off-by: Bard Liao
> > Reviewed-by: Pierre-Louis Bossart
> >
> > Reviewed-by: Ranjani Sridharan
> > ---
> > drivers/soundwire/intel.c | 45
> > +--
> > 1 file changed, 39 insertions(+), 6 deletions(-)
> >
> > diff --git a/drivers/soundwire/intel.c b/drivers/soundwire/intel.c
> > index 89a8ad1f80e8..7c63581270fd 100644
> > --- a/drivers/soundwire/intel.c
> > +++ b/drivers/soundwire/intel.c
> > @@ -941,11 +941,13 @@ static int intel_hw_params(struct
> > snd_pcm_substream *substream, static int intel_prepare(struct
> snd_pcm_substream *substream,
> > struct snd_soc_dai *dai)
> > {
> > + struct snd_soc_pcm_runtime *rtd = substream->private_data;
> > + struct snd_soc_dai *first_cpu_dai = asoc_rtd_to_cpu(rtd, 0);
> > struct sdw_cdns *cdns = snd_soc_dai_get_drvdata(dai);
> > struct sdw_intel *sdw = cdns_to_intel(cdns);
> > struct sdw_cdns_dma_data *dma;
> > int ch, dir;
> > - int ret;
> > + int ret = 0;
> >
> > dma = snd_soc_dai_get_dma_data(dai, substream);
> > if (!dma) {
> > @@ -985,7 +987,13 @@ static int intel_prepare(struct
> snd_pcm_substream *substream,
> > goto err;
> > }
> >
> > - ret = sdw_prepare_stream(dma->stream);
> > + /*
> > +* All cpu dais belong to a stream. To ensure sdw_prepare_stream
> > +* is called once per stream, we should call it only when
> > +* dai = first_cpu_dai.
> > +*/
> > + if (first_cpu_dai == dai)
> > + ret = sdw_prepare_stream(dma->stream);
>
> Hmmm why not use the one place which is unique in the card to call this,
> hint machine dais are only called once.
Yes, we can call it in machine driver. But, shouldn't it belong to platform
level? The point is that if we move the stuff to machine driver, it will
force people to implement these stuff on their own Intel machine driver.
>
> ~Vinod
RE: [PATCH 1/2] soundwire: add definition for maximum number of ports
> -Original Message-
> From: Vinod Koul
> Sent: Tuesday, August 18, 2020 2:36 PM
> To: Bard Liao
> Cc: alsa-de...@alsa-project.org; linux-kernel@vger.kernel.org; ti...@suse.de;
> broo...@kernel.org; gre...@linuxfoundation.org; j...@cadence.com;
> srinivas.kandaga...@linaro.org; rander.w...@linux.intel.com;
> ranjani.sridha...@linux.intel.com; hui.w...@canonical.com; pierre-
> louis.boss...@linux.intel.com; Kale, Sanyog R ; Lin,
> Mengdong ; Liao, Bard
> Subject: Re: [PATCH 1/2] soundwire: add definition for maximum number of
> ports
>
> On 18-08-20, 01:47, Bard Liao wrote:
> > From: Pierre-Louis Bossart
> >
> > A Device may have at most 15 physical ports (DP0, DP1..DP14).
> >
> > Signed-off-by: Pierre-Louis Bossart
> >
> > Reviewed-by: Rander Wang
> > Reviewed-by: Guennadi Liakhovetski
> >
> > Signed-off-by: Bard Liao
> > ---
> > include/linux/soundwire/sdw.h | 3 ++-
> > 1 file changed, 2 insertions(+), 1 deletion(-)
> >
> > diff --git a/include/linux/soundwire/sdw.h
> > b/include/linux/soundwire/sdw.h index 76052f12c9f7..0aa4c6af7554
> > 100644
> > --- a/include/linux/soundwire/sdw.h
> > +++ b/include/linux/soundwire/sdw.h
> > @@ -38,7 +38,8 @@ struct sdw_slave;
> > #define SDW_FRAME_CTRL_BITS48
> > #define SDW_MAX_DEVICES11
> >
> > -#define SDW_VALID_PORT_RANGE(n)((n) <= 14 && (n) >= 1)
> > +#define SDW_MAX_PORTS 15
> > +#define SDW_VALID_PORT_RANGE(n)((n) <
> SDW_MAX_PORTS && (n) >= 1)
>
> What is the use of this one if we are allocating all ports always, Also, I
> dont
> see it used in second patch?
It is used in drivers/soundwire/stream.c and drivers/soundwire/debugfs.c.
>
> >
> > enum {
> > SDW_PORT_DIRN_SINK = 0,
> > --
> > 2.17.1
>
> --
> ~Vinod
RE: [PATCH] soundwire: master: enable pm runtime
> -Original Message- > From: Greg KH > Sent: Friday, July 24, 2020 4:32 PM > To: Bard Liao > Cc: alsa-de...@alsa-project.org; vk...@kernel.org; vinod.k...@linaro.org; > linux-kernel@vger.kernel.org; ti...@suse.de; broo...@kernel.org; > j...@cadence.com; srinivas.kandaga...@linaro.org; > rander.w...@linux.intel.com; ranjani.sridha...@linux.intel.com; > hui.w...@canonical.com; pierre-louis.boss...@linux.intel.com; Kale, Sanyog > R ; Lin, Mengdong ; > Liao, Bard > Subject: Re: [PATCH] soundwire: master: enable pm runtime > > On Thu, Jul 23, 2020 at 09:49:02PM +0800, Bard Liao wrote: > > We should enable pm runtime. > > Because why? The hierarchy of soundwire devices is platform device -> M device -> S device. A S device is physically attached on the platform device. So the platform device should be resumed when a S device is resumed. As the bridge of platform device and S device, we have to implement runtime pm on M driver. We have set runtime pm ops in M driver already, but still need to enable runtime pm. > > Please read the documentation about how to write good changelog > comments... Sure. I will update the changelog in next version. Thanks for the advice. > > greg k-h
RE: [PATCH 7/9] soundwire: intel/cadence: merge Soundwire interrupt handlers/threads
> -Original Message- > From: Vinod Koul > Sent: Wednesday, July 1, 2020 1:42 PM > To: Pierre-Louis Bossart > Cc: Bard Liao ; alsa-de...@alsa-project.org; > ti...@suse.de; gre...@linuxfoundation.org; linux-kernel@vger.kernel.org; > ranjani.sridha...@linux.intel.com; hui.w...@canonical.com; > broo...@kernel.org; srinivas.kandaga...@linaro.org; j...@cadence.com; Lin, > Mengdong ; Blauciak, Slawomir > ; Kale, Sanyog R ; > rander.w...@linux.intel.com; Liao, Bard > Subject: Re: [PATCH 7/9] soundwire: intel/cadence: merge Soundwire interrupt > handlers/threads > > On 30-06-20, 11:46, Pierre-Louis Bossart wrote: > > > > Is this called from irq context or irq thread or something else? > > > > from IRQ thread, hence the name, see pointers above. > > > > The key part is that we could only make the hardware work as intended by > > using a single thread for all interrupt sources, and that patch is just the > > generalization of what was implemented for HDaudio in mid-2019 after > months > > of lost interrupts and IPC errors. See below the code from > > sound/soc/sof/intel/hda.c for interrupt handling. > > Sounds good. Now that you are already in irq thread, does it make sense > to spawn a worker thread for this and handle it there? Why not do in the > irq thread itself. Using a thread kind of defeats the whole point behind > concept of irq threads Not sure If you are talking about cdns_update_slave_status_work(). The reason we need to spawn a worker thread in sdw_cdns_irq() is that we will do sdw transfer which will generate an interrupt when a slave interrupt is triggered. And the handler will not be invoked if the previous handler is not return yet. Please see the scenario below for better explanation. 1. Slave interrupt arrives 2.1 Try to read Slave register and waiting for the transfer response 2.2 Get the transfer response interrupt and finish the sdw transfer. 3. Finish the Slave interrupt handling. Interrupts are triggered in step 1 and 2.2, but step 2.2's handler will not be invoked if step 1's handler is not return yet. What we do is to spawn a worker thread to do step 2 and return from step 1. So the handler can be invoked when the transfer response interrupt arrives. > > -- > ~Vinod
RE: [PATCH 2/2] soundwire: intel: transition to 3 steps initialization
> -Original Message- > From: Vinod Koul > Sent: Thursday, May 21, 2020 12:37 PM > To: Liao, Bard > Cc: Bard Liao ; alsa-de...@alsa-project.org; > linux-kernel@vger.kernel.org; ti...@suse.de; broo...@kernel.org; > gre...@linuxfoundation.org; j...@cadence.com; > srinivas.kandaga...@linaro.org; rander.w...@linux.intel.com; > ranjani.sridha...@linux.intel.com; hui.w...@canonical.com; pierre- > louis.boss...@linux.intel.com; Kale, Sanyog R ; > Blauciak, Slawomir ; Lin, Mengdong > > Subject: Re: [PATCH 2/2] soundwire: intel: transition to 3 steps > initialization > > On 21-05-20, 02:23, Liao, Bard wrote: > > > -Original Message- > > > From: Vinod Koul > > > Sent: Wednesday, May 20, 2020 9:54 PM > > > To: Bard Liao > > > Cc: alsa-de...@alsa-project.org; linux-kernel@vger.kernel.org; > > > ti...@suse.de; broo...@kernel.org; gre...@linuxfoundation.org; > > > j...@cadence.com; srinivas.kandaga...@linaro.org; > > > rander.w...@linux.intel.com; ranjani.sridha...@linux.intel.com; > > > hui.w...@canonical.com; pierre- louis.boss...@linux.intel.com; Kale, > > > Sanyog R ; Blauciak, Slawomir > > > ; Lin, Mengdong > > > ; Liao, Bard > > > Subject: Re: [PATCH 2/2] soundwire: intel: transition to 3 steps > > > initialization > > > > > > On 20-05-20, 03:19, Bard Liao wrote: > > > > From: Pierre-Louis Bossart > > > > > > > > Rather than a plain-vanilla init/exit, this patch provides 3 steps > > > > in the initialization (ACPI scan, probe, startup) which makes it > > > > easier to detect platform support for SoundWire, allocate required > > > > resources as early as possible, and conversely help make the > > > > startup() callback lighter-weight with only hardware register setup. > > > > > > Okay but can you add details in changelog on what each step would do? > > > > Sure. Will do. > > > > > > > > > @@ -1134,25 +1142,15 @@ static int intel_probe(struct > > > > platform_device > > > *pdev) > > > > > > > > intel_pdi_ch_update(sdw); > > > > > > > > - /* Acquire IRQ */ > > > > - ret = request_threaded_irq(sdw->link_res->irq, > > > > - sdw_cdns_irq, sdw_cdns_thread, > > > > - IRQF_SHARED, KBUILD_MODNAME, > > > >cdns); > > > > > > This is removed here but not added anywhere else, do we have no irq > > > after this patch? > > > > We use a single irq for all Intel Audio DSP events and it will be > > requested in the SOF driver. > > And how will the irq be propagated to sdw/cdns drivers here? We export the handler and call it on SOF driver. > > -- > ~Vinod
RE: [PATCH 2/2] soundwire: intel: transition to 3 steps initialization
> -Original Message-
> From: Vinod Koul
> Sent: Wednesday, May 20, 2020 9:54 PM
> To: Bard Liao
> Cc: alsa-de...@alsa-project.org; linux-kernel@vger.kernel.org; ti...@suse.de;
> broo...@kernel.org; gre...@linuxfoundation.org; j...@cadence.com;
> srinivas.kandaga...@linaro.org; rander.w...@linux.intel.com;
> ranjani.sridha...@linux.intel.com; hui.w...@canonical.com; pierre-
> louis.boss...@linux.intel.com; Kale, Sanyog R ;
> Blauciak, Slawomir ; Lin, Mengdong
> ; Liao, Bard
> Subject: Re: [PATCH 2/2] soundwire: intel: transition to 3 steps
> initialization
>
> On 20-05-20, 03:19, Bard Liao wrote:
> > From: Pierre-Louis Bossart
> >
> > Rather than a plain-vanilla init/exit, this patch provides 3 steps in
> > the initialization (ACPI scan, probe, startup) which makes it easier to
> > detect platform support for SoundWire, allocate required resources as
> > early as possible, and conversely help make the startup() callback
> > lighter-weight with only hardware register setup.
>
> Okay but can you add details in changelog on what each step would do?
Sure. Will do.
>
> > @@ -1134,25 +1142,15 @@ static int intel_probe(struct platform_device
> *pdev)
> >
> > intel_pdi_ch_update(sdw);
> >
> > - /* Acquire IRQ */
> > - ret = request_threaded_irq(sdw->link_res->irq,
> > - sdw_cdns_irq, sdw_cdns_thread,
> > - IRQF_SHARED, KBUILD_MODNAME,
> >cdns);
>
> This is removed here but not added anywhere else, do we have no irq
> after this patch?
We use a single irq for all Intel Audio DSP events and it will
be requested in the SOF driver.
>
> > @@ -1205,5 +1201,5 @@ static struct platform_driver sdw_intel_drv = {
> > module_platform_driver(sdw_intel_drv);
> >
> > MODULE_LICENSE("Dual BSD/GPL");
> > -MODULE_ALIAS("platform:int-sdw");
> > +MODULE_ALIAS("sdw:intel-sdw");
>
> it is still a platform device, so does sdw: tag make sense?
> This is used by modprobe to load the driver!
Will fix it
>
> > +/**
> > + * sdw_intel_probe() - SoundWire Intel probe routine
> > + * @res: resource data
> > + *
> > + * This creates SoundWire Master and Slave devices below the controller.
>
> I dont think the comment is correct, this is done in intel_master_probe
> which is platform device probe...
Thanks. Will fix it.
>
> > + * All the information necessary is stored in the context, and the res
> > + * argument pointer can be freed after this step.
> > + */
> > +struct sdw_intel_ctx
> > +*sdw_intel_probe(struct sdw_intel_res *res)
> > +{
> > + return sdw_intel_probe_controller(res);
> > +}
> > +EXPORT_SYMBOL(sdw_intel_probe);
>
> I guess this would be called by SOF driver, question is when..?
Will document it, thanks.
>
> > +/**
> > + * sdw_intel_startup() - SoundWire Intel startup
> > + * @ctx: SoundWire context allocated in the probe
> > + *
> > + */
> > +int sdw_intel_startup(struct sdw_intel_ctx *ctx)
> > +{
> > + return sdw_intel_startup_controller(ctx);
> > +}
> > +EXPORT_SYMBOL(sdw_intel_startup);
>
> when is this called, pls do document that
Will document it, thanks.
>
> --
> ~Vinod
RE: [PATCH 3/3] soundwire: bus_type: add sdw_master_device support
> -Original Message-
> From: Vinod Koul
> Sent: Monday, May 11, 2020 5:00 PM
> To: Liao, Bard
> Cc: Bard Liao ; alsa-de...@alsa-project.org;
> linux-kernel@vger.kernel.org; ti...@suse.de; broo...@kernel.org;
> gre...@linuxfoundation.org; j...@cadence.com;
> srinivas.kandaga...@linaro.org; rander.w...@linux.intel.com;
> ranjani.sridha...@linux.intel.com; hui.w...@canonical.com; pierre-
> louis.boss...@linux.intel.com; Kale, Sanyog R ;
> Blauciak, Slawomir ; Lin, Mengdong
>
> Subject: Re: [PATCH 3/3] soundwire: bus_type: add sdw_master_device support
>
> On 11-05-20, 08:04, Liao, Bard wrote:
> > > -Original Message-
> > > From: Vinod Koul
> > > Sent: Monday, May 11, 2020 2:32 PM
> > > To: Bard Liao
> > > Cc: alsa-de...@alsa-project.org; linux-kernel@vger.kernel.org;
> > > ti...@suse.de; broo...@kernel.org; gre...@linuxfoundation.org;
> > > j...@cadence.com; srinivas.kandaga...@linaro.org;
> > > rander.w...@linux.intel.com; ranjani.sridha...@linux.intel.com;
> > > hui.w...@canonical.com; pierre- louis.boss...@linux.intel.com; Kale,
> > > Sanyog R ; Blauciak, Slawomir
> > > ; Lin, Mengdong
> > > ; Liao, Bard
> > > Subject: Re: [PATCH 3/3] soundwire: bus_type: add sdw_master_device
> > > support
> > >
> > > On 30-04-20, 02:51, Bard Liao wrote:
> > > > @@ -24,9 +24,14 @@ int sdw_bus_master_add(struct sdw_bus *bus,
> > > > struct
> > > device *parent,
> > > > struct sdw_master_prop *prop = NULL;
> > > > int ret;
> > > >
> > > > - if (!bus->dev) {
> > > > - pr_err("SoundWire bus has no device\n");
> > > > - return -ENODEV;
> > >
> > > This check is removed and not moved into sdw_master_device_add()
> > > either, can you add here or in patch 1 and keep checking the parent
> > > device please
> >
> > We will set bus->dev = >dev in the end of sdw_master_device_add().
>
> We need to test if this is valid or not :)
>
> > That's why we remove the test. But now I am wandering does it make
> > sense to set bus->dev = >dev? Maybe it makes more sense to set
> > bus->dev = sdw control device.
> > A follow up question is that should slave device a child of bus device
> > or master device? I would prefer bus device if it makes sense.
> > I will check bus->dev and parent and remove bus->dev = >dev in the
> > next version.
>
> the parent is bus->dev and sdw_master_device created would be child of this
> and should be set as such. You can remove it from bus object and keep in
> sdw_master_device object, that is fine by me.
Looks like we don't need the parent and fwnode parameter since we can
get them from bus->dev
>
> The sdw_slave is child of sdw_master_device now and looks to be set correct.
So, it will be
bus device
-> master device
-> slave device
right?
I have a question here. We have pm supported on bus and slave devices,
but not master device. Will pm work with this architecture?
Can it be
bus device
-> master device & slave device?
>
> --
> ~Vinod
RE: [PATCH 3/3] soundwire: bus_type: add sdw_master_device support
> -Original Message-
> From: Vinod Koul
> Sent: Monday, May 11, 2020 2:32 PM
> To: Bard Liao
> Cc: alsa-de...@alsa-project.org; linux-kernel@vger.kernel.org; ti...@suse.de;
> broo...@kernel.org; gre...@linuxfoundation.org; j...@cadence.com;
> srinivas.kandaga...@linaro.org; rander.w...@linux.intel.com;
> ranjani.sridha...@linux.intel.com; hui.w...@canonical.com; pierre-
> louis.boss...@linux.intel.com; Kale, Sanyog R ;
> Blauciak, Slawomir ; Lin, Mengdong
> ; Liao, Bard
> Subject: Re: [PATCH 3/3] soundwire: bus_type: add sdw_master_device support
>
> On 30-04-20, 02:51, Bard Liao wrote:
> > @@ -24,9 +24,14 @@ int sdw_bus_master_add(struct sdw_bus *bus, struct
> device *parent,
> > struct sdw_master_prop *prop = NULL;
> > int ret;
> >
> > - if (!bus->dev) {
> > - pr_err("SoundWire bus has no device\n");
> > - return -ENODEV;
>
> This check is removed and not moved into sdw_master_device_add() either, can
> you add here or in patch 1 and keep checking the parent device please
We will set bus->dev = >dev in the end of sdw_master_device_add().
That's why we remove the test. But now I am wandering does it make sense
to set bus->dev = >dev? Maybe it makes more sense to set bus->dev =
sdw control device.
A follow up question is that should slave device a child of bus device or
master device? I would prefer bus device if it makes sense.
I will check bus->dev and parent and remove bus->dev = >dev in the
next version.
>
> > +int sdw_master_device_add(struct sdw_bus *bus, struct device *parent,
> > + struct fwnode_handle *fwnode)
> > +{
> > + struct sdw_master_device *md;
> > + int ret;
> > +
> > + if (!bus)
> > + return -EINVAL;
> > +
> > + /*
> > +* Unlike traditional devices, there's no allocation here since the
> > +* sdw_master_device is embedded in the bus structure.
> > +*/
>
> Looking at this and empty sdw_master_device_release() above, makes me
> wonder if it is a wise move? Should we rather allocate the
> sdw_master_device() and then free that up in sdw_master_device_release() or it
> is really overkill given that this is called when we remove the sdw_bus
> instance
> as well...
Yes, I will allocate sdw_master_device here and free it in
sdw_master_device_release().
>
> > + md = >md;
> > + md->dev.bus = _bus_type;
> > + md->dev.type = _master_type;
> > + md->dev.parent = parent;
> > + md->dev.of_node = parent->of_node;
> > + md->dev.fwnode = fwnode;
> > + md->dev.dma_mask = parent->dma_mask;
> > +
> > + dev_set_name(>dev, "sdw-master-%d", bus->link_id);
>
> This give nice sdw-master-0. In DT this comes from reg property. I dont seem
> to
> recall if the ACPI/Disco spec treats link_id as unique across the system, can
> you
> check that please, if not we would need to update this.
Sure, I will check it.
>
> --
> ~Vinod
