[PATCH 3/6] clk: Fix error handling in clk_get_parent()

[Sean Anderson]

On 3/4/23 15:46, Michal Suchánek wrote:
> On Sat, Mar 04, 2023 at 01:58:17PM -0600, Samuel Holland wrote:
>> On 2/20/23 04:39, Michal Suchánek wrote:
>>> On Sun, Feb 19, 2023 at 11:59:36PM -0600, Samuel Holland wrote:
>>>> Do not return both NULL and error pointers. The function is only
>>>> documented as returning error pointers.
>>>>
>>>> Fixes: 8a1661f20e6c ("drivers: clk: Handle gracefully NULL pointers")
>>>> Signed-off-by: Samuel Holland <samuel at sholland.org>
>>>> ---
>>>>
>>>>   drivers/clk/clk-uclass.c | 2 +-
>>>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/clk/clk-uclass.c b/drivers/clk/clk-uclass.c
>>>> index 78299dbceb2..5bce976b060 100644
>>>> --- a/drivers/clk/clk-uclass.c
>>>> +++ b/drivers/clk/clk-uclass.c
>>>> @@ -490,7 +490,7 @@ struct clk *clk_get_parent(struct clk *clk)
>>>>   
>>>>   	debug("%s(clk=%p)\n", __func__, clk);
>>>>   	if (!clk_valid(clk))
>>>> -		return NULL;
>>>> +		return ERR_PTR(-ENODEV);
>>>>   
>>>>   	pdev = dev_get_parent(clk->dev);
>>>>   	if (!pdev)
>>>
>>> Do we really need this?
>>>
>>> Who cares about the distinction?
>>>
>>> Just returning NULL on any error makes the code so much simpler and
>>> easier to understand.
>>
>> I'm fine with returning only NULL, or only ERR_PTR(-ENODEV) as done
>> later in the function, but returning both makes the error handling in
>> callers more complicated for no benefit. It sounds like you would prefer
>> I change the later ERR_PTR(-ENODEV) to NULL instead of this change? I
>> can do that for v2.
> 
> I think NULL is easier to check, and so long as there is no meaningful
> distinction between ERR_PTR and NULL we should converge towards NULL as
> the single error return value.

The only issue I can see is if you have e.g. an I2C clock and you need to
examine the register to determine the parent and the slave doesn't respond.
Is the distinction between "I have no parent" and "I have a parent
but I don't know which" meaningful? Linux uses one return because parents
must be determined at probe time. I'm fine with making this return NULL on
error; this should be uncommon enough that we can recommend dev_err()
reporting in the driver.

--Sean