Hi Miquèl, > > index aebb19f1b3a4..1d545879c000 100644 > > --- a/drivers/net/ieee802154/ca8210.c > > +++ b/drivers/net/ieee802154/ca8210.c > > @@ -2760,6 +2760,7 @@ static int ca8210_register_ext_clock(struct spi_device *spi) > > ret = of_clk_add_provider(np, of_clk_src_simple_get, priv->clk); > > if (ret) { > > clk_unregister(priv->clk); > > + priv->clk = NULL; > > This function is a bit convoluted. You could just return the result of > of_clk_add_provider() (keep the printk's if you want, they don't seem > very useful) and let ca8210_unregister_ext_clock() do the cleanup. Thanks for your advice! I will resend a new patch as suggested. > > > dev_crit( > > &spi->dev, > > "Failed to register external clock as clock provider\n" > > @@ -2780,7 +2781,7 @@ static void ca8210_unregister_ext_clock(struct spi_device *spi) > > { > > struct ca8210_priv *priv = spi_get_drvdata(spi); > > > > - if (!priv->clk) > > + if (IS_ERR_OR_NULL(priv->clk)) > > Does not look useful as you are enforcing priv->clock to be valid or > NULL, it cannot be an error code. I find that ca8210_register_ext_clock() uses IS_ERR to check priv->clk after calling clk_register_fixed_rate(). So I think priv->clk could be a non-null pointer even on failure. And a null pointer check may miss this case in ca8210_unregister_ext_clock(). Regards, Dinghao