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

Florian Fainelli
2022-01-17T04:25:33Z

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