Hi Stefan, > -----Original Message----- > From: Stefan Roese [mailto:sr@xxxxxxx] > Sent: Wednesday, March 14, 2012 1:28 PM > To: Bhupesh SHARMA > Cc: linux-i2c@xxxxxxxxxxxxxxx; spear-devel; ben-linux@xxxxxxxxx > Subject: Re: [PATCH] i2c: designware: Add support for 16bit register > access > > 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. ??? Breaking a single line of code into two for readability does not make them two separate executable statements. As per CodingStyle recommendation: Do not unnecessarily use braces where a single statement will do. if (condition) action(); So, please modify your patch as braces here waste precious screen space and reduce readability. > > > 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. Using a macro doesn't make things 'more complex', but more readable. Magic numbers must be avoided at all cost. A better named MACRO is always better (for anyone else reading the code) than a magic number like 0x0000ffff. > > 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. If we have not done it earlier, doesn't mean that we repeat the same mistake again. There is no reason to take access_16bit as an int when a bool will suffice. This wastes storage and on some platforms (which have real crunch of memory), such saving is critical. > So basically, I would prefer to not make the changes you suggested. But > in the > end its the decision of the maintainer(s). > Linux is a collaborative world and patches can be reviewed by literally anyone :) I am sure the maintainer(s) will find my comments worth adding in your patch.. Regards, Bhupesh -- 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
