i2c: meson: series with improvements
Subsystems (2)
cover i2c: meson: series with improvements
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(-)
Signature · 1 line
2.12.0
PATCH 1/12 i2c: meson: use min instead of min_t where min_t isn't needed
Use min instead of min_t where min_t isn't needed.
--- drivers/i2c/busses/i2c-meson.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-)
drivers/i2c/busses/i2c-meson.c +5 -5
@@ -156,10 +156,10 @@ static void meson_i2c_get_data(struct meson_i2c *i2c, char *buf, int len)dev_dbg (i2c->dev,"%s: data %08x %08x len %d\n" , __func__, rdata0, rdata1, len); -for (i =0 ; i <min_t (int,4 , len); i++) +for (i =0 ; i <min (4 , len); i++) *buf++ = (rdata0 >> i *8 ) &0xff ; -for (i =4 ; i <min_t (int,8 , len); i++) +for (i =4 ; i <min (8 , len); i++) *buf++ = (rdata1 >> (i -4 ) *8 ) &0xff ; }
@@ -168,10 +168,10 @@ static void meson_i2c_put_data(struct meson_i2c *i2c, char *buf, int len)u32 wdata0 =0 , wdata1 =0 ;int i; -for (i =0 ; i <min_t (int,4 , len); i++) +for (i =0 ; i <min (4 , len); i++) wdata0 |= *buf++ << (i *8 ); -for (i =4 ; i <min_t (int,8 , len); i++) +for (i =4 ; i <min (8 , len); i++) wdata1 |= *buf++ << ((i -4 ) *8 );writel (wdata0, i2c->regs +REG_TOK_WDATA0 );
@@ -186,7 +186,7 @@ static void meson_i2c_prepare_xfer(struct meson_i2c *i2c)bool write = !(i2c->msg->flags &I2C_M_RD );int i; - i2c->count =min_t (int, i2c->msg->len - i2c->pos,8 ); + i2c->count =min (i2c->msg->len - i2c->pos,8 );for (i =0 ; i < i2c->count -1 ; i++)meson_i2c_add_token (i2c,TOKEN_DATA );
Signature · 1 line
2.12.0
Jerome Brunet
reviewerPATCH 1/12 i2c: meson: use min instead of min_t where min_t isn't needed
Quoted 47 lines · Heiner Kallweit
Use min instead of
min_twheremin_tisn't needed.Signed-off-by Heiner Kallweit <[email protected]>---
?drivers/i2c/busses/i2c-meson.c | 10 +++++-----
?1 file changed, 5 insertions(+), 5 deletions(-)drivers/i2c/busses/i2c- renamed from drivers/i2c/busses/i2c-meson.c +0
-0
meson.c
index73b97c71..40e5da9a100644drivers/i2c/busses/i2c-meson.c +0
-0@@ -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);?
drivers/i2c/busses/i2c-meson.c +1
-1-for (i =0 ; i <min_t (int,4 , len); i++) +for (i =0 ; i <min (4 , len); i++)? *buf++ = (rdata0 >> i * 8) &
0xff;
?drivers/i2c/busses/i2c-meson.c +1
-1-for (i =4 ; i <min_t (int,8 , len); i++) +for (i =4 ; i <min (8 , len); i++)? *buf++ = (rdata1 >> (i - 4) * 8) &
0xff;
?}
?drivers/i2c/busses/i2c-meson.c +0
-0@@ -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;?
drivers/i2c/busses/i2c-meson.c +1
-1-for (i =0 ; i <min_t (int,4 , len); i++) +for (i =0 ; i <min (4 , len); i++)? wdata0 |= *buf++ << (i * 8);
?drivers/i2c/busses/i2c-meson.c +1
-1-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);drivers/i2c/busses/i2c-meson.c +0
-0@@ -186,7 +186,7 @@ static void meson_i2c_prepare_xfer(struct meson_i2c *i2c) ? bool write = !(i2c->msg->flags & I2C_M_RD); ? int i;?
drivers/i2c/busses/i2c-meson.c +1
-1- i2c->count =min_t (int, i2c->msg->len - i2c->pos,8 ); + i2c->count =min (i2c->msg->len - i2c->pos,8 );
Quoted 3 lines
?
? 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 2/12 i2c: meson: remove member irq from struct meson_i2c
In reply to Heiner KallweitMember irq can be replaced with a local variable in probe because it's nowhere else accessed.
--- drivers/i2c/busses/i2c-meson.c | 13 ++++++------- 1 file changed, 6 insertions(+), 7 deletions(-)
drivers/i2c/busses/i2c-meson.c +6 -7
@@ -82,7 +82,6 @@ struct meson_i2c {struct device *dev;void __iomem *regs;struct clk *clk; -int irq;struct i2c_msg *msg;int state;
@@ -391,7 +390,7 @@ static int meson_i2c_probe(struct platform_device *pdev)struct device_node *np = pdev->dev.of_node;struct meson_i2c *i2c;struct resource *mem; -int ret =0 ; +int irq, ret =0 ; i2c =devm_kzalloc (&pdev->dev,sizeof (struct meson_i2c ),GFP_KERNEL );if (!i2c)
@@ -418,14 +417,14 @@ static int meson_i2c_probe(struct platform_device *pdev)if (IS_ERR (i2c->regs))return PTR_ERR (i2c->regs); - i2c->irq =platform_get_irq (pdev,0 ); -if (i2c->irq <0 ) { + irq =platform_get_irq (pdev,0 ); +if (irq <0 ) {dev_err (&pdev->dev,"can't find IRQ\n" ); -return i2c->irq; +return irq; } - ret =devm_request_irq (&pdev->dev, i2c->irq, meson_i2c_irq, -0 ,dev_name (&pdev->dev), i2c); + ret =devm_request_irq (&pdev->dev, irq, meson_i2c_irq,0 , +dev_name (&pdev->dev), i2c);if (ret <0 ) {dev_err (&pdev->dev,"can't request IRQ\n" );return ret;
Signature · 1 line
2.12.0
Jerome Brunet
reviewerPATCH 2/12 i2c: meson: remove member irq from struct meson_i2c
Quoted 50 lines · Heiner Kallweit
Member irq can be replaced with a local variable in probe because it's nowhere else accessed.
Signed-off-by Heiner Kallweit <[email protected]>---
?drivers/i2c/busses/i2c-meson.c | 13 ++++++------- ?1 file changed, 6 insertions(+), 7 deletions(-)drivers/i2c/busses/i2c- renamed from drivers/i2c/busses/i2c-meson.c +0
-0
meson.c
index40e5da9a..50059d09100644drivers/i2c/busses/i2c-meson.c +0
-1@@ -82,7 +82,6 @@ struct meson_i2c { ? struct device *dev; ? void __iomem *regs; ? struct clk *clk; -int irq;?
? structi2c_msg*msg;
? int state;drivers/i2c/busses/i2c-meson.c +1
-1@@ -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)drivers/i2c/busses/i2c-meson.c +0
-0@@ -418,14 +417,14 @@ static int meson_i2c_probe(struct platform_device *pdev) ? if (IS_ERR(i2c->regs)) ? return PTR_ERR(i2c->regs);?
drivers/i2c/busses/i2c-meson.c +2
-2- 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");drivers/i2c/busses/i2c-meson.c +1
-1-return i2c->irq; +return irq;? }
?drivers/i2c/busses/i2c-meson.c +2
-2- 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);
Quoted 3 lines
? if (ret < 0) {
?dev_err(&pdev->dev, "can't request IRQ\n");
? return ret;
PATCH 3/12 i2c: meson: use i2c core for DT clock-frequency parsing
In reply to Heiner KallweitWe 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.
--- drivers/i2c/busses/i2c-meson.c | 13 +++++-------- 1 file changed, 5 insertions(+), 8 deletions(-)
drivers/i2c/busses/i2c-meson.c +5 -8
@@ -38,7 +38,6 @@#define REG_CTRL_CLKDIV_MASK ((BIT(10) - 1) << REG_CTRL_CLKDIV_SHIFT)#define I2C_TIMEOUT_MS 500 -#define DEFAULT_FREQ 100000enum {TOKEN_END =0 ,
@@ -73,7 +72,7 @@ enum {* @error: Flag set when an error is received * @lock: To avoid race conditions between irq handler and xfer code * @done: Completion used to wait for transfer termination -* @frequency: Operating frequency of I2C bus clock +* @timings: Struct including the bus frequency * @tokens: Sequence of tokens to be written to the device * @num_tokens: Number of tokens */
@@ -92,7 +91,7 @@ struct meson_i2c {spinlock_t lock;struct completion done; -unsigned int frequency; +struct i2c_timings timings;u32 tokens[2 ];int num_tokens; };
@@ -136,12 +135,12 @@ static void meson_i2c_set_clk_div(struct meson_i2c *i2c)unsigned long clk_rate =clk_get_rate (i2c->clk);unsigned int div; - div =DIV_ROUND_UP (clk_rate, i2c->frequency *4 ); + div =DIV_ROUND_UP (clk_rate, i2c->timings.bus_freq_hz *4 );meson_i2c_set_mask (i2c,REG_CTRL ,REG_CTRL_CLKDIV_MASK , div <<REG_CTRL_CLKDIV_SHIFT );dev_dbg (i2c->dev,"%s: clk %lu, freq %u, div %u\n" , __func__, - clk_rate, i2c->frequency, div); + clk_rate, i2c->timings.bus_freq_hz, div); }static void meson_i2c_get_data (struct meson_i2c *i2c,char *buf,int len)
@@ -396,9 +395,7 @@ static int meson_i2c_probe(struct platform_device *pdev)if (!i2c)return -ENOMEM ; -if (of_property_read_u32 (pdev->dev.of_node,"clock-frequency" , - &i2c->frequency)) - i2c->frequency =DEFAULT_FREQ ; +i2c_parse_fw_timings (&pdev->dev, &i2c->timings, true); i2c->dev = &pdev->dev;platform_set_drvdata (pdev, i2c);
Signature · 1 line
2.12.0
Jerome Brunet
reviewerPATCH 3/12 i2c: meson: use i2c core for DT clock-frequency parsing
Quoted 70 lines · Heiner Kallweit
We don't have to parse the DT manually to retrieve the bus frequency and we don't have to maintain an own default for the bus frequency. Let the i2c core do this for us.
Signed-off-by Heiner Kallweit <[email protected]>---
?drivers/i2c/busses/i2c-meson.c | 13 +++++-------- ?1 file changed, 5 insertions(+), 8 deletions(-)drivers/i2c/busses/i2c- renamed from drivers/i2c/busses/i2c-meson.c +0
-0
meson.c
index50059d09..5e243efa100644drivers/i2c/busses/i2c-meson.c +0
-0@@ -38,7 +38,6 @@?#define
REG_CTRL_CLKDIV_MASK((BIT(10) - 1) <<REG_CTRL_CLKDIV_SHIFT)
?
?#defineI2C_TIMEOUT_MS500
-#defineDEFAULT_FREQ100000
?
?enum {
?TOKEN_END= 0,drivers/i2c/busses/i2c-meson.c +0
-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
? */drivers/i2c/busses/i2c-meson.c +0
-0@@ -92,7 +91,7 @@ struct meson_i2c {?
?spinlock_tlock;
? struct completion done;drivers/i2c/busses/i2c-meson.c +1
-1-unsigned int frequency; +struct i2c_timings timings;? u32 tokens[2];
? intnum_tokens;
?};drivers/i2c/busses/i2c-meson.c +0
-0@@ -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;?
drivers/i2c/busses/i2c-meson.c +1
-1- 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__,drivers/i2c/busses/i2c-meson.c +1
-1- clk_rate, i2c->frequency, div); + clk_rate, i2c->timings.bus_freq_hz, div);?}
?
?static voidmeson_i2c_get_data(struct meson_i2c *i2c, char *buf, int len)drivers/i2c/busses/i2c-meson.c +0
-0@@ -396,9 +395,7 @@ static int meson_i2c_probe(struct platform_device *pdev) ? if (!i2c) ? return -ENOMEM;?
drivers/i2c/busses/i2c-meson.c +0
-1- if (of_property_read_u32(pdev->dev.of_node, "clock-frequency",
drivers/i2c/busses/i2c-meson.c +1
-2- ?&i2c->frequency)) - i2c->frequency =DEFAULT_FREQ ; +i2c_parse_fw_timings (&pdev->dev, &i2c->timings, true);
Quoted 3 lines
?
?i2c->dev=&pdev->dev;
?platform_set_drvdata(pdev, i2c);
PATCH 4/12 i2c: meson: use full 12 bits for clock divider
In reply to Heiner KallweitThe 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.
--- drivers/i2c/busses/i2c-meson.c | 13 +++++++++++-- 1 file changed, 11 insertions(+), 2 deletions(-)
drivers/i2c/busses/i2c-meson.c +11 -2
@@ -35,7 +35,9 @@#define REG_CTRL_STATUS BIT(2)#define REG_CTRL_ERROR BIT(3)#define REG_CTRL_CLKDIV_SHIFT 12 -#define REG_CTRL_CLKDIV_MASK ((BIT(10) - 1) << REG_CTRL_CLKDIV_SHIFT) +#define REG_CTRL_CLKDIV_MASK GENMASK(21, 12) +#define REG_CTRL_CLKDIVEXT_SHIFT 28 +#define REG_CTRL_CLKDIVEXT_MASK GENMASK(29, 28)#define I2C_TIMEOUT_MS 500
@@ -136,8 +138,15 @@ static void meson_i2c_set_clk_div(struct meson_i2c *i2c)unsigned int div; div =DIV_ROUND_UP (clk_rate, i2c->timings.bus_freq_hz *4 ); + +/* clock divider has 12 bits */ +WARN_ON (div >= (1 <<12 )); +meson_i2c_set_mask (i2c,REG_CTRL ,REG_CTRL_CLKDIV_MASK , - div <<REG_CTRL_CLKDIV_SHIFT ); + (div &GENMASK (9 ,0 )) <<REG_CTRL_CLKDIV_SHIFT ); + +meson_i2c_set_mask (i2c,REG_CTRL ,REG_CTRL_CLKDIVEXT_MASK , + (div >>10 ) <<REG_CTRL_CLKDIVEXT_SHIFT );dev_dbg (i2c->dev,"%s: clk %lu, freq %u, div %u\n" , __func__, clk_rate, i2c->timings.bus_freq_hz, div);
Signature · 1 line
2.12.0
Jerome Brunet
reviewerPATCH 4/12 i2c: meson: use full 12 bits for clock divider
Quoted 40 lines · Heiner Kallweit
The clock divider has 12 bits, splitted into a 10 bit field and a 2 bit field. The extra 2 bits aren't used currently.
Change this to use the full 12 bits and warn if the requested frequency is too low.
Signed-off-by Heiner Kallweit <[email protected]>---
?drivers/i2c/busses/i2c-meson.c | 13 +++++++++++-- ?1 file changed, 11 insertions(+), 2 deletions(-)drivers/i2c/busses/i2c-meson.c +0
-0@@ -35,7 +35,9 @@ ?#define REG_CTRL_STATUS BIT(2) ?#define REG_CTRL_ERROR BIT(3)?#define
REG_CTRL_CLKDIV_SHIFT12drivers/i2c/busses/i2c-meson.c +3
-1-#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)?
?#defineI2C_TIMEOUT_MS500
?drivers/i2c/busses/i2c-meson.c +0
-0@@ -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);drivers/i2c/busses/i2c-meson.c +4
-0+ +/* clock divider has 12 bits */ +WARN_ON (div >= (1 <<12 )); +? meson_i2c_set_mask(i2c,
REG_CTRL,REG_CTRL_CLKDIV_MASK,drivers/i2c/busses/i2c-meson.c +4
-1- ???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);
Quoted 3 lines
?
? dev_dbg(i2c->dev, "%s: clk %lu, freq %u, div %u\n",__func__,
?clk_rate,i2c->timings.bus_freq_hz, div);
PATCH 5/12 i2c: meson: set clock divider in probe instead of setting it for each transfer
In reply to Heiner KallweitThe 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.
--- drivers/i2c/busses/i2c-meson.c | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-)
drivers/i2c/busses/i2c-meson.c +7 -7
@@ -74,7 +74,6 @@ enum {* @error: Flag set when an error is received * @lock: To avoid race conditions between irq handler and xfer code * @done: Completion used to wait for transfer termination -* @timings: Struct including the bus frequency * @tokens: Sequence of tokens to be written to the device * @num_tokens: Number of tokens */
@@ -93,7 +92,6 @@ struct meson_i2c {spinlock_t lock;struct completion done; -struct i2c_timings timings;u32 tokens[2 ];int num_tokens; };
@@ -132,12 +130,12 @@ static void meson_i2c_write_tokens(struct meson_i2c *i2c)writel (i2c->tokens[1 ], i2c->regs +REG_TOK_LIST1 ); } -static void meson_i2c_set_clk_div (struct meson_i2c *i2c) +static void meson_i2c_set_clk_div (struct meson_i2c *i2c,unsigned int freq) {unsigned long clk_rate =clk_get_rate (i2c->clk);unsigned int div; - div =DIV_ROUND_UP (clk_rate, i2c->timings.bus_freq_hz *4 ); + div =DIV_ROUND_UP (clk_rate, freq *4 );/* clock divider has 12 bits */ WARN_ON (div >= (1 <<12 ));
@@ -149,7 +147,7 @@ static void meson_i2c_set_clk_div(struct meson_i2c *i2c) (div >> 10) << REG_CTRL_CLKDIVEXT_SHIFT);dev_dbg (i2c->dev,"%s: clk %lu, freq %u, div %u\n" , __func__, - clk_rate, i2c->timings.bus_freq_hz, div); + clk_rate, freq, div); }static void meson_i2c_get_data (struct meson_i2c *i2c,char *buf,int len)
@@ -369,7 +367,6 @@ static int meson_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs,int i, ret =0 , count =0 ;clk_enable (i2c->clk); -meson_i2c_set_clk_div (i2c);for (i =0 ; i < num; i++) { ret =meson_i2c_xfer_msg (i2c, msgs + i, i == num -1 );
@@ -396,6 +393,7 @@ static const struct i2c_algorithm meson_i2c_algorithm = {static int meson_i2c_probe (struct platform_device *pdev) {struct device_node *np = pdev->dev.of_node; +struct i2c_timings timings;struct meson_i2c *i2c;struct resource *mem;int irq, ret =0 ;
@@ -404,7 +402,7 @@ static int meson_i2c_probe(struct platform_device *pdev)if (!i2c)return -ENOMEM ; -i2c_parse_fw_timings (&pdev->dev, &i2c->timings, true); +i2c_parse_fw_timings (&pdev->dev, &timings, true); i2c->dev = &pdev->dev;platform_set_drvdata (pdev, i2c);
@@ -462,6 +460,8 @@ static int meson_i2c_probe(struct platform_device *pdev)return ret; } +meson_i2c_set_clk_div (i2c, timings.bus_freq_hz); +return 0 ; }
Signature · 1 line
2.12.0
PATCH 6/12 i2c: meson: remove variable count from meson_i2c_xfer
In reply to Heiner KallweitVariable 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.
--- drivers/i2c/busses/i2c-meson.c | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-)
drivers/i2c/busses/i2c-meson.c +3 -4
@@ -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)
Signature · 1 line
2.12.0
Jerome Brunet
reviewerPATCH 6/12 i2c: meson: remove variable count from meson_i2c_xfer
Quoted 24 lines · 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 <[email protected]>---
?drivers/i2c/busses/i2c-meson.c | 7 +++----
?1 file changed, 3 insertions(+), 4 deletions(-)drivers/i2c/busses/i2c-meson.c +1
-1@@ -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++) {
drivers/i2c/busses/i2c-meson.c static int meson_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs, · line 372
Quoted 2 lines
drivers/i2c/busses/i2c-meson.c +1
-1- 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.
Quoted 12 lines
? if (ret)
? break;drivers/i2c/busses/i2c-meson.c +0
-1- count++;? }
?
?clk_disable(i2c->clk);
?drivers/i2c/busses/i2c-meson.c +1
-1-return ret ? ret : count; +return ret ?: i;?}
?
?static u32meson_i2c_func(struct i2c_adapter *adap)
PATCH 7/12 i2c: meson: improve interrupt handler and detect spurious interrupts
In reply to Heiner KallweitIf 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.
--- drivers/i2c/busses/i2c-meson.c | 16 +++++++++------- 1 file changed, 9 insertions(+), 7 deletions(-)
drivers/i2c/busses/i2c-meson.c +9 -7
@@ -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 ®_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 ®_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 ;
Signature · 1 line
2.12.0
Jerome Brunet
reviewerPATCH 7/12 i2c: meson: improve interrupt handler and detect spurious interrupts
Quoted 19 lines · Heiner Kallweit
If state is
STATE_IDLEno interrupt should occur. Detect this case and warn.
In addition move resettingREG_CTRL_STARTbit to the start of the interrupt handler and remove a unneededREG_CTRL_STARTbit reset inmeson_i2c_xfer_msg.Signed-off-by Heiner Kallweit <[email protected]>---
?drivers/i2c/busses/i2c-meson.c | 16 +++++++++------- ?1 file changed, 9 insertions(+), 7 deletions(-)drivers/i2c/busses/i2c-meson.c +0
-0@@ -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);?
drivers/i2c/busses/i2c-meson.c static irqreturn_t meson_i2c_irq(int irqno, void *dev_id) · line 242
Quoted 8 lines
drivers/i2c/busses/i2c-meson.c +7
-1-if (ctrl ®_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.
Quoted 33 lines
drivers/i2c/busses/i2c-meson.c +2
-0+ +if (ctrl ®_CTRL_ERROR ) {? /*
? ?* The bit is set when theIGNORE_NAKbit is cleared
? ?* and the device didn't respond. In this case, thedrivers/i2c/busses/i2c-meson.c +0
-2@@ -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);drivers/i2c/busses/i2c-meson.c +0
-1-meson_i2c_set_mask (i2c,REG_CTRL ,REG_CTRL_START ,0 );? meson_i2c_set_mask(i2c,
REG_CTRL,REG_CTRL_START,
? ???REG_CTRL_START);
? }drivers/i2c/busses/i2c-meson.c +0
-0@@ -344,9 +349,6 @@ static int meson_i2c_xfer_msg(struct meson_i2c *i2c, struct i2c_msg *msg,? ?*/
?spin_lock_irqsave(&i2c->lock, flags);
?drivers/i2c/busses/i2c-meson.c +0
-3-/* 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 7/12 i2c: meson: improve interrupt handler and detect spurious interrupts
Am 08.03.2017 um 10:50 schrieb Jerome Brunet:
Quoted 20 lines · Heiner Kallweit
Quoted 19 lines
If state is
STATE_IDLEno interrupt should occur. Detect this case and warn.
In addition move resettingREG_CTRL_STARTbit to the start of the interrupt handler and remove a unneededREG_CTRL_STARTbit reset inmeson_i2c_xfer_msg.Signed-off-by Heiner Kallweit <[email protected]>--- drivers/i2c/busses/i2c-meson.c | 16 +++++++++------- 1 file changed, 9 insertions(+), 7 deletions(-)drivers/i2c/busses/i2c-meson.c +0
-0@@ -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);
Quoted 12 lines
drivers/i2c/busses/i2c-meson.c static irqreturn_t meson_i2c_irq(int irqno, void *dev_id) · line 242
Quoted 8 lines
drivers/i2c/busses/i2c-meson.c +7
-1-if (ctrl ®_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.
Quoted 38 lines
In any case, if it can be handled gracefully, I'd drop the trace in the interrupt handler.
Quoted 33 lines
drivers/i2c/busses/i2c-meson.c +2
-3+ +if (ctrl ®_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 );drivers/i2c/busses/i2c-meson.c +0
-3}@@ -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 7/12 i2c: meson: improve interrupt handler and detect spurious interrupts
Am 08.03.2017 um 22:02 schrieb Heiner Kallweit:
Quoted 75 lines
Am 08.03.2017 um 10:50 schrieb Jerome Brunet:
Quoted 20 lines · Heiner Kallweit
Quoted 19 lines
If state is
STATE_IDLEno interrupt should occur. Detect this case and warn.
In addition move resettingREG_CTRL_STARTbit to the start of the interrupt handler and remove a unneededREG_CTRL_STARTbit reset inmeson_i2c_xfer_msg.Signed-off-by Heiner Kallweit <[email protected]>--- drivers/i2c/busses/i2c-meson.c | 16 +++++++++------- 1 file changed, 9 insertions(+), 7 deletions(-)drivers/i2c/busses/i2c-meson.c +0
-0@@ -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);Quoted 12 lines
drivers/i2c/busses/i2c-meson.c static irqreturn_t meson_i2c_irq(int irqno, void *dev_id) · line 242
Quoted 8 lines
drivers/i2c/busses/i2c-meson.c +7
-1-if (ctrl ®_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_NONEinstead ofIRQ_HANDLEDin this potential error case.It's like other drivers checking in the interrupt handler whether any interrupt source is enabled and returning
IRQ_NONEif not.Quoted 31 lines
In any case, if it can be handled gracefully, I'd drop the trace in the interrupt handler.
Quoted 27 lines
drivers/i2c/busses/i2c-meson.c +2
-3+ +if (ctrl ®_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 );drivers/i2c/busses/i2c-meson.c +0
-0}@@ -344,9 +349,6 @@ static int meson_i2c_xfer_msg(struct meson_i2c *i2c,struct i2c_msg *msg, */ spin_lock_irqsave (&i2c->lock, flags);
Quoted 3 lines
Quoted 3 lines
drivers/i2c/busses/i2c-meson.c static int meson_i2c_xfer_msg(struct meson_i2c *i2c, struct i2c_msg *msg, · line 349
Quoted 3 lines
drivers/i2c/busses/i2c-meson.c +0
-3-/* 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.
Quoted 5 lines
Quoted 4 lines
Quoted 3 lines
drivers/i2c/busses/i2c-meson.c +0
-0if (!time_left) { i2c->state =STATE_IDLE ;ret =
-ETIMEDOUT;
PATCH 8/12 i2c: meson: explicitly ignore messages with length zero
In reply to Heiner KallweitExplicitely ignore messages with length zero. This also allows to remove some now unneeded checks during message processing.
--- drivers/i2c/busses/i2c-meson.c | 20 +++++++++----------- 1 file changed, 9 insertions(+), 11 deletions(-)
drivers/i2c/busses/i2c-meson.c +9 -11
@@ -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 ;
Signature · 1 line
2.12.0
A message that is not in the archive (sent off-list or not yet received): CANe6Qb9nKdLnOcwEwuk8NXEB+fxW2S_kkzP+U+qygYhNqz0Y0w@mail.gmail.com
PATCH 8/12 i2c: meson: explicitly ignore messages with length zero
Am 08.03.2017 um 08:53 schrieb Ben Dooks:
Quoted 2 lines
Probably a bad idea. Zero data could just be an address is there check
Thanks for pointing out.
Quoted 65 lines · On 8 Mar 2017 06:51, "Heiner Kallweit" <hkallweit1 at gmail.com…
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:[email protected]>>---
drivers/i2c/busses/i2c-meson.c| 20 +++++++++----------- 1 file changed, 9 insertions(+), 11 deletions(-)diff --git
a/drivers/i2c/busses/i2c-meson.cb/drivers/i2c/busses/i2c-meson.cindex b3b881f9..58414699 100644
---a/drivers/i2c/busses/i2c-meson.c
+++b/drivers/i2c/busses/i2c-meson.c
@@ -196,12 +196,10 @@ static voidmeson_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 @@ staticirqreturn_tmeson_i2c_irq(int irqno, void *dev_id)switch (
i2c->state) {
caseSTATE_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(structi2c_adapter*adap, structi2c_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:[email protected]>
http://lists.infradead.org/mailman/listinfo/linux-amlogic <http://lists.infradead.org/mailman/listinfo/linux-amlogic>
PATCH 9/12 i2c: meson: don't create separate token chain just for the stop command
In reply to Heiner KallweitWe 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.
--- drivers/i2c/busses/i2c-meson.c | 27 +++++++-------------------- 1 file changed, 7 insertions(+), 20 deletions(-)
drivers/i2c/busses/i2c-meson.c +7 -20
@@ -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:
Signature · 1 line
2.12.0
PATCH 10/12 i2c: meson: remove meson_i2c_write_tokens
In reply to Heiner Kallweitmeson_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.
--- drivers/i2c/busses/i2c-meson.c | 14 ++++---------- 1 file changed, 4 insertions(+), 10 deletions(-)
drivers/i2c/busses/i2c-meson.c +4 -10
@@ -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 */
Signature · 1 line
2.12.0
PATCH 11/12 i2c: meson: improve and simplify interrupt handler
In reply to Heiner KallweitThe preceding changes in this patch series now allow to simplify the interrupt handler significantly.
--- drivers/i2c/busses/i2c-meson.c | 35 +++++++++-------------------------- 1 file changed, 9 insertions(+), 26 deletions(-)
drivers/i2c/busses/i2c-meson.c +9 -26
@@ -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 ;
Signature · 1 line
2.12.0
PATCH 12/12 i2c: meson: use state in meson_i2c_prepare_xfer
In reply to Heiner KallweitLet's use the state here to make meson_i2c_prepare_xfer more in line with other parts of the driver.
--- drivers/i2c/busses/i2c-meson.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-)
drivers/i2c/busses/i2c-meson.c +2 -3
@@ -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 );
Signature · 1 line
2.12.0
Jerome Brunet
reviewercover i2c: meson: series with improvements
In reply to Heiner KallweitQuoted 2 lines · Heiner Kallweit
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.
Quoted 20 lines
Heiner Kallweit (12):
? i2c: meson: use min instead ofmin_twheremin_tisn't needed ? i2c: meson: remove member irq from structmeson_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 frommeson_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: removemeson_i2c_write_tokens
? i2c: meson: improve and simplify interrupt handler
? i2c: meson: use state inmeson_i2c_prepare_xfer?drivers/i2c/busses/i2c-meson.c | 157 ++++++++++++++++----------------------
---
?1 file changed, 62 insertions(+), 95 deletions(-)
cover i2c: meson: series with improvements
Am 08.03.2017 um 10:34 schrieb Jerome Brunet:
Quoted 8 lines · Heiner Kallweit
Quoted 2 lines
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
Quoted 24 lines
Personally, I don't have test case representative enough to test this series thoroughly.
Quoted 20 lines
Heiner Kallweit (12):
i2c: meson: use min instead ofmin_twheremin_tisn't needed
i2c: meson: remove member irq from structmeson_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 frommeson_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: removemeson_i2c_write_tokens
i2c: meson: improve and simplify interrupt handler
i2c: meson: use state inmeson_i2c_prepare_xferdrivers/i2c/busses/i2c-meson.c | 157 ++++++++++++++++---------------------- --- 1 file changed, 62 insertions(+), 95 deletions(-)