On 5/6/25 15:24, Leo Yan wrote:
On Fri, May 02, 2025 at 11:40:31AM +0530, Anshuman Khandual wrote:
Even though this might seem to be being bike shedding, the subject line above could be re-organized something like the following for better clarity.
coresight: Properly/Appropriately disable programming clocks
Sure. I will change the subject to this.
[...]
@@ -725,8 +723,6 @@ static void debug_platform_remove(struct platform_device *pdev) __debug_remove(&pdev->dev); pm_runtime_disable(&pdev->dev);
- if (!IS_ERR_OR_NULL(drvdata->pclk))
clk_put(drvdata->pclk);}
Should not these IS_ERR_OR_NULL() here be changed to IS_ERR() ?
For the case above, after changed to devm_clk_get_enabled() for the enabling programming clocks, we don't need any special handling and leave the clock disabling and releasing to the device model layer.
So it can be left unchanged for now and cleaned up later ?
Because now there could not be a NULL return value.
drvdata->pclk = coresight_get_enable_apb_pclk(&pdev->dev)
#ifdef CONFIG_PM static int debug_runtime_suspend(struct device *dev) { struct debug_drvdata *drvdata = dev_get_drvdata(dev);
if (drvdata && !IS_ERR_OR_NULL(drvdata->pclk)) clk_disable_unprepare(drvdata->pclk); return 0;}
static int debug_runtime_resume(struct device *dev) { struct debug_drvdata *drvdata = dev_get_drvdata(dev);
if (drvdata && !IS_ERR_OR_NULL(drvdata->pclk)) clk_prepare_enable(drvdata->pclk); return 0;} #endif
There might more instances like these as well. git grep IS_ERR_OR_NULL drivers/hwtracing/coresight/ | grep "drvdata->pclk" drivers/hwtracing/coresight/coresight-cpu-debug.c: if (drvdata && !IS_ERR_OR_NULL(drvdata->pclk)) drivers/hwtracing/coresight/coresight-cpu-debug.c: if (drvdata && !IS_ERR_OR_NULL(drvdata->pclk)) drivers/hwtracing/coresight/coresight-funnel.c: if (drvdata && !IS_ERR_OR_NULL(drvdata->pclk)) drivers/hwtracing/coresight/coresight-funnel.c: if (drvdata && !IS_ERR_OR_NULL(drvdata->pclk)) drivers/hwtracing/coresight/coresight-replicator.c: if (drvdata && !IS_ERR_OR_NULL(drvdata->pclk)) drivers/hwtracing/coresight/coresight-replicator.c: if (drvdata && !IS_ERR_OR_NULL(drvdata->pclk)) drivers/hwtracing/coresight/coresight-stm.c: if (drvdata && !IS_ERR_OR_NULL(drvdata->pclk)) drivers/hwtracing/coresight/coresight-stm.c: if (drvdata && !IS_ERR_OR_NULL(drvdata->pclk)) drivers/hwtracing/coresight/coresight-tpiu.c: if (drvdata && !IS_ERR_OR_NULL(drvdata->pclk)) drivers/hwtracing/coresight/coresight-tpiu.c: if (drvdata && !IS_ERR_OR_NULL(drvdata->pclk))
I would like the current patch to focus on the issue of disabling / releasing the programming clocks.
Though the IS_ERR_OR_NULL() check is redundant, it does not cause issue or regression. The refactoring is left in patch 09 for removing IS_ERR_OR_NULL() checks.
Does this make sense?
Yes, it does now.