Hi Grant,

On 11/23/2012 08:55 AM, Grant Likely wrote:

Ugh. and this is why I wanted the PWM and GPIO subsystems to use the
same namespace and binding. <grumble, mutter> But that's not your fault.

It's pretty horrible to have a separate translator node to convert a PWM
into a GPIO (with output only of course). The gpio properties should
appear directly in the PWM node itself and the translation code should
be in either the pwm or the gpio core. I don't think it should look like
a separate device.

Let me see if I understand your suggestion correctly. In the DT you suggest
something like this:

twl_pwmled: pwmled {
	compatible = "ti,twl4030-pwmled";
	#pwm-cells = <2>;
	#gpio-cells = <2>;
	gpio-controller;
};

led_user {
	compatible = "pwm-leds";
	pwms = <&twl_pwmled 1 7812500>; /* PWMB/LEDB from twl4030 */
	pwm-names = "PMU_STAT LED";

	label = "beagleboard::pmu_stat";
	max-brightness = <127>;
};

vdd_usbhost: fixedregulator-vdd-usbhost {
	compatible = "regulator-fixed";
	regulator-name = "USBHOST_POWER";
	regulator-min-microvolt = <5000000>;
	regulator-max-microvolt = <5000000>;
	gpio = <&twl_pwmled 0 7812500>; /* PWMA/LEDA from twl4030 */
	enable-active-high;
	regulator-boot-on;
};

With this I think this is what should happen in code level:
- the "pwm-gpo" driver does not have of_match_table at all.
- the driver for the "ti,twl4030-pwmled" is loaded.
- it prepares and calls pwmchip_add() to add the PWM chip.
- the of_pwmchip_add() will look for gpio-controller property of the node
 - if it is found it prepares the pdata (based on the PWM chip information)
for the "pwm-gpo" driver and registers the platform_device for it.
 - the "pwm-gpo" driver will use:
    priv->gpio_chip.of_node = pdev->dev.parent->of_node;

In DT boot we are fine with this I think.

When it comes to legacy boot (boot without DT) I think we should still have
the two layers to avoid big changes which would affect all existing pwm
drivers. Something like this in the board files:

static struct pwm_lookup pwm_lookup[] = {
	/* LEDA ->  nUSBHOST_PWR_EN */
	PWM_LOOKUP("twl-pwmled", 0, "pwm-gpo", "nUSBHOST_PWR_EN"),
	/* LEDB -> PMU_STAT */
	PWM_LOOKUP("twl-pwmled", 1, "leds_pwm", "beagleboard::pmu_stat"),
};

/* for the LED user of PWM */
static struct led_pwm pwm_leds[] = {
	{
		.name		= "beagleboard::pmu_stat",
		.max_brightness	= 127,
		.pwm_period_ns	= 7812500,
	},
};

static struct led_pwm_platform_data pwm_data = {
	.num_leds	= ARRAY_SIZE(pwm_leds),
	.leds		= pwm_leds,
};

static struct platform_device leds_pwm = {
	.name	= "leds_pwm",
	.id	= -1,
	.dev	= {
		.platform_data = &pwm_data,
	},
};

/* for the GPIO user of PWM */
static struct gpio_pwm pwm_gpios[] = {
	{
		.name		= "nUSBHOST_PWR_EN",
		.pwm_period_ns	= 7812500,
	},
};

static struct gpio_pwm_pdata pwm_gpio_data = {
	.num_gpos	= ARRAY_SIZE(pwm_gpios),
	.gpos		= pwm_gpios,
	.setup		= beagle_pwm_gpio_setup, /*to get the gpio base	*/
};

static struct platform_device gpos_pwm = {
	.name	= "pwm-gpo",
	.id	= -1,
	.dev	= {
		.platform_data = &pwm_gpio_data,
	},
};

static int beagle_pwm_gpio_setup(struct device *dev, unsigned gpio,
				 unsigned ngpio)
{
	beagle_usbhub_pdata.gpio = gpio; /* fixed_voltage_config struct */

	platform_device_register(&beagle_usbhub);
	return 0;
}

What do you think?

-- 
Péter