Hello!
This series performs refactoring of the Tegra I2C driver code and hardens
the atomic-transfer mode.
Changelog:
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 (31):
i2c: tegra: Make tegra_i2c_flush_fifos() usable in atomic transfer
i2c: tegra: Handle potential error of tegra_i2c_flush_fifos()
i2c: tegra: Initialization 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: Factor out runtime PM and hardware initialization
i2c: tegra: Move out all device-tree parsing into tegra_i2c_parse_dt()
i2c: tegra: Clean up probe function
i2c: tegra: Remove likely/unlikely from the code
i2c: tegra: Remove bogus barrier()
i2c: tegra: Remove "dma" variable from tegra_i2c_xfer_msg()
i2c: tegra: Improve formatting of function variables
i2c: tegra: Improve coding style of tegra_i2c_wait_for_config_load()
i2c: tegra: Rename wait/poll functions
i2c: tegra: Rename variable in tegra_i2c_issue_bus_clear()
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: Reorder location of functions in the code
i2c: tegra: Check errors for both positive and negative values
i2c: tegra: Consolidate error handling in tegra_i2c_xfer_msg()
i2c: tegra: Clean up printk messages
i2c: tegra: Clean up whitespaces, newlines and indentation
i2c: tegra: Improve driver module description
i2c: tegra: Clean up and improve comments
i2c: tegra: Rename couple "ret" variables to "err"
drivers/i2c/busses/i2c-tegra.c | 1272 +++++++++++++++-----------------
1 file changed, 612 insertions(+), 660 deletions(-)
--
2.27.0
The driver's probe function code is a bit difficult to read. This patch
reorders code of the probe function, forming groups of code that are easy
to work with. The reset_control_get() now may return -EPROBE_DEFER on
newer Tegra SoCs because they use BPMP driver that provides reset controls
and BPMP doesn't come up early during boot, previously rst was protected
by other checks error checks in the code, hence dev_err_probe() is used
now for the rst.
Signed-off-by: Dmitry Osipenko <[email protected]>
---
drivers/i2c/busses/i2c-tegra.c | 120 +++++++++++++++++----------------
1 file changed, 63 insertions(+), 57 deletions(-)
diff --git a/drivers/i2c/busses/i2c-tegra.c b/drivers/i2c/busses/i2c-tegra.c
index d6bc23bf765e..d8b7373673ea 100644
--- a/drivers/i2c/busses/i2c-tegra.c
+++ b/drivers/i2c/busses/i2c-tegra.c
@@ -440,6 +440,9 @@ static int tegra_i2c_init_dma(struct tegra_i2c_dev *i2c_dev)
i2c_dev->tx_dma_chan = chan;
+ i2c_dev->dma_buf_size = i2c_dev->hw->quirks->max_write_len +
+ I2C_PACKET_HEADER_SIZE;
+
dma_buf = dma_alloc_coherent(i2c_dev->dev, i2c_dev->dma_buf_size,
&dma_phys, GFP_KERNEL | __GFP_NOWARN);
if (!dma_buf) {
@@ -1711,85 +1714,88 @@ static int tegra_i2c_probe(struct platform_device *pdev)
{
struct tegra_i2c_dev *i2c_dev;
struct resource *res;
- void __iomem *base;
- phys_addr_t base_phys;
- int irq;
- int ret;
-
- base = devm_platform_get_and_ioremap_resource(pdev, 0, &res);
- if (IS_ERR(base))
- return PTR_ERR(base);
-
- base_phys = res->start;
-
- irq = platform_get_irq(pdev, 0);
- if (irq < 0)
- return irq;
+ int err;
i2c_dev = devm_kzalloc(&pdev->dev, sizeof(*i2c_dev), GFP_KERNEL);
if (!i2c_dev)
return -ENOMEM;
- i2c_dev->base = base;
- i2c_dev->base_phys = base_phys;
- i2c_dev->adapter.algo = &tegra_i2c_algo;
- i2c_dev->adapter.retries = 1;
- i2c_dev->adapter.timeout = 6 * HZ;
- i2c_dev->irq = irq;
+ platform_set_drvdata(pdev, i2c_dev);
+
+ init_completion(&i2c_dev->msg_complete);
+ init_completion(&i2c_dev->dma_complete);
+
+ i2c_dev->hw = of_device_get_match_data(&pdev->dev);
i2c_dev->cont_id = pdev->id;
i2c_dev->dev = &pdev->dev;
- i2c_dev->rst = devm_reset_control_get_exclusive(&pdev->dev, "i2c");
+ /* initialize virt base of hardware registers */
+ i2c_dev->base = devm_platform_get_and_ioremap_resource(pdev, 0, &res);
+ if (IS_ERR(i2c_dev->base))
+ return PTR_ERR(i2c_dev->base);
+
+ /* initialize phys base of hardware registers */
+ i2c_dev->base_phys = res->start;
+
+ /* initialize controller's interrupt */
+ err = platform_get_irq(pdev, 0);
+ if (err < 0)
+ return err;
+
+ i2c_dev->irq = err;
+
+ /* interrupt will be enabled during of transfer time */
+ irq_set_status_flags(i2c_dev->irq, IRQ_NOAUTOEN);
+
+ err = devm_request_irq(i2c_dev->dev, i2c_dev->irq, tegra_i2c_isr,
+ IRQF_NO_SUSPEND, dev_name(i2c_dev->dev),
+ i2c_dev);
+ if (err)
+ return err;
+
+ /* initialize hardware reset control */
+ i2c_dev->rst = devm_reset_control_get_exclusive(i2c_dev->dev, "i2c");
if (IS_ERR(i2c_dev->rst)) {
- dev_err(&pdev->dev, "missing controller reset\n");
+ 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;
-
- i2c_dev->hw = of_device_get_match_data(&pdev->dev);
- i2c_dev->adapter.quirks = i2c_dev->hw->quirks;
- i2c_dev->dma_buf_size = i2c_dev->adapter.quirks->max_write_len +
- I2C_PACKET_HEADER_SIZE;
- init_completion(&i2c_dev->msg_complete);
- init_completion(&i2c_dev->dma_complete);
-
- platform_set_drvdata(pdev, i2c_dev);
-
- if (i2c_dev->hw->supports_bus_clear)
- i2c_dev->adapter.bus_recovery_info = &tegra_i2c_recovery_info;
+ err = tegra_i2c_init_clocks(i2c_dev);
+ if (err)
+ return err;
- ret = tegra_i2c_init_dma(i2c_dev);
- if (ret < 0)
+ err = tegra_i2c_init_dma(i2c_dev);
+ if (err)
goto release_clocks;
- ret = tegra_i2c_init_runtime_pm_and_hardware(i2c_dev);
- if (ret)
+ err = tegra_i2c_init_runtime_pm_and_hardware(i2c_dev);
+ if (err)
goto release_dma;
- 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), i2c_dev);
- if (ret)
- goto release_rpm;
-
- i2c_set_adapdata(&i2c_dev->adapter, i2c_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;
i2c_dev->adapter.owner = THIS_MODULE;
i2c_dev->adapter.class = I2C_CLASS_DEPRECATED;
- strlcpy(i2c_dev->adapter.name, dev_name(&pdev->dev),
+ i2c_dev->adapter.algo = &tegra_i2c_algo;
+ i2c_dev->adapter.nr = i2c_dev->cont_id;
+
+ if (i2c_dev->hw->supports_bus_clear)
+ i2c_dev->adapter.bus_recovery_info = &tegra_i2c_recovery_info;
+
+ strlcpy(i2c_dev->adapter.name, dev_name(i2c_dev->dev),
sizeof(i2c_dev->adapter.name));
- i2c_dev->adapter.dev.parent = &pdev->dev;
- i2c_dev->adapter.nr = pdev->id;
- i2c_dev->adapter.dev.of_node = pdev->dev.of_node;
- ret = i2c_add_numbered_adapter(&i2c_dev->adapter);
- if (ret)
- goto release_dma;
+ i2c_set_adapdata(&i2c_dev->adapter, i2c_dev);
+
+ err = i2c_add_numbered_adapter(&i2c_dev->adapter);
+ if (err)
+ goto release_rpm;
return 0;
@@ -1800,7 +1806,7 @@ static int tegra_i2c_probe(struct platform_device *pdev)
release_clocks:
tegra_i2c_release_clocks(i2c_dev);
- return ret;
+ return err;
}
static int tegra_i2c_remove(struct platform_device *pdev)
--
2.27.0
Use common helper for retrieval of the interrupt number in order to make
code cleaner. Note that platform_get_irq() prints error message by itself.
Signed-off-by: Dmitry Osipenko <[email protected]>
---
drivers/i2c/busses/i2c-tegra.c | 9 +++------
1 file changed, 3 insertions(+), 6 deletions(-)
diff --git a/drivers/i2c/busses/i2c-tegra.c b/drivers/i2c/busses/i2c-tegra.c
index abcfb53d649c..f1c4334faed3 100644
--- a/drivers/i2c/busses/i2c-tegra.c
+++ b/drivers/i2c/busses/i2c-tegra.c
@@ -1672,12 +1672,9 @@ static int tegra_i2c_probe(struct platform_device *pdev)
base_phys = res->start;
- res = platform_get_resource(pdev, IORESOURCE_IRQ, 0);
- if (!res) {
- dev_err(&pdev->dev, "no irq resource\n");
- return -EINVAL;
- }
- irq = res->start;
+ irq = platform_get_irq(pdev, 0);
+ if (irq < 0)
+ return irq;
div_clk = devm_clk_get(&pdev->dev, "div-clk");
if (IS_ERR(div_clk)) {
--
2.27.0
The "non_hs_mode" divisor value is fixed, thus there is no need to have
the variable i2c_dev.clk_divisor_non_hs_mode struct member. Let's remove
it and move the mode selection into tegra_i2c_init() where it can be
united with the timing selection.
Signed-off-by: Dmitry Osipenko <[email protected]>
---
drivers/i2c/busses/i2c-tegra.c | 45 +++++++++++++++-------------------
1 file changed, 20 insertions(+), 25 deletions(-)
diff --git a/drivers/i2c/busses/i2c-tegra.c b/drivers/i2c/busses/i2c-tegra.c
index 56c5aff579b6..f5b9cdb65182 100644
--- a/drivers/i2c/busses/i2c-tegra.c
+++ b/drivers/i2c/busses/i2c-tegra.c
@@ -250,7 +250,6 @@ 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
- * @clk_divisor_non_hs_mode: clock divider for non-high-speed modes
* @is_multimaster_mode: track if I2C controller is in multi-master mode
* @tx_dma_chan: DMA transmit channel
* @rx_dma_chan: DMA receive channel
@@ -281,7 +280,6 @@ struct tegra_i2c_dev {
size_t msg_buf_remaining;
int msg_read;
u32 bus_clk_rate;
- u16 clk_divisor_non_hs_mode;
bool is_multimaster_mode;
struct dma_chan *tx_dma_chan;
struct dma_chan *rx_dma_chan;
@@ -783,6 +781,7 @@ static int tegra_i2c_init(struct tegra_i2c_dev *i2c_dev)
u32 val;
int err;
u32 clk_divisor, clk_multiplier;
+ u32 non_hs_mode;
u32 tsu_thd;
u8 tlow, thigh;
@@ -805,24 +804,32 @@ static int tegra_i2c_init(struct tegra_i2c_dev *i2c_dev)
if (i2c_dev->is_vi)
tegra_i2c_vi_init(i2c_dev);
- /* Make sure clock divisor programmed correctly */
- clk_divisor = FIELD_PREP(I2C_CLK_DIVISOR_HSMODE,
- i2c_dev->hw->clk_divisor_hs_mode) |
- FIELD_PREP(I2C_CLK_DIVISOR_STD_FAST_MODE,
- i2c_dev->clk_divisor_non_hs_mode);
- i2c_writel(i2c_dev, clk_divisor, I2C_CLK_DIVISOR);
-
- if (i2c_dev->bus_clk_rate > I2C_MAX_STANDARD_MODE_FREQ &&
- i2c_dev->bus_clk_rate <= I2C_MAX_FAST_MODE_PLUS_FREQ) {
+ switch (i2c_dev->bus_clk_rate) {
+ case I2C_MAX_STANDARD_MODE_FREQ + 1 ... I2C_MAX_FAST_MODE_PLUS_FREQ:
tlow = i2c_dev->hw->tlow_fast_fastplus_mode;
thigh = i2c_dev->hw->thigh_fast_fastplus_mode;
tsu_thd = i2c_dev->hw->setup_hold_time_fast_fast_plus_mode;
- } else {
+
+ if (i2c_dev->bus_clk_rate > I2C_MAX_FAST_MODE_FREQ)
+ non_hs_mode = i2c_dev->hw->clk_divisor_fast_plus_mode;
+ else
+ non_hs_mode = i2c_dev->hw->clk_divisor_fast_mode;
+ break;
+
+ default:
tlow = i2c_dev->hw->tlow_std_mode;
thigh = i2c_dev->hw->thigh_std_mode;
tsu_thd = i2c_dev->hw->setup_hold_time_std_mode;
+ non_hs_mode = i2c_dev->hw->clk_divisor_std_mode;
+ break;
}
+ /* Make sure clock divisor programmed correctly */
+ clk_divisor = FIELD_PREP(I2C_CLK_DIVISOR_HSMODE,
+ i2c_dev->hw->clk_divisor_hs_mode) |
+ FIELD_PREP(I2C_CLK_DIVISOR_STD_FAST_MODE, non_hs_mode);
+ i2c_writel(i2c_dev, clk_divisor, I2C_CLK_DIVISOR);
+
if (i2c_dev->hw->has_interface_timing_reg) {
val = FIELD_PREP(I2C_INTERFACE_TIMING_THIGH, thigh) |
FIELD_PREP(I2C_INTERFACE_TIMING_TLOW, tlow);
@@ -837,7 +844,7 @@ static int tegra_i2c_init(struct tegra_i2c_dev *i2c_dev)
i2c_writel(i2c_dev, tsu_thd, I2C_INTERFACE_TIMING_1);
clk_multiplier = tlow + thigh + 2;
- clk_multiplier *= i2c_dev->clk_divisor_non_hs_mode + 1;
+ clk_multiplier *= non_hs_mode + 1;
err = clk_set_rate(i2c_dev->div_clk,
i2c_dev->bus_clk_rate * clk_multiplier);
@@ -1748,18 +1755,6 @@ static int tegra_i2c_probe(struct platform_device *pdev)
goto unprepare_fast_clk;
}
- if (i2c_dev->bus_clk_rate > I2C_MAX_FAST_MODE_FREQ &&
- i2c_dev->bus_clk_rate <= I2C_MAX_FAST_MODE_PLUS_FREQ)
- i2c_dev->clk_divisor_non_hs_mode =
- i2c_dev->hw->clk_divisor_fast_plus_mode;
- else if (i2c_dev->bus_clk_rate > I2C_MAX_STANDARD_MODE_FREQ &&
- i2c_dev->bus_clk_rate <= I2C_MAX_FAST_MODE_FREQ)
- i2c_dev->clk_divisor_non_hs_mode =
- i2c_dev->hw->clk_divisor_fast_mode;
- else
- i2c_dev->clk_divisor_non_hs_mode =
- i2c_dev->hw->clk_divisor_std_mode;
-
ret = clk_prepare(i2c_dev->div_clk);
if (ret < 0) {
dev_err(i2c_dev->dev, "Clock prepare failed %d\n", ret);
--
2.27.0
On Sat, Sep 05, 2020 at 11:41:20PM +0300, Dmitry Osipenko wrote:
> Hello!
>
> This series performs refactoring of the Tegra I2C driver code and hardens
> the atomic-transfer mode.
[...]
Pending comments, all LGTM.
Best Regards,
Micha? Miros?aw
06.09.2020 02:16, Michał Mirosław пишет:
> On Sat, Sep 05, 2020 at 11:41:20PM +0300, Dmitry Osipenko wrote:
>> Hello!
>>
>> This series performs refactoring of the Tegra I2C driver code and hardens
>> the atomic-transfer mode.
> [...]
>
> Pending comments, all LGTM.
Thank you!