Re: [PATCH] ASoC: rsnd: adg: clearly handle clock error / NULL case

[Dan Carpenter <dan.carpenter@xxxxxxxxxx>]

On Fri, Jun 11, 2021 at 08:23:28AM +0900, Kuninori Morimoto wrote:
> 
> From: Kuninori Morimoto <kuninori.morimoto.gx@xxxxxxxxxxx>
> 
> This driver is assuming that all adg->clk[i] is not NULL.
> Because of this prerequisites, for_each_rsnd_clk() is possible to work
> for all clk without checking NULL. In other words, all adg->clk[i]
> should not NULL.
> 
> Some SoC might doesn't have clk_a/b/c/i. devm_clk_get() returns error in
> such case. This driver calls rsnd_adg_null_clk_get() and use null_clk
> instead of NULL in such cases.
> 
> But devm_clk_get() might returns NULL even though such clocks exist, but
> it doesn't mean error (user deliberately chose to disable the feature).
> NULL clk itself is not error from clk point of view, but is error from
> this driver point of view because it is not assuming such case.
> 
> But current code is using IS_ERR() which doesn't care NULL.
> This driver uses IS_ERR_OR_NULL() instead of IS_ERR() for clk check.
> And it uses ERR_CAST() to clarify null_clk error.
> 
> One concern here is that it unconditionally uses null_clk if clk_a/b/c/i
> was error. It is correct if it doesn't exist, but is not correct if it
> returns error even though it exist.
> It needs to check "clock-names" from DT before calling devm_clk_get() to
> handling such case. But let's assume it is overkill so far.
> 
> Link: https://lore.kernel.org/r/YMCmhfQUimHCSH/n@mwanda 
> Reported-by: Dan Carpenter <dan.carpenter@xxxxxxxxxx>
> Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@xxxxxxxxxxx>
> ---
>  sound/soc/sh/rcar/adg.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/sound/soc/sh/rcar/adg.c b/sound/soc/sh/rcar/adg.c
> index 0ebee1ed06a9..abe9d539709b 100644
> --- a/sound/soc/sh/rcar/adg.c
> +++ b/sound/soc/sh/rcar/adg.c
> @@ -393,7 +393,7 @@ static struct clk *rsnd_adg_create_null_clk(struct rsnd_priv *priv,
>  	clk = clk_register_fixed_rate(dev, name, parent, 0, 0);
>  	if (IS_ERR(clk)) {
>  		dev_err(dev, "create null clk error\n");
> -		return NULL;
> +		return ERR_CAST(clk);
>  	}
>  
>  	return clk;
> @@ -430,9 +430,9 @@ static int rsnd_adg_get_clkin(struct rsnd_priv *priv)
>  	for (i = 0; i < CLKMAX; i++) {
>  		clk = devm_clk_get(dev, clk_name[i]);
>  
> -		if (IS_ERR(clk))
> +		if (IS_ERR_OR_NULL(clk))
>  			clk = rsnd_adg_null_clk_get(priv);
> -		if (IS_ERR(clk))
> +		if (IS_ERR_OR_NULL(clk))

"clk" can't be NULL here, right?  So this should just be:

	if (IS_ERR(clk))

(because when a function returns NULL it shouldn't print an error)

regards,
dan carpenter