* Re: [PATCH net-next] microchipT1phy: Add driver for Microchip LAN87XX T1 PHYs
2018-04-25 18:49 [PATCH net-next] microchipT1phy: Add driver for Microchip LAN87XX T1 PHYs Nisar Sayed
@ 2018-04-25 14:21 ` Florian Fainelli
2018-04-26 7:02 ` Nisar.Sayed
0 siblings, 1 reply; 5+ messages in thread
From: Florian Fainelli @ 2018-04-25 14:21 UTC (permalink / raw)
To: Nisar Sayed, davem; +Cc: UNGLinuxDriver, netdev
On 04/25/2018 11:49 AM, Nisar Sayed wrote:
> Add driver for Microchip LAN87XX T1 PHYs
>
> This patch support driver for Microchp T1 PHYs.
> There will be followup patches to this driver to support T1 PHY
> features such as cable diagnostics, signal quality indicator(SQI),
> sleep and wakeup (TC10) support.
>
> Signed-off-by: Nisar Sayed <Nisar.Sayed@microchip.com>
> ---
> drivers/net/phy/Kconfig | 5 ++
> drivers/net/phy/Makefile | 1 +
> drivers/net/phy/microchipT1phy.c | 116 +++++++++++++++++++++++++++++++++++++++
> include/linux/microchipT1phy.h | 28 ++++++++++
> 4 files changed, 150 insertions(+)
> create mode 100644 drivers/net/phy/microchipT1phy.c
> create mode 100644 include/linux/microchipT1phy.h
>
> diff --git a/drivers/net/phy/Kconfig b/drivers/net/phy/Kconfig
> index bdfbabb..7b0b351 100644
> --- a/drivers/net/phy/Kconfig
> +++ b/drivers/net/phy/Kconfig
> @@ -354,6 +354,11 @@ config MICROCHIP_PHY
> help
> Supports the LAN88XX PHYs.
>
> +config MICROCHIP_T1_PHY
> + tristate "Microchip T1 PHYs"
> + ---help---
> + Supports the LAN87XX PHYs.
> +
> config MICROSEMI_PHY
> tristate "Microsemi PHYs"
> ---help---
> diff --git a/drivers/net/phy/Makefile b/drivers/net/phy/Makefile
> index 01acbcb..5706613 100644
> --- a/drivers/net/phy/Makefile
> +++ b/drivers/net/phy/Makefile
> @@ -70,6 +70,7 @@ obj-$(CONFIG_MESON_GXL_PHY) += meson-gxl.o
> obj-$(CONFIG_MICREL_KS8995MA) += spi_ks8995.o
> obj-$(CONFIG_MICREL_PHY) += micrel.o
> obj-$(CONFIG_MICROCHIP_PHY) += microchip.o
> +obj-$(CONFIG_MICROCHIP_T1_PHY) += microchipT1phy.o
> obj-$(CONFIG_MICROSEMI_PHY) += mscc.o
> obj-$(CONFIG_NATIONAL_PHY) += national.o
> obj-$(CONFIG_QSEMI_PHY) += qsemi.o
> diff --git a/drivers/net/phy/microchipT1phy.c b/drivers/net/phy/microchipT1phy.c
> new file mode 100644
> index 0000000..15fbb04
> --- /dev/null
> +++ b/drivers/net/phy/microchipT1phy.c
Can we avoid CamelCase file names? Any reasons why this is not part of
microchip.c?
> @@ -0,0 +1,116 @@
> +/*
> + * Copyright (C) 2018 Microchip Technology
Please use a SPDX license tag.
> + *
> + * 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/>.
> + */
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/mii.h>
> +#include <linux/phy.h>
> +#include <linux/microchipT1phy.h>
> +
> +#define DRIVER_AUTHOR "Nisar Sayed <nisar.sayed@microchip.com>"
> +#define DRIVER_DESC "Microchip LAN87XX T1 PHY driver"
> +
> +struct lan87xx_priv {
> + int chip_id;
> + int chip_rev;
> +};
> +
> +static int lan87xx_phy_config_intr(struct phy_device *phydev)
> +{
> + int rc = 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);
> + rc = phy_write(phydev, LAN87XX_INTERRUPT_MASK,
> + LAN87XX_MASK_LINK_UP | LAN87XX_MASK_LINK_DOWN);
> + } else {
> + rc = phy_write(phydev, LAN87XX_INTERRUPT_MASK, 0);
> + }
Since the last thing you do in both branches is write to
LAN87XX_INTERRUPT_MASK, just move that write outside of the branches and
make sure the value you will set is correct in both cases.
> +
> + 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 int lan87xx_probe(struct phy_device *phydev)
> +{
> + struct device *dev = &phydev->mdio.dev;
> + struct lan87xx_priv *priv;
> +
> + priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> + if (!priv)
> + return -ENOMEM;
> +
> + /* these values can be used to identify internal PHY */
> + priv->chip_id = phy_read(phydev, MII_PHYSID1);
> + priv->chip_rev = phy_read(phydev, MII_PHYSID2);
This is absolutely not necessary, phydev->phy_id stores that information
and you can use that at any point in your driver's lifetime. Besides,
the revision is not the full MII_PHYSID2, it's only the lower 4 bits of it.
Your probe() and remove() function then become unnecessary.
> +
> + phydev->priv = priv;
> +
> + return 0;
> +}
> +
> +static void lan87xx_remove(struct phy_device *phydev)
> +{
> + struct device *dev = &phydev->mdio.dev;
> + struct lan87xx_priv *priv = phydev->priv;
> +
> + if (priv)
> + devm_kfree(dev, priv);
> +}
> +
> +static struct phy_driver microchip_T1_phy_driver[] = {
> + {
> + .phy_id = 0x0007c150,
> + .phy_id_mask = 0xfffffff0,
> + .name = "Microchip LAN87xx",
> +
> + .features = SUPPORTED_100baseT_Full,
PHY_BASIC_FEATURES maybe? You cannot possibly be supporting just
100BaseT full duplex, 100BaseT half duplex, 10BaseT full and half duplex
are also supported, right?
> + .flags = PHY_HAS_INTERRUPT,
> +
> + .probe = lan87xx_probe,
> + .remove = lan87xx_remove,
> +
> + .config_init = genphy_config_init,
> + .config_aneg = genphy_config_aneg,
> +
> + .ack_interrupt = lan87xx_phy_ack_interrupt,
> + .config_intr = lan87xx_phy_config_intr,
> +
> + .suspend = genphy_suspend,
> + .resume = genphy_resume,
> + }
> +};
> +
> +module_phy_driver(microchip_T1_phy_driver);
> +
> +static struct mdio_device_id __maybe_unused microchip_T1_tbl[] = {
> + { 0x0007c150, 0xfffffff0 },
> + { }
> +};
> +
> +MODULE_DEVICE_TABLE(mdio, microchip_T1_tbl);
> +
> +MODULE_AUTHOR(DRIVER_AUTHOR);
> +MODULE_DESCRIPTION(DRIVER_DESC);
> +MODULE_LICENSE("GPL");
> diff --git a/include/linux/microchipT1phy.h b/include/linux/microchipT1phy.h
> new file mode 100644
> index 0000000..7353e7c
> --- /dev/null
> +++ b/include/linux/microchipT1phy.h
> @@ -0,0 +1,28 @@
> +/*
> + * Copyright (C) 2018 Microchip Technology
Same comment as before, please use SPDX license identifiers.
> + *
> + * 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/>.
> + */
> +#ifndef _MICROCHIPT1PHY_H_
> +#define _MICROCHIPT1PHY_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)
What's the point of that header file if all definitions are consumed by
the same driver?
> +
> +#endif /* _MICROCHIPT1PHY_H_ */
>
--
Florian
^ permalink raw reply [flat|nested] 5+ messages in thread
* [PATCH net-next] microchipT1phy: Add driver for Microchip LAN87XX T1 PHYs
@ 2018-04-25 18:49 Nisar Sayed
2018-04-25 14:21 ` Florian Fainelli
0 siblings, 1 reply; 5+ messages in thread
From: Nisar Sayed @ 2018-04-25 18:49 UTC (permalink / raw)
To: davem; +Cc: UNGLinuxDriver, netdev
Add driver for Microchip LAN87XX T1 PHYs
This patch support driver for Microchp T1 PHYs.
There will be followup patches to this driver to support T1 PHY
features such as cable diagnostics, signal quality indicator(SQI),
sleep and wakeup (TC10) support.
Signed-off-by: Nisar Sayed <Nisar.Sayed@microchip.com>
---
drivers/net/phy/Kconfig | 5 ++
drivers/net/phy/Makefile | 1 +
drivers/net/phy/microchipT1phy.c | 116 +++++++++++++++++++++++++++++++++++++++
include/linux/microchipT1phy.h | 28 ++++++++++
4 files changed, 150 insertions(+)
create mode 100644 drivers/net/phy/microchipT1phy.c
create mode 100644 include/linux/microchipT1phy.h
diff --git a/drivers/net/phy/Kconfig b/drivers/net/phy/Kconfig
index bdfbabb..7b0b351 100644
--- a/drivers/net/phy/Kconfig
+++ b/drivers/net/phy/Kconfig
@@ -354,6 +354,11 @@ config MICROCHIP_PHY
help
Supports the LAN88XX PHYs.
+config MICROCHIP_T1_PHY
+ tristate "Microchip T1 PHYs"
+ ---help---
+ Supports the LAN87XX PHYs.
+
config MICROSEMI_PHY
tristate "Microsemi PHYs"
---help---
diff --git a/drivers/net/phy/Makefile b/drivers/net/phy/Makefile
index 01acbcb..5706613 100644
--- a/drivers/net/phy/Makefile
+++ b/drivers/net/phy/Makefile
@@ -70,6 +70,7 @@ obj-$(CONFIG_MESON_GXL_PHY) += meson-gxl.o
obj-$(CONFIG_MICREL_KS8995MA) += spi_ks8995.o
obj-$(CONFIG_MICREL_PHY) += micrel.o
obj-$(CONFIG_MICROCHIP_PHY) += microchip.o
+obj-$(CONFIG_MICROCHIP_T1_PHY) += microchipT1phy.o
obj-$(CONFIG_MICROSEMI_PHY) += mscc.o
obj-$(CONFIG_NATIONAL_PHY) += national.o
obj-$(CONFIG_QSEMI_PHY) += qsemi.o
diff --git a/drivers/net/phy/microchipT1phy.c b/drivers/net/phy/microchipT1phy.c
new file mode 100644
index 0000000..15fbb04
--- /dev/null
+++ b/drivers/net/phy/microchipT1phy.c
@@ -0,0 +1,116 @@
+/*
+ * 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/>.
+ */
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/mii.h>
+#include <linux/phy.h>
+#include <linux/microchipT1phy.h>
+
+#define DRIVER_AUTHOR "Nisar Sayed <nisar.sayed@microchip.com>"
+#define DRIVER_DESC "Microchip LAN87XX T1 PHY driver"
+
+struct lan87xx_priv {
+ int chip_id;
+ int chip_rev;
+};
+
+static int lan87xx_phy_config_intr(struct phy_device *phydev)
+{
+ int rc = 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);
+ rc = phy_write(phydev, LAN87XX_INTERRUPT_MASK,
+ LAN87XX_MASK_LINK_UP | LAN87XX_MASK_LINK_DOWN);
+ } else {
+ rc = phy_write(phydev, LAN87XX_INTERRUPT_MASK, 0);
+ }
+
+ 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 int lan87xx_probe(struct phy_device *phydev)
+{
+ struct device *dev = &phydev->mdio.dev;
+ struct lan87xx_priv *priv;
+
+ priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
+ if (!priv)
+ return -ENOMEM;
+
+ /* these values can be used to identify internal PHY */
+ priv->chip_id = phy_read(phydev, MII_PHYSID1);
+ priv->chip_rev = phy_read(phydev, MII_PHYSID2);
+
+ phydev->priv = priv;
+
+ return 0;
+}
+
+static void lan87xx_remove(struct phy_device *phydev)
+{
+ struct device *dev = &phydev->mdio.dev;
+ struct lan87xx_priv *priv = phydev->priv;
+
+ if (priv)
+ devm_kfree(dev, priv);
+}
+
+static struct phy_driver microchip_T1_phy_driver[] = {
+ {
+ .phy_id = 0x0007c150,
+ .phy_id_mask = 0xfffffff0,
+ .name = "Microchip LAN87xx",
+
+ .features = SUPPORTED_100baseT_Full,
+ .flags = PHY_HAS_INTERRUPT,
+
+ .probe = lan87xx_probe,
+ .remove = lan87xx_remove,
+
+ .config_init = genphy_config_init,
+ .config_aneg = genphy_config_aneg,
+
+ .ack_interrupt = lan87xx_phy_ack_interrupt,
+ .config_intr = lan87xx_phy_config_intr,
+
+ .suspend = genphy_suspend,
+ .resume = genphy_resume,
+ }
+};
+
+module_phy_driver(microchip_T1_phy_driver);
+
+static struct mdio_device_id __maybe_unused microchip_T1_tbl[] = {
+ { 0x0007c150, 0xfffffff0 },
+ { }
+};
+
+MODULE_DEVICE_TABLE(mdio, microchip_T1_tbl);
+
+MODULE_AUTHOR(DRIVER_AUTHOR);
+MODULE_DESCRIPTION(DRIVER_DESC);
+MODULE_LICENSE("GPL");
diff --git a/include/linux/microchipT1phy.h b/include/linux/microchipT1phy.h
new file mode 100644
index 0000000..7353e7c
--- /dev/null
+++ b/include/linux/microchipT1phy.h
@@ -0,0 +1,28 @@
+/*
+ * 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/>.
+ */
+#ifndef _MICROCHIPT1PHY_H_
+#define _MICROCHIPT1PHY_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)
+
+#endif /* _MICROCHIPT1PHY_H_ */
--
2.14.1
^ permalink raw reply related [flat|nested] 5+ messages in thread
* RE: [PATCH net-next] microchipT1phy: Add driver for Microchip LAN87XX T1 PHYs
2018-04-25 14:21 ` Florian Fainelli
@ 2018-04-26 7:02 ` Nisar.Sayed
2018-04-26 12:27 ` Andrew Lunn
0 siblings, 1 reply; 5+ messages in thread
From: Nisar.Sayed @ 2018-04-26 7:02 UTC (permalink / raw)
To: f.fainelli, davem; +Cc: UNGLinuxDriver, netdev
Hi Florian
> On 04/25/2018 11:49 AM, Nisar Sayed wrote:
> > Add driver for Microchip LAN87XX T1 PHYs
> >
> > This patch support driver for Microchp T1 PHYs.
> > There will be followup patches to this driver to support T1 PHY
> > features such as cable diagnostics, signal quality indicator(SQI),
> > sleep and wakeup (TC10) support.
> >
> > Signed-off-by: Nisar Sayed <Nisar.Sayed@microchip.com>
> > ---
> > drivers/net/phy/Kconfig | 5 ++
> > drivers/net/phy/Makefile | 1 +
> > drivers/net/phy/microchipT1phy.c | 116
> +++++++++++++++++++++++++++++++++++++++
> > include/linux/microchipT1phy.h | 28 ++++++++++
> > 4 files changed, 150 insertions(+)
> > create mode 100644 drivers/net/phy/microchipT1phy.c create mode
> > 100644 include/linux/microchipT1phy.h
> >
> > diff --git a/drivers/net/phy/Kconfig b/drivers/net/phy/Kconfig index
> > bdfbabb..7b0b351 100644
> > --- a/drivers/net/phy/Kconfig
> > +++ b/drivers/net/phy/Kconfig
> > @@ -354,6 +354,11 @@ config MICROCHIP_PHY
> > help
> > Supports the LAN88XX PHYs.
> >
> > +config MICROCHIP_T1_PHY
> > + tristate "Microchip T1 PHYs"
> > + ---help---
> > + Supports the LAN87XX PHYs.
> > +
> > config MICROSEMI_PHY
> > tristate "Microsemi PHYs"
> > ---help---
> > diff --git a/drivers/net/phy/Makefile b/drivers/net/phy/Makefile index
> > 01acbcb..5706613 100644
> > --- a/drivers/net/phy/Makefile
> > +++ b/drivers/net/phy/Makefile
> > @@ -70,6 +70,7 @@ obj-$(CONFIG_MESON_GXL_PHY) += meson-
> gxl.o
> > obj-$(CONFIG_MICREL_KS8995MA) += spi_ks8995.o
> > obj-$(CONFIG_MICREL_PHY) += micrel.o
> > obj-$(CONFIG_MICROCHIP_PHY) += microchip.o
> > +obj-$(CONFIG_MICROCHIP_T1_PHY) += microchipT1phy.o
> > obj-$(CONFIG_MICROSEMI_PHY) += mscc.o
> > obj-$(CONFIG_NATIONAL_PHY) += national.o
> > obj-$(CONFIG_QSEMI_PHY) += qsemi.o
> > diff --git a/drivers/net/phy/microchipT1phy.c
> > b/drivers/net/phy/microchipT1phy.c
> > new file mode 100644
> > index 0000000..15fbb04
> > --- /dev/null
> > +++ b/drivers/net/phy/microchipT1phy.c
>
> Can we avoid CamelCase file names? Any reasons why this is not part of
> microchip.c?
>
Fine, will change the filename.
The reason for moving to separate file is that we have a series of T1 standard PHYs, which support cable diagnostics, signal quality indicator(SQI) and sleep and wakeup (TC10) support etc. we planned to keep all T1 standard PHYs separate to support additional features supported by these PHYs.
> > @@ -0,0 +1,116 @@
> > +/*
> > + * Copyright (C) 2018 Microchip Technology
>
> Please use a SPDX license tag.
>
Thanks, will update.
> > + *
> > + * 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/>.
> > + */
> > +#include <linux/kernel.h>
> > +#include <linux/module.h>
> > +#include <linux/mii.h>
> > +#include <linux/phy.h>
> > +#include <linux/microchipT1phy.h>
> > +
> > +#define DRIVER_AUTHOR "Nisar Sayed <nisar.sayed@microchip.com>"
> > +#define DRIVER_DESC "Microchip LAN87XX T1 PHY driver"
> > +
> > +struct lan87xx_priv {
> > + int chip_id;
> > + int chip_rev;
> > +};
> > +
> > +static int lan87xx_phy_config_intr(struct phy_device *phydev) {
> > + int rc = 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);
> > + rc = phy_write(phydev, LAN87XX_INTERRUPT_MASK,
> > + LAN87XX_MASK_LINK_UP |
> LAN87XX_MASK_LINK_DOWN);
> > + } else {
> > + rc = phy_write(phydev, LAN87XX_INTERRUPT_MASK, 0);
> > + }
>
> Since the last thing you do in both branches is write to
> LAN87XX_INTERRUPT_MASK, just move that write outside of the branches
> and make sure the value you will set is correct in both cases.
>
Thanks, Sure will modify as suggested.
> > +
> > + 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 int lan87xx_probe(struct phy_device *phydev) {
> > + struct device *dev = &phydev->mdio.dev;
> > + struct lan87xx_priv *priv;
> > +
> > + priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> > + if (!priv)
> > + return -ENOMEM;
> > +
> > + /* these values can be used to identify internal PHY */
> > + priv->chip_id = phy_read(phydev, MII_PHYSID1);
> > + priv->chip_rev = phy_read(phydev, MII_PHYSID2);
>
> This is absolutely not necessary, phydev->phy_id stores that information and
> you can use that at any point in your driver's lifetime. Besides, the revision is
> not the full MII_PHYSID2, it's only the lower 4 bits of it.
>
> Your probe() and remove() function then become unnecessary.
>
Thanks, will address in next version of submission.
> > +
> > + phydev->priv = priv;
> > +
> > + return 0;
> > +}
> > +
> > +static void lan87xx_remove(struct phy_device *phydev) {
> > + struct device *dev = &phydev->mdio.dev;
> > + struct lan87xx_priv *priv = phydev->priv;
> > +
> > + if (priv)
> > + devm_kfree(dev, priv);
> > +}
> > +
> > +static struct phy_driver microchip_T1_phy_driver[] = {
> > + {
> > + .phy_id = 0x0007c150,
> > + .phy_id_mask = 0xfffffff0,
> > + .name = "Microchip LAN87xx",
> > +
> > + .features = SUPPORTED_100baseT_Full,
>
> PHY_BASIC_FEATURES maybe? You cannot possibly be supporting just
> 100BaseT full duplex, 100BaseT half duplex, 10BaseT full and half duplex are
> also supported, right?
>
Yes, the current T1 PHY support only 100BaseT full duplex.
> > + .flags = PHY_HAS_INTERRUPT,
> > +
> > + .probe = lan87xx_probe,
> > + .remove = lan87xx_remove,
> > +
> > + .config_init = genphy_config_init,
> > + .config_aneg = genphy_config_aneg,
> > +
> > + .ack_interrupt = lan87xx_phy_ack_interrupt,
> > + .config_intr = lan87xx_phy_config_intr,
> > +
> > + .suspend = genphy_suspend,
> > + .resume = genphy_resume,
> > + }
> > +};
> > +
> > +module_phy_driver(microchip_T1_phy_driver);
> > +
> > +static struct mdio_device_id __maybe_unused microchip_T1_tbl[] = {
> > + { 0x0007c150, 0xfffffff0 },
> > + { }
> > +};
> > +
> > +MODULE_DEVICE_TABLE(mdio, microchip_T1_tbl);
> > +
> > +MODULE_AUTHOR(DRIVER_AUTHOR);
> > +MODULE_DESCRIPTION(DRIVER_DESC);
> > +MODULE_LICENSE("GPL");
> > diff --git a/include/linux/microchipT1phy.h
> > b/include/linux/microchipT1phy.h new file mode 100644 index
> > 0000000..7353e7c
> > --- /dev/null
> > +++ b/include/linux/microchipT1phy.h
> > @@ -0,0 +1,28 @@
> > +/*
> > + * Copyright (C) 2018 Microchip Technology
>
> Same comment as before, please use SPDX license identifiers.
>
Thanks, will update.
> > + *
> > + * 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/>.
> > + */
> > +#ifndef _MICROCHIPT1PHY_H_
> > +#define _MICROCHIPT1PHY_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)
>
> What's the point of that header file if all definitions are consumed by the
> same driver?
>
We have planned a series of patches where we planned to use this further.
- Nisar
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH net-next] microchipT1phy: Add driver for Microchip LAN87XX T1 PHYs
2018-04-26 7:02 ` Nisar.Sayed
@ 2018-04-26 12:27 ` Andrew Lunn
2018-04-26 17:24 ` Nisar.Sayed
0 siblings, 1 reply; 5+ messages in thread
From: Andrew Lunn @ 2018-04-26 12:27 UTC (permalink / raw)
To: Nisar.Sayed; +Cc: f.fainelli, davem, UNGLinuxDriver, netdev
> Fine, will change the filename.
> The reason for moving to separate file is that we have a series of
> T1 standard PHYs, which support cable diagnostics, signal quality
> indicator(SQI) and sleep and wakeup (TC10) support etc. we planned
> to keep all T1 standard PHYs separate to support additional features
> supported by these PHYs.
Is there anything shared with the other microchip PHYs? If there is
potential for code sharing, you should do it.
> > > + */
> > > +#ifndef _MICROCHIPT1PHY_H_
> > > +#define _MICROCHIPT1PHY_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)
> >
> > What's the point of that header file if all definitions are consumed by the
> > same driver?
> >
>
> We have planned a series of patches where we planned to use this further.
Are you adding multiple files which share the header? If not, just add
the defines to the C code.
Andrew
^ permalink raw reply [flat|nested] 5+ messages in thread
* RE: [PATCH net-next] microchipT1phy: Add driver for Microchip LAN87XX T1 PHYs
2018-04-26 12:27 ` Andrew Lunn
@ 2018-04-26 17:24 ` Nisar.Sayed
0 siblings, 0 replies; 5+ messages in thread
From: Nisar.Sayed @ 2018-04-26 17:24 UTC (permalink / raw)
To: andrew; +Cc: f.fainelli, davem, UNGLinuxDriver, netdev
Hi Andrew,
> > Fine, will change the filename.
>
> > The reason for moving to separate file is that we have a series of
> > T1 standard PHYs, which support cable diagnostics, signal quality
> > indicator(SQI) and sleep and wakeup (TC10) support etc. we planned to
> > keep all T1 standard PHYs separate to support additional features
> > supported by these PHYs.
>
> Is there anything shared with the other microchip PHYs? If there is potential
> for code sharing, you should do it.
Yes, there will be no code sharing between existing microchip PHYs and the newly getting added T1 phys.
>
> > > > + */
> > > > +#ifndef _MICROCHIPT1PHY_H_
> > > > +#define _MICROCHIPT1PHY_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)
> > >
> > > What's the point of that header file if all definitions are consumed
> > > by the same driver?
> > >
> >
> > We have planned a series of patches where we planned to use this further.
>
> Are you adding multiple files which share the header? If not, just add the
> defines to the C code.
>
> Andrew
We have a plan, I think as you suggested better to go with defines in C codes itself now.
Maybe we can create/move during future submissions.
- Nisar
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2018-04-26 17:24 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-04-25 18:49 [PATCH net-next] microchipT1phy: Add driver for Microchip LAN87XX T1 PHYs Nisar Sayed
2018-04-25 14:21 ` Florian Fainelli
2018-04-26 7:02 ` Nisar.Sayed
2018-04-26 12:27 ` Andrew Lunn
2018-04-26 17:24 ` Nisar.Sayed
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.