Re: [PATCH v4 net-next 05/10] net: dsa: microchip: add DSA support for microchip lan937x

[Prasanna Vengateshan]

On Thu, 2021-10-07 at 23:00 +0300, Vladimir Oltean wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the
> content is safe
> 
> On Thu, Oct 07, 2021 at 08:41:55PM +0530, Prasanna Vengateshan wrote:
> > +static int lan937x_mdio_register(struct dsa_switch *ds)
> > 
> > +
> > +     ret = of_mdiobus_register(ds->slave_mii_bus, mdio_np);
> 
> Please use devm_of_mdiobus_register if you're going to use
> devm_mdiobus_alloc, or no devres at all.
> https://patchwork.kernel.org/project/netdevbpf/patch/20210920214209.1733768-3-vladimir.oltean@xxxxxxx/

Sure, Will change it to devm_of_mdiobus_register.

> > 
> > +
> > +                     /* Check if the device tree have specific interface
> > +                      * setting otherwise read & assign from XMII register
> > +                      * for host port interface
> > +                      */
> > +                     interface = lan937x_get_interface(dev, i);
> 
> What does the CPU port have so special that you override it here?
> Again some compatibility with out-of-tree DT bindings?
> > 
> 
Device strapping method cannot be used since the phy-mode is expected
to be present in the DT. So above assignment have to be removed along
with lan937x_get_interface function.


> > +
> > +     /* maximum delay is 4ns */
> > +     if (val > 4000)
> > +             val = 4000;
> 
> These bindings are new. Given that you also document their min and max
> values, why don't you just error out on out-of-range values instead of
> silently doing what you think is going to be fine?

Sure, i think the driver can also notify rx/tx internal delay applied
message using dev_info.