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

[Peter Ujfalusi]

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@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/