[PATCH 1/1] clk: Document of_clk_get_by_name() return values

[PATCH 1/1] clk: Document of_clk_get_by_name() return values

[Dan Carpenter]

Callers should test the return from of_clk_get_by_name() with IS_ERR().
The function returns a valid clock on success and an error pointer on
failure; NULL is not a valid return value.

Document this explicitly to prevent callers from treating NULL as a
separate failure case.

Assisted-by: ChatGPT:gpt-5
Signed-off-by: Dan Carpenter <error27@gmail.com>
---
There are a few other functions which look like they return NULL but
never actually do.  This is one which has caused some confusion in
the past.

 drivers/clk/clk.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index f1756fe59372..a71bcf780de4 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -5430,6 +5430,9 @@ EXPORT_SYMBOL(of_clk_get);
  * This function parses the clocks and clock-names properties,
  * and uses them to look up the struct clk from the registered list of clock
  * providers.
+ *
+ * Returns: A clock pointer on success or an error pointer on failure.  This
+ * function never returns NULL.
  */
 struct clk *of_clk_get_by_name(struct device_node *np, const char *name)
 {
-- 
2.53.0

Re: [PATCH 1/1] clk: Document of_clk_get_by_name() return values

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] clk: false claim in of_clk_get_by_name() documentation
--

commit d09131dbb97111f2e3801d98bcbd5fbb8288397d
Author: Dan Carpenter <error27@gmail.com>

clk: Document of_clk_get_by_name() return values

This commit documents the return values for of_clk_get_by_name(), stating
that it returns a valid clock on success or an error pointer on failure. It
also explicitly claims that the function never returns NULL.

> diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
> index fef87167a60b5..7e1a88c3837a3 100644
> --- a/drivers/clk/clk.c
> +++ b/drivers/clk/clk.c
> @@ -5420,6 +5420,9 @@ EXPORT_SYMBOL(of_clk_get);
>   * This function parses the clocks and clock-names properties,
>   * and uses them to look up the struct clk from the registered list of clock
>   * providers.
> + *
> + * Returns: A clock pointer on success or an error pointer on failure.  This
> + * function never returns NULL.
>   */
>  struct clk *of_clk_get_by_name(struct device_node *np, const char *name)
>  {

[Severity: Medium]
Is it accurate that this function never returns NULL?

It appears of_clk_get_by_name() delegates to of_clk_get_hw_from_clkspec(),
which invokes the clock provider's get_hw callback. Providers often use
of_clk_hw_onecell_get() with sparse clock arrays (such as
struct clk_hw_onecell_data) where uninitialized entries are NULL.

When such an entry is requested, NULL propagates back through
clk_hw_create_clk() (which uses ERR_CAST(NULL)), causing
of_clk_get_by_name() to return NULL on success.

Could this documentation mislead driver developers into removing NULL checks
or introducing assertions (e.g., BUG_ON(!clk)) that will crash the kernel
when a valid dummy clock is returned?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/b3de48fdbaaade69cce126b59bd6dfb8f437b495.1789451862.git.error27@gmail.com?part=1

Re: [PATCH 1/1] clk: Document of_clk_get_by_name() return values

[Dan Carpenter]

On Tue, Sep 15, 2026 at 07:38:23PM +0300, Dan Carpenter wrote:
> Callers should test the return from of_clk_get_by_name() with IS_ERR().
> The function returns a valid clock on success and an error pointer on
> failure; NULL is not a valid return value.
> 
> Document this explicitly to prevent callers from treating NULL as a
> separate failure case.
> 
> Assisted-by: ChatGPT:gpt-5
> Signed-off-by: Dan Carpenter <error27@gmail.com>
> ---
> There are a few other functions which look like they return NULL but
> never actually do.  This is one which has caused some confusion in
> the past.

Sashiko says that it looks like it can return NULL.
https://lore.kernel.org/all/20260915164913.597D51F000FF@smtp.kernel.org/
It does *look* that way, which is why I'm adding the documentation
to say that looks are confusing.

It's pretty normal for these types of functions to return an error pointer
for errors and NULL for not found, but the of_clk_get_by_name() returns
-ENOENT on error.  https://lkml.iu.edu/1810.1/06936.html

Also in commit 6a636d203cc8 ("clk: renesas: Use IS_ERR() for pointers
that cannot be NULL") there was a static checker warning because the
NULL return wasn't handled and we "fixed" it by saying that NULL was
impossible.

regards,
dan carpenter

Re: [PATCH 1/1] clk: Document of_clk_get_by_name() return values

[Dan Carpenter]

On Tue, Sep 15, 2026 at 08:53:48PM +0300, Dan Carpenter wrote:
> On Tue, Sep 15, 2026 at 07:38:23PM +0300, Dan Carpenter wrote:
> > Callers should test the return from of_clk_get_by_name() with IS_ERR().
> > The function returns a valid clock on success and an error pointer on
> > failure; NULL is not a valid return value.
> > 
> > Document this explicitly to prevent callers from treating NULL as a
> > separate failure case.
> > 
> > Assisted-by: ChatGPT:gpt-5
> > Signed-off-by: Dan Carpenter <error27@gmail.com>
> > ---
> > There are a few other functions which look like they return NULL but
> > never actually do.  This is one which has caused some confusion in
> > the past.
> 
> Sashiko says that it looks like it can return NULL.
> https://lore.kernel.org/all/20260915164913.597D51F000FF@smtp.kernel.org/
> It does *look* that way, which is why I'm adding the documentation
> to say that looks are confusing.
> 
> It's pretty normal for these types of functions to return an error pointer
> for errors and NULL for not found, but the of_clk_get_by_name() returns
> -ENOENT on error.  https://lkml.iu.edu/1810.1/06936.html

Doh.  I meant -ENOENT on not found.  Error pointers for errros and also
for not found.

regards,
dan carpenter

Re: [PATCH 1/1] clk: Document of_clk_get_by_name() return values

[Jerome Brunet]

On mar. 15 sept. 2026 at 19:38, Dan Carpenter <error27@gmail.com> wrote:

> Callers should test the return from of_clk_get_by_name() with IS_ERR().
> The function returns a valid clock on success and an error pointer on
> failure; NULL is not a valid return value.
>
> Document this explicitly to prevent callers from treating NULL as a
> separate failure case.
>
> Assisted-by: ChatGPT:gpt-5
> Signed-off-by: Dan Carpenter <error27@gmail.com>
> ---
> There are a few other functions which look like they return NULL but
> never actually do.  This is one which has caused some confusion in
> the past.
>
>  drivers/clk/clk.c | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
> index f1756fe59372..a71bcf780de4 100644
> --- a/drivers/clk/clk.c
> +++ b/drivers/clk/clk.c
> @@ -5430,6 +5430,9 @@ EXPORT_SYMBOL(of_clk_get);
>   * This function parses the clocks and clock-names properties,
>   * and uses them to look up the struct clk from the registered list of clock
>   * providers.
> + *
> + * Returns: A clock pointer on success or an error pointer on failure.  This
> + * function never returns NULL.

Thanks Dan. I was about to apply the change but it feels a bit strange
to document what a function never does.

Is this a documentation that should be added everywhere the return value
is to be tested with IS_ERR() ?

What about being more direct then:

"Returns: A clock pointer on success or an error pointer on failure.
 Caller should test the return value with IS_ERR()"

?

>   */
>  struct clk *of_clk_get_by_name(struct device_node *np, const char *name)
>  {
> -- 
> 2.53.0
>

-- 
Jerome

Re: [PATCH 1/1] clk: Document of_clk_get_by_name() return values

[Dan Carpenter]

On Wed, Sep 16, 2026 at 09:28:56AM +0200, Jerome Brunet wrote:
> On mar. 15 sept. 2026 at 19:38, Dan Carpenter <error27@gmail.com> wrote:
> 
> > Callers should test the return from of_clk_get_by_name() with IS_ERR().
> > The function returns a valid clock on success and an error pointer on
> > failure; NULL is not a valid return value.
> >
> > Document this explicitly to prevent callers from treating NULL as a
> > separate failure case.
> >
> > Assisted-by: ChatGPT:gpt-5
> > Signed-off-by: Dan Carpenter <error27@gmail.com>
> > ---
> > There are a few other functions which look like they return NULL but
> > never actually do.  This is one which has caused some confusion in
> > the past.
> >
> >  drivers/clk/clk.c | 3 +++
> >  1 file changed, 3 insertions(+)
> >
> > diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
> > index f1756fe59372..a71bcf780de4 100644
> > --- a/drivers/clk/clk.c
> > +++ b/drivers/clk/clk.c
> > @@ -5430,6 +5430,9 @@ EXPORT_SYMBOL(of_clk_get);
> >   * This function parses the clocks and clock-names properties,
> >   * and uses them to look up the struct clk from the registered list of clock
> >   * providers.
> > + *
> > + * Returns: A clock pointer on success or an error pointer on failure.  This
> > + * function never returns NULL.
> 
> Thanks Dan. I was about to apply the change but it feels a bit strange
> to document what a function never does.
> 
> Is this a documentation that should be added everywhere the return value
> is to be tested with IS_ERR() ?
> 
> What about being more direct then:
> 
> "Returns: A clock pointer on success or an error pointer on failure.
>  Caller should test the return value with IS_ERR()"

First of all, I just want to confirm that actually it's true, right?
I've read the code but this isn't my background so I might have been
confused.

If it only returns an error pointer then, it's obvious that it should
only be tested with IS_ERR().  I would be fine with just saying the
first part:

Returns: A clock pointer on success or an error pointer on failure

regards,
dan carpenter

Re: [PATCH 1/1] clk: Document of_clk_get_by_name() return values

[Jerome Brunet]

On mer. 16 sept. 2026 at 10:49, Dan Carpenter <error27@gmail.com> wrote:

> On Wed, Sep 16, 2026 at 09:28:56AM +0200, Jerome Brunet wrote:
>> On mar. 15 sept. 2026 at 19:38, Dan Carpenter <error27@gmail.com> wrote:
>> 
>> > Callers should test the return from of_clk_get_by_name() with IS_ERR().
>> > The function returns a valid clock on success and an error pointer on
>> > failure; NULL is not a valid return value.
>> >
>> > Document this explicitly to prevent callers from treating NULL as a
>> > separate failure case.
>> >
>> > Assisted-by: ChatGPT:gpt-5
>> > Signed-off-by: Dan Carpenter <error27@gmail.com>
>> > ---
>> > There are a few other functions which look like they return NULL but
>> > never actually do.  This is one which has caused some confusion in
>> > the past.
>> >
>> >  drivers/clk/clk.c | 3 +++
>> >  1 file changed, 3 insertions(+)
>> >
>> > diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
>> > index f1756fe59372..a71bcf780de4 100644
>> > --- a/drivers/clk/clk.c
>> > +++ b/drivers/clk/clk.c
>> > @@ -5430,6 +5430,9 @@ EXPORT_SYMBOL(of_clk_get);
>> >   * This function parses the clocks and clock-names properties,
>> >   * and uses them to look up the struct clk from the registered list of clock
>> >   * providers.
>> > + *
>> > + * Returns: A clock pointer on success or an error pointer on failure.  This
>> > + * function never returns NULL.
>> 
>> Thanks Dan. I was about to apply the change but it feels a bit strange
>> to document what a function never does.
>> 
>> Is this a documentation that should be added everywhere the return value
>> is to be tested with IS_ERR() ?
>> 
>> What about being more direct then:
>> 
>> "Returns: A clock pointer on success or an error pointer on failure.
>>  Caller should test the return value with IS_ERR()"
>
> First of all, I just want to confirm that actually it's true, right?
> I've read the code but this isn't my background so I might have been
> confused.

I think you got it right. Instead of NULL, it should return
ERR_PTR(-ENOENT).

Looking more closely, It is not entirely impossible to get NULL. If a
provider returns NULL instead of ERR_PTR(-ENOENT), we will just pass it
back.

clk_hw_create_clk() has this

	if (IS_ERR_OR_NULL(hw))
		return ERR_CAST(hw);

I could turn it into

	if (!hw)
		return ERR_PTR(-ENOENT);
        else if (IS_ERR(hw))
		return ERR_CAST(hw);

>
> If it only returns an error pointer then, it's obvious that it should
> only be tested with IS_ERR().  I would be fine with just saying the
> first part:
>
> Returns: A clock pointer on success or an error pointer on failure

Even better. 

>
> regards,
> dan carpenter
>

-- 
Jerome