netdev - Re: [PATCH v2 net-next 1/2] net: phy: Add Edge-rate driver for Microsemi PHYs.

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Thu, 15 Sep 2016 15:56:37 +0530
From:   Raju Lakkaraju <Raju.Lakkaraju@...rosemi.com>
To:     Andrew Lunn <andrew@...n.ch>
CC:     <netdev@...r.kernel.org>, <f.fainelli@...il.com>,
        <Allan.Nielsen@...rosemi.com>, <robh+dt@...nel.org>
Subject: Re: [PATCH v2 net-next 1/2] net: phy: Add Edge-rate driver for
 Microsemi PHYs.

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.