[PATCH] i2c: designware: Add support for 16bit register access

[PATCH] i2c: designware: Add support for 16bit register access

[Stefan Roese]

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-ynQEQJNshbs@public.gmane.org>
---
 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);
+	}
 
 	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;
+		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;
 	struct i2c_adapter	adapter;
 	u32			functionality;
 	u32			master_cfg;
-- 
1.7.9.2

RE: [PATCH] i2c: designware: Add support for 16bit register access

[Bhupesh SHARMA]

Hi Stefan,

> -----Original Message-----
> From: Stefan Roese [mailto:sr-ynQEQJNshbs@public.gmane.org]
> Sent: Tuesday, March 13, 2012 9:24 PM
> To: linux-i2c-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> Cc: spear-devel; ben-linux-elnMNo+KYs3YtjvyW6yDsg@public.gmane.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 <sr-ynQEQJNshbs@public.gmane.org>
> ---
>  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.

>  	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?
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.

>  	struct i2c_adapter	adapter;
>  	u32			functionality;
>  	u32			master_cfg;
> --


Regards,
Bhupesh

Re: [PATCH] i2c: designware: Add support for 16bit register access

[Stefan Roese]

Hi Bhupesh,

On Wednesday 14 March 2012 04:29:23 Bhupesh SHARMA wrote:
> > -----Original Message-----
> > From: Stefan Roese [mailto:sr-ynQEQJNshbs@public.gmane.org]
> > Sent: Tuesday, March 13, 2012 9:24 PM
> > To: linux-i2c-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> > Cc: spear-devel; ben-linux-elnMNo+KYs3YtjvyW6yDsg@public.gmane.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 <sr-ynQEQJNshbs@public.gmane.org>
> > ---
> > 
> >  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

RE: [PATCH] i2c: designware: Add support for 16bit register access

[Bhupesh SHARMA]

Hi Stefan,

> -----Original Message-----
> From: Stefan Roese [mailto:sr-ynQEQJNshbs@public.gmane.org]
> Sent: Wednesday, March 14, 2012 1:28 PM
> To: Bhupesh SHARMA
> Cc: linux-i2c-u79uwXL29TY76Z2rM5mHXA@public.gmane.org; spear-devel; ben-linux-elnMNo+KYs3YtjvyW6yDsg@public.gmane.org
> 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-ynQEQJNshbs@public.gmane.org]
> > > Sent: Tuesday, March 13, 2012 9:24 PM
> > > To: linux-i2c-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> > > Cc: spear-devel; ben-linux-elnMNo+KYs3YtjvyW6yDsg@public.gmane.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 <sr-ynQEQJNshbs@public.gmane.org>
> > > ---
> > >
> > >  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

Re: [PATCH] i2c: designware: Add support for 16bit register access

[Jean Delvare]

On Wed, 14 Mar 2012 16:19:22 +0800, Bhupesh SHARMA wrote:
> > -----Original Message-----
> > From: Stefan Roese [mailto:sr-ynQEQJNshbs@public.gmane.org]
> > Sent: Wednesday, March 14, 2012 1:28 PM
> > To: Bhupesh SHARMA
> > Cc: linux-i2c-u79uwXL29TY76Z2rM5mHXA@public.gmane.org; spear-devel; ben-linux-elnMNo+KYs3YtjvyW6yDsg@public.gmane.org
> > Subject: Re: [PATCH] i2c: designware: Add support for 16bit register
> > access
> > 
> > On Wednesday 14 March 2012 04:29:23 Bhupesh SHARMA wrote:
> > > > -----Original Message-----
> > > > From: Stefan Roese [mailto:sr-ynQEQJNshbs@public.gmane.org]
> > > > Sent: Tuesday, March 13, 2012 9:24 PM
> > > > To: linux-i2c-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> > > > Cc: spear-devel; ben-linux-elnMNo+KYs3YtjvyW6yDsg@public.gmane.org
> > > > Subject: [PATCH] i2c: designware: Add support for 16bit register access
> > > > (...)
> > > > @@ -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.

I beg to disagree. Quite strongly, even.

For one thing, "at all cost" never holds in the real world. You always
have to make compromises.

For another, it only makes sense to define constants for values that
have a specific meaning. For example bits in a bit vector, or bit
masks. Here this is clearly not the case, so I fully agree with Stefan
that a define would make the code _less_ readable.

> > > > (...)
> > > > 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 :)

Likewise, everyone can send patches to address issues that bother them.
Want these ints turned into bools? Stop yelling at Stefan, and do it
yourself. If it is so critical, no doubt you'll find reviewers to ack
your patch and maintainers to apply it. And you'll even get the fame.

> I am sure the maintainer(s) will find my comments worth adding in your patch..  

I'm not in charge of this specific driver, so I can't speak for the
maintainer. And you shouldn't either...

Thanks,
-- 
Jean Delvare

RE: [PATCH] i2c: designware: Add support for 16bit register access

[Bhupesh SHARMA]

> -----Original Message-----
> From: Jean Delvare [mailto:khali-PUYAD+kWke1g9hUCZPvPmw@public.gmane.org]
> Sent: Wednesday, March 14, 2012 2:14 PM
> To: Bhupesh SHARMA
> Cc: Stefan Roese; linux-i2c-u79uwXL29TY76Z2rM5mHXA@public.gmane.org; spear-devel; ben-
> linux-elnMNo+KYs3YtjvyW6yDsg@public.gmane.org
> Subject: Re: [PATCH] i2c: designware: Add support for 16bit register
> access
> 
> On Wed, 14 Mar 2012 16:19:22 +0800, Bhupesh SHARMA wrote:
> > > -----Original Message-----
> > > From: Stefan Roese [mailto:sr-ynQEQJNshbs@public.gmane.org]
> > > Sent: Wednesday, March 14, 2012 1:28 PM
> > > To: Bhupesh SHARMA
> > > Cc: linux-i2c-u79uwXL29TY76Z2rM5mHXA@public.gmane.org; spear-devel; ben-linux-elnMNo+KYs3YtjvyW6yDsg@public.gmane.org
> > > Subject: Re: [PATCH] i2c: designware: Add support for 16bit
> register
> > > access
> > >
> > > On Wednesday 14 March 2012 04:29:23 Bhupesh SHARMA wrote:
> > > > > -----Original Message-----
> > > > > From: Stefan Roese [mailto:sr-ynQEQJNshbs@public.gmane.org]
> > > > > Sent: Tuesday, March 13, 2012 9:24 PM
> > > > > To: linux-i2c-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> > > > > Cc: spear-devel; ben-linux-elnMNo+KYs3YtjvyW6yDsg@public.gmane.org
> > > > > Subject: [PATCH] i2c: designware: Add support for 16bit
> register access
> > > > > (...)
> > > > > @@ -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.
> 
> I beg to disagree. Quite strongly, even.
> 
> For one thing, "at all cost" never holds in the real world. You always
> have to make compromises.
> 
> For another, it only makes sense to define constants for values that
> have a specific meaning. For example bits in a bit vector, or bit
> masks. Here this is clearly not the case, so I fully agree with Stefan
> that a define would make the code _less_ readable.

Well you can have your opinion, let's wait for the 
maintainer's words..

> > > > > (...)
> > > > > 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 :)
> 
> Likewise, everyone can send patches to address issues that bother them.
> Want these ints turned into bools? Stop yelling at Stefan, and do it
> yourself. If it is so critical, no doubt you'll find reviewers to ack
> your patch and maintainers to apply it. And you'll even get the fame.

You missed the point completely..

We have done a thing in 'X' way but later we realize that 'Y'
is a better way of doing it and when we bring out something new
we still use the 'X' way and claim that it was always done that way
doesn't make sense. It's better to use 'Y' way for everything new
and gradually shift older stuff too..

> > I am sure the maintainer(s) will find my comments worth adding in
> your patch..
> 
> I'm not in charge of this specific driver, so I can't speak for the
> maintainer. And you shouldn't either...
> 

Yes of-course.
Let's wait for the maintainer(s) words.

Regards,
Bhupesh

Re: [PATCH] i2c: designware: Add support for 16bit register access

[Stefan Roese]

On Wednesday 14 March 2012 09:19:22 Bhupesh SHARMA wrote:
> > > > +++ 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.

I have no strong preferences here. I it helps getting this patch accepted, 
I'll remove those braces.
 
> > > >  	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.

I really don't share your point of view here. I feel that in this case, the 
number is much better readable than a macro.

> > > 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.

Again, I have no big problem changing this to bool.

> > 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 :)

Sure.
 
> I am sure the maintainer(s) will find my comments worth adding in your
> patch..

Might be. But who is the maintainer of this driver?

Thanks,
Stefan

Re: [PATCH] i2c: designware: Add support for 16bit register access

[Stefan Roese]

Hi Ben,

On Wednesday 14 March 2012 10:05:40 Stefan Roese wrote:
> On Wednesday 14 March 2012 09:19:22 Bhupesh SHARMA wrote:
> > > > > +++ 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.
> 
> I have no strong preferences here. I it helps getting this patch accepted,
> I'll remove those braces.
> 
> > > > >  	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.
> 
> I really don't share your point of view here. I feel that in this case, the
> number is much better readable than a macro.
> 
> > > > 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.
> 
> Again, I have no big problem changing this to bool.
> 
> > > 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 :)
> 
> Sure.
> 
> > I am sure the maintainer(s) will find my comments worth adding in your
> > patch..
> 
> Might be. But who is the maintainer of this driver?

Ben, will you accept this patch as is, or should I make some of the suggested 
changes (braces, int->bool) and send a patch-v2 out?

Thanks,
Stefan

Re: [PATCH] i2c: designware: Add support for 16bit register access

[Wolfram Sang]

Hi,

my points to this discussion:

On 13/03/12 16:54, Stefan Roese wrote:
> 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-ynQEQJNshbs@public.gmane.org>
> ---
>   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);
> +	}

Since both blocks just have one statement, please remove the braces.

>
>   	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)) {

No need for a macro IMO. You are missing a space before &, though.

> +		dev->access_16bit = 1;
> +		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;

This struct is already quite large. My favourite would be convert swab 
and access_16bit to 'unsigend accessor_flags' and proper flags. A 
bitfield at the end of the struct should also do.

>   	struct i2c_adapter	adapter;
>   	u32			functionality;
>   	u32			master_cfg;

Re: [PATCH] i2c: designware: Add support for 16bit register access

[Wolfram Sang]

Hi,

second try, now with CC list. Forgive me the mangled lines.

My points to this discussion:

On 13/03/12 16:54, Stefan Roese wrote:
 > 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-ynQEQJNshbs@public.gmane.org>
 > ---
 >   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);
 > +    }

Since both blocks just have one statement, please remove the braces.

 >
 >       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)) {

No need for a macro IMO. You are missing a space before &, though.

 > +        dev->access_16bit = 1;
 > +        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;

This struct is already quite large. My favourite would be convert swab 
and access_16bit to 'unsigend accessor_flags' and proper flags. A 
bitfield at the end of the struct should also do.

 >       struct i2c_adapter    adapter;
 >       u32            functionality;
 >       u32            master_cfg;