diff mbox series

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

Message ID 20200908224006.25636-31-digetx@gmail.com
State New
Headers show
Series Improvements for Tegra I2C driver | expand

Checks

Context Check Description
tagr/GVS success 1124251
tagr/GVS pending 1124251
tagr/GVS success 1122533
tagr/GVS pending 1122533
tagr/GVS-1122491 fail None
tagr/GVS-1122491 pending None

Commit Message

Dmitry Osipenko Sept. 8, 2020, 10:40 p.m. UTC
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(-)

Comments

Thierry Reding Sept. 17, 2020, 12:21 p.m. UTC | #1
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)?

Thierry
Dmitry Osipenko Sept. 17, 2020, 3:43 p.m. UTC | #2
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.

It is possible (and maybe even better) to rewrite this function as:

static int __maybe_unused tegra_i2c_suspend(struct device *dev)
{
	struct tegra_i2c_dev *i2c_dev = dev_get_drvdata(dev);

	i2c_mark_adapter_suspended(&i2c_dev->adapter);

	if (!pm_runtime_status_suspended(dev)) {
		int err = tegra_i2c_runtime_suspend(dev);
		if (err)
			return err;
	}

	return 0;
}
Thierry Reding Sept. 21, 2020, 10:20 a.m. UTC | #3
On Wed, 09 Sep 2020 01:40:02 +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(-)

Tested-by: Thierry Reding <treding@nvidia.com>
Thierry Reding Sept. 21, 2020, 11:40 a.m. UTC | #4
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?

Thierry
Dmitry Osipenko Sept. 21, 2020, 3:18 p.m. UTC | #5
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.
Thierry Reding Sept. 21, 2020, 3:50 p.m. UTC | #6
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>
Dmitry Osipenko Sept. 21, 2020, 4:05 p.m. UTC | #7
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.
diff mbox series

Patch

diff --git a/drivers/i2c/busses/i2c-tegra.c b/drivers/i2c/busses/i2c-tegra.c
index 2376f502d299..fbdb206f0161 100644
--- a/drivers/i2c/busses/i2c-tegra.c
+++ b/drivers/i2c/busses/i2c-tegra.c
@@ -247,15 +247,15 @@  struct tegra_i2c_hw_feature {
  * @msg_buf_remaining: size of unsent data in the message buffer
  * @msg_read: identifies read transfers
  * @bus_clk_rate: current I2C bus clock rate
- * @is_multimaster_mode: track if I2C controller is in multi-master mode
+ * @multimaster_mode: indicates that I2C controller is in multi-master mode
  * @tx_dma_chan: DMA transmit channel
  * @rx_dma_chan: DMA receive channel
  * @dma_phys: handle to DMA resources
  * @dma_buf: pointer to allocated DMA buffer
  * @dma_buf_size: DMA buffer size
- * @is_curr_dma_xfer: indicates active DMA transfer
+ * @dma_mode: indicates active DMA transfer
  * @dma_complete: DMA completion notifier
- * @is_curr_atomic_xfer: indicates active atomic transfer
+ * @atomic_mode: indicates active atomic transfer
  */
 struct tegra_i2c_dev {
 	struct device *dev;
@@ -287,9 +287,9 @@  struct tegra_i2c_dev {
 	dma_addr_t dma_phys;
 	u32 *dma_buf;
 
-	bool is_multimaster_mode;
-	bool is_curr_atomic_xfer;
-	bool is_curr_dma_xfer;
+	bool multimaster_mode;
+	bool atomic_mode;
+	bool dma_mode;
 	bool msg_read;
 	bool is_dvc;
 	bool is_vi;
@@ -525,7 +525,7 @@  static int tegra_i2c_poll_register(struct tegra_i2c_dev *i2c_dev,
 	void __iomem *addr = i2c_dev->base + tegra_i2c_reg_addr(i2c_dev, reg);
 	u32 val;
 
-	if (!i2c_dev->is_curr_atomic_xfer)
+	if (!i2c_dev->atomic_mode)
 		return readl_relaxed_poll_timeout(addr, val, !(val & mask),
 						  delay_us, timeout_us);
 
@@ -673,7 +673,7 @@  static int tegra_i2c_init(struct tegra_i2c_dev *i2c_dev)
 	if (err)
 		return err;
 
-	if (i2c_dev->is_multimaster_mode && i2c_dev->hw->has_slcg_override_reg)
+	if (i2c_dev->multimaster_mode && i2c_dev->hw->has_slcg_override_reg)
 		i2c_writel(i2c_dev, I2C_MST_CORE_CLKEN_OVR, I2C_CLKEN_OVERRIDE);
 
 	err = tegra_i2c_wait_for_config_load(i2c_dev);
@@ -859,7 +859,7 @@  static irqreturn_t tegra_i2c_isr(int irq, void *dev_id)
 	if (i2c_dev->hw->supports_bus_clear && (status & I2C_INT_BUS_CLR_DONE))
 		goto err;
 
-	if (!i2c_dev->is_curr_dma_xfer) {
+	if (!i2c_dev->dma_mode) {
 		if (i2c_dev->msg_read && (status & I2C_INT_RX_FIFO_DATA_REQ)) {
 			if (tegra_i2c_empty_rx_fifo(i2c_dev)) {
 				/*
@@ -893,7 +893,7 @@  static irqreturn_t tegra_i2c_isr(int irq, void *dev_id)
 	 * so forcing msg_buf_remaining to 0 in DMA mode.
 	 */
 	if (status & I2C_INT_PACKET_XFER_COMPLETE) {
-		if (i2c_dev->is_curr_dma_xfer)
+		if (i2c_dev->dma_mode)
 			i2c_dev->msg_buf_remaining = 0;
 		/*
 		 * Underflow error condition: XFER_COMPLETE before message
@@ -917,7 +917,7 @@  static irqreturn_t tegra_i2c_isr(int irq, void *dev_id)
 	if (i2c_dev->is_dvc)
 		dvc_writel(i2c_dev, DVC_STATUS_I2C_DONE_INTR, DVC_STATUS);
 
-	if (i2c_dev->is_curr_dma_xfer) {
+	if (i2c_dev->dma_mode) {
 		if (i2c_dev->msg_read)
 			dmaengine_terminate_async(i2c_dev->rx_dma_chan);
 		else
@@ -936,14 +936,14 @@  static int tegra_i2c_config_fifo_trig(struct tegra_i2c_dev *i2c_dev, size_t len)
 	struct dma_slave_config slv_config = {0};
 	u32 val, reg, dma_burst, reg_offset;
 	struct dma_chan *chan;
-	int ret;
+	int err;
 
 	if (i2c_dev->hw->has_mst_fifo)
 		reg = I2C_MST_FIFO_CONTROL;
 	else
 		reg = I2C_FIFO_CONTROL;
 
-	if (i2c_dev->is_curr_dma_xfer) {
+	if (i2c_dev->dma_mode) {
 		if (len & 0xF)
 			dma_burst = 1;
 		else if (len & 0x10)
@@ -976,11 +976,11 @@  static int tegra_i2c_config_fifo_trig(struct tegra_i2c_dev *i2c_dev, size_t len)
 		}
 
 		slv_config.device_fc = true;
-		ret = dmaengine_slave_config(chan, &slv_config);
-		if (ret) {
+		err = dmaengine_slave_config(chan, &slv_config);
+		if (err) {
 			dev_err(i2c_dev->dev, "DMA slave config failed: %d\n",
-				ret);
-			return ret;
+				err);
+			return err;
 		}
 
 		goto out;
@@ -1030,7 +1030,7 @@  static unsigned long tegra_i2c_wait_completion(struct tegra_i2c_dev *i2c_dev,
 {
 	unsigned long ret;
 
-	if (i2c_dev->is_curr_atomic_xfer) {
+	if (i2c_dev->atomic_mode) {
 		ret = tegra_i2c_poll_completion(i2c_dev, complete, timeout_ms);
 	} else {
 		enable_irq(i2c_dev->irq);
@@ -1058,20 +1058,20 @@  static unsigned long tegra_i2c_wait_completion(struct tegra_i2c_dev *i2c_dev,
 static int tegra_i2c_issue_bus_clear(struct i2c_adapter *adap)
 {
 	struct tegra_i2c_dev *i2c_dev = i2c_get_adapdata(adap);
-	u32 reg, time_left;
+	u32 val, time_left;
 	int err;
 
 	reinit_completion(&i2c_dev->msg_complete);
-	reg = FIELD_PREP(I2C_BC_SCLK_THRESHOLD, 9) | I2C_BC_STOP_COND |
+	val = FIELD_PREP(I2C_BC_SCLK_THRESHOLD, 9) | I2C_BC_STOP_COND |
 	      I2C_BC_TERMINATE;
-	i2c_writel(i2c_dev, reg, I2C_BUS_CLEAR_CNFG);
+	i2c_writel(i2c_dev, val, I2C_BUS_CLEAR_CNFG);
 
 	err = tegra_i2c_wait_for_config_load(i2c_dev);
 	if (err)
 		return err;
 
-	reg |= I2C_BC_ENABLE;
-	i2c_writel(i2c_dev, reg, I2C_BUS_CLEAR_CNFG);
+	val |= I2C_BC_ENABLE;
+	i2c_writel(i2c_dev, val, I2C_BUS_CLEAR_CNFG);
 	tegra_i2c_unmask_irq(i2c_dev, I2C_INT_BUS_CLR_DONE);
 
 	time_left = tegra_i2c_wait_completion(i2c_dev, &i2c_dev->msg_complete, 50);
@@ -1082,8 +1082,8 @@  static int tegra_i2c_issue_bus_clear(struct i2c_adapter *adap)
 		return -ETIMEDOUT;
 	}
 
-	reg = i2c_readl(i2c_dev, I2C_BUS_CLEAR_STATUS);
-	if (!(reg & I2C_BC_STATUS)) {
+	val = i2c_readl(i2c_dev, I2C_BUS_CLEAR_STATUS);
+	if (!(val & I2C_BC_STATUS)) {
 		dev_err(i2c_dev->dev,
 			"un-recovered arbitration lost\n");
 		return -EIO;
@@ -1105,14 +1105,14 @@  static void tegra_i2c_push_packet_header(struct tegra_i2c_dev *i2c_dev,
 			FIELD_PREP(PACKET_HEADER0_CONT_ID, i2c_dev->cont_id) |
 			FIELD_PREP(PACKET_HEADER0_PACKET_ID, 1);
 
-	if (i2c_dev->is_curr_dma_xfer && !i2c_dev->msg_read)
+	if (i2c_dev->dma_mode && !i2c_dev->msg_read)
 		*dma_buf++ = packet_header;
 	else
 		i2c_writel(i2c_dev, packet_header, I2C_TX_FIFO);
 
 	packet_header = msg->len - 1;
 
-	if (i2c_dev->is_curr_dma_xfer && !i2c_dev->msg_read)
+	if (i2c_dev->dma_mode && !i2c_dev->msg_read)
 		*dma_buf++ = packet_header;
 	else
 		i2c_writel(i2c_dev, packet_header, I2C_TX_FIFO);
@@ -1137,7 +1137,7 @@  static void tegra_i2c_push_packet_header(struct tegra_i2c_dev *i2c_dev,
 	if (msg->flags & I2C_M_RD)
 		packet_header |= I2C_HEADER_READ;
 
-	if (i2c_dev->is_curr_dma_xfer && !i2c_dev->msg_read)
+	if (i2c_dev->dma_mode && !i2c_dev->msg_read)
 		*dma_buf++ = packet_header;
 	else
 		i2c_writel(i2c_dev, packet_header, I2C_TX_FIFO);
@@ -1153,7 +1153,7 @@  static int tegra_i2c_error_recover(struct tegra_i2c_dev *i2c_dev,
 
 	/* start recovery upon arbitration loss in single master mode */
 	if (i2c_dev->msg_err == I2C_ERR_ARBITRATION_LOST) {
-		if (!i2c_dev->is_multimaster_mode)
+		if (!i2c_dev->multimaster_mode)
 			return i2c_recover_bus(&i2c_dev->adapter);
 
 		return -EAGAIN;
@@ -1194,9 +1194,8 @@  static int tegra_i2c_xfer_msg(struct tegra_i2c_dev *i2c_dev,
 		xfer_size = msg->len + I2C_PACKET_HEADER_SIZE;
 
 	xfer_size = ALIGN(xfer_size, BYTES_PER_FIFO_WORD);
-	i2c_dev->is_curr_dma_xfer = (xfer_size > I2C_PIO_MODE_PREFERRED_LEN) &&
-				    i2c_dev->dma_buf &&
-				    !i2c_dev->is_curr_atomic_xfer;
+	i2c_dev->dma_mode = (xfer_size > I2C_PIO_MODE_PREFERRED_LEN) &&
+			    i2c_dev->dma_buf && !i2c_dev->atomic_mode;
 
 	err = tegra_i2c_config_fifo_trig(i2c_dev, xfer_size);
 	if (err)
@@ -1211,7 +1210,7 @@  static int tegra_i2c_xfer_msg(struct tegra_i2c_dev *i2c_dev,
 
 	int_mask = I2C_INT_NO_ACK | I2C_INT_ARBITRATION_LOST;
 	tegra_i2c_unmask_irq(i2c_dev, int_mask);
-	if (i2c_dev->is_curr_dma_xfer) {
+	if (i2c_dev->dma_mode) {
 		if (i2c_dev->msg_read) {
 			dma_sync_single_for_device(i2c_dev->dev,
 						   i2c_dev->dma_phys,
@@ -1236,7 +1235,7 @@  static int tegra_i2c_xfer_msg(struct tegra_i2c_dev *i2c_dev,
 	tegra_i2c_push_packet_header(i2c_dev, msg, end_state);
 
 	if (!i2c_dev->msg_read) {
-		if (i2c_dev->is_curr_dma_xfer) {
+		if (i2c_dev->dma_mode) {
 			memcpy(i2c_dev->dma_buf, msg->buf, msg->len);
 			dma_sync_single_for_device(i2c_dev->dev,
 						   i2c_dev->dma_phys,
@@ -1256,7 +1255,7 @@  static int tegra_i2c_xfer_msg(struct tegra_i2c_dev *i2c_dev,
 
 	if (i2c_dev->hw->has_per_pkt_xfer_complete_irq)
 		int_mask |= I2C_INT_PACKET_XFER_COMPLETE;
-	if (!i2c_dev->is_curr_dma_xfer) {
+	if (!i2c_dev->dma_mode) {
 		if (msg->flags & I2C_M_RD)
 			int_mask |= I2C_INT_RX_FIFO_DATA_REQ;
 		else if (i2c_dev->msg_buf_remaining)
@@ -1267,7 +1266,7 @@  static int tegra_i2c_xfer_msg(struct tegra_i2c_dev *i2c_dev,
 	dev_dbg(i2c_dev->dev, "unmasked irq: %02x\n",
 		i2c_readl(i2c_dev, I2C_INT_MASK));
 
-	if (i2c_dev->is_curr_dma_xfer) {
+	if (i2c_dev->dma_mode) {
 		time_left = tegra_i2c_wait_completion(i2c_dev,
 						      &i2c_dev->dma_complete,
 						      xfer_time);
@@ -1316,7 +1315,7 @@  static int tegra_i2c_xfer_msg(struct tegra_i2c_dev *i2c_dev,
 		time_left, completion_done(&i2c_dev->msg_complete),
 		i2c_dev->msg_err);
 
-	i2c_dev->is_curr_dma_xfer = false;
+	i2c_dev->dma_mode = false;
 
 	err = tegra_i2c_error_recover(i2c_dev, msg);
 	if (err)
@@ -1368,9 +1367,9 @@  static int tegra_i2c_xfer_atomic(struct i2c_adapter *adap,
 	struct tegra_i2c_dev *i2c_dev = i2c_get_adapdata(adap);
 	int ret;
 
-	i2c_dev->is_curr_atomic_xfer = true;
+	i2c_dev->atomic_mode = true;
 	ret = tegra_i2c_xfer(adap, msgs, num);
-	i2c_dev->is_curr_atomic_xfer = false;
+	i2c_dev->atomic_mode = false;
 
 	return ret;
 }
@@ -1595,15 +1594,15 @@  static void tegra_i2c_parse_dt(struct tegra_i2c_dev *i2c_dev)
 {
 	struct device_node *np = i2c_dev->dev->of_node;
 	bool multi_mode;
-	int ret;
+	int err;
 
-	ret = of_property_read_u32(np, "clock-frequency",
+	err = of_property_read_u32(np, "clock-frequency",
 				   &i2c_dev->bus_clk_rate);
-	if (ret)
+	if (err)
 		i2c_dev->bus_clk_rate = I2C_MAX_STANDARD_MODE_FREQ; /* default clock rate */
 
 	multi_mode = of_property_read_bool(np, "multi-master");
-	i2c_dev->is_multimaster_mode = multi_mode;
+	i2c_dev->multimaster_mode = multi_mode;
 
 	if (of_device_is_compatible(np, "nvidia,tegra20-i2c-dvc"))
 		i2c_dev->is_dvc = true;
@@ -1634,7 +1633,7 @@  static int tegra_i2c_init_clocks(struct tegra_i2c_dev *i2c_dev)
 		}
 	}
 
-	if (!i2c_dev->is_multimaster_mode)
+	if (!i2c_dev->multimaster_mode)
 		return 0;
 
 	err = clk_enable(i2c_dev->div_clk);
@@ -1653,7 +1652,7 @@  static int tegra_i2c_init_clocks(struct tegra_i2c_dev *i2c_dev)
 
 static void tegra_i2c_release_clocks(struct tegra_i2c_dev *i2c_dev)
 {
-	if (i2c_dev->is_multimaster_mode)
+	if (i2c_dev->multimaster_mode)
 		clk_disable(i2c_dev->div_clk);
 
 	clk_bulk_unprepare(i2c_dev->nclocks, i2c_dev->clocks);
@@ -1678,7 +1677,7 @@  static int tegra_i2c_probe(struct platform_device *pdev)
 {
 	struct tegra_i2c_dev *i2c_dev;
 	struct resource *res;
-	int ret;
+	int err;
 
 	i2c_dev = devm_kzalloc(&pdev->dev, sizeof(*i2c_dev), GFP_KERNEL);
 	if (!i2c_dev)
@@ -1699,36 +1698,36 @@  static int tegra_i2c_probe(struct platform_device *pdev)
 
 	i2c_dev->base_phys = res->start;
 
-	ret = platform_get_irq(pdev, 0);
-	if (ret < 0)
-		return ret;
+	err = platform_get_irq(pdev, 0);
+	if (err < 0)
+		return err;
 
-	i2c_dev->irq = ret;
+	i2c_dev->irq = err;
 
 	/* interrupt will be enabled during of transfer time */
 	irq_set_status_flags(i2c_dev->irq, IRQ_NOAUTOEN);
 
-	ret = devm_request_irq(&pdev->dev, i2c_dev->irq, tegra_i2c_isr,
-			       IRQF_NO_SUSPEND, dev_name(&pdev->dev),
+	err = devm_request_irq(i2c_dev->dev, i2c_dev->irq, tegra_i2c_isr,
+			       IRQF_NO_SUSPEND, dev_name(i2c_dev->dev),
 			       i2c_dev);
-	if (ret)
-		return ret;
+	if (err)
+		return err;
 
-	i2c_dev->rst = devm_reset_control_get_exclusive(&pdev->dev, "i2c");
+	i2c_dev->rst = devm_reset_control_get_exclusive(i2c_dev->dev, "i2c");
 	if (IS_ERR(i2c_dev->rst)) {
-		dev_err_probe(&pdev->dev, PTR_ERR(i2c_dev->rst),
+		dev_err_probe(i2c_dev->dev, PTR_ERR(i2c_dev->rst),
 			      "failed to get reset control\n");
 		return PTR_ERR(i2c_dev->rst);
 	}
 
 	tegra_i2c_parse_dt(i2c_dev);
 
-	ret = tegra_i2c_init_clocks(i2c_dev);
-	if (ret)
-		return ret;
+	err = tegra_i2c_init_clocks(i2c_dev);
+	if (err)
+		return err;
 
-	ret = tegra_i2c_init_dma(i2c_dev);
-	if (ret)
+	err = tegra_i2c_init_dma(i2c_dev);
+	if (err)
 		goto release_clocks;
 
 	/*
@@ -1739,16 +1738,16 @@  static int tegra_i2c_probe(struct platform_device *pdev)
 	 * not be used for atomic transfers.
 	 */
 	if (!i2c_dev->is_vi)
-		pm_runtime_irq_safe(&pdev->dev);
-	pm_runtime_enable(&pdev->dev);
+		pm_runtime_irq_safe(i2c_dev->dev);
+	pm_runtime_enable(i2c_dev->dev);
 
-	ret = tegra_i2c_init_hardware(i2c_dev);
-	if (ret)
+	err = tegra_i2c_init_hardware(i2c_dev);
+	if (err)
 		goto release_rpm;
 
 	i2c_set_adapdata(&i2c_dev->adapter, i2c_dev);
-	i2c_dev->adapter.dev.of_node = pdev->dev.of_node;
-	i2c_dev->adapter.dev.parent = &pdev->dev;
+	i2c_dev->adapter.dev.of_node = i2c_dev->dev->of_node;
+	i2c_dev->adapter.dev.parent = i2c_dev->dev;
 	i2c_dev->adapter.retries = 1;
 	i2c_dev->adapter.timeout = 6 * HZ;
 	i2c_dev->adapter.quirks = i2c_dev->hw->quirks;
@@ -1760,23 +1759,23 @@  static int tegra_i2c_probe(struct platform_device *pdev)
 	if (i2c_dev->hw->supports_bus_clear)
 		i2c_dev->adapter.bus_recovery_info = &tegra_i2c_recovery_info;
 
-	strlcpy(i2c_dev->adapter.name, dev_name(&pdev->dev),
+	strlcpy(i2c_dev->adapter.name, dev_name(i2c_dev->dev),
 		sizeof(i2c_dev->adapter.name));
 
-	ret = i2c_add_numbered_adapter(&i2c_dev->adapter);
-	if (ret)
+	err = i2c_add_numbered_adapter(&i2c_dev->adapter);
+	if (err)
 		goto release_rpm;
 
 	return 0;
 
 release_rpm:
-	pm_runtime_disable(&pdev->dev);
+	pm_runtime_disable(i2c_dev->dev);
 
 	tegra_i2c_release_dma(i2c_dev);
 release_clocks:
 	tegra_i2c_release_clocks(i2c_dev);
 
-	return ret;
+	return err;
 }
 
 static int tegra_i2c_remove(struct platform_device *pdev)
@@ -1785,7 +1784,7 @@  static int tegra_i2c_remove(struct platform_device *pdev)
 
 	i2c_del_adapter(&i2c_dev->adapter);
 
-	pm_runtime_disable(&pdev->dev);
+	pm_runtime_disable(i2c_dev->dev);
 
 	tegra_i2c_release_dma(i2c_dev);
 	tegra_i2c_release_clocks(i2c_dev);
@@ -1795,15 +1794,15 @@  static int tegra_i2c_remove(struct platform_device *pdev)
 static int __maybe_unused tegra_i2c_runtime_resume(struct device *dev)
 {
 	struct tegra_i2c_dev *i2c_dev = dev_get_drvdata(dev);
-	int ret;
+	int err;
 
-	ret = pinctrl_pm_select_default_state(i2c_dev->dev);
-	if (ret)
-		return ret;
+	err = pinctrl_pm_select_default_state(dev);
+	if (err)
+		return err;
 
-	ret = clk_bulk_enable(i2c_dev->nclocks, i2c_dev->clocks);
-	if (ret)
-		return ret;
+	err = clk_bulk_enable(i2c_dev->nclocks, i2c_dev->clocks);
+	if (err)
+		return err;
 
 	/*
 	 * VI I2C device is attached to VE power domain which goes through
@@ -1812,8 +1811,8 @@  static int __maybe_unused tegra_i2c_runtime_resume(struct device *dev)
 	 * domain ON.
 	 */
 	if (i2c_dev->is_vi) {
-		ret = tegra_i2c_init(i2c_dev);
-		if (ret)
+		err = tegra_i2c_init(i2c_dev);
+		if (err)
 			goto disable_clocks;
 	}
 
@@ -1822,7 +1821,7 @@  static int __maybe_unused tegra_i2c_runtime_resume(struct device *dev)
 disable_clocks:
 	clk_bulk_disable(i2c_dev->nclocks, i2c_dev->clocks);
 
-	return ret;
+	return err;
 }
 
 static int __maybe_unused tegra_i2c_runtime_suspend(struct device *dev)
@@ -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;
 }
 
 static int __maybe_unused tegra_i2c_resume(struct device *dev)