Hi Trent,
On Wed, Dec 10, 2008 at 07:41:40AM -0800, Trent Piepho wrote:
> Add bindings to support LEDs defined as of_platform devices in addition to
> the existing bindings for platform devices.
>
> New options in Kconfig allow the platform binding code and/or the
> of_platform code to be turned on. The of_platform code is of course only
> available on archs that have OF support.
>
> The existing probe and remove methods are refactored to use new functions
> create_gpio_led(), to create and register one led, and delete_gpio_led(),
> to unregister and free one led. The new probe and remove methods for the
> of_platform driver can then share most of the common probe and remove code
> with the platform driver.
>
> The suspend and resume methods aren't shared, but they are very short. The
> actual led driving code is the same for LEDs created by either binding.
>
> The OF bindings are based on patch by Anton Vorontsov
> <[EMAIL PROTECTED]>. They have been extended to allow multiple LEDs
> per device.
>
> Signed-off-by: Trent Piepho <[EMAIL PROTECTED]>
> Acked-by: Grant Likely <[EMAIL PROTECTED]>
> Acked-by: Sean MacLennan <[EMAIL PROTECTED]>
> CC: [EMAIL PROTECTED]
> ---
Some petty notes below...
[...]
> -static int gpio_led_probe(struct platform_device *pdev)
> +static int __devinit create_gpio_led(const struct gpio_led *template,
> + struct gpio_led_data *led_dat, struct device *parent,
> + int (*blink_set)(unsigned, unsigned long *, unsigned long *))
> +{
> + int ret;
> +
> + ret = gpio_request(template->gpio, template->name);
> + if (ret < 0)
> + return ret;
> +
> + led_dat->cdev.name = template->name;
> + led_dat->cdev.default_trigger = template->default_trigger;
> + led_dat->gpio = template->gpio;
> + led_dat->can_sleep = gpio_cansleep(template->gpio);
> + led_dat->active_low = template->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 = LED_OFF;
> +
> + gpio_direction_output(led_dat->gpio, led_dat->active_low);
This can fail (yeah, the original code didn't check return value
either).
> + 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 __devinit of_gpio_leds_probe(struct of_device *ofdev,
> + const struct of_device_id *match)
> +{
> + struct device_node *np = ofdev->node, *child;
> + struct gpio_led led;
> + struct gpio_led_of_platform_data *pdata;
> + int count = 0, ret;
> +
> + /* count LEDs defined by this device, so we know how much to allocate */
> + for_each_child_of_node(np, child)
> + count++;
> + if (!count)
> + return 0; /* or ENODEV? */
> +
> + pdata = kzalloc(sizeof(*pdata) + sizeof(struct gpio_led_data) * count,
> + GFP_KERNEL);
> + if (!pdata)
> + return -ENOMEM;
> +
> + memset(&led, 0, sizeof(led));
> + for_each_child_of_node(np, child) {
> + unsigned int flags;
I think it would be better to use `enum of_gpio_flags' type here,
just for clarity.
> +
> + led.gpio = of_get_gpio_flags(child, 0, &flags);
> + led.active_low = flags & OF_GPIO_ACTIVE_LOW;
> + led.name = of_get_property(child, "label", NULL) ? :
> child->name;
> + led.default_trigger =
> + of_get_property(child, "linux,default-trigger", NULL);
> +
> + ret = create_gpio_led(&led, &pdata->led_data[pdata->num_leds++],
> + &ofdev->dev, NULL);
> + if (ret < 0)
of_node_put(np); is missing here.
> + goto err;
> + }
> +
> + dev_set_drvdata(&ofdev->dev, pdata);
> +
> + return 0;
> +
> +err:
> + for (count = pdata->num_leds - 2; count >= 0; count--)
> + delete_gpio_led(&pdata->led_data[count]);
> +
> + kfree(pdata);
> +
> + return ret;
> +}
[...]
Thanks,
--
Anton Vorontsov
email: [EMAIL PROTECTED]
irc://irc.freenode.net/bd2
_______________________________________________
Linuxppc-dev mailing list
Linuxppc-dev@ozlabs.org
https://ozlabs.org/mailman/listinfo/linuxppc-dev
