On Wed, 28 Nov 2012 09:54:57 +0100, Peter Ujfalusi <peter.ujfalusi@xxxxxx> 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.
> lookup = devm_kzalloc(chip->dev, sizeof(*lookup) * chip->npwm,
> GFP_KERNEL);
> pdata = devm_kzalloc(chip->dev, sizeof(*pdata), GFP_KERNEL);
> gpos = devm_kzalloc(chip->dev, sizeof(*gpos) * chip->npwm,
> GFP_KERNEL);
>
> pdata->gpos = gpos;
> pdata->num_gpos = chip->npwm;
> pdata->gpio_base = -1;
>
> pdev = platform_device_alloc("pwm-gpo", chip->base);
> pdev->dev.parent = chip->dev;
>
> sprintf(gpodev_name, "pwm-gpo.%d", chip->base);
> for (i = 0; i < chip->npwm; i++) {
> struct gpio_pwm *gpo = &gpos[i];
> struct pwm_lookup *pl = &lookup[i];
> char con_id[15];
>
> sprintf(con_id, "pwm-gpo.%d", chip->base + i);
>
> /* prepare GPO information */
> gpo->pwm_period_ns = period_ns;
> gpo->name = kmemdup(con_id, sizeof(con_id), GFP_KERNEL);;
>
> /* prepare pwm lookup table */
> pl->provider = dev_name(chip->dev);
> pl->index = i;
> pl->dev_id = kmemdup(gpodev_name, sizeof(gpodev_name),
> GFP_KERNEL);
> pl->con_id = kmemdup(con_id, sizeof(con_id), GFP_KERNEL);
> }
>
> platform_device_add_data(pdev, pdata, sizeof(*pdata));
> pwm_add_table(lookup, chip->npwm);
> platform_device_add(pdev);
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.
g.
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
