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

[Tomasz Figa <tomasz.figa-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>]

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.

>  };
> 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;