[PATCH V3] net: phy: tja11xx: Add TJA11xx PHY driver

[PATCH V3] net: phy: tja11xx: Add TJA11xx PHY driver

[Marek Vasut]

Add driver for the NXP TJA1100 and TJA1101 PHYs. These PHYs are special
BroadRReach 100BaseT1 PHYs used in automotive.

Signed-off-by: Marek Vasut <marex@denx.de>
Cc: Andrew Lunn <andrew@lunn.ch>
Cc: Florian Fainelli <f.fainelli@gmail.com>
Cc: Heiner Kallweit <hkallweit1@gmail.com>
---
V2: - Use phy_modify(), phy_{set,clear}_bits()
    - Drop enable argument of tja11xx_enable_link_control()
    - Use PHY_BASIC_T1_FEATURES and dont modify supported/advertised
      features in config_init callback
    - Use genphy_soft_reset() instead of opencoding the reset sequence.
    - Drop the aneg parts, since the PHY datasheet claims it does not
      support aneg
V3: - Replace clr with mask
    - Add hwmon support
    - Check commstat in tja11xx_read_status() only if link is up
    - Use PHY_ID_MATCH_MODEL()
---
 drivers/net/phy/Kconfig       |   6 +
 drivers/net/phy/Makefile      |   1 +
 drivers/net/phy/nxp-tja11xx.c | 424 ++++++++++++++++++++++++++++++++++
 3 files changed, 431 insertions(+)
 create mode 100644 drivers/net/phy/nxp-tja11xx.c

diff --git a/drivers/net/phy/Kconfig b/drivers/net/phy/Kconfig
index 3d187cd50eb0..88b462ef1274 100644
--- a/drivers/net/phy/Kconfig
+++ b/drivers/net/phy/Kconfig
@@ -389,6 +389,12 @@ config NATIONAL_PHY
 	---help---
 	  Currently supports the DP83865 PHY.
 
+config NXP_TJA11XX_PHY
+	tristate "NXP TJA11xx PHYs support"
+	depends on HWMON
+	---help---
+	  Currently supports the NXP TJA1100 and TJA1101 PHY.
+
 config QSEMI_PHY
 	tristate "Quality Semiconductor PHYs"
 	---help---
diff --git a/drivers/net/phy/Makefile b/drivers/net/phy/Makefile
index 5805c0b7d60e..023e5db8c7cc 100644
--- a/drivers/net/phy/Makefile
+++ b/drivers/net/phy/Makefile
@@ -76,6 +76,7 @@ obj-$(CONFIG_MICROCHIP_PHY)	+= microchip.o
 obj-$(CONFIG_MICROCHIP_T1_PHY)	+= microchip_t1.o
 obj-$(CONFIG_MICROSEMI_PHY)	+= mscc.o
 obj-$(CONFIG_NATIONAL_PHY)	+= national.o
+obj-$(CONFIG_NXP_TJA11XX_PHY)	+= nxp-tja11xx.o
 obj-$(CONFIG_QSEMI_PHY)		+= qsemi.o
 obj-$(CONFIG_REALTEK_PHY)	+= realtek.o
 obj-$(CONFIG_RENESAS_PHY)	+= uPD60620.o
diff --git a/drivers/net/phy/nxp-tja11xx.c b/drivers/net/phy/nxp-tja11xx.c
new file mode 100644
index 000000000000..3b8d0917e2b0
--- /dev/null
+++ b/drivers/net/phy/nxp-tja11xx.c
@@ -0,0 +1,424 @@
+// SPDX-License-Identifier: GPL-2.0
+/* NXP TJA1100 BroadRReach PHY driver
+ *
+ * Copyright (C) 2018 Marek Vasut <marex@denx.de>
+ */
+#include <linux/delay.h>
+#include <linux/ethtool.h>
+#include <linux/kernel.h>
+#include <linux/mii.h>
+#include <linux/module.h>
+#include <linux/phy.h>
+#include <linux/hwmon.h>
+
+#define PHY_ID_MASK			0xfffffff0
+#define PHY_ID_TJA1100			0x0180dc40
+#define PHY_ID_TJA1101			0x0180dd00
+
+#define MII_ECTRL			17
+#define MII_ECTRL_LINK_CONTROL		BIT(15)
+#define MII_ECTRL_POWER_MODE_MASK	GENMASK(14, 11)
+#define MII_ECTRL_POWER_MODE_NO_CHANGE	(0x0 << 11)
+#define MII_ECTRL_POWER_MODE_NORMAL	(0x3 << 11)
+#define MII_ECTRL_POWER_MODE_STANDBY	(0xc << 11)
+#define MII_ECTRL_CONFIG_EN		BIT(2)
+#define MII_ECTRL_WAKE_REQUEST		BIT(0)
+
+#define MII_CFG1			18
+#define MII_CFG1_AUTO_OP		BIT(14)
+#define MII_CFG1_SLEEP_CONFIRM		BIT(6)
+#define MII_CFG1_LED_MODE_MASK		GENMASK(5, 4)
+#define MII_CFG1_LED_MODE_LINKUP	0
+#define MII_CFG1_LED_ENABLE		BIT(3)
+
+#define MII_CFG2			19
+#define MII_CFG2_SLEEP_REQUEST_TO	GENMASK(1, 0)
+#define MII_CFG2_SLEEP_REQUEST_TO_16MS	0x3
+
+#define MII_INTSRC			21
+#define MII_INTSRC_TEMP_ERR		BIT(1)
+#define MII_INTSRC_UV_ERR		BIT(3)
+
+#define MII_COMMSTAT			23
+#define MII_COMMSTAT_LINK_UP		BIT(15)
+
+#define MII_GENSTAT			24
+#define MII_GENSTAT_PLL_LOCKED		BIT(14)
+
+#define MII_COMMCFG			27
+#define MII_COMMCFG_AUTO_OP		BIT(15)
+
+struct tja11xx_priv {
+	char		*hwmon_name;
+	struct device	*hwmon_dev;
+};
+
+struct tja11xx_phy_stats {
+	const char	*string;
+	u8		reg;
+	u8		off;
+	u16		mask;
+};
+
+static struct tja11xx_phy_stats tja11xx_hw_stats[] = {
+	{ "phy_symbol_error_count", 20, 0, 0xffff },
+	{ "phy_polarity_detect", 25, 6, BIT(6) },
+	{ "phy_open_detect", 25, 7, BIT(7) },
+	{ "phy_short_detect", 25, 8, BIT(8) },
+	{ "phy_rem_rcvr_count", 26, 0, 0xff },
+	{ "phy_loc_rcvr_count", 26, 8, 0xff },
+};
+
+static int tja11xx_check(struct phy_device *phydev, u8 reg, u16 mask, u16 set)
+{
+	int i, ret;
+
+	for (i = 0; i < 200; i++) {
+		ret = phy_read(phydev, reg);
+		if (ret < 0)
+			return ret;
+
+		if ((ret & mask) == set)
+			return 0;
+
+		usleep_range(100, 150);
+	}
+
+	return -ETIMEDOUT;
+}
+
+static int phy_modify_check(struct phy_device *phydev, u8 reg,
+			    u16 mask, u16 set)
+{
+	int ret;
+
+	ret = phy_modify(phydev, reg, mask, set);
+	if (ret)
+		return ret;
+
+	return tja11xx_check(phydev, reg, mask, set);
+}
+
+static int tja11xx_enable_reg_write(struct phy_device *phydev)
+{
+	return phy_set_bits(phydev, MII_ECTRL, MII_ECTRL_CONFIG_EN);
+}
+
+static int tja11xx_enable_link_control(struct phy_device *phydev)
+{
+	return phy_set_bits(phydev, MII_ECTRL, MII_ECTRL_LINK_CONTROL);
+}
+
+static int tja11xx_wakeup(struct phy_device *phydev)
+{
+	int ret;
+
+	ret = phy_read(phydev, MII_ECTRL);
+	if (ret < 0)
+		return ret;
+
+	switch (ret & MII_ECTRL_POWER_MODE_MASK) {
+	case MII_ECTRL_POWER_MODE_NO_CHANGE:
+		break;
+	case MII_ECTRL_POWER_MODE_NORMAL:
+		ret = phy_set_bits(phydev, MII_ECTRL, MII_ECTRL_WAKE_REQUEST);
+		if (ret)
+			return ret;
+
+		ret = phy_clear_bits(phydev, MII_ECTRL, MII_ECTRL_WAKE_REQUEST);
+		if (ret)
+			return ret;
+		break;
+	case MII_ECTRL_POWER_MODE_STANDBY:
+		ret = phy_modify_check(phydev, MII_ECTRL,
+				       MII_ECTRL_POWER_MODE_MASK,
+				       MII_ECTRL_POWER_MODE_STANDBY);
+		if (ret)
+			return ret;
+
+		ret = phy_modify(phydev, MII_ECTRL, MII_ECTRL_POWER_MODE_MASK,
+				 MII_ECTRL_POWER_MODE_NORMAL);
+		if (ret)
+			return ret;
+
+		ret = phy_modify_check(phydev, MII_GENSTAT,
+				       MII_GENSTAT_PLL_LOCKED,
+				       MII_GENSTAT_PLL_LOCKED);
+		if (ret)
+			return ret;
+
+		return tja11xx_enable_link_control(phydev);
+	default:
+		break;
+	}
+
+	return 0;
+}
+
+static int tja11xx_soft_reset(struct phy_device *phydev)
+{
+	int ret;
+
+	ret = tja11xx_enable_reg_write(phydev);
+	if (ret)
+		return ret;
+
+	return genphy_soft_reset(phydev);
+}
+
+static int tja11xx_config_init(struct phy_device *phydev)
+{
+	int ret;
+
+	ret = tja11xx_enable_reg_write(phydev);
+	if (ret)
+		return ret;
+
+	phydev->irq = PHY_POLL;
+	phydev->autoneg = AUTONEG_DISABLE;
+	phydev->speed = SPEED_100;
+	phydev->duplex = DUPLEX_FULL;
+	phydev->pause = 0;
+	phydev->asym_pause = 0;
+
+	switch (phydev->phy_id & PHY_ID_MASK) {
+	case PHY_ID_TJA1100:
+		ret = phy_modify(phydev, MII_CFG1,
+				 MII_CFG1_AUTO_OP | MII_CFG1_LED_MODE_MASK |
+				 MII_CFG1_LED_ENABLE,
+				 MII_CFG1_AUTO_OP | MII_CFG1_LED_MODE_LINKUP |
+				 MII_CFG1_LED_ENABLE);
+		if (ret)
+			return ret;
+		break;
+	case PHY_ID_TJA1101:
+		ret = phy_set_bits(phydev, MII_COMMCFG, MII_COMMCFG_AUTO_OP);
+		if (ret)
+			return ret;
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	ret = phy_clear_bits(phydev, MII_CFG1, MII_CFG1_SLEEP_CONFIRM);
+	if (ret)
+		return ret;
+
+	ret = phy_modify(phydev, MII_CFG2, MII_CFG2_SLEEP_REQUEST_TO,
+			 MII_CFG2_SLEEP_REQUEST_TO_16MS);
+	if (ret)
+		return ret;
+
+	ret = tja11xx_wakeup(phydev);
+	if (ret < 0)
+		return ret;
+
+	/* ACK interrupts by reading the status register */
+	ret = phy_read(phydev, MII_INTSRC);
+	if (ret < 0)
+		return ret;
+
+	return 0;
+}
+
+static int tja11xx_read_status(struct phy_device *phydev)
+{
+	int ret;
+
+	ret = genphy_update_link(phydev);
+	if (ret)
+		return ret;
+
+	if (phydev->link) {
+		ret = phy_read(phydev, MII_COMMSTAT);
+		if (ret < 0)
+			return ret;
+
+		if (!(ret & MII_COMMSTAT_LINK_UP))
+			phydev->link = 0;
+	}
+
+	return 0;
+}
+
+static int tja11xx_get_sset_count(struct phy_device *phydev)
+{
+	return ARRAY_SIZE(tja11xx_hw_stats);
+}
+
+static void tja11xx_get_strings(struct phy_device *phydev, u8 *data)
+{
+	int i;
+
+	for (i = 0; i < ARRAY_SIZE(tja11xx_hw_stats); i++) {
+		strncpy(data + i * ETH_GSTRING_LEN,
+			tja11xx_hw_stats[i].string, ETH_GSTRING_LEN);
+	}
+}
+
+static void tja11xx_get_stats(struct phy_device *phydev,
+			      struct ethtool_stats *stats, u64 *data)
+{
+	int i, ret;
+
+	for (i = 0; i < ARRAY_SIZE(tja11xx_hw_stats); i++) {
+		ret = phy_read(phydev, tja11xx_hw_stats[i].reg);
+		if (ret < 0) {
+			data[i] = U64_MAX;
+		} else {
+			data[i] = ret & tja11xx_hw_stats[i].mask;
+			data[i] >>= tja11xx_hw_stats[i].off;
+		}
+	}
+}
+
+static int tja11xx_hwmon_read(struct device *dev,
+			      enum hwmon_sensor_types type,
+			      u32 attr, int channel, long *value)
+{
+	struct phy_device *phydev = dev_get_drvdata(dev);
+	int ret;
+
+	switch (attr) {
+	case hwmon_in_lcrit_alarm:
+		ret = phy_read(phydev, MII_INTSRC);
+		if (ret < 0)
+			return ret;
+
+		*value = !!(ret & MII_INTSRC_TEMP_ERR);
+		return 0;
+	case hwmon_temp_crit_alarm:
+		ret = phy_read(phydev, MII_INTSRC);
+		if (ret < 0)
+			return ret;
+
+		*value = !!(ret & MII_INTSRC_TEMP_ERR);
+		return 0;
+	default:
+		return -EOPNOTSUPP;
+	}
+}
+
+static umode_t tja11xx_hwmon_is_visible(const void *data,
+					enum hwmon_sensor_types type,
+					u32 attr, int channel)
+{
+	if (type == hwmon_in && attr == hwmon_in_lcrit_alarm)
+		return 0444;
+
+	if (type == hwmon_temp && attr == hwmon_temp_crit_alarm)
+		return 0444;
+
+	return 0;
+}
+
+static u32 tja11xx_hwmon_in_config[] = {
+	HWMON_I_LCRIT_ALARM,
+	0
+};
+
+static const struct hwmon_channel_info tja11xx_hwmon_in = {
+	.type		= hwmon_in,
+	.config		= tja11xx_hwmon_in_config,
+};
+
+static u32 tja11xx_hwmon_temp_config[] = {
+	HWMON_T_CRIT_ALARM,
+	0
+};
+
+static const struct hwmon_channel_info tja11xx_hwmon_temp = {
+	.type		= hwmon_temp,
+	.config		= tja11xx_hwmon_temp_config,
+};
+
+static const struct hwmon_channel_info *tja11xx_hwmon_info[] = {
+	&tja11xx_hwmon_in,
+	&tja11xx_hwmon_temp,
+	NULL
+};
+
+static const struct hwmon_ops tja11xx_hwmon_hwmon_ops = {
+	.is_visible	= tja11xx_hwmon_is_visible,
+	.read		= tja11xx_hwmon_read,
+};
+
+static const struct hwmon_chip_info tja11xx_hwmon_chip_info = {
+	.ops		= &tja11xx_hwmon_hwmon_ops,
+	.info		= tja11xx_hwmon_info,
+};
+
+static int tja11xx_probe(struct phy_device *phydev)
+{
+	struct device *dev = &phydev->mdio.dev;
+	struct tja11xx_priv *priv;
+	int i;
+
+	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
+	if (!priv)
+		return -ENOMEM;
+
+	priv->hwmon_name = devm_kstrdup(dev, dev_name(dev), GFP_KERNEL);
+	if (!priv->hwmon_name)
+		return -ENODEV;
+
+	for (i = 0; priv->hwmon_name[i]; i++)
+		if (hwmon_is_bad_char(priv->hwmon_name[i]))
+			priv->hwmon_name[i] = '_';
+
+	priv->hwmon_dev =
+		devm_hwmon_device_register_with_info(dev, priv->hwmon_name,
+						     phydev,
+						     &tja11xx_hwmon_chip_info,
+						     NULL);
+
+	return PTR_ERR_OR_ZERO(priv->hwmon_dev);
+}
+
+static struct phy_driver tja11xx_driver[] = {
+	{
+		PHY_ID_MATCH_MODEL(PHY_ID_TJA1100),
+		.name		= "NXP TJA1100",
+		.features       = PHY_BASIC_T1_FEATURES,
+		.probe		= tja11xx_probe,
+		.soft_reset	= tja11xx_soft_reset,
+		.config_init	= tja11xx_config_init,
+		.read_status	= tja11xx_read_status,
+		.suspend	= genphy_suspend,
+		.resume		= genphy_resume,
+		.set_loopback   = genphy_loopback,
+		/* Statistics */
+		.get_sset_count = tja11xx_get_sset_count,
+		.get_strings	= tja11xx_get_strings,
+		.get_stats	= tja11xx_get_stats,
+	}, {
+		PHY_ID_MATCH_MODEL(PHY_ID_TJA1101),
+		.name		= "NXP TJA1101",
+		.features       = PHY_BASIC_T1_FEATURES,
+		.probe		= tja11xx_probe,
+		.soft_reset	= tja11xx_soft_reset,
+		.config_init	= tja11xx_config_init,
+		.read_status	= tja11xx_read_status,
+		.suspend	= genphy_suspend,
+		.resume		= genphy_resume,
+		.set_loopback   = genphy_loopback,
+		/* Statistics */
+		.get_sset_count = tja11xx_get_sset_count,
+		.get_strings	= tja11xx_get_strings,
+		.get_stats	= tja11xx_get_stats,
+	}
+};
+
+module_phy_driver(tja11xx_driver);
+
+static struct mdio_device_id __maybe_unused tja11xx_tbl[] = {
+	{ PHY_ID_MATCH_MODEL(PHY_ID_TJA1100) },
+	{ PHY_ID_MATCH_MODEL(PHY_ID_TJA1101) },
+	{ }
+};
+
+MODULE_DEVICE_TABLE(mdio, tja11xx_tbl);
+
+MODULE_AUTHOR("Marek Vasut <marex@denx.de>");
+MODULE_DESCRIPTION("NXP TJA11xx BoardR-Reach PHY driver");
+MODULE_LICENSE("GPL");
-- 
2.19.2

Re: [PATCH V3] net: phy: tja11xx: Add TJA11xx PHY driver

[Heiner Kallweit]

On 22.12.2018 00:35, Marek Vasut wrote:
> Add driver for the NXP TJA1100 and TJA1101 PHYs. These PHYs are special
> BroadRReach 100BaseT1 PHYs used in automotive.
> 
> Signed-off-by: Marek Vasut <marex@denx.de>
> Cc: Andrew Lunn <andrew@lunn.ch>
> Cc: Florian Fainelli <f.fainelli@gmail.com>
> Cc: Heiner Kallweit <hkallweit1@gmail.com>
> ---
> V2: - Use phy_modify(), phy_{set,clear}_bits()
>     - Drop enable argument of tja11xx_enable_link_control()
>     - Use PHY_BASIC_T1_FEATURES and dont modify supported/advertised
>       features in config_init callback
>     - Use genphy_soft_reset() instead of opencoding the reset sequence.
>     - Drop the aneg parts, since the PHY datasheet claims it does not
>       support aneg
> V3: - Replace clr with mask
>     - Add hwmon support
>     - Check commstat in tja11xx_read_status() only if link is up
>     - Use PHY_ID_MATCH_MODEL()
> ---
>  drivers/net/phy/Kconfig       |   6 +
>  drivers/net/phy/Makefile      |   1 +
>  drivers/net/phy/nxp-tja11xx.c | 424 ++++++++++++++++++++++++++++++++++
>  3 files changed, 431 insertions(+)
>  create mode 100644 drivers/net/phy/nxp-tja11xx.c
> 
[...]
> +
> +struct tja11xx_phy_stats {
> +	const char	*string;
> +	u8		reg;
> +	u8		off;
> +	u16		mask;
> +};
> +
As written in my other mail, you could think of using
FIELD_GET() again. Things like
... n, BIT(n), 
... m, BIT(m), 
are simply redundant.

> +static struct tja11xx_phy_stats tja11xx_hw_stats[] = {
> +	{ "phy_symbol_error_count", 20, 0, 0xffff },
> +	{ "phy_polarity_detect", 25, 6, BIT(6) },
> +	{ "phy_open_detect", 25, 7, BIT(7) },
> +	{ "phy_short_detect", 25, 8, BIT(8) },
> +	{ "phy_rem_rcvr_count", 26, 0, 0xff },
> +	{ "phy_loc_rcvr_count", 26, 8, 0xff },

Shouldn't mask in the last line be 0xff00 ?
In the relevant code you do: val = (reg & mask) >> off

> +static int tja11xx_probe(struct phy_device *phydev)
> +{
> +	struct device *dev = &phydev->mdio.dev;
> +	struct tja11xx_priv *priv;
> +	int i;
> +
> +	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> +	if (!priv)
> +		return -ENOMEM;
> +
> +	priv->hwmon_name = devm_kstrdup(dev, dev_name(dev), GFP_KERNEL);
> +	if (!priv->hwmon_name)
> +		return -ENODEV;

Do you really need to make a copy of the device name?
Why not simply priv->hwmon_name = dev_name(dev) ?
And if devm_kstrdup fails, then most likely you have an out-of-memory
error, so why not return -ENOMEM as usual?

> +
> +	for (i = 0; priv->hwmon_name[i]; i++)
> +		if (hwmon_is_bad_char(priv->hwmon_name[i]))
> +			priv->hwmon_name[i] = '_';
> +
> +	priv->hwmon_dev =
> +		devm_hwmon_device_register_with_info(dev, priv->hwmon_name,
> +						     phydev,
> +						     &tja11xx_hwmon_chip_info,
> +						     NULL);
> +
Prerequisite for this call is that HWMON is configured in the kernel and
it's reachable. Something like "IS_REACHABLE(CONFIG_HWMON)" would be
needed. You can see driver rtc-ds1307 for an example.

Re: [PATCH V3] net: phy: tja11xx: Add TJA11xx PHY driver

[Heiner Kallweit]

On 22.12.2018 00:35, Marek Vasut wrote:
> Add driver for the NXP TJA1100 and TJA1101 PHYs. These PHYs are special
> BroadRReach 100BaseT1 PHYs used in automotive.
> 
> Signed-off-by: Marek Vasut <marex@denx.de>
> Cc: Andrew Lunn <andrew@lunn.ch>
> Cc: Florian Fainelli <f.fainelli@gmail.com>
> Cc: Heiner Kallweit <hkallweit1@gmail.com>
> ---
> V2: - Use phy_modify(), phy_{set,clear}_bits()
>     - Drop enable argument of tja11xx_enable_link_control()
>     - Use PHY_BASIC_T1_FEATURES and dont modify supported/advertised
>       features in config_init callback
>     - Use genphy_soft_reset() instead of opencoding the reset sequence.
>     - Drop the aneg parts, since the PHY datasheet claims it does not
>       support aneg
> V3: - Replace clr with mask
>     - Add hwmon support
>     - Check commstat in tja11xx_read_status() only if link is up
>     - Use PHY_ID_MATCH_MODEL()
> ---
>  drivers/net/phy/Kconfig       |   6 +
>  drivers/net/phy/Makefile      |   1 +
>  drivers/net/phy/nxp-tja11xx.c | 424 ++++++++++++++++++++++++++++++++++
>  3 files changed, 431 insertions(+)
>  create mode 100644 drivers/net/phy/nxp-tja11xx.c
[...]
> +static int tja11xx_hwmon_read(struct device *dev,
> +			      enum hwmon_sensor_types type,
> +			      u32 attr, int channel, long *value)
> +{
> +	struct phy_device *phydev = dev_get_drvdata(dev);
> +	int ret;
> +
> +	switch (attr) {
> +	case hwmon_in_lcrit_alarm:
> +		ret = phy_read(phydev, MII_INTSRC);
> +		if (ret < 0)
> +			return ret;
> +
> +		*value = !!(ret & MII_INTSRC_TEMP_ERR);
> +		return 0;
> +	case hwmon_temp_crit_alarm:
> +		ret = phy_read(phydev, MII_INTSRC);
> +		if (ret < 0)
> +			return ret;
> +
> +		*value = !!(ret & MII_INTSRC_TEMP_ERR);
> +		return 0;
> +	default:
> +		return -EOPNOTSUPP;
> +	}
> +}
This looks like a copy & paste error, in both cases you're doing the same.

Re: [PATCH V3] net: phy: tja11xx: Add TJA11xx PHY driver

[Marek Vasut]

On 12/22/18 9:51 PM, Heiner Kallweit wrote:
> On 22.12.2018 00:35, Marek Vasut wrote:
>> Add driver for the NXP TJA1100 and TJA1101 PHYs. These PHYs are special
>> BroadRReach 100BaseT1 PHYs used in automotive.
>>
>> Signed-off-by: Marek Vasut <marex@denx.de>
>> Cc: Andrew Lunn <andrew@lunn.ch>
>> Cc: Florian Fainelli <f.fainelli@gmail.com>
>> Cc: Heiner Kallweit <hkallweit1@gmail.com>
>> ---
>> V2: - Use phy_modify(), phy_{set,clear}_bits()
>>     - Drop enable argument of tja11xx_enable_link_control()
>>     - Use PHY_BASIC_T1_FEATURES and dont modify supported/advertised
>>       features in config_init callback
>>     - Use genphy_soft_reset() instead of opencoding the reset sequence.
>>     - Drop the aneg parts, since the PHY datasheet claims it does not
>>       support aneg
>> V3: - Replace clr with mask
>>     - Add hwmon support
>>     - Check commstat in tja11xx_read_status() only if link is up
>>     - Use PHY_ID_MATCH_MODEL()
>> ---
>>  drivers/net/phy/Kconfig       |   6 +
>>  drivers/net/phy/Makefile      |   1 +
>>  drivers/net/phy/nxp-tja11xx.c | 424 ++++++++++++++++++++++++++++++++++
>>  3 files changed, 431 insertions(+)
>>  create mode 100644 drivers/net/phy/nxp-tja11xx.c
> [...]
>> +static int tja11xx_hwmon_read(struct device *dev,
>> +			      enum hwmon_sensor_types type,
>> +			      u32 attr, int channel, long *value)
>> +{
>> +	struct phy_device *phydev = dev_get_drvdata(dev);
>> +	int ret;
>> +
>> +	switch (attr) {
>> +	case hwmon_in_lcrit_alarm:
>> +		ret = phy_read(phydev, MII_INTSRC);
>> +		if (ret < 0)
>> +			return ret;
>> +
>> +		*value = !!(ret & MII_INTSRC_TEMP_ERR);
>> +		return 0;
>> +	case hwmon_temp_crit_alarm:
>> +		ret = phy_read(phydev, MII_INTSRC);
>> +		if (ret < 0)
>> +			return ret;
>> +
>> +		*value = !!(ret & MII_INTSRC_TEMP_ERR);
>> +		return 0;
>> +	default:
>> +		return -EOPNOTSUPP;
>> +	}
>> +}
> This looks like a copy & paste error, in both cases you're doing the same.

Fixed, thanks. I'll send V4 once I can actually test this stuff.

-- 
Best regards,
Marek Vasut

Re: [PATCH V3] net: phy: tja11xx: Add TJA11xx PHY driver

[Marek Vasut]

On 12/22/18 6:39 PM, Heiner Kallweit wrote:
> On 22.12.2018 00:35, Marek Vasut wrote:
>> Add driver for the NXP TJA1100 and TJA1101 PHYs. These PHYs are special
>> BroadRReach 100BaseT1 PHYs used in automotive.
>>
>> Signed-off-by: Marek Vasut <marex@denx.de>
>> Cc: Andrew Lunn <andrew@lunn.ch>
>> Cc: Florian Fainelli <f.fainelli@gmail.com>
>> Cc: Heiner Kallweit <hkallweit1@gmail.com>
>> ---
>> V2: - Use phy_modify(), phy_{set,clear}_bits()
>>     - Drop enable argument of tja11xx_enable_link_control()
>>     - Use PHY_BASIC_T1_FEATURES and dont modify supported/advertised
>>       features in config_init callback
>>     - Use genphy_soft_reset() instead of opencoding the reset sequence.
>>     - Drop the aneg parts, since the PHY datasheet claims it does not
>>       support aneg
>> V3: - Replace clr with mask
>>     - Add hwmon support
>>     - Check commstat in tja11xx_read_status() only if link is up
>>     - Use PHY_ID_MATCH_MODEL()
>> ---
>>  drivers/net/phy/Kconfig       |   6 +
>>  drivers/net/phy/Makefile      |   1 +
>>  drivers/net/phy/nxp-tja11xx.c | 424 ++++++++++++++++++++++++++++++++++
>>  3 files changed, 431 insertions(+)
>>  create mode 100644 drivers/net/phy/nxp-tja11xx.c
>>
> [...]
>> +
>> +struct tja11xx_phy_stats {
>> +	const char	*string;
>> +	u8		reg;
>> +	u8		off;
>> +	u16		mask;
>> +};
>> +
> As written in my other mail, you could think of using
> FIELD_GET() again. Things like
> ... n, BIT(n), 
> ... m, BIT(m), 
> are simply redundant.

Done

>> +static struct tja11xx_phy_stats tja11xx_hw_stats[] = {
>> +	{ "phy_symbol_error_count", 20, 0, 0xffff },
>> +	{ "phy_polarity_detect", 25, 6, BIT(6) },
>> +	{ "phy_open_detect", 25, 7, BIT(7) },
>> +	{ "phy_short_detect", 25, 8, BIT(8) },
>> +	{ "phy_rem_rcvr_count", 26, 0, 0xff },
>> +	{ "phy_loc_rcvr_count", 26, 8, 0xff },
> 
> Shouldn't mask in the last line be 0xff00 ?
> In the relevant code you do: val = (reg & mask) >> off

Yes, fixed, thanks

>> +static int tja11xx_probe(struct phy_device *phydev)
>> +{
>> +	struct device *dev = &phydev->mdio.dev;
>> +	struct tja11xx_priv *priv;
>> +	int i;
>> +
>> +	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
>> +	if (!priv)
>> +		return -ENOMEM;
>> +
>> +	priv->hwmon_name = devm_kstrdup(dev, dev_name(dev), GFP_KERNEL);
>> +	if (!priv->hwmon_name)
>> +		return -ENODEV;
> 
> Do you really need to make a copy of the device name?
> Why not simply priv->hwmon_name = dev_name(dev) ?

Fine by me, but then maybe I don't quite understand why the other
drivers duplicate the name, eg. the sfp.c one.

> And if devm_kstrdup fails, then most likely you have an out-of-memory
> error, so why not return -ENOMEM as usual?

Fixed

>> +
>> +	for (i = 0; priv->hwmon_name[i]; i++)
>> +		if (hwmon_is_bad_char(priv->hwmon_name[i]))
>> +			priv->hwmon_name[i] = '_';
>> +
>> +	priv->hwmon_dev =
>> +		devm_hwmon_device_register_with_info(dev, priv->hwmon_name,
>> +						     phydev,
>> +						     &tja11xx_hwmon_chip_info,
>> +						     NULL);
>> +
> Prerequisite for this call is that HWMON is configured in the kernel and
> it's reachable. Something like "IS_REACHABLE(CONFIG_HWMON)" would be
> needed. You can see driver rtc-ds1307 for an example.

The driver depends on HWMON, so that should be sufficient ?

-- 
Best regards,
Marek Vasut

Re: [PATCH V3] net: phy: tja11xx: Add TJA11xx PHY driver

[Heiner Kallweit]

On 23.12.2018 10:16, Marek Vasut wrote:
> On 12/22/18 6:39 PM, Heiner Kallweit wrote:
>> On 22.12.2018 00:35, Marek Vasut wrote:
>>> Add driver for the NXP TJA1100 and TJA1101 PHYs. These PHYs are special
>>> BroadRReach 100BaseT1 PHYs used in automotive.
>>>
>>> Signed-off-by: Marek Vasut <marex@denx.de>
>>> Cc: Andrew Lunn <andrew@lunn.ch>
>>> Cc: Florian Fainelli <f.fainelli@gmail.com>
>>> Cc: Heiner Kallweit <hkallweit1@gmail.com>
>>> ---
>>> V2: - Use phy_modify(), phy_{set,clear}_bits()
>>>     - Drop enable argument of tja11xx_enable_link_control()
>>>     - Use PHY_BASIC_T1_FEATURES and dont modify supported/advertised
>>>       features in config_init callback
>>>     - Use genphy_soft_reset() instead of opencoding the reset sequence.
>>>     - Drop the aneg parts, since the PHY datasheet claims it does not
>>>       support aneg
>>> V3: - Replace clr with mask
>>>     - Add hwmon support
>>>     - Check commstat in tja11xx_read_status() only if link is up
>>>     - Use PHY_ID_MATCH_MODEL()
>>> ---
>>>  drivers/net/phy/Kconfig       |   6 +
>>>  drivers/net/phy/Makefile      |   1 +
>>>  drivers/net/phy/nxp-tja11xx.c | 424 ++++++++++++++++++++++++++++++++++
>>>  3 files changed, 431 insertions(+)
>>>  create mode 100644 drivers/net/phy/nxp-tja11xx.c
>>>
>> [...]
>>> +
>>> +struct tja11xx_phy_stats {
>>> +	const char	*string;
>>> +	u8		reg;
>>> +	u8		off;
>>> +	u16		mask;
>>> +};
>>> +
>> As written in my other mail, you could think of using
>> FIELD_GET() again. Things like
>> ... n, BIT(n), 
>> ... m, BIT(m), 
>> are simply redundant.
> 
> Done
> 
>>> +static struct tja11xx_phy_stats tja11xx_hw_stats[] = {
>>> +	{ "phy_symbol_error_count", 20, 0, 0xffff },
>>> +	{ "phy_polarity_detect", 25, 6, BIT(6) },
>>> +	{ "phy_open_detect", 25, 7, BIT(7) },
>>> +	{ "phy_short_detect", 25, 8, BIT(8) },
>>> +	{ "phy_rem_rcvr_count", 26, 0, 0xff },
>>> +	{ "phy_loc_rcvr_count", 26, 8, 0xff },
>>
>> Shouldn't mask in the last line be 0xff00 ?
>> In the relevant code you do: val = (reg & mask) >> off
> 
> Yes, fixed, thanks
> 
>>> +static int tja11xx_probe(struct phy_device *phydev)
>>> +{
>>> +	struct device *dev = &phydev->mdio.dev;
>>> +	struct tja11xx_priv *priv;
>>> +	int i;
>>> +
>>> +	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
>>> +	if (!priv)
>>> +		return -ENOMEM;
>>> +
>>> +	priv->hwmon_name = devm_kstrdup(dev, dev_name(dev), GFP_KERNEL);
>>> +	if (!priv->hwmon_name)
>>> +		return -ENODEV;
>>
>> Do you really need to make a copy of the device name?
>> Why not simply priv->hwmon_name = dev_name(dev) ?
> 
> Fine by me, but then maybe I don't quite understand why the other
> drivers duplicate the name, eg. the sfp.c one.
> 
It's a question of object lifetime. If the original object can go away
before your object, then you need to make a copy of the name.
However in our case I don't think priv can live longer than dev.

>> And if devm_kstrdup fails, then most likely you have an out-of-memory
>> error, so why not return -ENOMEM as usual?
> 
> Fixed
> 
>>> +
>>> +	for (i = 0; priv->hwmon_name[i]; i++)
>>> +		if (hwmon_is_bad_char(priv->hwmon_name[i]))
>>> +			priv->hwmon_name[i] = '_';
>>> +
>>> +	priv->hwmon_dev =
>>> +		devm_hwmon_device_register_with_info(dev, priv->hwmon_name,
>>> +						     phydev,
>>> +						     &tja11xx_hwmon_chip_info,
>>> +						     NULL);
>>> +
>> Prerequisite for this call is that HWMON is configured in the kernel and
>> it's reachable. Something like "IS_REACHABLE(CONFIG_HWMON)" would be
>> needed. You can see driver rtc-ds1307 for an example.
> 
> The driver depends on HWMON, so that should be sufficient ?
> 
Missed that, that's sufficient. Just something to think about:
Often HWMON is seen as an optional add-on feature. The driver itself would
work perfectly fine also w/o HWMON. In this case you don't want the hard
dependency. So it's up to you whether you want to allow that the driver is
used on systems w/o HWMON support.

Re: [PATCH V3] net: phy: tja11xx: Add TJA11xx PHY driver

[Andrew Lunn]

> >>> +	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> >>> +	if (!priv)
> >>> +		return -ENOMEM;
> >>> +
> >>> +	priv->hwmon_name = devm_kstrdup(dev, dev_name(dev), GFP_KERNEL);
> >>> +	if (!priv->hwmon_name)
> >>> +		return -ENODEV;
> >>
> >> Do you really need to make a copy of the device name?
> >> Why not simply priv->hwmon_name = dev_name(dev) ?
> > 
> > Fine by me, but then maybe I don't quite understand why the other
> > drivers duplicate the name, eg. the sfp.c one.
> > 
> It's a question of object lifetime. If the original object can go away
> before your object, then you need to make a copy of the name.
> However in our case I don't think priv can live longer than dev.
> 
> >> And if devm_kstrdup fails, then most likely you have an out-of-memory
> >> error, so why not return -ENOMEM as usual?
> > 
> > Fixed
> > 
> >>> +
> >>> +	for (i = 0; priv->hwmon_name[i]; i++)
> >>> +		if (hwmon_is_bad_char(priv->hwmon_name[i]))
> >>> +			priv->hwmon_name[i] = '_';

This is one reason to make a copy. You don't want to apply that to
main name of the device.

> >>> +
> >>> +	priv->hwmon_dev =
> >>> +		devm_hwmon_device_register_with_info(dev, priv->hwmon_name,
> >>> +						     phydev,
> >>> +						     &tja11xx_hwmon_chip_info,
> >>> +						     NULL);
> >>> +

The second reason is priv is released before dev, but what about
hwmon, especially if somebody has one of the files open? Is the
unregister synchronous?

	 Andrew

Re: [PATCH V3] net: phy: tja11xx: Add TJA11xx PHY driver

[Andrew Lunn]

> > +	switch (attr) {
> > +	case hwmon_in_lcrit_alarm:
> > +		ret = phy_read(phydev, MII_INTSRC);
> > +		if (ret < 0)
> > +			return ret;
> > +
> > +		*value = !!(ret & MII_INTSRC_TEMP_ERR);
> > +		return 0;
> > +	case hwmon_temp_crit_alarm:
> > +		ret = phy_read(phydev, MII_INTSRC);
> > +		if (ret < 0)
> > +			return ret;
> > +
> > +		*value = !!(ret & MII_INTSRC_TEMP_ERR);
> > +		return 0;
> > +	default:
> > +		return -EOPNOTSUPP;
> > +	}
> > +}
> This looks like a copy & paste error, in both cases you're doing the same.

You also should not do it like this. hwmon_temp_crit_alarm is in a
different set of enum's as hwmon_in_lcrit_alarm. hwmon_in_lcrit_alarm
= 14.  hwmon_temp_max_alarm also is 14. You should have two different
switch statements to take account of this.

       Andrew

Re: [PATCH V3] net: phy: tja11xx: Add TJA11xx PHY driver

[Marek Vasut]

On 12/23/18 10:41 AM, Heiner Kallweit wrote:
> On 23.12.2018 10:16, Marek Vasut wrote:
>> On 12/22/18 6:39 PM, Heiner Kallweit wrote:
>>> On 22.12.2018 00:35, Marek Vasut wrote:
>>>> Add driver for the NXP TJA1100 and TJA1101 PHYs. These PHYs are special
>>>> BroadRReach 100BaseT1 PHYs used in automotive.
>>>>
>>>> Signed-off-by: Marek Vasut <marex@denx.de>
>>>> Cc: Andrew Lunn <andrew@lunn.ch>
>>>> Cc: Florian Fainelli <f.fainelli@gmail.com>
>>>> Cc: Heiner Kallweit <hkallweit1@gmail.com>
>>>> ---
>>>> V2: - Use phy_modify(), phy_{set,clear}_bits()
>>>>     - Drop enable argument of tja11xx_enable_link_control()
>>>>     - Use PHY_BASIC_T1_FEATURES and dont modify supported/advertised
>>>>       features in config_init callback
>>>>     - Use genphy_soft_reset() instead of opencoding the reset sequence.
>>>>     - Drop the aneg parts, since the PHY datasheet claims it does not
>>>>       support aneg
>>>> V3: - Replace clr with mask
>>>>     - Add hwmon support
>>>>     - Check commstat in tja11xx_read_status() only if link is up
>>>>     - Use PHY_ID_MATCH_MODEL()
>>>> ---
>>>>  drivers/net/phy/Kconfig       |   6 +
>>>>  drivers/net/phy/Makefile      |   1 +
>>>>  drivers/net/phy/nxp-tja11xx.c | 424 ++++++++++++++++++++++++++++++++++
>>>>  3 files changed, 431 insertions(+)
>>>>  create mode 100644 drivers/net/phy/nxp-tja11xx.c
>>>>
>>> [...]
>>>> +
>>>> +struct tja11xx_phy_stats {
>>>> +	const char	*string;
>>>> +	u8		reg;
>>>> +	u8		off;
>>>> +	u16		mask;
>>>> +};
>>>> +
>>> As written in my other mail, you could think of using
>>> FIELD_GET() again. Things like
>>> ... n, BIT(n), 
>>> ... m, BIT(m), 
>>> are simply redundant.
>>
>> Done
>>
>>>> +static struct tja11xx_phy_stats tja11xx_hw_stats[] = {
>>>> +	{ "phy_symbol_error_count", 20, 0, 0xffff },
>>>> +	{ "phy_polarity_detect", 25, 6, BIT(6) },
>>>> +	{ "phy_open_detect", 25, 7, BIT(7) },
>>>> +	{ "phy_short_detect", 25, 8, BIT(8) },
>>>> +	{ "phy_rem_rcvr_count", 26, 0, 0xff },
>>>> +	{ "phy_loc_rcvr_count", 26, 8, 0xff },
>>>
>>> Shouldn't mask in the last line be 0xff00 ?
>>> In the relevant code you do: val = (reg & mask) >> off
>>
>> Yes, fixed, thanks
>>
>>>> +static int tja11xx_probe(struct phy_device *phydev)
>>>> +{
>>>> +	struct device *dev = &phydev->mdio.dev;
>>>> +	struct tja11xx_priv *priv;
>>>> +	int i;
>>>> +
>>>> +	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
>>>> +	if (!priv)
>>>> +		return -ENOMEM;
>>>> +
>>>> +	priv->hwmon_name = devm_kstrdup(dev, dev_name(dev), GFP_KERNEL);
>>>> +	if (!priv->hwmon_name)
>>>> +		return -ENODEV;
>>>
>>> Do you really need to make a copy of the device name?
>>> Why not simply priv->hwmon_name = dev_name(dev) ?
>>
>> Fine by me, but then maybe I don't quite understand why the other
>> drivers duplicate the name, eg. the sfp.c one.
>>
> It's a question of object lifetime. If the original object can go away
> before your object, then you need to make a copy of the name.
> However in our case I don't think priv can live longer than dev.
> 
>>> And if devm_kstrdup fails, then most likely you have an out-of-memory
>>> error, so why not return -ENOMEM as usual?
>>
>> Fixed
>>
>>>> +
>>>> +	for (i = 0; priv->hwmon_name[i]; i++)
>>>> +		if (hwmon_is_bad_char(priv->hwmon_name[i]))
>>>> +			priv->hwmon_name[i] = '_';
>>>> +
>>>> +	priv->hwmon_dev =
>>>> +		devm_hwmon_device_register_with_info(dev, priv->hwmon_name,
>>>> +						     phydev,
>>>> +						     &tja11xx_hwmon_chip_info,
>>>> +						     NULL);
>>>> +
>>> Prerequisite for this call is that HWMON is configured in the kernel and
>>> it's reachable. Something like "IS_REACHABLE(CONFIG_HWMON)" would be
>>> needed. You can see driver rtc-ds1307 for an example.
>>
>> The driver depends on HWMON, so that should be sufficient ?
>>
> Missed that, that's sufficient. Just something to think about:
> Often HWMON is seen as an optional add-on feature. The driver itself would
> work perfectly fine also w/o HWMON. In this case you don't want the hard
> dependency. So it's up to you whether you want to allow that the driver is
> used on systems w/o HWMON support.

Given that the HWMON indicates that the automotive device either
overheated or suffered undervolt, I presume it'd be safer not to make it
optional ?

-- 
Best regards,
Marek Vasut

Re: [PATCH V3] net: phy: tja11xx: Add TJA11xx PHY driver

[Andrew Lunn]

> Given that the HWMON indicates that the automotive device either
> overheated or suffered undervolt, I presume it'd be safer not to make it
> optional ?

Hi Marek

It is a niche device, where i expect a custom kernel configuration. Do
any of these devices exist on standard PCIe cards? Maybe a test PC
using a standard Linux Distribution? But then again, such
distributions turn on HWMON anyway.

      Andrew

Re: [PATCH V3] net: phy: tja11xx: Add TJA11xx PHY driver

[Marek Vasut]

On 12/23/18 11:35 AM, Andrew Lunn wrote:
>> Given that the HWMON indicates that the automotive device either
>> overheated or suffered undervolt, I presume it'd be safer not to make it
>> optional ?
> 
> Hi Marek
> 
> It is a niche device, where i expect a custom kernel configuration. Do
> any of these devices exist on standard PCIe cards? Maybe a test PC
> using a standard Linux Distribution? But then again, such
> distributions turn on HWMON anyway.

Not to my knowledge. I have two on various embedded systems and one on
SMSC95xx USB-ethernet device (although, the SMSC95xx driver needs to be
fixed to support that, cfr. the other thread).

-- 
Best regards,
Marek Vasut

Re: [PATCH V3] net: phy: tja11xx: Add TJA11xx PHY driver

[Marek Vasut]

On 12/23/18 11:06 AM, Andrew Lunn wrote:
>>> +	switch (attr) {
>>> +	case hwmon_in_lcrit_alarm:
>>> +		ret = phy_read(phydev, MII_INTSRC);
>>> +		if (ret < 0)
>>> +			return ret;
>>> +
>>> +		*value = !!(ret & MII_INTSRC_TEMP_ERR);
>>> +		return 0;
>>> +	case hwmon_temp_crit_alarm:
>>> +		ret = phy_read(phydev, MII_INTSRC);
>>> +		if (ret < 0)
>>> +			return ret;
>>> +
>>> +		*value = !!(ret & MII_INTSRC_TEMP_ERR);
>>> +		return 0;
>>> +	default:
>>> +		return -EOPNOTSUPP;
>>> +	}
>>> +}
>> This looks like a copy & paste error, in both cases you're doing the same.
> 
> You also should not do it like this. hwmon_temp_crit_alarm is in a
> different set of enum's as hwmon_in_lcrit_alarm. hwmon_in_lcrit_alarm
> = 14.  hwmon_temp_max_alarm also is 14. You should have two different
> switch statements to take account of this.

I can also use a simple conditional, since I don't expect the number of
HWMON properties to grow, eg.

if (type == hwmon_in && attr == hwmon_in_lcrit_alarm) {...}
if (type == hwmon_temp && attr == hwmon_temp_crit_alarm) {...}

I think that's a bit more readable.

-- 
Best regards,
Marek Vasut

Re: [PATCH V3] net: phy: tja11xx: Add TJA11xx PHY driver

[Andrew Lunn]

> I can also use a simple conditional, since I don't expect the number of
> HWMON properties to grow, eg.
> 
> if (type == hwmon_in && attr == hwmon_in_lcrit_alarm) {...}
> if (type == hwmon_temp && attr == hwmon_temp_crit_alarm) {...}

Yes, that is fine.

Please make sure you Cc: the HWMON maintainer on this patch.

       Andrew

Re: [PATCH V3] net: phy: tja11xx: Add TJA11xx PHY driver

[Marek Vasut]

On 12/23/18 11:58 AM, Andrew Lunn wrote:
>> I can also use a simple conditional, since I don't expect the number of
>> HWMON properties to grow, eg.
>>
>> if (type == hwmon_in && attr == hwmon_in_lcrit_alarm) {...}
>> if (type == hwmon_temp && attr == hwmon_temp_crit_alarm) {...}
> 
> Yes, that is fine.
> 
> Please make sure you Cc: the HWMON maintainer on this patch.

Fine

-- 
Best regards,
Marek Vasut

Re: [PATCH V3] net: phy: tja11xx: Add TJA11xx PHY driver

[Marek Vasut]

On 12/23/18 10:16 AM, Marek Vasut wrote:
> On 12/22/18 6:39 PM, Heiner Kallweit wrote:
>> On 22.12.2018 00:35, Marek Vasut wrote:
>>> Add driver for the NXP TJA1100 and TJA1101 PHYs. These PHYs are special
>>> BroadRReach 100BaseT1 PHYs used in automotive.
>>>
>>> Signed-off-by: Marek Vasut <marex@denx.de>
>>> Cc: Andrew Lunn <andrew@lunn.ch>
>>> Cc: Florian Fainelli <f.fainelli@gmail.com>
>>> Cc: Heiner Kallweit <hkallweit1@gmail.com>
>>> ---
>>> V2: - Use phy_modify(), phy_{set,clear}_bits()
>>>     - Drop enable argument of tja11xx_enable_link_control()
>>>     - Use PHY_BASIC_T1_FEATURES and dont modify supported/advertised
>>>       features in config_init callback
>>>     - Use genphy_soft_reset() instead of opencoding the reset sequence.
>>>     - Drop the aneg parts, since the PHY datasheet claims it does not
>>>       support aneg
>>> V3: - Replace clr with mask
>>>     - Add hwmon support
>>>     - Check commstat in tja11xx_read_status() only if link is up
>>>     - Use PHY_ID_MATCH_MODEL()
>>> ---
>>>  drivers/net/phy/Kconfig       |   6 +
>>>  drivers/net/phy/Makefile      |   1 +
>>>  drivers/net/phy/nxp-tja11xx.c | 424 ++++++++++++++++++++++++++++++++++
>>>  3 files changed, 431 insertions(+)
>>>  create mode 100644 drivers/net/phy/nxp-tja11xx.c
>>>
>> [...]
>>> +
>>> +struct tja11xx_phy_stats {
>>> +	const char	*string;
>>> +	u8		reg;
>>> +	u8		off;
>>> +	u16		mask;
>>> +};
>>> +
>> As written in my other mail, you could think of using
>> FIELD_GET() again. Things like
>> ... n, BIT(n), 
>> ... m, BIT(m), 
>> are simply redundant.
> 
> Done

And undone, FIELD_GET() requires a mask that is compile-time constant.
That's not the case here.

Replicating what FIELD_GET() does at runtime is inefficient, so we can
as well encode the field and offset back into the table. Or is there a
better suggestion ?

-- 
Best regards,
Marek Vasut

Re: [PATCH V3] net: phy: tja11xx: Add TJA11xx PHY driver

[Heiner Kallweit]

On 03.01.2019 03:09, Marek Vasut wrote:
> On 12/23/18 10:16 AM, Marek Vasut wrote:
>> On 12/22/18 6:39 PM, Heiner Kallweit wrote:
>>> On 22.12.2018 00:35, Marek Vasut wrote:
>>>> Add driver for the NXP TJA1100 and TJA1101 PHYs. These PHYs are special
>>>> BroadRReach 100BaseT1 PHYs used in automotive.
>>>>
>>>> Signed-off-by: Marek Vasut <marex@denx.de>
>>>> Cc: Andrew Lunn <andrew@lunn.ch>
>>>> Cc: Florian Fainelli <f.fainelli@gmail.com>
>>>> Cc: Heiner Kallweit <hkallweit1@gmail.com>
>>>> ---
>>>> V2: - Use phy_modify(), phy_{set,clear}_bits()
>>>>     - Drop enable argument of tja11xx_enable_link_control()
>>>>     - Use PHY_BASIC_T1_FEATURES and dont modify supported/advertised
>>>>       features in config_init callback
>>>>     - Use genphy_soft_reset() instead of opencoding the reset sequence.
>>>>     - Drop the aneg parts, since the PHY datasheet claims it does not
>>>>       support aneg
>>>> V3: - Replace clr with mask
>>>>     - Add hwmon support
>>>>     - Check commstat in tja11xx_read_status() only if link is up
>>>>     - Use PHY_ID_MATCH_MODEL()
>>>> ---
>>>>  drivers/net/phy/Kconfig       |   6 +
>>>>  drivers/net/phy/Makefile      |   1 +
>>>>  drivers/net/phy/nxp-tja11xx.c | 424 ++++++++++++++++++++++++++++++++++
>>>>  3 files changed, 431 insertions(+)
>>>>  create mode 100644 drivers/net/phy/nxp-tja11xx.c
>>>>
>>> [...]
>>>> +
>>>> +struct tja11xx_phy_stats {
>>>> +	const char	*string;
>>>> +	u8		reg;
>>>> +	u8		off;
>>>> +	u16		mask;
>>>> +};
>>>> +
>>> As written in my other mail, you could think of using
>>> FIELD_GET() again. Things like
>>> ... n, BIT(n), 
>>> ... m, BIT(m), 
>>> are simply redundant.
>>
>> Done
> 
> And undone, FIELD_GET() requires a mask that is compile-time constant.
> That's not the case here.
> 
> Replicating what FIELD_GET() does at runtime is inefficient, so we can
> as well encode the field and offset back into the table. Or is there a
> better suggestion ?
> 
You're right, FIELD_GET() requires a constant as mask. Then for the
case here I'm not aware of a better solution than having also the mask
in the table.

Re: [PATCH V3] net: phy: tja11xx: Add TJA11xx PHY driver

[Marek Vasut]

On 1/3/19 7:23 AM, Heiner Kallweit wrote:
> On 03.01.2019 03:09, Marek Vasut wrote:
>> On 12/23/18 10:16 AM, Marek Vasut wrote:
>>> On 12/22/18 6:39 PM, Heiner Kallweit wrote:
>>>> On 22.12.2018 00:35, Marek Vasut wrote:
>>>>> Add driver for the NXP TJA1100 and TJA1101 PHYs. These PHYs are special
>>>>> BroadRReach 100BaseT1 PHYs used in automotive.
>>>>>
>>>>> Signed-off-by: Marek Vasut <marex@denx.de>
>>>>> Cc: Andrew Lunn <andrew@lunn.ch>
>>>>> Cc: Florian Fainelli <f.fainelli@gmail.com>
>>>>> Cc: Heiner Kallweit <hkallweit1@gmail.com>
>>>>> ---
>>>>> V2: - Use phy_modify(), phy_{set,clear}_bits()
>>>>>     - Drop enable argument of tja11xx_enable_link_control()
>>>>>     - Use PHY_BASIC_T1_FEATURES and dont modify supported/advertised
>>>>>       features in config_init callback
>>>>>     - Use genphy_soft_reset() instead of opencoding the reset sequence.
>>>>>     - Drop the aneg parts, since the PHY datasheet claims it does not
>>>>>       support aneg
>>>>> V3: - Replace clr with mask
>>>>>     - Add hwmon support
>>>>>     - Check commstat in tja11xx_read_status() only if link is up
>>>>>     - Use PHY_ID_MATCH_MODEL()
>>>>> ---
>>>>>  drivers/net/phy/Kconfig       |   6 +
>>>>>  drivers/net/phy/Makefile      |   1 +
>>>>>  drivers/net/phy/nxp-tja11xx.c | 424 ++++++++++++++++++++++++++++++++++
>>>>>  3 files changed, 431 insertions(+)
>>>>>  create mode 100644 drivers/net/phy/nxp-tja11xx.c
>>>>>
>>>> [...]
>>>>> +
>>>>> +struct tja11xx_phy_stats {
>>>>> +	const char	*string;
>>>>> +	u8		reg;
>>>>> +	u8		off;
>>>>> +	u16		mask;
>>>>> +};
>>>>> +
>>>> As written in my other mail, you could think of using
>>>> FIELD_GET() again. Things like
>>>> ... n, BIT(n), 
>>>> ... m, BIT(m), 
>>>> are simply redundant.
>>>
>>> Done
>>
>> And undone, FIELD_GET() requires a mask that is compile-time constant.
>> That's not the case here.
>>
>> Replicating what FIELD_GET() does at runtime is inefficient, so we can
>> as well encode the field and offset back into the table. Or is there a
>> better suggestion ?
>>
> You're right, FIELD_GET() requires a constant as mask. Then for the
> case here I'm not aware of a better solution than having also the mask
> in the table.

V4 is out then.

-- 
Best regards,
Marek Vasut

Re: [PATCH V3] net: phy: tja11xx: Add TJA11xx PHY driver

[Marek Vasut]

On 1/4/19 3:19 AM, Marek Vasut wrote:

[...]

On a separate note, I was pondering about the master/slave mode of the
TJA11xx PHY. Right now, the driver only operates in Automatic mode,
however the PHY can be force-configured as either master or slave.
Would using .set_tunable with a new tunable type be a good way to expose
this to userspace ? The ethtool can then configure the PHY mode.

-- 
Best regards,
Marek Vasut

Re: [PATCH V3] net: phy: tja11xx: Add TJA11xx PHY driver

[Florian Fainelli]

On 1/3/2019 6:53 PM, Marek Vasut wrote:
> On 1/4/19 3:19 AM, Marek Vasut wrote:
> 
> [...]
> 
> On a separate note, I was pondering about the master/slave mode of the
> TJA11xx PHY. Right now, the driver only operates in Automatic mode,
> however the PHY can be force-configured as either master or slave.
> Would using .set_tunable with a new tunable type be a good way to expose
> this to userspace ? The ethtool can then configure the PHY mode.

Using an ethtool u8 tunable would be fine, with something like:

0 -> automatic
1 -> force master
2 -> force slave
-- 
Florian

Re: [PATCH V3] net: phy: tja11xx: Add TJA11xx PHY driver

[Marek Vasut]

On 1/4/19 3:57 AM, Florian Fainelli wrote:
> 
> 
> On 1/3/2019 6:53 PM, Marek Vasut wrote:
>> On 1/4/19 3:19 AM, Marek Vasut wrote:
>>
>> [...]
>>
>> On a separate note, I was pondering about the master/slave mode of the
>> TJA11xx PHY. Right now, the driver only operates in Automatic mode,
>> however the PHY can be force-configured as either master or slave.
>> Would using .set_tunable with a new tunable type be a good way to expose
>> this to userspace ? The ethtool can then configure the PHY mode.
> 
> Using an ethtool u8 tunable would be fine, with something like:
> 
> 0 -> automatic
> 1 -> force master
> 2 -> force slave

Fine, however I'll send it as a separate patch later.
How does this name/macro sound for the tunable ?

[ETHTOOL_PHY_BRR_MODE]  = "phy-broadrreach-mode",

-- 
Best regards,
Marek Vasut