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

[PATCH v2] 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>
Cc: Wolfram Sang <w.sang-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
Cc: Viresh Kumar <viresh.kumar-qxv4g6HH51o@public.gmane.org>
---
v2:
- Removed parenthesis for single-statement block
- Moved "swab" and "access_16bit" into "accessor_flags"

 drivers/i2c/busses/i2c-designware-core.c |   27 ++++++++++++++++++++++-----
 drivers/i2c/busses/i2c-designware-core.h |    5 ++++-
 2 files changed, 26 insertions(+), 6 deletions(-)

diff --git a/drivers/i2c/busses/i2c-designware-core.c b/drivers/i2c/busses/i2c-designware-core.c
index df87992..b3c5cfa 100644
--- a/drivers/i2c/busses/i2c-designware-core.c
+++ b/drivers/i2c/busses/i2c-designware-core.c
@@ -164,9 +164,15 @@ static char *abort_sources[] = {
 
 u32 dw_readl(struct dw_i2c_dev *dev, int offset)
 {
-	u32 value = readl(dev->base + offset);
+	u32 value;
 
-	if (dev->swab)
+	if (dev->accessor_flags & ACCESS_16BIT)
+		value = readw(dev->base + offset) |
+			(readw(dev->base + offset + 2) << 16);
+	else
+		value = readl(dev->base + offset);
+
+	if (dev->accessor_flags & ACCESS_SWAP)
 		return swab32(value);
 	else
 		return value;
@@ -174,10 +180,15 @@ u32 dw_readl(struct dw_i2c_dev *dev, int offset)
 
 void dw_writel(struct dw_i2c_dev *dev, u32 b, int offset)
 {
-	if (dev->swab)
+	if (dev->accessor_flags & ACCESS_SWAP)
 		b = swab32(b);
 
-	writel(b, dev->base + offset);
+	if (dev->accessor_flags & ACCESS_16BIT) {
+		writew((u16)b, dev->base + offset);
+		writew((u16)(b >> 16), dev->base + offset + 2);
+	} else {
+		writel(b, dev->base + offset);
+	}
 }
 
 static u32
@@ -254,7 +265,13 @@ int i2c_dw_init(struct dw_i2c_dev *dev)
 	/* Configure register endianess access */
 	reg = dw_readl(dev, DW_IC_COMP_TYPE);
 	if (reg == ___constant_swab32(DW_IC_COMP_TYPE_VALUE)) {
-		dev->swab = 1;
+		dev->accessor_flags |= ACCESS_SWAP;
+		reg = DW_IC_COMP_TYPE_VALUE;
+	}
+
+	/* Configure register access mode 16bit */
+	if (reg == (DW_IC_COMP_TYPE_VALUE & 0x0000ffff)) {
+		dev->accessor_flags |= ACCESS_16BIT;
 		reg = DW_IC_COMP_TYPE_VALUE;
 	}
 
diff --git a/drivers/i2c/busses/i2c-designware-core.h b/drivers/i2c/busses/i2c-designware-core.h
index 02d1a2d..9c1840e 100644
--- a/drivers/i2c/busses/i2c-designware-core.h
+++ b/drivers/i2c/busses/i2c-designware-core.h
@@ -82,7 +82,7 @@ struct dw_i2c_dev {
 	unsigned int		status;
 	u32			abort_source;
 	int			irq;
-	int			swab;
+	u32			accessor_flags;
 	struct i2c_adapter	adapter;
 	u32			functionality;
 	u32			master_cfg;
@@ -90,6 +90,9 @@ struct dw_i2c_dev {
 	unsigned int		rx_fifo_depth;
 };
 
+#define ACCESS_SWAP		0x00000001
+#define ACCESS_16BIT		0x00000002
+
 extern u32 dw_readl(struct dw_i2c_dev *dev, int offset);
 extern void dw_writel(struct dw_i2c_dev *dev, u32 b, int offset);
 extern int i2c_dw_init(struct dw_i2c_dev *dev);
-- 
1.7.10

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

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 3841 bytes --]

On Wed, Apr 18, 2012 at 09:33:19AM +0200, 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>

Thanks for the update, looks mostly good.

> Cc: Wolfram Sang <w.sang-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
> Cc: Viresh Kumar <viresh.kumar-qxv4g6HH51o@public.gmane.org>
> ---
> v2:
> - Removed parenthesis for single-statement block
> - Moved "swab" and "access_16bit" into "accessor_flags"
> 
>  drivers/i2c/busses/i2c-designware-core.c |   27 ++++++++++++++++++++++-----
>  drivers/i2c/busses/i2c-designware-core.h |    5 ++++-
>  2 files changed, 26 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-designware-core.c b/drivers/i2c/busses/i2c-designware-core.c
> index df87992..b3c5cfa 100644
> --- a/drivers/i2c/busses/i2c-designware-core.c
> +++ b/drivers/i2c/busses/i2c-designware-core.c
> @@ -164,9 +164,15 @@ static char *abort_sources[] = {
>  
>  u32 dw_readl(struct dw_i2c_dev *dev, int offset)
>  {
> -	u32 value = readl(dev->base + offset);
> +	u32 value;
>  
> -	if (dev->swab)
> +	if (dev->accessor_flags & ACCESS_16BIT)
> +		value = readw(dev->base + offset) |
> +			(readw(dev->base + offset + 2) << 16);
> +	else
> +		value = readl(dev->base + offset);
> +
> +	if (dev->accessor_flags & ACCESS_SWAP)
>  		return swab32(value);
>  	else
>  		return value;
> @@ -174,10 +180,15 @@ u32 dw_readl(struct dw_i2c_dev *dev, int offset)
>  
>  void dw_writel(struct dw_i2c_dev *dev, u32 b, int offset)
>  {
> -	if (dev->swab)
> +	if (dev->accessor_flags & ACCESS_SWAP)
>  		b = swab32(b);
>  
> -	writel(b, dev->base + offset);
> +	if (dev->accessor_flags & ACCESS_16BIT) {
> +		writew((u16)b, dev->base + offset);
> +		writew((u16)(b >> 16), dev->base + offset + 2);
> +	} else {
> +		writel(b, dev->base + offset);
> +	}
>  }
>  
>  static u32
> @@ -254,7 +265,13 @@ int i2c_dw_init(struct dw_i2c_dev *dev)
>  	/* Configure register endianess access */
>  	reg = dw_readl(dev, DW_IC_COMP_TYPE);
>  	if (reg == ___constant_swab32(DW_IC_COMP_TYPE_VALUE)) {
> -		dev->swab = 1;
> +		dev->accessor_flags |= ACCESS_SWAP;
> +		reg = DW_IC_COMP_TYPE_VALUE;

May I ask you to use proper if/elseif/else blocks instead of overwriting
reg? I know you didn't come up with the mechanism, yet it looks too
fragile to be extended IMO.

> +	}
> +
> +	/* Configure register access mode 16bit */
> +	if (reg == (DW_IC_COMP_TYPE_VALUE & 0x0000ffff)) {

Does it make sense to check reg + 2 for the upper part of the signature?

> +		dev->accessor_flags |= ACCESS_16BIT;
>  		reg = DW_IC_COMP_TYPE_VALUE;
>  	}
>  
> diff --git a/drivers/i2c/busses/i2c-designware-core.h b/drivers/i2c/busses/i2c-designware-core.h
> index 02d1a2d..9c1840e 100644
> --- a/drivers/i2c/busses/i2c-designware-core.h
> +++ b/drivers/i2c/busses/i2c-designware-core.h
> @@ -82,7 +82,7 @@ struct dw_i2c_dev {
>  	unsigned int		status;
>  	u32			abort_source;
>  	int			irq;
> -	int			swab;
> +	u32			accessor_flags;
>  	struct i2c_adapter	adapter;
>  	u32			functionality;
>  	u32			master_cfg;
> @@ -90,6 +90,9 @@ struct dw_i2c_dev {
>  	unsigned int		rx_fifo_depth;
>  };
>  
> +#define ACCESS_SWAP		0x00000001
> +#define ACCESS_16BIT		0x00000002
> +
>  extern u32 dw_readl(struct dw_i2c_dev *dev, int offset);
>  extern void dw_writel(struct dw_i2c_dev *dev, u32 b, int offset);
>  extern int i2c_dw_init(struct dw_i2c_dev *dev);
> -- 
> 1.7.10
> 

Thanks,

   Wolfram

-- 
Pengutronix e.K.                           | Wolfram Sang                |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

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

[Stefan Roese]

On Wednesday 18 April 2012 13:24:26 Wolfram Sang wrote:

<snip>

> > @@ -254,7 +265,13 @@ int i2c_dw_init(struct dw_i2c_dev *dev)
> > 
> >  	/* Configure register endianess access */
> >  	reg = dw_readl(dev, DW_IC_COMP_TYPE);
> >  	if (reg == ___constant_swab32(DW_IC_COMP_TYPE_VALUE)) {
> > 
> > -		dev->swab = 1;
> > +		dev->accessor_flags |= ACCESS_SWAP;
> > +		reg = DW_IC_COMP_TYPE_VALUE;
> 
> May I ask you to use proper if/elseif/else blocks instead of overwriting
> reg? I know you didn't come up with the mechanism, yet it looks too
> fragile to be extended IMO.

Okay, will do.
 
> > +	}
> > +
> > +	/* Configure register access mode 16bit */
> > +	if (reg == (DW_IC_COMP_TYPE_VALUE & 0x0000ffff)) {
> 
> Does it make sense to check reg + 2 for the upper part of the signature?

Not from my point of view. Its very unlikely that this driver will be used on 
another controller. Its configured (via its address in platform-data or DT) 
for exactly this controller. And matching 16bit of this 32bit register seems 
enough for me. I would even have no problem to remove this check completely.

Thanks,
Stefan