| Message ID | 20201027135325.22235-1-vincent.whitchurch@axis.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v2] gpio: mockup: Allow probing from device tree | expand |
On Tue, Oct 27, 2020 at 2:54 PM Vincent Whitchurch <vincent.whitchurch@axis.com> wrote: > > Allow the mockup driver to be probed via the device tree without any > module parameters, allowing it to be used to configure and test higher > level drivers like the leds-gpio driver and corresponding userspace > before actual hardware is available. > > Signed-off-by: Vincent Whitchurch <vincent.whitchurch@axis.com> > --- > v2: Remove most of the added code, since the latest driver doesn't need it. > Drop DT binding document, since Rob Herring was OK with not documenting > this: > https://lore.kernel.org/linux-devicetree/5baa1ae6.1c69fb81.847f2.3ab1@mx.google.com/ > > drivers/gpio/gpio-mockup.c | 13 +++++++++++-- > 1 file changed, 11 insertions(+), 2 deletions(-) > > diff --git a/drivers/gpio/gpio-mockup.c b/drivers/gpio/gpio-mockup.c > index 67ed4f238d43..c93892a6936a 100644 > --- a/drivers/gpio/gpio-mockup.c > +++ b/drivers/gpio/gpio-mockup.c > @@ -13,6 +13,7 @@ > #include <linux/gpio/driver.h> > #include <linux/interrupt.h> > #include <linux/irq.h> > +#include <linux/of.h> Please keep the includes ordered alphabetically. > #include <linux/irq_sim.h> > #include <linux/irqdomain.h> > #include <linux/module.h> > @@ -460,9 +461,18 @@ static int gpio_mockup_probe(struct platform_device *pdev) > return 0; > } > > +#ifdef CONFIG_OF > +static const struct of_device_id gpio_mockup_of_match[] = { > + { .compatible = "gpio-mockup", }, > + {}, > +}; > +MODULE_DEVICE_TABLE(of, gpio_mockup_of_match); > +#endif You don't need this ifdef - of_match_ptr() will evaluate to NULL if CONFIG_OF is disabled and the compiler will optimize this struct out. Bartosz > + > static struct platform_driver gpio_mockup_driver = { > .driver = { > .name = "gpio-mockup", > + .of_match_table = of_match_ptr(gpio_mockup_of_match), > }, > .probe = gpio_mockup_probe, > }; > @@ -556,8 +566,7 @@ static int __init gpio_mockup_init(void) > { > int i, num_chips, err; > > - if ((gpio_mockup_num_ranges < 2) || > - (gpio_mockup_num_ranges % 2) || > + if ((gpio_mockup_num_ranges % 2) || > (gpio_mockup_num_ranges > GPIO_MOCKUP_MAX_RANGES)) > return -EINVAL; > > -- > 2.28.0 >
On Tue, Oct 27, 2020 at 07:12:13PM +0100, Bartosz Golaszewski wrote: > On Tue, Oct 27, 2020 at 2:54 PM Vincent Whitchurch > <vincent.whitchurch@axis.com> wrote: > > diff --git a/drivers/gpio/gpio-mockup.c b/drivers/gpio/gpio-mockup.c > > index 67ed4f238d43..c93892a6936a 100644 > > --- a/drivers/gpio/gpio-mockup.c > > +++ b/drivers/gpio/gpio-mockup.c > > @@ -13,6 +13,7 @@ > > #include <linux/gpio/driver.h> > > #include <linux/interrupt.h> > > #include <linux/irq.h> > > +#include <linux/of.h> > > Please keep the includes ordered alphabetically. Thanks, fixed in v3. > > #include <linux/irq_sim.h> > > #include <linux/irqdomain.h> > > #include <linux/module.h> > > @@ -460,9 +461,18 @@ static int gpio_mockup_probe(struct platform_device *pdev) > > return 0; > > } > > > > +#ifdef CONFIG_OF > > +static const struct of_device_id gpio_mockup_of_match[] = { > > + { .compatible = "gpio-mockup", }, > > + {}, > > +}; > > +MODULE_DEVICE_TABLE(of, gpio_mockup_of_match); > > +#endif > > You don't need this ifdef - of_match_ptr() will evaluate to NULL if > CONFIG_OF is disabled and the compiler will optimize this struct out. The compiler can't optimise out the struct in the case of a module build since there is a reference from the MODULE_DEVICE_TABLE: $ grep CONFIG_OF .config # CONFIG_OF is not set $ nm drivers/gpio/gpio-mockup.ko | grep of_ 00000000 r gpio_mockup_of_match 00000000 R __mod_of__gpio_mockup_of_match_device_table But these few wasted bytes don't matter so I removed the CONFIG_OF anyway as you suggested.
On Wed, Oct 28, 2020 at 10:00 AM Vincent Whitchurch <vincent.whitchurch@axis.com> wrote: > > Allow the mockup driver to be probed via the device tree without any > module parameters, allowing it to be used to configure and test higher > level drivers like the leds-gpio driver and corresponding userspace > before actual hardware is available. You have to officially announce a DT binding for that.
On Wed, Oct 28, 2020 at 12:43:22PM +0100, Andy Shevchenko wrote: > On Wed, Oct 28, 2020 at 10:00 AM Vincent Whitchurch > <vincent.whitchurch@axis.com> wrote: > > Allow the mockup driver to be probed via the device tree without any > > module parameters, allowing it to be used to configure and test higher > > level drivers like the leds-gpio driver and corresponding userspace > > before actual hardware is available. > > You have to officially announce a DT binding for that. Rob Herring has earlier said that was not required for this driver. As I mentioned a little further down in the email you are replying to: | Drop DT binding document, since Rob Herring was OK with not documenting | this: | https://lore.kernel.org/linux-devicetree/5baa1ae6.1c69fb81.847f2.3ab1@mx.google.com/
On Wed, Oct 28, 2020 at 8:41 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote: > On Tue, Oct 27, 2020 at 2:54 PM Vincent Whitchurch > <vincent.whitchurch@axis.com> wrote: ... > > +#include <linux/of.h> > > Please keep the includes ordered alphabetically. Besides the fact that that is a wrong header to be included. mod_devicetable.h is the correct one. (See also below) ... > > +#ifdef CONFIG_OF > > +static const struct of_device_id gpio_mockup_of_match[] = { > > + { .compatible = "gpio-mockup", }, > > + {}, > > +}; > > +MODULE_DEVICE_TABLE(of, gpio_mockup_of_match); > > +#endif > > You don't need this ifdef - of_match_ptr() will evaluate to NULL if > CONFIG_OF is disabled and the compiler will optimize this struct out. It's not so. If you drop ugly ifdeffery (and I vote for that, see also above) the of_match_ptr() must be dropped as well. Otherwise the compiler will issue the warning. So it is either all or none.
On Wed, Oct 28, 2020 at 10:25 PM Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > On Wed, Oct 28, 2020 at 8:41 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote: ... > It's not so. If you drop ugly ifdeffery (and I vote for that, see also It's not so -> It's not so simple. > above) the of_match_ptr() must be dropped as well. > Otherwise the compiler will issue the warning. So it is either all or none.
On Wed, Oct 28, 2020 at 09:25:32PM +0100, Andy Shevchenko wrote: > On Wed, Oct 28, 2020 at 8:41 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote: > > On Tue, Oct 27, 2020 at 2:54 PM Vincent Whitchurch > > > +#ifdef CONFIG_OF > > > +static const struct of_device_id gpio_mockup_of_match[] = { > > > + { .compatible = "gpio-mockup", }, > > > + {}, > > > +}; > > > +MODULE_DEVICE_TABLE(of, gpio_mockup_of_match); > > > +#endif > > > > You don't need this ifdef - of_match_ptr() will evaluate to NULL if > > CONFIG_OF is disabled and the compiler will optimize this struct out. > > It's not so. If you drop ugly ifdeffery (and I vote for that, see also > above) the of_match_ptr() must be dropped as well. Otherwise the > compiler will issue the warning. So it is either all or none. Yes, you're right. I actually tested a !OF build before but it turns out that the warning is disabled by default and only enabled with W=1. I'll fix this and change the header in the next version. Thank you.
On Wed, Oct 28, 2020 at 9:24 PM Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > > On Wed, Oct 28, 2020 at 8:41 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote: > > On Tue, Oct 27, 2020 at 2:54 PM Vincent Whitchurch > > <vincent.whitchurch@axis.com> wrote: > > ... > > > > +#include <linux/of.h> > > > > Please keep the includes ordered alphabetically. > > Besides the fact that that is a wrong header to be included. > mod_devicetable.h is the correct one. > (See also below) > > ... > > > > +#ifdef CONFIG_OF > > > +static const struct of_device_id gpio_mockup_of_match[] = { > > > + { .compatible = "gpio-mockup", }, > > > + {}, > > > +}; > > > +MODULE_DEVICE_TABLE(of, gpio_mockup_of_match); > > > +#endif > > > > You don't need this ifdef - of_match_ptr() will evaluate to NULL if > > CONFIG_OF is disabled and the compiler will optimize this struct out. > > It's not so. If you drop ugly ifdeffery (and I vote for that, see also > above) the of_match_ptr() must be dropped as well. > Otherwise the compiler will issue the warning. So it is either all or none. > > -- > With Best Regards, > Andy Shevchenko Makes sense, I'll remove this from my tree for now then. Vincent: please send another iteration. Bartosz
diff --git a/drivers/gpio/gpio-mockup.c b/drivers/gpio/gpio-mockup.c index 67ed4f238d43..c93892a6936a 100644 --- a/drivers/gpio/gpio-mockup.c +++ b/drivers/gpio/gpio-mockup.c @@ -13,6 +13,7 @@ #include <linux/gpio/driver.h> #include <linux/interrupt.h> #include <linux/irq.h> +#include <linux/of.h> #include <linux/irq_sim.h> #include <linux/irqdomain.h> #include <linux/module.h> @@ -460,9 +461,18 @@ static int gpio_mockup_probe(struct platform_device *pdev) return 0; } +#ifdef CONFIG_OF +static const struct of_device_id gpio_mockup_of_match[] = { + { .compatible = "gpio-mockup", }, + {}, +}; +MODULE_DEVICE_TABLE(of, gpio_mockup_of_match); +#endif + static struct platform_driver gpio_mockup_driver = { .driver = { .name = "gpio-mockup", + .of_match_table = of_match_ptr(gpio_mockup_of_match), }, .probe = gpio_mockup_probe, }; @@ -556,8 +566,7 @@ static int __init gpio_mockup_init(void) { int i, num_chips, err; - if ((gpio_mockup_num_ranges < 2) || - (gpio_mockup_num_ranges % 2) || + if ((gpio_mockup_num_ranges % 2) || (gpio_mockup_num_ranges > GPIO_MOCKUP_MAX_RANGES)) return -EINVAL;
Allow the mockup driver to be probed via the device tree without any module parameters, allowing it to be used to configure and test higher level drivers like the leds-gpio driver and corresponding userspace before actual hardware is available. Signed-off-by: Vincent Whitchurch <vincent.whitchurch@axis.com> --- v2: Remove most of the added code, since the latest driver doesn't need it. Drop DT binding document, since Rob Herring was OK with not documenting this: https://lore.kernel.org/linux-devicetree/5baa1ae6.1c69fb81.847f2.3ab1@mx.google.com/ drivers/gpio/gpio-mockup.c | 13 +++++++++++-- 1 file changed, 11 insertions(+), 2 deletions(-)