Patchwork [PATCHv4,4/9] i2c: mv64xxx: Add Allwinner sun4i compatible

login
register
mail settings
Submitter Maxime Ripard
Date June 12, 2013, 8:07 a.m.
Message ID <1371024438-16631-5-git-send-email-maxime.ripard@free-electrons.com>
Download mbox | patch
Permalink /patch/250693/
State Superseded
Headers show

Comments

Maxime Ripard - June 12, 2013, 8:07 a.m.
Add the compatible string for the Allwinner A10 i2c controller and the
associated register layout.

Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>
---
 drivers/i2c/busses/Kconfig       |  3 ++-
 drivers/i2c/busses/i2c-mv64xxx.c |  1 +
 include/linux/mv643xx_i2c.h      | 10 ++++++++++
 3 files changed, 13 insertions(+), 1 deletion(-)
Tomasz Figa - June 12, 2013, 8:42 a.m.
Hi Maxime,

On Wednesday 12 of June 2013 10:07:13 Maxime Ripard wrote:
> Add the compatible string for the Allwinner A10 i2c controller and the
> associated register layout.
> 
> Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>
> ---
>  drivers/i2c/busses/Kconfig       |  3 ++-
>  drivers/i2c/busses/i2c-mv64xxx.c |  1 +
>  include/linux/mv643xx_i2c.h      | 10 ++++++++++
>  3 files changed, 13 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
> index 631736e..5dc4148 100644
> --- a/drivers/i2c/busses/Kconfig
> +++ b/drivers/i2c/busses/Kconfig
> @@ -507,10 +507,11 @@ config I2C_MPC
> 
>  config I2C_MV64XXX
>  	tristate "Marvell mv64xxx I2C Controller"
> -	depends on (MV64X60 || PLAT_ORION)
> +	depends on (MV64X60 || PLAT_ORION || ARCH_SUNXI)
>  	help
>  	  If you say yes to this option, support will be included for the
>  	  built-in I2C interface on the Marvell 64xxx line of host 
bridges.
> +	  This driver is also used for Allwinner SoCs I2C controllers.
> 
>  	  This driver can also be built as a module.  If so, the module
>  	  will be called i2c-mv64xxx.
> diff --git a/drivers/i2c/busses/i2c-mv64xxx.c
> b/drivers/i2c/busses/i2c-mv64xxx.c index f9e076e..febf5ba 100644
> --- a/drivers/i2c/busses/i2c-mv64xxx.c
> +++ b/drivers/i2c/busses/i2c-mv64xxx.c
> @@ -490,6 +490,7 @@ static const struct i2c_algorithm mv64xxx_i2c_algo =
> { */
>  #ifdef CONFIG_OF
>  static const struct of_device_id mv64xxx_i2c_of_match_table[] = {
> +	{ .compatible = "allwinner,sun4i-i2c", .data =
> &mv64xxx_i2c_regs_sun4i}, { .compatible = "marvell,mv64xxx-i2c", .data
> = &mv64xxx_i2c_regs_mv64xxx}, {}

Ah, OK, here you use compatible matching to get variant-specific data, 
fine. Still I think the same method (based on platform device name) should 
be used for non-DT variant.

>  };
> diff --git a/include/linux/mv643xx_i2c.h b/include/linux/mv643xx_i2c.h
> index 9304c94..737108e 100644
> --- a/include/linux/mv643xx_i2c.h
> +++ b/include/linux/mv643xx_i2c.h
> @@ -32,6 +32,16 @@ struct mv64xxx_i2c_regs mv64xxx_i2c_regs_mv64xxx = {
>  	.soft_reset	= 0x1c,
>  };
> 
> +struct mv64xxx_i2c_regs mv64xxx_i2c_regs_sun4i = {
> +	.addr		= 0x00,
> +	.ext_addr	= 0x04,
> +	.data		= 0x08,
> +	.control	= 0x0c,
> +	.status		= 0x10,
> +	.clock		= 0x14,
> +	.soft_reset	= 0x18,
> +};
> +

Hmm, header doesn't look like a correct place for a structure definition, 
especially when the structure isn't even static.

Best regards,
Tomasz

>  /* i2c Platform Device, Driver Data */
>  struct mv64xxx_i2c_pdata {
>  	u32			freq_m;
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Andrew Lunn - June 12, 2013, 10:56 a.m.
On Wed, Jun 12, 2013 at 10:07:13AM +0200, Maxime Ripard wrote:
> Add the compatible string for the Allwinner A10 i2c controller and the
> associated register layout.
> 
> Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>
> ---
>  drivers/i2c/busses/Kconfig       |  3 ++-
>  drivers/i2c/busses/i2c-mv64xxx.c |  1 +
>  include/linux/mv643xx_i2c.h      | 10 ++++++++++
>  3 files changed, 13 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
> index 631736e..5dc4148 100644
> --- a/drivers/i2c/busses/Kconfig
> +++ b/drivers/i2c/busses/Kconfig
> @@ -507,10 +507,11 @@ config I2C_MPC
>  
>  config I2C_MV64XXX
>  	tristate "Marvell mv64xxx I2C Controller"
> -	depends on (MV64X60 || PLAT_ORION)
> +	depends on (MV64X60 || PLAT_ORION || ARCH_SUNXI)

Hi Maxime.

What about MV64X60? It also needs to pass the registers in its
platform data.

	 Andrew
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Maxime Ripard - June 12, 2013, 11:27 a.m.
Hi Tomasz,

On Wed, Jun 12, 2013 at 10:42:31AM +0200, Tomasz Figa wrote:
> Hi Maxime,
> 
> On Wednesday 12 of June 2013 10:07:13 Maxime Ripard wrote:
> > Add the compatible string for the Allwinner A10 i2c controller and the
> > associated register layout.
> > 
> > Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>
> > ---
> >  drivers/i2c/busses/Kconfig       |  3 ++-
> >  drivers/i2c/busses/i2c-mv64xxx.c |  1 +
> >  include/linux/mv643xx_i2c.h      | 10 ++++++++++
> >  3 files changed, 13 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
> > index 631736e..5dc4148 100644
> > --- a/drivers/i2c/busses/Kconfig
> > +++ b/drivers/i2c/busses/Kconfig
> > @@ -507,10 +507,11 @@ config I2C_MPC
> > 
> >  config I2C_MV64XXX
> >  	tristate "Marvell mv64xxx I2C Controller"
> > -	depends on (MV64X60 || PLAT_ORION)
> > +	depends on (MV64X60 || PLAT_ORION || ARCH_SUNXI)
> >  	help
> >  	  If you say yes to this option, support will be included for the
> >  	  built-in I2C interface on the Marvell 64xxx line of host 
> bridges.
> > +	  This driver is also used for Allwinner SoCs I2C controllers.
> > 
> >  	  This driver can also be built as a module.  If so, the module
> >  	  will be called i2c-mv64xxx.
> > diff --git a/drivers/i2c/busses/i2c-mv64xxx.c
> > b/drivers/i2c/busses/i2c-mv64xxx.c index f9e076e..febf5ba 100644
> > --- a/drivers/i2c/busses/i2c-mv64xxx.c
> > +++ b/drivers/i2c/busses/i2c-mv64xxx.c
> > @@ -490,6 +490,7 @@ static const struct i2c_algorithm mv64xxx_i2c_algo =
> > { */
> >  #ifdef CONFIG_OF
> >  static const struct of_device_id mv64xxx_i2c_of_match_table[] = {
> > +	{ .compatible = "allwinner,sun4i-i2c", .data =
> > &mv64xxx_i2c_regs_sun4i}, { .compatible = "marvell,mv64xxx-i2c", .data
> > = &mv64xxx_i2c_regs_mv64xxx}, {}
> 
> Ah, OK, here you use compatible matching to get variant-specific data, 
> fine. Still I think the same method (based on platform device name) should 
> be used for non-DT variant.

Yes, you're right. I wasn't aware that you could do something similar
with the platform device name, hence why I used the platform data.

I'll fix this in the next iteration.

> >  };
> > diff --git a/include/linux/mv643xx_i2c.h b/include/linux/mv643xx_i2c.h
> > index 9304c94..737108e 100644
> > --- a/include/linux/mv643xx_i2c.h
> > +++ b/include/linux/mv643xx_i2c.h
> > @@ -32,6 +32,16 @@ struct mv64xxx_i2c_regs mv64xxx_i2c_regs_mv64xxx = {
> >  	.soft_reset	= 0x1c,
> >  };
> > 
> > +struct mv64xxx_i2c_regs mv64xxx_i2c_regs_sun4i = {
> > +	.addr		= 0x00,
> > +	.ext_addr	= 0x04,
> > +	.data		= 0x08,
> > +	.control	= 0x0c,
> > +	.status		= 0x10,
> > +	.clock		= 0x14,
> > +	.soft_reset	= 0x18,
> > +};
> > +
> 
> Hmm, header doesn't look like a correct place for a structure definition, 
> especially when the structure isn't even static.

I put it there because it had to be used by the board files to fill
their plateform_data. Obviously, it will be moved back into the driver.

Thanks,
Maxime
Maxime Ripard - June 12, 2013, 11:31 a.m.
On Wed, Jun 12, 2013 at 12:56:13PM +0200, Andrew Lunn wrote:
> On Wed, Jun 12, 2013 at 10:07:13AM +0200, Maxime Ripard wrote:
> > Add the compatible string for the Allwinner A10 i2c controller and the
> > associated register layout.
> > 
> > Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>
> > ---
> >  drivers/i2c/busses/Kconfig       |  3 ++-
> >  drivers/i2c/busses/i2c-mv64xxx.c |  1 +
> >  include/linux/mv643xx_i2c.h      | 10 ++++++++++
> >  3 files changed, 13 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
> > index 631736e..5dc4148 100644
> > --- a/drivers/i2c/busses/Kconfig
> > +++ b/drivers/i2c/busses/Kconfig
> > @@ -507,10 +507,11 @@ config I2C_MPC
> >  
> >  config I2C_MV64XXX
> >  	tristate "Marvell mv64xxx I2C Controller"
> > -	depends on (MV64X60 || PLAT_ORION)
> > +	depends on (MV64X60 || PLAT_ORION || ARCH_SUNXI)
> 
> Hi Maxime.
> 
> What about MV64X60? It also needs to pass the registers in its
> platform data.

I grep'ed for the users mv64xxx_i2c_pdata in arch/arm, and the
plat-orion/common.c was the only user. Obviously, I forgot to think
about the PPC architecture.

Anyway, this patch will be dropped.

Thanks!
Maxime

Patch

diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
index 631736e..5dc4148 100644
--- a/drivers/i2c/busses/Kconfig
+++ b/drivers/i2c/busses/Kconfig
@@ -507,10 +507,11 @@  config I2C_MPC
 
 config I2C_MV64XXX
 	tristate "Marvell mv64xxx I2C Controller"
-	depends on (MV64X60 || PLAT_ORION)
+	depends on (MV64X60 || PLAT_ORION || ARCH_SUNXI)
 	help
 	  If you say yes to this option, support will be included for the
 	  built-in I2C interface on the Marvell 64xxx line of host bridges.
+	  This driver is also used for Allwinner SoCs I2C controllers.
 
 	  This driver can also be built as a module.  If so, the module
 	  will be called i2c-mv64xxx.
diff --git a/drivers/i2c/busses/i2c-mv64xxx.c b/drivers/i2c/busses/i2c-mv64xxx.c
index f9e076e..febf5ba 100644
--- a/drivers/i2c/busses/i2c-mv64xxx.c
+++ b/drivers/i2c/busses/i2c-mv64xxx.c
@@ -490,6 +490,7 @@  static const struct i2c_algorithm mv64xxx_i2c_algo = {
  */
 #ifdef CONFIG_OF
 static const struct of_device_id mv64xxx_i2c_of_match_table[] = {
+	{ .compatible = "allwinner,sun4i-i2c", .data = &mv64xxx_i2c_regs_sun4i},
 	{ .compatible = "marvell,mv64xxx-i2c", .data = &mv64xxx_i2c_regs_mv64xxx},
 	{}
 };
diff --git a/include/linux/mv643xx_i2c.h b/include/linux/mv643xx_i2c.h
index 9304c94..737108e 100644
--- a/include/linux/mv643xx_i2c.h
+++ b/include/linux/mv643xx_i2c.h
@@ -32,6 +32,16 @@  struct mv64xxx_i2c_regs mv64xxx_i2c_regs_mv64xxx = {
 	.soft_reset	= 0x1c,
 };
 
+struct mv64xxx_i2c_regs mv64xxx_i2c_regs_sun4i = {
+	.addr		= 0x00,
+	.ext_addr	= 0x04,
+	.data		= 0x08,
+	.control	= 0x0c,
+	.status		= 0x10,
+	.clock		= 0x14,
+	.soft_reset	= 0x18,
+};
+
 /* i2c Platform Device, Driver Data */
 struct mv64xxx_i2c_pdata {
 	u32			freq_m;