Re: [PATCH net-next 5/9] net: phy: icplus: add IP101A/IP101G model detection

[Heiner Kallweit]

On 09.02.2021 17:40, Michael Walle wrote:
> Unfortunately, the IP101A and IP101G share the same PHY identifier.
> While most of the functions are somewhat backwards compatible, there is
> for example the APS_EN bit on the IP101A but on the IP101G this bit
> reserved. Also, the IP101G has many more functionalities.
> 
> Deduce the model by accessing the page select register which - according
> to the datasheet - is not available on the IP101A. If this register is
> writable, assume we have an IP101G.
> 
> Signed-off-by: Michael Walle <michael@xxxxxxxx>
> ---
>  drivers/net/phy/icplus.c | 43 +++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 42 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/phy/icplus.c b/drivers/net/phy/icplus.c
> index 036bac628b11..189a9a34ed5f 100644
> --- a/drivers/net/phy/icplus.c
> +++ b/drivers/net/phy/icplus.c
> @@ -44,6 +44,8 @@ MODULE_LICENSE("GPL");
>  #define IP101A_G_IRQ_DUPLEX_CHANGE	BIT(1)
>  #define IP101A_G_IRQ_LINK_CHANGE	BIT(0)
>  
> +#define IP101G_PAGE_CONTROL				0x14
> +#define IP101G_PAGE_CONTROL_MASK			GENMASK(4, 0)
>  #define IP101G_DIGITAL_IO_SPEC_CTRL			0x1d
>  #define IP101G_DIGITAL_IO_SPEC_CTRL_SEL_INTR32		BIT(2)
>  
> @@ -61,8 +63,14 @@ enum ip101gr_sel_intr32 {
>  	IP101GR_SEL_INTR32_RXER,
>  };
>  
> +enum ip101_model {
> +	IP101A,
> +	IP101G,
> +};
> +
>  struct ip101a_g_phy_priv {
>  	enum ip101gr_sel_intr32 sel_intr32;
> +	enum ip101_model model;
>  };
>  
>  static int ip175c_config_init(struct phy_device *phydev)
> @@ -175,6 +183,39 @@ static int ip175c_config_aneg(struct phy_device *phydev)
>  	return 0;
>  }
>  
> +/* The IP101A and the IP101G share the same PHY identifier.The IP101G seems to
> + * be a successor of the IP101A and implements more functions. Amongst other
> + * things a page select register, which is not available on the IP101. Use this
> + * to distinguish these two.
> + */
> +static int ip101a_g_detect_model(struct phy_device *phydev)
> +{
> +	struct ip101a_g_phy_priv *priv = phydev->priv;
> +	int oldval, ret;
> +
> +	oldval = phy_read(phydev, IP101G_PAGE_CONTROL);
> +	if (oldval < 0)
> +		return oldval;
> +
> +	ret = phy_write(phydev, IP101G_PAGE_CONTROL, 0xffff);
> +	if (ret)
> +		return ret;
> +
> +	ret = phy_read(phydev, IP101G_PAGE_CONTROL);
> +	if (ret < 0)
> +		return ret;
> +
> +	if (ret == IP101G_PAGE_CONTROL_MASK)
> +		priv->model = IP101G;
> +	else
> +		priv->model = IP101A;
> +
> +	phydev_dbg(phydev, "Detected %s\n",
> +		   priv->model == IP101G ? "IP101G" : "IP101A");
> +
> +	return phy_write(phydev, IP101G_PAGE_CONTROL, oldval);
> +}
> +
>  static int ip101a_g_probe(struct phy_device *phydev)
>  {
>  	struct device *dev = &phydev->mdio.dev;
> @@ -203,7 +244,7 @@ static int ip101a_g_probe(struct phy_device *phydev)
>  
>  	phydev->priv = priv;
>  
> -	return 0;
> +	return ip101a_g_detect_model(phydev);
>  }
>  
>  static int ip101a_g_config_init(struct phy_device *phydev)
> 

You could also implement the match_phy_device callback. Then you can
have separate PHY drivers for IP101A/IP101G. Would be cleaner I think.
See the Realtek PHY driver for an example.