Hi Jerome, On Thu, 29 Aug 2024 at 10:40, Jerome Forissier <[email protected]> wrote: > > > > On 8/29/24 16:04, Simon Glass wrote: > > Hi Jerome, > > > > On Wed, 28 Aug 2024 at 06:11, Jerome Forissier > > <[email protected]> wrote: > >> > >> dtsec_init_phy() is defined only with MII so add the proper conditional > >> in the caller code. > >> > >> Signed-off-by: Jerome Forissier <[email protected]> > >> --- > >> 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
