Hi Andrew,
Thank you for review the code.
On Fri, Sep 09, 2016 at 03:18:32PM +0200, Andrew Lunn wrote:
> EXTERNAL EMAIL
>
>
> > > > +static int vsc85xx_edge_rate_cntl_set(struct phy_device *phydev,
> > > > + u8 edge_rate)
> > >
> > > No spaces place.
> > >
> > I ran the checkpatch. I did not find any error. I created another workspace
> > and
> > applied the same patch. It shows the correct alignement. I have used tabs
> > (8 space width).
> > then some spaces to align braces.
>
> Sorry, i worded that poorly. I was meaning between the u8 and edge. A
> single space is enough.
>
I accepted your suggestion.
> > > > +#ifdef CONFIG_OF_MDIO
> > > > +static int vsc8531_of_init(struct phy_device *phydev)
> > > > +{
> > > > + int rc;
> > > > + struct vsc8531_private *vsc8531 = phydev->priv;
> > > > + struct device *dev = &phydev->mdio.dev;
> > > > + struct device_node *of_node = dev->of_node;
> > > > +
> > > > + if (!of_node)
> > > > + return -ENODEV;
> > > > +
> > > > + rc = of_property_read_u8(of_node, "vsc8531,edge-rate",
> > > > + &vsc8531->edge_rate);
> > >
> > > Until you have written the Documentation, it is hard for me to tell,
> > > but device tree bindings should use real units, like seconds, Ohms,
> > > Farads, etc. Is the edge rate in nS? Or is it some magic value which
> > > just gets written into the register?
> > >
> >
> > This is some magic value which just gets written into the register.
>
> Magic values are generally not accepted in device tree bindings. Both
> Micrel and Renesas define their clock skew in ps, for example. Since
> this is rise time, it should also be possible to define it in a unit
> of time.
>
I accepted your comment. I had discussion with my hardware team and explained
the code review comments.
They asked me to define as picoseconds as units.
> > > > static int vsc85xx_config_init(struct phy_device *phydev)
> > > > {
> > > > int rc;
> > > > + struct vsc8531_private *vsc8531;
> > > > +
> > > > + if (!phydev->priv) {
> > >
> > > How can this happen?
> > >
> >
> > VSC 8531 driver don't have any private structure assigned initially.
> > Allways priv points to NULL.
>
> So if it cannot happen, don't check for it.
>
> Also, by convention, you allocate memory in the .probe() function of a
> driver. Please do it there.
>
I accepted your review comment.
I will re-send the patch with updates.
> Andrew
---
Thanks,
Raju.
