Re: [PATCH net-next v3 1/2] ethtool: provide UAPI for PHY Signal Quality Index (SQI)
On Wed, May 20, 2020 at 05:30:01PM +0200, Andrew Lunn wrote: > > > I'm not sure if it's a good idea to define two separate callbacks. It > > > means adding two pointers instead of one (for every instance of the > > > structure, not only those implementing them), doing two calls, running > > > the same checks twice, locking twice, checking the result twice. > > > > > > Also, passing a structure pointer would mean less code changed if we > > > decide to add more related state values later. > > > > > > What do you think? > > > > > > If you don't agree, I have no objections so > > > > > > Reviewed-by: Michal Kubecek > > > > I have no strong opinion on it. Should I rework it? > > It is an internal API, so we can change it any time we want. > > I did wonder if MAX should just be a static value. It seems odd it > would change at run time. But we can re-evaulate this once we got some > more users. OK, then let's keep it for now. -- Pengutronix e.K. | | Steuerwalder Str. 21 | http://www.pengutronix.de/ | 31137 Hildesheim, Germany | Phone: +49-5121-206917-0| Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917- | signature.asc Description: PGP signature
Re: [PATCH net-next v3 1/2] ethtool: provide UAPI for PHY Signal Quality Index (SQI)
> > I'm not sure if it's a good idea to define two separate callbacks. It > > means adding two pointers instead of one (for every instance of the > > structure, not only those implementing them), doing two calls, running > > the same checks twice, locking twice, checking the result twice. > > > > Also, passing a structure pointer would mean less code changed if we > > decide to add more related state values later. > > > > What do you think? > > > > If you don't agree, I have no objections so > > > > Reviewed-by: Michal Kubecek > > I have no strong opinion on it. Should I rework it? It is an internal API, so we can change it any time we want. I did wonder if MAX should just be a static value. It seems odd it would change at run time. But we can re-evaulate this once we got some more users. Andrew
Re: [PATCH net-next v3 1/2] ethtool: provide UAPI for PHY Signal Quality Index (SQI)
On Wed, May 20, 2020 at 05:07:11PM +0200, Oleksij Rempel wrote:
> On Wed, May 20, 2020 at 04:45:44PM +0200, Michal Kubecek wrote:
> > On Wed, May 20, 2020 at 08:29:14AM +0200, Oleksij Rempel wrote:
> > > Signal Quality Index is a mandatory value required by "OPEN Alliance
> > > SIG" for the 100Base-T1 PHYs [1]. This indicator can be used for cable
> > > integrity diagnostic and investigating other noise sources and
> > > implement by at least two vendors: NXP[2] and TI[3].
> > >
> > > [1]
> > > http://www.opensig.org/download/document/218/Advanced_PHY_features_for_automotive_Ethernet_V1.0.pdf
> > > [2] https://www.nxp.com/docs/en/data-sheet/TJA1100.pdf
> > > [3] https://www.ti.com/product/DP83TC811R-Q1
> > >
> > > Signed-off-by: Oleksij Rempel
> > > ---
> >
> > This looks good to me, there is just one thing I'm not sure about:
> >
> > > diff --git a/include/linux/phy.h b/include/linux/phy.h
> > > index 59344db43fcb1..950ba479754bd 100644
> > > --- a/include/linux/phy.h
> > > +++ b/include/linux/phy.h
> > > @@ -706,6 +706,8 @@ struct phy_driver {
> > > struct ethtool_tunable *tuna,
> > > const void *data);
> > > int (*set_loopback)(struct phy_device *dev, bool enable);
> > > + int (*get_sqi)(struct phy_device *dev);
> > > + int (*get_sqi_max)(struct phy_device *dev);
> > > };
> > > #define to_phy_driver(d) container_of(to_mdio_common_driver(d),
> > > \
> > > struct phy_driver, mdiodrv)
> >
> > I'm not sure if it's a good idea to define two separate callbacks. It
> > means adding two pointers instead of one (for every instance of the
> > structure, not only those implementing them), doing two calls, running
> > the same checks twice, locking twice, checking the result twice.
> >
> > Also, passing a structure pointer would mean less code changed if we
> > decide to add more related state values later.
> >
> > What do you think?
> >
> > If you don't agree, I have no objections so
> >
> > Reviewed-by: Michal Kubecek
>
> I have no strong opinion on it. Should I rework it?
It's up to you. It was a suggestion for possible improvement but I have
no problem with this version being applied.
Michal
Re: [PATCH net-next v3 1/2] ethtool: provide UAPI for PHY Signal Quality Index (SQI)
On Wed, May 20, 2020 at 04:45:44PM +0200, Michal Kubecek wrote:
> On Wed, May 20, 2020 at 08:29:14AM +0200, Oleksij Rempel wrote:
> > Signal Quality Index is a mandatory value required by "OPEN Alliance
> > SIG" for the 100Base-T1 PHYs [1]. This indicator can be used for cable
> > integrity diagnostic and investigating other noise sources and
> > implement by at least two vendors: NXP[2] and TI[3].
> >
> > [1]
> > http://www.opensig.org/download/document/218/Advanced_PHY_features_for_automotive_Ethernet_V1.0.pdf
> > [2] https://www.nxp.com/docs/en/data-sheet/TJA1100.pdf
> > [3] https://www.ti.com/product/DP83TC811R-Q1
> >
> > Signed-off-by: Oleksij Rempel
> > ---
>
> This looks good to me, there is just one thing I'm not sure about:
>
> > diff --git a/include/linux/phy.h b/include/linux/phy.h
> > index 59344db43fcb1..950ba479754bd 100644
> > --- a/include/linux/phy.h
> > +++ b/include/linux/phy.h
> > @@ -706,6 +706,8 @@ struct phy_driver {
> > struct ethtool_tunable *tuna,
> > const void *data);
> > int (*set_loopback)(struct phy_device *dev, bool enable);
> > + int (*get_sqi)(struct phy_device *dev);
> > + int (*get_sqi_max)(struct phy_device *dev);
> > };
> > #define to_phy_driver(d) container_of(to_mdio_common_driver(d),
> > \
> > struct phy_driver, mdiodrv)
>
> I'm not sure if it's a good idea to define two separate callbacks. It
> means adding two pointers instead of one (for every instance of the
> structure, not only those implementing them), doing two calls, running
> the same checks twice, locking twice, checking the result twice.
>
> Also, passing a structure pointer would mean less code changed if we
> decide to add more related state values later.
>
> What do you think?
>
> If you don't agree, I have no objections so
>
> Reviewed-by: Michal Kubecek
I have no strong opinion on it. Should I rework it?
--
Pengutronix e.K. | |
Steuerwalder Str. 21 | http://www.pengutronix.de/ |
31137 Hildesheim, Germany | Phone: +49-5121-206917-0|
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917- |
signature.asc
Description: PGP signature
Re: [PATCH net-next v3 1/2] ethtool: provide UAPI for PHY Signal Quality Index (SQI)
On Wed, May 20, 2020 at 08:29:14AM +0200, Oleksij Rempel wrote:
> Signal Quality Index is a mandatory value required by "OPEN Alliance
> SIG" for the 100Base-T1 PHYs [1]. This indicator can be used for cable
> integrity diagnostic and investigating other noise sources and
> implement by at least two vendors: NXP[2] and TI[3].
>
> [1]
> http://www.opensig.org/download/document/218/Advanced_PHY_features_for_automotive_Ethernet_V1.0.pdf
> [2] https://www.nxp.com/docs/en/data-sheet/TJA1100.pdf
> [3] https://www.ti.com/product/DP83TC811R-Q1
>
> Signed-off-by: Oleksij Rempel
> ---
This looks good to me, there is just one thing I'm not sure about:
> diff --git a/include/linux/phy.h b/include/linux/phy.h
> index 59344db43fcb1..950ba479754bd 100644
> --- a/include/linux/phy.h
> +++ b/include/linux/phy.h
> @@ -706,6 +706,8 @@ struct phy_driver {
> struct ethtool_tunable *tuna,
> const void *data);
> int (*set_loopback)(struct phy_device *dev, bool enable);
> + int (*get_sqi)(struct phy_device *dev);
> + int (*get_sqi_max)(struct phy_device *dev);
> };
> #define to_phy_driver(d) container_of(to_mdio_common_driver(d),
> \
> struct phy_driver, mdiodrv)
I'm not sure if it's a good idea to define two separate callbacks. It
means adding two pointers instead of one (for every instance of the
structure, not only those implementing them), doing two calls, running
the same checks twice, locking twice, checking the result twice.
Also, passing a structure pointer would mean less code changed if we
decide to add more related state values later.
What do you think?
If you don't agree, I have no objections so
Reviewed-by: Michal Kubecek
Michal
signature.asc
Description: Digital signature
Re: [PATCH net-next v3 1/2] ethtool: provide UAPI for PHY Signal Quality Index (SQI)
On Wed, May 20, 2020 at 08:29:14AM +0200, Oleksij Rempel wrote: > Signal Quality Index is a mandatory value required by "OPEN Alliance > SIG" for the 100Base-T1 PHYs [1]. This indicator can be used for cable > integrity diagnostic and investigating other noise sources and > implement by at least two vendors: NXP[2] and TI[3]. > > [1] > http://www.opensig.org/download/document/218/Advanced_PHY_features_for_automotive_Ethernet_V1.0.pdf > [2] https://www.nxp.com/docs/en/data-sheet/TJA1100.pdf > [3] https://www.ti.com/product/DP83TC811R-Q1 > > Signed-off-by: Oleksij Rempel Reviewed-by: Andrew Lunn Andrew
