Re: [PATCH v3 6/8] ASoC: cs42l42: Add SoundWire support

Richard Fitzgerald
2023-01-31T10:59:33Z

On 30/01/2023 16:39, Pierre-Louis Bossart wrote:
>
>
> 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.
>
We'll do it later. As you say, to avoid dependencies.
I've thought a few times about making a function for this but
couldn't decide what to call it to distinguish from the existing
snd_soc_dai_set_dma_data().
My preference was to create a SoC function that does exactly the same as
cs42l42_sdw_dai_set_sdw_stream() so if that's all you need you can
point your snd_soc_dai_ops directly at it.