Prev: tracing: Fix infinite loop in ftrace_update_pid_func()
Next: cpu affinity issue on kvm-88
From: Thadeu Lima de Souza Cascardo on 29 Sep 2009 16:20
On Tue, Sep 29, 2009 at 01:05:22PM -0700, Andrew Morton wrote:
> On Mon, 28 Sep 2009 22:38:00 -0300
> Thadeu Lima de Souza Cascardo <cascardo(a)holoscopio.com> wrote:
>
> > This add supports for devices like keyboard, backlight, tablet and
> > accelerometer.
> >
> > This work is supported by International Syst S/A.
> >
>
> I need to have a big whine about the coding style.
>
> > +static acpi_status cmpc_start_accel(acpi_handle handle)
> > +{
> > + union acpi_object param[2];
> > + struct acpi_object_list input;
> > + acpi_status status;
> > + param[0].type = ACPI_TYPE_INTEGER;
> > + param[0].integer.value = 0x3;
> > + param[1].type = ACPI_TYPE_INTEGER;
> > + input.count = 2;
> > + input.pointer = param;
> > + status = acpi_evaluate_object(handle, "ACMD", &input, NULL);
> > + return status;
> > +}
>
> To the jaded kernel developer's eye, this is quite hard to read. Every
> function here squishes the declarations of the locals up against the
> start of the code. It's a small thing, but this:
>
> static acpi_status cmpc_start_accel(acpi_handle handle)
> {
> union acpi_object param[2];
> struct acpi_object_list input;
> acpi_status status;
>
> param[0].type = ACPI_TYPE_INTEGER;
> param[0].integer.value = 0x3;
> param[1].type = ACPI_TYPE_INTEGER;
> input.count = 2;
> input.pointer = param;
> status = acpi_evaluate_object(handle, "ACMD", &input, NULL);
> return status;
> }
>
> puts a smile on our faces.
>

Will put this into consideration on the second version. Thanks, Andrew.

> >
> > ...
> >
> > +static ssize_t cmpc_accel_sense_store(struct device *dev,
> > + struct device_attribute *attr,
> > + const char *buf, size_t count)
> > +{
> > + struct acpi_device *acpi;
> > + int sense;
> > + acpi = to_acpi_device(dev);
> > + if (sscanf(buf, "%d", &sense) <= 0)
> > + return -EINVAL;
> > + cmpc_accel_set_sense(acpi->handle, sense);
> > + return strnlen(buf, count);
> > +}
>
> This will treat input of the form "42foo" as a valid decimal number.
> That's a bit sloppy. Can we use strict_strtoul() here?
>

Sure! The second version is coming soon. Thanks again.

Best regards,
Cascardo.
From: Joe Perches on 29 Sep 2009 16:20
On Tue, 2009-09-29 at 13:05 -0700, Andrew Morton wrote:
> static acpi_status cmpc_start_accel(acpi_handle handle)
> {
> union acpi_object param[2];
> struct acpi_object_list input;
> acpi_status status;
>
> param[0].type = ACPI_TYPE_INTEGER;
> param[0].integer.value = 0x3;
> param[1].type = ACPI_TYPE_INTEGER;
> input.count = 2;
> input.pointer = param;
> status = acpi_evaluate_object(handle, "ACMD", &input, NULL);
> return status;
> }
>
> puts a smile on our faces.

Much better, but what appears to be incomplete initialization
of stack variables is also a bit offputting.

cheers, Joe


--
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: Bjorn Helgaas on 30 Sep 2009 12:00
On Monday 28 September 2009 07:38:00 pm Thadeu Lima de Souza Cascardo wrote:
> This add supports for devices like keyboard, backlight, tablet and
> accelerometer.

Something about the PNP IDs bothered me yesterday, but I couldn't
figure out what. You're using:

ACCE0000
TBLT0000
IPML200
FnBT0000

Normally these OEM-specific ACPI drivers claim PNP IDs like ATK0100,
TOS6200, FUJ02B1, etc., that are obviously OEM-specific.

But the ones you're claiming seem pretty generic, like things
we could imagine appearing on machines other than the Classmate.

So I guess my question is, "Are these really generic? If so, should
this be split into separate, non-Classmate named drivers, such as
'accelerometer', 'tablet', etc.?"

Bjorn

>
> Signed-off-by: Thadeu Lima de Souza Cascardo <cascardo(a)holoscopio.com>
> ---
> MAINTAINERS | 6 +
> drivers/platform/x86/Kconfig | 12 +
> drivers/platform/x86/Makefile | 1 +
> drivers/platform/x86/cmpc_acpi.c | 522 ++++++++++++++++++++++++++++++++++++++
> 4 files changed, 541 insertions(+), 0 deletions(-)
> create mode 100644 drivers/platform/x86/cmpc_acpi.c
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index c450f3a..9e14df1 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -1368,6 +1368,12 @@ L: linux-scsi(a)vger.kernel.org
> S: Supported
> F: drivers/scsi/fnic/
>
> +CMPC ACPI DRIVER
> +M: Thadeu Lima de Souza Cascardo <cascardo(a)holoscopio.com>
> +M: Daniel Oliveira Nascimento <don(a)syst.com.br>
> +S: Supported
> +F: drivers/platform/x86/cmpc_acpi.c
> +
> CODA FILE SYSTEM
> M: Jan Harkes <jaharkes(a)cs.cmu.edu>
> M: coda(a)cs.cmu.edu
> diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig
> index 55ca39d..2cbc6a6 100644
> --- a/drivers/platform/x86/Kconfig
> +++ b/drivers/platform/x86/Kconfig
> @@ -435,4 +435,16 @@ config ACPI_TOSHIBA
>
> If you have a legacy free Toshiba laptop (such as the Libretto L1
> series), say Y.
> +
> +config ACPI_CMPC
> + tristate "CMPC Laptop Extras"
> + depends on X86
> + select INPUT
> + select BACKLIGHT_CLASS_DEVICE
> + default n
> + help
> + Support for Intel Classmate PC ACPI devices, including some
> + keys as input device, backlight device, tablet and accelerometer
> + devices.
> +
> endif # X86_PLATFORM_DEVICES
> diff --git a/drivers/platform/x86/Makefile b/drivers/platform/x86/Makefile
> index d1c1621..fc92247 100644
> --- a/drivers/platform/x86/Makefile
> +++ b/drivers/platform/x86/Makefile
> @@ -21,3 +21,4 @@ obj-$(CONFIG_ACPI_WMI) += wmi.o
> obj-$(CONFIG_ACPI_ASUS) += asus_acpi.o
> obj-$(CONFIG_TOPSTAR_LAPTOP) += topstar-laptop.o
> obj-$(CONFIG_ACPI_TOSHIBA) += toshiba_acpi.o
> +obj-$(CONFIG_ACPI_CMPC) += cmpc_acpi.o
> diff --git a/drivers/platform/x86/cmpc_acpi.c b/drivers/platform/x86/cmpc_acpi.c
> new file mode 100644
> index 0000000..c77c855
> --- /dev/null
> +++ b/drivers/platform/x86/cmpc_acpi.c
> @@ -0,0 +1,522 @@
> +/*
> + * Copyright (C) 2009 Thadeu Lima de Souza Cascardo <cascardo(a)holoscopio.com>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License along
> + * with this program; if not, write to the Free Software Foundation, Inc.,
> + * 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA.
> + */
> +
> +
> +#include <linux/init.h>
> +#include <linux/module.h>
> +#include <linux/workqueue.h>
> +#include <acpi/acpi_drivers.h>
> +#include <linux/backlight.h>
> +#include <linux/input.h>
> +
> +MODULE_LICENSE("GPL");
> +
> +
> +/*
> + * Generic input device code.
> + */
> +
> +typedef void (*input_device_init)(struct input_dev *dev);
> +
> +static int cmpc_add_acpi_notify_device(struct acpi_device *acpi, char *name,
> + acpi_notify_handler handler,
> + input_device_init idev_init)
> +{
> + struct input_dev *inputdev;
> + acpi_status status;
> + int error;
> + inputdev = input_allocate_device();
> + if (!inputdev) {
> + error = -ENOMEM;
> + goto out;
> + }
> + inputdev->name = name;
> + inputdev->dev.parent = &acpi->dev;
> + idev_init(inputdev);
> + error = input_register_device(inputdev);
> + if (error)
> + goto err_reg;
> + dev_set_drvdata(&acpi->dev, inputdev);
> + status = acpi_install_notify_handler(acpi->handle, ACPI_DEVICE_NOTIFY,
> + handler, inputdev);
> + if (ACPI_FAILURE(status)) {
> + error = -ENODEV;
> + goto err_acpi;
> + }
> + return 0;
> +err_acpi:
> + input_unregister_device(inputdev);
> +err_reg:
> + input_free_device(inputdev);
> +out:
> + return error;
> +}
> +
> +static int cmpc_remove_acpi_notify_device(struct acpi_device *acpi,
> + acpi_notify_handler handler)
> +{
> + struct input_dev *inputdev;
> + acpi_status status;
> + status = acpi_remove_notify_handler(acpi->handle, ACPI_DEVICE_NOTIFY,
> + handler);
> + inputdev = dev_get_drvdata(&acpi->dev);
> + input_unregister_device(inputdev);
> + input_free_device(inputdev);
> + return 0;
> +}
> +
> +
> +/*
> + * Accelerometer code.
> + */
> +
> +static acpi_status cmpc_start_accel(acpi_handle handle)
> +{
> + union acpi_object param[2];
> + struct acpi_object_list input;
> + acpi_status status;
> + param[0].type = ACPI_TYPE_INTEGER;
> + param[0].integer.value = 0x3;
> + param[1].type = ACPI_TYPE_INTEGER;
> + input.count = 2;
> + input.pointer = param;
> + status = acpi_evaluate_object(handle, "ACMD", &input, NULL);
> + return status;
> +}
> +
> +static acpi_status cmpc_stop_accel(acpi_handle handle)
> +{
> + union acpi_object param[2];
> + struct acpi_object_list input;
> + acpi_status status;
> + param[0].type = ACPI_TYPE_INTEGER;
> + param[0].integer.value = 0x4;
> + param[1].type = ACPI_TYPE_INTEGER;
> + input.count = 2;
> + input.pointer = param;
> + status = acpi_evaluate_object(handle, "ACMD", &input, NULL);
> + return status;
> +}
> +
> +static acpi_status cmpc_accel_set_sense(acpi_handle handle, int val)
> +{
> + union acpi_object param[2];
> + struct acpi_object_list input;
> + param[0].type = ACPI_TYPE_INTEGER;
> + param[0].integer.value = 0x02;
> + param[1].type = ACPI_TYPE_INTEGER;
> + param[1].integer.value = val;
> + input.count = 2;
> + input.pointer = param;
> + return acpi_evaluate_object(handle, "ACMD", &input, NULL);
> +}
> +
> +static acpi_status cmpc_get_accel(acpi_handle handle,
> + unsigned char *x,
> + unsigned char *y,
> + unsigned char *z)
> +{
> + union acpi_object param[2];
> + struct acpi_object_list input;
> + struct acpi_buffer output = { ACPI_ALLOCATE_BUFFER, 0 };
> + unsigned char *locs;
> + acpi_status status;
> + param[0].type = ACPI_TYPE_INTEGER;
> + param[0].integer.value = 0x01;
> + param[1].type = ACPI_TYPE_INTEGER;
> + input.count = 2;
> + input.pointer = param;
> + status = acpi_evaluate_object(handle, "ACMD", &input, &output);
> + if (ACPI_SUCCESS(status)) {
> + union acpi_object *obj;
> + obj = output.pointer;
> + locs = obj->buffer.pointer;
> + *x = locs[0];
> + *y = locs[1];
> + *z = locs[2];
> + kfree(output.pointer);
> + }
> + return status;
> +}
> +
> +static void cmpc_accel_handler(acpi_handle handle, u32 event, void *ctx)
> +{
> + struct input_dev *inputdev = ctx;
> + acpi_status status;
> + unsigned char x, y, z;
> + if (event == 0x81) {
> + status = cmpc_get_accel(handle, &x, &y, &z);
> + if (ACPI_SUCCESS(status)) {
> + input_report_abs(inputdev, ABS_X, x);
> + input_report_abs(inputdev, ABS_Y, y);
> + input_report_abs(inputdev, ABS_Z, z);
> + input_sync(inputdev);
> + }
> + }
> +}
> +
> +static ssize_t cmpc_accel_sense_store(struct device *dev,
> + struct device_attribute *attr,
> + const char *buf, size_t count)
> +{
> + struct acpi_device *acpi;
> + int sense;
> + acpi = to_acpi_device(dev);
> + if (sscanf(buf, "%d", &sense) <= 0)
> + return -EINVAL;
> + cmpc_accel_set_sense(acpi->handle, sense);
> + return strnlen(buf, count);
> +}
> +
> +struct device_attribute cmpc_accel_sense_attr = {
> + .attr = { .name = "sense", .mode = 0220 },
> + .store = cmpc_accel_sense_store
> +};
> +
> +static int cmpc_accel_open(struct input_dev *input)
> +{
> + struct acpi_device *acpi;
> + acpi = to_acpi_device(input->dev.parent);
> + if (ACPI_SUCCESS(cmpc_start_accel(acpi->handle)))
> + return 0;
> + return -EIO;
> +}
> +
> +static void cmpc_accel_close(struct input_dev *input)
> +{
> + struct acpi_device *acpi;
> + acpi = to_acpi_device(input->dev.parent);
> + cmpc_stop_accel(acpi->handle);
> +}
> +
> +static void cmpc_accel_idev_init(struct input_dev *inputdev)
> +{
> + set_bit(EV_ABS, inputdev->evbit);
> + input_set_abs_params(inputdev, ABS_X, 0, 255, 8, 0);
> + input_set_abs_params(inputdev, ABS_Y, 0, 255, 8, 0);
> + input_set_abs_params(inputdev, ABS_Z, 0, 255, 8, 0);
> + inputdev->open = cmpc_accel_open;
> + inputdev->close = cmpc_accel_close;
> +}
> +
> +static int cmpc_accel_add(struct acpi_device *acpi)
> +{
> + int error;
> + error = device_create_file(&acpi->dev, &cmpc_accel_sense_attr);
> + if (error)
> + return error;
> + return cmpc_add_acpi_notify_device(acpi, "cmpc_accel",
> + cmpc_accel_handler,
> + cmpc_accel_idev_init);
> +}
> +
> +static int cmpc_accel_remove(struct acpi_device *acpi, int type)
> +{
> + device_remove_file(&acpi->dev, &cmpc_accel_sense_attr);
> + return cmpc_remove_acpi_notify_device(acpi, cmpc_accel_handler);
> +}
> +
> +static const struct acpi_device_id cmpc_accel_device_ids[] = {
> + {"ACCE0000", 0},
> + {"", 0}
> +};
> +MODULE_DEVICE_TABLE(acpi, cmpc_accel_device_ids);
> +
> +static struct acpi_driver cmpc_accel_acpi_driver = {
> + .name = "cmpc_accel",
> + .class = "cmpc_accel",
> + .ids = cmpc_accel_device_ids,
> + .ops = {
> + .add = cmpc_accel_add,
> + .remove = cmpc_accel_remove
> + }
> +};
> +
> +static bool cmpc_accel_driver_registered;
> +
> +
> +/*
> + * Tablet mode code.
> + */
> +static acpi_status cmpc_get_tablet(acpi_handle handle,
> + unsigned long long *value)
> +{
> + union acpi_object param;
> + struct acpi_object_list input;
> + unsigned long long output;
> + acpi_status status;
> + param.type = ACPI_TYPE_INTEGER;
> + param.integer.value = 0x01;
> + input.count = 1;
> + input.pointer = &param;
> + status = acpi_evaluate_integer(handle, "TCMD", &input, &output);
> + if (ACPI_SUCCESS(status))
> + *value = output;
> + return status;
> +}
> +
> +static void cmpc_tablet_handler(acpi_handle handle, u32 event, void *ctx)
> +{
> + unsigned long long val = 0;
> + struct input_dev *inputdev = ctx;
> + if (event == 0x81) {
> + if (ACPI_SUCCESS(cmpc_get_tablet(handle, &val)))
> + input_report_switch(inputdev, SW_TABLET_MODE, !val);
> + }
> +}
> +
> +static void cmpc_tablet_idev_init(struct input_dev *inputdev)
> +{
> + set_bit(EV_SW, inputdev->evbit);
> + set_bit(SW_TABLET_MODE, inputdev->swbit);
> +}
> +
> +static int cmpc_tablet_add(struct acpi_device *acpi)
> +{
> + return cmpc_add_acpi_notify_device(acpi, "cmpc_tablet",
> + cmpc_tablet_handler,
> + cmpc_tablet_idev_init);
> +}
> +
> +static int cmpc_tablet_remove(struct acpi_device *acpi, int type)
> +{
> + return cmpc_remove_acpi_notify_device(acpi, cmpc_tablet_handler);
> +}
> +
> +static const struct acpi_device_id cmpc_tablet_device_ids[] = {
> + {"TBLT0000", 0},
> + {"", 0}
> +};
> +MODULE_DEVICE_TABLE(acpi, cmpc_tablet_device_ids);
> +
> +static struct acpi_driver cmpc_tablet_acpi_driver = {
> + .name = "cmpc_tablet",
> + .class = "cmpc_tablet",
> + .ids = cmpc_tablet_device_ids,
> + .ops = {
> + .add = cmpc_tablet_add,
> + .remove = cmpc_tablet_remove
> + }
> +};
> +
> +static bool cmpc_tablet_driver_registered;
> +
> +
> +/*
> + * Backlight code.
> + */
> +
> +static acpi_status cmpc_get_brightness(acpi_handle handle,
> + unsigned long long *value)
> +{
> + union acpi_object param;
> + struct acpi_object_list input;
> + unsigned long long output;
> + acpi_status status;
> + param.type = ACPI_TYPE_INTEGER;
> + param.integer.value = 0xC0;
> + input.count = 1;
> + input.pointer = &param;
> + status = acpi_evaluate_integer(handle, "GRDI", &input, &output);
> + if (ACPI_SUCCESS(status))
> + *value = output;
> + return status;
> +}
> +
> +static acpi_status cmpc_set_brightness(acpi_handle handle,
> + unsigned long long value)
> +{
> + union acpi_object param[2];
> + struct acpi_object_list input;
> + acpi_status status;
> + unsigned long long output;
> + param[0].type = ACPI_TYPE_INTEGER;
> + param[0].integer.value = 0xC0;
> + param[1].type = ACPI_TYPE_INTEGER;
> + param[1].integer.value = value;
> + input.count = 2;
> + input.pointer = param;
> + status = acpi_evaluate_integer(handle, "GWRI", &input, &output);
> + return status;
> +}
> +
> +static int cmpc_bl_get_brightness(struct backlight_device *bd)
> +{
> + acpi_status status;
> + acpi_handle handle;
> + unsigned long long brightness;
> + handle = bl_get_data(bd);
> + status = cmpc_get_brightness(handle, &brightness);
> + if (ACPI_SUCCESS(status))
> + return brightness;
> + else
> + return -1;
> +}
> +
> +static int cmpc_bl_update_status(struct backlight_device *bd)
> +{
> + acpi_status status;
> + acpi_handle handle;
> + handle = bl_get_data(bd);
> + status = cmpc_set_brightness(handle, bd->props.brightness);
> + if (ACPI_SUCCESS(status))
> + return 0;
> + else
> + return -1;
> +}
> +
> +static struct backlight_ops cmpc_bl_ops = {
> + .get_brightness = cmpc_bl_get_brightness,
> + .update_status = cmpc_bl_update_status
> +};
> +
> +static int cmpc_bl_add(struct acpi_device *acpi)
> +{
> + struct backlight_device *bd;
> + bd = backlight_device_register("cmpc_bl", &acpi->dev,
> + acpi->handle, &cmpc_bl_ops);
> + bd->props.max_brightness = 7;
> + dev_set_drvdata(&acpi->dev, bd);
> + return 0;
> +}
> +
> +static int cmpc_bl_remove(struct acpi_device *acpi, int type)
> +{
> + struct backlight_device *bd;
> + bd = dev_get_drvdata(&acpi->dev);
> + backlight_device_unregister(bd);
> + return 0;
> +}
> +
> +static const struct acpi_device_id cmpc_device_ids[] = {
> + {"IPML200", 0},
> + {"", 0}
> +};
> +MODULE_DEVICE_TABLE(acpi, cmpc_device_ids);
> +
> +static struct acpi_driver cmpc_bl_acpi_driver = {
> + .name = "cmpc",
> + .class = "cmpc",
> + .ids = cmpc_device_ids,
> + .ops = {
> + .add = cmpc_bl_add,
> + .remove = cmpc_bl_remove
> + }
> +};
> +
> +static bool cmpc_bl_driver_registered;
> +
> +
> +/*
> + * Extra keys code.
> + */
> +static int cmpc_keys_codes[] = {
> + KEY_UNKNOWN,
> + KEY_WLAN,
> + KEY_SWITCHVIDEOMODE,
> + KEY_BRIGHTNESSDOWN,
> + KEY_BRIGHTNESSUP,
> + KEY_VENDOR,
> + KEY_MAX
> +};
> +
> +static void cmpc_keys_handler(acpi_handle handle, u32 event, void *ctx)
> +{
> + struct input_dev *inputdev;
> + int code = KEY_MAX;
> + if ((event & 0x0F) < ARRAY_SIZE(cmpc_keys_codes))
> + code = cmpc_keys_codes[event & 0x0F];
> + inputdev = ctx;
> + input_report_key(inputdev, code, !(event & 0x10));
> +}
> +
> +static void cmpc_keys_idev_init(struct input_dev *inputdev)
> +{
> + int i;
> + set_bit(EV_KEY, inputdev->evbit);
> + for (i = 0; cmpc_keys_codes[i] != KEY_MAX; i++)
> + set_bit(cmpc_keys_codes[i], inputdev->keybit);
> +}
> +
> +static int cmpc_keys_add(struct acpi_device *acpi)
> +{
> + return cmpc_add_acpi_notify_device(acpi, "cmpc_keys",
> + cmpc_keys_handler,
> + cmpc_keys_idev_init);
> +}
> +
> +static int cmpc_keys_remove(struct acpi_device *acpi, int type)
> +{
> + return cmpc_remove_acpi_notify_device(acpi, cmpc_keys_handler);
> +}
> +
> +static const struct acpi_device_id cmpc_keys_device_ids[] = {
> + {"FnBT0000", 0},
> + {"", 0}
> +};
> +MODULE_DEVICE_TABLE(acpi, cmpc_keys_device_ids);
> +
> +static struct acpi_driver cmpc_keys_acpi_driver = {
> + .name = "cmpc_keys",
> + .class = "cmpc_keys",
> + .ids = cmpc_keys_device_ids,
> + .ops = {
> + .add = cmpc_keys_add,
> + .remove = cmpc_keys_remove
> + }
> +};
> +
> +static bool cmpc_keys_driver_registered;
> +
> +
> +/*
> + * General init/exit code.
> + */
> +
> +static int cmpc_init(void)
> +{
> + int result;
> + result = acpi_bus_register_driver(&cmpc_keys_acpi_driver);
> + cmpc_keys_driver_registered = !!result;
> + result = acpi_bus_register_driver(&cmpc_bl_acpi_driver);
> + cmpc_bl_driver_registered = !!result;
> + result = acpi_bus_register_driver(&cmpc_tablet_acpi_driver);
> + cmpc_tablet_driver_registered = !!result;
> + result = acpi_bus_register_driver(&cmpc_accel_acpi_driver);
> + cmpc_accel_driver_registered = !!result;
> + /*
> + * Not every CMPC has every ACPI device supported here. So always return
> + * success.
> + */
> + return 0;
> +}
> +
> +static void cmpc_exit(void)
> +{
> + if (cmpc_accel_driver_registered)
> + acpi_bus_unregister_driver(&cmpc_accel_acpi_driver);
> + if (cmpc_tablet_driver_registered)
> + acpi_bus_unregister_driver(&cmpc_tablet_acpi_driver);
> + if (cmpc_bl_driver_registered)
> + acpi_bus_unregister_driver(&cmpc_bl_acpi_driver);
> + if (cmpc_keys_driver_registered)
> + acpi_bus_unregister_driver(&cmpc_keys_acpi_driver);
> +}
> +
> +module_init(cmpc_init);
> +module_exit(cmpc_exit);


--
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: Thadeu Lima de Souza Cascardo on 30 Sep 2009 14:00
On Wed, Sep 30, 2009 at 09:50:38AM -0600, Bjorn Helgaas wrote:
> On Monday 28 September 2009 07:38:00 pm Thadeu Lima de Souza Cascardo wrote:
> > This add supports for devices like keyboard, backlight, tablet and
> > accelerometer.
>
> Something about the PNP IDs bothered me yesterday, but I couldn't
> figure out what. You're using:
>
> ACCE0000
> TBLT0000
> IPML200
> FnBT0000
>
> Normally these OEM-specific ACPI drivers claim PNP IDs like ATK0100,
> TOS6200, FUJ02B1, etc., that are obviously OEM-specific.
>
> But the ones you're claiming seem pretty generic, like things
> we could imagine appearing on machines other than the Classmate.
>
> So I guess my question is, "Are these really generic? If so, should
> this be split into separate, non-Classmate named drivers, such as
> 'accelerometer', 'tablet', etc.?"
>

Although I agree that {ATK,TOS,FUJ,IBM}* give pretty much a certainty
these are OEM-specific, I think we'd need some evidence that
{ACCE,TBLT,IPML,FnBT}* are generic enough to grant them a non-classmate
driver.

What I mean is that we should push this driver forward and turn these
into a generic driver if we get some evidence these are not
Classmate-specific. And that will happen when we hit some other system
with these devices.

And what would be the consequences to this decision? Renaming the
driver, changing the config location and documentation. Splitting them.
I can't think of anything else. If splitting them would be interesting
by now, we can do it.

That's my opinion. Please, tell me what you think of it.

> Bjorn
>

Thanks for the comments and best regards,
Cascardo.
From: Bjorn Helgaas on 30 Sep 2009 14:30
On Wednesday 30 September 2009 11:51:43 am Thadeu Lima de Souza Cascardo wrote:
> On Wed, Sep 30, 2009 at 09:50:38AM -0600, Bjorn Helgaas wrote:
> > On Monday 28 September 2009 07:38:00 pm Thadeu Lima de Souza Cascardo wrote:
> > > This add supports for devices like keyboard, backlight, tablet and
> > > accelerometer.
> >
> > Something about the PNP IDs bothered me yesterday, but I couldn't
> > figure out what. You're using:
> >
> > ACCE0000
> > TBLT0000
> > IPML200
> > FnBT0000
> >
> > Normally these OEM-specific ACPI drivers claim PNP IDs like ATK0100,
> > TOS6200, FUJ02B1, etc., that are obviously OEM-specific.
> >
> > But the ones you're claiming seem pretty generic, like things
> > we could imagine appearing on machines other than the Classmate.
> >
> > So I guess my question is, "Are these really generic? If so, should
> > this be split into separate, non-Classmate named drivers, such as
> > 'accelerometer', 'tablet', etc.?"
> >
>
> Although I agree that {ATK,TOS,FUJ,IBM}* give pretty much a certainty
> these are OEM-specific, I think we'd need some evidence that
> {ACCE,TBLT,IPML,FnBT}* are generic enough to grant them a non-classmate
> driver.
>
> What I mean is that we should push this driver forward and turn these
> into a generic driver if we get some evidence these are not
> Classmate-specific. And that will happen when we hit some other system
> with these devices.

Yep, I agree with you that it's not worth splitting things up until
we find these devices on more systems. Just out of curiosity, where
did you find specs for these devices? If there's a public spec, it'd
be nice to have a pointer to it in the driver.

Bjorn
--
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/
First  |  Prev  |  Next  |  Last
Pages: 1 2 3
Prev: tracing: Fix infinite loop in ftrace_update_pid_func()
Next: cpu affinity issue on kvm-88