On 2/13/2025 7:58 PM, Dmitry Baryshkov wrote:
>>>>>> static const struct qcom_cc_desc lpass_audio_cc_reset_sc7280_desc = {
>>>>>> - .config = &lpass_audio_cc_sc7280_regmap_config,
>>>>>> + .config = &lpass_audio_cc_sc7280_reset_regmap_config,
>>>>>> .resets = lpass_audio_cc_sc7280_resets,
>>>>>> .num_resets = ARRAY_SIZE(lpass_audio_cc_sc7280_resets),
>>>>>> };
>>>>>>
>>>>>> static const struct of_device_id lpass_audio_cc_sc7280_match_table[] = {
>>>>>> - { .compatible = "qcom,sc7280-lpassaudiocc" },
>>>>>> + { .compatible = "qcom,qcm6490-lpassaudiocc", .data = &lpass_audio_cc_reset_sc7280_desc },
>>>>>> + { .compatible = "qcom,sc7280-lpassaudiocc", .data = &lpass_audio_cc_sc7280_desc },
>>>>>> { }
>>>>>> };
>>>>>> MODULE_DEVICE_TABLE(of, lpass_audio_cc_sc7280_match_table);
>>>>>> @@ -752,13 +763,17 @@ static int lpass_audio_cc_sc7280_probe(struct platform_device *pdev)
>>>>>> struct regmap *regmap;
>>>>>> int ret;
>>>>>>
>>>>>> + desc = device_get_match_data(&pdev->dev);
>>>>>> +
>>>>>> + if (desc->num_resets)
>>>>>> + return qcom_cc_probe_by_index(pdev, 1, desc);
>>>>> Won't this break SC7280 support by causing an early return?
>>>>>
>>>> The resets are not defined for SC7280.
>>>> static const struct qcom_cc_desc lpass_audio_cc_sc7280_desc = {
>>>> .config = &lpass_audio_cc_sc7280_regmap_config,
>>>> .clks = lpass_audio_cc_sc7280_clocks,
>>>> .num_clks = ARRAY_SIZE(lpass_audio_cc_sc7280_clocks),
>>>> };
>>>>
>>>> The reset get registered for SC7280 after the clocks are registered.
>>>> qcom_cc_probe_by_index(pdev, 1, &lpass_audio_cc_reset_sc7280_desc);
>>> Could you please make this condition more obvious and error-prone
>>> rather than checking one particular non-obvious property?
>>>
>> Dmitry, we had earlier tried [1], but seems like we could not align on
>> this patchset.
>>
>> If you are aligned, please let me know I can fall back on the approach.
> You have been using of_device_is_compatible(). Krzysztof suggested
> using mach data. Both approaches are fine with me (I'm sorry,
> Krzysztof, this is a clock driver for a single platform, it doesn't
> need to scale).
>
> You've settled on the second one. So far so good.
Sure, I will go ahead with the existing approach, but ensure I replace
the num_resets check with the of_device_is_compatible(), so it is more
readable. Hope this aligns with your thoughts as well.
>
> But! The problem is in readability. Checking for desc->num_resets is a
> _hidden_ or cryptic way of checking whether to register only a first
> controller or both.
>
> BTW: the commit message also tells nothing about the dropped power
> domain and skipped PM code. Is it not required anymore? Is it handled
> automatically by the firmware? But I see that audio codecs still use
> that power domain.
Yes, it will be taken care in the firmware and I will update in the
commit text.
Thanks,
Taniya.
