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 for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <50B67BF1.8030808@metafoo.de>
Date:	Wed, 28 Nov 2012 22:02:41 +0100
From:	Lars-Peter Clausen <lars@...afoo.de>
To:	Peter Ujfalusi <peter.ujfalusi@...com>
CC:	Grant Likely <grant.likely@...retlab.ca>,
	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/28/2012 09:54 AM, Peter Ujfalusi 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>;
> 	#gpio-cells = <2>;
> 	pwm,period_ns = <7812500>;
> };
> 
> 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;
> 	}
> 
> 	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);
> }
> 

The whole pwm lookup table creation is a bit ugly. I wonder if we can somehow
bypass this by using pwm_request_from_chip directly in the pwm-gpo driver.

- Lars

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