| Message ID | 20200908224006.25636-7-digetx@gmail.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | Improvements for Tegra I2C driver | expand |
| Context | Check | Description |
|---|---|---|
| tagr/GVS-1122491 | pending | None |
| tagr/GVS-1122491 | fail | None |
| tagr/GVS | pending | 1122533 |
| tagr/GVS | success | 1122533 |
| tagr/GVS | success | 1124251 |
| tagr/GVS | pending | 1124251 |
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
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.
On Wed, 09 Sep 2020 01:39:38 +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(-) Tested-by: Thierry Reding <treding@nvidia.com>
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>
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: + default: tlow = i2c_dev->hw->tlow_fast_fastplus_mode; thigh = i2c_dev->hw->thigh_fast_fastplus_mode; tsu_thd = i2c_dev->hw->setup_hold_time_fast_fast_plus_mode; - } else { + + if (i2c_dev->bus_clk_rate > I2C_MAX_FAST_MODE_FREQ) + non_hs_mode = i2c_dev->hw->clk_divisor_fast_plus_mode; + else + non_hs_mode = i2c_dev->hw->clk_divisor_fast_mode; + break; + + case 0 ... I2C_MAX_STANDARD_MODE_FREQ: tlow = i2c_dev->hw->tlow_std_mode; thigh = i2c_dev->hw->thigh_std_mode; tsu_thd = i2c_dev->hw->setup_hold_time_std_mode; + non_hs_mode = i2c_dev->hw->clk_divisor_std_mode; + break; } + /* Make sure clock divisor programmed correctly */ + clk_divisor = FIELD_PREP(I2C_CLK_DIVISOR_HSMODE, + i2c_dev->hw->clk_divisor_hs_mode) | + FIELD_PREP(I2C_CLK_DIVISOR_STD_FAST_MODE, non_hs_mode); + i2c_writel(i2c_dev, clk_divisor, I2C_CLK_DIVISOR); + if (i2c_dev->hw->has_interface_timing_reg) { val = FIELD_PREP(I2C_INTERFACE_TIMING_THIGH, thigh) | FIELD_PREP(I2C_INTERFACE_TIMING_TLOW, tlow); @@ -837,7 +845,7 @@ static int tegra_i2c_init(struct tegra_i2c_dev *i2c_dev) i2c_writel(i2c_dev, tsu_thd, I2C_INTERFACE_TIMING_1); clk_multiplier = tlow + thigh + 2; - clk_multiplier *= i2c_dev->clk_divisor_non_hs_mode + 1; + clk_multiplier *= non_hs_mode + 1; err = clk_set_rate(i2c_dev->div_clk, i2c_dev->bus_clk_rate * clk_multiplier); @@ -1751,18 +1759,6 @@ static int tegra_i2c_probe(struct platform_device *pdev) goto unprepare_fast_clk; } - if (i2c_dev->bus_clk_rate > I2C_MAX_FAST_MODE_FREQ && - i2c_dev->bus_clk_rate <= I2C_MAX_FAST_MODE_PLUS_FREQ) - i2c_dev->clk_divisor_non_hs_mode = - i2c_dev->hw->clk_divisor_fast_plus_mode; - else if (i2c_dev->bus_clk_rate > I2C_MAX_STANDARD_MODE_FREQ && - i2c_dev->bus_clk_rate <= I2C_MAX_FAST_MODE_FREQ) - i2c_dev->clk_divisor_non_hs_mode = - i2c_dev->hw->clk_divisor_fast_mode; - else - i2c_dev->clk_divisor_non_hs_mode = - i2c_dev->hw->clk_divisor_std_mode; - ret = clk_prepare(i2c_dev->div_clk); if (ret < 0) { dev_err(i2c_dev->dev, "Clock prepare failed %d\n", ret);