| Message ID | 1310592975-25773-5-git-send-email-manjugk@ti.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, Jul 14, 2011 at 7:06 AM, G, Manjunath Kondaiah <manjugk@ti.com> wrote: > > The i2c-omap driver is converted for supporting both > dt and non dt builds and driver is modified to use dt > data partially. > > Tested on OMAP3 beagle board. > > Signed-off-by: G, Manjunath Kondaiah <manjugk@ti.com> > --- > drivers/i2c/busses/i2c-omap.c | 48 ++++++++++++++++++++++++++++++++++++++++- > 1 files changed, 47 insertions(+), 1 deletions(-) > > diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c > index ae1545b..6d11a13 100644 > --- a/drivers/i2c/busses/i2c-omap.c > +++ b/drivers/i2c/busses/i2c-omap.c > @@ -38,9 +38,13 @@ > #include <linux/clk.h> > #include <linux/io.h> > #include <linux/of_i2c.h> > +#include <linux/of_irq.h> > +#include <linux/of_platform.h> > +#include <linux/of_address.h> > #include <linux/slab.h> > #include <linux/i2c-omap.h> > #include <linux/pm_runtime.h> > +#include <plat/i2c.h> > > /* I2C controller revisions */ > #define OMAP_I2C_REV_2 0x20 > @@ -972,6 +976,10 @@ static const struct i2c_algorithm omap_i2c_algo = { > .functionality = omap_i2c_func, > }; > > +#if defined(CONFIG_OF) > +static const struct of_device_id omap_i2c_of_match[]; > +#endif > + > static int __devinit > omap_i2c_probe(struct platform_device *pdev) > { > @@ -979,10 +987,17 @@ omap_i2c_probe(struct platform_device *pdev) > struct i2c_adapter *adap; > struct resource *mem, *irq, *ioarea; > struct omap_i2c_bus_platform_data *pdata = pdev->dev.platform_data; > +#if defined(CONFIG_OF) > + const struct of_device_id *match; > +#endif > irq_handler_t isr; > int r; > u32 speed = 0; > > +#if defined(CONFIG_OF) > + match = of_match_device(omap_i2c_of_match, &pdev->dev); > +#endif of_match_device() is an empty inline when CONFIG_OF is not defined. You can drop the #if defined() protection around this statement. > + > /* NOTE: driver uses the static register mapping */ > mem = platform_get_resource(pdev, IORESOURCE_MEM, 0); > if (!mem) { > @@ -1011,11 +1026,25 @@ omap_i2c_probe(struct platform_device *pdev) > if (pdata != NULL) { > speed = pdata->clkrate; > dev->set_mpu_wkup_lat = pdata->set_mpu_wkup_lat; > +#if defined(CONFIG_OF) > + } else if (pdev->dev.of_node) { > + u32 prop; > + if (!of_property_read_u32(pdev->dev.of_node, "clock-frequency", > + &prop)) > + speed = prop/100; > + else > + speed = 100; If you move the 'speed = 100' statement above the if(pdata != NULL) test, then this whole block can become simpler for both the pdata and DT situations. > +#else > } else { > speed = 100; /* Default speed */ > - dev->set_mpu_wkup_lat = NULL; > +#endif > } > > +#if defined(CONFIG_OF) > + /* TODO: remove this after DT depencies with hwmod are resolved */ > + if (match) > + return 0; > +#endif > dev->speed = speed; > dev->idle = 1; > dev->dev = &pdev->dev; > @@ -1096,7 +1125,9 @@ omap_i2c_probe(struct platform_device *pdev) > strlcpy(adap->name, "OMAP I2C adapter", sizeof(adap->name)); > adap->algo = &omap_i2c_algo; > adap->dev.parent = &pdev->dev; > +#if defined(CONFIG_OF) > adap->dev.of_node = pdev->dev.of_node; > +#endif The #if defined() can be safely removed here. > > /* i2c device drivers may be active on return from add_adapter() */ > adap->nr = pdev->id; > @@ -1106,7 +1137,9 @@ omap_i2c_probe(struct platform_device *pdev) > goto err_free_irq; > } > > +#if defined(CONFIG_OF) > of_i2c_register_devices(adap); > +#endif Ditto here. of_i2c_register_devices() is an empty inline when !CONFIG_OF > > return 0; > > @@ -1162,6 +1195,16 @@ static int omap_i2c_resume(struct device *dev) > return 0; > } > > +#if defined(CONFIG_OF) > +static const struct of_device_id omap_i2c_of_match[] = { > + {.compatible = "ti,omap3-i2c", }, > + {}, > +} > +MODULE_DEVICE_TABLE(of, omap_i2c_of_match); > +#else > +#define omap_i2c_of_match NULL > +#endif You can move this whole block up to where omap_i2c_of_match is forward declared, which will make the patch smaller. > + > static struct dev_pm_ops omap_i2c_pm_ops = { > .suspend = omap_i2c_suspend, > .resume = omap_i2c_resume, > @@ -1178,6 +1221,9 @@ static struct platform_driver omap_i2c_driver = { > .name = "omap_i2c", > .owner = THIS_MODULE, > .pm = OMAP_I2C_PM_OPS, > +#if defined(CONFIG_OF) > + .of_match_table = omap_i2c_of_match, > +#endif Drop the #if defined() protection. g. -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Grant, On 7/14/2011 1:20 AM, Grant Likely wrote: > On Thu, Jul 14, 2011 at 7:06 AM, G, Manjunath Kondaiah<manjugk@ti.com> wrote: >> >> The i2c-omap driver is converted for supporting both >> dt and non dt builds and driver is modified to use dt >> data partially. [...] >> /* NOTE: driver uses the static register mapping */ >> mem = platform_get_resource(pdev, IORESOURCE_MEM, 0); >> if (!mem) { >> @@ -1011,11 +1026,25 @@ omap_i2c_probe(struct platform_device *pdev) >> if (pdata != NULL) { >> speed = pdata->clkrate; >> dev->set_mpu_wkup_lat = pdata->set_mpu_wkup_lat; >> +#if defined(CONFIG_OF) >> + } else if (pdev->dev.of_node) { >> + u32 prop; >> + if (!of_property_read_u32(pdev->dev.of_node, "clock-frequency", >> +&prop)) >> + speed = prop/100; >> + else >> + speed = 100; Do we have to modify every drivers in order to take advantage of the DT bus? Cannot we init the already existing pdata during device creation and let the driver untouched? I think it is a pity to add all that #ifdefry in the driver to be able to support two kinds of bus. Cannot we have an intermediate phase that will deal only with the device creation with DT? That will allow us to clean the omap_device creation from hwmod we have today spread everywhere in plat-omap and mach-omap2. Regards Benoit -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Jul 28, 2011 at 11:34 AM, Cousson, Benoit <b-cousson@ti.com> wrote: > Hi Grant, > > On 7/14/2011 1:20 AM, Grant Likely wrote: >> >> On Thu, Jul 14, 2011 at 7:06 AM, G, Manjunath Kondaiah<manjugk@ti.com> >> wrote: >>> >>> The i2c-omap driver is converted for supporting both >>> dt and non dt builds and driver is modified to use dt >>> data partially. > > [...] > >>> /* NOTE: driver uses the static register mapping */ >>> mem = platform_get_resource(pdev, IORESOURCE_MEM, 0); >>> if (!mem) { >>> @@ -1011,11 +1026,25 @@ omap_i2c_probe(struct platform_device *pdev) >>> if (pdata != NULL) { >>> speed = pdata->clkrate; >>> dev->set_mpu_wkup_lat = pdata->set_mpu_wkup_lat; >>> +#if defined(CONFIG_OF) >>> + } else if (pdev->dev.of_node) { >>> + u32 prop; >>> + if (!of_property_read_u32(pdev->dev.of_node, >>> "clock-frequency", >>> +&prop)) >>> + speed = prop/100; >>> + else >>> + speed = 100; > > Do we have to modify every drivers in order to take advantage of the DT bus? > Cannot we init the already existing pdata during device creation and let the > driver untouched? > > I think it is a pity to add all that #ifdefry in the driver to be able to > support two kinds of bus. It's not supporting 2 different kinds of bus. There is no such think as a dt bus type (anymore. there used to be and of_platform_bus_type, but that was a mistake). DT is only a different configuration representation. > Cannot we have an intermediate phase that will deal only with the device > creation with DT? > > That will allow us to clean the omap_device creation from hwmod we have > today spread everywhere in plat-omap and mach-omap2. Consider what you're suggesting... Pretty much every driver that needs platform data has as a unique platform_data structure. That means that for every driver there needs to be a separate block of code to translate from DT data into struct <driver>_platform_data. There is no way to make that generic. That translation code needs to live somewhere, and something needs to be responsible for executing it when relevant devices appear. It can either be built into the driver, or it can be done sometime before the driver is probed. Option 1: If it is part of the driver, the sequence looks like this: ...early boot: 1) allocate and register platform_devices for DT nodes (generic code, attach device_node) ... boot progresses to initcalls... 2) probe platform_driver (DT translation code to obtain <device>_platform_data from device_node) Option 2: If it is part of platform_device registration, the sequence looks something like: ...early boot: 1) allocate and register platform_devices for DT nodes (call device specific translation code for each device to create <device>_platform_data) ...boot progresses to initcalls 2) probe platform_driver (use platform_data, nothing changes here) Option 3: decoupled from device registration and the device driver: ...early boot: 1) allocate and register platform_devices for DT nodes (generic code, attach device_node) ...boot progresses to initcalls 2) hook into device model *before* drivers get probed to call device specific translation code. 3) probe platform_driver (use platform_data, nothing changes here) I understand the desire to not change the drivers, but going with either option 2 or 3 causes a giant mess in figuring out how to make sure the correct translation code gets called. Option 2 is a disaster because the translation code for any device that may possibly be attached to the kernel must be statically linked in. It cannot be in modules because it must be available during early init. Option 2 is similarly a no-go because it requires a new hunk of infrastructure to take care of finding and executing translation code before the device can get probed by the device driver. Besides, no matter how you look at it, the DT translation code is always device-driver-specific. It isn't something that can be handled by generic code for all devices, and the *whole point* of the DT transition is to get away from board specific code during early init. Why wouldn't DT translation code live inside the one device driver that it supports? g. Note: This is only an issue if a driver requires platform data. Drivers that only need register and irq resources work without any additional code. As for the #ifdef issue, yes it can look ugly if it is done in the wrong way. Generally, I split the translation code out into a separate function that can be made an empty static inline when CONFIG_OF is not selected so that all the DT specific code can be factored out into a single #ifdef block. g. > > Regards > Benoit > > _______________________________________________ > linux-arm-kernel mailing list > linux-arm-kernel@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel >
On 7/28/2011 11:32 PM, Grant Likely wrote: > On Thu, Jul 28, 2011 at 11:34 AM, Cousson, Benoit<b-cousson@ti.com> wrote: [...] >> Do we have to modify every drivers in order to take advantage of the DT bus? >> Cannot we init the already existing pdata during device creation and let the >> driver untouched? >> >> I think it is a pity to add all that #ifdefry in the driver to be able to >> support two kinds of bus. > > It's not supporting 2 different kinds of bus. There is no such think > as a dt bus type (anymore. there used to be and of_platform_bus_type, > but that was a mistake). DT is only a different configuration > representation. That's the point, sorry for my confusing sentence. We are creating almost the exact same platform_device in both cases, but we still have to modify the platform_driver due to a change in the device creation method. No doubt that DT will clean up the messy devices creation from board file we have today in OMAP. But, for a smooth migration to DT, I would have defined a legacy mode that will avoid hacking every drivers just because we are using a different "bus enumeration" method. And then later we will be able to use DT for the configuration if needed. >> Cannot we have an intermediate phase that will deal only with the device >> creation with DT? >> >> That will allow us to clean the omap_device creation from hwmod we have >> today spread everywhere in plat-omap and mach-omap2. > > Consider what you're suggesting... Pretty much every driver that > needs platform data has as a unique platform_data structure. That > means that for every driver there needs to be a separate block of code > to translate from DT data into struct<driver>_platform_data. There > is no way to make that generic. That translation code needs to live > somewhere, and something needs to be responsible for executing it when > relevant devices appear. It can either be built into the driver, or > it can be done sometime before the driver is probed. > > Option 1: If it is part of the driver, the sequence looks like this: > ...early boot: > 1) allocate and register platform_devices for DT nodes (generic code, > attach device_node) > ... boot progresses to initcalls... > 2) probe platform_driver (DT translation code to obtain > <device>_platform_data from device_node) > > Option 2: If it is part of platform_device registration, the sequence > looks something like: > ...early boot: > 1) allocate and register platform_devices for DT nodes (call device > specific translation code for each device to create > <device>_platform_data) > ...boot progresses to initcalls > 2) probe platform_driver (use platform_data, nothing changes here) > > Option 3: decoupled from device registration and the device driver: > ...early boot: > 1) allocate and register platform_devices for DT nodes (generic code, > attach device_node) > ...boot progresses to initcalls > 2) hook into device model *before* drivers get probed to call device > specific translation code. > 3) probe platform_driver (use platform_data, nothing changes here) > > I understand the desire to not change the drivers, but going with > either option 2 or 3 causes a giant mess in figuring out how to make > sure the correct translation code gets called. Option 2 is a disaster > because the translation code for any device that may possibly be > attached to the kernel must be statically linked in. It cannot be in > modules because it must be available during early init. Option 2 is > similarly a no-go because it requires a new hunk of infrastructure to > take care of finding and executing translation code before the device > can get probed by the device driver. Mmm, cannot we have an option #4? It seems to me that you already have in place some hooks to provide pdata thanks to OF_DEV_AUXDATA. Adding a callback will potentially allow a custom board code to translate configuration from DT into legacy pdata. static struct omap_pdata i2c_pdata = { .clkrate= 400000, }; void init_i2c_pdata(...) { of_property_read_u32(pdev->dev.of_node, pdata->clkrate); } static struct of_dev_auxdata omap_auxdata_lookup[] __initdata = { OF_DEV_AUXDATA_CB("ti,omap-i2c", XXX, "i2c.1", &i2c_pdata, init_i2c_pdata), {} }; In legacy mode you get the pdata from the static board. If CONFIG_OF is set you can override the value from the callback. The driver remains the same. That being said, does it worth the effort? I'm not sure, but the idea was to allow focusing on the device creation first without hacking every drivers. Using a regular OF_DEV_AUXDATA with pdata, seems a good tradeoff already. > Besides, no matter how you look at it, the DT translation code is > always device-driver-specific. It isn't something that can be handled > by generic code for all devices, and the *whole point* of the DT > transition is to get away from board specific code during early init. Well, AFAIK, the main motivation, at least for OMAP, was to get rid of static data in the arm directory. Getting rid of board code is, I agree, quite interesting as well. > Why wouldn't DT translation code live inside the one device driver > that it supports? Because, it is still optional and force us to maintain two different methods in the driver code when pdata is needed. Ideally "of_property_xxx" will be a platform_property_xxx call always available, and DT will be one of the potential source of configuration. Drivers will then be able to use one and only one standard Linux API, and various implementation will allow the board static stuff for non-DT case or DT blob when available. That is just my .2 euros. Regards, Benoit -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c index ae1545b..6d11a13 100644 --- a/drivers/i2c/busses/i2c-omap.c +++ b/drivers/i2c/busses/i2c-omap.c @@ -38,9 +38,13 @@ #include <linux/clk.h> #include <linux/io.h> #include <linux/of_i2c.h> +#include <linux/of_irq.h> +#include <linux/of_platform.h> +#include <linux/of_address.h> #include <linux/slab.h> #include <linux/i2c-omap.h> #include <linux/pm_runtime.h> +#include <plat/i2c.h> /* I2C controller revisions */ #define OMAP_I2C_REV_2 0x20 @@ -972,6 +976,10 @@ static const struct i2c_algorithm omap_i2c_algo = { .functionality = omap_i2c_func, }; +#if defined(CONFIG_OF) +static const struct of_device_id omap_i2c_of_match[]; +#endif + static int __devinit omap_i2c_probe(struct platform_device *pdev) { @@ -979,10 +987,17 @@ omap_i2c_probe(struct platform_device *pdev) struct i2c_adapter *adap; struct resource *mem, *irq, *ioarea; struct omap_i2c_bus_platform_data *pdata = pdev->dev.platform_data; +#if defined(CONFIG_OF) + const struct of_device_id *match; +#endif irq_handler_t isr; int r; u32 speed = 0; +#if defined(CONFIG_OF) + match = of_match_device(omap_i2c_of_match, &pdev->dev); +#endif + /* NOTE: driver uses the static register mapping */ mem = platform_get_resource(pdev, IORESOURCE_MEM, 0); if (!mem) { @@ -1011,11 +1026,25 @@ omap_i2c_probe(struct platform_device *pdev) if (pdata != NULL) { speed = pdata->clkrate; dev->set_mpu_wkup_lat = pdata->set_mpu_wkup_lat; +#if defined(CONFIG_OF) + } else if (pdev->dev.of_node) { + u32 prop; + if (!of_property_read_u32(pdev->dev.of_node, "clock-frequency", + &prop)) + speed = prop/100; + else + speed = 100; +#else } else { speed = 100; /* Default speed */ - dev->set_mpu_wkup_lat = NULL; +#endif } +#if defined(CONFIG_OF) + /* TODO: remove this after DT depencies with hwmod are resolved */ + if (match) + return 0; +#endif dev->speed = speed; dev->idle = 1; dev->dev = &pdev->dev; @@ -1096,7 +1125,9 @@ omap_i2c_probe(struct platform_device *pdev) strlcpy(adap->name, "OMAP I2C adapter", sizeof(adap->name)); adap->algo = &omap_i2c_algo; adap->dev.parent = &pdev->dev; +#if defined(CONFIG_OF) adap->dev.of_node = pdev->dev.of_node; +#endif /* i2c device drivers may be active on return from add_adapter() */ adap->nr = pdev->id; @@ -1106,7 +1137,9 @@ omap_i2c_probe(struct platform_device *pdev) goto err_free_irq; } +#if defined(CONFIG_OF) of_i2c_register_devices(adap); +#endif return 0; @@ -1162,6 +1195,16 @@ static int omap_i2c_resume(struct device *dev) return 0; } +#if defined(CONFIG_OF) +static const struct of_device_id omap_i2c_of_match[] = { + {.compatible = "ti,omap3-i2c", }, + {}, +} +MODULE_DEVICE_TABLE(of, omap_i2c_of_match); +#else +#define omap_i2c_of_match NULL +#endif + static struct dev_pm_ops omap_i2c_pm_ops = { .suspend = omap_i2c_suspend, .resume = omap_i2c_resume, @@ -1178,6 +1221,9 @@ static struct platform_driver omap_i2c_driver = { .name = "omap_i2c", .owner = THIS_MODULE, .pm = OMAP_I2C_PM_OPS, +#if defined(CONFIG_OF) + .of_match_table = omap_i2c_of_match, +#endif }, };
The i2c-omap driver is converted for supporting both dt and non dt builds and driver is modified to use dt data partially. Tested on OMAP3 beagle board. Signed-off-by: G, Manjunath Kondaiah <manjugk@ti.com> --- drivers/i2c/busses/i2c-omap.c | 48 ++++++++++++++++++++++++++++++++++++++++- 1 files changed, 47 insertions(+), 1 deletions(-)