cmpc_acpi: Added support for Classmate PC ACPI devices.

Prev: tracing: Fix infinite loop in ftrace_update_pid_func()
Next: cpu affinity issue on kvm-88

From: Alan Jenkins on 29 Sep 2009 05:30

---

On 9/29/09, 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.
>
> Signed-off-by: Thadeu Lima de Souza Cascardo <cascardo(a)holoscopio.com>

Hi!

In general this driver was a pleasure to read. It looks like you have
a nice and clean ACPI interface to work with. I haven't seen anything
quite like the cmpc_{add,remove}_acpi_notify_device() functions
before, but they make a lot of sense here.

But I do have a few comments :-).

The "-laptop" naming convention is more informative than "-acpi".
Perhaps this driver should be called "classmate-laptop". I would keep
the cmpc_ prefix in the source code though.

More comments inline.

> +/*
> + * 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);

Dmitry the input maintainer says "input_free_device() should not be
called after input_unregister_device()." (This also applies to the
error handling in add_acpi_notify_device()).

> + 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
> +};

What does this do? What range of values can it take? Is it a common
attribute for input devices, or does it need specific documentation?

Would it make sense for the driver to initialize it to a default
value, so that we would always know what the current value is, and
provide a .show() method as well?

> +
> +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
> + }

Could you please set

.owner = THIS_MODULE,

on all the acpi drivers? It will provide data in
/sys/module/cmpc_acpi/drivers and
/sys/bus/acpi/drivers/<driver>/module. It seems to be forgotten
knowledge, but I'm trying to revive it :-).

> +};
> +
> +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);

Don't you need to initialize the switch value here?

Also, have you tested this with changing the switch value while
suspended? I _think_ you need to update the switch state on resume.
This is purely from looking at other acpi drivers and their evolution;
I don't have any practical experience with input drivers.

> +}
> +
> +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,

Please confirm that the brightness keys do not directly affect the
backlight, and these key events are a signal for userspace to adjust
the backlight itself.

If they _do_ affect the backlight, you should call the newly added
backlight_force_update() when they are pressed, and it will generate a
uevent on the backlight device. (I guess you will have to add an ugly
global variable for the backlight device, sorry about that). In this
case you should ideally avoid generating _input_ events for brightness
keys.

Currently userspace is expected to handle annoying devices which
generate KEY_BRIGHTNESS* while directly changing the brightness. But
requires a racy hack, so you should definitely try to avoid this for
new devices. It will encourage userspace to implement support for the
backlight device uevents :-).

> + 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;

These _registered flags are not necessary. Registration doesn't fail
if the hardware doesn't exist. It only fails if acpi=off, or on
-ENOMEM. It would be reasonable to fail loading the module if one of
the drivers fails to register.

> + /*
> + * 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);
> --
> 1.6.3.3
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
> the body of a message to majordomo(a)vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
--
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 29 Sep 2009 10:20

---

On Tue, Sep 29, 2009 at 10:20:44AM +0100, Alan Jenkins wrote:
> On 9/29/09, 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.
> >
> > Signed-off-by: Thadeu Lima de Souza Cascardo <cascardo(a)holoscopio.com>
>
> Hi!
>
> In general this driver was a pleasure to read. It looks like you have
> a nice and clean ACPI interface to work with. I haven't seen anything
> quite like the cmpc_{add,remove}_acpi_notify_device() functions
> before, but they make a lot of sense here.
>
> But I do have a few comments :-).
>
> The "-laptop" naming convention is more informative than "-acpi".
> Perhaps this driver should be called "classmate-laptop". I would keep
> the cmpc_ prefix in the source code though.
>
> More comments inline.
>

Hello, Allan.

Thanks for the feedback. I am considering an investigation whether we
have lots of other ACPI input devices that could share some code like
{add,remove}_acpi_notify_device. Regarding the driver naming, I will
send a second version with it and other modifications considering your
feedback and that of other people too.

I will also ask for some explicit feedback and add linux-input and
Dmitry to the loop.

> > +/*
> > + * 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);
>
> Dmitry the input maintainer says "input_free_device() should not be
> called after input_unregister_device()." (This also applies to the
> error handling in add_acpi_notify_device()).
>

That's right. My mistake here. And I've hit this mistake before.
input_free_device calls put_device (through input_put_device) and
input_unregister_device calls device_unregister, which also calls
put_device.

> > + 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
> > +};
>
> What does this do? What range of values can it take? Is it a common
> attribute for input devices, or does it need specific documentation?
>
> Would it make sense for the driver to initialize it to a default
> value, so that we would always know what the current value is, and
> provide a .show() method as well?
>

This changes the accelerometer device sensitivity. I will take a look at
the ACPI tables to get a range. If very much sensitive, the device will
generate too much ACPI notifications, even when the classmate PC is no a
table and there seems to be no movement. If not very much sensitive, you
must throw it spinning into the air to get anything. :-)

I will pick a default value, although I think we could let it to
userspace, but a sensible default value will not hurt here. As far as I
know, there is no ACPI method to do get_sense, but we can mirror the
last value written and provide a .show.

What do you recommend as a documentation? Something at
Documentation/ABI/testing/, perhaps? I don't know if there are any other
devices with something similar, but I would not be surprised if there
are some of them.

> > +
> > +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);

Any hints on how to pick up values for this calibration? Any testing
procedure or anything?

> > + 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
> > + }
>
> Could you please set
>
> .owner = THIS_MODULE,
>
> on all the acpi drivers? It will provide data in
> /sys/module/cmpc_acpi/drivers and
> /sys/bus/acpi/drivers/<driver>/module. It seems to be forgotten
> knowledge, but I'm trying to revive it :-).
>

Consider it done. Although I think this could be done automatically
someday.

> > +};
> > +
> > +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);
>
> Don't you need to initialize the switch value here?
>

No, input_allocate_device does kzalloc. Otherwise, this would apply to
the other bitmaps as well.

> Also, have you tested this with changing the switch value while
> suspended? I _think_ you need to update the switch state on resume.
> This is purely from looking at other acpi drivers and their evolution;
> I don't have any practical experience with input drivers.
>

Interesting point. I will do the testing.

> > +}
> > +
> > +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,
>
> Please confirm that the brightness keys do not directly affect the
> backlight, and these key events are a signal for userspace to adjust
> the backlight itself.
>

AFAIK, they don't. We even had to write a simple userspace daemon that
would change the backlight values through sysfs when receiving an input
event.

> If they _do_ affect the backlight, you should call the newly added
> backlight_force_update() when they are pressed, and it will generate a
> uevent on the backlight device. (I guess you will have to add an ugly
> global variable for the backlight device, sorry about that). In this
> case you should ideally avoid generating _input_ events for brightness
> keys.
>
> Currently userspace is expected to handle annoying devices which
> generate KEY_BRIGHTNESS* while directly changing the brightness. But
> requires a racy hack, so you should definitely try to avoid this for
> new devices. It will encourage userspace to implement support for the
> backlight device uevents :-).
>
> > + KEY_VENDOR,

This key is at the side of the LCD panel, not on the keyboard proper and
it has the drawing of a house. Any suggestions besides using 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;
>
> These _registered flags are not necessary. Registration doesn't fail
> if the hardware doesn't exist. It only fails if acpi=off, or on
> -ENOMEM. It would be reasonable to fail loading the module if one of
> the drivers fails to register.
>

OK. I had some doubts when writing it this way. I will rewrite it.

> > + /*
> > + * 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);
> > --
> > 1.6.3.3

Regards,
Thadeu Cascardo.

From: Bjorn Helgaas on 29 Sep 2009 12:20

---

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.
>
> This work is supported by International Syst S/A.

You should supply .notify() methods in the acpi_driver structs --
then you can get rid of cmpc_add_acpi_notify_device() (or maybe just
have it do the input_allocate_device()).

Please insert a blank line between the automatic variable list
and the first line of executable code.

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: Alan Jenkins on 29 Sep 2009 12:50

---

On 9/29/09, Thadeu Lima de Souza Cascardo <cascardo(a)holoscopio.com> wrote:
> On Tue, Sep 29, 2009 at 10:20:44AM +0100, Alan Jenkins wrote:
> Hello, Allan.
>
> Thanks for the feedback. I am considering an investigation whether we
> have lots of other ACPI input devices that could share some code like
> {add,remove}_acpi_notify_device. Regarding the driver naming, I will
> send a second version with it and other modifications considering your
> feedback and that of other people too.
>
> I will also ask for some explicit feedback and add linux-input and
> Dmitry to the loop.
>

I'll leave the input questions for the list. I don't have any
suggestions about your choice of keycode for the house key, or how to
calibrate accelerometers.

>> > +struct device_attribute cmpc_accel_sense_attr = {
>> > + .attr = { .name = "sense", .mode = 0220 },
>> > + .store = cmpc_accel_sense_store
>> > +};

> This changes the accelerometer device sensitivity. I will take a look at
> the ACPI tables to get a range. If very much sensitive, the device will
> generate too much ACPI notifications, even when the classmate PC is no a
> table and there seems to be no movement. If not very much sensitive, you
> must throw it spinning into the air to get anything. :-)
>
> I will pick a default value, although I think we could let it to
> userspace, but a sensible default value will not hurt here. As far as I
> know, there is no ACPI method to do get_sense, but we can mirror the
> last value written and provide a .show.
>
> What do you recommend as a documentation? Something at
> Documentation/ABI/testing/, perhaps? I don't know if there are any other
> devices with something similar, but I would not be surprised if there
> are some of them.

Documentation/ABI seems reasonable; "sysfs-platform-eeepc-laptop" was
accepted there.

You could try (either in place of documentation, or in addition to)
making the interface self-explanatory. Call the attribute
"sensitivity", and add a "max_sensitivity" attribute. It would also
make the interface more generic.

>> > +static void cmpc_tablet_idev_init(struct input_dev *inputdev)
>> > +{
>> > + set_bit(EV_SW, inputdev->evbit);
>> > + set_bit(SW_TABLET_MODE, inputdev->swbit);
>>
>> Don't you need to initialize the switch value here?
>>
>
> No, input_allocate_device does kzalloc. Otherwise, this would apply to
> the other bitmaps as well.

The other bitmaps aren't for switches though. Here's what prompted
this comment - a snippet from the old (2.6.29) version of
Documentation/rfkill.txt:

"2. Input device switches (sources of EV_SW events) DO store their current state
(so you *must* initialize it by issuing a gratuitous input layer event on
driver start-up and also when resuming from sleep), and that state CAN be
queried from userspace through IOCTLs."

>> Also, have you tested this with changing the switch value while
>> suspended? I _think_ you need to update the switch state on resume.
>> This is purely from looking at other acpi drivers and their evolution;
>> I don't have any practical experience with input drivers.
>>
>
> Interesting point. I will do the testing.

Thanks
Alan
--
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: Andrew Morton on 29 Sep 2009 16:10

---

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.

>
> ...
>
> +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?

--
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 3
Prev: tracing: Fix infinite loop in ftrace_update_pid_func()
Next: cpu affinity issue on kvm-88