Re: [PATCH] input: pwm-beeper: Add devicetree probing support

From: Thierry Reding <thierry.reding@avionic-design.de>
To: Lars-Peter Clausen <lars@metafoo.de>
Cc: Sascha Hauer <s.hauer@pengutronix.de>,
	Dmitry Torokhov <dmitry.torokhov@gmail.com>,
	Rob Herring <robherring2@gmail.com>,
	linux-input@vger.kernel.org, devicetree-discuss@lists.ozlabs.org,
	kernel@pengutronix.de
Subject: Re: [PATCH] input: pwm-beeper: Add devicetree probing support
Date: Mon, 24 Sep 2012 21:42:02 +0200	[thread overview]
Message-ID: <20120924194202.GB29044@avionic-0098.mockup.avionic-design.de> (raw)
In-Reply-To: <5060AF17.7070409@metafoo.de>

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

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

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