* [PATCH] RM: shmobile: lager: phy fixup needs CONFIG_PHYLIB
@ 2013-10-31 6:31 Simon Horman
2013-10-31 21:31 ` Sergei Shtylyov
0 siblings, 1 reply; 7+ messages in thread
From: Simon Horman @ 2013-10-31 6:31 UTC (permalink / raw)
To: linux-arm-kernel
Do not build the phy fixup unless CONFIG_PHYLIB is enabled.
Other than not being useful it is also not possible to compile
the code under this condition as phy_register_fixup_for_id()
is not defined.
This problem was introduced by 48c8b96f21817aad
("ARM: shmobile: Lager: add Micrel KSZ8041 PHY fixup")
Cc: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>
Signed-off-by: Simon Horman <horms+renesas@verge.net.au>
---
arch/arm/mach-shmobile/board-lager.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/arch/arm/mach-shmobile/board-lager.c b/arch/arm/mach-shmobile/board-lager.c
index 78a31b6..d1a8ddd 100644
--- a/arch/arm/mach-shmobile/board-lager.c
+++ b/arch/arm/mach-shmobile/board-lager.c
@@ -245,7 +245,9 @@ static void __init lager_init(void)
{
lager_add_standard_devices();
- phy_register_fixup_for_id("r8a7790-ether-ff:01", lager_ksz8041_fixup);
+ if (IS_ENABLED(CONFIG_PHYLIB))
+ phy_register_fixup_for_id("r8a7790-ether-ff:01",
+ lager_ksz8041_fixup);
}
static const char * const lager_boards_compat_dt[] __initconst = {
--
1.8.4
^ permalink raw reply related [flat|nested] 7+ messages in thread* [PATCH] RM: shmobile: lager: phy fixup needs CONFIG_PHYLIB
2013-10-31 6:31 [PATCH] RM: shmobile: lager: phy fixup needs CONFIG_PHYLIB Simon Horman
@ 2013-10-31 21:31 ` Sergei Shtylyov
2013-11-01 0:43 ` Simon Horman
2013-11-05 21:25 ` Sergei Shtylyov
0 siblings, 2 replies; 7+ messages in thread
From: Sergei Shtylyov @ 2013-10-31 21:31 UTC (permalink / raw)
To: linux-arm-kernel
On 10/31/2013 09:31 AM, Simon Horman wrote:
> Do not build the phy fixup unless CONFIG_PHYLIB is enabled.
> Other than not being useful it is also not possible to compile
s/compile/link/.
> the code under this condition as phy_register_fixup_for_id()
> is not defined.
Not only this function is absent...
> This problem was introduced by 48c8b96f21817aad
> ("ARM: shmobile: Lager: add Micrel KSZ8041 PHY fixup")
> Cc: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>
> Signed-off-by: Simon Horman <horms+renesas@verge.net.au>
> ---
> arch/arm/mach-shmobile/board-lager.c | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
> diff --git a/arch/arm/mach-shmobile/board-lager.c b/arch/arm/mach-shmobile/board-lager.c
> index 78a31b6..d1a8ddd 100644
> --- a/arch/arm/mach-shmobile/board-lager.c
> +++ b/arch/arm/mach-shmobile/board-lager.c
> @@ -245,7 +245,9 @@ static void __init lager_init(void)
> {
> lager_add_standard_devices();
>
> - phy_register_fixup_for_id("r8a7790-ether-ff:01", lager_ksz8041_fixup);
> + if (IS_ENABLED(CONFIG_PHYLIB))
> + phy_register_fixup_for_id("r8a7790-ether-ff:01",
> + lager_ksz8041_fixup);
Perhaps you would consider enclosing the fixup function itself into #ifdef
as it causes link errors as well? Doesn't seem necessary as gcc probably
drops it anyway but for completeness' sake...
WBR, Sergei
^ permalink raw reply [flat|nested] 7+ messages in thread* [PATCH] RM: shmobile: lager: phy fixup needs CONFIG_PHYLIB
2013-10-31 21:31 ` Sergei Shtylyov
@ 2013-11-01 0:43 ` Simon Horman
2013-11-02 0:30 ` Sergei Shtylyov
2013-11-05 21:25 ` Sergei Shtylyov
1 sibling, 1 reply; 7+ messages in thread
From: Simon Horman @ 2013-11-01 0:43 UTC (permalink / raw)
To: linux-arm-kernel
On Fri, Nov 01, 2013 at 12:31:02AM +0300, Sergei Shtylyov wrote:
> On 10/31/2013 09:31 AM, Simon Horman wrote:
>
> >Do not build the phy fixup unless CONFIG_PHYLIB is enabled.
> >Other than not being useful it is also not possible to compile
>
> s/compile/link/.
>
> >the code under this condition as phy_register_fixup_for_id()
> >is not defined.
>
> Not only this function is absent...
>
> >This problem was introduced by 48c8b96f21817aad
> >("ARM: shmobile: Lager: add Micrel KSZ8041 PHY fixup")
>
> >Cc: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>
> >Signed-off-by: Simon Horman <horms+renesas@verge.net.au>
> >---
> > arch/arm/mach-shmobile/board-lager.c | 4 +++-
> > 1 file changed, 3 insertions(+), 1 deletion(-)
>
> >diff --git a/arch/arm/mach-shmobile/board-lager.c b/arch/arm/mach-shmobile/board-lager.c
> >index 78a31b6..d1a8ddd 100644
> >--- a/arch/arm/mach-shmobile/board-lager.c
> >+++ b/arch/arm/mach-shmobile/board-lager.c
> >@@ -245,7 +245,9 @@ static void __init lager_init(void)
> > {
> > lager_add_standard_devices();
> >
> >- phy_register_fixup_for_id("r8a7790-ether-ff:01", lager_ksz8041_fixup);
> >+ if (IS_ENABLED(CONFIG_PHYLIB))
> >+ phy_register_fixup_for_id("r8a7790-ether-ff:01",
> >+ lager_ksz8041_fixup);
>
> Perhaps you would consider enclosing the fixup function itself
> into #ifdef as it causes link errors as well? Doesn't seem
> necessary as gcc probably drops it anyway but for completeness'
> sake...
My understanding of the motivation for IS_ENABLED() is to
take advantage of gcc optimising away unused code and making such
problems disappear.
With this patch in place do you still see any warnings?
^ permalink raw reply [flat|nested] 7+ messages in thread* [PATCH] RM: shmobile: lager: phy fixup needs CONFIG_PHYLIB
2013-11-01 0:43 ` Simon Horman
@ 2013-11-02 0:30 ` Sergei Shtylyov
2013-11-04 23:58 ` Simon Horman
0 siblings, 1 reply; 7+ messages in thread
From: Sergei Shtylyov @ 2013-11-02 0:30 UTC (permalink / raw)
To: linux-arm-kernel
On 11/01/2013 03:43 AM, Simon Horman wrote:
>>> Do not build the phy fixup unless CONFIG_PHYLIB is enabled.
>>> Other than not being useful it is also not possible to compile
>> s/compile/link/.
>>> the code under this condition as phy_register_fixup_for_id()
>>> is not defined.
>> Not only this function is absent...
>>> This problem was introduced by 48c8b96f21817aad
>>> ("ARM: shmobile: Lager: add Micrel KSZ8041 PHY fixup")
>>> Cc: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>
>>> Signed-off-by: Simon Horman <horms+renesas@verge.net.au>
>>> ---
>>> arch/arm/mach-shmobile/board-lager.c | 4 +++-
>>> 1 file changed, 3 insertions(+), 1 deletion(-)
>>> diff --git a/arch/arm/mach-shmobile/board-lager.c b/arch/arm/mach-shmobile/board-lager.c
>>> index 78a31b6..d1a8ddd 100644
>>> --- a/arch/arm/mach-shmobile/board-lager.c
>>> +++ b/arch/arm/mach-shmobile/board-lager.c
>>> @@ -245,7 +245,9 @@ static void __init lager_init(void)
>>> {
>>> lager_add_standard_devices();
>>>
>>> - phy_register_fixup_for_id("r8a7790-ether-ff:01", lager_ksz8041_fixup);
>>> + if (IS_ENABLED(CONFIG_PHYLIB))
>>> + phy_register_fixup_for_id("r8a7790-ether-ff:01",
>>> + lager_ksz8041_fixup);
>> Perhaps you would consider enclosing the fixup function itself
>> into #ifdef as it causes link errors as well? Doesn't seem
>> necessary as gcc probably drops it anyway but for completeness'
>> sake...
> My understanding of the motivation for IS_ENABLED() is to
> take advantage of gcc optimising away unused code and making such
> problems disappear.
> With this patch in place do you still see any warnings?
No (but those were errors :-).
I'd like the changelog to be adjusted at least...
WBR, Sergei
^ permalink raw reply [flat|nested] 7+ messages in thread* [PATCH] RM: shmobile: lager: phy fixup needs CONFIG_PHYLIB
2013-11-02 0:30 ` Sergei Shtylyov
@ 2013-11-04 23:58 ` Simon Horman
2013-11-05 17:53 ` Sergei Shtylyov
0 siblings, 1 reply; 7+ messages in thread
From: Simon Horman @ 2013-11-04 23:58 UTC (permalink / raw)
To: linux-arm-kernel
On Sat, Nov 02, 2013 at 03:30:25AM +0300, Sergei Shtylyov wrote:
> On 11/01/2013 03:43 AM, Simon Horman wrote:
>
> >>>Do not build the phy fixup unless CONFIG_PHYLIB is enabled.
> >>>Other than not being useful it is also not possible to compile
>
> >> s/compile/link/.
>
> >>>the code under this condition as phy_register_fixup_for_id()
> >>>is not defined.
>
> >> Not only this function is absent...
>
> >>>This problem was introduced by 48c8b96f21817aad
> >>>("ARM: shmobile: Lager: add Micrel KSZ8041 PHY fixup")
>
> >>>Cc: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>
> >>>Signed-off-by: Simon Horman <horms+renesas@verge.net.au>
> >>>---
> >>> arch/arm/mach-shmobile/board-lager.c | 4 +++-
> >>> 1 file changed, 3 insertions(+), 1 deletion(-)
>
> >>>diff --git a/arch/arm/mach-shmobile/board-lager.c b/arch/arm/mach-shmobile/board-lager.c
> >>>index 78a31b6..d1a8ddd 100644
> >>>--- a/arch/arm/mach-shmobile/board-lager.c
> >>>+++ b/arch/arm/mach-shmobile/board-lager.c
> >>>@@ -245,7 +245,9 @@ static void __init lager_init(void)
> >>> {
> >>> lager_add_standard_devices();
> >>>
> >>>- phy_register_fixup_for_id("r8a7790-ether-ff:01", lager_ksz8041_fixup);
> >>>+ if (IS_ENABLED(CONFIG_PHYLIB))
> >>>+ phy_register_fixup_for_id("r8a7790-ether-ff:01",
> >>>+ lager_ksz8041_fixup);
>
> >> Perhaps you would consider enclosing the fixup function itself
> >>into #ifdef as it causes link errors as well? Doesn't seem
> >>necessary as gcc probably drops it anyway but for completeness'
> >>sake...
>
> >My understanding of the motivation for IS_ENABLED() is to
> >take advantage of gcc optimising away unused code and making such
> >problems disappear.
>
> >With this patch in place do you still see any warnings?
>
> No (but those were errors :-).
> I'd like the changelog to be adjusted at least...
Sure. Would you like me to list in the changelog the compile problems that
gcc reports without this patch? Or do you have something else in mind?
^ permalink raw reply [flat|nested] 7+ messages in thread* [PATCH] RM: shmobile: lager: phy fixup needs CONFIG_PHYLIB
2013-11-04 23:58 ` Simon Horman
@ 2013-11-05 17:53 ` Sergei Shtylyov
0 siblings, 0 replies; 7+ messages in thread
From: Sergei Shtylyov @ 2013-11-05 17:53 UTC (permalink / raw)
To: linux-arm-kernel
Hello.
On 11/05/2013 02:58 AM, Simon Horman wrote:
>>>>> Do not build the phy fixup unless CONFIG_PHYLIB is enabled.
>>>>> Other than not being useful it is also not possible to compile
>>>> s/compile/link/.
>>>>> the code under this condition as phy_register_fixup_for_id()
>>>>> is not defined.
>>>> Not only this function is absent...
>>>>> This problem was introduced by 48c8b96f21817aad
>>>>> ("ARM: shmobile: Lager: add Micrel KSZ8041 PHY fixup")
>>>>> Cc: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>
>>>>> Signed-off-by: Simon Horman <horms+renesas@verge.net.au>
>>>>> ---
>>>>> arch/arm/mach-shmobile/board-lager.c | 4 +++-
>>>>> 1 file changed, 3 insertions(+), 1 deletion(-)
>>>>> diff --git a/arch/arm/mach-shmobile/board-lager.c b/arch/arm/mach-shmobile/board-lager.c
>>>>> index 78a31b6..d1a8ddd 100644
>>>>> --- a/arch/arm/mach-shmobile/board-lager.c
>>>>> +++ b/arch/arm/mach-shmobile/board-lager.c
>>>>> @@ -245,7 +245,9 @@ static void __init lager_init(void)
>>>>> {
>>>>> lager_add_standard_devices();
>>>>>
>>>>> - phy_register_fixup_for_id("r8a7790-ether-ff:01", lager_ksz8041_fixup);
>>>>> + if (IS_ENABLED(CONFIG_PHYLIB))
>>>>> + phy_register_fixup_for_id("r8a7790-ether-ff:01",
>>>>> + lager_ksz8041_fixup);
>>>> Perhaps you would consider enclosing the fixup function itself
>>>> into #ifdef as it causes link errors as well? Doesn't seem
>>>> necessary as gcc probably drops it anyway but for completeness'
>>>> sake...
>>> My understanding of the motivation for IS_ENABLED() is to
>>> take advantage of gcc optimising away unused code and making such
>>> problems disappear.
>>> With this patch in place do you still see any warnings?
>> No (but those were errors :-).
>> I'd like the changelog to be adjusted at least...
> Sure. Would you like me to list in the changelog the compile problems that
> gcc reports without this patch?
Erm... gcc doesn't report any compile problems, it's ld that does report
link errors. That would be good to include them, yes.
> Or do you have something else in mind?
See the top of the mail for my comments to the changelog. Also, you need
to fix the subject which has "RM:" instead of "ARM:"...
WBR, Sergei
^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH] RM: shmobile: lager: phy fixup needs CONFIG_PHYLIB
2013-10-31 21:31 ` Sergei Shtylyov
2013-11-01 0:43 ` Simon Horman
@ 2013-11-05 21:25 ` Sergei Shtylyov
1 sibling, 0 replies; 7+ messages in thread
From: Sergei Shtylyov @ 2013-11-05 21:25 UTC (permalink / raw)
To: linux-arm-kernel
Hello.
On 11/01/2013 12:31 AM, Sergei Shtylyov wrote:
>> Do not build the phy fixup unless CONFIG_PHYLIB is enabled.
>> Other than not being useful it is also not possible to compile
> s/compile/link/.
>> the code under this condition as phy_register_fixup_for_id()
>> is not defined.
> Not only this function is absent...
>> This problem was introduced by 48c8b96f21817aad
>> ("ARM: shmobile: Lager: add Micrel KSZ8041 PHY fixup")
>> Cc: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>
>> Signed-off-by: Simon Horman <horms+renesas@verge.net.au>
>> ---
>> arch/arm/mach-shmobile/board-lager.c | 4 +++-
>> 1 file changed, 3 insertions(+), 1 deletion(-)
>> diff --git a/arch/arm/mach-shmobile/board-lager.c
>> b/arch/arm/mach-shmobile/board-lager.c
>> index 78a31b6..d1a8ddd 100644
>> --- a/arch/arm/mach-shmobile/board-lager.c
>> +++ b/arch/arm/mach-shmobile/board-lager.c
>> @@ -245,7 +245,9 @@ static void __init lager_init(void)
>> {
>> lager_add_standard_devices();
>>
>> - phy_register_fixup_for_id("r8a7790-ether-ff:01", lager_ksz8041_fixup);
>> + if (IS_ENABLED(CONFIG_PHYLIB))
>> + phy_register_fixup_for_id("r8a7790-ether-ff:01",
>> + lager_ksz8041_fixup);
> Perhaps you would consider enclosing the fixup function itself into #ifdef
> as it causes link errors as well? Doesn't seem necessary as gcc probably
> drops it anyway but for completeness' sake...
It's not necessary and it won't even work (causes compilation failure) as
my testing has shown, so sorry for wasting your time on this suggestion.
WBR, Sergei
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2013-11-05 21:25 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2013-10-31 6:31 [PATCH] RM: shmobile: lager: phy fixup needs CONFIG_PHYLIB Simon Horman
2013-10-31 21:31 ` Sergei Shtylyov
2013-11-01 0:43 ` Simon Horman
2013-11-02 0:30 ` Sergei Shtylyov
2013-11-04 23:58 ` Simon Horman
2013-11-05 17:53 ` Sergei Shtylyov
2013-11-05 21:25 ` Sergei Shtylyov
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).