Re: [PATCH v7 30/34] i2c: tegra: Clean up variable names

[Dmitry Osipenko <digetx@gmail.com>]

21.09.2020 18:50, Thierry Reding пишет:
> On Mon, Sep 21, 2020 at 06:18:59PM +0300, Dmitry Osipenko wrote:
>> 21.09.2020 14:40, Thierry Reding пишет:
>>> On Thu, Sep 17, 2020 at 06:43:28PM +0300, Dmitry Osipenko wrote:
>>>> 17.09.2020 15:21, Thierry Reding пишет:
>>>>> On Wed, Sep 09, 2020 at 01:40:02AM +0300, Dmitry Osipenko wrote:
>>>>>> Rename "ret" variables to "err" in order to make code a bit more
>>>>>> expressive, emphasizing that the returned value is an error code.
>>>>>> Same vice versa, where appropriate.
>>>>>>
>>>>>> Rename variable "reg" to "val" in order to better reflect the actual
>>>>>> usage of the variable in the code and to make naming consistent with
>>>>>> the rest of the code.
>>>>>>
>>>>>> Use briefer names for a few members of the tegra_i2c_dev structure in
>>>>>> order to improve readability of the code.
>>>>>>
>>>>>> All dev/&pdev->dev are replaced with i2c_dev->dev in order to have uniform
>>>>>> code style across the driver.
>>>>>>
>>>>>> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
>>>>>> ---
>>>>>>  drivers/i2c/busses/i2c-tegra.c | 173 ++++++++++++++++-----------------
>>>>>>  1 file changed, 86 insertions(+), 87 deletions(-)
>>>>>
>>>>> That's indeed a nice improvement. One thing did spring out at me,
>>>>> though.
>>>>>
>>>>>> diff --git a/drivers/i2c/busses/i2c-tegra.c b/drivers/i2c/busses/i2c-tegra.c
>>>>> [...]
>>>>>> @@ -1831,20 +1830,20 @@ static int __maybe_unused tegra_i2c_runtime_suspend(struct device *dev)
>>>>>>  
>>>>>>  	clk_bulk_disable(i2c_dev->nclocks, i2c_dev->clocks);
>>>>>>  
>>>>>> -	return pinctrl_pm_select_idle_state(i2c_dev->dev);
>>>>>> +	return pinctrl_pm_select_idle_state(dev);
>>>>>>  }
>>>>>>  
>>>>>>  static int __maybe_unused tegra_i2c_suspend(struct device *dev)
>>>>>>  {
>>>>>>  	struct tegra_i2c_dev *i2c_dev = dev_get_drvdata(dev);
>>>>>> -	int err = 0;
>>>>>> +	int ret = 0;
>>>>>>  
>>>>>>  	i2c_mark_adapter_suspended(&i2c_dev->adapter);
>>>>>>  
>>>>>>  	if (!pm_runtime_status_suspended(dev))
>>>>>> -		err = tegra_i2c_runtime_suspend(dev);
>>>>>> +		ret = tegra_i2c_runtime_suspend(dev);
>>>>>>  
>>>>>> -	return err;
>>>>>> +	return ret;
>>>>>>  }
>>>>>
>>>>> Isn't this exactly the opposite of what the commit message says (and the
>>>>> rest of the patch does)?
>>>>
>>>> This change makes it to be consistent with the rest of the code. You may
>>>> notice that "Factor out hardware initialization into separate function"
>>>> made a similar change.
>>>>
>>>> The reason I'm doing this is that the "err" suggests that code returns a
>>>> error failure code, while it could be a success too and you don't know
>>>> for sure by looking only at the part of code. Hence it's cleaner to use
>>>> "err" when error code is returned.
>>>
>>> I don't follow that reasoning. Every error code obviously also has a
>>> value for success. Otherwise, what's the point of even having a function
>>> if all it can do is fail. Success has to be an option for code to be any
>>> useful at all, right?
>>>
>>> The "err" variable here transports the error code and if that error code
>>> happens to be 0 (meaning success), why does that no longer qualify as an
>>> error code?
>>
>> If you're naming variable as "err", then this implies to me that it will
>> contain a error code if error variable is returned directly. Error
>> shouldn't relate to a success. In practice nobody pays much attention to
>> variable naming, so usually there is a need to check what code actually
>> does anyways. I don't care much about this and just wanting to make a
>> minor improvement while at it.
> 
> Oh... I think I get what you're trying to do here now. You're saying
> that we may be storing a positive success result in this variable and
> therefore it would be wrong to call it "error", right?
> 
> And I always thought I was pedantic... =)
> 
> The way I see it, any success value can still be considered an error
> code. Typically you either propagate the value immediately for errors or
> you just ignore it on success. In that case, keeping it in a variable a
> bit beyond the assignment isn't a big issue. What matters is that you
> don't use it. There are some exceptions where this can look weird, such
> as:
> 
> 	err = platform_get_irq(pdev, 0);
> 	if (err < 0)
> 		return err;
> 
> 	chip->irq = err;
> 
> Although I think that's still okay and can be useful for example if
> chip->irq is an unsigned int, and hence you can't do:
> 
> 	chip->irq = platform_get_irq(pdev, 0);
> 	if (chip->irq < 0)
> 		return chip->irq;
> 
> My main gripe with variables named "ret" or "retval" is that I often see
> them not used as return value at all. Or the other extreme is that every
> variable is at some point a return value if it stores the result of a
> function call. So I think "ret" is just fundamentally a bad choice. But
> I also realize that that's very subjective.
> 
> Anyway, I would personally lean towards calling all these "err" instead
> of "ret", but I think consistency trumps personal preference, so I would
> not object to "ret" generally. But I think it's a bit extreme to use err
> everywhere else and use "ret" only when we don't immediately return the
> error code because I think that's just too subtle of a difference to
> make up for the inconsistency.
> 
> On the other hand, we've spent way too much time discussing this, so
> just pick whatever you want:
> 
> Acked-by: Thierry Reding <treding@nvidia.com>
> 

Thanks, I'll change it to make tegra_i2c_suspend() to use the same style
as tegra_i2c_resume() uses, which should be the best option in this case.