Hello Rob, On Tue Jul 9, 2024 at 12:24 AM CEST, Rob Herring wrote: > On Mon, Jul 8, 2024 at 2:55 AM Théo Lebrun <theo.lebrun@xxxxxxxxxxx> wrote: > > > > In the !CONFIG_OF case, replace the of_match_node() macro implementation > > by a static function. This ensures drivers calling of_match_node() can > > be COMPILE_TESTed. > > > > include/linux/of.h declares of_match_node() like this: > > > > #ifdef CONFIG_OF > > extern const struct of_device_id *of_match_node( > > const struct of_device_id *matches, const struct device_node *node); > > #else > > #define of_match_node(_matches, _node) NULL > > #endif > > > > When used inside an expression, those two implementations behave truly > > differently. The macro implementation has (at least) two pitfalls: > > > > - Arguments are removed by the preprocessor meaning they do not appear > > to the compiler. This can give "defined but not used" warnings. > > It also means the arguments don't have to be defined at all which is > the reasoning the commit adding the macro gave: > > I have chosen to use a macro instead of a function to > be able to avoid defining the first parameter. > In fact, this "struct of_device_id *" first parameter > is usualy not defined as well on non-dt builds. > > We could change our mind here, but I suspect applying this would > result in some build failures. It appears like it would and I did not think about this edge-case. It doesn't appear like it is a lot of drivers. I'm seeing 221 files with calls to of_match_node(). Out of those, 22 match for CONFIG_OF. Out of those, only 9 have their of_device_id table guarded but not the of_match_node() call. Remainders fall into two categories: - call is guarded by #ifdef CONFIG_OF as well, - neither of_device_id table nor of_match_node() call are guarded. The list of remaining culprits: drivers/dma/at_hdmac.c drivers/dma/dw/rzn1-dmamux.c drivers/gpu/drm/omapdrm/omap_dmm_tiler.c drivers/i2c/busses/i2c-at91-core.c drivers/i2c/busses/i2c-xiic.c drivers/misc/atmel-ssc.c drivers/net/can/at91_can.c drivers/net/ethernet/cadence/macb_main.c sound/soc/codecs/wm8904.c There could be build errors on drivers that do not match for CONFIG_OF, as well. > > - The returned value type is (void *) > > versus (const struct of_device_id *). > > It works okay if the value is stored in a variable, thanks to C's > > implicit void pointer casting rules. It causes build errors if used > > like `of_match_data(...)->data`. > > Really, the only places of_match_node() should be used are ones > without a struct device. Otherwise, of_device_get_match_data() or > device_get_match_data() should be used instead. I completely agree. Regards, -- Théo Lebrun, Bootlin Embedded Linux and Kernel engineering https://bootlin.com
