[PATCH RFC leds + net-next 0/7] netdev trigger offloading and LEDs on Marvell PHYs

[PATCH RFC leds + net-next 0/7] netdev trigger offloading and LEDs on Marvell PHYs

[Marek Behún]

Hello,

this RFC series adds API for transparent offloading of LED triggers
to hardware and implements this for the netdev trigger.
It is then used by Marvell PHY driver, which gains support for
probing LEDs connected to a PHY chip.

When a netdev trigger is enabled on a Marvell PHY LED and configured
in a compatible setting (the network device in the trigger settings must
be the one attached to the PHY, and the link/tx/rx/interval settings
must be supported by that particular LED), instead of blinking the LED
in software, blinking is done by the PHY itself.

Marek

Marek Behún (7):
  leds: trigger: netdev: don't explicitly zero kzalloced data
  leds: trigger: netdev: simplify the driver by using bit field members
  leds: trigger: add API for HW offloading of triggers
  leds: trigger: netdev: support HW offloading
  net: phy: add simple incrementing phyindex member to phy_device struct
  net: phy: add support for LEDs connected to ethernet PHYs
  net: phy: marvell: support LEDs connected on Marvell PHYs

 Documentation/leds/leds-class.rst     |  20 ++
 drivers/leds/led-triggers.c           |   1 +
 drivers/leds/trigger/ledtrig-netdev.c | 111 +++-----
 drivers/net/phy/marvell.c             | 388 +++++++++++++++++++++++++-
 drivers/net/phy/phy_device.c          | 143 ++++++++++
 include/linux/leds.h                  |  27 ++
 include/linux/ledtrig.h               |  40 +++
 include/linux/phy.h                   |  53 ++++
 8 files changed, 709 insertions(+), 74 deletions(-)
 create mode 100644 include/linux/ledtrig.h


base-commit: cd29296fdfca919590e4004a7e4905544f4c4a32
-- 
2.26.2

[PATCH RFC leds + net-next 1/7] leds: trigger: netdev: don't explicitly zero kzalloced data

[Marek Behún]

The trigger_data struct is allocated with kzalloc, so we do not need to
explicitly set members to zero.

Signed-off-by: Marek Behún <kabel@kernel.org>
---
 drivers/leds/trigger/ledtrig-netdev.c | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/drivers/leds/trigger/ledtrig-netdev.c b/drivers/leds/trigger/ledtrig-netdev.c
index d5e774d83021..4f6b73e3b491 100644
--- a/drivers/leds/trigger/ledtrig-netdev.c
+++ b/drivers/leds/trigger/ledtrig-netdev.c
@@ -406,12 +406,8 @@ static int netdev_trig_activate(struct led_classdev *led_cdev)
 	INIT_DELAYED_WORK(&trigger_data->work, netdev_trig_work);
 
 	trigger_data->led_cdev = led_cdev;
-	trigger_data->net_dev = NULL;
-	trigger_data->device_name[0] = 0;
 
-	trigger_data->mode = 0;
 	atomic_set(&trigger_data->interval, msecs_to_jiffies(50));
-	trigger_data->last_activity = 0;
 
 	led_set_trigger_data(led_cdev, trigger_data);
 
-- 
2.26.2

Re: [PATCH RFC leds + net-next 1/7] leds: trigger: netdev: don't explicitly zero kzalloced data

[Pavel Machek]

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

On Fri 2020-10-30 12:44:29, Marek Behún wrote:
> The trigger_data struct is allocated with kzalloc, so we do not need to
> explicitly set members to zero.
> 
> Signed-off-by: Marek Behún <kabel@kernel.org>

Acked-by: Pavel Machek <pavel@ucw.cz>

-- 
http://www.livejournal.com/~pavelmachek

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

[PATCH RFC leds + net-next 2/7] leds: trigger: netdev: simplify the driver by using bit field members

[Marek Behún]

Use bit fields members in struct led_netdev_data instead of one mode
member and set_bit/clear_bit/test_bit functions. These functions are
suitable for longer or variable length bit arrays.

Signed-off-by: Marek Behún <kabel@kernel.org>
---
 drivers/leds/trigger/ledtrig-netdev.c | 69 ++++++++++++---------------
 1 file changed, 30 insertions(+), 39 deletions(-)

diff --git a/drivers/leds/trigger/ledtrig-netdev.c b/drivers/leds/trigger/ledtrig-netdev.c
index 4f6b73e3b491..8f013b6df4fa 100644
--- a/drivers/leds/trigger/ledtrig-netdev.c
+++ b/drivers/leds/trigger/ledtrig-netdev.c
@@ -49,11 +49,11 @@ struct led_netdev_data {
 	atomic_t interval;
 	unsigned int last_activity;
 
-	unsigned long mode;
-#define NETDEV_LED_LINK	0
-#define NETDEV_LED_TX	1
-#define NETDEV_LED_RX	2
-#define NETDEV_LED_MODE_LINKUP	3
+	unsigned link:1;
+	unsigned tx:1;
+	unsigned rx:1;
+
+	unsigned linkup:1;
 };
 
 enum netdev_led_attr {
@@ -73,10 +73,10 @@ static void set_baseline_state(struct led_netdev_data *trigger_data)
 	if (!led_cdev->blink_brightness)
 		led_cdev->blink_brightness = led_cdev->max_brightness;
 
-	if (!test_bit(NETDEV_LED_MODE_LINKUP, &trigger_data->mode))
+	if (!trigger_data->linkup)
 		led_set_brightness(led_cdev, LED_OFF);
 	else {
-		if (test_bit(NETDEV_LED_LINK, &trigger_data->mode))
+		if (trigger_data->link)
 			led_set_brightness(led_cdev,
 					   led_cdev->blink_brightness);
 		else
@@ -85,8 +85,7 @@ static void set_baseline_state(struct led_netdev_data *trigger_data)
 		/* If we are looking for RX/TX start periodically
 		 * checking stats
 		 */
-		if (test_bit(NETDEV_LED_TX, &trigger_data->mode) ||
-		    test_bit(NETDEV_LED_RX, &trigger_data->mode))
+		if (trigger_data->tx || trigger_data->rx)
 			schedule_delayed_work(&trigger_data->work, 0);
 	}
 }
@@ -131,10 +130,10 @@ static ssize_t device_name_store(struct device *dev,
 		trigger_data->net_dev =
 		    dev_get_by_name(&init_net, trigger_data->device_name);
 
-	clear_bit(NETDEV_LED_MODE_LINKUP, &trigger_data->mode);
+	trigger_data->linkup = 0;
 	if (trigger_data->net_dev != NULL)
 		if (netif_carrier_ok(trigger_data->net_dev))
-			set_bit(NETDEV_LED_MODE_LINKUP, &trigger_data->mode);
+			trigger_data->linkup = 1;
 
 	trigger_data->last_activity = 0;
 
@@ -150,23 +149,24 @@ static ssize_t netdev_led_attr_show(struct device *dev, char *buf,
 	enum netdev_led_attr attr)
 {
 	struct led_netdev_data *trigger_data = led_trigger_get_drvdata(dev);
-	int bit;
+	int val;
 
 	switch (attr) {
 	case NETDEV_ATTR_LINK:
-		bit = NETDEV_LED_LINK;
+		val = trigger_data->link;
 		break;
 	case NETDEV_ATTR_TX:
-		bit = NETDEV_LED_TX;
+		val = trigger_data->tx;
 		break;
 	case NETDEV_ATTR_RX:
-		bit = NETDEV_LED_RX;
+		val = trigger_data->rx;
 		break;
 	default:
-		return -EINVAL;
+		/* unreachable */
+		break;
 	}
 
-	return sprintf(buf, "%u\n", test_bit(bit, &trigger_data->mode));
+	return sprintf(buf, "%u\n", val);
 }
 
 static ssize_t netdev_led_attr_store(struct device *dev, const char *buf,
@@ -175,33 +175,28 @@ static ssize_t netdev_led_attr_store(struct device *dev, const char *buf,
 	struct led_netdev_data *trigger_data = led_trigger_get_drvdata(dev);
 	unsigned long state;
 	int ret;
-	int bit;
 
 	ret = kstrtoul(buf, 0, &state);
 	if (ret)
 		return ret;
 
+	cancel_delayed_work_sync(&trigger_data->work);
+
 	switch (attr) {
 	case NETDEV_ATTR_LINK:
-		bit = NETDEV_LED_LINK;
+		trigger_data->link = state;
 		break;
 	case NETDEV_ATTR_TX:
-		bit = NETDEV_LED_TX;
+		trigger_data->tx = state;
 		break;
 	case NETDEV_ATTR_RX:
-		bit = NETDEV_LED_RX;
+		trigger_data->rx = state;
 		break;
 	default:
-		return -EINVAL;
+		/* unreachable */
+		break;
 	}
 
-	cancel_delayed_work_sync(&trigger_data->work);
-
-	if (state)
-		set_bit(bit, &trigger_data->mode);
-	else
-		clear_bit(bit, &trigger_data->mode);
-
 	set_baseline_state(trigger_data);
 
 	return size;
@@ -315,7 +310,7 @@ static int netdev_trig_notify(struct notifier_block *nb,
 
 	spin_lock_bh(&trigger_data->lock);
 
-	clear_bit(NETDEV_LED_MODE_LINKUP, &trigger_data->mode);
+	trigger_data->linkup = 0;
 	switch (evt) {
 	case NETDEV_CHANGENAME:
 	case NETDEV_REGISTER:
@@ -331,7 +326,7 @@ static int netdev_trig_notify(struct notifier_block *nb,
 	case NETDEV_UP:
 	case NETDEV_CHANGE:
 		if (netif_carrier_ok(dev))
-			set_bit(NETDEV_LED_MODE_LINKUP, &trigger_data->mode);
+			trigger_data->linkup = 1;
 		break;
 	}
 
@@ -360,21 +355,17 @@ static void netdev_trig_work(struct work_struct *work)
 	}
 
 	/* If we are not looking for RX/TX then return  */
-	if (!test_bit(NETDEV_LED_TX, &trigger_data->mode) &&
-	    !test_bit(NETDEV_LED_RX, &trigger_data->mode))
+	if (!trigger_data->tx && !trigger_data->rx)
 		return;
 
 	dev_stats = dev_get_stats(trigger_data->net_dev, &temp);
-	new_activity =
-	    (test_bit(NETDEV_LED_TX, &trigger_data->mode) ?
-		dev_stats->tx_packets : 0) +
-	    (test_bit(NETDEV_LED_RX, &trigger_data->mode) ?
-		dev_stats->rx_packets : 0);
+	new_activity = (trigger_data->tx ? dev_stats->tx_packets : 0) +
+		       (trigger_data->rx ? dev_stats->rx_packets : 0);
 
 	if (trigger_data->last_activity != new_activity) {
 		led_stop_software_blink(trigger_data->led_cdev);
 
-		invert = test_bit(NETDEV_LED_LINK, &trigger_data->mode);
+		invert = trigger_data->link;
 		interval = jiffies_to_msecs(
 				atomic_read(&trigger_data->interval));
 		/* base state is ON (link present) */
-- 
2.26.2

Re: [PATCH RFC leds + net-next 2/7] leds: trigger: netdev: simplify the driver by using bit field members

[Jacek Anaszewski]

Hi Marek,

Bitops are guaranteed to be atomic and this was done for a reason.

On 10/30/20 12:44 PM, Marek Behún wrote:
> Use bit fields members in struct led_netdev_data instead of one mode
> member and set_bit/clear_bit/test_bit functions. These functions are
> suitable for longer or variable length bit arrays.
> 
> Signed-off-by: Marek Behún <kabel@kernel.org>
> ---
>   drivers/leds/trigger/ledtrig-netdev.c | 69 ++++++++++++---------------
>   1 file changed, 30 insertions(+), 39 deletions(-)
> 
> diff --git a/drivers/leds/trigger/ledtrig-netdev.c b/drivers/leds/trigger/ledtrig-netdev.c
> index 4f6b73e3b491..8f013b6df4fa 100644
> --- a/drivers/leds/trigger/ledtrig-netdev.c
> +++ b/drivers/leds/trigger/ledtrig-netdev.c
> @@ -49,11 +49,11 @@ struct led_netdev_data {
>   	atomic_t interval;
>   	unsigned int last_activity;
>   
> -	unsigned long mode;
> -#define NETDEV_LED_LINK	0
> -#define NETDEV_LED_TX	1
> -#define NETDEV_LED_RX	2
> -#define NETDEV_LED_MODE_LINKUP	3
> +	unsigned link:1;
> +	unsigned tx:1;
> +	unsigned rx:1;
> +
> +	unsigned linkup:1;
>   };
>   
>   enum netdev_led_attr {
> @@ -73,10 +73,10 @@ static void set_baseline_state(struct led_netdev_data *trigger_data)
>   	if (!led_cdev->blink_brightness)
>   		led_cdev->blink_brightness = led_cdev->max_brightness;
>   
> -	if (!test_bit(NETDEV_LED_MODE_LINKUP, &trigger_data->mode))
> +	if (!trigger_data->linkup)
>   		led_set_brightness(led_cdev, LED_OFF);
[...]

-- 
Best regards,
Jacek Anaszewski

Re: [PATCH RFC leds + net-next 2/7] leds: trigger: netdev: simplify the driver by using bit field members

[Marek Behún]

On Fri, 30 Oct 2020 23:37:52 +0100
Jacek Anaszewski <jacek.anaszewski@gmail.com> wrote:

> Hi Marek,
> 
> Bitops are guaranteed to be atomic and this was done for a reason.

Hmm okay...
Sooo, netdev_trig_work cannot be executed at the same time as the
link/linkup/rx/tx changing stuff from netdev_trig_notify,
interval_store or netdev_led_attr_store, because all these functions
ensure cancelation of netdev_trig_work by calling
cancel_delayed_work_sync. Doesn't this somehow prevent the need for
memory barriers provided by atomic bitops?

BTW Jacek what do you think about the other patches?

Marek

Re: [PATCH RFC leds + net-next 2/7] leds: trigger: netdev: simplify the driver by using bit field members

[Jacek Anaszewski]

On 10/31/20 12:45 AM, Marek Behún wrote:
> On Fri, 30 Oct 2020 23:37:52 +0100
> Jacek Anaszewski <jacek.anaszewski@gmail.com> wrote:
> 
>> Hi Marek,
>>
>> Bitops are guaranteed to be atomic and this was done for a reason.
> 
> Hmm okay...
> Sooo, netdev_trig_work cannot be executed at the same time as the
> link/linkup/rx/tx changing stuff from netdev_trig_notify,
> interval_store or netdev_led_attr_store, because all these functions
> ensure cancelation of netdev_trig_work by calling
> cancel_delayed_work_sync. Doesn't this somehow prevent the need for
> memory barriers provided by atomic bitops?

That's true. But unless there is proven decline in performance related
to use of bitops, I don't see any gain in removing those from this
trigger. They improve code readability.

> BTW Jacek what do you think about the other patches?

I like the idea, but I'd need to spend more time reviewing it.
One thing coming to mind at first glance - it would be good to get rid
of blink_set op at this occasion since this is just another case of
trigger offloading. It would however need touching many drivers, so
that could possibly be done as a follow-up.

-- 
Best regards,
Jacek Anaszewski

Re: [PATCH RFC leds + net-next 2/7] leds: trigger: netdev: simplify the driver by using bit field members

[Pavel Machek]

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

On Fri 2020-10-30 12:44:30, Marek Behún wrote:
> Use bit fields members in struct led_netdev_data instead of one mode
> member and set_bit/clear_bit/test_bit functions. These functions are
> suitable for longer or variable length bit arrays.

They also provide atomicity guarantees. If you can explain why this is
safe, we can do this, but it needs _way_ better changelog.

								Pavel

> Signed-off-by: Marek Behún <kabel@kernel.org>
> ---
>  drivers/leds/trigger/ledtrig-netdev.c | 69 ++++++++++++---------------
>  1 file changed, 30 insertions(+), 39 deletions(-)
> 
> diff --git a/drivers/leds/trigger/ledtrig-netdev.c b/drivers/leds/trigger/ledtrig-netdev.c
> index 4f6b73e3b491..8f013b6df4fa 100644
> --- a/drivers/leds/trigger/ledtrig-netdev.c
> +++ b/drivers/leds/trigger/ledtrig-netdev.c
> @@ -49,11 +49,11 @@ struct led_netdev_data {
>  	atomic_t interval;
>  	unsigned int last_activity;
>  
> -	unsigned long mode;
> -#define NETDEV_LED_LINK	0
> -#define NETDEV_LED_TX	1
> -#define NETDEV_LED_RX	2
> -#define NETDEV_LED_MODE_LINKUP	3
> +	unsigned link:1;
> +	unsigned tx:1;
> +	unsigned rx:1;
> +
> +	unsigned linkup:1;
>  };
>  
>  enum netdev_led_attr {
> @@ -73,10 +73,10 @@ static void set_baseline_state(struct led_netdev_data *trigger_data)
>  	if (!led_cdev->blink_brightness)
>  		led_cdev->blink_brightness = led_cdev->max_brightness;
>  
> -	if (!test_bit(NETDEV_LED_MODE_LINKUP, &trigger_data->mode))
> +	if (!trigger_data->linkup)
>  		led_set_brightness(led_cdev, LED_OFF);
>  	else {
> -		if (test_bit(NETDEV_LED_LINK, &trigger_data->mode))
> +		if (trigger_data->link)
>  			led_set_brightness(led_cdev,
>  					   led_cdev->blink_brightness);
>  		else
> @@ -85,8 +85,7 @@ static void set_baseline_state(struct led_netdev_data *trigger_data)
>  		/* If we are looking for RX/TX start periodically
>  		 * checking stats
>  		 */
> -		if (test_bit(NETDEV_LED_TX, &trigger_data->mode) ||
> -		    test_bit(NETDEV_LED_RX, &trigger_data->mode))
> +		if (trigger_data->tx || trigger_data->rx)
>  			schedule_delayed_work(&trigger_data->work, 0);
>  	}
>  }
> @@ -131,10 +130,10 @@ static ssize_t device_name_store(struct device *dev,
>  		trigger_data->net_dev =
>  		    dev_get_by_name(&init_net, trigger_data->device_name);
>  
> -	clear_bit(NETDEV_LED_MODE_LINKUP, &trigger_data->mode);
> +	trigger_data->linkup = 0;
>  	if (trigger_data->net_dev != NULL)
>  		if (netif_carrier_ok(trigger_data->net_dev))
> -			set_bit(NETDEV_LED_MODE_LINKUP, &trigger_data->mode);
> +			trigger_data->linkup = 1;
>  
>  	trigger_data->last_activity = 0;
>  
> @@ -150,23 +149,24 @@ static ssize_t netdev_led_attr_show(struct device *dev, char *buf,
>  	enum netdev_led_attr attr)
>  {
>  	struct led_netdev_data *trigger_data = led_trigger_get_drvdata(dev);
> -	int bit;
> +	int val;
>  
>  	switch (attr) {
>  	case NETDEV_ATTR_LINK:
> -		bit = NETDEV_LED_LINK;
> +		val = trigger_data->link;
>  		break;
>  	case NETDEV_ATTR_TX:
> -		bit = NETDEV_LED_TX;
> +		val = trigger_data->tx;
>  		break;
>  	case NETDEV_ATTR_RX:
> -		bit = NETDEV_LED_RX;
> +		val = trigger_data->rx;
>  		break;
>  	default:
> -		return -EINVAL;
> +		/* unreachable */
> +		break;
>  	}
>  
> -	return sprintf(buf, "%u\n", test_bit(bit, &trigger_data->mode));
> +	return sprintf(buf, "%u\n", val);
>  }
>  
>  static ssize_t netdev_led_attr_store(struct device *dev, const char *buf,
> @@ -175,33 +175,28 @@ static ssize_t netdev_led_attr_store(struct device *dev, const char *buf,
>  	struct led_netdev_data *trigger_data = led_trigger_get_drvdata(dev);
>  	unsigned long state;
>  	int ret;
> -	int bit;
>  
>  	ret = kstrtoul(buf, 0, &state);
>  	if (ret)
>  		return ret;
>  
> +	cancel_delayed_work_sync(&trigger_data->work);
> +
>  	switch (attr) {
>  	case NETDEV_ATTR_LINK:
> -		bit = NETDEV_LED_LINK;
> +		trigger_data->link = state;
>  		break;
>  	case NETDEV_ATTR_TX:
> -		bit = NETDEV_LED_TX;
> +		trigger_data->tx = state;
>  		break;
>  	case NETDEV_ATTR_RX:
> -		bit = NETDEV_LED_RX;
> +		trigger_data->rx = state;
>  		break;
>  	default:
> -		return -EINVAL;
> +		/* unreachable */
> +		break;
>  	}
>  
> -	cancel_delayed_work_sync(&trigger_data->work);
> -
> -	if (state)
> -		set_bit(bit, &trigger_data->mode);
> -	else
> -		clear_bit(bit, &trigger_data->mode);
> -
>  	set_baseline_state(trigger_data);
>  
>  	return size;
> @@ -315,7 +310,7 @@ static int netdev_trig_notify(struct notifier_block *nb,
>  
>  	spin_lock_bh(&trigger_data->lock);
>  
> -	clear_bit(NETDEV_LED_MODE_LINKUP, &trigger_data->mode);
> +	trigger_data->linkup = 0;
>  	switch (evt) {
>  	case NETDEV_CHANGENAME:
>  	case NETDEV_REGISTER:
> @@ -331,7 +326,7 @@ static int netdev_trig_notify(struct notifier_block *nb,
>  	case NETDEV_UP:
>  	case NETDEV_CHANGE:
>  		if (netif_carrier_ok(dev))
> -			set_bit(NETDEV_LED_MODE_LINKUP, &trigger_data->mode);
> +			trigger_data->linkup = 1;
>  		break;
>  	}
>  
> @@ -360,21 +355,17 @@ static void netdev_trig_work(struct work_struct *work)
>  	}
>  
>  	/* If we are not looking for RX/TX then return  */
> -	if (!test_bit(NETDEV_LED_TX, &trigger_data->mode) &&
> -	    !test_bit(NETDEV_LED_RX, &trigger_data->mode))
> +	if (!trigger_data->tx && !trigger_data->rx)
>  		return;
>  
>  	dev_stats = dev_get_stats(trigger_data->net_dev, &temp);
> -	new_activity =
> -	    (test_bit(NETDEV_LED_TX, &trigger_data->mode) ?
> -		dev_stats->tx_packets : 0) +
> -	    (test_bit(NETDEV_LED_RX, &trigger_data->mode) ?
> -		dev_stats->rx_packets : 0);
> +	new_activity = (trigger_data->tx ? dev_stats->tx_packets : 0) +
> +		       (trigger_data->rx ? dev_stats->rx_packets : 0);
>  
>  	if (trigger_data->last_activity != new_activity) {
>  		led_stop_software_blink(trigger_data->led_cdev);
>  
> -		invert = test_bit(NETDEV_LED_LINK, &trigger_data->mode);
> +		invert = trigger_data->link;
>  		interval = jiffies_to_msecs(
>  				atomic_read(&trigger_data->interval));
>  		/* base state is ON (link present) */
> -- 
> 2.26.2

-- 
http://www.livejournal.com/~pavelmachek

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

[PATCH RFC leds + net-next 3/7] leds: trigger: add API for HW offloading of triggers

[Marek Behún]

Add method trigger_offload() and member variable `offloaded` to struct
led_classdev. Add helper functions led_trigger_offload() and
led_trigger_offload_stop().

The trigger_offload() method, when implemented by the LED driver, should
be called (via led_trigger_offload() function) from trigger code wanting
to be offloaded at the moment when configuration of the trigger changes.

If the trigger is successfully offloaded, this method returns 0 and the
trigger does not have to blink the LED in software.

If the trigger with given configuration cannot be offloaded, the method
should return -EOPNOTSUPP, in which case the trigger must blink the LED
in SW.

Signed-off-by: Marek Behún <kabel@kernel.org>
---
 Documentation/leds/leds-class.rst | 20 ++++++++++++++++++++
 drivers/leds/led-triggers.c       |  1 +
 include/linux/leds.h              | 27 +++++++++++++++++++++++++++
 3 files changed, 48 insertions(+)

diff --git a/Documentation/leds/leds-class.rst b/Documentation/leds/leds-class.rst
index a0708d3f3d0b..45360ff99f8d 100644
--- a/Documentation/leds/leds-class.rst
+++ b/Documentation/leds/leds-class.rst
@@ -169,6 +169,26 @@ Setting the brightness to zero with brightness_set() callback function
 should completely turn off the LED and cancel the previously programmed
 hardware blinking function, if any.
 
+Hardware offloading of LED triggers
+===================================
+
+Some LEDs can offload SW triggers to hardware (for example a LED connected to
+an ethernet PHY or an ethernet switch can be configured to blink on activity on
+the network, which in software is done by the netdev trigger).
+
+To do such offloading, both the trigger code and LED driver must support this.
+The LED must implement the trigger_offload() method and the trigger code must
+try to call this method (via led_trigger_offload() function) when configuration
+of the trigger (trigger_data) changes.
+
+The implementation of the trigger_offload() method by the LED driver must return
+0 if the offload is successful and -EOPNOTSUPP if the requested trigger
+configuration is not supported and the trigger should be executed in software.
+If trigger_offload() returns negative value, the trigger will be done in
+software, so any active offloading must also be disabled.
+
+If the second argument (enable) to the trigger_offload() method is false, any
+active HW offloading must be deactivated.
 
 Known Issues
 ============
diff --git a/drivers/leds/led-triggers.c b/drivers/leds/led-triggers.c
index 91da90cfb11d..9056a66f4661 100644
--- a/drivers/leds/led-triggers.c
+++ b/drivers/leds/led-triggers.c
@@ -177,6 +177,7 @@ int led_trigger_set(struct led_classdev *led_cdev, struct led_trigger *trig)
 			flags);
 		cancel_work_sync(&led_cdev->set_brightness_work);
 		led_stop_software_blink(led_cdev);
+		led_trigger_offload_stop(led_cdev);
 		if (led_cdev->trigger->deactivate)
 			led_cdev->trigger->deactivate(led_cdev);
 		device_remove_groups(led_cdev->dev, led_cdev->trigger->groups);
diff --git a/include/linux/leds.h b/include/linux/leds.h
index 6a8d6409c993..d7e17f1d6f3c 100644
--- a/include/linux/leds.h
+++ b/include/linux/leds.h
@@ -148,6 +148,11 @@ struct led_classdev {
 
 	/* LEDs that have private triggers have this set */
 	struct led_hw_trigger_type	*trigger_type;
+
+	/* some LEDs may be able to offload some SW triggers to HW */
+	int		(*trigger_offload)(struct led_classdev *led_cdev,
+					   bool enable);
+	bool			offloaded;
 #endif
 
 #ifdef CONFIG_LEDS_BRIGHTNESS_HW_CHANGED
@@ -405,6 +410,28 @@ static inline void *led_get_trigger_data(struct led_classdev *led_cdev)
 	return led_cdev->trigger_data;
 }
 
+static inline int led_trigger_offload(struct led_classdev *led_cdev)
+{
+	int ret;
+
+	if (!led_cdev->trigger_offload)
+		return -EOPNOTSUPP;
+
+	ret = led_cdev->trigger_offload(led_cdev, true);
+	led_cdev->offloaded = !ret;
+
+	return ret;
+}
+
+static inline void led_trigger_offload_stop(struct led_classdev *led_cdev)
+{
+	if (!led_cdev->trigger_offload)
+		return;
+
+	if (led_cdev->offloaded)
+		led_cdev->trigger_offload(led_cdev, false);
+}
+
 /**
  * led_trigger_rename_static - rename a trigger
  * @name: the new trigger name
-- 
2.26.2

Re: [PATCH RFC leds + net-next 3/7] leds: trigger: add API for HW offloading of triggers

[Pavel Machek]

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

Hi!

> If the trigger with given configuration cannot be offloaded, the method
> should return -EOPNOTSUPP, in which case the trigger must blink the LED
> in SW.
> 
> Signed-off-by: Marek Behún <kabel@kernel.org>

> +
> +If the second argument (enable) to the trigger_offload() method is false, any
> +active HW offloading must be deactivated.

Are errors permitted in the "disable" case?

> +static inline void led_trigger_offload_stop(struct led_classdev *led_cdev)
> +{
> +	if (!led_cdev->trigger_offload)
> +		return;
> +
> +	if (led_cdev->offloaded)
> +		led_cdev->trigger_offload(led_cdev, false);
> +}

Set offloaded to false?

Let me check the rest to decide if this makes sense.

Best regards,
									Pavel

-- 
http://www.livejournal.com/~pavelmachek

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

[PATCH RFC leds + net-next 4/7] leds: trigger: netdev: support HW offloading

[Marek Behún]

Add support for HW offloading of the netdev trigger.

We need to change spinlock to mutex, because if spinlock is used, the
trigger_offload() method cannot sleep, which can happen for ethernet
PHYs.

We can change the spinlock to mutex because, according to Jacek:
  register_netdevice_notifier() registers raw notifier chain,
  whose callbacks are not called from atomic context and there are
  no restrictions on callbacks. See include/linux/notifier.h.

Move struct led_trigger_data into global include directory, into file
linux/ledtrig.h, so that drivers wanting to offload the trigger can
access its settings.

Also export the netdev_led_trigger variable and declare it in ledtrih.h
so that drivers may check whether the LED is set to this trigger.

Signed-off-by: Marek Behún <kabel@kernel.org>
---
 drivers/leds/trigger/ledtrig-netdev.c | 48 ++++++++++-----------------
 include/linux/ledtrig.h               | 40 ++++++++++++++++++++++
 2 files changed, 57 insertions(+), 31 deletions(-)
 create mode 100644 include/linux/ledtrig.h

diff --git a/drivers/leds/trigger/ledtrig-netdev.c b/drivers/leds/trigger/ledtrig-netdev.c
index 8f013b6df4fa..d4fa031d3a9e 100644
--- a/drivers/leds/trigger/ledtrig-netdev.c
+++ b/drivers/leds/trigger/ledtrig-netdev.c
@@ -10,17 +10,16 @@
 //  Copyright 2005-2006 Openedhand Ltd.
 //  Author: Richard Purdie <rpurdie@openedhand.com>
 
-#include <linux/atomic.h>
 #include <linux/ctype.h>
 #include <linux/device.h>
 #include <linux/init.h>
 #include <linux/jiffies.h>
 #include <linux/kernel.h>
 #include <linux/leds.h>
+#include <linux/ledtrig.h>
 #include <linux/list.h>
 #include <linux/module.h>
 #include <linux/netdevice.h>
-#include <linux/spinlock.h>
 #include <linux/timer.h>
 #include "../leds.h"
 
@@ -36,26 +35,6 @@
  *
  */
 
-struct led_netdev_data {
-	spinlock_t lock;
-
-	struct delayed_work work;
-	struct notifier_block notifier;
-
-	struct led_classdev *led_cdev;
-	struct net_device *net_dev;
-
-	char device_name[IFNAMSIZ];
-	atomic_t interval;
-	unsigned int last_activity;
-
-	unsigned link:1;
-	unsigned tx:1;
-	unsigned rx:1;
-
-	unsigned linkup:1;
-};
-
 enum netdev_led_attr {
 	NETDEV_ATTR_LINK,
 	NETDEV_ATTR_TX,
@@ -73,6 +52,9 @@ static void set_baseline_state(struct led_netdev_data *trigger_data)
 	if (!led_cdev->blink_brightness)
 		led_cdev->blink_brightness = led_cdev->max_brightness;
 
+	if (!led_trigger_offload(led_cdev))
+		return;
+
 	if (!trigger_data->linkup)
 		led_set_brightness(led_cdev, LED_OFF);
 	else {
@@ -96,9 +78,9 @@ static ssize_t device_name_show(struct device *dev,
 	struct led_netdev_data *trigger_data = led_trigger_get_drvdata(dev);
 	ssize_t len;
 
-	spin_lock_bh(&trigger_data->lock);
+	mutex_lock(&trigger_data->lock);
 	len = sprintf(buf, "%s\n", trigger_data->device_name);
-	spin_unlock_bh(&trigger_data->lock);
+	mutex_unlock(&trigger_data->lock);
 
 	return len;
 }
@@ -114,7 +96,7 @@ static ssize_t device_name_store(struct device *dev,
 
 	cancel_delayed_work_sync(&trigger_data->work);
 
-	spin_lock_bh(&trigger_data->lock);
+	mutex_lock(&trigger_data->lock);
 
 	if (trigger_data->net_dev) {
 		dev_put(trigger_data->net_dev);
@@ -138,7 +120,7 @@ static ssize_t device_name_store(struct device *dev,
 	trigger_data->last_activity = 0;
 
 	set_baseline_state(trigger_data);
-	spin_unlock_bh(&trigger_data->lock);
+	mutex_unlock(&trigger_data->lock);
 
 	return size;
 }
@@ -295,6 +277,7 @@ static int netdev_trig_notify(struct notifier_block *nb,
 		netdev_notifier_info_to_dev((struct netdev_notifier_info *)dv);
 	struct led_netdev_data *trigger_data =
 		container_of(nb, struct led_netdev_data, notifier);
+	bool reset = true;
 
 	if (evt != NETDEV_UP && evt != NETDEV_DOWN && evt != NETDEV_CHANGE
 	    && evt != NETDEV_REGISTER && evt != NETDEV_UNREGISTER
@@ -308,7 +291,7 @@ static int netdev_trig_notify(struct notifier_block *nb,
 
 	cancel_delayed_work_sync(&trigger_data->work);
 
-	spin_lock_bh(&trigger_data->lock);
+	mutex_lock(&trigger_data->lock);
 
 	trigger_data->linkup = 0;
 	switch (evt) {
@@ -327,12 +310,14 @@ static int netdev_trig_notify(struct notifier_block *nb,
 	case NETDEV_CHANGE:
 		if (netif_carrier_ok(dev))
 			trigger_data->linkup = 1;
+		reset = !trigger_data->led_cdev->offloaded;
 		break;
 	}
 
-	set_baseline_state(trigger_data);
+	if (reset)
+		set_baseline_state(trigger_data);
 
-	spin_unlock_bh(&trigger_data->lock);
+	mutex_unlock(&trigger_data->lock);
 
 	return NOTIFY_DONE;
 }
@@ -389,7 +374,7 @@ static int netdev_trig_activate(struct led_classdev *led_cdev)
 	if (!trigger_data)
 		return -ENOMEM;
 
-	spin_lock_init(&trigger_data->lock);
+	mutex_init(&trigger_data->lock);
 
 	trigger_data->notifier.notifier_call = netdev_trig_notify;
 	trigger_data->notifier.priority = 10;
@@ -423,12 +408,13 @@ static void netdev_trig_deactivate(struct led_classdev *led_cdev)
 	kfree(trigger_data);
 }
 
-static struct led_trigger netdev_led_trigger = {
+struct led_trigger netdev_led_trigger = {
 	.name = "netdev",
 	.activate = netdev_trig_activate,
 	.deactivate = netdev_trig_deactivate,
 	.groups = netdev_trig_groups,
 };
+EXPORT_SYMBOL_GPL(netdev_led_trigger);
 
 static int __init netdev_trig_init(void)
 {
diff --git a/include/linux/ledtrig.h b/include/linux/ledtrig.h
new file mode 100644
index 000000000000..593b2bee6ca0
--- /dev/null
+++ b/include/linux/ledtrig.h
@@ -0,0 +1,40 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * LED trigger shared structures
+ */
+
+#ifndef __LINUX_LEDTRIG_H__
+#define __LINUX_LEDTRIG_H__
+
+#include <linux/atomic.h>
+#include <linux/leds.h>
+#include <linux/mutex.h>
+#include <linux/netdevice.h>
+
+#if IS_ENABLED(CONFIG_LEDS_TRIGGER_NETDEV)
+
+struct led_netdev_data {
+	struct mutex lock;
+
+	struct delayed_work work;
+	struct notifier_block notifier;
+
+	struct led_classdev *led_cdev;
+	struct net_device *net_dev;
+
+	char device_name[IFNAMSIZ];
+	atomic_t interval;
+	unsigned int last_activity;
+
+	unsigned link:1;
+	unsigned tx:1;
+	unsigned rx:1;
+
+	unsigned linkup:1;
+};
+
+extern struct led_trigger netdev_led_trigger;
+
+#endif /* IS_ENABLED(CONFIG_LEDS_TRIGGER_NETDEV) */
+
+#endif /* __LINUX_LEDTRIG_H__ */
-- 
2.26.2

Re: [PATCH RFC leds + net-next 4/7] leds: trigger: netdev: support HW offloading

[Marek Behún]

On Fri, 30 Oct 2020 12:44:32 +0100
Marek Behún <kabel@kernel.org> wrote:

> +	if (!led_trigger_offload(led_cdev))
> +		return;
> +

The offload method has to read the settings, so this code (better: the
whole set_baseline_state function) has to be always called with the
mutex locked. (I am talking about the mutex which was a spinlock before
this patch.)

I will fix this in next version.

Marek

Re: [PATCH RFC leds + net-next 4/7] leds: trigger: netdev: support HW offloading

[Pavel Machek]

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

Hi!

> Add support for HW offloading of the netdev trigger.
> 
> We need to change spinlock to mutex, because if spinlock is used, the
> trigger_offload() method cannot sleep, which can happen for ethernet
> PHYs.

Is that bugfix or just needed for offloading? Should be separate patch
in any case.

> Move struct led_trigger_data into global include directory, into file
> linux/ledtrig.h, so that drivers wanting to offload the trigger can
> access its settings.

Separate...

> @@ -327,12 +310,14 @@ static int netdev_trig_notify(struct notifier_block *nb,
>  	case NETDEV_CHANGE:
>  		if (netif_carrier_ok(dev))
>  			trigger_data->linkup = 1;
> +		reset = !trigger_data->led_cdev->offloaded;
>  		break;
>  	}
>  
> -	set_baseline_state(trigger_data);
> +	if (reset)
> +		set_baseline_state(trigger_data);
>  
> -	spin_unlock_bh(&trigger_data->lock);
> +	mutex_unlock(&trigger_data->lock);
>  
>  	return NOTIFY_DONE;
>  }

Is this the only thing it saves? Because.. that would not be worth
it.

Best regards,
									Pavel

-- 
http://www.livejournal.com/~pavelmachek

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

[PATCH RFC leds + net-next 5/7] net: phy: add simple incrementing phyindex member to phy_device struct

[Marek Behún]

Add a new integer member phyindex to struct phy_device. This member is
unique for every phy_device. Atomic incrementation occurs in
phy_device_register.

This can be used for example in LED sysfs API. The LED subsystem names
each LED in format `device:color:function`, but currently the PHY device
names are not suited for this, since in some situations a PHY device
name can look like this
  d0032004.mdio-mii:01
or even like this
  !soc!internal-regs@d0000000!mdio@32004!switch0@10!mdio:08
Clearly this cannot be used as the `device` part of a LED name.

Signed-off-by: Marek Behún <kabel@kernel.org>
---
 drivers/net/phy/phy_device.c | 3 +++
 include/linux/phy.h          | 3 +++
 2 files changed, 6 insertions(+)

diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index 5dab6be6fc38..38f581cc9713 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -9,6 +9,7 @@
 
 #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
 
+#include <linux/atomic.h>
 #include <linux/bitmap.h>
 #include <linux/delay.h>
 #include <linux/errno.h>
@@ -892,6 +893,7 @@ EXPORT_SYMBOL(get_phy_device);
  */
 int phy_device_register(struct phy_device *phydev)
 {
+	static atomic_t phyindex;
 	int err;
 
 	err = mdiobus_register_device(&phydev->mdio);
@@ -908,6 +910,7 @@ int phy_device_register(struct phy_device *phydev)
 		goto out;
 	}
 
+	phydev->phyindex = atomic_inc_return(&phyindex) - 1;
 	err = device_add(&phydev->mdio.dev);
 	if (err) {
 		phydev_err(phydev, "failed to add\n");
diff --git a/include/linux/phy.h b/include/linux/phy.h
index eb3cb1a98b45..6dd4a28135c3 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
@@ -480,6 +480,7 @@ struct macsec_ops;
  *
  * @mdio: MDIO bus this PHY is on
  * @drv: Pointer to the driver for this PHY instance
+ * @phyindex: a simple incrementing PHY index
  * @phy_id: UID for this device found during discovery
  * @c45_ids: 802.3-c45 Device Identifiers if is_c45.
  * @is_c45:  Set to true if this PHY uses clause 45 addressing.
@@ -551,6 +552,8 @@ struct phy_device {
 	/* And management functions */
 	struct phy_driver *drv;
 
+	int phyindex;
+
 	u32 phy_id;
 
 	struct phy_c45_device_ids c45_ids;
-- 
2.26.2

Re: [PATCH RFC leds + net-next 5/7] net: phy: add simple incrementing phyindex member to phy_device struct

[Pavel Machek]

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

On Fri 2020-10-30 12:44:33, Marek Behún wrote:
> Add a new integer member phyindex to struct phy_device. This member is
> unique for every phy_device. Atomic incrementation occurs in
> phy_device_register.
> 
> This can be used for example in LED sysfs API. The LED subsystem names
> each LED in format `device:color:function`, but currently the PHY device
> names are not suited for this, since in some situations a PHY device
> name can look like this
>   d0032004.mdio-mii:01
> or even like this
>   !soc!internal-regs@d0000000!mdio@32004!switch0@10!mdio:08
> Clearly this cannot be used as the `device` part of a LED name.
> 
> Signed-off-by: Marek Behún <kabel@kernel.org>

Atomic should _not_ be neccessary for this. Just make sure access is
serialised by some existing lock.
								Pavel
								
> @@ -892,6 +893,7 @@ EXPORT_SYMBOL(get_phy_device);
>   */
>  int phy_device_register(struct phy_device *phydev)
>  {
> +	static atomic_t phyindex;
>  	int err;
>  
>  	err = mdiobus_register_device(&phydev->mdio);
> @@ -908,6 +910,7 @@ int phy_device_register(struct phy_device *phydev)
>  		goto out;
>  	}
>  
> +	phydev->phyindex = atomic_inc_return(&phyindex) - 1;
>  	err = device_add(&phydev->mdio.dev);
>  	if (err) {
>  		phydev_err(phydev, "failed to add\n");

-- 
http://www.livejournal.com/~pavelmachek

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

[PATCH RFC leds + net-next 6/7] net: phy: add support for LEDs connected to ethernet PHYs

[Marek Behún]

Many an ethernet PHY chip has pins dedicated for LEDs. On some PHYs it
can be configured via registers whether the LED should be ON, OFF, or
whether its state should depend on events within the chip (link, rx/tx
activity and so on).

Add support for probing such LEDs.

A PHY driver wishing to utilize this API must implement methods
led_init() and led_brightness_set(). Methods led_blink_set() and
led_trigger_offload() are optional.

Signed-off-by: Marek Behún <kabel@kernel.org>
---
 drivers/net/phy/phy_device.c | 140 +++++++++++++++++++++++++++++++++++
 include/linux/phy.h          |  50 +++++++++++++
 2 files changed, 190 insertions(+)

diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index 38f581cc9713..6bea8635cac8 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -2816,6 +2816,140 @@ s32 phy_get_internal_delay(struct phy_device *phydev, struct device *dev,
 }
 EXPORT_SYMBOL(phy_get_internal_delay);
 
+#if IS_ENABLED(CONFIG_LEDS_CLASS)
+
+static int phy_led_brightness_set(struct led_classdev *cdev,
+				  enum led_brightness brightness)
+{
+	struct phy_device *phydev = to_phy_device(cdev->dev->parent);
+	struct phy_led *led = to_phy_led(cdev);
+	int ret;
+
+	mutex_lock(&phydev->lock);
+	ret = phydev->drv->led_brightness_set(phydev, led, brightness);
+	mutex_unlock(&phydev->lock);
+
+	return ret;
+}
+
+static int phy_led_blink_set(struct led_classdev *cdev, unsigned long *delay_on,
+			     unsigned long *delay_off)
+{
+	struct phy_device *phydev = to_phy_device(cdev->dev->parent);
+	struct phy_led *led = to_phy_led(cdev);
+	int ret;
+
+	mutex_lock(&phydev->lock);
+	ret = phydev->drv->led_blink_set(phydev, led, delay_on, delay_off);
+	mutex_unlock(&phydev->lock);
+
+	return ret;
+}
+
+#if IS_ENABLED(CONFIG_LEDS_TRIGGERS)
+static int phy_led_trigger_offload(struct led_classdev *cdev, bool enable)
+{
+	struct phy_device *phydev = to_phy_device(cdev->dev->parent);
+	struct phy_led *led = to_phy_led(cdev);
+	int ret;
+
+	mutex_lock(&phydev->lock);
+	ret = phydev->drv->led_trigger_offload(phydev, led, enable);
+	mutex_unlock(&phydev->lock);
+
+	return ret;
+}
+#endif /* IS_ENABLED(CONFIG_LEDS_TRIGGERS) */
+
+static int phy_probe_led(struct phy_device *phydev,
+			 struct fwnode_handle *fwnode, char *devicename)
+{
+	struct device *dev = &phydev->mdio.dev;
+	struct led_init_data init_data = {};
+	struct phy_led *led;
+	u32 reg;
+	int ret;
+
+	led = devm_kzalloc(dev, sizeof(*led), GFP_KERNEL);
+	if (!led)
+		return -ENOMEM;
+
+	led->addr = -1;
+	if (!fwnode_property_read_u32(fwnode, "reg", &reg))
+		led->addr = reg;
+
+	led->active_low = !fwnode_property_read_bool(fwnode,
+						     "enable-active-high");
+	led->tristate = fwnode_property_read_bool(fwnode, "tristate");
+
+	led->cdev.max_brightness = 1;
+	led->cdev.brightness_set_blocking = phy_led_brightness_set;
+	if (phydev->drv->led_blink_set)
+		led->cdev.blink_set = phy_led_blink_set;
+#if IS_ENABLED(CONFIG_LEDS_TRIGGERS)
+	if (phydev->drv->led_trigger_offload)
+		led->cdev.trigger_offload = phy_led_trigger_offload;
+#endif /* IS_ENABLED(CONFIG_LEDS_TRIGGERS) */
+
+	ret = phydev->drv->led_init(phydev, led, fwnode);
+	if (ret)
+		goto err;
+
+	init_data.fwnode = fwnode;
+	init_data.devname_mandatory = true;
+	init_data.devicename = devicename;
+
+	ret = devm_led_classdev_register_ext(dev, &led->cdev, &init_data);
+	if (ret)
+		goto err;
+
+	return 0;
+err:
+	devm_kfree(dev, led);
+	return ret;
+}
+
+static int phy_probe_leds(struct phy_device *phydev)
+{
+	struct fwnode_handle *leds, *led;
+	char devicename[32];
+	int ret;
+
+	if (!phydev->drv->led_init)
+		return 0;
+
+	if (WARN_ON(!phydev->drv->led_brightness_set))
+		return 0;
+
+	leds = device_get_named_child_node(&phydev->mdio.dev, "leds");
+	if (!leds)
+		return 0;
+
+	snprintf(devicename, sizeof(devicename), "ethernet-phy%i",
+		 phydev->phyindex);
+
+	fwnode_for_each_available_child_node(leds, led) {
+		ret = phy_probe_led(phydev, led, devicename);
+		if (ret) {
+			phydev_err(phydev, "Error probing PHY LED %pfw: %d\n",
+				   led, ret);
+			fwnode_handle_put(led);
+			return ret;
+		}
+	}
+
+	return 0;
+}
+
+#else /* !IS_ENABLED(CONFIG_LEDS_CLASS) */
+
+static inline int phy_probe_leds(struct phy_device *phydev)
+{
+	return 0;
+}
+
+#endif /* IS_ENABLED(CONFIG_LEDS_CLASS) */
+
 static bool phy_drv_supports_irq(struct phy_driver *phydrv)
 {
 	return phydrv->config_intr && phydrv->ack_interrupt;
@@ -2923,6 +3057,12 @@ static int phy_probe(struct device *dev)
 
 	mutex_unlock(&phydev->lock);
 
+	/* LEDs have to be registered with phydev mutex unlocked, because some
+	 * operations can be called during registration that lock the mutex.
+	 */
+	if (!err)
+		err = phy_probe_leds(phydev);
+
 	return err;
 }
 
diff --git a/include/linux/phy.h b/include/linux/phy.h
index 6dd4a28135c3..7df3f4632bdc 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
@@ -14,6 +14,7 @@
 #include <linux/compiler.h>
 #include <linux/spinlock.h>
 #include <linux/ethtool.h>
+#include <linux/leds.h>
 #include <linux/linkmode.h>
 #include <linux/netlink.h>
 #include <linux/mdio.h>
@@ -679,6 +680,28 @@ struct phy_tdr_config {
 };
 #define PHY_PAIR_ALL -1
 
+/**
+ * struct phy_led - a PHY connected LED instance
+ *
+ * @cdev: underlying LED classdev
+ * @addr: identifier of the LED on the PHY, -1 if not present
+ * @active_low: whether this LED is connected as active low
+ * @tristate: whether this LED should be put into tristate mode when off
+ * @priv: private data for the underlying driver
+ */
+struct phy_led {
+	struct led_classdev cdev;
+
+	/* These come from firmware/OF */
+	int addr;
+	unsigned active_low:1;
+	unsigned tristate:1;
+
+	/* driver private data */
+	void *priv;
+};
+#define to_phy_led(l) container_of(l, struct phy_led, cdev)
+
 /**
  * struct phy_driver - Driver structure for a particular PHY type
  *
@@ -884,6 +907,33 @@ struct phy_driver {
 	int (*get_sqi)(struct phy_device *dev);
 	/** @get_sqi_max: Get the maximum signal quality indication */
 	int (*get_sqi_max)(struct phy_device *dev);
+
+#if IS_ENABLED(CONFIG_LEDS_CLASS)
+	/* PHY connected LEDs operations */
+	/** @led_init: Check for valid configuration and initialize the LED */
+	int (*led_init)(struct phy_device *dev, struct phy_led *led,
+			struct fwnode_handle *fwnode);
+	/**
+	 * @led_brightness_set: Set the brightness of the LED. Mandatory if
+	 * led_init is present. Refer to method brightness_set_blocking() from
+	 * struct led_classdev in linux/leds.h
+	 */
+	int (*led_brightness_set)(struct phy_device *dev, struct phy_led *led,
+				  enum led_brightness brightness);
+	/**
+	 * @led_blink:set: Set HW blinking. Refer to method blink_set() from
+	 * struct led_classdev in linux/leds.h
+	 */
+	int (*led_blink_set)(struct phy_device *dev, struct phy_led *led,
+			     unsigned long *delay_on, unsigned long *delay_off);
+	/**
+	 * @led_trigger_offload: If possible, offload LED trigger to HW.
+	 * Refer to method trigger_offload() from struct led_classdev in
+	 * linux/leds.h
+	 */
+	int (*led_trigger_offload)(struct phy_device *dev, struct phy_led *led,
+				   bool enable);
+#endif /* IS_ENABLED(CONFIG_LEDS_CLASS) */
 };
 #define to_phy_driver(d) container_of(to_mdio_common_driver(d),		\
 				      struct phy_driver, mdiodrv)
-- 
2.26.2

Re: [PATCH RFC leds + net-next 6/7] net: phy: add support for LEDs connected to ethernet PHYs

[Pavel Machek]

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

HI!

> Many an ethernet PHY chip has pins dedicated for LEDs. On some PHYs it
> can be configured via registers whether the LED should be ON, OFF, or
> whether its state should depend on events within the chip (link, rx/tx
> activity and so on).
> 
> Add support for probing such LEDs.
> 
> A PHY driver wishing to utilize this API must implement methods
> led_init() and led_brightness_set(). Methods led_blink_set() and
> led_trigger_offload() are optional.
> 
> Signed-off-by: Marek Behún <kabel@kernel.org>
> ---
>  drivers/net/phy/phy_device.c | 140 +++++++++++++++++++++++++++++++++++
>  include/linux/phy.h          |  50 +++++++++++++
>  2 files changed, 190 insertions(+)

> +	led = devm_kzalloc(dev, sizeof(*led), GFP_KERNEL);
> +	if (!led)
> +		return -ENOMEM;
> +
> +	led->addr = -1;
> +	if (!fwnode_property_read_u32(fwnode, "reg", &reg))
> +		led->addr = reg;
> +
> +	led->active_low = !fwnode_property_read_bool(fwnode,
> +						     "enable-active-high");
> +	led->tristate = fwnode_property_read_bool(fwnode, "tristate");

Does this need binding documentation?

>  	mutex_unlock(&phydev->lock);
>  
> +	/* LEDs have to be registered with phydev mutex unlocked, because some
> +	 * operations can be called during registration that lock the mutex.
> +	 */
> +	if (!err)
> +		err = phy_probe_leds(phydev);
> +
>  	return err;
>  }

Is it safe to do the probing without the mutex?

Should error in LED probing fail the whole phy probe?

Best regards,
								Pavel
-- 
http://www.livejournal.com/~pavelmachek

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

[PATCH RFC leds + net-next 7/7] net: phy: marvell: support LEDs connected on Marvell PHYs

[Marek Behún]

Add support for controlling the LEDs connected to several families of
Marvell PHYs via Linux LED API. These families currently are: 88E1112,
88E1116R, 88E1118, 88E1121R, 88E1149R, 88E1240, 88E1318S, 88E1340S,
88E1510, 88E1545 and 88E1548P.

This does not yet add support for compound LED modes. This could be
achieved via the LED multicolor framework.

netdev trigger offloading is also implemented.

Signed-off-by: Marek Behún <kabel@kernel.org>
---
 drivers/net/phy/marvell.c | 388 +++++++++++++++++++++++++++++++++++++-
 1 file changed, 383 insertions(+), 5 deletions(-)

diff --git a/drivers/net/phy/marvell.c b/drivers/net/phy/marvell.c
index 5aec673a0120..ffbf8da7c0a3 100644
--- a/drivers/net/phy/marvell.c
+++ b/drivers/net/phy/marvell.c
@@ -19,6 +19,7 @@
 #include <linux/interrupt.h>
 #include <linux/init.h>
 #include <linux/delay.h>
+#include <linux/ledtrig.h>
 #include <linux/netdevice.h>
 #include <linux/etherdevice.h>
 #include <linux/skbuff.h>
@@ -143,9 +144,26 @@
 #define MII_88E1318S_PHY_WOL_CTRL_CLEAR_WOL_STATUS		BIT(12)
 #define MII_88E1318S_PHY_WOL_CTRL_MAGIC_PACKET_MATCH_ENABLE	BIT(14)
 
-#define MII_PHY_LED_CTRL	        16
-#define MII_88E1121_PHY_LED_DEF		0x0030
-#define MII_88E1510_PHY_LED_DEF		0x1177
+#define MII_PHY_LED_CTRL		0x10
+#define MII_PHY_LED45_CTRL		0x13
+#define MII_PHY_LED_CTRL_OFF		0x8
+#define MII_PHY_LED_CTRL_ON		0x9
+#define MII_PHY_LED_CTRL_BLINK		0xb
+
+#define MII_PHY_LED_POLARITY_CTRL	0x11
+
+#define MII_PHY_LED_TCR			0x12
+#define MII_PHY_LED_TCR_PULSESTR_MASK	0x7000
+#define MII_PHY_LED_TCR_PULSESTR_SHIFT	12
+#define MII_PHY_LED_TCR_BLINKRATE_MASK	0x0700
+#define MII_PHY_LED_TCR_BLINKRATE_SHIFT	8
+#define MII_PHY_LED_TCR_SPDOFF_MASK	0x000c
+#define MII_PHY_LED_TCR_SPDOFF_SHIFT	2
+#define MII_PHY_LED_TCR_SPDON_MASK	0x0003
+#define MII_PHY_LED_TCR_SPDON_SHIFT	0
+
+#define MII_88E1121_PHY_LED_DEF			0x0030
+#define MII_88E1510_PHY_LED_DEF			0x1177
 #define MII_88E1510_PHY_LED0_LINK_LED1_ACTIVE	0x1040
 
 #define MII_M1011_PHY_STATUS		0x11
@@ -280,6 +298,7 @@ struct marvell_priv {
 	u32 last;
 	u32 step;
 	s8 pair;
+	u16 legacy_led_config_mask;
 };
 
 static int marvell_read_page(struct phy_device *phydev)
@@ -662,8 +681,354 @@ static int m88e1510_config_aneg(struct phy_device *phydev)
 	return err;
 }
 
+#if IS_ENABLED(CONFIG_LEDS_CLASS)
+
+static inline int marvell_led_reg(int led)
+{
+	switch (led) {
+	case 0 ... 3:
+		return MII_PHY_LED_CTRL;
+	case 4 ... 5:
+		return MII_PHY_LED45_CTRL;
+	default:
+		return -EINVAL;
+	}
+}
+
+static int marvell_led_set_regval(struct phy_device *phydev, int led, u16 val)
+{
+	u16 mask;
+	int reg;
+
+	reg = marvell_led_reg(led);
+	if (reg < 0)
+		return reg;
+
+	val <<= (led % 4) * 4;
+	mask = 0xf << ((led % 4) * 4);
+
+	return phy_modify_paged(phydev, MII_MARVELL_LED_PAGE, reg, mask, val);
+}
+
+static int marvell_led_set_polarity(struct phy_device *phydev, int led,
+				    bool active_low, bool tristate)
+{
+	int reg, shift;
+	u16 mask, val;
+
+	switch (led) {
+	case 0 ... 3:
+		reg = MII_PHY_LED_POLARITY_CTRL;
+		break;
+	case 4 ... 5:
+		reg = MII_PHY_LED45_CTRL;
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	val = 0;
+	if (!active_low)
+		val |= BIT(0);
+	if (tristate)
+		val |= BIT(1);
+
+	shift = led * 2;
+	val <<= shift;
+	mask = 0x3 << shift;
+
+	return phy_modify_paged(phydev, MII_MARVELL_LED_PAGE, reg, mask, val);
+}
+
+static int marvell_led_brightness_set(struct phy_device *phydev,
+				      struct phy_led *led,
+				      enum led_brightness brightness)
+{
+	u8 val;
+
+	/* don't do anything if a trigger is offloaded to HW */
+	if (led->cdev.offloaded)
+		return 0;
+
+	val = brightness ? MII_PHY_LED_CTRL_ON : MII_PHY_LED_CTRL_OFF;
+
+	return marvell_led_set_regval(phydev, led->addr, val);
+}
+
+static int marvell_led_round_blink_rate(unsigned long *period)
+{
+	/* Each interval is (0.7 * p, 1.3 * p), where p is the period supported
+	 * by the chip. Should we change this so that there are no holes between
+	 * these intervals?
+	 */
+	switch (*period) {
+	case 29 ... 55:
+		*period = 42;
+		return 0;
+	case 58 ... 108:
+		*period = 84;
+		return 1;
+	case 119 ... 221:
+		*period = 170;
+		return 2;
+	case 238 ... 442:
+		*period = 340;
+		return 3;
+	case 469 ... 871:
+		*period = 670;
+		return 4;
+	default:
+		return -EOPNOTSUPP;
+	}
+}
+
+static int marvell_leds_set_blink_rate(struct phy_device *phydev,
+				       unsigned long *period)
+{
+	int val;
+
+	val = marvell_led_round_blink_rate(period);
+	if (val < 0)
+		return val;
+
+	val = val << MII_PHY_LED_TCR_BLINKRATE_SHIFT &
+	      MII_PHY_LED_TCR_BLINKRATE_MASK;
+
+	return phy_modify_paged(phydev, MII_MARVELL_LED_PAGE, MII_PHY_LED_TCR,
+				MII_PHY_LED_TCR_BLINKRATE_MASK, val);
+}
+
+static int marvell_led_blink_set(struct phy_device *phydev, struct phy_led *led,
+				 unsigned long *on, unsigned long *off)
+{
+	unsigned long period;
+	int ret;
+
+	/* default to period 670ms, which is the maximum the HW is capable of */
+	if (!*on || !*off)
+		*on = *off = 670 / 2;
+
+	/* blink in software if there is more than 30% difference between the
+	 * delays
+	 */
+	if (*on * 100 / *off > 130 || *off * 100 / *on > 130)
+		return -EOPNOTSUPP;
+
+	period = *on + *off;
+
+	ret = marvell_leds_set_blink_rate(phydev, &period);
+	if (ret < 0)
+		return ret;
+
+	ret = marvell_led_set_regval(phydev, led->addr, MII_PHY_LED_CTRL_BLINK);
+	if (ret < 0)
+		return ret;
+
+	*on = *off = period / 2;
+
+	return 0;
+}
+
+#define BITIF(i, cond)			((cond) ? BIT(i) : 0)
+#define LED_MODE(link, tx, rx)					\
+	(BITIF(0, (link)) | BITIF(1, (tx)) | BITIF(2, (rx)))
+
+struct marvell_led_mode_info {
+	u32 key;
+	s8 regval[6];
+	enum {
+		COMMON	= BIT(0),
+		L1V0_RX	= BIT(1),
+		L3V5_TX	= BIT(2),
+	} flags;
+};
+
+struct marvell_leds_info {
+	u32 family;
+	int nleds;
+	u32 flags;
+};
+
+#define LED(f, n, flg)							\
+	{								\
+		.family = MARVELL_PHY_FAMILY_ID(MARVELL_PHY_ID_88E##f),	\
+		.nleds = (n),						\
+		.flags = (flg),						\
+	}								\
+
+static const struct marvell_leds_info marvell_leds_info[] = {
+	LED(1112,  4, COMMON | L3V5_TX),
+	LED(1116R, 3, COMMON),
+	LED(1118,  3, COMMON),
+	LED(1121R, 3, COMMON),
+	LED(1149R, 4, COMMON | L3V5_TX),
+	LED(1240,  6, COMMON | L3V5_TX),
+	LED(1318S, 3, COMMON | L1V0_RX),
+	LED(1340S, 6, COMMON),
+	LED(1510,  3, COMMON | L1V0_RX),
+	LED(1545,  6, COMMON),
+	LED(1548P, 6, COMMON),
+};
+
+static const struct marvell_led_mode_info marvell_led_mode_info[] = {
+	{ LED_MODE(1, 0, 0), { 0x0,  -1, 0x0,  -1,  -1,  -1, }, COMMON },
+	{ LED_MODE(1, 1, 1), { 0x1, 0x1, 0x1, 0x1, 0x1, 0x1, }, COMMON },
+	{ LED_MODE(0, 1, 1), { 0x4, 0x4, 0x4, 0x4, 0x4, 0x4, }, COMMON },
+	{ LED_MODE(1, 0, 1), {  -1, 0x2,  -1, 0x2, 0x2, 0x2, }, COMMON },
+	{ LED_MODE(0, 1, 0), { 0x5,  -1, 0x5,  -1, 0x5, 0x5, }, COMMON },
+	{ LED_MODE(0, 1, 0), {  -1,  -1,  -1, 0x5,  -1,  -1, }, L3V5_TX },
+	{ LED_MODE(0, 0, 1), {  -1,  -1,  -1,  -1, 0x0, 0x0, }, COMMON },
+	{ LED_MODE(0, 0, 1), {  -1, 0x0,  -1,  -1,  -1,  -1, }, L1V0_RX },
+};
+
+static int marvell_find_led_mode(struct phy_device *phydev, struct phy_led *led,
+				 struct led_netdev_data *trig)
+{
+	const struct marvell_leds_info *info = led->priv;
+	const struct marvell_led_mode_info *mode;
+	u32 key;
+	int i;
+
+	key = LED_MODE(trig->link, trig->tx, trig->rx);
+
+	for (i = 0; i < ARRAY_SIZE(marvell_led_mode_info); ++i) {
+		mode = &marvell_led_mode_info[i];
+
+		if (key != mode->key || mode->regval[led->addr] == -1 ||
+		    !(info->flags & mode->flags))
+			continue;
+
+		return mode->regval[led->addr];
+	}
+
+	dev_dbg(led->cdev.dev,
+		"cannot offload trigger configuration (%s, link=%i, tx=%i, rx=%i)\n",
+		netdev_name(trig->net_dev), trig->link, trig->tx, trig->rx);
+
+	return -1;
+}
+
+/* FIXME: Blinking rate is shared by all LEDs on a PHY. Should we check whether
+ * another LED is currently blinking with incompatible rate? It would be cleaner
+ * if we in this case failed to offload blinking this LED.
+ * But consider this situation:
+ *   1. user sets LED[1] to blink with period 500ms for some reason. This would
+ *      start blinking LED[1] with perion 670ms here
+ *   2. user sets netdev trigger to LED[0] to blink on activity, default there
+ *      is 100ms period, which would translate here to 84ms. This is
+ *      incompatible with the already blinking LED, so we fail to offload to HW,
+ *      and netdev trigger does software offloading instead.
+ *   3. user unsets blinking od LED[1], so now we theoretically can offload
+ *      netdev trigger to LED[0], but we don't know about it, and so it is left
+ *      in SW triggering until user writes the settings again
+ * This could be solved by the netdev trigger periodically trying to offload to
+ * HW if we reported that it is theoretically possible (by returning -EAGAIN
+ * instead of -EOPNOTSUPP, for example). Do we want to do this?
+ */
+static int marvell_led_trigger_offload(struct phy_device *phydev,
+				       struct phy_led *led, bool enable)
+{
+	struct led_netdev_data *trig = led_get_trigger_data(&led->cdev);
+	struct device *dev = led->cdev.dev;
+	unsigned long period;
+	int mode;
+	int ret;
+
+	if (!enable)
+		goto offload_disable;
+
+	/* Sanity checks first */
+	if (led->cdev.trigger != &netdev_led_trigger || !phydev->attached_dev ||
+	    phydev->attached_dev != trig->net_dev)
+		goto offload_disable;
+
+	mode = marvell_find_led_mode(phydev, led, trig);
+	if (mode < 0)
+		goto offload_disable;
+
+	/* TODO: this should only be checked if blinking is needed */
+	period = jiffies_to_msecs(atomic_read(&trig->interval)) * 2;
+	ret = marvell_leds_set_blink_rate(phydev, &period);
+	if (ret) {
+		dev_dbg(dev, "cannot offload trigger (unsupported blinking period)\n");
+		goto offload_disable;
+	}
+
+	ret = marvell_led_set_regval(phydev, led->addr, mode);
+	if (!ret) {
+		atomic_set(&trig->interval, msecs_to_jiffies(period / 2));
+		dev_dbg(led->cdev.dev, "netdev trigger offloaded\n");
+	}
+
+	return ret;
+
+offload_disable:
+	ret = marvell_led_set_regval(phydev, led->addr, MII_PHY_LED_CTRL_OFF);
+	if (ret)
+		return ret;
+
+	return -EOPNOTSUPP;
+}
+
+static int marvell_led_init(struct phy_device *phydev, struct phy_led *led,
+			    struct fwnode_handle *fwnode)
+{
+	struct marvell_priv *priv = phydev->priv;
+	const struct marvell_leds_info *info;
+	int ret, i;
+
+	if (led->addr == -1) {
+		phydev_err(phydev, "Missing 'reg' property for node %pfw\n",
+			   fwnode);
+		return -EINVAL;
+	}
+
+	for (i = 0; i < ARRAY_SIZE(marvell_leds_info); ++i) {
+		info = &marvell_leds_info[i];
+		if (MARVELL_PHY_FAMILY_ID(phydev->phy_id) == info->family)
+			break;
+	}
+
+	if (i == ARRAY_SIZE(marvell_leds_info))
+		return -EOPNOTSUPP;
+
+	if (led->addr >= info->nleds) {
+		phydev_err(phydev, "Invalid 'reg' property for node %pfw\n",
+			   fwnode);
+		return -EINVAL;
+	}
+
+	led->priv = (void *)info;
+
+	ret = marvell_led_set_polarity(phydev, led->addr, led->active_low,
+				       led->tristate);
+	if (ret < 0)
+		return ret;
+
+	/* ensure marvell_config_led below does not change settings we have set
+	 * for this LED
+	 */
+	if (led->addr < 3)
+		priv->legacy_led_config_mask &= ~(0xf << (led->addr * 4));
+
+	return 0;
+}
+
+#define MARVELL_LED_OPS							\
+		.led_init = marvell_led_init,				\
+		.led_brightness_set = marvell_led_brightness_set,	\
+		.led_blink_set = marvell_led_blink_set,			\
+		.led_trigger_offload = marvell_led_trigger_offload,
+
+#else /* !IS_ENABLED(CONFIG_LEDS_CLASS) */
+
+#define MARVELL_LED_OPS
+
+#endif /* IS_ENABLED(CONFIG_LEDS_CLASS) */
+
 static void marvell_config_led(struct phy_device *phydev)
 {
+	struct marvell_priv *priv = phydev->priv;
 	u16 def_config;
 	int err;
 
@@ -688,8 +1053,9 @@ static void marvell_config_led(struct phy_device *phydev)
 		return;
 	}
 
-	err = phy_write_paged(phydev, MII_MARVELL_LED_PAGE, MII_PHY_LED_CTRL,
-			      def_config);
+	def_config &= priv->legacy_led_config_mask;
+	err = phy_modify_paged(phydev, MII_MARVELL_LED_PAGE, MII_PHY_LED_CTRL,
+			       priv->legacy_led_config_mask, def_config);
 	if (err < 0)
 		phydev_warn(phydev, "Fail to config marvell phy LED.\n");
 }
@@ -2574,6 +2940,7 @@ static int marvell_probe(struct phy_device *phydev)
 	if (!priv)
 		return -ENOMEM;
 
+	priv->legacy_led_config_mask = 0xffff;
 	phydev->priv = priv;
 
 	return 0;
@@ -2650,6 +3017,7 @@ static struct phy_driver marvell_drivers[] = {
 		.get_stats = marvell_get_stats,
 		.get_tunable = m88e1011_get_tunable,
 		.set_tunable = m88e1011_set_tunable,
+		MARVELL_LED_OPS
 	},
 	{
 		.phy_id = MARVELL_PHY_ID_88E1111,
@@ -2689,6 +3057,7 @@ static struct phy_driver marvell_drivers[] = {
 		.get_sset_count = marvell_get_sset_count,
 		.get_strings = marvell_get_strings,
 		.get_stats = marvell_get_stats,
+		MARVELL_LED_OPS
 	},
 	{
 		.phy_id = MARVELL_PHY_ID_88E1121R,
@@ -2711,6 +3080,7 @@ static struct phy_driver marvell_drivers[] = {
 		.get_stats = marvell_get_stats,
 		.get_tunable = m88e1011_get_tunable,
 		.set_tunable = m88e1011_set_tunable,
+		MARVELL_LED_OPS
 	},
 	{
 		.phy_id = MARVELL_PHY_ID_88E1318S,
@@ -2733,6 +3103,7 @@ static struct phy_driver marvell_drivers[] = {
 		.get_sset_count = marvell_get_sset_count,
 		.get_strings = marvell_get_strings,
 		.get_stats = marvell_get_stats,
+		MARVELL_LED_OPS
 	},
 	{
 		.phy_id = MARVELL_PHY_ID_88E1145,
@@ -2772,6 +3143,7 @@ static struct phy_driver marvell_drivers[] = {
 		.get_sset_count = marvell_get_sset_count,
 		.get_strings = marvell_get_strings,
 		.get_stats = marvell_get_stats,
+		MARVELL_LED_OPS
 	},
 	{
 		.phy_id = MARVELL_PHY_ID_88E1240,
@@ -2790,6 +3162,7 @@ static struct phy_driver marvell_drivers[] = {
 		.get_sset_count = marvell_get_sset_count,
 		.get_strings = marvell_get_strings,
 		.get_stats = marvell_get_stats,
+		MARVELL_LED_OPS
 	},
 	{
 		.phy_id = MARVELL_PHY_ID_88E1116R,
@@ -2809,6 +3182,7 @@ static struct phy_driver marvell_drivers[] = {
 		.get_stats = marvell_get_stats,
 		.get_tunable = m88e1011_get_tunable,
 		.set_tunable = m88e1011_set_tunable,
+		MARVELL_LED_OPS
 	},
 	{
 		.phy_id = MARVELL_PHY_ID_88E1510,
@@ -2838,6 +3212,7 @@ static struct phy_driver marvell_drivers[] = {
 		.cable_test_start = marvell_vct7_cable_test_start,
 		.cable_test_tdr_start = marvell_vct5_cable_test_tdr_start,
 		.cable_test_get_status = marvell_vct7_cable_test_get_status,
+		MARVELL_LED_OPS
 	},
 	{
 		.phy_id = MARVELL_PHY_ID_88E1540,
@@ -2890,6 +3265,7 @@ static struct phy_driver marvell_drivers[] = {
 		.cable_test_start = marvell_vct7_cable_test_start,
 		.cable_test_tdr_start = marvell_vct5_cable_test_tdr_start,
 		.cable_test_get_status = marvell_vct7_cable_test_get_status,
+		MARVELL_LED_OPS
 	},
 	{
 		.phy_id = MARVELL_PHY_ID_88E3016,
@@ -2958,6 +3334,7 @@ static struct phy_driver marvell_drivers[] = {
 		.get_stats = marvell_get_stats,
 		.get_tunable = m88e1540_get_tunable,
 		.set_tunable = m88e1540_set_tunable,
+		MARVELL_LED_OPS
 	},
 	{
 		.phy_id = MARVELL_PHY_ID_88E1548P,
@@ -2980,6 +3357,7 @@ static struct phy_driver marvell_drivers[] = {
 		.get_stats = marvell_get_stats,
 		.get_tunable = m88e1540_get_tunable,
 		.set_tunable = m88e1540_set_tunable,
+		MARVELL_LED_OPS
 	},
 };
 
-- 
2.26.2

Re: [PATCH RFC leds + net-next 7/7] net: phy: marvell: support LEDs connected on Marvell PHYs

[Pavel Machek]

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


> +/* FIXME: Blinking rate is shared by all LEDs on a PHY. Should we check whether
> + * another LED is currently blinking with incompatible rate? It would be cleaner
> + * if we in this case failed to offload blinking this LED.
> + * But consider this situation:
> + *   1. user sets LED[1] to blink with period 500ms for some reason. This would
> + *      start blinking LED[1] with perion 670ms here

period.

> + *   2. user sets netdev trigger to LED[0] to blink on activity, default there
> + *      is 100ms period, which would translate here to 84ms. This is
> + *      incompatible with the already blinking LED, so we fail to offload to HW,
> + *      and netdev trigger does software offloading instead.
> + *   3. user unsets blinking od LED[1], so now we theoretically can offload
> + *      netdev trigger to LED[0], but we don't know about it, and so it is left
> + *      in SW triggering until user writes the settings again
> + * This could be solved by the netdev trigger periodically trying to offload to
> + * HW if we reported that it is theoretically possible (by returning -EAGAIN
> + * instead of -EOPNOTSUPP, for example). Do we want to do this?
> + */

I believe we should check & fallback to software if there's already
incompatible rate in use. No need to periodically re-try to activate
the offload.

Best regards,
								Pavel

-- 
http://www.livejournal.com/~pavelmachek

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: [PATCH RFC leds + net-next 7/7] net: phy: marvell: support LEDs connected on Marvell PHYs

[Pavel Machek]

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

Hi!

> Add support for controlling the LEDs connected to several families of
> Marvell PHYs via Linux LED API. These families currently are: 88E1112,
> 88E1116R, 88E1118, 88E1121R, 88E1149R, 88E1240, 88E1318S, 88E1340S,
> 88E1510, 88E1545 and 88E1548P.
> 
> This does not yet add support for compound LED modes. This could be
> achieved via the LED multicolor framework.
> 
> netdev trigger offloading is also implemented.
> 
> Signed-off-by: Marek Behún <kabel@kernel.org>


> +	val = 0;
> +	if (!active_low)
> +		val |= BIT(0);
> +	if (tristate)
> +		val |= BIT(1);

You are parsing these parameters in core... but they are not going to
be common for all phys, right? Should you parse them in the driver?
Should the parameters be same we have for gpio-leds?

> +static const struct marvell_led_mode_info marvell_led_mode_info[] = {
> +	{ LED_MODE(1, 0, 0), { 0x0,  -1, 0x0,  -1,  -1,  -1, }, COMMON },
> +	{ LED_MODE(1, 1, 1), { 0x1, 0x1, 0x1, 0x1, 0x1, 0x1, }, COMMON },
> +	{ LED_MODE(0, 1, 1), { 0x4, 0x4, 0x4, 0x4, 0x4, 0x4, }, COMMON },
> +	{ LED_MODE(1, 0, 1), {  -1, 0x2,  -1, 0x2, 0x2, 0x2, }, COMMON },
> +	{ LED_MODE(0, 1, 0), { 0x5,  -1, 0x5,  -1, 0x5, 0x5, }, COMMON },
> +	{ LED_MODE(0, 1, 0), {  -1,  -1,  -1, 0x5,  -1,  -1, }, L3V5_TX },
> +	{ LED_MODE(0, 0, 1), {  -1,  -1,  -1,  -1, 0x0, 0x0, }, COMMON },
> +	{ LED_MODE(0, 0, 1), {  -1, 0x0,  -1,  -1,  -1,  -1, }, L1V0_RX },
> +};
> +
> +static int marvell_find_led_mode(struct phy_device *phydev, struct phy_led *led,
> +				 struct led_netdev_data *trig)
> +{
> +	const struct marvell_leds_info *info = led->priv;
> +	const struct marvell_led_mode_info *mode;
> +	u32 key;
> +	int i;
> +
> +	key = LED_MODE(trig->link, trig->tx, trig->rx);
> +
> +	for (i = 0; i < ARRAY_SIZE(marvell_led_mode_info); ++i) {
> +		mode = &marvell_led_mode_info[i];
> +
> +		if (key != mode->key || mode->regval[led->addr] == -1 ||
> +		    !(info->flags & mode->flags))
> +			continue;
> +
> +		return mode->regval[led->addr];
> +	}
> +
> +	dev_dbg(led->cdev.dev,
> +		"cannot offload trigger configuration (%s, link=%i, tx=%i, rx=%i)\n",
> +		netdev_name(trig->net_dev), trig->link, trig->tx, trig->rx);
> +
> +	return -1;
> +}

I'm wondering if it makes sense to offload changes on link
state. Those should be fairly infrequent and you are not saving
significant power there... and seems to complicate things.

The "shared frequency blinking" looks quite crazy.

Rest looks reasonably sane.

Best regards,
								Pavel

-- 
http://www.livejournal.com/~pavelmachek

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]