2020-08-31 22:53:05

by Dmitry Osipenko

[permalink] [raw]
Subject: [PATCH v1 00/12] Improvements for Tegra I2C driver

Hello!

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

Dmitry Osipenko (12):
i2c: tegra: Make tegra_i2c_flush_fifos() usable in atomic transfer
i2c: tegra: Add missing newline before returns
i2c: tegra: Clean up messages in the code
i2c: tegra: Don't ignore tegra_i2c_flush_fifos() error
i2c: tegra: Use reset_control_reset()
i2c: tegra: Improve formatting of function variables
i2c: tegra: Use dev_err_probe()
i2c: tegra: Runtime PM always available on Tegra
i2c: tegra: Clean up probe function
i2c: tegra: Drop '_timeout' from wait/poll function names
i2c: tegra: Remove likely/unlikely from the code
i2c: tegra: Factor out error recovery from tegra_i2c_xfer_msg()

drivers/i2c/busses/i2c-tegra.c | 601 ++++++++++++++++++---------------
1 file changed, 338 insertions(+), 263 deletions(-)

--
2.27.0


2020-08-31 22:53:26

by Dmitry Osipenko

[permalink] [raw]
Subject: [PATCH v1 07/12] i2c: tegra: Use dev_err_probe()

Use dev_err_probe() to replace the manual -EPROBE_DEFER handling, making
code cleaner.

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

diff --git a/drivers/i2c/busses/i2c-tegra.c b/drivers/i2c/busses/i2c-tegra.c
index 2c00f2e39514..525a757bdc66 100644
--- a/drivers/i2c/busses/i2c-tegra.c
+++ b/drivers/i2c/busses/i2c-tegra.c
@@ -1646,8 +1646,8 @@ MODULE_DEVICE_TABLE(of, tegra_i2c_of_match);

static int tegra_i2c_probe(struct platform_device *pdev)
{
+ struct clk *div_clk, *fast_clk, *slow_clk;
struct device *dev = &pdev->dev;
- struct clk *div_clk, *fast_clk;
struct tegra_i2c_dev *i2c_dev;
phys_addr_t base_phys;
struct resource *res;
@@ -1668,12 +1668,9 @@ static int tegra_i2c_probe(struct platform_device *pdev)
irq = res->start;

div_clk = devm_clk_get(&pdev->dev, "div-clk");
- if (IS_ERR(div_clk)) {
- if (PTR_ERR(div_clk) != -EPROBE_DEFER)
- dev_err(&pdev->dev, "missing controller clock\n");
-
- return PTR_ERR(div_clk);
- }
+ if (IS_ERR(div_clk))
+ return dev_err_probe(&pdev->dev, PTR_ERR(div_clk),
+ "failed to get div-clk\n");

i2c_dev = devm_kzalloc(&pdev->dev, sizeof(*i2c_dev), GFP_KERNEL);
if (!i2c_dev)
@@ -1712,24 +1709,20 @@ static int tegra_i2c_probe(struct platform_device *pdev)

if (!i2c_dev->hw->has_single_clk_source) {
fast_clk = devm_clk_get(&pdev->dev, "fast-clk");
- if (IS_ERR(fast_clk)) {
- dev_err(dev, "failed to get fast clock\n: %ld\n",
- PTR_ERR(fast_clk));
+ if (IS_ERR(fast_clk))
+ return dev_err_probe(&pdev->dev, PTR_ERR(fast_clk),
+ "failed to get fast clock\n");

- return PTR_ERR(fast_clk);
- }
i2c_dev->fast_clk = fast_clk;
}

if (i2c_dev->is_vi) {
- i2c_dev->slow_clk = devm_clk_get(dev, "slow");
- if (IS_ERR(i2c_dev->slow_clk)) {
- if (PTR_ERR(i2c_dev->slow_clk) != -EPROBE_DEFER)
- dev_err(dev, "failed to get slow clock: %ld\n",
- PTR_ERR(i2c_dev->slow_clk));
+ slow_clk = devm_clk_get(dev, "slow");
+ if (IS_ERR(slow_clk))
+ return dev_err_probe(&pdev->dev, PTR_ERR(slow_clk),
+ "failed to get slow clock\n");

- return PTR_ERR(i2c_dev->slow_clk);
- }
+ i2c_dev->slow_clk = slow_clk;
}

platform_set_drvdata(pdev, i2c_dev);
--
2.27.0

2020-08-31 22:53:28

by Dmitry Osipenko

[permalink] [raw]
Subject: [PATCH v1 08/12] i2c: tegra: Runtime PM always available on Tegra

The runtime PM is guaranteed to be always available on Tegra after commit
40b2bb1b132a ("ARM: tegra: enforce PM requirement"). Hence let's remove
all the RPM-availability checking and handling from the code.

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

diff --git a/drivers/i2c/busses/i2c-tegra.c b/drivers/i2c/busses/i2c-tegra.c
index 525a757bdc66..ca8c16b1c049 100644
--- a/drivers/i2c/busses/i2c-tegra.c
+++ b/drivers/i2c/busses/i2c-tegra.c
@@ -1767,18 +1767,10 @@ static int tegra_i2c_probe(struct platform_device *pdev)
if (!i2c_dev->is_vi)
pm_runtime_irq_safe(&pdev->dev);
pm_runtime_enable(&pdev->dev);
- if (!pm_runtime_enabled(&pdev->dev)) {
- ret = tegra_i2c_runtime_resume(&pdev->dev);
- if (ret < 0) {
- dev_err(dev, "runtime resume failed\n");
- goto unprepare_div_clk;
- }
- } else {
- ret = pm_runtime_get_sync(i2c_dev->dev);
- if (ret < 0) {
- dev_err(dev, "runtime resume failed\n");
- goto disable_rpm;
- }
+ ret = pm_runtime_get_sync(i2c_dev->dev);
+ if (ret < 0) {
+ dev_err(dev, "runtime resume failed\n");
+ goto disable_rpm;
}

if (i2c_dev->is_multimaster_mode) {
@@ -1836,16 +1828,10 @@ static int tegra_i2c_probe(struct platform_device *pdev)
clk_disable(i2c_dev->div_clk);

put_rpm:
- if (pm_runtime_enabled(&pdev->dev))
- pm_runtime_put_sync(&pdev->dev);
- else
- tegra_i2c_runtime_suspend(&pdev->dev);
+ pm_runtime_put_sync(&pdev->dev);

disable_rpm:
- if (pm_runtime_enabled(&pdev->dev))
- pm_runtime_disable(&pdev->dev);
-
-unprepare_div_clk:
+ pm_runtime_disable(&pdev->dev);
clk_unprepare(i2c_dev->div_clk);

unprepare_slow_clk:
@@ -1867,8 +1853,6 @@ static int tegra_i2c_remove(struct platform_device *pdev)
clk_disable(i2c_dev->div_clk);

pm_runtime_disable(&pdev->dev);
- if (!pm_runtime_status_suspended(&pdev->dev))
- tegra_i2c_runtime_suspend(&pdev->dev);

clk_unprepare(i2c_dev->div_clk);
clk_unprepare(i2c_dev->slow_clk);
--
2.27.0

2020-08-31 22:54:05

by Dmitry Osipenko

[permalink] [raw]
Subject: [PATCH v1 11/12] i2c: tegra: Remove likely/unlikely from the code

The likely/unlikely annotations should be used only in a hot paths of
performance-critical code. The I2C driver doesn't have such paths, and
thus, there is no justification for usage of likely/unlikely annotations
in the code. Hence remove them.

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

diff --git a/drivers/i2c/busses/i2c-tegra.c b/drivers/i2c/busses/i2c-tegra.c
index 8c8f3189928e..d9b9fe6b5637 100644
--- a/drivers/i2c/busses/i2c-tegra.c
+++ b/drivers/i2c/busses/i2c-tegra.c
@@ -910,7 +910,7 @@ static irqreturn_t tegra_i2c_isr(int irq, void *dev_id)
goto err;
}

- if (unlikely(status & status_err)) {
+ if (status & status_err) {
tegra_i2c_disable_packet_mode(i2c_dev);
if (status & I2C_INT_NO_ACK)
i2c_dev->msg_err |= I2C_ERR_NO_ACK;
@@ -1341,7 +1341,7 @@ static int tegra_i2c_xfer_msg(struct tegra_i2c_dev *i2c_dev,
i2c_dev->msg_err);

i2c_dev->is_curr_dma_xfer = false;
- if (likely(i2c_dev->msg_err == I2C_ERR_NONE))
+ if (i2c_dev->msg_err == I2C_ERR_NONE)
return 0;

tegra_i2c_init(i2c_dev, true);
--
2.27.0

2020-08-31 22:54:27

by Dmitry Osipenko

[permalink] [raw]
Subject: [PATCH v1 05/12] i2c: tegra: Use reset_control_reset()

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.

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

diff --git a/drivers/i2c/busses/i2c-tegra.c b/drivers/i2c/busses/i2c-tegra.c
index a7ef3a93e1b5..6957be34bc41 100644
--- a/drivers/i2c/busses/i2c-tegra.c
+++ b/drivers/i2c/busses/i2c-tegra.c
@@ -791,9 +791,7 @@ static int tegra_i2c_init(struct tegra_i2c_dev *i2c_dev, bool clk_reinit)
u32 tsu_thd;
u8 tlow, thigh;

- reset_control_assert(i2c_dev->rst);
- udelay(2);
- reset_control_deassert(i2c_dev->rst);
+ reset_control_reset(i2c_dev->rst);

if (i2c_dev->is_dvc)
tegra_dvc_init(i2c_dev);
--
2.27.0

2020-08-31 22:54:35

by Dmitry Osipenko

[permalink] [raw]
Subject: [PATCH v1 03/12] i2c: tegra: Clean up messages in the code

Use lowercase and consistent wording for all messages in the code.

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

diff --git a/drivers/i2c/busses/i2c-tegra.c b/drivers/i2c/busses/i2c-tegra.c
index 9bd91b6f32f4..efbb20049cf8 100644
--- a/drivers/i2c/busses/i2c-tegra.c
+++ b/drivers/i2c/busses/i2c-tegra.c
@@ -427,7 +427,7 @@ static int tegra_i2c_init_dma(struct tegra_i2c_dev *i2c_dev)
return 0;

if (!IS_ENABLED(CONFIG_TEGRA20_APB_DMA)) {
- dev_dbg(i2c_dev->dev, "Support for APB DMA not enabled!\n");
+ dev_dbg(i2c_dev->dev, "dma support not enabled\n");
return 0;
}

@@ -450,7 +450,7 @@ static int tegra_i2c_init_dma(struct tegra_i2c_dev *i2c_dev)
dma_buf = dma_alloc_coherent(i2c_dev->dev, i2c_dev->dma_buf_size,
&dma_phys, GFP_KERNEL | __GFP_NOWARN);
if (!dma_buf) {
- dev_err(i2c_dev->dev, "failed to allocate the DMA buffer\n");
+ dev_err(i2c_dev->dev, "failed to allocate dma buffer\n");
err = -ENOMEM;
goto err_out;
}
@@ -672,8 +672,7 @@ static int __maybe_unused tegra_i2c_runtime_resume(struct device *dev)

ret = clk_enable(i2c_dev->fast_clk);
if (ret < 0) {
- dev_err(i2c_dev->dev,
- "Enabling fast clk failed, err %d\n", ret);
+ dev_err(dev, "failed to enable fast clock: %d\n", ret);
return ret;
}

@@ -685,8 +684,7 @@ static int __maybe_unused tegra_i2c_runtime_resume(struct device *dev)

ret = clk_enable(i2c_dev->div_clk);
if (ret < 0) {
- dev_err(i2c_dev->dev,
- "Enabling div clk failed, err %d\n", ret);
+ dev_err(dev, "failed to enable div clock: %d\n", ret);
goto disable_slow_clk;
}

@@ -850,7 +848,7 @@ static int tegra_i2c_init(struct tegra_i2c_dev *i2c_dev, bool clk_reinit)
i2c_dev->bus_clk_rate * clk_multiplier);
if (err) {
dev_err(i2c_dev->dev,
- "failed changing clock rate: %d\n", err);
+ "failed to set div-clk rate: %d\n", err);
return err;
}
}
@@ -1052,7 +1050,7 @@ static void tegra_i2c_config_fifo_trig(struct tegra_i2c_dev *i2c_dev,
slv_config.device_fc = true;
ret = dmaengine_slave_config(chan, &slv_config);
if (ret < 0) {
- dev_err(i2c_dev->dev, "DMA slave config failed: %d\n",
+ dev_err(i2c_dev->dev, "dma slave config failed: %d\n",
ret);
dev_err(i2c_dev->dev, "falling back to PIO\n");
tegra_i2c_release_dma(i2c_dev);
@@ -1163,8 +1161,7 @@ static int tegra_i2c_issue_bus_clear(struct i2c_adapter *adap)

reg = i2c_readl(i2c_dev, I2C_BUS_CLEAR_STATUS);
if (!(reg & I2C_BC_STATUS)) {
- dev_err(i2c_dev->dev,
- "un-recovered arbitration lost\n");
+ dev_err(i2c_dev->dev, "un-recovered arbitration lost\n");
return -EIO;
}

@@ -1221,8 +1218,7 @@ static int tegra_i2c_xfer_msg(struct tegra_i2c_dev *i2c_dev,
err = tegra_i2c_dma_submit(i2c_dev, xfer_size);
if (err < 0) {
dev_err(i2c_dev->dev,
- "starting RX DMA failed, err %d\n",
- err);
+ "starting rx dma failed: %d\n", err);
return err;
}

@@ -1281,8 +1277,7 @@ static int tegra_i2c_xfer_msg(struct tegra_i2c_dev *i2c_dev,
err = tegra_i2c_dma_submit(i2c_dev, xfer_size);
if (err < 0) {
dev_err(i2c_dev->dev,
- "starting TX DMA failed, err %d\n",
- err);
+ "starting tx dma failed: %d\n", err);
return err;
}
} else {
@@ -1321,7 +1316,7 @@ static int tegra_i2c_xfer_msg(struct tegra_i2c_dev *i2c_dev,
i2c_dev->tx_dma_chan);

if (!time_left && !completion_done(&i2c_dev->dma_complete)) {
- dev_err(i2c_dev->dev, "DMA transfer timeout\n");
+ dev_err(i2c_dev->dev, "dma transfer timeout\n");
tegra_i2c_init(i2c_dev, true);
return -ETIMEDOUT;
}
@@ -1676,7 +1671,7 @@ static int tegra_i2c_probe(struct platform_device *pdev)

res = platform_get_resource(pdev, IORESOURCE_IRQ, 0);
if (!res) {
- dev_err(&pdev->dev, "no irq resource\n");
+ dev_err(dev, "no irq resource\n");
return -EINVAL;
}
irq = res->start;
@@ -1705,7 +1700,9 @@ static int tegra_i2c_probe(struct platform_device *pdev)

i2c_dev->rst = devm_reset_control_get_exclusive(&pdev->dev, "i2c");
if (IS_ERR(i2c_dev->rst)) {
- dev_err(&pdev->dev, "missing controller reset\n");
+ dev_err(dev, "failed to get controller reset: %pe\n",
+ i2c_dev->rst);
+
return PTR_ERR(i2c_dev->rst);
}

@@ -1725,7 +1722,9 @@ static int tegra_i2c_probe(struct platform_device *pdev)
if (!i2c_dev->hw->has_single_clk_source) {
fast_clk = devm_clk_get(&pdev->dev, "fast-clk");
if (IS_ERR(fast_clk)) {
- dev_err(&pdev->dev, "missing fast clock\n");
+ dev_err(dev, "failed to get fast clock\n: %ld\n",
+ PTR_ERR(fast_clk));
+
return PTR_ERR(fast_clk);
}
i2c_dev->fast_clk = fast_clk;
@@ -1746,7 +1745,7 @@ static int tegra_i2c_probe(struct platform_device *pdev)

ret = clk_prepare(i2c_dev->fast_clk);
if (ret < 0) {
- dev_err(i2c_dev->dev, "Clock prepare failed %d\n", ret);
+ dev_err(dev, "failed to prepare fast clock: %d\n", ret);
return ret;
}

@@ -1770,7 +1769,7 @@ static int tegra_i2c_probe(struct platform_device *pdev)

ret = clk_prepare(i2c_dev->div_clk);
if (ret < 0) {
- dev_err(i2c_dev->dev, "Clock prepare failed %d\n", ret);
+ dev_err(dev, "failed to prepare div-clk: %d\n", ret);
goto unprepare_slow_clk;
}

@@ -1787,13 +1786,13 @@ static int tegra_i2c_probe(struct platform_device *pdev)
if (!pm_runtime_enabled(&pdev->dev)) {
ret = tegra_i2c_runtime_resume(&pdev->dev);
if (ret < 0) {
- dev_err(&pdev->dev, "runtime resume failed\n");
+ dev_err(dev, "runtime resume failed\n");
goto unprepare_div_clk;
}
} else {
ret = pm_runtime_get_sync(i2c_dev->dev);
if (ret < 0) {
- dev_err(&pdev->dev, "runtime resume failed\n");
+ dev_err(dev, "runtime resume failed\n");
goto disable_rpm;
}
}
@@ -1801,8 +1800,7 @@ static int tegra_i2c_probe(struct platform_device *pdev)
if (i2c_dev->is_multimaster_mode) {
ret = clk_enable(i2c_dev->div_clk);
if (ret < 0) {
- dev_err(i2c_dev->dev, "div_clk enable failed %d\n",
- ret);
+ dev_err(dev, "failed to enable div-clk: %d\n", ret);
goto put_rpm;
}
}
@@ -1816,7 +1814,7 @@ static int tegra_i2c_probe(struct platform_device *pdev)

ret = tegra_i2c_init(i2c_dev, false);
if (ret) {
- dev_err(&pdev->dev, "Failed to initialize i2c controller\n");
+ dev_err(dev, "failed to initialize i2c controller\n");
goto release_dma;
}

@@ -1825,7 +1823,7 @@ static int tegra_i2c_probe(struct platform_device *pdev)
ret = devm_request_irq(&pdev->dev, i2c_dev->irq, tegra_i2c_isr,
IRQF_NO_SUSPEND, dev_name(&pdev->dev), i2c_dev);
if (ret) {
- dev_err(&pdev->dev, "Failed to request irq %i\n", i2c_dev->irq);
+ dev_err(dev, "failed to request irq %i\n", i2c_dev->irq);
goto release_dma;
}

--
2.27.0

2020-09-01 12:11:37

by Dmitry Osipenko

[permalink] [raw]
Subject: Re: [PATCH v1 03/12] i2c: tegra: Clean up messages in the code

31.08.2020 23:22, Dmitry Osipenko пишет:
> Use lowercase and consistent wording for all messages in the code.
>
> Signed-off-by: Dmitry Osipenko <[email protected]>
> ---
> drivers/i2c/busses/i2c-tegra.c | 50 ++++++++++++++++------------------
> 1 file changed, 24 insertions(+), 26 deletions(-)

I'll prepare a v2 because today noticed that there are couple places in
the code that I missed to change in this patch. I'll also add couple
more patches.

2020-09-02 20:44:32

by Michał Mirosław

[permalink] [raw]
Subject: Re: [PATCH v1 03/12] i2c: tegra: Clean up messages in the code

On Mon, Aug 31, 2020 at 11:22:54PM +0300, Dmitry Osipenko wrote:
> Use lowercase and consistent wording for all messages in the code.
>
> Signed-off-by: Dmitry Osipenko <[email protected]>
> ---
> drivers/i2c/busses/i2c-tegra.c | 50 ++++++++++++++++------------------
> 1 file changed, 24 insertions(+), 26 deletions(-)
>
> diff --git a/drivers/i2c/busses/i2c-tegra.c b/drivers/i2c/busses/i2c-tegra.c
> index 9bd91b6f32f4..efbb20049cf8 100644
> --- a/drivers/i2c/busses/i2c-tegra.c
> +++ b/drivers/i2c/busses/i2c-tegra.c
> @@ -427,7 +427,7 @@ static int tegra_i2c_init_dma(struct tegra_i2c_dev *i2c_dev)
> return 0;
>
> if (!IS_ENABLED(CONFIG_TEGRA20_APB_DMA)) {
> - dev_dbg(i2c_dev->dev, "Support for APB DMA not enabled!\n");
> + dev_dbg(i2c_dev->dev, "dma support not enabled\n");
> return 0;
[...]

DMA is an acronym and so I would usually write it in uppercase for grammatical
correctness unless in a function's name.

Best Regards,
Micha??Miros?aw

2020-09-02 21:18:15

by Dmitry Osipenko

[permalink] [raw]
Subject: Re: [PATCH v1 03/12] i2c: tegra: Clean up messages in the code

02.09.2020 23:42, Michał Mirosław пишет:
> On Mon, Aug 31, 2020 at 11:22:54PM +0300, Dmitry Osipenko wrote:
>> Use lowercase and consistent wording for all messages in the code.
>>
>> Signed-off-by: Dmitry Osipenko <[email protected]>
>> ---
>> drivers/i2c/busses/i2c-tegra.c | 50 ++++++++++++++++------------------
>> 1 file changed, 24 insertions(+), 26 deletions(-)
>>
>> diff --git a/drivers/i2c/busses/i2c-tegra.c b/drivers/i2c/busses/i2c-tegra.c
>> index 9bd91b6f32f4..efbb20049cf8 100644
>> --- a/drivers/i2c/busses/i2c-tegra.c
>> +++ b/drivers/i2c/busses/i2c-tegra.c
>> @@ -427,7 +427,7 @@ static int tegra_i2c_init_dma(struct tegra_i2c_dev *i2c_dev)
>> return 0;
>>
>> if (!IS_ENABLED(CONFIG_TEGRA20_APB_DMA)) {
>> - dev_dbg(i2c_dev->dev, "Support for APB DMA not enabled!\n");
>> + dev_dbg(i2c_dev->dev, "dma support not enabled\n");
>> return 0;
> [...]
>
> DMA is an acronym and so I would usually write it in uppercase for grammatical
> correctness unless in a function's name.

Andy Shevchenko suggested the same thing in other reply, I'll change it
in v3. Thanks!

2020-09-02 21:23:27

by Michał Mirosław

[permalink] [raw]
Subject: Re: [PATCH v1 00/12] Improvements for Tegra I2C driver

On Mon, Aug 31, 2020 at 11:22:51PM +0300, Dmitry Osipenko wrote:
> Hello!
>
> This series performs a small refactoring of the Tegra I2C driver code and
> hardens the atomic-transfer mode.
>
> Dmitry Osipenko (12):
> i2c: tegra: Make tegra_i2c_flush_fifos() usable in atomic transfer
> i2c: tegra: Add missing newline before returns
> i2c: tegra: Clean up messages in the code
> i2c: tegra: Don't ignore tegra_i2c_flush_fifos() error
> i2c: tegra: Use reset_control_reset()
> i2c: tegra: Improve formatting of function variables
> i2c: tegra: Use dev_err_probe()
> i2c: tegra: Runtime PM always available on Tegra
> i2c: tegra: Clean up probe function
> i2c: tegra: Drop '_timeout' from wait/poll function names
> i2c: tegra: Remove likely/unlikely from the code
> i2c: tegra: Factor out error recovery from tegra_i2c_xfer_msg()

For all, but #3 and #9:
Reviewed-by: Micha? Miros?aw <[email protected]>

BTW, I wonder if you could expose i2c_in_atomic_xfer_mode() and use it
to differentiate atomic_xfer from normal and get rid of the internal
flag and .master_xfer_atomic callback.

Best Regards,
Micha? Miros?aw

2020-09-03 01:17:24

by Dmitry Osipenko

[permalink] [raw]
Subject: Re: [PATCH v1 00/12] Improvements for Tegra I2C driver

03.09.2020 00:20, Michał Mirosław пишет:
> BTW, I wonder if you could expose i2c_in_atomic_xfer_mode() and use it
> to differentiate atomic_xfer from normal and get rid of the internal
> flag and .master_xfer_atomic callback.

The atomic transfer uses 90% of the code path that a non-atomic transfer
uses. I don't see how it could be exposed without duplicated lots of the
code, unless I'm not missing what you're suggesting.

2020-09-03 16:48:14

by Michał Mirosław

[permalink] [raw]
Subject: Re: [PATCH v1 00/12] Improvements for Tegra I2C driver

On Thu, Sep 03, 2020 at 04:12:13AM +0300, Dmitry Osipenko wrote:
> 03.09.2020 00:20, Michał Mirosław пишет:
> > BTW, I wonder if you could expose i2c_in_atomic_xfer_mode() and use it
> > to differentiate atomic_xfer from normal and get rid of the internal
> > flag and .master_xfer_atomic callback.
>
> The atomic transfer uses 90% of the code path that a non-atomic transfer
> uses. I don't see how it could be exposed without duplicated lots of the
> code, unless I'm not missing what you're suggesting.

The I2C core falls back to .master_xfer even in atomic mode if
.master_xfer_atomic is NULL, so what I'm suggesting is to make
i2c_in_atomic_xfer_mode() public (from i2c-core.h) and use it in
normal .master_xfer to choose atomic wait variants.

Best Regards,
Michał Mirosław

2020-09-03 22:20:53

by Dmitry Osipenko

[permalink] [raw]
Subject: Re: [PATCH v1 00/12] Improvements for Tegra I2C driver

03.09.2020 19:47, Michał Mirosław пишет:
> On Thu, Sep 03, 2020 at 04:12:13AM +0300, Dmitry Osipenko wrote:
>> 03.09.2020 00:20, Michał Mirosław пишет:
>>> BTW, I wonder if you could expose i2c_in_atomic_xfer_mode() and use it
>>> to differentiate atomic_xfer from normal and get rid of the internal
>>> flag and .master_xfer_atomic callback.
>>
>> The atomic transfer uses 90% of the code path that a non-atomic transfer
>> uses. I don't see how it could be exposed without duplicated lots of the
>> code, unless I'm not missing what you're suggesting.
>
> The I2C core falls back to .master_xfer even in atomic mode if
> .master_xfer_atomic is NULL, so what I'm suggesting is to make
> i2c_in_atomic_xfer_mode() public (from i2c-core.h) and use it in
> normal .master_xfer to choose atomic wait variants.

Okay, I see now. But the I2C core prints a noisy warning if
master_xfer_atomic is NULL in atomic transfer, so I'm not sure whether
changing all that code will bring much benefits to us and anyone else.
It's a bit too questionable change to me, but maybe I'm still missing
something. Will be great if you could provide an example patch.