Messages in this thread | | | Date | Fri, 11 Feb 2005 11:30:35 +0100 | From | Jean Delvare <> | Subject | Re: [PATCH, new ACPI driver] new sony_acpi driver |
| |
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 - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
| |