Re: [PATCH] clk: keystone: sci-clk: Handle missing get_num_parents operation

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] clk: keystone: dead code due to invalid get_num_parents capability check
--

commit 261ca89db50e7bf2199864fc566650d47bb2b916
Author: Beleswar Padhi <b-padhi@ti.com>

clk: keystone: sci-clk: Handle missing get_num_parents operation

This commit updates the sci-clk driver to handle missing get_num_parents
operations, specifically for system controllers like PDM on TDA54 that
manage clock parents internally. It avoids invoking the parent operations by
checking for the get_num_parents capability during DT and firmware scans.

> diff --git a/drivers/clk/keystone/sci-clk.c b/drivers/clk/keystone/sci-clk.c
> index 9d2094bd48e3b..5bf615893a8c0 100644
> --- a/drivers/clk/keystone/sci-clk.c
> +++ b/drivers/clk/keystone/sci-clk.c
> @@ -468,6 +468,13 @@ static int ti_sci_scan_clocks_from_fw(struct sci_clk_provider *provider)
>  	int gap_size = 0;
>  	struct device *dev = provider->dev;
>  
> +	/*
> +	 * Clocks are discovered by probing the firmware with get_num_parents,
> +	 * which is not available with every system firmware (e.g. ABI5.0 PDM).
> +	 */
> +	if (!provider->ops->get_num_parents)
> +		return -EOPNOTSUPP;
> +

[Severity: Medium]
Is this capability check effective? The TI SCI framework unconditionally
populates the get_num_parents pointer during ti_sci_setup_ops(), so it seems
this would never be NULL.

Does this mean this early return is dead code, and the driver will instead
silently scan and register an empty clock provider when probed on a platform
with firmware lacking this support (like TDA54)? Should this instead verify
the capability by checking the return value of get_num_parents() for an error
code (e.g., -ENODEV) instead of checking the pointer?

>  	while (1) {
>  		ret = provider->ops->get_num_parents(provider->sci, dev_id,

[ ... ]

> @@ -589,10 +596,18 @@ static int ti_sci_scan_clocks_from_dt(struct sci_clk_provider *provider)
>  				sci_clk->dev_id = args.args[0];
>  				sci_clk->clk_id = args.args[1];
>  				sci_clk->provider = provider;
> -				provider->ops->get_num_parents(provider->sci,
> -							       sci_clk->dev_id,
> -							       sci_clk->clk_id,
> -							       (void *)&sci_clk->num_parents);
> +				/*
> +				 * Firmware without get_num_parents (e.g. ABI5.0
> +				 * PDM) manages clock parents internally, so
> +				 * treat the clock as having a single parent.
> +				 */
> +				if (provider->ops->get_num_parents)
> +					provider->ops->get_num_parents(provider->sci,
> +								       sci_clk->dev_id,
> +								       sci_clk->clk_id,
> +								       &sci_clk->num_parents);
> +				else
> +					sci_clk->num_parents = 1;

[Severity: Medium]
Since the function pointer is unconditionally assigned, is this else branch
dead code? Should the capability be verified dynamically by evaluating the
error code returned from the API call rather than checking the pointer?

>  				list_add_tail(&sci_clk->node, &clks);
>  
>  				num_clks++;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260930190348.11720-1-b-padhi@ti.com?part=1