Message-ID: <7b61ac00-74af-5816-79bb-39eb208ab738@linux.intel.com>
Date:   Mon, 30 Jan 2023 10:39:29 -0600
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Firefox/102.0 Thunderbird/102.4.2
Subject: Re: [PATCH v3 6/8] ASoC: cs42l42: Add SoundWire support
Content-Language: en-US
To:     Stefan Binding <sbinding@opensource.cirrus.com>,
        Vinod Koul <vkoul@kernel.org>,
        Bard Liao <yung-chuan.liao@linux.intel.com>,
        Mark Brown <broonie@kernel.org>
Cc:     alsa-devel@alsa-project.org, linux-kernel@vger.kernel.org,
        patches@opensource.cirrus.com,
        Richard Fitzgerald <rf@opensource.cirrus.com>
References: <20230127165111.3010960-1-sbinding@opensource.cirrus.com>
 <20230127165111.3010960-7-sbinding@opensource.cirrus.com>
From:   Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
In-Reply-To: <20230127165111.3010960-7-sbinding@opensource.cirrus.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
Precedence: bulk


On 1/27/23 10:51, Stefan Binding wrote:
> From: Richard Fitzgerald <rf@opensource.cirrus.com>
> 
> This adds support for using CS42L42 as a SoundWire device.
> 
> SoundWire-specifics are kept separate from the I2S implementation as
> much as possible, aiming to limit the risk of breaking the I2C+I2S
> support.
> 
> There are some important differences in the silicon behaviour between
> I2S and SoundWire mode that are reflected in the implementation:
> 
> - ASP (I2S) most not be used in SoundWire mode because the two interfaces
>   share pins.
> 
> - The SoundWire capture (record) port only supports 1 channel. It does
>   not have left-to-right duplication like the ASP.
> 
> - DP2 can only be prepared if the HP has powered-up. DP1 can only be
>   prepared if the ADC has powered-up. (This ordering restriction does
>   not exist for ASPs.) The SoundWire core port-prepare step is
>   triggered by the DAI-link prepare(). This happens before the
>   codec DAI prepare() or the DAPM sequence so these cannot be used
>   to enable HP/ADC. Instead the HP/ADC enable/disable are done during
>   the port_prep callback.
> 
> - The SRCs are an integral part of the audio chain but in silicon their
>   power control is linked to the ASP. There is no equivalent power link
>   to SoundWire DPs so the driver must take "manual" control of SRC power.
> 
> - The SoundWire control registers occupy the lower part of the SoundWire
>   address space so cs42l42 registers are offset by 0x8000 (non-paged) in
>   SoundWire mode.
> 
> - Register addresses are 8-bit paged in I2C mode but 16-bit unpaged in
>   SoundWire.
> 
> - Special procedures are needed on register read/writes to (a) ensure
>   that the previous internal bus transaction has completed, and
>   (b) handle delayed read results, when the read value could not be
>   returned within the SoundWire read command.
> 
> There are also some differences in driver implementation between I2S
> and SoundWire operation:
> 
> - CS42L42 I2S does not runtime_suspend, but runtime_suspend/resume support
>   has been added into the driver in SoundWire mode as the most convenient
>   way to power-up the bus manager and to handle the unattach_request
>   condition, though the CS42L42 chip does not itself suspend or resume.
> 
> - Intel SoundWire host controllers have a low-power clock-stop mode that
>   requires resetting all peripherals when resuming. This means that the
>   interrupt registers will be reset in between the interrupt being
>   generated and the interrupt being handled, and since the interrupt
>   status is debounced, these values may not be accurate immediately,
>   and may cause spurious unplug events before settling.
> 
> - As in I2S mode, the PLL is only used while audio is active because
>   of clocking quirks in the silicon. For SoundWire the cs42l42_pll_config()
>   is deferred until the DAI prepare(), to allow the cs42l42_bus_config()
>   callback to set the SCLK.
> 
> Signed-off-by: Richard Fitzgerald <rf@opensource.cirrus.com>
> Signed-off-by: Stefan Binding <sbinding@opensource.cirrus.com>

Reviewed-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>

> +static int cs42l42_sdw_dai_set_sdw_stream(struct snd_soc_dai *dai, void *sdw_stream,
> +					  int direction)
> +{
> +	if (!sdw_stream)
> +		return 0;
> +
> +	if (direction == SNDRV_PCM_STREAM_PLAYBACK)
> +		dai->playback_dma_data = sdw_stream;
> +	else
> +		dai->capture_dma_data = sdw_stream;

This may need to be updated to
snd_soc_dai_dma_data_set(dai, direction, stream);

which is being introduced by Morimoto-san

To avoid dependencies this could be updated later.