[PATCH v5 0/2] cap11xx: add LED support

[PATCH v5 0/2] cap11xx: add LED support

[Matt Ranostay]

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(-)

-- 
1.9.1

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

[Matt Ranostay]

Signed-off-by: Matt Ranostay <mranostay@gmail.com>
---
 .../devicetree/bindings/input/cap11xx.txt          | 25 ++++++++++++++++++++++
 1 file changed, 25 insertions(+)

diff --git a/Documentation/devicetree/bindings/input/cap11xx.txt b/Documentation/devicetree/bindings/input/cap11xx.txt
index 7d0a300..09cdc43 100644
--- a/Documentation/devicetree/bindings/input/cap11xx.txt
+++ b/Documentation/devicetree/bindings/input/cap11xx.txt
@@ -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";
+		};
 	};
 }
-- 
1.9.1

[PATCH v5 2/2] cap11xx: add LED support

[Matt Ranostay]

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

Signed-off-by: Matt Ranostay <mranostay@gmail.com>
---
 drivers/input/keyboard/cap11xx.c | 141 ++++++++++++++++++++++++++++++++++++++-
 1 file changed, 138 insertions(+), 3 deletions(-)

diff --git a/drivers/input/keyboard/cap11xx.c b/drivers/input/keyboard/cap11xx.c
index f07461a..b48d72f 100644
--- a/drivers/input/keyboard/cap11xx.c
+++ b/drivers/input/keyboard/cap11xx.c
@@ -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);
 
 	/*
-- 
1.9.1

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

[Jacek Anaszewski]

Hi Matt,

On 06/18/2015 05:58 AM, Matt Ranostay wrote:
> Signed-off-by: Matt Ranostay <mranostay@gmail.com>
> ---
>   .../devicetree/bindings/input/cap11xx.txt          | 25 ++++++++++++++++++++++
>   1 file changed, 25 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/input/cap11xx.txt b/Documentation/devicetree/bindings/input/cap11xx.txt
> index 7d0a300..09cdc43 100644
> --- a/Documentation/devicetree/bindings/input/cap11xx.txt
> +++ b/Documentation/devicetree/bindings/input/cap11xx.txt
> @@ -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";
> +		};
>   	};
>   }
>

Acked-by: Jacek Anaszewski <j.anaszewski@samsung.com>

-- 
Best Regards,
Jacek Anaszewski

Re: [PATCH v5 2/2] cap11xx: add LED support

[Jacek Anaszewski]

Hi Matt,

On 06/18/2015 05:58 AM, Matt Ranostay wrote:
> Several cap11xx variants have LEDs that be can be controlled, this
> patchset implements this functionality.
>
> Signed-off-by: Matt Ranostay <mranostay@gmail.com>
> ---
>   drivers/input/keyboard/cap11xx.c | 141 ++++++++++++++++++++++++++++++++++++++-
>   1 file changed, 138 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/input/keyboard/cap11xx.c b/drivers/input/keyboard/cap11xx.c
> index f07461a..b48d72f 100644
> --- a/drivers/input/keyboard/cap11xx.c
> +++ b/drivers/input/keyboard/cap11xx.c
> @@ -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;

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

Acked-by: Jacek Anaszewski <j.anaszewski@samsung.com>

> +		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);
>
>   	/*
>


-- 
Best Regards,
Jacek Anaszewski

Re: [PATCH v5 2/2] cap11xx: add LED support

[Matt Ranostay]

On Wed, Jun 17, 2015 at 11:51 PM, Jacek Anaszewski
<j.anaszewski@samsung.com> wrote:
> Hi Matt,
>
>
> On 06/18/2015 05:58 AM, Matt Ranostay wrote:
>>
>> Several cap11xx variants have LEDs that be can be controlled, this
>> patchset implements this functionality.
>>
>> Signed-off-by: Matt Ranostay <mranostay@gmail.com>
>> ---
>>   drivers/input/keyboard/cap11xx.c | 141
>> ++++++++++++++++++++++++++++++++++++++-
>>   1 file changed, 138 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/input/keyboard/cap11xx.c
>> b/drivers/input/keyboard/cap11xx.c
>> index f07461a..b48d72f 100644
>> --- a/drivers/input/keyboard/cap11xx.c
>> +++ b/drivers/input/keyboard/cap11xx.c
>> @@ -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;
>
>
> I would add empty line here and after INIT_WORK.
> With this addressed:
>
> Acked-by: Jacek Anaszewski <j.anaszewski@samsung.com>

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

>
>> +               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);
>>
>>         /*
>>
>
>
> --
> Best Regards,
> Jacek Anaszewski
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in

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

[Dmitry Torokhov]

On Wed, Jun 17, 2015 at 08:58:16PM -0700, Matt Ranostay wrote:
> Signed-off-by: Matt Ranostay <mranostay@gmail.com>
> ---
>  .../devicetree/bindings/input/cap11xx.txt          | 25 ++++++++++++++++++++++
>  1 file changed, 25 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/input/cap11xx.txt b/Documentation/devicetree/bindings/input/cap11xx.txt
> index 7d0a300..09cdc43 100644
> --- a/Documentation/devicetree/bindings/input/cap11xx.txt
> +++ b/Documentation/devicetree/bindings/input/cap11xx.txt
> @@ -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.

> +
> +
>  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";
> +		};
>  	};
>  }
> -- 
> 1.9.1
> 

Thanks.

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

Re: [PATCH v5 2/2] cap11xx: add LED support

[Dmitry Torokhov]

On Wed, Jun 17, 2015 at 08:58:17PM -0700, Matt Ranostay wrote:
> Several cap11xx variants have LEDs that be can be controlled, this
> patchset implements this functionality.
> 
> Signed-off-by: Matt Ranostay <mranostay@gmail.com>
> ---
>  drivers/input/keyboard/cap11xx.c | 141 ++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 138 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/input/keyboard/cap11xx.c b/drivers/input/keyboard/cap11xx.c
> index f07461a..b48d72f 100644
> --- a/drivers/input/keyboard/cap11xx.c
> +++ b/drivers/input/keyboard/cap11xx.c
> @@ -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

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.

>  	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;

An error message might be nice here.

> +
> +	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.

> +
> +	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);

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

> +		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?

> +		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

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

>  	input_set_drvdata(priv->idev, priv);
>  
>  	/*
> -- 
> 1.9.1
> 

Thanks.

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

Re: [PATCH v5 2/2] cap11xx: add LED support

[Jacek Anaszewski]

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

>> +
>> +	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.

-- 
Best Regards,
Jacek Anaszewski

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

[Jacek Anaszewski]

On 06/22/2015 07:59 PM, Dmitry Torokhov wrote:
> On Wed, Jun 17, 2015 at 08:58:16PM -0700, Matt Ranostay wrote:
>> Signed-off-by: Matt Ranostay <mranostay@gmail.com>
>> ---
>>   .../devicetree/bindings/input/cap11xx.txt          | 25 ++++++++++++++++++++++
>>   1 file changed, 25 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/input/cap11xx.txt b/Documentation/devicetree/bindings/input/cap11xx.txt
>> index 7d0a300..09cdc43 100644
>> --- a/Documentation/devicetree/bindings/input/cap11xx.txt
>> +++ b/Documentation/devicetree/bindings/input/cap11xx.txt
>> @@ -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.

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.

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


-- 
Best Regards,
Jacek Anaszewski

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

[Matt Ranostay]

On Tue, Jun 23, 2015 at 1:36 AM, Jacek Anaszewski
<j.anaszewski-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org> wrote:
> On 06/22/2015 07:59 PM, Dmitry Torokhov wrote:
>>
>> On Wed, Jun 17, 2015 at 08:58:16PM -0700, Matt Ranostay wrote:
>>>
>>> Signed-off-by: Matt Ranostay <mranostay-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
>>> ---
>>>   .../devicetree/bindings/input/cap11xx.txt          | 25
>>> ++++++++++++++++++++++
>>>   1 file changed, 25 insertions(+)
>>>
>>> diff --git a/Documentation/devicetree/bindings/input/cap11xx.txt
>>> b/Documentation/devicetree/bindings/input/cap11xx.txt
>>> index 7d0a300..09cdc43 100644
>>> --- a/Documentation/devicetree/bindings/input/cap11xx.txt
>>> +++ b/Documentation/devicetree/bindings/input/cap11xx.txt
>>> @@ -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.
>
>
> 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?

>> 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 majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Dmitry Torokhov]

On Tue, Jun 23, 2015 at 10:23:57AM -0700, Matt Ranostay wrote:
> On Tue, Jun 23, 2015 at 1:36 AM, Jacek Anaszewski
> <j.anaszewski@samsung.com> wrote:
> > On 06/22/2015 07:59 PM, Dmitry Torokhov wrote:
> >>
> >> On Wed, Jun 17, 2015 at 08:58:16PM -0700, Matt Ranostay wrote:
> >>>
> >>> Signed-off-by: Matt Ranostay <mranostay@gmail.com>
> >>> ---
> >>>   .../devicetree/bindings/input/cap11xx.txt          | 25
> >>> ++++++++++++++++++++++
> >>>   1 file changed, 25 insertions(+)
> >>>
> >>> diff --git a/Documentation/devicetree/bindings/input/cap11xx.txt
> >>> b/Documentation/devicetree/bindings/input/cap11xx.txt
> >>> index 7d0a300..09cdc43 100644
> >>> --- a/Documentation/devicetree/bindings/input/cap11xx.txt
> >>> +++ b/Documentation/devicetree/bindings/input/cap11xx.txt
> >>> @@ -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.
> >
> >
> > 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?

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.

-- 
Dmitry

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

[Matt Ranostay]

On Tue, Jun 23, 2015 at 11:07 AM, Dmitry Torokhov
<dmitry.torokhov@gmail.com> wrote:
> On Tue, Jun 23, 2015 at 10:23:57AM -0700, Matt Ranostay wrote:
>> On Tue, Jun 23, 2015 at 1:36 AM, Jacek Anaszewski
>> <j.anaszewski@samsung.com> wrote:
>> > On 06/22/2015 07:59 PM, Dmitry Torokhov wrote:
>> >>
>> >> On Wed, Jun 17, 2015 at 08:58:16PM -0700, Matt Ranostay wrote:
>> >>>
>> >>> Signed-off-by: Matt Ranostay <mranostay@gmail.com>
>> >>> ---
>> >>>   .../devicetree/bindings/input/cap11xx.txt          | 25
>> >>> ++++++++++++++++++++++
>> >>>   1 file changed, 25 insertions(+)
>> >>>
>> >>> diff --git a/Documentation/devicetree/bindings/input/cap11xx.txt
>> >>> b/Documentation/devicetree/bindings/input/cap11xx.txt
>> >>> index 7d0a300..09cdc43 100644
>> >>> --- a/Documentation/devicetree/bindings/input/cap11xx.txt
>> >>> +++ b/Documentation/devicetree/bindings/input/cap11xx.txt
>> >>> @@ -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.
>> >
>> >
>> > 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?
>
> 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?

>
> Thanks.
>
> --
> Dmitry

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

[Dmitry Torokhov]

On Tue, Jun 23, 2015 at 11:24:42AM -0700, Matt Ranostay wrote:
> On Tue, Jun 23, 2015 at 11:07 AM, Dmitry Torokhov
> <dmitry.torokhov-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> > On Tue, Jun 23, 2015 at 10:23:57AM -0700, Matt Ranostay wrote:
> >> On Tue, Jun 23, 2015 at 1:36 AM, Jacek Anaszewski
> >> <j.anaszewski-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org> wrote:
> >> > On 06/22/2015 07:59 PM, Dmitry Torokhov wrote:
> >> >>
> >> >> On Wed, Jun 17, 2015 at 08:58:16PM -0700, Matt Ranostay wrote:
> >> >>>
> >> >>> Signed-off-by: Matt Ranostay <mranostay-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> >> >>> ---
> >> >>>   .../devicetree/bindings/input/cap11xx.txt          | 25
> >> >>> ++++++++++++++++++++++
> >> >>>   1 file changed, 25 insertions(+)
> >> >>>
> >> >>> diff --git a/Documentation/devicetree/bindings/input/cap11xx.txt
> >> >>> b/Documentation/devicetree/bindings/input/cap11xx.txt
> >> >>> index 7d0a300..09cdc43 100644
> >> >>> --- a/Documentation/devicetree/bindings/input/cap11xx.txt
> >> >>> +++ b/Documentation/devicetree/bindings/input/cap11xx.txt
> >> >>> @@ -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.
> >> >
> >> >
> >> > 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?
> >
> > 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.

-- 
Dmitry
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Jacek Anaszewski]

On 06/23/2015 08:07 PM, Dmitry Torokhov wrote:
> On Tue, Jun 23, 2015 at 10:23:57AM -0700, Matt Ranostay wrote:
>> On Tue, Jun 23, 2015 at 1:36 AM, Jacek Anaszewski
>> <j.anaszewski@samsung.com> wrote:
>>> On 06/22/2015 07:59 PM, Dmitry Torokhov wrote:
>>>>
>>>> On Wed, Jun 17, 2015 at 08:58:16PM -0700, Matt Ranostay wrote:
>>>>>
>>>>> Signed-off-by: Matt Ranostay <mranostay@gmail.com>
>>>>> ---
>>>>>    .../devicetree/bindings/input/cap11xx.txt          | 25
>>>>> ++++++++++++++++++++++
>>>>>    1 file changed, 25 insertions(+)
>>>>>
>>>>> diff --git a/Documentation/devicetree/bindings/input/cap11xx.txt
>>>>> b/Documentation/devicetree/bindings/input/cap11xx.txt
>>>>> index 7d0a300..09cdc43 100644
>>>>> --- a/Documentation/devicetree/bindings/input/cap11xx.txt
>>>>> +++ b/Documentation/devicetree/bindings/input/cap11xx.txt
>>>>> @@ -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.
>>>
>>>
>>> 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?

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

> 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.

-- 
Best Regards,
Jacek Anaszewski

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

[Jacek Anaszewski]

On 06/24/2015 09:28 AM, Jacek Anaszewski wrote:
> On 06/23/2015 08:07 PM, Dmitry Torokhov wrote:
>> On Tue, Jun 23, 2015 at 10:23:57AM -0700, Matt Ranostay wrote:
>>> On Tue, Jun 23, 2015 at 1:36 AM, Jacek Anaszewski
>>> <j.anaszewski@samsung.com> wrote:
>>>> On 06/22/2015 07:59 PM, Dmitry Torokhov wrote:
>>>>>
>>>>> On Wed, Jun 17, 2015 at 08:58:16PM -0700, Matt Ranostay wrote:
>>>>>>
>>>>>> Signed-off-by: Matt Ranostay <mranostay@gmail.com>
>>>>>> ---
>>>>>>    .../devicetree/bindings/input/cap11xx.txt          | 25
>>>>>> ++++++++++++++++++++++
>>>>>>    1 file changed, 25 insertions(+)
>>>>>>
>>>>>> diff --git a/Documentation/devicetree/bindings/input/cap11xx.txt
>>>>>> b/Documentation/devicetree/bindings/input/cap11xx.txt
>>>>>> index 7d0a300..09cdc43 100644
>>>>>> --- a/Documentation/devicetree/bindings/input/cap11xx.txt
>>>>>> +++ b/Documentation/devicetree/bindings/input/cap11xx.txt
>>>>>> @@ -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.
>>>>
>>>>
>>>> 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?
>
> 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.

-- 
Best Regards,
Jacek Anaszewski