| Message ID | 20120831122323.GC5962@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, Aug 31, 2012 at 2:23 PM, Lee Jones <lee.jones@linaro.org> wrote: (...) > static int nmk_i2c_probe(struct amba_device *adev, const struct amba_id *id) > { > int ret = 0; > struct nmk_i2c_controller *pdata = adev->dev.platform_data; > + struct device_node *np = adev->dev.of_node; > struct nmk_i2c_dev *dev; > struct i2c_adapter *adap; > > + if (np) { > + if (!pdata) { So, if no pdata is provided, we go on to allocate some ... > + pdata = devm_kzalloc(&adev->dev, sizeof(*pdata), GFP_KERNEL); > + if (!pdata) { > + ret = -ENOMEM; > + goto err_no_mem; > + } > + } > + /* Provide the default configuration as a base. */ > + pdata = &u8500_i2c; Then you just override that pointer with a pointer to the local config. > + nmk_i2c_of_probe(np, pdata); > + } > + > if (!pdata) > /* No i2c configuration found, using the default. */ > pdata = &u8500_i2c; This in it's entirety does not look sound. I *think* this is what you want to do, replace all of the above codde (including the last if (!pdata) clause) with: if (!pdata) { /* If no platform data passed in, use the default configuration as a base. */ pdata = &u8500_i2c; if (np) /* Further, if we have a DT node, override the default with this */ nmk_i2c_of_probe(np, pdata); } This makes any passed pdata take precedence, else default pdata complemented with DT info. Which is what we want. Yours, Linus Walleij
On Mon, Sep 03, 2012 at 11:22:28AM +0200, Linus Walleij wrote: > On Fri, Aug 31, 2012 at 2:23 PM, Lee Jones <lee.jones@linaro.org> wrote: > > (...) > > static int nmk_i2c_probe(struct amba_device *adev, const struct amba_id *id) > > { > > int ret = 0; > > struct nmk_i2c_controller *pdata = adev->dev.platform_data; > > + struct device_node *np = adev->dev.of_node; > > struct nmk_i2c_dev *dev; > > struct i2c_adapter *adap; > > > > + if (np) { > > + if (!pdata) { > > So, if no pdata is provided, we go on to allocate some ... > > > + pdata = devm_kzalloc(&adev->dev, sizeof(*pdata), GFP_KERNEL); > > + if (!pdata) { > > + ret = -ENOMEM; > > + goto err_no_mem; > > + } > > + } > > + /* Provide the default configuration as a base. */ > > + pdata = &u8500_i2c; > > Then you just override that pointer with a pointer to the local config. > > > + nmk_i2c_of_probe(np, pdata); > > + } > > + > > if (!pdata) > > /* No i2c configuration found, using the default. */ > > pdata = &u8500_i2c; > > This in it's entirety does not look sound. I *think* this is what you > want to do, > replace all of the above codde (including the last if (!pdata) clause) with: > > if (!pdata) { > /* If no platform data passed in, use the default configuration as > a base. */ > pdata = &u8500_i2c; > if (np) > /* Further, if we have a DT node, override the default with this */ > nmk_i2c_of_probe(np, pdata); > } > > This makes any passed pdata take precedence, else default pdata > complemented with DT info. Which is what we want. No. of_probe modifies pdata which in this case the default config which might already be in use. So, you will get problems if you have two instances with different configuration. So, we need to allocate memory but copy the content of the default data. The patch above just copies the pointer which is bogus.
On Mon, Sep 03, 2012 at 11:44:48AM +0200, Wolfram Sang wrote: > On Mon, Sep 03, 2012 at 11:22:28AM +0200, Linus Walleij wrote: > > On Fri, Aug 31, 2012 at 2:23 PM, Lee Jones <lee.jones@linaro.org> wrote: > > > > (...) > > > static int nmk_i2c_probe(struct amba_device *adev, const struct amba_id *id) > > > { > > > int ret = 0; > > > struct nmk_i2c_controller *pdata = adev->dev.platform_data; > > > + struct device_node *np = adev->dev.of_node; > > > struct nmk_i2c_dev *dev; > > > struct i2c_adapter *adap; > > > > > > + if (np) { > > > + if (!pdata) { > > > > So, if no pdata is provided, we go on to allocate some ... > > > > > + pdata = devm_kzalloc(&adev->dev, sizeof(*pdata), GFP_KERNEL); > > > + if (!pdata) { > > > + ret = -ENOMEM; > > > + goto err_no_mem; > > > + } > > > + } > > > + /* Provide the default configuration as a base. */ > > > + pdata = &u8500_i2c; > > > > Then you just override that pointer with a pointer to the local config. > > > > > + nmk_i2c_of_probe(np, pdata); > > > + } > > > + > > > if (!pdata) > > > /* No i2c configuration found, using the default. */ > > > pdata = &u8500_i2c; > > > > This in it's entirety does not look sound. I *think* this is what you > > want to do, > > replace all of the above codde (including the last if (!pdata) clause) with: > > > > if (!pdata) { > > /* If no platform data passed in, use the default configuration as > > a base. */ > > pdata = &u8500_i2c; > > if (np) > > /* Further, if we have a DT node, override the default with this */ > > nmk_i2c_of_probe(np, pdata); > > } > > > > This makes any passed pdata take precedence, else default pdata > > complemented with DT info. Which is what we want. > > No. of_probe modifies pdata which in this case the default config which > might already be in use. So, you will get problems if you have two > instances with different configuration. So, we need to allocate memory > but copy the content of the default data. The patch above just copies > the pointer which is bogus. Agreed. I'll fixup.
diff --git a/drivers/i2c/busses/i2c-nomadik.c b/drivers/i2c/busses/i2c-nomadik.c index 61b00ed..2c85de1 100644 --- a/drivers/i2c/busses/i2c-nomadik.c +++ b/drivers/i2c/busses/i2c-nomadik.c @@ -25,6 +25,7 @@ #include <linux/regulator/consumer.h> #include <linux/pm_runtime.h> #include <linux/platform_data/i2c-nomadik.h> +#include <linux/of.h> #define DRIVER_NAME "nmk-i2c" @@ -920,15 +921,41 @@ static struct nmk_i2c_controller u8500_i2c = { .sm = I2C_FREQ_MODE_FAST, }; +static void nmk_i2c_of_probe(struct device_node *np, + struct nmk_i2c_controller *pdata) +{ + of_property_read_u32(np, "clock-frequency", &pdata->clk_freq); + + /* This driver only supports 'standard' and 'fast' modes of operation. */ + if (pdata->clk_freq <= 100000) + pdata->sm = I2C_FREQ_MODE_STANDARD; + else + pdata->sm = I2C_FREQ_MODE_FAST; +} + static atomic_t adapter_id = ATOMIC_INIT(0); static int nmk_i2c_probe(struct amba_device *adev, const struct amba_id *id) { int ret = 0; struct nmk_i2c_controller *pdata = adev->dev.platform_data; + struct device_node *np = adev->dev.of_node; struct nmk_i2c_dev *dev; struct i2c_adapter *adap; + 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. */ + pdata = &u8500_i2c; + nmk_i2c_of_probe(np, pdata); + } + if (!pdata) /* No i2c configuration found, using the default. */ pdata = &u8500_i2c; diff --git a/include/linux/platform_data/i2c-nomadik.h b/include/linux/platform_data/i2c-nomadik.h index c2303c3..3a8be9c 100644 --- a/include/linux/platform_data/i2c-nomadik.h +++ b/include/linux/platform_data/i2c-nomadik.h @@ -28,7 +28,7 @@ enum i2c_freq_mode { * @sm: speed mode */ struct nmk_i2c_controller { - unsigned long clk_freq; + u32 clk_freq; unsigned short slsu; unsigned char tft; unsigned char rft;
Hopefully this is more to your liking: Author: Lee Jones <lee.jones@linaro.org> Date: Mon Aug 6 11:09:57 2012 +0100 i2c: nomadik: Add Device Tree support to the Nomadik I2C driver Here we apply the bindings required for successful Device Tree probing of the i2c-nomadik driver. Cc: linux-i2c@vger.kernel.org Signed-off-by: Lee Jones <lee.jones@linaro.org>