Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1550048 > unrolled thread

[PATCH net-next 0/3] net: dsa: restore HWMON support in dsa2

Back to article view | Back to linux.kernel

Contents

  [PATCH net-next 0/3] net: dsa: restore HWMON support in dsa2 Vivien Didelot <vivien.didelot@savoirfairelinux.com> - 2017-01-03 19:20 +0100
    [PATCH net-next 1/3] net: dsa: remove out label in dsa_switch_setup_one Vivien Didelot <vivien.didelot@savoirfairelinux.com> - 2017-01-03 19:20 +0100
      Re: [PATCH net-next 1/3] net: dsa: remove out label in  dsa_switch_setup_one Andrew Lunn <andrew@lunn.ch> - 2017-01-03 19:40 +0100
        Re: [PATCH net-next 1/3] net: dsa: remove out label in dsa_switch_setup_one Vivien Didelot <vivien.didelot@savoirfairelinux.com> - 2017-01-03 20:20 +0100
    [PATCH net-next 2/3] net: dsa: move HWMON support to its own file Vivien Didelot <vivien.didelot@savoirfairelinux.com> - 2017-01-03 19:20 +0100
    Re: [PATCH net-next 0/3] net: dsa: restore HWMON support in dsa2 Andrew Lunn <andrew@lunn.ch> - 2017-01-03 19:40 +0100
      Re: [PATCH net-next 0/3] net: dsa: restore HWMON support in dsa2 Vivien Didelot <vivien.didelot@savoirfairelinux.com> - 2017-01-03 20:30 +0100
        Re: [PATCH net-next 0/3] net: dsa: restore HWMON support in dsa2 Florian Fainelli <f.fainelli@gmail.com> - 2017-01-03 20:40 +0100
          Re: [PATCH net-next 0/3] net: dsa: restore HWMON support in dsa2 Vivien Didelot <vivien.didelot@savoirfairelinux.com> - 2017-01-03 20:50 +0100
          Re: [PATCH net-next 0/3] net: dsa: restore HWMON support in dsa2 Andrew Lunn <andrew@lunn.ch> - 2017-01-03 21:20 +0100

#1550048 — [PATCH net-next 0/3] net: dsa: restore HWMON support in dsa2

The current HWMON support in DSA in embedded in the legacy code.
Extract it to its own file and register it in the newer DSA code.

Tested on ZII Rev B boards.

Vivien Didelot (3):
  net: dsa: remove out label in dsa_switch_setup_one
  net: dsa: move HWMON support to its own file
  net: dsa: restore HWMON support in dsa2

 net/dsa/Makefile   |   1 +
 net/dsa/dsa.c      | 171 +++++------------------------------------------------
 net/dsa/dsa2.c     |   4 ++
 net/dsa/dsa_priv.h |   9 +++
 net/dsa/hwmon.c    | 149 ++++++++++++++++++++++++++++++++++++++++++++++
 5 files changed, 178 insertions(+), 156 deletions(-)
 create mode 100644 net/dsa/hwmon.c

-- 
2.11.0

[toc] | [next] | [standalone]

#1550049 — [PATCH net-next 1/3] net: dsa: remove out label in dsa_switch_setup_one

The "out" label in dsa_switch_setup_one() is useless, thus remove it.
---
 net/dsa/dsa.c | 40 +++++++++++++---------------------------
 1 file changed, 13 insertions(+), 27 deletions(-)

diff --git a/net/dsa/dsa.c b/net/dsa/dsa.c
index 7899919cd9f0..89e66b623d73 100644
--- a/net/dsa/dsa.c
+++ b/net/dsa/dsa.c
@@ -329,8 +329,7 @@ static int dsa_switch_setup_one(struct dsa_switch *ds, struct device *parent)
 			if (dst->cpu_switch != -1) {
 				netdev_err(dst->master_netdev,
 					   "multiple cpu ports?!\n");
-				ret = -EINVAL;
-				goto out;
+				return -EINVAL;
 			}
 			dst->cpu_switch = index;
 			dst->cpu_port = i;
@@ -343,10 +342,8 @@ static int dsa_switch_setup_one(struct dsa_switch *ds, struct device *parent)
 		valid_name_found = true;
 	}
 
-	if (!valid_name_found && i == DSA_MAX_PORTS) {
-		ret = -EINVAL;
-		goto out;
-	}
+	if (!valid_name_found && i == DSA_MAX_PORTS)
+		return -EINVAL;
 
 	/* Make the built-in MII bus mask match the number of ports,
 	 * switch drivers can override this later
@@ -363,10 +360,8 @@ static int dsa_switch_setup_one(struct dsa_switch *ds, struct device *parent)
 
 		tag_protocol = ops->get_tag_protocol(ds);
 		dst->tag_ops = dsa_resolve_tag_protocol(tag_protocol);
-		if (IS_ERR(dst->tag_ops)) {
-			ret = PTR_ERR(dst->tag_ops);
-			goto out;
-		}
+		if (IS_ERR(dst->tag_ops))
+			return PTR_ERR(dst->tag_ops);
 
 		dst->rcv = dst->tag_ops->rcv;
 	}
@@ -378,25 +373,23 @@ static int dsa_switch_setup_one(struct dsa_switch *ds, struct device *parent)
 	 */
 	ret = ops->setup(ds);
 	if (ret < 0)
-		goto out;
+		return ret;
 
 	if (ops->set_addr) {
 		ret = ops->set_addr(ds, dst->master_netdev->dev_addr);
 		if (ret < 0)
-			goto out;
+			return ret;
 	}
 
 	if (!ds->slave_mii_bus && ops->phy_read) {
 		ds->slave_mii_bus = devm_mdiobus_alloc(parent);
-		if (!ds->slave_mii_bus) {
-			ret = -ENOMEM;
-			goto out;
-		}
+		if (!ds->slave_mii_bus)
+			return -ENOMEM;
 		dsa_slave_mii_bus_init(ds);
 
 		ret = mdiobus_register(ds->slave_mii_bus);
 		if (ret < 0)
-			goto out;
+			return ret;
 	}
 
 	/*
@@ -409,20 +402,16 @@ static int dsa_switch_setup_one(struct dsa_switch *ds, struct device *parent)
 			continue;
 
 		ret = dsa_slave_create(ds, parent, i, cd->port_names[i]);
-		if (ret < 0) {
+		if (ret < 0)
 			netdev_err(dst->master_netdev, "[%d]: can't create dsa slave device for port %d(%s): %d\n",
 				   index, i, cd->port_names[i], ret);
-			ret = 0;
-		}
 	}
 
 	/* Perform configuration of the CPU and DSA ports */
 	ret = dsa_cpu_dsa_setups(ds, parent);
-	if (ret < 0) {
+	if (ret < 0)
 		netdev_err(dst->master_netdev, "[%d] : can't configure CPU and DSA ports\n",
 			   index);
-		ret = 0;
-	}
 
 	ret = dsa_cpu_port_ethtool_setup(ds);
 	if (ret)
@@ -453,10 +442,7 @@ static int dsa_switch_setup_one(struct dsa_switch *ds, struct device *parent)
 	}
 #endif /* CONFIG_NET_DSA_HWMON */
 
-	return ret;
-
-out:
-	return ret;
+	return 0;
 }
 
 static struct dsa_switch *
-- 
2.11.0

[toc] | [prev] | [next] | [standalone]

#1550083 — Re: [PATCH net-next 1/3] net: dsa: remove out label in dsa_switch_setup_one

On Tue, Jan 03, 2017 at 01:15:36PM -0500, Vivien Didelot wrote:
> The "out" label in dsa_switch_setup_one() is useless, thus remove it.

Hi Vivien

Missing a SOB.

This one patch i'm happy with. Assuming we drop the other two, please
could you submit this on its own.

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

    Andrew


> ---
>  net/dsa/dsa.c | 40 +++++++++++++---------------------------
>  1 file changed, 13 insertions(+), 27 deletions(-)
> 
> diff --git a/net/dsa/dsa.c b/net/dsa/dsa.c
> index 7899919cd9f0..89e66b623d73 100644
> --- a/net/dsa/dsa.c
> +++ b/net/dsa/dsa.c
> @@ -329,8 +329,7 @@ static int dsa_switch_setup_one(struct dsa_switch *ds, struct device *parent)
>  			if (dst->cpu_switch != -1) {
>  				netdev_err(dst->master_netdev,
>  					   "multiple cpu ports?!\n");
> -				ret = -EINVAL;
> -				goto out;
> +				return -EINVAL;
>  			}
>  			dst->cpu_switch = index;
>  			dst->cpu_port = i;
> @@ -343,10 +342,8 @@ static int dsa_switch_setup_one(struct dsa_switch *ds, struct device *parent)
>  		valid_name_found = true;
>  	}
>  
> -	if (!valid_name_found && i == DSA_MAX_PORTS) {
> -		ret = -EINVAL;
> -		goto out;
> -	}
> +	if (!valid_name_found && i == DSA_MAX_PORTS)
> +		return -EINVAL;
>  
>  	/* Make the built-in MII bus mask match the number of ports,
>  	 * switch drivers can override this later
> @@ -363,10 +360,8 @@ static int dsa_switch_setup_one(struct dsa_switch *ds, struct device *parent)
>  
>  		tag_protocol = ops->get_tag_protocol(ds);
>  		dst->tag_ops = dsa_resolve_tag_protocol(tag_protocol);
> -		if (IS_ERR(dst->tag_ops)) {
> -			ret = PTR_ERR(dst->tag_ops);
> -			goto out;
> -		}
> +		if (IS_ERR(dst->tag_ops))
> +			return PTR_ERR(dst->tag_ops);
>  
>  		dst->rcv = dst->tag_ops->rcv;
>  	}
> @@ -378,25 +373,23 @@ static int dsa_switch_setup_one(struct dsa_switch *ds, struct device *parent)
>  	 */
>  	ret = ops->setup(ds);
>  	if (ret < 0)
> -		goto out;
> +		return ret;
>  
>  	if (ops->set_addr) {
>  		ret = ops->set_addr(ds, dst->master_netdev->dev_addr);
>  		if (ret < 0)
> -			goto out;
> +			return ret;
>  	}
>  
>  	if (!ds->slave_mii_bus && ops->phy_read) {
>  		ds->slave_mii_bus = devm_mdiobus_alloc(parent);
> -		if (!ds->slave_mii_bus) {
> -			ret = -ENOMEM;
> -			goto out;
> -		}
> +		if (!ds->slave_mii_bus)
> +			return -ENOMEM;
>  		dsa_slave_mii_bus_init(ds);
>  
>  		ret = mdiobus_register(ds->slave_mii_bus);
>  		if (ret < 0)
> -			goto out;
> +			return ret;
>  	}
>  
>  	/*
> @@ -409,20 +402,16 @@ static int dsa_switch_setup_one(struct dsa_switch *ds, struct device *parent)
>  			continue;
>  
>  		ret = dsa_slave_create(ds, parent, i, cd->port_names[i]);
> -		if (ret < 0) {
> +		if (ret < 0)
>  			netdev_err(dst->master_netdev, "[%d]: can't create dsa slave device for port %d(%s): %d\n",
>  				   index, i, cd->port_names[i], ret);
> -			ret = 0;
> -		}
>  	}
>  
>  	/* Perform configuration of the CPU and DSA ports */
>  	ret = dsa_cpu_dsa_setups(ds, parent);
> -	if (ret < 0) {
> +	if (ret < 0)
>  		netdev_err(dst->master_netdev, "[%d] : can't configure CPU and DSA ports\n",
>  			   index);
> -		ret = 0;
> -	}
>  
>  	ret = dsa_cpu_port_ethtool_setup(ds);
>  	if (ret)
> @@ -453,10 +442,7 @@ static int dsa_switch_setup_one(struct dsa_switch *ds, struct device *parent)
>  	}
>  #endif /* CONFIG_NET_DSA_HWMON */
>  
> -	return ret;
> -
> -out:
> -	return ret;
> +	return 0;
>  }
>  
>  static struct dsa_switch *
> -- 
> 2.11.0
> 

[toc] | [prev] | [next] | [standalone]

#1550109 — Re: [PATCH net-next 1/3] net: dsa: remove out label in dsa_switch_setup_one

Hi Andrew,

Andrew Lunn <andrew@lunn.ch> writes:

> On Tue, Jan 03, 2017 at 01:15:36PM -0500, Vivien Didelot wrote:
>> The "out" label in dsa_switch_setup_one() is useless, thus remove it.
>
> Hi Vivien
>
> Missing a SOB.
>
> This one patch i'm happy with. Assuming we drop the other two, please
> could you submit this on its own.
>
> Reviewed-by: Andrew Lunn <andrew@lunn.ch>

You're right, thanks, will do.

       Vivien

[toc] | [prev] | [next] | [standalone]

#1550060 — [PATCH net-next 2/3] net: dsa: move HWMON support to its own file

The HWMON support in DSA is currently embedded in the legacy DSA code.
Move it out in its own file, so that it can be reused in newer DSA code.
---
 net/dsa/Makefile   |   1 +
 net/dsa/dsa.c      | 131 +---------------------------------------------
 net/dsa/dsa_priv.h |   9 ++++
 net/dsa/hwmon.c    | 149 +++++++++++++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 161 insertions(+), 129 deletions(-)
 create mode 100644 net/dsa/hwmon.c

diff --git a/net/dsa/Makefile b/net/dsa/Makefile
index a3380ed0e0be..560b6747c276 100644
--- a/net/dsa/Makefile
+++ b/net/dsa/Makefile
@@ -1,6 +1,7 @@
 # the core
 obj-$(CONFIG_NET_DSA) += dsa_core.o
 dsa_core-y += dsa.o slave.o dsa2.o
+dsa_core-$(CONFIG_NET_DSA_HWMON) += hwmon.o
 
 # tagging formats
 dsa_core-$(CONFIG_NET_DSA_TAG_BRCM) += tag_brcm.o
diff --git a/net/dsa/dsa.c b/net/dsa/dsa.c
index 89e66b623d73..aa73f923e9b5 100644
--- a/net/dsa/dsa.c
+++ b/net/dsa/dsa.c
@@ -9,9 +9,7 @@
  * (at your option) any later version.
  */
 
-#include <linux/ctype.h>
 #include <linux/device.h>
-#include <linux/hwmon.h>
 #include <linux/list.h>
 #include <linux/platform_device.h>
 #include <linux/slab.h>
@@ -110,105 +108,6 @@ dsa_switch_probe(struct device *parent, struct device *host_dev, int sw_addr,
 	return ret;
 }
 
-/* hwmon support ************************************************************/
-
-#ifdef CONFIG_NET_DSA_HWMON
-
-static ssize_t temp1_input_show(struct device *dev,
-				struct device_attribute *attr, char *buf)
-{
-	struct dsa_switch *ds = dev_get_drvdata(dev);
-	int temp, ret;
-
-	ret = ds->ops->get_temp(ds, &temp);
-	if (ret < 0)
-		return ret;
-
-	return sprintf(buf, "%d\n", temp * 1000);
-}
-static DEVICE_ATTR_RO(temp1_input);
-
-static ssize_t temp1_max_show(struct device *dev,
-			      struct device_attribute *attr, char *buf)
-{
-	struct dsa_switch *ds = dev_get_drvdata(dev);
-	int temp, ret;
-
-	ret = ds->ops->get_temp_limit(ds, &temp);
-	if (ret < 0)
-		return ret;
-
-	return sprintf(buf, "%d\n", temp * 1000);
-}
-
-static ssize_t temp1_max_store(struct device *dev,
-			       struct device_attribute *attr, const char *buf,
-			       size_t count)
-{
-	struct dsa_switch *ds = dev_get_drvdata(dev);
-	int temp, ret;
-
-	ret = kstrtoint(buf, 0, &temp);
-	if (ret < 0)
-		return ret;
-
-	ret = ds->ops->set_temp_limit(ds, DIV_ROUND_CLOSEST(temp, 1000));
-	if (ret < 0)
-		return ret;
-
-	return count;
-}
-static DEVICE_ATTR_RW(temp1_max);
-
-static ssize_t temp1_max_alarm_show(struct device *dev,
-				    struct device_attribute *attr, char *buf)
-{
-	struct dsa_switch *ds = dev_get_drvdata(dev);
-	bool alarm;
-	int ret;
-
-	ret = ds->ops->get_temp_alarm(ds, &alarm);
-	if (ret < 0)
-		return ret;
-
-	return sprintf(buf, "%d\n", alarm);
-}
-static DEVICE_ATTR_RO(temp1_max_alarm);
-
-static struct attribute *dsa_hwmon_attrs[] = {
-	&dev_attr_temp1_input.attr,	/* 0 */
-	&dev_attr_temp1_max.attr,	/* 1 */
-	&dev_attr_temp1_max_alarm.attr,	/* 2 */
-	NULL
-};
-
-static umode_t dsa_hwmon_attrs_visible(struct kobject *kobj,
-				       struct attribute *attr, int index)
-{
-	struct device *dev = container_of(kobj, struct device, kobj);
-	struct dsa_switch *ds = dev_get_drvdata(dev);
-	struct dsa_switch_ops *ops = ds->ops;
-	umode_t mode = attr->mode;
-
-	if (index == 1) {
-		if (!ops->get_temp_limit)
-			mode = 0;
-		else if (!ops->set_temp_limit)
-			mode &= ~S_IWUSR;
-	} else if (index == 2 && !ops->get_temp_alarm) {
-		mode = 0;
-	}
-	return mode;
-}
-
-static const struct attribute_group dsa_hwmon_group = {
-	.attrs = dsa_hwmon_attrs,
-	.is_visible = dsa_hwmon_attrs_visible,
-};
-__ATTRIBUTE_GROUPS(dsa_hwmon);
-
-#endif /* CONFIG_NET_DSA_HWMON */
-
 /* basic switch operations **************************************************/
 int dsa_cpu_dsa_setup(struct dsa_switch *ds, struct device *dev,
 		      struct device_node *port_dn, int port)
@@ -417,30 +316,7 @@ static int dsa_switch_setup_one(struct dsa_switch *ds, struct device *parent)
 	if (ret)
 		return ret;
 
-#ifdef CONFIG_NET_DSA_HWMON
-	/* If the switch provides a temperature sensor,
-	 * register with hardware monitoring subsystem.
-	 * Treat registration error as non-fatal and ignore it.
-	 */
-	if (ops->get_temp) {
-		const char *netname = netdev_name(dst->master_netdev);
-		char hname[IFNAMSIZ + 1];
-		int i, j;
-
-		/* Create valid hwmon 'name' attribute */
-		for (i = j = 0; i < IFNAMSIZ && netname[i]; i++) {
-			if (isalnum(netname[i]))
-				hname[j++] = netname[i];
-		}
-		hname[j] = '\0';
-		scnprintf(ds->hwmon_name, sizeof(ds->hwmon_name), "%s_dsa%d",
-			  hname, index);
-		ds->hwmon_dev = hwmon_device_register_with_groups(NULL,
-					ds->hwmon_name, ds, dsa_hwmon_groups);
-		if (IS_ERR(ds->hwmon_dev))
-			ds->hwmon_dev = NULL;
-	}
-#endif /* CONFIG_NET_DSA_HWMON */
+	dsa_hwmon_register(ds);
 
 	return 0;
 }
@@ -500,10 +376,7 @@ static void dsa_switch_destroy(struct dsa_switch *ds)
 {
 	int port;
 
-#ifdef CONFIG_NET_DSA_HWMON
-	if (ds->hwmon_dev)
-		hwmon_device_unregister(ds->hwmon_dev);
-#endif
+	dsa_hwmon_unregister(ds);
 
 	/* Destroy network devices for physical switch ports. */
 	for (port = 0; port < DSA_MAX_PORTS; port++) {
diff --git a/net/dsa/dsa_priv.h b/net/dsa/dsa_priv.h
index 6cfd7388834e..8cdbc4a7b678 100644
--- a/net/dsa/dsa_priv.h
+++ b/net/dsa/dsa_priv.h
@@ -57,6 +57,15 @@ const struct dsa_device_ops *dsa_resolve_tag_protocol(int tag_protocol);
 int dsa_cpu_port_ethtool_setup(struct dsa_switch *ds);
 void dsa_cpu_port_ethtool_restore(struct dsa_switch *ds);
 
+/* hwmon.c */
+#if CONFIG_NET_DSA_HWMON
+void dsa_hwmon_register(struct dsa_switch *ds);
+void dsa_hwmon_unregister(struct dsa_switch *ds);
+#else
+static inline void dsa_hwmon_register(struct dsa_switch *ds) { }
+static inline void dsa_hwmon_unregister(struct dsa_switch *ds) { }
+#endif
+
 /* slave.c */
 extern const struct dsa_device_ops notag_netdev_ops;
 void dsa_slave_mii_bus_init(struct dsa_switch *ds);
diff --git a/net/dsa/hwmon.c b/net/dsa/hwmon.c
new file mode 100644
index 000000000000..7b3d1a9c2be2
--- /dev/null
+++ b/net/dsa/hwmon.c
@@ -0,0 +1,149 @@
+/*
+ * net/dsa/hwmon.c - HWMON subsystem support
+ * Copyright (c) 2014 Guenter Roeck <linux@roeck-us.net>
+ * Copyright (c) 2017 Vivien Didelot <vivien.didelot@savoirfairelinux.com>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ */
+
+#include <linux/ctype.h>
+#include <linux/hwmon.h>
+#include <net/dsa.h>
+
+#include "dsa_priv.h"
+
+static ssize_t temp1_input_show(struct device *dev,
+				struct device_attribute *attr, char *buf)
+{
+	struct dsa_switch *ds = dev_get_drvdata(dev);
+	int temp, ret;
+
+	ret = ds->ops->get_temp(ds, &temp);
+	if (ret < 0)
+		return ret;
+
+	return sprintf(buf, "%d\n", temp * 1000);
+}
+static DEVICE_ATTR_RO(temp1_input);
+
+static ssize_t temp1_max_show(struct device *dev,
+			      struct device_attribute *attr, char *buf)
+{
+	struct dsa_switch *ds = dev_get_drvdata(dev);
+	int temp, ret;
+
+	ret = ds->ops->get_temp_limit(ds, &temp);
+	if (ret < 0)
+		return ret;
+
+	return sprintf(buf, "%d\n", temp * 1000);
+}
+
+static ssize_t temp1_max_store(struct device *dev,
+			       struct device_attribute *attr, const char *buf,
+			       size_t count)
+{
+	struct dsa_switch *ds = dev_get_drvdata(dev);
+	int temp, ret;
+
+	ret = kstrtoint(buf, 0, &temp);
+	if (ret < 0)
+		return ret;
+
+	ret = ds->ops->set_temp_limit(ds, DIV_ROUND_CLOSEST(temp, 1000));
+	if (ret < 0)
+		return ret;
+
+	return count;
+}
+static DEVICE_ATTR_RW(temp1_max);
+
+static ssize_t temp1_max_alarm_show(struct device *dev,
+				    struct device_attribute *attr, char *buf)
+{
+	struct dsa_switch *ds = dev_get_drvdata(dev);
+	bool alarm;
+	int ret;
+
+	ret = ds->ops->get_temp_alarm(ds, &alarm);
+	if (ret < 0)
+		return ret;
+
+	return sprintf(buf, "%d\n", alarm);
+}
+static DEVICE_ATTR_RO(temp1_max_alarm);
+
+static struct attribute *dsa_hwmon_attrs[] = {
+	&dev_attr_temp1_input.attr,	/* 0 */
+	&dev_attr_temp1_max.attr,	/* 1 */
+	&dev_attr_temp1_max_alarm.attr,	/* 2 */
+	NULL
+};
+
+static umode_t dsa_hwmon_attrs_visible(struct kobject *kobj,
+				       struct attribute *attr, int index)
+{
+	struct device *dev = container_of(kobj, struct device, kobj);
+	struct dsa_switch *ds = dev_get_drvdata(dev);
+	struct dsa_switch_ops *ops = ds->ops;
+	umode_t mode = attr->mode;
+
+	if (index == 1) {
+		if (!ops->get_temp_limit)
+			mode = 0;
+		else if (!ops->set_temp_limit)
+			mode &= ~S_IWUSR;
+	} else if (index == 2 && !ops->get_temp_alarm) {
+		mode = 0;
+	}
+	return mode;
+}
+
+static const struct attribute_group dsa_hwmon_group = {
+	.attrs = dsa_hwmon_attrs,
+	.is_visible = dsa_hwmon_attrs_visible,
+};
+__ATTRIBUTE_GROUPS(dsa_hwmon);
+
+void dsa_hwmon_register(struct dsa_switch *ds)
+{
+	const char *netname = netdev_name(ds->dst->master_netdev);
+	char hname[IFNAMSIZ + 1];
+	int i, j;
+
+	/* If the switch provides temperature accessors, register with hardware
+	 * monitoring subsystem. Treat registration error as non-fatal.
+	 */
+	if (!ds->ops->get_temp)
+		return;
+
+	/* Create valid hwmon 'name' attribute */
+	for (i = j = 0; i < IFNAMSIZ && netname[i]; i++) {
+		if (isalnum(netname[i]))
+			hname[j++] = netname[i];
+	}
+	hname[j] = '\0';
+	scnprintf(ds->hwmon_name, sizeof(ds->hwmon_name), "%s_dsa%d", hname,
+		  ds->index);
+	ds->hwmon_dev = hwmon_device_register_with_groups(NULL, ds->hwmon_name,
+							  ds, dsa_hwmon_groups);
+	if (IS_ERR(ds->hwmon_dev)) {
+		pr_warn("DSA: failed to register HWMON subsystem for switch %d\n",
+			ds->index);
+		ds->hwmon_dev = NULL;
+	} else {
+		pr_info("DSA: registered HWMON subsystem for switch %d\n",
+			ds->index);
+	}
+}
+
+void dsa_hwmon_unregister(struct dsa_switch *ds)
+{
+	if (ds->hwmon_dev) {
+		hwmon_device_unregister(ds->hwmon_dev);
+		ds->hwmon_dev = NULL;
+	}
+}
-- 
2.11.0

[toc] | [prev] | [next] | [standalone]

#1550073

On Tue, Jan 03, 2017 at 01:15:35PM -0500, Vivien Didelot wrote:
> The current HWMON support in DSA in embedded in the legacy code.
> Extract it to its own file and register it in the newer DSA code.

Hi Vivien

I would really prefer not to do this.

The temperature sensor is in the embedded PHYs of the switch. Many of
Marvell discrete PHYs also have the same temperature sensor. The
correct thing to do is move this code into drivers/net/phy/marvell.c.

	Andrew

[toc] | [prev] | [next] | [standalone]

#1550115

Hi Andrew,

Andrew Lunn <andrew@lunn.ch> writes:

> On Tue, Jan 03, 2017 at 01:15:35PM -0500, Vivien Didelot wrote:
>> The current HWMON support in DSA in embedded in the legacy code.
>> Extract it to its own file and register it in the newer DSA code.
>
> I would really prefer not to do this.
>
> The temperature sensor is in the embedded PHYs of the switch. Many of
> Marvell discrete PHYs also have the same temperature sensor. The
> correct thing to do is move this code into drivers/net/phy/marvell.c.

I agree that the temperature code in the mv88e6xxx driver must be moved
to the Marvell PHY driver.

However I still think this patchset is still valuable because at least
it isolates the HWMON code in DSA which is good, and I think some chips
do have temperature sensors in their cores, not in their PHYs. So the
new DSA code should benefit from the HWMON support instead of
considering this a regression for its users.

Thanks,

        Vivien

[toc] | [prev] | [next] | [standalone]

#1550119

On 01/03/2017 11:24 AM, Vivien Didelot wrote:
> Hi Andrew,
> 
> Andrew Lunn <andrew@lunn.ch> writes:
> 
>> On Tue, Jan 03, 2017 at 01:15:35PM -0500, Vivien Didelot wrote:
>>> The current HWMON support in DSA in embedded in the legacy code.
>>> Extract it to its own file and register it in the newer DSA code.
>>
>> I would really prefer not to do this.
>>
>> The temperature sensor is in the embedded PHYs of the switch. Many of
>> Marvell discrete PHYs also have the same temperature sensor. The
>> correct thing to do is move this code into drivers/net/phy/marvell.c.
> 
> I agree that the temperature code in the mv88e6xxx driver must be moved
> to the Marvell PHY driver.
> 
> However I still think this patchset is still valuable because at least
> it isolates the HWMON code in DSA which is good, and I think some chips
> do have temperature sensors in their cores, not in their PHYs. So the
> new DSA code should benefit from the HWMON support instead of
> considering this a regression for its users.

Well, I agree with the regression part, but an argument could
definitively be made that HWMON did not belong in the DSA layer in the
first place, unless we were able to find some commonality between
devices which AFAICT, we could not yet.

I don't have a strong preference, but it seems like the HWMON
functionality should have been part of the switch and/or PHY driver all
along.
-- 
Florian

[toc] | [prev] | [next] | [standalone]

#1550125

Hi Florian,

Florian Fainelli <f.fainelli@gmail.com> writes:

>>>> The current HWMON support in DSA in embedded in the legacy code.
>>>> Extract it to its own file and register it in the newer DSA code.
>>>
>>> I would really prefer not to do this.
>>>
>>> The temperature sensor is in the embedded PHYs of the switch. Many of
>>> Marvell discrete PHYs also have the same temperature sensor. The
>>> correct thing to do is move this code into drivers/net/phy/marvell.c.
>> 
>> I agree that the temperature code in the mv88e6xxx driver must be moved
>> to the Marvell PHY driver.
>> 
>> However I still think this patchset is still valuable because at least
>> it isolates the HWMON code in DSA which is good, and I think some chips
>> do have temperature sensors in their cores, not in their PHYs. So the
>> new DSA code should benefit from the HWMON support instead of
>> considering this a regression for its users.
>
> Well, I agree with the regression part, but an argument could
> definitively be made that HWMON did not belong in the DSA layer in the
> first place, unless we were able to find some commonality between
> devices which AFAICT, we could not yet.
>
> I don't have a strong preference, but it seems like the HWMON
> functionality should have been part of the switch and/or PHY driver all
> along.

I see what you mean. Indeed the drivers could register their own HWMON
device until we figure any many chips have an embedded temperature
sensor in their core.

What about sending the patch 2/3 on its own?

Thanks,

        Vivien

[toc] | [prev] | [next] | [standalone]

#1550153

> Well, I agree with the regression part, but an argument could
> definitively be made that HWMON did not belong in the DSA layer in the
> first place, unless we were able to find some commonality between
> devices which AFAICT, we could not yet.

Florian, does SF2 or b53 have a temperature sensor?
John, does the qca8k have a temperature sensor?

If we do have a switch with a temperature sensor, making the HWMON
code available as a library for switch drivers to use makes sense.
However, if we move the Marvell code into the PHY driver, and there
are no other switches with temperature sensors, we should just remove
it.

	Andrew

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web