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@xxxxxxxxx>
>>>>>> ---
>>>>>> 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@xxxxxxxxxx>
>
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.
