| Message ID | 20120903100656.GC5782@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
You're right. I rushed it to get it out the door, as I'm working on something else. Leave it with me. I'll spend more time on the semantics before posting again. On 3 September 2012 12:07, Linus Walleij <linus.walleij@linaro.org> wrote: > 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 >
Looking at this again ... On 3 September 2012 12:07, Linus Walleij <linus.walleij@linaro.org> wrote: > 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. > Right, this should be in 'if (!pdata) {}' . I'll correct this now. > > > + > > + 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. > This is correct, however. If there is DT, it should override any platform data. Actually, if there is DT then no platform data should be passed in the first place. > 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; > } > > No, this is wrong. Platform data should not override DT. If DT is enabled and passed, it should have highest priority.
On Mon, Sep 3, 2012 at 1:32 PM, Lee Jones <lee.jones@linaro.org> wrote: > No, this is wrong. Platform data should not override DT. > > If DT is enabled and passed, it should have highest priority. Oh is that so. Rob: do we have a clear consensus on this? Then we should document it in Documentation/devicetree/usage-model.txt. (That document isn't part of the binding I believe, so we could define Linux-specific behaviours in it.) I always thought it was the other way around, that pdata took priority. Usecase: hardcoded bootloader passer erroneous DT to a platform. No way out. What to do? Override with pdata. Yours, Linus Walleij
On Mon, Sep 03, 2012 at 01:58:04PM +0200, Linus Walleij wrote: > On Mon, Sep 3, 2012 at 1:32 PM, Lee Jones <lee.jones@linaro.org> wrote: > > > No, this is wrong. Platform data should not override DT. > > > > If DT is enabled and passed, it should have highest priority. > > Oh is that so. :) > Rob: do we have a clear consensus on this? Then we should document > it in Documentation/devicetree/usage-model.txt. > > (That document isn't part of the binding I believe, so we could define > Linux-specific behaviours in it.) > > I always thought it was the other way around, that pdata took priority. > > Usecase: hardcoded bootloader passer erroneous DT to a platform. > No way out. What to do? Override with pdata. > > Yours, > Linus Walleij Hmmm... I see your point, but this won't work. When booting DT booting take a different path and no platform data is passed. We can't boot DT AND register devices with platform data or else we will double probe every device. The only way to pass pdata when booting with DT is with AUX_DATA() and that's a hack to get around things we don't have support for yet. Up until now that has been DMA bindings, clock and pinctrl names and call-backs. If DT is corrupt or missing the kernel will boot using platform data, but np will always be NULL, so we don't have the problem you were alluding to above. Let me know if I didn't explain that well enough and I will have another go.
On Mon, Sep 3, 2012 at 2:34 PM, Lee Jones <lee.jones@linaro.org> wrote: > When booting DT booting take a different path and no platform data > is passed. We can't boot DT AND register devices with platform data > or else we will double probe every device. The only way to pass > pdata when booting with DT is with AUX_DATA() and that's a hack to > get around things we don't have support for yet. Up until now that > has been DMA bindings, clock and pinctrl names and call-backs. So if we pass some augmented platform data using AUX_DATA() that appears as pdata in this case, and gets discarded. Thus we cannot use AUX_DATA() to override a broken, as in "the interrupt number is wrong" device tree. > If DT is corrupt or missing the kernel will boot using platform > data, but np will always be NULL, so we don't have the problem you > were alluding to above. That was not the problem I had in mind. I had a valid, but incorrect device tree in mind. I.e the device is there, but with wrong base address, or wrong IRQ number. If pdata takes precedence, we can use AUX_DATA() to override such errors from the platform, since drivers/of/platform.c helpfully pokes in the auxdata as the platform data. I thought this was one of the reasons why auxdata exist at all. Or is the proper solution to runtime-patch the device tree per se in such cases? How is that actually done then? Yours, Linus Walleij
On Mon, Sep 03, 2012 at 03:19:13PM +0200, Linus Walleij wrote: > On Mon, Sep 3, 2012 at 2:34 PM, Lee Jones <lee.jones@linaro.org> wrote: > > > When booting DT booting take a different path and no platform data > > is passed. We can't boot DT AND register devices with platform data > > or else we will double probe every device. The only way to pass > > pdata when booting with DT is with AUX_DATA() and that's a hack to > > get around things we don't have support for yet. Up until now that > > has been DMA bindings, clock and pinctrl names and call-backs. > > So if we pass some augmented platform data using AUX_DATA() > that appears as pdata in this case, and gets discarded. > > Thus we cannot use AUX_DATA() to override a broken, as in > "the interrupt number is wrong" device tree. No, you cannot to that. You'd have to fix it in DT (which is easier). > > If DT is corrupt or missing the kernel will boot using platform > > data, but np will always be NULL, so we don't have the problem you > > were alluding to above. > > That was not the problem I had in mind. > > I had a valid, but incorrect device tree in mind. I.e the device > is there, but with wrong base address, or wrong IRQ number. > > If pdata takes precedence, we can use AUX_DATA() to > override such errors from the platform, since drivers/of/platform.c > helpfully pokes in the auxdata as the platform data. Yes, that's what happens. > I thought this was one of the reasons why auxdata exist > at all. I don't think so. I've been told that AUXDATA is just a hack. My aim is to rid the boardfile of all AUXDATA entries. > Or is the proper solution to runtime-patch the device tree > per se in such cases? How is that actually done then? No, you can't do that either. DT is only read once at boot-time. The correct solution would be to fix the broken DT.
On 09/03/2012 05:58 AM, Linus Walleij wrote: > On Mon, Sep 3, 2012 at 1:32 PM, Lee Jones <lee.jones@linaro.org> wrote: > >> No, this is wrong. Platform data should not override DT. >> >> If DT is enabled and passed, it should have highest priority. No, that's wrong. If platform data is specified, it overrides DT, so that if the DT needs any fixup, it can be provided using platform data.
On Mon, Sep 3, 2012 at 4:33 PM, Stephen Warren <swarren@nvidia.com> wrote: > On 09/03/2012 05:58 AM, Linus Walleij wrote: >> On Mon, Sep 3, 2012 at 1:32 PM, Lee Jones <lee.jones@linaro.org> wrote: >> >>> No, this is wrong. Platform data should not override DT. >>> >>> If DT is enabled and passed, it should have highest priority. > > No, that's wrong. If platform data is specified, it overrides DT, so > that if the DT needs any fixup, it can be provided using platform data. Thanks Stephen, now there are two of us saying this, Lee please follow this design pattern. (Unless Rob/Grant start shouting counter-orders...) Yours, Linus Walleij
On 09/03/2012 09:35 AM, Linus Walleij wrote: > On Mon, Sep 3, 2012 at 4:33 PM, Stephen Warren <swarren@nvidia.com> wrote: >> On 09/03/2012 05:58 AM, Linus Walleij wrote: >>> On Mon, Sep 3, 2012 at 1:32 PM, Lee Jones <lee.jones@linaro.org> wrote: >>> >>>> No, this is wrong. Platform data should not override DT. >>>> >>>> If DT is enabled and passed, it should have highest priority. >> >> No, that's wrong. If platform data is specified, it overrides DT, so >> that if the DT needs any fixup, it can be provided using platform data. > > Thanks Stephen, now there are two of us saying this, Lee please > follow this design pattern. > > (Unless Rob/Grant start shouting counter-orders...) Ideally, you only use DT or platform_data and you override DT with a new DTB. Hopefully we can ultimately remove platform_data or all but parts that can't be described in DT (i.e. function callouts). But if you are handling both, then I agree that platform_data should override DT. Rob
On Mon, Sep 03, 2012 at 10:09:34AM -0500, Rob Herring wrote: > On 09/03/2012 09:35 AM, Linus Walleij wrote: > > On Mon, Sep 3, 2012 at 4:33 PM, Stephen Warren <swarren@nvidia.com> wrote: > >> On 09/03/2012 05:58 AM, Linus Walleij wrote: > >>> On Mon, Sep 3, 2012 at 1:32 PM, Lee Jones <lee.jones@linaro.org> wrote: > >>> > >>>> No, this is wrong. Platform data should not override DT. > >>>> > >>>> If DT is enabled and passed, it should have highest priority. > >> > >> No, that's wrong. If platform data is specified, it overrides DT, so > >> that if the DT needs any fixup, it can be provided using platform data. > > > > Thanks Stephen, now there are two of us saying this, Lee please > > follow this design pattern. > > > > (Unless Rob/Grant start shouting counter-orders...) > > Ideally, you only use DT or platform_data and you override DT with a new > DTB. Hopefully we can ultimately remove platform_data or all but parts > that can't be described in DT (i.e. function callouts). Exactly. I don't believe that AUX_DATA() should be used as a facility to override DT settings from platform_data. > But if you are handling both, then I agree that platform_data should > override DT. I do agree with this, but I haven't stumbled over such a use-case yet. I have only provided; clock names, DMA settings and call-back information via AUX_DATA() thus far, and those are being removed too when a) the correct bindings are mainlined and b) I have the time.
On Monday 03 September 2012, Lee Jones wrote: > > But if you are handling both, then I agree that platform_data should > > override DT. > > I do agree with this, but I haven't stumbled over such a use-case yet. > I have only provided; clock names, DMA settings and call-back information > via AUX_DATA() thus far, and those are being removed too when a) the > correct bindings are mainlined and b) I have the time. I'd prefer if you just disallow the case where pdata and DT have conflicting information. We don't seem to have a clear rule that is enforced over the kernel, so I don't think we can rely on either one taking precedence over the other in general. In this particular case, we don't have a single board file providing a struct nmk_i2c_controller definition for platform data, so the best way to handle this IMHO is to remove the header file with the platform data definition, and just encode the defaults in the driver. Arnd
On Tue, Sep 4, 2012 at 4:28 PM, Arnd Bergmann <arnd@arndb.de> wrote: > In this particular case, we don't have a single board file providing a > struct nmk_i2c_controller definition for platform data, so the best way > to handle this IMHO is to remove the header file with the platform > data definition, and just encode the defaults in the driver. Alessandro Rubini is actively working on bridging this (and other amba_device primecells) to PCI, that is the reason why it was recently converted to an amba_device. How is he then supposed to get the proper parameters into the driver? Note that the PCI ID is no help at all since the parameters depend on what is connected to the I2C bus, not on what it itself is connected to. Isn't platform data used in such cases? Yours, Linus Walleij
> Alessandro Rubini is actively working on bridging this (and > other amba_device primecells) to PCI, that is the reason why it > was recently converted to an amba_device. Yes, I've been inactive for a while but I'm on it right now. > How is he then supposed to get the proper parameters into the > driver? Note that the PCI ID is no help at all since the parameters > depend on what is connected to the I2C bus, not on what it itself is > connected to. Isn't platform data used in such cases? I'm using platform data currently, but Davide Ciminaghi is actively working to convert the configuration to device-tree: the way we pass platform data to the pci device (and thus amba) is not considered acceptable by Peter Anvin. I'm thus asking Davide if he's happy to remove the platform data configuration path right now (I personally wouldn't be very happy, but I acknowledge it should happen, sooner or later). /alessandro
On Tue, Sep 04, 2012 at 07:27:10PM +0200, Linus Walleij wrote: > On Tue, Sep 4, 2012 at 4:28 PM, Arnd Bergmann <arnd@arndb.de> wrote: > > > In this particular case, we don't have a single board file providing a > > struct nmk_i2c_controller definition for platform data, so the best way > > to handle this IMHO is to remove the header file with the platform > > data definition, and just encode the defaults in the driver. > > Alessandro Rubini is actively working on bridging this (and > other amba_device primecells) to PCI, that is the reason why it > was recently converted to an amba_device. How is he then supposed to > get the proper parameters into the driver? Note that the PCI ID > is no help at all since the parameters depend on what is connected > to the I2C bus, not on what it itself is connected to. Isn't platform data > used in such cases? So why can't Alessandro continue to use Platform Data in the normal way?
On Wed, Sep 5, 2012 at 8:41 AM, Lee Jones <lee.jones@linaro.org> wrote: > On Tue, Sep 04, 2012 at 07:27:10PM +0200, Linus Walleij wrote: >> On Tue, Sep 4, 2012 at 4:28 PM, Arnd Bergmann <arnd@arndb.de> wrote: >> >> > In this particular case, we don't have a single board file providing a >> > struct nmk_i2c_controller definition for platform data, so the best way >> > to handle this IMHO is to remove the header file with the platform >> > data definition, and just encode the defaults in the driver. >> >> Alessandro Rubini is actively working on bridging this (and >> other amba_device primecells) to PCI, that is the reason why it >> was recently converted to an amba_device. How is he then supposed to >> get the proper parameters into the driver? Note that the PCI ID >> is no help at all since the parameters depend on what is connected >> to the I2C bus, not on what it itself is connected to. Isn't platform data >> used in such cases? > > So why can't Alessandro continue to use Platform Data in the normal way? He probably can, this is not an argument about that, what I am worried about is Arnd's suggestion to delete the platform data header if there are potential users of it. Yours, Linus Walleij
diff --git a/drivers/i2c/busses/i2c-nomadik.c b/drivers/i2c/busses/i2c-nomadik.c index 61b00ed..c2da711 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) +{ + 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. */ + memcpy(pdata, &u8500_i2c, sizeof(struct nmk_i2c_controller)); + + 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;
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>