Message-ID: <fe70d83a-dc5c-3d5a-1162-dacd2d2c6039@opensource.cirrus.com>
Date:   Tue, 31 Jan 2023 10:59:03 +0000
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.6.0
Subject: Re: [PATCH v3 6/8] ASoC: cs42l42: Add SoundWire support
Content-Language: en-US
To:     Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>,
        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>
References: <20230127165111.3010960-1-sbinding@opensource.cirrus.com>
 <20230127165111.3010960-7-sbinding@opensource.cirrus.com>
 <7b61ac00-74af-5816-79bb-39eb208ab738@linux.intel.com>
From:   Richard Fitzgerald <rf@opensource.cirrus.com>
In-Reply-To: <7b61ac00-74af-5816-79bb-39eb208ab738@linux.intel.com>
Content-Type: text/plain; charset="UTF-8"; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk

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.