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: <aac3942d-bc0b-7763-0d12-f5d6c585cc97@linaro.org>
Date:   Thu, 1 Sep 2022 10:30:24 +0300
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101
 Thunderbird/91.13.0
Subject: Re: [PATCH 6/6] ASoC: codecs: tx-macro: add support for sm8450 and
 sc8280xp
Content-Language: en-US
To:     Srinivas Kandagatla <srinivas.kandagatla@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>
From:   Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
In-Reply-To: <20220818134619.3432-7-srinivas.kandagatla@linaro.org>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
Precedence: bulk

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.

This also explains Rob's concerns about unneeded entries in of_device_id
table.


Best regards,
Krzysztof