Re: [PATCH 6/6] ASoC: codecs: tx-macro: add support for sm8450 and sc8280xp

Srinivas Kandagatla
2022-09-01T08:50:46Z

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