[PATCH 0/2] i2c: open cores I2C changes for XLP

[PATCH 0/2] i2c: open cores I2C changes for XLP

[Jayachandran C]

The two patches are to support the OpenCores based I2C controller
integrated into the Netlogic XLP SoC (this is different from the
Netlogic XLR I2C controller submitted earlier).

The first patch is to register i2c devices with of_i2c_register_devices(),
which is needed to add i2c devices from the FDT. The second patch is to
support a 'regwidth' parameter (both platform and device tree), that allows
us do 16-bit or 32-bit register read/write. XLP requires 32-bit IO to its
I2C register space and will use 'regwidth = 4' in its device tree.

Please let us know your comments.

Thanks,
JC.

Ganesan Ramalingam (2):
  i2c-ocore: register OF i2c devices
  i2c-ocore: support 16 and 32-bit wide registers

 drivers/i2c/busses/i2c-ocores.c |   37 ++++++++++++++++++++++++++++++++++---
 include/linux/i2c-ocores.h      |    1 +
 2 files changed, 35 insertions(+), 3 deletions(-)

-- 
1.7.9.5

[PATCH 1/2] i2c-ocore: register OF i2c devices

[Jayachandran C]

From: Ganesan Ramalingam <ganesanr-dY08KVG/lbpWk0Htik3J/w@public.gmane.org>

Call of_i2c_register_devices() in probe function to register i2c devices
specified in the device tree or OF.

Signed-off-by: Ganesan Ramalingam <ganesanr-dY08KVG/lbpWk0Htik3J/w@public.gmane.org>
Signed-off-by: Jayachandran C <jayachandranc-oSioyQM9ZPnuBjGU1YDckgC/G2K4zDHf@public.gmane.org>
---
 drivers/i2c/busses/i2c-ocores.c |    5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/i2c/busses/i2c-ocores.c b/drivers/i2c/busses/i2c-ocores.c
index 18068de..ebd2700 100644
--- a/drivers/i2c/busses/i2c-ocores.c
+++ b/drivers/i2c/busses/i2c-ocores.c
@@ -55,6 +55,7 @@
 #include <linux/i2c-ocores.h>
 #include <linux/slab.h>
 #include <linux/io.h>
+#include <linux/of_i2c.h>
 
 struct ocores_i2c {
 	void __iomem *base;
@@ -343,7 +344,9 @@ static int __devinit ocores_i2c_probe(struct platform_device *pdev)
 	if (pdata) {
 		for (i = 0; i < pdata->num_devices; i++)
 			i2c_new_device(&i2c->adap, pdata->devices + i);
-	}
+	} else
+		of_i2c_register_devices(&i2c->adap);
+
 
 	return 0;
 }
-- 
1.7.9.5

[PATCH 2/2] i2c-ocore: support 16 and 32-bit wide registers

[Jayachandran C]

From: Ganesan Ramalingam <ganesanr-dY08KVG/lbpWk0Htik3J/w@public.gmane.org>

Some architectures supports only 16-bit or 32-bit read/write access
to their iospace. Add a 'regwidth' platform and OF parameter which
specifies the IO width to support these platforms.

regwidth can be specified as 1, 2 or 4, and has a default value
of 1 if it is unspecified.

Signed-off-by: Ganesan Ramalingam <ganesanr-dY08KVG/lbpWk0Htik3J/w@public.gmane.org>
Signed-off-by: Jayachandran C <jayachandranc-oSioyQM9ZPnuBjGU1YDckgC/G2K4zDHf@public.gmane.org>
---
 drivers/i2c/busses/i2c-ocores.c |   32 ++++++++++++++++++++++++++++++--
 include/linux/i2c-ocores.h      |    1 +
 2 files changed, 31 insertions(+), 2 deletions(-)

diff --git a/drivers/i2c/busses/i2c-ocores.c b/drivers/i2c/busses/i2c-ocores.c
index ebd2700..1313145 100644
--- a/drivers/i2c/busses/i2c-ocores.c
+++ b/drivers/i2c/busses/i2c-ocores.c
@@ -19,6 +19,9 @@
  * - regstep         : size of device registers in bytes
  * - clock-frequency : frequency of bus clock in Hz
  * 
+ * Optional properties:
+ * - regwidth        : io register width in bytes (1, 2 or 4)
+
  * Example:
  *
  *  i2c0: ocores@a0000000 {
@@ -27,6 +30,7 @@
  *              interrupts = <10>;
  *
  *              regstep = <1>;
+ *              regwidth = <1>;
  *              clock-frequency = <20000000>;
  *
  * -- Devices connected on this I2C bus get
@@ -60,6 +64,7 @@
 struct ocores_i2c {
 	void __iomem *base;
 	int regstep;
+	int regwidth;
 	wait_queue_head_t wait;
 	struct i2c_adapter adap;
 	struct i2c_msg *msg;
@@ -102,12 +107,22 @@ struct ocores_i2c {
 
 static inline void oc_setreg(struct ocores_i2c *i2c, int reg, u8 value)
 {
-	iowrite8(value, i2c->base + reg * i2c->regstep);
+	if (i2c->regwidth == 4)
+		iowrite32(value, i2c->base + reg * i2c->regstep);
+	else if (i2c->regwidth == 2)
+		iowrite16(value, i2c->base + reg * i2c->regstep);
+	else
+		iowrite8(value, i2c->base + reg * i2c->regstep);
 }
 
 static inline u8 oc_getreg(struct ocores_i2c *i2c, int reg)
 {
-	return ioread8(i2c->base + reg * i2c->regstep);
+	if (i2c->regwidth == 4)
+		return ioread32(i2c->base + reg * i2c->regstep);
+	else if (i2c->regwidth == 2)
+		return ioread16(i2c->base + reg * i2c->regstep);
+	else
+		return ioread8(i2c->base + reg * i2c->regstep);
 }
 
 static void ocores_process(struct ocores_i2c *i2c)
@@ -267,6 +282,10 @@ static int ocores_i2c_of_probe(struct platform_device* pdev,
 	}
 	i2c->clock_khz = be32_to_cpup(val) / 1000;
 
+	val = of_get_property(pdev->dev.of_node, "regwidth", NULL);
+	if (val)
+		i2c->regwidth = be32_to_cpup(val);
+
 	return 0;
 }
 #else
@@ -309,12 +328,21 @@ static int __devinit ocores_i2c_probe(struct platform_device *pdev)
 	pdata = pdev->dev.platform_data;
 	if (pdata) {
 		i2c->regstep = pdata->regstep;
+		i2c->regwidth = pdata->regwidth;
 		i2c->clock_khz = pdata->clock_khz;
 	} else {
 		ret = ocores_i2c_of_probe(pdev, i2c);
 		if (ret)
 			return ret;
 	}
+	if (i2c->regwidth == 0)
+		i2c->regwidth = 1;	/* not configured, use default */
+	else if (i2c->regwidth != 1 && i2c->regwidth != 2 &&
+			i2c->regwidth != 4) {
+		dev_err(&pdev->dev, "Invalid register width %d\n",
+			i2c->regwidth);
+		return -EINVAL;
+	}
 
 	ocores_init(i2c);
 
diff --git a/include/linux/i2c-ocores.h b/include/linux/i2c-ocores.h
index 4d5e57f..d1258ad 100644
--- a/include/linux/i2c-ocores.h
+++ b/include/linux/i2c-ocores.h
@@ -13,6 +13,7 @@
 
 struct ocores_i2c_platform_data {
 	u32 regstep;   /* distance between registers */
+	u32 regwidth;  /* io read/write register witdth */
 	u32 clock_khz; /* input clock in kHz */
 	u8 num_devices; /* number of devices in the devices list */
 	struct i2c_board_info const *devices; /* devices connected to the bus */
-- 
1.7.9.5

Re: [PATCH 2/2] i2c-ocore: support 16 and 32-bit wide registers

[Shubhrajyoti Datta]

Hi,

On Tue, May 8, 2012 at 6:55 PM, Jayachandran C
<jayachandranc-oSioyQM9ZPnuBjGU1YDckgC/G2K4zDHf@public.gmane.org> wrote:
> From: Ganesan Ramalingam <ganesanr-dY08KVG/lbpWk0Htik3J/w@public.gmane.org>
>
> Some architectures supports only 16-bit or 32-bit read/write access
> to their iospace. Add a 'regwidth' platform and OF parameter which
> specifies the IO width to support these platforms.
>
> regwidth can be specified as 1, 2 or 4, and has a default value
> of 1 if it is unspecified.
>
> Signed-off-by: Ganesan Ramalingam <ganesanr-dY08KVG/lbpWk0Htik3J/w@public.gmane.org>
> Signed-off-by: Jayachandran C <jayachandranc-oSioyQM9ZPnuBjGU1YDckgC/G2K4zDHf@public.gmane.org>
> ---
>  drivers/i2c/busses/i2c-ocores.c |   32 ++++++++++++++++++++++++++++++--
>  include/linux/i2c-ocores.h      |    1 +
>  2 files changed, 31 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/i2c/busses/i2c-ocores.c b/drivers/i2c/busses/i2c-ocores.c
> index ebd2700..1313145 100644
> --- a/drivers/i2c/busses/i2c-ocores.c
> +++ b/drivers/i2c/busses/i2c-ocores.c
> @@ -19,6 +19,9 @@
>  * - regstep         : size of device registers in bytes
>  * - clock-frequency : frequency of bus clock in Hz
>  *
> + * Optional properties:
> + * - regwidth        : io register width in bytes (1, 2 or 4)
> +
>  * Example:
>  *
>  *  i2c0: ocores@a0000000 {
> @@ -27,6 +30,7 @@
>  *              interrupts = <10>;
>  *
>  *              regstep = <1>;
> + *              regwidth = <1>;
>  *              clock-frequency = <20000000>;
>  *
>  * -- Devices connected on this I2C bus get
> @@ -60,6 +64,7 @@
>  struct ocores_i2c {
>        void __iomem *base;
>        int regstep;
> +       int regwidth;

Do we need it to be signed?

>        wait_queue_head_t wait;
>        struct i2c_adapter adap;
>        struct i2c_msg *msg;
> @@ -102,12 +107,22 @@ struct ocores_i2c {
>
>  static inline void oc_setreg(struct ocores_i2c *i2c, int reg, u8 value)
>  {
> -       iowrite8(value, i2c->base + reg * i2c->regstep);
> +       if (i2c->regwidth == 4)
> +               iowrite32(value, i2c->base + reg * i2c->regstep);
> +       else if (i2c->regwidth == 2)
> +               iowrite16(value, i2c->base + reg * i2c->regstep);
> +       else
> +               iowrite8(value, i2c->base + reg * i2c->regstep);
>  }
>
>  static inline u8 oc_getreg(struct ocores_i2c *i2c, int reg)
Shouldnt the return type also change?

>  {
> -       return ioread8(i2c->base + reg * i2c->regstep);
> +       if (i2c->regwidth == 4)
> +               return ioread32(i2c->base + reg * i2c->regstep);
> +       else if (i2c->regwidth == 2)
> +               return ioread16(i2c->base + reg * i2c->regstep);
> +       else
> +               return ioread8(i2c->base + reg * i2c->regstep);
>  }
>
>  static void ocores_process(struct ocores_i2c *i2c)
> @@ -267,6 +282,10 @@ static int ocores_i2c_of_probe(struct platform_device* pdev,
>        }
>        i2c->clock_khz = be32_to_cpup(val) / 1000;
>
> +       val = of_get_property(pdev->dev.of_node, "regwidth", NULL);
> +       if (val)
> +               i2c->regwidth = be32_to_cpup(val);
> +
>        return 0;
>  }
>  #else
> @@ -309,12 +328,21 @@ static int __devinit ocores_i2c_probe(struct platform_device *pdev)
>        pdata = pdev->dev.platform_data;
>        if (pdata) {
>                i2c->regstep = pdata->regstep;
> +               i2c->regwidth = pdata->regwidth;
>                i2c->clock_khz = pdata->clock_khz;
>        } else {
>                ret = ocores_i2c_of_probe(pdev, i2c);
>                if (ret)
>                        return ret;
>        }
> +       if (i2c->regwidth == 0)
> +               i2c->regwidth = 1;      /* not configured, use default */
> +       else if (i2c->regwidth != 1 && i2c->regwidth != 2 &&
> +                       i2c->regwidth != 4) {
> +               dev_err(&pdev->dev, "Invalid register width %d\n",
> +                       i2c->regwidth);
> +               return -EINVAL;
> +       }
>
>        ocores_init(i2c);
>
> diff --git a/include/linux/i2c-ocores.h b/include/linux/i2c-ocores.h
> index 4d5e57f..d1258ad 100644
> --- a/include/linux/i2c-ocores.h
> +++ b/include/linux/i2c-ocores.h
> @@ -13,6 +13,7 @@
>
>  struct ocores_i2c_platform_data {
>        u32 regstep;   /* distance between registers */
> +       u32 regwidth;  /* io read/write register witdth */
>        u32 clock_khz; /* input clock in kHz */
>        u8 num_devices; /* number of devices in the devices list */
>        struct i2c_board_info const *devices; /* devices connected to the bus */
> --
> 1.7.9.5
>
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/2] i2c-ocore: support 16 and 32-bit wide registers

[Jayachandran C.]

On Tue, May 08, 2012 at 10:28:34PM +0530, Shubhrajyoti Datta wrote:
> Hi,
> 
> On Tue, May 8, 2012 at 6:55 PM, Jayachandran C
> <jayachandranc-oSioyQM9ZPnuBjGU1YDckgC/G2K4zDHf@public.gmane.org> wrote:
> > From: Ganesan Ramalingam <ganesanr-dY08KVG/lbpWk0Htik3J/w@public.gmane.org>
> >
> > Some architectures supports only 16-bit or 32-bit read/write access
> > to their iospace. Add a 'regwidth' platform and OF parameter which
> > specifies the IO width to support these platforms.
> >
> > regwidth can be specified as 1, 2 or 4, and has a default value
> > of 1 if it is unspecified.
> >
> > Signed-off-by: Ganesan Ramalingam <ganesanr-dY08KVG/lbpWk0Htik3J/w@public.gmane.org>
> > Signed-off-by: Jayachandran C <jayachandranc-oSioyQM9ZPnuBjGU1YDckgC/G2K4zDHf@public.gmane.org>
> > ---
> >  drivers/i2c/busses/i2c-ocores.c |   32 ++++++++++++++++++++++++++++++--
> >  include/linux/i2c-ocores.h      |    1 +
> >  2 files changed, 31 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/i2c/busses/i2c-ocores.c b/drivers/i2c/busses/i2c-ocores.c
> > index ebd2700..1313145 100644
> > --- a/drivers/i2c/busses/i2c-ocores.c
> > +++ b/drivers/i2c/busses/i2c-ocores.c
> > @@ -60,6 +64,7 @@
> >  struct ocores_i2c {
> >        void __iomem *base;
> >        int regstep;
> > +       int regwidth;
> 
> Do we need it to be signed?

regstep and regwidth can be both unsigned, but since regstep is already 
defined as int, we just followed that convention.

> 
> >        wait_queue_head_t wait;
> >        struct i2c_adapter adap;
> >        struct i2c_msg *msg;
> > @@ -102,12 +107,22 @@ struct ocores_i2c {
> >
> >  static inline void oc_setreg(struct ocores_i2c *i2c, int reg, u8 value)
> >  {
> > -       iowrite8(value, i2c->base + reg * i2c->regstep);
> > +       if (i2c->regwidth == 4)
> > +               iowrite32(value, i2c->base + reg * i2c->regstep);
> > +       else if (i2c->regwidth == 2)
> > +               iowrite16(value, i2c->base + reg * i2c->regstep);
> > +       else
> > +               iowrite8(value, i2c->base + reg * i2c->regstep);
> >  }
> >
> >  static inline u8 oc_getreg(struct ocores_i2c *i2c, int reg)
>
> Shouldnt the return type also change?
>

No, the registers have only 8bit of data, and we do a 16/32 bit read and
return the lowest 8 bits.

Regards,
JC.

Re: [PATCH 1/2] i2c-ocore: register OF i2c devices

[Wolfram Sang]

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

On Tue, May 08, 2012 at 06:55:28PM +0530, Jayachandran C wrote:
> From: Ganesan Ramalingam <ganesanr-dY08KVG/lbpWk0Htik3J/w@public.gmane.org>
> 
> Call of_i2c_register_devices() in probe function to register i2c devices
> specified in the device tree or OF.
> 
> Signed-off-by: Ganesan Ramalingam <ganesanr-dY08KVG/lbpWk0Htik3J/w@public.gmane.org>
> Signed-off-by: Jayachandran C <jayachandranc-oSioyQM9ZPnuBjGU1YDckgC/G2K4zDHf@public.gmane.org>

Applied to next with a minor fixup.

More important: If you are happily using this driver without issues,
please consider removing EXPERIMENTAL for this driver from Kconfig.

> ---
>  drivers/i2c/busses/i2c-ocores.c |    5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/i2c/busses/i2c-ocores.c b/drivers/i2c/busses/i2c-ocores.c
> index 18068de..ebd2700 100644
> --- a/drivers/i2c/busses/i2c-ocores.c
> +++ b/drivers/i2c/busses/i2c-ocores.c
> @@ -55,6 +55,7 @@
>  #include <linux/i2c-ocores.h>
>  #include <linux/slab.h>
>  #include <linux/io.h>
> +#include <linux/of_i2c.h>
>  
>  struct ocores_i2c {
>  	void __iomem *base;
> @@ -343,7 +344,9 @@ static int __devinit ocores_i2c_probe(struct platform_device *pdev)
>  	if (pdata) {
>  		for (i = 0; i < pdata->num_devices; i++)
>  			i2c_new_device(&i2c->adap, pdata->devices + i);
> -	}
> +	} else
> +		of_i2c_register_devices(&i2c->adap);
> +

This empty line was too much; also the else branch could have had
braces.

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 2/2] i2c-ocore: support 16 and 32-bit wide registers

[Wolfram Sang]

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

On Tue, May 08, 2012 at 06:55:29PM +0530, Jayachandran C wrote:
> From: Ganesan Ramalingam <ganesanr-dY08KVG/lbpWk0Htik3J/w@public.gmane.org>
> 
> Some architectures supports only 16-bit or 32-bit read/write access
> to their iospace. Add a 'regwidth' platform and OF parameter which
> specifies the IO width to support these platforms.
> 
> regwidth can be specified as 1, 2 or 4, and has a default value
> of 1 if it is unspecified.
> 
> Signed-off-by: Ganesan Ramalingam <ganesanr-dY08KVG/lbpWk0Htik3J/w@public.gmane.org>
> Signed-off-by: Jayachandran C <jayachandranc-oSioyQM9ZPnuBjGU1YDckgC/G2K4zDHf@public.gmane.org>

Hmm, this is a generic issue and should not be handled with a driver
specific property. CCing devicetree-discuss, maybe there is already a
way to handle this? The already existing 'regstep' looks suspicious, too?
Can't find any documentation for it.

Thanks,

   Wolfram

> ---
>  drivers/i2c/busses/i2c-ocores.c |   32 ++++++++++++++++++++++++++++++--
>  include/linux/i2c-ocores.h      |    1 +
>  2 files changed, 31 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-ocores.c b/drivers/i2c/busses/i2c-ocores.c
> index ebd2700..1313145 100644
> --- a/drivers/i2c/busses/i2c-ocores.c
> +++ b/drivers/i2c/busses/i2c-ocores.c
> @@ -19,6 +19,9 @@
>   * - regstep         : size of device registers in bytes
>   * - clock-frequency : frequency of bus clock in Hz
>   * 
> + * Optional properties:
> + * - regwidth        : io register width in bytes (1, 2 or 4)
> +
>   * Example:
>   *
>   *  i2c0: ocores@a0000000 {
> @@ -27,6 +30,7 @@
>   *              interrupts = <10>;
>   *
>   *              regstep = <1>;
> + *              regwidth = <1>;
>   *              clock-frequency = <20000000>;
>   *
>   * -- Devices connected on this I2C bus get
> @@ -60,6 +64,7 @@
>  struct ocores_i2c {
>  	void __iomem *base;
>  	int regstep;
> +	int regwidth;
>  	wait_queue_head_t wait;
>  	struct i2c_adapter adap;
>  	struct i2c_msg *msg;
> @@ -102,12 +107,22 @@ struct ocores_i2c {
>  
>  static inline void oc_setreg(struct ocores_i2c *i2c, int reg, u8 value)
>  {
> -	iowrite8(value, i2c->base + reg * i2c->regstep);
> +	if (i2c->regwidth == 4)
> +		iowrite32(value, i2c->base + reg * i2c->regstep);
> +	else if (i2c->regwidth == 2)
> +		iowrite16(value, i2c->base + reg * i2c->regstep);
> +	else
> +		iowrite8(value, i2c->base + reg * i2c->regstep);
>  }
>  
>  static inline u8 oc_getreg(struct ocores_i2c *i2c, int reg)
>  {
> -	return ioread8(i2c->base + reg * i2c->regstep);
> +	if (i2c->regwidth == 4)
> +		return ioread32(i2c->base + reg * i2c->regstep);
> +	else if (i2c->regwidth == 2)
> +		return ioread16(i2c->base + reg * i2c->regstep);
> +	else
> +		return ioread8(i2c->base + reg * i2c->regstep);
>  }
>  
>  static void ocores_process(struct ocores_i2c *i2c)
> @@ -267,6 +282,10 @@ static int ocores_i2c_of_probe(struct platform_device* pdev,
>  	}
>  	i2c->clock_khz = be32_to_cpup(val) / 1000;
>  
> +	val = of_get_property(pdev->dev.of_node, "regwidth", NULL);
> +	if (val)
> +		i2c->regwidth = be32_to_cpup(val);
> +
>  	return 0;
>  }
>  #else
> @@ -309,12 +328,21 @@ static int __devinit ocores_i2c_probe(struct platform_device *pdev)
>  	pdata = pdev->dev.platform_data;
>  	if (pdata) {
>  		i2c->regstep = pdata->regstep;
> +		i2c->regwidth = pdata->regwidth;
>  		i2c->clock_khz = pdata->clock_khz;
>  	} else {
>  		ret = ocores_i2c_of_probe(pdev, i2c);
>  		if (ret)
>  			return ret;
>  	}
> +	if (i2c->regwidth == 0)
> +		i2c->regwidth = 1;	/* not configured, use default */
> +	else if (i2c->regwidth != 1 && i2c->regwidth != 2 &&
> +			i2c->regwidth != 4) {
> +		dev_err(&pdev->dev, "Invalid register width %d\n",
> +			i2c->regwidth);
> +		return -EINVAL;
> +	}
>  
>  	ocores_init(i2c);
>  
> diff --git a/include/linux/i2c-ocores.h b/include/linux/i2c-ocores.h
> index 4d5e57f..d1258ad 100644
> --- a/include/linux/i2c-ocores.h
> +++ b/include/linux/i2c-ocores.h
> @@ -13,6 +13,7 @@
>  
>  struct ocores_i2c_platform_data {
>  	u32 regstep;   /* distance between registers */
> +	u32 regwidth;  /* io read/write register witdth */
>  	u32 clock_khz; /* input clock in kHz */
>  	u8 num_devices; /* number of devices in the devices list */
>  	struct i2c_board_info const *devices; /* devices connected to the bus */
> -- 
> 1.7.9.5
> 
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

-- 
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 2/2] i2c-ocore: support 16 and 32-bit wide registers

[Jayachandran C.]

On Sat, May 12, 2012 at 04:56:48PM +0200, Wolfram Sang wrote:
> On Tue, May 08, 2012 at 06:55:29PM +0530, Jayachandran C wrote:
> > From: Ganesan Ramalingam <ganesanr-dY08KVG/lbpWk0Htik3J/w@public.gmane.org>
> > 
> > Some architectures supports only 16-bit or 32-bit read/write access
> > to their iospace. Add a 'regwidth' platform and OF parameter which
> > specifies the IO width to support these platforms.
> > 
> > regwidth can be specified as 1, 2 or 4, and has a default value
> > of 1 if it is unspecified.
> > 
> > Signed-off-by: Ganesan Ramalingam <ganesanr-dY08KVG/lbpWk0Htik3J/w@public.gmane.org>
> > Signed-off-by: Jayachandran C <jayachandranc-oSioyQM9ZPnuBjGU1YDckgC/G2K4zDHf@public.gmane.org>
> 
> Hmm, this is a generic issue and should not be handled with a driver
> specific property. CCing devicetree-discuss, maybe there is already a
> way to handle this? The already existing 'regstep' looks suspicious, too?
> Can't find any documentation for it.

My suggestion would be to use "reg-shift" and "reg-io-width" like the
uart and some other devices. I am not sure why "regstep" was used when
there is already a standard "reg-shift" parameter.

We will post an updated patch that deprecates "regstep" and uses "reg-shift"
and "reg-io-width" if that is preferred.

Thanks,
JC.

> 
> > ---
> >  drivers/i2c/busses/i2c-ocores.c |   32 ++++++++++++++++++++++++++++++--
> >  include/linux/i2c-ocores.h      |    1 +
> >  2 files changed, 31 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/i2c/busses/i2c-ocores.c b/drivers/i2c/busses/i2c-ocores.c
> > index ebd2700..1313145 100644
> > --- a/drivers/i2c/busses/i2c-ocores.c
> > +++ b/drivers/i2c/busses/i2c-ocores.c
> > @@ -19,6 +19,9 @@
> >   * - regstep         : size of device registers in bytes
> >   * - clock-frequency : frequency of bus clock in Hz
> >   * 
> > + * Optional properties:
> > + * - regwidth        : io register width in bytes (1, 2 or 4)
> > +
> >   * Example:
> >   *
> >   *  i2c0: ocores@a0000000 {
> > @@ -27,6 +30,7 @@
> >   *              interrupts = <10>;
> >   *
> >   *              regstep = <1>;
> > + *              regwidth = <1>;
> >   *              clock-frequency = <20000000>;
> >   *
> >   * -- Devices connected on this I2C bus get
> > @@ -60,6 +64,7 @@
> >  struct ocores_i2c {
> >  	void __iomem *base;
> >  	int regstep;
> > +	int regwidth;
> >  	wait_queue_head_t wait;
> >  	struct i2c_adapter adap;
> >  	struct i2c_msg *msg;
> > @@ -102,12 +107,22 @@ struct ocores_i2c {
> >  
> >  static inline void oc_setreg(struct ocores_i2c *i2c, int reg, u8 value)
> >  {
> > -	iowrite8(value, i2c->base + reg * i2c->regstep);
> > +	if (i2c->regwidth == 4)
> > +		iowrite32(value, i2c->base + reg * i2c->regstep);
> > +	else if (i2c->regwidth == 2)
> > +		iowrite16(value, i2c->base + reg * i2c->regstep);
> > +	else
> > +		iowrite8(value, i2c->base + reg * i2c->regstep);
> >  }
> >  
> >  static inline u8 oc_getreg(struct ocores_i2c *i2c, int reg)
> >  {
> > -	return ioread8(i2c->base + reg * i2c->regstep);
> > +	if (i2c->regwidth == 4)
> > +		return ioread32(i2c->base + reg * i2c->regstep);
> > +	else if (i2c->regwidth == 2)
> > +		return ioread16(i2c->base + reg * i2c->regstep);
> > +	else
> > +		return ioread8(i2c->base + reg * i2c->regstep);
> >  }
> >  
> >  static void ocores_process(struct ocores_i2c *i2c)
> > @@ -267,6 +282,10 @@ static int ocores_i2c_of_probe(struct platform_device* pdev,
> >  	}
> >  	i2c->clock_khz = be32_to_cpup(val) / 1000;
> >  
> > +	val = of_get_property(pdev->dev.of_node, "regwidth", NULL);
> > +	if (val)
> > +		i2c->regwidth = be32_to_cpup(val);
> > +
> >  	return 0;
> >  }
> >  #else
> > @@ -309,12 +328,21 @@ static int __devinit ocores_i2c_probe(struct platform_device *pdev)
> >  	pdata = pdev->dev.platform_data;
> >  	if (pdata) {
> >  		i2c->regstep = pdata->regstep;
> > +		i2c->regwidth = pdata->regwidth;
> >  		i2c->clock_khz = pdata->clock_khz;
> >  	} else {
> >  		ret = ocores_i2c_of_probe(pdev, i2c);
> >  		if (ret)
> >  			return ret;
> >  	}
> > +	if (i2c->regwidth == 0)
> > +		i2c->regwidth = 1;	/* not configured, use default */
> > +	else if (i2c->regwidth != 1 && i2c->regwidth != 2 &&
> > +			i2c->regwidth != 4) {
> > +		dev_err(&pdev->dev, "Invalid register width %d\n",
> > +			i2c->regwidth);
> > +		return -EINVAL;
> > +	}
> >  
> >  	ocores_init(i2c);
> >  
> > diff --git a/include/linux/i2c-ocores.h b/include/linux/i2c-ocores.h
> > index 4d5e57f..d1258ad 100644
> > --- a/include/linux/i2c-ocores.h
> > +++ b/include/linux/i2c-ocores.h
> > @@ -13,6 +13,7 @@
> >  
> >  struct ocores_i2c_platform_data {
> >  	u32 regstep;   /* distance between registers */
> > +	u32 regwidth;  /* io read/write register witdth */
> >  	u32 clock_khz; /* input clock in kHz */
> >  	u8 num_devices; /* number of devices in the devices list */
> >  	struct i2c_board_info const *devices; /* devices connected to the bus */
> > -- 
> > 1.7.9.5
> > 
> > 
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
> > the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 
> -- 
> Pengutronix e.K.                           | Wolfram Sang                |
> Industrial Linux Solutions                 | http://www.pengutronix.de/  |

Re: [PATCH 1/2] i2c-ocore: register OF i2c devices

[Peter Korsgaard]

>>>>> "Wolfram" == Wolfram Sang <w.sang-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org> writes:

 Wolfram> On Tue, May 08, 2012 at 06:55:28PM +0530, Jayachandran C wrote:
 >> From: Ganesan Ramalingam <ganesanr-dY08KVG/lbpWk0Htik3J/w@public.gmane.org>
 >> 
 >> Call of_i2c_register_devices() in probe function to register i2c devices
 >> specified in the device tree or OF.
 >> 
 >> Signed-off-by: Ganesan Ramalingam <ganesanr-dY08KVG/lbpWk0Htik3J/w@public.gmane.org>
 >> Signed-off-by: Jayachandran C <jayachandranc-oSioyQM9ZPnuBjGU1YDckgC/G2K4zDHf@public.gmane.org>

 Wolfram> Applied to next with a minor fixup.

 Wolfram> More important: If you are happily using this driver without issues,
 Wolfram> please consider removing EXPERIMENTAL for this driver from Kconfig.

And also please consider CC'ing the maintainer on patches (E.G. me) - I
didn't notice this before now.

-- 
Bye, Peter Korsgaard