Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Tue, 23 May 2023 15:16:51 +0200
From:   Andrew Lunn <andrew@lunn.ch>
To:     David Epping <david.epping@missinglinkelectronics.com>
Cc:     Vladimir Oltean <olteanv@gmail.com>,
        Russell King <linux@armlinux.org.uk>,
        Heiner Kallweit <hkallweit1@gmail.com>,
        "David S . Miller" <davem@davemloft.net>,
        Eric Dumazet <edumazet@google.com>,
        Jakub Kicinski <kuba@kernel.org>,
        Paolo Abeni <pabeni@redhat.com>, netdev@vger.kernel.org,
        linux-kernel@vger.kernel.org, UNGLinuxDriver@microchip.com
Subject: Re: [PATCH net v2 0/3] net: phy: mscc: support VSC8501
Message-ID: <c613298d-53bc-46ef-9cb2-4b385e21ba7b@lunn.ch>
References: <20230523090405.10655-1-david.epping@missinglinkelectronics.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20230523090405.10655-1-david.epping@missinglinkelectronics.com>
Precedence: bulk

> - I left the mutex_lock(&phydev->lock) in the
>   vsc85xx_update_rgmii_cntl() function, as I'm not sure whether it
>   is required to repeatedly access phydev->interface and
>   phy_interface_is_rgmii(phydev) in a consistent way.

Just adding to Russell comment.

As a general rule of thumb, if your driver is doing something which no
other driver is doing, you have to consider if it is correct. A PHY
driver taking phydev->lock is very unusual. So at minimum you should
be able to explain why it is needed. And when it comes to locking,
locking is hard, so you really should understand it.

Now the mscc is an odd device, because it has multiple PHYs in the
package, and a number of registers are shared between these PHYs. So
it does have different locking requirements to most PHYs. However, i
don't think that is involved here. Those oddities are hidden behind
phy_base_write() and phy_base_read().

	Andrew