| Message ID | 20231207074022.14116-1-stanley_chang@realtek.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v3,RESEND,1/4] phy: core: add notify_connect and notify_disconnect callback | expand |
On 12/7/23 10:38 AM, Stanley Chang wrote: > In Realtek SoC, the parameter of usb phy is designed to can dynamic > tuning base on port status. Therefore, add a notify callback of phy To be able to do dynamic tuning based in the port status, maybe? > driver when usb connection/disconnection change. > > Signed-off-by: Stanley Chang <stanley_chang@realtek.com> [...] > diff --git a/drivers/phy/phy-core.c b/drivers/phy/phy-core.c > index 96a0b1e111f3..a84ad4896b7f 100644 > --- a/drivers/phy/phy-core.c > +++ b/drivers/phy/phy-core.c > @@ -489,6 +489,53 @@ int phy_calibrate(struct phy *phy) > } > EXPORT_SYMBOL_GPL(phy_calibrate); > > +/** > + * phy_notify_connect() - phy connect notify Notification? > + * @phy: the phy returned by phy_get() > + * @port: the port index for connect > + * > + * If phy need the get connection status, the callback can be used. If the PHY needs to get the connection status, maybe? > + * Returns: %0 if successful, a negative error code otherwise > + */ > +int phy_notify_connect(struct phy *phy, int port) > +{ > + int ret; > + > + if (!phy || !phy->ops->connect) > + return 0; > + > + mutex_lock(&phy->mutex); > + ret = phy->ops->connect(phy, port); > + mutex_unlock(&phy->mutex); > + > + return ret; > +} > +EXPORT_SYMBOL_GPL(phy_notify_connect); > + > +/** > + * phy_notify_disconnect() - phy disconnect notify Notification? > + * @phy: the phy returned by phy_get() > + * @port: the port index for disconnect > + * > + * If phy need the get disconnection status, the callback can be used. If the PHY needs to get the connection status, maybe? [...] > diff --git a/include/linux/phy/phy.h b/include/linux/phy/phy.h > index f6d607ef0e80..cf98cb29ddaa 100644 > --- a/include/linux/phy/phy.h > +++ b/include/linux/phy/phy.h [...] MBR, Sergey
Hi Sergei, > > > On 12/7/23 10:38 AM, Stanley Chang wrote: > > > In Realtek SoC, the parameter of usb phy is designed to can dynamic > > tuning base on port status. Therefore, add a notify callback of phy > > To be able to do dynamic tuning based in the port status, maybe? Okay, I will revise. > > driver when usb connection/disconnection change. > > Signed-off-by: Stanley Chang <stanley_chang@realtek.com> > [...] > > diff --git a/drivers/phy/phy-core.c b/drivers/phy/phy-core.c index > > 96a0b1e111f3..a84ad4896b7f 100644 > > --- a/drivers/phy/phy-core.c > > +++ b/drivers/phy/phy-core.c > > @@ -489,6 +489,53 @@ int phy_calibrate(struct phy *phy) } > > EXPORT_SYMBOL_GPL(phy_calibrate); > > > > +/** > > + * phy_notify_connect() - phy connect notify > > Notification? Okay, I will revise. > > + * @phy: the phy returned by phy_get() > > + * @port: the port index for connect > > + * > > + * If phy need the get connection status, the callback can be used. > > If the PHY needs to get the connection status, maybe? > > > + * Returns: %0 if successful, a negative error code otherwise */ int > > +phy_notify_connect(struct phy *phy, int port) { > > + int ret; > > + > > + if (!phy || !phy->ops->connect) > > + return 0; > > + > > + mutex_lock(&phy->mutex); > > + ret = phy->ops->connect(phy, port); > > + mutex_unlock(&phy->mutex); > > + > > + return ret; > > +} > > +EXPORT_SYMBOL_GPL(phy_notify_connect); > > + > > +/** > > + * phy_notify_disconnect() - phy disconnect notify > > Notification? Okay, I will revise. > > + * @phy: the phy returned by phy_get() > > + * @port: the port index for disconnect > > + * > > + * If phy need the get disconnection status, the callback can be used. > > If the PHY needs to get the connection status, maybe? Okay, I will revise. Thanks, Stanley
On Thu, Dec 07, 2023 at 03:38:04PM +0800, Stanley Chang wrote: > In Realtek SoC, the parameter of usb phy is designed to can dynamic > tuning base on port status. Therefore, add a notify callback of phy > driver when usb connection/disconnection change. > > Signed-off-by: Stanley Chang <stanley_chang@realtek.com> > --- > RESEND: > Because there is no extcon device provided in the USB framework to > notify connect and disconnect. > Therefore, I added the notification connection/disconnection based > on the generic phy. So I no use the EXTCON framework for notifying > connect/disconnect. > v2 to v3: > No change > v1 to v2: > No change > --- > drivers/phy/phy-core.c | 47 +++++++++++++++++++++++++++++++++++++++++ > include/linux/phy/phy.h | 18 ++++++++++++++++ > 2 files changed, 65 insertions(+) > > diff --git a/drivers/phy/phy-core.c b/drivers/phy/phy-core.c > index 96a0b1e111f3..a84ad4896b7f 100644 > --- a/drivers/phy/phy-core.c > +++ b/drivers/phy/phy-core.c > @@ -489,6 +489,53 @@ int phy_calibrate(struct phy *phy) > } > EXPORT_SYMBOL_GPL(phy_calibrate); > > +/** > + * phy_notify_connect() - phy connect notify > + * @phy: the phy returned by phy_get() > + * @port: the port index for connect > + * > + * If phy need the get connection status, the callback can be used. > + * Returns: %0 if successful, a negative error code otherwise > + */ > +int phy_notify_connect(struct phy *phy, int port) > +{ > + int ret; > + > + if (!phy || !phy->ops->connect) > + return 0; How can phy be null? And it is not successful if connect is not valid, so why not return an error there? > + > + mutex_lock(&phy->mutex); > + ret = phy->ops->connect(phy, port); > + mutex_unlock(&phy->mutex); > + > + return ret; > +} > +EXPORT_SYMBOL_GPL(phy_notify_connect); > + > +/** > + * phy_notify_disconnect() - phy disconnect notify > + * @phy: the phy returned by phy_get() > + * @port: the port index for disconnect > + * > + * If phy need the get disconnection status, the callback can be used. > + * > + * Returns: %0 if successful, a negative error code otherwise > + */ > +int phy_notify_disconnect(struct phy *phy, int port) > +{ > + int ret; > + > + if (!phy || !phy->ops->disconnect) > + return 0; Same as above. > + > + mutex_lock(&phy->mutex); > + ret = phy->ops->disconnect(phy, port); > + mutex_unlock(&phy->mutex); > + > + return ret; > +} > +EXPORT_SYMBOL_GPL(phy_notify_disconnect); > + > /** > * phy_configure() - Changes the phy parameters > * @phy: the phy returned by phy_get() > diff --git a/include/linux/phy/phy.h b/include/linux/phy/phy.h > index f6d607ef0e80..cf98cb29ddaa 100644 > --- a/include/linux/phy/phy.h > +++ b/include/linux/phy/phy.h > @@ -122,6 +122,8 @@ struct phy_ops { > union phy_configure_opts *opts); > int (*reset)(struct phy *phy); > int (*calibrate)(struct phy *phy); > + int (*connect)(struct phy *phy, int port); > + int (*disconnect)(struct phy *phy, int port); You forgot to document these and would have a warning from the documentation build if this was applied :( > void (*release)(struct phy *phy); > struct module *owner; > }; > @@ -243,6 +245,8 @@ static inline enum phy_mode phy_get_mode(struct phy *phy) > } > int phy_reset(struct phy *phy); > int phy_calibrate(struct phy *phy); > +int phy_notify_connect(struct phy *phy, int port); > +int phy_notify_disconnect(struct phy *phy, int port); > static inline int phy_get_bus_width(struct phy *phy) > { > return phy->attrs.bus_width; > @@ -396,6 +400,20 @@ static inline int phy_calibrate(struct phy *phy) > return -ENOSYS; > } > > +static inline int phy_notify_connect(struct phy *phy, int index) > +{ > + if (!phy) > + return 0; Why check this? > + return -ENOSYS; > +} > + > +static inline int phy_notify_disconnect(struct phy *phy, int index) > +{ > + if (!phy) > + return 0; Again, why check this? thanks, greg k-h
Hi Greg, > > On Thu, Dec 07, 2023 at 03:38:04PM +0800, Stanley Chang wrote: > > In Realtek SoC, the parameter of usb phy is designed to can dynamic > > tuning base on port status. Therefore, add a notify callback of phy > > driver when usb connection/disconnection change. > > > > Signed-off-by: Stanley Chang <stanley_chang@realtek.com> > > --- > > RESEND: > > Because there is no extcon device provided in the USB framework to > > notify connect and disconnect. > > Therefore, I added the notification connection/disconnection based > > on the generic phy. So I no use the EXTCON framework for notifying > > connect/disconnect. > > v2 to v3: > > No change > > v1 to v2: > > No change > > --- > > drivers/phy/phy-core.c | 47 > > +++++++++++++++++++++++++++++++++++++++++ > > include/linux/phy/phy.h | 18 ++++++++++++++++ > > 2 files changed, 65 insertions(+) > > > > diff --git a/drivers/phy/phy-core.c b/drivers/phy/phy-core.c index > > 96a0b1e111f3..a84ad4896b7f 100644 > > --- a/drivers/phy/phy-core.c > > +++ b/drivers/phy/phy-core.c > > @@ -489,6 +489,53 @@ int phy_calibrate(struct phy *phy) } > > EXPORT_SYMBOL_GPL(phy_calibrate); > > > > +/** > > + * phy_notify_connect() - phy connect notify > > + * @phy: the phy returned by phy_get() > > + * @port: the port index for connect > > + * > > + * If phy need the get connection status, the callback can be used. > > + * Returns: %0 if successful, a negative error code otherwise */ int > > +phy_notify_connect(struct phy *phy, int port) { > > + int ret; > > + > > + if (!phy || !phy->ops->connect) > > + return 0; > > How can phy be null? > > And it is not successful if connect is not valid, so why not return an error there? This is possible. If a caller no use the generic phy or no define the connect callback. And if no define connect callback, it will do nothing. So no any error. > > + > > + mutex_lock(&phy->mutex); > > + ret = phy->ops->connect(phy, port); > > + mutex_unlock(&phy->mutex); > > + > > + return ret; > > +} > > +EXPORT_SYMBOL_GPL(phy_notify_connect); > > + > > +/** > > + * phy_notify_disconnect() - phy disconnect notify > > + * @phy: the phy returned by phy_get() > > + * @port: the port index for disconnect > > + * > > + * If phy need the get disconnection status, the callback can be used. > > + * > > + * Returns: %0 if successful, a negative error code otherwise */ int > > +phy_notify_disconnect(struct phy *phy, int port) { > > + int ret; > > + > > + if (!phy || !phy->ops->disconnect) > > + return 0; > > Same as above. Same as connect callback. > > + > > + mutex_lock(&phy->mutex); > > + ret = phy->ops->disconnect(phy, port); > > + mutex_unlock(&phy->mutex); > > + > > + return ret; > > +} > > +EXPORT_SYMBOL_GPL(phy_notify_disconnect); > > + > > /** > > * phy_configure() - Changes the phy parameters > > * @phy: the phy returned by phy_get() diff --git > > a/include/linux/phy/phy.h b/include/linux/phy/phy.h index > > f6d607ef0e80..cf98cb29ddaa 100644 > > --- a/include/linux/phy/phy.h > > +++ b/include/linux/phy/phy.h > > @@ -122,6 +122,8 @@ struct phy_ops { > > union phy_configure_opts *opts); > > int (*reset)(struct phy *phy); > > int (*calibrate)(struct phy *phy); > > + int (*connect)(struct phy *phy, int port); > > + int (*disconnect)(struct phy *phy, int port); > > You forgot to document these and would have a warning from the > documentation build if this was applied :( > I will add a description. > > void (*release)(struct phy *phy); > > struct module *owner; > > }; > > @@ -243,6 +245,8 @@ static inline enum phy_mode phy_get_mode(struct > > phy *phy) } int phy_reset(struct phy *phy); int > > phy_calibrate(struct phy *phy); > > +int phy_notify_connect(struct phy *phy, int port); int > > +phy_notify_disconnect(struct phy *phy, int port); > > static inline int phy_get_bus_width(struct phy *phy) { > > return phy->attrs.bus_width; > > @@ -396,6 +400,20 @@ static inline int phy_calibrate(struct phy *phy) > > return -ENOSYS; > > } > > > > +static inline int phy_notify_connect(struct phy *phy, int index) { > > + if (!phy) > > + return 0; > > Why check this? It is normal for phy to be null. But if phy is not empty, -ENOSYS should be returned to indicate the error "Operation not supported". Other callbacks also follow this rule. > > + return -ENOSYS; > > +} > > + > > +static inline int phy_notify_disconnect(struct phy *phy, int index) { > > + if (!phy) > > + return 0; > > Again, why check this? As above. Thanks, Stanley
diff --git a/drivers/phy/phy-core.c b/drivers/phy/phy-core.c index 96a0b1e111f3..a84ad4896b7f 100644 --- a/drivers/phy/phy-core.c +++ b/drivers/phy/phy-core.c @@ -489,6 +489,53 @@ int phy_calibrate(struct phy *phy) } EXPORT_SYMBOL_GPL(phy_calibrate); +/** + * phy_notify_connect() - phy connect notify + * @phy: the phy returned by phy_get() + * @port: the port index for connect + * + * If phy need the get connection status, the callback can be used. + * Returns: %0 if successful, a negative error code otherwise + */ +int phy_notify_connect(struct phy *phy, int port) +{ + int ret; + + if (!phy || !phy->ops->connect) + return 0; + + mutex_lock(&phy->mutex); + ret = phy->ops->connect(phy, port); + mutex_unlock(&phy->mutex); + + return ret; +} +EXPORT_SYMBOL_GPL(phy_notify_connect); + +/** + * phy_notify_disconnect() - phy disconnect notify + * @phy: the phy returned by phy_get() + * @port: the port index for disconnect + * + * If phy need the get disconnection status, the callback can be used. + * + * Returns: %0 if successful, a negative error code otherwise + */ +int phy_notify_disconnect(struct phy *phy, int port) +{ + int ret; + + if (!phy || !phy->ops->disconnect) + return 0; + + mutex_lock(&phy->mutex); + ret = phy->ops->disconnect(phy, port); + mutex_unlock(&phy->mutex); + + return ret; +} +EXPORT_SYMBOL_GPL(phy_notify_disconnect); + /** * phy_configure() - Changes the phy parameters * @phy: the phy returned by phy_get() diff --git a/include/linux/phy/phy.h b/include/linux/phy/phy.h index f6d607ef0e80..cf98cb29ddaa 100644 --- a/include/linux/phy/phy.h +++ b/include/linux/phy/phy.h @@ -122,6 +122,8 @@ struct phy_ops { union phy_configure_opts *opts); int (*reset)(struct phy *phy); int (*calibrate)(struct phy *phy); + int (*connect)(struct phy *phy, int port); + int (*disconnect)(struct phy *phy, int port); void (*release)(struct phy *phy); struct module *owner; }; @@ -243,6 +245,8 @@ static inline enum phy_mode phy_get_mode(struct phy *phy) } int phy_reset(struct phy *phy); int phy_calibrate(struct phy *phy); +int phy_notify_connect(struct phy *phy, int port); +int phy_notify_disconnect(struct phy *phy, int port); static inline int phy_get_bus_width(struct phy *phy) { return phy->attrs.bus_width; @@ -396,6 +400,20 @@ static inline int phy_calibrate(struct phy *phy) return -ENOSYS; } +static inline int phy_notify_connect(struct phy *phy, int index) +{ + if (!phy) + return 0; + return -ENOSYS; +} + +static inline int phy_notify_disconnect(struct phy *phy, int index) +{ + if (!phy) + return 0; + return -ENOSYS; +} + static inline int phy_configure(struct phy *phy, union phy_configure_opts *opts) {
In Realtek SoC, the parameter of usb phy is designed to can dynamic tuning base on port status. Therefore, add a notify callback of phy driver when usb connection/disconnection change. Signed-off-by: Stanley Chang <stanley_chang@realtek.com> --- RESEND: Because there is no extcon device provided in the USB framework to notify connect and disconnect. Therefore, I added the notification connection/disconnection based on the generic phy. So I no use the EXTCON framework for notifying connect/disconnect. v2 to v3: No change v1 to v2: No change --- drivers/phy/phy-core.c | 47 +++++++++++++++++++++++++++++++++++++++++ include/linux/phy/phy.h | 18 ++++++++++++++++ 2 files changed, 65 insertions(+)