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

[Luiz Angelo Daros de Luca <luizluca@gmail.com>]

> > +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.

I'll check for error (non zero for write, negative for read) and
return that value

> > +
> > +     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.

Without the REALTEK_MDIO_START_OP Alvin asked, it is not worth it anymore.

>
> > +     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?
>

I merged them. I also changed the write_reg_noack signature to match
regmap->write_reg, so I can use it without a wrapper.

> > +
> > +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?

I'll add that check but it is not likely to happen.

>
> > +     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.

Renamed to mdio_addr. As you said, I just used what is typically used
but you know best.

>
> Looks fine otherwise.

Thanks, Florian.

Luiz