Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:2 as permitted sender) client-ip=2620:137:e000::3:2;
Date:   Thu, 7 Dec 2023 11:02:43 +0100
From:   Greg Kroah-Hartman <gregkh@linuxfoundation.org>
To:     Stanley Chang <stanley_chang@realtek.com>
Cc:     Vinod Koul <vkoul@kernel.org>, Johan Hovold <johan@kernel.org>,
        Kishon Vijay Abraham I <kishon@kernel.org>,
        Geert Uytterhoeven <geert+renesas@glider.be>,
        Jinjie Ruan <ruanjinjie@huawei.com>,
        Rob Herring <robh@kernel.org>,
        Alan Stern <stern@rowland.harvard.edu>,
        Heikki Krogerus <heikki.krogerus@linux.intel.com>,
        Flavio Suligoi <f.suligoi@asem.it>,
        Ricardo =?iso-8859-1?Q?Ca=F1uelo?= 
        <ricardo.canuelo@collabora.com>, linux-kernel@vger.kernel.org,
        linux-phy@lists.infradead.org, linux-usb@vger.kernel.org
Subject: Re: [PATCH v3 RESEND 1/4] phy: core: add notify_connect and
 notify_disconnect callback
Message-ID: <2023120750-giggle-annotate-dc4f@gregkh>
References: <20231207074022.14116-1-stanley_chang@realtek.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20231207074022.14116-1-stanley_chang@realtek.com>
Precedence: bulk

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