| Message ID | 506088C9.8030002@metafoo.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Mon, Sep 24, 2012 at 06:22:33PM +0200, Lars-Peter Clausen wrote: > On 09/24/2012 05:56 PM, Dmitry Torokhov wrote: > > On Mon, Sep 24, 2012 at 07:55:38AM -0500, Rob Herring wrote: > >> On 09/24/2012 02:37 AM, Sascha Hauer wrote: > >>> A very simple binding, the only property is the phandle to the PWM. > >>> > >>> Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de> > >> > >> Acked-by: Rob Herring <rob.herring@calxeda.com> > >> > >>> --- > >>> Documentation/devicetree/bindings/input/pwm-beeper.txt | 7 +++++++ > >>> drivers/input/misc/pwm-beeper.c | 11 ++++++++++- > >>> 2 files changed, 17 insertions(+), 1 deletion(-) > >>> create mode 100644 Documentation/devicetree/bindings/input/pwm-beeper.txt > >>> > >>> diff --git a/Documentation/devicetree/bindings/input/pwm-beeper.txt b/Documentation/devicetree/bindings/input/pwm-beeper.txt > >>> new file mode 100644 > >>> index 0000000..7388b82 > >>> --- /dev/null > >>> +++ b/Documentation/devicetree/bindings/input/pwm-beeper.txt > >>> @@ -0,0 +1,7 @@ > >>> +* PWM beeper device tree bindings > >>> + > >>> +Registers a PWM device as beeper. > >>> + > >>> +Required properties: > >>> +- compatible: should be "pwm-beeper" > >>> +- pwms: phandle to the physical pwm device > >>> diff --git a/drivers/input/misc/pwm-beeper.c b/drivers/input/misc/pwm-beeper.c > >>> index fc84c8a..a6aa48c 100644 > >>> --- a/drivers/input/misc/pwm-beeper.c > >>> +++ b/drivers/input/misc/pwm-beeper.c > >>> @@ -75,7 +75,10 @@ static int __devinit pwm_beeper_probe(struct platform_device *pdev) > >>> if (!beeper) > >>> return -ENOMEM; > >>> > >>> - beeper->pwm = pwm_request(pwm_id, "pwm beeper"); > >>> + if (pdev->dev.platform_data) > >>> + beeper->pwm = pwm_request(pwm_id, "pwm beeper"); > >>> + else > >>> + beeper->pwm = pwm_get(&pdev->dev, NULL); > > > > Hmm, pwm_id == 0 is a valid ID I think, but your change makes it go into > > DT branch, potentially breaking it. My bad, I missed that platform_data is casted to an unsigned long. I thought I would test for a pointer. The obvious clean way would be to use a pointer for platform_data, but given that this will vanish anyway soon, I think we could just test for existence of dev->of_node instead of dev->platform_data. > > Yes, this a bit tricky, but we only have a single in-tree user of the > pwm-beeper which uses a id != 0. And now that all the PWM providers have > been converted to the new generic PWM framework the old legacy API will go > away soon anyway. So this if () else branch should hopefully only be > necessary for a transitional period of 1-2 releases. So I think this change > should be OK. > > But I think the patch is missing a change to the Kconfig entry to allow the > driver to be selected if the generic PWM framework is available. > > --- a/drivers/input/misc/Kconfig > +++ b/drivers/input/misc/Kconfig > @@ -444,7 +444,7 @@ config INPUT_PCF8574 > > config INPUT_PWM_BEEPER > tristate "PWM beeper support" > - depends on HAVE_PWM > + depends on HAVE_PWM || PWM Is this the preferred way to do this? Instead of doing the above I added a 'select HAVE_PWM' to the pwm framework instead. I found a patch for that, but there were comments to it that this is not good Sascha
On 09/24/2012 08:49 PM, Sascha Hauer wrote: > On Mon, Sep 24, 2012 at 06:22:33PM +0200, Lars-Peter Clausen wrote: >> On 09/24/2012 05:56 PM, Dmitry Torokhov wrote: >>> On Mon, Sep 24, 2012 at 07:55:38AM -0500, Rob Herring wrote: >>>> On 09/24/2012 02:37 AM, Sascha Hauer wrote: >>>>> A very simple binding, the only property is the phandle to the PWM. >>>>> >>>>> Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de> >>>> >>>> Acked-by: Rob Herring <rob.herring@calxeda.com> >>>> >>>>> --- >>>>> Documentation/devicetree/bindings/input/pwm-beeper.txt | 7 +++++++ >>>>> drivers/input/misc/pwm-beeper.c | 11 ++++++++++- >>>>> 2 files changed, 17 insertions(+), 1 deletion(-) >>>>> create mode 100644 Documentation/devicetree/bindings/input/pwm-beeper.txt >>>>> >>>>> diff --git a/Documentation/devicetree/bindings/input/pwm-beeper.txt b/Documentation/devicetree/bindings/input/pwm-beeper.txt >>>>> new file mode 100644 >>>>> index 0000000..7388b82 >>>>> --- /dev/null >>>>> +++ b/Documentation/devicetree/bindings/input/pwm-beeper.txt >>>>> @@ -0,0 +1,7 @@ >>>>> +* PWM beeper device tree bindings >>>>> + >>>>> +Registers a PWM device as beeper. >>>>> + >>>>> +Required properties: >>>>> +- compatible: should be "pwm-beeper" >>>>> +- pwms: phandle to the physical pwm device >>>>> diff --git a/drivers/input/misc/pwm-beeper.c b/drivers/input/misc/pwm-beeper.c >>>>> index fc84c8a..a6aa48c 100644 >>>>> --- a/drivers/input/misc/pwm-beeper.c >>>>> +++ b/drivers/input/misc/pwm-beeper.c >>>>> @@ -75,7 +75,10 @@ static int __devinit pwm_beeper_probe(struct platform_device *pdev) >>>>> if (!beeper) >>>>> return -ENOMEM; >>>>> >>>>> - beeper->pwm = pwm_request(pwm_id, "pwm beeper"); >>>>> + if (pdev->dev.platform_data) >>>>> + beeper->pwm = pwm_request(pwm_id, "pwm beeper"); >>>>> + else >>>>> + beeper->pwm = pwm_get(&pdev->dev, NULL); >>> >>> Hmm, pwm_id == 0 is a valid ID I think, but your change makes it go into >>> DT branch, potentially breaking it. > > My bad, I missed that platform_data is casted to an unsigned long. I > thought I would test for a pointer. > The obvious clean way would be to use a pointer for platform_data, but > given that this will vanish anyway soon, I think we could just test for > existence of dev->of_node instead of dev->platform_data. I think the plan is to convert the existing board file platforms to pwm_table and then remove the old pwm_request API. So this wouldn't work too well if we'd test for of_node. Maybe we can just run pwm_get unconditionally and fallback to pwm_request if it failed. That's also what the PWM backlight driver currently does. > >> >> Yes, this a bit tricky, but we only have a single in-tree user of the >> pwm-beeper which uses a id != 0. And now that all the PWM providers have >> been converted to the new generic PWM framework the old legacy API will go >> away soon anyway. So this if () else branch should hopefully only be >> necessary for a transitional period of 1-2 releases. So I think this change >> should be OK. >> >> But I think the patch is missing a change to the Kconfig entry to allow the >> driver to be selected if the generic PWM framework is available. >> >> --- a/drivers/input/misc/Kconfig >> +++ b/drivers/input/misc/Kconfig >> @@ -444,7 +444,7 @@ config INPUT_PCF8574 >> >> config INPUT_PWM_BEEPER >> tristate "PWM beeper support" >> - depends on HAVE_PWM >> + depends on HAVE_PWM || PWM > > Is this the preferred way to do this? Instead of doing the above I added > a 'select HAVE_PWM' to the pwm framework instead. I found a patch for that, > but there were comments to it that this is not good > Thierry said that this is his preferred solution. Given that HAVE_PWM will be extinct soon anyway I think it is fine. - Lars -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Sep 24, 2012 at 09:05:59PM +0200, Lars-Peter Clausen wrote: > On 09/24/2012 08:49 PM, Sascha Hauer wrote: > > > > My bad, I missed that platform_data is casted to an unsigned long. I > > thought I would test for a pointer. > > The obvious clean way would be to use a pointer for platform_data, but > > given that this will vanish anyway soon, I think we could just test for > > existence of dev->of_node instead of dev->platform_data. > > I think the plan is to convert the existing board file platforms to pwm_table > and then remove the old pwm_request API. So this wouldn't work too well if we'd > test for of_node. Maybe we can just run pwm_get unconditionally and fallback to > pwm_request if it failed. That's also what the PWM backlight driver currently does. Fine with me. > > > > >> > >> Yes, this a bit tricky, but we only have a single in-tree user of the > >> pwm-beeper which uses a id != 0. And now that all the PWM providers have > >> been converted to the new generic PWM framework the old legacy API will go > >> away soon anyway. So this if () else branch should hopefully only be > >> necessary for a transitional period of 1-2 releases. So I think this change > >> should be OK. > >> > >> But I think the patch is missing a change to the Kconfig entry to allow the > >> driver to be selected if the generic PWM framework is available. > >> > >> --- a/drivers/input/misc/Kconfig > >> +++ b/drivers/input/misc/Kconfig > >> @@ -444,7 +444,7 @@ config INPUT_PCF8574 > >> > >> config INPUT_PWM_BEEPER > >> tristate "PWM beeper support" > >> - depends on HAVE_PWM > >> + depends on HAVE_PWM || PWM > > > > Is this the preferred way to do this? Instead of doing the above I added > > a 'select HAVE_PWM' to the pwm framework instead. I found a patch for that, > > but there were comments to it that this is not good > > > > Thierry said that this is his preferred solution. Given that HAVE_PWM will be > extinct soon anyway I think it is fine. Ok. Will send an updated patch tomorrow. Thanks Sascha
On Mon, Sep 24, 2012 at 09:05:59PM +0200, Lars-Peter Clausen wrote: > On 09/24/2012 08:49 PM, Sascha Hauer wrote: > > On Mon, Sep 24, 2012 at 06:22:33PM +0200, Lars-Peter Clausen wrote: > >> On 09/24/2012 05:56 PM, Dmitry Torokhov wrote: > >>> On Mon, Sep 24, 2012 at 07:55:38AM -0500, Rob Herring wrote: > >>>> On 09/24/2012 02:37 AM, Sascha Hauer wrote: > >>>>> A very simple binding, the only property is the phandle to the PWM. > >>>>> > >>>>> Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de> > >>>> > >>>> Acked-by: Rob Herring <rob.herring@calxeda.com> > >>>> > >>>>> --- > >>>>> Documentation/devicetree/bindings/input/pwm-beeper.txt | 7 +++++++ > >>>>> drivers/input/misc/pwm-beeper.c | 11 ++++++++++- > >>>>> 2 files changed, 17 insertions(+), 1 deletion(-) > >>>>> create mode 100644 Documentation/devicetree/bindings/input/pwm-beeper.txt > >>>>> > >>>>> diff --git a/Documentation/devicetree/bindings/input/pwm-beeper.txt b/Documentation/devicetree/bindings/input/pwm-beeper.txt > >>>>> new file mode 100644 > >>>>> index 0000000..7388b82 > >>>>> --- /dev/null > >>>>> +++ b/Documentation/devicetree/bindings/input/pwm-beeper.txt > >>>>> @@ -0,0 +1,7 @@ > >>>>> +* PWM beeper device tree bindings > >>>>> + > >>>>> +Registers a PWM device as beeper. > >>>>> + > >>>>> +Required properties: > >>>>> +- compatible: should be "pwm-beeper" > >>>>> +- pwms: phandle to the physical pwm device > >>>>> diff --git a/drivers/input/misc/pwm-beeper.c b/drivers/input/misc/pwm-beeper.c > >>>>> index fc84c8a..a6aa48c 100644 > >>>>> --- a/drivers/input/misc/pwm-beeper.c > >>>>> +++ b/drivers/input/misc/pwm-beeper.c > >>>>> @@ -75,7 +75,10 @@ static int __devinit pwm_beeper_probe(struct platform_device *pdev) > >>>>> if (!beeper) > >>>>> return -ENOMEM; > >>>>> > >>>>> - beeper->pwm = pwm_request(pwm_id, "pwm beeper"); > >>>>> + if (pdev->dev.platform_data) > >>>>> + beeper->pwm = pwm_request(pwm_id, "pwm beeper"); > >>>>> + else > >>>>> + beeper->pwm = pwm_get(&pdev->dev, NULL); > >>> > >>> Hmm, pwm_id == 0 is a valid ID I think, but your change makes it go into > >>> DT branch, potentially breaking it. > > > > My bad, I missed that platform_data is casted to an unsigned long. I > > thought I would test for a pointer. > > The obvious clean way would be to use a pointer for platform_data, but > > given that this will vanish anyway soon, I think we could just test for > > existence of dev->of_node instead of dev->platform_data. > > I think the plan is to convert the existing board file platforms to pwm_table > and then remove the old pwm_request API. So this wouldn't work too well if we'd > test for of_node. Maybe we can just run pwm_get unconditionally and fallback to > pwm_request if it failed. That's also what the PWM backlight driver currently does. I agree. Calling pwm_get() will automatically switch to using the entries from a PWM table if the board adds it. > >> Yes, this a bit tricky, but we only have a single in-tree user of the > >> pwm-beeper which uses a id != 0. And now that all the PWM providers have > >> been converted to the new generic PWM framework the old legacy API will go > >> away soon anyway. So this if () else branch should hopefully only be > >> necessary for a transitional period of 1-2 releases. So I think this change > >> should be OK. > >> > >> But I think the patch is missing a change to the Kconfig entry to allow the > >> driver to be selected if the generic PWM framework is available. > >> > >> --- a/drivers/input/misc/Kconfig > >> +++ b/drivers/input/misc/Kconfig > >> @@ -444,7 +444,7 @@ config INPUT_PCF8574 > >> > >> config INPUT_PWM_BEEPER > >> tristate "PWM beeper support" > >> - depends on HAVE_PWM > >> + depends on HAVE_PWM || PWM > > > > Is this the preferred way to do this? Instead of doing the above I added > > a 'select HAVE_PWM' to the pwm framework instead. I found a patch for that, > > but there were comments to it that this is not good > > > > Thierry said that this is his preferred solution. Given that HAVE_PWM will be > extinct soon anyway I think it is fine. The reason is that some platforms, actually the majority, do not define HAVE_PWM property. We ran into that problem when the PWM tree was first added to linux-next because builds would break on PowerPC, which is one of the architectures that doesn't define HAVE_PWM. Conceptually what we're saying here is that either the legacy PWM API (HAVE_PWM) or the new PWM framework (PWM) can satisfy the dependency. Note that if you make the change above, it is no longer enough to depend on HAVE_PWM because that doesn't provide the pwm_get() function. Also we should be able to remove HAVE_PWM for 3.7 already as all legacy implementations have been moved. The plan is to get rid of pwm_request() and pwm_free() for 3.8. All that's required is converting all the board files to use PWM lookup tables. Thierry
--- a/drivers/input/misc/Kconfig +++ b/drivers/input/misc/Kconfig @@ -444,7 +444,7 @@ config INPUT_PCF8574 config INPUT_PWM_BEEPER tristate "PWM beeper support" - depends on HAVE_PWM + depends on HAVE_PWM || PWM help Say Y here to get support for PWM based beeper devices.