| Message ID | 1370038972-2318779-16-git-send-email-arnd@arndb.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, May 31, 2013 at 5:22 PM, Arnd Bergmann <arnd@arndb.de> wrote: > A lot of code uses the functions from of_platform.h when built for > devicetree-enabled platforms but can also be built without them. > In order to avoid using #ifdef everywhere in drivers, this > makes all the function declarations visible, which means we > can use "if (IS_ENABLED(CONFIG_OF))" in driver code and get build > coverage over the code but let the compiler drop the reference > in the object code. > > Signed-off-by: Arnd Bergmann <arnd@arndb.de> > Cc: Grant Likely <grant.likely@linaro.org> > Cc: Rob Herring <rob.herring@calxeda.com> I've got a more complete series for 3.11 that removes OF_DEVICE and of_platform_driver completely. I'm waiting on ack from Ben H. Rob > --- > include/linux/of_platform.h | 14 +++----------- > 1 file changed, 3 insertions(+), 11 deletions(-) > > diff --git a/include/linux/of_platform.h b/include/linux/of_platform.h > index 2a93b64..7747ad0 100644 > --- a/include/linux/of_platform.h > +++ b/include/linux/of_platform.h > @@ -13,8 +13,6 @@ > > #include <linux/device.h> > #include <linux/mod_devicetable.h> > - > -#ifdef CONFIG_OF_DEVICE > #include <linux/pm.h> > #include <linux/of_device.h> > #include <linux/platform_device.h> > @@ -82,7 +80,6 @@ extern struct platform_device *of_device_alloc(struct device_node *np, > struct device *parent); > extern struct platform_device *of_find_device_by_node(struct device_node *np); > > -#ifdef CONFIG_OF_ADDRESS /* device reg helpers depend on OF_ADDRESS */ > /* Platform devices and busses creation */ > extern struct platform_device *of_platform_device_create(struct device_node *np, > const char *bus_id, > @@ -91,17 +88,12 @@ extern struct platform_device *of_platform_device_create(struct device_node *np, > extern int of_platform_bus_probe(struct device_node *root, > const struct of_device_id *matches, > struct device *parent); > +#ifdef CONFIG_OF_ADDRESS > extern int of_platform_populate(struct device_node *root, > const struct of_device_id *matches, > const struct of_dev_auxdata *lookup, > struct device *parent); > -#endif /* CONFIG_OF_ADDRESS */ > - > -#endif /* CONFIG_OF_DEVICE */ > - > -#if !defined(CONFIG_OF_ADDRESS) > -struct of_dev_auxdata; > -struct device_node; > +#else > static inline int of_platform_populate(struct device_node *root, > const struct of_device_id *matches, > const struct of_dev_auxdata *lookup, > @@ -109,6 +101,6 @@ static inline int of_platform_populate(struct device_node *root, > { > return -ENODEV; > } > -#endif /* !CONFIG_OF_ADDRESS */ > +#endif > > #endif /* _LINUX_OF_PLATFORM_H */ > -- > 1.8.1.2 > > > _______________________________________________ > linux-arm-kernel mailing list > linux-arm-kernel@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
On Saturday 01 June 2013, Rob Herring wrote: > On Fri, May 31, 2013 at 5:22 PM, Arnd Bergmann <arnd@arndb.de> wrote: > > A lot of code uses the functions from of_platform.h when built for > > devicetree-enabled platforms but can also be built without them. > > In order to avoid using #ifdef everywhere in drivers, this > > makes all the function declarations visible, which means we > > can use "if (IS_ENABLED(CONFIG_OF))" in driver code and get build > > coverage over the code but let the compiler drop the reference > > in the object code. > > > > Signed-off-by: Arnd Bergmann <arnd@arndb.de> > > Cc: Grant Likely <grant.likely@linaro.org> > > Cc: Rob Herring <rob.herring@calxeda.com> > > I've got a more complete series for 3.11 that removes OF_DEVICE and > of_platform_driver completely. I'm waiting on ack from Ben H. Ok. Does your series also remove the #ifdef CONFIG_OF part from this header? Arnd
On 06/01/2013 03:03 PM, Arnd Bergmann wrote: > On Saturday 01 June 2013, Rob Herring wrote: >> On Fri, May 31, 2013 at 5:22 PM, Arnd Bergmann <arnd@arndb.de> wrote: >>> A lot of code uses the functions from of_platform.h when built for >>> devicetree-enabled platforms but can also be built without them. >>> In order to avoid using #ifdef everywhere in drivers, this >>> makes all the function declarations visible, which means we >>> can use "if (IS_ENABLED(CONFIG_OF))" in driver code and get build >>> coverage over the code but let the compiler drop the reference >>> in the object code. >>> >>> Signed-off-by: Arnd Bergmann <arnd@arndb.de> >>> Cc: Grant Likely <grant.likely@linaro.org> >>> Cc: Rob Herring <rob.herring@calxeda.com> >> >> I've got a more complete series for 3.11 that removes OF_DEVICE and >> of_platform_driver completely. I'm waiting on ack from Ben H. > > Ok. Does your series also remove the #ifdef CONFIG_OF part from this > header? No, we still need empty functions. Here is what of_device.h looks like: http://tinyurl.com/l2azz5m BTW, it has your ack. Rob
On Saturday 01 June 2013, Rob Herring wrote: > No, we still need empty functions. Here is what of_device.h looks like: > > http://tinyurl.com/l2azz5m > > BTW, it has your ack. > Could you add a patch on top that only puts the function declarations inside of #ifdef that don't have an inline wrapper? It's really annoying to have to change the header file every time one needs to call a function from a driver in the DT-only case. Arnd
On Sat, Jun 1, 2013 at 3:57 PM, Arnd Bergmann <arnd@arndb.de> wrote: > On Saturday 01 June 2013, Rob Herring wrote: >> No, we still need empty functions. Here is what of_device.h looks like: >> >> http://tinyurl.com/l2azz5m >> >> BTW, it has your ack. >> > > Could you add a patch on top that only puts the function declarations > inside of #ifdef that don't have an inline wrapper? I'm confused. You mean that DO have an inline? Like this: void foo(void); #ifdef CONFIG_OF void bar(void); #else static inline void bar(void) {} #endif > It's really annoying to have to change the header file every time one > needs to call a function from a driver in the DT-only case. The functions without inlines are ones that drivers should not call and should only be called from OF enabled code. That's why we have not done a complete pass of adding inlines for everything. Rob
On Tuesday 04 June 2013, Rob Herring wrote: > On Sat, Jun 1, 2013 at 3:57 PM, Arnd Bergmann <arnd@arndb.de> wrote: > > On Saturday 01 June 2013, Rob Herring wrote: > >> No, we still need empty functions. Here is what of_device.h looks like: > >> > >> http://tinyurl.com/l2azz5m > >> > >> BTW, it has your ack. > >> > > > > Could you add a patch on top that only puts the function declarations > > inside of #ifdef that don't have an inline wrapper? > > I'm confused. You mean that DO have an inline? Like this: > > void foo(void); > > #ifdef CONFIG_OF > void bar(void); > #else > static inline void bar(void) {} > #endif Yes, sorry. That's what I meant. > > It's really annoying to have to change the header file every time one > > needs to call a function from a driver in the DT-only case. > > The functions without inlines are ones that drivers should not call > and should only be called from OF enabled code. That's why we have not > done a complete pass of adding inlines for everything. The problem is that it's a bad default. The convention in kernel code is not to hide declarations in #ifdef, for multiple reasons: * It avoids unnecessary code rebuilds when the symbol changes between two 'make' runs. * It lets drivers and platform code call the function from dead code without causing a build error, thus improving compile coverage. * It's much nicer to read when can write your code like void __init exynos_init_io(struct map_desc *mach_desc, int size) { if (IS_ENABLED_(CONFIG_OF) && initial_boot_params) of_scan_flat_dt(exynos_fdt_map_chipid, NULL); else iotable_init(exynos_iodesc, ARRAY_SIZE(exynos_iodesc)); ... } instead of void __init exynos_init_io(struct map_desc *mach_desc, int size) { #ifdef CONFIG_OF if (initial_boot_params) of_scan_flat_dt(exynos_fdt_map_chipid, NULL); else #endif iotable_init(exynos_iodesc, ARRAY_SIZE(exynos_iodesc)); ... } The first one looks like actual C code, the second is really ugly, and an inline wrapper wouldn't even do the right thing here. Arnd
On Tue, Jun 4, 2013 at 12:51 PM, Arnd Bergmann <arnd@arndb.de> wrote: > On Tuesday 04 June 2013, Rob Herring wrote: >> On Sat, Jun 1, 2013 at 3:57 PM, Arnd Bergmann <arnd@arndb.de> wrote: >> > On Saturday 01 June 2013, Rob Herring wrote: >> >> No, we still need empty functions. Here is what of_device.h looks like: >> >> >> >> http://tinyurl.com/l2azz5m >> >> >> >> BTW, it has your ack. >> >> >> > >> > Could you add a patch on top that only puts the function declarations >> > inside of #ifdef that don't have an inline wrapper? >> >> I'm confused. You mean that DO have an inline? Like this: >> >> void foo(void); >> >> #ifdef CONFIG_OF >> void bar(void); >> #else >> static inline void bar(void) {} >> #endif > > Yes, sorry. That's what I meant. > >> > It's really annoying to have to change the header file every time one >> > needs to call a function from a driver in the DT-only case. >> >> The functions without inlines are ones that drivers should not call >> and should only be called from OF enabled code. That's why we have not >> done a complete pass of adding inlines for everything. > > The problem is that it's a bad default. The convention in kernel code > is not to hide declarations in #ifdef, for multiple reasons: > > * It avoids unnecessary code rebuilds when the symbol changes between > two 'make' runs. > > * It lets drivers and platform code call the function from dead code > without causing a build error, thus improving compile coverage. > > * It's much nicer to read when can write your code like > > void __init exynos_init_io(struct map_desc *mach_desc, int size) > { > if (IS_ENABLED_(CONFIG_OF) && initial_boot_params) > of_scan_flat_dt(exynos_fdt_map_chipid, NULL); > else > iotable_init(exynos_iodesc, ARRAY_SIZE(exynos_iodesc)); > ... > } > > instead of > > void __init exynos_init_io(struct map_desc *mach_desc, int size) > { > #ifdef CONFIG_OF > if (initial_boot_params) > of_scan_flat_dt(exynos_fdt_map_chipid, NULL); > else > #endif > iotable_init(exynos_iodesc, ARRAY_SIZE(exynos_iodesc)); > ... > } > > The first one looks like actual C code, the second is really ugly, > and an inline wrapper wouldn't even do the right thing here. Right. I get all that. You still have to go add inlines if you want to make: if (IS_ENABLED(CONFIG_OF)) of_foo(); just be: of_foo(); There are situations for both and only inlines cover both cases. I don't see a reason we would want to allow the first case and not allow the second case. I am tired of taking patches adding the inlines 1 by 1, so perhaps we need to refactor the OF headers to better separate core infrastructure includes vs. driver only includes if that is really a concern. Rob
On Tuesday 04 June 2013 17:24:51 Rob Herring wrote: > > Right. I get all that. You still have to go add inlines if you want to make: > > if (IS_ENABLED(CONFIG_OF)) > of_foo(); > > just be: > > of_foo(); > > There are situations for both and only inlines cover both cases. I > don't see a reason we would want to allow the first case and not allow > the second case. I am tired of taking patches adding the inlines 1 by > 1, so perhaps we need to refactor the OF headers to better separate > core infrastructure includes vs. driver only includes if that is > really a concern. Ok, fair enough. Putting internal declarations into a local header file would be best of course. Arnd
On Tue, 4 Jun 2013 17:24:51 -0500, Rob Herring <robherring2@gmail.com> wrote: > On Tue, Jun 4, 2013 at 12:51 PM, Arnd Bergmann <arnd@arndb.de> wrote: > > On Tuesday 04 June 2013, Rob Herring wrote: > >> On Sat, Jun 1, 2013 at 3:57 PM, Arnd Bergmann <arnd@arndb.de> wrote: > >> > On Saturday 01 June 2013, Rob Herring wrote: > >> >> No, we still need empty functions. Here is what of_device.h looks like: > >> >> > >> >> http://tinyurl.com/l2azz5m > >> >> > >> >> BTW, it has your ack. > >> >> > >> > > >> > Could you add a patch on top that only puts the function declarations > >> > inside of #ifdef that don't have an inline wrapper? > >> > >> I'm confused. You mean that DO have an inline? Like this: > >> > >> void foo(void); > >> > >> #ifdef CONFIG_OF > >> void bar(void); > >> #else > >> static inline void bar(void) {} > >> #endif > > > > Yes, sorry. That's what I meant. > > > >> > It's really annoying to have to change the header file every time one > >> > needs to call a function from a driver in the DT-only case. > >> > >> The functions without inlines are ones that drivers should not call > >> and should only be called from OF enabled code. That's why we have not > >> done a complete pass of adding inlines for everything. > > > > The problem is that it's a bad default. The convention in kernel code > > is not to hide declarations in #ifdef, for multiple reasons: > > > > * It avoids unnecessary code rebuilds when the symbol changes between > > two 'make' runs. > > > > * It lets drivers and platform code call the function from dead code > > without causing a build error, thus improving compile coverage. > > > > * It's much nicer to read when can write your code like > > > > void __init exynos_init_io(struct map_desc *mach_desc, int size) > > { > > if (IS_ENABLED_(CONFIG_OF) && initial_boot_params) > > of_scan_flat_dt(exynos_fdt_map_chipid, NULL); > > else > > iotable_init(exynos_iodesc, ARRAY_SIZE(exynos_iodesc)); > > ... > > } > > > > instead of > > > > void __init exynos_init_io(struct map_desc *mach_desc, int size) > > { > > #ifdef CONFIG_OF > > if (initial_boot_params) > > of_scan_flat_dt(exynos_fdt_map_chipid, NULL); > > else > > #endif > > iotable_init(exynos_iodesc, ARRAY_SIZE(exynos_iodesc)); > > ... > > } A big part of the reason for not having the headers was to discourage code like the above; so instead of having a whole bunch of DT functions inline in a drivers probe hook, they would be collected off in a DT parse function that can all get compiled away as a block. However, that's the first time I've thought about the code coverage issue and it is a good point. > > > > The first one looks like actual C code, the second is really ugly, > > and an inline wrapper wouldn't even do the right thing here. > > Right. I get all that. You still have to go add inlines if you want to make: > > if (IS_ENABLED(CONFIG_OF)) > of_foo(); > > just be: > > of_foo(); > > There are situations for both and only inlines cover both cases. I > don't see a reason we would want to allow the first case and not allow > the second case. I am tired of taking patches adding the inlines 1 by > 1, so perhaps we need to refactor the OF headers to better separate > core infrastructure includes vs. driver only includes if that is > really a concern. I'm fine with that. Attitudes have changed quite a bit in the last few years about DT code in device drivers so it isn't as important to make a hard distinction about when DT functions can be called. g.
diff --git a/include/linux/of_platform.h b/include/linux/of_platform.h index 2a93b64..7747ad0 100644 --- a/include/linux/of_platform.h +++ b/include/linux/of_platform.h @@ -13,8 +13,6 @@ #include <linux/device.h> #include <linux/mod_devicetable.h> - -#ifdef CONFIG_OF_DEVICE #include <linux/pm.h> #include <linux/of_device.h> #include <linux/platform_device.h> @@ -82,7 +80,6 @@ extern struct platform_device *of_device_alloc(struct device_node *np, struct device *parent); extern struct platform_device *of_find_device_by_node(struct device_node *np); -#ifdef CONFIG_OF_ADDRESS /* device reg helpers depend on OF_ADDRESS */ /* Platform devices and busses creation */ extern struct platform_device *of_platform_device_create(struct device_node *np, const char *bus_id, @@ -91,17 +88,12 @@ extern struct platform_device *of_platform_device_create(struct device_node *np, extern int of_platform_bus_probe(struct device_node *root, const struct of_device_id *matches, struct device *parent); +#ifdef CONFIG_OF_ADDRESS extern int of_platform_populate(struct device_node *root, const struct of_device_id *matches, const struct of_dev_auxdata *lookup, struct device *parent); -#endif /* CONFIG_OF_ADDRESS */ - -#endif /* CONFIG_OF_DEVICE */ - -#if !defined(CONFIG_OF_ADDRESS) -struct of_dev_auxdata; -struct device_node; +#else static inline int of_platform_populate(struct device_node *root, const struct of_device_id *matches, const struct of_dev_auxdata *lookup, @@ -109,6 +101,6 @@ static inline int of_platform_populate(struct device_node *root, { return -ENODEV; } -#endif /* !CONFIG_OF_ADDRESS */ +#endif #endif /* _LINUX_OF_PLATFORM_H */
A lot of code uses the functions from of_platform.h when built for devicetree-enabled platforms but can also be built without them. In order to avoid using #ifdef everywhere in drivers, this makes all the function declarations visible, which means we can use "if (IS_ENABLED(CONFIG_OF))" in driver code and get build coverage over the code but let the compiler drop the reference in the object code. Signed-off-by: Arnd Bergmann <arnd@arndb.de> Cc: Grant Likely <grant.likely@linaro.org> Cc: Rob Herring <rob.herring@calxeda.com> --- include/linux/of_platform.h | 14 +++----------- 1 file changed, 3 insertions(+), 11 deletions(-)