[PATCH net-next 0/5] net: phy: marvell: some HWMON updates

[PATCH net-next 0/5] net: phy: marvell: some HWMON updates

[Marek Behún]

Here are some updates for Marvell PHY HWMON, mainly
- refactoring for code deduplication
- Amethyst PHY support

Marek Behún (5):
  net: phy: marvell: refactor HWMON OOP style
  net: phy: marvell: fix HWMON enable register for 6390
  net: phy: marvell: use assignment by bitwise AND operator
  net: dsa: mv88e6xxx: simulate Amethyst PHY model number
  net: phy: marvell: add support for Amethyst internal PHY

 drivers/net/dsa/mv88e6xxx/chip.c |   1 +
 drivers/net/phy/marvell.c        | 507 +++++++++++++++----------------
 include/linux/marvell_phy.h      |   1 +
 3 files changed, 252 insertions(+), 257 deletions(-)

-- 
2.26.3

[PATCH net-next 1/5] net: phy: marvell: refactor HWMON OOP style

[Marek Behún]

Use a structure of Marvell PHY specific HWMON methods to reduce code
duplication. Store a pointer to this structure into the PHY driver's
driver_data member.

Signed-off-by: Marek Behún <kabel@kernel.org>
---
 drivers/net/phy/marvell.c | 371 +++++++++++++-------------------------
 1 file changed, 127 insertions(+), 244 deletions(-)

diff --git a/drivers/net/phy/marvell.c b/drivers/net/phy/marvell.c
index 8018ddf7f316..63788d5c13eb 100644
--- a/drivers/net/phy/marvell.c
+++ b/drivers/net/phy/marvell.c
@@ -2216,6 +2216,19 @@ static int marvell_vct7_cable_test_get_status(struct phy_device *phydev,
 }
 
 #ifdef CONFIG_HWMON
+struct marvell_hwmon_ops {
+	int (*get_temp)(struct phy_device *phydev, long *temp);
+	int (*get_temp_critical)(struct phy_device *phydev, long *temp);
+	int (*set_temp_critical)(struct phy_device *phydev, long temp);
+	int (*get_temp_alarm)(struct phy_device *phydev, long *alarm);
+};
+
+static const struct marvell_hwmon_ops *
+to_marvell_hwmon_ops(const struct phy_device *phydev)
+{
+	return phydev->drv->driver_data;
+}
+
 static int m88e1121_get_temp(struct phy_device *phydev, long *temp)
 {
 	int oldpage;
@@ -2259,75 +2272,6 @@ static int m88e1121_get_temp(struct phy_device *phydev, long *temp)
 	return phy_restore_page(phydev, oldpage, ret);
 }
 
-static int m88e1121_hwmon_read(struct device *dev,
-			       enum hwmon_sensor_types type,
-			       u32 attr, int channel, long *temp)
-{
-	struct phy_device *phydev = dev_get_drvdata(dev);
-	int err;
-
-	switch (attr) {
-	case hwmon_temp_input:
-		err = m88e1121_get_temp(phydev, temp);
-		break;
-	default:
-		return -EOPNOTSUPP;
-	}
-
-	return err;
-}
-
-static umode_t m88e1121_hwmon_is_visible(const void *data,
-					 enum hwmon_sensor_types type,
-					 u32 attr, int channel)
-{
-	if (type != hwmon_temp)
-		return 0;
-
-	switch (attr) {
-	case hwmon_temp_input:
-		return 0444;
-	default:
-		return 0;
-	}
-}
-
-static u32 m88e1121_hwmon_chip_config[] = {
-	HWMON_C_REGISTER_TZ,
-	0
-};
-
-static const struct hwmon_channel_info m88e1121_hwmon_chip = {
-	.type = hwmon_chip,
-	.config = m88e1121_hwmon_chip_config,
-};
-
-static u32 m88e1121_hwmon_temp_config[] = {
-	HWMON_T_INPUT,
-	0
-};
-
-static const struct hwmon_channel_info m88e1121_hwmon_temp = {
-	.type = hwmon_temp,
-	.config = m88e1121_hwmon_temp_config,
-};
-
-static const struct hwmon_channel_info *m88e1121_hwmon_info[] = {
-	&m88e1121_hwmon_chip,
-	&m88e1121_hwmon_temp,
-	NULL
-};
-
-static const struct hwmon_ops m88e1121_hwmon_hwmon_ops = {
-	.is_visible = m88e1121_hwmon_is_visible,
-	.read = m88e1121_hwmon_read,
-};
-
-static const struct hwmon_chip_info m88e1121_hwmon_chip_info = {
-	.ops = &m88e1121_hwmon_hwmon_ops,
-	.info = m88e1121_hwmon_info,
-};
-
 static int m88e1510_get_temp(struct phy_device *phydev, long *temp)
 {
 	int ret;
@@ -2390,92 +2334,6 @@ static int m88e1510_get_temp_alarm(struct phy_device *phydev, long *alarm)
 	return 0;
 }
 
-static int m88e1510_hwmon_read(struct device *dev,
-			       enum hwmon_sensor_types type,
-			       u32 attr, int channel, long *temp)
-{
-	struct phy_device *phydev = dev_get_drvdata(dev);
-	int err;
-
-	switch (attr) {
-	case hwmon_temp_input:
-		err = m88e1510_get_temp(phydev, temp);
-		break;
-	case hwmon_temp_crit:
-		err = m88e1510_get_temp_critical(phydev, temp);
-		break;
-	case hwmon_temp_max_alarm:
-		err = m88e1510_get_temp_alarm(phydev, temp);
-		break;
-	default:
-		return -EOPNOTSUPP;
-	}
-
-	return err;
-}
-
-static int m88e1510_hwmon_write(struct device *dev,
-				enum hwmon_sensor_types type,
-				u32 attr, int channel, long temp)
-{
-	struct phy_device *phydev = dev_get_drvdata(dev);
-	int err;
-
-	switch (attr) {
-	case hwmon_temp_crit:
-		err = m88e1510_set_temp_critical(phydev, temp);
-		break;
-	default:
-		return -EOPNOTSUPP;
-	}
-	return err;
-}
-
-static umode_t m88e1510_hwmon_is_visible(const void *data,
-					 enum hwmon_sensor_types type,
-					 u32 attr, int channel)
-{
-	if (type != hwmon_temp)
-		return 0;
-
-	switch (attr) {
-	case hwmon_temp_input:
-	case hwmon_temp_max_alarm:
-		return 0444;
-	case hwmon_temp_crit:
-		return 0644;
-	default:
-		return 0;
-	}
-}
-
-static u32 m88e1510_hwmon_temp_config[] = {
-	HWMON_T_INPUT | HWMON_T_CRIT | HWMON_T_MAX_ALARM,
-	0
-};
-
-static const struct hwmon_channel_info m88e1510_hwmon_temp = {
-	.type = hwmon_temp,
-	.config = m88e1510_hwmon_temp_config,
-};
-
-static const struct hwmon_channel_info *m88e1510_hwmon_info[] = {
-	&m88e1121_hwmon_chip,
-	&m88e1510_hwmon_temp,
-	NULL
-};
-
-static const struct hwmon_ops m88e1510_hwmon_hwmon_ops = {
-	.is_visible = m88e1510_hwmon_is_visible,
-	.read = m88e1510_hwmon_read,
-	.write = m88e1510_hwmon_write,
-};
-
-static const struct hwmon_chip_info m88e1510_hwmon_chip_info = {
-	.ops = &m88e1510_hwmon_hwmon_ops,
-	.info = m88e1510_hwmon_info,
-};
-
 static int m88e6390_get_temp(struct phy_device *phydev, long *temp)
 {
 	int sum = 0;
@@ -2534,63 +2392,114 @@ static int m88e6390_get_temp(struct phy_device *phydev, long *temp)
 	return ret;
 }
 
-static int m88e6390_hwmon_read(struct device *dev,
-			       enum hwmon_sensor_types type,
-			       u32 attr, int channel, long *temp)
+static int marvell_hwmon_read(struct device *dev, enum hwmon_sensor_types type,
+			      u32 attr, int channel, long *temp)
 {
 	struct phy_device *phydev = dev_get_drvdata(dev);
-	int err;
+	const struct marvell_hwmon_ops *ops = to_marvell_hwmon_ops(phydev);
+	int err = -EOPNOTSUPP;
 
 	switch (attr) {
 	case hwmon_temp_input:
-		err = m88e6390_get_temp(phydev, temp);
+		if (ops->get_temp)
+			err = ops->get_temp(phydev, temp);
+		break;
+	case hwmon_temp_crit:
+		if (ops->get_temp_critical)
+			err = ops->get_temp_critical(phydev, temp);
+		break;
+	case hwmon_temp_max_alarm:
+		if (ops->get_temp_alarm)
+			err = ops->get_temp_alarm(phydev, temp);
 		break;
 	default:
-		return -EOPNOTSUPP;
+		fallthrough;
+	}
+
+	return err;
+}
+
+static int marvell_hwmon_write(struct device *dev, enum hwmon_sensor_types type,
+			       u32 attr, int channel, long temp)
+{
+	struct phy_device *phydev = dev_get_drvdata(dev);
+	const struct marvell_hwmon_ops *ops = to_marvell_hwmon_ops(phydev);
+	int err = -EOPNOTSUPP;
+
+	switch (attr) {
+	case hwmon_temp_crit:
+		if (ops->set_temp_critical)
+			err = ops->set_temp_critical(phydev, temp);
+		break;
+	default:
+		fallthrough;
 	}
 
 	return err;
 }
 
-static umode_t m88e6390_hwmon_is_visible(const void *data,
-					 enum hwmon_sensor_types type,
-					 u32 attr, int channel)
+static umode_t marvell_hwmon_is_visible(const void *data,
+					enum hwmon_sensor_types type,
+					u32 attr, int channel)
 {
+	const struct phy_device *phydev = data;
+	const struct marvell_hwmon_ops *ops = to_marvell_hwmon_ops(phydev);
+
 	if (type != hwmon_temp)
 		return 0;
 
 	switch (attr) {
 	case hwmon_temp_input:
-		return 0444;
+		return ops->get_temp ? 0444 : 0;
+	case hwmon_temp_max_alarm:
+		return ops->get_temp_alarm ? 0444 : 0;
+	case hwmon_temp_crit:
+		return (ops->get_temp_critical ? 0444 : 0) |
+		       (ops->set_temp_critical ? 0200 : 0);
 	default:
 		return 0;
 	}
 }
 
-static u32 m88e6390_hwmon_temp_config[] = {
-	HWMON_T_INPUT,
+static u32 marvell_hwmon_chip_config[] = {
+	HWMON_C_REGISTER_TZ,
+	0
+};
+
+static const struct hwmon_channel_info marvell_hwmon_chip = {
+	.type = hwmon_chip,
+	.config = marvell_hwmon_chip_config,
+};
+
+/* we can define HWMON_T_CRIT and HWMON_T_MAX_ALARM even though these are not
+ * defined for all PHYs, because the hwmon code checks whether the attributes
+ * exists via the .is_visible method
+ */
+static u32 marvell_hwmon_temp_config[] = {
+	HWMON_T_INPUT | HWMON_T_CRIT | HWMON_T_MAX_ALARM,
 	0
 };
 
-static const struct hwmon_channel_info m88e6390_hwmon_temp = {
+static const struct hwmon_channel_info marvell_hwmon_temp = {
 	.type = hwmon_temp,
-	.config = m88e6390_hwmon_temp_config,
+	.config = marvell_hwmon_temp_config,
 };
 
-static const struct hwmon_channel_info *m88e6390_hwmon_info[] = {
-	&m88e1121_hwmon_chip,
-	&m88e6390_hwmon_temp,
+static const struct hwmon_channel_info *marvell_hwmon_info[] = {
+	&marvell_hwmon_chip,
+	&marvell_hwmon_temp,
 	NULL
 };
 
-static const struct hwmon_ops m88e6390_hwmon_hwmon_ops = {
-	.is_visible = m88e6390_hwmon_is_visible,
-	.read = m88e6390_hwmon_read,
+static const struct hwmon_ops marvell_hwmon_hwmon_ops = {
+	.is_visible = marvell_hwmon_is_visible,
+	.read = marvell_hwmon_read,
+	.write = marvell_hwmon_write,
 };
 
-static const struct hwmon_chip_info m88e6390_hwmon_chip_info = {
-	.ops = &m88e6390_hwmon_hwmon_ops,
-	.info = m88e6390_hwmon_info,
+static const struct hwmon_chip_info marvell_hwmon_chip_info = {
+	.ops = &marvell_hwmon_hwmon_ops,
+	.info = marvell_hwmon_info,
 };
 
 static int marvell_hwmon_name(struct phy_device *phydev)
@@ -2613,49 +2522,48 @@ static int marvell_hwmon_name(struct phy_device *phydev)
 	return 0;
 }
 
-static int marvell_hwmon_probe(struct phy_device *phydev,
-			       const struct hwmon_chip_info *chip)
+static int marvell_hwmon_probe(struct phy_device *phydev)
 {
+	const struct marvell_hwmon_ops *ops = to_marvell_hwmon_ops(phydev);
 	struct marvell_priv *priv = phydev->priv;
 	struct device *dev = &phydev->mdio.dev;
 	int err;
 
+	if (!ops)
+		return 0;
+
 	err = marvell_hwmon_name(phydev);
 	if (err)
 		return err;
 
 	priv->hwmon_dev = devm_hwmon_device_register_with_info(
-		dev, priv->hwmon_name, phydev, chip, NULL);
+		dev, priv->hwmon_name, phydev, &marvell_hwmon_chip_info, NULL);
 
 	return PTR_ERR_OR_ZERO(priv->hwmon_dev);
 }
 
-static int m88e1121_hwmon_probe(struct phy_device *phydev)
-{
-	return marvell_hwmon_probe(phydev, &m88e1121_hwmon_chip_info);
-}
+static const struct marvell_hwmon_ops m88e1121_hwmon_ops = {
+	.get_temp = m88e1121_get_temp,
+};
 
-static int m88e1510_hwmon_probe(struct phy_device *phydev)
-{
-	return marvell_hwmon_probe(phydev, &m88e1510_hwmon_chip_info);
-}
+static const struct marvell_hwmon_ops m88e1510_hwmon_ops = {
+	.get_temp = m88e1510_get_temp,
+	.get_temp_critical = m88e1510_get_temp_critical,
+	.set_temp_critical = m88e1510_set_temp_critical,
+	.get_temp_alarm = m88e1510_get_temp_alarm,
+};
+
+static const struct marvell_hwmon_ops m88e6390_hwmon_ops = {
+	.get_temp = m88e6390_get_temp,
+};
+
+#define DEF_MARVELL_HWMON_OPS(s) (&(s))
 
-static int m88e6390_hwmon_probe(struct phy_device *phydev)
-{
-	return marvell_hwmon_probe(phydev, &m88e6390_hwmon_chip_info);
-}
 #else
-static int m88e1121_hwmon_probe(struct phy_device *phydev)
-{
-	return 0;
-}
 
-static int m88e1510_hwmon_probe(struct phy_device *phydev)
-{
-	return 0;
-}
+#define DEF_MARVELL_HWMON_OPS(s) NULL
 
-static int m88e6390_hwmon_probe(struct phy_device *phydev)
+static int marvell_hwmon_probe(struct phy_device *phydev)
 {
 	return 0;
 }
@@ -2671,40 +2579,7 @@ static int marvell_probe(struct phy_device *phydev)
 
 	phydev->priv = priv;
 
-	return 0;
-}
-
-static int m88e1121_probe(struct phy_device *phydev)
-{
-	int err;
-
-	err = marvell_probe(phydev);
-	if (err)
-		return err;
-
-	return m88e1121_hwmon_probe(phydev);
-}
-
-static int m88e1510_probe(struct phy_device *phydev)
-{
-	int err;
-
-	err = marvell_probe(phydev);
-	if (err)
-		return err;
-
-	return m88e1510_hwmon_probe(phydev);
-}
-
-static int m88e6390_probe(struct phy_device *phydev)
-{
-	int err;
-
-	err = marvell_probe(phydev);
-	if (err)
-		return err;
-
-	return m88e6390_hwmon_probe(phydev);
+	return marvell_hwmon_probe(phydev);
 }
 
 static struct phy_driver marvell_drivers[] = {
@@ -2810,8 +2685,9 @@ static struct phy_driver marvell_drivers[] = {
 		.phy_id = MARVELL_PHY_ID_88E1121R,
 		.phy_id_mask = MARVELL_PHY_ID_MASK,
 		.name = "Marvell 88E1121R",
+		.driver_data = DEF_MARVELL_HWMON_OPS(m88e1121_hwmon_ops),
 		/* PHY_GBIT_FEATURES */
-		.probe = m88e1121_probe,
+		.probe = marvell_probe,
 		.config_init = marvell_config_init,
 		.config_aneg = m88e1121_config_aneg,
 		.read_status = marvell_read_status,
@@ -2927,9 +2803,10 @@ static struct phy_driver marvell_drivers[] = {
 		.phy_id = MARVELL_PHY_ID_88E1510,
 		.phy_id_mask = MARVELL_PHY_ID_MASK,
 		.name = "Marvell 88E1510",
+		.driver_data = DEF_MARVELL_HWMON_OPS(m88e1510_hwmon_ops),
 		.features = PHY_GBIT_FIBRE_FEATURES,
 		.flags = PHY_POLL_CABLE_TEST,
-		.probe = m88e1510_probe,
+		.probe = marvell_probe,
 		.config_init = m88e1510_config_init,
 		.config_aneg = m88e1510_config_aneg,
 		.read_status = marvell_read_status,
@@ -2955,9 +2832,10 @@ static struct phy_driver marvell_drivers[] = {
 		.phy_id = MARVELL_PHY_ID_88E1540,
 		.phy_id_mask = MARVELL_PHY_ID_MASK,
 		.name = "Marvell 88E1540",
+		.driver_data = DEF_MARVELL_HWMON_OPS(m88e1510_hwmon_ops),
 		/* PHY_GBIT_FEATURES */
 		.flags = PHY_POLL_CABLE_TEST,
-		.probe = m88e1510_probe,
+		.probe = marvell_probe,
 		.config_init = marvell_config_init,
 		.config_aneg = m88e1510_config_aneg,
 		.read_status = marvell_read_status,
@@ -2980,7 +2858,8 @@ static struct phy_driver marvell_drivers[] = {
 		.phy_id = MARVELL_PHY_ID_88E1545,
 		.phy_id_mask = MARVELL_PHY_ID_MASK,
 		.name = "Marvell 88E1545",
-		.probe = m88e1510_probe,
+		.driver_data = DEF_MARVELL_HWMON_OPS(m88e1510_hwmon_ops),
+		.probe = marvell_probe,
 		/* PHY_GBIT_FEATURES */
 		.flags = PHY_POLL_CABLE_TEST,
 		.config_init = marvell_config_init,
@@ -3024,9 +2903,10 @@ static struct phy_driver marvell_drivers[] = {
 		.phy_id = MARVELL_PHY_ID_88E6341_FAMILY,
 		.phy_id_mask = MARVELL_PHY_ID_MASK,
 		.name = "Marvell 88E6341 Family",
+		.driver_data = DEF_MARVELL_HWMON_OPS(m88e1510_hwmon_ops),
 		/* PHY_GBIT_FEATURES */
 		.flags = PHY_POLL_CABLE_TEST,
-		.probe = m88e1510_probe,
+		.probe = marvell_probe,
 		.config_init = marvell_config_init,
 		.config_aneg = m88e6390_config_aneg,
 		.read_status = marvell_read_status,
@@ -3049,9 +2929,10 @@ static struct phy_driver marvell_drivers[] = {
 		.phy_id = MARVELL_PHY_ID_88E6390_FAMILY,
 		.phy_id_mask = MARVELL_PHY_ID_MASK,
 		.name = "Marvell 88E6390 Family",
+		.driver_data = DEF_MARVELL_HWMON_OPS(m88e6390_hwmon_ops),
 		/* PHY_GBIT_FEATURES */
 		.flags = PHY_POLL_CABLE_TEST,
-		.probe = m88e6390_probe,
+		.probe = marvell_probe,
 		.config_init = marvell_config_init,
 		.config_aneg = m88e6390_config_aneg,
 		.read_status = marvell_read_status,
@@ -3074,7 +2955,8 @@ static struct phy_driver marvell_drivers[] = {
 		.phy_id = MARVELL_PHY_ID_88E1340S,
 		.phy_id_mask = MARVELL_PHY_ID_MASK,
 		.name = "Marvell 88E1340S",
-		.probe = m88e1510_probe,
+		.driver_data = DEF_MARVELL_HWMON_OPS(m88e1510_hwmon_ops),
+		.probe = marvell_probe,
 		/* PHY_GBIT_FEATURES */
 		.config_init = marvell_config_init,
 		.config_aneg = m88e1510_config_aneg,
@@ -3095,7 +2977,8 @@ static struct phy_driver marvell_drivers[] = {
 		.phy_id = MARVELL_PHY_ID_88E1548P,
 		.phy_id_mask = MARVELL_PHY_ID_MASK,
 		.name = "Marvell 88E1548P",
-		.probe = m88e1510_probe,
+		.driver_data = DEF_MARVELL_HWMON_OPS(m88e1510_hwmon_ops),
+		.probe = marvell_probe,
 		.features = PHY_GBIT_FIBRE_FEATURES,
 		.config_init = marvell_config_init,
 		.config_aneg = m88e1510_config_aneg,
-- 
2.26.3

[PATCH net-next 2/5] net: phy: marvell: fix HWMON enable register for 6390

[Marek Behún]

Register 27_6.15:14 has the following description in 88E6393X
documentation:
  Temperature Sensor Enable
    0x0 - Sample every 1s
    0x1 - Sense rate decided by bits 10:8 of this register
    0x2 - Use 26_6.5 (One shot Temperature Sample) to enable
    0x3 - Disable

This is compatible with how the 6390 code uses this register currently,
but the 6390 code handles it as two 1-bit registers (somewhat), instead
of one register with 4 possible values.

Rename this register and define all 4 values according to 6393X
documentation.

Signed-off-by: Marek Behún <kabel@kernel.org>
---
 drivers/net/phy/marvell.c | 19 +++++++++----------
 1 file changed, 9 insertions(+), 10 deletions(-)

diff --git a/drivers/net/phy/marvell.c b/drivers/net/phy/marvell.c
index 63788d5c13eb..bae2a225b550 100644
--- a/drivers/net/phy/marvell.c
+++ b/drivers/net/phy/marvell.c
@@ -113,11 +113,11 @@
 #define MII_88E1540_COPPER_CTRL3_FAST_LINK_DOWN		BIT(9)
 
 #define MII_88E6390_MISC_TEST		0x1b
-#define MII_88E6390_MISC_TEST_SAMPLE_1S		0
-#define MII_88E6390_MISC_TEST_SAMPLE_10MS	BIT(14)
-#define MII_88E6390_MISC_TEST_SAMPLE_DISABLE	BIT(15)
-#define MII_88E6390_MISC_TEST_SAMPLE_ENABLE	0
-#define MII_88E6390_MISC_TEST_SAMPLE_MASK	(0x3 << 14)
+#define MII_88E6390_MISC_TEST_TEMP_SENSOR_ENABLE_SAMPLE_1S	(0x0 << 14)
+#define MII_88E6390_MISC_TEST_TEMP_SENSOR_ENABLE		(0x1 << 14)
+#define MII_88E6390_MISC_TEST_TEMP_SENSOR_ENABLE_ONESHOT	(0x2 << 14)
+#define MII_88E6390_MISC_TEST_TEMP_SENSOR_DISABLE		(0x3 << 14)
+#define MII_88E6390_MISC_TEST_TEMP_SENSOR_MASK			(0x3 << 14)
 
 #define MII_88E6390_TEMP_SENSOR		0x1c
 #define MII_88E6390_TEMP_SENSOR_MASK	0xff
@@ -2352,9 +2352,8 @@ static int m88e6390_get_temp(struct phy_device *phydev, long *temp)
 	if (ret < 0)
 		goto error;
 
-	ret = ret & ~MII_88E6390_MISC_TEST_SAMPLE_MASK;
-	ret |= MII_88E6390_MISC_TEST_SAMPLE_ENABLE |
-		MII_88E6390_MISC_TEST_SAMPLE_1S;
+	ret = ret & ~MII_88E6390_MISC_TEST_TEMP_SENSOR_MASK;
+	ret |= MII_88E6390_MISC_TEST_TEMP_SENSOR_ENABLE_SAMPLE_1S;
 
 	ret = __phy_write(phydev, MII_88E6390_MISC_TEST, ret);
 	if (ret < 0)
@@ -2381,8 +2380,8 @@ static int m88e6390_get_temp(struct phy_device *phydev, long *temp)
 	if (ret < 0)
 		goto error;
 
-	ret = ret & ~MII_88E6390_MISC_TEST_SAMPLE_MASK;
-	ret |= MII_88E6390_MISC_TEST_SAMPLE_DISABLE;
+	ret = ret & ~MII_88E6390_MISC_TEST_TEMP_SENSOR_MASK;
+	ret |= MII_88E6390_MISC_TEST_TEMP_SENSOR_DISABLE;
 
 	ret = __phy_write(phydev, MII_88E6390_MISC_TEST, ret);
 
-- 
2.26.3

[PATCH net-next 3/5] net: phy: marvell: use assignment by bitwise AND operator

[Marek Behún]

Use the &= operator instead of
  ret = ret & ...

Signed-off-by: Marek Behún <kabel@kernel.org>
---
 drivers/net/phy/marvell.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/phy/marvell.c b/drivers/net/phy/marvell.c
index bae2a225b550..9eb65898da83 100644
--- a/drivers/net/phy/marvell.c
+++ b/drivers/net/phy/marvell.c
@@ -2352,7 +2352,7 @@ static int m88e6390_get_temp(struct phy_device *phydev, long *temp)
 	if (ret < 0)
 		goto error;
 
-	ret = ret & ~MII_88E6390_MISC_TEST_TEMP_SENSOR_MASK;
+	ret &= ~MII_88E6390_MISC_TEST_TEMP_SENSOR_MASK;
 	ret |= MII_88E6390_MISC_TEST_TEMP_SENSOR_ENABLE_SAMPLE_1S;
 
 	ret = __phy_write(phydev, MII_88E6390_MISC_TEST, ret);
-- 
2.26.3

[PATCH net-next 4/5] net: dsa: mv88e6xxx: simulate Amethyst PHY model number

[Marek Behún]

Amethyst internal PHYs also report empty model number in MII_PHYSID2.

Fill in switch product number, as is done for Topaz and Peridot.

Signed-off-by: Marek Behún <kabel@kernel.org>
---
 drivers/net/dsa/mv88e6xxx/chip.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/net/dsa/mv88e6xxx/chip.c b/drivers/net/dsa/mv88e6xxx/chip.c
index 860dca41cf4b..9c4f8517c34b 100644
--- a/drivers/net/dsa/mv88e6xxx/chip.c
+++ b/drivers/net/dsa/mv88e6xxx/chip.c
@@ -3165,6 +3165,7 @@ static int mv88e6xxx_setup(struct dsa_switch *ds)
 static const u16 family_prod_id_table[] = {
 	[MV88E6XXX_FAMILY_6341] = MV88E6XXX_PORT_SWITCH_ID_PROD_6341,
 	[MV88E6XXX_FAMILY_6390] = MV88E6XXX_PORT_SWITCH_ID_PROD_6390,
+	[MV88E6XXX_FAMILY_6393] = MV88E6XXX_PORT_SWITCH_ID_PROD_6393X,
 };
 
 static int mv88e6xxx_mdio_read(struct mii_bus *bus, int phy, int reg)
-- 
2.26.3

[PATCH net-next 5/5] net: phy: marvell: add support for Amethyst internal PHY

[Marek Behún]

Add support for Amethyst internal PHY.

The only difference from Peridot is HWMON.

Signed-off-by: Marek Behún <kabel@kernel.org>
---
 drivers/net/phy/marvell.c   | 117 +++++++++++++++++++++++++++++++++++-
 include/linux/marvell_phy.h |   1 +
 2 files changed, 115 insertions(+), 3 deletions(-)

diff --git a/drivers/net/phy/marvell.c b/drivers/net/phy/marvell.c
index 9eb65898da83..5bf3663fa248 100644
--- a/drivers/net/phy/marvell.c
+++ b/drivers/net/phy/marvell.c
@@ -118,10 +118,21 @@
 #define MII_88E6390_MISC_TEST_TEMP_SENSOR_ENABLE_ONESHOT	(0x2 << 14)
 #define MII_88E6390_MISC_TEST_TEMP_SENSOR_DISABLE		(0x3 << 14)
 #define MII_88E6390_MISC_TEST_TEMP_SENSOR_MASK			(0x3 << 14)
+#define MII_88E6393_MISC_TEST_SAMPLES_4096	0x0000
+#define MII_88E6393_MISC_TEST_SAMPLES_8192	0x0800
+#define MII_88E6393_MISC_TEST_SAMPLES_16384	0x1000
+#define MII_88E6393_MISC_TEST_SAMPLES_32768	0x1800
+#define MII_88E6393_MISC_TEST_SAMPLES_MASK	0x1800
+#define MII_88E6393_MISC_TEST_RATE_2_3MS	0x0500
+#define MII_88E6393_MISC_TEST_RATE_6_4MS	0x0600
+#define MII_88E6393_MISC_TEST_RATE_11_9MS	0x0700
+#define MII_88E6393_MISC_TEST_RATE_MASK		0x0700
 
 #define MII_88E6390_TEMP_SENSOR		0x1c
-#define MII_88E6390_TEMP_SENSOR_MASK	0xff
-#define MII_88E6390_TEMP_SENSOR_SAMPLES 10
+#define MII_88E6393_TEMP_SENSOR_THRESHOLD_MASK	0xff00
+#define MII_88E6393_TEMP_SENSOR_THRESHOLD_SHIFT	8
+#define MII_88E6390_TEMP_SENSOR_MASK		0xff
+#define MII_88E6390_TEMP_SENSOR_SAMPLES		10
 
 #define MII_88E1318S_PHY_MSCR1_REG	16
 #define MII_88E1318S_PHY_MSCR1_PAD_ODD	BIT(6)
@@ -2217,6 +2228,7 @@ static int marvell_vct7_cable_test_get_status(struct phy_device *phydev,
 
 #ifdef CONFIG_HWMON
 struct marvell_hwmon_ops {
+	int (*config)(struct phy_device *phydev);
 	int (*get_temp)(struct phy_device *phydev, long *temp);
 	int (*get_temp_critical)(struct phy_device *phydev, long *temp);
 	int (*set_temp_critical)(struct phy_device *phydev, long temp);
@@ -2391,6 +2403,65 @@ static int m88e6390_get_temp(struct phy_device *phydev, long *temp)
 	return ret;
 }
 
+static int m88e6393_get_temp(struct phy_device *phydev, long *temp)
+{
+	int err;
+
+	err = m88e1510_get_temp(phydev, temp);
+
+	/* 88E1510 measures T + 25, while the PHY on 88E6393X switch
+	 * T + 75, so we have to subtract another 50
+	 */
+	*temp -= 50000;
+
+	return err;
+}
+
+static int m88e6393_get_temp_critical(struct phy_device *phydev, long *temp)
+{
+	int ret;
+
+	*temp = 0;
+
+	ret = phy_read_paged(phydev, MII_MARVELL_MISC_TEST_PAGE,
+			     MII_88E6390_TEMP_SENSOR);
+	if (ret < 0)
+		return ret;
+
+	*temp = (((ret & MII_88E6393_TEMP_SENSOR_THRESHOLD_MASK) >>
+		  MII_88E6393_TEMP_SENSOR_THRESHOLD_SHIFT) - 75) * 1000;
+
+	return 0;
+}
+
+static int m88e6393_set_temp_critical(struct phy_device *phydev, long temp)
+{
+	temp = (temp / 1000) + 75;
+
+	return phy_modify_paged(phydev, MII_MARVELL_MISC_TEST_PAGE,
+				MII_88E6390_TEMP_SENSOR,
+				MII_88E6393_TEMP_SENSOR_THRESHOLD_MASK,
+				temp << MII_88E6393_TEMP_SENSOR_THRESHOLD_SHIFT);
+}
+
+static int m88e6393_hwmon_config(struct phy_device *phydev)
+{
+	int err;
+
+	err = m88e6393_set_temp_critical(phydev, 100000);
+	if (err)
+		return err;
+
+	return phy_modify_paged(phydev, MII_MARVELL_MISC_TEST_PAGE,
+				MII_88E6390_MISC_TEST,
+				MII_88E6390_MISC_TEST_TEMP_SENSOR_MASK |
+				MII_88E6393_MISC_TEST_SAMPLES_MASK |
+				MII_88E6393_MISC_TEST_RATE_MASK,
+				MII_88E6390_MISC_TEST_TEMP_SENSOR_ENABLE |
+				MII_88E6393_MISC_TEST_SAMPLES_4096 |
+				MII_88E6393_MISC_TEST_RATE_2_3MS);
+}
+
 static int marvell_hwmon_read(struct device *dev, enum hwmon_sensor_types type,
 			      u32 attr, int channel, long *temp)
 {
@@ -2537,8 +2608,13 @@ static int marvell_hwmon_probe(struct phy_device *phydev)
 
 	priv->hwmon_dev = devm_hwmon_device_register_with_info(
 		dev, priv->hwmon_name, phydev, &marvell_hwmon_chip_info, NULL);
+	if (IS_ERR(priv->hwmon_dev))
+		return PTR_ERR(priv->hwmon_dev);
 
-	return PTR_ERR_OR_ZERO(priv->hwmon_dev);
+	if (ops->config)
+		err = ops->config(phydev);
+
+	return err;
 }
 
 static const struct marvell_hwmon_ops m88e1121_hwmon_ops = {
@@ -2556,6 +2632,14 @@ static const struct marvell_hwmon_ops m88e6390_hwmon_ops = {
 	.get_temp = m88e6390_get_temp,
 };
 
+static const struct marvell_hwmon_ops m88e6393_hwmon_ops = {
+	.config = m88e6393_hwmon_config,
+	.get_temp = m88e6393_get_temp,
+	.get_temp_critical = m88e6393_get_temp_critical,
+	.set_temp_critical = m88e6393_set_temp_critical,
+	.get_temp_alarm = m88e1510_get_temp_alarm,
+};
+
 #define DEF_MARVELL_HWMON_OPS(s) (&(s))
 
 #else
@@ -2950,6 +3034,32 @@ static struct phy_driver marvell_drivers[] = {
 		.cable_test_tdr_start = marvell_vct5_cable_test_tdr_start,
 		.cable_test_get_status = marvell_vct7_cable_test_get_status,
 	},
+	{
+		.phy_id = MARVELL_PHY_ID_88E6393_FAMILY,
+		.phy_id_mask = MARVELL_PHY_ID_MASK,
+		.name = "Marvell 88E6393 Family",
+		.driver_data = DEF_MARVELL_HWMON_OPS(m88e6393_hwmon_ops),
+		/* PHY_GBIT_FEATURES */
+		.flags = PHY_POLL_CABLE_TEST,
+		.probe = marvell_probe,
+		.config_init = marvell_config_init,
+		.config_aneg = m88e1510_config_aneg,
+		.read_status = marvell_read_status,
+		.config_intr = marvell_config_intr,
+		.handle_interrupt = marvell_handle_interrupt,
+		.resume = genphy_resume,
+		.suspend = genphy_suspend,
+		.read_page = marvell_read_page,
+		.write_page = marvell_write_page,
+		.get_sset_count = marvell_get_sset_count,
+		.get_strings = marvell_get_strings,
+		.get_stats = marvell_get_stats,
+		.get_tunable = m88e1540_get_tunable,
+		.set_tunable = m88e1540_set_tunable,
+		.cable_test_start = marvell_vct7_cable_test_start,
+		.cable_test_tdr_start = marvell_vct5_cable_test_tdr_start,
+		.cable_test_get_status = marvell_vct7_cable_test_get_status,
+	},
 	{
 		.phy_id = MARVELL_PHY_ID_88E1340S,
 		.phy_id_mask = MARVELL_PHY_ID_MASK,
@@ -3016,6 +3126,7 @@ static struct mdio_device_id __maybe_unused marvell_tbl[] = {
 	{ MARVELL_PHY_ID_88E3016, MARVELL_PHY_ID_MASK },
 	{ MARVELL_PHY_ID_88E6341_FAMILY, MARVELL_PHY_ID_MASK },
 	{ MARVELL_PHY_ID_88E6390_FAMILY, MARVELL_PHY_ID_MASK },
+	{ MARVELL_PHY_ID_88E6393_FAMILY, MARVELL_PHY_ID_MASK },
 	{ MARVELL_PHY_ID_88E1340S, MARVELL_PHY_ID_MASK },
 	{ MARVELL_PHY_ID_88E1548P, MARVELL_PHY_ID_MASK },
 	{ }
diff --git a/include/linux/marvell_phy.h b/include/linux/marvell_phy.h
index f61d82c53f30..acee44b9db26 100644
--- a/include/linux/marvell_phy.h
+++ b/include/linux/marvell_phy.h
@@ -39,6 +39,7 @@
  */
 #define MARVELL_PHY_ID_88E6341_FAMILY	0x01410f41
 #define MARVELL_PHY_ID_88E6390_FAMILY	0x01410f90
+#define MARVELL_PHY_ID_88E6393_FAMILY	0x002b0b9b
 
 #define MARVELL_PHY_FAMILY_ID(id)	((id) >> 4)
 
-- 
2.26.3

Re: [PATCH net-next 2/5] net: phy: marvell: fix HWMON enable register for 6390

[Andrew Lunn]

On Tue, Apr 13, 2021 at 09:55:35AM +0200, Marek Behún wrote:
> Register 27_6.15:14 has the following description in 88E6393X
> documentation:
>   Temperature Sensor Enable
>     0x0 - Sample every 1s
>     0x1 - Sense rate decided by bits 10:8 of this register
>     0x2 - Use 26_6.5 (One shot Temperature Sample) to enable
>     0x3 - Disable
> 
> This is compatible with how the 6390 code uses this register currently,
> but the 6390 code handles it as two 1-bit registers (somewhat), instead
> of one register with 4 possible values.
> 
> Rename this register and define all 4 values according to 6393X
> documentation.
> 
> Signed-off-by: Marek Behún <kabel@kernel.org>
> ---
>  drivers/net/phy/marvell.c | 19 +++++++++----------
>  1 file changed, 9 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/net/phy/marvell.c b/drivers/net/phy/marvell.c
> index 63788d5c13eb..bae2a225b550 100644
> --- a/drivers/net/phy/marvell.c
> +++ b/drivers/net/phy/marvell.c
> @@ -113,11 +113,11 @@
>  #define MII_88E1540_COPPER_CTRL3_FAST_LINK_DOWN		BIT(9)
>  
>  #define MII_88E6390_MISC_TEST		0x1b
> -#define MII_88E6390_MISC_TEST_SAMPLE_1S		0
> -#define MII_88E6390_MISC_TEST_SAMPLE_10MS	BIT(14)
> -#define MII_88E6390_MISC_TEST_SAMPLE_DISABLE	BIT(15)
> -#define MII_88E6390_MISC_TEST_SAMPLE_ENABLE	0
> -#define MII_88E6390_MISC_TEST_SAMPLE_MASK	(0x3 << 14)
> +#define MII_88E6390_MISC_TEST_TEMP_SENSOR_ENABLE_SAMPLE_1S	(0x0 << 14)
> +#define MII_88E6390_MISC_TEST_TEMP_SENSOR_ENABLE		(0x1 << 14)
> +#define MII_88E6390_MISC_TEST_TEMP_SENSOR_ENABLE_ONESHOT	(0x2 << 14)
> +#define MII_88E6390_MISC_TEST_TEMP_SENSOR_DISABLE		(0x3 << 14)
> +#define MII_88E6390_MISC_TEST_TEMP_SENSOR_MASK			(0x3 << 14)
>  
>  #define MII_88E6390_TEMP_SENSOR		0x1c
>  #define MII_88E6390_TEMP_SENSOR_MASK	0xff
> @@ -2352,9 +2352,8 @@ static int m88e6390_get_temp(struct phy_device *phydev, long *temp)
>  	if (ret < 0)
>  		goto error;
>  
> -	ret = ret & ~MII_88E6390_MISC_TEST_SAMPLE_MASK;
> -	ret |= MII_88E6390_MISC_TEST_SAMPLE_ENABLE |
> -		MII_88E6390_MISC_TEST_SAMPLE_1S;
> +	ret = ret & ~MII_88E6390_MISC_TEST_TEMP_SENSOR_MASK;
> +	ret |= MII_88E6390_MISC_TEST_TEMP_SENSOR_ENABLE_SAMPLE_1S;

So this is identical

>  
>  	ret = __phy_write(phydev, MII_88E6390_MISC_TEST, ret);
>  	if (ret < 0)
> @@ -2381,8 +2380,8 @@ static int m88e6390_get_temp(struct phy_device *phydev, long *temp)
>  	if (ret < 0)
>  		goto error;
>  
> -	ret = ret & ~MII_88E6390_MISC_TEST_SAMPLE_MASK;
> -	ret |= MII_88E6390_MISC_TEST_SAMPLE_DISABLE;
> +	ret = ret & ~MII_88E6390_MISC_TEST_TEMP_SENSOR_MASK;
> +	ret |= MII_88E6390_MISC_TEST_TEMP_SENSOR_DISABLE;

And here we have gone from 0x2 to 0x3?

Have you checked the 6390 datasheet for this?

I will test these patches later.

     Andrew

Re: [PATCH net-next 3/5] net: phy: marvell: use assignment by bitwise AND operator

[Andrew Lunn]

On Tue, Apr 13, 2021 at 09:55:36AM +0200, Marek Behún wrote:
> Use the &= operator instead of
>   ret = ret & ...
> 
> Signed-off-by: Marek Behún <kabel@kernel.org>

Reviewed-by: Andrew Lunn <andrew@lunn.ch>

    Andrew

Re: [PATCH net-next 1/5] net: phy: marvell: refactor HWMON OOP style

[kernel test robot]

[-- Attachment #1: Type: text/plain, Size: 3075 bytes --]

Hi "Marek,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on net/master]
[also build test ERROR on next-20210413]
[cannot apply to net-next/master ipvs/master linus/master v5.12-rc7]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Marek-Beh-n/net-phy-marvell-some-HWMON-updates/20210413-155751
base:   https://git.kernel.org/pub/scm/linux/kernel/git/davem/net.git ccb39c6285581992f0225c45e4de704028a8ec17
config: x86_64-randconfig-a011-20210413 (attached as .config)
compiler: clang version 13.0.0 (https://github.com/llvm/llvm-project 9829f5e6b1bca9b61efc629770d28bb9014dec45)
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # install x86_64 cross compiling tool for clang build
        # apt-get install binutils-x86-64-linux-gnu
        # https://github.com/0day-ci/linux/commit/594b70c48fa643c6864722ab488bbfba0b210852
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Marek-Beh-n/net-phy-marvell-some-HWMON-updates/20210413-155751
        git checkout 594b70c48fa643c6864722ab488bbfba0b210852
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross ARCH=x86_64 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

>> drivers/net/phy/marvell.c:2416:3: error: fallthrough annotation does not directly precede switch label
                   fallthrough;
                   ^
   include/linux/compiler_attributes.h:208:41: note: expanded from macro 'fallthrough'
   # define fallthrough                    __attribute__((__fallthrough__))
                                           ^
   1 error generated.


vim +2416 drivers/net/phy/marvell.c

  2394	
  2395	static int marvell_hwmon_read(struct device *dev, enum hwmon_sensor_types type,
  2396				      u32 attr, int channel, long *temp)
  2397	{
  2398		struct phy_device *phydev = dev_get_drvdata(dev);
  2399		const struct marvell_hwmon_ops *ops = to_marvell_hwmon_ops(phydev);
  2400		int err = -EOPNOTSUPP;
  2401	
  2402		switch (attr) {
  2403		case hwmon_temp_input:
  2404			if (ops->get_temp)
  2405				err = ops->get_temp(phydev, temp);
  2406			break;
  2407		case hwmon_temp_crit:
  2408			if (ops->get_temp_critical)
  2409				err = ops->get_temp_critical(phydev, temp);
  2410			break;
  2411		case hwmon_temp_max_alarm:
  2412			if (ops->get_temp_alarm)
  2413				err = ops->get_temp_alarm(phydev, temp);
  2414			break;
  2415		default:
> 2416			fallthrough;
  2417		}
  2418	
  2419		return err;
  2420	}
  2421	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 31878 bytes --]

Re: [PATCH net-next 4/5] net: dsa: mv88e6xxx: simulate Amethyst PHY model number

[Andrew Lunn]

On Tue, Apr 13, 2021 at 09:55:37AM +0200, Marek Behún wrote:
> Amethyst internal PHYs also report empty model number in MII_PHYSID2.
> 
> Fill in switch product number, as is done for Topaz and Peridot.
> 
> Signed-off-by: Marek Behún <kabel@kernel.org>

Reviewed-by: Andrew Lunn <andrew@lunn.ch>

    Andrew

Re: [PATCH net-next 5/5] net: phy: marvell: add support for Amethyst internal PHY

[Andrew Lunn]

>  #define MII_88E6390_MISC_TEST_TEMP_SENSOR_ENABLE_ONESHOT	(0x2 << 14)
>  #define MII_88E6390_MISC_TEST_TEMP_SENSOR_DISABLE		(0x3 << 14)
>  #define MII_88E6390_MISC_TEST_TEMP_SENSOR_MASK			(0x3 << 14)
> +#define MII_88E6393_MISC_TEST_SAMPLES_4096	0x0000
> +#define MII_88E6393_MISC_TEST_SAMPLES_8192	0x0800
> +#define MII_88E6393_MISC_TEST_SAMPLES_16384	0x1000
> +#define MII_88E6393_MISC_TEST_SAMPLES_32768	0x1800
> +#define MII_88E6393_MISC_TEST_SAMPLES_MASK	0x1800

Please represent these as (0x0 << 11), (0x1 << 11) etc. It makes it
easier to map it back to the datasheet which always talks about values
in bit fields, now the complete word.

> +#define MII_88E6393_MISC_TEST_RATE_2_3MS	0x0500
> +#define MII_88E6393_MISC_TEST_RATE_6_4MS	0x0600
> +#define MII_88E6393_MISC_TEST_RATE_11_9MS	0x0700
> +#define MII_88E6393_MISC_TEST_RATE_MASK		0x0700

Same here.

     Andrew

Re: [PATCH net-next 1/5] net: phy: marvell: refactor HWMON OOP style

[Andrew Lunn]

> +static int marvell_hwmon_read(struct device *dev, enum hwmon_sensor_types type,
> +			      u32 attr, int channel, long *temp)
>  {
>  	struct phy_device *phydev = dev_get_drvdata(dev);
> -	int err;
> +	const struct marvell_hwmon_ops *ops = to_marvell_hwmon_ops(phydev);
> +	int err = -EOPNOTSUPP;
>  
>  	switch (attr) {
>  	case hwmon_temp_input:
> -		err = m88e6390_get_temp(phydev, temp);
> +		if (ops->get_temp)
> +			err = ops->get_temp(phydev, temp);
> +		break;
> +	case hwmon_temp_crit:
> +		if (ops->get_temp_critical)
> +			err = ops->get_temp_critical(phydev, temp);
> +		break;
> +	case hwmon_temp_max_alarm:
> +		if (ops->get_temp_alarm)
> +			err = ops->get_temp_alarm(phydev, temp);
>  		break;
>  	default:
> -		return -EOPNOTSUPP;
> +		fallthrough;
> +	}

Does the default clause actually service any purpose?

And it is not falling through, it is falling out :-)

    Andrew

Re: [PATCH net-next 1/5] net: phy: marvell: refactor HWMON OOP style

[Marek Behún]

On Tue, 13 Apr 2021 16:36:35 +0200
Andrew Lunn <andrew@lunn.ch> wrote:

> > +static int marvell_hwmon_read(struct device *dev, enum hwmon_sensor_types type,
> > +			      u32 attr, int channel, long *temp)
> >  {
> >  	struct phy_device *phydev = dev_get_drvdata(dev);
> > -	int err;
> > +	const struct marvell_hwmon_ops *ops = to_marvell_hwmon_ops(phydev);
> > +	int err = -EOPNOTSUPP;
> >  
> >  	switch (attr) {
> >  	case hwmon_temp_input:
> > -		err = m88e6390_get_temp(phydev, temp);
> > +		if (ops->get_temp)
> > +			err = ops->get_temp(phydev, temp);
> > +		break;
> > +	case hwmon_temp_crit:
> > +		if (ops->get_temp_critical)
> > +			err = ops->get_temp_critical(phydev, temp);
> > +		break;
> > +	case hwmon_temp_max_alarm:
> > +		if (ops->get_temp_alarm)
> > +			err = ops->get_temp_alarm(phydev, temp);
> >  		break;
> >  	default:
> > -		return -EOPNOTSUPP;
> > +		fallthrough;
> > +	}  
> 
> Does the default clause actually service any purpose?
> 
> And it is not falling through, it is falling out :-)
> 
>     Andrew

Seem like I forgot to remove a line :)

Re: [PATCH net-next 2/5] net: phy: marvell: fix HWMON enable register for 6390

[Marek Behún]

On Tue, 13 Apr 2021 16:25:33 +0200
Andrew Lunn <andrew@lunn.ch> wrote:

> On Tue, Apr 13, 2021 at 09:55:35AM +0200, Marek Behún wrote:
> > Register 27_6.15:14 has the following description in 88E6393X
> > documentation:
> >   Temperature Sensor Enable
> >     0x0 - Sample every 1s
> >     0x1 - Sense rate decided by bits 10:8 of this register
> >     0x2 - Use 26_6.5 (One shot Temperature Sample) to enable
> >     0x3 - Disable
> > 
> > This is compatible with how the 6390 code uses this register currently,
> > but the 6390 code handles it as two 1-bit registers (somewhat), instead
> > of one register with 4 possible values.
> > 
> > Rename this register and define all 4 values according to 6393X
> > documentation.
> > 
> > Signed-off-by: Marek Behún <kabel@kernel.org>
> > ---
> >  drivers/net/phy/marvell.c | 19 +++++++++----------
> >  1 file changed, 9 insertions(+), 10 deletions(-)
> > 
> > diff --git a/drivers/net/phy/marvell.c b/drivers/net/phy/marvell.c
> > index 63788d5c13eb..bae2a225b550 100644
> > --- a/drivers/net/phy/marvell.c
> > +++ b/drivers/net/phy/marvell.c
> > @@ -113,11 +113,11 @@
> >  #define MII_88E1540_COPPER_CTRL3_FAST_LINK_DOWN		BIT(9)
> >  
> >  #define MII_88E6390_MISC_TEST		0x1b
> > -#define MII_88E6390_MISC_TEST_SAMPLE_1S		0
> > -#define MII_88E6390_MISC_TEST_SAMPLE_10MS	BIT(14)
> > -#define MII_88E6390_MISC_TEST_SAMPLE_DISABLE	BIT(15)
> > -#define MII_88E6390_MISC_TEST_SAMPLE_ENABLE	0
> > -#define MII_88E6390_MISC_TEST_SAMPLE_MASK	(0x3 << 14)
> > +#define MII_88E6390_MISC_TEST_TEMP_SENSOR_ENABLE_SAMPLE_1S	(0x0 << 14)
> > +#define MII_88E6390_MISC_TEST_TEMP_SENSOR_ENABLE		(0x1 << 14)
> > +#define MII_88E6390_MISC_TEST_TEMP_SENSOR_ENABLE_ONESHOT	(0x2 << 14)
> > +#define MII_88E6390_MISC_TEST_TEMP_SENSOR_DISABLE		(0x3 << 14)
> > +#define MII_88E6390_MISC_TEST_TEMP_SENSOR_MASK			(0x3 << 14)
> >  
> >  #define MII_88E6390_TEMP_SENSOR		0x1c
> >  #define MII_88E6390_TEMP_SENSOR_MASK	0xff
> > @@ -2352,9 +2352,8 @@ static int m88e6390_get_temp(struct phy_device *phydev, long *temp)
> >  	if (ret < 0)
> >  		goto error;
> >  
> > -	ret = ret & ~MII_88E6390_MISC_TEST_SAMPLE_MASK;
> > -	ret |= MII_88E6390_MISC_TEST_SAMPLE_ENABLE |
> > -		MII_88E6390_MISC_TEST_SAMPLE_1S;
> > +	ret = ret & ~MII_88E6390_MISC_TEST_TEMP_SENSOR_MASK;
> > +	ret |= MII_88E6390_MISC_TEST_TEMP_SENSOR_ENABLE_SAMPLE_1S;  
> 
> So this is identical
> 
> >  
> >  	ret = __phy_write(phydev, MII_88E6390_MISC_TEST, ret);
> >  	if (ret < 0)
> > @@ -2381,8 +2380,8 @@ static int m88e6390_get_temp(struct phy_device *phydev, long *temp)
> >  	if (ret < 0)
> >  		goto error;
> >  
> > -	ret = ret & ~MII_88E6390_MISC_TEST_SAMPLE_MASK;
> > -	ret |= MII_88E6390_MISC_TEST_SAMPLE_DISABLE;
> > +	ret = ret & ~MII_88E6390_MISC_TEST_TEMP_SENSOR_MASK;
> > +	ret |= MII_88E6390_MISC_TEST_TEMP_SENSOR_DISABLE;  
> 
> And here we have gone from 0x2 to 0x3?
> 
> Have you checked the 6390 datasheet for this?
> 
> I will test these patches later.
> 
>      Andrew

The 6390 datasheet does not contain specification for temperature
sensor anymore. I will look into older versions.