On Fri, Feb 11, 2005 at 11:30:35AM +0100, Jean Delvare wrote:

Hi Stelian, all,

quoted

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.

thanks.

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

quoted

+static int debug = 0;

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

sure.

quoted

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

done.

quoted

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

of course.

quoted

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

done.

quoted

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

quoted

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

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

Indeed, sscanf calls simple_strtoul.

quoted

--- /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.

I agree. Let's see if the acpi people follow :)

quoted

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

doh :)

quoted

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

yup.

quoted

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

Agreed.

quoted

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

correct.

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

Hey, that's not so bad :)

An updated version will follow.

Stelian.
-- 
Stelian Pop [off-list ref]