Hi Bhupesh,
On Wednesday 14 March 2012 04:29:23 Bhupesh SHARMA wrote:
> > -----Original Message-----
> > From: Stefan Roese [mailto:s...@denx.de]
> > Sent: Tuesday, March 13, 2012 9:24 PM
> > To: linux-i2c@vger.kernel.org
> > Cc: spear-devel; ben-li...@fluff.org
> > 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 <s...@denx.de>
> > ---
> >
> > 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 majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
