Hi Jean-Jacques, On 22/11/2013 09:58, jean-jacques hiblot wrote: > * removed unneeded 'volatile' qualifiers and casts > * use the dev_dbg, dev_err etc. instead of printk > * removed unneeded members for the driver's private data > > Signed-off-by: jean-jacques hiblot <[email protected]>
This one was easy to review, and I didn't find any thing to say, so you can add my: Reviewed-by: Gregory CLEMENT <[email protected]> Thanks, Gregory > --- > drivers/i2c/busses/i2c-ibm_iic.c | 109 > ++++++++++++++++++++------------------- > drivers/i2c/busses/i2c-ibm_iic.h | 9 ++-- > 2 files changed, 61 insertions(+), 57 deletions(-) > > diff --git a/drivers/i2c/busses/i2c-ibm_iic.c > b/drivers/i2c/busses/i2c-ibm_iic.c > index ff3caa0..9cdef65 100644 > --- a/drivers/i2c/busses/i2c-ibm_iic.c > +++ b/drivers/i2c/busses/i2c-ibm_iic.c > @@ -69,21 +69,23 @@ MODULE_PARM_DESC(iic_force_fast, "Force fast mode (400 > kHz)"); > #endif > > #if DBG_LEVEL > 0 > -# define DBG(f,x...) printk(KERN_DEBUG "ibm-iic" f, ##x) > +# define DBG(dev, f, x...) dev_dbg(dev->adap.dev.parent, f, ##x) > #else > -# define DBG(f,x...) ((void)0) > +# define DBG(dev, f, x...) ((void)0) > #endif > #if DBG_LEVEL > 1 > -# define DBG2(f,x...) DBG(f, ##x) > +# define DBG2(dev, f, x...) DBG(dev, f, ##x) > #else > -# define DBG2(f,x...) ((void)0) > +# define DBG2(dev, f, x...) ((void)0) > #endif > #if DBG_LEVEL > 2 > static void dump_iic_regs(const char* header, struct ibm_iic_private* dev) > { > - volatile struct iic_regs __iomem *iic = dev->vaddr; > - printk(KERN_DEBUG "ibm-iic%d: %s\n", dev->idx, header); > - printk(KERN_DEBUG > + struct iic_regs __iomem *iic = dev->vaddr; > + struct device *device = dev.adap.dev.parent; > + > + dev_dbg(device, ": %s\n", header); > + dev_dbg(device, > " cntl = 0x%02x, mdcntl = 0x%02x\n" > " sts = 0x%02x, extsts = 0x%02x\n" > " clkdiv = 0x%02x, xfrcnt = 0x%02x\n" > @@ -133,9 +135,9 @@ static inline void iic_interrupt_mode(struct > ibm_iic_private* dev, int enable) > */ > static void iic_dev_init(struct ibm_iic_private* dev) > { > - volatile struct iic_regs __iomem *iic = dev->vaddr; > + struct iic_regs __iomem *iic = dev->vaddr; > > - DBG("%d: init\n", dev->idx); > + DBG(dev, "init\n"); > > /* Clear master address */ > out_8(&iic->lmadr, 0); > @@ -178,11 +180,11 @@ static void iic_dev_init(struct ibm_iic_private* dev) > */ > static void iic_dev_reset(struct ibm_iic_private* dev) > { > - volatile struct iic_regs __iomem *iic = dev->vaddr; > + struct iic_regs __iomem *iic = dev->vaddr; > int i; > u8 dc; > > - DBG("%d: soft reset\n", dev->idx); > + DBG(dev, "soft reset\n"); > DUMP_REGS("reset", dev); > > /* Place chip in the reset state */ > @@ -191,7 +193,7 @@ static void iic_dev_reset(struct ibm_iic_private* dev) > /* Check if bus is free */ > dc = in_8(&iic->directcntl); > if (!DIRCTNL_FREE(dc)){ > - DBG("%d: trying to regain bus control\n", dev->idx); > + DBG(dev, "trying to regain bus control\n"); > > /* Try to set bus free state */ > out_8(&iic->directcntl, DIRCNTL_SDAC | DIRCNTL_SCC); > @@ -226,7 +228,7 @@ static void iic_dev_reset(struct ibm_iic_private* dev) > */ > > /* Wait for SCL and/or SDA to be high */ > -static int iic_dc_wait(volatile struct iic_regs __iomem *iic, u8 mask) > +static int iic_dc_wait(struct iic_regs __iomem *iic, u8 mask) > { > unsigned long x = jiffies + HZ / 28 + 2; > while ((in_8(&iic->directcntl) & mask) != mask){ > @@ -239,19 +241,18 @@ static int iic_dc_wait(volatile struct iic_regs __iomem > *iic, u8 mask) > > static int iic_smbus_quick(struct ibm_iic_private* dev, const struct > i2c_msg* p) > { > - volatile struct iic_regs __iomem *iic = dev->vaddr; > + struct iic_regs __iomem *iic = dev->vaddr; > const struct i2c_timings* t = &timings[dev->fast_mode ? 1 : 0]; > u8 mask, v, sda; > int i, res; > > /* Only 7-bit addresses are supported */ > if (unlikely(p->flags & I2C_M_TEN)){ > - DBG("%d: smbus_quick - 10 bit addresses are not supported\n", > - dev->idx); > + DBG(dev, "smbus_quick - 10 bit addresses are not supported\n"); > return -EINVAL; > } > > - DBG("%d: smbus_quick(0x%02x)\n", dev->idx, p->addr); > + DBG(dev, "smbus_quick(0x%02x)\n", p->addr); > > /* Reset IIC interface */ > out_8(&iic->xtcntlss, XTCNTLSS_SRST); > @@ -304,7 +305,7 @@ static int iic_smbus_quick(struct ibm_iic_private* dev, > const struct i2c_msg* p) > > ndelay(t->buf); > > - DBG("%d: smbus_quick -> %s\n", dev->idx, res ? "NACK" : "ACK"); > + DBG(dev, "smbus_quick -> %s\n", res ? "NACK" : "ACK"); > out: > /* Remove reset */ > out_8(&iic->xtcntlss, 0); > @@ -314,7 +315,7 @@ out: > > return res; > err: > - DBG("%d: smbus_quick - bus is stuck\n", dev->idx); > + DBG(dev, "smbus_quick - bus is stuck\n"); > res = -EREMOTEIO; > goto out; > } > @@ -325,10 +326,10 @@ err: > static irqreturn_t iic_handler(int irq, void *dev_id) > { > struct ibm_iic_private* dev = (struct ibm_iic_private*)dev_id; > - volatile struct iic_regs __iomem *iic = dev->vaddr; > + struct iic_regs __iomem *iic = dev->vaddr; > > - DBG2("%d: irq handler, STS = 0x%02x, EXTSTS = 0x%02x\n", > - dev->idx, in_8(&iic->sts), in_8(&iic->extsts)); > + DBG2(dev, "irq handler, STS = 0x%02x, EXTSTS = 0x%02x\n", > + in_8(&iic->sts), in_8(&iic->extsts)); > > /* Acknowledge IRQ and wakeup iic_wait_for_tc */ > out_8(&iic->sts, STS_IRQA | STS_SCMP); > @@ -343,10 +344,10 @@ static irqreturn_t iic_handler(int irq, void *dev_id) > */ > static int iic_xfer_result(struct ibm_iic_private* dev) > { > - volatile struct iic_regs __iomem *iic = dev->vaddr; > + struct iic_regs __iomem *iic = dev->vaddr; > > if (unlikely(in_8(&iic->sts) & STS_ERR)){ > - DBG("%d: xfer error, EXTSTS = 0x%02x\n", dev->idx, > + DBG(dev, "xfer error, EXTSTS = 0x%02x\n", > in_8(&iic->extsts)); > > /* Clear errors and possible pending IRQs */ > @@ -362,7 +363,7 @@ static int iic_xfer_result(struct ibm_iic_private* dev) > * state, the only way out - soft reset. > */ > if ((in_8(&iic->extsts) & EXTSTS_BCS_MASK) != EXTSTS_BCS_FREE){ > - DBG("%d: bus is stuck, resetting\n", dev->idx); > + DBG(dev, "bus is stuck, resetting\n"); > iic_dev_reset(dev); > } > return -EREMOTEIO; > @@ -376,10 +377,10 @@ static int iic_xfer_result(struct ibm_iic_private* dev) > */ > static void iic_abort_xfer(struct ibm_iic_private* dev) > { > - volatile struct iic_regs __iomem *iic = dev->vaddr; > + struct iic_regs __iomem *iic = dev->vaddr; > unsigned long x; > > - DBG("%d: iic_abort_xfer\n", dev->idx); > + DBG(dev, "iic_abort_xfer\n"); > > out_8(&iic->cntl, CNTL_HMT); > > @@ -390,7 +391,7 @@ static void iic_abort_xfer(struct ibm_iic_private* dev) > x = jiffies + 2; > while ((in_8(&iic->extsts) & EXTSTS_BCS_MASK) != EXTSTS_BCS_FREE){ > if (time_after(jiffies, x)){ > - DBG("%d: abort timeout, resetting...\n", dev->idx); > + DBG(dev, "abort timeout, resetting...\n"); > iic_dev_reset(dev); > return; > } > @@ -408,7 +409,7 @@ static void iic_abort_xfer(struct ibm_iic_private* dev) > */ > static int iic_wait_for_tc(struct ibm_iic_private* dev){ > > - volatile struct iic_regs __iomem *iic = dev->vaddr; > + struct iic_regs __iomem *iic = dev->vaddr; > int ret = 0; > > if (dev->irq >= 0){ > @@ -417,9 +418,9 @@ static int iic_wait_for_tc(struct ibm_iic_private* dev){ > !(in_8(&iic->sts) & STS_PT), dev->adap.timeout); > > if (unlikely(ret < 0)) > - DBG("%d: wait interrupted\n", dev->idx); > + DBG(dev, "wait interrupted\n"); > else if (unlikely(in_8(&iic->sts) & STS_PT)){ > - DBG("%d: wait timeout\n", dev->idx); > + DBG(dev, "wait timeout\n"); > ret = -ETIMEDOUT; > } > } > @@ -429,13 +430,13 @@ static int iic_wait_for_tc(struct ibm_iic_private* dev){ > > while (in_8(&iic->sts) & STS_PT){ > if (unlikely(time_after(jiffies, x))){ > - DBG("%d: poll timeout\n", dev->idx); > + DBG(dev, "poll timeout\n"); > ret = -ETIMEDOUT; > break; > } > > if (unlikely(signal_pending(current))){ > - DBG("%d: poll interrupted\n", dev->idx); > + DBG(dev, "poll interrupted\n"); > ret = -ERESTARTSYS; > break; > } > @@ -448,7 +449,7 @@ static int iic_wait_for_tc(struct ibm_iic_private* dev){ > else > ret = iic_xfer_result(dev); > > - DBG2("%d: iic_wait_for_tc -> %d\n", dev->idx, ret); > + DBG2(dev, "iic_wait_for_tc -> %d\n", ret); > > return ret; > } > @@ -459,7 +460,7 @@ static int iic_wait_for_tc(struct ibm_iic_private* dev){ > static int iic_xfer_bytes(struct ibm_iic_private* dev, struct i2c_msg* pm, > int combined_xfer) > { > - volatile struct iic_regs __iomem *iic = dev->vaddr; > + struct iic_regs __iomem *iic = dev->vaddr; > char* buf = pm->buf; > int i, j, loops, ret = 0; > int len = pm->len; > @@ -475,14 +476,14 @@ static int iic_xfer_bytes(struct ibm_iic_private* dev, > struct i2c_msg* pm, > > if (!(cntl & CNTL_RW)) > for (j = 0; j < count; ++j) > - out_8((void __iomem *)&iic->mdbuf, *buf++); > + out_8(&iic->mdbuf, *buf++); > > if (i < loops - 1) > cmd |= CNTL_CHT; > else if (combined_xfer) > cmd |= CNTL_RPST; > > - DBG2("%d: xfer_bytes, %d, CNTL = 0x%02x\n", dev->idx, count, > cmd); > + DBG2(dev, "xfer_bytes, %d, CNTL = 0x%02x\n", count, cmd); > > /* Start transfer */ > out_8(&iic->cntl, cmd); > @@ -493,8 +494,8 @@ static int iic_xfer_bytes(struct ibm_iic_private* dev, > struct i2c_msg* pm, > if (unlikely(ret < 0)) > break; > else if (unlikely(ret != count)){ > - DBG("%d: xfer_bytes, requested %d, transferred %d\n", > - dev->idx, count, ret); > + DBG(dev, "xfer_bytes, requested %d, transferred %d\n", > + count, ret); > > /* If it's not a last part of xfer, abort it */ > if (combined_xfer || (i < loops - 1)) > @@ -506,7 +507,7 @@ static int iic_xfer_bytes(struct ibm_iic_private* dev, > struct i2c_msg* pm, > > if (cntl & CNTL_RW) > for (j = 0; j < count; ++j) > - *buf++ = in_8((void __iomem *)&iic->mdbuf); > + *buf++ = in_8(&iic->mdbuf); > } > > return ret > 0 ? 0 : ret; > @@ -517,10 +518,10 @@ static int iic_xfer_bytes(struct ibm_iic_private* dev, > struct i2c_msg* pm, > */ > static inline void iic_address(struct ibm_iic_private* dev, struct i2c_msg* > msg) > { > - volatile struct iic_regs __iomem *iic = dev->vaddr; > + struct iic_regs __iomem *iic = dev->vaddr; > u16 addr = msg->addr; > > - DBG2("%d: iic_address, 0x%03x (%d-bit)\n", dev->idx, > + DBG2(dev, "iic_address, 0x%03x (%d-bit)\n", > addr, msg->flags & I2C_M_TEN ? 10 : 7); > > if (msg->flags & I2C_M_TEN){ > @@ -553,10 +554,10 @@ static inline int iic_address_neq(const struct i2c_msg* > p1, > static int iic_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs, int num) > { > struct ibm_iic_private* dev = (struct > ibm_iic_private*)(i2c_get_adapdata(adap)); > - volatile struct iic_regs __iomem *iic = dev->vaddr; > + struct iic_regs __iomem *iic = dev->vaddr; > int i, ret = 0; > > - DBG2("%d: iic_xfer, %d msg(s)\n", dev->idx, num); > + DBG2(dev, "iic_xfer, %d msg(s)\n", num); > > if (!num) > return 0; > @@ -565,7 +566,7 @@ static int iic_xfer(struct i2c_adapter *adap, struct > i2c_msg *msgs, int num) > * Uhh, generic i2c layer is more suitable place for such code... > */ > if (unlikely(iic_invalid_address(&msgs[0]))){ > - DBG("%d: invalid address 0x%03x (%d-bit)\n", dev->idx, > + DBG(dev, "invalid address 0x%03x (%d-bit)\n", > msgs[0].addr, msgs[0].flags & I2C_M_TEN ? 10 : 7); > return -EINVAL; > } > @@ -578,19 +579,19 @@ static int iic_xfer(struct i2c_adapter *adap, struct > i2c_msg *msgs, int num) > */ > return iic_smbus_quick(dev, &msgs[0]); > } > - DBG("%d: invalid len %d in msg[%d]\n", dev->idx, > + DBG(dev, "invalid len %d in msg[%d]\n", > msgs[i].len, i); > return -EINVAL; > } > if (unlikely(iic_address_neq(&msgs[0], &msgs[i]))){ > - DBG("%d: invalid addr in msg[%d]\n", dev->idx, i); > + DBG(dev, "invalid addr in msg[%d]\n", i); > return -EINVAL; > } > } > > /* Check bus state */ > if (unlikely((in_8(&iic->extsts) & EXTSTS_BCS_MASK) != > EXTSTS_BCS_FREE)){ > - DBG("%d: iic_xfer, bus is not free\n", dev->idx); > + DBG(dev, "iic_xfer, bus is not free\n"); > > /* Usually it means something serious has happened. > * We *cannot* have unfinished previous transfer > @@ -603,7 +604,7 @@ static int iic_xfer(struct i2c_adapter *adap, struct > i2c_msg *msgs, int num) > iic_dev_reset(dev); > > if ((in_8(&iic->extsts) & EXTSTS_BCS_MASK) != EXTSTS_BCS_FREE){ > - DBG("%d: iic_xfer, bus is still not free\n", dev->idx); > + DBG(dev, "iic_xfer, bus is still not free\n"); > return -EREMOTEIO; > } > } > @@ -635,15 +636,17 @@ static const struct i2c_algorithm iic_algo = { > /* > * Calculates IICx_CLCKDIV value for a specific OPB clock frequency > */ > -static inline u8 iic_clckdiv(unsigned int opb) > +static inline u8 iic_clckdiv(struct ibm_iic_private *dev, unsigned int opb) > { > + struct device *device = dev->adap.dev.parent; > + > /* Compatibility kludge, should go away after all cards > * are fixed to fill correct value for opbfreq. > * Previous driver version used hardcoded divider value 4, > * it corresponds to OPB frequency from the range (40, 50] MHz > */ > if (!opb){ > - printk(KERN_WARNING "ibm-iic: using compatibility value for OPB > freq," > + dev_warn(device, "iic_clckdiv: using compatibility value for > OPB freq," > " fix your board specific setup\n"); > opb = 50000000; > } > @@ -652,7 +655,7 @@ static inline u8 iic_clckdiv(unsigned int opb) > opb /= 1000000; > > if (opb < 20 || opb > 150){ > - printk(KERN_WARNING "ibm-iic: invalid OPB clock frequency %u > MHz\n", > + dev_warn(device, "iic_clckdiv: invalid OPB clock frequency %u > MHz\n", > opb); > opb = opb < 20 ? 20 : 150; > } > @@ -733,7 +736,7 @@ static int iic_probe(struct platform_device *ofdev) > } > } > > - dev->clckdiv = iic_clckdiv(*freq); > + dev->clckdiv = iic_clckdiv(dev, *freq); > dev_dbg(&ofdev->dev, "clckdiv = %d\n", dev->clckdiv); > > /* Initialize IIC interface */ > diff --git a/drivers/i2c/busses/i2c-ibm_iic.h > b/drivers/i2c/busses/i2c-ibm_iic.h > index fdaa482..1efce5d 100644 > --- a/drivers/i2c/busses/i2c-ibm_iic.h > +++ b/drivers/i2c/busses/i2c-ibm_iic.h > @@ -25,8 +25,10 @@ > #include <linux/i2c.h> > > struct iic_regs { > - u16 mdbuf; > - u16 sbbuf; > + u8 mdbuf; > + u8 reserved1; > + u8 sbbuf; > + u8 reserved2; > u8 lmadr; > u8 hmadr; > u8 cntl; > @@ -44,9 +46,8 @@ struct iic_regs { > > struct ibm_iic_private { > struct i2c_adapter adap; > - volatile struct iic_regs __iomem *vaddr; > + struct iic_regs __iomem *vaddr; > wait_queue_head_t wq; > - int idx; > int irq; > int fast_mode; > u8 clckdiv; > -- Gregory Clement, Free Electrons Kernel, drivers, real-time and embedded Linux development, consulting, training and support. http://free-electrons.com -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
