On 23/08/2025 13:49, Inbaraj E wrote:
>>
>>> +
>>> + ret = fsd_csis_clk_get(csis);
>>> + if (ret < 0)
>>> + return ret;
>>> +
>>> + pm_runtime_enable(dev);
>>> + if (!pm_runtime_enabled(dev)) {
>>
>> That's odd code. Why?
>>
>>> + ret = fsd_csis_runtime_resume(dev);
>>
>> Even more questions why?
>
> If CONFIG_PM is enabled, the clocks are enabled manually in the
> driver through fsd_csis_runtime_resume API.
>
> If CONFIG_PM is enabled, the clocks are managed through the PM
> runtime framework.
>
> Can you please help me understand what wrong here?
I think I see such code for the first time, so wrong is doing something
common in completely unusual way.
>
>>
>>> + if (ret < 0)
>>> + return ret;
>>> + }
>>> +
>>> + platform_set_drvdata(pdev, csis);
>>> +
>>> + ret = fsd_csis_enable_pll(csis);
>>> + if (ret)
>>> + return ret;
>>> +
>>> + ret = fsd_csis_media_init(csis);
>>> + if (ret)
>>> + return ret;
>>
>> I think you miss clean up of csis->pll completely. Just use
>> devm_clk_get_enabled and convert everything here to devm.
>>
>>
>
> I'll fix in next patchset.
>
>>> +
>>> + ret = fsd_csis_async_register(csis);
>>> + if (ret)
>>> + goto err_media_cleanup;
>>> +
>>> + return 0;
>>> +
>>> +err_media_cleanup:
>>> + fsd_csis_media_cleanup(csis);
>>
>> Also this...
>>
>
> if fsd_csis_media_init fails, the cleanup is handled internally.
What does it mean internally?
> Here, cleanup is used only for fsd_csis_async_register failure.
>
> can you please help me understand what is wrong here?
Yeah, you leak clock resources.
Best regards,
Krzysztof