| Message ID | 20180226183849.11562-3-vz@mleia.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Herbert Xu |
| Headers | show |
On Mon, 26 Feb 2018 20:38:49 +0200 Vladimir Zapolskiy <vz@mleia.com> wrote: > +#ifdef CONFIG_OF > +static const struct of_device_id mxc_rnga_of_match[] = { > + { .compatible = "fsl,imx31-rnga", }, > + { /* sentinel */ }, > +}; > +MODULE_DEVICE_TABLE(of, mxc_rnga_of_match); > +#endif > + > static struct platform_driver mxc_rnga_driver = { > .driver = { > - .name = "mxc_rnga", > - }, > + .name = "mxc_rnga", > + .of_match_table = of_match_ptr(mxc_rnga_of_match), Does this build if CONFIG_OF is not set? Thanks, Kim
On 02/27/2018 05:49 PM, Kim Phillips wrote: > On Mon, 26 Feb 2018 20:38:49 +0200 > Vladimir Zapolskiy <vz@mleia.com> wrote: > >> +#ifdef CONFIG_OF >> +static const struct of_device_id mxc_rnga_of_match[] = { >> + { .compatible = "fsl,imx31-rnga", }, >> + { /* sentinel */ }, >> +}; >> +MODULE_DEVICE_TABLE(of, mxc_rnga_of_match); >> +#endif >> + >> static struct platform_driver mxc_rnga_driver = { >> .driver = { >> - .name = "mxc_rnga", >> - }, >> + .name = "mxc_rnga", >> + .of_match_table = of_match_ptr(mxc_rnga_of_match), > > Does this build if CONFIG_OF is not set? > Definitely it is expected to be built, you can verify it directly or check of_match_ptr() macro definition from include/linux/of.h -- With best wishes, Vladimir
On Tue, 27 Feb 2018 18:53:08 +0200 Vladimir Zapolskiy <vz@mleia.com> wrote: > On 02/27/2018 05:49 PM, Kim Phillips wrote: > > On Mon, 26 Feb 2018 20:38:49 +0200 > > Vladimir Zapolskiy <vz@mleia.com> wrote: > > > >> +#ifdef CONFIG_OF > >> +static const struct of_device_id mxc_rnga_of_match[] = { > >> + { .compatible = "fsl,imx31-rnga", }, > >> + { /* sentinel */ }, > >> +}; > >> +MODULE_DEVICE_TABLE(of, mxc_rnga_of_match); > >> +#endif > >> + > >> static struct platform_driver mxc_rnga_driver = { > >> .driver = { > >> - .name = "mxc_rnga", > >> - }, > >> + .name = "mxc_rnga", > >> + .of_match_table = of_match_ptr(mxc_rnga_of_match), > > > > Does this build if CONFIG_OF is not set? > > Definitely it is expected to be built, you can verify it directly or > check of_match_ptr() macro definition from include/linux/of.h Thanks, I verified it by removing the SOC_IMX31 dependency, and with netwinder_defconfig as a base. I also verified that the #ifdef CONFIG_OF protecting the mxc_rnga_of_match definition is also not needed. Kim
On 02/27/2018 09:39 PM, Kim Phillips wrote: > On Tue, 27 Feb 2018 18:53:08 +0200 > Vladimir Zapolskiy <vz@mleia.com> wrote: > >> On 02/27/2018 05:49 PM, Kim Phillips wrote: >>> On Mon, 26 Feb 2018 20:38:49 +0200 >>> Vladimir Zapolskiy <vz@mleia.com> wrote: >>> >>>> +#ifdef CONFIG_OF >>>> +static const struct of_device_id mxc_rnga_of_match[] = { >>>> + { .compatible = "fsl,imx31-rnga", }, >>>> + { /* sentinel */ }, >>>> +}; >>>> +MODULE_DEVICE_TABLE(of, mxc_rnga_of_match); >>>> +#endif >>>> + >>>> static struct platform_driver mxc_rnga_driver = { >>>> .driver = { >>>> - .name = "mxc_rnga", >>>> - }, >>>> + .name = "mxc_rnga", >>>> + .of_match_table = of_match_ptr(mxc_rnga_of_match), >>> >>> Does this build if CONFIG_OF is not set? >> >> Definitely it is expected to be built, you can verify it directly or >> check of_match_ptr() macro definition from include/linux/of.h > > Thanks, I verified it by removing the SOC_IMX31 dependency, and with > netwinder_defconfig as a base. I also verified that the #ifdef > CONFIG_OF protecting the mxc_rnga_of_match definition is also not > needed. That's a commonplace observation, but I have serious doubts, if it has become a common practice to remove CONFIG_OF and CONFIG_ACPI macro guards around device id lists. Still I would prefer to save compiled code size. Arnd or Greg, your valued opinion is much appreciated. -- With best wishes, Vladimir
On 02/27/2018 10:07 PM, Vladimir Zapolskiy wrote: > On 02/27/2018 09:39 PM, Kim Phillips wrote: >> On Tue, 27 Feb 2018 18:53:08 +0200 >> Vladimir Zapolskiy <vz@mleia.com> wrote: >> >>> On 02/27/2018 05:49 PM, Kim Phillips wrote: >>>> On Mon, 26 Feb 2018 20:38:49 +0200 >>>> Vladimir Zapolskiy <vz@mleia.com> wrote: >>>> >>>>> +#ifdef CONFIG_OF >>>>> +static const struct of_device_id mxc_rnga_of_match[] = { >>>>> + { .compatible = "fsl,imx31-rnga", }, >>>>> + { /* sentinel */ }, >>>>> +}; >>>>> +MODULE_DEVICE_TABLE(of, mxc_rnga_of_match); >>>>> +#endif >>>>> + >>>>> static struct platform_driver mxc_rnga_driver = { >>>>> .driver = { >>>>> - .name = "mxc_rnga", >>>>> - }, >>>>> + .name = "mxc_rnga", >>>>> + .of_match_table = of_match_ptr(mxc_rnga_of_match), >>>> >>>> Does this build if CONFIG_OF is not set? >>> >>> Definitely it is expected to be built, you can verify it directly or >>> check of_match_ptr() macro definition from include/linux/of.h >> >> Thanks, I verified it by removing the SOC_IMX31 dependency, and with >> netwinder_defconfig as a base. I also verified that the #ifdef >> CONFIG_OF protecting the mxc_rnga_of_match definition is also not >> needed. > > That's a commonplace observation, but I have serious doubts, if it > has become a common practice to remove CONFIG_OF and CONFIG_ACPI > macro guards around device id lists. Still I would prefer to save > compiled code size. > I checked that all flavours of i.MX SoCs are under multiplatform build. FWIW only 1 iMX31 board has DT support and 10 of them use platform data, and I'd like to change the ratio. So it would be proper to remove the set CONFIG_OF guard. But what is significantly more important is that i.MX31 RNGA should be defined as compatible with i.MX21 RNGA, and it obligates me to send v2 with the corrected compatible name, I'll make both changes. Thanks Kim for attracting my attention to possible improvements. -- With best wishes, Vladimir
diff --git a/drivers/char/hw_random/mxc-rnga.c b/drivers/char/hw_random/mxc-rnga.c index 4673622..b749e00 100644 --- a/drivers/char/hw_random/mxc-rnga.c +++ b/drivers/char/hw_random/mxc-rnga.c @@ -16,16 +16,13 @@ * This driver is based on other RNG drivers. */ -#include <linux/module.h> -#include <linux/init.h> -#include <linux/kernel.h> #include <linux/clk.h> -#include <linux/err.h> -#include <linux/ioport.h> -#include <linux/platform_device.h> -#include <linux/hw_random.h> #include <linux/delay.h> +#include <linux/hw_random.h> #include <linux/io.h> +#include <linux/module.h> +#include <linux/of.h> +#include <linux/platform_device.h> /* RNGA Registers */ #define RNGA_CONTROL 0x00 @@ -197,10 +194,19 @@ static int __exit mxc_rnga_remove(struct platform_device *pdev) return 0; } +#ifdef CONFIG_OF +static const struct of_device_id mxc_rnga_of_match[] = { + { .compatible = "fsl,imx31-rnga", }, + { /* sentinel */ }, +}; +MODULE_DEVICE_TABLE(of, mxc_rnga_of_match); +#endif + static struct platform_driver mxc_rnga_driver = { .driver = { - .name = "mxc_rnga", - }, + .name = "mxc_rnga", + .of_match_table = of_match_ptr(mxc_rnga_of_match), + }, .remove = __exit_p(mxc_rnga_remove), };
The driver works well on i.MX31 powered boards with device description taken from board device tree, the only change to add to the driver is the missing OF device id, the affected list of included headers and platform driver struct are beautified a little. Signed-off-by: Vladimir Zapolskiy <vz@mleia.com> --- drivers/char/hw_random/mxc-rnga.c | 24 +++++++++++++++--------- 1 file changed, 15 insertions(+), 9 deletions(-)