| Message ID | 20210218161451.3489955-3-steen.hegelund@microchip.com (mailing list archive) |
|---|---|
| State | Deferred |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | Adding the Sparx5 Serdes driver | expand |
| Context | Check | Description |
|---|---|---|
| netdev/cover_letter | success | Link |
| netdev/fixes_present | success | Link |
| netdev/patch_count | success | Link |
| netdev/tree_selection | success | Guessed tree name to be net-next |
| netdev/subject_prefix | success | Link |
| netdev/cc_maintainers | success | CCed 2 of 2 maintainers |
| netdev/source_inline | success | Was 0 now: 0 |
| netdev/verify_signedoff | success | Link |
| netdev/module_param | success | Was 0 now: 0 |
| netdev/build_32bit | success | Errors and warnings before: 195 this patch: 195 |
| netdev/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| netdev/verify_fixes | success | Link |
| netdev/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 92 lines checked |
| netdev/build_allmodconfig_warn | success | Errors and warnings before: 195 this patch: 195 |
| netdev/header_inline | success | Link |
| netdev/stable | success | Stable not CCed |
On Thu, Feb 18, 2021 at 05:14:49PM +0100, Steen Hegelund wrote: > Provide new phy configuration interfaces for media type and speed that > allows e.g. PHYs used for ethernet to be configured with this > information. > > Signed-off-by: Lars Povlsen <lars.povlsen@microchip.com> > Signed-off-by: Steen Hegelund <steen.hegelund@microchip.com> > Reviewed-by: Andrew Lunn <andrew@lunn.ch> > Reviewed-by: Alexandre Belloni <alexandre.belloni@bootlin.com> > --- > drivers/phy/phy-core.c | 30 ++++++++++++++++++++++++++++++ > include/linux/phy/phy.h | 26 ++++++++++++++++++++++++++ > 2 files changed, 56 insertions(+) > > diff --git a/drivers/phy/phy-core.c b/drivers/phy/phy-core.c > index 71cb10826326..ccb575b13777 100644 > --- a/drivers/phy/phy-core.c > +++ b/drivers/phy/phy-core.c > @@ -373,6 +373,36 @@ int phy_set_mode_ext(struct phy *phy, enum phy_mode mode, int submode) > } > EXPORT_SYMBOL_GPL(phy_set_mode_ext); > > +int phy_set_media(struct phy *phy, enum phy_media media) > +{ > + int ret; > + > + if (!phy || !phy->ops->set_media) > + return 0; > + > + mutex_lock(&phy->mutex); > + ret = phy->ops->set_media(phy, media); > + mutex_unlock(&phy->mutex); > + > + return ret; > +} > +EXPORT_SYMBOL_GPL(phy_set_media); > + > +int phy_set_speed(struct phy *phy, int speed) > +{ > + int ret; > + > + if (!phy || !phy->ops->set_speed) > + return 0; > + > + mutex_lock(&phy->mutex); > + ret = phy->ops->set_speed(phy, speed); > + mutex_unlock(&phy->mutex); > + > + return ret; > +} > +EXPORT_SYMBOL_GPL(phy_set_speed); > + > int phy_reset(struct phy *phy) > { > int ret; > diff --git a/include/linux/phy/phy.h b/include/linux/phy/phy.h > index e435bdb0bab3..0ed434d02196 100644 > --- a/include/linux/phy/phy.h > +++ b/include/linux/phy/phy.h > @@ -44,6 +44,12 @@ enum phy_mode { > PHY_MODE_DP > }; > > +enum phy_media { > + PHY_MEDIA_DEFAULT, > + PHY_MEDIA_SR, > + PHY_MEDIA_DAC, > +}; > + > /** > * union phy_configure_opts - Opaque generic phy configuration > * > @@ -64,6 +70,8 @@ union phy_configure_opts { > * @power_on: powering on the phy > * @power_off: powering off the phy > * @set_mode: set the mode of the phy > + * @set_media: set the media type of the phy (optional) > + * @set_speed: set the speed of the phy (optional) > * @reset: resetting the phy > * @calibrate: calibrate the phy > * @release: ops to be performed while the consumer relinquishes the PHY > @@ -75,6 +83,8 @@ struct phy_ops { > int (*power_on)(struct phy *phy); > int (*power_off)(struct phy *phy); > int (*set_mode)(struct phy *phy, enum phy_mode mode, int submode); > + int (*set_media)(struct phy *phy, enum phy_media media); > + int (*set_speed)(struct phy *phy, int speed); > > /** > * @configure: > @@ -215,6 +225,8 @@ int phy_power_off(struct phy *phy); > int phy_set_mode_ext(struct phy *phy, enum phy_mode mode, int submode); > #define phy_set_mode(phy, mode) \ > phy_set_mode_ext(phy, mode, 0) > +int phy_set_media(struct phy *phy, enum phy_media media); > +int phy_set_speed(struct phy *phy, int speed); > int phy_configure(struct phy *phy, union phy_configure_opts *opts); > int phy_validate(struct phy *phy, enum phy_mode mode, int submode, > union phy_configure_opts *opts); > @@ -344,6 +356,20 @@ static inline int phy_set_mode_ext(struct phy *phy, enum phy_mode mode, > #define phy_set_mode(phy, mode) \ > phy_set_mode_ext(phy, mode, 0) > > +static inline int phy_set_media(struct phy *phy, enum phy_media media) > +{ > + if (!phy) > + return 0; I'm curious, why do you check for the NULL in all newly introduced functions? How is it possible that calls to phy_*() supply NULL as the main struct? Thanks > + return -ENODEV; > +} > + > +static inline int phy_set_speed(struct phy *phy, int speed) > +{ > + if (!phy) > + return 0; > + return -ENODEV; > +} > + > static inline enum phy_mode phy_get_mode(struct phy *phy) > { > return PHY_MODE_INVALID; > -- > 2.30.0 >
Hi Leon, On Sun, 2021-02-21 at 07:59 +0200, Leon Romanovsky wrote: > EXTERNAL EMAIL: Do not click links or open attachments unless you > know the content is safe > > On Thu, Feb 18, 2021 at 05:14:49PM +0100, Steen Hegelund wrote: > > Provide new phy configuration interfaces for media type and speed > > that > > allows e.g. PHYs used for ethernet to be configured with this > > information. > > > > Signed-off-by: Lars Povlsen <lars.povlsen@microchip.com> > > Signed-off-by: Steen Hegelund <steen.hegelund@microchip.com> > > Reviewed-by: Andrew Lunn <andrew@lunn.ch> > > Reviewed-by: Alexandre Belloni <alexandre.belloni@bootlin.com> > > --- > > ... > > int phy_validate(struct phy *phy, enum phy_mode mode, int submode, > > union phy_configure_opts *opts); > > @@ -344,6 +356,20 @@ static inline int phy_set_mode_ext(struct phy > > *phy, enum phy_mode mode, > > #define phy_set_mode(phy, mode) \ > > phy_set_mode_ext(phy, mode, 0) > > > > +static inline int phy_set_media(struct phy *phy, enum phy_media > > media) > > +{ > > + if (!phy) > > + return 0; > > I'm curious, why do you check for the NULL in all newly introduced > functions? > How is it possible that calls to phy_*() supply NULL as the main > struct? > > Thanks I do not know the history of that, but all the functions in the interface that takes a phy as input and returns a status follow that pattern. Maybe Kishon and Vinod knows the origin? > > > + return -ENODEV; > > +} > > + > > +static inline int phy_set_speed(struct phy *phy, int speed) > > +{ > > + if (!phy) > > + return 0; > > + return -ENODEV; > > +} > > + > > static inline enum phy_mode phy_get_mode(struct phy *phy) > > { > > return PHY_MODE_INVALID; > > -- > > 2.30.0 > > Best Regards Steen
Hi Leon, On 22/02/21 1:30 pm, Steen Hegelund wrote: > Hi Leon, > > On Sun, 2021-02-21 at 07:59 +0200, Leon Romanovsky wrote: >> EXTERNAL EMAIL: Do not click links or open attachments unless you >> know the content is safe >> >> On Thu, Feb 18, 2021 at 05:14:49PM +0100, Steen Hegelund wrote: >>> Provide new phy configuration interfaces for media type and speed >>> that >>> allows e.g. PHYs used for ethernet to be configured with this >>> information. >>> >>> Signed-off-by: Lars Povlsen <lars.povlsen@microchip.com> >>> Signed-off-by: Steen Hegelund <steen.hegelund@microchip.com> >>> Reviewed-by: Andrew Lunn <andrew@lunn.ch> >>> Reviewed-by: Alexandre Belloni <alexandre.belloni@bootlin.com> >>> --- >>> > > ... > >>> int phy_validate(struct phy *phy, enum phy_mode mode, int submode, >>> union phy_configure_opts *opts); >>> @@ -344,6 +356,20 @@ static inline int phy_set_mode_ext(struct phy >>> *phy, enum phy_mode mode, >>> #define phy_set_mode(phy, mode) \ >>> phy_set_mode_ext(phy, mode, 0) >>> >>> +static inline int phy_set_media(struct phy *phy, enum phy_media >>> media) >>> +{ >>> + if (!phy) >>> + return 0; >> >> I'm curious, why do you check for the NULL in all newly introduced >> functions? >> How is it possible that calls to phy_*() supply NULL as the main >> struct? >> >> Thanks > > I do not know the history of that, but all the functions in the > interface that takes a phy as input and returns a status follow that > pattern. Maybe Kishon and Vinod knows the origin? It is to make handling optional PHYs simpler. See here for the origin :-) http://lore.kernel.org/r/1391264157-2112-1-git-send-email-andrew@lunn.ch Thanks Kishon > >> >>> + return -ENODEV; >>> +} >>> + >>> +static inline int phy_set_speed(struct phy *phy, int speed) >>> +{ >>> + if (!phy) >>> + return 0; >>> + return -ENODEV; >>> +} >>> + >>> static inline enum phy_mode phy_get_mode(struct phy *phy) >>> { >>> return PHY_MODE_INVALID; >>> -- >>> 2.30.0 >>> > > Best Regards > Steen >
On Tue, Feb 23, 2021 at 05:52:14PM +0530, Kishon Vijay Abraham I wrote: > Hi Leon, > > On 22/02/21 1:30 pm, Steen Hegelund wrote: > > Hi Leon, > > > > On Sun, 2021-02-21 at 07:59 +0200, Leon Romanovsky wrote: > >> EXTERNAL EMAIL: Do not click links or open attachments unless you > >> know the content is safe > >> > >> On Thu, Feb 18, 2021 at 05:14:49PM +0100, Steen Hegelund wrote: > >>> Provide new phy configuration interfaces for media type and speed > >>> that > >>> allows e.g. PHYs used for ethernet to be configured with this > >>> information. > >>> > >>> Signed-off-by: Lars Povlsen <lars.povlsen@microchip.com> > >>> Signed-off-by: Steen Hegelund <steen.hegelund@microchip.com> > >>> Reviewed-by: Andrew Lunn <andrew@lunn.ch> > >>> Reviewed-by: Alexandre Belloni <alexandre.belloni@bootlin.com> > >>> --- > >>> > > > > ... > > > >>> int phy_validate(struct phy *phy, enum phy_mode mode, int submode, > >>> union phy_configure_opts *opts); > >>> @@ -344,6 +356,20 @@ static inline int phy_set_mode_ext(struct phy > >>> *phy, enum phy_mode mode, > >>> #define phy_set_mode(phy, mode) \ > >>> phy_set_mode_ext(phy, mode, 0) > >>> > >>> +static inline int phy_set_media(struct phy *phy, enum phy_media > >>> media) > >>> +{ > >>> + if (!phy) > >>> + return 0; > >> > >> I'm curious, why do you check for the NULL in all newly introduced > >> functions? > >> How is it possible that calls to phy_*() supply NULL as the main > >> struct? > >> > >> Thanks > > > > I do not know the history of that, but all the functions in the > > interface that takes a phy as input and returns a status follow that > > pattern. Maybe Kishon and Vinod knows the origin? > > It is to make handling optional PHYs simpler. See here for the origin :-) > http://lore.kernel.org/r/1391264157-2112-1-git-send-email-andrew@lunn.ch Thanks for the pointer, it is good to know. I personally would do it differently, but whatever. > > Thanks > Kishon > > > >> > >>> + return -ENODEV; > >>> +} > >>> + > >>> +static inline int phy_set_speed(struct phy *phy, int speed) > >>> +{ > >>> + if (!phy) > >>> + return 0; > >>> + return -ENODEV; > >>> +} > >>> + > >>> static inline enum phy_mode phy_get_mode(struct phy *phy) > >>> { > >>> return PHY_MODE_INVALID; > >>> -- > >>> 2.30.0 > >>> > > > > Best Regards > > Steen > >
On 18/02/21 9:44 pm, Steen Hegelund wrote: > Provide new phy configuration interfaces for media type and speed that > allows e.g. PHYs used for ethernet to be configured with this > information. > > Signed-off-by: Lars Povlsen <lars.povlsen@microchip.com> > Signed-off-by: Steen Hegelund <steen.hegelund@microchip.com> > Reviewed-by: Andrew Lunn <andrew@lunn.ch> > Reviewed-by: Alexandre Belloni <alexandre.belloni@bootlin.com> Acked-By: Kishon Vijay Abraham I <kishon@ti.com> > --- > drivers/phy/phy-core.c | 30 ++++++++++++++++++++++++++++++ > include/linux/phy/phy.h | 26 ++++++++++++++++++++++++++ > 2 files changed, 56 insertions(+) > > diff --git a/drivers/phy/phy-core.c b/drivers/phy/phy-core.c > index 71cb10826326..ccb575b13777 100644 > --- a/drivers/phy/phy-core.c > +++ b/drivers/phy/phy-core.c > @@ -373,6 +373,36 @@ int phy_set_mode_ext(struct phy *phy, enum phy_mode mode, int submode) > } > EXPORT_SYMBOL_GPL(phy_set_mode_ext); > > +int phy_set_media(struct phy *phy, enum phy_media media) > +{ > + int ret; > + > + if (!phy || !phy->ops->set_media) > + return 0; > + > + mutex_lock(&phy->mutex); > + ret = phy->ops->set_media(phy, media); > + mutex_unlock(&phy->mutex); > + > + return ret; > +} > +EXPORT_SYMBOL_GPL(phy_set_media); > + > +int phy_set_speed(struct phy *phy, int speed) > +{ > + int ret; > + > + if (!phy || !phy->ops->set_speed) > + return 0; > + > + mutex_lock(&phy->mutex); > + ret = phy->ops->set_speed(phy, speed); > + mutex_unlock(&phy->mutex); > + > + return ret; > +} > +EXPORT_SYMBOL_GPL(phy_set_speed); > + > int phy_reset(struct phy *phy) > { > int ret; > diff --git a/include/linux/phy/phy.h b/include/linux/phy/phy.h > index e435bdb0bab3..0ed434d02196 100644 > --- a/include/linux/phy/phy.h > +++ b/include/linux/phy/phy.h > @@ -44,6 +44,12 @@ enum phy_mode { > PHY_MODE_DP > }; > > +enum phy_media { > + PHY_MEDIA_DEFAULT, > + PHY_MEDIA_SR, > + PHY_MEDIA_DAC, > +}; > + > /** > * union phy_configure_opts - Opaque generic phy configuration > * > @@ -64,6 +70,8 @@ union phy_configure_opts { > * @power_on: powering on the phy > * @power_off: powering off the phy > * @set_mode: set the mode of the phy > + * @set_media: set the media type of the phy (optional) > + * @set_speed: set the speed of the phy (optional) > * @reset: resetting the phy > * @calibrate: calibrate the phy > * @release: ops to be performed while the consumer relinquishes the PHY > @@ -75,6 +83,8 @@ struct phy_ops { > int (*power_on)(struct phy *phy); > int (*power_off)(struct phy *phy); > int (*set_mode)(struct phy *phy, enum phy_mode mode, int submode); > + int (*set_media)(struct phy *phy, enum phy_media media); > + int (*set_speed)(struct phy *phy, int speed); > > /** > * @configure: > @@ -215,6 +225,8 @@ int phy_power_off(struct phy *phy); > int phy_set_mode_ext(struct phy *phy, enum phy_mode mode, int submode); > #define phy_set_mode(phy, mode) \ > phy_set_mode_ext(phy, mode, 0) > +int phy_set_media(struct phy *phy, enum phy_media media); > +int phy_set_speed(struct phy *phy, int speed); > int phy_configure(struct phy *phy, union phy_configure_opts *opts); > int phy_validate(struct phy *phy, enum phy_mode mode, int submode, > union phy_configure_opts *opts); > @@ -344,6 +356,20 @@ static inline int phy_set_mode_ext(struct phy *phy, enum phy_mode mode, > #define phy_set_mode(phy, mode) \ > phy_set_mode_ext(phy, mode, 0) > > +static inline int phy_set_media(struct phy *phy, enum phy_media media) > +{ > + if (!phy) > + return 0; > + return -ENODEV; > +} > + > +static inline int phy_set_speed(struct phy *phy, int speed) > +{ > + if (!phy) > + return 0; > + return -ENODEV; > +} > + > static inline enum phy_mode phy_get_mode(struct phy *phy) > { > return PHY_MODE_INVALID; >
diff --git a/drivers/phy/phy-core.c b/drivers/phy/phy-core.c index 71cb10826326..ccb575b13777 100644 --- a/drivers/phy/phy-core.c +++ b/drivers/phy/phy-core.c @@ -373,6 +373,36 @@ int phy_set_mode_ext(struct phy *phy, enum phy_mode mode, int submode) } EXPORT_SYMBOL_GPL(phy_set_mode_ext); +int phy_set_media(struct phy *phy, enum phy_media media) +{ + int ret; + + if (!phy || !phy->ops->set_media) + return 0; + + mutex_lock(&phy->mutex); + ret = phy->ops->set_media(phy, media); + mutex_unlock(&phy->mutex); + + return ret; +} +EXPORT_SYMBOL_GPL(phy_set_media); + +int phy_set_speed(struct phy *phy, int speed) +{ + int ret; + + if (!phy || !phy->ops->set_speed) + return 0; + + mutex_lock(&phy->mutex); + ret = phy->ops->set_speed(phy, speed); + mutex_unlock(&phy->mutex); + + return ret; +} +EXPORT_SYMBOL_GPL(phy_set_speed); + int phy_reset(struct phy *phy) { int ret; diff --git a/include/linux/phy/phy.h b/include/linux/phy/phy.h index e435bdb0bab3..0ed434d02196 100644 --- a/include/linux/phy/phy.h +++ b/include/linux/phy/phy.h @@ -44,6 +44,12 @@ enum phy_mode { PHY_MODE_DP }; +enum phy_media { + PHY_MEDIA_DEFAULT, + PHY_MEDIA_SR, + PHY_MEDIA_DAC, +}; + /** * union phy_configure_opts - Opaque generic phy configuration * @@ -64,6 +70,8 @@ union phy_configure_opts { * @power_on: powering on the phy * @power_off: powering off the phy * @set_mode: set the mode of the phy + * @set_media: set the media type of the phy (optional) + * @set_speed: set the speed of the phy (optional) * @reset: resetting the phy * @calibrate: calibrate the phy * @release: ops to be performed while the consumer relinquishes the PHY @@ -75,6 +83,8 @@ struct phy_ops { int (*power_on)(struct phy *phy); int (*power_off)(struct phy *phy); int (*set_mode)(struct phy *phy, enum phy_mode mode, int submode); + int (*set_media)(struct phy *phy, enum phy_media media); + int (*set_speed)(struct phy *phy, int speed); /** * @configure: @@ -215,6 +225,8 @@ int phy_power_off(struct phy *phy); int phy_set_mode_ext(struct phy *phy, enum phy_mode mode, int submode); #define phy_set_mode(phy, mode) \ phy_set_mode_ext(phy, mode, 0) +int phy_set_media(struct phy *phy, enum phy_media media); +int phy_set_speed(struct phy *phy, int speed); int phy_configure(struct phy *phy, union phy_configure_opts *opts); int phy_validate(struct phy *phy, enum phy_mode mode, int submode, union phy_configure_opts *opts); @@ -344,6 +356,20 @@ static inline int phy_set_mode_ext(struct phy *phy, enum phy_mode mode, #define phy_set_mode(phy, mode) \ phy_set_mode_ext(phy, mode, 0) +static inline int phy_set_media(struct phy *phy, enum phy_media media) +{ + if (!phy) + return 0; + return -ENODEV; +} + +static inline int phy_set_speed(struct phy *phy, int speed) +{ + if (!phy) + return 0; + return -ENODEV; +} + static inline enum phy_mode phy_get_mode(struct phy *phy) { return PHY_MODE_INVALID;