Message-ID: <Pine.LNX.4.64.0807162143460.26314@t2.domain.actdsltmp>
Date:	Wed, 16 Jul 2008 22:13:06 -0700 (PDT)
From:	Trent Piepho <tpiepho@...escale.com>
To:	Grant Likely <grant.likely@...retlab.ca>
cc:	Anton Vorontsov <avorontsov@...mvista.com>,
	Richard Purdie <rpurdie@...ys.net>,
	Stephen Rothwell <sfr@...b.auug.org.au>,
	Kumar Gala <galak@...nel.crashing.org>,
	linux-kernel@...r.kernel.org, linuxppc-dev@...abs.org
Subject: Re: [PATCH v3] leds: implement OpenFirmare GPIO LED driver

on Wed, 16 Jul 2008, Grant Likely wrote:
> On Wed, Jul 16, 2008 at 04:18:52PM -0700, Trent Piepho wrote:
>> On Tue, 15 Jul 2008, Anton Vorontsov wrote:
>>> Despite leds-gpio and leds-openfirmware-gpio similar purposes, there
>>> is not much code can be shared between the two drivers (both are mostly
>>> driver bindings anyway).
>>
>> Why can't this driver use the existing gpio-led driver?  Basically, do
>> something like this:
>>
>
> Ugh; that means registering *2* 'struct device' with the kernel instead of
> one.  One as a platform device and one as an of_platform device.
> It's bad enough that the LED scheme we're using for OF bindings has a
> separate registration for every single LED.

Ok, how about adding code the existing leds-gpio driver so that it can creates
LEDs from of_platform devices too?

I've made a patch to do this and it works ok.  The code added to leds-gpio is
about half what was involved in Anton's new driver.  What I did was re-factor
the existing platform device probe function to use a new function that creates
a single led.  Then a new of_platform probe function can use it too.  That way
most of the probe code is shared.  remove, suspend and resume aren't shared,
but they're short.  And the existing code to actually drive the led gets
reused as is.

There is still one of_platform device per led because of how the bindings work
(but that could be changed with new bindings), but there are zero extra
platform devices created.

Here's an example patch.  It won't apply to the git kernel as is, but should
make it clear how this works.

diff --git a/drivers/leds/leds-gpio.c b/drivers/leds/leds-gpio.c
index a4a2838..12e681e 100644
--- a/drivers/leds/leds-gpio.c
+++ b/drivers/leds/leds-gpio.c
@@ -71,11 +71,45 @@ static int gpio_blink_set(struct led_classdev *led_cdev,
  	return led_dat->platform_gpio_blink_set(led_dat->gpio, delay_on, delay_off);
  }

+static int create_gpio_led(struct gpio_led *cur_led,
+	struct gpio_led_data *led_dat, struct device *parent,
+	int (*blink_set)(unsigned, unsigned long *, unsigned long *))
+
+{
+	int ret;
+
+	ret = gpio_request(cur_led->gpio, cur_led->name);
+	if (ret < 0)
+		return ret;
+
+	led_dat->cdev.name = cur_led->name;
+	led_dat->cdev.default_trigger = cur_led->default_trigger;
+	led_dat->gpio = cur_led->gpio;
+	led_dat->can_sleep = gpio_cansleep(cur_led->gpio);
+	led_dat->active_low = cur_led->active_low;
+	if (blink_set) {
+		led_dat->platform_gpio_blink_set = blink_set;
+		led_dat->cdev.blink_set = gpio_blink_set;
+	}
+	led_dat->cdev.brightness_set = gpio_led_set;
+	led_dat->cdev.brightness = cur_led->start_on ? LED_FULL : LED_OFF;
+
+	gpio_direction_output(led_dat->gpio,
+			      led_dat->active_low ^ cur_led->start_on);
+
+	INIT_WORK(&led_dat->work, gpio_led_work);
+
+	ret = led_classdev_register(parent, &led_dat->cdev);
+	if (ret < 0)
+		gpio_free(led_dat->gpio);
+
+	return ret;
+}
+
  static int gpio_led_probe(struct platform_device *pdev)
  {
  	struct gpio_led_platform_data *pdata = pdev->dev.platform_data;
-	struct gpio_led *cur_led;
-	struct gpio_led_data *leds_data, *led_dat;
+	struct gpio_led_data *leds_data;
  	int i, ret = 0;

  	if (!pdata)
@@ -87,36 +121,10 @@ static int gpio_led_probe(struct platform_device *pdev)
  		return -ENOMEM;

  	for (i = 0; i < pdata->num_leds; i++) {
-		cur_led = &pdata->leds[i];
-		led_dat = &leds_data[i];
-
-		ret = gpio_request(cur_led->gpio, cur_led->name);
+		ret = create_gpio_led(&pdata->leds[i], &leds_data[i],
+				      &pdev->dev, pdata->gpio_blink_set);
  		if (ret < 0)
  			goto err;
-
-		led_dat->cdev.name = cur_led->name;
-		led_dat->cdev.default_trigger = cur_led->default_trigger;
-		led_dat->gpio = cur_led->gpio;
-		led_dat->can_sleep = gpio_cansleep(cur_led->gpio);
-		led_dat->active_low = cur_led->active_low;
-		if (pdata->gpio_blink_set) {
-			led_dat->platform_gpio_blink_set = pdata->gpio_blink_set;
-			led_dat->cdev.blink_set = gpio_blink_set;
-		}
-		led_dat->cdev.brightness_set = gpio_led_set;
-		led_dat->cdev.brightness =
-			cur_led->start_on ? LED_FULL : LED_OFF;
-
-		gpio_direction_output(led_dat->gpio,
-				      led_dat->active_low ^ cur_led->start_on);
-
-		INIT_WORK(&led_dat->work, gpio_led_work);
-
-		ret = led_classdev_register(&pdev->dev, &led_dat->cdev);
-		if (ret < 0) {
-			gpio_free(led_dat->gpio);
-			goto err;
-		}
  	}

  	platform_set_drvdata(pdev, leds_data);
@@ -217,3 +225,105 @@ MODULE_AUTHOR("Raphael Assenat <raph@...com>");
  MODULE_DESCRIPTION("GPIO LED driver");
  MODULE_LICENSE("GPL");
  MODULE_ALIAS("platform:leds-gpio");
+
+
+/* #ifdef CONFIG_LEDS_GPIO_OF */
+/* OpenFirmware bindings */
+#include <linux/of_platform.h>
+
+/* crap for old kernel, ignore */
+static inline const char *dev_name(struct device *dev)
+{ return dev->bus_id; }
+int of_get_gpio(struct device_node *np, int index)
+{ const u32 *pp = of_get_property(np, "gpio", NULL); return pp ? *pp : -1; }
+
+static int __devinit of_gpio_leds_probe(struct of_device *ofdev,
+					const struct of_device_id *match)
+{
+	struct device_node *np = ofdev->node;
+	struct gpio_led led;
+	struct gpio_led_data *led_dat;
+	int ret;
+
+	led_dat = kzalloc(sizeof(*led_dat), GFP_KERNEL);
+	if (!led_dat)
+		return -ENOMEM;
+
+	memset(&led, 0, sizeof(led));
+	led.gpio = of_get_gpio(np, 0);
+	led.name = of_get_property(np, "label", NULL);
+	if (!led.name)
+		led.name = dev_name(&ofdev->dev);
+
+	ret = create_gpio_led(&led, led_dat, &ofdev->dev, NULL);
+	if (ret < 0) {
+		kfree(led_dat);
+		return ret;
+	}
+
+	dev_set_drvdata(&ofdev->dev, led_dat);
+
+	return 0;
+}
+
+static int __devexit of_gpio_leds_remove(struct of_device *ofdev)
+{
+	struct gpio_led_data *led = dev_get_drvdata(&ofdev->dev);
+
+	led_classdev_unregister(&led->cdev);
+	cancel_work_sync(&led->work);
+	gpio_free(led->gpio);
+	kfree(led);
+
+	return 0;
+}
+
+#ifdef CONFIG_PM
+static int of_gpio_led_suspend(struct of_device *ofdev, pm_message_t state)
+{
+	struct gpio_led_data *led = dev_get_drvdata(&ofdev->dev);
+
+	led_classdev_suspend(&led->cdev);
+	return 0;
+}
+
+static int of_gpio_led_resume(struct of_device *ofdev)
+{
+	struct gpio_led_data *led = dev_get_drvdata(&ofdev->dev);
+
+	led_classdev_resume(&led->cdev);
+	return 0;
+}
+#else
+#define of_gpio_led_suspend NULL
+#define of_gpio_led_resume NULL
+#endif /* CONFIG_PM */
+
+static const struct of_device_id of_gpio_leds_match[] = {
+	{ .compatible = "gpio-led", },
+	{},
+};
+
+static struct of_platform_driver of_gpio_leds_driver = {
+	.driver = {
+		.name = "of_gpio_leds",
+		.owner = THIS_MODULE,
+	},
+	.match_table = of_gpio_leds_match,
+	.probe = of_gpio_leds_probe,
+	.remove = __devexit_p(of_gpio_leds_remove),
+	.suspend = of_gpio_led_suspend,
+	.resume = of_gpio_led_resume,
+};
+
+static int __init of_gpio_leds_init(void)
+{
+	return of_register_platform_driver(&of_gpio_leds_driver);
+}
+module_init(of_gpio_leds_init);
+
+static void __exit of_gpio_leds_exit(void)
+{
+	of_unregister_platform_driver(&of_gpio_leds_driver);
+}
+module_exit(of_gpio_leds_exit);
--
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/

Powered by blists - more mailing lists