LinuxLists.cc - [PATCH v7 00/34] Improvements for Tegra I2C driver

2020-09-08 22:47:48

Subject: [PATCH v7 00/34] Improvements for Tegra I2C driver

Hello!

This series performs refactoring of the Tegra I2C driver code and hardens
the atomic-transfer mode.

Changelog:

v7: - Reworked the "Clean up probe function" patch by moving out all
variable renamings into the "Clean up variable names" patch.
This results in a nicer diff, which was asked by Andy Shevchenko.

- Squashed "Improve coding style of tegra_i2c_wait_for_config_load()"
patch into "Factor out register polling into separate function" in
order avoid unnecessary ping-pong changes, which was asked by
Andy Shevchenko.

- Added more indentation improvements, it should be ideal now.

- I haven't changed order of the "Clean up variable types" patch,
which was suggested by Andy Shevchenko, because I already moved
that patch multiple times and we decided to sort patches starting
with more important cleanups and down to less important. The type
changes are more important than shuffling code around, IMO.

v6: - Added new patch that adds missing RPM puts, thanks to Andy Shevchenko
for the suggestion.

- Improved commit messages by extending them with more a more detailed
explanation of the changes.

- Added clarifying comment to the "Use reset_control_reset()" change,
which was asked by Andy Shevchenko.

- Refactored the "Clean up probe function" patch by moving the
dev_err_probe() change into the "Use clk-bulk helpers" patch,
which was suggested by Andy Shevchenko.

- Improved ordering of the patches like it was suggested by
Andy Shevchenko.

- Added Andy Shevchenko to suggested-by of the "Use clk-bulk helpers"
patch.

- Improved "Remove i2c_dev.clk_divisor_non_hs_mode member" patch by
making the case-switch to use "fast plus mode" timing if clock rate
is out-of-range. Just to make it more consistent.

- The "Improve tegra_i2c_dev structure" patch is squashed into
"Improve formatting of variables" and "Clean up types/names" patches.

- All variable-renaming changes are squashed into a single "Clean up
variable names" patch.

- Made extra minor improvement to various patches, like more comments
and indentations improved.

v5: - Dropped the "Factor out runtime PM and hardware initialization"
patch, like it was suggested by Michał Mirosław. Instead a less
invasive "Factor out hardware initialization into separate function"
patch added, it doesn't touch the RPM initialization.

- The "Remove outdated barrier()" patch now removes outdated comments.

- Updated commit description of the "Remove "dma" variable" patch,
saying that the transfer mode may be changed by a callee. This was
suggested by Michał Mirosław.

- Reworked the "Clean up and improve comments" patch. Couple more
comments are corrected and reworded now.

- Added r-b's from Michał Mirosław.

- New patches:

i2c: tegra: Mask interrupt in tegra_i2c_issue_bus_clear()
i2c: tegra: Remove redundant check in tegra_i2c_issue_bus_clear()
i2c: tegra: Don't fall back to PIO mode if DMA configuration fails
i2c: tegra: Clean up variable types
i2c: tegra: Improve tegra_i2c_dev structure

v4: - Reordered patches in the fixes/features/cleanups order like it was
suggested by Andy Shevchenko.

- Now using clk-bulk API, which was suggested by Andy Shevchenko.

- Reworked "Make tegra_i2c_flush_fifos() usable in atomic transfer"
patch to use iopoll API, which was suggested by Andy Shevchenko.

- Separated "Clean up probe function" into several smaller patches.

- Squashed "Add missing newline before returns" patch into
"Clean up whitespaces, newlines and indentation".

- The "Drop '_timeout' from wait/poll function names" is renamed to
"Rename wait/poll functions".

- The "Use reset_control_reset()" is changed to not fail tegra_i2c_init(),
but only emit warning. This should be more friendly behaviour in oppose
to having a non-bootable machine if reset-control fails.

- New patches:

i2c: tegra: Remove error message used for devm_request_irq() failure
i2c: tegra: Use devm_platform_get_and_ioremap_resource()
i2c: tegra: Use platform_get_irq()
i2c: tegra: Use clk-bulk helpers
i2c: tegra: Remove bogus barrier()
i2c: tegra: Factor out register polling into separate function
i2c: tegra: Consolidate error handling in tegra_i2c_xfer_msg()
i2c: tegra: Clean up and improve comments
i2c: tegra: Rename couple "ret" variables to "err"

v3: - Optimized "Make tegra_i2c_flush_fifos() usable in atomic transfer"
patch by pre-checking FIFO state before starting to poll using
ktime API, which may be expensive under some circumstances.

- The "Clean up messages in the code" patch now makes all messages
to use proper capitalization of abbreviations. Thanks to Andy Shevchenko
and Michał Mirosław for the suggestion.

- The "Remove unnecessary whitespaces and newlines" patch is transformed
into "Clean up whitespaces and newlines", it now also adds missing
newlines and spaces.

- Reworked the "Clean up probe function" patch in accordance to
suggestion from Michał Mirosław by factoring out only parts of
the code that make error unwinding cleaner.

- Added r-b from Michał Mirosław.

- Added more patches:

i2c: tegra: Reorder location of functions in the code
i2c: tegra: Factor out packet header setup from tegra_i2c_xfer_msg()
i2c: tegra: Remove "dma" variable
i2c: tegra: Initialization div-clk rate unconditionally
i2c: tegra: Remove i2c_dev.clk_divisor_non_hs_mode member

v2: - Cleaned more messages in the "Clean up messages in the code" patch.

- The error code of reset_control_reset() is checked now.

- Added these new patches to clean up couple more things:

i2c: tegra: Check errors for both positive and negative values
i2c: tegra: Improve coding style of tegra_i2c_wait_for_config_load()
i2c: tegra: Remove unnecessary whitespaces and newlines
i2c: tegra: Rename variable in tegra_i2c_issue_bus_clear()
i2c: tegra: Improve driver module description

Dmitry Osipenko (34):
i2c: tegra: Make tegra_i2c_flush_fifos() usable in atomic transfer
i2c: tegra: Add missing pm_runtime_put()
i2c: tegra: Handle potential error of tegra_i2c_flush_fifos()
i2c: tegra: Mask interrupt in tegra_i2c_issue_bus_clear()
i2c: tegra: Initialize div-clk rate unconditionally
i2c: tegra: Remove i2c_dev.clk_divisor_non_hs_mode member
i2c: tegra: Runtime PM always available on Tegra
i2c: tegra: Remove error message used for devm_request_irq() failure
i2c: tegra: Use reset_control_reset()
i2c: tegra: Use devm_platform_get_and_ioremap_resource()
i2c: tegra: Use platform_get_irq()
i2c: tegra: Use clk-bulk helpers
i2c: tegra: Move out all device-tree parsing into tegra_i2c_parse_dt()
i2c: tegra: Clean up probe function
i2c: tegra: Reorder location of functions in the code
i2c: tegra: Clean up variable types
i2c: tegra: Remove outdated barrier()
i2c: tegra: Remove likely/unlikely from the code
i2c: tegra: Remove redundant check in tegra_i2c_issue_bus_clear()
i2c: tegra: Remove "dma" variable from tegra_i2c_xfer_msg()
i2c: tegra: Don't fall back to PIO mode if DMA configuration fails
i2c: tegra: Rename wait/poll functions
i2c: tegra: Factor out error recovery from tegra_i2c_xfer_msg()
i2c: tegra: Factor out packet header setup from tegra_i2c_xfer_msg()
i2c: tegra: Factor out register polling into separate function
i2c: tegra: Factor out hardware initialization into separate function
i2c: tegra: Check errors for both positive and negative values
i2c: tegra: Consolidate error handling in tegra_i2c_xfer_msg()
i2c: tegra: Improve formatting of variables
i2c: tegra: Clean up variable names
i2c: tegra: Clean up printk messages
i2c: tegra: Clean up and improve comments
i2c: tegra: Clean up whitespaces, newlines and indentation
i2c: tegra: Improve driver module description

drivers/i2c/busses/i2c-tegra.c | 1435 ++++++++++++++++----------------
1 file changed, 701 insertions(+), 734 deletions(-)

--
2.27.0

2020-09-08 22:48:03

by Dmitry Osipenko

[permalink] [raw]

Subject: [PATCH v7 16/34] i2c: tegra: Clean up variable types

Don't use signed types for unsigned values and use consistent types
for sibling variables.

Signed-off-by: Dmitry Osipenko <[email protected]>
---
drivers/i2c/busses/i2c-tegra.c | 38 +++++++++++++++++-----------------
1 file changed, 19 insertions(+), 19 deletions(-)

diff --git a/drivers/i2c/busses/i2c-tegra.c b/drivers/i2c/busses/i2c-tegra.c
index 5a0bb5b3876c..71e82a68c942 100644
--- a/drivers/i2c/busses/i2c-tegra.c
+++ b/drivers/i2c/busses/i2c-tegra.c
@@ -206,20 +206,20 @@ struct tegra_i2c_hw_feature {
bool has_continue_xfer_support;
bool has_per_pkt_xfer_complete_irq;
bool has_config_load_reg;
- int clk_divisor_hs_mode;
- int clk_divisor_std_mode;
- int clk_divisor_fast_mode;
- u16 clk_divisor_fast_plus_mode;
+ u32 clk_divisor_hs_mode;
+ u32 clk_divisor_std_mode;
+ u32 clk_divisor_fast_mode;
+ u32 clk_divisor_fast_plus_mode;
bool has_multi_master_mode;
bool has_slcg_override_reg;
bool has_mst_fifo;
const struct i2c_adapter_quirks *quirks;
bool supports_bus_clear;
bool has_apb_dma;
- u8 tlow_std_mode;
- u8 thigh_std_mode;
- u8 tlow_fast_fastplus_mode;
- u8 thigh_fast_fastplus_mode;
+ u32 tlow_std_mode;
+ u32 thigh_std_mode;
+ u32 tlow_fast_fastplus_mode;
+ u32 thigh_fast_fastplus_mode;
u32 setup_hold_time_std_mode;
u32 setup_hold_time_fast_fast_plus_mode;
u32 setup_hold_time_hs_mode;
@@ -267,15 +267,15 @@ struct tegra_i2c_dev {
struct reset_control *rst;
void __iomem *base;
phys_addr_t base_phys;
- int cont_id;
- int irq;
- int is_dvc;
+ unsigned int cont_id;
+ unsigned int irq;
+ bool is_dvc;
bool is_vi;
struct completion msg_complete;
int msg_err;
u8 *msg_buf;
size_t msg_buf_remaining;
- int msg_read;
+ bool msg_read;
u32 bus_clk_rate;
bool is_multimaster_mode;
struct dma_chan *tx_dma_chan;
@@ -329,13 +329,13 @@ static u32 i2c_readl(struct tegra_i2c_dev *i2c_dev, unsigned long reg)
}

static void i2c_writesl(struct tegra_i2c_dev *i2c_dev, void *data,
- unsigned long reg, int len)
+ unsigned long reg, unsigned int len)
{
writesl(i2c_dev->base + tegra_i2c_reg_addr(i2c_dev, reg), data, len);
}

static void i2c_readsl(struct tegra_i2c_dev *i2c_dev, void *data,
- unsigned long reg, int len)
+ unsigned long reg, unsigned int len)
{
readsl(i2c_dev->base + tegra_i2c_reg_addr(i2c_dev, reg), data, len);
}
@@ -712,10 +712,10 @@ static int tegra_i2c_disable_packet_mode(struct tegra_i2c_dev *i2c_dev)
static int tegra_i2c_empty_rx_fifo(struct tegra_i2c_dev *i2c_dev)
{
u32 val;
- int rx_fifo_avail;
+ unsigned int rx_fifo_avail;
u8 *buf = i2c_dev->msg_buf;
size_t buf_remaining = i2c_dev->msg_buf_remaining;
- int words_to_transfer;
+ unsigned int words_to_transfer;

/*
* Catch overflow due to message fully sent
@@ -773,10 +773,10 @@ static int tegra_i2c_empty_rx_fifo(struct tegra_i2c_dev *i2c_dev)
static int tegra_i2c_fill_tx_fifo(struct tegra_i2c_dev *i2c_dev)
{
u32 val;
- int tx_fifo_avail;
+ unsigned int tx_fifo_avail;
u8 *buf = i2c_dev->msg_buf;
size_t buf_remaining = i2c_dev->msg_buf_remaining;
- int words_to_transfer;
+ unsigned int words_to_transfer;

if (i2c_dev->hw->has_mst_fifo) {
val = i2c_readl(i2c_dev, I2C_MST_FIFO_STATUS);
@@ -1134,7 +1134,7 @@ static int tegra_i2c_xfer_msg(struct tegra_i2c_dev *i2c_dev,
i2c_dev->msg_buf = msg->buf;
i2c_dev->msg_buf_remaining = msg->len;
i2c_dev->msg_err = I2C_ERR_NONE;
- i2c_dev->msg_read = (msg->flags & I2C_M_RD);
+ i2c_dev->msg_read = !!(msg->flags & I2C_M_RD);
reinit_completion(&i2c_dev->msg_complete);

if (i2c_dev->msg_read)
--
2.27.0

2020-09-08 22:48:11

by Dmitry Osipenko

[permalink] [raw]

Subject: [PATCH v7 05/34] i2c: tegra: Initialize div-clk rate unconditionally

It doesn't make sense to conditionalize the div-clk rate changes because
rate is fixed and it won't ever change once it's set at the driver's probe
time. All further changes are NO-OPs because CCF caches rate and skips
rate-change if rate is unchanged.

Reviewed-by: Michał Mirosław <[email protected]>
Signed-off-by: Dmitry Osipenko <[email protected]>
---
drivers/i2c/busses/i2c-tegra.c | 34 ++++++++++++++++------------------
1 file changed, 16 insertions(+), 18 deletions(-)

diff --git a/drivers/i2c/busses/i2c-tegra.c b/drivers/i2c/busses/i2c-tegra.c
index 1d1ce266255a..720a75439e91 100644
--- a/drivers/i2c/busses/i2c-tegra.c
+++ b/drivers/i2c/busses/i2c-tegra.c
@@ -293,7 +293,7 @@ struct tegra_i2c_dev {
bool is_curr_atomic_xfer;
};

-static int tegra_i2c_init(struct tegra_i2c_dev *i2c_dev, bool clk_reinit);
+static int tegra_i2c_init(struct tegra_i2c_dev *i2c_dev);

static void dvc_writel(struct tegra_i2c_dev *i2c_dev, u32 val,
unsigned long reg)
@@ -691,7 +691,7 @@ static int __maybe_unused tegra_i2c_runtime_resume(struct device *dev)
* domain ON.
*/
if (i2c_dev->is_vi) {
- ret = tegra_i2c_init(i2c_dev, true);
+ ret = tegra_i2c_init(i2c_dev);
if (ret)
goto disable_div_clk;
}
@@ -778,7 +778,7 @@ static void tegra_i2c_vi_init(struct tegra_i2c_dev *i2c_dev)
i2c_writel(i2c_dev, 0x0, I2C_TLOW_SEXT);
}

-static int tegra_i2c_init(struct tegra_i2c_dev *i2c_dev, bool clk_reinit)
+static int tegra_i2c_init(struct tegra_i2c_dev *i2c_dev)
{
u32 val;
int err;
@@ -836,16 +836,14 @@ static int tegra_i2c_init(struct tegra_i2c_dev *i2c_dev, bool clk_reinit)
if (i2c_dev->hw->has_interface_timing_reg && tsu_thd)
i2c_writel(i2c_dev, tsu_thd, I2C_INTERFACE_TIMING_1);

- if (!clk_reinit) {
- clk_multiplier = (tlow + thigh + 2);
- clk_multiplier *= (i2c_dev->clk_divisor_non_hs_mode + 1);
- err = clk_set_rate(i2c_dev->div_clk,
- i2c_dev->bus_clk_rate * clk_multiplier);
- if (err) {
- dev_err(i2c_dev->dev,
- "failed changing clock rate: %d\n", err);
- return err;
- }
+ clk_multiplier = tlow + thigh + 2;
+ clk_multiplier *= i2c_dev->clk_divisor_non_hs_mode + 1;
+
+ err = clk_set_rate(i2c_dev->div_clk,
+ i2c_dev->bus_clk_rate * clk_multiplier);
+ if (err) {
+ dev_err(i2c_dev->dev, "failed to set div-clk rate: %d\n", err);
+ return err;
}

if (!i2c_dev->is_dvc && !i2c_dev->is_vi) {
@@ -1319,7 +1317,7 @@ static int tegra_i2c_xfer_msg(struct tegra_i2c_dev *i2c_dev,

if (!time_left && !completion_done(&i2c_dev->dma_complete)) {
dev_err(i2c_dev->dev, "DMA transfer timeout\n");
- tegra_i2c_init(i2c_dev, true);
+ tegra_i2c_init(i2c_dev);
return -ETIMEDOUT;
}

@@ -1340,7 +1338,7 @@ static int tegra_i2c_xfer_msg(struct tegra_i2c_dev *i2c_dev,

if (time_left == 0) {
dev_err(i2c_dev->dev, "i2c transfer timed out\n");
- tegra_i2c_init(i2c_dev, true);
+ tegra_i2c_init(i2c_dev);
return -ETIMEDOUT;
}

@@ -1352,7 +1350,7 @@ static int tegra_i2c_xfer_msg(struct tegra_i2c_dev *i2c_dev,
if (likely(i2c_dev->msg_err == I2C_ERR_NONE))
return 0;

- tegra_i2c_init(i2c_dev, true);
+ tegra_i2c_init(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)
@@ -1811,7 +1809,7 @@ static int tegra_i2c_probe(struct platform_device *pdev)
if (ret < 0)
goto disable_div_clk;

- ret = tegra_i2c_init(i2c_dev, false);
+ ret = tegra_i2c_init(i2c_dev);
if (ret) {
dev_err(&pdev->dev, "Failed to initialize i2c controller\n");
goto release_dma;
@@ -1918,7 +1916,7 @@ static int __maybe_unused tegra_i2c_resume(struct device *dev)
if (err)
return err;

- err = tegra_i2c_init(i2c_dev, false);
+ err = tegra_i2c_init(i2c_dev);
if (err)
return err;

--
2.27.0

2020-09-09 09:13:27

by Andy Shevchenko

[permalink] [raw]

Subject: Re: [PATCH v7 00/34] Improvements for Tegra I2C driver

On Wed, Sep 9, 2020 at 1:40 AM Dmitry Osipenko <[email protected]> wrote:
>
> Hello!
>
> This series performs refactoring of the Tegra I2C driver code and hardens
> the atomic-transfer mode.

I think there is still room for improvement, but let not block it, FWIW,
Reviewed-by: Andy Shevchenko <[email protected]>

> Changelog:
>
> v7: - Reworked the "Clean up probe function" patch by moving out all
> variable renamings into the "Clean up variable names" patch.
> This results in a nicer diff, which was asked by Andy Shevchenko.
>
> - Squashed "Improve coding style of tegra_i2c_wait_for_config_load()"
> patch into "Factor out register polling into separate function" in
> order avoid unnecessary ping-pong changes, which was asked by
> Andy Shevchenko.
>
> - Added more indentation improvements, it should be ideal now.
>
> - I haven't changed order of the "Clean up variable types" patch,
> which was suggested by Andy Shevchenko, because I already moved
> that patch multiple times and we decided to sort patches starting
> with more important cleanups and down to less important. The type
> changes are more important than shuffling code around, IMO.
>
> v6: - Added new patch that adds missing RPM puts, thanks to Andy Shevchenko
> for the suggestion.
>
> - Improved commit messages by extending them with more a more detailed
> explanation of the changes.
>
> - Added clarifying comment to the "Use reset_control_reset()" change,
> which was asked by Andy Shevchenko.
>
> - Refactored the "Clean up probe function" patch by moving the
> dev_err_probe() change into the "Use clk-bulk helpers" patch,
> which was suggested by Andy Shevchenko.
>
> - Improved ordering of the patches like it was suggested by
> Andy Shevchenko.
>
> - Added Andy Shevchenko to suggested-by of the "Use clk-bulk helpers"
> patch.
>
> - Improved "Remove i2c_dev.clk_divisor_non_hs_mode member" patch by
> making the case-switch to use "fast plus mode" timing if clock rate
> is out-of-range. Just to make it more consistent.
>
> - The "Improve tegra_i2c_dev structure" patch is squashed into
> "Improve formatting of variables" and "Clean up types/names" patches.
>
> - All variable-renaming changes are squashed into a single "Clean up
> variable names" patch.
>
> - Made extra minor improvement to various patches, like more comments
> and indentations improved.
>
> v5: - Dropped the "Factor out runtime PM and hardware initialization"
> patch, like it was suggested by Michał Mirosław. Instead a less
> invasive "Factor out hardware initialization into separate function"
> patch added, it doesn't touch the RPM initialization.
>
> - The "Remove outdated barrier()" patch now removes outdated comments.
>
> - Updated commit description of the "Remove "dma" variable" patch,
> saying that the transfer mode may be changed by a callee. This was
> suggested by Michał Mirosław.
>
> - Reworked the "Clean up and improve comments" patch. Couple more
> comments are corrected and reworded now.
>
> - Added r-b's from Michał Mirosław.
>
> - New patches:
>
> i2c: tegra: Mask interrupt in tegra_i2c_issue_bus_clear()
> i2c: tegra: Remove redundant check in tegra_i2c_issue_bus_clear()
> i2c: tegra: Don't fall back to PIO mode if DMA configuration fails
> i2c: tegra: Clean up variable types
> i2c: tegra: Improve tegra_i2c_dev structure
>
> v4: - Reordered patches in the fixes/features/cleanups order like it was
> suggested by Andy Shevchenko.
>
> - Now using clk-bulk API, which was suggested by Andy Shevchenko.
>
> - Reworked "Make tegra_i2c_flush_fifos() usable in atomic transfer"
> patch to use iopoll API, which was suggested by Andy Shevchenko.
>
> - Separated "Clean up probe function" into several smaller patches.
>
> - Squashed "Add missing newline before returns" patch into
> "Clean up whitespaces, newlines and indentation".
>
> - The "Drop '_timeout' from wait/poll function names" is renamed to
> "Rename wait/poll functions".
>
> - The "Use reset_control_reset()" is changed to not fail tegra_i2c_init(),
> but only emit warning. This should be more friendly behaviour in oppose
> to having a non-bootable machine if reset-control fails.
>
> - New patches:
>
> i2c: tegra: Remove error message used for devm_request_irq() failure
> i2c: tegra: Use devm_platform_get_and_ioremap_resource()
> i2c: tegra: Use platform_get_irq()
> i2c: tegra: Use clk-bulk helpers
> i2c: tegra: Remove bogus barrier()
> i2c: tegra: Factor out register polling into separate function
> i2c: tegra: Consolidate error handling in tegra_i2c_xfer_msg()
> i2c: tegra: Clean up and improve comments
> i2c: tegra: Rename couple "ret" variables to "err"
>
> v3: - Optimized "Make tegra_i2c_flush_fifos() usable in atomic transfer"
> patch by pre-checking FIFO state before starting to poll using
> ktime API, which may be expensive under some circumstances.
>
> - The "Clean up messages in the code" patch now makes all messages
> to use proper capitalization of abbreviations. Thanks to Andy Shevchenko
> and Michał Mirosław for the suggestion.
>
> - The "Remove unnecessary whitespaces and newlines" patch is transformed
> into "Clean up whitespaces and newlines", it now also adds missing
> newlines and spaces.
>
> - Reworked the "Clean up probe function" patch in accordance to
> suggestion from Michał Mirosław by factoring out only parts of
> the code that make error unwinding cleaner.
>
> - Added r-b from Michał Mirosław.
>
> - Added more patches:
>
> i2c: tegra: Reorder location of functions in the code
> i2c: tegra: Factor out packet header setup from tegra_i2c_xfer_msg()
> i2c: tegra: Remove "dma" variable
> i2c: tegra: Initialization div-clk rate unconditionally
> i2c: tegra: Remove i2c_dev.clk_divisor_non_hs_mode member
>
> v2: - Cleaned more messages in the "Clean up messages in the code" patch.
>
> - The error code of reset_control_reset() is checked now.
>
> - Added these new patches to clean up couple more things:
>
> i2c: tegra: Check errors for both positive and negative values
> i2c: tegra: Improve coding style of tegra_i2c_wait_for_config_load()
> i2c: tegra: Remove unnecessary whitespaces and newlines
> i2c: tegra: Rename variable in tegra_i2c_issue_bus_clear()
> i2c: tegra: Improve driver module description
>
> Dmitry Osipenko (34):
> i2c: tegra: Make tegra_i2c_flush_fifos() usable in atomic transfer
> i2c: tegra: Add missing pm_runtime_put()
> i2c: tegra: Handle potential error of tegra_i2c_flush_fifos()
> i2c: tegra: Mask interrupt in tegra_i2c_issue_bus_clear()
> i2c: tegra: Initialize div-clk rate unconditionally
> i2c: tegra: Remove i2c_dev.clk_divisor_non_hs_mode member
> i2c: tegra: Runtime PM always available on Tegra
> i2c: tegra: Remove error message used for devm_request_irq() failure
> i2c: tegra: Use reset_control_reset()
> i2c: tegra: Use devm_platform_get_and_ioremap_resource()
> i2c: tegra: Use platform_get_irq()
> i2c: tegra: Use clk-bulk helpers
> i2c: tegra: Move out all device-tree parsing into tegra_i2c_parse_dt()
> i2c: tegra: Clean up probe function
> i2c: tegra: Reorder location of functions in the code
> i2c: tegra: Clean up variable types
> i2c: tegra: Remove outdated barrier()
> i2c: tegra: Remove likely/unlikely from the code
> i2c: tegra: Remove redundant check in tegra_i2c_issue_bus_clear()
> i2c: tegra: Remove "dma" variable from tegra_i2c_xfer_msg()
> i2c: tegra: Don't fall back to PIO mode if DMA configuration fails
> i2c: tegra: Rename wait/poll functions
> i2c: tegra: Factor out error recovery from tegra_i2c_xfer_msg()
> i2c: tegra: Factor out packet header setup from tegra_i2c_xfer_msg()
> i2c: tegra: Factor out register polling into separate function
> i2c: tegra: Factor out hardware initialization into separate function
> i2c: tegra: Check errors for both positive and negative values
> i2c: tegra: Consolidate error handling in tegra_i2c_xfer_msg()
> i2c: tegra: Improve formatting of variables
> i2c: tegra: Clean up variable names
> i2c: tegra: Clean up printk messages
> i2c: tegra: Clean up and improve comments
> i2c: tegra: Clean up whitespaces, newlines and indentation
> i2c: tegra: Improve driver module description
>
> drivers/i2c/busses/i2c-tegra.c | 1435 ++++++++++++++++----------------
> 1 file changed, 701 insertions(+), 734 deletions(-)
>
> --
> 2.27.0
>

--
With Best Regards,
Andy Shevchenko

2020-09-09 15:54:06

On Mon, Sep 21, 2020 at 10:18:27AM +0000, Thierry Reding wrote:
> On Wed, 09 Sep 2020 01:39:32 +0300, Dmitry Osipenko wrote:
> > Hello!
> >
> > This series performs refactoring of the Tegra I2C driver code and hardens
> > the atomic-transfer mode.
> >
> > Changelog:
> >
> > v7: - Reworked the "Clean up probe function" patch by moving out all
> > variable renamings into the "Clean up variable names" patch.
> > This results in a nicer diff, which was asked by Andy Shevchenko.
> >
> > - Squashed "Improve coding style of tegra_i2c_wait_for_config_load()"
> > patch into "Factor out register polling into separate function" in
> > order avoid unnecessary ping-pong changes, which was asked by
> > Andy Shevchenko.
> >
> > - Added more indentation improvements, it should be ideal now.
> >
> > - I haven't changed order of the "Clean up variable types" patch,
> > which was suggested by Andy Shevchenko, because I already moved
> > that patch multiple times and we decided to sort patches starting
> > with more important cleanups and down to less important. The type
> > changes are more important than shuffling code around, IMO.
> >
> > v6: - Added new patch that adds missing RPM puts, thanks to Andy Shevchenko
> > for the suggestion.
> >
> > - Improved commit messages by extending them with more a more detailed
> > explanation of the changes.
> >
> > - Added clarifying comment to the "Use reset_control_reset()" change,
> > which was asked by Andy Shevchenko.
> >
> > - Refactored the "Clean up probe function" patch by moving the
> > dev_err_probe() change into the "Use clk-bulk helpers" patch,
> > which was suggested by Andy Shevchenko.
> >
> > - Improved ordering of the patches like it was suggested by
> > Andy Shevchenko.
> >
> > - Added Andy Shevchenko to suggested-by of the "Use clk-bulk helpers"
> > patch.
> >
> > - Improved "Remove i2c_dev.clk_divisor_non_hs_mode member" patch by
> > making the case-switch to use "fast plus mode" timing if clock rate
> > is out-of-range. Just to make it more consistent.
> >
> > - The "Improve tegra_i2c_dev structure" patch is squashed into
> > "Improve formatting of variables" and "Clean up types/names" patches.
> >
> > - All variable-renaming changes are squashed into a single "Clean up
> > variable names" patch.
> >
> > - Made extra minor improvement to various patches, like more comments
> > and indentations improved.
> >
> > v5: - Dropped the "Factor out runtime PM and hardware initialization"
> > patch, like it was suggested by Michał Mirosław. Instead a less
> > invasive "Factor out hardware initialization into separate function"
> > patch added, it doesn't touch the RPM initialization.
> >
> > - The "Remove outdated barrier()" patch now removes outdated comments.
> >
> > - Updated commit description of the "Remove "dma" variable" patch,
> > saying that the transfer mode may be changed by a callee. This was
> > suggested by Michał Mirosław.
> >
> > - Reworked the "Clean up and improve comments" patch. Couple more
> > comments are corrected and reworded now.
> >
> > - Added r-b's from Michał Mirosław.
> >
> > - New patches:
> >
> > i2c: tegra: Mask interrupt in tegra_i2c_issue_bus_clear()
> > i2c: tegra: Remove redundant check in tegra_i2c_issue_bus_clear()
> > i2c: tegra: Don't fall back to PIO mode if DMA configuration fails
> > i2c: tegra: Clean up variable types
> > i2c: tegra: Improve tegra_i2c_dev structure
> >
> > v4: - Reordered patches in the fixes/features/cleanups order like it was
> > suggested by Andy Shevchenko.
> >
> > - Now using clk-bulk API, which was suggested by Andy Shevchenko.
> >
> > - Reworked "Make tegra_i2c_flush_fifos() usable in atomic transfer"
> > patch to use iopoll API, which was suggested by Andy Shevchenko.
> >
> > - Separated "Clean up probe function" into several smaller patches.
> >
> > - Squashed "Add missing newline before returns" patch into
> > "Clean up whitespaces, newlines and indentation".
> >
> > - The "Drop '_timeout' from wait/poll function names" is renamed to
> > "Rename wait/poll functions".
> >
> > - The "Use reset_control_reset()" is changed to not fail tegra_i2c_init(),
> > but only emit warning. This should be more friendly behaviour in oppose
> > to having a non-bootable machine if reset-control fails.
> >
> > - New patches:
> >
> > i2c: tegra: Remove error message used for devm_request_irq() failure
> > i2c: tegra: Use devm_platform_get_and_ioremap_resource()
> > i2c: tegra: Use platform_get_irq()
> > i2c: tegra: Use clk-bulk helpers
> > i2c: tegra: Remove bogus barrier()
> > i2c: tegra: Factor out register polling into separate function
> > i2c: tegra: Consolidate error handling in tegra_i2c_xfer_msg()
> > i2c: tegra: Clean up and improve comments
> > i2c: tegra: Rename couple "ret" variables to "err"
> >
> > v3: - Optimized "Make tegra_i2c_flush_fifos() usable in atomic transfer"
> > patch by pre-checking FIFO state before starting to poll using
> > ktime API, which may be expensive under some circumstances.
> >
> > - The "Clean up messages in the code" patch now makes all messages
> > to use proper capitalization of abbreviations. Thanks to Andy Shevchenko
> > and Michał Mirosław for the suggestion.
> >
> > - The "Remove unnecessary whitespaces and newlines" patch is transformed
> > into "Clean up whitespaces and newlines", it now also adds missing
> > newlines and spaces.
> >
> > - Reworked the "Clean up probe function" patch in accordance to
> > suggestion from Michał Mirosław by factoring out only parts of
> > the code that make error unwinding cleaner.
> >
> > - Added r-b from Michał Mirosław.
> >
> > - Added more patches:
> >
> > i2c: tegra: Reorder location of functions in the code
> > i2c: tegra: Factor out packet header setup from tegra_i2c_xfer_msg()
> > i2c: tegra: Remove "dma" variable
> > i2c: tegra: Initialization div-clk rate unconditionally
> > i2c: tegra: Remove i2c_dev.clk_divisor_non_hs_mode member
> >
> > v2: - Cleaned more messages in the "Clean up messages in the code" patch.
> >
> > - The error code of reset_control_reset() is checked now.
> >
> > - Added these new patches to clean up couple more things:
> >
> > i2c: tegra: Check errors for both positive and negative values
> > i2c: tegra: Improve coding style of tegra_i2c_wait_for_config_load()
> > i2c: tegra: Remove unnecessary whitespaces and newlines
> > i2c: tegra: Rename variable in tegra_i2c_issue_bus_clear()
> > i2c: tegra: Improve driver module description
> >
> > Dmitry Osipenko (34):
> > i2c: tegra: Make tegra_i2c_flush_fifos() usable in atomic transfer
> > i2c: tegra: Add missing pm_runtime_put()
> > i2c: tegra: Handle potential error of tegra_i2c_flush_fifos()
> > i2c: tegra: Mask interrupt in tegra_i2c_issue_bus_clear()
> > i2c: tegra: Initialize div-clk rate unconditionally
> > i2c: tegra: Remove i2c_dev.clk_divisor_non_hs_mode member
> > i2c: tegra: Runtime PM always available on Tegra
> > i2c: tegra: Remove error message used for devm_request_irq() failure
> > i2c: tegra: Use reset_control_reset()
> > i2c: tegra: Use devm_platform_get_and_ioremap_resource()
> > i2c: tegra: Use platform_get_irq()
> > i2c: tegra: Use clk-bulk helpers
> > i2c: tegra: Move out all device-tree parsing into tegra_i2c_parse_dt()
> > i2c: tegra: Clean up probe function
> > i2c: tegra: Reorder location of functions in the code
> > i2c: tegra: Clean up variable types
> > i2c: tegra: Remove outdated barrier()
> > i2c: tegra: Remove likely/unlikely from the code
> > i2c: tegra: Remove redundant check in tegra_i2c_issue_bus_clear()
> > i2c: tegra: Remove "dma" variable from tegra_i2c_xfer_msg()
> > i2c: tegra: Don't fall back to PIO mode if DMA configuration fails
> > i2c: tegra: Rename wait/poll functions
> > i2c: tegra: Factor out error recovery from tegra_i2c_xfer_msg()
> > i2c: tegra: Factor out packet header setup from tegra_i2c_xfer_msg()
> > i2c: tegra: Factor out register polling into separate function
> > i2c: tegra: Factor out hardware initialization into separate function
> > i2c: tegra: Check errors for both positive and negative values
> > i2c: tegra: Consolidate error handling in tegra_i2c_xfer_msg()
> > i2c: tegra: Improve formatting of variables
> > i2c: tegra: Clean up variable names
> > i2c: tegra: Clean up printk messages
> > i2c: tegra: Clean up and improve comments
> > i2c: tegra: Clean up whitespaces, newlines and indentation
> > i2c: tegra: Improve driver module description
> >
> > drivers/i2c/busses/i2c-tegra.c | 1435 ++++++++++++++++----------------
> > 1 file changed, 701 insertions(+), 734 deletions(-)
> > Reviewed-by: Andy Shevchenko <[email protected]>

Hm... not sure how this ended up here. My reporting script is parsing
the mailbox from patchwork, so perhaps this is patchwork injecting any
stored tags into the mailbox?

> Test results:
> 14 builds: 14 pass, 0 fail
> 9 boots: 9 pass, 0 fail
> 47 tests: 47 pass, 0 fail
>
> Boards tested: tegra20-ventana, tegra30-cardhu-a04, tegra124-jetson-tk1,
> tegra186-p2771-0000, tegra194-p2972-0000

Looks like something went wrong here. Some additional boards were
tested, but the reporting script seems to have failed to retrieve
some of the logs and then decided not to include the boards here.

Anyway, I don't see any failures with this set of patches applied
so I think it's all good.

Thierry

Attachments:

(No filename) (9.98 kB)
signature.asc (849.00 B)
Download all attachments