* [PATCH 01/12] i2c: meson: use min instead of min_t where min_t isn't needed
2017-03-08 6:41 [PATCH 00/12] i2c: meson: series with improvements Heiner Kallweit
@ 2017-03-08 6:42 ` Heiner Kallweit
2017-03-08 9:25 ` Jerome Brunet
2017-03-08 6:43 ` [PATCH 02/12] i2c: meson: remove member irq from struct meson_i2c Heiner Kallweit
` (11 subsequent siblings)
12 siblings, 1 reply; 24+ messages in thread
From: Heiner Kallweit @ 2017-03-08 6:42 UTC (permalink / raw)
To: Wolfram Sang; +Cc: linux-i2c@vger.kernel.org, linux-amlogic
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
^ permalink raw reply related [flat|nested] 24+ messages in thread* Re: [PATCH 01/12] i2c: meson: use min instead of min_t where min_t isn't needed
2017-03-08 6:42 ` [PATCH 01/12] i2c: meson: use min instead of min_t where min_t isn't needed Heiner Kallweit
@ 2017-03-08 9:25 ` Jerome Brunet
0 siblings, 0 replies; 24+ messages in thread
From: Jerome Brunet @ 2017-03-08 9:25 UTC (permalink / raw)
To: Heiner Kallweit, Wolfram Sang; +Cc: linux-amlogic, linux-i2c@vger.kernel.org
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
^ permalink raw reply [flat|nested] 24+ messages in thread
* [PATCH 02/12] i2c: meson: remove member irq from struct meson_i2c
2017-03-08 6:41 [PATCH 00/12] i2c: meson: series with improvements Heiner Kallweit
2017-03-08 6:42 ` [PATCH 01/12] i2c: meson: use min instead of min_t where min_t isn't needed Heiner Kallweit
@ 2017-03-08 6:43 ` Heiner Kallweit
2017-03-08 9:07 ` Jerome Brunet
2017-03-08 6:44 ` [PATCH 03/12] i2c: meson: use i2c core for DT clock-frequency parsing Heiner Kallweit
` (10 subsequent siblings)
12 siblings, 1 reply; 24+ messages in thread
From: Heiner Kallweit @ 2017-03-08 6:43 UTC (permalink / raw)
To: Wolfram Sang; +Cc: linux-i2c@vger.kernel.org, linux-amlogic
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
^ permalink raw reply related [flat|nested] 24+ messages in thread* Re: [PATCH 02/12] i2c: meson: remove member irq from struct meson_i2c
2017-03-08 6:43 ` [PATCH 02/12] i2c: meson: remove member irq from struct meson_i2c Heiner Kallweit
@ 2017-03-08 9:07 ` Jerome Brunet
0 siblings, 0 replies; 24+ messages in thread
From: Jerome Brunet @ 2017-03-08 9:07 UTC (permalink / raw)
To: Heiner Kallweit, Wolfram Sang; +Cc: linux-amlogic, linux-i2c@vger.kernel.org
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>
^ permalink raw reply [flat|nested] 24+ messages in thread
* [PATCH 03/12] i2c: meson: use i2c core for DT clock-frequency parsing
2017-03-08 6:41 [PATCH 00/12] i2c: meson: series with improvements Heiner Kallweit
2017-03-08 6:42 ` [PATCH 01/12] i2c: meson: use min instead of min_t where min_t isn't needed Heiner Kallweit
2017-03-08 6:43 ` [PATCH 02/12] i2c: meson: remove member irq from struct meson_i2c Heiner Kallweit
@ 2017-03-08 6:44 ` Heiner Kallweit
2017-03-08 9:10 ` Jerome Brunet
2017-03-08 6:44 ` [PATCH 04/12] i2c: meson: use full 12 bits for clock divider Heiner Kallweit
` (9 subsequent siblings)
12 siblings, 1 reply; 24+ messages in thread
From: Heiner Kallweit @ 2017-03-08 6:44 UTC (permalink / raw)
To: Wolfram Sang; +Cc: linux-i2c@vger.kernel.org, linux-amlogic
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
^ permalink raw reply related [flat|nested] 24+ messages in thread* Re: [PATCH 03/12] i2c: meson: use i2c core for DT clock-frequency parsing
2017-03-08 6:44 ` [PATCH 03/12] i2c: meson: use i2c core for DT clock-frequency parsing Heiner Kallweit
@ 2017-03-08 9:10 ` Jerome Brunet
0 siblings, 0 replies; 24+ messages in thread
From: Jerome Brunet @ 2017-03-08 9:10 UTC (permalink / raw)
To: Heiner Kallweit, Wolfram Sang; +Cc: linux-amlogic, linux-i2c@vger.kernel.org
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>
^ permalink raw reply [flat|nested] 24+ messages in thread
* [PATCH 04/12] i2c: meson: use full 12 bits for clock divider
2017-03-08 6:41 [PATCH 00/12] i2c: meson: series with improvements Heiner Kallweit
` (2 preceding siblings ...)
2017-03-08 6:44 ` [PATCH 03/12] i2c: meson: use i2c core for DT clock-frequency parsing Heiner Kallweit
@ 2017-03-08 6:44 ` Heiner Kallweit
2017-03-08 9:19 ` Jerome Brunet
2017-03-08 6:45 ` [PATCH 05/12] i2c: meson: set clock divider in probe instead of setting it for each transfer Heiner Kallweit
` (8 subsequent siblings)
12 siblings, 1 reply; 24+ messages in thread
From: Heiner Kallweit @ 2017-03-08 6:44 UTC (permalink / raw)
To: Wolfram Sang; +Cc: linux-i2c@vger.kernel.org, linux-amlogic
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
^ permalink raw reply related [flat|nested] 24+ messages in thread* Re: [PATCH 04/12] i2c: meson: use full 12 bits for clock divider
2017-03-08 6:44 ` [PATCH 04/12] i2c: meson: use full 12 bits for clock divider Heiner Kallweit
@ 2017-03-08 9:19 ` Jerome Brunet
0 siblings, 0 replies; 24+ messages in thread
From: Jerome Brunet @ 2017-03-08 9:19 UTC (permalink / raw)
To: Heiner Kallweit, Wolfram Sang; +Cc: linux-amlogic, linux-i2c@vger.kernel.org
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>
^ permalink raw reply [flat|nested] 24+ messages in thread
* [PATCH 05/12] i2c: meson: set clock divider in probe instead of setting it for each transfer
2017-03-08 6:41 [PATCH 00/12] i2c: meson: series with improvements Heiner Kallweit
` (3 preceding siblings ...)
2017-03-08 6:44 ` [PATCH 04/12] i2c: meson: use full 12 bits for clock divider Heiner Kallweit
@ 2017-03-08 6:45 ` Heiner Kallweit
2017-03-08 6:46 ` [PATCH 06/12] i2c: meson: remove variable count from meson_i2c_xfer Heiner Kallweit
` (7 subsequent siblings)
12 siblings, 0 replies; 24+ messages in thread
From: Heiner Kallweit @ 2017-03-08 6:45 UTC (permalink / raw)
To: Wolfram Sang; +Cc: linux-i2c@vger.kernel.org, linux-amlogic
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
^ permalink raw reply related [flat|nested] 24+ messages in thread* [PATCH 06/12] i2c: meson: remove variable count from meson_i2c_xfer
2017-03-08 6:41 [PATCH 00/12] i2c: meson: series with improvements Heiner Kallweit
` (4 preceding siblings ...)
2017-03-08 6:45 ` [PATCH 05/12] i2c: meson: set clock divider in probe instead of setting it for each transfer Heiner Kallweit
@ 2017-03-08 6:46 ` Heiner Kallweit
2017-03-08 9:28 ` Jerome Brunet
2017-03-08 6:47 ` [PATCH 07/12] i2c: meson: improve interrupt handler and detect spurious interrupts Heiner Kallweit
` (6 subsequent siblings)
12 siblings, 1 reply; 24+ messages in thread
From: Heiner Kallweit @ 2017-03-08 6:46 UTC (permalink / raw)
To: Wolfram Sang; +Cc: linux-i2c@vger.kernel.org, linux-amlogic
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
^ permalink raw reply related [flat|nested] 24+ messages in thread* Re: [PATCH 06/12] i2c: meson: remove variable count from meson_i2c_xfer
2017-03-08 6:46 ` [PATCH 06/12] i2c: meson: remove variable count from meson_i2c_xfer Heiner Kallweit
@ 2017-03-08 9:28 ` Jerome Brunet
0 siblings, 0 replies; 24+ messages in thread
From: Jerome Brunet @ 2017-03-08 9:28 UTC (permalink / raw)
To: Heiner Kallweit, Wolfram Sang; +Cc: linux-amlogic, linux-i2c@vger.kernel.org
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)
^ permalink raw reply [flat|nested] 24+ messages in thread
* [PATCH 07/12] i2c: meson: improve interrupt handler and detect spurious interrupts
2017-03-08 6:41 [PATCH 00/12] i2c: meson: series with improvements Heiner Kallweit
` (5 preceding siblings ...)
2017-03-08 6:46 ` [PATCH 06/12] i2c: meson: remove variable count from meson_i2c_xfer Heiner Kallweit
@ 2017-03-08 6:47 ` Heiner Kallweit
2017-03-08 9:50 ` Jerome Brunet
2017-03-08 6:47 ` [PATCH 08/12] i2c: meson: explicitly ignore messages with length zero Heiner Kallweit
` (5 subsequent siblings)
12 siblings, 1 reply; 24+ messages in thread
From: Heiner Kallweit @ 2017-03-08 6:47 UTC (permalink / raw)
To: Wolfram Sang; +Cc: linux-i2c@vger.kernel.org, linux-amlogic
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
^ permalink raw reply related [flat|nested] 24+ messages in thread* Re: [PATCH 07/12] i2c: meson: improve interrupt handler and detect spurious interrupts
2017-03-08 6:47 ` [PATCH 07/12] i2c: meson: improve interrupt handler and detect spurious interrupts Heiner Kallweit
@ 2017-03-08 9:50 ` Jerome Brunet
2017-03-08 21:02 ` Heiner Kallweit
0 siblings, 1 reply; 24+ messages in thread
From: Jerome Brunet @ 2017-03-08 9:50 UTC (permalink / raw)
To: Heiner Kallweit, Wolfram Sang; +Cc: linux-amlogic, linux-i2c@vger.kernel.org
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;
^ permalink raw reply [flat|nested] 24+ messages in thread* Re: [PATCH 07/12] i2c: meson: improve interrupt handler and detect spurious interrupts
2017-03-08 9:50 ` Jerome Brunet
@ 2017-03-08 21:02 ` Heiner Kallweit
2017-03-09 20:16 ` Heiner Kallweit
0 siblings, 1 reply; 24+ messages in thread
From: Heiner Kallweit @ 2017-03-08 21:02 UTC (permalink / raw)
To: Jerome Brunet, Wolfram Sang; +Cc: linux-amlogic, linux-i2c@vger.kernel.org
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;
>
^ permalink raw reply [flat|nested] 24+ messages in thread* Re: [PATCH 07/12] i2c: meson: improve interrupt handler and detect spurious interrupts
2017-03-08 21:02 ` Heiner Kallweit
@ 2017-03-09 20:16 ` Heiner Kallweit
0 siblings, 0 replies; 24+ messages in thread
From: Heiner Kallweit @ 2017-03-09 20:16 UTC (permalink / raw)
To: Jerome Brunet, Wolfram Sang; +Cc: linux-amlogic, linux-i2c@vger.kernel.org
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;
>>
>
^ permalink raw reply [flat|nested] 24+ messages in thread
* [PATCH 08/12] i2c: meson: explicitly ignore messages with length zero
2017-03-08 6:41 [PATCH 00/12] i2c: meson: series with improvements Heiner Kallweit
` (6 preceding siblings ...)
2017-03-08 6:47 ` [PATCH 07/12] i2c: meson: improve interrupt handler and detect spurious interrupts Heiner Kallweit
@ 2017-03-08 6:47 ` Heiner Kallweit
[not found] ` <CANe6Qb9nKdLnOcwEwuk8NXEB+fxW2S_kkzP+U+qygYhNqz0Y0w@mail.gmail.com>
2017-03-08 6:48 ` [PATCH 09/12] i2c: meson: don't create separate token chain just for the stop command Heiner Kallweit
` (4 subsequent siblings)
12 siblings, 1 reply; 24+ messages in thread
From: Heiner Kallweit @ 2017-03-08 6:47 UTC (permalink / raw)
To: Wolfram Sang; +Cc: linux-i2c@vger.kernel.org, linux-amlogic
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
^ permalink raw reply related [flat|nested] 24+ messages in thread* [PATCH 09/12] i2c: meson: don't create separate token chain just for the stop command
2017-03-08 6:41 [PATCH 00/12] i2c: meson: series with improvements Heiner Kallweit
` (7 preceding siblings ...)
2017-03-08 6:47 ` [PATCH 08/12] i2c: meson: explicitly ignore messages with length zero Heiner Kallweit
@ 2017-03-08 6:48 ` Heiner Kallweit
2017-03-08 6:49 ` [PATCH 10/12] i2c: meson: remove meson_i2c_write_tokens Heiner Kallweit
` (3 subsequent siblings)
12 siblings, 0 replies; 24+ messages in thread
From: Heiner Kallweit @ 2017-03-08 6:48 UTC (permalink / raw)
To: Wolfram Sang; +Cc: linux-i2c@vger.kernel.org, linux-amlogic
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
^ permalink raw reply related [flat|nested] 24+ messages in thread* [PATCH 10/12] i2c: meson: remove meson_i2c_write_tokens
2017-03-08 6:41 [PATCH 00/12] i2c: meson: series with improvements Heiner Kallweit
` (8 preceding siblings ...)
2017-03-08 6:48 ` [PATCH 09/12] i2c: meson: don't create separate token chain just for the stop command Heiner Kallweit
@ 2017-03-08 6:49 ` Heiner Kallweit
2017-03-08 6:49 ` [PATCH 11/12] i2c: meson: improve and simplify interrupt handler Heiner Kallweit
` (2 subsequent siblings)
12 siblings, 0 replies; 24+ messages in thread
From: Heiner Kallweit @ 2017-03-08 6:49 UTC (permalink / raw)
To: Wolfram Sang; +Cc: linux-i2c@vger.kernel.org, linux-amlogic
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
^ permalink raw reply related [flat|nested] 24+ messages in thread* [PATCH 11/12] i2c: meson: improve and simplify interrupt handler
2017-03-08 6:41 [PATCH 00/12] i2c: meson: series with improvements Heiner Kallweit
` (9 preceding siblings ...)
2017-03-08 6:49 ` [PATCH 10/12] i2c: meson: remove meson_i2c_write_tokens Heiner Kallweit
@ 2017-03-08 6:49 ` Heiner Kallweit
2017-03-08 6:50 ` [PATCH 12/12] i2c: meson: use state in meson_i2c_prepare_xfer Heiner Kallweit
2017-03-08 9:34 ` [PATCH 00/12] i2c: meson: series with improvements Jerome Brunet
12 siblings, 0 replies; 24+ messages in thread
From: Heiner Kallweit @ 2017-03-08 6:49 UTC (permalink / raw)
To: Wolfram Sang; +Cc: linux-i2c@vger.kernel.org, linux-amlogic
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
^ permalink raw reply related [flat|nested] 24+ messages in thread* [PATCH 12/12] i2c: meson: use state in meson_i2c_prepare_xfer
2017-03-08 6:41 [PATCH 00/12] i2c: meson: series with improvements Heiner Kallweit
` (10 preceding siblings ...)
2017-03-08 6:49 ` [PATCH 11/12] i2c: meson: improve and simplify interrupt handler Heiner Kallweit
@ 2017-03-08 6:50 ` Heiner Kallweit
2017-03-08 9:34 ` [PATCH 00/12] i2c: meson: series with improvements Jerome Brunet
12 siblings, 0 replies; 24+ messages in thread
From: Heiner Kallweit @ 2017-03-08 6:50 UTC (permalink / raw)
To: Wolfram Sang; +Cc: linux-i2c@vger.kernel.org, linux-amlogic
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
^ permalink raw reply related [flat|nested] 24+ messages in thread* Re: [PATCH 00/12] i2c: meson: series with improvements
2017-03-08 6:41 [PATCH 00/12] i2c: meson: series with improvements Heiner Kallweit
` (11 preceding siblings ...)
2017-03-08 6:50 ` [PATCH 12/12] i2c: meson: use state in meson_i2c_prepare_xfer Heiner Kallweit
@ 2017-03-08 9:34 ` Jerome Brunet
2017-03-08 20:49 ` Heiner Kallweit
12 siblings, 1 reply; 24+ messages in thread
From: Jerome Brunet @ 2017-03-08 9:34 UTC (permalink / raw)
To: Heiner Kallweit, Wolfram Sang; +Cc: linux-amlogic, linux-i2c@vger.kernel.org
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(-)
>
^ permalink raw reply [flat|nested] 24+ messages in thread* Re: [PATCH 00/12] i2c: meson: series with improvements
2017-03-08 9:34 ` [PATCH 00/12] i2c: meson: series with improvements Jerome Brunet
@ 2017-03-08 20:49 ` Heiner Kallweit
0 siblings, 0 replies; 24+ messages in thread
From: Heiner Kallweit @ 2017-03-08 20:49 UTC (permalink / raw)
To: Jerome Brunet, Wolfram Sang; +Cc: linux-amlogic, linux-i2c@vger.kernel.org
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(-)
>>
>
^ permalink raw reply [flat|nested] 24+ messages in thread