From: Bartosz Golaszewski <[email protected]>
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 <[email protected]>
---
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
On Thu, Feb 21, 2019 at 05:26:27PM +0100, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <[email protected]>
>
> 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 <[email protected]>
> ---
> drivers/base/platform.c | 2 ++
> 1 file changed, 2 insertions(+)
Acked-by: Greg Kroah-Hartman <[email protected]>
On Thu, Feb 21, 2019 at 6:27 PM Bartosz Golaszewski <[email protected]> wrote:
>
> From: Bartosz Golaszewski <[email protected]>
>
> 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
On Thu, Feb 21, 2019 at 9:55 PM Andy Shevchenko
<[email protected]> wrote:
>
> On Thu, Feb 21, 2019 at 6:27 PM Bartosz Golaszewski <[email protected]> wrote:
> >
> > From: Bartosz Golaszewski <[email protected]>
> >
> > 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
czw., 21 lut 2019 o 20:56 Andy Shevchenko <[email protected]>
napisał(a):
>
> On Thu, Feb 21, 2019 at 9:55 PM Andy Shevchenko
> <[email protected]> wrote:
> >
> > On Thu, Feb 21, 2019 at 6:27 PM Bartosz Golaszewski <[email protected]> wrote:
> > >
> > > From: Bartosz Golaszewski <[email protected]>
> > >
> > > 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
On Fri, Feb 22, 2019 at 11:04 AM Bartosz Golaszewski <[email protected]> wrote:
>
> czw., 21 lut 2019 o 20:56 Andy Shevchenko <[email protected]>
> napisał(a):
> >
> > On Thu, Feb 21, 2019 at 9:55 PM Andy Shevchenko
> > <[email protected]> wrote:
> > >
> > > On Thu, Feb 21, 2019 at 6:27 PM Bartosz Golaszewski <[email protected]> wrote:
> > > >
> > > > From: Bartosz Golaszewski <[email protected]>
> > > >
> > > > 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
On Thu, Feb 21, 2019 at 1:56 PM Andy Shevchenko
<[email protected]> wrote:
>
> On Thu, Feb 21, 2019 at 6:27 PM Bartosz Golaszewski <[email protected]> wrote:
> >
> > From: Bartosz Golaszewski <[email protected]>
> >
> > 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
On Fri, Feb 22, 2019 at 3:22 PM Rob Herring <[email protected]> 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
From: Markus Elfring <[email protected]>
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 <[email protected]>
---
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
pt., 14 cze 2019 o 18:50 Markus Elfring <[email protected]> napisał(a):
>
> From: Markus Elfring <[email protected]>
> 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 <[email protected]>
> ---
> 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
On 24.06.19 10:29, Bartosz Golaszewski wrote:
> pt., 14 cze 2019 o 18:50 Markus Elfring <[email protected]> napisał(a):
>>
>> From: Markus Elfring <[email protected]>
>> 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 <[email protected]>
>> ---
>> 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
[email protected] -- +49-151-27565287
pon., 24 cze 2019 o 12:05 Enrico Weigelt, metux IT consult
<[email protected]> napisał(a):
>
> On 24.06.19 10:29, Bartosz Golaszewski wrote:
> > pt., 14 cze 2019 o 18:50 Markus Elfring <[email protected]> napisał(a):
> >>
> >> From: Markus Elfring <[email protected]>
> >> 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 <[email protected]>
> >> ---
> >> 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
>> +++ 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
> 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
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
[email protected] -- +49-151-27565287
pon., 24 cze 2019 o 20:22 Enrico Weigelt, metux IT consult
<[email protected]> 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
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
> <[email protected]> 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
> 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
wt., 25 cze 2019 o 09:30 Greg Kroah-Hartman
<[email protected]> 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
> > <[email protected]> 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
> 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