Skip to contentSkip to conversation

i2c: meson: series with improvements

PATCH Superseded Heiner Kallweit 3 participants 15 messages Mar 9, 2017 – Mar 10, 2017

Subsystems (2)
Arm/Amlogic Meson SoC support I2C subsystem host drivers

v2 of 5 Superseded Latest is v5 →

lore
Share & tools
lore

Heiner Kallweit

author
#0 lore raw

v2 cover i2c: meson: series with improvements

This patch series includes several improvements for the i2c-meson driver.

Changes in v2:
- removed patches 8 and 12
- removed one small change in patch 6
- don't print error message in patch 7

Heiner Kallweit (12):
i2c: meson: use min instead of min_t where min_t isn't needed
i2c: meson: remove member irq from struct meson_i2c
i2c: meson: use i2c core for DT clock-frequency parsing
i2c: meson: use full 12 bits for clock divider
i2c: meson: set clock divider in probe instead of setting it for each transfer
i2c: meson: remove variable count from meson_i2c_xfer
i2c: meson: improve interrupt handler and detect spurious interrupts
i2c: meson: don't create separate token chain just for the stop command
i2c: meson: remove meson_i2c_write_tokens
i2c: meson: improve and simplify interrupt handler

 drivers/i2c/busses/i2c-meson.c | 136 ++++++++++++++++-------------------------
 1 file changed, 51 insertions(+), 85 deletions(-)
Signature · 1 line
2.12.0
author
#1 lore raw

PATCH v2 1/10 i2c: meson: i2c: meson: use min instead of min_t where min_t isn't needed

Use min instead of min_t where min_t isn't needed.

Signed-off-by Heiner Kallweit <[email protected]>

---
v2:
- no changes

---
 drivers/i2c/busses/i2c-meson.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)
drivers/i2c/busses/i2c-meson.c +5 -5
index 73b97c71..40e5da9a 100644
@@ -156,10 +156,10 @@ static void meson_i2c_get_data(struct meson_i2c *i2c, char *buf, int len)
 	dev_dbg(i2c->dev, "%s: data %08x %08x len %d\n", __func__,
 		rdata0, rdata1, len);
 
-	for (i = 0; i < min_t(int, 4, len); i++)
+	for (i = 0; i < min(4, len); i++)
 		*buf++ = (rdata0 >> i * 8) & 0xff;
 
-	for (i = 4; i < min_t(int, 8, len); i++)
+	for (i = 4; i < min(8, len); i++)
 		*buf++ = (rdata1 >> (i - 4) * 8) & 0xff;
 }
 

@@ -168,10 +168,10 @@ static void meson_i2c_put_data(struct meson_i2c *i2c, char *buf, int len)
 	u32 wdata0 = 0, wdata1 = 0;
 	int i;
 
-	for (i = 0; i < min_t(int, 4, len); i++)
+	for (i = 0; i < min(4, len); i++)
 		wdata0 |= *buf++ << (i * 8);
 
-	for (i = 4; i < min_t(int, 8, len); i++)
+	for (i = 4; i < min(8, len); i++)
 		wdata1 |= *buf++ << ((i - 4) * 8);
 
 	writel(wdata0, i2c->regs + REG_TOK_WDATA0);

@@ -186,7 +186,7 @@ static void meson_i2c_prepare_xfer(struct meson_i2c *i2c)
 	bool write = !(i2c->msg->flags & I2C_M_RD);
 	int i;
 
-	i2c->count = min_t(int, i2c->msg->len - i2c->pos, 8);
+	i2c->count = min(i2c->msg->len - i2c->pos, 8);
 
 	for (i = 0; i < i2c->count - 1; i++)
 		meson_i2c_add_token(i2c, TOKEN_DATA);
Signature · 1 line
2.12.0
author
#2 lore raw

PATCH v2 2/10 i2c: meson: remove member irq from struct meson_i2c

In reply to Heiner Kallweit

Member irq can be replaced with a local variable in probe because it's nowhere else accessed.

Signed-off-by Heiner Kallweit <[email protected]>
Reviewed-by Jerome Brunet <[email protected]>

---
v2:
- added Reviewed-by

---
 drivers/i2c/busses/i2c-meson.c | 13 ++++++-------
 1 file changed, 6 insertions(+), 7 deletions(-)
drivers/i2c/busses/i2c-meson.c +6 -7
index 40e5da9a..50059d09 100644
@@ -82,7 +82,6 @@ struct meson_i2c {
 	struct device		*dev;
 	void __iomem		*regs;
 	struct clk		*clk;
-	int			irq;
 
 	struct i2c_msg		*msg;
 	int			state;

@@ -391,7 +390,7 @@ static int meson_i2c_probe(struct platform_device *pdev)
 	struct device_node *np = pdev->dev.of_node;
 	struct meson_i2c *i2c;
 	struct resource *mem;
-	int ret = 0;
+	int irq, ret = 0;
 
 	i2c = devm_kzalloc(&pdev->dev, sizeof(struct meson_i2c), GFP_KERNEL);
 	if (!i2c)

@@ -418,14 +417,14 @@ static int meson_i2c_probe(struct platform_device *pdev)
 	if (IS_ERR(i2c->regs))
 		return PTR_ERR(i2c->regs);
 
-	i2c->irq = platform_get_irq(pdev, 0);
-	if (i2c->irq < 0) {
+	irq = platform_get_irq(pdev, 0);
+	if (irq < 0) {
 		dev_err(&pdev->dev, "can't find IRQ\n");
-		return i2c->irq;
+		return irq;
 	}
 
-	ret = devm_request_irq(&pdev->dev, i2c->irq, meson_i2c_irq,
-			       0, dev_name(&pdev->dev), i2c);
+	ret = devm_request_irq(&pdev->dev, irq, meson_i2c_irq, 0,
+			       dev_name(&pdev->dev), i2c);
 	if (ret < 0) {
 		dev_err(&pdev->dev, "can't request IRQ\n");
 		return ret;
Signature · 1 line
2.12.0
author
#3 lore raw

PATCH v2 3/10 i2c: meson: use i2c core for DT clock-frequency parsing

In reply to Heiner Kallweit

We don't have to parse the DT manually to retrieve the bus frequency and we don't have to maintain an own default for the bus frequency. Let the i2c core do this for us.

Signed-off-by Heiner Kallweit <[email protected]>
Reviewed-by Jerome Brunet <[email protected]>

---
v2:
- added Reviewed-by

---
 drivers/i2c/busses/i2c-meson.c | 13 +++++--------
 1 file changed, 5 insertions(+), 8 deletions(-)
drivers/i2c/busses/i2c-meson.c +5 -8
index 50059d09..5e243efa 100644
@@ -38,7 +38,6 @@
 #define REG_CTRL_CLKDIV_MASK	((BIT(10) - 1) << REG_CTRL_CLKDIV_SHIFT)
 
 #define I2C_TIMEOUT_MS		500
-#define DEFAULT_FREQ		100000
 
 enum {
 	TOKEN_END = 0,

@@ -73,7 +72,7 @@ enum {
  * @error:	Flag set when an error is received
  * @lock:	To avoid race conditions between irq handler and xfer code
  * @done:	Completion used to wait for transfer termination
- * @frequency:	Operating frequency of I2C bus clock
+ * @timings:	Struct including the bus frequency
  * @tokens:	Sequence of tokens to be written to the device
  * @num_tokens:	Number of tokens
  */

@@ -92,7 +91,7 @@ struct meson_i2c {
 
 	spinlock_t		lock;
 	struct completion	done;
-	unsigned int		frequency;
+	struct i2c_timings	timings;
 	u32			tokens[2];
 	int			num_tokens;
 };

@@ -136,12 +135,12 @@ static void meson_i2c_set_clk_div(struct meson_i2c *i2c)
 	unsigned long clk_rate = clk_get_rate(i2c->clk);
 	unsigned int div;
 
-	div = DIV_ROUND_UP(clk_rate, i2c->frequency * 4);
+	div = DIV_ROUND_UP(clk_rate, i2c->timings.bus_freq_hz * 4);
 	meson_i2c_set_mask(i2c, REG_CTRL, REG_CTRL_CLKDIV_MASK,
 			   div << REG_CTRL_CLKDIV_SHIFT);
 
 	dev_dbg(i2c->dev, "%s: clk %lu, freq %u, div %u\n", __func__,
-		clk_rate, i2c->frequency, div);
+		clk_rate, i2c->timings.bus_freq_hz, div);
 }
 
 static void meson_i2c_get_data(struct meson_i2c *i2c, char *buf, int len)

@@ -396,9 +395,7 @@ static int meson_i2c_probe(struct platform_device *pdev)
 	if (!i2c)
 		return -ENOMEM;
 
-	if (of_property_read_u32(pdev->dev.of_node, "clock-frequency",
-				 &i2c->frequency))
-		i2c->frequency = DEFAULT_FREQ;
+	i2c_parse_fw_timings(&pdev->dev, &i2c->timings, true);
 
 	i2c->dev = &pdev->dev;
 	platform_set_drvdata(pdev, i2c);
Signature · 1 line
2.12.0
#4 lore raw

PATCH v2 3/10 i2c: meson: use i2c core for DT clock-frequency parsing

Quoted 60 lines · Heiner Kallweit

We don't have to parse the DT manually to retrieve the bus frequency and we don't have to maintain an own default for the bus frequency. Let the i2c core do this for us.

Signed-off-by Heiner Kallweit <[email protected]>
Reviewed-by Jerome Brunet <[email protected]>

---
v2:
- added Reviewed-by

---
 drivers/i2c/busses/i2c-meson.c | 13 +++++--------
 1 file changed, 5 insertions(+), 8 deletions(-)
drivers/i2c/busses/i2c-meson.c +4 -5
index 50059d09..5e243efa 100644
@@ -38,7 +38,6 @@
 #define REG_CTRL_CLKDIV_MASK	((BIT(10) - 1) << REG_CTRL_CLKDIV_SHIFT)
 
 #define I2C_TIMEOUT_MS		500
-#define DEFAULT_FREQ		100000
 
 enum {
 	TOKEN_END = 0,

@@ -73,7 +72,7 @@ enum {
  * @error:	Flag set when an error is received
  * @lock:	To avoid race conditions between irq handler and xfer code
  * @done:	Completion used to wait for transfer termination
- * @frequency:	Operating frequency of I2C bus clock
+ * @timings:	Struct including the bus frequency
  * @tokens:	Sequence of tokens to be written to the device
  * @num_tokens:	Number of tokens
  */

@@ -92,7 +91,7 @@ struct meson_i2c {
 
 	spinlock_t		lock;
 	struct completion	done;
-	unsigned int		frequency;
+	struct i2c_timings	timings;
 	u32			tokens[2];
 	int			num_tokens;
 };

@@ -136,12 +135,12 @@ static void meson_i2c_set_clk_div(struct meson_i2c *i2c)
 	unsigned long clk_rate = clk_get_rate(i2c->clk);
 	unsigned int div;
 
-	div = DIV_ROUND_UP(clk_rate, i2c->frequency * 4);
+	div = DIV_ROUND_UP(clk_rate, i2c->timings.bus_freq_hz * 4);
 	meson_i2c_set_mask(i2c, REG_CTRL, REG_CTRL_CLKDIV_MASK,
 			   div << REG_CTRL_CLKDIV_SHIFT);
 
 	dev_dbg(i2c->dev, "%s: clk %lu, freq %u, div %u\n", __func__,
-		clk_rate, i2c->frequency, div);
+		clk_rate, i2c->timings.bus_freq_hz, div);
 }
 
 static void meson_i2c_get_data(struct meson_i2c *i2c, char *buf, int len)

drivers/i2c/busses/i2c-meson.c static int meson_i2c_probe(struct platform_device *pdev) · line 401

Quoted 10 lines
drivers/i2c/busses/i2c-meson.c +1 -3
@@ -396,9 +395,7 @@ static int meson_i2c_probe(struct platform_device *pdev)
 	if (!i2c)
 		return -ENOMEM;
 
-	if (of_property_read_u32(pdev->dev.of_node, "clock-frequency",
-				 &i2c->frequency))
-		i2c->frequency = DEFAULT_FREQ;
+	i2c_parse_fw_timings(&pdev->dev, &i2c->timings, true);
 
 	i2c->dev = &pdev->dev;
 	platform_set_drvdata(pdev, i2c);

Hi Heiner,

Please update the bindings in Documentation/devicetree/bindings/i2c/i2c-meson.txt for these new optional attributes by adding :

---><-----
Please also refer to i2c.txt in this directory for common i2c controller binding usage.
---><-----

With this additional patch :

Reviewed-by Neil Armstrong <[email protected]>

Neil

author
#5 lore raw

PATCH v2 3/10 i2c: meson: use i2c core for DT clock-frequency parsing

Am 09.03.2017 um 09:26 schrieb Neil Armstrong:

Quoted 68 lines · Heiner Kallweit
Quoted 67 lines

We don't have to parse the DT manually to retrieve the bus frequency and we don't have to maintain an own default for the bus frequency. Let the i2c core do this for us.

Signed-off-by Heiner Kallweit <[email protected]>
Reviewed-by Jerome Brunet <[email protected]>

---
v2:
- added Reviewed-by

---
 drivers/i2c/busses/i2c-meson.c | 13 +++++--------
 1 file changed, 5 insertions(+), 8 deletions(-)
drivers/i2c/busses/i2c-meson.c +5 -8
index 50059d09..5e243efa 100644
@@ -38,7 +38,6 @@
 #define REG_CTRL_CLKDIV_MASK	((BIT(10) - 1) << REG_CTRL_CLKDIV_SHIFT)
 
 #define I2C_TIMEOUT_MS		500
-#define DEFAULT_FREQ		100000
 
 enum {
 	TOKEN_END = 0,

@@ -73,7 +72,7 @@ enum {
  * @error:	Flag set when an error is received
  * @lock:	To avoid race conditions between irq handler and xfer code
  * @done:	Completion used to wait for transfer termination
- * @frequency:	Operating frequency of I2C bus clock
+ * @timings:	Struct including the bus frequency
  * @tokens:	Sequence of tokens to be written to the device
  * @num_tokens:	Number of tokens
  */

@@ -92,7 +91,7 @@ struct meson_i2c {
 
 	spinlock_t		lock;
 	struct completion	done;
-	unsigned int		frequency;
+	struct i2c_timings	timings;
 	u32			tokens[2];
 	int			num_tokens;
 };

@@ -136,12 +135,12 @@ static void meson_i2c_set_clk_div(struct meson_i2c *i2c)
 	unsigned long clk_rate = clk_get_rate(i2c->clk);
 	unsigned int div;
 
-	div = DIV_ROUND_UP(clk_rate, i2c->frequency * 4);
+	div = DIV_ROUND_UP(clk_rate, i2c->timings.bus_freq_hz * 4);
 	meson_i2c_set_mask(i2c, REG_CTRL, REG_CTRL_CLKDIV_MASK,
 			   div << REG_CTRL_CLKDIV_SHIFT);
 
 	dev_dbg(i2c->dev, "%s: clk %lu, freq %u, div %u\n", __func__,
-		clk_rate, i2c->frequency, div);
+		clk_rate, i2c->timings.bus_freq_hz, div);
 }
 
 static void meson_i2c_get_data(struct meson_i2c *i2c, char *buf, int len)

@@ -396,9 +395,7 @@ static int meson_i2c_probe(struct platform_device *pdev)
 	if (!i2c)
 		return -ENOMEM;
 
-	if (of_property_read_u32(pdev->dev.of_node, "clock-frequency",
-				 &i2c->frequency))
-		i2c->frequency = DEFAULT_FREQ;
+	i2c_parse_fw_timings(&pdev->dev, &i2c->timings, true);
Quoted 9 lines

drivers/i2c/busses/i2c-meson.c static int meson_i2c_probe(struct platform_device *pdev) · line 401

Quoted 3 lines
drivers/i2c/busses/i2c-meson.c +0 -0
 	i2c->dev = &pdev->dev;
 	platform_set_drvdata(pdev, i2c);

Hi Heiner,

Please update the bindings in Documentation/devicetree/bindings/i2c/i2c-meson.txt for these new optional attributes by adding :

Hi Neil,

not sure which new attributes you mean. i2c_parse_fw_timings parses additional attributes (apart from bus frequency), that right. But they are not used by the driver and therefore effectively aren't supported.

Heiner

Quoted 10 lines

---><-----
Please also refer to i2c.txt in this directory for common i2c controller binding usage.
---><-----

With this additional patch :

Reviewed-by Neil Armstrong <[email protected]>

Neil

author
#6 lore raw

PATCH v2 4/10 i2c: meson: use full 12 bits for clock divider

In reply to Heiner Kallweit

The clock divider has 12 bits, splitted into a 10 bit field and a 2 bit field. The extra 2 bits aren't used currently.

Change this to use the full 12 bits and warn if the requested frequency is too low.

Signed-off-by Heiner Kallweit <[email protected]>
Acked-by Jerome Brunet <[email protected]>

---
v2:
- added Acked-by

---
 drivers/i2c/busses/i2c-meson.c | 13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)
drivers/i2c/busses/i2c-meson.c +11 -2
index 5e243efa..a5764be5 100644
@@ -35,7 +35,9 @@
 #define REG_CTRL_STATUS		BIT(2)
 #define REG_CTRL_ERROR		BIT(3)
 #define REG_CTRL_CLKDIV_SHIFT	12
-#define REG_CTRL_CLKDIV_MASK	((BIT(10) - 1) << REG_CTRL_CLKDIV_SHIFT)
+#define REG_CTRL_CLKDIV_MASK	GENMASK(21, 12)
+#define REG_CTRL_CLKDIVEXT_SHIFT 28
+#define REG_CTRL_CLKDIVEXT_MASK	GENMASK(29, 28)
 
 #define I2C_TIMEOUT_MS		500
 

@@ -136,8 +138,15 @@ static void meson_i2c_set_clk_div(struct meson_i2c *i2c)
 	unsigned int div;
 
 	div = DIV_ROUND_UP(clk_rate, i2c->timings.bus_freq_hz * 4);
+
+	/* clock divider has 12 bits */
+	WARN_ON(div >= (1 << 12));
+
 	meson_i2c_set_mask(i2c, REG_CTRL, REG_CTRL_CLKDIV_MASK,
-			   div << REG_CTRL_CLKDIV_SHIFT);
+			   (div & GENMASK(9, 0)) << REG_CTRL_CLKDIV_SHIFT);
+
+	meson_i2c_set_mask(i2c, REG_CTRL, REG_CTRL_CLKDIVEXT_MASK,
+			   (div >> 10) << REG_CTRL_CLKDIVEXT_SHIFT);
 
 	dev_dbg(i2c->dev, "%s: clk %lu, freq %u, div %u\n", __func__,
 		clk_rate, i2c->timings.bus_freq_hz, div);
Signature · 1 line
2.12.0
author
#7 lore raw

PATCH v2 5/10 i2c: meson: set clock divider in probe instead of setting it for each transfer

In reply to Heiner Kallweit

The bus frequency won't change, therefore we can set the clock divider in probe already and we don't have to set it for each transfer.

Signed-off-by Heiner Kallweit <[email protected]>

---
v2:
- no changes

---
 drivers/i2c/busses/i2c-meson.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)
drivers/i2c/busses/i2c-meson.c +7 -7
index a5764be5..594fec22 100644
@@ -74,7 +74,6 @@ enum {
  * @error:	Flag set when an error is received
  * @lock:	To avoid race conditions between irq handler and xfer code
  * @done:	Completion used to wait for transfer termination
- * @timings:	Struct including the bus frequency
  * @tokens:	Sequence of tokens to be written to the device
  * @num_tokens:	Number of tokens
  */

@@ -93,7 +92,6 @@ struct meson_i2c {
 
 	spinlock_t		lock;
 	struct completion	done;
-	struct i2c_timings	timings;
 	u32			tokens[2];
 	int			num_tokens;
 };

@@ -132,12 +130,12 @@ static void meson_i2c_write_tokens(struct meson_i2c *i2c)
 	writel(i2c->tokens[1], i2c->regs + REG_TOK_LIST1);
 }
 
-static void meson_i2c_set_clk_div(struct meson_i2c *i2c)
+static void meson_i2c_set_clk_div(struct meson_i2c *i2c, unsigned int freq)
 {
 	unsigned long clk_rate = clk_get_rate(i2c->clk);
 	unsigned int div;
 
-	div = DIV_ROUND_UP(clk_rate, i2c->timings.bus_freq_hz * 4);
+	div = DIV_ROUND_UP(clk_rate, freq * 4);
 
 	/* clock divider has 12 bits */
 	WARN_ON(div >= (1 << 12));

@@ -149,7 +147,7 @@ static void meson_i2c_set_clk_div(struct meson_i2c *i2c)
 			   (div >> 10) << REG_CTRL_CLKDIVEXT_SHIFT);
 
 	dev_dbg(i2c->dev, "%s: clk %lu, freq %u, div %u\n", __func__,
-		clk_rate, i2c->timings.bus_freq_hz, div);
+		clk_rate, freq, div);
 }
 
 static void meson_i2c_get_data(struct meson_i2c *i2c, char *buf, int len)

@@ -369,7 +367,6 @@ static int meson_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs,
 	int i, ret = 0, count = 0;
 
 	clk_enable(i2c->clk);
-	meson_i2c_set_clk_div(i2c);
 
 	for (i = 0; i < num; i++) {
 		ret = meson_i2c_xfer_msg(i2c, msgs + i, i == num - 1);

@@ -396,6 +393,7 @@ static const struct i2c_algorithm meson_i2c_algorithm = {
 static int meson_i2c_probe(struct platform_device *pdev)
 {
 	struct device_node *np = pdev->dev.of_node;
+	struct i2c_timings timings;
 	struct meson_i2c *i2c;
 	struct resource *mem;
 	int irq, ret = 0;

@@ -404,7 +402,7 @@ static int meson_i2c_probe(struct platform_device *pdev)
 	if (!i2c)
 		return -ENOMEM;
 
-	i2c_parse_fw_timings(&pdev->dev, &i2c->timings, true);
+	i2c_parse_fw_timings(&pdev->dev, &timings, true);
 
 	i2c->dev = &pdev->dev;
 	platform_set_drvdata(pdev, i2c);

@@ -462,6 +460,8 @@ static int meson_i2c_probe(struct platform_device *pdev)
 		return ret;
 	}
 
+	meson_i2c_set_clk_div(i2c, timings.bus_freq_hz);
+
 	return 0;
 }
Signature · 1 line
2.12.0
maintainer
#8 lore raw

PATCH v2 5/10 i2c: meson: set clock divider in probe instead of setting it for each transfer

Quoted 16 lines · Heiner Kallweit

The bus frequency won't change, therefore we can set the clock divider in probe already and we don't have to set it for each transfer.

Signed-off-by Heiner Kallweit <[email protected]>

---
v2:
- no changes

---
 drivers/i2c/busses/i2c-meson.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)
drivers/i2c/busses/i2c-meson.c +0 -0
index a5764be5..594fec22 100644

drivers/i2c/busses/i2c-meson.c enum { · line 77

Quoted 4 lines
drivers/i2c/busses/i2c-meson.c +0 -1
@@ -74,7 +74,6 @@ enum {
  * @error:	Flag set when an error is received
  * @lock:	To avoid race conditions between irq handler and xfer code
  * @done:	Completion used to wait for transfer termination
- * @timings:	Struct including the bus frequency

You just added this field in PATCH 03/10. Why not do this patch first, which will simplify the other.

Kevin

Quoted 68 lines
drivers/i2c/busses/i2c-meson.c +7 -6
  * @tokens:	Sequence of tokens to be written to the device
  * @num_tokens:	Number of tokens
  */

@@ -93,7 +92,6 @@ struct meson_i2c {
 
 	spinlock_t		lock;
 	struct completion	done;
-	struct i2c_timings	timings;
 	u32			tokens[2];
 	int			num_tokens;
 };

@@ -132,12 +130,12 @@ static void meson_i2c_write_tokens(struct meson_i2c *i2c)
 	writel(i2c->tokens[1], i2c->regs + REG_TOK_LIST1);
 }
 
-static void meson_i2c_set_clk_div(struct meson_i2c *i2c)
+static void meson_i2c_set_clk_div(struct meson_i2c *i2c, unsigned int freq)
 {
 	unsigned long clk_rate = clk_get_rate(i2c->clk);
 	unsigned int div;
 
-	div = DIV_ROUND_UP(clk_rate, i2c->timings.bus_freq_hz * 4);
+	div = DIV_ROUND_UP(clk_rate, freq * 4);
 
 	/* clock divider has 12 bits */
 	WARN_ON(div >= (1 << 12));

@@ -149,7 +147,7 @@ static void meson_i2c_set_clk_div(struct meson_i2c *i2c)
 			   (div >> 10) << REG_CTRL_CLKDIVEXT_SHIFT);
 
 	dev_dbg(i2c->dev, "%s: clk %lu, freq %u, div %u\n", __func__,
-		clk_rate, i2c->timings.bus_freq_hz, div);
+		clk_rate, freq, div);
 }
 
 static void meson_i2c_get_data(struct meson_i2c *i2c, char *buf, int len)

@@ -369,7 +367,6 @@ static int meson_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs,
 	int i, ret = 0, count = 0;
 
 	clk_enable(i2c->clk);
-	meson_i2c_set_clk_div(i2c);
 
 	for (i = 0; i < num; i++) {
 		ret = meson_i2c_xfer_msg(i2c, msgs + i, i == num - 1);

@@ -396,6 +393,7 @@ static const struct i2c_algorithm meson_i2c_algorithm = {
 static int meson_i2c_probe(struct platform_device *pdev)
 {
 	struct device_node *np = pdev->dev.of_node;
+	struct i2c_timings timings;
 	struct meson_i2c *i2c;
 	struct resource *mem;
 	int irq, ret = 0;

@@ -404,7 +402,7 @@ static int meson_i2c_probe(struct platform_device *pdev)
 	if (!i2c)
 		return -ENOMEM;
 
-	i2c_parse_fw_timings(&pdev->dev, &i2c->timings, true);
+	i2c_parse_fw_timings(&pdev->dev, &timings, true);
 
 	i2c->dev = &pdev->dev;
 	platform_set_drvdata(pdev, i2c);

@@ -462,6 +460,8 @@ static int meson_i2c_probe(struct platform_device *pdev)
 		return ret;
 	}
 
+	meson_i2c_set_clk_div(i2c, timings.bus_freq_hz);
+
 	return 0;
 }
author
#9 lore raw

PATCH v2 5/10 i2c: meson: set clock divider in probe instead of setting it for each transfer

Am 11.03.2017 um 00:17 schrieb Kevin Hilman:

Quoted 13 lines
Quoted 11 lines · Heiner Kallweit

The bus frequency won't change, therefore we can set the clock divider in probe already and we don't have to set it for each transfer.

Signed-off-by Heiner Kallweit <[email protected]>

---
v2:
- no changes

---
 drivers/i2c/busses/i2c-meson.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)
Quoted 13 lines

drivers/i2c/busses/i2c-meson.c enum { · line 77

Quoted 9 lines
drivers/i2c/busses/i2c-meson.c +0 -1
index a5764be5..594fec22 100644
@@ -74,7 +74,6 @@ enum {
  * @error:	Flag set when an error is received
  * @lock:	To avoid race conditions between irq handler and xfer code
  * @done:	Completion used to wait for transfer termination
- * @timings:	Struct including the bus frequency

You just added this field in PATCH 03/10. Why not do this patch first, which will simplify the other.

Yes, most likely this would be better. Will keep this in mind for the upcoming v3.

Quoted 71 lines

Kevin

Quoted 68 lines
drivers/i2c/busses/i2c-meson.c +7 -6
  * @tokens:	Sequence of tokens to be written to the device
  * @num_tokens:	Number of tokens
  */

@@ -93,7 +92,6 @@ struct meson_i2c {
 
 	spinlock_t		lock;
 	struct completion	done;
-	struct i2c_timings	timings;
 	u32			tokens[2];
 	int			num_tokens;
 };

@@ -132,12 +130,12 @@ static void meson_i2c_write_tokens(struct meson_i2c *i2c)
 	writel(i2c->tokens[1], i2c->regs + REG_TOK_LIST1);
 }
 
-static void meson_i2c_set_clk_div(struct meson_i2c *i2c)
+static void meson_i2c_set_clk_div(struct meson_i2c *i2c, unsigned int freq)
 {
 	unsigned long clk_rate = clk_get_rate(i2c->clk);
 	unsigned int div;
 
-	div = DIV_ROUND_UP(clk_rate, i2c->timings.bus_freq_hz * 4);
+	div = DIV_ROUND_UP(clk_rate, freq * 4);
 
 	/* clock divider has 12 bits */
 	WARN_ON(div >= (1 << 12));

@@ -149,7 +147,7 @@ static void meson_i2c_set_clk_div(struct meson_i2c *i2c)
 			   (div >> 10) << REG_CTRL_CLKDIVEXT_SHIFT);
 
 	dev_dbg(i2c->dev, "%s: clk %lu, freq %u, div %u\n", __func__,
-		clk_rate, i2c->timings.bus_freq_hz, div);
+		clk_rate, freq, div);
 }
 
 static void meson_i2c_get_data(struct meson_i2c *i2c, char *buf, int len)

@@ -369,7 +367,6 @@ static int meson_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs,
 	int i, ret = 0, count = 0;
 
 	clk_enable(i2c->clk);
-	meson_i2c_set_clk_div(i2c);
 
 	for (i = 0; i < num; i++) {
 		ret = meson_i2c_xfer_msg(i2c, msgs + i, i == num - 1);

@@ -396,6 +393,7 @@ static const struct i2c_algorithm meson_i2c_algorithm = {
 static int meson_i2c_probe(struct platform_device *pdev)
 {
 	struct device_node *np = pdev->dev.of_node;
+	struct i2c_timings timings;
 	struct meson_i2c *i2c;
 	struct resource *mem;
 	int irq, ret = 0;

@@ -404,7 +402,7 @@ static int meson_i2c_probe(struct platform_device *pdev)
 	if (!i2c)
 		return -ENOMEM;
 
-	i2c_parse_fw_timings(&pdev->dev, &i2c->timings, true);
+	i2c_parse_fw_timings(&pdev->dev, &timings, true);
 
 	i2c->dev = &pdev->dev;
 	platform_set_drvdata(pdev, i2c);

@@ -462,6 +460,8 @@ static int meson_i2c_probe(struct platform_device *pdev)
 		return ret;
 	}
 
+	meson_i2c_set_clk_div(i2c, timings.bus_freq_hz);
+
 	return 0;
 }
author
#10 lore raw

PATCH v2 6/10 i2c: meson: remove variable count from meson_i2c_xfer

In reply to Heiner Kallweit

Variable count has always the same value as i, so we don't need it.

Signed-off-by Heiner Kallweit <[email protected]>

---
v2:
- remove one small change from v1

---
 drivers/i2c/busses/i2c-meson.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)
drivers/i2c/busses/i2c-meson.c +2 -3
index 594fec22..56e15a74 100644
@@ -364,7 +364,7 @@ static int meson_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs,
 			  int num)
 {
 	struct meson_i2c *i2c = adap->algo_data;
-	int i, ret = 0, count = 0;
+	int i, ret = 0;
 
 	clk_enable(i2c->clk);
 

@@ -372,12 +372,11 @@ static int meson_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs,
 		ret = meson_i2c_xfer_msg(i2c, msgs + i, i == num - 1);
 		if (ret)
 			break;
-		count++;
 	}
 
 	clk_disable(i2c->clk);
 
-	return ret ? ret : count;
+	return ret ?: i;
 }
 
 static u32 meson_i2c_func(struct i2c_adapter *adap)
Signature · 1 line
2.12.0
author
#11 lore raw

PATCH v2 7/10 i2c: meson: improve interrupt handler and detect spurious interrupts

In reply to Heiner Kallweit

If state is STATE_IDLE no interrupt should occur. Detect this case and warn.
In addition move resetting REG_CTRL_START bit to the start of the interrupt handler and remove a unneeded REG_CTRL_START bit reset in meson_i2c_xfer_msg.

Signed-off-by Heiner Kallweit <[email protected]>

---
v2:
- don't print an error if spurious interrupt is detected

---
 drivers/i2c/busses/i2c-meson.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)
drivers/i2c/busses/i2c-meson.c +7 -7
index 56e15a74..74d433cd 100644
@@ -228,12 +228,18 @@ static irqreturn_t meson_i2c_irq(int irqno, void *dev_id)
 	spin_lock(&i2c->lock);
 
 	meson_i2c_reset_tokens(i2c);
+	meson_i2c_set_mask(i2c, REG_CTRL, REG_CTRL_START, 0);
 	ctrl = readl(i2c->regs + REG_CTRL);
 
 	dev_dbg(i2c->dev, "irq: state %d, pos %d, count %d, ctrl %08x\n",
 		i2c->state, i2c->pos, i2c->count, ctrl);
 
-	if (ctrl & REG_CTRL_ERROR && i2c->state != STATE_IDLE) {
+	if (i2c->state == STATE_IDLE) {
+		spin_unlock(&i2c->lock);
+		return IRQ_NONE;
+	}
+
+	if (ctrl & REG_CTRL_ERROR) {
 		/*
 		 * The bit is set when the IGNORE_NAK bit is cleared
 		 * and the device didn't respond. In this case, the

@@ -276,15 +282,12 @@ static irqreturn_t meson_i2c_irq(int irqno, void *dev_id)
 		i2c->state = STATE_IDLE;
 		complete(&i2c->done);
 		break;
-	case STATE_IDLE:
-		break;
 	}
 
 out:
 	if (i2c->state != STATE_IDLE) {
 		/* Restart the processing */
 		meson_i2c_write_tokens(i2c);
-		meson_i2c_set_mask(i2c, REG_CTRL, REG_CTRL_START, 0);
 		meson_i2c_set_mask(i2c, REG_CTRL, REG_CTRL_START,
 				   REG_CTRL_START);
 	}

@@ -344,9 +347,6 @@ static int meson_i2c_xfer_msg(struct meson_i2c *i2c, struct i2c_msg *msg,
 	 */
 	spin_lock_irqsave(&i2c->lock, flags);
 
-	/* Abort any active operation */
-	meson_i2c_set_mask(i2c, REG_CTRL, REG_CTRL_START, 0);
-
 	if (!time_left) {
 		i2c->state = STATE_IDLE;
 		ret = -ETIMEDOUT;
Signature · 1 line
2.12.0
author
#12 lore raw

PATCH v2 8/10 i2c: meson: don't create separate token chain just for the stop command

In reply to Heiner Kallweit

We can directly add the stop token to the token chain including the last transfer chunk. This is more efficient than creating a separate token chain just for the stop command.
And it allows us to get rid of state STATE_STOP completely.

Signed-off-by Heiner Kallweit <[email protected]>

---
v2:
- rebased

---
 drivers/i2c/busses/i2c-meson.c | 23 +++++------------------
 1 file changed, 5 insertions(+), 18 deletions(-)
drivers/i2c/busses/i2c-meson.c +5 -18
index 74d433cd..3e6f5e50 100644
@@ -55,7 +55,6 @@ enum {
 	STATE_IDLE,
 	STATE_READ,
 	STATE_WRITE,
-	STATE_STOP,
 };
 
 /**

@@ -205,19 +204,9 @@ static void meson_i2c_prepare_xfer(struct meson_i2c *i2c)
 
 	if (write)
 		meson_i2c_put_data(i2c, i2c->msg->buf + i2c->pos, i2c->count);
-}
-
-static void meson_i2c_stop(struct meson_i2c *i2c)
-{
-	dev_dbg(i2c->dev, "%s: last %d\n", __func__, i2c->last);
 
-	if (i2c->last) {
-		i2c->state = STATE_STOP;
+	if (i2c->last && i2c->pos + i2c->count >= i2c->msg->len)
 		meson_i2c_add_token(i2c, TOKEN_STOP);
-	} else {
-		i2c->state = STATE_IDLE;
-		complete(&i2c->done);
-	}
 }
 
 static irqreturn_t meson_i2c_irq(int irqno, void *dev_id)

@@ -262,7 +251,8 @@ static irqreturn_t meson_i2c_irq(int irqno, void *dev_id)
 		}
 
 		if (i2c->pos >= i2c->msg->len) {
-			meson_i2c_stop(i2c);
+			i2c->state = STATE_IDLE;
+			complete(&i2c->done);
 			break;
 		}
 

@@ -272,16 +262,13 @@ static irqreturn_t meson_i2c_irq(int irqno, void *dev_id)
 		i2c->pos += i2c->count;
 
 		if (i2c->pos >= i2c->msg->len) {
-			meson_i2c_stop(i2c);
+			i2c->state = STATE_IDLE;
+			complete(&i2c->done);
 			break;
 		}
 
 		meson_i2c_prepare_xfer(i2c);
 		break;
-	case STATE_STOP:
-		i2c->state = STATE_IDLE;
-		complete(&i2c->done);
-		break;
 	}
 
 out:
Signature · 1 line
2.12.0
author
#13 lore raw

PATCH v2 9/10 i2c: meson: remove meson_i2c_write_tokens

In reply to Heiner Kallweit

meson_i2c_write_tokens is always called directly after meson_i2c_prepare_xfer (and only then). So we can simplify the code by removing meson_i2c_write_tokens and moving the two statements of meson_i2c_write_tokens to the end of meson_i2c_prepare_xfer.

Signed-off-by Heiner Kallweit <[email protected]>

---
v2:
- rebased

---
 drivers/i2c/busses/i2c-meson.c | 14 ++++----------
 1 file changed, 4 insertions(+), 10 deletions(-)
drivers/i2c/busses/i2c-meson.c +4 -10
index 3e6f5e50..8f4658de 100644
@@ -123,12 +123,6 @@ static void meson_i2c_add_token(struct meson_i2c *i2c, int token)
 	i2c->num_tokens++;
 }
 
-static void meson_i2c_write_tokens(struct meson_i2c *i2c)
-{
-	writel(i2c->tokens[0], i2c->regs + REG_TOK_LIST0);
-	writel(i2c->tokens[1], i2c->regs + REG_TOK_LIST1);
-}
-
 static void meson_i2c_set_clk_div(struct meson_i2c *i2c, unsigned int freq)
 {
 	unsigned long clk_rate = clk_get_rate(i2c->clk);

@@ -207,6 +201,9 @@ static void meson_i2c_prepare_xfer(struct meson_i2c *i2c)
 
 	if (i2c->last && i2c->pos + i2c->count >= i2c->msg->len)
 		meson_i2c_add_token(i2c, TOKEN_STOP);
+
+	writel(i2c->tokens[0], i2c->regs + REG_TOK_LIST0);
+	writel(i2c->tokens[1], i2c->regs + REG_TOK_LIST1);
 }
 
 static irqreturn_t meson_i2c_irq(int irqno, void *dev_id)

@@ -272,12 +269,10 @@ static irqreturn_t meson_i2c_irq(int irqno, void *dev_id)
 	}
 
 out:
-	if (i2c->state != STATE_IDLE) {
+	if (i2c->state != STATE_IDLE)
 		/* Restart the processing */
-		meson_i2c_write_tokens(i2c);
 		meson_i2c_set_mask(i2c, REG_CTRL, REG_CTRL_START,
 				   REG_CTRL_START);
-	}
 
 	spin_unlock(&i2c->lock);
 

@@ -318,7 +313,6 @@ static int meson_i2c_xfer_msg(struct meson_i2c *i2c, struct i2c_msg *msg,
 
 	i2c->state = (msg->flags & I2C_M_RD) ? STATE_READ : STATE_WRITE;
 	meson_i2c_prepare_xfer(i2c);
-	meson_i2c_write_tokens(i2c);
 	reinit_completion(&i2c->done);
 
 	/* Start the transfer */
Signature · 1 line
2.12.0
author
#14 lore raw

PATCH v2 10/10 i2c: meson: improve and simplify interrupt handler

In reply to Heiner Kallweit

The preceding changes in this patch series now allow to simplify the interrupt handler significantly.

Signed-off-by Heiner Kallweit <[email protected]>

---
v2:
- rebased

---
 drivers/i2c/busses/i2c-meson.c | 40 ++++++++++------------------------------
 1 file changed, 10 insertions(+), 30 deletions(-)
drivers/i2c/busses/i2c-meson.c +10 -30
index 8f4658de..25415690 100644
@@ -239,41 +239,21 @@ static irqreturn_t meson_i2c_irq(int irqno, void *dev_id)
 		goto out;
 	}
 
-	switch (i2c->state) {
-	case STATE_READ:
-		if (i2c->count > 0) {
-			meson_i2c_get_data(i2c, i2c->msg->buf + i2c->pos,
-					   i2c->count);
-			i2c->pos += i2c->count;
-		}
-
-		if (i2c->pos >= i2c->msg->len) {
-			i2c->state = STATE_IDLE;
-			complete(&i2c->done);
-			break;
-		}
-
-		meson_i2c_prepare_xfer(i2c);
-		break;
-	case STATE_WRITE:
-		i2c->pos += i2c->count;
+	if (i2c->state == STATE_READ && i2c->count)
+		meson_i2c_get_data(i2c, i2c->msg->buf + i2c->pos, i2c->count);
 
-		if (i2c->pos >= i2c->msg->len) {
-			i2c->state = STATE_IDLE;
-			complete(&i2c->done);
-			break;
-		}
+	i2c->pos += i2c->count;
 
-		meson_i2c_prepare_xfer(i2c);
-		break;
+	if (i2c->pos >= i2c->msg->len) {
+		i2c->state = STATE_IDLE;
+		complete(&i2c->done);
+		goto out;
 	}
 
+	/* Restart the processing */
+	meson_i2c_prepare_xfer(i2c);
+	meson_i2c_set_mask(i2c, REG_CTRL, REG_CTRL_START, REG_CTRL_START);
 out:
-	if (i2c->state != STATE_IDLE)
-		/* Restart the processing */
-		meson_i2c_set_mask(i2c, REG_CTRL, REG_CTRL_START,
-				   REG_CTRL_START);
-
 	spin_unlock(&i2c->lock);
 
 	return IRQ_HANDLED;
Signature · 1 line
2.12.0