Re: [bug report] ASoC: rsnd: add null CLOCKIN support

[Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>]

Hi Dan

Thank you for your feedback

> >  	if (IS_ERR(clk)) {
> >  		dev_err(dev, "create null clk error\n");
> > -		return NULL;
> > +		return PTR_ERR(clk);
> 
> Yes, I think this part is correct.  If an error happens, then it should
> be reported to the user so they can fix it.

Good !

> > @@ -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))
> >  			goto err;
> 
> But this is not correct.
> 
> If a function like devm_clk_get() returns NULL, then it's not an error,
> it's something where the user deliberately chose to disable the feature.
> It shouldn't trigger an error message and the rest of the driver should
> be written to accomodate it.
> 
> >  
> >  		adg->clk[i] = clk;
> 
> So we should assign the NULL pointer here and add NULL checks to make
> sure that it doesn't lead to a NULL dereference.

Ah, in this driver, if it got error or NULL clk,
it try to call rsnd_adg_null_clk_get() and use null_clk instead of NULL.
In other words, all adg->clk[i] should not NULL.
If one of them was NULL, it is error for this driver.
If so, my suggested code was OK, I hope.

Thank you for your help !!

Best regards
---
Kuninori Morimoto