[PATCH 0/5] Fix various issues with leds-pwm

[PATCH 0/5] Fix various issues with leds-pwm

[Russell King - ARM Linux]

This patch series fixes various problems with leds-pwm.

The first patch is very important to solve a boot time oops on OMAP
boards (and probably others) which has recently shown its face.

The next two clean up leds-pwm so that we can avoid the madness of
having entirely separate initialisation paths for DT and non-DT, which
I believe is the core cause of the initial problem.

The last two patches add support for LEDs connected in the opposite
sense to PWM outputs, where either the PWM does not support inversion,
or does not support it in a way that works sanely with leds-pwm.  These
add a new property to leds-pwm, hence why that patch (and this cover)
copies the DT people.

The first patch is an absolute must for going.

 .../devicetree/bindings/leds/leds-pwm.txt          |   2 +
 drivers/leds/leds-pwm.c                            | 161 ++++++++++-----------
 2 files changed, 82 insertions(+), 81 deletions(-)

-- 
FTTC broadband for 0.8mile line: now at 9.7Mbps down 460kbps up... slowly
improving, and getting towards what was expected from it.

[PATCH 1/5] leds: leds-pwm: properly clean up after probe failure

[Russell King]

When probing with DT, we add each LED one at a time.  If we find a LED
without a PWM device (because it is not available yet) we fail the
initialisation, unregister previous LEDs, and then by way of managed
resources, we free the structure.

The problem with this is we may have a scheduled and active work_struct
in this structure, and this results in a nasty kernel oops.

We need to cancel this work_struct properly upon cleanup - and the
cleanup we require is the same cleanup as we do when the LED platform
device is removed.  Rather than writing this same code three times,
move it into a separate function and use it in all three places.

Fixes: c971ff185f64 ("leds: leds-pwm: Defer led_pwm_set() if PWM can sleep")
Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk>
---
 drivers/leds/leds-pwm.c | 23 +++++++++++++----------
 1 file changed, 13 insertions(+), 10 deletions(-)

diff --git a/drivers/leds/leds-pwm.c b/drivers/leds/leds-pwm.c
index 605047428b5a..a7b369fc3554 100644
--- a/drivers/leds/leds-pwm.c
+++ b/drivers/leds/leds-pwm.c
@@ -84,6 +84,15 @@ static inline size_t sizeof_pwm_leds_priv(int num_leds)
 		      (sizeof(struct led_pwm_data) * num_leds);
 }
 
+static void led_pwm_cleanup(struct led_pwm_priv *priv)
+{
+	while (priv->num_leds--) {
+		led_classdev_unregister(&priv->leds[priv->num_leds].cdev);
+		if (priv->leds[priv->num_leds].can_sleep)
+			cancel_work_sync(&priv->leds[priv->num_leds].work);
+	}
+}
+
 static int led_pwm_create_of(struct platform_device *pdev,
 			     struct led_pwm_priv *priv)
 {
@@ -131,8 +140,7 @@ static int led_pwm_create_of(struct platform_device *pdev,
 
 	return 0;
 err:
-	while (priv->num_leds--)
-		led_classdev_unregister(&priv->leds[priv->num_leds].cdev);
+	led_pwm_cleanup(priv);
 
 	return ret;
 }
@@ -200,8 +208,8 @@ static int led_pwm_probe(struct platform_device *pdev)
 	return 0;
 
 err:
-	while (i--)
-		led_classdev_unregister(&priv->leds[i].cdev);
+	priv->num_leds = i;
+	led_pwm_cleanup(priv);
 
 	return ret;
 }
@@ -209,13 +217,8 @@ static int led_pwm_probe(struct platform_device *pdev)
 static int led_pwm_remove(struct platform_device *pdev)
 {
 	struct led_pwm_priv *priv = platform_get_drvdata(pdev);
-	int i;
 
-	for (i = 0; i < priv->num_leds; i++) {
-		led_classdev_unregister(&priv->leds[i].cdev);
-		if (priv->leds[i].can_sleep)
-			cancel_work_sync(&priv->leds[i].work);
-	}
+	led_pwm_cleanup(priv);
 
 	return 0;
 }
-- 
1.8.3.1

[PATCH 2/5] leds: leds-pwm: provide a common function to setup a single led-pwm device

[Russell King]

Provide a common function to setup a single led-pwm device, replacing
the platform data initialisation path with this function.  This allows
us to have a common method of creating these devices in a consistent
manner, which then allows us to place the probe failure cleanup in one
place.

Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk>
---
 drivers/leds/leds-pwm.c | 85 ++++++++++++++++++++++++++-----------------------
 1 file changed, 46 insertions(+), 39 deletions(-)

diff --git a/drivers/leds/leds-pwm.c b/drivers/leds/leds-pwm.c
index a7b369fc3554..2e6b6c5ca0e9 100644
--- a/drivers/leds/leds-pwm.c
+++ b/drivers/leds/leds-pwm.c
@@ -93,6 +93,44 @@ static void led_pwm_cleanup(struct led_pwm_priv *priv)
 	}
 }
 
+static int led_pwm_add(struct device *dev, struct led_pwm_priv *priv,
+		       struct led_pwm *led)
+{
+	struct led_pwm_data *led_data = &priv->leds[priv->num_leds];
+	int ret;
+
+	led_data->active_low = led->active_low;
+	led_data->period = led->pwm_period_ns;
+	led_data->cdev.name = led->name;
+	led_data->cdev.default_trigger = led->default_trigger;
+	led_data->cdev.brightness_set = led_pwm_set;
+	led_data->cdev.brightness = LED_OFF;
+	led_data->cdev.max_brightness = led->max_brightness;
+	led_data->cdev.flags = LED_CORE_SUSPENDRESUME;
+
+	led_data->pwm = devm_pwm_get(dev, led->name);
+	if (IS_ERR(led_data->pwm)) {
+		ret = PTR_ERR(led_data->pwm);
+		dev_err(dev, "unable to request PWM for %s: %d\n",
+			led->name, ret);
+		return ret;
+	}
+
+	led_data->can_sleep = pwm_can_sleep(led_data->pwm);
+	if (led_data->can_sleep)
+		INIT_WORK(&led_data->work, led_pwm_work);
+
+	ret = led_classdev_register(dev, &led_data->cdev);
+	if (ret == 0) {
+		priv->num_leds++;
+	} else {
+		dev_err(dev, "failed to register PWM led for %s: %d\n",
+			led->name, ret);
+	}
+
+	return ret;
+}
+
 static int led_pwm_create_of(struct platform_device *pdev,
 			     struct led_pwm_priv *priv)
 {
@@ -140,8 +178,6 @@ static int led_pwm_create_of(struct platform_device *pdev,
 
 	return 0;
 err:
-	led_pwm_cleanup(priv);
-
 	return ret;
 }
 
@@ -167,51 +203,22 @@ static int led_pwm_probe(struct platform_device *pdev)
 
 	if (pdata) {
 		for (i = 0; i < count; i++) {
-			struct led_pwm *cur_led = &pdata->leds[i];
-			struct led_pwm_data *led_dat = &priv->leds[i];
-
-			led_dat->pwm = devm_pwm_get(&pdev->dev, cur_led->name);
-			if (IS_ERR(led_dat->pwm)) {
-				ret = PTR_ERR(led_dat->pwm);
-				dev_err(&pdev->dev,
-					"unable to request PWM for %s\n",
-					cur_led->name);
-				goto err;
-			}
-
-			led_dat->cdev.name = cur_led->name;
-			led_dat->cdev.default_trigger = cur_led->default_trigger;
-			led_dat->active_low = cur_led->active_low;
-			led_dat->period = cur_led->pwm_period_ns;
-			led_dat->cdev.brightness_set = led_pwm_set;
-			led_dat->cdev.brightness = LED_OFF;
-			led_dat->cdev.max_brightness = cur_led->max_brightness;
-			led_dat->cdev.flags |= LED_CORE_SUSPENDRESUME;
-
-			led_dat->can_sleep = pwm_can_sleep(led_dat->pwm);
-			if (led_dat->can_sleep)
-				INIT_WORK(&led_dat->work, led_pwm_work);
-
-			ret = led_classdev_register(&pdev->dev, &led_dat->cdev);
-			if (ret < 0)
-				goto err;
+			ret = led_pwm_add(&pdev->dev, priv, &pdata->leds[i]);
+			if (ret)
+				break;
 		}
-		priv->num_leds = count;
 	} else {
 		ret = led_pwm_create_of(pdev, priv);
-		if (ret)
-			return ret;
+	}
+
+	if (ret) {
+		led_pwm_cleanup(priv);
+		return ret;
 	}
 
 	platform_set_drvdata(pdev, priv);
 
 	return 0;
-
-err:
-	priv->num_leds = i;
-	led_pwm_cleanup(priv);
-
-	return ret;
 }
 
 static int led_pwm_remove(struct platform_device *pdev)
-- 
1.8.3.1

[PATCH 3/5] leds: leds-pwm: convert OF parsing code to use led_pwm_add()

[Russell King]

Convert the OF parsing code to use the common PWM LED registration code,
which means we have a consistent method, and single point where the
registration happens for both paths.

Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk>
---
 drivers/leds/leds-pwm.c | 62 ++++++++++++++++++-------------------------------
 1 file changed, 23 insertions(+), 39 deletions(-)

diff --git a/drivers/leds/leds-pwm.c b/drivers/leds/leds-pwm.c
index 2e6b6c5ca0e9..e1b4c23a409a 100644
--- a/drivers/leds/leds-pwm.c
+++ b/drivers/leds/leds-pwm.c
@@ -94,7 +94,7 @@ static void led_pwm_cleanup(struct led_pwm_priv *priv)
 }
 
 static int led_pwm_add(struct device *dev, struct led_pwm_priv *priv,
-		       struct led_pwm *led)
+		       struct led_pwm *led, struct device_node *child)
 {
 	struct led_pwm_data *led_data = &priv->leds[priv->num_leds];
 	int ret;
@@ -108,7 +108,10 @@ static int led_pwm_add(struct device *dev, struct led_pwm_priv *priv,
 	led_data->cdev.max_brightness = led->max_brightness;
 	led_data->cdev.flags = LED_CORE_SUSPENDRESUME;
 
-	led_data->pwm = devm_pwm_get(dev, led->name);
+	if (child)
+		led_data->pwm = devm_of_pwm_get(dev, child, NULL);
+	else
+		led_data->pwm = devm_pwm_get(dev, led->name);
 	if (IS_ERR(led_data->pwm)) {
 		ret = PTR_ERR(led_data->pwm);
 		dev_err(dev, "unable to request PWM for %s: %d\n",
@@ -116,6 +119,9 @@ static int led_pwm_add(struct device *dev, struct led_pwm_priv *priv,
 		return ret;
 	}
 
+	if (child)
+		led_data->period = pwm_get_period(led_data->pwm);
+
 	led_data->can_sleep = pwm_can_sleep(led_data->pwm);
 	if (led_data->can_sleep)
 		INIT_WORK(&led_data->work, led_pwm_work);
@@ -131,53 +137,30 @@ static int led_pwm_add(struct device *dev, struct led_pwm_priv *priv,
 	return ret;
 }
 
-static int led_pwm_create_of(struct platform_device *pdev,
-			     struct led_pwm_priv *priv)
+static int led_pwm_create_of(struct device *dev, struct led_pwm_priv *priv)
 {
 	struct device_node *child;
-	int ret;
-
-	for_each_child_of_node(pdev->dev.of_node, child) {
-		struct led_pwm_data *led_dat = &priv->leds[priv->num_leds];
+	struct led_pwm led;
+	int ret = 0;
 
-		led_dat->cdev.name = of_get_property(child, "label",
-						     NULL) ? : child->name;
+	memset(&led, 0, sizeof(led));
 
-		led_dat->pwm = devm_of_pwm_get(&pdev->dev, child, NULL);
-		if (IS_ERR(led_dat->pwm)) {
-			dev_err(&pdev->dev, "unable to request PWM for %s\n",
-				led_dat->cdev.name);
-			ret = PTR_ERR(led_dat->pwm);
-			goto err;
-		}
-		/* Get the period from PWM core when n*/
-		led_dat->period = pwm_get_period(led_dat->pwm);
+	for_each_child_of_node(dev->of_node, child) {
+		led.name = of_get_property(child, "label", NULL) ? :
+			   child->name;
 
-		led_dat->cdev.default_trigger = of_get_property(child,
+		led.default_trigger = of_get_property(child,
 						"linux,default-trigger", NULL);
 		of_property_read_u32(child, "max-brightness",
-				     &led_dat->cdev.max_brightness);
-
-		led_dat->cdev.brightness_set = led_pwm_set;
-		led_dat->cdev.brightness = LED_OFF;
-		led_dat->cdev.flags |= LED_CORE_SUSPENDRESUME;
-
-		led_dat->can_sleep = pwm_can_sleep(led_dat->pwm);
-		if (led_dat->can_sleep)
-			INIT_WORK(&led_dat->work, led_pwm_work);
+				     &led.max_brightness);
 
-		ret = led_classdev_register(&pdev->dev, &led_dat->cdev);
-		if (ret < 0) {
-			dev_err(&pdev->dev, "failed to register for %s\n",
-				led_dat->cdev.name);
+		ret = led_pwm_add(dev, priv, &led, child);
+		if (ret) {
 			of_node_put(child);
-			goto err;
+			break;
 		}
-		priv->num_leds++;
 	}
 
-	return 0;
-err:
 	return ret;
 }
 
@@ -203,12 +186,13 @@ static int led_pwm_probe(struct platform_device *pdev)
 
 	if (pdata) {
 		for (i = 0; i < count; i++) {
-			ret = led_pwm_add(&pdev->dev, priv, &pdata->leds[i]);
+			ret = led_pwm_add(&pdev->dev, priv, &pdata->leds[i],
+					  NULL);
 			if (ret)
 				break;
 		}
 	} else {
-		ret = led_pwm_create_of(pdev, priv);
+		ret = led_pwm_create_of(&pdev->dev, priv);
 	}
 
 	if (ret) {
-- 
1.8.3.1

[PATCH 4/5] leds: leds-pwm: implement PWM inversion

[Russell King]

Some PWM outputs are wired such that the LED they're controlling is
connected to supply rather than ground.  These PWMs may not support
output inversion, or when they do, disabling the PWM may set the
PWM output low, causing a "brightness" value of zero to turn the LED
fully on.

The platform data for this driver already indicates that this was
thought about, and we have the "active_low" property there already.
However, the implementation for this is missing.

Add the trivial implementation for this feature.

Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk>
---
 drivers/leds/leds-pwm.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/leds/leds-pwm.c b/drivers/leds/leds-pwm.c
index e1b4c23a409a..1d47742c551f 100644
--- a/drivers/leds/leds-pwm.c
+++ b/drivers/leds/leds-pwm.c
@@ -70,6 +70,10 @@ static void led_pwm_set(struct led_classdev *led_cdev,
 
 	duty *= brightness;
 	do_div(duty, max);
+
+	if (led_dat->active_low)
+		duty = led_dat->period - duty;
+
 	led_dat->duty = duty;
 
 	if (led_dat->can_sleep)
-- 
1.8.3.1

[PATCH 5/5] leds: leds-pwm: add DT support for LEDs wired to supply

[Russell King]

The non-DT driver allowed an active low property to be specified, but DT
is missing this in its description.  Add the property to the DT binding
document, making it optional.  It defaults to active high, which retains
compatibility with existing descriptions.

This should only be used for causes where the LED is wired to supply,
and the PWM does not sensibly support its own inversion.

Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk>
---
 Documentation/devicetree/bindings/leds/leds-pwm.txt | 2 ++
 drivers/leds/leds-pwm.c                             | 1 +
 2 files changed, 3 insertions(+)

diff --git a/Documentation/devicetree/bindings/leds/leds-pwm.txt b/Documentation/devicetree/bindings/leds/leds-pwm.txt
index 7297107cf832..6c6583c35f2f 100644
--- a/Documentation/devicetree/bindings/leds/leds-pwm.txt
+++ b/Documentation/devicetree/bindings/leds/leds-pwm.txt
@@ -13,6 +13,8 @@ node's name represents the name of the corresponding LED.
   For the pwms and pwm-names property please refer to:
   Documentation/devicetree/bindings/pwm/pwm.txt
 - max-brightness : Maximum brightness possible for the LED
+- active-low : (optional) For PWMs where the LED is wired to supply
+  rather than ground.
 - label :  (optional)
   see Documentation/devicetree/bindings/leds/common.txt
 - linux,default-trigger :  (optional)
diff --git a/drivers/leds/leds-pwm.c b/drivers/leds/leds-pwm.c
index 1d47742c551f..fb1eafe09324 100644
--- a/drivers/leds/leds-pwm.c
+++ b/drivers/leds/leds-pwm.c
@@ -155,6 +155,7 @@ static int led_pwm_create_of(struct device *dev, struct led_pwm_priv *priv)
 
 		led.default_trigger = of_get_property(child,
 						"linux,default-trigger", NULL);
+		led.active_low = of_property_read_bool(child, "active-low");
 		of_property_read_u32(child, "max-brightness",
 				     &led.max_brightness);
 
-- 
1.8.3.1

Re: [PATCH 4/5] leds: leds-pwm: implement PWM inversion

[Alexandre Belloni]

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

(adding Thierry Reding)

Hi,

On 06/04/2014 at 23:20:18 +0100, Russell King wrote :
> Some PWM outputs are wired such that the LED they're controlling is
> connected to supply rather than ground.  These PWMs may not support
> output inversion, or when they do, disabling the PWM may set the
> PWM output low, causing a "brightness" value of zero to turn the LED
> fully on.
> 
> The platform data for this driver already indicates that this was
> thought about, and we have the "active_low" property there already.
> However, the implementation for this is missing.
> 
> Add the trivial implementation for this feature.
> 
> Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk>
> ---
>  drivers/leds/leds-pwm.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/drivers/leds/leds-pwm.c b/drivers/leds/leds-pwm.c
> index e1b4c23a409a..1d47742c551f 100644
> --- a/drivers/leds/leds-pwm.c
> +++ b/drivers/leds/leds-pwm.c
> @@ -70,6 +70,10 @@ static void led_pwm_set(struct led_classdev *led_cdev,
>  
>  	duty *= brightness;
>  	do_div(duty, max);
> +
> +	if (led_dat->active_low)
> +		duty = led_dat->period - duty;
> +
>  	led_dat->duty = duty;
>  

This will conflict with my patch (which is still lacking proper review)
there:
http://thread.gmane.org/gmane.linux.leds/482

I would say that it is better to hide the polarity inversion in the PWM
driver for your specific PWM. Else we will end up with all the drivers
using PWMs trying to detect whether the PWM supports inversion and if it
is not the case, calculating the inverted duty cycle.

So, I would go for my patch which is adding the missing polarity
inversion setting when using platform data and then implement software
polarity inversion in your underlying PWM driver. That also avoids patch
5/5 and I believe not adding a DT property is always a good idea.

What is your PWM that is not supporting polarity inversion ?

-- 
Alexandre Belloni, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

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

Re: [PATCH 4/5] leds: leds-pwm: implement PWM inversion

[Russell King - ARM Linux]

On Mon, Apr 07, 2014 at 10:46:51AM +0200, Alexandre Belloni wrote:
> (adding Thierry Reding)
> 
> Hi,
> 
> On 06/04/2014 at 23:20:18 +0100, Russell King wrote :
> > Some PWM outputs are wired such that the LED they're controlling is
> > connected to supply rather than ground.  These PWMs may not support
> > output inversion, or when they do, disabling the PWM may set the
> > PWM output low, causing a "brightness" value of zero to turn the LED
> > fully on.
> > 
> > The platform data for this driver already indicates that this was
> > thought about, and we have the "active_low" property there already.
> > However, the implementation for this is missing.
> > 
> > Add the trivial implementation for this feature.
> > 
> > Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk>
> > ---
> >  drivers/leds/leds-pwm.c | 4 ++++
> >  1 file changed, 4 insertions(+)
> > 
> > diff --git a/drivers/leds/leds-pwm.c b/drivers/leds/leds-pwm.c
> > index e1b4c23a409a..1d47742c551f 100644
> > --- a/drivers/leds/leds-pwm.c
> > +++ b/drivers/leds/leds-pwm.c
> > @@ -70,6 +70,10 @@ static void led_pwm_set(struct led_classdev *led_cdev,
> >  
> >  	duty *= brightness;
> >  	do_div(duty, max);
> > +
> > +	if (led_dat->active_low)
> > +		duty = led_dat->period - duty;
> > +
> >  	led_dat->duty = duty;
> >  
> 
> This will conflict with my patch (which is still lacking proper review)
> there:
> http://thread.gmane.org/gmane.linux.leds/482
> 
> I would say that it is better to hide the polarity inversion in the PWM
> driver for your specific PWM. Else we will end up with all the drivers
> using PWMs trying to detect whether the PWM supports inversion and if it
> is not the case, calculating the inverted duty cycle.
> 
> So, I would go for my patch which is adding the missing polarity
> inversion setting when using platform data and then implement software
> polarity inversion in your underlying PWM driver. That also avoids patch
> 5/5 and I believe not adding a DT property is always a good idea.
> 
> What is your PWM that is not supporting polarity inversion ?

Did you read the commit message properly, particularly the last sentence
of the first paragraph which refers to the problem with your approach?

-- 
FTTC broadband for 0.8mile line: now at 9.7Mbps down 460kbps up... slowly
improving, and getting towards what was expected from it.

Re: [PATCH 4/5] leds: leds-pwm: implement PWM inversion

[Alexandre Belloni]

On 07/04/2014 at 09:52:45 +0100, Russell King - ARM Linux wrote :
> On Mon, Apr 07, 2014 at 10:46:51AM +0200, Alexandre Belloni wrote:
> > 
> > This will conflict with my patch (which is still lacking proper review)
> > there:
> > http://thread.gmane.org/gmane.linux.leds/482
> > 
> > I would say that it is better to hide the polarity inversion in the PWM
> > driver for your specific PWM. Else we will end up with all the drivers
> > using PWMs trying to detect whether the PWM supports inversion and if it
> > is not the case, calculating the inverted duty cycle.
> > 
> > So, I would go for my patch which is adding the missing polarity
> > inversion setting when using platform data and then implement software
> > polarity inversion in your underlying PWM driver. That also avoids patch
> > 5/5 and I believe not adding a DT property is always a good idea.
> > 
> > What is your PWM that is not supporting polarity inversion ?
> 
> Did you read the commit message properly, particularly the last sentence
> of the first paragraph which refers to the problem with your approach?
> 

If disabling the PWM is putting the output low, I would then enable the
channel on pwm_request() and disable it on pwm_free() because anyway,
you'll have to let it run continuously to get the correct result.
However, Thierry seems to believe that there is an other underlying
issue in the PWM driver when this happens.

Going with your approach will end up with all the drivers trying to use
PWMs having to use the same logic and I'm sure a lot of people will get
confused between polarity inversion and using active_low...

-- 
Alexandre Belloni, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

Re: [PATCH 4/5] leds: leds-pwm: implement PWM inversion

[Russell King - ARM Linux]

On Mon, Apr 07, 2014 at 11:28:03AM +0200, Alexandre Belloni wrote:
> On 07/04/2014 at 09:52:45 +0100, Russell King - ARM Linux wrote :
> > On Mon, Apr 07, 2014 at 10:46:51AM +0200, Alexandre Belloni wrote:
> > > 
> > > This will conflict with my patch (which is still lacking proper review)
> > > there:
> > > http://thread.gmane.org/gmane.linux.leds/482
> > > 
> > > I would say that it is better to hide the polarity inversion in the PWM
> > > driver for your specific PWM. Else we will end up with all the drivers
> > > using PWMs trying to detect whether the PWM supports inversion and if it
> > > is not the case, calculating the inverted duty cycle.
> > > 
> > > So, I would go for my patch which is adding the missing polarity
> > > inversion setting when using platform data and then implement software
> > > polarity inversion in your underlying PWM driver. That also avoids patch
> > > 5/5 and I believe not adding a DT property is always a good idea.
> > > 
> > > What is your PWM that is not supporting polarity inversion ?
> > 
> > Did you read the commit message properly, particularly the last sentence
> > of the first paragraph which refers to the problem with your approach?
> > 
> 
> If disabling the PWM is putting the output low, I would then enable the
> channel on pwm_request() and disable it on pwm_free() because anyway,
> you'll have to let it run continuously to get the correct result.
> However, Thierry seems to believe that there is an other underlying
> issue in the PWM driver when this happens.
> 
> Going with your approach will end up with all the drivers trying to use
> PWMs having to use the same logic and I'm sure a lot of people will get
> confused between polarity inversion and using active_low...

Whatever.  It needs fixing.  It's three months on from when I first
raised this and so far it remains broken.

-- 
FTTC broadband for 0.8mile line: now at 9.7Mbps down 460kbps up... slowly
improving, and getting towards what was expected from it.

Re: [PATCH 4/5] leds: leds-pwm: implement PWM inversion

[Thierry Reding]

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

On Mon, Apr 07, 2014 at 10:46:51AM +0200, Alexandre Belloni wrote:
> (adding Thierry Reding)
> 
> Hi,
> 
> On 06/04/2014 at 23:20:18 +0100, Russell King wrote :
> > Some PWM outputs are wired such that the LED they're controlling is
> > connected to supply rather than ground.  These PWMs may not support
> > output inversion, or when they do, disabling the PWM may set the
> > PWM output low, causing a "brightness" value of zero to turn the LED
> > fully on.
> > 
> > The platform data for this driver already indicates that this was
> > thought about, and we have the "active_low" property there already.
> > However, the implementation for this is missing.
> > 
> > Add the trivial implementation for this feature.
> > 
> > Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk>
> > ---
> >  drivers/leds/leds-pwm.c | 4 ++++
> >  1 file changed, 4 insertions(+)
> > 
> > diff --git a/drivers/leds/leds-pwm.c b/drivers/leds/leds-pwm.c
> > index e1b4c23a409a..1d47742c551f 100644
> > --- a/drivers/leds/leds-pwm.c
> > +++ b/drivers/leds/leds-pwm.c
> > @@ -70,6 +70,10 @@ static void led_pwm_set(struct led_classdev *led_cdev,
> >  
> >  	duty *= brightness;
> >  	do_div(duty, max);
> > +
> > +	if (led_dat->active_low)
> > +		duty = led_dat->period - duty;
> > +
> >  	led_dat->duty = duty;
> >  
> 
> This will conflict with my patch (which is still lacking proper review)
> there:
> http://thread.gmane.org/gmane.linux.leds/482
> 
> I would say that it is better to hide the polarity inversion in the PWM
> driver for your specific PWM. Else we will end up with all the drivers
> using PWMs trying to detect whether the PWM supports inversion and if it
> is not the case, calculating the inverted duty cycle.

If the PWM hardware really does support inversion of the polarity, then
by all means that's what you should be using. However, and I've said
this a few times already, polarity inversion is not the same as
reversing the duty-cycle. The *effect* will be the same for LEDs and
backlights, but the signal is not in fact inverted.

There were at my latest count exactly two drivers who used PWM devices,
so the extent is somewhat limited. But if you're concerned about code
duplication, then perhaps a helper can be created that checks whether or
not a PWM channel's signal can be inverted and otherwise reverses the
duty-cycle. That can then be used for the special case where only the
effect of the PWM signal matters rather than the signal's inversion.

Thierry

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

Re: [PATCH 4/5] leds: leds-pwm: implement PWM inversion

[Russell King - ARM Linux]

On Mon, Apr 07, 2014 at 01:10:05PM +0200, Thierry Reding wrote:
> On Mon, Apr 07, 2014 at 10:46:51AM +0200, Alexandre Belloni wrote:
> > (adding Thierry Reding)
> > 
> > Hi,
> > 
> > On 06/04/2014 at 23:20:18 +0100, Russell King wrote :
> > > Some PWM outputs are wired such that the LED they're controlling is
> > > connected to supply rather than ground.  These PWMs may not support
> > > output inversion, or when they do, disabling the PWM may set the
> > > PWM output low, causing a "brightness" value of zero to turn the LED
> > > fully on.
> > > 
> > > The platform data for this driver already indicates that this was
> > > thought about, and we have the "active_low" property there already.
> > > However, the implementation for this is missing.
> > > 
> > > Add the trivial implementation for this feature.
> > > 
> > > Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk>
> > > ---
> > >  drivers/leds/leds-pwm.c | 4 ++++
> > >  1 file changed, 4 insertions(+)
> > > 
> > > diff --git a/drivers/leds/leds-pwm.c b/drivers/leds/leds-pwm.c
> > > index e1b4c23a409a..1d47742c551f 100644
> > > --- a/drivers/leds/leds-pwm.c
> > > +++ b/drivers/leds/leds-pwm.c
> > > @@ -70,6 +70,10 @@ static void led_pwm_set(struct led_classdev *led_cdev,
> > >  
> > >  	duty *= brightness;
> > >  	do_div(duty, max);
> > > +
> > > +	if (led_dat->active_low)
> > > +		duty = led_dat->period - duty;
> > > +
> > >  	led_dat->duty = duty;
> > >  
> > 
> > This will conflict with my patch (which is still lacking proper review)
> > there:
> > http://thread.gmane.org/gmane.linux.leds/482
> > 
> > I would say that it is better to hide the polarity inversion in the PWM
> > driver for your specific PWM. Else we will end up with all the drivers
> > using PWMs trying to detect whether the PWM supports inversion and if it
> > is not the case, calculating the inverted duty cycle.
> 
> If the PWM hardware really does support inversion of the polarity, then
> by all means that's what you should be using. However, and I've said
> this a few times already, polarity inversion is not the same as
> reversing the duty-cycle. The *effect* will be the same for LEDs and
> backlights, but the signal is not in fact inverted.

Sorry, for the general case, you're talking rubbish there.  The PWM layer
does not define the starting point in the PWM cycle, so there's no precise
definition of the exact waveform.  Therefore:

Here's a 10% duty cycle:		~_________~_________~_________
Here's another 10% duty cycle:		_________~_________~_________~
which are both equally valid implementations of a 10% PWM.

Here's a 90% duty cycle:		~~~~~~~~~_~~~~~~~~~_~~~~~~~~~_
Here's another 90% duty cycle:		_~~~~~~~~~_~~~~~~~~~_~~~~~~~~~
which are again both equally valid implementations of a 90% PWM.

Here's the first 10% duty cycle,
inverted:				_~~~~~~~~~_~~~~~~~~~_~~~~~~~~~
which is the same as the second example of a 90% PWM signal.

Here's the second 10% duty cycle,
inverted:				~~~~~~~~~_~~~~~~~~~_~~~~~~~~~_
which is the same as the first example of a 90% PWM signal.

Now, if PWM did define the starting point of the wave (which it doesn't,
or if it does, it's not documented which means we probably have loads of
buggy drivers) then yes, there would be some grounds to object.

In any case, we probably already have drivers which implement both
variants of the 10% signal, so really there's no grounds to say "a 90%
duty cycle is not the same as an inverted 10% duty cycle signal".

The electronic engineer in me says you're talking rubbish too, from the
point of view of designing stuff to produce a PWM signal.

So please, justify your statement.

-- 
FTTC broadband for 0.8mile line: now at 9.7Mbps down 460kbps up... slowly
improving, and getting towards what was expected from it.

Re: [PATCH 4/5] leds: leds-pwm: implement PWM inversion

[Thierry Reding]

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

On Mon, Apr 07, 2014 at 12:35:47PM +0100, Russell King - ARM Linux wrote:
> On Mon, Apr 07, 2014 at 01:10:05PM +0200, Thierry Reding wrote:
> > On Mon, Apr 07, 2014 at 10:46:51AM +0200, Alexandre Belloni wrote:
> > > (adding Thierry Reding)
> > > 
> > > Hi,
> > > 
> > > On 06/04/2014 at 23:20:18 +0100, Russell King wrote :
> > > > Some PWM outputs are wired such that the LED they're controlling is
> > > > connected to supply rather than ground.  These PWMs may not support
> > > > output inversion, or when they do, disabling the PWM may set the
> > > > PWM output low, causing a "brightness" value of zero to turn the LED
> > > > fully on.
> > > > 
> > > > The platform data for this driver already indicates that this was
> > > > thought about, and we have the "active_low" property there already.
> > > > However, the implementation for this is missing.
> > > > 
> > > > Add the trivial implementation for this feature.
> > > > 
> > > > Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk>
> > > > ---
> > > >  drivers/leds/leds-pwm.c | 4 ++++
> > > >  1 file changed, 4 insertions(+)
> > > > 
> > > > diff --git a/drivers/leds/leds-pwm.c b/drivers/leds/leds-pwm.c
> > > > index e1b4c23a409a..1d47742c551f 100644
> > > > --- a/drivers/leds/leds-pwm.c
> > > > +++ b/drivers/leds/leds-pwm.c
> > > > @@ -70,6 +70,10 @@ static void led_pwm_set(struct led_classdev *led_cdev,
> > > >  
> > > >  	duty *= brightness;
> > > >  	do_div(duty, max);
> > > > +
> > > > +	if (led_dat->active_low)
> > > > +		duty = led_dat->period - duty;
> > > > +
> > > >  	led_dat->duty = duty;
> > > >  
> > > 
> > > This will conflict with my patch (which is still lacking proper review)
> > > there:
> > > http://thread.gmane.org/gmane.linux.leds/482
> > > 
> > > I would say that it is better to hide the polarity inversion in the PWM
> > > driver for your specific PWM. Else we will end up with all the drivers
> > > using PWMs trying to detect whether the PWM supports inversion and if it
> > > is not the case, calculating the inverted duty cycle.
> > 
> > If the PWM hardware really does support inversion of the polarity, then
> > by all means that's what you should be using. However, and I've said
> > this a few times already, polarity inversion is not the same as
> > reversing the duty-cycle. The *effect* will be the same for LEDs and
> > backlights, but the signal is not in fact inverted.
> 
> Sorry, for the general case, you're talking rubbish there.  The PWM layer
> does not define the starting point in the PWM cycle, so there's no precise
> definition of the exact waveform.  Therefore:
> 
> Here's a 10% duty cycle:		~_________~_________~_________
> Here's another 10% duty cycle:		_________~_________~_________~
> which are both equally valid implementations of a 10% PWM.
> 
> Here's a 90% duty cycle:		~~~~~~~~~_~~~~~~~~~_~~~~~~~~~_
> Here's another 90% duty cycle:		_~~~~~~~~~_~~~~~~~~~_~~~~~~~~~
> which are again both equally valid implementations of a 90% PWM.
> 
> Here's the first 10% duty cycle,
> inverted:				_~~~~~~~~~_~~~~~~~~~_~~~~~~~~~
> which is the same as the second example of a 90% PWM signal.
> 
> Here's the second 10% duty cycle,
> inverted:				~~~~~~~~~_~~~~~~~~~_~~~~~~~~~_
> which is the same as the first example of a 90% PWM signal.

Good, I'm glad we agree on the basics here.

> Now, if PWM did define the starting point of the wave (which it doesn't,
> or if it does, it's not documented which means we probably have loads of
> buggy drivers) then yes, there would be some grounds to object.

PWM does in fact define the starting point. And it's even documented
(see enum pwm_polarity in include/linux/pwm.h). The fact that you
thought it wasn't documented probably indicates that it's the wrong
place, so perhaps you can suggest a better location. One where you
would've looked.

The convention defined there does match the example signals that you
gave above.

> In any case, we probably already have drivers which implement both
> variants of the 10% signal, so really there's no grounds to say "a 90%
> duty cycle is not the same as an inverted 10% duty cycle signal".

Like you said, since it's documented any of the drivers that implement
it wrongly should then be considered buggy. After all the documentation
was there from the beginning (commit 0aa0869c3c9b "pwm: Add support for
configuring the PWM polarity").

One of the reasons that I insisted on having this defined rigorously is
that all the way back when I took on maintainership there was a fair bit
of discussion and somebody (unfortunately I no longer recall where or
who) did mentioned that PWMs are used to trigger processes (I vaguely
remember synchronization of multiple processes being an issue as well).
One of the points made during the discussion was that polarity was
indeed important for certain use-cases and hence the rigorous definition
of enum pwm_polarity.

There's of course the issue that there could be hardware that doesn't
allow changing the polarity but doesn't produce the signal as expected
by the "normal" polarity. The only way I can think of handling that
situation would be to allow drivers to provide a .get_polarity() and
have client drivers chicken out if they can't cope with it. Nobody's
ever had any incentive to be that rigorous, probably because all drivers
really care about is the power of the signal.

> The electronic engineer in me says you're talking rubbish too, from the
> point of view of designing stuff to produce a PWM signal.

Can you please elaborate. I don't understand what you're saying.

Thierry

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

Re: [PATCH 4/5] leds: leds-pwm: implement PWM inversion

[Russell King - ARM Linux]

On Mon, Apr 07, 2014 at 02:01:45PM +0200, Thierry Reding wrote:
> On Mon, Apr 07, 2014 at 12:35:47PM +0100, Russell King - ARM Linux wrote:
> > Now, if PWM did define the starting point of the wave (which it doesn't,
> > or if it does, it's not documented which means we probably have loads of
> > buggy drivers) then yes, there would be some grounds to object.
> 
> PWM does in fact define the starting point. And it's even documented
> (see enum pwm_polarity in include/linux/pwm.h). The fact that you
> thought it wasn't documented probably indicates that it's the wrong
> place, so perhaps you can suggest a better location. One where you
> would've looked.

It's not documented in Documentation/pwm.txt, which is where I was
looking.

Okay, so what about this case - which is a case you need if you're
driving a H-bridge configuration with four synchronised PWMs (and
there are PWMs out there which can do this):

0: ~_________~_________~_________ (bottom left)
1: _____~_________~_________~____ (bottom right)
2: ~~~~~_~~~~~~~~~_~~~~~~~~~_~~~~ (top left)
3: _~~~~~~~~~_~~~~~~~~~_~~~~~~~~~ (top right)

The PWM design doesn't quite stretch to this use case.  (Example shown
driving four mosfets, two n-channel, two p-channel hence the inversion
requirement.)

In such a configuration, you must never end up turning both left or
both right mosfets on at the same time, otherwise you short the
supply and the magic blue smoke escapes.

Not quite as simple as you wanted it to be with your basic "rigorous"
definitions :)

> There's of course the issue that there could be hardware that doesn't
> allow changing the polarity but doesn't produce the signal as expected
> by the "normal" polarity. The only way I can think of handling that
> situation would be to allow drivers to provide a .get_polarity() and
> have client drivers chicken out if they can't cope with it. Nobody's
> ever had any incentive to be that rigorous, probably because all drivers
> really care about is the power of the signal.

There's also the issue of what value the PWM output is when you disable
the PWM, which is part of the problem here.

> > The electronic engineer in me says you're talking rubbish too, from the
> > point of view of designing stuff to produce a PWM signal.
> 
> Can you please elaborate. I don't understand what you're saying.

If I were designing a circuit to produce a PWM signal, and I wanted to
produce an inverted signal, I would not design a circuit to produce a
normal signal and then stick an inverter on the output.  I would instead
design it to produce the signal required directly.  Also, because the
application doesn't care about where the count starts, I would do which
ever turns out the easiest.

However, for this patch we are not talking about H bridge drivers, we
are not even talking about synchronised devices.  We are talking about
a *LED* which you have already acknowledged does not care about the
aspect of where the signal starts.  Even at low rates, where we want
the LED to blink with a duty cycle, it does not matter.  What matters
is the duty cycle emitted by the LED, not by the precise timing.

However, I'll meet you at the middle ground: I'll change the DT property
to "invert-duty" instead of "active-low".  Is that acceptable?

-- 
FTTC broadband for 0.8mile line: now at 9.7Mbps down 460kbps up... slowly
improving, and getting towards what was expected from it.

Re: [PATCH 4/5] leds: leds-pwm: implement PWM inversion

[Thierry Reding]

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

On Mon, Apr 07, 2014 at 01:37:50PM +0100, Russell King - ARM Linux wrote:
> On Mon, Apr 07, 2014 at 02:01:45PM +0200, Thierry Reding wrote:
> > On Mon, Apr 07, 2014 at 12:35:47PM +0100, Russell King - ARM Linux wrote:
> > > Now, if PWM did define the starting point of the wave (which it doesn't,
> > > or if it does, it's not documented which means we probably have loads of
> > > buggy drivers) then yes, there would be some grounds to object.
> > 
> > PWM does in fact define the starting point. And it's even documented
> > (see enum pwm_polarity in include/linux/pwm.h). The fact that you
> > thought it wasn't documented probably indicates that it's the wrong
> > place, so perhaps you can suggest a better location. One where you
> > would've looked.
> 
> It's not documented in Documentation/pwm.txt, which is where I was
> looking.

Okay, I'll work up a patch to include a paragraph about the expected
signals in that file.

> Okay, so what about this case - which is a case you need if you're
> driving a H-bridge configuration with four synchronised PWMs (and
> there are PWMs out there which can do this):
> 
> 0: ~_________~_________~_________ (bottom left)
> 1: _____~_________~_________~____ (bottom right)
> 2: ~~~~~_~~~~~~~~~_~~~~~~~~~_~~~~ (top left)
> 3: _~~~~~~~~~_~~~~~~~~~_~~~~~~~~~ (top right)

That's a very interesting use-case. I'll need to look into that further.

> The PWM design doesn't quite stretch to this use case.  (Example shown
> driving four mosfets, two n-channel, two p-channel hence the inversion
> requirement.)
> 
> In such a configuration, you must never end up turning both left or
> both right mosfets on at the same time, otherwise you short the
> supply and the magic blue smoke escapes.

So that's exactly a case where the inversion matters. If some driver
were to simply emulate inversion by reversing the duty cycle, then
you'll short the supply. Doesn't that support my argument of not
allowing drivers to emulate inversion in software?

> Not quite as simple as you wanted it to be with your basic "rigorous"
> definitions :)

Well, at least PWM is aware that such requirements exist. Having a
rigorous definition may not be all, but it's definitely necessary for
cases such as this.

I wonder how software is usually set up to drive such an H-bridge. You
say that there are PWMs out there which can do this, so there must be a
way for software to control this. Or perhaps those chips will only
output synchronized signals in which case they should be able to work
with the PWM subsystem just fine.

I'd appreciate any insight you could provide, since I'm interested in
supporting such use-cases. What I'm trying to avoid is tayloring the
subsystem to special cases, such as backlight or LED where signal
inversion and duty cycle reversion are effectively the same.

> > There's of course the issue that there could be hardware that doesn't
> > allow changing the polarity but doesn't produce the signal as expected
> > by the "normal" polarity. The only way I can think of handling that
> > situation would be to allow drivers to provide a .get_polarity() and
> > have client drivers chicken out if they can't cope with it. Nobody's
> > ever had any incentive to be that rigorous, probably because all drivers
> > really care about is the power of the signal.
> 
> There's also the issue of what value the PWM output is when you disable
> the PWM, which is part of the problem here.

I had assumed that for normal polarity the output was low when disabled,
and the reverse being true for inverse polarity. But apparently that's
not the case. I've wanted to document the expected behaviour as well,
but it seems like there's nothing drivers can really do about it. In
this case I'm not sure an unambiguous definition would be any good if
drivers have no influence over it at all.

It always seemed to me that this should be solvable on a board-level,
too, using pinctrl or what not, but so far nobody ever investigated as
far as I can tell.

> > > The electronic engineer in me says you're talking rubbish too, from the
> > > point of view of designing stuff to produce a PWM signal.
> > 
> > Can you please elaborate. I don't understand what you're saying.
> 
> If I were designing a circuit to produce a PWM signal, and I wanted to
> produce an inverted signal, I would not design a circuit to produce a
> normal signal and then stick an inverter on the output.  I would instead
> design it to produce the signal required directly.  Also, because the
> application doesn't care about where the count starts, I would do which
> ever turns out the easiest.

Apparently not everybody thinks as sanely as you do. Usually the reason
why people need polarity inversion is because there's actually an
inverter somewhere on the board that requires that the PWM be inverted
in order to make some backlight or LED light up properly.

> However, for this patch we are not talking about H bridge drivers, we
> are not even talking about synchronised devices.  We are talking about
> a *LED* which you have already acknowledged does not care about the
> aspect of where the signal starts.  Even at low rates, where we want
> the LED to blink with a duty cycle, it does not matter.  What matters
> is the duty cycle emitted by the LED, not by the precise timing.

I fully agree. All of which are reasons why the LED driver should be
implementing the inversion. It is the only place where the knowledge
exists that only the signal power matters rather than the polarity.

> However, I'll meet you at the middle ground: I'll change the DT property
> to "invert-duty" instead of "active-low".  Is that acceptable?

Maybe there was a misunderstanding. I wasn't objecting to your patch at
all. If you want to call the DT property active-low, then by all means
go ahead. What I'm objecting to is that people start to implement PWM
polarity support in PWM drivers by "emulating it in software", which is
just another way of saying they compute "duty = period - duty;".

So in fact I'm all in favour of your patch to implement this within the
leds-pwm driver. Because that driver knows exactly that the effect can
be achieved either by inverting the polarity or by reversing the duty
cycle.

But emulation in the PWM driver is wrong, in my opinion, because then a
driver that needs to know about the *real* polarity of the signal will
be fooled into shorting the supply in your H-bridge setup.

Thierry

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

Re: [PATCH 4/5] leds: leds-pwm: implement PWM inversion

[Alexandre Belloni]

On 07/04/2014 at 15:37:59 +0200, Thierry Reding wrote :
> On Mon, Apr 07, 2014 at 01:37:50PM +0100, Russell King - ARM Linux wrote:
> > On Mon, Apr 07, 2014 at 02:01:45PM +0200, Thierry Reding wrote:
> > > On Mon, Apr 07, 2014 at 12:35:47PM +0100, Russell King - ARM Linux wrote:
> > > > Now, if PWM did define the starting point of the wave (which it doesn't,
> > > > or if it does, it's not documented which means we probably have loads of
> > > > buggy drivers) then yes, there would be some grounds to object.
> > > 
> > > PWM does in fact define the starting point. And it's even documented
> > > (see enum pwm_polarity in include/linux/pwm.h). The fact that you
> > > thought it wasn't documented probably indicates that it's the wrong
> > > place, so perhaps you can suggest a better location. One where you
> > > would've looked.
> > 
> > It's not documented in Documentation/pwm.txt, which is where I was
> > looking.
> 
> Okay, I'll work up a patch to include a paragraph about the expected
> signals in that file.
> 
> > Okay, so what about this case - which is a case you need if you're
> > driving a H-bridge configuration with four synchronised PWMs (and
> > there are PWMs out there which can do this):
> > 
> > 0: ~_________~_________~_________ (bottom left)
> > 1: _____~_________~_________~____ (bottom right)
> > 2: ~~~~~_~~~~~~~~~_~~~~~~~~~_~~~~ (top left)
> > 3: _~~~~~~~~~_~~~~~~~~~_~~~~~~~~~ (top right)
> 
> That's a very interesting use-case. I'll need to look into that further.
> 
> > The PWM design doesn't quite stretch to this use case.  (Example shown
> > driving four mosfets, two n-channel, two p-channel hence the inversion
> > requirement.)
> > 
> > In such a configuration, you must never end up turning both left or
> > both right mosfets on at the same time, otherwise you short the
> > supply and the magic blue smoke escapes.
> 
> So that's exactly a case where the inversion matters. If some driver
> were to simply emulate inversion by reversing the duty cycle, then
> you'll short the supply. Doesn't that support my argument of not
> allowing drivers to emulate inversion in software?
> 
> > Not quite as simple as you wanted it to be with your basic "rigorous"
> > definitions :)
> 
> Well, at least PWM is aware that such requirements exist. Having a
> rigorous definition may not be all, but it's definitely necessary for
> cases such as this.
> 
> I wonder how software is usually set up to drive such an H-bridge. You
> say that there are PWMs out there which can do this, so there must be a
> way for software to control this. Or perhaps those chips will only
> output synchronized signals in which case they should be able to work
> with the PWM subsystem just fine.
> 
> I'd appreciate any insight you could provide, since I'm interested in
> supporting such use-cases. What I'm trying to avoid is tayloring the
> subsystem to special cases, such as backlight or LED where signal
> inversion and duty cycle reversion are effectively the same.
> 

The Allwinner A31 ans the Atmel sama5d3 are able to output complementary
signals on different pins. You don't have much to do in that case. But
the sama5d3 is able to generate a dead time between the edges of the two
complimentary outputs.

> > > There's of course the issue that there could be hardware that doesn't
> > > allow changing the polarity but doesn't produce the signal as expected
> > > by the "normal" polarity. The only way I can think of handling that
> > > situation would be to allow drivers to provide a .get_polarity() and
> > > have client drivers chicken out if they can't cope with it. Nobody's
> > > ever had any incentive to be that rigorous, probably because all drivers
> > > really care about is the power of the signal.
> > 
> > There's also the issue of what value the PWM output is when you disable
> > the PWM, which is part of the problem here.
> 
> I had assumed that for normal polarity the output was low when disabled,
> and the reverse being true for inverse polarity. But apparently that's
> not the case. I've wanted to document the expected behaviour as well,
> but it seems like there's nothing drivers can really do about it. In
> this case I'm not sure an unambiguous definition would be any good if
> drivers have no influence over it at all.
> 
> It always seemed to me that this should be solvable on a board-level,
> too, using pinctrl or what not, but so far nobody ever investigated as
> far as I can tell.
> 

Actually, I had that issue at first with the atmel PWM driver, back in
September. After investigation it turns out that your assumptions are
still correct for that IP.
However, I'm still wondering what we should do when we know that the PWM
controller can't honor that expected behaviour. Would moving from
pwm_enable()/pwm_disable() to pwm_request/pwm_free() be the solution ?
As you commented that would mean that the PWM will always be running but
that is what we need anyway if we want to achieve the desired result...

> > > > The electronic engineer in me says you're talking rubbish too, from the
> > > > point of view of designing stuff to produce a PWM signal.
> > > 
> > > Can you please elaborate. I don't understand what you're saying.
> > 
> > If I were designing a circuit to produce a PWM signal, and I wanted to
> > produce an inverted signal, I would not design a circuit to produce a
> > normal signal and then stick an inverter on the output.  I would instead
> > design it to produce the signal required directly.  Also, because the
> > application doesn't care about where the count starts, I would do which
> > ever turns out the easiest.
> 
> Apparently not everybody thinks as sanely as you do. Usually the reason
> why people need polarity inversion is because there's actually an
> inverter somewhere on the board that requires that the PWM be inverted
> in order to make some backlight or LED light up properly.
> 
> > However, for this patch we are not talking about H bridge drivers, we
> > are not even talking about synchronised devices.  We are talking about
> > a *LED* which you have already acknowledged does not care about the
> > aspect of where the signal starts.  Even at low rates, where we want
> > the LED to blink with a duty cycle, it does not matter.  What matters
> > is the duty cycle emitted by the LED, not by the precise timing.
> 
> I fully agree. All of which are reasons why the LED driver should be
> implementing the inversion. It is the only place where the knowledge
> exists that only the signal power matters rather than the polarity.
> 
> > However, I'll meet you at the middle ground: I'll change the DT property
> > to "invert-duty" instead of "active-low".  Is that acceptable?
> 
> Maybe there was a misunderstanding. I wasn't objecting to your patch at
> all. If you want to call the DT property active-low, then by all means
> go ahead. What I'm objecting to is that people start to implement PWM
> polarity support in PWM drivers by "emulating it in software", which is
> just another way of saying they compute "duty = period - duty;".
> 
> So in fact I'm all in favour of your patch to implement this within the
> leds-pwm driver. Because that driver knows exactly that the effect can
> be achieved either by inverting the polarity or by reversing the duty
> cycle.
> 
> But emulation in the PWM driver is wrong, in my opinion, because then a
> driver that needs to know about the *real* polarity of the signal will
> be fooled into shorting the supply in your H-bridge setup.
> 

Point taken. I'm still a bit afraid about people then setting both the
inverted polarity on the PWM and active-low. Or not knowing when to
choose which one. I would expect that inverting the PWM is still the
better choice as it allows to disable the PWM.


-- 
Alexandre Belloni, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

Re: [PATCH 4/5] leds: leds-pwm: implement PWM inversion

[Russell King - ARM Linux]

On Mon, Apr 07, 2014 at 04:20:24PM +0200, Alexandre Belloni wrote:
> Point taken. I'm still a bit afraid about people then setting both the
> inverted polarity on the PWM and active-low. Or not knowing when to
> choose which one. I would expect that inverting the PWM is still the
> better choice as it allows to disable the PWM.

That depends.  Consider the case that I have - a LED connected between
supply and the PWM output.  The PWM controller, when disabled, sets the
output to ground.  When when the PWM controller supports output inversion,
when the controller is disabled, it still sets the output to ground.

That's the root of my problem - the iMX6 "inversion" isn't a real
inversion of the output itself, it's a change in the way the logic
inside the PWM works.

The design appears to be:

1. A counter which resets to zero, and then increments to a programmable
   maximum value.
2. A comparitor which compares the counter output with the sample value.
3. A set-reset flip flop on the output
4. Muxes on the set and reset input to select which signals are routed
   to the flip-flop.

What this means is that when the counter is reset to zero, the output
flip flop can be either set or cleared.  When the counter reaches the
sample value, the output can be either cleared or set.

While the PWM is running, this results in apparant polarity changes as
per the description in linux/pwm.h.  However, when disabled, the output
is always forced to zero.

What is not clearly specified is what happens at enable time.  The
documentations use of "the output pin is set to start a new period"
is rather ambiguous when they've already shown that there's a set-reset
latch - does it mean that a pulse is always sent to the 'set' input
causing the output to go high when the PWM is enabled... which would
mean it would end up remaining high until the end of the first period.

-- 
FTTC broadband for 0.8mile line: now at 9.7Mbps down 460kbps up... slowly
improving, and getting towards what was expected from it.

Re: [PATCH 0/5] Fix various issues with leds-pwm

[Bryan Wu]

On Sun, Apr 6, 2014 at 3:18 PM, Russell King - ARM Linux
<linux@arm.linux.org.uk> wrote:
> This patch series fixes various problems with leds-pwm.
>
> The first patch is very important to solve a boot time oops on OMAP
> boards (and probably others) which has recently shown its face.
>
> The next two clean up leds-pwm so that we can avoid the madness of
> having entirely separate initialisation paths for DT and non-DT, which
> I believe is the core cause of the initial problem.
>
> The last two patches add support for LEDs connected in the opposite
> sense to PWM outputs, where either the PWM does not support inversion,
> or does not support it in a way that works sanely with leds-pwm.  These
> add a new property to leds-pwm, hence why that patch (and this cover)
> copies the DT people.
>
> The first patch is an absolute must for going.
>

Cool, I think Thierry is good for this patchset. I'm going to merge it
with Thierry's ack and queue it for 3.16 merge window.

-Bryan


>  .../devicetree/bindings/leds/leds-pwm.txt          |   2 +
>  drivers/leds/leds-pwm.c                            | 161 ++++++++++-----------
>  2 files changed, 82 insertions(+), 81 deletions(-)
>
> --
> FTTC broadband for 0.8mile line: now at 9.7Mbps down 460kbps up... slowly
> improving, and getting towards what was expected from it.

Re: [PATCH 0/5] Fix various issues with leds-pwm

[Russell King - ARM Linux]

On Mon, Apr 07, 2014 at 02:31:00PM -0700, Bryan Wu wrote:
> On Sun, Apr 6, 2014 at 3:18 PM, Russell King - ARM Linux
> <linux@arm.linux.org.uk> wrote:
> > This patch series fixes various problems with leds-pwm.
> >
> > The first patch is very important to solve a boot time oops on OMAP
> > boards (and probably others) which has recently shown its face.
> >
> > The next two clean up leds-pwm so that we can avoid the madness of
> > having entirely separate initialisation paths for DT and non-DT, which
> > I believe is the core cause of the initial problem.
> >
> > The last two patches add support for LEDs connected in the opposite
> > sense to PWM outputs, where either the PWM does not support inversion,
> > or does not support it in a way that works sanely with leds-pwm.  These
> > add a new property to leds-pwm, hence why that patch (and this cover)
> > copies the DT people.
> >
> > The first patch is an absolute must for going.
> >
> 
> Cool, I think Thierry is good for this patchset. I'm going to merge it
> with Thierry's ack and queue it for 3.16 merge window.

I don't mind the majority of the patch set being queued for the next
merge window, but I object to the first patch being delayed.  The
first patch is a very necessary bug fix for a regression that has
cropped up since -rc7.

It may not be the fault of leds-pwm, but it's leds-pwm which is most
certainly buggy.  kfree'ing a work_struct which is active is *not*
on.

-- 
FTTC broadband for 0.8mile line: now at 9.7Mbps down 460kbps up... slowly
improving, and getting towards what was expected from it.

Re: [PATCH 0/5] Fix various issues with leds-pwm

[Bryan Wu]

On Mon, Apr 7, 2014 at 2:33 PM, Russell King - ARM Linux
<linux@arm.linux.org.uk> wrote:
> On Mon, Apr 07, 2014 at 02:31:00PM -0700, Bryan Wu wrote:
>> On Sun, Apr 6, 2014 at 3:18 PM, Russell King - ARM Linux
>> <linux@arm.linux.org.uk> wrote:
>> > This patch series fixes various problems with leds-pwm.
>> >
>> > The first patch is very important to solve a boot time oops on OMAP
>> > boards (and probably others) which has recently shown its face.
>> >
>> > The next two clean up leds-pwm so that we can avoid the madness of
>> > having entirely separate initialisation paths for DT and non-DT, which
>> > I believe is the core cause of the initial problem.
>> >
>> > The last two patches add support for LEDs connected in the opposite
>> > sense to PWM outputs, where either the PWM does not support inversion,
>> > or does not support it in a way that works sanely with leds-pwm.  These
>> > add a new property to leds-pwm, hence why that patch (and this cover)
>> > copies the DT people.
>> >
>> > The first patch is an absolute must for going.
>> >
>>
>> Cool, I think Thierry is good for this patchset. I'm going to merge it
>> with Thierry's ack and queue it for 3.16 merge window.
>
> I don't mind the majority of the patch set being queued for the next
> merge window, but I object to the first patch being delayed.  The
> first patch is a very necessary bug fix for a regression that has
> cropped up since -rc7.
>
> It may not be the fault of leds-pwm, but it's leds-pwm which is most
> certainly buggy.  kfree'ing a work_struct which is active is *not*
> on.
>

Sure, actually just after I sent out my previous email, I think I need
add your first patch into my git pull request for 3.15 merge window.
So no worries, I will do that.

Thanks,
-Bryan

Re: [PATCH 0/5] Fix various issues with leds-pwm

[Russell King - ARM Linux]

On Mon, Apr 07, 2014 at 02:36:26PM -0700, Bryan Wu wrote:
> On Mon, Apr 7, 2014 at 2:33 PM, Russell King - ARM Linux
> <linux@arm.linux.org.uk> wrote:
> > On Mon, Apr 07, 2014 at 02:31:00PM -0700, Bryan Wu wrote:
> >> On Sun, Apr 6, 2014 at 3:18 PM, Russell King - ARM Linux
> >> <linux@arm.linux.org.uk> wrote:
> >> > This patch series fixes various problems with leds-pwm.
> >> >
> >> > The first patch is very important to solve a boot time oops on OMAP
> >> > boards (and probably others) which has recently shown its face.
> >> >
> >> > The next two clean up leds-pwm so that we can avoid the madness of
> >> > having entirely separate initialisation paths for DT and non-DT, which
> >> > I believe is the core cause of the initial problem.
> >> >
> >> > The last two patches add support for LEDs connected in the opposite
> >> > sense to PWM outputs, where either the PWM does not support inversion,
> >> > or does not support it in a way that works sanely with leds-pwm.  These
> >> > add a new property to leds-pwm, hence why that patch (and this cover)
> >> > copies the DT people.
> >> >
> >> > The first patch is an absolute must for going.
> >> >
> >>
> >> Cool, I think Thierry is good for this patchset. I'm going to merge it
> >> with Thierry's ack and queue it for 3.16 merge window.
> >
> > I don't mind the majority of the patch set being queued for the next
> > merge window, but I object to the first patch being delayed.  The
> > first patch is a very necessary bug fix for a regression that has
> > cropped up since -rc7.
> >
> > It may not be the fault of leds-pwm, but it's leds-pwm which is most
> > certainly buggy.  kfree'ing a work_struct which is active is *not*
> > on.
> >
> 
> Sure, actually just after I sent out my previous email, I think I need
> add your first patch into my git pull request for 3.15 merge window.
> So no worries, I will do that.

So, I see this commit in mainline:

commit e81d372ff9f694e13fa46e8b5aaed505c7fd2a1f
Merge: 75e300c8ba58 2f05e1d4458f
Author: Linus Torvalds <torvalds@linux-foundation.org>
Date:   Sat Dec 15 12:52:42 2012 -0800

    Merge branch 'for-next' of git://git.kernel.org/pub/scm/linux/kernel/git/cooloney/linux-leds
    
    Pull LED subsystem update from Bryan Wu.
    
    * 'for-next' of git://git.kernel.org/pub/scm/linux/kernel/git/cooloney/linux-leds: (47 commits)

Yet I don't see this patch series in it.  What's the story behind the
lack of progress on this?

Your April response clearly indicated that the entire patch set was going
in during the 3.16 merge window.

-- 
FTTC broadband for 0.8mile line: now at 9.7Mbps down 460kbps up... slowly
improving, and getting towards what was expected from it.

Re: [PATCH 0/5] Fix various issues with leds-pwm

[Bryan Wu]

On Thu, Jun 12, 2014 at 10:12 AM, Russell King - ARM Linux
<linux@arm.linux.org.uk> wrote:
> On Mon, Apr 07, 2014 at 02:36:26PM -0700, Bryan Wu wrote:
>> On Mon, Apr 7, 2014 at 2:33 PM, Russell King - ARM Linux
>> <linux@arm.linux.org.uk> wrote:
>> > On Mon, Apr 07, 2014 at 02:31:00PM -0700, Bryan Wu wrote:
>> >> On Sun, Apr 6, 2014 at 3:18 PM, Russell King - ARM Linux
>> >> <linux@arm.linux.org.uk> wrote:
>> >> > This patch series fixes various problems with leds-pwm.
>> >> >
>> >> > The first patch is very important to solve a boot time oops on OMAP
>> >> > boards (and probably others) which has recently shown its face.
>> >> >
>> >> > The next two clean up leds-pwm so that we can avoid the madness of
>> >> > having entirely separate initialisation paths for DT and non-DT, which
>> >> > I believe is the core cause of the initial problem.
>> >> >
>> >> > The last two patches add support for LEDs connected in the opposite
>> >> > sense to PWM outputs, where either the PWM does not support inversion,
>> >> > or does not support it in a way that works sanely with leds-pwm.  These
>> >> > add a new property to leds-pwm, hence why that patch (and this cover)
>> >> > copies the DT people.
>> >> >
>> >> > The first patch is an absolute must for going.
>> >> >
>> >>
>> >> Cool, I think Thierry is good for this patchset. I'm going to merge it
>> >> with Thierry's ack and queue it for 3.16 merge window.
>> >
>> > I don't mind the majority of the patch set being queued for the next
>> > merge window, but I object to the first patch being delayed.  The
>> > first patch is a very necessary bug fix for a regression that has
>> > cropped up since -rc7.
>> >
>> > It may not be the fault of leds-pwm, but it's leds-pwm which is most
>> > certainly buggy.  kfree'ing a work_struct which is active is *not*
>> > on.
>> >
>>
>> Sure, actually just after I sent out my previous email, I think I need
>> add your first patch into my git pull request for 3.15 merge window.
>> So no worries, I will do that.
>
> So, I see this commit in mainline:
>
> commit e81d372ff9f694e13fa46e8b5aaed505c7fd2a1f
> Merge: 75e300c8ba58 2f05e1d4458f
> Author: Linus Torvalds <torvalds@linux-foundation.org>
> Date:   Sat Dec 15 12:52:42 2012 -0800
>

Are you sure? this was a git pull in 2012. At that time I didn't get
any patch from you.

The latest pull was

commit 4162877d3ffa900b618c369c490c7faa6af60e47
Merge: 6c61403 14f5716
Author: Linus Torvalds <torvalds@linux-foundation.org>
Date:   Thu Apr 10 09:06:10 2014 -0700

    Merge branch 'for-next' of
git://git.kernel.org/pub/scm/linux/kernel/git/cooloney/linux-leds

    Pull LED updates from Bryan Wu:
     "This cycle we got:
       - new driver for leds-mc13783
       - bug fixes
       - code cleanup"

    * 'for-next' of
git://git.kernel.org/pub/scm/linux/kernel/git/cooloney/linux-leds:
      leds: make sure we unregister a trigger only once
      leds: leds-pwm: properly clean up after probe failure

Here is your patch for the critical fixing which was merged.

      leds: clevo-mail: Make probe function __init
      leds-ot200: Fix dependencies
      leds-gpio: of: introduce MODULE_DEVICE_TABLE for module autoloading
      leds: clevo-mail: remove __initdata marker
      leds: leds-ss4200: remove __initdata marker
      leds: blinkm: remove unnecessary spaces
      leds: lp5562: remove unnecessary parentheses
      leds: leds-ss4200: remove DEFINE_PCI_DEVICE_TABLE macro
      leds: leds-s3c24xx: Trivial cleanup in header file
      drivers/leds: delete non-required instances of include <linux/init.h>
      leds: leds-gpio: add retain-state-suspended property
      leds: leds-mc13783: Add devicetree support
      leds: leds-mc13783: Remove unnecessary cleaning of registers on exit
      leds: leds-mc13783: Use proper "max_brightness" value fo LEDs
      leds: leds-mc13783: Use LED core PM functions
      leds: leds-mc13783: Add MC34708 LED support
      leds: Turn off led if blinking is disabled
      ledtrig-cpu: Handle CPU hot(un)plugging


>     Merge branch 'for-next' of git://git.kernel.org/pub/scm/linux/kernel/git/cooloney/linux-leds
>
>     Pull LED subsystem update from Bryan Wu.
>
>     * 'for-next' of git://git.kernel.org/pub/scm/linux/kernel/git/cooloney/linux-leds: (47 commits)
>
> Yet I don't see this patch series in it.  What's the story behind the
> lack of progress on this?
>
> Your April response clearly indicated that the entire patch set was going
> in during the 3.16 merge window.
>

The rest of them are queued in my tree
http://git.kernel.org/cgit/linux/kernel/git/cooloney/linux-leds.git/log/?h=for-next

And I will send them out during 3.16 merge window but now mainline is
still 3.15-rc8. I believe this is what we talked about before.

Oh, no, I'm sorry, I messed up 3.16 merge window which is before
3.15-rc1 with 3.17 merge window. And I really mean 3.17 merge window
which is before 3.16-rc1. My bad, I will definitely send them all out
during 3.17 merge window.

Thanks,
-Bryan

Re: [PATCH 0/5] Fix various issues with leds-pwm

[Alexandre Belloni]

Hi,

On 12/06/2014 at 10:37:43 -0700, Bryan Wu wrote :
> On Thu, Jun 12, 2014 at 10:12 AM, Russell King - ARM Linux
> <linux@arm.linux.org.uk> wrote:
> > So, I see this commit in mainline:
> >
> > commit e81d372ff9f694e13fa46e8b5aaed505c7fd2a1f
> > Merge: 75e300c8ba58 2f05e1d4458f
> > Author: Linus Torvalds <torvalds@linux-foundation.org>
> > Date:   Sat Dec 15 12:52:42 2012 -0800
> >
> 
> Are you sure? this was a git pull in 2012. At that time I didn't get
> any patch from you.
> 
> The latest pull was
> 
> commit 4162877d3ffa900b618c369c490c7faa6af60e47
> Merge: 6c61403 14f5716
> Author: Linus Torvalds <torvalds@linux-foundation.org>
> Date:   Thu Apr 10 09:06:10 2014 -0700
> 
>     Merge branch 'for-next' of
> git://git.kernel.org/pub/scm/linux/kernel/git/cooloney/linux-leds
> 

This pull request was for 3.15.

> > Yet I don't see this patch series in it.  What's the story behind the
> > lack of progress on this?
> >
> > Your April response clearly indicated that the entire patch set was going
> > in during the 3.16 merge window.
> >
> 
> The rest of them are queued in my tree
> http://git.kernel.org/cgit/linux/kernel/git/cooloney/linux-leds.git/log/?h=for-next
> 
> And I will send them out during 3.16 merge window but now mainline is
> still 3.15-rc8. I believe this is what we talked about before.
> 
> Oh, no, I'm sorry, I messed up 3.16 merge window which is before
> 3.15-rc1 with 3.17 merge window. And I really mean 3.17 merge window
> which is before 3.16-rc1. My bad, I will definitely send them all out
> during 3.17 merge window.
> 

Your for-next branch was in linux-next for a while. You don't seem to
have sent a pull request for 3.16 and the merge window is not closed yet
so you can probably do it now.


-- 
Alexandre Belloni, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

Re: [PATCH 0/5] Fix various issues with leds-pwm

[Bryan Wu]

On Thu, Jun 12, 2014 at 10:56 AM, Alexandre Belloni
<alexandre.belloni@free-electrons.com> wrote:
> Hi,
>
> On 12/06/2014 at 10:37:43 -0700, Bryan Wu wrote :
>> On Thu, Jun 12, 2014 at 10:12 AM, Russell King - ARM Linux
>> <linux@arm.linux.org.uk> wrote:
>> > So, I see this commit in mainline:
>> >
>> > commit e81d372ff9f694e13fa46e8b5aaed505c7fd2a1f
>> > Merge: 75e300c8ba58 2f05e1d4458f
>> > Author: Linus Torvalds <torvalds@linux-foundation.org>
>> > Date:   Sat Dec 15 12:52:42 2012 -0800
>> >
>>
>> Are you sure? this was a git pull in 2012. At that time I didn't get
>> any patch from you.
>>
>> The latest pull was
>>
>> commit 4162877d3ffa900b618c369c490c7faa6af60e47
>> Merge: 6c61403 14f5716
>> Author: Linus Torvalds <torvalds@linux-foundation.org>
>> Date:   Thu Apr 10 09:06:10 2014 -0700
>>
>>     Merge branch 'for-next' of
>> git://git.kernel.org/pub/scm/linux/kernel/git/cooloney/linux-leds
>>
>
> This pull request was for 3.15.
>
>> > Yet I don't see this patch series in it.  What's the story behind the
>> > lack of progress on this?
>> >
>> > Your April response clearly indicated that the entire patch set was going
>> > in during the 3.16 merge window.
>> >
>>
>> The rest of them are queued in my tree
>> http://git.kernel.org/cgit/linux/kernel/git/cooloney/linux-leds.git/log/?h=for-next
>>
>> And I will send them out during 3.16 merge window but now mainline is
>> still 3.15-rc8. I believe this is what we talked about before.
>>
>> Oh, no, I'm sorry, I messed up 3.16 merge window which is before
>> 3.15-rc1 with 3.17 merge window. And I really mean 3.17 merge window
>> which is before 3.16-rc1. My bad, I will definitely send them all out
>> during 3.17 merge window.
>>
>
> Your for-next branch was in linux-next for a while. You don't seem to
> have sent a pull request for 3.16 and the merge window is not closed yet
> so you can probably do it now.
>
>

You guys are right. Today I'm not in good status to work. Looks like
3.16 merge window is open , right? I will send out pull request soon,
actually that's my plan to do that.

-Bryan

Re: [PATCH 0/5] Fix various issues with leds-pwm

[Russell King - ARM Linux]

On Thu, Jun 12, 2014 at 10:37:43AM -0700, Bryan Wu wrote:
> > So, I see this commit in mainline:
> >
> > commit e81d372ff9f694e13fa46e8b5aaed505c7fd2a1f
> > Merge: 75e300c8ba58 2f05e1d4458f
> > Author: Linus Torvalds <torvalds@linux-foundation.org>
> > Date:   Sat Dec 15 12:52:42 2012 -0800
> 
> Are you sure? this was a git pull in 2012. At that time I didn't get
> any patch from you.

Yes, it's the wrong commit, sorry.

> The rest of them are queued in my tree
> http://git.kernel.org/cgit/linux/kernel/git/cooloney/linux-leds.git/log/?h=for-next
> 
> And I will send them out during 3.16 merge window but now mainline is
> still 3.15-rc8. I believe this is what we talked about before.

I'm not sure why you think that.

First, the merge window is almost over - it opened one week early when
-rc8 was released:

https://lkml.org/lkml/2014/6/1/264

Date	Sun, 1 Jun 2014 19:37:20 -0700
Subject	Linux 3.15-rc8 ... and merge window for 3.16

Secondly, 3.15 was released last Sunday:

Date	Sun, 8 Jun 2014 11:52:27 -0700
Subject	Linux 3.15 .. and continuation of merge window

> Oh, no, I'm sorry, I messed up 3.16 merge window which is before
> 3.15-rc1 with 3.17 merge window. And I really mean 3.17 merge window
> which is before 3.16-rc1. My bad, I will definitely send them all out
> during 3.17 merge window.

Sorry, I sent the patches in /good/ time for the 3.16 merge window -
two months before the current merge window opened... and now you seem
to be saying that you intended to get them *not* into the merge window
two months away from that point, but *five* months away from the point
that you claimed to accept them?

I'm also told that my patches are in next-20140528, and indeed in the
last linux-next, they are still there:

http://git.kernel.org/cgit/linux/kernel/git/next/linux-next.git/log/drivers/leds/leds-pwm.c

So you /can/ send them right now to Linus - the merge window is still
open for 3.16.

As for which merge window... here's a lesson.  This is how it normally
works:

3.15-rc8
3.15
<merge window for 3.16>
3.16-rc1
3.16-rc2
3.16-rc3
...
3.16-rc7
...
3.16
<merge window for 3.17>
3.17-rc1

So, your statement "3.16 merge window which is before 3.15-rc1" is
totally wrong.  The 3.X merge window happens immediately before
3.X-rc1 is released, for any X.

-- 
FTTC broadband for 0.8mile line: now at 9.7Mbps down 460kbps up... slowly
improving, and getting towards what was expected from it.

Re: [PATCH 0/5] Fix various issues with leds-pwm

[Bryan Wu]

On Thu, Jun 12, 2014 at 11:03 AM, Russell King - ARM Linux
<linux@arm.linux.org.uk> wrote:
> On Thu, Jun 12, 2014 at 10:37:43AM -0700, Bryan Wu wrote:
>> > So, I see this commit in mainline:
>> >
>> > commit e81d372ff9f694e13fa46e8b5aaed505c7fd2a1f
>> > Merge: 75e300c8ba58 2f05e1d4458f
>> > Author: Linus Torvalds <torvalds@linux-foundation.org>
>> > Date:   Sat Dec 15 12:52:42 2012 -0800
>>
>> Are you sure? this was a git pull in 2012. At that time I didn't get
>> any patch from you.
>
> Yes, it's the wrong commit, sorry.
>
>> The rest of them are queued in my tree
>> http://git.kernel.org/cgit/linux/kernel/git/cooloney/linux-leds.git/log/?h=for-next
>>
>> And I will send them out during 3.16 merge window but now mainline is
>> still 3.15-rc8. I believe this is what we talked about before.
>
> I'm not sure why you think that.
>
> First, the merge window is almost over - it opened one week early when
> -rc8 was released:
>
> https://lkml.org/lkml/2014/6/1/264
>
> Date    Sun, 1 Jun 2014 19:37:20 -0700
> Subject Linux 3.15-rc8 ... and merge window for 3.16
>
> Secondly, 3.15 was released last Sunday:
>
> Date    Sun, 8 Jun 2014 11:52:27 -0700
> Subject Linux 3.15 .. and continuation of merge window
>
>> Oh, no, I'm sorry, I messed up 3.16 merge window which is before
>> 3.15-rc1 with 3.17 merge window. And I really mean 3.17 merge window
>> which is before 3.16-rc1. My bad, I will definitely send them all out
>> during 3.17 merge window.
>
> Sorry, I sent the patches in /good/ time for the 3.16 merge window -
> two months before the current merge window opened... and now you seem
> to be saying that you intended to get them *not* into the merge window
> two months away from that point, but *five* months away from the point
> that you claimed to accept them?
>
> I'm also told that my patches are in next-20140528, and indeed in the
> last linux-next, they are still there:
>
> http://git.kernel.org/cgit/linux/kernel/git/next/linux-next.git/log/drivers/leds/leds-pwm.c
>
> So you /can/ send them right now to Linus - the merge window is still
> open for 3.16.
>
> As for which merge window... here's a lesson.  This is how it normally
> works:
>
> 3.15-rc8
> 3.15
> <merge window for 3.16>
> 3.16-rc1
> 3.16-rc2
> 3.16-rc3
> ...
> 3.16-rc7
> ...
> 3.16
> <merge window for 3.17>
> 3.17-rc1
>
> So, your statement "3.16 merge window which is before 3.15-rc1" is
> totally wrong.  The 3.X merge window happens immediately before
> 3.X-rc1 is released, for any X.
>

Indeed, let me refresh my brain from working on a strange bug recently.

The root cause is I didn't realize the merge window is open and forget
to send out pull request.  My original plan is to sent them out during
3.16 merge window. I just did.

And you previous email confused me, so I misunderstood the 3.16 merge
window is 3.17-rc1 which should be 3.16-rc1. God damn it, I messed up.

So actually everything is fine, I just send out the pull request.
Your first critical patch was merged during 3.15 cycle and the rest
for 3.16 merge window are in my pull request.

We are good now, right? -:((

-Bryan

Re: [PATCH 0/5] Fix various issues with leds-pwm

[Russell King - ARM Linux]

On Thu, Jun 12, 2014 at 11:16:50AM -0700, Bryan Wu wrote:
> Indeed, let me refresh my brain from working on a strange bug recently.
> 
> The root cause is I didn't realize the merge window is open and forget
> to send out pull request.  My original plan is to sent them out during
> 3.16 merge window. I just did.
> 
> And you previous email confused me, so I misunderstood the 3.16 merge
> window is 3.17-rc1 which should be 3.16-rc1. God damn it, I messed up.
> 
> So actually everything is fine, I just send out the pull request.
> Your first critical patch was merged during 3.15 cycle and the rest
> for 3.16 merge window are in my pull request.
> 
> We are good now, right? -:((

I hope so...  thanks.

-- 
FTTC broadband for 0.8mile line: now at 9.7Mbps down 460kbps up... slowly
improving, and getting towards what was expected from it.