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:   Fri, 31 Mar 2023 15:07:42 +0200
From:   Andrew Lunn <andrew@lunn.ch>
To:     "Radu Pirea (OSS)" <radu-nicolae.pirea@oss.nxp.com>
Cc:     hkallweit1@gmail.com, linux@armlinux.org.uk, davem@davemloft.net,
        edumazet@google.com, kuba@kernel.org, pabeni@redhat.com,
        netdev@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [RFC net-next] net: phy: introduce phy_reg_field interface
Message-ID: <d001e708-b5ac-4aa5-9624-4d9ae375d282@lunn.ch>
References: <20230331123259.567627-1-radu-nicolae.pirea@oss.nxp.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20230331123259.567627-1-radu-nicolae.pirea@oss.nxp.com>
Precedence: bulk

On Fri, Mar 31, 2023 at 03:32:59PM +0300, Radu Pirea (OSS) wrote:
> Some PHYs can be heavily modified between revisions, and the addresses of
> the registers are changed and the register fields are moved from one
> register to another.
> 
> To integrate more PHYs in the same driver with the same register fields,
> but these register fields were located in different registers at
> different offsets, I introduced the phy_reg_fied structure.

Maybe you are solving the wrong problem. Maybe you should be telling
the hardware/firmware engineers not to do this!

How many drivers can actually use this?	I don't	really want to
encourage vendors to make such a mess of their hardware, so i'm
wondering if this should be hidden away in the driver, if there	is
only one driver which needs it.	If there are multiple drivers which
can use this, please do	modify at least	one other driver to use	it,
hence showing it is generic.

> +int phy_read_reg_field(struct phy_device *phydev,
> +		       const struct phy_reg_field *reg_field)
> +{
> +	u16 mask;
> +	int ret;
> +
> +	if (reg_field->size == 0) {
> +		phydev_warn(phydev, "Trying to read a reg field of size 0.");
> +		return -EINVAL;
> +	}
> +
> +	phy_lock_mdio_bus(phydev);
> +	if (reg_field->mmd)
> +		ret = __phy_read_mmd(phydev, reg_field->devad,
> +				     reg_field->reg);
> +	else
> +		ret = __phy_read(phydev, reg_field->reg);
> +	phy_unlock_mdio_bus(phydev);
> +

Could you please explain the locking. It appears you are trying to
protect reg_field->mmd? Does that really change? Especially since you
have _const_ struct phy_reg_field *

      Andrew