Add DT support for netxbig LEDs

This patch sets the led_classdev max_brightness to the maximum level value supported by hardware. This allows to get rid of the brightness conversion operation (from software [0:LED_FULL] to hardware ranges) in brightness_set().

Signed-off-by Simon Guinot <[email protected]>

---
  drivers/leds/leds-netxbig.c | 12 ++++--------
  1 file changed, 4 insertions(+), 8 deletions(-)

drivers/leds/leds-netxbig.c +4 -8

index b166fd9f4186..4b88b93244be 100644
@@ -116,7 +116,6 @@ struct netxbig_led_data {
  	int			mode_addr;
  	int			*mode_val;
  	int			bright_addr;
-	int			bright_max;
  	struct			netxbig_led_timer *timer;
  	int			num_timer;
  	enum netxbig_led_mode	mode;

@@ -178,7 +177,7 @@ static void netxbig_led_set(struct led_classdev *led_cdev,
  	struct netxbig_led_data *led_dat =
  		container_of(led_cdev, struct netxbig_led_data, cdev);
  	enum netxbig_led_mode mode;
-	int mode_val, bright_val;
+	int mode_val;
  	int set_brightness = 1;
  	unsigned long flags;
 

@@ -204,12 +203,9 @@ static void netxbig_led_set(struct led_classdev *led_cdev,
  	 * SATA LEDs. So, change the brightness setting for a single
  	 * SATA LED will affect all the others.
  	 */
-	if (set_brightness) {
-		bright_val = DIV_ROUND_UP(value * led_dat->bright_max,
-					  LED_FULL);
+	if (set_brightness)
  		gpio_ext_set_value(led_dat->gpio_ext,
-				   led_dat->bright_addr, bright_val);
-	}
+				   led_dat->bright_addr, value);
 
  	spin_unlock_irqrestore(&led_dat->lock, flags);
  }

@@ -306,11 +302,11 @@ static int create_netxbig_led(struct platform_device *pdev,
  	 */
  	led_dat->sata = 0;
  	led_dat->cdev.brightness = LED_OFF;
+	led_dat->cdev.max_brightness = template->bright_max;
  	led_dat->cdev.flags |= LED_CORE_SUSPENDRESUME;
  	led_dat->mode_addr = template->mode_addr;
  	led_dat->mode_val = template->mode_val;
  	led_dat->bright_addr = template->bright_addr;
-	led_dat->bright_max = template->bright_max;
  	led_dat->timer = pdata->timer;
  	led_dat->num_timer = pdata->num_timer;
  	/*

Hi Simon,

Does your device support reading the brightness currently set?

If so, it would be good to implement brightness_get op, because AFAIR you mentioned that the firmware you are working with sets always maximum brightness value. Having the op implemented would allow to find this out.

Quoted 60 lines

This patch sets the led_classdev max_brightness to the maximum level value supported by hardware. This allows to get rid of the brightness conversion operation (from software [0:LED_FULL] to hardware ranges) in brightness_set().

Signed-off-by Simon Guinot <[email protected]>

---
 drivers/leds/leds-netxbig.c | 12 ++++--------
 1 file changed, 4 insertions(+), 8 deletions(-)

drivers/leds/leds-netxbig.c +4 -8

index b166fd9f4186..4b88b93244be 100644
@@ -116,7 +116,6 @@ struct netxbig_led_data {
 	int			mode_addr;
 	int			*mode_val;
 	int			bright_addr;
-	int			bright_max;
 	struct			netxbig_led_timer *timer;
 	int			num_timer;
 	enum netxbig_led_mode	mode;

@@ -178,7 +177,7 @@ static void netxbig_led_set(struct led_classdev *led_cdev,
 	struct netxbig_led_data *led_dat =
 		container_of(led_cdev, struct netxbig_led_data, cdev);
 	enum netxbig_led_mode mode;
-	int mode_val, bright_val;
+	int mode_val;
 	int set_brightness = 1;
 	unsigned long flags;
 

@@ -204,12 +203,9 @@ static void netxbig_led_set(struct led_classdev *led_cdev,
 	 * SATA LEDs. So, change the brightness setting for a single
 	 * SATA LED will affect all the others.
 	 */
-	if (set_brightness) {
-		bright_val = DIV_ROUND_UP(value * led_dat->bright_max,
-					  LED_FULL);
+	if (set_brightness)
 		gpio_ext_set_value(led_dat->gpio_ext,
-				   led_dat->bright_addr, bright_val);
-	}
+				   led_dat->bright_addr, value);
 
 	spin_unlock_irqrestore(&led_dat->lock, flags);
 }

@@ -306,11 +302,11 @@ static int create_netxbig_led(struct platform_device *pdev,
 	 */
 	led_dat->sata = 0;
 	led_dat->cdev.brightness = LED_OFF;
+	led_dat->cdev.max_brightness = template->bright_max;
 	led_dat->cdev.flags |= LED_CORE_SUSPENDRESUME;
 	led_dat->mode_addr = template->mode_addr;
 	led_dat->mode_val = template->mode_val;
 	led_dat->bright_addr = template->bright_addr;
-	led_dat->bright_max = template->bright_max;
 	led_dat->timer = pdata->timer;
 	led_dat->num_timer = pdata->num_timer;
 	/*

Signature · 6 lines

Best Regards,
Jacek Anaszewski
--
To unsubscribe from this list: send the line "unsubscribe linux-leds" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Quoted 1 line · Jacek Anaszewski

Hi Simon,

Hi Jacek,

Quoted 2 lines

Does your device support reading the brightness currently set?

No it don't.

Quoted 4 lines

If so, it would be good to implement brightness_get op, because AFAIR you mentioned that the firmware you are working with sets always maximum brightness value. Having the op implemented would allow to find this out.

I don't understand how this can help. I mean, the only issue is that at startup the initial LED state is unknown. And the software brightness value could be false. But once the LED is configured, the brightness values for software and hardware are synchronized. The brightness value is stored/cached in led_classdev and it can be retrieved by the user via sysfs...

For my own knowledge, is there some interest in having brightness_get(), aside of guessing the LED initial state ?

What does the embedded firmware is writing 255 or 0 into the brightness sysfs attribute. The max_brightness value is ignored. After this patch, writing 255 and 0 still allows to configure the LED in the same way:
maximum brightness or off. Thus, I believe there is no compatibility issue.

But of course, I will update the firmware as well :)

Quoted 22 lines · Simon Guinot

Quoted 1 line · Jacek Anaszewski

Hi Simon,

Hi Jacek,

Quoted 2 lines

Does your device support reading the brightness currently set?

No it don't.

Quoted 4 lines

If so, it would be good to implement brightness_get op, because AFAIR you mentioned that the firmware you are working with sets always maximum brightness value. Having the op implemented would allow to find this out.

I don't understand how this can help. I mean, the only issue is that at startup the initial LED state is unknown. And the software brightness value could be false. But once the LED is configured, the brightness values for software and hardware are synchronized. The brightness value is stored/cached in led_classdev and it can be retrieved by the user via sysfs...

Quoted 2 lines

For my own knowledge, is there some interest in having brightness_get(), aside of guessing the LED initial state ?

Some LED controllers can adjust brightness in case battery voltage level falls below some threshold.

Quoted 6 lines

What does the embedded firmware is writing 255 or 0 into the brightness sysfs attribute. The max_brightness value is ignored. After this patch, writing 255 and 0 still allows to configure the LED in the same way:
maximum brightness or off. Thus, I believe there is no compatibility issue.

LED core always assures that brightness value passed to brightness_set op does not exceed max_brightness value. So, now after executing "echo 255 > brightness", LED core will adjust it to max_brightness (e.g. 7) before passing to brightness_set.

In the message [1], you mentioned that "LEDs are only enabled at their maximum level", so IIUC following is possible:

#echo 3 > "brightness"

firmware sets brightness to max_brightness from DT (e.g. 7), but

#cat brightness
#3

Is it true?

Quoted 23 lines · Jacek Anaszewski

Quoted 22 lines · Simon Guinot

Quoted 1 line · Jacek Anaszewski

Hi Simon,

Hi Jacek,

Quoted 2 lines

Does your device support reading the brightness currently set?

No it don't.

Quoted 4 lines

If so, it would be good to implement brightness_get op, because AFAIR you mentioned that the firmware you are working with sets always maximum brightness value. Having the op implemented would allow to find this out.

I don't understand how this can help. I mean, the only issue is that at startup the initial LED state is unknown. And the software brightness value could be false. But once the LED is configured, the brightness values for software and hardware are synchronized. The brightness value is stored/cached in led_classdev and it can be retrieved by the user via sysfs...

Quoted 5 lines

Quoted 2 lines

For my own knowledge, is there some interest in having brightness_get(), aside of guessing the LED initial state ?

Some LED controllers can adjust brightness in case battery voltage level falls below some threshold.

OK, thanks for the explanation.

Quoted 22 lines

Quoted 6 lines

What does the embedded firmware is writing 255 or 0 into the brightness sysfs attribute. The max_brightness value is ignored. After this patch, writing 255 and 0 still allows to configure the LED in the same way:
maximum brightness or off. Thus, I believe there is no compatibility issue.

LED core always assures that brightness value passed to brightness_set op does not exceed max_brightness value. So, now after executing "echo 255 > brightness", LED core will adjust it to max_brightness (e.g. 7) before passing to brightness_set.

In the message [1], you mentioned that "LEDs are only enabled at their maximum level", so IIUC following is possible:

#echo 3 > "brightness"

firmware sets brightness to max_brightness from DT (e.g. 7), but

#cat brightness
#3

Quoted 1 line

Is it true?

Oh no sorry, it is a misunderstanding. By "LEDs are only enabled at their maximum level", I was meaning "LEDs are only enabled at their maximum level by the LaCie stock firmware". The firmware don't make use of the different hardware brightness levels available. But the feature works perfectly. If you write 3 into sysfs "brightness", then you get the third brightness level.

OK, I understand you remark now. Sorry for not being very clear in a first place.

Quoted 54 lines · Simon Guinot

Quoted 23 lines · Jacek Anaszewski

Quoted 22 lines · Simon Guinot

Quoted 1 line · Jacek Anaszewski

Hi Simon,

Hi Jacek,

Quoted 2 lines

Does your device support reading the brightness currently set?

No it don't.

Quoted 4 lines

If so, it would be good to implement brightness_get op, because AFAIR you mentioned that the firmware you are working with sets always maximum brightness value. Having the op implemented would allow to find this out.

I don't understand how this can help. I mean, the only issue is that at startup the initial LED state is unknown. And the software brightness value could be false. But once the LED is configured, the brightness values for software and hardware are synchronized. The brightness value is stored/cached in led_classdev and it can be retrieved by the user via sysfs...

Quoted 5 lines

Quoted 2 lines

For my own knowledge, is there some interest in having brightness_get(), aside of guessing the LED initial state ?

Some LED controllers can adjust brightness in case battery voltage level falls below some threshold.

OK, thanks for the explanation.

Quoted 22 lines

Quoted 6 lines

What does the embedded firmware is writing 255 or 0 into the brightness sysfs attribute. The max_brightness value is ignored. After this patch, writing 255 and 0 still allows to configure the LED in the same way:
maximum brightness or off. Thus, I believe there is no compatibility issue.

LED core always assures that brightness value passed to brightness_set op does not exceed max_brightness value. So, now after executing "echo 255 > brightness", LED core will adjust it to max_brightness (e.g. 7) before passing to brightness_set.

In the message [1], you mentioned that "LEDs are only enabled at their maximum level", so IIUC following is possible:

#echo 3 > "brightness"

firmware sets brightness to max_brightness from DT (e.g. 7), but

#cat brightness
#3

Quoted 8 lines

Quoted 1 line

Is it true?

Oh no sorry, it is a misunderstanding. By "LEDs are only enabled at their maximum level", I was meaning "LEDs are only enabled at their maximum level by the LaCie stock firmware". The firmware don't make use of the different hardware brightness levels available. But the feature works perfectly. If you write 3 into sysfs "brightness", then you get the third brightness level.

I thought that driver talks to firmware, which controls the LEDs. From your explanation I infer that this driver replaces LaCie stock firmware, am I right? There must be however some circuit that controls LED brightness then.

Quoted 64 lines · Jacek Anaszewski

Quoted 54 lines · Simon Guinot

Quoted 23 lines · Jacek Anaszewski

Quoted 22 lines · Simon Guinot

Quoted 1 line · Jacek Anaszewski

Hi Simon,

Hi Jacek,

Quoted 2 lines

Does your device support reading the brightness currently set?

No it don't.

Quoted 4 lines

If so, it would be good to implement brightness_get op, because AFAIR you mentioned that the firmware you are working with sets always maximum brightness value. Having the op implemented would allow to find this out.

I don't understand how this can help. I mean, the only issue is that at startup the initial LED state is unknown. And the software brightness value could be false. But once the LED is configured, the brightness values for software and hardware are synchronized. The brightness value is stored/cached in led_classdev and it can be retrieved by the user via sysfs...

Quoted 5 lines

Quoted 2 lines

For my own knowledge, is there some interest in having brightness_get(), aside of guessing the LED initial state ?

Some LED controllers can adjust brightness in case battery voltage level falls below some threshold.

OK, thanks for the explanation.

Quoted 22 lines

Quoted 6 lines

What does the embedded firmware is writing 255 or 0 into the brightness sysfs attribute. The max_brightness value is ignored. After this patch, writing 255 and 0 still allows to configure the LED in the same way:
maximum brightness or off. Thus, I believe there is no compatibility issue.

LED core always assures that brightness value passed to brightness_set op does not exceed max_brightness value. So, now after executing "echo 255 > brightness", LED core will adjust it to max_brightness (e.g. 7) before passing to brightness_set.

In the message [1], you mentioned that "LEDs are only enabled at their maximum level", so IIUC following is possible:

#echo 3 > "brightness"

firmware sets brightness to max_brightness from DT (e.g. 7), but

#cat brightness
#3

Quoted 8 lines

Quoted 1 line

Is it true?

Oh no sorry, it is a misunderstanding. By "LEDs are only enabled at their maximum level", I was meaning "LEDs are only enabled at their maximum level by the LaCie stock firmware". The firmware don't make use of the different hardware brightness levels available. But the feature works perfectly. If you write 3 into sysfs "brightness", then you get the third brightness level.

Quoted 4 lines

I thought that driver talks to firmware, which controls the LEDs.
From your explanation I infer that this driver replaces LaCie stock
firmware, am I right? There must be however some circuit that controls LED brightness then.

OK, I think I may have managed to confuse you completely.

The LEDs are controlled by a CPLD. The leds-netxbig driver talks to the CPLD via the gpio-ext "kind of" bus:

leds-netxbig -> gpio-ext bus -> CPLD -> LEDs

"LaCie stock firmware" don't refer to the firmware embedded in the CPLD but to the stock LaCie Linux system running *on* the board.

"LEDs are only enabled at their maximum level by the LaCie stock firmware" means: "In the Linux LaCie system running on the board, userland only configures the LED brightness to 0 or 255".

It don't mean that the CPLD does some weird stuff implying that the feature is somewhat broken. No, the feature works. Simply userland (in the stock LaCie system aka firmware) don't use it.

Remember that your original question was:

"Doesn't specification of your device say what current value given
brightness level reflects?"

Let me rephrase my answer without using "LaCie stock firmware":

No this information is not available in the board specification. It is probably because this feature is not used by the LaCie Linux userland. The LED brightness is configured to off or full. Other levels are not used. That's probably why no one took care of measuring the LED current consumption.

Let me know if it is still unclear for you.

Hello,

This patch series adds DT support for the LEDs found on the Kirkwood-based LaCie boards 2Big and 5Big Network v2.

Changes for v2:
- Check timer mode value retrieved from DT.
- In netxbig_leds_get_of_pdata, don't use unsigned long variables to get timer delay values from DT with function of_property_read_u32_index. Instead, use a temporary u32 variable. This allows to silence a static checker warning.
- Make timer property optional in the binding documentation. It is now aligned with the driver code.

Changes for v3:
- Fix pointer usage with the temporary u32 variable while calling of_property_read_u32_index.

Changes for v4:
- In DT binding document netxbig-gpio-ext.txt, detail byte order for registers and latch mechanism for "enable-gpio".
- In leds-netxbig.c, add some error messages.
- In leds-netxbig.c, fix some "sizeof" style issues.
- In leds-netxbig.c, in netxbig_leds_get_of_pdata(), move the of_property_read_string() calls after the error-prone checks.
- Add some Acked-by.

Changes for v5:
- Rename DT property "bright-max" into the more common "max-brightness".
- Make use of the "max-brightness" DT property. Instead of counting the data pins of the GPIO extension bus, use "max-brightness" to get the maximum brightness level.
- Add a patch to convert the leds-netxbig driver to the devm_ functions.

Changes for v6:
- Add a patch to set led_classdev max_brightness in the leds-netxbig driver.
- Decrement the current child node refcount with of_node_put() when an error happens while iterating with for_each_child_of_node().

Thanks,

Simon

Simon Guinot (5):
leds: netxbig: add device tree binding
ARM: Kirkwood: add LED DT entries for netxbig boards
ARM: mvebu: remove static LED setup for netxbig boards
leds: netxbig: convert to use the devm_ functions
leds: netxbig: set led_classdev max_brightness

  .../devicetree/bindings/gpio/netxbig-gpio-ext.txt  |  22 ++
  .../devicetree/bindings/leds/leds-netxbig.txt      |  92 ++++++
  arch/arm/boot/dts/kirkwood-net5big.dts             |  60 ++++
  arch/arm/boot/dts/kirkwood-netxbig.dtsi            |  80 +++++
  arch/arm/mach-mvebu/Kconfig                        |   7 -
  arch/arm/mach-mvebu/Makefile                       |   1 -
  arch/arm/mach-mvebu/board.h                        |  21 --
  arch/arm/mach-mvebu/kirkwood.c                     |   4 -
  arch/arm/mach-mvebu/netxbig.c                      | 191 ------------
  drivers/leds/leds-netxbig.c                        | 336 ++++++++++++++++-----
  include/dt-bindings/leds/leds-netxbig.h            |  18 ++
  .../linux/platform_data/leds-kirkwood-netxbig.h    |   1 +
  12 files changed, 531 insertions(+), 302 deletions(-)

create mode 100644 Documentation/devicetree/bindings/gpio/netxbig-gpio-ext.txt create mode 100644 Documentation/devicetree/bindings/leds/leds-netxbig.txt delete mode 100644 arch/arm/mach-mvebu/board.h
delete mode 100644 arch/arm/mach-mvebu/netxbig.c
create mode 100644 include/dt-bindings/leds/leds-netxbig.h

Quoted 1 line · Simon Guinot

Hello,

Quoted 21 lines

Simon Guinot (5):
leds: netxbig: add device tree binding
ARM: Kirkwood: add LED DT entries for netxbig boards
ARM: mvebu: remove static LED setup for netxbig boards
leds: netxbig: convert to use the devm_ functions
leds: netxbig: set led_classdev max_brightness

  .../devicetree/bindings/gpio/netxbig-gpio-ext.txt  |  22 ++
  .../devicetree/bindings/leds/leds-netxbig.txt      |  92 ++++++
  arch/arm/boot/dts/kirkwood-net5big.dts             |  60 ++++
  arch/arm/boot/dts/kirkwood-netxbig.dtsi            |  80 +++++
  arch/arm/mach-mvebu/Kconfig                        |   7 -
  arch/arm/mach-mvebu/Makefile                       |   1 -
  arch/arm/mach-mvebu/board.h                        |  21 --
  arch/arm/mach-mvebu/kirkwood.c                     |   4 -
  arch/arm/mach-mvebu/netxbig.c                      | 191 ------------
  drivers/leds/leds-netxbig.c                        | 336 ++++++++++++++++-----
  include/dt-bindings/leds/leds-netxbig.h            |  18 ++
  .../linux/platform_data/leds-kirkwood-netxbig.h    |   1 +
  12 files changed, 531 insertions(+), 302 deletions(-)

Quoted 6 lines

create mode 100644 Documentation/devicetree/bindings/gpio/netxbig-gpio-ext.txt create mode 100644 Documentation/devicetree/bindings/leds/leds-netxbig.txt delete mode 100644 arch/arm/mach-mvebu/board.h
delete mode 100644 arch/arm/mach-mvebu/netxbig.c
create mode 100644 include/dt-bindings/leds/leds-netxbig.h

Since no comment regarding DT bindings has appeared for a long time, I decided to merge the patch set without DT maintainer's ack, thanks.

Signature · 3 lines

-- 
Best Regards,
Jacek Anaszewski

Hi Jacek,

Quoted 2 lines · Jacek Anaszewski

Quoted 1 line · Simon Guinot

Hello,

[..]

Quoted 28 lines

Quoted 27 lines

Simon Guinot (5):
leds: netxbig: add device tree binding
ARM: Kirkwood: add LED DT entries for netxbig boards
ARM: mvebu: remove static LED setup for netxbig boards
leds: netxbig: convert to use the devm_ functions
leds: netxbig: set led_classdev max_brightness

   .../devicetree/bindings/gpio/netxbig-gpio-ext.txt  |  22 ++
   .../devicetree/bindings/leds/leds-netxbig.txt      |  92 ++++++
   arch/arm/boot/dts/kirkwood-net5big.dts             |  60 ++++
   arch/arm/boot/dts/kirkwood-netxbig.dtsi            |  80 +++++
   arch/arm/mach-mvebu/Kconfig                        |   7 -
   arch/arm/mach-mvebu/Makefile                       |   1 -
   arch/arm/mach-mvebu/board.h                        |  21 --
   arch/arm/mach-mvebu/kirkwood.c                     |   4 -
   arch/arm/mach-mvebu/netxbig.c                      | 191 ------------
   drivers/leds/leds-netxbig.c                        | 336 ++++++++++++++++-----
   include/dt-bindings/leds/leds-netxbig.h            |  18 ++
   .../linux/platform_data/leds-kirkwood-netxbig.h    |   1 +
   12 files changed, 531 insertions(+), 302 deletions(-)

create mode 100644 Documentation/devicetree/bindings/gpio/netxbig-gpio-ext.txt create mode 100644 Documentation/devicetree/bindings/leds/leds-netxbig.txt delete mode 100644 arch/arm/mach-mvebu/board.h
delete mode 100644 arch/arm/mach-mvebu/netxbig.c
create mode 100644 include/dt-bindings/leds/leds-netxbig.h

Quoted 2 lines

Since no comment regarding DT bindings has appeared for a long time, I decided to merge the patch set without DT maintainer's ack, thanks.

it is a good news that this series will be finally merged. However did you also take the patches "ARM: Kirkwood: add LED DT entries for netxbig boards" and "ARM: mvebu: remove static LED setup for netxbig boards" ?