Re: [PATCH, new ACPI driver] new sony_acpi driver

[Jean Delvare <khali@linux-fr.org>]

Hi Stelian, all,

> This driver has been submitted (almost unchanged) on lkml and 
> on acpi-devel twice, first on July 21, 2004, then again on
> September 17, 2004. It has been quietly ignored.
> 
> Privately I've had many positive feedbacks from users of this driver
> (and no negative feedback), including Linux distributions who wish
> to include it into their kernels. The reports are increasing in number,
> it would seem that newer Sony Vaios are more and more incompatible
> with sonypi and require sony_acpi to control the screen brightness.

I'm sorry I missed the first two announcements. I'll give a try to your 
driver this evening, and report how it is working for me.

In the meantime, I'd have some comments on your patch:

> +static int debug = 0;

No need to initialize it to 0, the compiler does it for you (and more 
efficiently at that).

> +module_param(debug, int, 0);
> +MODULE_PARM_DESC(debug,"set this to 1 (and RTFM) if you want to help the development of this driver");

Lack of space after comma, and line too long, split it please.

> +static struct acpi_driver sony_acpi_driver = {
> +	name:	ACPI_SNC_DRIVER_NAME,
> +	class:	ACPI_SNC_CLASS,
> +	ids:	ACPI_SNC_HID,
> +	ops:	{
> +			add:	sony_acpi_add,
> +			remove:	sony_acpi_remove,
> +		},
> +};

As far as I know, you are supposed to use C99-style for structure 
initialization:
     .name = ACPI_SNC_DRIVER_NAME,
etc.

> +	printk(KERN_WARNING "acpi_callreadfunc failed\n");

Please prepend the driver name before such messages (everywhere in the 
driver). It's annoying when you see error messages in your logs and 
don't know which driver caused them.

> +static int parse_buffer(const char __user *buffer, unsigned long count, int *val) {
> +	char s[32];
> +	
> +	if (count > 31)
> +		return -EINVAL;
> +	if (copy_from_user(s, buffer, count))
> +		return -EFAULT;
> +	s[count] = 0;

= '\0' would look better IMHO.

> +	if (sscanf(s, "%i", val) != 1)

Can't you use simple_strtoul instead? This would be more efficient, or 
so I guess.

> --- /dev/null	2005-02-10 10:35:32.824183288 +0100
> +++ linux-2.6-stelian/Documentation/sony_acpi.txt	2005-01-31 17:00:09.000000000 +0100

Could be the time to create a Documentation/acpi directory and start 
populating it. It's odd that such a large subsystem has no documentation 
in the kernel tree.

> +You should start by trying the sonypi driver, which does
> +all this and many other things. But the sonypi driver does
> +not work on all sonypi laptops, whereas sony_acpi should 
> +work everywhere.

s/all sonypi laptops/all Sony laptops/?

> +Loading the sony_acpi.ko module will create a /proc/acpi/sony/
> +directory populated with a couple of files (only one for the
> +moment).

s/sony_acpi\.ko/sony_acpi/
.ko is an implementation detail the user-space should not have to care 
about.

> +For example:
> +	# echo "1" > /proc/acpi/sony/brt

BTW, why not naming the file "brightness" instead? Most acpi files have 
"long" names like that. At least people can easily figure out what the 
files are for.

> +config ACPI_SONY
> +	tristate "Sony Laptop Extras" 
> +	depends on X86
> +	depends on ACPI_INTERPRETER
> +	default m
> +	  ---help---
> +	  This mini-driver drives the ACPI SNC device present in the
> +	  ACPI BIOS of the Sony Vaio laptops.
> +
> +	  It gives access to some extra laptop functionalities. In
> +	  its current form, the only thing this driver does is letting 
> +	  the user set or query the screen brightness.
> +
> +	  Read <Documentation/sony_acpi.txt> for more information.

I think I remember this should be <file:Documentation...> or something 
similar.

Note that I have next to zero knowledge of the acpi internals so I 
couldn't comment on that part.

Thanks,
-- 
Jean Delvare