| Message ID | 1352799313-21019-1-git-send-email-peter.ujfalusi@ti.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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 -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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.
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 -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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
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?
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
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 -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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;
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(-)