Re: [PATCH v8 2/5] include: linux: phy: Add phy attribute "type" and associated helpers

[Vinod Koul <vkoul@kernel.org>]

On 13-09-26, 20:10, Krishna Kurapati wrote:
> In cases like USB High-speed phys which can be either USB2 or EUSB2, it is
> required to know the type of phy (rather than the operating mode) because
> DP and DM interrupt lines need to be configured differently for each of
> them.

Does the controller care? In preceding patch you defined the EUSB2 in dt
type, so this describes the hardware. So phy knows it is usb or eusb...
> 
> Add support to cache the PHY_TYPE and add the following helpers:
> 
> - phy_set_type() for the phy drivers (like m31_eusb2 or snps-eusb2) to
> declare what type of PHY they are (in this case PHY_TYPE_EUSB2).
> 
> - phy_get_type() for the consumers (like USB controllers) to query the
> type of phy connected to them.

I am not convinced that this is the way to go... Driver already knows
the type and should use it...
we alreayd have mode, i am inclined to not say yes to adding type here

> 
> Reviewed-by: Thinh Nguyen <Thinh.Nguyen@synopsys.com>
> Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@oss.qualcomm.com>
> Signed-off-by: Krishna Kurapati <krishna.kurapati@oss.qualcomm.com>
> ---
>  include/linux/phy/phy.h | 27 +++++++++++++++++++++++++++
>  1 file changed, 27 insertions(+)
> 
> diff --git a/include/linux/phy/phy.h b/include/linux/phy/phy.h
> index ea47975e288a..038c2b58bbe1 100644
> --- a/include/linux/phy/phy.h
> +++ b/include/linux/phy/phy.h
> @@ -21,6 +21,8 @@
>  #include <linux/phy/phy-lvds.h>
>  #include <linux/phy/phy-mipi-dphy.h>
>  
> +#include <dt-bindings/phy/phy.h>
> +
>  struct phy;
>  
>  enum phy_mode {
> @@ -152,11 +154,13 @@ struct phy_ops {
>   * @bus_width: Data path width implemented by PHY
>   * @max_link_rate: Maximum link rate supported by PHY (units to be decided by producer and consumer)
>   * @mode: PHY mode
> + * @type: PHY type
>   */
>  struct phy_attrs {
>  	u32			bus_width;
>  	u32			max_link_rate;
>  	enum phy_mode		mode;
> +	int			type;
>  };
>  
>  /**
> @@ -262,6 +266,20 @@ static inline enum phy_mode phy_get_mode(struct phy *phy)
>  {
>  	return phy->attrs.mode;
>  }
> +
> +static inline int phy_get_type(struct phy *phy)
> +{
> +	if (phy)
> +		return phy->attrs.type;
> +
> +	return PHY_NONE;
> +}
> +
> +static inline void phy_set_type(struct phy *phy, int type)
> +{
> +	phy->attrs.type = type;
> +}
> +
>  int phy_reset(struct phy *phy);
>  int phy_calibrate(struct phy *phy);
>  int phy_notify_connect(struct phy *phy, int port);
> @@ -393,6 +411,15 @@ static inline enum phy_mode phy_get_mode(struct phy *phy)
>  	return PHY_MODE_INVALID;
>  }
>  
> +static inline int phy_get_type(struct phy *phy)
> +{
> +	return PHY_NONE;
> +}
> +
> +static inline void phy_set_type(struct phy *phy, int type)
> +{
> +}
> +
>  static inline int phy_reset(struct phy *phy)
>  {
>  	if (!phy)
> 
> -- 
> 2.34.1

-- 
~Vinod