cap11xx: Add LED support to driver
Subsystems (4)
v3 cover cap11xx: Add LED support to driver
Changes from v2:
* Use LED subsystem correctly, and dynamically allot used LEDs
* Add LED trigger support (e.g. heartbeat, mmc trigger, etc)
* Add DT properties for configuring LED usage, labels, and brightness
Matt Ranostay (2):
dt: add cap11xx LED documentation
cap11xx: add LED support
.../devicetree/bindings/input/cap11xx.txt | 23 ++++ drivers/input/keyboard/cap11xx.c | 121 ++++++++++++++++++++- 2 files changed, 141 insertions(+), 3 deletions(-)
Signature · 1 line
1.9.1
PATCH v3 1/2 dt: add cap11xx LED documentation
Some cap11xx devices have LEDs that can be controlled from userpace and via triggers. Document their use and functionality here.
--- .../devicetree/bindings/input/cap11xx.txt | 23 ++++++++++++++++++++++ 1 file changed, 23 insertions(+)
Documentation/devicetree/bindings/input/cap11xx.txt +23 -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 0-15. + By default a value of 15 is set. + + Example: i2c_controller {
@@ -55,5 +60,23 @@ i2c_controller { <105>, /* KEY_LEFT */ <109>, /* KEY_PAGEDOWN */ <104>; /* KEY_PAGEUP */ + + linux,led-brightness = <15>; + + 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
Re: [PATCH v3 1/2] dt: add cap11xx LED documentation
Hi Matt,
Quoted 13 lines · Matt Ranostay
Some cap11xx devices have LEDs that can be controlled from userpace and via triggers. Document their use and functionality here.
Signed-off-by Matt Ranostay <[email protected]>--- .../devicetree/bindings/input/cap11xx.txt | 23 ++++++++++++++++++++++ 1 file changed, 23 insertions(+)Documentation/devicetree/bindings/input/cap11xx.txt +0
-0
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 0-15. + By default a value of 15 is set.
The ON brightness is controlled with the 'brightness' sysfs property. The levels start from 1 and 0 turns the LED off. I think that this property is useless then.
Quoted 29 lines
Documentation/devicetree/bindings/input/cap11xx.txt +19
-0+ Example: i2c_controller {@@ -55,5 +60,23 @@ i2c_controller { <105>, /* KEY_LEFT */ <109>, /* KEY_PAGEDOWN */ <104>; /* KEY_PAGEUP */ + + linux,led-brightness = <15>; + + 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 · 2 lines
Best Regards, Jacek Anaszewski
A message that is not in the archive (sent off-list or not yet received): [email protected]
Re: [PATCH v3 1/2] dt: add cap11xx LED documentation
Quoted 22 lines · Jacek Anaszewski
Hi Matt,
Quoted 19 lines · Matt Ranostay
Some cap11xx devices have LEDs that can be controlled from userpace and via triggers. Document their use and functionality here.
Signed-off-by Matt Ranostay <[email protected]>--- .../devicetree/bindings/input/cap11xx.txt | 23 ++++++++++++++++++++++ 1 file changed, 23 insertions(+)Documentation/devicetree/bindings/input/cap11xx.txt +0
-0@@ -38,6 +38,11 @@ Optional properties: defaults. The array must have exactly six entries.
Quoted 11 lines
Quoted 5 lines
Documentation/devicetree/bindings/input/cap11xx.txt +1
-0+ linux,led-brightness: Defines the ON brightness when theoptional LED
+ functionality is used. Valid values are
0-15.
+ By default a value of 15 is set.The ON brightness is controlled with the 'brightness' sysfs property. The levels start from 1 and 0 turns the LED off. I think that this property is useless then.
Yes but this defines the brightness for the ON state.. since it can be from 0-100% duty cycle.
Quoted 26 lines
Quoted 26 lines
Documentation/devicetree/bindings/input/cap11xx.txt +18
-0+ Example: i2c_controller {@@ -55,5 +60,23 @@ i2c_controller { <105>, /* KEY_LEFT */ <109>, /* KEY_PAGEDOWN */ <104>; /* KEY_PAGEUP */ + + linux,led-brightness = <15>; + + 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";
Quoted 8 lines
Documentation/devicetree/bindings/input/cap11xx.txt i2c_controller { · line 82
Quoted 4 lines
Documentation/devicetree/bindings/input/cap11xx.txt +1
-0+ }; }; }--
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
Re: [PATCH v3 1/2] dt: add cap11xx LED documentation
Quoted 36 lines · Matt Ranostay
Quoted 22 lines · Jacek Anaszewski
Hi Matt,
Quoted 19 lines · Matt Ranostay
Some cap11xx devices have LEDs that can be controlled from userpace and via triggers. Document their use and functionality here.
Signed-off-by Matt Ranostay <[email protected]>--- .../devicetree/bindings/input/cap11xx.txt | 23 ++++++++++++++++++++++ 1 file changed, 23 insertions(+)Documentation/devicetree/bindings/input/cap11xx.txt +0
-0@@ -38,6 +38,11 @@ Optional properties: defaults. The array must have exactly six entries.Quoted 11 lines
Quoted 5 lines
Documentation/devicetree/bindings/input/cap11xx.txt +1
-0+ linux,led-brightness: Defines the ON brightness when theoptional LED
+ functionality is used. Valid values are
0-15.
+ By default a value of 15 is set.The ON brightness is controlled with the 'brightness' sysfs property. The levels start from 1 and 0 turns the LED off. I think that this property is useless then.
Quoted 2 lines
Yes but this defines the brightness for the ON state.. since it can be from 0-100% duty cycle.
Doesn't 0 mean that LED is off? If so, then valid values should start from 1.
Quoted 35 lines
Quoted 34 lines
Quoted 20 lines
Documentation/devicetree/bindings/input/cap11xx.txt +11
-0+ Example: i2c_controller {@@ -55,5 +60,23 @@ i2c_controller { <105>, /* KEY_LEFT */ <109>, /* KEY_PAGEDOWN */ <104>; /* KEY_PAGEUP */ + + linux,led-brightness = <15>; + + usr@0 { + label = "cap11xx:green:usr0"; + reg = <0>; + }; + + usr@1 { + label = "cap11xx:green:usr1";Documentation/devicetree/bindings/input/cap11xx.txt i2c_controller { · line 82
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"; + }; }; }--
Best Regards,
Jacek Anaszewski
Signature · 2 lines
Best Regards, Jacek Anaszewski
Re: [PATCH v3 1/2] dt: add cap11xx LED documentation
Quoted 42 lines · Jacek Anaszewski
Quoted 41 lines · Matt Ranostay
Quoted 37 lines · Jacek Anaszewski
Hi Matt,
Quoted 21 lines · Matt Ranostay
Some cap11xx devices have LEDs that can be controlled from userpace and via triggers. Document their use and functionality here.
Signed-off-by Matt Ranostay <[email protected]>--- .../devicetree/bindings/input/cap11xx.txt | 23 ++++++++++++++++++++++ 1 file changed, 23 insertions(+)Documentation/devicetree/bindings/input/cap11xx.txt +0
-0@@ -38,6 +38,11 @@ Optional properties: defaults. The array must have exactlysix
entries.Quoted 5 lines
+ linux,led-brightness: Defines the ON brightness when the
optional LED
+ functionality is used. Valid values are
0-15.
+ By default a value of 15 is set.The ON brightness is controlled with the 'brightness' sysfs property. The levels start from 1 and 0 turns the LED off. I think that this property is useless then.
Quoted 7 lines
Quoted 2 lines
Yes but this defines the brightness for the ON state.. since it can be from 0-100% duty cycle.
Doesn't 0 mean that LED is off? If so, then valid values should start from 1.
Ok makes sense since a 0% duty cycle is useless :)
Quoted 42 lines
Quoted 27 lines
Quoted 27 lines
Quoted 27 lines
Documentation/devicetree/bindings/input/cap11xx.txt +18
-0+ Example: i2c_controller {@@ -55,5 +60,23 @@ i2c_controller { <105>, /* KEY_LEFT */ <109>, /* KEY_PAGEDOWN */ <104>; /* KEY_PAGEUP */ + + linux,led-brightness = <15>; + + 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";Quoted 10 lines
Quoted 8 lines
Documentation/devicetree/bindings/input/cap11xx.txt i2c_controller { · line 82
Quoted 4 lines
Documentation/devicetree/bindings/input/cap11xx.txt +1
-0+ }; }; }--
Best Regards,
Jacek Anaszewski--
Best Regards,
Jacek Anaszewski
PATCH v3 2/2 cap11xx: add LED support
In reply to Matt RanostaySeveral cap11xx variants have LEDs that be can be controlled, this patchset implements this functionality.
--- drivers/input/keyboard/cap11xx.c | 121 ++++++++++++++++++++++++++++++++++++++- 1 file changed, 118 insertions(+), 3 deletions(-)
drivers/input/keyboard/cap11xx.c +118 -3
@@ -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 }, };
@@ -196,6 +227,84 @@ 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; + +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); + + led->new_brightness = value; +schedule_work (&led->work); +} + +static int cap11xx_init_leds (struct device *dev,struct cap11xx_priv *priv) +{ +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 (!priv->num_leds || !cnt) +return 0 ; +if (cnt > priv->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" , ®); +if (error || reg >CAP11XX_REG_LED_DUTY_MAX_VALUE ) + reg =CAP11XX_REG_LED_DUTY_MAX_VALUE ; + + 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" , ®); +if (error !=0 || reg >= priv->num_leds) +continue ; + led->reg = reg; + led->priv = priv; + + error =devm_led_classdev_register (dev, &led->cdev); +if (error <0 ) +return -EINVAL ; + +INIT_WORK (&led->work, cap11xx_led_work); +schedule_work (&led->work); + led++; + }; + +return 0 ; +} +#endif +static int cap11xx_i2c_probe (struct i2c_client *i2c_client,const struct i2c_device_id *id) {
@@ -316,6 +425,12 @@ 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 + priv->num_leds = cap->num_leds; + error =cap11xx_init_leds (dev, priv); +if (error) +return error; +#endif input_set_drvdata (priv->idev, priv);/*
Signature · 1 line
1.9.1
Re: [PATCH v3 2/2] cap11xx: add LED support
Hi Matt,
Quoted 98 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 | 121 ++++++++++++++++++++++++++++++++++++++- 1 file changed, 118 insertions(+), 3 deletions(-)drivers/input/keyboard/cap11xx.c +34
-3@@ -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 }, };@@ -196,6 +227,84 @@ static void cap11xx_input_close(struct input_dev *idev)cap11xx_set_sleep (priv, true); }
drivers/input/keyboard/cap11xx.c static void cap11xx_input_close(struct input_dev *idev) · line 238
Quoted 10 lines
drivers/input/keyboard/cap11xx.c +9
-0+#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; + +regmap_update_bits (priv->regmap,CAP11XX_REG_LED_OUTPUT_CONTROL , +BIT (led->reg), !!value ?BIT (led->reg) :0 );
You should set here the brightness value, not only turn the led on/off.
Quoted 23 lines
drivers/input/keyboard/cap11xx.c +23
-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); + + led->new_brightness = value; +schedule_work (&led->work); +} + +static int cap11xx_init_leds (struct device *dev,struct cap11xx_priv *priv) +{ +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 (!priv->num_leds || !cnt) +return 0 ; +if (cnt > priv->num_leds) +return -EINVAL ;
drivers/input/keyboard/cap11xx.c static void cap11xx_input_close(struct input_dev *idev) · line 271
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" , ®); +if (error || reg >CAP11XX_REG_LED_DUTY_MAX_VALUE ) + reg =CAP11XX_REG_LED_DUTY_MAX_VALUE ;
This property seems to be redundant, as brightness is controlled with 'brightness' sysfs property.
drivers/input/keyboard/cap11xx.c static void cap11xx_input_close(struct input_dev *idev) · line 275
Quoted 3 lines
drivers/input/keyboard/cap11xx.c +3
-0+ 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 );
This probably should be called from brightness_set op.
drivers/input/keyboard/cap11xx.c static void cap11xx_input_close(struct input_dev *idev) · line 284
Quoted 9 lines
drivers/input/keyboard/cap11xx.c +9
-0+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 ;
You don't need to clear flags, as the memory is zeroed by kcalloc.
drivers/input/keyboard/cap11xx.c static void cap11xx_input_close(struct input_dev *idev) · line 286
Quoted 2 lines
drivers/input/keyboard/cap11xx.c +2
-0+ led->cdev.brightness_set = cap11xx_led_set; + led->cdev.max_brightness =1 ;
You introduced linux,led-brightness DT property and defined its range to 0-15 (it should be 1-15 btw), but here you allow for brightness to be max 1.
drivers/input/keyboard/cap11xx.c static void cap11xx_input_close(struct input_dev *idev) · line 300
Quoted 13 lines
drivers/input/keyboard/cap11xx.c +13
-0+ led->cdev.brightness =LED_OFF ; + + error =of_property_read_u32 (child,"reg" , ®); +if (error !=0 || reg >= priv->num_leds) +continue ; + led->reg = reg; + led->priv = priv; + + error =devm_led_classdev_register (dev, &led->cdev); +if (error <0 ) +return -EINVAL ; +INIT_WORK (&led->work, cap11xx_led_work); +schedule_work (&led->work);
Work queue should be initialized before registration of the LED class device.
Quoted 24 lines
drivers/input/keyboard/cap11xx.c +13
-0+ led++; + }; + +return 0 ; +} +#endif +static int cap11xx_i2c_probe (struct i2c_client *i2c_client,const struct i2c_device_id *id) {@@ -316,6 +425,12 @@ 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 + priv->num_leds = cap->num_leds; + error =cap11xx_init_leds (dev, priv); +if (error) +return error; +#endif input_set_drvdata (priv->idev, priv);/*
Signature · 2 lines
Best Regards, Jacek Anaszewski
Re: [PATCH v3 2/2] cap11xx: add LED support
Quoted 111 lines · Jacek Anaszewski
Hi Matt,
Quoted 107 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 | 121 ++++++++++++++++++++++++++++++++++++++- 1 file changed, 118 insertions(+), 3 deletions(-)drivers/input/keyboard/cap11xx.c +33
-3@@ -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 +0
-0@@ -196,6 +227,84 @@ static void cap11xx_input_close(struct input_dev *idev)cap11xx_set_sleep (priv, true); }
Quoted 15 lines
drivers/input/keyboard/cap11xx.c static void cap11xx_input_close(struct input_dev *idev)
Quoted 11 lines
drivers/input/keyboard/cap11xx.c +9
-0+#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; + +regmap_update_bits (priv->regmap,CAP11XX_REG_LED_OUTPUT_CONTROL , +BIT (led->reg), !!value ?BIT (led->reg) :0 );You should set here the brightness value, not only turn the led on/off.
The issue is that the setting is across _ALL_ leds for brightness. But on/off state can clearly vary
Quoted 32 lines
Quoted 32 lines
drivers/input/keyboard/cap11xx.c +6
-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 +6
-0+ + led->new_brightness = value; +schedule_work (&led->work); +} + +static int cap11xx_init_leds(struct device *dev, struct cap11xx_priv*priv)
drivers/input/keyboard/cap11xx.c +17
-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 (!priv->num_leds || !cnt) +return 0 ; +if (cnt > priv->num_leds) +return -EINVAL ; + + led =devm_kcalloc (dev, cnt, +sizeof (struct cap11xx_led ),GFP_KERNEL ); +if (!led) +return -ENOMEM ; +
Quoted 8 lines
drivers/input/keyboard/cap11xx.c static void cap11xx_input_close(struct input_dev *idev) · line 271
Quoted 4 lines
drivers/input/keyboard/cap11xx.c +4
-0+ priv->leds = led; + error =of_property_read_u32 (node,"linux,led-brightness" , ®); +if (error || reg >CAP11XX_REG_LED_DUTY_MAX_VALUE ) + reg =CAP11XX_REG_LED_DUTY_MAX_VALUE ;This property seems to be redundant, as brightness is controlled with 'brightness' sysfs property.
See above comment.
Quoted 9 lines
Quoted 6 lines
+ 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);This probably should be called from
brightness_setop.
Above :)
Quoted 28 lines
Quoted 10 lines
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",Quoted 2 lines
NULL);
+led->cdev.flags= 0;You don't need to clear flags, as the memory is zeroed by kcalloc.
drivers/input/keyboard/cap11xx.c static void cap11xx_input_close(struct input_dev *idev) · line 286
Quoted 2 lines
drivers/input/keyboard/cap11xx.c +2
-0+ led->cdev.brightness_set = cap11xx_led_set; + led->cdev.max_brightness =1 ;You introduced linux,led-brightness DT property and defined its range to 0-15 (it should be 1-15 btw), but here you allow for brightness to be max 1.
Quoted 4 lines
drivers/input/keyboard/cap11xx.c +2
-0+ led->cdev.brightness =LED_OFF ; +
Quoted 15 lines
drivers/input/keyboard/cap11xx.c static void cap11xx_input_close(struct input_dev *idev) · line 300
Quoted 11 lines
drivers/input/keyboard/cap11xx.c +11
-0+ error =of_property_read_u32 (child,"reg" , ®); +if (error !=0 || reg >= priv->num_leds) +continue ; + led->reg = reg; + led->priv = priv; + + error =devm_led_classdev_register (dev, &led->cdev); +if (error <0 ) +return -EINVAL ; +INIT_WORK (&led->work, cap11xx_led_work); +schedule_work (&led->work);Work queue should be initialized before registration of the LED class device.
What is the reason for this? Just curious
Quoted 31 lines
Quoted 24 lines
drivers/input/keyboard/cap11xx.c +13
-0+ led++; + }; + +return 0 ; +} +#endif +static int cap11xx_i2c_probe (struct i2c_client *i2c_client,const struct i2c_device_id *id) {@@ -316,6 +425,12 @@ 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 + priv->num_leds = cap->num_leds; + error =cap11xx_init_leds (dev, priv); +if (error) +return error; +#endif input_set_drvdata (priv->idev, priv);Quoted 2 lines
/*--
Best Regards,
Jacek Anaszewski
Re: [PATCH v3 2/2] cap11xx: add LED support
Quoted 10 lines · Matt Ranostay
Quoted 7 lines · Jacek Anaszewski
Quoted 5 lines
drivers/input/keyboard/cap11xx.c +5
-0+ error =devm_led_classdev_register (dev, &led->cdev); +if (error <0 ) +return -EINVAL ; +INIT_WORK (&led->work, cap11xx_led_work); +schedule_work (&led->work);
Quoted 3 lines
Quoted 2 lines
Work queue should be initialized before registration of the LED class device.
What is the reason for this? Just curious
It's a race condition. The LED device is accessible once devm_led_classdev_register() returns. So a user could be calling into cap11xx_led_set() before the work queue is initialized, which would hence access a NULL pointer.
Daniel
Re: [PATCH v3 2/2] cap11xx: add LED support
Quoted 11 lines · Daniel Mack
Quoted 10 lines · Matt Ranostay
Quoted 7 lines · Jacek Anaszewski
Quoted 5 lines
drivers/input/keyboard/cap11xx.c +5
-0+ error =devm_led_classdev_register (dev, &led->cdev); +if (error <0 ) +return -EINVAL ; +INIT_WORK (&led->work, cap11xx_led_work); +schedule_work (&led->work);
Quoted 8 lines
Quoted 3 lines
Quoted 2 lines
Work queue should be initialized before registration of the LED class device.
What is the reason for this? Just curious
It's a race condition. The LED device is accessible once
devm_led_classdev_register()returns. So a user could be calling intocap11xx_led_set()before the work queue is initialized, which would hence access a NULL pointer.
Ah thanks for explaining. Will fix in v4.
Quoted 4 lines
Daniel
Re: [PATCH v3 2/2] cap11xx: add LED support
In reply to Matt RanostayQuoted 129 lines · Matt Ranostay
Quoted 111 lines · Jacek Anaszewski
Hi Matt,
Quoted 107 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 | 121 ++++++++++++++++++++++++++++++++++++++- 1 file changed, 118 insertions(+), 3 deletions(-)drivers/input/keyboard/cap11xx.c +33
-3@@ -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 +0
-0@@ -196,6 +227,84 @@ static void cap11xx_input_close(struct input_dev *idev)cap11xx_set_sleep (priv, true); }Quoted 15 lines
drivers/input/keyboard/cap11xx.c static void cap11xx_input_close(struct input_dev *idev)
Quoted 11 lines
drivers/input/keyboard/cap11xx.c +9
-0+#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; + +regmap_update_bits (priv->regmap,CAP11XX_REG_LED_OUTPUT_CONTROL , +BIT (led->reg), !!value ?BIT (led->reg) :0 );You should set here the brightness value, not only turn the led on/off.
Quoted 2 lines
The issue is that the setting is across _ALL_ leds for brightness. But on/off state can clearly vary
OK. The comment describing this would be nice here.
Quoted 96 lines
Quoted 32 lines
Quoted 32 lines
drivers/input/keyboard/cap11xx.c +6
-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 +6
-0+ + led->new_brightness = value; +schedule_work (&led->work); +} + +static int cap11xx_init_leds(struct device *dev, struct cap11xx_priv*priv)
drivers/input/keyboard/cap11xx.c +17
-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 (!priv->num_leds || !cnt) +return 0 ; +if (cnt > priv->num_leds) +return -EINVAL ; + + led =devm_kcalloc (dev, cnt, +sizeof (struct cap11xx_led ),GFP_KERNEL ); +if (!led) +return -ENOMEM ; +Quoted 8 lines
drivers/input/keyboard/cap11xx.c static void cap11xx_input_close(struct input_dev *idev) · line 271
Quoted 4 lines
drivers/input/keyboard/cap11xx.c +4
-0+ priv->leds = led; + error =of_property_read_u32 (node,"linux,led-brightness" , ®); +if (error || reg >CAP11XX_REG_LED_DUTY_MAX_VALUE ) + reg =CAP11XX_REG_LED_DUTY_MAX_VALUE ;This property seems to be redundant, as brightness is controlled with 'brightness' sysfs property.
See above comment.
Quoted 9 lines
Quoted 6 lines
+ 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);This probably should be called from
brightness_setop.Above :)
Quoted 28 lines
Quoted 10 lines
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",Quoted 2 lines
NULL);
+led->cdev.flags= 0;You don't need to clear flags, as the memory is zeroed by kcalloc.
drivers/input/keyboard/cap11xx.c static void cap11xx_input_close(struct input_dev *idev) · line 286
Quoted 2 lines
drivers/input/keyboard/cap11xx.c +2
-0+ led->cdev.brightness_set = cap11xx_led_set; + led->cdev.max_brightness =1 ;You introduced linux,led-brightness DT property and defined its range to 0-15 (it should be 1-15 btw), but here you allow for brightness to be max 1.
Quoted 4 lines
drivers/input/keyboard/cap11xx.c +2
-0+ led->cdev.brightness =LED_OFF ; +Quoted 15 lines
drivers/input/keyboard/cap11xx.c static void cap11xx_input_close(struct input_dev *idev) · line 300
Quoted 11 lines
drivers/input/keyboard/cap11xx.c +11
-0+ error =of_property_read_u32 (child,"reg" , ®); +if (error !=0 || reg >= priv->num_leds) +continue ; + led->reg = reg; + led->priv = priv; + + error =devm_led_classdev_register (dev, &led->cdev); +if (error <0 ) +return -EINVAL ; +INIT_WORK (&led->work, cap11xx_led_work); +schedule_work (&led->work);Work queue should be initialized before registration of the LED class device.
Quoted 1 line
What is the reason for this? Just curious
See Daniel's explanation.
Quoted 32 lines
Quoted 31 lines
Quoted 24 lines
drivers/input/keyboard/cap11xx.c +13
-0+ led++; + }; + +return 0 ; +} +#endif +static int cap11xx_i2c_probe (struct i2c_client *i2c_client,const struct i2c_device_id *id) {@@ -316,6 +425,12 @@ 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 + priv->num_leds = cap->num_leds; + error =cap11xx_init_leds (dev, priv); +if (error) +return error; +#endif input_set_drvdata (priv->idev, priv);Quoted 2 lines
/*--
Best Regards,
Jacek Anaszewski
Signature · 2 lines
Best Regards, Jacek Anaszewski
Re: [PATCH v3 2/2] cap11xx: add LED support
Quoted 138 lines · Jacek Anaszewski
Quoted 137 lines · Matt Ranostay
Quoted 133 lines · Jacek Anaszewski
Hi Matt,
Quoted 113 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 | 121 ++++++++++++++++++++++++++++++++++++++- 1 file changed, 118 insertions(+), 3 deletions(-)drivers/input/keyboard/cap11xx.c +31
-3@@ -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 +0
-0@@ -111,6 +141,7 @@ static const struct reg_defaultcap11xx_reg_defaults[]
= { { CAP11XX_REG_STANDBY_SENSITIVITY, 0x02 }, { CAP11XX_REG_STANDBY_THRESH, 0x40 }, { CAP11XX_REG_CONFIG2, 0x40 },drivers/input/keyboard/cap11xx.c +2
-0+ { CAP11XX_REG_LED_POLARITY, 0x00 }, { CAP11XX_REG_SENSOR_CALIB_LSB1, 0x00 }, { CAP11XX_REG_SENSOR_CALIB_LSB2, 0x00 }, };@@ -196,6 +227,84 @@ static void cap11xx_input_close(struct input_dev *idev)cap11xx_set_sleep (priv, true); } +#ifdef CONFIG_LEDS_CLASS drivers/input/keyboard/cap11xx.c static void cap11xx_input_close(struct input_dev · line 241
Quoted 10 lines
drivers/input/keyboard/cap11xx.c +8
-0+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; + +regmap_update_bits (priv->regmap,CAP11XX_REG_LED_OUTPUT_CONTROL , +BIT (led->reg), !!value ?BIT (led->reg) :0 );You should set here the brightness value, not only turn the led on/off.
Quoted 5 lines
Quoted 2 lines
The issue is that the setting is across _ALL_ leds for brightness. But on/off state can clearly vary
OK. The comment describing this would be nice here.
Ok will make this more clear in v4.
Thanks,
Matt
Quoted 154 lines
Quoted 110 lines
Quoted 36 lines
Quoted 36 lines
drivers/input/keyboard/cap11xx.c +6
-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 +6
-0+ + led->new_brightness = value; +schedule_work (&led->work); +} + +static int cap11xx_init_leds(struct device *dev, struct cap11xx_priv*priv)
drivers/input/keyboard/cap11xx.c +19
-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 (!priv->num_leds || !cnt) +return 0 ; +if (cnt > priv->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",Quoted 8 lines
drivers/input/keyboard/cap11xx.c static void cap11xx_input_close(struct input_dev *idev) · line 271
Quoted 3 lines
®);
drivers/input/keyboard/cap11xx.c +2
-0+if (error || reg >CAP11XX_REG_LED_DUTY_MAX_VALUE ) + reg =CAP11XX_REG_LED_DUTY_MAX_VALUE ;This property seems to be redundant, as brightness is controlled with 'brightness' sysfs property.
See above comment.
Quoted 11 lines
Quoted 7 lines
+ 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);This probably should be called from
brightness_setop.Above :)
Quoted 32 lines
Quoted 11 lines
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",Quoted 2 lines
NULL);
+led->cdev.flags= 0;You don't need to clear flags, as the memory is zeroed by kcalloc.
drivers/input/keyboard/cap11xx.c static void cap11xx_input_close(struct input_dev *idev) · line 286
Quoted 2 lines
drivers/input/keyboard/cap11xx.c +2
-0+ led->cdev.brightness_set = cap11xx_led_set; + led->cdev.max_brightness =1 ;You introduced linux,led-brightness DT property and defined its range to 0-15 (it should be 1-15 btw), but here you allow for brightness to be max 1.
Quoted 5 lines
drivers/input/keyboard/cap11xx.c +2
-0+ led->cdev.brightness =LED_OFF ; +Quoted 16 lines
drivers/input/keyboard/cap11xx.c static void cap11xx_input_close(struct input_dev *idev) · line 300
Quoted 11 lines
drivers/input/keyboard/cap11xx.c +11
-0+ error =of_property_read_u32 (child,"reg" , ®); +if (error !=0 || reg >= priv->num_leds) +continue ; + led->reg = reg; + led->priv = priv; + + error =devm_led_classdev_register (dev, &led->cdev); +if (error <0 ) +return -EINVAL ; +INIT_WORK (&led->work, cap11xx_led_work); +schedule_work (&led->work);Work queue should be initialized before registration of the LED class device.
Quoted 1 line
What is the reason for this? Just curious
See Daniel's explanation.
Quoted 24 lines
Quoted 24 lines
Quoted 24 lines
drivers/input/keyboard/cap11xx.c +13
-0+ led++; + }; + +return 0 ; +} +#endif +static int cap11xx_i2c_probe (struct i2c_client *i2c_client,const struct i2c_device_id *id) {@@ -316,6 +425,12 @@ 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 + priv->num_leds = cap->num_leds; + error =cap11xx_init_leds (dev, priv); +if (error) +return error; +#endif input_set_drvdata (priv->idev, priv);Quoted 9 lines
Quoted 7 lines
Quoted 2 lines
/*--
Best Regards,
Jacek Anaszewski--
Best Regards,
Jacek Anaszewski