netdev - RE: [PATCH v1 net-next] microchip_t1: Add driver for Microchip LAN87XX T1 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] [day] [month] [year] [list]

Message-ID: <CE371C1263339941885964188A0225FA3B496F@CHN-SV-EXMX03.mchp-main.com>
Date:   Wed, 2 May 2018 10:14:18 +0000
From:   <Nisar.Sayed@...rochip.com>
To:     <f.fainelli@...il.com>, <davem@...emloft.net>
CC:     <UNGLinuxDriver@...rochip.com>, <netdev@...r.kernel.org>
Subject: RE: [PATCH v1 net-next] microchip_t1: Add driver for Microchip
 LAN87XX T1 PHYs

Hi Florian 

> > diff --git a/drivers/net/phy/microchip_t1.c
> > b/drivers/net/phy/microchip_t1.c new file mode 100644 index
> > 0000000..1f6f299
> > --- /dev/null
> > +++ b/drivers/net/phy/microchip_t1.c
> > @@ -0,0 +1,88 @@
> > +/* SPDX-License-Identifier: GPL-2.0 */
> 
> This is not the standard comment for a .c file, it should be: // (C++ style)
> 

Thanks, will update it.

> > +/*
> > + * Copyright (C) 2018 Microchip Technology
> > + *
> > + * This program is free software; you can redistribute it and/or
> > + * modify it under the terms of the GNU General Public License
> > + * as published by the Free Software Foundation; either version 2
> > + * of the License, or (at your option) any later version.
> > + *
> > + * This program is distributed in the hope that it will be useful,
> > + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> > + * GNU General Public License for more details.
> > + *
> > + * You should have received a copy of the GNU General Public License
> > + * along with this program; if not, see <http://www.gnu.org/licenses/>.
> 
> That needs to go away now that you used SPDX
> 

Ok fine will remove it.

> > + */
> > +#include <linux/kernel.h>
> > +#include <linux/module.h>
> > +#include <linux/mii.h>
> > +#include <linux/phy.h>
> > +
> > +/* Interrupt Source Register */
> > +#define LAN87XX_INTERRUPT_SOURCE                (0x18)
> > +
> > +/* Interrupt Mask Register */
> > +#define LAN87XX_INTERRUPT_MASK                  (0x19)
> > +#define LAN87XX_MASK_LINK_UP                    (0x0004)
> > +#define LAN87XX_MASK_LINK_DOWN                  (0x0002)
> > +
> > +#define DRIVER_AUTHOR	"Nisar Sayed <nisar.sayed@...rochip.com>"
> > +#define DRIVER_DESC	"Microchip LAN87XX T1 PHY driver"
> > +
> > +static int lan87xx_phy_config_intr(struct phy_device *phydev) {
> > +	int rc, val = 0;
> > +
> > +	if (phydev->interrupts == PHY_INTERRUPT_ENABLED) {
> > +		/* unmask all source and clear them before enable */
> > +		rc = phy_write(phydev, LAN87XX_INTERRUPT_MASK,
> 0x7FFF);
> > +		rc = phy_read(phydev, LAN87XX_INTERRUPT_SOURCE);
> > +		val = (LAN87XX_MASK_LINK_UP |
> LAN87XX_MASK_LINK_DOWN);
> 
> The parenthesis are not necessary here.

Thanks, will remove as suggested.

> 
> > +	}
> > +
> > +	rc = phy_write(phydev, LAN87XX_INTERRUPT_MASK, val);
> > +
> > +	return rc < 0 ? rc : 0;
> > +}
> > +
> > +static int lan87xx_phy_ack_interrupt(struct phy_device *phydev) {
> > +	int rc = phy_read(phydev, LAN87XX_INTERRUPT_SOURCE);
> > +
> > +	return rc < 0 ? rc : 0;
> > +}
> > +
> > +static struct phy_driver microchip_t1_phy_driver[] = {
> > +	{
> > +		.phy_id         = 0x0007c150,
> > +		.phy_id_mask    = 0xfffffff0,
> > +		.name           = "Microchip LAN87xx",
> 
> Would you want to name this "Microchip LAN87xx T1"?

Thanks, yes it's good to mention, will update and send v2

- Nisar