Message-ID: <20090929141618.GD18847@vespa.holoscopio.com>
Date:	Tue, 29 Sep 2009 11:16:19 -0300
From:	Thadeu Lima de Souza Cascardo <cascardo@...oscopio.com>
To:	Alan Jenkins <sourcejedi.lkml@...glemail.com>
Cc:	linux-kernel@...r.kernel.org, len.brown@...el.com, don@...t.com.br,
	linux-acpi@...r.kernel.org, linux-input@...r.kernel.org,
	dtor@...l.ru, dmitry.torokhov@...il.com
Subject: Re: [PATCH] cmpc_acpi: Added support for Classmate PC ACPI devices.

On Tue, Sep 29, 2009 at 10:20:44AM +0100, Alan Jenkins wrote:
> On 9/29/09, Thadeu Lima de Souza Cascardo <cascardo@...oscopio.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@...oscopio.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.

Download attachment "signature.asc" of type "application/pgp-signature" (198 bytes)

Powered by blists - more mailing lists