[PATCH] gpio: twl4030: Use only TWL4030_MODULE_LED for LED configuration

[PATCH] gpio: twl4030: Use only TWL4030_MODULE_LED for LED configuration

[Peter Ujfalusi]

Avoid using the TWL4030_MODULE_PWMA/B as module ID. The LEDEN, PWMA ON/OFF
and PWMB ON/OFF registers are in a continuous range starting from LED base.
This is going to be helpful for further cleanup in the twl stack.

No functional changes.

Signed-off-by: Peter Ujfalusi <peter.ujfalusi@ti.com>
---
 drivers/gpio/gpio-twl4030.c | 35 ++++++++++++++++++-----------------
 1 file changed, 18 insertions(+), 17 deletions(-)

diff --git a/drivers/gpio/gpio-twl4030.c b/drivers/gpio/gpio-twl4030.c
index c5f8ca2..d2138b0 100644
--- a/drivers/gpio/gpio-twl4030.c
+++ b/drivers/gpio/gpio-twl4030.c
@@ -88,11 +88,15 @@ static inline int gpio_twl4030_write(u8 address, u8 data)
 /*----------------------------------------------------------------------*/
 
 /*
- * LED register offsets (use TWL4030_MODULE_{LED,PWMA,PWMB}))
+ * LED register offsets from TWL_MODULE_LED base
  * PWMs A and B are dedicated to LEDs A and B, respectively.
  */
 
-#define TWL4030_LED_LEDEN	0x0
+#define TWL4030_LED_LEDEN_REG	0x00
+#define TWL4030_PWMAON_REG	0x01
+#define TWL4030_PWMAOFF_REG	0x02
+#define TWL4030_PWMBON_REG	0x03
+#define TWL4030_PWMBOFF_REG	0x04
 
 /* LEDEN bits */
 #define LEDEN_LEDAON		BIT(0)
@@ -104,9 +108,6 @@ static inline int gpio_twl4030_write(u8 address, u8 data)
 #define LEDEN_PWM_LENGTHA	BIT(6)
 #define LEDEN_PWM_LENGTHB	BIT(7)
 
-#define TWL4030_PWMx_PWMxON	0x0
-#define TWL4030_PWMx_PWMxOFF	0x1
-
 #define PWMxON_LENGTH		BIT(7)
 
 /*----------------------------------------------------------------------*/
@@ -145,7 +146,7 @@ static void twl4030_led_set_value(int led, int value)
 	else
 		cached_leden |= mask;
 	status = twl_i2c_write_u8(TWL4030_MODULE_LED, cached_leden,
-			TWL4030_LED_LEDEN);
+				  TWL4030_LED_LEDEN_REG);
 	mutex_unlock(&gpio_lock);
 }
 
@@ -216,33 +217,33 @@ static int twl_request(struct gpio_chip *chip, unsigned offset)
 	if (offset >= TWL4030_GPIO_MAX) {
 		u8	ledclr_mask = LEDEN_LEDAON | LEDEN_LEDAEXT
 				| LEDEN_LEDAPWM | LEDEN_PWM_LENGTHA;
-		u8	module = TWL4030_MODULE_PWMA;
+		u8	reg = TWL4030_PWMAON_REG;
 
 		offset -= TWL4030_GPIO_MAX;
 		if (offset) {
 			ledclr_mask <<= 1;
-			module = TWL4030_MODULE_PWMB;
+			reg = TWL4030_PWMBON_REG;
 		}
 
 		/* initialize PWM to always-drive */
-		status = twl_i2c_write_u8(module, 0x7f,
-				TWL4030_PWMx_PWMxOFF);
+		/* Configure PWM OFF register first */
+		status = twl_i2c_write_u8(TWL4030_MODULE_LED, 0x7f, reg + 1);
 		if (status < 0)
 			goto done;
-		status = twl_i2c_write_u8(module, 0x7f,
-				TWL4030_PWMx_PWMxON);
+
+		/* Followed by PWM ON register */
+		status = twl_i2c_write_u8(TWL4030_MODULE_LED, 0x7f, reg);
 		if (status < 0)
 			goto done;
 
 		/* init LED to not-driven (high) */
-		module = TWL4030_MODULE_LED;
-		status = twl_i2c_read_u8(module, &cached_leden,
-				TWL4030_LED_LEDEN);
+		status = twl_i2c_read_u8(TWL4030_MODULE_LED, &cached_leden,
+					 TWL4030_LED_LEDEN_REG);
 		if (status < 0)
 			goto done;
 		cached_leden &= ~ledclr_mask;
-		status = twl_i2c_write_u8(module, cached_leden,
-				TWL4030_LED_LEDEN);
+		status = twl_i2c_write_u8(TWL4030_MODULE_LED, cached_leden,
+					  TWL4030_LED_LEDEN_REG);
 		if (status < 0)
 			goto done;
 
-- 
1.8.0

Re: [PATCH] gpio: twl4030: Use only TWL4030_MODULE_LED for LED configuration

[Linus Walleij]

On Tue, Nov 13, 2012 at 10:35 AM, Peter Ujfalusi <peter.ujfalusi@ti.com> wrote:

> Avoid using the TWL4030_MODULE_PWMA/B as module ID. The LEDEN, PWMA ON/OFF
> and PWMB ON/OFF registers are in a continuous range starting from LED base.
> This is going to be helpful for further cleanup in the twl stack.
>
> No functional changes.
>
> Signed-off-by: Peter Ujfalusi <peter.ujfalusi@ti.com>

OK if you say so.

Patch applied!

Yours,
Linus Walleij

Re: [PATCH] gpio: twl4030: Use only TWL4030_MODULE_LED for LED configuration

[Peter Ujfalusi]

Hi Linus,

On 11/17/2012 09:16 PM, Linus Walleij wrote:
> On Tue, Nov 13, 2012 at 10:35 AM, Peter Ujfalusi <peter.ujfalusi@ti.com> wrote:
> 
>> Avoid using the TWL4030_MODULE_PWMA/B as module ID. The LEDEN, PWMA ON/OFF
>> and PWMB ON/OFF registers are in a continuous range starting from LED base.
>> This is going to be helpful for further cleanup in the twl stack.
>>
>> No functional changes.
>>
>> Signed-off-by: Peter Ujfalusi <peter.ujfalusi@ti.com>
> 
> OK if you say so.
> 
> Patch applied!

Thanks,

I was actually tempted to remove the whole LED (PWM) thing from the
gpio-twl4030 driver. It was a big surprise to me to see something like this in
there.
It turns out that on BeagleBoard the USB host enable signal is connected to
LEDA (PWMA) of twl4030... It is an enable signal. Seriously. So what we do
here is either configure the PWMs as full on, or turn it off.
I'm not sure how common is this practice (PWM to drive enable signal) but if
we have more than one board out there with this type of connection we might be
better to add a 'gpio-pwm.c' driver to handle them.
Either way this is wrong IMHO to handle the LEDA/B via the gpio-twl4030 driver.

-- 
Péter

Re: [PATCH] gpio: twl4030: Use only TWL4030_MODULE_LED for LED configuration

[Linus Walleij]

On Mon, Nov 19, 2012 at 9:52 AM, Peter Ujfalusi <peter.ujfalusi@ti.com> wrote:

> I was actually tempted to remove the whole LED (PWM) thing from the
> gpio-twl4030 driver. It was a big surprise to me to see something like this in
> there.

It looks wrong. In theory I think this should be ripped out and moved to
drivers/leds/leds-twl4030.c. But that would only be in case it was *always*
exclusively used for a LED and as you say:

> It turns out that on BeagleBoard the USB host enable signal is connected to
> LEDA (PWMA) of twl4030... It is an enable signal. Seriously. So what we do
> here is either configure the PWMs as full on, or turn it off.

Sounds like some hardware engineer has been having fun or was
just out of GPIOs to use... So this LED PWM is also used as a
GPIO.

I think this part of the driver should be moved to
drivers/pwm/pwm-twl4030.c and modeled as a PWM. If some
platform need to use the PWM as if it was a GPIO then that's just a
special usecase. (Setting duty cycles to something like
INT_MAX and just switching polarity to toggle it...)

If it needs to be used by a LED there needs to be some generic
LED type just using a standard pwm_request() to get some
specific PWM, such as leds/leds-pwm.c or so. This way the
PWM can be used for LEDs if need be or other things...

Sounds correct, Sascha?

> Either way this is wrong IMHO to handle the LEDA/B via the gpio-twl4030 driver.

It's confusing indeed.

So what we're dealing with is: a LED-specific PWM, being used
as a PWM with eternal dutycycle and then being used as GPIO.

Well, we get to deal with it ... :-/

Yours,
Linus Walleij

Re: [PATCH] gpio: twl4030: Use only TWL4030_MODULE_LED for LED configuration

[Sascha Hauer]

On Mon, Nov 19, 2012 at 11:40:30AM +0100, Linus Walleij wrote:
> On Mon, Nov 19, 2012 at 9:52 AM, Peter Ujfalusi <peter.ujfalusi@ti.com> wrote:
> 
> > I was actually tempted to remove the whole LED (PWM) thing from the
> > gpio-twl4030 driver. It was a big surprise to me to see something like this in
> > there.
> 
> It looks wrong. In theory I think this should be ripped out and moved to
> drivers/leds/leds-twl4030.c. But that would only be in case it was *always*
> exclusively used for a LED and as you say:
> 
> > It turns out that on BeagleBoard the USB host enable signal is connected to
> > LEDA (PWMA) of twl4030... It is an enable signal. Seriously. So what we do
> > here is either configure the PWMs as full on, or turn it off.
> 
> Sounds like some hardware engineer has been having fun or was
> just out of GPIOs to use... So this LED PWM is also used as a
> GPIO.
> 
> I think this part of the driver should be moved to
> drivers/pwm/pwm-twl4030.c and modeled as a PWM. If some
> platform need to use the PWM as if it was a GPIO then that's just a
> special usecase. (Setting duty cycles to something like
> INT_MAX and just switching polarity to toggle it...)
> 
> If it needs to be used by a LED there needs to be some generic
> LED type just using a standard pwm_request() to get some
> specific PWM, such as leds/leds-pwm.c or so. This way the
> PWM can be used for LEDs if need be or other things...

Either I'm misinterpreting you or you didn't have a look in the tree.
Actually drivers/leds/leds-pwm.c already exists.

> 
> Sounds correct, Sascha?

Yes, the twl4030 pwm should go to drivers/pwm/pwm-twl4030.c.

Sascha


-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH] gpio: twl4030: Use only TWL4030_MODULE_LED for LED configuration

[Peter Ujfalusi]

Hi Linus,

On 11/19/2012 11:40 AM, Linus Walleij wrote:
> On Mon, Nov 19, 2012 at 9:52 AM, Peter Ujfalusi <peter.ujfalusi@ti.com> wrote:
> 
>> I was actually tempted to remove the whole LED (PWM) thing from the
>> gpio-twl4030 driver. It was a big surprise to me to see something like this in
>> there.
> 
> It looks wrong. In theory I think this should be ripped out and moved to
> drivers/leds/leds-twl4030.c. But that would only be in case it was *always*
> exclusively used for a LED and as you say:
> 
>> It turns out that on BeagleBoard the USB host enable signal is connected to
>> LEDA (PWMA) of twl4030... It is an enable signal. Seriously. So what we do
>> here is either configure the PWMs as full on, or turn it off.
> 
> Sounds like some hardware engineer has been having fun or was
> just out of GPIOs to use... So this LED PWM is also used as a
> GPIO.

My guess it was out of fun. On BealgeBoard GPIO13 from twl4030 is free
(LEDSYNC/GPIO.13) so it could have been used for this, or GPIO16/17 as well.

> I think this part of the driver should be moved to
> drivers/pwm/pwm-twl4030.c and modeled as a PWM. If some
> platform need to use the PWM as if it was a GPIO then that's just a
> special usecase. (Setting duty cycles to something like
> INT_MAX and just switching polarity to toggle it...)
> 
> If it needs to be used by a LED there needs to be some generic
> LED type just using a standard pwm_request() to get some
> specific PWM, such as leds/leds-pwm.c or so. This way the
> PWM can be used for LEDs if need be or other things...

I have already sent a series to support the PWMs in twl4030/6030:
https://lkml.org/lkml/2012/11/8/219
It adds support for both type of PWMs (the one named as PWM and the ones named
as LED since they are just PWMs).

We already have leds/leds-pwm driver in upstream so we are going to use that
if there is a led connected to one of them:
https://lkml.org/lkml/2012/11/12/222

We can use the backlight-pwm if the LCD backlight has been hooked to one of
the PWM (as it is done on SDP4430).

This is why I was thinking of adding gpio/gpio-pwm driver. With that we can
use the PWM as GPO line.

> Sounds correct, Sascha?
> 
>> Either way this is wrong IMHO to handle the LEDA/B via the gpio-twl4030 driver.
> 
> It's confusing indeed.
> 
> So what we're dealing with is: a LED-specific PWM, being used
> as a PWM with eternal dutycycle and then being used as GPIO.
> 
> Well, we get to deal with it ... :-/

gpio/gpio-pwm driver?

-- 
Péter

Re: [PATCH] gpio: twl4030: Use only TWL4030_MODULE_LED for LED configuration

[Peter Ujfalusi]

Hi Sascha,

On 11/19/2012 11:57 AM, Sascha Hauer wrote:
> Either I'm misinterpreting you or you didn't have a look in the tree.
> Actually drivers/leds/leds-pwm.c already exists.
>>
>> Sounds correct, Sascha?
> 
> Yes, the twl4030 pwm should go to drivers/pwm/pwm-twl4030.c.

I have already done the generic PWM drivers for the two type of PWMs (named
PWM, and the LED PWMs):
https://lkml.org/lkml/2012/11/8/219

They are happily using leds-pwm and backlight-pwm on my working tree.
I also sent cleanup and DT support for leds-pwm:
https://lkml.org/lkml/2012/11/12/222

-- 
Péter

Re: [PATCH] gpio: twl4030: Use only TWL4030_MODULE_LED for LED configuration

[Linus Walleij]

On Mon, Nov 19, 2012 at 1:19 PM, Peter Ujfalusi <peter.ujfalusi@ti.com> wrote:
> On 11/19/2012 11:40 AM, Linus Walleij wrote:
>> So what we're dealing with is: a LED-specific PWM, being used
>> as a PWM with eternal dutycycle and then being used as GPIO.
>>
>> Well, we get to deal with it ... :-/
>
> gpio/gpio-pwm driver?

Yeah something like that, but such a driver would need some huge
boilerplate saying that this approach isn't entirely sound.

Yours,
Linus Walleij