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]

Date:	Fri, 30 Nov 2012 12:00:03 +0100
From:	Peter Ujfalusi <peter.ujfalusi@...com>
To:	Grant Likely <grant.likely@...retlab.ca>
CC:	Lars-Peter Clausen <lars@...afoo.de>,
	Thierry Reding <thierry.reding@...onic-design.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 11/29/2012 05:10 PM, Grant Likely wrote:
>> 	/* Enable GPIO us of the PWMs */
>> 	gpio-controller = <1>;
> 
> This line should be simply (the property shouldn't have any data):
> 	gpio-controller;

Yes I know. It is like this already in my code. I just mixed up things while
hacking it together.

> 
>> 	#gpio-cells = <2>;
>> 	pwm,period_ns = <7812500>;
> 
> Nit: property names should use '-' instead of '_'.

Yes, true.

>> 	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.

Some PWM hw can select between different frequencies. They do that based on
the period_ns.
We can not just pass random non 0 number to them since they might fail with
the check for supported frequencies.

> Ugh, yeah this isn't the way to go. Don't register a new platform_device
> for the gpio functionality. It should be inline with the rest of the PWM
> setup and should trigger the registration of a gpio_chip directly.

We also need to take care of the non DT booted cases as well.
What we could do:
this driver with removed DT support for legacy booted systems.
GPO layer implementation within drivers/pwm/ to handle DT boot.

When we boot without DT we need to tell back to the machine driver about the
GPIO start of the gpio chip we just allocated.

Hrm, we could do this from the pwm-twl* drivers as well by adding an optional
callback and flag to the pwm core to register the gpio chip.
In this case we can move the whole whole gpio-pwm code under drivers/pwm, add
the 'struct gpio_chip' to struct pwm_chip{}
and register the gpio chip from within the pwm core.
We still have to provide the period_ns in same way to the PWM instance used as
GPIO, probably via platform data. Not sure about it right now.

-- 
Péter
--
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/