[PATCH v5 net-next 08/13] net: phy: marvell-88q2xxx: add support for temperature sensor

[PATCH v5 net-next 08/13] net: phy: marvell-88q2xxx: add support for temperature sensor

[Dimitri Fedrau]

Marvell 88q2xxx devices have an inbuilt temperature sensor. Add hwmon
support for this sensor.

Signed-off-by: Dimitri Fedrau <dima.fedrau@gmail.com>
---
 drivers/net/phy/marvell-88q2xxx.c | 152 ++++++++++++++++++++++++++++++
 1 file changed, 152 insertions(+)

diff --git a/drivers/net/phy/marvell-88q2xxx.c b/drivers/net/phy/marvell-88q2xxx.c
index 4cb8fe524795..6900bad275d0 100644
--- a/drivers/net/phy/marvell-88q2xxx.c
+++ b/drivers/net/phy/marvell-88q2xxx.c
@@ -5,6 +5,7 @@
 #include <linux/ethtool_netlink.h>
 #include <linux/marvell_phy.h>
 #include <linux/phy.h>
+#include <linux/hwmon.h>
 
 #define PHY_ID_88Q2220_REVB0	(MARVELL_PHY_ID_88Q2220 | 0x1)
 
@@ -33,6 +34,19 @@
 #define MDIO_MMD_PCS_MV_GPIO_INT_CTRL			32787
 #define MDIO_MMD_PCS_MV_GPIO_INT_CTRL_TRI_DIS		0x0800
 
+#define MDIO_MMD_PCS_MV_TEMP_SENSOR1			32833
+#define MDIO_MMD_PCS_MV_TEMP_SENSOR1_RAW_INT		0x0001
+#define MDIO_MMD_PCS_MV_TEMP_SENSOR1_INT		0x0040
+#define MDIO_MMD_PCS_MV_TEMP_SENSOR1_INT_EN		0x0080
+
+#define MDIO_MMD_PCS_MV_TEMP_SENSOR2			32834
+#define MDIO_MMD_PCS_MV_TEMP_SENSOR2_DIS_MASK		0xc000
+
+#define MDIO_MMD_PCS_MV_TEMP_SENSOR3			32835
+#define MDIO_MMD_PCS_MV_TEMP_SENSOR3_INT_THRESH_MASK	0xff00
+#define MDIO_MMD_PCS_MV_TEMP_SENSOR3_INT_THRESH_SHIFT	8
+#define MDIO_MMD_PCS_MV_TEMP_SENSOR3_MASK		0x00ff
+
 #define MDIO_MMD_PCS_MV_100BT1_STAT1			33032
 #define MDIO_MMD_PCS_MV_100BT1_STAT1_IDLE_ERROR		0x00ff
 #define MDIO_MMD_PCS_MV_100BT1_STAT1_JABBER		0x0100
@@ -488,6 +502,143 @@ static int mv88q2xxx_resume(struct phy_device *phydev)
 	return phy_clear_bits_mmd(phydev, MDIO_MMD_PMAPMD, MDIO_CTRL1,
 				  MDIO_CTRL1_LPOWER);
 }
+#ifdef CONFIG_HWMON
+static const struct hwmon_channel_info * const mv88q2xxx_hwmon_info[] = {
+	HWMON_CHANNEL_INFO(temp, HWMON_T_INPUT | HWMON_T_MAX | HWMON_T_ALARM),
+	NULL
+};
+
+static umode_t mv88q2xxx_hwmon_is_visible(const void *data,
+					  enum hwmon_sensor_types type,
+					  u32 attr, int channel)
+{
+	switch (attr) {
+	case hwmon_temp_input:
+		return 0444;
+	case hwmon_temp_max:
+		return 0644;
+	case hwmon_temp_alarm:
+		return 0444;
+	default:
+		return 0;
+	}
+}
+
+static int mv88q2xxx_hwmon_read(struct device *dev,
+				enum hwmon_sensor_types type,
+				u32 attr, int channel, long *val)
+{
+	struct phy_device *phydev = dev_get_drvdata(dev);
+	int ret;
+
+	switch (attr) {
+	case hwmon_temp_input:
+		ret = phy_read_mmd(phydev, MDIO_MMD_PCS,
+				   MDIO_MMD_PCS_MV_TEMP_SENSOR3);
+		if (ret < 0)
+			return ret;
+
+		*val = ((ret & MDIO_MMD_PCS_MV_TEMP_SENSOR3_MASK) - 75) * 1000;
+		return 0;
+	case hwmon_temp_max:
+		ret = phy_read_mmd(phydev, MDIO_MMD_PCS,
+				   MDIO_MMD_PCS_MV_TEMP_SENSOR3);
+		if (ret < 0)
+			return ret;
+
+		*val = (((ret & MDIO_MMD_PCS_MV_TEMP_SENSOR3_INT_THRESH_MASK) >>
+			MDIO_MMD_PCS_MV_TEMP_SENSOR3_INT_THRESH_SHIFT) - 75) *
+			1000;
+		return 0;
+	case hwmon_temp_alarm:
+		ret = phy_read_mmd(phydev, MDIO_MMD_PCS,
+				   MDIO_MMD_PCS_MV_TEMP_SENSOR1);
+		if (ret < 0)
+			return ret;
+
+		*val = !!(ret & MDIO_MMD_PCS_MV_TEMP_SENSOR1_RAW_INT);
+		return 0;
+	default:
+		return -EOPNOTSUPP;
+	}
+}
+
+static int mv88q2xxx_hwmon_write(struct device *dev,
+				 enum hwmon_sensor_types type, u32 attr,
+				 int channel, long val)
+{
+	struct phy_device *phydev = dev_get_drvdata(dev);
+
+	switch (attr) {
+	case hwmon_temp_max:
+		if (val < -75000 || val > 180000)
+			return -EINVAL;
+
+		val = ((val / 1000) + 75) <<
+		       MDIO_MMD_PCS_MV_TEMP_SENSOR3_INT_THRESH_SHIFT;
+		return phy_modify_mmd(phydev, MDIO_MMD_PCS,
+				      MDIO_MMD_PCS_MV_TEMP_SENSOR3,
+				      MDIO_MMD_PCS_MV_TEMP_SENSOR3_INT_THRESH_MASK,
+				      val);
+	default:
+		return -EOPNOTSUPP;
+	}
+}
+
+static const struct hwmon_ops mv88q2xxx_hwmon_hwmon_ops = {
+	.is_visible = mv88q2xxx_hwmon_is_visible,
+	.read = mv88q2xxx_hwmon_read,
+	.write = mv88q2xxx_hwmon_write,
+};
+
+static const struct hwmon_chip_info mv88q2xxx_hwmon_chip_info = {
+	.ops = &mv88q2xxx_hwmon_hwmon_ops,
+	.info = mv88q2xxx_hwmon_info,
+};
+
+static int mv88q2xxx_hwmon_probe(struct phy_device *phydev)
+{
+	struct device *dev = &phydev->mdio.dev;
+	struct device *hwmon;
+	char *hwmon_name;
+	int ret;
+
+	/* Enable temperature sensor interrupt */
+	ret = phy_set_bits_mmd(phydev, MDIO_MMD_PCS,
+			       MDIO_MMD_PCS_MV_TEMP_SENSOR1,
+			       MDIO_MMD_PCS_MV_TEMP_SENSOR1_INT_EN);
+	if (ret < 0)
+		return ret;
+
+	/* Enable temperature sense */
+	ret = phy_modify_mmd(phydev, MDIO_MMD_PCS, MDIO_MMD_PCS_MV_TEMP_SENSOR2,
+			     MDIO_MMD_PCS_MV_TEMP_SENSOR2_DIS_MASK, 0);
+	if (ret < 0)
+		return ret;
+
+	hwmon_name = devm_hwmon_sanitize_name(dev, dev_name(dev));
+	if (IS_ERR(hwmon_name))
+		return PTR_ERR(hwmon_name);
+
+	hwmon = devm_hwmon_device_register_with_info(dev,
+						     hwmon_name,
+						     phydev,
+						     &mv88q2xxx_hwmon_chip_info,
+						     NULL);
+
+	return PTR_ERR_OR_ZERO(hwmon);
+}
+
+#else
+static int mv88q2xxx_hwmon_probe(struct phy_device *phydev)
+{
+	return 0;
+}
+#endif
+static int mv88q2xxx_probe(struct phy_device *phydev)
+{
+	return mv88q2xxx_hwmon_probe(phydev);
+}
 
 static int mv88q222x_soft_reset(struct phy_device *phydev)
 {
@@ -583,6 +734,7 @@ static struct phy_driver mv88q2xxx_driver[] = {
 	{
 		PHY_ID_MATCH_EXACT(PHY_ID_88Q2220_REVB0),
 		.name			= "mv88q2220",
+		.probe			= mv88q2xxx_probe,
 		.get_features		= mv88q2xxx_get_features,
 		.config_aneg		= mv88q222x_config_aneg,
 		.aneg_done		= genphy_c45_aneg_done,
-- 
2.39.2

Re: [PATCH v5 net-next 08/13] net: phy: marvell-88q2xxx: add support for temperature sensor

[Russell King (Oracle)]

On Mon, Jan 22, 2024 at 10:28:41PM +0100, Dimitri Fedrau wrote:

	int tmp;

> +	switch (attr) {
> +	case hwmon_temp_input:
> +		ret = phy_read_mmd(phydev, MDIO_MMD_PCS,
> +				   MDIO_MMD_PCS_MV_TEMP_SENSOR3);
> +		if (ret < 0)
> +			return ret;
> +
> +		*val = ((ret & MDIO_MMD_PCS_MV_TEMP_SENSOR3_MASK) - 75) * 1000;

		tmp = FIELD_GET(MDIO_MMD_PCS_MV_TEMP_SENSOR3_MASK, ret);
		*val = (tmp - 75) * 1000;

> +		return 0;
> +	case hwmon_temp_max:
> +		ret = phy_read_mmd(phydev, MDIO_MMD_PCS,
> +				   MDIO_MMD_PCS_MV_TEMP_SENSOR3);
> +		if (ret < 0)
> +			return ret;
> +
> +		*val = (((ret & MDIO_MMD_PCS_MV_TEMP_SENSOR3_INT_THRESH_MASK) >>
> +			MDIO_MMD_PCS_MV_TEMP_SENSOR3_INT_THRESH_SHIFT) - 75) *
> +			1000;

		tmp = FIELD_GET(MDIO_MMD_PCS_MV_TEMP_SENSOR3_INT_THRESH_MASK,
				ret);
		*val = (tmp - 75) * 1000;

> +		return 0;
> +	case hwmon_temp_alarm:
> +		ret = phy_read_mmd(phydev, MDIO_MMD_PCS,
> +				   MDIO_MMD_PCS_MV_TEMP_SENSOR1);
> +		if (ret < 0)
> +			return ret;
> +
> +		*val = !!(ret & MDIO_MMD_PCS_MV_TEMP_SENSOR1_RAW_INT);
> +		return 0;
> +	default:
> +		return -EOPNOTSUPP;
> +	}
> +}
> +
> +static int mv88q2xxx_hwmon_write(struct device *dev,
> +				 enum hwmon_sensor_types type, u32 attr,
> +				 int channel, long val)
> +{
> +	struct phy_device *phydev = dev_get_drvdata(dev);
> +
> +	switch (attr) {
> +	case hwmon_temp_max:
> +		if (val < -75000 || val > 180000)
> +			return -EINVAL;
> +
> +		val = ((val / 1000) + 75) <<
> +		       MDIO_MMD_PCS_MV_TEMP_SENSOR3_INT_THRESH_SHIFT;

		val = (val / 1000) + 75;
		val = FIELD_PREP(MDIO_MMD_PCS_MV_TEMP_SENSOR3_INT_THRESH_MASK,
				 val);

... and therefore no need for the _SHIFT constants.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH v5 net-next 08/13] net: phy: marvell-88q2xxx: add support for temperature sensor

[Dimitri Fedrau]

Am Tue, Jan 30, 2024 at 09:18:31AM +0000 schrieb Russell King (Oracle):
> On Mon, Jan 22, 2024 at 10:28:41PM +0100, Dimitri Fedrau wrote:
> 
> 	int tmp;
> 
> > +	switch (attr) {
> > +	case hwmon_temp_input:
> > +		ret = phy_read_mmd(phydev, MDIO_MMD_PCS,
> > +				   MDIO_MMD_PCS_MV_TEMP_SENSOR3);
> > +		if (ret < 0)
> > +			return ret;
> > +
> > +		*val = ((ret & MDIO_MMD_PCS_MV_TEMP_SENSOR3_MASK) - 75) * 1000;
> 
> 		tmp = FIELD_GET(MDIO_MMD_PCS_MV_TEMP_SENSOR3_MASK, ret);
> 		*val = (tmp - 75) * 1000;
> 
> > +		return 0;
> > +	case hwmon_temp_max:
> > +		ret = phy_read_mmd(phydev, MDIO_MMD_PCS,
> > +				   MDIO_MMD_PCS_MV_TEMP_SENSOR3);
> > +		if (ret < 0)
> > +			return ret;
> > +
> > +		*val = (((ret & MDIO_MMD_PCS_MV_TEMP_SENSOR3_INT_THRESH_MASK) >>
> > +			MDIO_MMD_PCS_MV_TEMP_SENSOR3_INT_THRESH_SHIFT) - 75) *
> > +			1000;
> 
> 		tmp = FIELD_GET(MDIO_MMD_PCS_MV_TEMP_SENSOR3_INT_THRESH_MASK,
> 				ret);
> 		*val = (tmp - 75) * 1000;
> 
> > +		return 0;
> > +	case hwmon_temp_alarm:
> > +		ret = phy_read_mmd(phydev, MDIO_MMD_PCS,
> > +				   MDIO_MMD_PCS_MV_TEMP_SENSOR1);
> > +		if (ret < 0)
> > +			return ret;
> > +
> > +		*val = !!(ret & MDIO_MMD_PCS_MV_TEMP_SENSOR1_RAW_INT);
> > +		return 0;
> > +	default:
> > +		return -EOPNOTSUPP;
> > +	}
> > +}
> > +
> > +static int mv88q2xxx_hwmon_write(struct device *dev,
> > +				 enum hwmon_sensor_types type, u32 attr,
> > +				 int channel, long val)
> > +{
> > +	struct phy_device *phydev = dev_get_drvdata(dev);
> > +
> > +	switch (attr) {
> > +	case hwmon_temp_max:
> > +		if (val < -75000 || val > 180000)
> > +			return -EINVAL;
> > +
> > +		val = ((val / 1000) + 75) <<
> > +		       MDIO_MMD_PCS_MV_TEMP_SENSOR3_INT_THRESH_SHIFT;
> 
> 		val = (val / 1000) + 75;
> 		val = FIELD_PREP(MDIO_MMD_PCS_MV_TEMP_SENSOR3_INT_THRESH_MASK,
> 				 val);
> 
> ... and therefore no need for the _SHIFT constants.
>
Will fix it.
> -- 
> RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
> FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH v5 net-next 08/13] net: phy: marvell-88q2xxx: add support for temperature sensor

[Andrew Lunn]

> +static int mv88q2xxx_hwmon_probe(struct phy_device *phydev)
> +{
> +	struct device *dev = &phydev->mdio.dev;
> +	struct device *hwmon;
> +	char *hwmon_name;
> +	int ret;
> +
> +	/* Enable temperature sensor interrupt */
> +	ret = phy_set_bits_mmd(phydev, MDIO_MMD_PCS,
> +			       MDIO_MMD_PCS_MV_TEMP_SENSOR1,
> +			       MDIO_MMD_PCS_MV_TEMP_SENSOR1_INT_EN);

You enable an interrupt, but i don't see any changes to the interrupt
handler to handle any interrupts which are generated?

	Andrew

Re: [PATCH v5 net-next 08/13] net: phy: marvell-88q2xxx: add support for temperature sensor

[Dimitri Fedrau]

Am Wed, Jan 31, 2024 at 04:17:06PM +0100 schrieb Andrew Lunn:
> > +static int mv88q2xxx_hwmon_probe(struct phy_device *phydev)
> > +{
> > +	struct device *dev = &phydev->mdio.dev;
> > +	struct device *hwmon;
> > +	char *hwmon_name;
> > +	int ret;
> > +
> > +	/* Enable temperature sensor interrupt */
> > +	ret = phy_set_bits_mmd(phydev, MDIO_MMD_PCS,
> > +			       MDIO_MMD_PCS_MV_TEMP_SENSOR1,
> > +			       MDIO_MMD_PCS_MV_TEMP_SENSOR1_INT_EN);
> 
> You enable an interrupt, but i don't see any changes to the interrupt
> handler to handle any interrupts which are generated?
>
Hi Andrew,

you are right. Have to remove these lines. Besides enabling the interrupt
in MDIO_MMD_PCS_MV_TEMP_SENSOR1, there are two further register writes
necessary to make the interrupt propagate. I didn't want it to propagate.
Anyway it's wrong. I couldn't find a good solution to use the temperature
interrupt. Will have a look into this, and probably figuring out how to
do so. But it won't be part of this patch series.

Dimitri

Re: [PATCH v5 net-next 08/13] net: phy: marvell-88q2xxx: add support for temperature sensor

[Andrew Lunn]

> Anyway it's wrong. I couldn't find a good solution to use the temperature
> interrupt. Will have a look into this, and probably figuring out how to
> do so. But it won't be part of this patch series.

I don't know of any PHY driver you can follow, those that do have a
temperature sensor just report the temperature and don't do anything
in addition.

You might need to look at thermal zones, and indicate there has been a
thermal trip point. That could then be used by the thermal subsystem
to increase cooling via a fan, etc. In theory, you could also make the
PHY active to thermal pressure, by forcing the link to renegotiate to
a lower link speed. If you decide to go this route, please try to make
is generic to any PHY. But its going to be quite a disruptive thing,
the link will be lost of a little over a second...

   Andrew

Re: [PATCH v5 net-next 08/13] net: phy: marvell-88q2xxx: add support for temperature sensor

[Dimitri Fedrau]

Am Thu, Feb 01, 2024 at 02:23:15PM +0100 schrieb Andrew Lunn:
> > Anyway it's wrong. I couldn't find a good solution to use the temperature
> > interrupt. Will have a look into this, and probably figuring out how to
> > do so. But it won't be part of this patch series.
> 
> I don't know of any PHY driver you can follow, those that do have a
> temperature sensor just report the temperature and don't do anything
> in addition.
> 
> You might need to look at thermal zones, and indicate there has been a
> thermal trip point. That could then be used by the thermal subsystem
> to increase cooling via a fan, etc. In theory, you could also make the
> PHY active to thermal pressure, by forcing the link to renegotiate to
> a lower link speed. If you decide to go this route, please try to make
> is generic to any PHY. But its going to be quite a disruptive thing,
> the link will be lost of a little over a second...
> 
Making the PHY active to thermal pressure sounds interesting. Will look
into this.

Re: [PATCH v5 net-next 08/13] net: phy: marvell-88q2xxx: add support for temperature sensor

[Guenter Roeck]

On 1/31/24 23:11, Dimitri Fedrau wrote:
> Am Wed, Jan 31, 2024 at 04:17:06PM +0100 schrieb Andrew Lunn:
>>> +static int mv88q2xxx_hwmon_probe(struct phy_device *phydev)
>>> +{
>>> +	struct device *dev = &phydev->mdio.dev;
>>> +	struct device *hwmon;
>>> +	char *hwmon_name;
>>> +	int ret;
>>> +
>>> +	/* Enable temperature sensor interrupt */
>>> +	ret = phy_set_bits_mmd(phydev, MDIO_MMD_PCS,
>>> +			       MDIO_MMD_PCS_MV_TEMP_SENSOR1,
>>> +			       MDIO_MMD_PCS_MV_TEMP_SENSOR1_INT_EN);
>>
>> You enable an interrupt, but i don't see any changes to the interrupt
>> handler to handle any interrupts which are generated?
>>
> Hi Andrew,
> 
> you are right. Have to remove these lines. Besides enabling the interrupt
> in MDIO_MMD_PCS_MV_TEMP_SENSOR1, there are two further register writes
> necessary to make the interrupt propagate. I didn't want it to propagate.
> Anyway it's wrong. I couldn't find a good solution to use the temperature
> interrupt. Will have a look into this, and probably figuring out how to
> do so. But it won't be part of this patch series.
> 

 From hwmon perspective, the expected use of such an interrupt would be
to call hwmon_notify_event() with the affected limit attribute as argument.
This would notify the thermal subsystem if the sensor is registered with it
(your patch doesn't set the necessary flag when registering the driver,
so this would not happen), it will send a notification to the sysfs
attribute, and generate a udev event.

Guenter

Re: [PATCH v5 net-next 08/13] net: phy: marvell-88q2xxx: add support for temperature sensor

[Dimitri Fedrau]

Am Thu, Feb 01, 2024 at 05:34:05AM -0800 schrieb Guenter Roeck:
> On 1/31/24 23:11, Dimitri Fedrau wrote:
> > Am Wed, Jan 31, 2024 at 04:17:06PM +0100 schrieb Andrew Lunn:
> > > > +static int mv88q2xxx_hwmon_probe(struct phy_device *phydev)
> > > > +{
> > > > +	struct device *dev = &phydev->mdio.dev;
> > > > +	struct device *hwmon;
> > > > +	char *hwmon_name;
> > > > +	int ret;
> > > > +
> > > > +	/* Enable temperature sensor interrupt */
> > > > +	ret = phy_set_bits_mmd(phydev, MDIO_MMD_PCS,
> > > > +			       MDIO_MMD_PCS_MV_TEMP_SENSOR1,
> > > > +			       MDIO_MMD_PCS_MV_TEMP_SENSOR1_INT_EN);
> > > 
> > > You enable an interrupt, but i don't see any changes to the interrupt
> > > handler to handle any interrupts which are generated?
> > > 
> > Hi Andrew,
> > 
> > you are right. Have to remove these lines. Besides enabling the interrupt
> > in MDIO_MMD_PCS_MV_TEMP_SENSOR1, there are two further register writes
> > necessary to make the interrupt propagate. I didn't want it to propagate.
> > Anyway it's wrong. I couldn't find a good solution to use the temperature
> > interrupt. Will have a look into this, and probably figuring out how to
> > do so. But it won't be part of this patch series.
> > 
> 
> From hwmon perspective, the expected use of such an interrupt would be
> to call hwmon_notify_event() with the affected limit attribute as argument.
> This would notify the thermal subsystem if the sensor is registered with it
> (your patch doesn't set the necessary flag when registering the driver,
> so this would not happen), it will send a notification to the sysfs
> attribute, and generate a udev event.
>
Thanks, noted it down. Didn't know about the notification to the thermal
subsystem and the generated udev event. :)

Dimitri

Re: [PATCH v5 net-next 08/13] net: phy: marvell-88q2xxx: add support for temperature sensor

[Guenter Roeck]

On Thu, Feb 01, 2024 at 05:14:35PM +0100, Dimitri Fedrau wrote:
> Am Thu, Feb 01, 2024 at 05:34:05AM -0800 schrieb Guenter Roeck:
> > On 1/31/24 23:11, Dimitri Fedrau wrote:
> > > Am Wed, Jan 31, 2024 at 04:17:06PM +0100 schrieb Andrew Lunn:
> > > > > +static int mv88q2xxx_hwmon_probe(struct phy_device *phydev)
> > > > > +{
> > > > > +	struct device *dev = &phydev->mdio.dev;
> > > > > +	struct device *hwmon;
> > > > > +	char *hwmon_name;
> > > > > +	int ret;
> > > > > +
> > > > > +	/* Enable temperature sensor interrupt */
> > > > > +	ret = phy_set_bits_mmd(phydev, MDIO_MMD_PCS,
> > > > > +			       MDIO_MMD_PCS_MV_TEMP_SENSOR1,
> > > > > +			       MDIO_MMD_PCS_MV_TEMP_SENSOR1_INT_EN);
> > > > 
> > > > You enable an interrupt, but i don't see any changes to the interrupt
> > > > handler to handle any interrupts which are generated?
> > > > 
> > > Hi Andrew,
> > > 
> > > you are right. Have to remove these lines. Besides enabling the interrupt
> > > in MDIO_MMD_PCS_MV_TEMP_SENSOR1, there are two further register writes
> > > necessary to make the interrupt propagate. I didn't want it to propagate.
> > > Anyway it's wrong. I couldn't find a good solution to use the temperature
> > > interrupt. Will have a look into this, and probably figuring out how to
> > > do so. But it won't be part of this patch series.
> > > 
> > 
> > From hwmon perspective, the expected use of such an interrupt would be
> > to call hwmon_notify_event() with the affected limit attribute as argument.
> > This would notify the thermal subsystem if the sensor is registered with it
> > (your patch doesn't set the necessary flag when registering the driver,
> > so this would not happen), it will send a notification to the sysfs
> > attribute, and generate a udev event.
> >
> Thanks, noted it down. Didn't know about the notification to the thermal
> subsystem and the generated udev event. :)
> 

Note that you'd have to add something like

	HWMON_CHANNEL_INFO(chip, HWMON_C_REGISTER_TZ),

to the code to register the sensor as thermal zone.

Guenter

Re: [PATCH v5 net-next 08/13] net: phy: marvell-88q2xxx: add support for temperature sensor

[Guenter Roeck]

On 1/22/24 13:28, Dimitri Fedrau wrote:
> Marvell 88q2xxx devices have an inbuilt temperature sensor. Add hwmon
> support for this sensor.
> 
> Signed-off-by: Dimitri Fedrau <dima.fedrau@gmail.com>
> ---
>   drivers/net/phy/marvell-88q2xxx.c | 152 ++++++++++++++++++++++++++++++
>   1 file changed, 152 insertions(+)
> 
> diff --git a/drivers/net/phy/marvell-88q2xxx.c b/drivers/net/phy/marvell-88q2xxx.c
> index 4cb8fe524795..6900bad275d0 100644
> --- a/drivers/net/phy/marvell-88q2xxx.c
> +++ b/drivers/net/phy/marvell-88q2xxx.c
> @@ -5,6 +5,7 @@
>   #include <linux/ethtool_netlink.h>
>   #include <linux/marvell_phy.h>
>   #include <linux/phy.h>
> +#include <linux/hwmon.h>
>   
>   #define PHY_ID_88Q2220_REVB0	(MARVELL_PHY_ID_88Q2220 | 0x1)
>   
> @@ -33,6 +34,19 @@
>   #define MDIO_MMD_PCS_MV_GPIO_INT_CTRL			32787
>   #define MDIO_MMD_PCS_MV_GPIO_INT_CTRL_TRI_DIS		0x0800
>   
> +#define MDIO_MMD_PCS_MV_TEMP_SENSOR1			32833
> +#define MDIO_MMD_PCS_MV_TEMP_SENSOR1_RAW_INT		0x0001
> +#define MDIO_MMD_PCS_MV_TEMP_SENSOR1_INT		0x0040
> +#define MDIO_MMD_PCS_MV_TEMP_SENSOR1_INT_EN		0x0080
> +
> +#define MDIO_MMD_PCS_MV_TEMP_SENSOR2			32834
> +#define MDIO_MMD_PCS_MV_TEMP_SENSOR2_DIS_MASK		0xc000
> +
> +#define MDIO_MMD_PCS_MV_TEMP_SENSOR3			32835
> +#define MDIO_MMD_PCS_MV_TEMP_SENSOR3_INT_THRESH_MASK	0xff00
> +#define MDIO_MMD_PCS_MV_TEMP_SENSOR3_INT_THRESH_SHIFT	8
> +#define MDIO_MMD_PCS_MV_TEMP_SENSOR3_MASK		0x00ff
> +
>   #define MDIO_MMD_PCS_MV_100BT1_STAT1			33032
>   #define MDIO_MMD_PCS_MV_100BT1_STAT1_IDLE_ERROR		0x00ff
>   #define MDIO_MMD_PCS_MV_100BT1_STAT1_JABBER		0x0100
> @@ -488,6 +502,143 @@ static int mv88q2xxx_resume(struct phy_device *phydev)
>   	return phy_clear_bits_mmd(phydev, MDIO_MMD_PMAPMD, MDIO_CTRL1,
>   				  MDIO_CTRL1_LPOWER);
>   }
> +#ifdef CONFIG_HWMON

HWMON is tristate, so this may be problematic if the driver is built
into the kernel and hwmon is built as module.

[ ... ]
> +
> +static int mv88q2xxx_hwmon_write(struct device *dev,
> +				 enum hwmon_sensor_types type, u32 attr,
> +				 int channel, long val)
> +{
> +	struct phy_device *phydev = dev_get_drvdata(dev);
> +
> +	switch (attr) {
> +	case hwmon_temp_max:
> +		if (val < -75000 || val > 180000)
> +			return -EINVAL;
> +

Not that it matters much, but we typically use clamp_val() to limit
the range of temperature limits because the valid range differs for
each chip and is otherwise difficult to determine for the user.

Guenter

Re: [PATCH v5 net-next 08/13] net: phy: marvell-88q2xxx: add support for temperature sensor

[Andrew Lunn]

> > +#ifdef CONFIG_HWMON
> 
> HWMON is tristate, so this may be problematic if the driver is built
> into the kernel and hwmon is built as module.

There should be Kconfig in addition to this, e.g.

config MAXLINEAR_GPHY
        tristate "Maxlinear Ethernet PHYs"
        select POLYNOMIAL if HWMON
        depends on HWMON || HWMON=n
        help
          Support for the Maxlinear GPY115, GPY211, GPY212, GPY215,
          GPY241, GPY245 PHYs.

So its forced to being built in, or not built at all.

	Andrew

Re: [PATCH v5 net-next 08/13] net: phy: marvell-88q2xxx: add support for temperature sensor

[Guenter Roeck]

On 2/1/24 05:27, Andrew Lunn wrote:
>>> +#ifdef CONFIG_HWMON
>>
>> HWMON is tristate, so this may be problematic if the driver is built
>> into the kernel and hwmon is built as module.
> 
> There should be Kconfig in addition to this, e.g.
> 
> config MAXLINEAR_GPHY
>          tristate "Maxlinear Ethernet PHYs"
>          select POLYNOMIAL if HWMON
>          depends on HWMON || HWMON=n
>          help
>            Support for the Maxlinear GPY115, GPY211, GPY212, GPY215,
>            GPY241, GPY245 PHYs.
> 
> So its forced to being built in, or not built at all.
> 

Even then it should be "#if IS_ENABLED(HWMON)" in the code.

Thanks,
Guenter

Re: [PATCH v5 net-next 08/13] net: phy: marvell-88q2xxx: add support for temperature sensor

[Dimitri Fedrau]

Am Thu, Feb 01, 2024 at 05:39:25AM -0800 schrieb Guenter Roeck:
> On 2/1/24 05:27, Andrew Lunn wrote:
> > > > +#ifdef CONFIG_HWMON
> > > 
> > > HWMON is tristate, so this may be problematic if the driver is built
> > > into the kernel and hwmon is built as module.
> > 
> > There should be Kconfig in addition to this, e.g.
> > 
> > config MAXLINEAR_GPHY
> >          tristate "Maxlinear Ethernet PHYs"
> >          select POLYNOMIAL if HWMON
> >          depends on HWMON || HWMON=n
> >          help
> >            Support for the Maxlinear GPY115, GPY211, GPY212, GPY215,
> >            GPY241, GPY245 PHYs.
> > 
> > So its forced to being built in, or not built at all.
> > 
> 
> Even then it should be "#if IS_ENABLED(HWMON)" in the code.
> 
>
If using "#if IS_ENABLED(HWMON)" do I have to add the dependency in
the KConfig file ? When looking at other PHY drivers, they do.

Dimitri

Re: [PATCH v5 net-next 08/13] net: phy: marvell-88q2xxx: add support for temperature sensor

[Andrew Lunn]

On Thu, Feb 01, 2024 at 05:23:49PM +0100, Dimitri Fedrau wrote:
> Am Thu, Feb 01, 2024 at 05:39:25AM -0800 schrieb Guenter Roeck:
> > On 2/1/24 05:27, Andrew Lunn wrote:
> > > > > +#ifdef CONFIG_HWMON
> > > > 
> > > > HWMON is tristate, so this may be problematic if the driver is built
> > > > into the kernel and hwmon is built as module.
> > > 
> > > There should be Kconfig in addition to this, e.g.
> > > 
> > > config MAXLINEAR_GPHY
> > >          tristate "Maxlinear Ethernet PHYs"
> > >          select POLYNOMIAL if HWMON
> > >          depends on HWMON || HWMON=n
> > >          help
> > >            Support for the Maxlinear GPY115, GPY211, GPY212, GPY215,
> > >            GPY241, GPY245 PHYs.
> > > 
> > > So its forced to being built in, or not built at all.
> > > 
> > 
> > Even then it should be "#if IS_ENABLED(HWMON)" in the code.
> > 
> >
> If using "#if IS_ENABLED(HWMON)" do I have to add the dependency in
> the KConfig file ? When looking at other PHY drivers, they do.

Please follow what other drivers do. Its easy to break the build,
resulting is undefined symbols. What we have now works.

	Andrew	  

Re: [PATCH v5 net-next 08/13] net: phy: marvell-88q2xxx: add support for temperature sensor

[Dimitri Fedrau]

Am Thu, Feb 01, 2024 at 05:47:21PM +0100 schrieb Andrew Lunn:
> On Thu, Feb 01, 2024 at 05:23:49PM +0100, Dimitri Fedrau wrote:
> > Am Thu, Feb 01, 2024 at 05:39:25AM -0800 schrieb Guenter Roeck:
> > > On 2/1/24 05:27, Andrew Lunn wrote:
> > > > > > +#ifdef CONFIG_HWMON
> > > > > 
> > > > > HWMON is tristate, so this may be problematic if the driver is built
> > > > > into the kernel and hwmon is built as module.
> > > > 
> > > > There should be Kconfig in addition to this, e.g.
> > > > 
> > > > config MAXLINEAR_GPHY
> > > >          tristate "Maxlinear Ethernet PHYs"
> > > >          select POLYNOMIAL if HWMON
> > > >          depends on HWMON || HWMON=n
> > > >          help
> > > >            Support for the Maxlinear GPY115, GPY211, GPY212, GPY215,
> > > >            GPY241, GPY245 PHYs.
> > > > 
> > > > So its forced to being built in, or not built at all.
> > > > 
> > > 
> > > Even then it should be "#if IS_ENABLED(HWMON)" in the code.
> > > 
> > >
> > If using "#if IS_ENABLED(HWMON)" do I have to add the dependency in
> > the KConfig file ? When looking at other PHY drivers, they do.
> 
> Please follow what other drivers do. Its easy to break the build,
> resulting is undefined symbols. What we have now works.

Sure.

Re: [PATCH v5 net-next 08/13] net: phy: marvell-88q2xxx: add support for temperature sensor

[Guenter Roeck]

On Thu, Feb 01, 2024 at 05:23:49PM +0100, Dimitri Fedrau wrote:
> Am Thu, Feb 01, 2024 at 05:39:25AM -0800 schrieb Guenter Roeck:
> > On 2/1/24 05:27, Andrew Lunn wrote:
> > > > > +#ifdef CONFIG_HWMON
> > > > 
> > > > HWMON is tristate, so this may be problematic if the driver is built
> > > > into the kernel and hwmon is built as module.
> > > 
> > > There should be Kconfig in addition to this, e.g.
> > > 
> > > config MAXLINEAR_GPHY
> > >          tristate "Maxlinear Ethernet PHYs"
> > >          select POLYNOMIAL if HWMON
> > >          depends on HWMON || HWMON=n
> > >          help
> > >            Support for the Maxlinear GPY115, GPY211, GPY212, GPY215,
> > >            GPY241, GPY245 PHYs.
> > > 
> > > So its forced to being built in, or not built at all.
> > > 
> > 
> > Even then it should be "#if IS_ENABLED(HWMON)" in the code.
> > 
> >
> If using "#if IS_ENABLED(HWMON)" do I have to add the dependency in
> the KConfig file ? When looking at other PHY drivers, they do.

Yes, to handle CONFIG_HWMON=m. Note that it is "IS_ENABLED(CONFIG_HWMON)"
      				                           ^^^^^^^

Guenter

Re: [PATCH v5 net-next 08/13] net: phy: marvell-88q2xxx: add support for temperature sensor

[Dimitri Fedrau]

Am Thu, Feb 01, 2024 at 05:18:23AM -0800 schrieb Guenter Roeck:
[...]
> > +
> > +static int mv88q2xxx_hwmon_write(struct device *dev,
> > +				 enum hwmon_sensor_types type, u32 attr,
> > +				 int channel, long val)
> > +{
> > +	struct phy_device *phydev = dev_get_drvdata(dev);
> > +
> > +	switch (attr) {
> > +	case hwmon_temp_max:
> > +		if (val < -75000 || val > 180000)
> > +			return -EINVAL;
> > +
> 
> Not that it matters much, but we typically use clamp_val() to limit
> the range of temperature limits because the valid range differs for
> each chip and is otherwise difficult to determine for the user.
>

Will fix it.

Dimitri