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: <50AF3E21.4000009@ti.com>
Date:	Fri, 23 Nov 2012 10:13:05 +0100
From:	Peter Ujfalusi <peter.ujfalusi@...com>
To:	Grant Likely <grant.likely@...retlab.ca>
CC:	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

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