Message-Id: <20121130102039.0267E3E129E@localhost>
Date:	Fri, 30 Nov 2012 10:20:38 +0000
From:	Grant Likely <grant.likely@...retlab.ca>
To:	Thierry Reding <thierry.reding@...onic-design.de>
Cc:	Peter Ujfalusi <peter.ujfalusi@...com>,
	Lars-Peter Clausen <lars@...afoo.de>,
	Linus Walleij <linus.walleij@...aro.org>,
	Rob Landley <rob@...dley.net>,
	devicetree-discuss@...ts.ozlabs.org, linux-doc@...r.kernel.org,
	linux-kernel@...r.kernel.org,
	Mark Brown <broonie@...nsource.wolfsonmicro.com>,
	linux-omap@...r.kernel.org
Subject: Re: [PATCH] gpio: New driver for GPO emulation using PWM generators

On Fri, 30 Nov 2012 07:47:52 +0100, Thierry Reding <thierry.reding@...onic-design.de> wrote:
> On Thu, Nov 29, 2012 at 04:10:24PM +0000, Grant Likely wrote:
> > On Wed, 28 Nov 2012 09:54:57 +0100, Peter Ujfalusi <peter.ujfalusi@...com> wrote:
> > > Hi Grant, Lars, Thierry,
> > > 
> > > On 11/26/2012 04:46 PM, Grant Likely wrote:
> > > > You're effectively asking the pwm layer to behave like a gpio (which
> > > > is completely reasonable). Having a completely separate translation node
> > > > really doesn't make sense because it is entirely a software construct.
> > > > In fact, the way your using it is *entirely* to make the Linux driver
> > > > model instantiate the translation code. It has *nothing* to do with the
> > > > structure of the hardware. It makes complete sense that if a PWM is
> > > > going to be used as a GPIO, then the PWM node should conform to the GPIO
> > > > binding.
> > > 
> > > I understand your point around this. I might say I agree with it as well...
> > > I spent yesterday with prototyping and I'm not really convinced that it is a
> > > good approach from C code point of view. I got it working, yes.
> > > In essence this is what I have on top of the slightly modified gpio-pwm.c
> > > driver I have submitted:
> > > 
> > > DTS files:
> > > twl_pwm: pwm {
> > > 	/* provides two PWMs (id 0, 1 for PWM1 and PWM2) */
> > > 	compatible = "ti,twl6030-pwm";
> > > 	#pwm-cells = <2>;
> > > 
> > > 	/* Enable GPIO us of the PWMs */
> > > 	gpio-controller = <1>;
> > 
> > This line should be simply (the property shouldn't have any data):
> > 	gpio-controller;
> > 
> > > 	#gpio-cells = <2>;
> > > 	pwm,period_ns = <7812500>;
> > 
> > Nit: property names should use '-' instead of '_'.
> > 
> > > };
> > > 
> > > leds {
> > > 	compatible = "gpio-leds";
> > > 	backlight {
> > > 		label = "omap4::backlight";
> > > 		gpios = <&twl_pwm 1 0>; /* PWM1 of twl6030 */
> > > 	};
> > > 
> > > 	keypad {
> > > 		label = "omap4::keypad";
> > > 		gpios = <&twl_pwm 0 0>; /* PWM0 of twl6030 */
> > > 	};
> > > };
> > > 
> > > The bulk of the code in drivers/pwm/core.c to create the pwm-gpo device when
> > > it is requested going to look something like this. I have removed the error
> > > checks for now and I still don't have the code to clean up the allocated
> > > memory for the created device on error, or in case the module is unloaded. We
> > > should also prevent the pwm core from removal when the pwm-gpo driver is loaded.
> > > We need to create the platform device for gpo-pwm, create the pdata structure
> > > for it and fill it in. We also need to hand craft the pwm_lookup table so we
> > > can use pwm_get() to request the PWM. I have other minor changes around this
> > > to get things working when we booted with DT.
> > > So the function to do the heavy lifting is something like this:
> > > static void of_pwmchip_as_gpio(struct pwm_chip *chip)
> > > {
> > > 	struct platform_device *pdev;
> > > 	struct gpio_pwm *gpos;
> > > 	struct gpio_pwm_pdata *pdata;
> > > 	struct pwm_lookup *lookup;
> > > 	char gpodev_name[15];
> > > 	int i;
> > > 	u32 gpio_mode = 0;
> > > 	u32 period_ns = 0;
> > > 
> > > 	of_property_read_u32(chip->dev->of_node, "gpio-controller",
> > > 			     &gpio_mode);
> > > 	if (!gpio_mode)
> > > 		return;
> > > 
> > > 	of_property_read_u32(chip->dev->of_node, "pwm,period_ns", &period_ns);
> > > 	if (!period_ns) {
> > > 		dev_err(chip->dev,
> > > 			"period_ns is not specified for GPIO use\n");
> > > 		return;
> > > 	}
> > 
> > This property name seems ambiguous. What do you need to encode here? It
> > looks like there is a specific PWM period used for the 'on' state. What
> > about the 'off' state? Would different pwm outputs have different
> > frequencies required for GPIO usage.
> > 
> > Actually, I'm a bit surprised here that a period value is needed at all.
> > I would expect if a PWM is used as a GPIO then the driver would already
> > know how to set it up that way.
> 
> Just to make sure we're talking about the same thing here: if a PWM is
> used as GPIO the assumption is that it would be set to 0% duty-cycle
> when the GPIO value is set to 0 and 100% duty-cycle when set to the 1.
> The period will still need to be set here, otherwise how would the PWM
> core know what the hardware even supports?

Umm, I agree with you on duty cycle, but that's got nothing to do with
period. 100% duty cycle looks exactly the same whether the period is
10ns or 100s.

> Unless you're proposing to not include that in the PWM core but rather
> in individual drivers. Then I suppose the driver could choose some
> sensible default.

Mostly I'm asking questions because I'm not familiar with the subsystem.
If the property is needed then I have no objections, but at the moment
it doesn't make any sense to me.

> One other problem is that some PWM devices cannot be setup to achieve a
> 0% or 100% duty-cycle but instead will toggle for at least one period.
> This would be another argument in favour of moving the functionality to
> the individual drivers, perhaps with some functionality provided by the
> core to do the gpio_chip registration (a period could be passed to that
> function at registration time), which will likely be the same for all
> hardware that can and wants to support this feature.

It is a bit of an oddball case where if the hardware engineer wires up a
PWM to use as a GPIO, then he better be sure that it is actually fit for
the purpose. That doesn't prevent the PWM core having some
gpio-emulation helper functionality, but do need to be careful about it.

g.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists