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

[Maxime Ripard <maxime.ripard-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>]

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-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
> > ---
> >  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, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com