Re: [PATCH v2 12/12] media: fsd-csis: Add support for FSD CSIS DMA

[Krzysztof Kozlowski <krzk@kernel.org>]

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