Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <8f907b3f-b02f-774a-f3f7-844527795fbd@linaro.org>
Date:   Thu, 1 Sep 2022 09:50:35 +0100
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101
 Thunderbird/91.11.0
Subject: Re: [PATCH 6/6] ASoC: codecs: tx-macro: add support for sm8450 and
 sc8280xp
Content-Language: en-US
To:     Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>,
        broonie@kernel.org
Cc:     robh+dt@kernel.org, krzysztof.kozlowski+dt@linaro.org,
        perex@perex.cz, tiwai@suse.com,
        pierre-louis.bossart@linux.intel.com,
        linux-arm-msm@vger.kernel.org, alsa-devel@alsa-project.org,
        devicetree@vger.kernel.org, linux-kernel@vger.kernel.org
References: <20220818134619.3432-1-srinivas.kandagatla@linaro.org>
 <20220818134619.3432-7-srinivas.kandagatla@linaro.org>
 <aac3942d-bc0b-7763-0d12-f5d6c585cc97@linaro.org>
From:   Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
In-Reply-To: <aac3942d-bc0b-7763-0d12-f5d6c585cc97@linaro.org>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk


On 01/09/2022 08:30, Krzysztof Kozlowski wrote:
> On 18/08/2022 16:46, Srinivas Kandagatla wrote:
>> LPASS VA Macro now has soundwire master to deal with access to
>> analog mic in low power island use cases.
>>
>> This is added after sc8280xp, add support for this.
>> Along with this also add compatibles for sm8450 and sc8280xp.
>>
>> Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
>> ---
>>   sound/soc/codecs/lpass-va-macro.c | 64 ++++++++++++++++++++++++++++++-
>>   1 file changed, 62 insertions(+), 2 deletions(-)
>>
>> diff --git a/sound/soc/codecs/lpass-va-macro.c b/sound/soc/codecs/lpass-va-macro.c
>> index a35f684053d2..f8b0c8caa1db 100644
>> --- a/sound/soc/codecs/lpass-va-macro.c
>> +++ b/sound/soc/codecs/lpass-va-macro.c
>> @@ -25,6 +25,10 @@
>>   #define CDC_VA_FS_CONTROL_EN			BIT(0)
>>   #define CDC_VA_FS_COUNTER_CLR			BIT(1)
>>   #define CDC_VA_CLK_RST_CTRL_SWR_CONTROL		(0x0008)
>> +#define CDC_VA_SWR_RESET_MASK		BIT(1)
>> +#define CDC_VA_SWR_RESET_ENABLE		BIT(1)
>> +#define CDC_VA_SWR_CLK_EN_MASK		BIT(0)
>> +#define CDC_VA_SWR_CLK_ENABLE		BIT(0)
>>   #define CDC_VA_TOP_CSR_TOP_CFG0			(0x0080)
>>   #define CDC_VA_FS_BROADCAST_EN			BIT(1)
>>   #define CDC_VA_TOP_CSR_DMIC0_CTL		(0x0084)
>> @@ -66,6 +70,8 @@
>>   #define CDC_VA_TOP_CSR_SWR_MIC_CTL0		(0x00D0)
>>   #define CDC_VA_TOP_CSR_SWR_MIC_CTL1		(0x00D4)
>>   #define CDC_VA_TOP_CSR_SWR_MIC_CTL2		(0x00D8)
>> +#define CDC_VA_SWR_MIC_CLK_SEL_0_1_MASK		(0xEE)
>> +#define CDC_VA_SWR_MIC_CLK_SEL_0_1_DIV1		(0xCC)
>>   #define CDC_VA_TOP_CSR_SWR_CTRL			(0x00DC)
>>   #define CDC_VA_INP_MUX_ADC_MUX0_CFG0		(0x0100)
>>   #define CDC_VA_INP_MUX_ADC_MUX0_CFG1		(0x0104)
>> @@ -194,6 +200,8 @@ struct va_macro {
>>   	unsigned long active_ch_mask[VA_MACRO_MAX_DAIS];
>>   	unsigned long active_ch_cnt[VA_MACRO_MAX_DAIS];
>>   	u16 dmic_clk_div;
>> +	bool has_swr_master;
>> +	bool reset_swr;
>>   
>>   	int dec_mode[VA_MACRO_NUM_DECIMATORS];
>>   	struct regmap *regmap;
>> @@ -325,6 +333,9 @@ static bool va_is_rw_register(struct device *dev, unsigned int reg)
>>   	case CDC_VA_TOP_CSR_DMIC2_CTL:
>>   	case CDC_VA_TOP_CSR_DMIC3_CTL:
>>   	case CDC_VA_TOP_CSR_DMIC_CFG:
>> +	case CDC_VA_TOP_CSR_SWR_MIC_CTL0:
>> +	case CDC_VA_TOP_CSR_SWR_MIC_CTL1:
>> +	case CDC_VA_TOP_CSR_SWR_MIC_CTL2:
>>   	case CDC_VA_TOP_CSR_DEBUG_BUS:
>>   	case CDC_VA_TOP_CSR_DEBUG_EN:
>>   	case CDC_VA_TOP_CSR_TX_I2S_CTL:
>> @@ -1306,12 +1317,40 @@ static const struct snd_soc_component_driver va_macro_component_drv = {
>>   
>>   static int fsgen_gate_enable(struct clk_hw *hw)
>>   {
>> -	return va_macro_mclk_enable(to_va_macro(hw), true);
>> +	struct va_macro *va = to_va_macro(hw);
>> +	struct regmap *regmap = va->regmap;
>> +	int ret;
>> +
>> +	ret = va_macro_mclk_enable(va, true);
>> +	if (!va->has_swr_master)
>> +		return ret;
>> +
>> +	if (va->reset_swr)
>> +		regmap_update_bits(regmap, CDC_VA_CLK_RST_CTRL_SWR_CONTROL,
>> +				   CDC_VA_SWR_RESET_MASK,
>> +				   CDC_VA_SWR_RESET_ENABLE);
>> +
>> +	regmap_update_bits(regmap, CDC_VA_CLK_RST_CTRL_SWR_CONTROL,
>> +			   CDC_VA_SWR_CLK_EN_MASK,
>> +			   CDC_VA_SWR_CLK_ENABLE);
>> +	if (va->reset_swr)
>> +		regmap_update_bits(regmap, CDC_VA_CLK_RST_CTRL_SWR_CONTROL,
>> +				   CDC_VA_SWR_RESET_MASK, 0x0);
>> +	va->reset_swr = false;
>> +
>> +	return ret;
>>   }
>>   
>>   static void fsgen_gate_disable(struct clk_hw *hw)
>>   {
>> -	va_macro_mclk_enable(to_va_macro(hw), false);
>> +	struct va_macro *va = to_va_macro(hw);
>> +	struct regmap *regmap = va->regmap;
>> +
>> +	if (va->has_swr_master)
>> +		regmap_update_bits(regmap, CDC_VA_CLK_RST_CTRL_SWR_CONTROL,
>> +			   CDC_VA_SWR_CLK_EN_MASK, 0x0);
>> +
>> +	va_macro_mclk_enable(va, false);
>>   }
>>   
>>   static int fsgen_gate_is_enabled(struct clk_hw *hw)
>> @@ -1459,6 +1498,11 @@ static int va_macro_probe(struct platform_device *pdev)
>>   
>>   	dev_set_drvdata(dev, va);
>>   
>> +	if (of_device_is_compatible(dev->of_node, "qcom,sm8450-lpass-va-macro") ||
>> +		of_device_is_compatible(dev->of_node, "qcom,sc8280xp-lpass-va-macro")) {
>> +		va->has_swr_master = true;
>> +		va->reset_swr = true;
> 
> This should go to driver_data. Either via quirks/flags or device type
> (enum for each device). Usually the first (flags) is more flexible if
> you want to support many devices.

Yes, at-least this case is easily doable with driver_data, let me try to 
add this in next spin.


--srini
> 
> This also explains Rob's concerns about unneeded entries in of_device_id
> table.
> 
> 
> Best regards,
> Krzysztof