[PATCH 00/12] i2c: meson: series with improvements

[PATCH 00/12] i2c: meson: series with improvements

[Heiner Kallweit]

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

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: explicitly ignore messages with length zero
  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
  i2c: meson: use state in meson_i2c_prepare_xfer

 drivers/i2c/busses/i2c-meson.c | 157 ++++++++++++++++-------------------------
 1 file changed, 62 insertions(+), 95 deletions(-)

-- 
2.12.0

[PATCH 00/12] i2c: meson: series with improvements

[Heiner Kallweit]

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

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: explicitly ignore messages with length zero
  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
  i2c: meson: use state in meson_i2c_prepare_xfer

 drivers/i2c/busses/i2c-meson.c | 157 ++++++++++++++++-------------------------
 1 file changed, 62 insertions(+), 95 deletions(-)

-- 
2.12.0

[PATCH 01/12] i2c: meson: use min instead of min_t where min_t isn't needed

[Heiner Kallweit]

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

Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
 drivers/i2c/busses/i2c-meson.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/i2c/busses/i2c-meson.c b/drivers/i2c/busses/i2c-meson.c
index 73b97c71..40e5da9a 100644
--- a/drivers/i2c/busses/i2c-meson.c
+++ b/drivers/i2c/busses/i2c-meson.c
@@ -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);
-- 
2.12.0

[PATCH 01/12] i2c: meson: use min instead of min_t where min_t isn't needed

[Heiner Kallweit]

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

Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
 drivers/i2c/busses/i2c-meson.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/i2c/busses/i2c-meson.c b/drivers/i2c/busses/i2c-meson.c
index 73b97c71..40e5da9a 100644
--- a/drivers/i2c/busses/i2c-meson.c
+++ b/drivers/i2c/busses/i2c-meson.c
@@ -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);
-- 
2.12.0

[PATCH 02/12] i2c: meson: remove member irq from struct meson_i2c

[Heiner Kallweit]

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

Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
 drivers/i2c/busses/i2c-meson.c | 13 ++++++-------
 1 file changed, 6 insertions(+), 7 deletions(-)

diff --git a/drivers/i2c/busses/i2c-meson.c b/drivers/i2c/busses/i2c-meson.c
index 40e5da9a..50059d09 100644
--- a/drivers/i2c/busses/i2c-meson.c
+++ b/drivers/i2c/busses/i2c-meson.c
@@ -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;
-- 
2.12.0

[PATCH 02/12] i2c: meson: remove member irq from struct meson_i2c

[Heiner Kallweit]

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

Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
 drivers/i2c/busses/i2c-meson.c | 13 ++++++-------
 1 file changed, 6 insertions(+), 7 deletions(-)

diff --git a/drivers/i2c/busses/i2c-meson.c b/drivers/i2c/busses/i2c-meson.c
index 40e5da9a..50059d09 100644
--- a/drivers/i2c/busses/i2c-meson.c
+++ b/drivers/i2c/busses/i2c-meson.c
@@ -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;
-- 
2.12.0

[PATCH 03/12] i2c: meson: use i2c core for DT clock-frequency parsing

[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 <hkallweit1@gmail.com>
---
 drivers/i2c/busses/i2c-meson.c | 13 +++++--------
 1 file changed, 5 insertions(+), 8 deletions(-)

diff --git a/drivers/i2c/busses/i2c-meson.c b/drivers/i2c/busses/i2c-meson.c
index 50059d09..5e243efa 100644
--- a/drivers/i2c/busses/i2c-meson.c
+++ b/drivers/i2c/busses/i2c-meson.c
@@ -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);
-- 
2.12.0

[PATCH 03/12] i2c: meson: use i2c core for DT clock-frequency parsing

[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 <hkallweit1@gmail.com>
---
 drivers/i2c/busses/i2c-meson.c | 13 +++++--------
 1 file changed, 5 insertions(+), 8 deletions(-)

diff --git a/drivers/i2c/busses/i2c-meson.c b/drivers/i2c/busses/i2c-meson.c
index 50059d09..5e243efa 100644
--- a/drivers/i2c/busses/i2c-meson.c
+++ b/drivers/i2c/busses/i2c-meson.c
@@ -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);
-- 
2.12.0

[PATCH 04/12] i2c: meson: use full 12 bits for clock divider

[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 <hkallweit1@gmail.com>
---
 drivers/i2c/busses/i2c-meson.c | 13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)

diff --git a/drivers/i2c/busses/i2c-meson.c b/drivers/i2c/busses/i2c-meson.c
index 5e243efa..a5764be5 100644
--- a/drivers/i2c/busses/i2c-meson.c
+++ b/drivers/i2c/busses/i2c-meson.c
@@ -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);
-- 
2.12.0

[PATCH 04/12] i2c: meson: use full 12 bits for clock divider

[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 <hkallweit1@gmail.com>
---
 drivers/i2c/busses/i2c-meson.c | 13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)

diff --git a/drivers/i2c/busses/i2c-meson.c b/drivers/i2c/busses/i2c-meson.c
index 5e243efa..a5764be5 100644
--- a/drivers/i2c/busses/i2c-meson.c
+++ b/drivers/i2c/busses/i2c-meson.c
@@ -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);
-- 
2.12.0

[PATCH 05/12] i2c: meson: set clock divider in probe instead of setting it for each transfer

[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 <hkallweit1@gmail.com>
---
 drivers/i2c/busses/i2c-meson.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/drivers/i2c/busses/i2c-meson.c b/drivers/i2c/busses/i2c-meson.c
index a5764be5..594fec22 100644
--- a/drivers/i2c/busses/i2c-meson.c
+++ b/drivers/i2c/busses/i2c-meson.c
@@ -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;
 }
 
-- 
2.12.0

[PATCH 05/12] i2c: meson: set clock divider in probe instead of setting it for each transfer

[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 <hkallweit1@gmail.com>
---
 drivers/i2c/busses/i2c-meson.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/drivers/i2c/busses/i2c-meson.c b/drivers/i2c/busses/i2c-meson.c
index a5764be5..594fec22 100644
--- a/drivers/i2c/busses/i2c-meson.c
+++ b/drivers/i2c/busses/i2c-meson.c
@@ -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;
 }
 
-- 
2.12.0

[PATCH 06/12] i2c: meson: remove variable count from meson_i2c_xfer

[Heiner Kallweit]

Variable count has always the same value as i, so we don't need it.
In addition use &msgs[i] instead of msgs + i to improve readability
of code a little.

Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
 drivers/i2c/busses/i2c-meson.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/drivers/i2c/busses/i2c-meson.c b/drivers/i2c/busses/i2c-meson.c
index 594fec22..81304840 100644
--- a/drivers/i2c/busses/i2c-meson.c
+++ b/drivers/i2c/busses/i2c-meson.c
@@ -364,20 +364,19 @@ 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);
 
 	for (i = 0; i < num; i++) {
-		ret = meson_i2c_xfer_msg(i2c, msgs + i, i == num - 1);
+		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)
-- 
2.12.0

[PATCH 06/12] i2c: meson: remove variable count from meson_i2c_xfer

[Heiner Kallweit]

Variable count has always the same value as i, so we don't need it.
In addition use &msgs[i] instead of msgs + i to improve readability
of code a little.

Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
 drivers/i2c/busses/i2c-meson.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/drivers/i2c/busses/i2c-meson.c b/drivers/i2c/busses/i2c-meson.c
index 594fec22..81304840 100644
--- a/drivers/i2c/busses/i2c-meson.c
+++ b/drivers/i2c/busses/i2c-meson.c
@@ -364,20 +364,19 @@ 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);
 
 	for (i = 0; i < num; i++) {
-		ret = meson_i2c_xfer_msg(i2c, msgs + i, i == num - 1);
+		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)
-- 
2.12.0

[PATCH 07/12] i2c: meson: improve interrupt handler and detect spurious interrupts

[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 <hkallweit1@gmail.com>
---
 drivers/i2c/busses/i2c-meson.c | 16 +++++++++-------
 1 file changed, 9 insertions(+), 7 deletions(-)

diff --git a/drivers/i2c/busses/i2c-meson.c b/drivers/i2c/busses/i2c-meson.c
index 81304840..b3b881f9 100644
--- a/drivers/i2c/busses/i2c-meson.c
+++ b/drivers/i2c/busses/i2c-meson.c
@@ -233,7 +233,15 @@ static irqreturn_t meson_i2c_irq(int irqno, void *dev_id)
 	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) {
+	meson_i2c_set_mask(i2c, REG_CTRL, REG_CTRL_START, 0);
+
+	if (i2c->state == STATE_IDLE) {
+		dev_notice(i2c->dev, "spurious interrupt detected\n");
+		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 +284,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 +349,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;
-- 
2.12.0

[PATCH 07/12] i2c: meson: improve interrupt handler and detect spurious interrupts

[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 <hkallweit1@gmail.com>
---
 drivers/i2c/busses/i2c-meson.c | 16 +++++++++-------
 1 file changed, 9 insertions(+), 7 deletions(-)

diff --git a/drivers/i2c/busses/i2c-meson.c b/drivers/i2c/busses/i2c-meson.c
index 81304840..b3b881f9 100644
--- a/drivers/i2c/busses/i2c-meson.c
+++ b/drivers/i2c/busses/i2c-meson.c
@@ -233,7 +233,15 @@ static irqreturn_t meson_i2c_irq(int irqno, void *dev_id)
 	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) {
+	meson_i2c_set_mask(i2c, REG_CTRL, REG_CTRL_START, 0);
+
+	if (i2c->state == STATE_IDLE) {
+		dev_notice(i2c->dev, "spurious interrupt detected\n");
+		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 +284,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 +349,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;
-- 
2.12.0

[PATCH 08/12] i2c: meson: explicitly ignore messages with length zero

[Heiner Kallweit]

Explicitely ignore messages with length zero. This also allows to
remove some now unneeded checks during message processing.

Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
 drivers/i2c/busses/i2c-meson.c | 20 +++++++++-----------
 1 file changed, 9 insertions(+), 11 deletions(-)

diff --git a/drivers/i2c/busses/i2c-meson.c b/drivers/i2c/busses/i2c-meson.c
index b3b881f9..58414699 100644
--- a/drivers/i2c/busses/i2c-meson.c
+++ b/drivers/i2c/busses/i2c-meson.c
@@ -196,12 +196,10 @@ static void meson_i2c_prepare_xfer(struct meson_i2c *i2c)
 	for (i = 0; i < i2c->count - 1; i++)
 		meson_i2c_add_token(i2c, TOKEN_DATA);
 
-	if (i2c->count) {
-		if (write || i2c->pos + i2c->count < i2c->msg->len)
-			meson_i2c_add_token(i2c, TOKEN_DATA);
-		else
-			meson_i2c_add_token(i2c, TOKEN_DATA_LAST);
-	}
+	if (write || i2c->pos + i2c->count < i2c->msg->len)
+		meson_i2c_add_token(i2c, TOKEN_DATA);
+	else
+		meson_i2c_add_token(i2c, TOKEN_DATA_LAST);
 
 	if (write)
 		meson_i2c_put_data(i2c, i2c->msg->buf + i2c->pos, i2c->count);
@@ -257,11 +255,8 @@ static irqreturn_t meson_i2c_irq(int irqno, void *dev_id)
 
 	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;
-		}
+		meson_i2c_get_data(i2c, i2c->msg->buf + i2c->pos, i2c->count);
+		i2c->pos += i2c->count;
 
 		if (i2c->pos >= i2c->msg->len) {
 			meson_i2c_stop(i2c);
@@ -371,6 +366,9 @@ static int meson_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs,
 	clk_enable(i2c->clk);
 
 	for (i = 0; i < num; i++) {
+		/* ignore messages with length 0 */
+		if (!msgs[i].len)
+			continue;
 		ret = meson_i2c_xfer_msg(i2c, &msgs[i], i == num - 1);
 		if (ret)
 			break;
-- 
2.12.0

[PATCH 08/12] i2c: meson: explicitly ignore messages with length zero

[Heiner Kallweit]

Explicitely ignore messages with length zero. This also allows to
remove some now unneeded checks during message processing.

Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
 drivers/i2c/busses/i2c-meson.c | 20 +++++++++-----------
 1 file changed, 9 insertions(+), 11 deletions(-)

diff --git a/drivers/i2c/busses/i2c-meson.c b/drivers/i2c/busses/i2c-meson.c
index b3b881f9..58414699 100644
--- a/drivers/i2c/busses/i2c-meson.c
+++ b/drivers/i2c/busses/i2c-meson.c
@@ -196,12 +196,10 @@ static void meson_i2c_prepare_xfer(struct meson_i2c *i2c)
 	for (i = 0; i < i2c->count - 1; i++)
 		meson_i2c_add_token(i2c, TOKEN_DATA);
 
-	if (i2c->count) {
-		if (write || i2c->pos + i2c->count < i2c->msg->len)
-			meson_i2c_add_token(i2c, TOKEN_DATA);
-		else
-			meson_i2c_add_token(i2c, TOKEN_DATA_LAST);
-	}
+	if (write || i2c->pos + i2c->count < i2c->msg->len)
+		meson_i2c_add_token(i2c, TOKEN_DATA);
+	else
+		meson_i2c_add_token(i2c, TOKEN_DATA_LAST);
 
 	if (write)
 		meson_i2c_put_data(i2c, i2c->msg->buf + i2c->pos, i2c->count);
@@ -257,11 +255,8 @@ static irqreturn_t meson_i2c_irq(int irqno, void *dev_id)
 
 	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;
-		}
+		meson_i2c_get_data(i2c, i2c->msg->buf + i2c->pos, i2c->count);
+		i2c->pos += i2c->count;
 
 		if (i2c->pos >= i2c->msg->len) {
 			meson_i2c_stop(i2c);
@@ -371,6 +366,9 @@ static int meson_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs,
 	clk_enable(i2c->clk);
 
 	for (i = 0; i < num; i++) {
+		/* ignore messages with length 0 */
+		if (!msgs[i].len)
+			continue;
 		ret = meson_i2c_xfer_msg(i2c, &msgs[i], i == num - 1);
 		if (ret)
 			break;
-- 
2.12.0

[PATCH 09/12] i2c: meson: don't create separate token chain just for the stop command

[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 <hkallweit1@gmail.com>
---
 drivers/i2c/busses/i2c-meson.c | 27 +++++++--------------------
 1 file changed, 7 insertions(+), 20 deletions(-)

diff --git a/drivers/i2c/busses/i2c-meson.c b/drivers/i2c/busses/i2c-meson.c
index 58414699..b11ecfdd 100644
--- a/drivers/i2c/busses/i2c-meson.c
+++ b/drivers/i2c/busses/i2c-meson.c
@@ -55,7 +55,6 @@ enum {
 	STATE_IDLE,
 	STATE_READ,
 	STATE_WRITE,
-	STATE_STOP,
 };
 
 /**
@@ -201,23 +200,13 @@ static void meson_i2c_prepare_xfer(struct meson_i2c *i2c)
 	else
 		meson_i2c_add_token(i2c, TOKEN_DATA_LAST);
 
+	if (i2c->last && i2c->pos + i2c->count >= i2c->msg->len)
+		meson_i2c_add_token(i2c, TOKEN_STOP);
+
 	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;
-		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)
 {
 	struct meson_i2c *i2c = dev_id;
@@ -259,7 +248,8 @@ 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;
 		}
 
@@ -269,16 +259,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:
-- 
2.12.0

[PATCH 09/12] i2c: meson: don't create separate token chain just for the stop command

[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 <hkallweit1@gmail.com>
---
 drivers/i2c/busses/i2c-meson.c | 27 +++++++--------------------
 1 file changed, 7 insertions(+), 20 deletions(-)

diff --git a/drivers/i2c/busses/i2c-meson.c b/drivers/i2c/busses/i2c-meson.c
index 58414699..b11ecfdd 100644
--- a/drivers/i2c/busses/i2c-meson.c
+++ b/drivers/i2c/busses/i2c-meson.c
@@ -55,7 +55,6 @@ enum {
 	STATE_IDLE,
 	STATE_READ,
 	STATE_WRITE,
-	STATE_STOP,
 };
 
 /**
@@ -201,23 +200,13 @@ static void meson_i2c_prepare_xfer(struct meson_i2c *i2c)
 	else
 		meson_i2c_add_token(i2c, TOKEN_DATA_LAST);
 
+	if (i2c->last && i2c->pos + i2c->count >= i2c->msg->len)
+		meson_i2c_add_token(i2c, TOKEN_STOP);
+
 	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;
-		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)
 {
 	struct meson_i2c *i2c = dev_id;
@@ -259,7 +248,8 @@ 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;
 		}
 
@@ -269,16 +259,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:
-- 
2.12.0

[PATCH 10/12] i2c: meson: remove meson_i2c_write_tokens

[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 <hkallweit1@gmail.com>
---
 drivers/i2c/busses/i2c-meson.c | 14 ++++----------
 1 file changed, 4 insertions(+), 10 deletions(-)

diff --git a/drivers/i2c/busses/i2c-meson.c b/drivers/i2c/busses/i2c-meson.c
index b11ecfdd..c73bb57a 100644
--- a/drivers/i2c/busses/i2c-meson.c
+++ b/drivers/i2c/busses/i2c-meson.c
@@ -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);
@@ -205,6 +199,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);
+
+	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)
@@ -269,12 +266,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);
 
@@ -315,7 +310,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 */
-- 
2.12.0

[PATCH 10/12] i2c: meson: remove meson_i2c_write_tokens

[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 <hkallweit1@gmail.com>
---
 drivers/i2c/busses/i2c-meson.c | 14 ++++----------
 1 file changed, 4 insertions(+), 10 deletions(-)

diff --git a/drivers/i2c/busses/i2c-meson.c b/drivers/i2c/busses/i2c-meson.c
index b11ecfdd..c73bb57a 100644
--- a/drivers/i2c/busses/i2c-meson.c
+++ b/drivers/i2c/busses/i2c-meson.c
@@ -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);
@@ -205,6 +199,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);
+
+	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)
@@ -269,12 +266,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);
 
@@ -315,7 +310,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 */
-- 
2.12.0

[PATCH 11/12] i2c: meson: improve and simplify interrupt handler

[Heiner Kallweit]

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

Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
 drivers/i2c/busses/i2c-meson.c | 35 +++++++++--------------------------
 1 file changed, 9 insertions(+), 26 deletions(-)

diff --git a/drivers/i2c/busses/i2c-meson.c b/drivers/i2c/busses/i2c-meson.c
index c73bb57a..7b29f077 100644
--- a/drivers/i2c/busses/i2c-meson.c
+++ b/drivers/i2c/busses/i2c-meson.c
@@ -239,38 +239,21 @@ static irqreturn_t meson_i2c_irq(int irqno, void *dev_id)
 		goto out;
 	}
 
-	switch (i2c->state) {
-	case STATE_READ:
+	if (i2c->state == STATE_READ)
 		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;
+	i2c->pos += i2c->count;
 
-		if (i2c->pos >= i2c->msg->len) {
-			i2c->state = STATE_IDLE;
-			complete(&i2c->done);
-			break;
-		}
-
-		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;
-- 
2.12.0

[PATCH 11/12] i2c: meson: improve and simplify interrupt handler

[Heiner Kallweit]

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

Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
 drivers/i2c/busses/i2c-meson.c | 35 +++++++++--------------------------
 1 file changed, 9 insertions(+), 26 deletions(-)

diff --git a/drivers/i2c/busses/i2c-meson.c b/drivers/i2c/busses/i2c-meson.c
index c73bb57a..7b29f077 100644
--- a/drivers/i2c/busses/i2c-meson.c
+++ b/drivers/i2c/busses/i2c-meson.c
@@ -239,38 +239,21 @@ static irqreturn_t meson_i2c_irq(int irqno, void *dev_id)
 		goto out;
 	}
 
-	switch (i2c->state) {
-	case STATE_READ:
+	if (i2c->state == STATE_READ)
 		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;
+	i2c->pos += i2c->count;
 
-		if (i2c->pos >= i2c->msg->len) {
-			i2c->state = STATE_IDLE;
-			complete(&i2c->done);
-			break;
-		}
-
-		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;
-- 
2.12.0

[PATCH 12/12] i2c: meson: use state in meson_i2c_prepare_xfer

[Heiner Kallweit]

Let's use the state here to make meson_i2c_prepare_xfer more in line
with other parts of the driver.

Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
 drivers/i2c/busses/i2c-meson.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/i2c/busses/i2c-meson.c b/drivers/i2c/busses/i2c-meson.c
index 7b29f077..eacb6f3d 100644
--- a/drivers/i2c/busses/i2c-meson.c
+++ b/drivers/i2c/busses/i2c-meson.c
@@ -181,7 +181,6 @@ static void meson_i2c_put_data(struct meson_i2c *i2c, char *buf, int len)
 
 static void meson_i2c_prepare_xfer(struct meson_i2c *i2c)
 {
-	bool write = !(i2c->msg->flags & I2C_M_RD);
 	int i;
 
 	i2c->count = min(i2c->msg->len - i2c->pos, 8);
@@ -189,7 +188,7 @@ static void meson_i2c_prepare_xfer(struct meson_i2c *i2c)
 	for (i = 0; i < i2c->count - 1; i++)
 		meson_i2c_add_token(i2c, TOKEN_DATA);
 
-	if (write || i2c->pos + i2c->count < i2c->msg->len)
+	if (i2c->state == STATE_WRITE || i2c->pos + i2c->count < i2c->msg->len)
 		meson_i2c_add_token(i2c, TOKEN_DATA);
 	else
 		meson_i2c_add_token(i2c, TOKEN_DATA_LAST);
@@ -197,7 +196,7 @@ 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);
 
-	if (write)
+	if (i2c->state == STATE_WRITE)
 		meson_i2c_put_data(i2c, i2c->msg->buf + i2c->pos, i2c->count);
 
 	writel(i2c->tokens[0], i2c->regs + REG_TOK_LIST0);
-- 
2.12.0

[PATCH 12/12] i2c: meson: use state in meson_i2c_prepare_xfer

[Heiner Kallweit]

Let's use the state here to make meson_i2c_prepare_xfer more in line
with other parts of the driver.

Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
 drivers/i2c/busses/i2c-meson.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/i2c/busses/i2c-meson.c b/drivers/i2c/busses/i2c-meson.c
index 7b29f077..eacb6f3d 100644
--- a/drivers/i2c/busses/i2c-meson.c
+++ b/drivers/i2c/busses/i2c-meson.c
@@ -181,7 +181,6 @@ static void meson_i2c_put_data(struct meson_i2c *i2c, char *buf, int len)
 
 static void meson_i2c_prepare_xfer(struct meson_i2c *i2c)
 {
-	bool write = !(i2c->msg->flags & I2C_M_RD);
 	int i;
 
 	i2c->count = min(i2c->msg->len - i2c->pos, 8);
@@ -189,7 +188,7 @@ static void meson_i2c_prepare_xfer(struct meson_i2c *i2c)
 	for (i = 0; i < i2c->count - 1; i++)
 		meson_i2c_add_token(i2c, TOKEN_DATA);
 
-	if (write || i2c->pos + i2c->count < i2c->msg->len)
+	if (i2c->state == STATE_WRITE || i2c->pos + i2c->count < i2c->msg->len)
 		meson_i2c_add_token(i2c, TOKEN_DATA);
 	else
 		meson_i2c_add_token(i2c, TOKEN_DATA_LAST);
@@ -197,7 +196,7 @@ 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);
 
-	if (write)
+	if (i2c->state == STATE_WRITE)
 		meson_i2c_put_data(i2c, i2c->msg->buf + i2c->pos, i2c->count);
 
 	writel(i2c->tokens[0], i2c->regs + REG_TOK_LIST0);
-- 
2.12.0

Re: [PATCH 02/12] i2c: meson: remove member irq from struct meson_i2c

[Jerome Brunet]

On Wed, 2017-03-08 at 07:43 +0100, Heiner Kallweit wrote:
> Member irq can be replaced with a local variable in probe
> because it's nowhere else accessed.
> 
> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
> ---
>  drivers/i2c/busses/i2c-meson.c | 13 ++++++-------
>  1 file changed, 6 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-meson.c b/drivers/i2c/busses/i2c-
> meson.c
> index 40e5da9a..50059d09 100644
> --- a/drivers/i2c/busses/i2c-meson.c
> +++ b/drivers/i2c/busses/i2c-meson.c
> @@ -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;

Reviewed-by: Jerome Brunet <jbrunet@baylibre.com>

[PATCH 02/12] i2c: meson: remove member irq from struct meson_i2c

[Jerome Brunet]

On Wed, 2017-03-08 at 07:43 +0100, Heiner Kallweit wrote:
> Member irq can be replaced with a local variable in probe
> because it's nowhere else accessed.
> 
> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
> ---
> ?drivers/i2c/busses/i2c-meson.c | 13 ++++++-------
> ?1 file changed, 6 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-meson.c b/drivers/i2c/busses/i2c-
> meson.c
> index 40e5da9a..50059d09 100644
> --- a/drivers/i2c/busses/i2c-meson.c
> +++ b/drivers/i2c/busses/i2c-meson.c
> @@ -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;

Reviewed-by:?Jerome Brunet <jbrunet@baylibre.com>

Re: [PATCH 03/12] i2c: meson: use i2c core for DT clock-frequency parsing

[Jerome Brunet]

On Wed, 2017-03-08 at 07:44 +0100, Heiner Kallweit wrote:
> 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 <hkallweit1@gmail.com>
> ---
>  drivers/i2c/busses/i2c-meson.c | 13 +++++--------
>  1 file changed, 5 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-meson.c b/drivers/i2c/busses/i2c-
> meson.c
> index 50059d09..5e243efa 100644
> --- a/drivers/i2c/busses/i2c-meson.c
> +++ b/drivers/i2c/busses/i2c-meson.c
> @@ -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);

Reviewed-by: Jerome Brunet <jbrunet@baylibre.com>

[PATCH 03/12] i2c: meson: use i2c core for DT clock-frequency parsing

[Jerome Brunet]

On Wed, 2017-03-08 at 07:44 +0100, Heiner Kallweit wrote:
> 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 <hkallweit1@gmail.com>
> ---
> ?drivers/i2c/busses/i2c-meson.c | 13 +++++--------
> ?1 file changed, 5 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-meson.c b/drivers/i2c/busses/i2c-
> meson.c
> index 50059d09..5e243efa 100644
> --- a/drivers/i2c/busses/i2c-meson.c
> +++ b/drivers/i2c/busses/i2c-meson.c
> @@ -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);

Reviewed-by: Jerome Brunet <jbrunet@baylibre.com>

Re: [PATCH 04/12] i2c: meson: use full 12 bits for clock divider

[Jerome Brunet]

On Wed, 2017-03-08 at 07:44 +0100, Heiner Kallweit wrote:
> 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 <hkallweit1@gmail.com>
> ---
>  drivers/i2c/busses/i2c-meson.c | 13 +++++++++++--
>  1 file changed, 11 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-meson.c b/drivers/i2c/busses/i2c-meson.c
> index 5e243efa..a5764be5 100644
> --- a/drivers/i2c/busses/i2c-meson.c
> +++ b/drivers/i2c/busses/i2c-meson.c
> @@ -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);

Acked-by: Jerome Brunet <jbrunet@baylibre.com>

[PATCH 04/12] i2c: meson: use full 12 bits for clock divider

[Jerome Brunet]

On Wed, 2017-03-08 at 07:44 +0100, Heiner Kallweit wrote:
> 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 <hkallweit1@gmail.com>
> ---
> ?drivers/i2c/busses/i2c-meson.c | 13 +++++++++++--
> ?1 file changed, 11 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-meson.c b/drivers/i2c/busses/i2c-meson.c
> index 5e243efa..a5764be5 100644
> --- a/drivers/i2c/busses/i2c-meson.c
> +++ b/drivers/i2c/busses/i2c-meson.c
> @@ -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);

Acked-by: Jerome Brunet <jbrunet@baylibre.com>

Re: [PATCH 01/12] i2c: meson: use min instead of min_t where min_t isn't needed

[Jerome Brunet]

On Wed, 2017-03-08 at 07:42 +0100, Heiner Kallweit wrote:
> Use min instead of min_t where min_t isn't needed.
> 
> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
> ---
>  drivers/i2c/busses/i2c-meson.c | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-meson.c b/drivers/i2c/busses/i2c-
> meson.c
> index 73b97c71..40e5da9a 100644
> --- a/drivers/i2c/busses/i2c-meson.c
> +++ b/drivers/i2c/busses/i2c-meson.c
> @@ -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);

Since the second argument of min is always a constant, this patch does
not change anything, but it doesn't hurt either. I suppose it is OK

[PATCH 01/12] i2c: meson: use min instead of min_t where min_t isn't needed

[Jerome Brunet]

On Wed, 2017-03-08 at 07:42 +0100, Heiner Kallweit wrote:
> Use min instead of min_t where min_t isn't needed.
> 
> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
> ---
> ?drivers/i2c/busses/i2c-meson.c | 10 +++++-----
> ?1 file changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-meson.c b/drivers/i2c/busses/i2c-
> meson.c
> index 73b97c71..40e5da9a 100644
> --- a/drivers/i2c/busses/i2c-meson.c
> +++ b/drivers/i2c/busses/i2c-meson.c
> @@ -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);

Since the second argument of min is always a constant, this patch does
not change anything, but it doesn't hurt either. I suppose it is OK

Re: [PATCH 06/12] i2c: meson: remove variable count from meson_i2c_xfer

[Jerome Brunet]

On Wed, 2017-03-08 at 07:46 +0100, Heiner Kallweit wrote:
> Variable count has always the same value as i, so we don't need it.
> In addition use &msgs[i] instead of msgs + i to improve readability
> of code a little.
> 
> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
> ---
>  drivers/i2c/busses/i2c-meson.c | 7 +++----
>  1 file changed, 3 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-meson.c b/drivers/i2c/busses/i2c-meson.c
> index 594fec22..81304840 100644
> --- a/drivers/i2c/busses/i2c-meson.c
> +++ b/drivers/i2c/busses/i2c-meson.c
> @@ -364,20 +364,19 @@ 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);
>  
>  	for (i = 0; i < num; i++) {
> -		ret = meson_i2c_xfer_msg(i2c, msgs + i, i == num - 1);
> +		ret = meson_i2c_xfer_msg(i2c, &msgs[i], i == num - 1);

It's a matter of taste whether this is more readable. I personally prefer it the
way it was.


>  		if (ret)
>  			break;
> -		count++;
>  	}
>  
>  	clk_disable(i2c->clk);
>  
> -	return ret ? ret : count;
> +	return ret ?: i;
>  }
>  
>  static u32 meson_i2c_func(struct i2c_adapter *adap)

[PATCH 06/12] i2c: meson: remove variable count from meson_i2c_xfer

[Jerome Brunet]

On Wed, 2017-03-08 at 07:46 +0100, Heiner Kallweit wrote:
> Variable count has always the same value as i, so we don't need it.
> In addition use &msgs[i] instead of msgs + i to improve readability
> of code a little.
> 
> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
> ---
> ?drivers/i2c/busses/i2c-meson.c | 7 +++----
> ?1 file changed, 3 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-meson.c b/drivers/i2c/busses/i2c-meson.c
> index 594fec22..81304840 100644
> --- a/drivers/i2c/busses/i2c-meson.c
> +++ b/drivers/i2c/busses/i2c-meson.c
> @@ -364,20 +364,19 @@ 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);
> ?
> ?	for (i = 0; i < num; i++) {
> -		ret = meson_i2c_xfer_msg(i2c, msgs + i, i == num - 1);
> +		ret = meson_i2c_xfer_msg(i2c, &msgs[i], i == num - 1);

It's a matter of taste whether this is more readable. I personally prefer it the
way it was.


> ?		if (ret)
> ?			break;
> -		count++;
> ?	}
> ?
> ?	clk_disable(i2c->clk);
> ?
> -	return ret ? ret : count;
> +	return ret ?: i;
> ?}
> ?
> ?static u32 meson_i2c_func(struct i2c_adapter *adap)

Re: [PATCH 00/12] i2c: meson: series with improvements

[Jerome Brunet]

On Wed, 2017-03-08 at 07:41 +0100, Heiner Kallweit wrote:
> This patch series includes several improvements for the i2c-meson
> driver.

Heiner, Could give us a some details about how you tested this series (platform,
board, i2c devices, exemple of transfers, etc ...). The more details you give,
the easiest it is to be confident about the patches.

Personally, I don't have test case representative enough to test this series
thoroughly.

> 
> 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: explicitly ignore messages with length zero
>   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
>   i2c: meson: use state in meson_i2c_prepare_xfer
> 
>  drivers/i2c/busses/i2c-meson.c | 157 ++++++++++++++++----------------------
> ---
>  1 file changed, 62 insertions(+), 95 deletions(-)
> 

[PATCH 00/12] i2c: meson: series with improvements

[Jerome Brunet]

On Wed, 2017-03-08 at 07:41 +0100, Heiner Kallweit wrote:
> This patch series includes several improvements for the i2c-meson
> driver.

Heiner, Could give us a some details about how you tested this series (platform,
board, i2c devices, exemple of transfers, etc ...). The more details you give,
the easiest it is to be confident about the patches.

Personally, I don't have test case representative enough to test this series
thoroughly.

> 
> 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: explicitly ignore messages with length zero
> ? 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
> ? i2c: meson: use state in meson_i2c_prepare_xfer
> 
> ?drivers/i2c/busses/i2c-meson.c | 157 ++++++++++++++++----------------------
> ---
> ?1 file changed, 62 insertions(+), 95 deletions(-)
> 

Re: [PATCH 07/12] i2c: meson: improve interrupt handler and detect spurious interrupts

[Jerome Brunet]

On Wed, 2017-03-08 at 07:47 +0100, Heiner Kallweit wrote:
> 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 <hkallweit1@gmail.com>
> ---
>  drivers/i2c/busses/i2c-meson.c | 16 +++++++++-------
>  1 file changed, 9 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-meson.c b/drivers/i2c/busses/i2c-meson.c
> index 81304840..b3b881f9 100644
> --- a/drivers/i2c/busses/i2c-meson.c
> +++ b/drivers/i2c/busses/i2c-meson.c
> @@ -233,7 +233,15 @@ static irqreturn_t meson_i2c_irq(int irqno, void *dev_id)
>  	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) {
> +	meson_i2c_set_mask(i2c, REG_CTRL, REG_CTRL_START, 0);
> +
> +	if (i2c->state == STATE_IDLE) {
> +		dev_notice(i2c->dev, "spurious interrupt detected\n");
> +		spin_unlock(&i2c->lock);
> +		return IRQ_NONE;
> +	}

Does it really happen ? Did you see any specific issue you'd like to share ?
If not, I'm not a big fan of this change.

In any case, if it can be handled gracefully, I'd drop the trace in the
interrupt handler.


> +
> +	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 +284,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 +349,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;

[PATCH 07/12] i2c: meson: improve interrupt handler and detect spurious interrupts

[Jerome Brunet]

On Wed, 2017-03-08 at 07:47 +0100, Heiner Kallweit wrote:
> 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 <hkallweit1@gmail.com>
> ---
> ?drivers/i2c/busses/i2c-meson.c | 16 +++++++++-------
> ?1 file changed, 9 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-meson.c b/drivers/i2c/busses/i2c-meson.c
> index 81304840..b3b881f9 100644
> --- a/drivers/i2c/busses/i2c-meson.c
> +++ b/drivers/i2c/busses/i2c-meson.c
> @@ -233,7 +233,15 @@ static irqreturn_t meson_i2c_irq(int irqno, void *dev_id)
> ?	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) {
> +	meson_i2c_set_mask(i2c, REG_CTRL, REG_CTRL_START, 0);
> +
> +	if (i2c->state == STATE_IDLE) {
> +		dev_notice(i2c->dev, "spurious interrupt detected\n");
> +		spin_unlock(&i2c->lock);
> +		return IRQ_NONE;
> +	}

Does it really happen ? Did you see any specific issue you'd like to share ?
If not, I'm not a big fan of this change.

In any case, if it can be handled gracefully, I'd drop the trace in the
interrupt handler.


> +
> +	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 +284,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 +349,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;

Re: [PATCH 00/12] i2c: meson: series with improvements

[Heiner Kallweit]

Am 08.03.2017 um 10:34 schrieb Jerome Brunet:
> On Wed, 2017-03-08 at 07:41 +0100, Heiner Kallweit wrote:
>> This patch series includes several improvements for the i2c-meson
>> driver.
> 
> Heiner, Could give us a some details about how you tested this series (platform,
> board, i2c devices, exemple of transfers, etc ...). The more details you give,
> the easiest it is to be confident about the patches.
> 
Sure. System is Odroid C2 and I test with a DS3231-based RTC module connected
to I2C-A (driver rtc-ds1307). Bus speed is set to 400 kHz.

That's the debug messages (with the patch series applied) from the interrupt
handler when booting the system and setting the system clock from RTC.

meson-i2c c1108500.i2c: irq: state 2, pos 0, count 1, ctrl 05869031
meson-i2c c1108500.i2c: irq: state 1, pos 0, count 2, ctrl 07869251
meson-i2c c1108500.i2c: irq: state 2, pos 0, count 2, ctrl 07869051
meson-i2c c1108500.i2c: irq: state 2, pos 0, count 1, ctrl 05869031
meson-i2c c1108500.i2c: irq: state 1, pos 0, count 8, ctrl 078698b1
meson-i2c c1108500.i2c: irq: state 2, pos 0, count 1, ctrl 05869031
meson-i2c c1108500.i2c: irq: state 1, pos 0, count 7, ctrl 078697a1
meson-i2c c1108500.i2c: irq: state 2, pos 0, count 1, ctrl 05869031
meson-i2c c1108500.i2c: irq: state 1, pos 0, count 7, ctrl 078697a1
meson-i2c c1108500.i2c: irq: state 2, pos 0, count 1, ctrl 05869031
meson-i2c c1108500.i2c: irq: state 1, pos 0, count 7, ctrl 078697a1
meson-i2c c1108500.i2c: irq: state 2, pos 0, count 1, ctrl 05869031
meson-i2c c1108500.i2c: irq: state 1, pos 0, count 1, ctrl 07869141


> Personally, I don't have test case representative enough to test this series
> thoroughly.
> 
>>
>> 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: explicitly ignore messages with length zero
>>   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
>>   i2c: meson: use state in meson_i2c_prepare_xfer
>>
>>  drivers/i2c/busses/i2c-meson.c | 157 ++++++++++++++++----------------------
>> ---
>>  1 file changed, 62 insertions(+), 95 deletions(-)
>>
> 

[PATCH 00/12] i2c: meson: series with improvements

[Heiner Kallweit]

Am 08.03.2017 um 10:34 schrieb Jerome Brunet:
> On Wed, 2017-03-08 at 07:41 +0100, Heiner Kallweit wrote:
>> This patch series includes several improvements for the i2c-meson
>> driver.
> 
> Heiner, Could give us a some details about how you tested this series (platform,
> board, i2c devices, exemple of transfers, etc ...). The more details you give,
> the easiest it is to be confident about the patches.
> 
Sure. System is Odroid C2 and I test with a DS3231-based RTC module connected
to I2C-A (driver rtc-ds1307). Bus speed is set to 400 kHz.

That's the debug messages (with the patch series applied) from the interrupt
handler when booting the system and setting the system clock from RTC.

meson-i2c c1108500.i2c: irq: state 2, pos 0, count 1, ctrl 05869031
meson-i2c c1108500.i2c: irq: state 1, pos 0, count 2, ctrl 07869251
meson-i2c c1108500.i2c: irq: state 2, pos 0, count 2, ctrl 07869051
meson-i2c c1108500.i2c: irq: state 2, pos 0, count 1, ctrl 05869031
meson-i2c c1108500.i2c: irq: state 1, pos 0, count 8, ctrl 078698b1
meson-i2c c1108500.i2c: irq: state 2, pos 0, count 1, ctrl 05869031
meson-i2c c1108500.i2c: irq: state 1, pos 0, count 7, ctrl 078697a1
meson-i2c c1108500.i2c: irq: state 2, pos 0, count 1, ctrl 05869031
meson-i2c c1108500.i2c: irq: state 1, pos 0, count 7, ctrl 078697a1
meson-i2c c1108500.i2c: irq: state 2, pos 0, count 1, ctrl 05869031
meson-i2c c1108500.i2c: irq: state 1, pos 0, count 7, ctrl 078697a1
meson-i2c c1108500.i2c: irq: state 2, pos 0, count 1, ctrl 05869031
meson-i2c c1108500.i2c: irq: state 1, pos 0, count 1, ctrl 07869141


> Personally, I don't have test case representative enough to test this series
> thoroughly.
> 
>>
>> 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: explicitly ignore messages with length zero
>>   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
>>   i2c: meson: use state in meson_i2c_prepare_xfer
>>
>>  drivers/i2c/busses/i2c-meson.c | 157 ++++++++++++++++----------------------
>> ---
>>  1 file changed, 62 insertions(+), 95 deletions(-)
>>
> 

Re: [PATCH 07/12] i2c: meson: improve interrupt handler and detect spurious interrupts

[Heiner Kallweit]

Am 08.03.2017 um 10:50 schrieb Jerome Brunet:
> On Wed, 2017-03-08 at 07:47 +0100, Heiner Kallweit wrote:
>> 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 <hkallweit1@gmail.com>
>> ---
>>  drivers/i2c/busses/i2c-meson.c | 16 +++++++++-------
>>  1 file changed, 9 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/i2c/busses/i2c-meson.c b/drivers/i2c/busses/i2c-meson.c
>> index 81304840..b3b881f9 100644
>> --- a/drivers/i2c/busses/i2c-meson.c
>> +++ b/drivers/i2c/busses/i2c-meson.c
>> @@ -233,7 +233,15 @@ static irqreturn_t meson_i2c_irq(int irqno, void *dev_id)
>>  	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) {
>> +	meson_i2c_set_mask(i2c, REG_CTRL, REG_CTRL_START, 0);
>> +
>> +	if (i2c->state == STATE_IDLE) {
>> +		dev_notice(i2c->dev, "spurious interrupt detected\n");
>> +		spin_unlock(&i2c->lock);
>> +		return IRQ_NONE;
>> +	}
> 
> Does it really happen ? Did you see any specific issue you'd like to share ?
> If not, I'm not a big fan of this change.
> 
No,it did not happen. It's just that in few parts of the interrupt handler
we deal with this situation that should never happen.
And if it would happen on some system, we wouldn't know because it's
silently ignored.

I'm fine with dropping the error message. Still I would prefer to at least
return IRQ_NONE instead of IRQ_HANDLED in this potential error case.

It's like other drivers checking in the interrupt handler whether any
interrupt source is enabled and returning IRQ_NONE if not.

> In any case, if it can be handled gracefully, I'd drop the trace in the
> interrupt handler.
> 
> 
>> +
>> +	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 +284,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 +349,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;
> 

[PATCH 07/12] i2c: meson: improve interrupt handler and detect spurious interrupts

[Heiner Kallweit]

Am 08.03.2017 um 10:50 schrieb Jerome Brunet:
> On Wed, 2017-03-08 at 07:47 +0100, Heiner Kallweit wrote:
>> 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 <hkallweit1@gmail.com>
>> ---
>>  drivers/i2c/busses/i2c-meson.c | 16 +++++++++-------
>>  1 file changed, 9 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/i2c/busses/i2c-meson.c b/drivers/i2c/busses/i2c-meson.c
>> index 81304840..b3b881f9 100644
>> --- a/drivers/i2c/busses/i2c-meson.c
>> +++ b/drivers/i2c/busses/i2c-meson.c
>> @@ -233,7 +233,15 @@ static irqreturn_t meson_i2c_irq(int irqno, void *dev_id)
>>  	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) {
>> +	meson_i2c_set_mask(i2c, REG_CTRL, REG_CTRL_START, 0);
>> +
>> +	if (i2c->state == STATE_IDLE) {
>> +		dev_notice(i2c->dev, "spurious interrupt detected\n");
>> +		spin_unlock(&i2c->lock);
>> +		return IRQ_NONE;
>> +	}
> 
> Does it really happen ? Did you see any specific issue you'd like to share ?
> If not, I'm not a big fan of this change.
> 
No,it did not happen. It's just that in few parts of the interrupt handler
we deal with this situation that should never happen.
And if it would happen on some system, we wouldn't know because it's
silently ignored.

I'm fine with dropping the error message. Still I would prefer to at least
return IRQ_NONE instead of IRQ_HANDLED in this potential error case.

It's like other drivers checking in the interrupt handler whether any
interrupt source is enabled and returning IRQ_NONE if not.

> In any case, if it can be handled gracefully, I'd drop the trace in the
> interrupt handler.
> 
> 
>> +
>> +	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 +284,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 +349,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;
> 

Re: [PATCH 08/12] i2c: meson: explicitly ignore messages with length zero

[Heiner Kallweit]

Am 08.03.2017 um 08:53 schrieb Ben Dooks:
> Probably a bad idea. Zero data could just be an address is there check
> 
Thanks for pointing out.

> On 8 Mar 2017 06:51, "Heiner Kallweit" <hkallweit1@gmail.com <mailto:hkallweit1@gmail.com>> wrote:
> 
>     Explicitely ignore messages with length zero. This also allows to
>     remove some now unneeded checks during message processing.
> 
>     Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com <mailto:hkallweit1@gmail.com>>
>     ---
>      drivers/i2c/busses/i2c-meson.c | 20 +++++++++-----------
>      1 file changed, 9 insertions(+), 11 deletions(-)
> 
>     diff --git a/drivers/i2c/busses/i2c-meson.c b/drivers/i2c/busses/i2c-meson.c
>     index b3b881f9..58414699 100644
>     --- a/drivers/i2c/busses/i2c-meson.c
>     +++ b/drivers/i2c/busses/i2c-meson.c
>     @@ -196,12 +196,10 @@ static void meson_i2c_prepare_xfer(struct meson_i2c *i2c)
>             for (i = 0; i < i2c->count - 1; i++)
>                     meson_i2c_add_token(i2c, TOKEN_DATA);
> 
>     -       if (i2c->count) {
>     -               if (write || i2c->pos + i2c->count < i2c->msg->len)
>     -                       meson_i2c_add_token(i2c, TOKEN_DATA);
>     -               else
>     -                       meson_i2c_add_token(i2c, TOKEN_DATA_LAST);
>     -       }
>     +       if (write || i2c->pos + i2c->count < i2c->msg->len)
>     +               meson_i2c_add_token(i2c, TOKEN_DATA);
>     +       else
>     +               meson_i2c_add_token(i2c, TOKEN_DATA_LAST);
> 
>             if (write)
>                     meson_i2c_put_data(i2c, i2c->msg->buf + i2c->pos, i2c->count);
>     @@ -257,11 +255,8 @@ static irqreturn_t meson_i2c_irq(int irqno, void *dev_id)
> 
>             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;
>     -               }
>     +               meson_i2c_get_data(i2c, i2c->msg->buf + i2c->pos, i2c->count);
>     +               i2c->pos += i2c->count;
> 
>                     if (i2c->pos >= i2c->msg->len) {
>                             meson_i2c_stop(i2c);
>     @@ -371,6 +366,9 @@ static int meson_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs,
>             clk_enable(i2c->clk);
> 
>             for (i = 0; i < num; i++) {
>     +               /* ignore messages with length 0 */
>     +               if (!msgs[i].len)
>     +                       continue;
>                     ret = meson_i2c_xfer_msg(i2c, &msgs[i], i == num - 1);
>                     if (ret)
>                             break;
>     --
>     2.12.0
> 
> 
> 
>     _______________________________________________
>     linux-amlogic mailing list
>     linux-amlogic@lists.infradead.org <mailto:linux-amlogic@lists.infradead.org>
>     http://lists.infradead.org/mailman/listinfo/linux-amlogic <http://lists.infradead.org/mailman/listinfo/linux-amlogic>
> 

[PATCH 08/12] i2c: meson: explicitly ignore messages with length zero

[Heiner Kallweit]

Am 08.03.2017 um 08:53 schrieb Ben Dooks:
> Probably a bad idea. Zero data could just be an address is there check
> 
Thanks for pointing out.

> On 8 Mar 2017 06:51, "Heiner Kallweit" <hkallweit1 at gmail.com <mailto:hkallweit1@gmail.com>> wrote:
> 
>     Explicitely ignore messages with length zero. This also allows to
>     remove some now unneeded checks during message processing.
> 
>     Signed-off-by: Heiner Kallweit <hkallweit1 at gmail.com <mailto:hkallweit1@gmail.com>>
>     ---
>      drivers/i2c/busses/i2c-meson.c | 20 +++++++++-----------
>      1 file changed, 9 insertions(+), 11 deletions(-)
> 
>     diff --git a/drivers/i2c/busses/i2c-meson.c b/drivers/i2c/busses/i2c-meson.c
>     index b3b881f9..58414699 100644
>     --- a/drivers/i2c/busses/i2c-meson.c
>     +++ b/drivers/i2c/busses/i2c-meson.c
>     @@ -196,12 +196,10 @@ static void meson_i2c_prepare_xfer(struct meson_i2c *i2c)
>             for (i = 0; i < i2c->count - 1; i++)
>                     meson_i2c_add_token(i2c, TOKEN_DATA);
> 
>     -       if (i2c->count) {
>     -               if (write || i2c->pos + i2c->count < i2c->msg->len)
>     -                       meson_i2c_add_token(i2c, TOKEN_DATA);
>     -               else
>     -                       meson_i2c_add_token(i2c, TOKEN_DATA_LAST);
>     -       }
>     +       if (write || i2c->pos + i2c->count < i2c->msg->len)
>     +               meson_i2c_add_token(i2c, TOKEN_DATA);
>     +       else
>     +               meson_i2c_add_token(i2c, TOKEN_DATA_LAST);
> 
>             if (write)
>                     meson_i2c_put_data(i2c, i2c->msg->buf + i2c->pos, i2c->count);
>     @@ -257,11 +255,8 @@ static irqreturn_t meson_i2c_irq(int irqno, void *dev_id)
> 
>             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;
>     -               }
>     +               meson_i2c_get_data(i2c, i2c->msg->buf + i2c->pos, i2c->count);
>     +               i2c->pos += i2c->count;
> 
>                     if (i2c->pos >= i2c->msg->len) {
>                             meson_i2c_stop(i2c);
>     @@ -371,6 +366,9 @@ static int meson_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs,
>             clk_enable(i2c->clk);
> 
>             for (i = 0; i < num; i++) {
>     +               /* ignore messages with length 0 */
>     +               if (!msgs[i].len)
>     +                       continue;
>                     ret = meson_i2c_xfer_msg(i2c, &msgs[i], i == num - 1);
>                     if (ret)
>                             break;
>     --
>     2.12.0
> 
> 
> 
>     _______________________________________________
>     linux-amlogic mailing list
>     linux-amlogic at lists.infradead.org <mailto:linux-amlogic@lists.infradead.org>
>     http://lists.infradead.org/mailman/listinfo/linux-amlogic <http://lists.infradead.org/mailman/listinfo/linux-amlogic>
> 

Re: [PATCH 07/12] i2c: meson: improve interrupt handler and detect spurious interrupts

[Heiner Kallweit]

Am 08.03.2017 um 22:02 schrieb Heiner Kallweit:
> Am 08.03.2017 um 10:50 schrieb Jerome Brunet:
>> On Wed, 2017-03-08 at 07:47 +0100, Heiner Kallweit wrote:
>>> 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 <hkallweit1@gmail.com>
>>> ---
>>>  drivers/i2c/busses/i2c-meson.c | 16 +++++++++-------
>>>  1 file changed, 9 insertions(+), 7 deletions(-)
>>>
>>> diff --git a/drivers/i2c/busses/i2c-meson.c b/drivers/i2c/busses/i2c-meson.c
>>> index 81304840..b3b881f9 100644
>>> --- a/drivers/i2c/busses/i2c-meson.c
>>> +++ b/drivers/i2c/busses/i2c-meson.c
>>> @@ -233,7 +233,15 @@ static irqreturn_t meson_i2c_irq(int irqno, void *dev_id)
>>>  	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) {
>>> +	meson_i2c_set_mask(i2c, REG_CTRL, REG_CTRL_START, 0);
>>> +
>>> +	if (i2c->state == STATE_IDLE) {
>>> +		dev_notice(i2c->dev, "spurious interrupt detected\n");
>>> +		spin_unlock(&i2c->lock);
>>> +		return IRQ_NONE;
>>> +	}
>>
>> Does it really happen ? Did you see any specific issue you'd like to share ?
>> If not, I'm not a big fan of this change.
>>
> No,it did not happen. It's just that in few parts of the interrupt handler
> we deal with this situation that should never happen.
> And if it would happen on some system, we wouldn't know because it's
> silently ignored.
> 
> I'm fine with dropping the error message. Still I would prefer to at least
> return IRQ_NONE instead of IRQ_HANDLED in this potential error case.
> 
> It's like other drivers checking in the interrupt handler whether any
> interrupt source is enabled and returning IRQ_NONE if not.
> 
>> In any case, if it can be handled gracefully, I'd drop the trace in the
>> interrupt handler.
>>
>>
>>> +
>>> +	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 +284,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 +349,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);
>>> -
Just recognized that we need to keep this statement to properly handle
the timeout case. So I'll make this part of a v3 together with potential
changes based on further review feedback.

>>>  	if (!time_left) {
>>>  		i2c->state = STATE_IDLE;
>>>  		ret = -ETIMEDOUT;
>>
> 

[PATCH 07/12] i2c: meson: improve interrupt handler and detect spurious interrupts

[Heiner Kallweit]

Am 08.03.2017 um 22:02 schrieb Heiner Kallweit:
> Am 08.03.2017 um 10:50 schrieb Jerome Brunet:
>> On Wed, 2017-03-08 at 07:47 +0100, Heiner Kallweit wrote:
>>> 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 <hkallweit1@gmail.com>
>>> ---
>>>  drivers/i2c/busses/i2c-meson.c | 16 +++++++++-------
>>>  1 file changed, 9 insertions(+), 7 deletions(-)
>>>
>>> diff --git a/drivers/i2c/busses/i2c-meson.c b/drivers/i2c/busses/i2c-meson.c
>>> index 81304840..b3b881f9 100644
>>> --- a/drivers/i2c/busses/i2c-meson.c
>>> +++ b/drivers/i2c/busses/i2c-meson.c
>>> @@ -233,7 +233,15 @@ static irqreturn_t meson_i2c_irq(int irqno, void *dev_id)
>>>  	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) {
>>> +	meson_i2c_set_mask(i2c, REG_CTRL, REG_CTRL_START, 0);
>>> +
>>> +	if (i2c->state == STATE_IDLE) {
>>> +		dev_notice(i2c->dev, "spurious interrupt detected\n");
>>> +		spin_unlock(&i2c->lock);
>>> +		return IRQ_NONE;
>>> +	}
>>
>> Does it really happen ? Did you see any specific issue you'd like to share ?
>> If not, I'm not a big fan of this change.
>>
> No,it did not happen. It's just that in few parts of the interrupt handler
> we deal with this situation that should never happen.
> And if it would happen on some system, we wouldn't know because it's
> silently ignored.
> 
> I'm fine with dropping the error message. Still I would prefer to at least
> return IRQ_NONE instead of IRQ_HANDLED in this potential error case.
> 
> It's like other drivers checking in the interrupt handler whether any
> interrupt source is enabled and returning IRQ_NONE if not.
> 
>> In any case, if it can be handled gracefully, I'd drop the trace in the
>> interrupt handler.
>>
>>
>>> +
>>> +	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 +284,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 +349,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);
>>> -
Just recognized that we need to keep this statement to properly handle
the timeout case. So I'll make this part of a v3 together with potential
changes based on further review feedback.

>>>  	if (!time_left) {
>>>  		i2c->state = STATE_IDLE;
>>>  		ret = -ETIMEDOUT;
>>
>