[OpenWrt-Devel] [PATCH 2/4] switch: allow Ethernet port LEDs to show specific link speeds only

Hartmut Knaack knaack.h at gmx.de
Fri Feb 12 18:54:46 EST 2016


-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA256

Michal schrieb am 12.02.2016 um 00:45:
> From: Michal Cieslakiewicz <michal.cieslakiewicz at wp.pl>
> Subject: [PATCH 2/4] switch: allow Ethernet port LEDs to show specific link speeds only
> 
> This patch adds speed_mask special file to LEDs connected to switch ports
> via 'switch' trigger. It allows to choose which speeds to signal when link
> is up. If router has more than one LED per port, they may light up differently
> depending on how fast connection is. Default setting is 'all speeds' so
> backward compatibilty with system scripts (for example uci) is maintained.
> 

There are a few more style issues, than John pointed out. These can be pointed
out by using scripts/checkpatch.pl from the linux kernel sources (also
Documentation/Coding-Style.txt is a good reference). Please also find some other
recommendations inline.

> Signed-off-by: Michal Cieslakiewicz <michal.cieslakiewicz at wp.pl>
> ---
>  Link speed to speed_mask bit mapping:
>  bit 0 (0x01) - unknown port speed, may indicate cabling problem
>  bit 1 (0x02) - 10 Mbps
>  bit 2 (0x04) - 100 Mbps
>  bit 3 (0x08) - 1000 Mbps aka 1 Gbps
>  bits 4-7 - reserved for future use, always 0, ignored for now
> 
>  .../generic/files/drivers/net/phy/swconfig_leds.c  | 106 ++++++++++++++++++++-
>  1 file changed, 102 insertions(+), 4 deletions(-)
> 
> diff --git a/target/linux/generic/files/drivers/net/phy/swconfig_leds.c b/target/linux/generic/files/drivers/net/phy/swconfig_leds.c
> index 7d122d2..433a472 100644
> --- a/target/linux/generic/files/drivers/net/phy/swconfig_leds.c
> +++ b/target/linux/generic/files/drivers/net/phy/swconfig_leds.c
> @@ -20,6 +20,15 @@
>  #define SWCONFIG_LED_TIMER_INTERVAL	(HZ / 10)
>  #define SWCONFIG_LED_NUM_PORTS		32
>  
> +#define SWCONFIG_LED_PORT_SPEED_NA	0x01	/* unknown speed */
> +#define SWCONFIG_LED_PORT_SPEED_10	0x02	/* 10 Mbps */
> +#define SWCONFIG_LED_PORT_SPEED_100	0x04	/* 100 Mbps */
> +#define SWCONFIG_LED_PORT_SPEED_1000	0x08	/* 1000 Mbps */
> +#define SWCONFIG_LED_PORT_SPEED_ALL	( SWCONFIG_LED_PORT_SPEED_NA | \
> +					  SWCONFIG_LED_PORT_SPEED_10 | \
> +					  SWCONFIG_LED_PORT_SPEED_100 | \
> +					  SWCONFIG_LED_PORT_SPEED_1000 )
> +
>  struct switch_led_trigger {
>  	struct led_trigger trig;
>  	struct switch_dev *swdev;
> @@ -28,6 +37,7 @@ struct switch_led_trigger {
>  	u32 port_mask;
>  	u32 port_link;
>  	unsigned long port_traffic[SWCONFIG_LED_NUM_PORTS];
> +	u8 link_speed[SWCONFIG_LED_NUM_PORTS];
>  };
>  
>  struct swconfig_trig_data {
> @@ -40,6 +50,7 @@ struct swconfig_trig_data {
>  	bool prev_link;
>  	unsigned long prev_traffic;
>  	enum led_brightness prev_brightness;
> +	u8 speed_mask;
>  };
>  
>  static void
> @@ -135,6 +146,46 @@ swconfig_trig_port_mask_show(struct device *dev, struct device_attribute *attr,
>  static DEVICE_ATTR(port_mask, 0644, swconfig_trig_port_mask_show,
>  		   swconfig_trig_port_mask_store);
>  
> +/* speed_mask file handler - display value */
> +static ssize_t swconfig_trig_speed_mask_show(struct device *dev,
> +					     struct device_attribute *attr,
> +					     char *buf)
> +{
> +	struct led_classdev *led_cdev = dev_get_drvdata(dev);
> +	struct swconfig_trig_data *trig_data = led_cdev->trigger_data;
> +
> +	read_lock(&trig_data->lock);
> +	sprintf(buf, "%#x\n", trig_data->speed_mask);
> +	read_unlock(&trig_data->lock);
> +
> +	return strlen(buf) + 1;
> +}
> +
> +/* speed_mask file handler - store value */
> +static ssize_t swconfig_trig_speed_mask_store(struct device *dev,
> +					      struct device_attribute *attr,
> +					      const char *buf, size_t size)
> +{
> +	struct led_classdev *led_cdev = dev_get_drvdata(dev);
> +	struct swconfig_trig_data *trig_data = led_cdev->trigger_data;
> +	u8 speed_mask;
> +	int ret;
> +
> +	ret = kstrtou8(buf, 0, &speed_mask);
> +	if (ret)
> +		return ret;
> +
> +	write_lock(&trig_data->lock);
> +	trig_data->speed_mask = speed_mask & SWCONFIG_LED_PORT_SPEED_ALL;
> +	write_unlock(&trig_data->lock);
> +
> +	return size;
> +}
> +
> +/* speed_mask special file */
> +static DEVICE_ATTR(speed_mask, 0644, swconfig_trig_speed_mask_show,
> +		   swconfig_trig_speed_mask_store);
> +
>  static void
>  swconfig_trig_activate(struct led_classdev *led_cdev)
>  {
> @@ -154,14 +205,22 @@ swconfig_trig_activate(struct led_classdev *led_cdev)
>  	rwlock_init(&trig_data->lock);
>  	trig_data->led_cdev = led_cdev;
>  	trig_data->swdev = sw_trig->swdev;
> +	trig_data->speed_mask = SWCONFIG_LED_PORT_SPEED_ALL;
>  	led_cdev->trigger_data = trig_data;
>  
>  	err = device_create_file(led_cdev->dev, &dev_attr_port_mask);
>  	if (err)
>  		goto err_free;
>  
> +	err = device_create_file(led_cdev->dev, &dev_attr_speed_mask);
> +	if (err)
> +		goto err_dev_free;
> +
>  	return;
>  
> +err_dev_free:
> +	device_remove_file(led_cdev->dev, &dev_attr_port_mask);
> +
>  err_free:
>  	led_cdev->trigger_data = NULL;
>  	kfree(trig_data);
> @@ -177,6 +236,7 @@ swconfig_trig_deactivate(struct led_classdev *led_cdev)
>  	trig_data = (void *) led_cdev->trigger_data;
>  	if (trig_data) {
>  		device_remove_file(led_cdev->dev, &dev_attr_port_mask);
> +		device_remove_file(led_cdev->dev, &dev_attr_speed_mask);
>  		kfree(trig_data);
>  	}
>  }
> @@ -188,6 +248,7 @@ swconfig_trig_led_event(struct switch_led_trigger *sw_trig,
>  	struct swconfig_trig_data *trig_data;
>  	u32 port_mask;
>  	bool link;
> +	u8 speed_mask;
>  
>  	trig_data = led_cdev->trigger_data;
>  	if (!trig_data)
> @@ -195,6 +256,7 @@ swconfig_trig_led_event(struct switch_led_trigger *sw_trig,
>  
>  	read_lock(&trig_data->lock);
>  	port_mask = trig_data->port_mask;
> +	speed_mask = trig_data->speed_mask;
>  	read_unlock(&trig_data->lock);
>  
>  	link = !!(sw_trig->port_link & port_mask);
> @@ -203,17 +265,30 @@ swconfig_trig_led_event(struct switch_led_trigger *sw_trig,
>  			swconfig_trig_set_brightness(trig_data, LED_OFF);
>  	} else {
>  		unsigned long traffic;
> +		bool speedok;
>  		int i;
>  
>  		traffic = 0;
> +		speedok = 0;

bools are commonly assigned true or false. But there is an easier way, which makes
the assignment above obsolete and the code slightly shorter:

>  		for (i = 0; i < SWCONFIG_LED_NUM_PORTS; i++) {
>  			if (port_mask & (1 << i))
> -				traffic += sw_trig->port_traffic[i];
> +				if (sw_trig->link_speed[i] & speed_mask)
> +				{
> +					traffic += sw_trig->port_traffic[i];
> +					speedok = 1;
> +				}

				speedok = sw_trig->link_speed[i] & speed_mask;
				if (speedok)
					traffic += sw_trig->port_traffic[i];

>  		}
>  
> -		if (trig_data->prev_brightness != LED_FULL)
> -			swconfig_trig_set_brightness(trig_data, LED_FULL);
> -		else if (traffic != trig_data->prev_traffic)
> +		if (speedok)
> +		{
> +			if (trig_data->prev_brightness != LED_FULL)
> +				swconfig_trig_set_brightness(trig_data,
> +							     LED_FULL);
> +			else if (traffic != trig_data->prev_traffic)
> +				swconfig_trig_set_brightness(trig_data,
> +							     LED_OFF);
> +		}
> +		else if (trig_data->prev_brightness != LED_OFF)
>  			swconfig_trig_set_brightness(trig_data, LED_OFF);
>  
>  		trig_data->prev_traffic = traffic;
> @@ -258,6 +333,8 @@ swconfig_led_work_func(struct work_struct *work)
>  	for (i = 0; i < SWCONFIG_LED_NUM_PORTS; i++) {
>  		u32 port_bit;
>  
> +		sw_trig->link_speed[i] = 0;
> +
>  		port_bit = BIT(i);
>  		if ((port_mask & port_bit) == 0)
>  			continue;
> @@ -269,7 +346,28 @@ swconfig_led_work_func(struct work_struct *work)
>  			swdev->ops->get_port_link(swdev, i, &port_link);
>  
>  			if (port_link.link)
> +			{
>  				link |= port_bit;
> +				switch(port_link.speed)
> +				{
> +					case SWITCH_PORT_SPEED_UNKNOWN:
> +					sw_trig->link_speed[i] =
> +						SWCONFIG_LED_PORT_SPEED_NA;
> +					break;
> +					case SWITCH_PORT_SPEED_10:
> +					sw_trig->link_speed[i] =
> +						SWCONFIG_LED_PORT_SPEED_10;
> +					break;
> +					case SWITCH_PORT_SPEED_100:
> +					sw_trig->link_speed[i] =
> +						SWCONFIG_LED_PORT_SPEED_100;
> +					break;
> +					case SWITCH_PORT_SPEED_1000:
> +					sw_trig->link_speed[i] =
> +						SWCONFIG_LED_PORT_SPEED_1000;
> +					break;
> +				}
> +			}

I would factor out this switch block into a separate function and call it with the
parameters port_link.speed and sw_trig->link_speed[i] (by reference). That gives
you way more space due to lower indentation level.
Thanks,

Hartmut

>  		}
>  
>  		if (swdev->ops->get_port_stats) {
> 

-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0xFAC89148.asc
Type: application/pgp-keys
Size: 3104 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/openwrt-devel/attachments/20160213/194cc21d/attachment.bin>
-------------- next part --------------
_______________________________________________
openwrt-devel mailing list
openwrt-devel at lists.openwrt.org
https://lists.openwrt.org/cgi-bin/mailman/listinfo/openwrt-devel


More information about the openwrt-devel mailing list