Re: [PATCH net-next v4 06/11] net: dsa: realtek: add new mdio interface for drivers

[Florian Fainelli <f.fainelli@gmail.com>]

On 1/4/2022 7:15 PM, Luiz Angelo Daros de Luca wrote:
> This driver is a mdio_driver instead of a platform driver (like
> realtek-smi).
> 
> Signed-off-by: Luiz Angelo Daros de Luca <luizluca@gmail.com>
> Tested-by: Arınç ÜNAL <arinc.unal@arinc9.com>
> ---
>   drivers/net/dsa/realtek/Kconfig        |  11 +-
>   drivers/net/dsa/realtek/Makefile       |   1 +
>   drivers/net/dsa/realtek/realtek-mdio.c | 221 +++++++++++++++++++++++++
>   drivers/net/dsa/realtek/realtek.h      |   2 +
>   4 files changed, 233 insertions(+), 2 deletions(-)
>   create mode 100644 drivers/net/dsa/realtek/realtek-mdio.c
> 
> diff --git a/drivers/net/dsa/realtek/Kconfig b/drivers/net/dsa/realtek/Kconfig
> index cd1aa95b7bf0..73b26171fade 100644
> --- a/drivers/net/dsa/realtek/Kconfig
> +++ b/drivers/net/dsa/realtek/Kconfig
> @@ -9,6 +9,13 @@ menuconfig NET_DSA_REALTEK
>   	help
>   	  Select to enable support for Realtek Ethernet switch chips.
>   
> +config NET_DSA_REALTEK_MDIO
> +	tristate "Realtek MDIO connected switch driver"
> +	depends on NET_DSA_REALTEK
> +	default y

I suppose this is fine since we depend on NET_DSA_REALTEK.

[snip]

> +static int realtek_mdio_read_reg(struct realtek_priv *priv, u32 addr, u32 *data)
> +{
> +	u32 phy_id = priv->phy_id;
> +	struct mii_bus *bus = priv->bus;
> +
> +	mutex_lock(&bus->mdio_lock);
> +
> +	bus->write(bus, phy_id, REALTEK_MDIO_CTRL0_REG, REALTEK_MDIO_ADDR_OP);
> +	bus->write(bus, phy_id, REALTEK_MDIO_START_REG, REALTEK_MDIO_START_OP);
> +	bus->write(bus, phy_id, REALTEK_MDIO_ADDRESS_REG, addr);
> +	bus->write(bus, phy_id, REALTEK_MDIO_START_REG, REALTEK_MDIO_START_OP);
> +	bus->write(bus, phy_id, REALTEK_MDIO_CTRL1_REG, REALTEK_MDIO_READ_OP);
> +	bus->write(bus, phy_id, REALTEK_MDIO_START_REG, REALTEK_MDIO_START_OP);
> +	*data = bus->read(bus, phy_id, REALTEK_MDIO_DATA_READ_REG);

Do you have no way to return an error for instance, if you read from a 
non-existent PHY device on the MDIO bus, -EIO would be expected for 
instance. If the data returned is 0xffff that ought to be enough.

> +
> +	mutex_unlock(&bus->mdio_lock);
> +
> +	return 0;
> +}
> +
> +static int realtek_mdio_write_reg(struct realtek_priv *priv, u32 addr, u32 data)
> +{
> +	u32 phy_id = priv->phy_id;
> +	struct mii_bus *bus = priv->bus;
> +
> +	mutex_lock(&bus->mdio_lock);
> +
> +	bus->write(bus, phy_id, REALTEK_MDIO_CTRL0_REG, REALTEK_MDIO_ADDR_OP);
> +	bus->write(bus, phy_id, REALTEK_MDIO_START_REG, REALTEK_MDIO_START_OP);
> +	bus->write(bus, phy_id, REALTEK_MDIO_ADDRESS_REG, addr);

This repeats between read and writes, might be worth a helper function.

> +	bus->write(bus, phy_id, REALTEK_MDIO_START_REG, REALTEK_MDIO_START_OP);
> +	bus->write(bus, phy_id, REALTEK_MDIO_DATA_WRITE_REG, data);
> +	bus->write(bus, phy_id, REALTEK_MDIO_START_REG, REALTEK_MDIO_START_OP);
> +	bus->write(bus, phy_id, REALTEK_MDIO_CTRL1_REG, REALTEK_MDIO_WRITE_OP);
> +
> +	mutex_unlock(&bus->mdio_lock);
> +
> +	return 0;
> +}
> +
> +/* Regmap accessors */
> +
> +static int realtek_mdio_write(void *ctx, u32 reg, u32 val)
> +{
> +	struct realtek_priv *priv = ctx;
> +
> +	return realtek_mdio_write_reg(priv, reg, val);
> +}
> +
> +static int realtek_mdio_read(void *ctx, u32 reg, u32 *val)
> +{
> +	struct realtek_priv *priv = ctx;
> +
> +	return realtek_mdio_read_reg(priv, reg, val);
> +}

Do you see a value for this function as oppposed to inlining the bodies 
of realtek_mdio_read_reg and realtek_mdio_write_reg directly into these 
two functions?

> +
> +static const struct regmap_config realtek_mdio_regmap_config = {
> +	.reg_bits = 10, /* A4..A0 R4..R0 */
> +	.val_bits = 16,
> +	.reg_stride = 1,
> +	/* PHY regs are at 0x8000 */
> +	.max_register = 0xffff,
> +	.reg_format_endian = REGMAP_ENDIAN_BIG,
> +	.reg_read = realtek_mdio_read,
> +	.reg_write = realtek_mdio_write,
> +	.cache_type = REGCACHE_NONE,
> +};
> +
> +static int realtek_mdio_probe(struct mdio_device *mdiodev)
> +{
> +	struct realtek_priv *priv;
> +	struct device *dev = &mdiodev->dev;
> +	const struct realtek_variant *var;
> +	int ret;
> +	struct device_node *np;
> +
> +	var = of_device_get_match_data(dev);

Don't you have to check that var is non-NULL just in case?

> +	priv = devm_kzalloc(&mdiodev->dev, sizeof(*priv), GFP_KERNEL);
> +	if (!priv)
> +		return -ENOMEM;
> +
> +	priv->map = devm_regmap_init(dev, NULL, priv, &realtek_mdio_regmap_config);
> +	if (IS_ERR(priv->map)) {
> +		ret = PTR_ERR(priv->map);
> +		dev_err(dev, "regmap init failed: %d\n", ret);
> +		return ret;
> +	}
> +
> +	priv->phy_id = mdiodev->addr;

Please use a more descriptive variable name such as mdio_addr or 
something like that. I know that phy_id is typically used but it could 
also mean a 32-bit PHY unique identifier, which a MDIO device does not 
have typically.

Looks fine otherwise.
-- 
Florian