Re: [PATCH v3 3/3] pwm: New driver to support PWM driven LEDs on TWL4030/6030 series of PMICs
On 11/26/2012 10:17 AM, Thierry Reding wrote:
>>> Doesn't this belong in the drivers/leds subsystem? Besides that, the
>>> same comments as for the previous patch apply. One additional note
>>> below.
>>
>> The PINs itself are called as LED but they are PWMs at the end. If we
>> represent them as PWMs they can be used for different purposes which is going
>> to be needed for example in BeagleBoard, where the LEDA (PWMA) is used as a
>> GPO to enable/disable the USB host power.
>
> Heh, that's an interesting use-case for a PWM. =)
You should have seen the expression on my face when I saw this on the
schematics ;)
>> Also the removed 'twl6030-pwm' driver was only controlled the LED part of
>> twl6030.
>> With this series I enable the use of the PWMs and the PWMs behind of the LED
>> functions to give us flexibility on how we are using them.
>
> Alright, we can keep it in the PWM subsystem then.
Thank you.
+static struct platform_driver twl_pwmled_driver = {
+ .driver = {
+ .name = "twl-pwmled",
+ .of_match_table = of_match_ptr(twl_pwmled_of_match),
+ },
+ .probe = twl_pwmled_probe,
+ .remove = __devexit_p(twl_pwmled_remove),
>>>
>>> You didn't annotate twl_pwmled_remove() with __devexit, so __devexit_p
>>> isn't needed here either.
>>
>> Oh yes, I have also received patches from a series which removes the
>> _devexit_p() from the kernel.
>> But should the __devexit need to be added to the remove function?
>
> __devexit_p without a corresponding __devexit doesn't make sense. But as
> all of __devinit, __devexit and __devexit_p will be removed sooner or
> later, new code just shouldn't bother adding it. In this case, just drop
> __devexit_p.
I'll get rid of them.
Thank you,
Péter
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 3/3] pwm: New driver to support PWM driven LEDs on TWL4030/6030 series of PMICs
On Mon, Nov 26, 2012 at 09:30:03AM +0100, Peter Ujfalusi wrote:
> On 11/23/2012 04:04 PM, Thierry Reding wrote:
> > On Tue, Nov 20, 2012 at 10:56:22AM +0100, Peter Ujfalusi wrote:
> >> The driver supports the following LED outputs as generic PWM driver:
> >> TWL4030 LEDA and LEDB (PWMA and PWMB)
> >> TWL6030 Charging indicator LED (PWM LED)
> >>
> >> On TWL6030 when the PWM requested LED is configured to be controlled by SW.
> >> In this case the user can enable/disable and set the duty period freely.
> >> When the PWM has been freed, the LED driver is put back to HW control.
> >>
> >> Signed-off-by: Peter Ujfalusi
> >> ---
> >> drivers/pwm/Kconfig | 10 ++
> >> drivers/pwm/Makefile | 1 +
> >> drivers/pwm/pwm-twl-led.c | 303
> >> ++
> >> 3 files changed, 314 insertions(+)
> >> create mode 100644 drivers/pwm/pwm-twl-led.c
> >
> > Doesn't this belong in the drivers/leds subsystem? Besides that, the
> > same comments as for the previous patch apply. One additional note
> > below.
>
> The PINs itself are called as LED but they are PWMs at the end. If we
> represent them as PWMs they can be used for different purposes which is going
> to be needed for example in BeagleBoard, where the LEDA (PWMA) is used as a
> GPO to enable/disable the USB host power.
Heh, that's an interesting use-case for a PWM. =)
> Also the removed 'twl6030-pwm' driver was only controlled the LED part of
> twl6030.
> With this series I enable the use of the PWMs and the PWMs behind of the LED
> functions to give us flexibility on how we are using them.
Alright, we can keep it in the PWM subsystem then.
> >> +static struct platform_driver twl_pwmled_driver = {
> >> + .driver = {
> >> + .name = "twl-pwmled",
> >> + .of_match_table = of_match_ptr(twl_pwmled_of_match),
> >> + },
> >> + .probe = twl_pwmled_probe,
> >> + .remove = __devexit_p(twl_pwmled_remove),
> >
> > You didn't annotate twl_pwmled_remove() with __devexit, so __devexit_p
> > isn't needed here either.
>
> Oh yes, I have also received patches from a series which removes the
> _devexit_p() from the kernel.
> But should the __devexit need to be added to the remove function?
__devexit_p without a corresponding __devexit doesn't make sense. But as
all of __devinit, __devexit and __devexit_p will be removed sooner or
later, new code just shouldn't bother adding it. In this case, just drop
__devexit_p.
Thierry
pgpQDvwjBEQsV.pgp
Description: PGP signature
Re: [PATCH v3 3/3] pwm: New driver to support PWM driven LEDs on TWL4030/6030 series of PMICs
On 11/23/2012 04:04 PM, Thierry Reding wrote:
> On Tue, Nov 20, 2012 at 10:56:22AM +0100, Peter Ujfalusi wrote:
>> The driver supports the following LED outputs as generic PWM driver:
>> TWL4030 LEDA and LEDB (PWMA and PWMB)
>> TWL6030 Charging indicator LED (PWM LED)
>>
>> On TWL6030 when the PWM requested LED is configured to be controlled by SW.
>> In this case the user can enable/disable and set the duty period freely.
>> When the PWM has been freed, the LED driver is put back to HW control.
>>
>> Signed-off-by: Peter Ujfalusi
>> ---
>> drivers/pwm/Kconfig | 10 ++
>> drivers/pwm/Makefile | 1 +
>> drivers/pwm/pwm-twl-led.c | 303
>> ++
>> 3 files changed, 314 insertions(+)
>> create mode 100644 drivers/pwm/pwm-twl-led.c
>
> Doesn't this belong in the drivers/leds subsystem? Besides that, the
> same comments as for the previous patch apply. One additional note
> below.
The PINs itself are called as LED but they are PWMs at the end. If we
represent them as PWMs they can be used for different purposes which is going
to be needed for example in BeagleBoard, where the LEDA (PWMA) is used as a
GPO to enable/disable the USB host power.
Also the removed 'twl6030-pwm' driver was only controlled the LED part of
twl6030.
With this series I enable the use of the PWMs and the PWMs behind of the LED
functions to give us flexibility on how we are using them.
>
>> +static struct platform_driver twl_pwmled_driver = {
>> +.driver = {
>> +.name = "twl-pwmled",
>> +.of_match_table = of_match_ptr(twl_pwmled_of_match),
>> +},
>> +.probe = twl_pwmled_probe,
>> +.remove = __devexit_p(twl_pwmled_remove),
>
> You didn't annotate twl_pwmled_remove() with __devexit, so __devexit_p
> isn't needed here either.
Oh yes, I have also received patches from a series which removes the
_devexit_p() from the kernel.
But should the __devexit need to be added to the remove function?
--
Péter
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 3/3] pwm: New driver to support PWM driven LEDs on TWL4030/6030 series of PMICs
On Tue, Nov 20, 2012 at 10:56:22AM +0100, Peter Ujfalusi wrote:
> The driver supports the following LED outputs as generic PWM driver:
> TWL4030 LEDA and LEDB (PWMA and PWMB)
> TWL6030 Charging indicator LED (PWM LED)
>
> On TWL6030 when the PWM requested LED is configured to be controlled by SW.
> In this case the user can enable/disable and set the duty period freely.
> When the PWM has been freed, the LED driver is put back to HW control.
>
> Signed-off-by: Peter Ujfalusi
> ---
> drivers/pwm/Kconfig | 10 ++
> drivers/pwm/Makefile | 1 +
> drivers/pwm/pwm-twl-led.c | 303
> ++
> 3 files changed, 314 insertions(+)
> create mode 100644 drivers/pwm/pwm-twl-led.c
Doesn't this belong in the drivers/leds subsystem? Besides that, the
same comments as for the previous patch apply. One additional note
below.
> +static struct platform_driver twl_pwmled_driver = {
> + .driver = {
> + .name = "twl-pwmled",
> + .of_match_table = of_match_ptr(twl_pwmled_of_match),
> + },
> + .probe = twl_pwmled_probe,
> + .remove = __devexit_p(twl_pwmled_remove),
You didn't annotate twl_pwmled_remove() with __devexit, so __devexit_p
isn't needed here either.
Thierry
pgpOYHLTcsefh.pgp
Description: PGP signature
Re: [PATCH v3 3/3] pwm: New driver to support PWM driven LEDs on TWL4030/6030 series of PMICs
Hi Thierry, On 11/20/2012 12:58 PM, Thierry Reding wrote: >> Oh, I missed to save the updated comment before I commited the change. >> Can I just send and update patch instead of the whole series again? > > Sure. > > Sorry for taking so long to review this. I'm rather busy with other > things but I'll try to find some time to review properly tonight or > tomorrow. I have sent the update patch for the series: https://lkml.org/lkml/2012/11/20/268 I know you are busy, but would you be able to take a look at the series? Thank you, Péter -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 3/3] pwm: New driver to support PWM driven LEDs on TWL4030/6030 series of PMICs
On Tue, Nov 20, 2012 at 12:54:11PM +0100, Peter Ujfalusi wrote:
> On 11/20/2012 10:56 AM, Peter Ujfalusi wrote:
> > The driver supports the following LED outputs as generic PWM driver:
> > TWL4030 LEDA and LEDB (PWMA and PWMB)
> > TWL6030 Charging indicator LED (PWM LED)
> >
> > On TWL6030 when the PWM requested LED is configured to be controlled by SW.
> > In this case the user can enable/disable and set the duty period freely.
> > When the PWM has been freed, the LED driver is put back to HW control.
> >
> > Signed-off-by: Peter Ujfalusi
> > ---
> > drivers/pwm/Kconfig | 10 ++
> > drivers/pwm/Makefile | 1 +
> > drivers/pwm/pwm-twl-led.c | 303
> > ++
> > 3 files changed, 314 insertions(+)
> > create mode 100644 drivers/pwm/pwm-twl-led.c
>
> ...
>
> > +static int twl4030_pwmled_config(struct pwm_chip *chip, struct pwm_device
> > *pwm,
> > + int duty_ns, int period_ns)
> > +{
> > + int duty_cycle = DIV_ROUND_UP(duty_ns * TWL4030_LED_MAX, period_ns);
> > + u8 pwm_config[2] = {1, 0};
> > + int base, ret;
> > +
> > + /*
> > +* To configure the duty period:
> > +* On-cycle is set to 1 (the minimum allowed value)
> > +* The off time of 0 is not configurable, so the mapping is:
> > +* 0 -> off cycle = 2,
> > +* 1 -> off cycle = 2,
> > +* 2 -> off cycle = 3,
> > +* 125 - > off cycle 127,
> > +* 126 - > off cycle 1
>
> Oh, I missed to save the updated comment before I commited the change.
> Can I just send and update patch instead of the whole series again?
Sure.
Sorry for taking so long to review this. I'm rather busy with other
things but I'll try to find some time to review properly tonight or
tomorrow.
Thierry
pgpNoPTamFVbI.pgp
Description: PGP signature
Re: [PATCH v3 3/3] pwm: New driver to support PWM driven LEDs on TWL4030/6030 series of PMICs
On 11/20/2012 10:56 AM, Peter Ujfalusi wrote:
> The driver supports the following LED outputs as generic PWM driver:
> TWL4030 LEDA and LEDB (PWMA and PWMB)
> TWL6030 Charging indicator LED (PWM LED)
>
> On TWL6030 when the PWM requested LED is configured to be controlled by SW.
> In this case the user can enable/disable and set the duty period freely.
> When the PWM has been freed, the LED driver is put back to HW control.
>
> Signed-off-by: Peter Ujfalusi
> ---
> drivers/pwm/Kconfig | 10 ++
> drivers/pwm/Makefile | 1 +
> drivers/pwm/pwm-twl-led.c | 303
> ++
> 3 files changed, 314 insertions(+)
> create mode 100644 drivers/pwm/pwm-twl-led.c
...
> +static int twl4030_pwmled_config(struct pwm_chip *chip, struct pwm_device
> *pwm,
> + int duty_ns, int period_ns)
> +{
> + int duty_cycle = DIV_ROUND_UP(duty_ns * TWL4030_LED_MAX, period_ns);
> + u8 pwm_config[2] = {1, 0};
> + int base, ret;
> +
> + /*
> + * To configure the duty period:
> + * On-cycle is set to 1 (the minimum allowed value)
> + * The off time of 0 is not configurable, so the mapping is:
> + * 0 -> off cycle = 2,
> + * 1 -> off cycle = 2,
> + * 2 -> off cycle = 3,
> + * 125 - > off cycle 127,
> + * 126 - > off cycle 1
Oh, I missed to save the updated comment before I commited the change.
Can I just send and update patch instead of the whole series again?
> + * When on cycle == off cycle the PWM will be always on
> + */
--
Péter
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
