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
