Re: [RFC net-next] net: phy: introduce phy_reg_field interface

["Russell King (Oracle)" <linux@armlinux.org.uk>]

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!