Prev: vfs: add NOFOLLOW flag to umount(2)
Next: [PATCH] Staging: dream: camera: msm_camera: fix coding style issues
From: Matthew Garrett on 11 Feb 2010 13:10
On Thu, Feb 11, 2010 at 10:05:19AM -0800, Dmitry Torokhov wrote:

> Or stuck it in dell-wmi.c...

I'd prefer not to put it in dell-wmi. There's no logical association
between it and the rest of the code there, so it'd just be overhead on
the majority of systems. With the exception of cases where the event and
functional interfaces are different GUIDs, I'd prefer a one GUID per
driver model.

--
Matthew Garrett | mjg59(a)srcf.ucam.org
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: Dmitry Torokhov on 11 Feb 2010 13:10
On Thu, Feb 11, 2010 at 05:59:14PM +0000, Richard Purdie wrote:
> On Mon, 2010-02-08 at 14:49 -0600, Bob Rodgers wrote:
> > This patch adds an LED driver to support the Dell Activity LED on the
> > Dell Latitude 2100 netbook and future products to come. The Activity LED
> > is visible externally in the lid so classroom instructors can observe it
> > from a distance. The driver uses the sysfs led_class and provides a
> > standard LED interface. This driver is ready for submission upstream.
>
> A couple of comments:
>
> > diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
> > index 8a0e1ec..40dd693 100644
> > --- a/drivers/leds/Kconfig
> > +++ b/drivers/leds/Kconfig
> > @@ -269,6 +269,13 @@ config LEDS_ADP5520
> > To compile this driver as a module, choose M here: the module will
> > be called leds-adp5520.
> >
> > +config LEDS_DELL_NETBOOKS
> > + tristate "External LED on Dell Business Netbooks"
> > + depends on LEDS_CLASS
> > + help
> > + This adds support for the Latitude 2100 and similar
> > + notebooks that have an external LED.
> > +
> > comment "LED Triggers"
>
> I assume this driver applies to X86 only? Is there anything else this
> config option should be depending on?
>
> > +static int __devinit dell_led_probe(struct platform_device *pdev)
> > +{
> > + return led_classdev_register(&pdev->dev, &dell_led);
> > +}
> > +
> > +static int dell_led_remove(struct platform_device *pdev)
> > +{
> > + led_classdev_unregister(&dell_led);
> > + return 0;
> > +}
> > +
> > +static struct platform_driver dell_led_driver = {
> > + .probe = dell_led_probe,
> > + .remove = dell_led_remove,
> > + .driver = {
> > + .name = KBUILD_MODNAME,
> > + .owner = THIS_MODULE,
> > + },
> > +};
> > +
> > +static struct platform_device *pdev;
> > +
> > +static int __init dell_led_init(void)
> > +{
> > + int error = 0;
> > +
> > + if (!wmi_has_guid(DELL_LED_BIOS_GUID)) {
> > + printk(KERN_DEBUG KBUILD_MODNAME
> > + ": could not find: DELL_LED_BIOS_GUID\n");
> > + return -ENODEV;
> > + }
> > +
> > + error = led_off();
> > + if (error != 0) {
> > + printk(KERN_DEBUG KBUILD_MODNAME
> > + ": could not communicate with LED"
> > + ": error %d\n", error);
> > + return -ENODEV;
> > + }
> > +
> > + error = platform_driver_register(&dell_led_driver);
> > + if (error < 0)
> > + return error;
> > +
> > + pdev = platform_device_register_simple(KBUILD_MODNAME, -1, NULL, 0);
> > + if (IS_ERR(pdev)) {
> > + error = PTR_ERR(pdev);
> > + platform_driver_unregister(&dell_led_driver);
> > + }
> > +
> > + return error;
> > +}
>
> Rather than add all this overhead of a platform device, why not just
> pass NULL as the parent to led_classdev_register()?
>

Or stuck it in dell-wmi.c...

--
Dmitry
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: Richard Purdie on 11 Feb 2010 13:10
On Mon, 2010-02-08 at 14:49 -0600, Bob Rodgers wrote:
> This patch adds an LED driver to support the Dell Activity LED on the
> Dell Latitude 2100 netbook and future products to come. The Activity LED
> is visible externally in the lid so classroom instructors can observe it
> from a distance. The driver uses the sysfs led_class and provides a
> standard LED interface. This driver is ready for submission upstream.

A couple of comments:

> diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
> index 8a0e1ec..40dd693 100644
> --- a/drivers/leds/Kconfig
> +++ b/drivers/leds/Kconfig
> @@ -269,6 +269,13 @@ config LEDS_ADP5520
> To compile this driver as a module, choose M here: the module will
> be called leds-adp5520.
>
> +config LEDS_DELL_NETBOOKS
> + tristate "External LED on Dell Business Netbooks"
> + depends on LEDS_CLASS
> + help
> + This adds support for the Latitude 2100 and similar
> + notebooks that have an external LED.
> +
> comment "LED Triggers"

I assume this driver applies to X86 only? Is there anything else this
config option should be depending on?

> +static int __devinit dell_led_probe(struct platform_device *pdev)
> +{
> + return led_classdev_register(&pdev->dev, &dell_led);
> +}
> +
> +static int dell_led_remove(struct platform_device *pdev)
> +{
> + led_classdev_unregister(&dell_led);
> + return 0;
> +}
> +
> +static struct platform_driver dell_led_driver = {
> + .probe = dell_led_probe,
> + .remove = dell_led_remove,
> + .driver = {
> + .name = KBUILD_MODNAME,
> + .owner = THIS_MODULE,
> + },
> +};
> +
> +static struct platform_device *pdev;
> +
> +static int __init dell_led_init(void)
> +{
> + int error = 0;
> +
> + if (!wmi_has_guid(DELL_LED_BIOS_GUID)) {
> + printk(KERN_DEBUG KBUILD_MODNAME
> + ": could not find: DELL_LED_BIOS_GUID\n");
> + return -ENODEV;
> + }
> +
> + error = led_off();
> + if (error != 0) {
> + printk(KERN_DEBUG KBUILD_MODNAME
> + ": could not communicate with LED"
> + ": error %d\n", error);
> + return -ENODEV;
> + }
> +
> + error = platform_driver_register(&dell_led_driver);
> + if (error < 0)
> + return error;
> +
> + pdev = platform_device_register_simple(KBUILD_MODNAME, -1, NULL, 0);
> + if (IS_ERR(pdev)) {
> + error = PTR_ERR(pdev);
> + platform_driver_unregister(&dell_led_driver);
> + }
> +
> + return error;
> +}

Rather than add all this overhead of a platform device, why not just
pass NULL as the parent to led_classdev_register()?

Cheers,

Richard

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: Matthew Garrett on 11 Feb 2010 13:20
On Thu, Feb 11, 2010 at 05:59:14PM +0000, Richard Purdie wrote:
> > +config LEDS_DELL_NETBOOKS
> > + tristate "External LED on Dell Business Netbooks"
> > + depends on LEDS_CLASS
> > + help
> > + This adds support for the Latitude 2100 and similar
> > + notebooks that have an external LED.
> > +
> > comment "LED Triggers"
>
> I assume this driver applies to X86 only? Is there anything else this
> config option should be depending on?

Sorry for not catching that. It ought to depend on ACPI_WMI as well.

--
Matthew Garrett | mjg59(a)srcf.ucam.org
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: Dmitry Torokhov on 11 Feb 2010 13:30
On Thu, Feb 11, 2010 at 06:07:15PM +0000, Matthew Garrett wrote:
> On Thu, Feb 11, 2010 at 10:05:19AM -0800, Dmitry Torokhov wrote:
>
> > Or stuck it in dell-wmi.c...
>
> I'd prefer not to put it in dell-wmi. There's no logical association
> between it and the rest of the code there, so it'd just be overhead on
> the majority of systems. With the exception of cases where the event and
> functional interfaces are different GUIDs, I'd prefer a one GUID per
> driver model.
>

This is an opposite direction form the rest of the platform drivers
which tend to combine functionality on per-vendor basis in one driver.

I'd say that in this case with this LED driver overhead on DELL systems
not supportinfg it is miniscule. The benefit is that all vendor-specific
sub-devices are children of one parent platform device.

--
Dmitry
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
 |  Next  |  Last
Pages: 1 2
Prev: vfs: add NOFOLLOW flag to umount(2)
Next: [PATCH] Staging: dream: camera: msm_camera: fix coding style issues