diff mbox series

[05/13] net: fm: call dtsec_init_phy() only when it is defined

Message ID 679e20bfd9ac3de6560e202f82452563b01fca7a.1724846454.git.jerome.forissier@linaro.org
State New
Headers show
Series Miscellaneous fixes | expand

Commit Message

Jérôme Forissier Aug. 28, 2024, 12:10 p.m. UTC 
dtsec_init_phy() is defined only with MII so add the proper conditional
in the caller code.

Signed-off-by: Jerome Forissier <jerome.forissier@linaro.org>
---
 drivers/net/fm/eth.c | 2 ++
 1 file changed, 2 insertions(+)

Comments

Simon Glass Aug. 29, 2024, 2:04 p.m. UTC | #1 
Hi Jerome,

On Wed, 28 Aug 2024 at 06:11, Jerome Forissier
<jerome.forissier@linaro.org> wrote:
>
> dtsec_init_phy() is defined only with MII so add the proper conditional
> in the caller code.
>
> Signed-off-by: Jerome Forissier <jerome.forissier@linaro.org>
> ---
>  drivers/net/fm/eth.c | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/drivers/net/fm/eth.c b/drivers/net/fm/eth.c
> index 19f3f0fef0..e4ec769693 100644
> --- a/drivers/net/fm/eth.c
> +++ b/drivers/net/fm/eth.c
> @@ -701,8 +701,10 @@ static int init_phy(struct fm_eth *fm_eth)
>                 supported |= SUPPORTED_2500baseX_Full;
>  #endif
>
> +#if defined(CONFIG_MII) || defined(CONFIG_CMD_MII) && !defined(BITBANGMII)
>         if (fm_eth->type == FM_ETH_1G_E)
>                 dtsec_init_phy(fm_eth);
> +#endif

We really want to avoid #ifdefs in the code as they create different
build paths. Can you use if (IS_ENABLED) ? Also BITBANGMII seems liike
it needs a CONFIG_ prefix.

>
>  #ifdef CONFIG_PHYLIB
>  #ifdef CONFIG_DM_MDIO
> --
> 2.40.1
>

Regards,
Simon
Jérôme Forissier Aug. 29, 2024, 4:40 p.m. UTC | #2 
On 8/29/24 16:04, Simon Glass wrote:
> Hi Jerome,
> 
> On Wed, 28 Aug 2024 at 06:11, Jerome Forissier
> <jerome.forissier@linaro.org> wrote:
>>
>> dtsec_init_phy() is defined only with MII so add the proper conditional
>> in the caller code.
>>
>> Signed-off-by: Jerome Forissier <jerome.forissier@linaro.org>
>> ---
>>  drivers/net/fm/eth.c | 2 ++
>>  1 file changed, 2 insertions(+)
>>
>> diff --git a/drivers/net/fm/eth.c b/drivers/net/fm/eth.c
>> index 19f3f0fef0..e4ec769693 100644
>> --- a/drivers/net/fm/eth.c
>> +++ b/drivers/net/fm/eth.c
>> @@ -701,8 +701,10 @@ static int init_phy(struct fm_eth *fm_eth)
>>                 supported |= SUPPORTED_2500baseX_Full;
>>  #endif
>>
>> +#if defined(CONFIG_MII) || defined(CONFIG_CMD_MII) && !defined(BITBANGMII)
>>         if (fm_eth->type == FM_ETH_1G_E)
>>                 dtsec_init_phy(fm_eth);
>> +#endif
> 
> We really want to avoid #ifdefs in the code as they create different
> build paths. Can you use if (IS_ENABLED) ?

Sure.

> Also BITBANGMII seems liike
> it needs a CONFIG_ prefix.

I agree. I have copied the line verbatim from line 29 because the condition
needs to be the same. And I suspect there is another issue with the operator
precedence. Do we want "MII || (CMD_MII && !BITBANGMII)" or
do we rather want "(MII || CMD_MII) && !BITBANGMII"? I guess the latter.

So how about:

diff --git a/drivers/net/fm/eth.c b/drivers/net/fm/eth.c
index 19f3f0fef0..22025b6a27 100644
--- a/drivers/net/fm/eth.c
+++ b/drivers/net/fm/eth.c
@@ -26,7 +26,8 @@
 
 #include "fm.h"
 
-#if defined(CONFIG_MII) || defined(CONFIG_CMD_MII) && !defined(BITBANGMII)
+#if ((defined(CONFIG_MII) || defined(CONFIG_CMD_MII)) && \
+     !defined(CONFIG_BITBANGMII))
 
 #define TBIANA_SETTINGS (TBIANA_ASYMMETRIC_PAUSE | TBIANA_SYMMETRIC_PAUSE | \
 			 TBIANA_FULL_DUPLEX)
@@ -701,8 +702,11 @@ static int init_phy(struct fm_eth *fm_eth)
 		supported |= SUPPORTED_2500baseX_Full;
 #endif
 
-	if (fm_eth->type == FM_ETH_1G_E)
-		dtsec_init_phy(fm_eth);
+	if ((IS_ENABLED(CONFIG_MII) || IS_ENABLED(CONFIG_CMD_MII)) &&
+	    !IS_ENABLED(CONFIG_BITBANGMII)) {
+		if (fm_eth->type == FM_ETH_1G_E)
+			dtsec_init_phy(fm_eth);
+	}
 
 #ifdef CONFIG_PHYLIB
 #ifdef CONFIG_DM_MDIO


???

Thanks,
Simon Glass Aug. 30, 2024, 12:58 a.m. UTC | #3 
Hi Jerome,

On Thu, 29 Aug 2024 at 10:40, Jerome Forissier
<jerome.forissier@linaro.org> wrote:
>
>
>
> On 8/29/24 16:04, Simon Glass wrote:
> > Hi Jerome,
> >
> > On Wed, 28 Aug 2024 at 06:11, Jerome Forissier
> > <jerome.forissier@linaro.org> wrote:
> >>
> >> dtsec_init_phy() is defined only with MII so add the proper conditional
> >> in the caller code.
> >>
> >> Signed-off-by: Jerome Forissier <jerome.forissier@linaro.org>
> >> ---
> >>  drivers/net/fm/eth.c | 2 ++
> >>  1 file changed, 2 insertions(+)
> >>
> >> diff --git a/drivers/net/fm/eth.c b/drivers/net/fm/eth.c
> >> index 19f3f0fef0..e4ec769693 100644
> >> --- a/drivers/net/fm/eth.c
> >> +++ b/drivers/net/fm/eth.c
> >> @@ -701,8 +701,10 @@ static int init_phy(struct fm_eth *fm_eth)
> >>                 supported |= SUPPORTED_2500baseX_Full;
> >>  #endif
> >>
> >> +#if defined(CONFIG_MII) || defined(CONFIG_CMD_MII) && !defined(BITBANGMII)
> >>         if (fm_eth->type == FM_ETH_1G_E)
> >>                 dtsec_init_phy(fm_eth);
> >> +#endif
> >
> > We really want to avoid #ifdefs in the code as they create different
> > build paths. Can you use if (IS_ENABLED) ?
>
> Sure.
>
> > Also BITBANGMII seems liike
> > it needs a CONFIG_ prefix.
>
> I agree. I have copied the line verbatim from line 29 because the condition
> needs to be the same. And I suspect there is another issue with the operator
> precedence. Do we want "MII || (CMD_MII && !BITBANGMII)" or
> do we rather want "(MII || CMD_MII) && !BITBANGMII"? I guess the latter.
>
> So how about:
>
> diff --git a/drivers/net/fm/eth.c b/drivers/net/fm/eth.c
> index 19f3f0fef0..22025b6a27 100644
> --- a/drivers/net/fm/eth.c
> +++ b/drivers/net/fm/eth.c
> @@ -26,7 +26,8 @@
>
>  #include "fm.h"
>
> -#if defined(CONFIG_MII) || defined(CONFIG_CMD_MII) && !defined(BITBANGMII)
> +#if ((defined(CONFIG_MII) || defined(CONFIG_CMD_MII)) && \
> +     !defined(CONFIG_BITBANGMII))
>
>  #define TBIANA_SETTINGS (TBIANA_ASYMMETRIC_PAUSE | TBIANA_SYMMETRIC_PAUSE | \
>                          TBIANA_FULL_DUPLEX)
> @@ -701,8 +702,11 @@ static int init_phy(struct fm_eth *fm_eth)
>                 supported |= SUPPORTED_2500baseX_Full;
>  #endif
>
> -       if (fm_eth->type == FM_ETH_1G_E)
> -               dtsec_init_phy(fm_eth);
> +       if ((IS_ENABLED(CONFIG_MII) || IS_ENABLED(CONFIG_CMD_MII)) &&
> +           !IS_ENABLED(CONFIG_BITBANGMII)) {
> +               if (fm_eth->type == FM_ETH_1G_E)
> +                       dtsec_init_phy(fm_eth);
> +       }
>

That's good

>  #ifdef CONFIG_PHYLIB
>  #ifdef CONFIG_DM_MDIO
>
>
> ???

You don't have to fix every #ifdef in every file you touch...it's just
that if you are already touching code, you might as well bring it up
to newer standards.

Regards,
Simon
diff mbox series

Patch

diff --git a/drivers/net/fm/eth.c b/drivers/net/fm/eth.c
index 19f3f0fef0..e4ec769693 100644
--- a/drivers/net/fm/eth.c
+++ b/drivers/net/fm/eth.c
@@ -701,8 +701,10 @@  static int init_phy(struct fm_eth *fm_eth)
 		supported |= SUPPORTED_2500baseX_Full;
 #endif
 
+#if defined(CONFIG_MII) || defined(CONFIG_CMD_MII) && !defined(BITBANGMII)
 	if (fm_eth->type == FM_ETH_1G_E)
 		dtsec_init_phy(fm_eth);
+#endif
 
 #ifdef CONFIG_PHYLIB
 #ifdef CONFIG_DM_MDIO