linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] drivers: depend on HAS_IOMEM for devm_platform_ioremap_resource()
@ 2019-02-21 16:26 Bartosz Golaszewski
  2019-02-21 16:47 ` Greg Kroah-Hartman
                   ` (2 more replies)
  0 siblings, 3 replies; 20+ messages in thread
From: Bartosz Golaszewski @ 2019-02-21 16:26 UTC (permalink / raw)
  To: Greg Kroah-Hartman, Rafael J . Wysocki, Keerthy, Linus Walleij,
	Andy Shevchenko
  Cc: linux-kernel, linux-gpio, Bartosz Golaszewski

From: Bartosz Golaszewski <bgolaszewski@baylibre.com>

We only build devm_ioremap_resource() if HAS_IOMEM is selected, so this
dependency must cascade down to devm_platform_ioremap_resource().

Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
---
 drivers/base/platform.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/base/platform.c b/drivers/base/platform.c
index f82691e1c26c..5f837f2e4f41 100644
--- a/drivers/base/platform.c
+++ b/drivers/base/platform.c
@@ -87,6 +87,7 @@ EXPORT_SYMBOL_GPL(platform_get_resource);
  *        resource managemend
  * @index: resource index
  */
+#ifdef CONFIG_HAS_IOMEM
 void __iomem *devm_platform_ioremap_resource(struct platform_device *pdev,
 					     unsigned int index)
 {
@@ -96,6 +97,7 @@ void __iomem *devm_platform_ioremap_resource(struct platform_device *pdev,
 	return devm_ioremap_resource(&pdev->dev, res);
 }
 EXPORT_SYMBOL_GPL(devm_platform_ioremap_resource);
+#endif /* CONFIG_HAS_IOMEM */
 
 /**
  * platform_get_irq - get an IRQ for a device
-- 
2.20.1


^ permalink raw reply related	[flat|nested] 20+ messages in thread

* Re: [PATCH] drivers: depend on HAS_IOMEM for devm_platform_ioremap_resource()
  2019-02-21 16:26 [PATCH] drivers: depend on HAS_IOMEM for devm_platform_ioremap_resource() Bartosz Golaszewski
@ 2019-02-21 16:47 ` Greg Kroah-Hartman
  2019-02-21 19:55 ` Andy Shevchenko
  2019-06-14 16:50 ` [PATCH] drivers: Adjust scope for CONFIG_HAS_IOMEM before devm_platform_ioremap_resource() Markus Elfring
  2 siblings, 0 replies; 20+ messages in thread
From: Greg Kroah-Hartman @ 2019-02-21 16:47 UTC (permalink / raw)
  To: Bartosz Golaszewski
  Cc: Rafael J . Wysocki, Keerthy, Linus Walleij, Andy Shevchenko,
	linux-kernel, linux-gpio, Bartosz Golaszewski

On Thu, Feb 21, 2019 at 05:26:27PM +0100, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> 
> We only build devm_ioremap_resource() if HAS_IOMEM is selected, so this
> dependency must cascade down to devm_platform_ioremap_resource().
> 
> Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> ---
>  drivers/base/platform.c | 2 ++
>  1 file changed, 2 insertions(+)

Acked-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH] drivers: depend on HAS_IOMEM for devm_platform_ioremap_resource()
  2019-02-21 16:26 [PATCH] drivers: depend on HAS_IOMEM for devm_platform_ioremap_resource() Bartosz Golaszewski
  2019-02-21 16:47 ` Greg Kroah-Hartman
@ 2019-02-21 19:55 ` Andy Shevchenko
  2019-02-21 19:56   ` Andy Shevchenko
  2019-02-22 14:22   ` Rob Herring
  2019-06-14 16:50 ` [PATCH] drivers: Adjust scope for CONFIG_HAS_IOMEM before devm_platform_ioremap_resource() Markus Elfring
  2 siblings, 2 replies; 20+ messages in thread
From: Andy Shevchenko @ 2019-02-21 19:55 UTC (permalink / raw)
  To: Bartosz Golaszewski
  Cc: Greg Kroah-Hartman, Rafael J . Wysocki, Keerthy, Linus Walleij,
	Andy Shevchenko, Linux Kernel Mailing List,
	open list:GPIO SUBSYSTEM, Bartosz Golaszewski

On Thu, Feb 21, 2019 at 6:27 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote:
>
> From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
>
> We only build devm_ioremap_resource() if HAS_IOMEM is selected, so this
> dependency must cascade down to devm_platform_ioremap_resource().

> +#ifdef CONFIG_HAS_IOMEM
>  void __iomem *devm_platform_ioremap_resource(struct platform_device *pdev,
>                                              unsigned int index)
>  {
> @@ -96,6 +97,7 @@ void __iomem *devm_platform_ioremap_resource(struct platform_device *pdev,
>         return devm_ioremap_resource(&pdev->dev, res);
>  }
>  EXPORT_SYMBOL_GPL(devm_platform_ioremap_resource);
> +#endif /* CONFIG_HAS_IOMEM */

What about declaration in *.h?

Perhaps you may just do this inside the function?

-- 
With Best Regards,
Andy Shevchenko

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH] drivers: depend on HAS_IOMEM for devm_platform_ioremap_resource()
  2019-02-21 19:55 ` Andy Shevchenko
@ 2019-02-21 19:56   ` Andy Shevchenko
  2019-02-22  9:04     ` Bartosz Golaszewski
  2019-02-22 14:22   ` Rob Herring
  1 sibling, 1 reply; 20+ messages in thread
From: Andy Shevchenko @ 2019-02-21 19:56 UTC (permalink / raw)
  To: Bartosz Golaszewski
  Cc: Greg Kroah-Hartman, Rafael J . Wysocki, Keerthy, Linus Walleij,
	Andy Shevchenko, Linux Kernel Mailing List,
	open list:GPIO SUBSYSTEM, Bartosz Golaszewski

On Thu, Feb 21, 2019 at 9:55 PM Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
>
> On Thu, Feb 21, 2019 at 6:27 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote:
> >
> > From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> >
> > We only build devm_ioremap_resource() if HAS_IOMEM is selected, so this
> > dependency must cascade down to devm_platform_ioremap_resource().
>
> > +#ifdef CONFIG_HAS_IOMEM
> >  void __iomem *devm_platform_ioremap_resource(struct platform_device *pdev,
> >                                              unsigned int index)
> >  {
> > @@ -96,6 +97,7 @@ void __iomem *devm_platform_ioremap_resource(struct platform_device *pdev,
> >         return devm_ioremap_resource(&pdev->dev, res);
> >  }
> >  EXPORT_SYMBOL_GPL(devm_platform_ioremap_resource);
> > +#endif /* CONFIG_HAS_IOMEM */
>
> What about declaration in *.h?
>
> Perhaps you may just do this inside the function?

#ifdef ...
#else
return ERR_PTR(-ENOTSUPP);
#endif


-- 
With Best Regards,
Andy Shevchenko

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH] drivers: depend on HAS_IOMEM for devm_platform_ioremap_resource()
  2019-02-21 19:56   ` Andy Shevchenko
@ 2019-02-22  9:04     ` Bartosz Golaszewski
  2019-02-22 10:24       ` Andy Shevchenko
  0 siblings, 1 reply; 20+ messages in thread
From: Bartosz Golaszewski @ 2019-02-22  9:04 UTC (permalink / raw)
  To: Andy Shevchenko
  Cc: Greg Kroah-Hartman, Rafael J . Wysocki, Keerthy, Linus Walleij,
	Andy Shevchenko, Linux Kernel Mailing List,
	open list:GPIO SUBSYSTEM, Bartosz Golaszewski

czw., 21 lut 2019 o 20:56 Andy Shevchenko <andy.shevchenko@gmail.com>
napisał(a):
>
> On Thu, Feb 21, 2019 at 9:55 PM Andy Shevchenko
> <andy.shevchenko@gmail.com> wrote:
> >
> > On Thu, Feb 21, 2019 at 6:27 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote:
> > >
> > > From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> > >
> > > We only build devm_ioremap_resource() if HAS_IOMEM is selected, so this
> > > dependency must cascade down to devm_platform_ioremap_resource().
> >
> > > +#ifdef CONFIG_HAS_IOMEM
> > >  void __iomem *devm_platform_ioremap_resource(struct platform_device *pdev,
> > >                                              unsigned int index)
> > >  {
> > > @@ -96,6 +97,7 @@ void __iomem *devm_platform_ioremap_resource(struct platform_device *pdev,
> > >         return devm_ioremap_resource(&pdev->dev, res);
> > >  }
> > >  EXPORT_SYMBOL_GPL(devm_platform_ioremap_resource);
> > > +#endif /* CONFIG_HAS_IOMEM */
> >
> > What about declaration in *.h?
> >
> > Perhaps you may just do this inside the function?
>
> #ifdef ...
> #else
> return ERR_PTR(-ENOTSUPP);
> #endif
>

I followed the example of devm_ioremap_resource() which doesn't do
this - it just expects never to be used by systems without IOMEM.

Bart

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH] drivers: depend on HAS_IOMEM for devm_platform_ioremap_resource()
  2019-02-22  9:04     ` Bartosz Golaszewski
@ 2019-02-22 10:24       ` Andy Shevchenko
  0 siblings, 0 replies; 20+ messages in thread
From: Andy Shevchenko @ 2019-02-22 10:24 UTC (permalink / raw)
  To: Bartosz Golaszewski
  Cc: Greg Kroah-Hartman, Rafael J . Wysocki, Keerthy, Linus Walleij,
	Andy Shevchenko, Linux Kernel Mailing List,
	open list:GPIO SUBSYSTEM, Bartosz Golaszewski

On Fri, Feb 22, 2019 at 11:04 AM Bartosz Golaszewski <brgl@bgdev.pl> wrote:
>
> czw., 21 lut 2019 o 20:56 Andy Shevchenko <andy.shevchenko@gmail.com>
> napisał(a):
> >
> > On Thu, Feb 21, 2019 at 9:55 PM Andy Shevchenko
> > <andy.shevchenko@gmail.com> wrote:
> > >
> > > On Thu, Feb 21, 2019 at 6:27 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote:
> > > >
> > > > From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> > > >
> > > > We only build devm_ioremap_resource() if HAS_IOMEM is selected, so this
> > > > dependency must cascade down to devm_platform_ioremap_resource().
> > >
> > > > +#ifdef CONFIG_HAS_IOMEM
> > > >  void __iomem *devm_platform_ioremap_resource(struct platform_device *pdev,
> > > >                                              unsigned int index)
> > > >  {
> > > > @@ -96,6 +97,7 @@ void __iomem *devm_platform_ioremap_resource(struct platform_device *pdev,
> > > >         return devm_ioremap_resource(&pdev->dev, res);
> > > >  }
> > > >  EXPORT_SYMBOL_GPL(devm_platform_ioremap_resource);
> > > > +#endif /* CONFIG_HAS_IOMEM */
> > >
> > > What about declaration in *.h?
> > >
> > > Perhaps you may just do this inside the function?
> >
> > #ifdef ...
> > #else
> > return ERR_PTR(-ENOTSUPP);
> > #endif
> >
>
> I followed the example of devm_ioremap_resource() which doesn't do
> this - it just expects never to be used by systems without IOMEM.

Okay, that's fine!


-- 
With Best Regards,
Andy Shevchenko

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH] drivers: depend on HAS_IOMEM for devm_platform_ioremap_resource()
  2019-02-21 19:55 ` Andy Shevchenko
  2019-02-21 19:56   ` Andy Shevchenko
@ 2019-02-22 14:22   ` Rob Herring
  2019-02-22 16:22     ` Linus Walleij
  1 sibling, 1 reply; 20+ messages in thread
From: Rob Herring @ 2019-02-22 14:22 UTC (permalink / raw)
  To: Andy Shevchenko
  Cc: Bartosz Golaszewski, Greg Kroah-Hartman, Rafael J . Wysocki,
	Keerthy, Linus Walleij, Andy Shevchenko,
	Linux Kernel Mailing List, open list:GPIO SUBSYSTEM,
	Bartosz Golaszewski

On Thu, Feb 21, 2019 at 1:56 PM Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
>
> On Thu, Feb 21, 2019 at 6:27 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote:
> >
> > From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> >
> > We only build devm_ioremap_resource() if HAS_IOMEM is selected, so this
> > dependency must cascade down to devm_platform_ioremap_resource().
>
> > +#ifdef CONFIG_HAS_IOMEM
> >  void __iomem *devm_platform_ioremap_resource(struct platform_device *pdev,
> >                                              unsigned int index)
> >  {
> > @@ -96,6 +97,7 @@ void __iomem *devm_platform_ioremap_resource(struct platform_device *pdev,
> >         return devm_ioremap_resource(&pdev->dev, res);
> >  }
> >  EXPORT_SYMBOL_GPL(devm_platform_ioremap_resource);
> > +#endif /* CONFIG_HAS_IOMEM */
>
> What about declaration in *.h?
>
> Perhaps you may just do this inside the function?

Just a drive by comment, but IMO we should just get rid of HAS_IOMEM
altogether. It is really just a !UML option as I think every other
arch enables it. If folks really want to disable drivers on UML, just
disable the subsystems.

Though now with kunit mocking, there's a desire to enable HAS_IOMEM on UML, too.

Rob

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH] drivers: depend on HAS_IOMEM for devm_platform_ioremap_resource()
  2019-02-22 14:22   ` Rob Herring
@ 2019-02-22 16:22     ` Linus Walleij
  0 siblings, 0 replies; 20+ messages in thread
From: Linus Walleij @ 2019-02-22 16:22 UTC (permalink / raw)
  To: Rob Herring
  Cc: Andy Shevchenko, Bartosz Golaszewski, Greg Kroah-Hartman,
	Rafael J . Wysocki, Keerthy, Andy Shevchenko,
	Linux Kernel Mailing List, open list:GPIO SUBSYSTEM,
	Bartosz Golaszewski, Arnd Bergmann

On Fri, Feb 22, 2019 at 3:22 PM Rob Herring <rob.herring@linaro.org> wrote:

> Just a drive by comment, but IMO we should just get rid of HAS_IOMEM
> altogether. It is really just a !UML option as I think every other
> arch enables it. If folks really want to disable drivers on UML, just
> disable the subsystems.

I just got a build failure from S390 for the same thing so apparently
there is S390 Linux without IOMEM. I have no idea how that works
but whenever Arnd start talking to me about how S390 works
my head start spinning.

Yours,
Linus Walleij

^ permalink raw reply	[flat|nested] 20+ messages in thread

* [PATCH] drivers: Adjust scope for CONFIG_HAS_IOMEM before devm_platform_ioremap_resource()
  2019-02-21 16:26 [PATCH] drivers: depend on HAS_IOMEM for devm_platform_ioremap_resource() Bartosz Golaszewski
  2019-02-21 16:47 ` Greg Kroah-Hartman
  2019-02-21 19:55 ` Andy Shevchenko
@ 2019-06-14 16:50 ` Markus Elfring
  2019-06-24  8:29   ` Bartosz Golaszewski
  2 siblings, 1 reply; 20+ messages in thread
From: Markus Elfring @ 2019-06-14 16:50 UTC (permalink / raw)
  To: Bartosz Golaszewski, Andy Shevchenko, Greg Kroah-Hartman,
	Keerthy, Linus Walleij, Rafael J. Wysocki
  Cc: Bartosz Golaszewski, linux-gpio, kernel-janitors, linux-kernel

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Fri, 14 Jun 2019 17:45:13 +0200

Move the preprocessor statement “#ifdef CONFIG_HAS_IOMEM” so that
the corresponding scope for conditional compilation includes also comments
for this function implementation.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/base/platform.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/base/platform.c b/drivers/base/platform.c
index 4d1729853d1a..a5f40974a6ef 100644
--- a/drivers/base/platform.c
+++ b/drivers/base/platform.c
@@ -78,6 +78,7 @@ struct resource *platform_get_resource(struct platform_device *dev,
 	return NULL;
 }
 EXPORT_SYMBOL_GPL(platform_get_resource);
+#ifdef CONFIG_HAS_IOMEM

 /**
  * devm_platform_ioremap_resource - call devm_ioremap_resource() for a platform
@@ -87,7 +88,6 @@ EXPORT_SYMBOL_GPL(platform_get_resource);
  *        resource management
  * @index: resource index
  */
-#ifdef CONFIG_HAS_IOMEM
 void __iomem *devm_platform_ioremap_resource(struct platform_device *pdev,
 					     unsigned int index)
 {
--
2.22.0


^ permalink raw reply related	[flat|nested] 20+ messages in thread

* Re: [PATCH] drivers: Adjust scope for CONFIG_HAS_IOMEM before devm_platform_ioremap_resource()
  2019-06-14 16:50 ` [PATCH] drivers: Adjust scope for CONFIG_HAS_IOMEM before devm_platform_ioremap_resource() Markus Elfring
@ 2019-06-24  8:29   ` Bartosz Golaszewski
  2019-06-24 10:05     ` Enrico Weigelt, metux IT consult
  2019-06-24 10:51     ` [PATCH] " Markus Elfring
  0 siblings, 2 replies; 20+ messages in thread
From: Bartosz Golaszewski @ 2019-06-24  8:29 UTC (permalink / raw)
  To: Markus Elfring
  Cc: Bartosz Golaszewski, Andy Shevchenko, Greg Kroah-Hartman,
	Keerthy, Linus Walleij, Rafael J. Wysocki, linux-gpio,
	kernel-janitors, LKML

pt., 14 cze 2019 o 18:50 Markus Elfring <Markus.Elfring@web.de> napisał(a):
>
> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Fri, 14 Jun 2019 17:45:13 +0200
>
> Move the preprocessor statement “#ifdef CONFIG_HAS_IOMEM” so that
> the corresponding scope for conditional compilation includes also comments
> for this function implementation.
>
> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
> ---
>  drivers/base/platform.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/base/platform.c b/drivers/base/platform.c
> index 4d1729853d1a..a5f40974a6ef 100644
> --- a/drivers/base/platform.c
> +++ b/drivers/base/platform.c
> @@ -78,6 +78,7 @@ struct resource *platform_get_resource(struct platform_device *dev,
>         return NULL;
>  }
>  EXPORT_SYMBOL_GPL(platform_get_resource);
> +#ifdef CONFIG_HAS_IOMEM
>
>  /**
>   * devm_platform_ioremap_resource - call devm_ioremap_resource() for a platform
> @@ -87,7 +88,6 @@ EXPORT_SYMBOL_GPL(platform_get_resource);
>   *        resource management
>   * @index: resource index
>   */
> -#ifdef CONFIG_HAS_IOMEM
>  void __iomem *devm_platform_ioremap_resource(struct platform_device *pdev,
>                                              unsigned int index)
>  {
> --
> 2.22.0
>

And what is the purpose of that?

Bart

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH] drivers: Adjust scope for CONFIG_HAS_IOMEM before devm_platform_ioremap_resource()
  2019-06-24  8:29   ` Bartosz Golaszewski
@ 2019-06-24 10:05     ` Enrico Weigelt, metux IT consult
  2019-06-24 10:46       ` Bartosz Golaszewski
  2019-06-24 10:51     ` [PATCH] " Markus Elfring
  1 sibling, 1 reply; 20+ messages in thread
From: Enrico Weigelt, metux IT consult @ 2019-06-24 10:05 UTC (permalink / raw)
  To: Bartosz Golaszewski, Markus Elfring
  Cc: Bartosz Golaszewski, Andy Shevchenko, Greg Kroah-Hartman,
	Keerthy, Linus Walleij, Rafael J. Wysocki, linux-gpio,
	kernel-janitors, LKML

On 24.06.19 10:29, Bartosz Golaszewski wrote:
> pt., 14 cze 2019 o 18:50 Markus Elfring <Markus.Elfring@web.de> napisał(a):
>>
>> From: Markus Elfring <elfring@users.sourceforge.net>
>> Date: Fri, 14 Jun 2019 17:45:13 +0200
>>
>> Move the preprocessor statement “#ifdef CONFIG_HAS_IOMEM” so that
>> the corresponding scope for conditional compilation includes also comments
>> for this function implementation.
>>
>> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
>> ---
>>  drivers/base/platform.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/base/platform.c b/drivers/base/platform.c
>> index 4d1729853d1a..a5f40974a6ef 100644
>> --- a/drivers/base/platform.c
>> +++ b/drivers/base/platform.c
>> @@ -78,6 +78,7 @@ struct resource *platform_get_resource(struct platform_device *dev,
>>         return NULL;
>>  }
>>  EXPORT_SYMBOL_GPL(platform_get_resource);
>> +#ifdef CONFIG_HAS_IOMEM
>>
>>  /**
>>   * devm_platform_ioremap_resource - call devm_ioremap_resource() for a platform
>> @@ -87,7 +88,6 @@ EXPORT_SYMBOL_GPL(platform_get_resource);
>>   *        resource management
>>   * @index: resource index
>>   */
>> -#ifdef CONFIG_HAS_IOMEM
>>  void __iomem *devm_platform_ioremap_resource(struct platform_device *pdev,
>>                                              unsigned int index)
>>  {
>> --
>> 2.22.0
>>
> 
> And what is the purpose of that?

I can imagine that this could improve readability a little bit. Maybe if
one uses same fancy ide/editor that can fold code blocks like functions
and conditionals, this patch could make the code prettier.

The patch seems pretty trivial and doesn't change any actual code, so
I don't see hard resons for rejecting it.


--mtx


-- 
Enrico Weigelt, metux IT consult
Free software and Linux embedded engineering
info@metux.net -- +49-151-27565287

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH] drivers: Adjust scope for CONFIG_HAS_IOMEM before devm_platform_ioremap_resource()
  2019-06-24 10:05     ` Enrico Weigelt, metux IT consult
@ 2019-06-24 10:46       ` Bartosz Golaszewski
  2019-06-24 11:00         ` Markus Elfring
  2019-06-24 18:22         ` [PATCH] " Enrico Weigelt, metux IT consult
  0 siblings, 2 replies; 20+ messages in thread
From: Bartosz Golaszewski @ 2019-06-24 10:46 UTC (permalink / raw)
  To: Enrico Weigelt, metux IT consult
  Cc: Bartosz Golaszewski, Markus Elfring, Andy Shevchenko,
	Greg Kroah-Hartman, Keerthy, Linus Walleij, Rafael J. Wysocki,
	linux-gpio, kernel-janitors, LKML

pon., 24 cze 2019 o 12:05 Enrico Weigelt, metux IT consult
<lkml@metux.net> napisał(a):
>
> On 24.06.19 10:29, Bartosz Golaszewski wrote:
> > pt., 14 cze 2019 o 18:50 Markus Elfring <Markus.Elfring@web.de> napisał(a):
> >>
> >> From: Markus Elfring <elfring@users.sourceforge.net>
> >> Date: Fri, 14 Jun 2019 17:45:13 +0200
> >>
> >> Move the preprocessor statement “#ifdef CONFIG_HAS_IOMEM” so that
> >> the corresponding scope for conditional compilation includes also comments
> >> for this function implementation.
> >>
> >> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
> >> ---
> >>  drivers/base/platform.c | 2 +-
> >>  1 file changed, 1 insertion(+), 1 deletion(-)
> >>
> >> diff --git a/drivers/base/platform.c b/drivers/base/platform.c
> >> index 4d1729853d1a..a5f40974a6ef 100644
> >> --- a/drivers/base/platform.c
> >> +++ b/drivers/base/platform.c
> >> @@ -78,6 +78,7 @@ struct resource *platform_get_resource(struct platform_device *dev,
> >>         return NULL;
> >>  }
> >>  EXPORT_SYMBOL_GPL(platform_get_resource);
> >> +#ifdef CONFIG_HAS_IOMEM
> >>
> >>  /**
> >>   * devm_platform_ioremap_resource - call devm_ioremap_resource() for a platform
> >> @@ -87,7 +88,6 @@ EXPORT_SYMBOL_GPL(platform_get_resource);
> >>   *        resource management
> >>   * @index: resource index
> >>   */
> >> -#ifdef CONFIG_HAS_IOMEM
> >>  void __iomem *devm_platform_ioremap_resource(struct platform_device *pdev,
> >>                                              unsigned int index)
> >>  {
> >> --
> >> 2.22.0
> >>
> >
> > And what is the purpose of that?
>
> I can imagine that this could improve readability a little bit. Maybe if
> one uses same fancy ide/editor that can fold code blocks like functions
> and conditionals, this patch could make the code prettier.
>
> The patch seems pretty trivial and doesn't change any actual code, so
> I don't see hard resons for rejecting it.
>

In its current form it makes the code even less readable. The #ifdef
should actually be one line lower and touch the comment instead of the
EXPORT_SYMBOL() related to a different function.

Bart

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH] drivers: Adjust scope for CONFIG_HAS_IOMEM before devm_platform_ioremap_resource()
  2019-06-24  8:29   ` Bartosz Golaszewski
  2019-06-24 10:05     ` Enrico Weigelt, metux IT consult
@ 2019-06-24 10:51     ` Markus Elfring
  1 sibling, 0 replies; 20+ messages in thread
From: Markus Elfring @ 2019-06-24 10:51 UTC (permalink / raw)
  To: Bartosz Golaszewski, kernel-janitors
  Cc: Bartosz Golaszewski, Andy Shevchenko, Greg Kroah-Hartman,
	Keerthy, Linus Walleij, Rafael J. Wysocki, linux-gpio, LKML,
	Enrico Weigelt

>> +++ b/drivers/base/platform.c
>> @@ -78,6 +78,7 @@ struct resource *platform_get_resource(struct platform_device *dev,
>>         return NULL;
>>  }
>>  EXPORT_SYMBOL_GPL(platform_get_resource);
>> +#ifdef CONFIG_HAS_IOMEM
>>
>>  /**
>>   * devm_platform_ioremap_resource - call devm_ioremap_resource() for a platform
>> @@ -87,7 +88,6 @@ EXPORT_SYMBOL_GPL(platform_get_resource);
>>   *        resource management
>>   * @index: resource index
>>   */
>> -#ifdef CONFIG_HAS_IOMEM
>>  void __iomem *devm_platform_ioremap_resource(struct platform_device *pdev,
>>                                              unsigned int index)
>>  {
> And what is the purpose of that?

I recommend to let the availability of additional documentation for this function
depend also on the mentioned preprocessor symbol

Regards,
Markus

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: drivers: Adjust scope for CONFIG_HAS_IOMEM before devm_platform_ioremap_resource()
  2019-06-24 10:46       ` Bartosz Golaszewski
@ 2019-06-24 11:00         ` Markus Elfring
  2019-06-24 18:22         ` [PATCH] " Enrico Weigelt, metux IT consult
  1 sibling, 0 replies; 20+ messages in thread
From: Markus Elfring @ 2019-06-24 11:00 UTC (permalink / raw)
  To: Bartosz Golaszewski, kernel-janitors
  Cc: Enrico Weigelt, Bartosz Golaszewski, Andy Shevchenko,
	Greg Kroah-Hartman, Keerthy, Linus Walleij, Rafael J. Wysocki,
	linux-gpio, LKML

> In its current form it makes the code even less readable.

This can be your development opinion.


> The #ifdef should actually be one line lower

I suggested that the conditional compilation can contain also a blank line
together with a comment block.


> and touch the comment instead of the EXPORT_SYMBOL() related to a different function.

I find that this macro call was kept unchanged.

Regards,
Markus

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH] drivers: Adjust scope for CONFIG_HAS_IOMEM before devm_platform_ioremap_resource()
  2019-06-24 10:46       ` Bartosz Golaszewski
  2019-06-24 11:00         ` Markus Elfring
@ 2019-06-24 18:22         ` Enrico Weigelt, metux IT consult
  2019-06-25  7:10           ` Bartosz Golaszewski
  1 sibling, 1 reply; 20+ messages in thread
From: Enrico Weigelt, metux IT consult @ 2019-06-24 18:22 UTC (permalink / raw)
  To: Bartosz Golaszewski
  Cc: Bartosz Golaszewski, Markus Elfring, Andy Shevchenko,
	Greg Kroah-Hartman, Keerthy, Linus Walleij, Rafael J. Wysocki,
	linux-gpio, kernel-janitors, LKML

On 24.06.19 12:46, Bartosz Golaszewski wrote:

>> The patch seems pretty trivial and doesn't change any actual code, so
>> I don't see hard resons for rejecting it.
>>
> 
> In its current form it makes the code even less readable. The #ifdef
> should actually be one line lower and touch the comment instead of the
> EXPORT_SYMBOL() related to a different function.

Okay, that missing newline should be fixed (as well as the extra one
after the #ifdef). Besides that, I don't see any further problems.

--mtx

-- 
Enrico Weigelt, metux IT consult
Free software and Linux embedded engineering
info@metux.net -- +49-151-27565287

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH] drivers: Adjust scope for CONFIG_HAS_IOMEM before devm_platform_ioremap_resource()
  2019-06-24 18:22         ` [PATCH] " Enrico Weigelt, metux IT consult
@ 2019-06-25  7:10           ` Bartosz Golaszewski
  2019-06-25  7:30             ` Greg Kroah-Hartman
  0 siblings, 1 reply; 20+ messages in thread
From: Bartosz Golaszewski @ 2019-06-25  7:10 UTC (permalink / raw)
  To: Enrico Weigelt, metux IT consult, Greg Kroah-Hartman
  Cc: Bartosz Golaszewski, Markus Elfring, Andy Shevchenko, Keerthy,
	Linus Walleij, Rafael J. Wysocki, linux-gpio, kernel-janitors,
	LKML

pon., 24 cze 2019 o 20:22 Enrico Weigelt, metux IT consult
<lkml@metux.net> napisał(a):
>
> On 24.06.19 12:46, Bartosz Golaszewski wrote:
>
> >> The patch seems pretty trivial and doesn't change any actual code, so
> >> I don't see hard resons for rejecting it.
> >>
> >
> > In its current form it makes the code even less readable. The #ifdef
> > should actually be one line lower and touch the comment instead of the
> > EXPORT_SYMBOL() related to a different function.
>
> Okay, that missing newline should be fixed (as well as the extra one
> after the #ifdef). Besides that, I don't see any further problems.
>

Are we sure this even changes something? Does kernel documentation get
generated according to current config options? I really think this
patch just pollutes the history for now apparent reason.

Greg, could you give your opinion on this?

Bart

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH] drivers: Adjust scope for CONFIG_HAS_IOMEM before devm_platform_ioremap_resource()
  2019-06-25  7:10           ` Bartosz Golaszewski
@ 2019-06-25  7:30             ` Greg Kroah-Hartman
  2019-06-25  7:32               ` Bartosz Golaszewski
  2019-06-25  7:42               ` Markus Elfring
  0 siblings, 2 replies; 20+ messages in thread
From: Greg Kroah-Hartman @ 2019-06-25  7:30 UTC (permalink / raw)
  To: Bartosz Golaszewski
  Cc: Enrico Weigelt, metux IT consult, Bartosz Golaszewski,
	Markus Elfring, Andy Shevchenko, Keerthy, Linus Walleij,
	Rafael J. Wysocki, linux-gpio, kernel-janitors, LKML

On Tue, Jun 25, 2019 at 09:10:25AM +0200, Bartosz Golaszewski wrote:
> pon., 24 cze 2019 o 20:22 Enrico Weigelt, metux IT consult
> <lkml@metux.net> napisał(a):
> >
> > On 24.06.19 12:46, Bartosz Golaszewski wrote:
> >
> > >> The patch seems pretty trivial and doesn't change any actual code, so
> > >> I don't see hard resons for rejecting it.
> > >>
> > >
> > > In its current form it makes the code even less readable. The #ifdef
> > > should actually be one line lower and touch the comment instead of the
> > > EXPORT_SYMBOL() related to a different function.
> >
> > Okay, that missing newline should be fixed (as well as the extra one
> > after the #ifdef). Besides that, I don't see any further problems.
> >
> 
> Are we sure this even changes something? Does kernel documentation get
> generated according to current config options? I really think this
> patch just pollutes the history for now apparent reason.
> 
> Greg, could you give your opinion on this?

Why are you all arguing with a all-but-instinguishable-from-a-bot
persona about a patch that I will never apply?

greg k-h

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH] drivers: Adjust scope for CONFIG_HAS_IOMEM before devm_platform_ioremap_resource()
  2019-06-25  7:30             ` Greg Kroah-Hartman
@ 2019-06-25  7:32               ` Bartosz Golaszewski
  2019-06-25  7:51                 ` Markus Elfring
  2019-06-25  7:42               ` Markus Elfring
  1 sibling, 1 reply; 20+ messages in thread
From: Bartosz Golaszewski @ 2019-06-25  7:32 UTC (permalink / raw)
  To: Greg Kroah-Hartman
  Cc: Enrico Weigelt, metux IT consult, Bartosz Golaszewski,
	Markus Elfring, Andy Shevchenko, Keerthy, Linus Walleij,
	Rafael J. Wysocki, linux-gpio, kernel-janitors, LKML

wt., 25 cze 2019 o 09:30 Greg Kroah-Hartman
<gregkh@linuxfoundation.org> napisał(a):
>
> On Tue, Jun 25, 2019 at 09:10:25AM +0200, Bartosz Golaszewski wrote:
> > pon., 24 cze 2019 o 20:22 Enrico Weigelt, metux IT consult
> > <lkml@metux.net> napisał(a):
> > >
> > > On 24.06.19 12:46, Bartosz Golaszewski wrote:
> > >
> > > >> The patch seems pretty trivial and doesn't change any actual code, so
> > > >> I don't see hard resons for rejecting it.
> > > >>
> > > >
> > > > In its current form it makes the code even less readable. The #ifdef
> > > > should actually be one line lower and touch the comment instead of the
> > > > EXPORT_SYMBOL() related to a different function.
> > >
> > > Okay, that missing newline should be fixed (as well as the extra one
> > > after the #ifdef). Besides that, I don't see any further problems.
> > >
> >
> > Are we sure this even changes something? Does kernel documentation get
> > generated according to current config options? I really think this
> > patch just pollutes the history for now apparent reason.
> >
> > Greg, could you give your opinion on this?
>
> Why are you all arguing with a all-but-instinguishable-from-a-bot
> persona about a patch that I will never apply?
>
> greg k-h

Oh so it's another troll then? Good to know, ignoring from now on.

Thanks,
Bart

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: drivers: Adjust scope for CONFIG_HAS_IOMEM before devm_platform_ioremap_resource()
  2019-06-25  7:30             ` Greg Kroah-Hartman
  2019-06-25  7:32               ` Bartosz Golaszewski
@ 2019-06-25  7:42               ` Markus Elfring
  1 sibling, 0 replies; 20+ messages in thread
From: Markus Elfring @ 2019-06-25  7:42 UTC (permalink / raw)
  To: Greg Kroah-Hartman
  Cc: Bartosz Golaszewski, Enrico Weigelt, Bartosz Golaszewski,
	Andy Shevchenko, Keerthy, Linus Walleij, Rafael J. Wysocki,
	linux-gpio, kernel-janitors, LKML

> Why are you all arguing with a all-but-instinguishable-from-a-bot persona

I am curious if another meeting at a Linux conference
can adjust this view.


> about a patch that I will never apply?

I hope that you can get into a more constructive mood a bit later
for the reconsideration of the position for the preprocessor
statement “#ifdef CONFIG_HAS_IOMEM” before a function implementation.

Regards,
Markus

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: drivers: Adjust scope for CONFIG_HAS_IOMEM before devm_platform_ioremap_resource()
  2019-06-25  7:32               ` Bartosz Golaszewski
@ 2019-06-25  7:51                 ` Markus Elfring
  0 siblings, 0 replies; 20+ messages in thread
From: Markus Elfring @ 2019-06-25  7:51 UTC (permalink / raw)
  To: Bartosz Golaszewski, kernel-janitors
  Cc: Greg Kroah-Hartman, Enrico Weigelt, Bartosz Golaszewski,
	Andy Shevchenko, Keerthy, Linus Walleij, Rafael J. Wysocki,
	linux-gpio, LKML

> Oh so it's another troll then?

I am just a contributor.


> Good to know, ignoring from now on.

The opinions can vary for my contributions as usual.

I hope that the software development attention can evolve in more
positive ways again.

Regards,
Markus

^ permalink raw reply	[flat|nested] 20+ messages in thread

end of thread, other threads:[~2019-06-25  7:51 UTC | newest]

Thread overview: 20+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-02-21 16:26 [PATCH] drivers: depend on HAS_IOMEM for devm_platform_ioremap_resource() Bartosz Golaszewski
2019-02-21 16:47 ` Greg Kroah-Hartman
2019-02-21 19:55 ` Andy Shevchenko
2019-02-21 19:56   ` Andy Shevchenko
2019-02-22  9:04     ` Bartosz Golaszewski
2019-02-22 10:24       ` Andy Shevchenko
2019-02-22 14:22   ` Rob Herring
2019-02-22 16:22     ` Linus Walleij
2019-06-14 16:50 ` [PATCH] drivers: Adjust scope for CONFIG_HAS_IOMEM before devm_platform_ioremap_resource() Markus Elfring
2019-06-24  8:29   ` Bartosz Golaszewski
2019-06-24 10:05     ` Enrico Weigelt, metux IT consult
2019-06-24 10:46       ` Bartosz Golaszewski
2019-06-24 11:00         ` Markus Elfring
2019-06-24 18:22         ` [PATCH] " Enrico Weigelt, metux IT consult
2019-06-25  7:10           ` Bartosz Golaszewski
2019-06-25  7:30             ` Greg Kroah-Hartman
2019-06-25  7:32               ` Bartosz Golaszewski
2019-06-25  7:51                 ` Markus Elfring
2019-06-25  7:42               ` Markus Elfring
2019-06-24 10:51     ` [PATCH] " Markus Elfring

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).