diff mbox series

[v7,06/34] i2c: tegra: Remove i2c_dev.clk_divisor_non_hs_mode member

Message ID 20200908224006.25636-7-digetx@gmail.com
State New
Headers show
Series Improvements for Tegra I2C driver | expand

Checks

Context Check Description
tagr/GVS success 1122533
tagr/GVS pending 1122533
tagr/GVS-1122491 fail None
tagr/GVS-1122491 pending None

Commit Message

Dmitry Osipenko Sept. 8, 2020, 10:39 p.m. UTC
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(-)

Comments

Thierry Reding Sept. 17, 2020, 11:25 a.m. UTC | #1
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
Dmitry Osipenko Sept. 17, 2020, 3:27 p.m. UTC | #2
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.
diff mbox series

Patch

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);