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 13:46:07 +0100
From:   "Russell King (Oracle)" <linux@armlinux.org.uk>
To:     "Radu Pirea (OSS)" <radu-nicolae.pirea@oss.nxp.com>
Cc:     andrew@lunn.ch, hkallweit1@gmail.com, 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: <ZCbWD7TiiCzxgWoI@shell.armlinux.org.uk>
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>
Sender: Russell King (Oracle) <linux@armlinux.org.uk>
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.
> 
> phy_reg_fied structure abstracts the register fields differences.

Oh no, not more perliferation of different accessors...

> +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);
> +
> +	if (ret < 0)
> +		return ret;
> +
> +	mask = reg_field->size == 1 ? BIT(reg_field->offset) :
> +		GENMASK(reg_field->offset + reg_field->size - 1, reg_field->offset);
> +	ret &= mask;
> +	ret >>= reg_field->offset;
> +
> +	return ret;
> +}
> +EXPORT_SYMBOL_GPL(phy_read_reg_field);

I guess next we'll eventually see that we need __phy_read_reg_field
which doesn't take the lock, so that several accesses can be done
together. E.g. to access some form of paging mechanism.

> +/**
> + * phy_write_reg_field - Convenience function for writing a register field
> + * on a given PHY.
> + * @phydev: the phy_device struct
> + * @reg_field: the phy_reg_field structure to be written
> + * @val: value to write to @reg_field
> + *
> + * Return: 0 on success, -errno on failure.
> + */
> +int phy_write_reg_field(struct phy_device *phydev,
> +			const struct phy_reg_field *reg_field, u16 val)
> +{
> +	u16 mask;
> +	u16 set;
> +	int ret;
> +
> +	if (reg_field->size == 0) {
> +		phydev_warn(phydev, "Trying to write a reg field of size 0.");
> +		return -EINVAL;
> +	}
> +
> +	mask = reg_field->size == 1 ? BIT(reg_field->offset) :
> +		GENMASK(reg_field->offset + reg_field->size - 1, reg_field->offset);
> +	set = val << reg_field->offset;
> +
> +	phy_lock_mdio_bus(phydev);
> +	if (reg_field->mmd)
> +		ret = __phy_modify_mmd_changed(phydev, reg_field->devad,
> +					       reg_field->reg, mask, set);
> +	else
> +		ret = __phy_modify_changed(phydev, reg_field->reg,
> +					   mask, set);
> +	phy_unlock_mdio_bus(phydev);
> +
> +	return ret;
> +}
> +EXPORT_SYMBOL_GPL(phy_write_reg_field);

More or less the same for this too.

In order to properly review this, we need the patch which has the use
case for these new accessors.

Thanks.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!