| Message ID | 20200908224006.25636-1-digetx@gmail.com |
|---|---|
| Headers | show |
| Series | Improvements for Tegra I2C driver | expand |
On Wed, Sep 09, 2020 at 06:36:50PM +0300, Dmitry Osipenko wrote: > 09.09.2020 12:11, Andy Shevchenko пишет: > > On Wed, Sep 9, 2020 at 1:40 AM Dmitry Osipenko <digetx@gmail.com> 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 <andy.shevchenko@gmail.com> > > Thank you and Michał for helping with the review! Very appreciate this! Yes, thanks everyone so far! Is there some internal testfarm where this should be regression tested? Otherwise, I'd trust Dmitry, Andy, and Michał here and would apply it this week after some generic high-level review.
On Wed, Sep 09, 2020 at 01:39:34AM +0300, Dmitry Osipenko wrote: > The pm_runtime_get_sync() always bumps refcount regardless of whether it > succeeds or fails. Hence driver is responsible for restoring of the RPM > refcounting. This patch adds missing RPM puts which restore refcounting > in a case of pm_runtime_get_sync() error. > > Suggested-by: Andy Shevchenko <andy.shevchenko@gmail.com> > Signed-off-by: Dmitry Osipenko <digetx@gmail.com> > --- > drivers/i2c/busses/i2c-tegra.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) Reviewed-by: Thierry Reding <treding@nvidia.com>
On Wed, Sep 09, 2020 at 01:39:35AM +0300, Dmitry Osipenko wrote: > Technically the tegra_i2c_flush_fifos() may fail and transfer should be > aborted in this case, but this shouldn't ever happen in practice unless > there is a bug somewhere in the driver. Let's add the error check just > for completeness. > > Reviewed-by: Michał Mirosław <mirq-linux@rere.qmqm.pl> > Signed-off-by: Dmitry Osipenko <digetx@gmail.com> > --- > drivers/i2c/busses/i2c-tegra.c | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) Reviewed-by: Thierry Reding <treding@nvidia.com> > diff --git a/drivers/i2c/busses/i2c-tegra.c b/drivers/i2c/busses/i2c-tegra.c > index 4e7d0eec0dd3..88d6e7bb14a2 100644 > --- a/drivers/i2c/busses/i2c-tegra.c > +++ b/drivers/i2c/busses/i2c-tegra.c > @@ -1177,7 +1177,9 @@ static int tegra_i2c_xfer_msg(struct tegra_i2c_dev *i2c_dev, > bool dma; > u16 xfer_time = 100; > > - tegra_i2c_flush_fifos(i2c_dev); > + err = tegra_i2c_flush_fifos(i2c_dev); > + if (err) > + return err; > > i2c_dev->msg_buf = msg->buf; > i2c_dev->msg_buf_remaining = msg->len; > -- > 2.27.0 >
On Wed, Sep 09, 2020 at 01:39:37AM +0300, Dmitry Osipenko wrote: > 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 <mirq-linux@rere.qmqm.pl> > Signed-off-by: Dmitry Osipenko <digetx@gmail.com> > --- > drivers/i2c/busses/i2c-tegra.c | 34 ++++++++++++++++------------------ > 1 file changed, 16 insertions(+), 18 deletions(-) Acked-by: Thierry Reding <treding@nvidia.com>
On Wed, Sep 09, 2020 at 01:39:38AM +0300, Dmitry Osipenko wrote: > 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. > > Reviewed-by: Michał Mirosław <mirq-linux@rere.qmqm.pl> > Signed-off-by: Dmitry Osipenko <digetx@gmail.com> > --- > drivers/i2c/busses/i2c-tegra.c | 46 ++++++++++++++++------------------ > 1 file changed, 21 insertions(+), 25 deletions(-) > > diff --git a/drivers/i2c/busses/i2c-tegra.c b/drivers/i2c/busses/i2c-tegra.c > index 720a75439e91..85ed0e02d48c 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,33 @@ 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: Is there are particular reason for switching the simple conditional to a switch here? The old variant looks much easier to understand to me. Thierry
On Wed, Sep 09, 2020 at 01:39:39AM +0300, Dmitry Osipenko wrote: > 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. > > Reviewed-by: Michał Mirosław <mirq-linux@rere.qmqm.pl> > Signed-off-by: Dmitry Osipenko <digetx@gmail.com> > --- > drivers/i2c/busses/i2c-tegra.c | 29 ++++++----------------------- > 1 file changed, 6 insertions(+), 23 deletions(-) Reviewed-by: Thierry Reding <treding@nvidia.com>
On Wed, Sep 09, 2020 at 01:39:40AM +0300, Dmitry Osipenko wrote: > The error message prints number of vIRQ, which isn't a useful information. > In practice devm_request_irq() never fails, hence let's remove the bogus > message in order to make code cleaner. > > Reviewed-by: Michał Mirosław <mirq-linux@rere.qmqm.pl> > Signed-off-by: Dmitry Osipenko <digetx@gmail.com> > --- > drivers/i2c/busses/i2c-tegra.c | 4 +--- > 1 file changed, 1 insertion(+), 3 deletions(-) I think I have seen this fail occasionally when something's wrong in the IRQ code, or when we are not properly configuring the IRQ in device tree for example. So I'd prefer if this error message stayed here. I agree that it's not a terribly useful error message, so perhaps adding the error code to it would make it more helpful? Thierry > diff --git a/drivers/i2c/busses/i2c-tegra.c b/drivers/i2c/busses/i2c-tegra.c > index a52c72135390..b813c0976c10 100644 > --- a/drivers/i2c/busses/i2c-tegra.c > +++ b/drivers/i2c/busses/i2c-tegra.c > @@ -1807,10 +1807,8 @@ 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); > + if (ret) > goto release_dma; > - } > > i2c_set_adapdata(&i2c_dev->adapter, i2c_dev); > i2c_dev->adapter.owner = THIS_MODULE; > -- > 2.27.0 >
On Wed, Sep 09, 2020 at 01:39:44AM +0300, Dmitry Osipenko wrote: > Use clk-bulk helpers and factor out clocks initialization into separate > function in order to make code cleaner. > > The clocks initialization now performed after reset-control initialization > in order to avoid a noisy -PROBE_DEFER errors on T186+ from the clk-bulk > helper which doesn't silence this error code. Hence 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 the clocks retrieval and now this > patch makes dev_err_probe() to be used for the rst error handling. > > Suggested-by: Andy Shevchenko <andy.shevchenko@gmail.com> > Signed-off-by: Dmitry Osipenko <digetx@gmail.com> > --- > drivers/i2c/busses/i2c-tegra.c | 187 ++++++++++++--------------------- > 1 file changed, 67 insertions(+), 120 deletions(-) This is tempting from a diffstat point of view, but the downside is that we can now no longer validate that all of the necessary clocks are given in device tree. Previously the driver would fail to probe the I2C controller if any of the expected clocks were not defined in device tree, but now it's just going to continue without it and not give any indication as to what's wrong. Thierry
On Wed, Sep 09, 2020 at 01:39:53AM +0300, Dmitry Osipenko wrote: > The DMA code path has been tested well enough and the DMA configuration > performed by tegra_i2c_config_fifo_trig() shouldn't ever fail in practice. > Hence let's remove the obscure transfer-mode switching in order to have a > cleaner and simpler code. Now I2C transfer will be failed if DMA > configuration fails. > > Signed-off-by: Dmitry Osipenko <digetx@gmail.com> > --- > drivers/i2c/busses/i2c-tegra.c | 18 ++++++++++-------- > 1 file changed, 10 insertions(+), 8 deletions(-) I'm not sure that's a good idea. It's always possible that the DMA setup is going to break because of something that's not related to the I2C driver itself. Having the system completely break instead of falling back to PIO mode seems like it would only complicate troubleshooting any such issues. Thierry
On Wed, Sep 09, 2020 at 01:39:54AM +0300, Dmitry Osipenko wrote: > Drop '_timeout' postfix from the wait/poll completion function names in > order to make the names shorter, making code cleaner a tad. > > Reviewed-by: Michał Mirosław <mirq-linux@rere.qmqm.pl> > Signed-off-by: Dmitry Osipenko <digetx@gmail.com> > --- > drivers/i2c/busses/i2c-tegra.c | 32 ++++++++++++++------------------ > 1 file changed, 14 insertions(+), 18 deletions(-) Not sure this is really worth it, but I don't feel strongly, so: Acked-by: Thierry Reding <treding@nvidia.com>
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 <mirq-linux@rere.qmqm.pl> > Signed-off-by: Dmitry Osipenko <digetx@gmail.com> > --- > 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, 1000000); > - else > - err = readl_relaxed_poll_timeout(addr, val, !(val & mask), > - 1000, 1000000); > - > + err = tegra_i2c_poll_register(i2c_dev, offset, mask, 1000, 1000000); > 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, 0xffffffff, > + 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
On Wed, Sep 09, 2020 at 01:40:00AM +0300, Dmitry Osipenko wrote: > Consolidate error handling in tegra_i2c_xfer_msg() into a common code > path in order to make code cleaner. > > Reviewed-by: Michał Mirosław <mirq-linux@rere.qmqm.pl> > Signed-off-by: Dmitry Osipenko <digetx@gmail.com> > --- > drivers/i2c/busses/i2c-tegra.c | 13 +++++++++---- > 1 file changed, 9 insertions(+), 4 deletions(-) I'm really not sure this is cleaner. You've got a net positive diffstat and you add goto. That's not always bad, but in this case there is no need for any complicated error unwinding, so I don't think this is any better than the previous code. Thierry
On Wed, Sep 09, 2020 at 01:40:01AM +0300, Dmitry Osipenko wrote: > Reorder definition of variables in the code to have them sorted by length > and grouped logically, also replace "unsigned long" with "u32". Do this in > order to make code easier to read. > > Signed-off-by: Dmitry Osipenko <digetx@gmail.com> > --- > drivers/i2c/busses/i2c-tegra.c | 97 ++++++++++++++++------------------ > 1 file changed, 45 insertions(+), 52 deletions(-) > > diff --git a/drivers/i2c/busses/i2c-tegra.c b/drivers/i2c/busses/i2c-tegra.c > index ac40c87f1c21..2376f502d299 100644 > --- a/drivers/i2c/busses/i2c-tegra.c > +++ b/drivers/i2c/busses/i2c-tegra.c > @@ -259,42 +259,48 @@ struct tegra_i2c_hw_feature { > */ > struct tegra_i2c_dev { > struct device *dev; > - const struct tegra_i2c_hw_feature *hw; > struct i2c_adapter adapter; > - struct clk *div_clk; > - struct clk_bulk_data *clocks; > - unsigned int nclocks; > + > + const struct tegra_i2c_hw_feature *hw; > struct reset_control *rst; > - void __iomem *base; > - phys_addr_t base_phys; > unsigned int cont_id; > unsigned int irq; > - bool is_dvc; > - bool is_vi; > + > + phys_addr_t base_phys; > + void __iomem *base; > + > + struct clk_bulk_data *clocks; > + unsigned int nclocks; > + > + struct clk *div_clk; > + u32 bus_clk_rate; > + > struct completion msg_complete; > + size_t msg_buf_remaining; > int msg_err; > u8 *msg_buf; > - size_t msg_buf_remaining; > - bool msg_read; > - u32 bus_clk_rate; > - bool is_multimaster_mode; > + > + struct completion dma_complete; > struct dma_chan *tx_dma_chan; > struct dma_chan *rx_dma_chan; > + unsigned int dma_buf_size; > dma_addr_t dma_phys; > u32 *dma_buf; > - unsigned int dma_buf_size; > - bool is_curr_dma_xfer; > - struct completion dma_complete; > + > + bool is_multimaster_mode; > bool is_curr_atomic_xfer; > + bool is_curr_dma_xfer; > + bool msg_read; > + bool is_dvc; > + bool is_vi; > }; > > -static void dvc_writel(struct tegra_i2c_dev *i2c_dev, u32 val, > - unsigned long reg) > +static void dvc_writel(struct tegra_i2c_dev *i2c_dev, u32 val, u32 reg) I actually prefer unsigned long/int over u32 for offsets because it makes it clearer that this is not in fact a 32-bit value that we're writing into a register. This is especially true for these register accessors where the "offset" is called "reg" and may be easily mistaken for a register value. Thierry
On Wed, Sep 09, 2020 at 01:40:02AM +0300, Dmitry Osipenko wrote: > Rename "ret" variables to "err" in order to make code a bit more > expressive, emphasizing that the returned value is an error code. > Same vice versa, where appropriate. > > Rename variable "reg" to "val" in order to better reflect the actual > usage of the variable in the code and to make naming consistent with > the rest of the code. > > Use briefer names for a few members of the tegra_i2c_dev structure in > order to improve readability of the code. > > All dev/&pdev->dev are replaced with i2c_dev->dev in order to have uniform > code style across the driver. > > Signed-off-by: Dmitry Osipenko <digetx@gmail.com> > --- > drivers/i2c/busses/i2c-tegra.c | 173 ++++++++++++++++----------------- > 1 file changed, 86 insertions(+), 87 deletions(-) That's indeed a nice improvement. One thing did spring out at me, though. > diff --git a/drivers/i2c/busses/i2c-tegra.c b/drivers/i2c/busses/i2c-tegra.c [...] > @@ -1831,20 +1830,20 @@ static int __maybe_unused tegra_i2c_runtime_suspend(struct device *dev) > > clk_bulk_disable(i2c_dev->nclocks, i2c_dev->clocks); > > - return pinctrl_pm_select_idle_state(i2c_dev->dev); > + return pinctrl_pm_select_idle_state(dev); > } > > static int __maybe_unused tegra_i2c_suspend(struct device *dev) > { > struct tegra_i2c_dev *i2c_dev = dev_get_drvdata(dev); > - int err = 0; > + int ret = 0; > > i2c_mark_adapter_suspended(&i2c_dev->adapter); > > if (!pm_runtime_status_suspended(dev)) > - err = tegra_i2c_runtime_suspend(dev); > + ret = tegra_i2c_runtime_suspend(dev); > > - return err; > + return ret; > } Isn't this exactly the opposite of what the commit message says (and the rest of the patch does)? Thierry
On Wed, Sep 09, 2020 at 01:40:04AM +0300, Dmitry Osipenko wrote: > Make all comments to be consistent in regards to capitalization and > punctuation, correct spelling and grammar errors, improve wording. > > Signed-off-by: Dmitry Osipenko <digetx@gmail.com> > --- > drivers/i2c/busses/i2c-tegra.c | 88 ++++++++++++++++++---------------- > 1 file changed, 47 insertions(+), 41 deletions(-) > > diff --git a/drivers/i2c/busses/i2c-tegra.c b/drivers/i2c/busses/i2c-tegra.c > index 558b1f2934a0..31fbc6181dd5 100644 > --- a/drivers/i2c/busses/i2c-tegra.c > +++ b/drivers/i2c/busses/i2c-tegra.c > @@ -136,7 +136,7 @@ > /* configuration load timeout in microseconds */ > #define I2C_CONFIG_LOAD_TIMEOUT 1000000 > > -/* Packet header size in bytes */ > +/* packet header size in bytes */ > #define I2C_PACKET_HEADER_SIZE 12 > > /* > @@ -148,11 +148,10 @@ > #define I2C_PIO_MODE_PREFERRED_LEN 32 > > /* > - * msg_end_type: The bus control which need to be send at end of transfer. > - * @MSG_END_STOP: Send stop pulse at end of transfer. > - * @MSG_END_REPEAT_START: Send repeat start at end of transfer. > - * @MSG_END_CONTINUE: The following on message is coming and so do not send > - * stop or repeat start. > + * msg_end_type: The bus control which needs to be sent at end of transfer. > + * @MSG_END_STOP: Send stop pulse. > + * @MSG_END_REPEAT_START: Send repeat-start. > + * @MSG_END_CONTINUE: Don't send stop or repeat-start. > */ > enum msg_end_type { > MSG_END_STOP, > @@ -161,10 +160,10 @@ enum msg_end_type { > }; > > /** > - * struct tegra_i2c_hw_feature : Different HW support on Tegra > - * @has_continue_xfer_support: Continue transfer supports. > + * struct tegra_i2c_hw_feature : per hardware generation features I think that space before ':' can go away. Although that's preexisting, so could also be a separate patch, I guess. > + * @has_continue_xfer_support: Continue-transfer supported. This isn't a proper sentence, so I don't think it should have a full-stop. And it shouldn't start with an uppercase letter, either. > * @has_per_pkt_xfer_complete_irq: Has enable/disable capability for transfer > - * complete interrupt per packet basis. > + * completion interrupt on per packet basis. > * @has_config_load_reg: Has the config load register to load the new > * configuration. > * @clk_divisor_hs_mode: Clock divisor in HS mode. > @@ -184,7 +183,7 @@ enum msg_end_type { > * @has_mst_fifo: The I2C controller contains the new MST FIFO interface that > * provides additional features and allows for longer messages to > * be transferred in one go. > - * @quirks: i2c adapter quirks for limiting write/read transfer size and not > + * @quirks: I2C adapter quirks for limiting write/read transfer size and not > * allowing 0 length transfers. > * @supports_bus_clear: Bus Clear support to recover from bus hang during > * SDA stuck low from device for some unknown reasons. > @@ -245,7 +244,7 @@ struct tegra_i2c_hw_feature { > * @msg_err: error code for completed message > * @msg_buf: pointer to current message data > * @msg_buf_remaining: size of unsent data in the message buffer > - * @msg_read: identifies read transfers > + * @msg_read: indicates read direction of a transfer Hm... "read" is the "direction". Perhaps "indicates the direction of a transfer"? Or perhaps "indicates that the transfer is a read access"? [...] > @@ -1797,9 +1804,8 @@ static int __maybe_unused tegra_i2c_runtime_resume(struct device *dev) > > /* > * VI I2C device is attached to VE power domain which goes through > - * power ON/OFF during PM runtime resume/suspend. So, controller > - * should go through reset and need to re-initialize after power > - * domain ON. > + * power ON/OFF during of runtime PM resume/suspend, meaning that s/during of runtime PM/during runtime PM/ Thierry
On Wed, Sep 09, 2020 at 01:40:06AM +0300, Dmitry Osipenko wrote: > Use proper spelling of "NVIDIA" and don't designate driver as Tegra2-only > since newer SoC generations are supported as well. > > Reviewed-by: Michał Mirosław <mirq-linux@rere.qmqm.pl> > Signed-off-by: Dmitry Osipenko <digetx@gmail.com> > --- > drivers/i2c/busses/i2c-tegra.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) Acked-by: Thierry Reding <treding@nvidia.com>
On Wed, Sep 09, 2020 at 01:39:48AM +0300, Dmitry Osipenko wrote: > Don't use signed types for unsigned values and use consistent types > for sibling variables. > > Signed-off-by: Dmitry Osipenko <digetx@gmail.com> > --- > drivers/i2c/busses/i2c-tegra.c | 38 +++++++++++++++++----------------- > 1 file changed, 19 insertions(+), 19 deletions(-) Acked-by: Thierry Reding <treding@nvidia.com>
On Wed, Sep 09, 2020 at 01:39:51AM +0300, Dmitry Osipenko wrote: > The tegra_i2c_wait_for_config_load() checks for 'has_config_load_reg' by > itself, hence there is no need to duplicate the check. > > Signed-off-by: Dmitry Osipenko <digetx@gmail.com> > --- > drivers/i2c/busses/i2c-tegra.c | 9 ++++----- > 1 file changed, 4 insertions(+), 5 deletions(-) Acked-by: Thierry Reding <treding@nvidia.com>
On Wed, Sep 09, 2020 at 05:49:02PM +0200, Wolfram Sang wrote: > On Wed, Sep 09, 2020 at 06:36:50PM +0300, Dmitry Osipenko wrote: > > 09.09.2020 12:11, Andy Shevchenko пишет: > > > On Wed, Sep 9, 2020 at 1:40 AM Dmitry Osipenko <digetx@gmail.com> 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 <andy.shevchenko@gmail.com> > > > > Thank you and Michał for helping with the review! Very appreciate this! > > Yes, thanks everyone so far! > > Is there some internal testfarm where this should be regression tested? > Otherwise, I'd trust Dmitry, Andy, and Michał here and would apply it > this week after some generic high-level review. I'll queue this for a run on the test farm. I had a couple of minor comments, but after going through the full series I'm pretty happy overall with the result, so I'll go over my comments again and will reevaluate. Thierry
On Thu, Sep 17, 2020 at 2:38 PM Thierry Reding <thierry.reding@gmail.com> wrote: > On Wed, Sep 09, 2020 at 01:39:44AM +0300, Dmitry Osipenko wrote: > > Use clk-bulk helpers and factor out clocks initialization into separate > > function in order to make code cleaner. > > > > The clocks initialization now performed after reset-control initialization > > in order to avoid a noisy -PROBE_DEFER errors on T186+ from the clk-bulk > > helper which doesn't silence this error code. Hence 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 the clocks retrieval and now this > > patch makes dev_err_probe() to be used for the rst error handling. > > > > Suggested-by: Andy Shevchenko <andy.shevchenko@gmail.com> > > Signed-off-by: Dmitry Osipenko <digetx@gmail.com> > > --- > > drivers/i2c/busses/i2c-tegra.c | 187 ++++++++++++--------------------- > > 1 file changed, 67 insertions(+), 120 deletions(-) > > This is tempting from a diffstat point of view, but the downside is that > we can now no longer validate that all of the necessary clocks are given > in device tree. > > Previously the driver would fail to probe the I2C controller if any of > the expected clocks were not defined in device tree, but now it's just > going to continue without it and not give any indication as to what's > wrong. You may print an error in the error path as previously. Since both clocks are mandatory (as far as I understood the code) user will need to check DT in any case.
17.09.2020 14:28, Thierry Reding пишет: > On Wed, Sep 09, 2020 at 01:39:40AM +0300, Dmitry Osipenko wrote: >> The error message prints number of vIRQ, which isn't a useful information. >> In practice devm_request_irq() never fails, hence let's remove the bogus >> message in order to make code cleaner. >> >> Reviewed-by: Michał Mirosław <mirq-linux@rere.qmqm.pl> >> Signed-off-by: Dmitry Osipenko <digetx@gmail.com> >> --- >> drivers/i2c/busses/i2c-tegra.c | 4 +--- >> 1 file changed, 1 insertion(+), 3 deletions(-) > > I think I have seen this fail occasionally when something's wrong in the > IRQ code, or when we are not properly configuring the IRQ in device tree > for example. > > So I'd prefer if this error message stayed here. I agree that it's not a > terribly useful error message, so perhaps adding the error code to it > would make it more helpful? We have dtbs_check nowadays and I assume you only saw a such problem during of hardware bring-up, correct? Realistically, this error should never happen "randomly" and even if it will happen, then you will still know that I2C driver failed because driver-core will tell you about that. Hence there is no much value in the error message, it should be more practical to remove it in order to keep the code cleaner. >> diff --git a/drivers/i2c/busses/i2c-tegra.c b/drivers/i2c/busses/i2c-tegra.c >> index a52c72135390..b813c0976c10 100644 >> --- a/drivers/i2c/busses/i2c-tegra.c >> +++ b/drivers/i2c/busses/i2c-tegra.c >> @@ -1807,10 +1807,8 @@ 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); >> + if (ret) >> goto release_dma; >> - } >> >> i2c_set_adapdata(&i2c_dev->adapter, i2c_dev); >> i2c_dev->adapter.owner = THIS_MODULE; >> -- >> 2.27.0 >>
17.09.2020 14:38, Thierry Reding пишет: > On Wed, Sep 09, 2020 at 01:39:44AM +0300, Dmitry Osipenko wrote: >> Use clk-bulk helpers and factor out clocks initialization into separate >> function in order to make code cleaner. >> >> The clocks initialization now performed after reset-control initialization >> in order to avoid a noisy -PROBE_DEFER errors on T186+ from the clk-bulk >> helper which doesn't silence this error code. Hence 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 the clocks retrieval and now this >> patch makes dev_err_probe() to be used for the rst error handling. >> >> Suggested-by: Andy Shevchenko <andy.shevchenko@gmail.com> >> Signed-off-by: Dmitry Osipenko <digetx@gmail.com> >> --- >> drivers/i2c/busses/i2c-tegra.c | 187 ++++++++++++--------------------- >> 1 file changed, 67 insertions(+), 120 deletions(-) > > This is tempting from a diffstat point of view, but the downside is that > we can now no longer validate that all of the necessary clocks are given > in device tree. > > Previously the driver would fail to probe the I2C controller if any of > the expected clocks were not defined in device tree, but now it's just > going to continue without it and not give any indication as to what's > wrong. The clocks can't be missed randomly in a device-tree because they are a part of the core tegra.dtsi The tegra-i2c DT binding isn't converted to YAML, but once it will be, then the dtbs_check will tell you about such obvious problems like a missing mandatory property. Even if clock is missing, then you won't miss this problem since I2C shouldn't work in that case. There is a Qualcomm I2C driver that already uses clk_bulk_get_all() and doesn't worry about "accidentally" missing clocks. It's still possible to add the clk-num checking, but it should be unpractical. We could always add it later on if there will be a real incident. Do you agree?
17.09.2020 15:32, Thierry Reding пишет: ... >> /** >> - * struct tegra_i2c_hw_feature : Different HW support on Tegra >> - * @has_continue_xfer_support: Continue transfer supports. >> + * struct tegra_i2c_hw_feature : per hardware generation features > > I think that space before ':' can go away. Although that's preexisting, > so could also be a separate patch, I guess. I haven't even noticed that!
17.09.2020 14:47, Thierry Reding пишет: > On Wed, Sep 09, 2020 at 01:39:53AM +0300, Dmitry Osipenko wrote: >> The DMA code path has been tested well enough and the DMA configuration >> performed by tegra_i2c_config_fifo_trig() shouldn't ever fail in practice. >> Hence let's remove the obscure transfer-mode switching in order to have a >> cleaner and simpler code. Now I2C transfer will be failed if DMA >> configuration fails. >> >> Signed-off-by: Dmitry Osipenko <digetx@gmail.com> >> --- >> drivers/i2c/busses/i2c-tegra.c | 18 ++++++++++-------- >> 1 file changed, 10 insertions(+), 8 deletions(-) > > I'm not sure that's a good idea. It's always possible that the DMA setup > is going to break because of something that's not related to the I2C > driver itself. Having the system completely break instead of falling > back to PIO mode seems like it would only complicate troubleshooting any > such issues. That code has zero test coverage because this problem never happens in practice, hence it should be better to have it removed. We may consider re-adding it back if there will be a real-world incident, okay?
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 <mirq-linux@rere.qmqm.pl> >> Signed-off-by: Dmitry Osipenko <digetx@gmail.com> >> --- >> 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, 1000000); >> - else >> - err = readl_relaxed_poll_timeout(addr, val, !(val & mask), >> - 1000, 1000000); >> - >> + err = tegra_i2c_poll_register(i2c_dev, offset, mask, 1000, 1000000); >> 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, 0xffffffff, >> + 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.
17.09.2020 15:16, Thierry Reding пишет: > On Wed, Sep 09, 2020 at 01:40:01AM +0300, Dmitry Osipenko wrote: >> Reorder definition of variables in the code to have them sorted by length >> and grouped logically, also replace "unsigned long" with "u32". Do this in >> order to make code easier to read. >> >> Signed-off-by: Dmitry Osipenko <digetx@gmail.com> >> --- >> drivers/i2c/busses/i2c-tegra.c | 97 ++++++++++++++++------------------ >> 1 file changed, 45 insertions(+), 52 deletions(-) >> >> diff --git a/drivers/i2c/busses/i2c-tegra.c b/drivers/i2c/busses/i2c-tegra.c >> index ac40c87f1c21..2376f502d299 100644 >> --- a/drivers/i2c/busses/i2c-tegra.c >> +++ b/drivers/i2c/busses/i2c-tegra.c >> @@ -259,42 +259,48 @@ struct tegra_i2c_hw_feature { >> */ >> struct tegra_i2c_dev { >> struct device *dev; >> - const struct tegra_i2c_hw_feature *hw; >> struct i2c_adapter adapter; >> - struct clk *div_clk; >> - struct clk_bulk_data *clocks; >> - unsigned int nclocks; >> + >> + const struct tegra_i2c_hw_feature *hw; >> struct reset_control *rst; >> - void __iomem *base; >> - phys_addr_t base_phys; >> unsigned int cont_id; >> unsigned int irq; >> - bool is_dvc; >> - bool is_vi; >> + >> + phys_addr_t base_phys; >> + void __iomem *base; >> + >> + struct clk_bulk_data *clocks; >> + unsigned int nclocks; >> + >> + struct clk *div_clk; >> + u32 bus_clk_rate; >> + >> struct completion msg_complete; >> + size_t msg_buf_remaining; >> int msg_err; >> u8 *msg_buf; >> - size_t msg_buf_remaining; >> - bool msg_read; >> - u32 bus_clk_rate; >> - bool is_multimaster_mode; >> + >> + struct completion dma_complete; >> struct dma_chan *tx_dma_chan; >> struct dma_chan *rx_dma_chan; >> + unsigned int dma_buf_size; >> dma_addr_t dma_phys; >> u32 *dma_buf; >> - unsigned int dma_buf_size; >> - bool is_curr_dma_xfer; >> - struct completion dma_complete; >> + >> + bool is_multimaster_mode; >> bool is_curr_atomic_xfer; >> + bool is_curr_dma_xfer; >> + bool msg_read; >> + bool is_dvc; >> + bool is_vi; >> }; >> >> -static void dvc_writel(struct tegra_i2c_dev *i2c_dev, u32 val, >> - unsigned long reg) >> +static void dvc_writel(struct tegra_i2c_dev *i2c_dev, u32 val, u32 reg) > > I actually prefer unsigned long/int over u32 for offsets because it > makes it clearer that this is not in fact a 32-bit value that we're > writing into a register. This is especially true for these register > accessors where the "offset" is called "reg" and may be easily > mistaken for a register value. That is a bit questionable, at least it definitely won't save me from a mistake :)
17.09.2020 18:02, Dmitry Osipenko пишет: > 17.09.2020 15:32, Thierry Reding пишет: > ... >>> /** >>> - * struct tegra_i2c_hw_feature : Different HW support on Tegra >>> - * @has_continue_xfer_support: Continue transfer supports. >>> + * struct tegra_i2c_hw_feature : per hardware generation features >> >> I think that space before ':' can go away. Although that's preexisting, >> so could also be a separate patch, I guess. > > I haven't even noticed that! > Wait, that ':' is used only for the struct description, hence it actually looks natural in the code.
17.09.2020 14:25, Thierry Reding пишет: > On Wed, Sep 09, 2020 at 01:39:38AM +0300, Dmitry Osipenko wrote: >> 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. >> >> Reviewed-by: Michał Mirosław <mirq-linux@rere.qmqm.pl> >> Signed-off-by: Dmitry Osipenko <digetx@gmail.com> >> --- >> drivers/i2c/busses/i2c-tegra.c | 46 ++++++++++++++++------------------ >> 1 file changed, 21 insertions(+), 25 deletions(-) >> >> diff --git a/drivers/i2c/busses/i2c-tegra.c b/drivers/i2c/busses/i2c-tegra.c >> index 720a75439e91..85ed0e02d48c 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,33 @@ 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: > > Is there are particular reason for switching the simple conditional to a > switch here? The old variant looks much easier to understand to me. The reason is make it readable :) For me it's too difficult to read > <= && { } + no proper indentation. The switches are more suitable for ranges, IMO. Especially when there are multiple ranges, and there could be more ranges in the future in this code.
17.09.2020 15:21, Thierry Reding пишет: > On Wed, Sep 09, 2020 at 01:40:02AM +0300, Dmitry Osipenko wrote: >> Rename "ret" variables to "err" in order to make code a bit more >> expressive, emphasizing that the returned value is an error code. >> Same vice versa, where appropriate. >> >> Rename variable "reg" to "val" in order to better reflect the actual >> usage of the variable in the code and to make naming consistent with >> the rest of the code. >> >> Use briefer names for a few members of the tegra_i2c_dev structure in >> order to improve readability of the code. >> >> All dev/&pdev->dev are replaced with i2c_dev->dev in order to have uniform >> code style across the driver. >> >> Signed-off-by: Dmitry Osipenko <digetx@gmail.com> >> --- >> drivers/i2c/busses/i2c-tegra.c | 173 ++++++++++++++++----------------- >> 1 file changed, 86 insertions(+), 87 deletions(-) > > That's indeed a nice improvement. One thing did spring out at me, > though. > >> diff --git a/drivers/i2c/busses/i2c-tegra.c b/drivers/i2c/busses/i2c-tegra.c > [...] >> @@ -1831,20 +1830,20 @@ static int __maybe_unused tegra_i2c_runtime_suspend(struct device *dev) >> >> clk_bulk_disable(i2c_dev->nclocks, i2c_dev->clocks); >> >> - return pinctrl_pm_select_idle_state(i2c_dev->dev); >> + return pinctrl_pm_select_idle_state(dev); >> } >> >> static int __maybe_unused tegra_i2c_suspend(struct device *dev) >> { >> struct tegra_i2c_dev *i2c_dev = dev_get_drvdata(dev); >> - int err = 0; >> + int ret = 0; >> >> i2c_mark_adapter_suspended(&i2c_dev->adapter); >> >> if (!pm_runtime_status_suspended(dev)) >> - err = tegra_i2c_runtime_suspend(dev); >> + ret = tegra_i2c_runtime_suspend(dev); >> >> - return err; >> + return ret; >> } > > Isn't this exactly the opposite of what the commit message says (and the > rest of the patch does)? This change makes it to be consistent with the rest of the code. You may notice that "Factor out hardware initialization into separate function" made a similar change. The reason I'm doing this is that the "err" suggests that code returns a error failure code, while it could be a success too and you don't know for sure by looking only at the part of code. Hence it's cleaner to use "err" when error code is returned. It is possible (and maybe even better) to rewrite this function as: static int __maybe_unused tegra_i2c_suspend(struct device *dev) { struct tegra_i2c_dev *i2c_dev = dev_get_drvdata(dev); i2c_mark_adapter_suspended(&i2c_dev->adapter); if (!pm_runtime_status_suspended(dev)) { int err = tegra_i2c_runtime_suspend(dev); if (err) return err; } return 0; }
On Thu, Sep 17, 2020 at 02:44:18PM +0200, Thierry Reding wrote: > On Wed, Sep 09, 2020 at 05:49:02PM +0200, Wolfram Sang wrote: > > On Wed, Sep 09, 2020 at 06:36:50PM +0300, Dmitry Osipenko wrote: > > > 09.09.2020 12:11, Andy Shevchenko пишет: > > > > On Wed, Sep 9, 2020 at 1:40 AM Dmitry Osipenko <digetx@gmail.com> 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 <andy.shevchenko@gmail.com> > > > > > > Thank you and Michał for helping with the review! Very appreciate this! > > > > Yes, thanks everyone so far! > > > > Is there some internal testfarm where this should be regression tested? > > Otherwise, I'd trust Dmitry, Andy, and Michał here and would apply it > > this week after some generic high-level review. > > I'll queue this for a run on the test farm. I had a couple of minor > comments, but after going through the full series I'm pretty happy > overall with the result, so I'll go over my comments again and will > reevaluate. Cool, thanks! You guys just let me know if v7 is fine please. Otherwise I will surely notice if a v8 hits the list ;)
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 <andy.shevchenko@gmail.com> 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
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 <andy.shevchenko@gmail.com> 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
On Thu, Sep 17, 2020 at 06:27:28PM +0300, Dmitry Osipenko wrote: > 17.09.2020 14:25, Thierry Reding пишет: > > On Wed, Sep 09, 2020 at 01:39:38AM +0300, Dmitry Osipenko wrote: > >> 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. > >> > >> Reviewed-by: Michał Mirosław <mirq-linux@rere.qmqm.pl> > >> Signed-off-by: Dmitry Osipenko <digetx@gmail.com> > >> --- > >> drivers/i2c/busses/i2c-tegra.c | 46 ++++++++++++++++------------------ > >> 1 file changed, 21 insertions(+), 25 deletions(-) > >> > >> diff --git a/drivers/i2c/busses/i2c-tegra.c b/drivers/i2c/busses/i2c-tegra.c > >> index 720a75439e91..85ed0e02d48c 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,33 @@ 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: > > > > Is there are particular reason for switching the simple conditional to a > > switch here? The old variant looks much easier to understand to me. > > The reason is make it readable :) For me it's too difficult to read > <= > && { } + no proper indentation. Guess this is very subjective then... It would've been nice to avoid the + 1 and instead use the correct enumeration value. I think that would've made it a little bit easier on the eye. But anyway, no reason to hold up this patch set: Reviewed-by: Thierry Reding <treding@nvidia.com>
On Thu, Sep 17, 2020 at 05:59:28PM +0300, Dmitry Osipenko wrote: > 17.09.2020 14:28, Thierry Reding пишет: > > On Wed, Sep 09, 2020 at 01:39:40AM +0300, Dmitry Osipenko wrote: > >> The error message prints number of vIRQ, which isn't a useful information. > >> In practice devm_request_irq() never fails, hence let's remove the bogus > >> message in order to make code cleaner. > >> > >> Reviewed-by: Michał Mirosław <mirq-linux@rere.qmqm.pl> > >> Signed-off-by: Dmitry Osipenko <digetx@gmail.com> > >> --- > >> drivers/i2c/busses/i2c-tegra.c | 4 +--- > >> 1 file changed, 1 insertion(+), 3 deletions(-) > > > > I think I have seen this fail occasionally when something's wrong in the > > IRQ code, or when we are not properly configuring the IRQ in device tree > > for example. > > > > So I'd prefer if this error message stayed here. I agree that it's not a > > terribly useful error message, so perhaps adding the error code to it > > would make it more helpful? > > We have dtbs_check nowadays and I assume you only saw a such problem > during of hardware bring-up, correct? dtbs_check is far from perfect, especially since only a handful of bindings have been converted for Tegra. It's also not going to catch any functional issues. You could still have a valid combination of flags passed via DTB and still the interrupt allocation could fail because it just so happens that the particular combination wasn't valid in that particular setup. > Realistically, this error should never happen "randomly" and even if it > will happen, then you will still know that I2C driver failed because > driver-core will tell you about that. And yes, this is the type of error that you typically get during bring-up and it does obviously go away once you've fixed it and then tends not to come back. But that's exactly what happens with most errors and having error messages helps in finding what went wrong. If we drop the error message, then I may notice that the I2C driver didn't probe correctly. But in order to find out why it didn't, I have to go and add error messages to narrow down where I need to look. The whole point of having error messages in the code is to avoid having to go in and modify the code in order to troubleshoot. In the interest of moving this along I'm fine with this for now. But I suspect that we'll run into an issue with this eventually and that we'll have to add an error message here again. If that happens we can always reintroduce one, and perhaps at that time do a better job of making it actually useful. Reviewed-by: Thierry Reding <treding@nvidia.com>
On Thu, Sep 17, 2020 at 04:54:28PM +0300, Andy Shevchenko wrote: > On Thu, Sep 17, 2020 at 2:38 PM Thierry Reding <thierry.reding@gmail.com> wrote: > > On Wed, Sep 09, 2020 at 01:39:44AM +0300, Dmitry Osipenko wrote: > > > Use clk-bulk helpers and factor out clocks initialization into separate > > > function in order to make code cleaner. > > > > > > The clocks initialization now performed after reset-control initialization > > > in order to avoid a noisy -PROBE_DEFER errors on T186+ from the clk-bulk > > > helper which doesn't silence this error code. Hence 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 the clocks retrieval and now this > > > patch makes dev_err_probe() to be used for the rst error handling. > > > > > > Suggested-by: Andy Shevchenko <andy.shevchenko@gmail.com> > > > Signed-off-by: Dmitry Osipenko <digetx@gmail.com> > > > --- > > > drivers/i2c/busses/i2c-tegra.c | 187 ++++++++++++--------------------- > > > 1 file changed, 67 insertions(+), 120 deletions(-) > > > > This is tempting from a diffstat point of view, but the downside is that > > we can now no longer validate that all of the necessary clocks are given > > in device tree. > > > > Previously the driver would fail to probe the I2C controller if any of > > the expected clocks were not defined in device tree, but now it's just > > going to continue without it and not give any indication as to what's > > wrong. > > You may print an error in the error path as previously. Since both > clocks are mandatory (as far as I understood the code) user will need > to check DT in any case. The problem is that the number of required clocks depends on the variant of the IP block that's implemented. Some require just one clock and others require two or three. With this patch the driver is just going to pick whatever clocks are given in device tree, but it removes any possibility of detecting whether the device trees contain the correct clocks. So we may very well run into a situation where the driver now successfully probes but then malfunctions because one or more of the clocks were not specified in device tree. Thierry
On Thu, Sep 17, 2020 at 06:01:56PM +0300, Dmitry Osipenko wrote: > 17.09.2020 14:38, Thierry Reding пишет: > > On Wed, Sep 09, 2020 at 01:39:44AM +0300, Dmitry Osipenko wrote: > >> Use clk-bulk helpers and factor out clocks initialization into separate > >> function in order to make code cleaner. > >> > >> The clocks initialization now performed after reset-control initialization > >> in order to avoid a noisy -PROBE_DEFER errors on T186+ from the clk-bulk > >> helper which doesn't silence this error code. Hence 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 the clocks retrieval and now this > >> patch makes dev_err_probe() to be used for the rst error handling. > >> > >> Suggested-by: Andy Shevchenko <andy.shevchenko@gmail.com> > >> Signed-off-by: Dmitry Osipenko <digetx@gmail.com> > >> --- > >> drivers/i2c/busses/i2c-tegra.c | 187 ++++++++++++--------------------- > >> 1 file changed, 67 insertions(+), 120 deletions(-) > > > > This is tempting from a diffstat point of view, but the downside is that > > we can now no longer validate that all of the necessary clocks are given > > in device tree. > > > > Previously the driver would fail to probe the I2C controller if any of > > the expected clocks were not defined in device tree, but now it's just > > going to continue without it and not give any indication as to what's > > wrong. > > The clocks can't be missed randomly in a device-tree because they are a > part of the core tegra.dtsi You can easily generate a device-tree without using the core DTS includes. > The tegra-i2c DT binding isn't converted to YAML, but once it will be, > then the dtbs_check will tell you about such obvious problems like a > missing mandatory property. Once that has happened, yes, I think we may be able to simplify the driver. But before that happens I don't think we should throw away our only line of defense against broken device trees. > Even if clock is missing, then you won't miss this problem since I2C > shouldn't work in that case. But the whole point of this error handling here is so that we can make it easier to find the cause of an error. I2C malfunctioning can be subtle. You could have some EEPROM on I2C that you normally don't touch, but then at some point you try to access it from userspace and you read garbage and then you need to start looking why this is happening. The whole point of error messages is so that you can easily find the root cause. > There is a Qualcomm I2C driver that already uses clk_bulk_get_all() and > doesn't worry about "accidentally" missing clocks. Just because one particular driver doesn't care doesn't mean everybody else should stop caring. > It's still possible to add the clk-num checking, but it should be > unpractical. We could always add it later on if there will be a real > incident. Do you agree? If there's an incident it's already too late. The whole point of error checking here is to avoid any accidental breakage. Thierry
On Thu, Sep 17, 2020 at 06:01:56PM +0300, Dmitry Osipenko wrote: [...] > It's still possible to add the clk-num checking, but it should be > unpractical. We could always add it later on if there will be a real > incident. Do you agree? There's also clk_bulk_get(), which allows you to specify the number of clocks and their consumer IDs that you want to request. That seems like it would allow us to both avoid the repetitive calls to clk APIs and at the same time allows us to specify exactly which clocks we need. Would that not work as a compromise? Thierry
On Mon, Sep 21, 2020 at 2:02 PM Thierry Reding <thierry.reding@gmail.com> wrote: > On Thu, Sep 17, 2020 at 04:54:28PM +0300, Andy Shevchenko wrote: > > On Thu, Sep 17, 2020 at 2:38 PM Thierry Reding <thierry.reding@gmail.com> wrote: > > > On Wed, Sep 09, 2020 at 01:39:44AM +0300, Dmitry Osipenko wrote: ... > > > This is tempting from a diffstat point of view, but the downside is that > > > we can now no longer validate that all of the necessary clocks are given > > > in device tree. > > > > > > Previously the driver would fail to probe the I2C controller if any of > > > the expected clocks were not defined in device tree, but now it's just > > > going to continue without it and not give any indication as to what's > > > wrong. > > > > You may print an error in the error path as previously. Since both > > clocks are mandatory (as far as I understood the code) user will need > > to check DT in any case. > > The problem is that the number of required clocks depends on the variant > of the IP block that's implemented. Some require just one clock and > others require two or three. With this patch the driver is just going to > pick whatever clocks are given in device tree, but it removes any > possibility of detecting whether the device trees contain the correct > clocks. So we may very well run into a situation where the driver now > successfully probes but then malfunctions because one or more of the > clocks were not specified in device tree. > > Thierry I still failed to get this. Are you suggesting that CCF bulk operations are fundamentally broken? In the above case one may add more checks. AFAICS is_vi won't be removed, so can be easily checked. Basically that for-loop for div_clk is questionable. I agree on that. -- With Best Regards, Andy Shevchenko
On Thu, Sep 17, 2020 at 06:03:04PM +0300, Dmitry Osipenko wrote: > 17.09.2020 14:47, Thierry Reding пишет: > > On Wed, Sep 09, 2020 at 01:39:53AM +0300, Dmitry Osipenko wrote: > >> The DMA code path has been tested well enough and the DMA configuration > >> performed by tegra_i2c_config_fifo_trig() shouldn't ever fail in practice. > >> Hence let's remove the obscure transfer-mode switching in order to have a > >> cleaner and simpler code. Now I2C transfer will be failed if DMA > >> configuration fails. > >> > >> Signed-off-by: Dmitry Osipenko <digetx@gmail.com> > >> --- > >> drivers/i2c/busses/i2c-tegra.c | 18 ++++++++++-------- > >> 1 file changed, 10 insertions(+), 8 deletions(-) > > > > I'm not sure that's a good idea. It's always possible that the DMA setup > > is going to break because of something that's not related to the I2C > > driver itself. Having the system completely break instead of falling > > back to PIO mode seems like it would only complicate troubleshooting any > > such issues. > > That code has zero test coverage because this problem never happens in > practice, hence it should be better to have it removed. We may consider > re-adding it back if there will be a real-world incident, okay? Again, I think throwing out fallbacks and error messages out the window just because they "don't happen in practice" is misguided. Just because they don't *usually* happen doesn't mean they can't happen. And in case they do happen we absolutely do want some way of dealing with it rather than just have the driver stop working without any explanation. Thierry
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 <mirq-linux@rere.qmqm.pl> > >> Signed-off-by: Dmitry Osipenko <digetx@gmail.com> > >> --- > >> 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, 1000000); > >> - else > >> - err = readl_relaxed_poll_timeout(addr, val, !(val & mask), > >> - 1000, 1000000); > >> - > >> + err = tegra_i2c_poll_register(i2c_dev, offset, mask, 1000, 1000000); > >> 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, 0xffffffff, > >> + 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 <treding@nvidia.com>
On Thu, Sep 17, 2020 at 06:13:36PM +0300, Dmitry Osipenko wrote: > 17.09.2020 15:16, Thierry Reding пишет: > > On Wed, Sep 09, 2020 at 01:40:01AM +0300, Dmitry Osipenko wrote: > >> Reorder definition of variables in the code to have them sorted by length > >> and grouped logically, also replace "unsigned long" with "u32". Do this in > >> order to make code easier to read. > >> > >> Signed-off-by: Dmitry Osipenko <digetx@gmail.com> > >> --- > >> drivers/i2c/busses/i2c-tegra.c | 97 ++++++++++++++++------------------ > >> 1 file changed, 45 insertions(+), 52 deletions(-) > >> > >> diff --git a/drivers/i2c/busses/i2c-tegra.c b/drivers/i2c/busses/i2c-tegra.c > >> index ac40c87f1c21..2376f502d299 100644 > >> --- a/drivers/i2c/busses/i2c-tegra.c > >> +++ b/drivers/i2c/busses/i2c-tegra.c > >> @@ -259,42 +259,48 @@ struct tegra_i2c_hw_feature { > >> */ > >> struct tegra_i2c_dev { > >> struct device *dev; > >> - const struct tegra_i2c_hw_feature *hw; > >> struct i2c_adapter adapter; > >> - struct clk *div_clk; > >> - struct clk_bulk_data *clocks; > >> - unsigned int nclocks; > >> + > >> + const struct tegra_i2c_hw_feature *hw; > >> struct reset_control *rst; > >> - void __iomem *base; > >> - phys_addr_t base_phys; > >> unsigned int cont_id; > >> unsigned int irq; > >> - bool is_dvc; > >> - bool is_vi; > >> + > >> + phys_addr_t base_phys; > >> + void __iomem *base; > >> + > >> + struct clk_bulk_data *clocks; > >> + unsigned int nclocks; > >> + > >> + struct clk *div_clk; > >> + u32 bus_clk_rate; > >> + > >> struct completion msg_complete; > >> + size_t msg_buf_remaining; > >> int msg_err; > >> u8 *msg_buf; > >> - size_t msg_buf_remaining; > >> - bool msg_read; > >> - u32 bus_clk_rate; > >> - bool is_multimaster_mode; > >> + > >> + struct completion dma_complete; > >> struct dma_chan *tx_dma_chan; > >> struct dma_chan *rx_dma_chan; > >> + unsigned int dma_buf_size; > >> dma_addr_t dma_phys; > >> u32 *dma_buf; > >> - unsigned int dma_buf_size; > >> - bool is_curr_dma_xfer; > >> - struct completion dma_complete; > >> + > >> + bool is_multimaster_mode; > >> bool is_curr_atomic_xfer; > >> + bool is_curr_dma_xfer; > >> + bool msg_read; > >> + bool is_dvc; > >> + bool is_vi; > >> }; > >> > >> -static void dvc_writel(struct tegra_i2c_dev *i2c_dev, u32 val, > >> - unsigned long reg) > >> +static void dvc_writel(struct tegra_i2c_dev *i2c_dev, u32 val, u32 reg) > > > > I actually prefer unsigned long/int over u32 for offsets because it > > makes it clearer that this is not in fact a 32-bit value that we're > > writing into a register. This is especially true for these register > > accessors where the "offset" is called "reg" and may be easily > > mistaken for a register value. > > That is a bit questionable, at least it definitely won't save me from a > mistake :) There's obviously no way of guaranteeing that nobody makes a mistake. But especially with accessors the value and offset parameters are inconsistently ordered, so any help we can provide at the API level may help avoid mistakes. If you only look at the prototype, having two u32 parameters doesn't give you *any* clue about what they are. But a 32-bit accessor that takes a u32 and an unsigned int is pretty obviously expecting the 32-bit value in the u32 parameter and then the unsigned int must obviously be the offset. Thierry
On Thu, Sep 17, 2020 at 06:43:28PM +0300, Dmitry Osipenko wrote: > 17.09.2020 15:21, Thierry Reding пишет: > > On Wed, Sep 09, 2020 at 01:40:02AM +0300, Dmitry Osipenko wrote: > >> Rename "ret" variables to "err" in order to make code a bit more > >> expressive, emphasizing that the returned value is an error code. > >> Same vice versa, where appropriate. > >> > >> Rename variable "reg" to "val" in order to better reflect the actual > >> usage of the variable in the code and to make naming consistent with > >> the rest of the code. > >> > >> Use briefer names for a few members of the tegra_i2c_dev structure in > >> order to improve readability of the code. > >> > >> All dev/&pdev->dev are replaced with i2c_dev->dev in order to have uniform > >> code style across the driver. > >> > >> Signed-off-by: Dmitry Osipenko <digetx@gmail.com> > >> --- > >> drivers/i2c/busses/i2c-tegra.c | 173 ++++++++++++++++----------------- > >> 1 file changed, 86 insertions(+), 87 deletions(-) > > > > That's indeed a nice improvement. One thing did spring out at me, > > though. > > > >> diff --git a/drivers/i2c/busses/i2c-tegra.c b/drivers/i2c/busses/i2c-tegra.c > > [...] > >> @@ -1831,20 +1830,20 @@ static int __maybe_unused tegra_i2c_runtime_suspend(struct device *dev) > >> > >> clk_bulk_disable(i2c_dev->nclocks, i2c_dev->clocks); > >> > >> - return pinctrl_pm_select_idle_state(i2c_dev->dev); > >> + return pinctrl_pm_select_idle_state(dev); > >> } > >> > >> static int __maybe_unused tegra_i2c_suspend(struct device *dev) > >> { > >> struct tegra_i2c_dev *i2c_dev = dev_get_drvdata(dev); > >> - int err = 0; > >> + int ret = 0; > >> > >> i2c_mark_adapter_suspended(&i2c_dev->adapter); > >> > >> if (!pm_runtime_status_suspended(dev)) > >> - err = tegra_i2c_runtime_suspend(dev); > >> + ret = tegra_i2c_runtime_suspend(dev); > >> > >> - return err; > >> + return ret; > >> } > > > > Isn't this exactly the opposite of what the commit message says (and the > > rest of the patch does)? > > This change makes it to be consistent with the rest of the code. You may > notice that "Factor out hardware initialization into separate function" > made a similar change. > > The reason I'm doing this is that the "err" suggests that code returns a > error failure code, while it could be a success too and you don't know > for sure by looking only at the part of code. Hence it's cleaner to use > "err" when error code is returned. I don't follow that reasoning. Every error code obviously also has a value for success. Otherwise, what's the point of even having a function if all it can do is fail. Success has to be an option for code to be any useful at all, right? The "err" variable here transports the error code and if that error code happens to be 0 (meaning success), why does that no longer qualify as an error code? Thierry
On Thu, Sep 17, 2020 at 06:17:39PM +0300, Dmitry Osipenko wrote: > 17.09.2020 18:02, Dmitry Osipenko пишет: > > 17.09.2020 15:32, Thierry Reding пишет: > > ... > >>> /** > >>> - * struct tegra_i2c_hw_feature : Different HW support on Tegra > >>> - * @has_continue_xfer_support: Continue transfer supports. > >>> + * struct tegra_i2c_hw_feature : per hardware generation features > >> > >> I think that space before ':' can go away. Although that's preexisting, > >> so could also be a separate patch, I guess. > > > > I haven't even noticed that! > > > > Wait, that ':' is used only for the struct description, hence it > actually looks natural in the code. What makes the struct description different from the field descriptions? A description list is basically: <term>: <description> And it doesn't matter what exactly <term> is. Thierry
On Mon, Sep 21, 2020 at 01:43:20PM +0200, Thierry Reding wrote: > On Thu, Sep 17, 2020 at 06:17:39PM +0300, Dmitry Osipenko wrote: > > 17.09.2020 18:02, Dmitry Osipenko пишет: > > > 17.09.2020 15:32, Thierry Reding пишет: > > > ... > > >>> /** > > >>> - * struct tegra_i2c_hw_feature : Different HW support on Tegra > > >>> - * @has_continue_xfer_support: Continue transfer supports. > > >>> + * struct tegra_i2c_hw_feature : per hardware generation features > > >> > > >> I think that space before ':' can go away. Although that's preexisting, > > >> so could also be a separate patch, I guess. > > > > > > I haven't even noticed that! > > > > > > > Wait, that ':' is used only for the struct description, hence it > > actually looks natural in the code. > > What makes the struct description different from the field descriptions? > A description list is basically: > > <term>: <description> > > And it doesn't matter what exactly <term> is. Anyway, like I said, this is preexisting, so we can always fix that up in another patch: Reviewed-by: Thierry Reding <treding@nvidia.com>
On Mon, Sep 21, 2020 at 02:15:09PM +0300, Andy Shevchenko wrote: > On Mon, Sep 21, 2020 at 2:02 PM Thierry Reding <thierry.reding@gmail.com> wrote: > > On Thu, Sep 17, 2020 at 04:54:28PM +0300, Andy Shevchenko wrote: > > > On Thu, Sep 17, 2020 at 2:38 PM Thierry Reding <thierry.reding@gmail.com> wrote: > > > > On Wed, Sep 09, 2020 at 01:39:44AM +0300, Dmitry Osipenko wrote: > > ... > > > > > This is tempting from a diffstat point of view, but the downside is that > > > > we can now no longer validate that all of the necessary clocks are given > > > > in device tree. > > > > > > > > Previously the driver would fail to probe the I2C controller if any of > > > > the expected clocks were not defined in device tree, but now it's just > > > > going to continue without it and not give any indication as to what's > > > > wrong. > > > > > > You may print an error in the error path as previously. Since both > > > clocks are mandatory (as far as I understood the code) user will need > > > to check DT in any case. > > > > The problem is that the number of required clocks depends on the variant > > of the IP block that's implemented. Some require just one clock and > > others require two or three. With this patch the driver is just going to > > pick whatever clocks are given in device tree, but it removes any > > possibility of detecting whether the device trees contain the correct > > clocks. So we may very well run into a situation where the driver now > > successfully probes but then malfunctions because one or more of the > > clocks were not specified in device tree. > > > > Thierry > > I still failed to get this. Are you suggesting that CCF bulk > operations are fundamentally broken? No, I'm not suggesting that. All I'm saying is the way that they are used here is causing the driver to behave differently that it was before. Taking for example the VI I2C controller instantiation. That requires the "slow" clock to be specified. Previously if the VI I2C device tree node didn't have that "slow" clock specified, the I2C driver probe would exit with an error code. After this change it will simply not see the "slow" clock and then just continue without it as if it was optional. In other words, after this patch we have no way of saying which clocks are required and which are optional. They all become optional, basically and the driver would attempt to continue (and most likely hang) if no clocks at all had been specified in device tree. > In the above case one may add more checks. AFAICS is_vi won't be > removed, so can be easily checked. > Basically that for-loop for div_clk is questionable. I agree on that. But we need that one to find which of the clocks is the divider clock so that we can call clk_set_rate() on it later on. It's a bit odd that we'd just continue even if we didn't find the divider clock. I think the CCF does handle this transparently and will just no-op all the calls for NULL clocks, but it still means that we won't be running the I2C bus at the right frequency if the divider clock was not specified in device tree. Thierry
21.09.2020 14:12, Thierry Reding пишет: > On Thu, Sep 17, 2020 at 06:01:56PM +0300, Dmitry Osipenko wrote: > [...] >> It's still possible to add the clk-num checking, but it should be >> unpractical. We could always add it later on if there will be a real >> incident. Do you agree? > > There's also clk_bulk_get(), which allows you to specify the number of > clocks and their consumer IDs that you want to request. That seems like > it would allow us to both avoid the repetitive calls to clk APIs and at > the same time allows us to specify exactly which clocks we need. Would > that not work as a compromise? I'll change to use clk_bulk_get(), thanks.
21.09.2020 14:40, Thierry Reding пишет: > On Thu, Sep 17, 2020 at 06:43:28PM +0300, Dmitry Osipenko wrote: >> 17.09.2020 15:21, Thierry Reding пишет: >>> On Wed, Sep 09, 2020 at 01:40:02AM +0300, Dmitry Osipenko wrote: >>>> Rename "ret" variables to "err" in order to make code a bit more >>>> expressive, emphasizing that the returned value is an error code. >>>> Same vice versa, where appropriate. >>>> >>>> Rename variable "reg" to "val" in order to better reflect the actual >>>> usage of the variable in the code and to make naming consistent with >>>> the rest of the code. >>>> >>>> Use briefer names for a few members of the tegra_i2c_dev structure in >>>> order to improve readability of the code. >>>> >>>> All dev/&pdev->dev are replaced with i2c_dev->dev in order to have uniform >>>> code style across the driver. >>>> >>>> Signed-off-by: Dmitry Osipenko <digetx@gmail.com> >>>> --- >>>> drivers/i2c/busses/i2c-tegra.c | 173 ++++++++++++++++----------------- >>>> 1 file changed, 86 insertions(+), 87 deletions(-) >>> >>> That's indeed a nice improvement. One thing did spring out at me, >>> though. >>> >>>> diff --git a/drivers/i2c/busses/i2c-tegra.c b/drivers/i2c/busses/i2c-tegra.c >>> [...] >>>> @@ -1831,20 +1830,20 @@ static int __maybe_unused tegra_i2c_runtime_suspend(struct device *dev) >>>> >>>> clk_bulk_disable(i2c_dev->nclocks, i2c_dev->clocks); >>>> >>>> - return pinctrl_pm_select_idle_state(i2c_dev->dev); >>>> + return pinctrl_pm_select_idle_state(dev); >>>> } >>>> >>>> static int __maybe_unused tegra_i2c_suspend(struct device *dev) >>>> { >>>> struct tegra_i2c_dev *i2c_dev = dev_get_drvdata(dev); >>>> - int err = 0; >>>> + int ret = 0; >>>> >>>> i2c_mark_adapter_suspended(&i2c_dev->adapter); >>>> >>>> if (!pm_runtime_status_suspended(dev)) >>>> - err = tegra_i2c_runtime_suspend(dev); >>>> + ret = tegra_i2c_runtime_suspend(dev); >>>> >>>> - return err; >>>> + return ret; >>>> } >>> >>> Isn't this exactly the opposite of what the commit message says (and the >>> rest of the patch does)? >> >> This change makes it to be consistent with the rest of the code. You may >> notice that "Factor out hardware initialization into separate function" >> made a similar change. >> >> The reason I'm doing this is that the "err" suggests that code returns a >> error failure code, while it could be a success too and you don't know >> for sure by looking only at the part of code. Hence it's cleaner to use >> "err" when error code is returned. > > I don't follow that reasoning. Every error code obviously also has a > value for success. Otherwise, what's the point of even having a function > if all it can do is fail. Success has to be an option for code to be any > useful at all, right? > > The "err" variable here transports the error code and if that error code > happens to be 0 (meaning success), why does that no longer qualify as an > error code? If you're naming variable as "err", then this implies to me that it will contain a error code if error variable is returned directly. Error shouldn't relate to a success. In practice nobody pays much attention to variable naming, so usually there is a need to check what code actually does anyways. I don't care much about this and just wanting to make a minor improvement while at it.
On Mon, Sep 21, 2020 at 06:18:59PM +0300, Dmitry Osipenko wrote: > 21.09.2020 14:40, Thierry Reding пишет: > > On Thu, Sep 17, 2020 at 06:43:28PM +0300, Dmitry Osipenko wrote: > >> 17.09.2020 15:21, Thierry Reding пишет: > >>> On Wed, Sep 09, 2020 at 01:40:02AM +0300, Dmitry Osipenko wrote: > >>>> Rename "ret" variables to "err" in order to make code a bit more > >>>> expressive, emphasizing that the returned value is an error code. > >>>> Same vice versa, where appropriate. > >>>> > >>>> Rename variable "reg" to "val" in order to better reflect the actual > >>>> usage of the variable in the code and to make naming consistent with > >>>> the rest of the code. > >>>> > >>>> Use briefer names for a few members of the tegra_i2c_dev structure in > >>>> order to improve readability of the code. > >>>> > >>>> All dev/&pdev->dev are replaced with i2c_dev->dev in order to have uniform > >>>> code style across the driver. > >>>> > >>>> Signed-off-by: Dmitry Osipenko <digetx@gmail.com> > >>>> --- > >>>> drivers/i2c/busses/i2c-tegra.c | 173 ++++++++++++++++----------------- > >>>> 1 file changed, 86 insertions(+), 87 deletions(-) > >>> > >>> That's indeed a nice improvement. One thing did spring out at me, > >>> though. > >>> > >>>> diff --git a/drivers/i2c/busses/i2c-tegra.c b/drivers/i2c/busses/i2c-tegra.c > >>> [...] > >>>> @@ -1831,20 +1830,20 @@ static int __maybe_unused tegra_i2c_runtime_suspend(struct device *dev) > >>>> > >>>> clk_bulk_disable(i2c_dev->nclocks, i2c_dev->clocks); > >>>> > >>>> - return pinctrl_pm_select_idle_state(i2c_dev->dev); > >>>> + return pinctrl_pm_select_idle_state(dev); > >>>> } > >>>> > >>>> static int __maybe_unused tegra_i2c_suspend(struct device *dev) > >>>> { > >>>> struct tegra_i2c_dev *i2c_dev = dev_get_drvdata(dev); > >>>> - int err = 0; > >>>> + int ret = 0; > >>>> > >>>> i2c_mark_adapter_suspended(&i2c_dev->adapter); > >>>> > >>>> if (!pm_runtime_status_suspended(dev)) > >>>> - err = tegra_i2c_runtime_suspend(dev); > >>>> + ret = tegra_i2c_runtime_suspend(dev); > >>>> > >>>> - return err; > >>>> + return ret; > >>>> } > >>> > >>> Isn't this exactly the opposite of what the commit message says (and the > >>> rest of the patch does)? > >> > >> This change makes it to be consistent with the rest of the code. You may > >> notice that "Factor out hardware initialization into separate function" > >> made a similar change. > >> > >> The reason I'm doing this is that the "err" suggests that code returns a > >> error failure code, while it could be a success too and you don't know > >> for sure by looking only at the part of code. Hence it's cleaner to use > >> "err" when error code is returned. > > > > I don't follow that reasoning. Every error code obviously also has a > > value for success. Otherwise, what's the point of even having a function > > if all it can do is fail. Success has to be an option for code to be any > > useful at all, right? > > > > The "err" variable here transports the error code and if that error code > > happens to be 0 (meaning success), why does that no longer qualify as an > > error code? > > If you're naming variable as "err", then this implies to me that it will > contain a error code if error variable is returned directly. Error > shouldn't relate to a success. In practice nobody pays much attention to > variable naming, so usually there is a need to check what code actually > does anyways. I don't care much about this and just wanting to make a > minor improvement while at it. Oh... I think I get what you're trying to do here now. You're saying that we may be storing a positive success result in this variable and therefore it would be wrong to call it "error", right? And I always thought I was pedantic... =) The way I see it, any success value can still be considered an error code. Typically you either propagate the value immediately for errors or you just ignore it on success. In that case, keeping it in a variable a bit beyond the assignment isn't a big issue. What matters is that you don't use it. There are some exceptions where this can look weird, such as: err = platform_get_irq(pdev, 0); if (err < 0) return err; chip->irq = err; Although I think that's still okay and can be useful for example if chip->irq is an unsigned int, and hence you can't do: chip->irq = platform_get_irq(pdev, 0); if (chip->irq < 0) return chip->irq; My main gripe with variables named "ret" or "retval" is that I often see them not used as return value at all. Or the other extreme is that every variable is at some point a return value if it stores the result of a function call. So I think "ret" is just fundamentally a bad choice. But I also realize that that's very subjective. Anyway, I would personally lean towards calling all these "err" instead of "ret", but I think consistency trumps personal preference, so I would not object to "ret" generally. But I think it's a bit extreme to use err everywhere else and use "ret" only when we don't immediately return the error code because I think that's just too subtle of a difference to make up for the inconsistency. On the other hand, we've spent way too much time discussing this, so just pick whatever you want: Acked-by: Thierry Reding <treding@nvidia.com>
21.09.2020 18:50, Thierry Reding пишет: > On Mon, Sep 21, 2020 at 06:18:59PM +0300, Dmitry Osipenko wrote: >> 21.09.2020 14:40, Thierry Reding пишет: >>> On Thu, Sep 17, 2020 at 06:43:28PM +0300, Dmitry Osipenko wrote: >>>> 17.09.2020 15:21, Thierry Reding пишет: >>>>> On Wed, Sep 09, 2020 at 01:40:02AM +0300, Dmitry Osipenko wrote: >>>>>> Rename "ret" variables to "err" in order to make code a bit more >>>>>> expressive, emphasizing that the returned value is an error code. >>>>>> Same vice versa, where appropriate. >>>>>> >>>>>> Rename variable "reg" to "val" in order to better reflect the actual >>>>>> usage of the variable in the code and to make naming consistent with >>>>>> the rest of the code. >>>>>> >>>>>> Use briefer names for a few members of the tegra_i2c_dev structure in >>>>>> order to improve readability of the code. >>>>>> >>>>>> All dev/&pdev->dev are replaced with i2c_dev->dev in order to have uniform >>>>>> code style across the driver. >>>>>> >>>>>> Signed-off-by: Dmitry Osipenko <digetx@gmail.com> >>>>>> --- >>>>>> drivers/i2c/busses/i2c-tegra.c | 173 ++++++++++++++++----------------- >>>>>> 1 file changed, 86 insertions(+), 87 deletions(-) >>>>> >>>>> That's indeed a nice improvement. One thing did spring out at me, >>>>> though. >>>>> >>>>>> diff --git a/drivers/i2c/busses/i2c-tegra.c b/drivers/i2c/busses/i2c-tegra.c >>>>> [...] >>>>>> @@ -1831,20 +1830,20 @@ static int __maybe_unused tegra_i2c_runtime_suspend(struct device *dev) >>>>>> >>>>>> clk_bulk_disable(i2c_dev->nclocks, i2c_dev->clocks); >>>>>> >>>>>> - return pinctrl_pm_select_idle_state(i2c_dev->dev); >>>>>> + return pinctrl_pm_select_idle_state(dev); >>>>>> } >>>>>> >>>>>> static int __maybe_unused tegra_i2c_suspend(struct device *dev) >>>>>> { >>>>>> struct tegra_i2c_dev *i2c_dev = dev_get_drvdata(dev); >>>>>> - int err = 0; >>>>>> + int ret = 0; >>>>>> >>>>>> i2c_mark_adapter_suspended(&i2c_dev->adapter); >>>>>> >>>>>> if (!pm_runtime_status_suspended(dev)) >>>>>> - err = tegra_i2c_runtime_suspend(dev); >>>>>> + ret = tegra_i2c_runtime_suspend(dev); >>>>>> >>>>>> - return err; >>>>>> + return ret; >>>>>> } >>>>> >>>>> Isn't this exactly the opposite of what the commit message says (and the >>>>> rest of the patch does)? >>>> >>>> This change makes it to be consistent with the rest of the code. You may >>>> notice that "Factor out hardware initialization into separate function" >>>> made a similar change. >>>> >>>> The reason I'm doing this is that the "err" suggests that code returns a >>>> error failure code, while it could be a success too and you don't know >>>> for sure by looking only at the part of code. Hence it's cleaner to use >>>> "err" when error code is returned. >>> >>> I don't follow that reasoning. Every error code obviously also has a >>> value for success. Otherwise, what's the point of even having a function >>> if all it can do is fail. Success has to be an option for code to be any >>> useful at all, right? >>> >>> The "err" variable here transports the error code and if that error code >>> happens to be 0 (meaning success), why does that no longer qualify as an >>> error code? >> >> If you're naming variable as "err", then this implies to me that it will >> contain a error code if error variable is returned directly. Error >> shouldn't relate to a success. In practice nobody pays much attention to >> variable naming, so usually there is a need to check what code actually >> does anyways. I don't care much about this and just wanting to make a >> minor improvement while at it. > > Oh... I think I get what you're trying to do here now. You're saying > that we may be storing a positive success result in this variable and > therefore it would be wrong to call it "error", right? > > And I always thought I was pedantic... =) > > The way I see it, any success value can still be considered an error > code. Typically you either propagate the value immediately for errors or > you just ignore it on success. In that case, keeping it in a variable a > bit beyond the assignment isn't a big issue. What matters is that you > don't use it. There are some exceptions where this can look weird, such > as: > > err = platform_get_irq(pdev, 0); > if (err < 0) > return err; > > chip->irq = err; > > Although I think that's still okay and can be useful for example if > chip->irq is an unsigned int, and hence you can't do: > > chip->irq = platform_get_irq(pdev, 0); > if (chip->irq < 0) > return chip->irq; > > My main gripe with variables named "ret" or "retval" is that I often see > them not used as return value at all. Or the other extreme is that every > variable is at some point a return value if it stores the result of a > function call. So I think "ret" is just fundamentally a bad choice. But > I also realize that that's very subjective. > > Anyway, I would personally lean towards calling all these "err" instead > of "ret", but I think consistency trumps personal preference, so I would > not object to "ret" generally. But I think it's a bit extreme to use err > everywhere else and use "ret" only when we don't immediately return the > error code because I think that's just too subtle of a difference to > make up for the inconsistency. > > On the other hand, we've spent way too much time discussing this, so > just pick whatever you want: > > Acked-by: Thierry Reding <treding@nvidia.com> > Thanks, I'll change it to make tegra_i2c_suspend() to use the same style as tegra_i2c_resume() uses, which should be the best option in this case.
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(-)