On Tue, Feb 18, 2025 at 10:53:05AM +0530, Taniya Das wrote:
>
>
> 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.
Ack, thanks.
>
> >
> > 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.
Ack, thanks.
>
>
> Thanks,
> Taniya.
--
With best wishes
Dmitry
