Skip to contentSkip to conversation

cap11xx: add LED support

PATCH Superseded Matt Ranostay 5 participants 16 messages Jun 18, 2015 – Jun 24, 2015 2 messages not in the archive

Subsystems (4)
Device tree bindings HID Input drivers LED

v5 of 6 Superseded Latest is v6 →

lore
Share & tools
lore

Matt Ranostay

author
#0 lore raw

v5 cover cap11xx: add LED support

Changes from v4:
* Update docs to show proper address and size properties for DT bindings
* Make all LEDs blank on startup, and remove racey schedule_work calls that did this in last revisions
* Disable cap11xx_set_sleep() if any LEDs registered
* TODO: Make cap11xx_set_sleep() able to detect if LEDs are in use and calculate which power state it should be in.

Matt Ranostay (2):
dt: add cap11xx LED documentation
cap11xx: add LED support

 .../devicetree/bindings/input/cap11xx.txt          |  25 ++++
 drivers/input/keyboard/cap11xx.c                   | 140 ++++++++++++++++++++-
 2 files changed, 162 insertions(+), 3 deletions(-)
Signature · 1 line
1.9.1
author
#1 lore raw

PATCH v5 1/2 dt: add cap11xx LED documentation

Signed-off-by Matt Ranostay <[email protected]>
---
 .../devicetree/bindings/input/cap11xx.txt          | 25 ++++++++++++++++++++++
 1 file changed, 25 insertions(+)
Documentation/devicetree/bindings/input/cap11xx.txt +25 -0
index 7d0a300..09cdc43 100644
@@ -38,6 +38,11 @@ Optional properties:
 				defaults. The array must have exactly six
 				entries.
 
+	linux,led-brightness:	Defines the ON brightness when the optional LED
+				functionality is used. Valid values are 1-15.
+				By default a value of 15 is set.
+
+
 Example:
 
 i2c_controller {

@@ -55,5 +60,25 @@ i2c_controller {
 				 <105>,		/* KEY_LEFT */
 				 <109>,		/* KEY_PAGEDOWN */
 				 <104>;		/* KEY_PAGEUP */
+
+		linux,led-brightness = <15>;
+		#address-cells = <1>;
+		#size-cells = <0>;
+
+		usr@0 {
+			label = "cap11xx:green:usr0";
+			reg = <0>;
+		};
+
+		usr@1 {
+			label = "cap11xx:green:usr1";
+			reg = <1>;
+		};
+
+		alive@2 {
+			label = "cap11xx:green:alive";
+			reg = <2>;
+			linux,default_trigger = "heartbeat";
+		};
 	};
 }
Signature · 1 line
1.9.1
Acked-by on v5 1/2
#2 lore raw

Re: [PATCH v5 1/2] dt: add cap11xx LED documentation

Hi Matt,

Quoted 38 lines · Matt Ranostay
Signed-off-by Matt Ranostay <[email protected]>
---
  .../devicetree/bindings/input/cap11xx.txt          | 25 ++++++++++++++++++++++
  1 file changed, 25 insertions(+)
Documentation/devicetree/bindings/input/cap11xx.txt +17 -0
index 7d0a300..09cdc43 100644
@@ -38,6 +38,11 @@ Optional properties:
  				defaults. The array must have exactly six
  				entries.
 
+	linux,led-brightness:	Defines the ON brightness when the optional LED
+				functionality is used. Valid values are 1-15.
+				By default a value of 15 is set.
+
+
  Example:
 
  i2c_controller {

@@ -55,5 +60,25 @@ i2c_controller {
  				 <105>,		/* KEY_LEFT */
  				 <109>,		/* KEY_PAGEDOWN */
  				 <104>;		/* KEY_PAGEUP */
+
+		linux,led-brightness = <15>;
+		#address-cells = <1>;
+		#size-cells = <0>;
+
+		usr@0 {
+			label = "cap11xx:green:usr0";
+			reg = <0>;
+		};
+
+		usr@1 {
+			label = "cap11xx:green:usr1";

Documentation/devicetree/bindings/input/cap11xx.txt i2c_controller { · line 84

Quoted 10 lines
Documentation/devicetree/bindings/input/cap11xx.txt +8 -0
+			reg = <1>;
+		};
+
+		alive@2 {
+			label = "cap11xx:green:alive";
+			reg = <2>;
+			linux,default_trigger = "heartbeat";
+		};
  	};
  }
Acked-by Jacek Anaszewski <[email protected]>
Signature · 2 lines
Best Regards,
Jacek Anaszewski
maintainer
#3 lore raw

Re: [PATCH v5 1/2] dt: add cap11xx LED documentation

In reply to Matt Ranostay
Quoted 10 lines · Matt Ranostay
Signed-off-by Matt Ranostay <[email protected]>
---
 .../devicetree/bindings/input/cap11xx.txt          | 25 ++++++++++++++++++++++
 1 file changed, 25 insertions(+)
Documentation/devicetree/bindings/input/cap11xx.txt +0 -0
index 7d0a300..09cdc43 100644

Documentation/devicetree/bindings/input/cap11xx.txt Optional properties: · line 43

Quoted 6 lines
Documentation/devicetree/bindings/input/cap11xx.txt +3 -0
@@ -38,6 +38,11 @@ Optional properties:
 				defaults. The array must have exactly six
 				entries.
 
+	linux,led-brightness:	Defines the ON brightness when the optional LED
+				functionality is used. Valid values are 1-15.
+				By default a value of 15 is set.

Please mention the device does not allow controlling brightness of leds individually and that is why this property is at device level, not individual led level.

Also, why does it have "linux" prefix? It does not appear to control any linux-specific functionality.

Quoted 34 lines
Documentation/devicetree/bindings/input/cap11xx.txt +22 -0
+
+
 Example:
 
 i2c_controller {

@@ -55,5 +60,25 @@ i2c_controller {
 				 <105>,		/* KEY_LEFT */
 				 <109>,		/* KEY_PAGEDOWN */
 				 <104>;		/* KEY_PAGEUP */
+
+		linux,led-brightness = <15>;
+		#address-cells = <1>;
+		#size-cells = <0>;
+
+		usr@0 {
+			label = "cap11xx:green:usr0";
+			reg = <0>;
+		};
+
+		usr@1 {
+			label = "cap11xx:green:usr1";
+			reg = <1>;
+		};
+
+		alive@2 {
+			label = "cap11xx:green:alive";
+			reg = <2>;
+			linux,default_trigger = "heartbeat";
+		};
 	};
 }
Signature · 1 line
1.9.1

Thanks.

Signature · 3 lines
Dmitry
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
#4 lore raw

Re: [PATCH v5 1/2] dt: add cap11xx LED documentation

Quoted 14 lines · Dmitry Torokhov
Quoted 13 lines · Matt Ranostay
Signed-off-by Matt Ranostay <[email protected]>
---
  .../devicetree/bindings/input/cap11xx.txt          | 25 ++++++++++++++++++++++
  1 file changed, 25 insertions(+)
Documentation/devicetree/bindings/input/cap11xx.txt +0 -0
index 7d0a300..09cdc43 100644
@@ -38,6 +38,11 @@ Optional properties:
  				defaults. The array must have exactly six
  				entries.
Quoted 7 lines

Documentation/devicetree/bindings/input/cap11xx.txt Optional properties: · line 43

Quoted 3 lines
Documentation/devicetree/bindings/input/cap11xx.txt +3 -0
+	linux,led-brightness:	Defines the ON brightness when the optional LED
+				functionality is used. Valid values are 1-15.
+				By default a value of 15 is set.

Please mention the device does not allow controlling brightness of leds individually and that is why this property is at device level, not individual led level.

I've just noticed that we have drivers/leds/leds-netxbig.c driver, which also doesn't allow controlling the LEDs on extension board individually, but it still does allow changing their brightness. I am leaning towards allowing this also for this driver and adding similar comment in the source code like at the line 218 of the aforementioned driver. As a result this property wouldn't be required.

Quoted 2 lines

Also, why does it have "linux" prefix? It does not appear to control any linux-specific functionality.

Signature · 2 lines
Best Regards,
Jacek Anaszewski

A message that is not in the archive (sent off-list or not yet received): [email protected]

#6 lore raw

Re: [PATCH v5 1/2] dt: add cap11xx LED documentation

Quoted 31 lines · Jacek Anaszewski
Quoted 18 lines · Dmitry Torokhov
Quoted 16 lines · Matt Ranostay
Signed-off-by Matt Ranostay <[email protected]>
---
  .../devicetree/bindings/input/cap11xx.txt          | 25
++++++++++++++++++++++
  1 file changed, 25 insertions(+)
Documentation/devicetree/bindings/input/cap11xx.txt +0 -0
index 7d0a300..09cdc43 100644
@@ -38,6 +38,11 @@ Optional properties:
                                defaults. The array must have exactly six
                                entries.
Quoted 10 lines
Quoted 5 lines
Documentation/devicetree/bindings/input/cap11xx.txt +1 -0
+       linux,led-brightness:   Defines the ON brightness when the

optional LED
+ functionality is used. Valid values are
1-15.
+ By default a value of 15 is set.

Please mention the device does not allow controlling brightness of leds individually and that is why this property is at device level, not individual led level.

Quoted 7 lines

I've just noticed that we have drivers/leds/leds-netxbig.c driver, which also doesn't allow controlling the LEDs on extension board individually, but it still does allow changing their brightness. I am leaning towards allowing this also for this driver and adding similar comment in the source code like at the line 218 of the aforementioned driver. As a result this property wouldn't be required.

Ok that should be pretty simple to do. But seems kind weird to have each led channel to be changing the brightness of all. Wouldn't the brightness sysfs entries of the other led channels be showing incorrect values?

Quoted 8 lines
Quoted 2 lines

Also, why does it have "linux" prefix? It does not appear to control any linux-specific functionality.

--
Best Regards,
Jacek Anaszewski

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

maintainer
#7 lore raw

Re: [PATCH v5 1/2] dt: add cap11xx LED documentation

Quoted 41 lines · Matt Ranostay
Quoted 31 lines · Jacek Anaszewski
Quoted 18 lines · Dmitry Torokhov
Quoted 16 lines · Matt Ranostay
Signed-off-by Matt Ranostay <[email protected]>
---
  .../devicetree/bindings/input/cap11xx.txt          | 25
++++++++++++++++++++++
  1 file changed, 25 insertions(+)
Documentation/devicetree/bindings/input/cap11xx.txt +0 -0
index 7d0a300..09cdc43 100644
@@ -38,6 +38,11 @@ Optional properties:
                                defaults. The array must have exactly six
                                entries.
Quoted 10 lines
Quoted 5 lines
Documentation/devicetree/bindings/input/cap11xx.txt +1 -0
+       linux,led-brightness:   Defines the ON brightness when the

optional LED
+ functionality is used. Valid values are
1-15.
+ By default a value of 15 is set.

Please mention the device does not allow controlling brightness of leds individually and that is why this property is at device level, not individual led level.

Quoted 7 lines

I've just noticed that we have drivers/leds/leds-netxbig.c driver, which also doesn't allow controlling the LEDs on extension board individually, but it still does allow changing their brightness. I am leaning towards allowing this also for this driver and adding similar comment in the source code like at the line 218 of the aforementioned driver. As a result this property wouldn't be required.

Quoted 4 lines

Ok that should be pretty simple to do. But seems kind weird to have each led channel to be changing the brightness of all. Wouldn't the brightness sysfs entries of the other led channels be showing incorrect values?

I agree, this is kind of weird. Maybe we should have a device-specific attribute (on the platform device level) that allows controlling overall brightness, but I think LEDs should be just on/off with max brightness of 1. Userspace should not have to be aware about the fact that on that particular device LEDs are not completely independent as far as their brightness goes.

Thanks.

Signature · 1 line
Dmitry
author
#8 lore raw

Re: [PATCH v5 1/2] dt: add cap11xx LED documentation

Quoted 47 lines · Dmitry Torokhov
Quoted 41 lines · Matt Ranostay
Quoted 31 lines · Jacek Anaszewski
Quoted 18 lines · Dmitry Torokhov
Quoted 16 lines · Matt Ranostay
Signed-off-by Matt Ranostay <[email protected]>
---
  .../devicetree/bindings/input/cap11xx.txt          | 25
++++++++++++++++++++++
  1 file changed, 25 insertions(+)
Documentation/devicetree/bindings/input/cap11xx.txt +0 -0
index 7d0a300..09cdc43 100644
@@ -38,6 +38,11 @@ Optional properties:
                                defaults. The array must have exactly six
                                entries.
Quoted 10 lines
Quoted 5 lines
Documentation/devicetree/bindings/input/cap11xx.txt +1 -0
+       linux,led-brightness:   Defines the ON brightness when the

optional LED
+ functionality is used. Valid values are
1-15.
+ By default a value of 15 is set.

Please mention the device does not allow controlling brightness of leds individually and that is why this property is at device level, not individual led level.

Quoted 7 lines

I've just noticed that we have drivers/leds/leds-netxbig.c driver, which also doesn't allow controlling the LEDs on extension board individually, but it still does allow changing their brightness. I am leaning towards allowing this also for this driver and adding similar comment in the source code like at the line 218 of the aforementioned driver. As a result this property wouldn't be required.

Quoted 4 lines

Ok that should be pretty simple to do. But seems kind weird to have each led channel to be changing the brightness of all. Wouldn't the brightness sysfs entries of the other led channels be showing incorrect values?

Quoted 6 lines

I agree, this is kind of weird. Maybe we should have a device-specific attribute (on the platform device level) that allows controlling overall brightness, but I think LEDs should be just on/off with max brightness of 1. Userspace should not have to be aware about the fact that on that particular device LEDs are not completely independent as far as their brightness goes.

So should I drop the devicetree part of the patch in v6?

Quoted 5 lines

Thanks.

--
Dmitry

A message that is not in the archive (sent off-list or not yet received): CAKzfze_n7hf629=aUb6j-tcPcBLcwTH-a8yLJWS0cLM=dOkEVQ-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org

#10 lore raw

Re: [PATCH v5 1/2] dt: add cap11xx LED documentation

Quoted 49 lines · Matt Ranostay
Quoted 47 lines · Dmitry Torokhov
Quoted 45 lines · Matt Ranostay
Quoted 31 lines · Jacek Anaszewski
Quoted 18 lines · Dmitry Torokhov
Quoted 16 lines · Matt Ranostay
Signed-off-by Matt Ranostay <[email protected]>
---
  .../devicetree/bindings/input/cap11xx.txt          | 25
++++++++++++++++++++++
  1 file changed, 25 insertions(+)
Documentation/devicetree/bindings/input/cap11xx.txt +0 -0
index 7d0a300..09cdc43 100644
@@ -38,6 +38,11 @@ Optional properties:
                                defaults. The array must have exactly six
                                entries.
Quoted 10 lines
Quoted 5 lines
Documentation/devicetree/bindings/input/cap11xx.txt +1 -0
+       linux,led-brightness:   Defines the ON brightness when the

optional LED
+ functionality is used. Valid values are
1-15.
+ By default a value of 15 is set.

Please mention the device does not allow controlling brightness of leds individually and that is why this property is at device level, not individual led level.

Quoted 7 lines

I've just noticed that we have drivers/leds/leds-netxbig.c driver, which also doesn't allow controlling the LEDs on extension board individually, but it still does allow changing their brightness. I am leaning towards allowing this also for this driver and adding similar comment in the source code like at the line 218 of the aforementioned driver. As a result this property wouldn't be required.

Ok that should be pretty simple to do. But seems kind weird to have each led channel to be changing the brightness of all. Wouldn't the brightness sysfs entries of the other led channels be showing incorrect values?

Quoted 8 lines
Quoted 6 lines

I agree, this is kind of weird. Maybe we should have a device-specific attribute (on the platform device level) that allows controlling overall brightness, but I think LEDs should be just on/off with max brightness of 1. Userspace should not have to be aware about the fact that on that particular device LEDs are not completely independent as far as their brightness goes.

So should I drop the devicetree part of the patch in v6?

I think so. Let's have the device come up with LEDs brightness at default value and then users can adjust it as they need via sysfs.

Thanks.

Signature · 5 lines
Dmitry
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
#11 lore raw

Re: [PATCH v5 1/2] dt: add cap11xx LED documentation

In reply to Dmitry Torokhov
Quoted 42 lines · Dmitry Torokhov
Quoted 41 lines · Matt Ranostay
Quoted 38 lines · Jacek Anaszewski
Quoted 18 lines · Dmitry Torokhov
Quoted 16 lines · Matt Ranostay
Signed-off-by Matt Ranostay <[email protected]>
---
   .../devicetree/bindings/input/cap11xx.txt          | 25
++++++++++++++++++++++
   1 file changed, 25 insertions(+)
Documentation/devicetree/bindings/input/cap11xx.txt +0 -0
index 7d0a300..09cdc43 100644
@@ -38,6 +38,11 @@ Optional properties:
                                 defaults. The array must have exactly six
                                 entries.
Quoted 10 lines
Quoted 5 lines
Documentation/devicetree/bindings/input/cap11xx.txt +1 -0
+       linux,led-brightness:   Defines the ON brightness when the

optional LED
+ functionality is used. Valid values are
1-15.
+ By default a value of 15 is set.

Please mention the device does not allow controlling brightness of leds individually and that is why this property is at device level, not individual led level.

I've just noticed that we have drivers/leds/leds-netxbig.c driver, which also doesn't allow controlling the LEDs on extension board individually, but it still does allow changing their brightness. I am leaning towards allowing this also for this driver and adding similar comment in the source code like at the line 218 of the aforementioned driver. As a result this property wouldn't be required.

Quoted 4 lines
Quoted 4 lines

Ok that should be pretty simple to do. But seems kind weird to have each led channel to be changing the brightness of all. Wouldn't the brightness sysfs entries of the other led channels be showing incorrect values?

If you implemented brightness_get op it would show proper value. Assuming that the device allows reading current brightness.

Quoted 6 lines

I agree, this is kind of weird. Maybe we should have a device-specific attribute (on the platform device level) that allows controlling overall brightness, but I think LEDs should be just on/off with max brightness of 1. Userspace should not have to be aware about the fact that on that particular device LEDs are not completely independent as far as their brightness goes.

This way we are removing the possibility of controlling LED brightness at all, the feature that the hardware supports. Anyway, we will have to add a mechanism to the LED subsystem for detecting which LED class devices are controlled by the same hardware. This will allow for describing this type of dependencies. This is on "to do" list.

Signature · 2 lines
Best Regards,
Jacek Anaszewski
#12 lore raw

Re: [PATCH v5 1/2] dt: add cap11xx LED documentation

Quoted 48 lines · Jacek Anaszewski
Quoted 47 lines · Dmitry Torokhov
Quoted 46 lines · Matt Ranostay
Quoted 43 lines · Jacek Anaszewski
Quoted 19 lines · Dmitry Torokhov
Quoted 17 lines · Matt Ranostay
Signed-off-by Matt Ranostay <[email protected]>
---
   .../devicetree/bindings/input/cap11xx.txt          | 25
++++++++++++++++++++++
   1 file changed, 25 insertions(+)
Documentation/devicetree/bindings/input/cap11xx.txt +0 -0
index 7d0a300..09cdc43 100644
@@ -38,6 +38,11 @@ Optional properties:
                                 defaults. The array must have

exactly six
entries.

Quoted 12 lines
Quoted 6 lines

+ linux,led-brightness: Defines the ON brightness when the
optional LED
+ functionality is used. Valid
values are
1-15.
+ By default a value of 15 is set.

Please mention the device does not allow controlling brightness of leds
individually and that is why this property is at device level, not individual led level.

I've just noticed that we have drivers/leds/leds-netxbig.c driver, which
also doesn't allow controlling the LEDs on extension board individually,
but it still does allow changing their brightness. I am leaning towards allowing this also for this driver and adding similar comment in the source code like at the line 218 of the aforementioned driver. As a result this property wouldn't be required.

Quoted 7 lines
Quoted 4 lines
Quoted 4 lines

Ok that should be pretty simple to do. But seems kind weird to have each led channel to be changing the brightness of all. Wouldn't the brightness sysfs entries of the other led channels be showing incorrect values?

If you implemented brightness_get op it would show proper value. Assuming that the device allows reading current brightness.

Of course brightness_get wouldn't have to read the brightness from the device, but return the value cached on on each call to brightness_set for any LED class device exposed by the driver.

Signature · 2 lines
Best Regards,
Jacek Anaszewski
author
#13 lore raw

PATCH v5 2/2 cap11xx: add LED support

In reply to Matt Ranostay

Several cap11xx variants have LEDs that be can be controlled, this patchset implements this functionality.

Signed-off-by Matt Ranostay <[email protected]>
---
 drivers/input/keyboard/cap11xx.c | 141 ++++++++++++++++++++++++++++++++++++++-
 1 file changed, 138 insertions(+), 3 deletions(-)
drivers/input/keyboard/cap11xx.c +138 -3
index f07461a..b48d72f 100644
@@ -12,6 +12,7 @@
 #include <linux/module.h>
 #include <linux/interrupt.h>
 #include <linux/input.h>
+#include <linux/leds.h>
 #include <linux/of_irq.h>
 #include <linux/regmap.h>
 #include <linux/i2c.h>

@@ -47,6 +48,20 @@
 #define CAP11XX_REG_CONFIG2		0x44
 #define CAP11XX_REG_CONFIG2_ALT_POL	BIT(6)
 #define CAP11XX_REG_SENSOR_BASE_CNT(X)	(0x50 + (X))
+#define CAP11XX_REG_LED_POLARITY	0x73
+#define CAP11XX_REG_LED_OUTPUT_CONTROL	0x74
+
+#define CAP11XX_REG_LED_DUTY_CYCLE_1	0x90
+#define CAP11XX_REG_LED_DUTY_CYCLE_2	0x91
+#define CAP11XX_REG_LED_DUTY_CYCLE_3	0x92
+#define CAP11XX_REG_LED_DUTY_CYCLE_4	0x93
+
+#define CAP11XX_REG_LED_DUTY_MIN_MASK	(0x0f)
+#define CAP11XX_REG_LED_DUTY_MIN_MASK_SHIFT	(0)
+#define CAP11XX_REG_LED_DUTY_MAX_MASK	(0xf0)
+#define CAP11XX_REG_LED_DUTY_MAX_MASK_SHIFT	(4)
+#define CAP11XX_REG_LED_DUTY_MAX_VALUE	(15)
+
 #define CAP11XX_REG_SENSOR_CALIB	(0xb1 + (X))
 #define CAP11XX_REG_SENSOR_CALIB_LSB1	0xb9
 #define CAP11XX_REG_SENSOR_CALIB_LSB2	0xba

@@ -56,10 +71,24 @@
 
 #define CAP11XX_MANUFACTURER_ID	0x5d
 
+#ifdef CONFIG_LEDS_CLASS
+struct cap11xx_led {
+	struct cap11xx_priv *priv;
+	struct led_classdev cdev;
+	struct work_struct work;
+	u32 reg;
+	enum led_brightness new_brightness;
+};
+#endif
+
 struct cap11xx_priv {
 	struct regmap *regmap;
 	struct input_dev *idev;
 
+#ifdef CONFIG_LEDS_CLASS
+	struct cap11xx_led *leds;
+	int num_leds;
+#endif
 	/* config */
 	u32 keycodes[];
 };

@@ -67,6 +96,7 @@ struct cap11xx_priv {
 struct cap11xx_hw_model {
 	u8 product_id;
 	unsigned int num_channels;
+	unsigned int num_leds;
 };
 
 enum {

@@ -76,9 +106,9 @@ enum {
 };
 
 static const struct cap11xx_hw_model cap11xx_devices[] = {
-	[CAP1106] = { .product_id = 0x55, .num_channels = 6 },
-	[CAP1126] = { .product_id = 0x53, .num_channels = 6 },
-	[CAP1188] = { .product_id = 0x50, .num_channels = 8 },
+	[CAP1106] = { .product_id = 0x55, .num_channels = 6, .num_leds = 0 },
+	[CAP1126] = { .product_id = 0x53, .num_channels = 6, .num_leds = 2 },
+	[CAP1188] = { .product_id = 0x50, .num_channels = 8, .num_leds = 8 },
 };
 
 static const struct reg_default cap11xx_reg_defaults[] = {

@@ -111,6 +141,7 @@ static const struct reg_default cap11xx_reg_defaults[] = {
 	{ CAP11XX_REG_STANDBY_SENSITIVITY,	0x02 },
 	{ CAP11XX_REG_STANDBY_THRESH,		0x40 },
 	{ CAP11XX_REG_CONFIG2,			0x40 },
+	{ CAP11XX_REG_LED_POLARITY,		0x00 },
 	{ CAP11XX_REG_SENSOR_CALIB_LSB1,	0x00 },
 	{ CAP11XX_REG_SENSOR_CALIB_LSB2,	0x00 },
 };

@@ -177,6 +208,13 @@ out:
 
 static int cap11xx_set_sleep(struct cap11xx_priv *priv, bool sleep)
 {
+#ifdef CONFIG_LEDS_CLASS
+	/*
+	 * DLSEEP mode will turn off all LEDS, prevent this
+	 */
+	if (priv->num_leds)
+		return 0;
+#endif
 	return regmap_update_bits(priv->regmap, CAP11XX_REG_MAIN_CONTROL,
 				  CAP11XX_REG_MAIN_CONTROL_DLSEEP,
 				  sleep ? CAP11XX_REG_MAIN_CONTROL_DLSEEP : 0);

@@ -196,6 +234,98 @@ static void cap11xx_input_close(struct input_dev *idev)
 	cap11xx_set_sleep(priv, true);
 }
 
+#ifdef CONFIG_LEDS_CLASS
+static void cap11xx_led_work(struct work_struct *work)
+{
+	struct cap11xx_led *led = container_of(work, struct cap11xx_led, work);
+	struct cap11xx_priv *priv = led->priv;
+	int value = led->new_brightness;
+
+	/*
+	 * All LEDs share the same duty cycle as this is a HW limitation.
+	 * Brightness levels per LED are either 0 (OFF) and 1 (ON).
+	 *
+	 * Actual brightness level for the ON state for all LEDS is set via the
+	 * "linux,led-brightness" DT property.
+	 */
+	regmap_update_bits(priv->regmap, CAP11XX_REG_LED_OUTPUT_CONTROL,
+				BIT(led->reg), value ? BIT(led->reg) : 0);
+}
+
+static void cap11xx_led_set(struct led_classdev *cdev,
+			   enum led_brightness value)
+{
+	struct cap11xx_led *led = container_of(cdev, struct cap11xx_led, cdev);
+
+	if (led->new_brightness == value)
+		return;
+
+	led->new_brightness = value;
+	schedule_work(&led->work);
+}
+
+static int cap11xx_init_leds(struct device *dev,
+			     struct cap11xx_priv *priv, int num_leds)
+{
+	struct device_node *node = dev->of_node, *child;
+	struct cap11xx_led *led;
+	int cnt = of_get_child_count(node);
+	u32 reg = 0;
+	int error;
+
+	if (!num_leds || !cnt)
+		return 0;
+	if (cnt > num_leds)
+		return -EINVAL;
+
+	led = devm_kcalloc(dev, cnt,
+		sizeof(struct cap11xx_led), GFP_KERNEL);
+	if (!led)
+		return -ENOMEM;
+
+	priv->leds = led;
+	error = of_property_read_u32(node, "linux,led-brightness", &reg);
+	if (error || reg == 0 || reg > CAP11XX_REG_LED_DUTY_MAX_VALUE)
+		reg = CAP11XX_REG_LED_DUTY_MAX_VALUE;
+
+	error = regmap_update_bits(priv->regmap,
+				CAP11XX_REG_LED_OUTPUT_CONTROL, 0xff, 0);
+	if (error)
+		return error;
+	error = regmap_update_bits(priv->regmap, CAP11XX_REG_LED_DUTY_CYCLE_4,
+				CAP11XX_REG_LED_DUTY_MAX_MASK,
+				reg << CAP11XX_REG_LED_DUTY_MAX_MASK_SHIFT);
+	if (error)
+		return error;
+
+	for_each_child_of_node(node, child) {
+		led->cdev.name =
+			of_get_property(child, "label", NULL) ? : child->name;
+		led->cdev.default_trigger =
+			of_get_property(child, "linux,default-trigger", NULL);
+		led->cdev.flags = 0;
+		led->cdev.brightness_set = cap11xx_led_set;
+		led->cdev.max_brightness = 1;
+		led->cdev.brightness = LED_OFF;
+
+		error = of_property_read_u32(child, "reg", &reg);
+		if (error != 0 || reg >= num_leds)
+			continue;
+		led->reg = reg;
+		led->priv = priv;
+
+		INIT_WORK(&led->work, cap11xx_led_work);
+		error = devm_led_classdev_register(dev, &led->cdev);
+		if (error < 0)
+			return -EINVAL;
+		priv->num_leds++;
+		led++;
+	};
+
+	return 0;
+}
+#endif
+
 static int cap11xx_i2c_probe(struct i2c_client *i2c_client,
 			     const struct i2c_device_id *id)
 {

@@ -316,6 +446,11 @@ static int cap11xx_i2c_probe(struct i2c_client *i2c_client,
 	priv->idev->open = cap11xx_input_open;
 	priv->idev->close = cap11xx_input_close;
 
+#ifdef CONFIG_LEDS_CLASS
+	error = cap11xx_init_leds(dev, priv, cap->num_leds);
+	if (error)
+		return error;
+#endif
 	input_set_drvdata(priv->idev, priv);
 
 	/*
Signature · 1 line
1.9.1
Acked-by on v5 2/2
#14 lore raw

Hi Matt,

Quoted 187 lines · Matt Ranostay

Several cap11xx variants have LEDs that be can be controlled, this patchset implements this functionality.

Signed-off-by Matt Ranostay <[email protected]>
---
  drivers/input/keyboard/cap11xx.c | 141 ++++++++++++++++++++++++++++++++++++++-
  1 file changed, 138 insertions(+), 3 deletions(-)
drivers/input/keyboard/cap11xx.c +115 -3
index f07461a..b48d72f 100644
@@ -12,6 +12,7 @@
  #include <linux/module.h>
  #include <linux/interrupt.h>
  #include <linux/input.h>
+#include <linux/leds.h>
  #include <linux/of_irq.h>
  #include <linux/regmap.h>
  #include <linux/i2c.h>

@@ -47,6 +48,20 @@
  #define CAP11XX_REG_CONFIG2		0x44
  #define CAP11XX_REG_CONFIG2_ALT_POL	BIT(6)
  #define CAP11XX_REG_SENSOR_BASE_CNT(X)	(0x50 + (X))
+#define CAP11XX_REG_LED_POLARITY	0x73
+#define CAP11XX_REG_LED_OUTPUT_CONTROL	0x74
+
+#define CAP11XX_REG_LED_DUTY_CYCLE_1	0x90
+#define CAP11XX_REG_LED_DUTY_CYCLE_2	0x91
+#define CAP11XX_REG_LED_DUTY_CYCLE_3	0x92
+#define CAP11XX_REG_LED_DUTY_CYCLE_4	0x93
+
+#define CAP11XX_REG_LED_DUTY_MIN_MASK	(0x0f)
+#define CAP11XX_REG_LED_DUTY_MIN_MASK_SHIFT	(0)
+#define CAP11XX_REG_LED_DUTY_MAX_MASK	(0xf0)
+#define CAP11XX_REG_LED_DUTY_MAX_MASK_SHIFT	(4)
+#define CAP11XX_REG_LED_DUTY_MAX_VALUE	(15)
+
  #define CAP11XX_REG_SENSOR_CALIB	(0xb1 + (X))
  #define CAP11XX_REG_SENSOR_CALIB_LSB1	0xb9
  #define CAP11XX_REG_SENSOR_CALIB_LSB2	0xba

@@ -56,10 +71,24 @@
 
  #define CAP11XX_MANUFACTURER_ID	0x5d
 
+#ifdef CONFIG_LEDS_CLASS
+struct cap11xx_led {
+	struct cap11xx_priv *priv;
+	struct led_classdev cdev;
+	struct work_struct work;
+	u32 reg;
+	enum led_brightness new_brightness;
+};
+#endif
+
  struct cap11xx_priv {
  	struct regmap *regmap;
  	struct input_dev *idev;
 
+#ifdef CONFIG_LEDS_CLASS
+	struct cap11xx_led *leds;
+	int num_leds;
+#endif
  	/* config */
  	u32 keycodes[];
  };

@@ -67,6 +96,7 @@ struct cap11xx_priv {
  struct cap11xx_hw_model {
  	u8 product_id;
  	unsigned int num_channels;
+	unsigned int num_leds;
  };
 
  enum {

@@ -76,9 +106,9 @@ enum {
  };
 
  static const struct cap11xx_hw_model cap11xx_devices[] = {
-	[CAP1106] = { .product_id = 0x55, .num_channels = 6 },
-	[CAP1126] = { .product_id = 0x53, .num_channels = 6 },
-	[CAP1188] = { .product_id = 0x50, .num_channels = 8 },
+	[CAP1106] = { .product_id = 0x55, .num_channels = 6, .num_leds = 0 },
+	[CAP1126] = { .product_id = 0x53, .num_channels = 6, .num_leds = 2 },
+	[CAP1188] = { .product_id = 0x50, .num_channels = 8, .num_leds = 8 },
  };
 
  static const struct reg_default cap11xx_reg_defaults[] = {

@@ -111,6 +141,7 @@ static const struct reg_default cap11xx_reg_defaults[] = {
  	{ CAP11XX_REG_STANDBY_SENSITIVITY,	0x02 },
  	{ CAP11XX_REG_STANDBY_THRESH,		0x40 },
  	{ CAP11XX_REG_CONFIG2,			0x40 },
+	{ CAP11XX_REG_LED_POLARITY,		0x00 },
  	{ CAP11XX_REG_SENSOR_CALIB_LSB1,	0x00 },
  	{ CAP11XX_REG_SENSOR_CALIB_LSB2,	0x00 },
  };

@@ -177,6 +208,13 @@ out:
 
  static int cap11xx_set_sleep(struct cap11xx_priv *priv, bool sleep)
  {
+#ifdef CONFIG_LEDS_CLASS
+	/*
+	 * DLSEEP mode will turn off all LEDS, prevent this
+	 */
+	if (priv->num_leds)
+		return 0;
+#endif
  	return regmap_update_bits(priv->regmap, CAP11XX_REG_MAIN_CONTROL,
  				  CAP11XX_REG_MAIN_CONTROL_DLSEEP,
  				  sleep ? CAP11XX_REG_MAIN_CONTROL_DLSEEP : 0);

@@ -196,6 +234,98 @@ static void cap11xx_input_close(struct input_dev *idev)
  	cap11xx_set_sleep(priv, true);
  }
 
+#ifdef CONFIG_LEDS_CLASS
+static void cap11xx_led_work(struct work_struct *work)
+{
+	struct cap11xx_led *led = container_of(work, struct cap11xx_led, work);
+	struct cap11xx_priv *priv = led->priv;
+	int value = led->new_brightness;
+
+	/*
+	 * All LEDs share the same duty cycle as this is a HW limitation.
+	 * Brightness levels per LED are either 0 (OFF) and 1 (ON).
+	 *
+	 * Actual brightness level for the ON state for all LEDS is set via the
+	 * "linux,led-brightness" DT property.
+	 */
+	regmap_update_bits(priv->regmap, CAP11XX_REG_LED_OUTPUT_CONTROL,
+				BIT(led->reg), value ? BIT(led->reg) : 0);
+}
+
+static void cap11xx_led_set(struct led_classdev *cdev,
+			   enum led_brightness value)
+{
+	struct cap11xx_led *led = container_of(cdev, struct cap11xx_led, cdev);
+
+	if (led->new_brightness == value)
+		return;
+
+	led->new_brightness = value;
+	schedule_work(&led->work);
+}
+
+static int cap11xx_init_leds(struct device *dev,
+			     struct cap11xx_priv *priv, int num_leds)
+{
+	struct device_node *node = dev->of_node, *child;
+	struct cap11xx_led *led;
+	int cnt = of_get_child_count(node);
+	u32 reg = 0;
+	int error;
+
+	if (!num_leds || !cnt)
+		return 0;
+	if (cnt > num_leds)
+		return -EINVAL;
+
+	led = devm_kcalloc(dev, cnt,
+		sizeof(struct cap11xx_led), GFP_KERNEL);
+	if (!led)
+		return -ENOMEM;
+
+	priv->leds = led;
+	error = of_property_read_u32(node, "linux,led-brightness", &reg);
+	if (error || reg == 0 || reg > CAP11XX_REG_LED_DUTY_MAX_VALUE)
+		reg = CAP11XX_REG_LED_DUTY_MAX_VALUE;
+
+	error = regmap_update_bits(priv->regmap,
+				CAP11XX_REG_LED_OUTPUT_CONTROL, 0xff, 0);
+	if (error)
+		return error;
+	error = regmap_update_bits(priv->regmap, CAP11XX_REG_LED_DUTY_CYCLE_4,
+				CAP11XX_REG_LED_DUTY_MAX_MASK,
+				reg << CAP11XX_REG_LED_DUTY_MAX_MASK_SHIFT);
+	if (error)
+		return error;
+
+	for_each_child_of_node(node, child) {
+		led->cdev.name =
+			of_get_property(child, "label", NULL) ? : child->name;
+		led->cdev.default_trigger =
+			of_get_property(child, "linux,default-trigger", NULL);
+		led->cdev.flags = 0;
+		led->cdev.brightness_set = cap11xx_led_set;
+		led->cdev.max_brightness = 1;
+		led->cdev.brightness = LED_OFF;
+

drivers/input/keyboard/cap11xx.c static void cap11xx_input_close(struct input_dev *idev) · line 320

Quoted 10 lines
drivers/input/keyboard/cap11xx.c +10 -0
+		error = of_property_read_u32(child, "reg", &reg);
+		if (error != 0 || reg >= num_leds)
+			continue;
+		led->reg = reg;
+		led->priv = priv;
+
+		INIT_WORK(&led->work, cap11xx_led_work);
+		error = devm_led_classdev_register(dev, &led->cdev);
+		if (error < 0)
+			return -EINVAL;

I would add empty line here and after INIT_WORK.
With this addressed:

Acked-by Jacek Anaszewski <[email protected]>
Quoted 24 lines
drivers/input/keyboard/cap11xx.c +13 -0
+		priv->num_leds++;
+		led++;
+	};
+
+	return 0;
+}
+#endif
+
  static int cap11xx_i2c_probe(struct i2c_client *i2c_client,
  			     const struct i2c_device_id *id)
  {

@@ -316,6 +446,11 @@ static int cap11xx_i2c_probe(struct i2c_client *i2c_client,
  	priv->idev->open = cap11xx_input_open;
  	priv->idev->close = cap11xx_input_close;
 
+#ifdef CONFIG_LEDS_CLASS
+	error = cap11xx_init_leds(dev, priv, cap->num_leds);
+	if (error)
+		return error;
+#endif
  	input_set_drvdata(priv->idev, priv);
 
  	/*
Signature · 2 lines
Best Regards,
Jacek Anaszewski
author
#15 lore raw
Quoted 222 lines · Jacek Anaszewski

Hi Matt,

Quoted 203 lines · Matt Ranostay

Several cap11xx variants have LEDs that be can be controlled, this patchset implements this functionality.

Signed-off-by Matt Ranostay <[email protected]>
---
  drivers/input/keyboard/cap11xx.c | 141
++++++++++++++++++++++++++++++++++++++-
  1 file changed, 138 insertions(+), 3 deletions(-)
drivers/input/keyboard/cap11xx.c +33 -3
index f07461a..b48d72f 100644
@@ -12,6 +12,7 @@
  #include <linux/module.h>
  #include <linux/interrupt.h>
  #include <linux/input.h>
+#include <linux/leds.h>
  #include <linux/of_irq.h>
  #include <linux/regmap.h>
  #include <linux/i2c.h>

@@ -47,6 +48,20 @@
  #define CAP11XX_REG_CONFIG2           0x44
  #define CAP11XX_REG_CONFIG2_ALT_POL   BIT(6)
  #define CAP11XX_REG_SENSOR_BASE_CNT(X)        (0x50 + (X))
+#define CAP11XX_REG_LED_POLARITY       0x73
+#define CAP11XX_REG_LED_OUTPUT_CONTROL 0x74
+
+#define CAP11XX_REG_LED_DUTY_CYCLE_1   0x90
+#define CAP11XX_REG_LED_DUTY_CYCLE_2   0x91
+#define CAP11XX_REG_LED_DUTY_CYCLE_3   0x92
+#define CAP11XX_REG_LED_DUTY_CYCLE_4   0x93
+
+#define CAP11XX_REG_LED_DUTY_MIN_MASK  (0x0f)
+#define CAP11XX_REG_LED_DUTY_MIN_MASK_SHIFT    (0)
+#define CAP11XX_REG_LED_DUTY_MAX_MASK  (0xf0)
+#define CAP11XX_REG_LED_DUTY_MAX_MASK_SHIFT    (4)
+#define CAP11XX_REG_LED_DUTY_MAX_VALUE (15)
+
  #define CAP11XX_REG_SENSOR_CALIB      (0xb1 + (X))
  #define CAP11XX_REG_SENSOR_CALIB_LSB1 0xb9
  #define CAP11XX_REG_SENSOR_CALIB_LSB2 0xba

@@ -56,10 +71,24 @@
 
  #define CAP11XX_MANUFACTURER_ID       0x5d
 
+#ifdef CONFIG_LEDS_CLASS
+struct cap11xx_led {
+       struct cap11xx_priv *priv;
+       struct led_classdev cdev;
+       struct work_struct work;
+       u32 reg;
+       enum led_brightness new_brightness;
+};
+#endif
+
  struct cap11xx_priv {
        struct regmap *regmap;
        struct input_dev *idev;
 
+#ifdef CONFIG_LEDS_CLASS
+       struct cap11xx_led *leds;
+       int num_leds;
+#endif
        /* config */
        u32 keycodes[];
  };

@@ -67,6 +96,7 @@ struct cap11xx_priv {
  struct cap11xx_hw_model {
        u8 product_id;
        unsigned int num_channels;
+       unsigned int num_leds;
  };
 
  enum {

@@ -76,9 +106,9 @@ enum {
  };
 
  static const struct cap11xx_hw_model cap11xx_devices[] = {
-       [CAP1106] = { .product_id = 0x55, .num_channels = 6 },
-       [CAP1126] = { .product_id = 0x53, .num_channels = 6 },
-       [CAP1188] = { .product_id = 0x50, .num_channels = 8 },
+       [CAP1106] = { .product_id = 0x55, .num_channels = 6, .num_leds = 0
 },
+       [CAP1126] = { .product_id = 0x53, .num_channels = 6, .num_leds = 2
 },
+       [CAP1188] = { .product_id = 0x50, .num_channels = 8, .num_leds = 8
 },

  };

  static const struct reg_default cap11xx_reg_defaults[] = {
drivers/input/keyboard/cap11xx.c +1 -0
@@ -111,6 +141,7 @@ static const struct reg_default cap11xx_reg_defaults[]
 = {
        { CAP11XX_REG_STANDBY_SENSITIVITY,      0x02 },
        { CAP11XX_REG_STANDBY_THRESH,           0x40 },
        { CAP11XX_REG_CONFIG2,                  0x40 },
+       { CAP11XX_REG_LED_POLARITY,             0x00 },
        { CAP11XX_REG_SENSOR_CALIB_LSB1,        0x00 },
        { CAP11XX_REG_SENSOR_CALIB_LSB2,        0x00 },

  };
drivers/input/keyboard/cap11xx.c +7 -0
@@ -177,6 +208,13 @@ out:
 
  static int cap11xx_set_sleep(struct cap11xx_priv *priv, bool sleep)
  {
+#ifdef CONFIG_LEDS_CLASS
+       /*
+        * DLSEEP mode will turn off all LEDS, prevent this
+        */
+       if (priv->num_leds)
+               return 0;
+#endif
        return regmap_update_bits(priv->regmap, CAP11XX_REG_MAIN_CONTROL,
                                  CAP11XX_REG_MAIN_CONTROL_DLSEEP,
                                  sleep ? CAP11XX_REG_MAIN_CONTROL_DLSEEP

: 0);

drivers/input/keyboard/cap11xx.c +12 -0
@@ -196,6 +234,98 @@ static void cap11xx_input_close(struct input_dev
 *idev)
        cap11xx_set_sleep(priv, true);
  }
 
+#ifdef CONFIG_LEDS_CLASS
+static void cap11xx_led_work(struct work_struct *work)
+{
+       struct cap11xx_led *led = container_of(work, struct cap11xx_led,
 work);
+       struct cap11xx_priv *priv = led->priv;
+       int value = led->new_brightness;
+
+       /*
+        * All LEDs share the same duty cycle as this is a HW limitation.
+        * Brightness levels per LED are either 0 (OFF) and 1 (ON).
+        *
+        * Actual brightness level for the ON state for all LEDS is set

via the

drivers/input/keyboard/cap11xx.c +10 -0
+        * "linux,led-brightness" DT property.
+        */
+       regmap_update_bits(priv->regmap, CAP11XX_REG_LED_OUTPUT_CONTROL,
+                               BIT(led->reg), value ? BIT(led->reg) : 0);
+}
+
+static void cap11xx_led_set(struct led_classdev *cdev,
+                          enum led_brightness value)
+{
+       struct cap11xx_led *led = container_of(cdev, struct cap11xx_led,

cdev);

drivers/input/keyboard/cap11xx.c +37 -0
+
+       if (led->new_brightness == value)
+               return;
+
+       led->new_brightness = value;
+       schedule_work(&led->work);
+}
+
+static int cap11xx_init_leds(struct device *dev,
+                            struct cap11xx_priv *priv, int num_leds)
+{
+       struct device_node *node = dev->of_node, *child;
+       struct cap11xx_led *led;
+       int cnt = of_get_child_count(node);
+       u32 reg = 0;
+       int error;
+
+       if (!num_leds || !cnt)
+               return 0;
+       if (cnt > num_leds)
+               return -EINVAL;
+
+       led = devm_kcalloc(dev, cnt,
+               sizeof(struct cap11xx_led), GFP_KERNEL);
+       if (!led)
+               return -ENOMEM;
+
+       priv->leds = led;
+       error = of_property_read_u32(node, "linux,led-brightness", &reg);
+       if (error || reg == 0 || reg > CAP11XX_REG_LED_DUTY_MAX_VALUE)
+               reg = CAP11XX_REG_LED_DUTY_MAX_VALUE;
+
+       error = regmap_update_bits(priv->regmap,
+                               CAP11XX_REG_LED_OUTPUT_CONTROL, 0xff, 0);
+       if (error)
+               return error;
+       error = regmap_update_bits(priv->regmap,

CAP11XX_REG_LED_DUTY_CYCLE_4,

drivers/input/keyboard/cap11xx.c +2 -0
+                               CAP11XX_REG_LED_DUTY_MAX_MASK,
+                               reg <<

CAP11XX_REG_LED_DUTY_MAX_MASK_SHIFT);

drivers/input/keyboard/cap11xx.c +6 -0
+       if (error)
+               return error;
+
+       for_each_child_of_node(node, child) {
+               led->cdev.name =
+                       of_get_property(child, "label", NULL) ? :

child->name;

drivers/input/keyboard/cap11xx.c +2 -0
+               led->cdev.default_trigger =
+                       of_get_property(child, "linux,default-trigger",

NULL);

drivers/input/keyboard/cap11xx.c +5 -0
+               led->cdev.flags = 0;
+               led->cdev.brightness_set = cap11xx_led_set;
+               led->cdev.max_brightness = 1;
+               led->cdev.brightness = LED_OFF;
+

drivers/input/keyboard/cap11xx.c static void cap11xx_input_close(struct input_dev *idev) · line 320

Quoted 10 lines
drivers/input/keyboard/cap11xx.c +10 -0
+               error = of_property_read_u32(child, "reg", &reg);
+               if (error != 0 || reg >= num_leds)
+                       continue;
+               led->reg = reg;
+               led->priv = priv;
+
+               INIT_WORK(&led->work, cap11xx_led_work);
+               error = devm_led_classdev_register(dev, &led->cdev);
+               if (error < 0)
+                       return -EINVAL;

I would add empty line here and after INIT_WORK.
With this addressed:

Quoted 1 line
Acked-by Jacek Anaszewski <[email protected]>

Dmitry do you have any input for this rev? If not will you fix the newline issue here? Seems silly to do a v6 for this

I plan to do a runtime PM patchset to clean up this crude check within a week or two.

Thanks,

MAtt

Quoted 24 lines
Quoted 24 lines
drivers/input/keyboard/cap11xx.c +13 -0
+               priv->num_leds++;
+               led++;
+       };
+
+       return 0;
+}
+#endif
+
  static int cap11xx_i2c_probe(struct i2c_client *i2c_client,
                             const struct i2c_device_id *id)
  {

@@ -316,6 +446,11 @@ static int cap11xx_i2c_probe(struct i2c_client
 *i2c_client,
        priv->idev->open = cap11xx_input_open;
        priv->idev->close = cap11xx_input_close;
 
+#ifdef CONFIG_LEDS_CLASS
+       error = cap11xx_init_leds(dev, priv, cap->num_leds);
+       if (error)
+               return error;
+#endif
        input_set_drvdata(priv->idev, priv);
Quoted 7 lines
Quoted 2 lines
        /*

--
Best Regards,
Jacek Anaszewski

--
To unsubscribe from this list: send the line "unsubscribe linux-input" in

maintainer
#16 lore raw
In reply to Matt Ranostay
Quoted 96 lines · Matt Ranostay

Several cap11xx variants have LEDs that be can be controlled, this patchset implements this functionality.

Signed-off-by Matt Ranostay <[email protected]>
---
 drivers/input/keyboard/cap11xx.c | 141 ++++++++++++++++++++++++++++++++++++++-
 1 file changed, 138 insertions(+), 3 deletions(-)
drivers/input/keyboard/cap11xx.c +34 -3
index f07461a..b48d72f 100644
@@ -12,6 +12,7 @@
 #include <linux/module.h>
 #include <linux/interrupt.h>
 #include <linux/input.h>
+#include <linux/leds.h>
 #include <linux/of_irq.h>
 #include <linux/regmap.h>
 #include <linux/i2c.h>

@@ -47,6 +48,20 @@
 #define CAP11XX_REG_CONFIG2		0x44
 #define CAP11XX_REG_CONFIG2_ALT_POL	BIT(6)
 #define CAP11XX_REG_SENSOR_BASE_CNT(X)	(0x50 + (X))
+#define CAP11XX_REG_LED_POLARITY	0x73
+#define CAP11XX_REG_LED_OUTPUT_CONTROL	0x74
+
+#define CAP11XX_REG_LED_DUTY_CYCLE_1	0x90
+#define CAP11XX_REG_LED_DUTY_CYCLE_2	0x91
+#define CAP11XX_REG_LED_DUTY_CYCLE_3	0x92
+#define CAP11XX_REG_LED_DUTY_CYCLE_4	0x93
+
+#define CAP11XX_REG_LED_DUTY_MIN_MASK	(0x0f)
+#define CAP11XX_REG_LED_DUTY_MIN_MASK_SHIFT	(0)
+#define CAP11XX_REG_LED_DUTY_MAX_MASK	(0xf0)
+#define CAP11XX_REG_LED_DUTY_MAX_MASK_SHIFT	(4)
+#define CAP11XX_REG_LED_DUTY_MAX_VALUE	(15)
+
 #define CAP11XX_REG_SENSOR_CALIB	(0xb1 + (X))
 #define CAP11XX_REG_SENSOR_CALIB_LSB1	0xb9
 #define CAP11XX_REG_SENSOR_CALIB_LSB2	0xba

@@ -56,10 +71,24 @@
 
 #define CAP11XX_MANUFACTURER_ID	0x5d
 
+#ifdef CONFIG_LEDS_CLASS
+struct cap11xx_led {
+	struct cap11xx_priv *priv;
+	struct led_classdev cdev;
+	struct work_struct work;
+	u32 reg;
+	enum led_brightness new_brightness;
+};
+#endif
+
 struct cap11xx_priv {
 	struct regmap *regmap;
 	struct input_dev *idev;
 
+#ifdef CONFIG_LEDS_CLASS
+	struct cap11xx_led *leds;
+	int num_leds;
+#endif
 	/* config */
 	u32 keycodes[];
 };

@@ -67,6 +96,7 @@ struct cap11xx_priv {
 struct cap11xx_hw_model {
 	u8 product_id;
 	unsigned int num_channels;
+	unsigned int num_leds;
 };
 
 enum {

@@ -76,9 +106,9 @@ enum {
 };
 
 static const struct cap11xx_hw_model cap11xx_devices[] = {
-	[CAP1106] = { .product_id = 0x55, .num_channels = 6 },
-	[CAP1126] = { .product_id = 0x53, .num_channels = 6 },
-	[CAP1188] = { .product_id = 0x50, .num_channels = 8 },
+	[CAP1106] = { .product_id = 0x55, .num_channels = 6, .num_leds = 0 },
+	[CAP1126] = { .product_id = 0x53, .num_channels = 6, .num_leds = 2 },
+	[CAP1188] = { .product_id = 0x50, .num_channels = 8, .num_leds = 8 },
 };
 
 static const struct reg_default cap11xx_reg_defaults[] = {

@@ -111,6 +141,7 @@ static const struct reg_default cap11xx_reg_defaults[] = {
 	{ CAP11XX_REG_STANDBY_SENSITIVITY,	0x02 },
 	{ CAP11XX_REG_STANDBY_THRESH,		0x40 },
 	{ CAP11XX_REG_CONFIG2,			0x40 },
+	{ CAP11XX_REG_LED_POLARITY,		0x00 },
 	{ CAP11XX_REG_SENSOR_CALIB_LSB1,	0x00 },
 	{ CAP11XX_REG_SENSOR_CALIB_LSB2,	0x00 },
 };

drivers/input/keyboard/cap11xx.c out: · line 217

Quoted 10 lines
drivers/input/keyboard/cap11xx.c +7 -0
@@ -177,6 +208,13 @@ out:
 
 static int cap11xx_set_sleep(struct cap11xx_priv *priv, bool sleep)
 {
+#ifdef CONFIG_LEDS_CLASS
+	/*
+	 * DLSEEP mode will turn off all LEDS, prevent this
+	 */
+	if (priv->num_leds)
+		return 0;
+#endif

Do we need to have #ifdef here? I think we can spare a pointer and an integer in device structure even if leds are disabled.

Quoted 40 lines
drivers/input/keyboard/cap11xx.c +33 -0
 	return regmap_update_bits(priv->regmap, CAP11XX_REG_MAIN_CONTROL,
 				  CAP11XX_REG_MAIN_CONTROL_DLSEEP,
 				  sleep ? CAP11XX_REG_MAIN_CONTROL_DLSEEP : 0);

@@ -196,6 +234,98 @@ static void cap11xx_input_close(struct input_dev *idev)
 	cap11xx_set_sleep(priv, true);
 }
 
+#ifdef CONFIG_LEDS_CLASS
+static void cap11xx_led_work(struct work_struct *work)
+{
+	struct cap11xx_led *led = container_of(work, struct cap11xx_led, work);
+	struct cap11xx_priv *priv = led->priv;
+	int value = led->new_brightness;
+
+	/*
+	 * All LEDs share the same duty cycle as this is a HW limitation.
+	 * Brightness levels per LED are either 0 (OFF) and 1 (ON).
+	 *
+	 * Actual brightness level for the ON state for all LEDS is set via the
+	 * "linux,led-brightness" DT property.
+	 */
+	regmap_update_bits(priv->regmap, CAP11XX_REG_LED_OUTPUT_CONTROL,
+				BIT(led->reg), value ? BIT(led->reg) : 0);
+}
+
+static void cap11xx_led_set(struct led_classdev *cdev,
+			   enum led_brightness value)
+{
+	struct cap11xx_led *led = container_of(cdev, struct cap11xx_led, cdev);
+
+	if (led->new_brightness == value)
+		return;
+
+	led->new_brightness = value;
+	schedule_work(&led->work);
+}
+
+static int cap11xx_init_leds(struct device *dev,
+			     struct cap11xx_priv *priv, int num_leds)
+{

drivers/input/keyboard/cap11xx.c static void cap11xx_input_close(struct input_dev *idev) · line 279

Quoted 10 lines
drivers/input/keyboard/cap11xx.c +10 -0
+	struct device_node *node = dev->of_node, *child;
+	struct cap11xx_led *led;
+	int cnt = of_get_child_count(node);
+	u32 reg = 0;
+	int error;
+
+	if (!num_leds || !cnt)
+		return 0;
+	if (cnt > num_leds)
+		return -EINVAL;

An error message might be nice here.

drivers/input/keyboard/cap11xx.c static void cap11xx_input_close(struct input_dev *idev) · line 289

Quoted 10 lines
drivers/input/keyboard/cap11xx.c +10 -0
+
+	led = devm_kcalloc(dev, cnt,
+		sizeof(struct cap11xx_led), GFP_KERNEL);
+	if (!led)
+		return -ENOMEM;
+
+	priv->leds = led;
+	error = of_property_read_u32(node, "linux,led-brightness", &reg);
+	if (error || reg == 0 || reg > CAP11XX_REG_LED_DUTY_MAX_VALUE)
+		reg = CAP11XX_REG_LED_DUTY_MAX_VALUE;

I'd rather we error out if invalid brightness value was present in DTS.

Quoted 6 lines
drivers/input/keyboard/cap11xx.c +6 -0
+
+	error = regmap_update_bits(priv->regmap,
+				CAP11XX_REG_LED_OUTPUT_CONTROL, 0xff, 0);
+	if (error)
+		return error;
+	error = regmap_update_bits(priv->regmap, CAP11XX_REG_LED_DUTY_CYCLE_4,

drivers/input/keyboard/cap11xx.c static void cap11xx_input_close(struct input_dev *idev) · line 305

Quoted 10 lines
drivers/input/keyboard/cap11xx.c +10 -0
+				CAP11XX_REG_LED_DUTY_MAX_MASK,
+				reg << CAP11XX_REG_LED_DUTY_MAX_MASK_SHIFT);
+	if (error)
+		return error;
+
+	for_each_child_of_node(node, child) {
+		led->cdev.name =
+			of_get_property(child, "label", NULL) ? : child->name;
+		led->cdev.default_trigger =
+			of_get_property(child, "linux,default-trigger", NULL);

Hmm, we do not have standard LEDs fwnode parser? Maybe we should have one.

drivers/input/keyboard/cap11xx.c static void cap11xx_input_close(struct input_dev *idev) · line 313

Quoted 8 lines
drivers/input/keyboard/cap11xx.c +8 -0
+		led->cdev.flags = 0;
+		led->cdev.brightness_set = cap11xx_led_set;
+		led->cdev.max_brightness = 1;
+		led->cdev.brightness = LED_OFF;
+
+		error = of_property_read_u32(child, "reg", &reg);
+		if (error != 0 || reg >= num_leds)
+			continue;

Why do we silently skip entries with invalid OF data instead of returning error?

Quoted 19 lines
drivers/input/keyboard/cap11xx.c +15 -0
+		led->reg = reg;
+		led->priv = priv;
+
+		INIT_WORK(&led->work, cap11xx_led_work);
+		error = devm_led_classdev_register(dev, &led->cdev);
+		if (error < 0)
+			return -EINVAL;
+		priv->num_leds++;
+		led++;
+	};
+
+	return 0;
+}
+#endif
+
 static int cap11xx_i2c_probe(struct i2c_client *i2c_client,
 			     const struct i2c_device_id *id)
 {

drivers/input/keyboard/cap11xx.c static int cap11xx_i2c_probe(struct i2c_client *i2c_client, · line 453

Quoted 8 lines
drivers/input/keyboard/cap11xx.c +5 -0
@@ -316,6 +446,11 @@ static int cap11xx_i2c_probe(struct i2c_client *i2c_client,
 	priv->idev->open = cap11xx_input_open;
 	priv->idev->close = cap11xx_input_close;
 
+#ifdef CONFIG_LEDS_CLASS
+	error = cap11xx_init_leds(dev, priv, cap->num_leds);
+	if (error)
+		return error;
+#endif

Please provide stub for cap11xx_init_leds so we do not need to have #ifdef here.

Quoted 6 lines
drivers/input/keyboard/cap11xx.c +0 -0
 	input_set_drvdata(priv->idev, priv);
 
 	/*
Signature · 1 line
1.9.1

Thanks.

Signature · 3 lines
Dmitry
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
#17 lore raw

On 06/22/2015 08:08 PM, Dmitry Torokhov wrote:
[...]

Quoted 9 lines

drivers/input/keyboard/cap11xx.c static void cap11xx_input_close(struct input_dev *idev) · line 305

Quoted 6 lines
drivers/input/keyboard/cap11xx.c +6 -0
+
+	for_each_child_of_node(node, child) {
+		led->cdev.name =
+			of_get_property(child, "label", NULL) ? : child->name;
+		led->cdev.default_trigger =
+			of_get_property(child, "linux,default-trigger", NULL);

Hmm, we do not have standard LEDs fwnode parser? Maybe we should have one.

We don't have one. That's something to be added.

Signature · 2 lines
Best Regards,
Jacek Anaszewski