diff mbox series

[v5,08/36] i2c: tegra: Use reset_control_reset()

Message ID 20200906185039.22700-9-digetx@gmail.com
State Changes Requested
Headers show
Series Improvements for Tegra I2C driver | expand

Commit Message

Dmitry Osipenko Sept. 6, 2020, 6:50 p.m. UTC 
Use a single reset_control_reset() instead of assert/deasset couple in
order to make code cleaner a tad. Note that the reset_control_reset()
uses 1 microsecond delay instead of 2 that was used previously, but this
shouldn't matter. In addition don't ignore potential error of the reset
control by emitting a noisy warning if it fails, which shouldn't ever
happen in practice.

Reviewed-by: Michał Mirosław <mirq-linux@rere.qmqm.pl>
Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
---
 drivers/i2c/busses/i2c-tegra.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

Comments

Andy Shevchenko Sept. 7, 2020, 8:13 a.m. UTC | #1 
On Sun, Sep 6, 2020 at 9:51 PM Dmitry Osipenko <digetx@gmail.com> wrote:
>
> Use a single reset_control_reset() instead of assert/deasset couple in
> order to make code cleaner a tad. Note that the reset_control_reset()
> uses 1 microsecond delay instead of 2 that was used previously, but this
> shouldn't matter. In addition don't ignore potential error of the reset
> control by emitting a noisy warning if it fails, which shouldn't ever
> happen in practice.

Still it's not clear if you check the datasheet or not. Some
elaboration would be good to have.

...

> +       WARN_ON_ONCE(err);

Why screaming here? Wouldn't be dev_warn() enough?
Dmitry Osipenko Sept. 7, 2020, 2:33 p.m. UTC | #2 
07.09.2020 11:13, Andy Shevchenko пишет:
> On Sun, Sep 6, 2020 at 9:51 PM Dmitry Osipenko <digetx@gmail.com> wrote:
>>
>> Use a single reset_control_reset() instead of assert/deasset couple in
>> order to make code cleaner a tad. Note that the reset_control_reset()
>> uses 1 microsecond delay instead of 2 that was used previously, but this
>> shouldn't matter. In addition don't ignore potential error of the reset
>> control by emitting a noisy warning if it fails, which shouldn't ever
>> happen in practice.
> 
> Still it's not clear if you check the datasheet or not. Some
> elaboration would be good to have.

I'll update the commit message with more details. Thanks!

> ...
> 
>> +       WARN_ON_ONCE(err);
> 
> Why screaming here? Wouldn't be dev_warn() enough?

The error condition is an indicator of a severe problem because the
reset shouldn't ever fail in practice, hence screaming is a preferred
behavour. I'll a add a comment to the code, telling this.
diff mbox series

Patch

diff --git a/drivers/i2c/busses/i2c-tegra.c b/drivers/i2c/busses/i2c-tegra.c
index 043b5ce52a6e..52e15ec246b3 100644
--- a/drivers/i2c/busses/i2c-tegra.c
+++ b/drivers/i2c/busses/i2c-tegra.c
@@ -785,9 +785,8 @@  static int tegra_i2c_init(struct tegra_i2c_dev *i2c_dev)
 	u32 tsu_thd;
 	u8 tlow, thigh;
 
-	reset_control_assert(i2c_dev->rst);
-	udelay(2);
-	reset_control_deassert(i2c_dev->rst);
+	err = reset_control_reset(i2c_dev->rst);
+	WARN_ON_ONCE(err);
 
 	if (i2c_dev->is_dvc)
 		tegra_dvc_init(i2c_dev);