Re: [PATCH 3/3] i2c: nomadik: Add Device Tree support to the Nomadik I2C driver

[Linus Walleij <linus.walleij@linaro.org>]

On Mon, Sep 3, 2012 at 12:07 PM, Lee Jones <lee.jones@linaro.org> wrote:

(...)
> +       if (np) {
> +               if (!pdata) {
> +                       pdata = devm_kzalloc(&adev->dev, sizeof(*pdata), GFP_KERNEL);
> +                       if (!pdata) {
> +                               ret = -ENOMEM;
> +                               goto err_no_mem;
> +                       }
> +               }
> +               /* Provide the default configuration as a base. */
> +               memcpy(pdata, &u8500_i2c, sizeof(struct nmk_i2c_controller));

Here you blank out any pdata passed from say a board file or
whatever if pdata != NULL.

> +
> +               nmk_i2c_of_probe(np, pdata);
> +       }
> +
>         if (!pdata)
>                 /* No i2c configuration found, using the default. */
>                 pdata = &u8500_i2c;

So this is still wrong, if pdata is passed to the driver it will
not override the DT, you have the semantics the other way
around, DT overrides pdata.

Look at the switch statement in my previous comment,
just add the allocations and a memcpy() and it still holds:

if (!pdata) {
    if (np) {
              pdata = devm_kzalloc(&adev->dev, sizeof(*pdata), GFP_KERNEL);
                      if (!pdata) {
                              ret = -ENOMEM;
                              goto err_no_mem;
                      }
              }
              /* Provide the default configuration as a base. */
              memcpy(pdata, &u8500_i2c, sizeof(struct nmk_i2c_controller));
              nmk_i2c_of_probe(np, pdata);
    } else
         /* Just use the static pdata */
         pdata = &u8500_i2c;
}

Yours,
Linus Walleij