linux-kernel - Re: [PATCH v3 2/4] clk: qcom: lpassaudiocc-sc7280: Add support for LPASS resets for QCM6490

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <30e84d27-6047-492a-8602-8822b5d492fe@quicinc.com>
Date: Tue, 18 Feb 2025 10:53:05 +0530
From: Taniya Das <quic_tdas@...cinc.com>
To: Dmitry Baryshkov <dmitry.baryshkov@...aro.org>
CC: Bjorn Andersson <andersson@...nel.org>,
        Michael Turquette
	<mturquette@...libre.com>,
        Stephen Boyd <sboyd@...nel.org>, Rob Herring
	<robh@...nel.org>,
        Krzysztof Kozlowski <krzk+dt@...nel.org>,
        Conor Dooley
	<conor+dt@...nel.org>,
        Konrad Dybcio <konradybcio@...nel.org>,
        Ajit Pandey
	<quic_ajipan@...cinc.com>,
        Imran Shaik <quic_imrashai@...cinc.com>,
        "Jagadeesh Kona" <quic_jkona@...cinc.com>,
        <linux-arm-msm@...r.kernel.org>, <linux-clk@...r.kernel.org>,
        <devicetree@...r.kernel.org>, <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v3 2/4] clk: qcom: lpassaudiocc-sc7280: Add support for
 LPASS resets for QCM6490



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.