2024-02-01 13:29:12

by Andrew Lunn

Subject: Re: [PATCH v5 net-next 08/13] net: phy: marvell-88q2xxx: add support for temperature sensor

> > +#ifdef CONFIG_HWMON
>
> HWMON is tristate, so this may be problematic if the driver is built
> into the kernel and hwmon is built as module.

There should be Kconfig in addition to this, e.g.

config MAXLINEAR_GPHY
tristate "Maxlinear Ethernet PHYs"
select POLYNOMIAL if HWMON
depends on HWMON || HWMON=n
help
Support for the Maxlinear GPY115, GPY211, GPY212, GPY215,
GPY241, GPY245 PHYs.

So its forced to being built in, or not built at all.

Andrew

2024-02-01 13:39:45

by Guenter Roeck

Subject: Re: [PATCH v5 net-next 08/13] net: phy: marvell-88q2xxx: add support for temperature sensor

On 2/1/24 05:27, Andrew Lunn wrote:
>>> +#ifdef CONFIG_HWMON
>>
>> HWMON is tristate, so this may be problematic if the driver is built
>> into the kernel and hwmon is built as module.
>
> There should be Kconfig in addition to this, e.g.
>
> config MAXLINEAR_GPHY
> tristate "Maxlinear Ethernet PHYs"
> select POLYNOMIAL if HWMON
> depends on HWMON || HWMON=n
> help
> Support for the Maxlinear GPY115, GPY211, GPY212, GPY215,
> GPY241, GPY245 PHYs.
>
> So its forced to being built in, or not built at all.
>

Even then it should be "#if IS_ENABLED(HWMON)" in the code.

Thanks,
Guenter

2024-02-01 16:27:02

by Dimitri Fedrau

Subject: Re: [PATCH v5 net-next 08/13] net: phy: marvell-88q2xxx: add support for temperature sensor

Am Thu, Feb 01, 2024 at 05:39:25AM -0800 schrieb Guenter Roeck:
> On 2/1/24 05:27, Andrew Lunn wrote:
> > > > +#ifdef CONFIG_HWMON
> > >
> > > HWMON is tristate, so this may be problematic if the driver is built
> > > into the kernel and hwmon is built as module.
> >
> > There should be Kconfig in addition to this, e.g.
> >
> > config MAXLINEAR_GPHY
> > tristate "Maxlinear Ethernet PHYs"
> > select POLYNOMIAL if HWMON
> > depends on HWMON || HWMON=n
> > help
> > Support for the Maxlinear GPY115, GPY211, GPY212, GPY215,
> > GPY241, GPY245 PHYs.
> >
> > So its forced to being built in, or not built at all.
> >
>
> Even then it should be "#if IS_ENABLED(HWMON)" in the code.
>
>
If using "#if IS_ENABLED(HWMON)" do I have to add the dependency in
the KConfig file ? When looking at other PHY drivers, they do.

Dimitri

2024-02-01 16:48:00

by Andrew Lunn

Subject: Re: [PATCH v5 net-next 08/13] net: phy: marvell-88q2xxx: add support for temperature sensor

On Thu, Feb 01, 2024 at 05:23:49PM +0100, Dimitri Fedrau wrote:
> Am Thu, Feb 01, 2024 at 05:39:25AM -0800 schrieb Guenter Roeck:
> > On 2/1/24 05:27, Andrew Lunn wrote:
> > > > > +#ifdef CONFIG_HWMON
> > > >
> > > > HWMON is tristate, so this may be problematic if the driver is built
> > > > into the kernel and hwmon is built as module.
> > >
> > > There should be Kconfig in addition to this, e.g.
> > >
> > > config MAXLINEAR_GPHY
> > > tristate "Maxlinear Ethernet PHYs"
> > > select POLYNOMIAL if HWMON
> > > depends on HWMON || HWMON=n
> > > help
> > > Support for the Maxlinear GPY115, GPY211, GPY212, GPY215,
> > > GPY241, GPY245 PHYs.
> > >
> > > So its forced to being built in, or not built at all.
> > >
> >
> > Even then it should be "#if IS_ENABLED(HWMON)" in the code.
> >
> >
> If using "#if IS_ENABLED(HWMON)" do I have to add the dependency in
> the KConfig file ? When looking at other PHY drivers, they do.

Please follow what other drivers do. Its easy to break the build,
resulting is undefined symbols. What we have now works.

Andrew

2024-02-01 16:59:41

by Dimitri Fedrau

Subject: Re: [PATCH v5 net-next 08/13] net: phy: marvell-88q2xxx: add support for temperature sensor

Am Thu, Feb 01, 2024 at 05:47:21PM +0100 schrieb Andrew Lunn:
> On Thu, Feb 01, 2024 at 05:23:49PM +0100, Dimitri Fedrau wrote:
> > Am Thu, Feb 01, 2024 at 05:39:25AM -0800 schrieb Guenter Roeck:
> > > On 2/1/24 05:27, Andrew Lunn wrote:
> > > > > > +#ifdef CONFIG_HWMON
> > > > >
> > > > > HWMON is tristate, so this may be problematic if the driver is built
> > > > > into the kernel and hwmon is built as module.
> > > >
> > > > There should be Kconfig in addition to this, e.g.
> > > >
> > > > config MAXLINEAR_GPHY
> > > > tristate "Maxlinear Ethernet PHYs"
> > > > select POLYNOMIAL if HWMON
> > > > depends on HWMON || HWMON=n
> > > > help
> > > > Support for the Maxlinear GPY115, GPY211, GPY212, GPY215,
> > > > GPY241, GPY245 PHYs.
> > > >
> > > > So its forced to being built in, or not built at all.
> > > >
> > >
> > > Even then it should be "#if IS_ENABLED(HWMON)" in the code.
> > >
> > >
> > If using "#if IS_ENABLED(HWMON)" do I have to add the dependency in
> > the KConfig file ? When looking at other PHY drivers, they do.
>
> Please follow what other drivers do. Its easy to break the build,
> resulting is undefined symbols. What we have now works.

Sure.

2024-02-01 18:23:10

by Guenter Roeck

Subject: Re: [PATCH v5 net-next 08/13] net: phy: marvell-88q2xxx: add support for temperature sensor

On Thu, Feb 01, 2024 at 05:23:49PM +0100, Dimitri Fedrau wrote:
> Am Thu, Feb 01, 2024 at 05:39:25AM -0800 schrieb Guenter Roeck:
> > On 2/1/24 05:27, Andrew Lunn wrote:
> > > > > +#ifdef CONFIG_HWMON
> > > >
> > > > HWMON is tristate, so this may be problematic if the driver is built
> > > > into the kernel and hwmon is built as module.
> > >
> > > There should be Kconfig in addition to this, e.g.
> > >
> > > config MAXLINEAR_GPHY
> > > tristate "Maxlinear Ethernet PHYs"
> > > select POLYNOMIAL if HWMON
> > > depends on HWMON || HWMON=n
> > > help
> > > Support for the Maxlinear GPY115, GPY211, GPY212, GPY215,
> > > GPY241, GPY245 PHYs.
> > >
> > > So its forced to being built in, or not built at all.
> > >
> >
> > Even then it should be "#if IS_ENABLED(HWMON)" in the code.
> >
> >
> If using "#if IS_ENABLED(HWMON)" do I have to add the dependency in
> the KConfig file ? When looking at other PHY drivers, they do.

Yes, to handle CONFIG_HWMON=m. Note that it is "IS_ENABLED(CONFIG_HWMON)"
^^^^^^^

Guenter