Re: [PATCH net-next] net: phy: micrel: Add callback for restoring context

[Paolo Abeni <pabeni@redhat.com>]

On 7/7/25 4:29 PM, Biju Das wrote:
> @@ -374,6 +376,7 @@ static struct kszphy_hw_stat kszphy_hw_stats[] = {
>  };
>  
>  struct kszphy_type {
> +	void (*resume)(struct phy_device *phydev);
>  	u32 led_mode_reg;
>  	u16 interrupt_level_mask;
>  	u16 cable_diag_reg;

Why adding another callback? I think you could avoid it using a
ksz9131-specific phy_driver->resume.

> @@ -444,6 +447,7 @@ struct kszphy_priv {
>  	bool rmii_ref_clk_sel;
>  	bool rmii_ref_clk_sel_val;
>  	bool clk_enable;
> +	bool is_suspended;
>  	u64 stats[ARRAY_SIZE(kszphy_hw_stats)];
>  	struct kszphy_phy_stats phy_stats;
>  };
> @@ -491,6 +495,7 @@ static const struct kszphy_type ksz9021_type = {
>  };
>  
>  static const struct kszphy_type ksz9131_type = {
> +	.resume = ksz9131_restore_rgmii_delay,
>  	.interrupt_level_mask	= BIT(14),
>  	.disable_dll_tx_bit	= BIT(12),
>  	.disable_dll_rx_bit	= BIT(12),
> @@ -1387,6 +1392,12 @@ static int ksz9131_config_rgmii_delay(struct phy_device *phydev)
>  			      txcdll_val);
>  }
>  
> +static void ksz9131_restore_rgmii_delay(struct phy_device *phydev)
> +{
> +	if (phy_interface_is_rgmii(phydev))
> +		ksz9131_config_rgmii_delay(phydev);
> +}
> +
>  /* Silicon Errata DS80000693B
>   *
>   * When LEDs are configured in Individual Mode, LED1 is ON in a no-link
> @@ -2345,6 +2356,11 @@ static int kszphy_generic_suspend(struct phy_device *phydev)
>  
>  static int kszphy_suspend(struct phy_device *phydev)
>  {
> +	struct kszphy_priv *priv = phydev->priv;
> +
> +	if (priv)
> +		priv->is_suspended = true;

Under which circumstances `priv` could be NULL? AFAICS it should always
not NULL after probe.

> +
>  	/* Disable PHY Interrupts */
>  	if (phy_interrupt_is_valid(phydev)) {
>  		phydev->interrupts = PHY_INTERRUPT_DISABLED;
> @@ -2381,8 +2397,17 @@ static void kszphy_parse_led_mode(struct phy_device *phydev)
>  
>  static int kszphy_resume(struct phy_device *phydev)
>  {
> +	struct kszphy_priv *priv = phydev->priv;
>  	int ret;
>  
> +	if (priv && priv->is_suspended) {

I think you can use phydev->suspended instead of adding another flag.

Also, can resume be invoked without the phydev being suspended?

/P