Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Subject: Re: [PATCH v1] net: phy: at803x: add ar9331 support
To:     Oleksij Rempel <o.rempel@pengutronix.de>,
        Andrew Lunn <andrew@lunn.ch>,
        Florian Fainelli <f.fainelli@gmail.com>
Cc:     Pengutronix Kernel Team <kernel@pengutronix.de>,
        "David S. Miller" <davem@davemloft.net>, netdev@vger.kernel.org,
        linux-kernel@vger.kernel.org
References: <20190930092710.32739-1-o.rempel@pengutronix.de>
From:   Heiner Kallweit <hkallweit1@gmail.com>
Message-ID: <142d584d-e3b1-a544-fd78-ea93a02b594f@gmail.com>
Date:   Mon, 30 Sep 2019 20:19:30 +0200
User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:60.0) Gecko/20100101
 Thunderbird/60.9.0
MIME-Version: 1.0
In-Reply-To: <20190930092710.32739-1-o.rempel@pengutronix.de>
Content-Type: text/plain; charset=utf-8
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On 30.09.2019 11:27, Oleksij Rempel wrote:
> Mostly this hardware can work with generic PHY driver, but this change
> is needed to provided interrupt handling support.
> Tested with dsa ar9331-switch driver.
> 
> Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
> ---
>  drivers/net/phy/at803x.c | 14 ++++++++++++++
>  1 file changed, 14 insertions(+)
> 
> diff --git a/drivers/net/phy/at803x.c b/drivers/net/phy/at803x.c
> index 6ad8b1c63c34..d62a77adb8e7 100644
> --- a/drivers/net/phy/at803x.c
> +++ b/drivers/net/phy/at803x.c
> @@ -53,6 +53,7 @@
>  #define AT803X_DEBUG_REG_5			0x05
>  #define AT803X_DEBUG_TX_CLK_DLY_EN		BIT(8)
>  
> +#define AR9331_PHY_ID 0x004dd041
>  #define ATH8030_PHY_ID 0x004dd076
>  #define ATH8031_PHY_ID 0x004dd074
>  #define ATH8035_PHY_ID 0x004dd072
> @@ -406,11 +407,24 @@ static struct phy_driver at803x_driver[] = {
>  	.aneg_done		= at803x_aneg_done,
>  	.ack_interrupt		= &at803x_ack_interrupt,
>  	.config_intr		= &at803x_config_intr,
> +}, {
> +	/* ATHEROS AR9331 */
> +	.phy_id			= AR9331_PHY_ID,
> +	.name			= "Atheros AR9331 built-in PHY",
> +	.phy_id_mask		= AT803X_PHY_ID_MASK,

The ID mask of 0xffffffef is quite strange, it ignores the last
bit of the model number and requires exact match of the revision
number. Unfortunately the original commit doesn't explain why
this mask was chosen. It would only make sense if there are
functionally identical PHY's with e.g. id 0x004dd051.
If in doubt I'd suggest you use macro PHY_ID_MATCH_EXACT.

> +	.probe			= at803x_probe,

After 5c5f626bcace ("net: phy: improve handling link_change_notify
callback") struct at803x_priv isn't used any longer and the probe
callback could be removed. I didn't do that as part of this commit
because I could compile-test the change only.

> +	.config_init		= at803x_config_init,
> +	.suspend		= at803x_suspend,
> +	.resume			= at803x_resume,
> +	/* PHY_BASIC_FEATURES */
> +	.ack_interrupt		= &at803x_ack_interrupt,
> +	.config_intr		= &at803x_config_intr,
>  } };
>  
>  module_phy_driver(at803x_driver);
>  
>  static struct mdio_device_id __maybe_unused atheros_tbl[] = {
> +	{ AR9331_PHY_ID, AT803X_PHY_ID_MASK },

See comment above regarding the id mask.

>  	{ ATH8030_PHY_ID, AT803X_PHY_ID_MASK },
>  	{ ATH8031_PHY_ID, AT803X_PHY_ID_MASK },
>  	{ ATH8035_PHY_ID, AT803X_PHY_ID_MASK },
> 
Heiner