i2c: mpc: Refactor to improve responsiveness
Subsystems (2)
Chris Packham
maintainerv3 cover i2c: mpc: Refactor to improve responsiveness
This is an update to what is already in i2c/for-next. I've included "i2c: mpc:
use device managed APIs" which had some problems in the remove code path which Wei Yongjun kindly pointed out with a fix. I've incorporated those changes into this version in case the original is reverted.
I've tested on T2081 and P2041 based systems with a number of i2c and smbus devices. Also this time I included a few iterations of module insert/remove which would have caught the earlier errors.
Chris Packham (4):
i2c: mpc: use device managed APIs
i2c: mpc: Interrupt driven transfer
i2c: mpc: Remove redundant NULL check
MAINTAINERS: Add Chris Packham as FREESCALE MPC I2C maintainer
MAINTAINERS | 7 + drivers/i2c/busses/i2c-mpc.c | 488 +++++++++++++++++++---------------- 2 files changed, 267 insertions(+), 228 deletions(-)
Signature · 1 line
2.31.1
Chris Packham
maintainerPATCH v3 1/4 i2c: mpc: use device managed APIs
Use device managed functions an clean up error handling.
---
Notes:
Changes in v3:
- Assuming 09aab7add7bf is reverted I've folded in the fix from Wei Yongjun[1] into the original patch. If Wei's patch is applied on top of whats already in i2c/for-next then this patch can be ignored.
[1] - https://lore.kernel.org/linux-i2c/[email protected]/
drivers/i2c/busses/i2c-mpc.c | 52 +++++++++++++----------------------- 1 file changed, 18 insertions(+), 34 deletions(-)
drivers/i2c/busses/i2c-mpc.c +18 -34
@@ -654,7 +654,6 @@ static int fsl_i2c_probe(struct platform_device *op)u32 clock =MPC_I2C_CLOCK_LEGACY ;int result =0 ;int plen; -struct resource res;struct clk *clk;int err;
@@ -662,7 +661,7 @@ static int fsl_i2c_probe(struct platform_device *op)if (!match)return -EINVAL ; - i2c =kzalloc (sizeof (*i2c),GFP_KERNEL ); + i2c =devm_kzalloc (&op->dev,sizeof (*i2c),GFP_KERNEL );if (!i2c)return -ENOMEM ;
@@ -670,24 +669,21 @@ static int fsl_i2c_probe(struct platform_device *op)init_waitqueue_head (&i2c->queue); - i2c->base =of_iomap (op->dev.of_node,0 ); -if (!i2c->base) { + i2c->base =devm_platform_ioremap_resource (op,0 ); +if (IS_ERR (i2c->base)) {dev_err (i2c->dev,"failed to map controller\n" ); - result = -ENOMEM ; - goto fail_map; +return PTR_ERR (i2c->base); } - i2c->irq =irq_of_parse_and_map (op->dev.of_node,0 ); -if (i2c->irq <0 ) { - result = i2c->irq; - goto fail_map; - } + i2c->irq =platform_get_irq (op,0 ); +if (i2c->irq <0 ) +return i2c->irq; - result =request_irq (i2c->irq, mpc_i2c_isr, + result =devm_request_irq (&op->dev, i2c->irq, mpc_i2c_isr,IRQF_SHARED ,"i2c-mpc" , i2c);if (result <0 ) {dev_err (i2c->dev,"failed to attach interrupt\n" ); - goto fail_request; +return result; }/*
@@ -699,7 +695,7 @@ static int fsl_i2c_probe(struct platform_device *op) err =clk_prepare_enable (clk);if (err) {dev_err (&op->dev,"failed to enable clock\n" ); - goto fail_request; +return err; }else { i2c->clk_per = clk; }
@@ -731,32 +727,26 @@ static int fsl_i2c_probe(struct platform_device *op) }dev_info (i2c->dev,"timeout %u us\n" , mpc_ops.timeout *1000000 /HZ ); -platform_set_drvdata (op, i2c); - i2c->adap = mpc_ops; -of_address_to_resource (op->dev.of_node,0 , &res);scnprintf (i2c->adap.name,sizeof (i2c->adap.name), -"MPC adapter at 0x%llx" , (unsigned long long )res.start); -i2c_set_adapdata (&i2c->adap, i2c); +"MPC adapter (%s)" ,of_node_full_name (op->dev.of_node)); i2c->adap.dev.parent = &op->dev; + i2c->adap.nr = op->id; i2c->adap.dev.of_node =of_node_get (op->dev.of_node); i2c->adap.bus_recovery_info = &fsl_i2c_recovery_info; +platform_set_drvdata (op, i2c); +i2c_set_adapdata (&i2c->adap, i2c); - result =i2c_add_adapter (&i2c->adap); -if (result <0 ) + result =i2c_add_numbered_adapter (&i2c->adap); +if (result) goto fail_add; -return result; +return 0 ; fail_add:if (i2c->clk_per)clk_disable_unprepare (i2c->clk_per); -free_irq (i2c->irq, i2c); - fail_request: -irq_dispose_mapping (i2c->irq); -iounmap (i2c->base); - fail_map: -kfree (i2c); +return result; };
@@ -769,12 +759,6 @@ static int fsl_i2c_remove(struct platform_device *op)if (i2c->clk_per)clk_disable_unprepare (i2c->clk_per); -if (i2c->irq) -free_irq (i2c->irq, i2c); - -irq_dispose_mapping (i2c->irq); -iounmap (i2c->base); -kfree (i2c);return 0 ; };
Signature · 1 line
2.31.1
Re: [PATCH v3 1/4] i2c: mpc: use device managed APIs
Quoted 3 lines
Yongjun[1] into the original patch. If Wei's patch is applied on top of whats already in i2c/for-next then this patch can be ignored.
I applied Wei's patch instead. It was easier.
Re: [PATCH v3 1/4] i2c: mpc: use device managed APIs
Quoted 6 lines · Wolfram Sang
Quoted 4 lines
Yongjun[1] into the original patch. If Wei's patch is applied on top of whats already in i2c/for-next then this patch can be ignored.
I applied Wei's patch instead. It was easier.
Where is it? i2c/for-next shows the v2 of this series being applied.
Signature · 2 lines
With Best Regards, Andy Shevchenko
Re: [PATCH v3 1/4] i2c: mpc: use device managed APIs
Quoted 10 lines · Andy Shevchenko
Quoted 6 lines · Wolfram Sang
Quoted 4 lines
Yongjun[1] into the original patch. If Wei's patch is applied on top of whats already in i2c/for-next then this patch can be ignored.
I applied Wei's patch instead. It was easier.
Where is it? i2c/for-next shows the v2 of this series being applied.
Ah, I see it's a follow up.
I thought it was a replacement.
Signature · 2 lines
With Best Regards, Andy Shevchenko
Chris Packham
maintainerPATCH v3 2/4 i2c: mpc: Interrupt driven transfer
In reply to Chris PackhamThe fsl-i2c controller will generate an interrupt after every byte transferred. Make use of this interrupt to drive a state machine which allows the next part of a transfer to happen as soon as the interrupt is received. This is particularly helpful with SMBUS devices like the LM81 which will timeout if we take too long between bytes in a transfer.
---
Notes:
Changes in v3:
- use WARN/WARN_ON instead of BUG/BUG_ON
Changes in v2:
- add static_assert for state debug strings
- remove superfluous space
drivers/i2c/busses/i2c-mpc.c | 434 +++++++++++++++++++---------------- 1 file changed, 241 insertions(+), 193 deletions(-)
drivers/i2c/busses/i2c-mpc.c +241 -193
@@ -1,16 +1,11 @@ +// SPDX-License-Identifier: GPL-2.0 /* -* (C) Copyright 2003-2004 -* Humboldt Solutions Ltd, [email protected]. -* This is a combined i2c adapter and algorithm driver for the * MPC107/Tsi107 PowerPC northbridge and processors that include * the same I2C unit (8240, 8245, 85xx). * -* Release 0.8 -* -* This file is licensed under the terms of the GNU General Public -* License version 2. This program is licensed "as is" without any -* warranty of any kind, whether express or implied. +* Copyright (C) 2003-2004 Humboldt Solutions Ltd, [email protected] +* Copyright (C) 2021 Allied Telesis Labs */ #include <linux/kernel.h>
@@ -58,11 +53,36 @@#define CSR_MIF 0x02#define CSR_RXAK 0x01 +enum mpc_i2c_action { +MPC_I2C_ACTION_INVALID =0 , +MPC_I2C_ACTION_START , +MPC_I2C_ACTION_RESTART , +MPC_I2C_ACTION_READ_BEGIN , +MPC_I2C_ACTION_READ_BYTE , +MPC_I2C_ACTION_WRITE , +MPC_I2C_ACTION_STOP , + + __MPC_I2C_ACTION_CNT +}; + +static char *action_str[] = { +"invalid" , +"start" , +"restart" , +"read begin" , +"read" , +"write" , +"stop" , +}; + +static_assert (ARRAY_SIZE (action_str) == __MPC_I2C_ACTION_CNT); +struct mpc_i2c {struct device *dev;void __iomem *base;u32 interrupt; -wait_queue_head_t queue; +wait_queue_head_t waitq; +spinlock_t lock;struct i2c_adapter adap;int irq;u32 real_clk;
@@ -70,6 +90,16 @@ struct mpc_i2c {u8 fdr, dfsrr;#endif struct clk *clk_per; +u32 cntl_bits; +enum mpc_i2c_action action; +struct i2c_msg *msgs; +int num_msgs; +int curr_msg; +u32 byte_posn; +u32 block; +int rc; +int expect_rxack; + };struct mpc_i2c_divider {
@@ -86,19 +116,6 @@ static inline void writeccr(struct mpc_i2c *i2c, u32 x)writeb (x, i2c->base +MPC_I2C_CR ); } -static irqreturn_t mpc_i2c_isr (int irq,void *dev_id) -{ -struct mpc_i2c *i2c = dev_id; -if (readb (i2c->base +MPC_I2C_SR ) &CSR_MIF ) { -/* Read again to allow register to stabilise */ - i2c->interrupt =readb (i2c->base +MPC_I2C_SR ); -writeb (0 , i2c->base +MPC_I2C_SR ); -wake_up (&i2c->queue); -return IRQ_HANDLED ; - } -return IRQ_NONE ; -} -/* Sometimes 9th clock pulse isn't generated, and slave doesn't release * the bus, because it wants to send ACK. * Following sequence of enabling/disabling and sending start/stop generates
@@ -121,45 +138,6 @@ static void mpc_i2c_fixup(struct mpc_i2c *i2c) } } -static int i2c_wait (struct mpc_i2c *i2c,unsigned timeout,int writing) -{ -u32 cmd_err; -int result; - - result =wait_event_timeout (i2c->queue, - (i2c->interrupt &CSR_MIF ), timeout); - -if (unlikely (!(i2c->interrupt &CSR_MIF ))) { -dev_dbg (i2c->dev,"wait timeout\n" ); -writeccr (i2c,0 ); - result = -ETIMEDOUT ; - } - - cmd_err = i2c->interrupt; - i2c->interrupt =0 ; - -if (result <0 ) -return result; - -if (!(cmd_err &CSR_MCF )) { -dev_dbg (i2c->dev,"unfinished\n" ); -return -EIO ; - } - -if (cmd_err &CSR_MAL ) { -dev_dbg (i2c->dev,"MAL\n" ); -return -EAGAIN ; - } - -if (writing && (cmd_err &CSR_RXAK )) { -dev_dbg (i2c->dev,"No RXAK\n" ); -/* generate stop */ -writeccr (i2c,CCR_MEN ); -return -ENXIO ; - } -return 0 ; -} -#if defined(CONFIG_PPC_MPC52xx) || defined(CONFIG_PPC_MPC512x)static const struct mpc_i2c_divider mpc_i2c_dividers_52xx[] = { {20, 0x20}, {22, 0x21}, {24, 0x22}, {26, 0x23},
@@ -434,168 +412,209 @@ static void mpc_i2c_setup_8xxx(struct device_node *node, }#endif /* CONFIG_FSL_SOC */ -static void mpc_i2c_start (struct mpc_i2c *i2c) +static void mpc_i2c_finish (struct mpc_i2c *i2c,int rc) { -/* Clear arbitration */ -writeb (0 , i2c->base +MPC_I2C_SR ); -/* Start with MEN */ -writeccr (i2c,CCR_MEN ); + i2c->rc = rc; + i2c->block =0 ; + i2c->cntl_bits =CCR_MEN ; +writeccr (i2c, i2c->cntl_bits); +wake_up (&i2c->waitq); } -static void mpc_i2c_stop (struct mpc_i2c *i2c) +static void mpc_i2c_do_action (struct mpc_i2c *i2c) { -writeccr (i2c,CCR_MEN ); -} +struct i2c_msg *msg = &i2c->msgs[i2c->curr_msg]; +int dir =0 ; +int recv_len =0 ; +u8 byte; + +dev_dbg (i2c->dev,"%s: action = %s\n" , __func__, + action_str[i2c->action]); + + i2c->cntl_bits &= ~(CCR_RSTA |CCR_MTX |CCR_TXAK ); + +if (msg->flags &I2C_M_RD ) + dir =1 ; +if (msg->flags &I2C_M_RECV_LEN ) + recv_len =1 ; + +switch (i2c->action) { +case MPC_I2C_ACTION_RESTART : + i2c->cntl_bits |=CCR_RSTA ; + fallthrough; + +case MPC_I2C_ACTION_START : + i2c->cntl_bits |=CCR_MSTA |CCR_MTX ; +writeccr (i2c, i2c->cntl_bits); +writeb ((msg->addr <<1 ) | dir, i2c->base +MPC_I2C_DR ); + i2c->expect_rxack =1 ; + i2c->action = dir ?MPC_I2C_ACTION_READ_BEGIN :MPC_I2C_ACTION_WRITE ; +break ; + +case MPC_I2C_ACTION_READ_BEGIN : +if (msg->len) { +if (msg->len ==1 && !(msg->flags &I2C_M_RECV_LEN )) + i2c->cntl_bits |=CCR_TXAK ; + +writeccr (i2c, i2c->cntl_bits); +/* Dummy read */ +readb (i2c->base +MPC_I2C_DR ); + } + i2c->action =MPC_I2C_ACTION_READ_BYTE ; +break ; -static int mpc_write (struct mpc_i2c *i2c,int target, -const u8 *data,int length,int restart) -{ -int i, result; -unsigned timeout = i2c->adap.timeout; -u32 flags = restart ?CCR_RSTA :0 ; +case MPC_I2C_ACTION_READ_BYTE : +if (i2c->byte_posn || !recv_len) { +/* Generate txack on next to last byte */ +if (i2c->byte_posn == msg->len -2 ) + i2c->cntl_bits |=CCR_TXAK ; +/* Do not generate stop on last byte */ +if (i2c->byte_posn == msg->len -1 ) + i2c->cntl_bits |=CCR_MTX ; -/* Start as master */ -writeccr (i2c,CCR_MIEN |CCR_MEN |CCR_MSTA |CCR_MTX | flags); -/* Write target byte */ -writeb ((target <<1 ), i2c->base +MPC_I2C_DR ); +writeccr (i2c, i2c->cntl_bits); + } - result =i2c_wait (i2c, timeout,1 ); -if (result <0 ) -return result; + byte =readb (i2c->base +MPC_I2C_DR ); -for (i =0 ; i < length; i++) { -/* Write data byte */ -writeb (data[i], i2c->base +MPC_I2C_DR ); +if (i2c->byte_posn ==0 && recv_len) { +if (byte ==0 || byte >I2C_SMBUS_BLOCK_MAX ) { +mpc_i2c_finish (i2c, -EPROTO ); +return ; + } + msg->len += byte; +/* +* For block reads, generate txack here if data length +* is 1 byte (total length is 2 bytes). +*/ +if (msg->len ==2 ) { + i2c->cntl_bits |=CCR_TXAK ; +writeccr (i2c, i2c->cntl_bits); + } + } - result =i2c_wait (i2c, timeout,1 ); -if (result <0 ) -return result; +dev_dbg (i2c->dev,"%s: %s %02x\n" , __func__, + action_str[i2c->action], byte); + msg->buf[i2c->byte_posn++] = byte; +break ; + +case MPC_I2C_ACTION_WRITE : +dev_dbg (i2c->dev,"%s: %s %02x\n" , __func__, + action_str[i2c->action], msg->buf[i2c->byte_posn]); +writeb (msg->buf[i2c->byte_posn++], i2c->base +MPC_I2C_DR ); + i2c->expect_rxack =1 ; +break ; + +case MPC_I2C_ACTION_STOP : +mpc_i2c_finish (i2c,0 ); +break ; + +case MPC_I2C_ACTION_INVALID : +default : +WARN (1 ,"Unexpected action %d\n" , i2c->action); +break ; } -return 0 ; +if (msg->len == i2c->byte_posn) { + i2c->curr_msg++; + i2c->byte_posn =0 ; + +if (i2c->curr_msg == i2c->num_msgs) { + i2c->action =MPC_I2C_ACTION_STOP ; +/* +* We don't get another interrupt on read so +* finish the transfer now +*/ +if (dir) +mpc_i2c_finish (i2c,0 ); + }else { + i2c->action =MPC_I2C_ACTION_RESTART ; + } + } } -static int mpc_read (struct mpc_i2c *i2c,int target, -u8 *data,int length,int restart,bool recv_len) +static void mpc_i2c_do_intr (struct mpc_i2c *i2c,u8 status) { -unsigned timeout = i2c->adap.timeout; -int i, result; -u32 flags = restart ?CCR_RSTA :0 ; +unsigned long flags; -/* Switch to read - restart */ -writeccr (i2c,CCR_MIEN |CCR_MEN |CCR_MSTA |CCR_MTX | flags); -/* Write target address byte - this time with the read flag set */ -writeb ((target <<1 ) |1 , i2c->base +MPC_I2C_DR ); +spin_lock_irqsave (&i2c->lock, flags); - result =i2c_wait (i2c, timeout,1 ); -if (result <0 ) -return result; +if (!(status &CSR_MCF )) { +dev_dbg (i2c->dev,"unfinished\n" ); +mpc_i2c_finish (i2c, -EIO ); + goto out; + } -if (length) { -if (length ==1 && !recv_len) -writeccr (i2c,CCR_MIEN |CCR_MEN |CCR_MSTA |CCR_TXAK ); -else -writeccr (i2c,CCR_MIEN |CCR_MEN |CCR_MSTA ); -/* Dummy read */ -readb (i2c->base +MPC_I2C_DR ); +if (status &CSR_MAL ) { +dev_dbg (i2c->dev,"arbiritration lost\n" ); +mpc_i2c_finish (i2c, -EAGAIN ); + goto out; } -for (i =0 ; i < length; i++) { -u8 byte; +if (i2c->expect_rxack && (status &CSR_RXAK )) { +dev_dbg (i2c->dev,"no RXAK\n" ); +mpc_i2c_finish (i2c, -ENXIO ); + goto out; + } + i2c->expect_rxack =0 ; - result =i2c_wait (i2c, timeout,0 ); -if (result <0 ) -return result; +mpc_i2c_do_action (i2c); -/* -* For block reads, we have to know the total length (1st byte) -* before we can determine if we are done. -*/ -if (i || !recv_len) { -/* Generate txack on next to last byte */ -if (i == length -2 ) -writeccr (i2c,CCR_MIEN |CCR_MEN |CCR_MSTA - |CCR_TXAK ); -/* Do not generate stop on last byte */ -if (i == length -1 ) -writeccr (i2c,CCR_MIEN |CCR_MEN |CCR_MSTA - |CCR_MTX ); - } +out: +spin_unlock_irqrestore (&i2c->lock, flags); +} - byte =readb (i2c->base +MPC_I2C_DR ); +static irqreturn_t mpc_i2c_isr (int irq,void *dev_id) +{ +struct mpc_i2c *i2c = dev_id; +u8 status =readb (i2c->base +MPC_I2C_SR ); -/* -* Adjust length if first received byte is length. -* The length is 1 length byte plus actually data length -*/ -if (i ==0 && recv_len) { -if (byte ==0 || byte >I2C_SMBUS_BLOCK_MAX ) -return -EPROTO ; - length += byte; -/* -* For block reads, generate txack here if data length -* is 1 byte (total length is 2 bytes). -*/ -if (length ==2 ) -writeccr (i2c,CCR_MIEN |CCR_MEN |CCR_MSTA - |CCR_TXAK ); - } - data[i] = byte; +if (status &CSR_MIF ) { +writeb (0 , i2c->base +MPC_I2C_SR ); +mpc_i2c_do_intr (i2c, status); +return IRQ_HANDLED ; } +return IRQ_NONE ; +} + +static void mpc_i2c_wait_for_completion (struct mpc_i2c *i2c) +{ +long time_left; -return length; + time_left =wait_event_timeout (i2c->waitq, !i2c->block, i2c->adap.timeout); + +if (!time_left) + i2c->rc = -ETIMEDOUT ; +else if (time_left <0 ) + i2c->rc = time_left; } -static int mpc_xfer (struct i2c_adapter *adap,struct i2c_msg *msgs,int num) +static int mpc_i2c_execute_msg (struct mpc_i2c *i2c) { -struct i2c_msg *pmsg; -int i; -int ret =0 ; -unsigned long orig_jiffies = jiffies; -struct mpc_i2c *i2c =i2c_get_adapdata (adap); +unsigned long orig_jiffies; +unsigned long flags; -mpc_i2c_start (i2c); +spin_lock_irqsave (&i2c->lock, flags); -/* Allow bus up to 1s to become not busy */ -while (readb (i2c->base +MPC_I2C_SR ) &CSR_MBB ) { -if (signal_pending (current)) { -dev_dbg (i2c->dev,"Interrupted\n" ); -writeccr (i2c,0 ); -return -EINTR ; - } -if (time_after (jiffies, orig_jiffies +HZ )) { -u8 status =readb (i2c->base +MPC_I2C_SR ); + i2c->curr_msg =0 ; + i2c->rc =0 ; + i2c->byte_posn =0 ; + i2c->block =1 ; + i2c->action =MPC_I2C_ACTION_START ; -dev_dbg (i2c->dev,"timeout\n" ); -if ((status & (CSR_MCF |CSR_MBB |CSR_RXAK )) !=0 ) { -writeb (status & ~CSR_MAL , - i2c->base +MPC_I2C_SR ); -i2c_recover_bus (&i2c->adap); - } -return -EIO ; - } -schedule (); - } + i2c->cntl_bits =CCR_MEN |CCR_MIEN ; +writeb (0 , i2c->base +MPC_I2C_SR ); +writeccr (i2c, i2c->cntl_bits); + +mpc_i2c_do_action (i2c); + +spin_unlock_irqrestore (&i2c->lock, flags); + +mpc_i2c_wait_for_completion (i2c); + +if (i2c->rc == -EIO || i2c->rc == -EAGAIN || i2c->rc == -ETIMEDOUT ) +i2c_recover_bus (&i2c->adap); -for (i =0 ; ret >=0 && i < num; i++) { - pmsg = &msgs[i]; -dev_dbg (i2c->dev, -"Doing %s %d bytes to 0x%02x - %d of %d messages\n" , - pmsg->flags &I2C_M_RD ?"read" :"write" , - pmsg->len, pmsg->addr, i +1 , num); -if (pmsg->flags &I2C_M_RD ) { -bool recv_len = pmsg->flags &I2C_M_RECV_LEN ; - - ret =mpc_read (i2c, pmsg->addr, pmsg->buf, pmsg->len, i, - recv_len); -if (recv_len && ret >0 ) - pmsg->len = ret; - }else { - ret = -mpc_write (i2c, pmsg->addr, pmsg->buf, pmsg->len, i); - } - } -mpc_i2c_stop (i2c);/* Initiate STOP */ orig_jiffies = jiffies;/* Wait until STOP is seen, allow up to 1 s */ while (readb (i2c->base +MPC_I2C_SR ) &CSR_MBB ) {
@@ -612,7 +631,35 @@ static int mpc_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs, int num) }cond_resched (); } -return (ret <0 ) ? ret : num; + +return i2c->rc; +} + +static int mpc_xfer (struct i2c_adapter *adap,struct i2c_msg *msgs,int num) +{ +int rc, ret = num; +struct mpc_i2c *i2c =i2c_get_adapdata (adap); +int i; + +dev_dbg (i2c->dev,"%s: num = %d\n" , __func__, num); +for (i =0 ; i < num; i++) +dev_dbg (i2c->dev," addr = %02x, flags = %02x, len = %d, %*ph\n" , + msgs[i].addr, msgs[i].flags, msgs[i].len, + msgs[i].flags &I2C_M_RD ?0 : msgs[i].len, + msgs[i].buf); + +WARN_ON (i2c->msgs !=NULL ); + i2c->msgs = msgs; + i2c->num_msgs = num; + + rc =mpc_i2c_execute_msg (i2c); +if (rc <0 ) + ret = rc; + + i2c->num_msgs =0 ; + i2c->msgs =NULL ; + +return ret; }static u32 mpc_functionality (struct i2c_adapter *adap)
@@ -667,7 +714,8 @@ static int fsl_i2c_probe(struct platform_device *op) i2c->dev = &op->dev;/* for debug and error output */ -init_waitqueue_head (&i2c->queue); +init_waitqueue_head (&i2c->waitq); +spin_lock_init (&i2c->lock); i2c->base =devm_platform_ioremap_resource (op,0 );if (IS_ERR (i2c->base)) {
Signature · 1 line
2.31.1
Re: [PATCH v3 2/4] i2c: mpc: Interrupt driven transfer
Quoted 7 lines · Chris Packham
The fsl-i2c controller will generate an interrupt after every byte transferred. Make use of this interrupt to drive a state machine which allows the next part of a transfer to happen as soon as the interrupt is received. This is particularly helpful with SMBUS devices like the LM81 which will timeout if we take too long between bytes in a transfer.
Signed-off-by Chris Packham <[email protected]>
Thanks for the update. I'll wait a little, because IIRC Andy wanted to review.
Re: [PATCH v3 2/4] i2c: mpc: Interrupt driven transfer
In reply to Chris PackhamQuoted 6 lines · Chris Packham
The fsl-i2c controller will generate an interrupt after every byte transferred. Make use of this interrupt to drive a state machine which allows the next part of a transfer to happen as soon as the interrupt is received. This is particularly helpful with SMBUS devices like the LM81 which will timeout if we take too long between bytes in a transfer.
Also see my other comments below.
...
Quoted 1 line
+// SPDX-License-Identifier: GPL-2.0
I think it is better to split this with a removal of old stuff and updating a copyright notice and go as a last one in the series.
...
Quoted 1 line
+static char *action_str[] = {
static const char * const action_str[]
Quoted 8 lines
drivers/i2c/busses/i2c-mpc.c +8
-0+"invalid" , +"start" , +"restart" , +"read begin" , +"read" , +"write" , + "stop", +};
...
drivers/i2c/busses/i2c-mpc.c static void mpc_i2c_setup_8xxx(struct device_node *node, · line 432
Quoted 2 lines
drivers/i2c/busses/i2c-mpc.c +2
-0+dev_dbg (i2c->dev,"%s: action = %s\n" , __func__, + action_str[i2c->action]);
Drop useless __func__. With Dynamic Debug enabled it can be turned on and off at run time.
...
Quoted 1 line
+ /* Generate txack on next to last byte */
Tx ACK ? Ditto for other comments.
...
drivers/i2c/busses/i2c-mpc.c static void mpc_i2c_setup_8xxx(struct device_node *node, · line 497
Quoted 2 lines
drivers/i2c/busses/i2c-mpc.c +2
-0+dev_dbg (i2c->dev,"%s: %s %02x\n" , __func__, + action_str[i2c->action], byte);
You already printed action. Anything changed?
Review
Quoted 2 lines
(quoted diff) +2
-0+ dev_dbg(i2c->dev, "%s: %s %02x\n", __func__, + action_str[i2c->action], msg->buf[i2c->byte_posn]);
Deduplicate this. Perhaps at the end of switch-case print once with whatever temporary variable value you want to.
...
drivers/i2c/busses/i2c-mpc.c static void mpc_i2c_setup_8xxx(struct device_node *node, · line 513
Quoted 2 lines
drivers/i2c/busses/i2c-mpc.c +2
-0+case MPC_I2C_ACTION_INVALID : +default :
Does the first one deserve loud WARN?
Otherwise, why is it defined at all?
Quoted 2 lines
drivers/i2c/busses/i2c-mpc.c +2
-0+WARN (1 ,"Unexpected action %d\n" , i2c->action); +break ;
...
drivers/i2c/busses/i2c-mpc.c static void mpc_i2c_setup_8xxx(struct device_node *node, · line 540
Quoted 4 lines
drivers/i2c/busses/i2c-mpc.c +2
-0+static void mpc_i2c_do_intr (struct mpc_i2c *i2c,u8 status) { +spin_lock_irqsave (&i2c->lock, flags);
Why _irqsave?
...
Quoted 1 line
+
dev_dbg(i2c->dev, "arbiritration lost\n");
arbitration
...
drivers/i2c/busses/i2c-mpc.c static void mpc_i2c_setup_8xxx(struct device_node *node, · line 555
Quoted 2 lines
drivers/i2c/busses/i2c-mpc.c +2
-0+if (i2c->expect_rxack && (status &CSR_RXAK )) { +dev_dbg (i2c->dev,"no RXAK\n" );
You see, you have to be consistent in comments and messages. Either use TXAK/RXAK, or more verbose 'Tx ACK/Rx ACK' everywhere.
...
Quoted 1 line
+out:
out_unlock:
Quoted 1 line
+
spin_unlock_irqrestore(&i2c->lock, flags);
...
drivers/i2c/busses/i2c-mpc.c static void mpc_i2c_setup_8xxx(struct device_node *node,
Quoted 5 lines
drivers/i2c/busses/i2c-mpc.c +4
-0+static irqreturn_t mpc_i2c_isr (int irq,void *dev_id) +{ +struct mpc_i2c *i2c = dev_id; +u8 status =readb (i2c->base +MPC_I2C_SR );
I would split this assignment, so it will be closer to its user.
Quoted 7 lines
drivers/i2c/busses/i2c-mpc.c +6
-0+if (status &CSR_MIF ) { +writeb (0 , i2c->base +MPC_I2C_SR ); +mpc_i2c_do_intr (i2c, status); +return IRQ_HANDLED ; } +return IRQ_NONE ; +}
...
drivers/i2c/busses/i2c-mpc.c static void mpc_i2c_setup_8xxx(struct device_node *node, · line 608
Quoted 3 lines
drivers/i2c/busses/i2c-mpc.c +2
-0+ time_left =wait_event_timeout (i2c->waitq, !i2c->block, i2c->adap.timeout); +
No need for a blank line here.
drivers/i2c/busses/i2c-mpc.c static void mpc_i2c_setup_8xxx(struct device_node *node,
Quoted 4 lines
drivers/i2c/busses/i2c-mpc.c +3
-0+if (!time_left) + i2c->rc = -ETIMEDOUT ; +else if (time_left <0 )
Redundant 'else'
Quoted 1 line
+
i2c->rc=time_left;
Can't you return an error code from here, rather than injecting it somewhere where it doesn't belong to?
Quoted 1 line
}
--
With Best Regards,
Andy Shevchenko
Chris Packham
maintainerRe: [PATCH v3 2/4] i2c: mpc: Interrupt driven transfer
Quoted 9 lines · Andy Shevchenko
Quoted 5 lines · Chris Packham
The fsl-i2c controller will generate an interrupt after every byte transferred. Make use of this interrupt to drive a state machine which allows the next part of a transfer to happen as soon as the interrupt is received. This is particularly helpful with SMBUS devices like the LM81 which will timeout if we take too long between bytes in a transfer.
Also see my other comments below.
Quoted 7 lines
...
Quoted 1 line
+// SPDX-License-Identifier: GPL-2.0
I think it is better to split this with a removal of old stuff and updating a copyright notice and go as a last one in the series.
...
Have split out into new patch.
Quoted 2 lines
Quoted 1 line
+static char *action_str[] = {
static const char * const action_str[]
Ack.
Quoted 8 lines
drivers/i2c/busses/i2c-mpc.c line 76
Quoted 8 lines
drivers/i2c/busses/i2c-mpc.c +8
-0+"invalid" , +"start" , +"restart" , +"read begin" , +"read" , +"write" , + "stop", +};
Quoted 6 lines
...
drivers/i2c/busses/i2c-mpc.c static void mpc_i2c_setup_8xxx(struct device_node *node, · line 432
Quoted 2 lines
drivers/i2c/busses/i2c-mpc.c +2
-0+dev_dbg (i2c->dev,"%s: action = %s\n" , __func__, + action_str[i2c->action]);Drop useless
__func__. With Dynamic Debug enabled it can be turned on and off at run time.
Ack. Other instances of __func__ also.
Quoted 7 lines
...
Quoted 1 line
+ /* Generate txack on next to last byte */
Tx ACK ? Ditto for other comments.
...
ACK.
Quoted 4 lines
drivers/i2c/busses/i2c-mpc.c static void mpc_i2c_setup_8xxx(struct device_node *node, · line 497
Quoted 3 lines
drivers/i2c/busses/i2c-mpc.c +2
-0+dev_dbg (i2c->dev,"%s: %s %02x\n" , __func__, + action_str[i2c->action], byte);You already printed action. Anything changed?
It's mainly the addition of the byte read. I couldn't figure out a sensible way of always printing the action then appending the data in the read/write case. Open to suggestions.
Quoted 7 lines
Review
Quoted 3 lines
(quoted diff) +2
-0+ dev_dbg(i2c->dev, "%s: %s %02x\n", __func__, + action_str[i2c->action], msg->buf[i2c->byte_posn]);Deduplicate this. Perhaps at the end of switch-case print once with whatever temporary variable value you want to.
...
I thought about this but decided not to because in the write case it's printed before going to hardware and in the read case it's after. If I moved it after the case I'd have to use something other than i2c->byte_posn which seemed error prone.
Quoted 5 lines
drivers/i2c/busses/i2c-mpc.c static void mpc_i2c_setup_8xxx(struct device_node *node, · line 513
Quoted 3 lines
drivers/i2c/busses/i2c-mpc.c +2
-0+case MPC_I2C_ACTION_INVALID : +default :Does the first one deserve loud WARN?
Otherwise, why is it defined at all?
I added MPC_I2C_ACTION_INVALID to make sure that a value of 0 was not something that would naturally happen via a zeroed initialization. I could probably achieve the same thing by making MPC_I2C_ACTION_START = 1.
Quoted 10 lines
Quoted 2 lines
drivers/i2c/busses/i2c-mpc.c +2
-0+WARN (1 ,"Unexpected action %d\n" , i2c->action); +break ;...
drivers/i2c/busses/i2c-mpc.c static void mpc_i2c_setup_8xxx(struct device_node *node, · line 540
Quoted 3 lines
drivers/i2c/busses/i2c-mpc.c +2
-0+static void mpc_i2c_do_intr (struct mpc_i2c *i2c,u8 status) { +spin_lock_irqsave (&i2c->lock, flags);Why
_irqsave?...
Primarily because it's the only one I've ever used and it was the one similar i2c drivers used when I started this work. I see they've now been updated so I don't think there will be a problem switching to spin_lock().
Quoted 2 lines
Quoted 1 line
+
dev_dbg(i2c->dev, "arbiritration lost\n");arbitration
Ack.
Quoted 8 lines
...
drivers/i2c/busses/i2c-mpc.c static void mpc_i2c_setup_8xxx(struct device_node *node, · line 555
Quoted 2 lines
drivers/i2c/busses/i2c-mpc.c +2
-0+if (i2c->expect_rxack && (status &CSR_RXAK )) { +dev_dbg (i2c->dev,"no RXAK\n" );You see, you have to be consistent in comments and messages. Either use TXAK/RXAK, or more verbose 'Tx ACK/Rx ACK' everywhere.
...
Updated to "Rx ACK". I think I've got them all now.
Quoted 4 lines
Quoted 2 lines
+out:
out_unlock:
Quoted 8 lines
Quoted 1 line
+
spin_unlock_irqrestore(&i2c->lock, flags);...
drivers/i2c/busses/i2c-mpc.c static void mpc_i2c_setup_8xxx(struct device_node *node, · line 570
Quoted 4 lines
drivers/i2c/busses/i2c-mpc.c +4
-0+static irqreturn_t mpc_i2c_isr (int irq,void *dev_id) +{ +struct mpc_i2c *i2c = dev_id; +u8 status =readb (i2c->base +MPC_I2C_SR );I would split this assignment, so it will be closer to its user.
Ack.
Quoted 7 lines
drivers/i2c/busses/i2c-mpc.c static void mpc_i2c_setup_8xxx(struct device_node *node, · line 578
Quoted 7 lines
drivers/i2c/busses/i2c-mpc.c +6
-0+if (status &CSR_MIF ) { +writeb (0 , i2c->base +MPC_I2C_SR ); +mpc_i2c_do_intr (i2c, status); +return IRQ_HANDLED ; } +return IRQ_NONE ; +}
Quoted 5 lines
...
Review
Quoted 2 lines
(quoted diff) +2
-0+ time_left = wait_event_timeout(i2c->waitq, !i2c->block, i2c->adap.timeout); +No need for a blank line here.
Ack.
Quoted 4 lines
drivers/i2c/busses/i2c-mpc.c static void mpc_i2c_setup_8xxx(struct device_node *node, · line 588
Quoted 3 lines
drivers/i2c/busses/i2c-mpc.c +3
-0+if (!time_left) + i2c->rc = -ETIMEDOUT ; +else if (time_left <0 )Redundant 'else'
Ack.
Quoted 3 lines
Quoted 1 line
+
i2c->rc=time_left;Can't you return an error code from here, rather than injecting it somewhere where it doesn't belong to?
Yes I think so. If I make mpc_i2c_wait_for_completion() return an int then have mpc_i2c_execute_msg() check it and set i2c->rc if needed.
Quoted 4 lines
Quoted 1 line
}--
With Best Regards,
Andy Shevchenko
Chris Packham
maintainerRe: [PATCH v3 2/4] i2c: mpc: Interrupt driven transfer
Quoted 14 lines · Chris Packham
Quoted 12 lines · Andy Shevchenko
Quoted 6 lines · Chris Packham
The fsl-i2c controller will generate an interrupt after every byte transferred. Make use of this interrupt to drive a state machine which allows the next part of a transfer to happen as soon as the interrupt is
received. This is particularly helpful with SMBUS devices like the LM81 which will timeout if we take too long between bytes in a transfer.Also see my other comments below.
...
...
Quoted 11 lines
Quoted 4 lines
Quoted 3 lines
+ dev_dbg(i2c->dev, "%s: %s %02x\n",
__func__, + action_str[i2c->action], byte);You already printed action. Anything changed?
It's mainly the addition of the byte read. I couldn't figure out a sensible way of always printing the action then appending the data in the read/write case. Open to suggestions.
Quoted 4 lines
Quoted 4 lines
+ dev_dbg(i2c->dev, "%s: %s %02x\n",
__func__, + action_str[i2c->action], msg->buf[i2c->byte_posn]);
Quoted 8 lines
Quoted 4 lines
Deduplicate this. Perhaps at the end of switch-case print once with whatever temporary variable value you want to.
...
I thought about this but decided not to because in the write case it's printed before going to hardware and in the read case it's after. If I moved it after the case I'd have to use something other than
i2c->byte_posnwhich seemed error prone.
I looked at a few options for this. Avoiding repeating the action is doable but I feel it needs to be replaced by something otherwise it's just a random byte value in the output (e.g. "buf = "/"byte = "). Rolling everything into a single line of output seems really hard to do to cover all the possible actions.
Completely deduplicating this means I need to add code to store the action before the case and check it afterward which will run all the time. This seems overkill for the sake of avoiding duplicate code which is usually compiled out.
I'm erring on the side of just removing __func__ and leaving the rest as-is. Unless you feel really strongly that something else should be done.
One option not mentioned is to remove these two debug statements. I'd be OK with that.
Chris Packham
maintainerPATCH v3 3/4 i2c: mpc: Remove redundant NULL check
In reply to Chris PackhamIn mpc_i2c_get_fdr_8xxx div is assigned as we iterate through the mpc_i2c_dividers_8xxx array. By the time we exit the loop div will either have the value that matches the requested speed or be pointing at the last entry in mpc_i2c_dividers_8xxx. Checking for div being NULL after the loop is redundant so remove the check.
--- drivers/i2c/busses/i2c-mpc.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/i2c/busses/i2c-mpc.c +1 -1
@@ -377,7 +377,7 @@ static int mpc_i2c_get_fdr_8xxx(struct device_node *node, u32 clock, } *real_clk =fsl_get_sys_freq () / prescaler / div->divider; -return div ? (int )div->fdr : -EINVAL ; +return (int )div->fdr; } static void mpc_i2c_setup_8xxx(struct device_node *node,
Signature · 1 line
2.31.1
Wolfram Sang
AppliedRe: [PATCH v3 3/4] i2c: mpc: Remove redundant NULL check
Quoted 8 lines · Chris Packham
In
mpc_i2c_get_fdr_8xxxdiv is assigned as we iterate through thempc_i2c_dividers_8xxxarray. By the time we exit the loop div will either have the value that matches the requested speed or be pointing at the last entry inmpc_i2c_dividers_8xxx. Checking for div being NULL after the loop is redundant so remove the check.Reported-by Wolfram Sang <[email protected]>Signed-off-by Chris Packham <[email protected]>
Applied to for-next, thanks!
Chris Packham
maintainerPATCH v3 3/3 MAINTAINERS: Add Chris Packham as FREESCALE MPC I2C maintainer
In reply to Chris PackhamAdd Chris Packham as FREESCALE MPC I2C maintainer.
--- MAINTAINERS | 7 +++++++ 1 file changed, 7 insertions(+)
MAINTAINERS +7 -0
@@ -7135,6 +7135,13 @@ S: Maintained F: Documentation/devicetree/bindings/i2c/i2c-imx-lpi2c.yaml F: drivers/i2c/busses/i2c-imx-lpi2c.c +FREESCALE MPC I2C DRIVER +M: Chris Packham <[email protected]> +L: [email protected] +S: Maintained +F: Documentation/devicetree/bindings/i2c/i2c-mpc.yaml +F: drivers/i2c/busses/i2c-mpc.c + FREESCALE QORIQ DPAA ETHERNET DRIVER M: Madalin Bucur <[email protected]> L: [email protected]
Signature · 1 line
2.31.1
Chris Packham
maintainerRe: [PATCH v3 3/3] MAINTAINERS: Add Chris Packham as FREESCALE MPC I2C maintainer
Quoted 3 lines · Chris Packham
Add Chris Packham as FREESCALE MPC I2C maintainer.
Signed-off-by Chris Packham <[email protected]>
Sorry for the duplicate. I had existing output from an earlier invocation of git format-patch lying around. "[PATCH v3 4/4]MAINTAINERS: ..." is the one I intended to send (although the content is the same).
Quoted 22 lines
--- MAINTAINERS | 7 +++++++ 1 file changed, 7 insertions(+)MAINTAINERS +7
-0@@ -7135,6 +7135,13 @@ S: Maintained F: Documentation/devicetree/bindings/i2c/i2c-imx-lpi2c.yaml F: drivers/i2c/busses/i2c-imx-lpi2c.c +FREESCALE MPC I2C DRIVER +M: Chris Packham <[email protected]> +L: [email protected] +S: Maintained +F: Documentation/devicetree/bindings/i2c/i2c-mpc.yaml +F: drivers/i2c/busses/i2c-mpc.c + FREESCALE QORIQ DPAA ETHERNET DRIVER M: Madalin Bucur <[email protected]> L: [email protected]
Re: [PATCH v3 3/3] MAINTAINERS: Add Chris Packham as FREESCALE MPC I2C maintainer
Quoted 8 lines · Chris Packham
Quoted 3 lines · Chris Packham
Add Chris Packham as FREESCALE MPC I2C maintainer.
Signed-off-by Chris Packham <[email protected]>Sorry for the duplicate. I had existing output from an earlier invocation of git format-patch lying around. "[PATCH v3 4/4]
MAINTAINERS: ..." is the one I intended to send (although the content is the same).
For myself I wrote a script [1] to send patches :-) No more duplicates or strangers in the Cc list.
[1]: https://github.com/andy-shev/home-bin-tools/blob/master/ge2maintainer.sh
Signature · 2 lines
With Best Regards, Andy Shevchenko
Chris Packham
maintainerPATCH v3 4/4 MAINTAINERS: Add Chris Packham as FREESCALE MPC I2C maintainer
In reply to Chris PackhamAdd Chris Packham as FREESCALE MPC I2C maintainer.
--- MAINTAINERS | 7 +++++++ 1 file changed, 7 insertions(+)
MAINTAINERS +7 -0
@@ -7135,6 +7135,13 @@ S: Maintained F: Documentation/devicetree/bindings/i2c/i2c-imx-lpi2c.yaml F: drivers/i2c/busses/i2c-imx-lpi2c.c +FREESCALE MPC I2C DRIVER +M: Chris Packham <[email protected]> +L: [email protected] +S: Maintained +F: Documentation/devicetree/bindings/i2c/i2c-mpc.yaml +F: drivers/i2c/busses/i2c-mpc.c + FREESCALE QORIQ DPAA ETHERNET DRIVER M: Madalin Bucur <[email protected]> L: [email protected]
Signature · 1 line
2.31.1
Wolfram Sang
AppliedRe: [PATCH v3 4/4] MAINTAINERS: Add Chris Packham as FREESCALE MPC I2C maintainer
Quoted 3 lines · Chris Packham
Add Chris Packham as FREESCALE MPC I2C maintainer.
Signed-off-by Chris Packham <[email protected]>
Applied to for-next, thanks for stepping up, much appreciated!