| Message ID | 1345734087-21803-3-git-send-email-lee.jones@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, Aug 23, 2012 at 8:01 AM, Lee Jones <lee.jones@linaro.org> wrote: > Here we apply the bindings required for successful Device Tree > probing of the i2c-nomadik driver. > > Cc: linux-i2c@vger.kernel.org > Acked-by: srinidhi kasagar <srinidhi.kasagar@stericsson.com> > Signed-off-by: Lee Jones <lee.jones@linaro.org> Acked-by: Linus Walleij <linus.walleij@linaro.org> Yours, Linus Walleij
On Mon, Aug 27, 2012 at 04:42:49PM -0700, Linus Walleij wrote: > On Thu, Aug 23, 2012 at 8:01 AM, Lee Jones <lee.jones@linaro.org> wrote: > > > Here we apply the bindings required for successful Device Tree > > probing of the i2c-nomadik driver. > > > > Cc: linux-i2c@vger.kernel.org > > Acked-by: srinidhi kasagar <srinidhi.kasagar@stericsson.com> > > Signed-off-by: Lee Jones <lee.jones@linaro.org> > > Acked-by: Linus Walleij <linus.walleij@linaro.org> I'm assuming I still need Wolfram's Ack on this?
On Thu, Aug 23, 2012 at 04:01:27PM +0100, Lee Jones wrote: > Here we apply the bindings required for successful Device Tree > probing of the i2c-nomadik driver. > > Cc: linux-i2c@vger.kernel.org > Acked-by: srinidhi kasagar <srinidhi.kasagar@stericsson.com> Two acks? I'd think this cannot work for multiple reasons. BTW, patch 2 and 3 should be merged. It is a lot easier to review the code with the binding description together. Is there some dependency other than updating the dts files? If not, I'd like to pick up the patch via I2C. > Signed-off-by: Lee Jones <lee.jones@linaro.org> > --- > drivers/i2c/busses/i2c-nomadik.c | 28 ++++++++++++++++++++++++++++ > 1 file changed, 28 insertions(+) > > diff --git a/drivers/i2c/busses/i2c-nomadik.c b/drivers/i2c/busses/i2c-nomadik.c > index 61b00ed..8168389 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,42 @@ 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) > +{ > + /* Provide the default configuration as a base. */ > + pdata = &u8500_i2c; ?????? I wonder how that could work... have you tested the patch? > + > + of_property_read_u32(np, "clock-frequency", (u32*)&pdata->clk_freq); Might be worth changing clk_freq to u32? > + > + /* This driver only supports 'standard' and 'fast' modes of operation. */ > + if (pdata->clk_freq <= 100000) > + pdata->sm = I2C_FREQ_MODE_STANDARD; Is standard == 100000 Hz? > + else > + pdata->sm = I2C_FREQ_MODE_FAST; If those two are fixed frequencies, you should omit a warning if the devicetree has a different frequency set and report which one is going to be used actually. > +} > + > 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; > + } > + } > + nmk_i2c_of_probe(np, pdata); > + } > + > if (!pdata) > /* No i2c configuration found, using the default. */ > pdata = &u8500_i2c; > -- > 1.7.9.5 >
> Is there some dependency other than updating the dts files? If not, I'd > like to pick up the patch via I2C. There's no other dependency. Feel free to take them into your tree. > > +static void nmk_i2c_of_probe(struct device_node *np, > > + struct nmk_i2c_controller *pdata) > > +{ > > + /* Provide the default configuration as a base. */ > > + pdata = &u8500_i2c; > > ?????? I wonder how that could work... have you tested the patch? Wow, that's a great spot! I have tested the patch, but I don't have any i2c devices, so can't test full functionality. I, wrongly it seems, assumed there would be a complaint from the I2C subsystem if any of the values seemed wrong. What I will do before next submission is print out the entire pdata structure to ensure it's populated in the correct way. > > + > > + of_property_read_u32(np, "clock-frequency", (u32*)&pdata->clk_freq); > > Might be worth changing clk_freq to u32? Yes, makes sense. > > + > > + /* This driver only supports 'standard' and 'fast' modes of operation. */ > > + if (pdata->clk_freq <= 100000) > > + pdata->sm = I2C_FREQ_MODE_STANDARD; > > Is standard == 100000 Hz? Well it depends on how you interpret the comments in: include/linux/platform_data/i2c-nomadik.h enum i2c_freq_mode { I2C_FREQ_MODE_STANDARD, /* up to 100 Kb/s */ I2C_FREQ_MODE_FAST, /* up to 400 Kb/s */ I2C_FREQ_MODE_HIGH_SPEED, /* up to 3.4 Mb/s */ I2C_FREQ_MODE_FAST_PLUS, /* up to 1 Mb/s */ }; I guess your guess would be better than mine. > > + else > > + pdata->sm = I2C_FREQ_MODE_FAST; > > If those two are fixed frequencies, you should omit a warning if the > devicetree has a different frequency set and report which one is going > to be used actually. Well, again by the comments above I would say that in between values were valid, but I'm willing to bow down to your knowledge if you think they are fixed values? Thanks for reviewing.
diff --git a/drivers/i2c/busses/i2c-nomadik.c b/drivers/i2c/busses/i2c-nomadik.c index 61b00ed..8168389 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,42 @@ 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) +{ + /* Provide the default configuration as a base. */ + pdata = &u8500_i2c; + + of_property_read_u32(np, "clock-frequency", (u32*)&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; + } + } + nmk_i2c_of_probe(np, pdata); + } + if (!pdata) /* No i2c configuration found, using the default. */ pdata = &u8500_i2c;