linux-kernel - Re: [PATCH] gpio: New driver for GPO emulation using PWM generators

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20121130064752.GB26474@avionic-0098.adnet.avionic-design.de>
Date:	Fri, 30 Nov 2012 07:47:52 +0100
From:	Thierry Reding <thierry.reding@...onic-design.de>
To:	Grant Likely <grant.likely@...retlab.ca>
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 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?

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.

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.

Thierry

Content of type "application/pgp-signature" skipped