Re: [PATCH v7 25/34] i2c: tegra: Factor out register polling into separate function
On Thu, Sep 17, 2020 at 06:05:41PM +0300, Dmitry Osipenko wrote:
> 17.09.2020 14:58, Thierry Reding пишет:
> > On Wed, Sep 09, 2020 at 01:39:57AM +0300, Dmitry Osipenko wrote:
> >> Factor out register polling into a separate function in order to remove
> >> boilerplate code and make code cleaner.
> >>
> >> Reviewed-by: Michał Mirosław
> >> Signed-off-by: Dmitry Osipenko
> >> ---
> >> drivers/i2c/busses/i2c-tegra.c | 57 +++---
> >> 1 file changed, 25 insertions(+), 32 deletions(-)
> >>
> >> diff --git a/drivers/i2c/busses/i2c-tegra.c
> >> b/drivers/i2c/busses/i2c-tegra.c
> >> index 405b87e28a98..e071de9ce106 100644
> >> --- a/drivers/i2c/busses/i2c-tegra.c
> >> +++ b/drivers/i2c/busses/i2c-tegra.c
> >> @@ -514,10 +514,24 @@ static void tegra_i2c_vi_init(struct tegra_i2c_dev
> >> *i2c_dev)
> >>i2c_writel(i2c_dev, 0x0, I2C_TLOW_SEXT);
> >> }
> >>
> >> +static int tegra_i2c_poll_register(struct tegra_i2c_dev *i2c_dev,
> >> + u32 reg, u32 mask, u32 delay_us,
> >> + u32 timeout_us)
> >> +{
> >> + void __iomem *addr = i2c_dev->base + tegra_i2c_reg_addr(i2c_dev, reg);
> >> + u32 val;
> >> +
> >> + if (!i2c_dev->is_curr_atomic_xfer)
> >> + return readl_relaxed_poll_timeout(addr, val, !(val & mask),
> >> +delay_us, timeout_us);
> >> +
> >> + return readl_relaxed_poll_timeout_atomic(addr, val, !(val & mask),
> >> + delay_us, timeout_us);
> >> +}
> >> +
> >> static int tegra_i2c_flush_fifos(struct tegra_i2c_dev *i2c_dev)
> >> {
> >> - u32 mask, val, offset, reg_offset;
> >> - void __iomem *addr;
> >> + u32 mask, val, offset;
> >>int err;
> >>
> >>if (i2c_dev->hw->has_mst_fifo) {
> >> @@ -534,16 +548,7 @@ static int tegra_i2c_flush_fifos(struct tegra_i2c_dev
> >> *i2c_dev)
> >>val |= mask;
> >>i2c_writel(i2c_dev, val, offset);
> >>
> >> - reg_offset = tegra_i2c_reg_addr(i2c_dev, offset);
> >> - addr = i2c_dev->base + reg_offset;
> >> -
> >> - if (i2c_dev->is_curr_atomic_xfer)
> >> - err = readl_relaxed_poll_timeout_atomic(addr, val, !(val &
> >> mask),
> >> - 1000, 100);
> >> - else
> >> - err = readl_relaxed_poll_timeout(addr, val, !(val & mask),
> >> - 1000, 100);
> >> -
> >> + err = tegra_i2c_poll_register(i2c_dev, offset, mask, 1000, 100);
> >>if (err) {
> >>dev_err(i2c_dev->dev, "failed to flush FIFO\n");
> >>return err;
> >> @@ -553,30 +558,18 @@ static int tegra_i2c_flush_fifos(struct
> >> tegra_i2c_dev *i2c_dev)
> >>
> >> static int tegra_i2c_wait_for_config_load(struct tegra_i2c_dev *i2c_dev)
> >> {
> >> - unsigned long reg_offset;
> >> - void __iomem *addr;
> >> - u32 val;
> >>int err;
> >>
> >> - if (i2c_dev->hw->has_config_load_reg) {
> >> - reg_offset = tegra_i2c_reg_addr(i2c_dev, I2C_CONFIG_LOAD);
> >> - addr = i2c_dev->base + reg_offset;
> >> - i2c_writel(i2c_dev, I2C_MSTR_CONFIG_LOAD, I2C_CONFIG_LOAD);
> >> + if (!i2c_dev->hw->has_config_load_reg)
> >> + return 0;
> >>
> >> - if (i2c_dev->is_curr_atomic_xfer)
> >> - err = readl_relaxed_poll_timeout_atomic(
> >> - addr, val, val == 0, 1000,
> >> - I2C_CONFIG_LOAD_TIMEOUT);
> >> - else
> >> - err = readl_relaxed_poll_timeout(
> >> - addr, val, val == 0, 1000,
> >> - I2C_CONFIG_LOAD_TIMEOUT);
> >> + i2c_writel(i2c_dev, I2C_MSTR_CONFIG_LOAD, I2C_CONFIG_LOAD);
> >>
> >> - if (err) {
> >> - dev_warn(i2c_dev->dev,
> >> - "timeout waiting for config load\n");
> >> - return err;
> >> - }
> >> + err = tegra_i2c_poll_register(i2c_dev, I2C_CONFIG_LOAD, 0x,
> >> +1000, I2C_CONFIG_LOAD_TIMEOUT);
> >> + if (err) {
> >> + dev_warn(i2c_dev->dev, "timeout waiting for config load\n");
> >> + return err;
> >>}
> >
> > The deindentation in this hunk is messing up the diffstat in my opinion.
> > It would probably be worth splitting that into a separate patch to make
> > it more evident that this patch actually removes boilerplate.
>
> This is intentional and it's mentioned in the v7 changelog.
>
> Previously there was another patch that did what you're suggesting, but
> Andy Shevchenko objected that it was causing a "ping-pong" code changes
> where one patch did a change and then next patch changes the changed
> code again.
Hm... alright then:
Reviewed-by: Thierry Reding
signature.asc
Description: PGP signature
Re: [PATCH v7 25/34] i2c: tegra: Factor out register polling into separate function
On Wed, 09 Sep 2020 01:39:57 +0300, Dmitry Osipenko wrote: > Factor out register polling into a separate function in order to remove > boilerplate code and make code cleaner. > > Reviewed-by: Michał Mirosław > Signed-off-by: Dmitry Osipenko > --- > drivers/i2c/busses/i2c-tegra.c | 57 +++--- > 1 file changed, 25 insertions(+), 32 deletions(-) Tested-by: Thierry Reding
Re: [PATCH v7 25/34] i2c: tegra: Factor out register polling into separate function
17.09.2020 14:58, Thierry Reding пишет:
> On Wed, Sep 09, 2020 at 01:39:57AM +0300, Dmitry Osipenko wrote:
>> Factor out register polling into a separate function in order to remove
>> boilerplate code and make code cleaner.
>>
>> Reviewed-by: Michał Mirosław
>> Signed-off-by: Dmitry Osipenko
>> ---
>> drivers/i2c/busses/i2c-tegra.c | 57 +++---
>> 1 file changed, 25 insertions(+), 32 deletions(-)
>>
>> diff --git a/drivers/i2c/busses/i2c-tegra.c b/drivers/i2c/busses/i2c-tegra.c
>> index 405b87e28a98..e071de9ce106 100644
>> --- a/drivers/i2c/busses/i2c-tegra.c
>> +++ b/drivers/i2c/busses/i2c-tegra.c
>> @@ -514,10 +514,24 @@ static void tegra_i2c_vi_init(struct tegra_i2c_dev
>> *i2c_dev)
>> i2c_writel(i2c_dev, 0x0, I2C_TLOW_SEXT);
>> }
>>
>> +static int tegra_i2c_poll_register(struct tegra_i2c_dev *i2c_dev,
>> + u32 reg, u32 mask, u32 delay_us,
>> + u32 timeout_us)
>> +{
>> +void __iomem *addr = i2c_dev->base + tegra_i2c_reg_addr(i2c_dev, reg);
>> +u32 val;
>> +
>> +if (!i2c_dev->is_curr_atomic_xfer)
>> +return readl_relaxed_poll_timeout(addr, val, !(val & mask),
>> + delay_us, timeout_us);
>> +
>> +return readl_relaxed_poll_timeout_atomic(addr, val, !(val & mask),
>> + delay_us, timeout_us);
>> +}
>> +
>> static int tegra_i2c_flush_fifos(struct tegra_i2c_dev *i2c_dev)
>> {
>> -u32 mask, val, offset, reg_offset;
>> -void __iomem *addr;
>> +u32 mask, val, offset;
>> int err;
>>
>> if (i2c_dev->hw->has_mst_fifo) {
>> @@ -534,16 +548,7 @@ static int tegra_i2c_flush_fifos(struct tegra_i2c_dev
>> *i2c_dev)
>> val |= mask;
>> i2c_writel(i2c_dev, val, offset);
>>
>> -reg_offset = tegra_i2c_reg_addr(i2c_dev, offset);
>> -addr = i2c_dev->base + reg_offset;
>> -
>> -if (i2c_dev->is_curr_atomic_xfer)
>> -err = readl_relaxed_poll_timeout_atomic(addr, val, !(val &
>> mask),
>> -1000, 100);
>> -else
>> -err = readl_relaxed_poll_timeout(addr, val, !(val & mask),
>> - 1000, 100);
>> -
>> +err = tegra_i2c_poll_register(i2c_dev, offset, mask, 1000, 100);
>> if (err) {
>> dev_err(i2c_dev->dev, "failed to flush FIFO\n");
>> return err;
>> @@ -553,30 +558,18 @@ static int tegra_i2c_flush_fifos(struct tegra_i2c_dev
>> *i2c_dev)
>>
>> static int tegra_i2c_wait_for_config_load(struct tegra_i2c_dev *i2c_dev)
>> {
>> -unsigned long reg_offset;
>> -void __iomem *addr;
>> -u32 val;
>> int err;
>>
>> -if (i2c_dev->hw->has_config_load_reg) {
>> -reg_offset = tegra_i2c_reg_addr(i2c_dev, I2C_CONFIG_LOAD);
>> -addr = i2c_dev->base + reg_offset;
>> -i2c_writel(i2c_dev, I2C_MSTR_CONFIG_LOAD, I2C_CONFIG_LOAD);
>> +if (!i2c_dev->hw->has_config_load_reg)
>> +return 0;
>>
>> -if (i2c_dev->is_curr_atomic_xfer)
>> -err = readl_relaxed_poll_timeout_atomic(
>> -addr, val, val == 0, 1000,
>> -I2C_CONFIG_LOAD_TIMEOUT);
>> -else
>> -err = readl_relaxed_poll_timeout(
>> -addr, val, val == 0, 1000,
>> -I2C_CONFIG_LOAD_TIMEOUT);
>> +i2c_writel(i2c_dev, I2C_MSTR_CONFIG_LOAD, I2C_CONFIG_LOAD);
>>
>> -if (err) {
>> -dev_warn(i2c_dev->dev,
>> - "timeout waiting for config load\n");
>> -return err;
>> -}
>> +err = tegra_i2c_poll_register(i2c_dev, I2C_CONFIG_LOAD, 0x,
>> + 1000, I2C_CONFIG_LOAD_TIMEOUT);
>> +if (err) {
>> +dev_warn(i2c_dev->dev, "timeout waiting for config load\n");
>> +return err;
>> }
>
> The deindentation in this hunk is messing up the diffstat in my opinion.
> It would probably be worth splitting that into a separate patch to make
> it more evident that this patch actually removes boilerplate.
This is intentional and it's mentioned in the v7 changelog.
Previously there was another patch that did what you're suggesting, but
Andy Shevchenko objected that it was causing a "ping-pong" code changes
where one patch did a change and then next patch changes the changed
code again.
Re: [PATCH v7 25/34] i2c: tegra: Factor out register polling into separate function
On Wed, Sep 09, 2020 at 01:39:57AM +0300, Dmitry Osipenko wrote:
> Factor out register polling into a separate function in order to remove
> boilerplate code and make code cleaner.
>
> Reviewed-by: Michał Mirosław
> Signed-off-by: Dmitry Osipenko
> ---
> drivers/i2c/busses/i2c-tegra.c | 57 +++---
> 1 file changed, 25 insertions(+), 32 deletions(-)
>
> diff --git a/drivers/i2c/busses/i2c-tegra.c b/drivers/i2c/busses/i2c-tegra.c
> index 405b87e28a98..e071de9ce106 100644
> --- a/drivers/i2c/busses/i2c-tegra.c
> +++ b/drivers/i2c/busses/i2c-tegra.c
> @@ -514,10 +514,24 @@ static void tegra_i2c_vi_init(struct tegra_i2c_dev
> *i2c_dev)
> i2c_writel(i2c_dev, 0x0, I2C_TLOW_SEXT);
> }
>
> +static int tegra_i2c_poll_register(struct tegra_i2c_dev *i2c_dev,
> +u32 reg, u32 mask, u32 delay_us,
> +u32 timeout_us)
> +{
> + void __iomem *addr = i2c_dev->base + tegra_i2c_reg_addr(i2c_dev, reg);
> + u32 val;
> +
> + if (!i2c_dev->is_curr_atomic_xfer)
> + return readl_relaxed_poll_timeout(addr, val, !(val & mask),
> + delay_us, timeout_us);
> +
> + return readl_relaxed_poll_timeout_atomic(addr, val, !(val & mask),
> + delay_us, timeout_us);
> +}
> +
> static int tegra_i2c_flush_fifos(struct tegra_i2c_dev *i2c_dev)
> {
> - u32 mask, val, offset, reg_offset;
> - void __iomem *addr;
> + u32 mask, val, offset;
> int err;
>
> if (i2c_dev->hw->has_mst_fifo) {
> @@ -534,16 +548,7 @@ static int tegra_i2c_flush_fifos(struct tegra_i2c_dev
> *i2c_dev)
> val |= mask;
> i2c_writel(i2c_dev, val, offset);
>
> - reg_offset = tegra_i2c_reg_addr(i2c_dev, offset);
> - addr = i2c_dev->base + reg_offset;
> -
> - if (i2c_dev->is_curr_atomic_xfer)
> - err = readl_relaxed_poll_timeout_atomic(addr, val, !(val &
> mask),
> - 1000, 100);
> - else
> - err = readl_relaxed_poll_timeout(addr, val, !(val & mask),
> - 1000, 100);
> -
> + err = tegra_i2c_poll_register(i2c_dev, offset, mask, 1000, 100);
> if (err) {
> dev_err(i2c_dev->dev, "failed to flush FIFO\n");
> return err;
> @@ -553,30 +558,18 @@ static int tegra_i2c_flush_fifos(struct tegra_i2c_dev
> *i2c_dev)
>
> static int tegra_i2c_wait_for_config_load(struct tegra_i2c_dev *i2c_dev)
> {
> - unsigned long reg_offset;
> - void __iomem *addr;
> - u32 val;
> int err;
>
> - if (i2c_dev->hw->has_config_load_reg) {
> - reg_offset = tegra_i2c_reg_addr(i2c_dev, I2C_CONFIG_LOAD);
> - addr = i2c_dev->base + reg_offset;
> - i2c_writel(i2c_dev, I2C_MSTR_CONFIG_LOAD, I2C_CONFIG_LOAD);
> + if (!i2c_dev->hw->has_config_load_reg)
> + return 0;
>
> - if (i2c_dev->is_curr_atomic_xfer)
> - err = readl_relaxed_poll_timeout_atomic(
> - addr, val, val == 0, 1000,
> - I2C_CONFIG_LOAD_TIMEOUT);
> - else
> - err = readl_relaxed_poll_timeout(
> - addr, val, val == 0, 1000,
> - I2C_CONFIG_LOAD_TIMEOUT);
> + i2c_writel(i2c_dev, I2C_MSTR_CONFIG_LOAD, I2C_CONFIG_LOAD);
>
> - if (err) {
> - dev_warn(i2c_dev->dev,
> - "timeout waiting for config load\n");
> - return err;
> - }
> + err = tegra_i2c_poll_register(i2c_dev, I2C_CONFIG_LOAD, 0x,
> + 1000, I2C_CONFIG_LOAD_TIMEOUT);
> + if (err) {
> + dev_warn(i2c_dev->dev, "timeout waiting for config load\n");
> + return err;
> }
The deindentation in this hunk is messing up the diffstat in my opinion.
It would probably be worth splitting that into a separate patch to make
it more evident that this patch actually removes boilerplate.
Thierry
signature.asc
Description: PGP signature
[PATCH v7 25/34] i2c: tegra: Factor out register polling into separate function
Factor out register polling into a separate function in order to remove
boilerplate code and make code cleaner.
Reviewed-by: Michał Mirosław
Signed-off-by: Dmitry Osipenko
---
drivers/i2c/busses/i2c-tegra.c | 57 +++---
1 file changed, 25 insertions(+), 32 deletions(-)
diff --git a/drivers/i2c/busses/i2c-tegra.c b/drivers/i2c/busses/i2c-tegra.c
index 405b87e28a98..e071de9ce106 100644
--- a/drivers/i2c/busses/i2c-tegra.c
+++ b/drivers/i2c/busses/i2c-tegra.c
@@ -514,10 +514,24 @@ static void tegra_i2c_vi_init(struct tegra_i2c_dev
*i2c_dev)
i2c_writel(i2c_dev, 0x0, I2C_TLOW_SEXT);
}
+static int tegra_i2c_poll_register(struct tegra_i2c_dev *i2c_dev,
+ u32 reg, u32 mask, u32 delay_us,
+ u32 timeout_us)
+{
+ void __iomem *addr = i2c_dev->base + tegra_i2c_reg_addr(i2c_dev, reg);
+ u32 val;
+
+ if (!i2c_dev->is_curr_atomic_xfer)
+ return readl_relaxed_poll_timeout(addr, val, !(val & mask),
+ delay_us, timeout_us);
+
+ return readl_relaxed_poll_timeout_atomic(addr, val, !(val & mask),
+delay_us, timeout_us);
+}
+
static int tegra_i2c_flush_fifos(struct tegra_i2c_dev *i2c_dev)
{
- u32 mask, val, offset, reg_offset;
- void __iomem *addr;
+ u32 mask, val, offset;
int err;
if (i2c_dev->hw->has_mst_fifo) {
@@ -534,16 +548,7 @@ static int tegra_i2c_flush_fifos(struct tegra_i2c_dev
*i2c_dev)
val |= mask;
i2c_writel(i2c_dev, val, offset);
- reg_offset = tegra_i2c_reg_addr(i2c_dev, offset);
- addr = i2c_dev->base + reg_offset;
-
- if (i2c_dev->is_curr_atomic_xfer)
- err = readl_relaxed_poll_timeout_atomic(addr, val, !(val &
mask),
- 1000, 100);
- else
- err = readl_relaxed_poll_timeout(addr, val, !(val & mask),
-1000, 100);
-
+ err = tegra_i2c_poll_register(i2c_dev, offset, mask, 1000, 100);
if (err) {
dev_err(i2c_dev->dev, "failed to flush FIFO\n");
return err;
@@ -553,30 +558,18 @@ static int tegra_i2c_flush_fifos(struct tegra_i2c_dev
*i2c_dev)
static int tegra_i2c_wait_for_config_load(struct tegra_i2c_dev *i2c_dev)
{
- unsigned long reg_offset;
- void __iomem *addr;
- u32 val;
int err;
- if (i2c_dev->hw->has_config_load_reg) {
- reg_offset = tegra_i2c_reg_addr(i2c_dev, I2C_CONFIG_LOAD);
- addr = i2c_dev->base + reg_offset;
- i2c_writel(i2c_dev, I2C_MSTR_CONFIG_LOAD, I2C_CONFIG_LOAD);
+ if (!i2c_dev->hw->has_config_load_reg)
+ return 0;
- if (i2c_dev->is_curr_atomic_xfer)
- err = readl_relaxed_poll_timeout_atomic(
- addr, val, val == 0, 1000,
- I2C_CONFIG_LOAD_TIMEOUT);
- else
- err = readl_relaxed_poll_timeout(
- addr, val, val == 0, 1000,
- I2C_CONFIG_LOAD_TIMEOUT);
+ i2c_writel(i2c_dev, I2C_MSTR_CONFIG_LOAD, I2C_CONFIG_LOAD);
- if (err) {
- dev_warn(i2c_dev->dev,
-"timeout waiting for config load\n");
- return err;
- }
+ err = tegra_i2c_poll_register(i2c_dev, I2C_CONFIG_LOAD, 0x,
+ 1000, I2C_CONFIG_LOAD_TIMEOUT);
+ if (err) {
+ dev_warn(i2c_dev->dev, "timeout waiting for config load\n");
+ return err;
}
return 0;
--
2.27.0
