Hi Bhupesh, On Wednesday 14 March 2012 04:29:23 Bhupesh SHARMA wrote: > > -----Original Message----- > > From: Stefan Roese [mailto:sr@xxxxxxx] > > Sent: Tuesday, March 13, 2012 9:24 PM > > To: linux-i2c@xxxxxxxxxxxxxxx > > Cc: spear-devel; ben-linux@xxxxxxxxx > > Subject: [PATCH] i2c: designware: Add support for 16bit register access > > > > The STM SPEAr platform can only access the i2c controller register > > via 16bit read/write functions. This patch adds support to > > automatically detect this 16bit access mode. > > > > Signed-off-by: Stefan Roese <sr@xxxxxxx> > > --- > > > > drivers/i2c/busses/i2c-designware-core.c | 22 ++++++++++++++++++++-- > > drivers/i2c/busses/i2c-designware-core.h | 1 + > > 2 files changed, 21 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/i2c/busses/i2c-designware-core.c > > b/drivers/i2c/busses/i2c-designware-core.c > > index df87992..d1facbc 100644 > > --- a/drivers/i2c/busses/i2c-designware-core.c > > +++ b/drivers/i2c/busses/i2c-designware-core.c > > @@ -164,7 +164,14 @@ static char *abort_sources[] = { > > > > u32 dw_readl(struct dw_i2c_dev *dev, int offset) > > { > > > > - u32 value = readl(dev->base + offset); > > + u32 value; > > + > > + if (dev->access_16bit) { > > + value = readw(dev->base + offset) | > > + (readw(dev->base + offset + 2) << 16); > > + } else { > > + value = readl(dev->base + offset); > > + } > > We can do away with '{' parenthesis as these are single line > of code inside both the 'if-else' blocks. It's not a single-line statement. The first block extends spans 2 lines. At least that how I interpret this CodingStyle recommendation. > > if (dev->swab) > > > > return swab32(value); > > > > @@ -177,7 +184,12 @@ void dw_writel(struct dw_i2c_dev *dev, u32 b, int > > offset) > > > > if (dev->swab) > > > > b = swab32(b); > > > > - writel(b, dev->base + offset); > > + if (dev->access_16bit) { > > + writew((u16)b, dev->base + offset); > > + writew((u16)(b >> 16), dev->base + offset + 2); > > + } else { > > + writel(b, dev->base + offset); > > + } > > > > } > > > > static u32 > > > > @@ -258,6 +270,12 @@ int i2c_dw_init(struct dw_i2c_dev *dev) > > > > reg = DW_IC_COMP_TYPE_VALUE; > > > > } > > > > + /* Configure register access mode 16bit */ > > + if (reg == (DW_IC_COMP_TYPE_VALUE & 0x0000ffff)) { > > + dev->access_16bit = 1; > > Can we use a suitable macro for 0x0000ffff? Hmmm. Wouldn't that make it more complex? 0x0000ffff is easy to understand. A marco would "hide" this value. I would prefer to keep the value. > Also, if dev->access_16bit is bool we can simply set: > dev->access_16bit = true; > > more on that below... > > > + reg = DW_IC_COMP_TYPE_VALUE; > > + } > > + > > > > if (reg != DW_IC_COMP_TYPE_VALUE) { > > > > dev_err(dev->dev, "Unknown Synopsys component type: " > > > > "0x%08x\n", reg); > > > > diff --git a/drivers/i2c/busses/i2c-designware-core.h > > b/drivers/i2c/busses/i2c-designware-core.h > > index 02d1a2d..f5af101 100644 > > --- a/drivers/i2c/busses/i2c-designware-core.h > > +++ b/drivers/i2c/busses/i2c-designware-core.h > > @@ -83,6 +83,7 @@ struct dw_i2c_dev { > > > > u32 abort_source; > > int irq; > > int swab; > > > > + int access_16bit; > > ... > int?? Probably we are better off with making this as bool. I'm not a big fan of bool's. But I have no strong preference here. My reasoning here was consistency. As we already have "int swab" for a similar issue. So basically, I would prefer to not make the changes you suggested. But in the end its the decision of the maintainer(s). Thanks, Stefan -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
