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

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

[Vivien Didelot]

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

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

[Vivien Didelot]

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

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

[Vivien Didelot]

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

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

[Vivien Didelot]

The HWMON support was only registered in the legacy DSA code. Register
it in the newer DSA code (dsa2) as well.
---
 net/dsa/dsa2.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/net/dsa/dsa2.c b/net/dsa/dsa2.c
index 5fff951a0a49..668aa2974d01 100644
--- a/net/dsa/dsa2.c
+++ b/net/dsa/dsa2.c
@@ -348,6 +348,8 @@ static int dsa_ds_apply(struct dsa_switch_tree *dst, struct dsa_switch *ds)
 			continue;
 	}
 
+	dsa_hwmon_register(ds);
+
 	return 0;
 }
 
@@ -356,6 +358,8 @@ static void dsa_ds_unapply(struct dsa_switch_tree *dst, struct dsa_switch *ds)
 	struct device_node *port;
 	u32 index;
 
+	dsa_hwmon_unregister(ds);
+
 	for (index = 0; index < DSA_MAX_PORTS; index++) {
 		port = ds->ports[index].dn;
 		if (!port)
-- 
2.11.0

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

[Andrew Lunn]

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

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

[Andrew Lunn]

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
> 

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

[Vivien Didelot]

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

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

[Vivien Didelot]

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

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

[Florian Fainelli]

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

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

[Vivien Didelot]

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

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

[Andrew Lunn]

> 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

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

[kbuild test robot]

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

Hi Vivien,

[auto build test WARNING on net-next/master]

url:    https://github.com/0day-ci/linux/commits/Vivien-Didelot/net-dsa-restore-HWMON-support-in-dsa2/20170104-055351
config: i386-randconfig-x005-201701 (attached as .config)
compiler: gcc-6 (Debian 6.2.0-3) 6.2.0 20160901
reproduce:
        # save the attached .config to linux build tree
        make ARCH=i386 

All warnings (new ones prefixed by >>):

   In file included from net/dsa/dsa.c:26:0:
>> net/dsa/dsa_priv.h:61:5: warning: "CONFIG_NET_DSA_HWMON" is not defined [-Wundef]
    #if CONFIG_NET_DSA_HWMON
        ^~~~~~~~~~~~~~~~~~~~

vim +/CONFIG_NET_DSA_HWMON +61 net/dsa/dsa_priv.h

    45		struct net_device	*bridge_dev;
    46	#ifdef CONFIG_NET_POLL_CONTROLLER
    47		struct netpoll		*netpoll;
    48	#endif
    49	};
    50	
    51	/* dsa.c */
    52	extern char dsa_driver_version[];
    53	int dsa_cpu_dsa_setup(struct dsa_switch *ds, struct device *dev,
    54			      struct device_node *port_dn, int port);
    55	void dsa_cpu_dsa_destroy(struct device_node *port_dn);
    56	const struct dsa_device_ops *dsa_resolve_tag_protocol(int tag_protocol);
    57	int dsa_cpu_port_ethtool_setup(struct dsa_switch *ds);
    58	void dsa_cpu_port_ethtool_restore(struct dsa_switch *ds);
    59	
    60	/* hwmon.c */
  > 61	#if CONFIG_NET_DSA_HWMON
    62	void dsa_hwmon_register(struct dsa_switch *ds);
    63	void dsa_hwmon_unregister(struct dsa_switch *ds);
    64	#else
    65	static inline void dsa_hwmon_register(struct dsa_switch *ds) { }
    66	static inline void dsa_hwmon_unregister(struct dsa_switch *ds) { }
    67	#endif
    68	
    69	/* slave.c */

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

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