Re: [PATCH v4 1/3] i2c-mpc: use __devinit[data] for initialization functions and data

[Grant Likely <grant.likely-s3s/WqlpOiPyB63q8FvJNQ@public.gmane.org>]

On Thu, Jan 28, 2010 at 6:25 AM, Wolfgang Grandegger <wg-5Yr1BZd7O62+XT7JhA+gdA@public.gmane.org> wrote:
> From: Wolfgang Grandegger <wg-ynQEQJNshbs@public.gmane.org>
>
> "__devinit[data]" has not yet been used for all initialization functions
> and data. To avoid truncating lines, the struct "mpc_i2c_match_data" has
> been renamed to "mpc_i2c_data", which is even the better name.
>
> Signed-off-by: Wolfgang Grandegger <wg-ynQEQJNshbs@public.gmane.org>

Several comments below.

> ---
>  drivers/i2c/busses/i2c-mpc.c |  103 +++++++++++++++++++----------------------
>  1 files changed, 48 insertions(+), 55 deletions(-)
>
> diff --git a/drivers/i2c/busses/i2c-mpc.c b/drivers/i2c/busses/i2c-mpc.c
> index f627001..275ebe6 100644
> --- a/drivers/i2c/busses/i2c-mpc.c
> +++ b/drivers/i2c/busses/i2c-mpc.c
> @@ -66,7 +66,7 @@ struct mpc_i2c_divider {
>        u16 fdr;        /* including dfsrr */
>  };
>
> -struct mpc_i2c_match_data {
> +struct mpc_i2c_data {
>        void (*setclock)(struct device_node *node,
>                         struct mpc_i2c *i2c,
>                         u32 clock, u32 prescaler);
> @@ -165,7 +165,7 @@ static int i2c_wait(struct mpc_i2c *i2c, unsigned timeout, int writing)
>  }
>
>  #ifdef CONFIG_PPC_MPC52xx
> -static const struct mpc_i2c_divider mpc_i2c_dividers_52xx[] = {
> +static const struct __devinitdata mpc_i2c_divider mpc_i2c_dividers_52xx[] = {

__devinitdata goes at the end, immediately before the '='.  Ditto
throughout the file.

>        {20, 0x20}, {22, 0x21}, {24, 0x22}, {26, 0x23},
>        {28, 0x24}, {30, 0x01}, {32, 0x25}, {34, 0x02},
>        {36, 0x26}, {40, 0x27}, {44, 0x04}, {48, 0x28},
> @@ -582,44 +584,35 @@ static int __devexit fsl_i2c_remove(struct of_device *op)
>        return 0;
>  };
>
> +static struct mpc_i2c_data __devinitdata mpc_i2c_data_52xx = {
> +       .setclock = mpc_i2c_setclock_52xx,
> +};
> +
> +static struct mpc_i2c_data __devinitdata mpc_i2c_data_8313 = {
> +       .setclock = mpc_i2c_setclock_8xxx,
> +};
> +
> +static struct mpc_i2c_data __devinitdata mpc_i2c_data_8543 = {
> +       .setclock = mpc_i2c_setclock_8xxx,
> +       .prescaler = 2,
> +};
> +
> +static struct mpc_i2c_data __devinitdata mpc_i2c_data_8544 = {
> +       .setclock = mpc_i2c_setclock_8xxx,
> +       .prescaler = 3,
> +};
> +
>  static const struct of_device_id mpc_i2c_of_match[] = {

You can make this __devinitdata too.

> -       {.compatible = "mpc5200-i2c",
> -        .data = &(struct mpc_i2c_match_data) {
> -                       .setclock = mpc_i2c_setclock_52xx,
> -               },
> -       },
> -       {.compatible = "fsl,mpc5200b-i2c",
> -        .data = &(struct mpc_i2c_match_data) {
> -                       .setclock = mpc_i2c_setclock_52xx,
> -               },
> -       },
> -       {.compatible = "fsl,mpc5200-i2c",
> -        .data = &(struct mpc_i2c_match_data) {
> -                       .setclock = mpc_i2c_setclock_52xx,
> -               },
> -       },
> -       {.compatible = "fsl,mpc8313-i2c",
> -        .data = &(struct mpc_i2c_match_data) {
> -                       .setclock = mpc_i2c_setclock_8xxx,
> -               },
> -       },
> -       {.compatible = "fsl,mpc8543-i2c",
> -        .data = &(struct mpc_i2c_match_data) {
> -                       .setclock = mpc_i2c_setclock_8xxx,
> -                       .prescaler = 2,
> -               },
> -       },
> -       {.compatible = "fsl,mpc8544-i2c",
> -        .data = &(struct mpc_i2c_match_data) {
> -                       .setclock = mpc_i2c_setclock_8xxx,
> -                       .prescaler = 3,
> -               },
> +       {.compatible = "mpc5200-i2c", .data = &mpc_i2c_data_52xx, },
> +       {.compatible = "fsl,mpc5200b-i2c", .data = &mpc_i2c_data_52xx, },
> +       {.compatible = "fsl,mpc5200-i2c", .data = &mpc_i2c_data_52xx, },
> +       {.compatible = "fsl,mpc8313-i2c", .data = &mpc_i2c_data_8313, },
> +       {.compatible = "fsl,mpc8543-i2c", .data = &mpc_i2c_data_8543, },
> +       {.compatible = "fsl,mpc8544-i2c", .data = &mpc_i2c_data_8544, },
>        /* Backward compatibility */
> -       },
>        {.compatible = "fsl-i2c", },
>        {},
>  };
> -
>  MODULE_DEVICE_TABLE(of, mpc_i2c_of_match);
>
>
> @@ -634,7 +627,7 @@ static struct of_platform_driver mpc_i2c_driver = {
>        },
>  };
>
> -static int __init fsl_i2c_init(void)
> +static int __devinit fsl_i2c_init(void)

No, __init was correct.  This is called as a module_init() not as a .probe hook.

>  {
>        int rv;
>
> @@ -645,7 +638,7 @@ static int __init fsl_i2c_init(void)
>        return rv;
>  }
>
> -static void __exit fsl_i2c_exit(void)
> +static void __devexit fsl_i2c_exit(void)

Same here for __exit.

>  {
>        of_unregister_platform_driver(&mpc_i2c_driver);
>  }
> --
> 1.6.2.5
>
> _______________________________________________
> Linuxppc-dev mailing list
> Linuxppc-dev-uLR06cmDAlY/bJ5BZ2RsiQ@public.gmane.org
> https://lists.ozlabs.org/listinfo/linuxppc-dev
>



-- 
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.