[PATCH net-next 0/2] net: phy: aquantia: add hwmon support

[PATCH net-next 0/2] net: phy: aquantia: add hwmon support

[Heiner Kallweit]

This series adds HWMON support for the temperature sensor and the
related alarms on the 107/108/109 chips.

Heiner Kallweit (2):
  net: phy: aquantia: rename aquantia.c to aquantia_main.c
  net: phy: aquantia: add hwmon support

 drivers/net/phy/Makefile                      |   1 +
 drivers/net/phy/aquantia_hwmon.c              | 263 ++++++++++++++++++
 drivers/net/phy/aquantia_hwmon.h              |  15 +
 .../net/phy/{aquantia.c => aquantia_main.c}   |   4 +
 4 files changed, 283 insertions(+)
 create mode 100644 drivers/net/phy/aquantia_hwmon.c
 create mode 100644 drivers/net/phy/aquantia_hwmon.h
 rename drivers/net/phy/{aquantia.c => aquantia_main.c} (98%)

-- 
2.20.1

[PATCH net-next 1/2] net: phy: aquantia: rename aquantia.c to aquantia_main.c

[Heiner Kallweit]

Rename aquantia.c to aquantia_main.c to be prepared for adding new
functionality to separate source code files.

Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
 drivers/net/phy/Makefile                        | 1 +
 drivers/net/phy/{aquantia.c => aquantia_main.c} | 0
 2 files changed, 1 insertion(+)
 rename drivers/net/phy/{aquantia.c => aquantia_main.c} (100%)

diff --git a/drivers/net/phy/Makefile b/drivers/net/phy/Makefile
index 5805c0b7d..b0845adaf 100644
--- a/drivers/net/phy/Makefile
+++ b/drivers/net/phy/Makefile
@@ -45,6 +45,7 @@ sfp-obj-$(CONFIG_SFP)		+= sfp-bus.o
 obj-y				+= $(sfp-obj-y) $(sfp-obj-m)
 
 obj-$(CONFIG_AMD_PHY)		+= amd.o
+aquantia-objs			+= aquantia_main.o
 obj-$(CONFIG_AQUANTIA_PHY)	+= aquantia.o
 obj-$(CONFIG_ASIX_PHY)		+= asix.o
 obj-$(CONFIG_AT803X_PHY)	+= at803x.o
diff --git a/drivers/net/phy/aquantia.c b/drivers/net/phy/aquantia_main.c
similarity index 100%
rename from drivers/net/phy/aquantia.c
rename to drivers/net/phy/aquantia_main.c
-- 
2.20.1

[PATCH net-next 2/2] net: phy: aquantia: add hwmon support

[Heiner Kallweit]

This adds HWMON support for the temperature sensor and the related
alarms on the 107/108/109 chips. This patch is based on work from
Nikita and Andrew. I added:
- support for changing alarm thresholds via sysfs
- move HWMON code to a separate source file to improve maintainability
- smaller changes like using IS_REACHABLE instead of ifdef
  (avoids problems if PHY driver is built in and HWMON is a module)

Signed-off-by: Nikita Yushchenko <nikita.yoush@cogentembedded.com>
Signed-off-by: Andrew Lunn <andrew@lunn.ch>
Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
 drivers/net/phy/Makefile         |   2 +-
 drivers/net/phy/aquantia_hwmon.c | 263 +++++++++++++++++++++++++++++++
 drivers/net/phy/aquantia_hwmon.h |  15 ++
 drivers/net/phy/aquantia_main.c  |   4 +
 4 files changed, 283 insertions(+), 1 deletion(-)
 create mode 100644 drivers/net/phy/aquantia_hwmon.c
 create mode 100644 drivers/net/phy/aquantia_hwmon.h

diff --git a/drivers/net/phy/Makefile b/drivers/net/phy/Makefile
index b0845adaf..c48596626 100644
--- a/drivers/net/phy/Makefile
+++ b/drivers/net/phy/Makefile
@@ -45,7 +45,7 @@ sfp-obj-$(CONFIG_SFP)		+= sfp-bus.o
 obj-y				+= $(sfp-obj-y) $(sfp-obj-m)
 
 obj-$(CONFIG_AMD_PHY)		+= amd.o
-aquantia-objs			+= aquantia_main.o
+aquantia-objs			+= aquantia_main.o aquantia_hwmon.o
 obj-$(CONFIG_AQUANTIA_PHY)	+= aquantia.o
 obj-$(CONFIG_ASIX_PHY)		+= asix.o
 obj-$(CONFIG_AT803X_PHY)	+= at803x.o
diff --git a/drivers/net/phy/aquantia_hwmon.c b/drivers/net/phy/aquantia_hwmon.c
new file mode 100644
index 000000000..c0dd695f6
--- /dev/null
+++ b/drivers/net/phy/aquantia_hwmon.c
@@ -0,0 +1,263 @@
+// SPDX-License-Identifier: GPL-2.0
+/* HWMON driver for Aquantia PHY
+ *
+ * Author: Nikita Yushchenko <nikita.yoush@cogentembedded.com>
+ * Author: Andrew Lunn <andrew@lunn.ch>
+ * Author: Heiner Kallweit <hkallweit1@gmail.com>
+ */
+
+#include <linux/phy.h>
+#include <linux/device.h>
+#include <linux/ctype.h>
+#include <linux/hwmon.h>
+
+#include "aquantia_hwmon.h"
+
+/* Vendor specific 1, MDIO_MMD_VEND1 */
+#define VEND1_THERMAL_PROV_HIGH_TEMP_FAIL	0xc421
+#define VEND1_THERMAL_PROV_LOW_TEMP_FAIL	0xc422
+#define VEND1_THERMAL_PROV_HIGH_TEMP_WARN	0xc423
+#define VEND1_THERMAL_PROV_LOW_TEMP_WARN	0xc424
+#define VEND1_THERMAL_STAT1			0xc820
+#define VEND1_THERMAL_STAT2			0xc821
+#define VEND1_THERMAL_STAT2_VALID		BIT(0)
+#define VEND1_GENERAL_STAT1			0xc830
+#define VEND1_GENERAL_STAT1_HIGH_TEMP_FAIL	BIT(14)
+#define VEND1_GENERAL_STAT1_LOW_TEMP_FAIL	BIT(13)
+#define VEND1_GENERAL_STAT1_HIGH_TEMP_WARN	BIT(12)
+#define VEND1_GENERAL_STAT1_LOW_TEMP_WARN	BIT(11)
+
+struct aqr_priv {
+	struct device *hwmon_dev;
+	char *hwmon_name;
+};
+
+#if IS_REACHABLE(CONFIG_HWMON)
+
+static umode_t aqr_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_min_alarm:
+	case hwmon_temp_max_alarm:
+	case hwmon_temp_lcrit_alarm:
+	case hwmon_temp_crit_alarm:
+		return 0444;
+	case hwmon_temp_min:
+	case hwmon_temp_max:
+	case hwmon_temp_lcrit:
+	case hwmon_temp_crit:
+		return 0644;
+	default:
+		return 0;
+	}
+}
+
+static int aqr_hwmon_get(struct phy_device *phydev, int reg, long *value)
+{
+	int temp = phy_read_mmd(phydev, MDIO_MMD_VEND1, reg);
+
+	if (temp < 0)
+		return temp;
+
+	/* register value is 2's complement with LSB = 1/256th degree Celsius */
+	if (temp > 0x8000)
+		temp -= 0x10000;
+
+	*value = temp * 1000 / 256;
+
+	return 0;
+}
+
+static int aqr_hwmon_set(struct phy_device *phydev, int reg, long value)
+{
+	int temp;
+
+	if (value >= 128000 || value < -128000)
+		return -ERANGE;
+
+	temp = value * 256 / 1000;
+
+	if (temp < 0)
+		temp += 0x10000;
+
+	return phy_write_mmd(phydev, MDIO_MMD_VEND1, reg, temp);
+}
+
+static int aqr_hwmon_status1(struct phy_device *phydev, int bit, long *value)
+{
+	int reg = phy_read_mmd(phydev, MDIO_MMD_VEND1, VEND1_GENERAL_STAT1);
+
+	if (reg < 0)
+		return reg;
+
+	*value = !!(reg & bit);
+
+	return 0;
+}
+
+static int aqr_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 reg;
+
+	if (type != hwmon_temp)
+		return -EOPNOTSUPP;
+
+	switch (attr) {
+	case hwmon_temp_input:
+		reg = phy_read_mmd(phydev, MDIO_MMD_VEND1,
+				   VEND1_THERMAL_STAT2);
+		if (reg < 0)
+			return reg;
+		if (!(reg & VEND1_THERMAL_STAT2_VALID))
+			return -EIO;
+
+		return aqr_hwmon_get(phydev, VEND1_THERMAL_STAT1, value);
+
+	case hwmon_temp_lcrit:
+		return aqr_hwmon_get(phydev, VEND1_THERMAL_PROV_LOW_TEMP_FAIL,
+				     value);
+	case hwmon_temp_min:
+		return aqr_hwmon_get(phydev, VEND1_THERMAL_PROV_LOW_TEMP_WARN,
+				     value);
+	case hwmon_temp_max:
+		return aqr_hwmon_get(phydev, VEND1_THERMAL_PROV_HIGH_TEMP_WARN,
+				     value);
+	case hwmon_temp_crit:
+		return aqr_hwmon_get(phydev, VEND1_THERMAL_PROV_HIGH_TEMP_FAIL,
+				     value);
+	case hwmon_temp_lcrit_alarm:
+		return aqr_hwmon_status1(phydev,
+					 VEND1_GENERAL_STAT1_LOW_TEMP_FAIL,
+					 value);
+	case hwmon_temp_min_alarm:
+		return aqr_hwmon_status1(phydev,
+					 VEND1_GENERAL_STAT1_LOW_TEMP_WARN,
+					 value);
+	case hwmon_temp_max_alarm:
+		return aqr_hwmon_status1(phydev,
+					 VEND1_GENERAL_STAT1_HIGH_TEMP_WARN,
+					 value);
+	case hwmon_temp_crit_alarm:
+		return aqr_hwmon_status1(phydev,
+					 VEND1_GENERAL_STAT1_HIGH_TEMP_FAIL,
+					 value);
+	default:
+		return -EOPNOTSUPP;
+	}
+}
+
+static int aqr_hwmon_write(struct device *dev, enum hwmon_sensor_types type,
+			   u32 attr, int channel, long value)
+{
+	struct phy_device *phydev = dev_get_drvdata(dev);
+
+	if (type != hwmon_temp)
+		return -EOPNOTSUPP;
+
+	switch (attr) {
+	case hwmon_temp_lcrit:
+		return aqr_hwmon_set(phydev, VEND1_THERMAL_PROV_LOW_TEMP_FAIL,
+				     value);
+	case hwmon_temp_min:
+		return aqr_hwmon_set(phydev, VEND1_THERMAL_PROV_LOW_TEMP_WARN,
+				     value);
+	case hwmon_temp_max:
+		return aqr_hwmon_set(phydev, VEND1_THERMAL_PROV_HIGH_TEMP_WARN,
+				     value);
+	case hwmon_temp_crit:
+		return aqr_hwmon_set(phydev, VEND1_THERMAL_PROV_HIGH_TEMP_FAIL,
+				     value);
+	default:
+		return -EOPNOTSUPP;
+	}
+}
+
+static const struct hwmon_ops aqr_hwmon_ops = {
+	.is_visible = aqr_hwmon_is_visible,
+	.read = aqr_hwmon_read,
+	.write = aqr_hwmon_write,
+};
+
+static u32 aqr_hwmon_chip_config[] = {
+	HWMON_C_REGISTER_TZ,
+	0,
+};
+
+static const struct hwmon_channel_info aqr_hwmon_chip = {
+	.type = hwmon_chip,
+	.config = aqr_hwmon_chip_config,
+};
+
+static u32 aqr_hwmon_temp_config[] = {
+	HWMON_T_INPUT |
+	HWMON_T_MAX | HWMON_T_MIN |
+	HWMON_T_MAX_ALARM | HWMON_T_MIN_ALARM |
+	HWMON_T_CRIT | HWMON_T_LCRIT |
+	HWMON_T_CRIT_ALARM | HWMON_T_LCRIT_ALARM,
+	0,
+};
+
+static const struct hwmon_channel_info aqr_hwmon_temp = {
+	.type = hwmon_temp,
+	.config = aqr_hwmon_temp_config,
+};
+
+static const struct hwmon_channel_info *aqr_hwmon_info[] = {
+	&aqr_hwmon_chip,
+	&aqr_hwmon_temp,
+	NULL,
+};
+
+static const struct hwmon_chip_info aqr_hwmon_chip_info = {
+	.ops = &aqr_hwmon_ops,
+	.info = aqr_hwmon_info,
+};
+
+static int aqr_hwmon_really_probe(struct phy_device *phydev)
+{
+	struct device *dev = &phydev->mdio.dev;
+	struct aqr_priv *priv = phydev->priv;
+	int i, j;
+
+	priv->hwmon_name = devm_kstrdup(dev, dev_name(dev), GFP_KERNEL);
+	if (!priv->hwmon_name)
+		return -ENOMEM;
+
+	for (i = j = 0; priv->hwmon_name[i]; i++) {
+		if (isalnum(priv->hwmon_name[i])) {
+			if (i != j)
+				priv->hwmon_name[j] = priv->hwmon_name[i];
+			j++;
+		}
+	}
+	priv->hwmon_name[j] = '\0';
+
+	priv->hwmon_dev = devm_hwmon_device_register_with_info(dev,
+				priv->hwmon_name, phydev,
+				&aqr_hwmon_chip_info, NULL);
+
+	return PTR_ERR_OR_ZERO(priv->hwmon_dev);
+}
+
+int aqr_hwmon_probe(struct phy_device *phydev)
+{
+	struct aqr_priv *priv;
+
+	priv = devm_kzalloc(&phydev->mdio.dev, sizeof(*priv), GFP_KERNEL);
+	if (!priv)
+		return -ENOMEM;
+
+	phydev->priv = priv;
+
+	return aqr_hwmon_really_probe(phydev);
+}
+
+#endif
diff --git a/drivers/net/phy/aquantia_hwmon.h b/drivers/net/phy/aquantia_hwmon.h
new file mode 100644
index 000000000..607e68758
--- /dev/null
+++ b/drivers/net/phy/aquantia_hwmon.h
@@ -0,0 +1,15 @@
+/* SPDX-License-Identifier: GPL-2.0
+ * HWMON driver for Aquantia PHY
+ *
+ * Author: Nikita Yushchenko <nikita.yoush@cogentembedded.com>
+ * Author: Andrew Lunn <andrew@lunn.ch>
+ * Author: Heiner Kallweit <hkallweit1@gmail.com>
+ */
+
+#include <linux/phy.h>
+
+#if IS_REACHABLE(CONFIG_HWMON)
+int aqr_hwmon_probe(struct phy_device *phydev);
+#else
+static inline int aqr_hwmon_probe(struct phy_device *phydev) { return 0; }
+#endif
diff --git a/drivers/net/phy/aquantia_main.c b/drivers/net/phy/aquantia_main.c
index 0f0eb5682..af6dc3588 100644
--- a/drivers/net/phy/aquantia_main.c
+++ b/drivers/net/phy/aquantia_main.c
@@ -12,6 +12,8 @@
 #include <linux/delay.h>
 #include <linux/phy.h>
 
+#include "aquantia_hwmon.h"
+
 #define PHY_ID_AQ1202	0x03a1b445
 #define PHY_ID_AQ2104	0x03a1b460
 #define PHY_ID_AQR105	0x03a1b4a2
@@ -231,6 +233,7 @@ static struct phy_driver aqr_driver[] = {
 	.name		= "Aquantia AQR107",
 	.aneg_done	= genphy_c45_aneg_done,
 	.get_features	= genphy_c45_pma_read_abilities,
+	.probe		= aqr_hwmon_probe,
 	.config_aneg    = aqr_config_aneg,
 	.config_intr	= aqr_config_intr,
 	.ack_interrupt	= aqr_ack_interrupt,
@@ -241,6 +244,7 @@ static struct phy_driver aqr_driver[] = {
 	.name		= "Aquantia AQCS109",
 	.aneg_done	= genphy_c45_aneg_done,
 	.get_features	= genphy_c45_pma_read_abilities,
+	.probe		= aqr_hwmon_probe,
 	.config_init	= aqcs109_config_init,
 	.config_aneg    = aqr_config_aneg,
 	.config_intr	= aqr_config_intr,
-- 
2.20.1

Re: [PATCH net-next 2/2] net: phy: aquantia: add hwmon support

[Andrew Lunn]

> diff --git a/drivers/net/phy/aquantia_hwmon.c b/drivers/net/phy/aquantia_hwmon.c
> new file mode 100644
> index 000000000..c0dd695f6
> --- /dev/null
> +++ b/drivers/net/phy/aquantia_hwmon.c
> @@ -0,0 +1,263 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/* HWMON driver for Aquantia PHY
> + *
> + * Author: Nikita Yushchenko <nikita.yoush@cogentembedded.com>
> + * Author: Andrew Lunn <andrew@lunn.ch>
> + * Author: Heiner Kallweit <hkallweit1@gmail.com>
> + */
> +
> +#include <linux/phy.h>
> +#include <linux/device.h>
> +#include <linux/ctype.h>
> +#include <linux/hwmon.h>
> +
> +#include "aquantia_hwmon.h"
> +
> +/* Vendor specific 1, MDIO_MMD_VEND1 */
> +#define VEND1_THERMAL_PROV_HIGH_TEMP_FAIL	0xc421
> +#define VEND1_THERMAL_PROV_LOW_TEMP_FAIL	0xc422
> +#define VEND1_THERMAL_PROV_HIGH_TEMP_WARN	0xc423
> +#define VEND1_THERMAL_PROV_LOW_TEMP_WARN	0xc424
> +#define VEND1_THERMAL_STAT1			0xc820
> +#define VEND1_THERMAL_STAT2			0xc821
> +#define VEND1_THERMAL_STAT2_VALID		BIT(0)
> +#define VEND1_GENERAL_STAT1			0xc830
> +#define VEND1_GENERAL_STAT1_HIGH_TEMP_FAIL	BIT(14)
> +#define VEND1_GENERAL_STAT1_LOW_TEMP_FAIL	BIT(13)
> +#define VEND1_GENERAL_STAT1_HIGH_TEMP_WARN	BIT(12)
> +#define VEND1_GENERAL_STAT1_LOW_TEMP_WARN	BIT(11)
> +
> +struct aqr_priv {
> +	struct device *hwmon_dev;
> +	char *hwmon_name;
> +};

Hi Heiner

It seems a bit odd having the driver private structure here. I expect
with time we are going to need other things in it which are not
HWMON. e.g many of the statistics counters are clear on read. So we
need to keep the running totals somewhere.

I would keep the probe code and the allocation of this structure in
the main driver file.

    Andrew

Re: [PATCH net-next 2/2] net: phy: aquantia: add hwmon support

[Heiner Kallweit]

On 24.02.2019 21:29, Andrew Lunn wrote:
>> diff --git a/drivers/net/phy/aquantia_hwmon.c b/drivers/net/phy/aquantia_hwmon.c
>> new file mode 100644
>> index 000000000..c0dd695f6
>> --- /dev/null
>> +++ b/drivers/net/phy/aquantia_hwmon.c
>> @@ -0,0 +1,263 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/* HWMON driver for Aquantia PHY
>> + *
>> + * Author: Nikita Yushchenko <nikita.yoush@cogentembedded.com>
>> + * Author: Andrew Lunn <andrew@lunn.ch>
>> + * Author: Heiner Kallweit <hkallweit1@gmail.com>
>> + */
>> +
>> +#include <linux/phy.h>
>> +#include <linux/device.h>
>> +#include <linux/ctype.h>
>> +#include <linux/hwmon.h>
>> +
>> +#include "aquantia_hwmon.h"
>> +
>> +/* Vendor specific 1, MDIO_MMD_VEND1 */
>> +#define VEND1_THERMAL_PROV_HIGH_TEMP_FAIL	0xc421
>> +#define VEND1_THERMAL_PROV_LOW_TEMP_FAIL	0xc422
>> +#define VEND1_THERMAL_PROV_HIGH_TEMP_WARN	0xc423
>> +#define VEND1_THERMAL_PROV_LOW_TEMP_WARN	0xc424
>> +#define VEND1_THERMAL_STAT1			0xc820
>> +#define VEND1_THERMAL_STAT2			0xc821
>> +#define VEND1_THERMAL_STAT2_VALID		BIT(0)
>> +#define VEND1_GENERAL_STAT1			0xc830
>> +#define VEND1_GENERAL_STAT1_HIGH_TEMP_FAIL	BIT(14)
>> +#define VEND1_GENERAL_STAT1_LOW_TEMP_FAIL	BIT(13)
>> +#define VEND1_GENERAL_STAT1_HIGH_TEMP_WARN	BIT(12)
>> +#define VEND1_GENERAL_STAT1_LOW_TEMP_WARN	BIT(11)
>> +
>> +struct aqr_priv {
>> +	struct device *hwmon_dev;
>> +	char *hwmon_name;
>> +};
> 
> Hi Heiner
> 
> It seems a bit odd having the driver private structure here. I expect
> with time we are going to need other things in it which are not
> HWMON. e.g many of the statistics counters are clear on read. So we
> need to keep the running totals somewhere.
> 
> I would keep the probe code and the allocation of this structure in
> the main driver file.
> 
I just see that we don't need struct aqr_priv at all for hwmon
because it uses device-managed versions of the relevant functions.
So we can add such a struct when it's actually needed.

>     Andrew
> 
Heiner

Re: [PATCH net-next 2/2] net: phy: aquantia: add hwmon support

[Andrew Lunn]

> I just see that we don't need struct aqr_priv at all for hwmon
> because it uses device-managed versions of the relevant functions.
> So we can add such a struct when it's actually needed.

Hi Heiner

Yes, i had not noticed that.

     Andrew