Fixes:
arch/arm/mach-at91/built-in.o: In function `ksz9021rn_phy_fixup':
:(.text+0x1174): undefined reference to `mdiobus_write'
:(.text+0x1188): undefined reference to `mdiobus_write'
:(.text+0x119c): undefined reference to `mdiobus_write'
:(.text+0x11b0): undefined reference to `mdiobus_write'
arch/arm/mach-at91/built-in.o: In function `sama5_dt_device_init':
:(.init.text+0x1e34): undefined reference to `phy_register_fixup_for_uid'
when CONFIG_PHYLIB is not selected.
Signed-off-by: Alexandre Belloni <[email protected]>
Acked-by: Ludovic Desroches <[email protected]>
---
Changes in v3:
added SoB
added Acked-By
Changes in v2:
use IS_BUILTIN
use CONFIG_PHYLIB and not CONFIG_PHY
arch/arm/mach-at91/board-dt-sama5.c | 22 ++++++++++++----------
1 file changed, 12 insertions(+), 10 deletions(-)
diff --git a/arch/arm/mach-at91/board-dt-sama5.c b/arch/arm/mach-at91/board-dt-sama5.c
index 705305e..e9ce541 100644
--- a/arch/arm/mach-at91/board-dt-sama5.c
+++ b/arch/arm/mach-at91/board-dt-sama5.c
@@ -47,22 +47,24 @@ static int ksz9021rn_phy_fixup(struct phy_device *phy)
#define GMII_ERCR 11
#define GMII_ERDWR 12
- /* Set delay values */
- value = GMII_RCCPSR | 0x8000;
- phy_write(phy, GMII_ERCR, value);
- value = 0xF2F4;
- phy_write(phy, GMII_ERDWR, value);
- value = GMII_RRDPSR | 0x8000;
- phy_write(phy, GMII_ERCR, value);
- value = 0x2222;
- phy_write(phy, GMII_ERDWR, value);
+ if (IS_BUILTIN(CONFIG_PHYLIB)) {
+ /* Set delay values */
+ value = GMII_RCCPSR | 0x8000;
+ phy_write(phy, GMII_ERCR, value);
+ value = 0xF2F4;
+ phy_write(phy, GMII_ERDWR, value);
+ value = GMII_RRDPSR | 0x8000;
+ phy_write(phy, GMII_ERCR, value);
+ value = 0x2222;
+ phy_write(phy, GMII_ERDWR, value);
+ }
return 0;
}
static void __init sama5_dt_device_init(void)
{
- if (of_machine_is_compatible("atmel,sama5d3xcm"))
+ if (of_machine_is_compatible("atmel,sama5d3xcm") && IS_BUILTIN(CONFIG_PHYLIB))
phy_register_fixup_for_uid(PHY_ID_KSZ9021, MICREL_PHY_ID_MASK,
ksz9021rn_phy_fixup);
--
1.8.1.2
On May 27, 2013, at 6:48 PM, Alexandre Belloni <[email protected]> wrote:
> Fixes:
> arch/arm/mach-at91/built-in.o: In function `ksz9021rn_phy_fixup':
> :(.text+0x1174): undefined reference to `mdiobus_write'
> :(.text+0x1188): undefined reference to `mdiobus_write'
> :(.text+0x119c): undefined reference to `mdiobus_write'
> :(.text+0x11b0): undefined reference to `mdiobus_write'
> arch/arm/mach-at91/built-in.o: In function `sama5_dt_device_init':
> :(.init.text+0x1e34): undefined reference to `phy_register_fixup_for_uid'
>
> when CONFIG_PHYLIB is not selected.
>
> Signed-off-by: Alexandre Belloni <[email protected]>
> Acked-by: Ludovic Desroches <[email protected]>
no for this change
add an inline version of phy_register_fixup_for_uid
this will do the same for every platform
> ---
> Changes in v3:
> added SoB
> added Acked-By
>
> Changes in v2:
> use IS_BUILTIN
> use CONFIG_PHYLIB and not CONFIG_PHY
>
> arch/arm/mach-at91/board-dt-sama5.c | 22 ++++++++++++----------
> 1 file changed, 12 insertions(+), 10 deletions(-)
>
> diff --git a/arch/arm/mach-at91/board-dt-sama5.c b/arch/arm/mach-at91/board-dt-sama5.c
> index 705305e..e9ce541 100644
> --- a/arch/arm/mach-at91/board-dt-sama5.c
> +++ b/arch/arm/mach-at91/board-dt-sama5.c
> @@ -47,22 +47,24 @@ static int ksz9021rn_phy_fixup(struct phy_device *phy)
> #define GMII_ERCR 11
> #define GMII_ERDWR 12
>
> - /* Set delay values */
> - value = GMII_RCCPSR | 0x8000;
> - phy_write(phy, GMII_ERCR, value);
> - value = 0xF2F4;
> - phy_write(phy, GMII_ERDWR, value);
> - value = GMII_RRDPSR | 0x8000;
> - phy_write(phy, GMII_ERCR, value);
> - value = 0x2222;
> - phy_write(phy, GMII_ERDWR, value);
> + if (IS_BUILTIN(CONFIG_PHYLIB)) {
> + /* Set delay values */
> + value = GMII_RCCPSR | 0x8000;
> + phy_write(phy, GMII_ERCR, value);
> + value = 0xF2F4;
> + phy_write(phy, GMII_ERDWR, value);
> + value = GMII_RRDPSR | 0x8000;
> + phy_write(phy, GMII_ERCR, value);
> + value = 0x2222;
> + phy_write(phy, GMII_ERDWR, value);
> + }
>
> return 0;
> }
>
> static void __init sama5_dt_device_init(void)
> {
> - if (of_machine_is_compatible("atmel,sama5d3xcm"))
> + if (of_machine_is_compatible("atmel,sama5d3xcm") && IS_BUILTIN(CONFIG_PHYLIB))
> phy_register_fixup_for_uid(PHY_ID_KSZ9021, MICREL_PHY_ID_MASK,
> ksz9021rn_phy_fixup);
>
> --
> 1.8.1.2
>
On 27/05/2013 12:48, Alexandre Belloni :
> Fixes:
> arch/arm/mach-at91/built-in.o: In function `ksz9021rn_phy_fixup':
> :(.text+0x1174): undefined reference to `mdiobus_write'
> :(.text+0x1188): undefined reference to `mdiobus_write'
> :(.text+0x119c): undefined reference to `mdiobus_write'
> :(.text+0x11b0): undefined reference to `mdiobus_write'
> arch/arm/mach-at91/built-in.o: In function `sama5_dt_device_init':
> :(.init.text+0x1e34): undefined reference to `phy_register_fixup_for_uid'
>
> when CONFIG_PHYLIB is not selected.
>
> Signed-off-by: Alexandre Belloni <[email protected]>
> Acked-by: Ludovic Desroches <[email protected]>
> ---
> Changes in v3:
> added SoB
> added Acked-By
A v3 is not needed simply for this. Thanks, anyway!
One question though: why not just add
select PHYLIB
In Kconfig entry:
config MACH_SAMA5_DT
?
(note that I like to play with everybody's nerves ;-))
Bye,
> Changes in v2:
> use IS_BUILTIN
> use CONFIG_PHYLIB and not CONFIG_PHY
>
> arch/arm/mach-at91/board-dt-sama5.c | 22 ++++++++++++----------
> 1 file changed, 12 insertions(+), 10 deletions(-)
>
> diff --git a/arch/arm/mach-at91/board-dt-sama5.c b/arch/arm/mach-at91/board-dt-sama5.c
> index 705305e..e9ce541 100644
> --- a/arch/arm/mach-at91/board-dt-sama5.c
> +++ b/arch/arm/mach-at91/board-dt-sama5.c
> @@ -47,22 +47,24 @@ static int ksz9021rn_phy_fixup(struct phy_device *phy)
> #define GMII_ERCR 11
> #define GMII_ERDWR 12
>
> - /* Set delay values */
> - value = GMII_RCCPSR | 0x8000;
> - phy_write(phy, GMII_ERCR, value);
> - value = 0xF2F4;
> - phy_write(phy, GMII_ERDWR, value);
> - value = GMII_RRDPSR | 0x8000;
> - phy_write(phy, GMII_ERCR, value);
> - value = 0x2222;
> - phy_write(phy, GMII_ERDWR, value);
> + if (IS_BUILTIN(CONFIG_PHYLIB)) {
> + /* Set delay values */
> + value = GMII_RCCPSR | 0x8000;
> + phy_write(phy, GMII_ERCR, value);
> + value = 0xF2F4;
> + phy_write(phy, GMII_ERDWR, value);
> + value = GMII_RRDPSR | 0x8000;
> + phy_write(phy, GMII_ERCR, value);
> + value = 0x2222;
> + phy_write(phy, GMII_ERDWR, value);
> + }
>
> return 0;
> }
>
> static void __init sama5_dt_device_init(void)
> {
> - if (of_machine_is_compatible("atmel,sama5d3xcm"))
> + if (of_machine_is_compatible("atmel,sama5d3xcm") && IS_BUILTIN(CONFIG_PHYLIB))
> phy_register_fixup_for_uid(PHY_ID_KSZ9021, MICREL_PHY_ID_MASK,
> ksz9021rn_phy_fixup);
>
>
--
Nicolas Ferre
On Monday 27 May 2013 14:05:13 Nicolas Ferre wrote:
> On 27/05/2013 12:48, Alexandre Belloni :
> A v3 is not needed simply for this. Thanks, anyway!
>
> One question though: why not just add
>
> select PHYLIB
>
> In Kconfig entry:
>
> config MACH_SAMA5_DT
>
> ?
>
> (note that I like to play with everybody's nerves ;-))
That would make it impossible to build a sama5 kernel without networking
support, which is probably not what you want.
Jean-Christophe's suggestion to add the inline helper is best. Aside from
that, please note that
> > diff --git a/arch/arm/mach-at91/board-dt-sama5.c b/arch/arm/mach-at91/board-dt-sama5.c
> > index 705305e..e9ce541 100644
> > --- a/arch/arm/mach-at91/board-dt-sama5.c
> > +++ b/arch/arm/mach-at91/board-dt-sama5.c
> > @@ -47,22 +47,24 @@ static int ksz9021rn_phy_fixup(struct phy_device *phy)
> > #define GMII_ERCR 11
> > #define GMII_ERDWR 12
> >
> > - /* Set delay values */
> > - value = GMII_RCCPSR | 0x8000;
> > - phy_write(phy, GMII_ERCR, value);
> > - value = 0xF2F4;
> > - phy_write(phy, GMII_ERDWR, value);
> > - value = GMII_RRDPSR | 0x8000;
> > - phy_write(phy, GMII_ERCR, value);
> > - value = 0x2222;
> > - phy_write(phy, GMII_ERDWR, value);
> > + if (IS_BUILTIN(CONFIG_PHYLIB)) {
> > + /* Set delay values */
> > + value = GMII_RCCPSR | 0x8000;
> > + phy_write(phy, GMII_ERCR, value);
> > + value = 0xF2F4;
> > + phy_write(phy, GMII_ERDWR, value);
> > + value = GMII_RRDPSR | 0x8000;
> > + phy_write(phy, GMII_ERCR, value);
> > + value = 0x2222;
> > + phy_write(phy, GMII_ERDWR, value);
> > + }
> >
> > return 0;
> > }
This part of the patch is not actually needed if you
> > static void __init sama5_dt_device_init(void)
> > {
> > - if (of_machine_is_compatible("atmel,sama5d3xcm"))
> > + if (of_machine_is_compatible("atmel,sama5d3xcm") && IS_BUILTIN(CONFIG_PHYLIB))
> > phy_register_fixup_for_uid(PHY_ID_KSZ9021, MICREL_PHY_ID_MASK,
> > ksz9021rn_phy_fixup);
do this change, or the inline helper.
Arnd
On 27/05/2013 14:05, Nicolas Ferre wrote:
> On 27/05/2013 12:48, Alexandre Belloni :
>> Fixes:
>> arch/arm/mach-at91/built-in.o: In function `ksz9021rn_phy_fixup':
>> :(.text+0x1174): undefined reference to `mdiobus_write'
>> :(.text+0x1188): undefined reference to `mdiobus_write'
>> :(.text+0x119c): undefined reference to `mdiobus_write'
>> :(.text+0x11b0): undefined reference to `mdiobus_write'
>> arch/arm/mach-at91/built-in.o: In function `sama5_dt_device_init':
>> :(.init.text+0x1e34): undefined reference to
>> `phy_register_fixup_for_uid'
>>
>> when CONFIG_PHYLIB is not selected.
>>
>> Signed-off-by: Alexandre Belloni <[email protected]>
>> Acked-by: Ludovic Desroches <[email protected]>
>> ---
>> Changes in v3:
>> added SoB
>> added Acked-By
>
> A v3 is not needed simply for this. Thanks, anyway!
>
It was not much and I have to stop forgetting my SoB ;)
> One question though: why not just add
>
> select PHYLIB
>
> In Kconfig entry:
>
> config MACH_SAMA5_DT
>
> ?
>
That would simply mean that you have to enable NET when using a SAMA5
which is not what every user will want.
Regards,
--
Alexandre Belloni, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com
On 27/05/2013 13:26, Jean-Christophe PLAGNIOL-VILLARD wrote:
> On May 27, 2013, at 6:48 PM, Alexandre Belloni <[email protected]> wrote:
>
>> Fixes:
>> arch/arm/mach-at91/built-in.o: In function `ksz9021rn_phy_fixup':
>> :(.text+0x1174): undefined reference to `mdiobus_write'
>> :(.text+0x1188): undefined reference to `mdiobus_write'
>> :(.text+0x119c): undefined reference to `mdiobus_write'
>> :(.text+0x11b0): undefined reference to `mdiobus_write'
>> arch/arm/mach-at91/built-in.o: In function `sama5_dt_device_init':
>> :(.init.text+0x1e34): undefined reference to `phy_register_fixup_for_uid'
>>
>> when CONFIG_PHYLIB is not selected.
>>
>> Signed-off-by: Alexandre Belloni <[email protected]>
>> Acked-by: Ludovic Desroches <[email protected]>
> no for this change
>
> add an inline version of phy_register_fixup_for_uid
>
> this will do the same for every platform
I don't actually see the added value of doing that. Using IS_BUILTIN is
readable and ensure that you will definitely be able to link. The
functions will be optimized out and removed by gcc even when you have
CONFIG_PHYLIB=m.
>> ---
>> Changes in v3:
>> added SoB
>> added Acked-By
>>
>> Changes in v2:
>> use IS_BUILTIN
>> use CONFIG_PHYLIB and not CONFIG_PHY
>>
>> arch/arm/mach-at91/board-dt-sama5.c | 22 ++++++++++++----------
>> 1 file changed, 12 insertions(+), 10 deletions(-)
>>
>> diff --git a/arch/arm/mach-at91/board-dt-sama5.c b/arch/arm/mach-at91/board-dt-sama5.c
>> index 705305e..e9ce541 100644
>> --- a/arch/arm/mach-at91/board-dt-sama5.c
>> +++ b/arch/arm/mach-at91/board-dt-sama5.c
>> @@ -47,22 +47,24 @@ static int ksz9021rn_phy_fixup(struct phy_device *phy)
>> #define GMII_ERCR 11
>> #define GMII_ERDWR 12
>>
>> - /* Set delay values */
>> - value = GMII_RCCPSR | 0x8000;
>> - phy_write(phy, GMII_ERCR, value);
>> - value = 0xF2F4;
>> - phy_write(phy, GMII_ERDWR, value);
>> - value = GMII_RRDPSR | 0x8000;
>> - phy_write(phy, GMII_ERCR, value);
>> - value = 0x2222;
>> - phy_write(phy, GMII_ERDWR, value);
>> + if (IS_BUILTIN(CONFIG_PHYLIB)) {
>> + /* Set delay values */
>> + value = GMII_RCCPSR | 0x8000;
>> + phy_write(phy, GMII_ERCR, value);
>> + value = 0xF2F4;
>> + phy_write(phy, GMII_ERDWR, value);
>> + value = GMII_RRDPSR | 0x8000;
>> + phy_write(phy, GMII_ERCR, value);
>> + value = 0x2222;
>> + phy_write(phy, GMII_ERDWR, value);
>> + }
>>
>> return 0;
>> }
>>
>> static void __init sama5_dt_device_init(void)
>> {
>> - if (of_machine_is_compatible("atmel,sama5d3xcm"))
>> + if (of_machine_is_compatible("atmel,sama5d3xcm") && IS_BUILTIN(CONFIG_PHYLIB))
>> phy_register_fixup_for_uid(PHY_ID_KSZ9021, MICREL_PHY_ID_MASK,
>> ksz9021rn_phy_fixup);
>>
>> --
>> 1.8.1.2
>>
--
Alexandre Belloni, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com
On May 27, 2013, at 8:05 PM, Nicolas Ferre <[email protected]> wrote:
> On 27/05/2013 12:48, Alexandre Belloni :
>> Fixes:
>> arch/arm/mach-at91/built-in.o: In function `ksz9021rn_phy_fixup':
>> :(.text+0x1174): undefined reference to `mdiobus_write'
>> :(.text+0x1188): undefined reference to `mdiobus_write'
>> :(.text+0x119c): undefined reference to `mdiobus_write'
>> :(.text+0x11b0): undefined reference to `mdiobus_write'
>> arch/arm/mach-at91/built-in.o: In function `sama5_dt_device_init':
>> :(.init.text+0x1e34): undefined reference to `phy_register_fixup_for_uid'
>>
>> when CONFIG_PHYLIB is not selected.
>>
>> Signed-off-by: Alexandre Belloni <[email protected]>
>> Acked-by: Ludovic Desroches <[email protected]>
>> ---
>> Changes in v3:
>> added SoB
>> added Acked-By
>
> A v3 is not needed simply for this. Thanks, anyway!
>
> One question though: why not just add
>
> select PHYLIB
>
> In Kconfig entry:
>
> config MACH_SAMA5_DT
>
> ?
>
> (note that I like to play with everybody's nerves ;-))
>
I prefer this than the IS_BUILTIN
> Bye,
>
>> Changes in v2:
>> use IS_BUILTIN
>> use CONFIG_PHYLIB and not CONFIG_PHY
>>
>> arch/arm/mach-at91/board-dt-sama5.c | 22 ++++++++++++----------
>> 1 file changed, 12 insertions(+), 10 deletions(-)
>>
>> diff --git a/arch/arm/mach-at91/board-dt-sama5.c b/arch/arm/mach-at91/board-dt-sama5.c
>> index 705305e..e9ce541 100644
>> --- a/arch/arm/mach-at91/board-dt-sama5.c
>> +++ b/arch/arm/mach-at91/board-dt-sama5.c
>> @@ -47,22 +47,24 @@ static int ksz9021rn_phy_fixup(struct phy_device *phy)
>> #define GMII_ERCR 11
>> #define GMII_ERDWR 12
>>
>> - /* Set delay values */
>> - value = GMII_RCCPSR | 0x8000;
>> - phy_write(phy, GMII_ERCR, value);
>> - value = 0xF2F4;
>> - phy_write(phy, GMII_ERDWR, value);
>> - value = GMII_RRDPSR | 0x8000;
>> - phy_write(phy, GMII_ERCR, value);
>> - value = 0x2222;
>> - phy_write(phy, GMII_ERDWR, value);
>> + if (IS_BUILTIN(CONFIG_PHYLIB)) {
>> + /* Set delay values */
>> + value = GMII_RCCPSR | 0x8000;
>> + phy_write(phy, GMII_ERCR, value);
>> + value = 0xF2F4;
>> + phy_write(phy, GMII_ERDWR, value);
>> + value = GMII_RRDPSR | 0x8000;
>> + phy_write(phy, GMII_ERCR, value);
>> + value = 0x2222;
>> + phy_write(phy, GMII_ERDWR, value);
>> + }
>>
>> return 0;
>> }
>>
>> static void __init sama5_dt_device_init(void)
>> {
>> - if (of_machine_is_compatible("atmel,sama5d3xcm"))
>> + if (of_machine_is_compatible("atmel,sama5d3xcm") && IS_BUILTIN(CONFIG_PHYLIB))
>> phy_register_fixup_for_uid(PHY_ID_KSZ9021, MICREL_PHY_ID_MASK,
>> ksz9021rn_phy_fixup);
>>
>>
>
>
> --
> Nicolas Ferre
On May 27, 2013, at 8:50 PM, Alexandre Belloni <[email protected]> wrote:
> On 27/05/2013 13:26, Jean-Christophe PLAGNIOL-VILLARD wrote:
>> On May 27, 2013, at 6:48 PM, Alexandre Belloni <[email protected]> wrote:
>>
>>> Fixes:
>>> arch/arm/mach-at91/built-in.o: In function `ksz9021rn_phy_fixup':
>>> :(.text+0x1174): undefined reference to `mdiobus_write'
>>> :(.text+0x1188): undefined reference to `mdiobus_write'
>>> :(.text+0x119c): undefined reference to `mdiobus_write'
>>> :(.text+0x11b0): undefined reference to `mdiobus_write'
>>> arch/arm/mach-at91/built-in.o: In function `sama5_dt_device_init':
>>> :(.init.text+0x1e34): undefined reference to `phy_register_fixup_for_uid'
>>>
>>> when CONFIG_PHYLIB is not selected.
>>>
>>> Signed-off-by: Alexandre Belloni <[email protected]>
>>> Acked-by: Ludovic Desroches <[email protected]>
>> no for this change
>>
>> add an inline version of phy_register_fixup_for_uid
>>
>> this will do the same for every platform
>
> I don't actually see the added value of doing that. Using IS_BUILTIN is
> readable and ensure that you will definitely be able to link. The
> functions will be optimized out and removed by gcc even when you have
> CONFIG_PHYLIB=m.
no will not as you do ifdef CONFIG_PHYLIB and if CONFIG_PHYLIB=m the macro is defended
we do this all the time in the kernel
I add the IS_BUILTIN & co to be used when a simple inline is not sufficient
Best Regards,
J/
>
>>> ---
>>> Changes in v3:
>>> added SoB
>>> added Acked-By
>>>
>>> Changes in v2:
>>> use IS_BUILTIN
>>> use CONFIG_PHYLIB and not CONFIG_PHY
>>>
>>> arch/arm/mach-at91/board-dt-sama5.c | 22 ++++++++++++----------
>>> 1 file changed, 12 insertions(+), 10 deletions(-)
>>>
>>> diff --git a/arch/arm/mach-at91/board-dt-sama5.c b/arch/arm/mach-at91/board-dt-sama5.c
>>> index 705305e..e9ce541 100644
>>> --- a/arch/arm/mach-at91/board-dt-sama5.c
>>> +++ b/arch/arm/mach-at91/board-dt-sama5.c
>>> @@ -47,22 +47,24 @@ static int ksz9021rn_phy_fixup(struct phy_device *phy)
>>> #define GMII_ERCR 11
>>> #define GMII_ERDWR 12
>>>
>>> - /* Set delay values */
>>> - value = GMII_RCCPSR | 0x8000;
>>> - phy_write(phy, GMII_ERCR, value);
>>> - value = 0xF2F4;
>>> - phy_write(phy, GMII_ERDWR, value);
>>> - value = GMII_RRDPSR | 0x8000;
>>> - phy_write(phy, GMII_ERCR, value);
>>> - value = 0x2222;
>>> - phy_write(phy, GMII_ERDWR, value);
>>> + if (IS_BUILTIN(CONFIG_PHYLIB)) {
>>> + /* Set delay values */
>>> + value = GMII_RCCPSR | 0x8000;
>>> + phy_write(phy, GMII_ERCR, value);
>>> + value = 0xF2F4;
>>> + phy_write(phy, GMII_ERDWR, value);
>>> + value = GMII_RRDPSR | 0x8000;
>>> + phy_write(phy, GMII_ERCR, value);
>>> + value = 0x2222;
>>> + phy_write(phy, GMII_ERDWR, value);
>>> + }
>>>
>>> return 0;
>>> }
>>>
>>> static void __init sama5_dt_device_init(void)
>>> {
>>> - if (of_machine_is_compatible("atmel,sama5d3xcm"))
>>> + if (of_machine_is_compatible("atmel,sama5d3xcm") && IS_BUILTIN(CONFIG_PHYLIB))
>>> phy_register_fixup_for_uid(PHY_ID_KSZ9021, MICREL_PHY_ID_MASK,
>>> ksz9021rn_phy_fixup);
>>>
>>> --
>>> 1.8.1.2
>>>
>
>
> --
> Alexandre Belloni, Free Electrons
> Embedded Linux, Kernel and Android engineering
> http://free-electrons.com
>
On 27/05/2013 14:59, Jean-Christophe PLAGNIOL-VILLARD wrote:
>
> no will not as you do ifdef CONFIG_PHYLIB and if CONFIG_PHYLIB=m the macro is defended
>
> we do this all the time in the kernel
>
> I add the IS_BUILTIN & co to be used when a simple inline is not sufficient
Ok, it took me a while to understand what you were trying to explain.
I'm sending a patch set now. I don't believe it will be taken in for
3.10 though.
Regards,
--
Alexandre Belloni, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com
On 27/05/2013 14:42, Arnd Bergmann :
> On Monday 27 May 2013 14:05:13 Nicolas Ferre wrote:
>> On 27/05/2013 12:48, Alexandre Belloni :
>> A v3 is not needed simply for this. Thanks, anyway!
>>
>> One question though: why not just add
>>
>> select PHYLIB
>>
>> In Kconfig entry:
>>
>> config MACH_SAMA5_DT
>>
>> ?
>>
>> (note that I like to play with everybody's nerves ;-))
>
> That would make it impossible to build a sama5 kernel without networking
> support, which is probably not what you want.
>
> Jean-Christophe's suggestion to add the inline helper is best.
It seems that it has been clearly rejected by David. So the best
solution remains to...
> Aside from
> that, please note that
>
>>> diff --git a/arch/arm/mach-at91/board-dt-sama5.c b/arch/arm/mach-at91/board-dt-sama5.c
>>> index 705305e..e9ce541 100644
>>> --- a/arch/arm/mach-at91/board-dt-sama5.c
>>> +++ b/arch/arm/mach-at91/board-dt-sama5.c
>>> @@ -47,22 +47,24 @@ static int ksz9021rn_phy_fixup(struct phy_device *phy)
>>> #define GMII_ERCR 11
>>> #define GMII_ERDWR 12
>>>
>>> - /* Set delay values */
>>> - value = GMII_RCCPSR | 0x8000;
>>> - phy_write(phy, GMII_ERCR, value);
>>> - value = 0xF2F4;
>>> - phy_write(phy, GMII_ERDWR, value);
>>> - value = GMII_RRDPSR | 0x8000;
>>> - phy_write(phy, GMII_ERCR, value);
>>> - value = 0x2222;
>>> - phy_write(phy, GMII_ERDWR, value);
>>> + if (IS_BUILTIN(CONFIG_PHYLIB)) {
>>> + /* Set delay values */
>>> + value = GMII_RCCPSR | 0x8000;
>>> + phy_write(phy, GMII_ERCR, value);
>>> + value = 0xF2F4;
>>> + phy_write(phy, GMII_ERDWR, value);
>>> + value = GMII_RRDPSR | 0x8000;
>>> + phy_write(phy, GMII_ERCR, value);
>>> + value = 0x2222;
>>> + phy_write(phy, GMII_ERDWR, value);
>>> + }
>>>
>>> return 0;
>>> }
>
> This part of the patch is not actually needed if you
>
>>> static void __init sama5_dt_device_init(void)
>>> {
>>> - if (of_machine_is_compatible("atmel,sama5d3xcm"))
>>> + if (of_machine_is_compatible("atmel,sama5d3xcm") && IS_BUILTIN(CONFIG_PHYLIB))
>>> phy_register_fixup_for_uid(PHY_ID_KSZ9021, MICREL_PHY_ID_MASK,
>>> ksz9021rn_phy_fixup);
>
> do this change, or the inline helper.
... simply use this "IS_BUILTIN(CONFIG_PHYLIB)" here -----^^^^^^^^^ (and
not above as suggested by Arnd).
Jean-Christophe, Alexandre: do you agree with this conclusion?
I plan to stack this patch for next at91-fixes batch.
Bye,
--
Nicolas Ferre
On 04/06/2013 15:49, Nicolas Ferre wrote:
>
> ... simply use this "IS_BUILTIN(CONFIG_PHYLIB)" here -----^^^^^^^^^
> (and not above as suggested by Arnd).
>
> Jean-Christophe, Alexandre: do you agree with this conclusion?
>
> I plan to stack this patch for next at91-fixes batch.
>
Yeah, I prepared and tested that last week but didn't send it yet. I can
do it now if you want but you could probably just edit my patch. I'd
like to fix the other platforms too but that probably can wait.
I believe we would still have to fix the phy_register_fixup*() works as
it is always called from arch/ and is clearly not fool proofed enough.
Maybe DT could help ?
Arnd, do you have any input ?
--
Alexandre Belloni, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com
On Tuesday 04 June 2013 16:12:24 Alexandre Belloni wrote:
> On 04/06/2013 15:49, Nicolas Ferre wrote:
> >
> > ... simply use this "IS_BUILTIN(CONFIG_PHYLIB)" here -----^^^^^^^^^
> > (and not above as suggested by Arnd).
> >
> > Jean-Christophe, Alexandre: do you agree with this conclusion?
> >
> > I plan to stack this patch for next at91-fixes batch.
> >
>
> Yeah, I prepared and tested that last week but didn't send it yet. I can
> do it now if you want but you could probably just edit my patch. I'd
> like to fix the other platforms too but that probably can wait.
>
> I believe we would still have to fix the phy_register_fixup*() works as
> it is always called from arch/ and is clearly not fool proofed enough.
> Maybe DT could help ?
>
> Arnd, do you have any input ?
I've replied to the earlier thread now. Using if(IS_ENABLED(CONFIG_NET))
or if(IS_ENABLED(CONFIG_PHYLIB)) in platform code should be just fine and
is not a lof of extra effort.
As I explained in the other mail, I think IS_ENABLED() is better than
IS_BUILTIN() because the case where the network driver is a loadable
module would cause a silent run-time error with IS_BUILTIN, but a
link time error with IS_ENABLED, and we can prevent that error by
correctly expressing the dependency in Kconfig, e.g.
config THIS_BOARD
select PHYLIB if THIS_BOARDS_NIC
Arnd