Re: [PATCH net-next 7/9] net: phy: icplus: select page before writing control register

[Heiner Kallweit <hkallweit1@gmail.com>]

On 09.02.2021 17:40, Michael Walle wrote:
> Registers >= 16 are paged. Be sure to set the page. It seems this was
> working for now, because the default is correct for the registers used
> in the driver at the moment. But this will also assume, nobody will
> change the page select register before linux is started. The page select
> register is _not_ reset with a soft reset of the PHY.
> 
> Add read_page()/write_page() support for the IP101G and use it
> accordingly.
> 
> Signed-off-by: Michael Walle <michael@walle.cc>
> ---
>  drivers/net/phy/icplus.c | 50 +++++++++++++++++++++++++++++++---------
>  1 file changed, 39 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/net/phy/icplus.c b/drivers/net/phy/icplus.c
> index a6e1c7611f15..858b9326a72d 100644
> --- a/drivers/net/phy/icplus.c
> +++ b/drivers/net/phy/icplus.c
> @@ -49,6 +49,8 @@ MODULE_LICENSE("GPL");
>  #define IP101G_DIGITAL_IO_SPEC_CTRL			0x1d
>  #define IP101G_DIGITAL_IO_SPEC_CTRL_SEL_INTR32		BIT(2)
>  
> +#define IP101G_DEFAULT_PAGE			16
> +
>  #define IP175C_PHY_ID 0x02430d80
>  #define IP1001_PHY_ID 0x02430d90
>  #define IP101A_PHY_ID 0x02430c54
> @@ -250,23 +252,25 @@ static int ip101a_g_probe(struct phy_device *phydev)
>  static int ip101a_g_config_init(struct phy_device *phydev)
>  {
>  	struct ip101a_g_phy_priv *priv = phydev->priv;
> -	int err;
> +	int oldpage, err;
> +
> +	oldpage = phy_select_page(phydev, IP101G_DEFAULT_PAGE);
>  
>  	/* configure the RXER/INTR_32 pin of the 32-pin IP101GR if needed: */
>  	switch (priv->sel_intr32) {
>  	case IP101GR_SEL_INTR32_RXER:
> -		err = phy_modify(phydev, IP101G_DIGITAL_IO_SPEC_CTRL,
> -				 IP101G_DIGITAL_IO_SPEC_CTRL_SEL_INTR32, 0);
> +		err = __phy_modify(phydev, IP101G_DIGITAL_IO_SPEC_CTRL,
> +				   IP101G_DIGITAL_IO_SPEC_CTRL_SEL_INTR32, 0);
>  		if (err < 0)
> -			return err;
> +			goto out;
>  		break;
>  
>  	case IP101GR_SEL_INTR32_INTR:
> -		err = phy_modify(phydev, IP101G_DIGITAL_IO_SPEC_CTRL,
> -				 IP101G_DIGITAL_IO_SPEC_CTRL_SEL_INTR32,
> -				 IP101G_DIGITAL_IO_SPEC_CTRL_SEL_INTR32);
> +		err = __phy_modify(phydev, IP101G_DIGITAL_IO_SPEC_CTRL,
> +				   IP101G_DIGITAL_IO_SPEC_CTRL_SEL_INTR32,
> +				   IP101G_DIGITAL_IO_SPEC_CTRL_SEL_INTR32);
>  		if (err < 0)
> -			return err;
> +			goto out;
>  		break;
>  
>  	default:
> @@ -284,12 +288,14 @@ static int ip101a_g_config_init(struct phy_device *phydev)
>  	 * reserved as 'write-one'.
>  	 */
>  	if (priv->model == IP101A) {
> -		err = phy_set_bits(phydev, IP10XX_SPEC_CTRL_STATUS, IP101A_G_APS_ON);
> +		err = __phy_set_bits(phydev, IP10XX_SPEC_CTRL_STATUS,
> +				     IP101A_G_APS_ON);
>  		if (err)
> -			return err;
> +			goto out;
>  	}
>  
> -	return 0;
> +out:
> +	return phy_restore_page(phydev, oldpage, err);

If a random page was set before entering config_init, do we actually want
to restore it? Or wouldn't it be better to set the default page as part
of initialization?

>  }
>  
>  static int ip101a_g_ack_interrupt(struct phy_device *phydev)
> @@ -347,6 +353,26 @@ static irqreturn_t ip101a_g_handle_interrupt(struct phy_device *phydev)
>  	return IRQ_HANDLED;
>  }
>  
> +static int ip101a_g_read_page(struct phy_device *phydev)
> +{
> +	struct ip101a_g_phy_priv *priv = phydev->priv;
> +
> +	if (priv->model == IP101A)
> +		return 0;
> +
> +	return __phy_read(phydev, IP101G_PAGE_CONTROL);
> +}
> +
> +static int ip101a_g_write_page(struct phy_device *phydev, int page)
> +{
> +	struct ip101a_g_phy_priv *priv = phydev->priv;
> +
> +	if (priv->model == IP101A)
> +		return 0;
> +
> +	return __phy_write(phydev, IP101G_PAGE_CONTROL, page);
> +}
> +
>  static struct phy_driver icplus_driver[] = {
>  {
>  	PHY_ID_MATCH_MODEL(IP175C_PHY_ID),
> @@ -373,6 +399,8 @@ static struct phy_driver icplus_driver[] = {
>  	.config_intr	= ip101a_g_config_intr,
>  	.handle_interrupt = ip101a_g_handle_interrupt,
>  	.config_init	= ip101a_g_config_init,
> +	.read_page	= ip101a_g_read_page,
> +	.write_page	= ip101a_g_write_page,
>  	.soft_reset	= genphy_soft_reset,
>  	.suspend	= genphy_suspend,
>  	.resume		= genphy_resume,
>