[PATCH 0/3] Generic sysfs support for ACPI ALS and other ALS devices

[PATCH 0/3] Generic sysfs support for ACPI ALS and other ALS devices

[Zhang Rui]

Hi, all,

This is the patch set I made to introduce ACPI ALS device driver
and a generic sysfs I/F for all the ALS devices, like ACPI ALS,
platform ALS, etc.

Patch 01 introduces the ACPI ALS device driver.

Patch 02 introduces ALS sysfs class.
	Two sysfs I/F are created for each ALS device.
  /sys/class/als/alsX/illuminance:
	the amount of light incident upon a specified surface area.
  /sys/class/als/alsX/mappings:
	ambient light illuminance to display luminance mappings
	that can be used by an OS to calibrate its ambient light policy
	this is what I got on a test box:
	cat /sys/class/als/als0/mappings
	Illuminance	Adjustment
	          0	       70
	         10	       73
	         80	       85
	        300	      100
	       1000	      150
	 - noting that display luminance adjustment values are specified
	using relative percentages in order simplify the means by which
	these adjustments are applied in lieu of changes to the user’s
	display brightness preference.

Patch 03 introduces the generic sysfs I/F for ACPI ALS devices.

Patch 02/03 are RFC patches because I'm not sure if these sysfs I/F are
generic enough for other ALS devices, and if there is any attribute that
I missed.
For example, ACPI ALS has some optional properties like ambient light
temperature and ambient light color chromaticity, but I'm not sure if
they should be exported to user spaces via ALS sysfs class.

Any comments are welcome.

thanks,
rui

Re: [PATCH 0/3] Generic sysfs support for ACPI ALS and other ALS devices

[ykzhao]

On Mon, 2009-08-03 at 17:10 +0800, Zhang Rui wrote:
> Hi, all,
> 
> This is the patch set I made to introduce ACPI ALS device driver
> and a generic sysfs I/F for all the ALS devices, like ACPI ALS,
> platform ALS, etc.
> 
> Patch 01 introduces the ACPI ALS device driver.
Great jobs.
But I still have two questions about the patch set.
1. _ALP polling method
   This optional object returns the recommended polling frequency for
the ambient sensor. And OSPM can use the recommended polling frequency
to poll the ambient sensor.
   Of course the spec 9.2.6 also says that the use of polling is allowed
but strongly discouraged by this specification. In such case we had
better not use the polling frequency. Instead it will totally depend on
the async notification event.
   Can some message be printed that the polling frequency is depreciated
when the _ALP is not zero?
   
2. what is the ALS policy and how to use it?
   The main purpose of ambient sensor is to adjust the brightness
according to the current luminance environment. From this patch set it
seems that this will be done in user space. But it is not very clear how
to adjust the brightness based on the current luminance. 
   Can we describe it more clearly?
  
  
Another  little issue is the memory leak in the patch 1.
When it receives the notification event(0x82), it will re-evaluate the
mapping between brightness and luminance. The memory space occupied by
previous mapping should be freed.

Thanks.
    
   
> 
> Patch 02 introduces ALS sysfs class.
> 	Two sysfs I/F are created for each ALS device.
>   /sys/class/als/alsX/illuminance:
> 	the amount of light incident upon a specified surface area.
>   /sys/class/als/alsX/mappings:
> 	ambient light illuminance to display luminance mappings
> 	that can be used by an OS to calibrate its ambient light policy
> 	this is what I got on a test box:
> 	cat /sys/class/als/als0/mappings
> 	Illuminance	Adjustment
> 	          0	       70
> 	         10	       73
> 	         80	       85
> 	        300	      100
> 	       1000	      150
> 	 - noting that display luminance adjustment values are specified
> 	using relative percentages in order simplify the means by which
> 	these adjustments are applied in lieu of changes to the user’s
> 	display brightness preference.
> 
> Patch 03 introduces the generic sysfs I/F for ACPI ALS devices.
> 
> Patch 02/03 are RFC patches because I'm not sure if these sysfs I/F are
> generic enough for other ALS devices, and if there is any attribute that
> I missed.
> For example, ACPI ALS has some optional properties like ambient light
> temperature and ambient light color chromaticity, but I'm not sure if
> they should be exported to user spaces via ALS sysfs class.
> 
> Any comments are welcome.
> 
> thanks,
> rui
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 0/3] Generic sysfs support for ACPI ALS and other ALS devices

[Zhang Rui]

On Tue, 2009-08-04 at 09:12 +0800, Zhao, Yakui wrote:
> On Mon, 2009-08-03 at 17:10 +0800, Zhang Rui wrote:
> > Hi, all,
> > 
> > This is the patch set I made to introduce ACPI ALS device driver
> > and a generic sysfs I/F for all the ALS devices, like ACPI ALS,
> > platform ALS, etc.
> > 
> > Patch 01 introduces the ACPI ALS device driver.
> Great jobs.
> But I still have two questions about the patch set.
> 1. _ALP polling method
>    This optional object returns the recommended polling frequency for
> the ambient sensor. And OSPM can use the recommended polling frequency
> to poll the ambient sensor.
>    Of course the spec 9.2.6 also says that the use of polling is allowed
> but strongly discouraged by this specification. In such case we had
> better not use the polling frequency. Instead it will totally depend on
> the async notification event.
>    Can some message be printed that the polling frequency is depreciated
> when the _ALP is not zero?

In fact, currently the driver just follows ACPI spec to support _ALP
method for ACPI ALS device. But _ALP return value is not used at all.

As we don't have any platform that _ALP is actually implemented, why not
leave the code there for now. And a warning message is reasonable.

> 2. what is the ALS policy and how to use it?
>    The main purpose of ambient sensor is to adjust the brightness
> according to the current luminance environment. From this patch set it
> seems that this will be done in user space. But it is not very clear how
> to adjust the brightness based on the current luminance. 
>    Can we describe it more clearly?
> 
Generally, in order to make ALS work, user space needs to:
1. set an backlight value as the base point, i.e. the display brightness
   level when the display luminance adjustment value is 100%.
2. when the ambient light illuminance changes, find a proper element in
   /sys/class/als/als0/mappings, and get the display adjustment value.
3. use this adjustment value to get the proper display brightness
   level and set it via the backlight sysfs I/F.

I'll add the ALS documentation to describe how to use this sysfs I/F,
if the current approach is accepted. :)

>   
> Another  little issue is the memory leak in the patch 1.
> When it receives the notification event(0x82), it will re-evaluate the
> mapping between brightness and luminance. The memory space occupied by
> previous mapping should be freed.
> 
good catch.

refreshed patch has been sent out.

thanks,
rui

Re: [PATCH 0/3] Generic sysfs support for ACPI ALS and other ALS devices

[Pavel Machek]

On Mon 2009-08-03 17:10:57, Zhang Rui wrote:
> Hi, all,
> 
> This is the patch set I made to introduce ACPI ALS device driver
> and a generic sysfs I/F for all the ALS devices, like ACPI ALS,
> platform ALS, etc.
> 
> Patch 01 introduces the ACPI ALS device driver.
> 
> Patch 02 introduces ALS sysfs class.
> 	Two sysfs I/F are created for each ALS device.
>   /sys/class/als/alsX/illuminance:
> 	the amount of light incident upon a specified surface area.
>   /sys/class/als/alsX/mappings:
> 	ambient light illuminance to display luminance mappings
> 	that can be used by an OS to calibrate its ambient light policy
> 	this is what I got on a test box:
> 	cat /sys/class/als/als0/mappings
> 	???Illuminance	Adjustment
> 	          0	       70
> 	         10	       73
> 	         80	       85
> 	        300	      100
> 	       1000	      150

There's one value per file for sysfs... You should definitely have the
header. 

Is there chance to already return adjusted values, avoiding this
uglyness?

Plus I'd say Documentation/ file is needed.

									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [PATCH 0/3] Generic sysfs support for ACPI ALS and other ALS devices

[Greg KH]

On Tue, Aug 04, 2009 at 03:21:29PM +0200, Pavel Machek wrote:
> On Mon 2009-08-03 17:10:57, Zhang Rui wrote:
> > Hi, all,
> > 
> > This is the patch set I made to introduce ACPI ALS device driver
> > and a generic sysfs I/F for all the ALS devices, like ACPI ALS,
> > platform ALS, etc.
> > 
> > Patch 01 introduces the ACPI ALS device driver.
> > 
> > Patch 02 introduces ALS sysfs class.
> > 	Two sysfs I/F are created for each ALS device.
> >   /sys/class/als/alsX/illuminance:
> > 	the amount of light incident upon a specified surface area.
> >   /sys/class/als/alsX/mappings:
> > 	ambient light illuminance to display luminance mappings
> > 	that can be used by an OS to calibrate its ambient light policy
> > 	this is what I got on a test box:
> > 	cat /sys/class/als/als0/mappings
> > 	???Illuminance	Adjustment
> > 	          0	       70
> > 	         10	       73
> > 	         80	       85
> > 	        300	      100
> > 	       1000	      150
> 
> There's one value per file for sysfs... You should definitely have the
> header. 

No, no "header", just don't do this, it's not allowed. Again,
one-value-per-sysfs-file is the rule, please do not violate it.

> Plus I'd say Documentation/ file is needed.

It's required for any new sysfs file being added to the kernel.

thanks,

greg k-h

Re: [PATCH 0/3] Generic sysfs support for ACPI ALS and other ALS devices

[Valdis.Kletnieks]

[-- Attachment #1: Type: text/plain, Size: 769 bytes --]

On Tue, 04 Aug 2009 08:10:42 PDT, Greg KH said:
> On Tue, Aug 04, 2009 at 03:21:29PM +0200, Pavel Machek wrote:
> > On Mon 2009-08-03 17:10:57, Zhang Rui wrote:

> > > 	cat /sys/class/als/als0/mappings
> > > 	???Illuminance	Adjustment
> > > 	          0	       70
> > > 	         10	       73
> > > 	         80	       85
> > > 	        300	      100
> > > 	       1000	      150
> > 
> > There's one value per file for sysfs... You should definitely have the
> > header. 
> 
> No, no "header", just don't do this, it's not allowed. Again,
> one-value-per-sysfs-file is the rule, please do not violate it.

What's the intended sysfs solution here, then? Make 'mappings' a directory,
and populate it with files called 0, 10, 80, 300, 1000, each with one number
in them?

[-- Attachment #2: Type: application/pgp-signature, Size: 226 bytes --]

Re: [PATCH 0/3] Generic sysfs support for ACPI ALS and other ALS devices

[Greg KH]

On Tue, Aug 04, 2009 at 01:24:24PM -0400, Valdis.Kletnieks@vt.edu wrote:
> On Tue, 04 Aug 2009 08:10:42 PDT, Greg KH said:
> > On Tue, Aug 04, 2009 at 03:21:29PM +0200, Pavel Machek wrote:
> > > On Mon 2009-08-03 17:10:57, Zhang Rui wrote:
> 
> > > > 	cat /sys/class/als/als0/mappings
> > > > 	???Illuminance	Adjustment
> > > > 	          0	       70
> > > > 	         10	       73
> > > > 	         80	       85
> > > > 	        300	      100
> > > > 	       1000	      150
> > > 
> > > There's one value per file for sysfs... You should definitely have the
> > > header. 
> > 
> > No, no "header", just don't do this, it's not allowed. Again,
> > one-value-per-sysfs-file is the rule, please do not violate it.
> 
> What's the intended sysfs solution here, then? Make 'mappings' a directory,
> and populate it with files called 0, 10, 80, 300, 1000, each with one number
> in them?

That's one acceptable solution.  Or how about files in this directory
called "mapping_0", "mapping_10", and so on, containing the adjustment
value?

thanks,

greg k-h

Re: [PATCH 0/3] Generic sysfs support for ACPI ALS and other ALS devices

[Zhang Rui]

On Wed, 2009-08-05 at 01:36 +0800, Greg KH wrote:
> On Tue, Aug 04, 2009 at 01:24:24PM -0400, Valdis.Kletnieks@vt.edu wrote:
> > On Tue, 04 Aug 2009 08:10:42 PDT, Greg KH said:
> > > On Tue, Aug 04, 2009 at 03:21:29PM +0200, Pavel Machek wrote:
> > > > On Mon 2009-08-03 17:10:57, Zhang Rui wrote:
> > 
> > > > > 	cat /sys/class/als/als0/mappings
> > > > > 	???Illuminance	Adjustment
> > > > > 	          0	       70
> > > > > 	         10	       73
> > > > > 	         80	       85
> > > > > 	        300	      100
> > > > > 	       1000	      150
> > > > 
> > > > There's one value per file for sysfs... You should definitely have the
> > > > header. 
> > > 
> > > No, no "header", just don't do this, it's not allowed. Again,
> > > one-value-per-sysfs-file is the rule, please do not violate it.
> > 
> > What's the intended sysfs solution here, then? Make 'mappings' a directory,
> > and populate it with files called 0, 10, 80, 300, 1000, each with one number
> > in them?
> 
> That's one acceptable solution.  Or how about files in this directory
> called "mapping_0", "mapping_10", and so on, containing the adjustment
> value?
> 
great. that's what I'm going to do.
refreshed patch will be sent out later. :)

thanks,
rui

Re: [PATCH 0/3] Generic sysfs support for ACPI ALS and other ALS devices

[Valdis.Kletnieks]

[-- Attachment #1: Type: text/plain, Size: 1797 bytes --]

On Wed, 05 Aug 2009 09:04:52 +0800, Zhang Rui said:
> On Wed, 2009-08-05 at 01:36 +0800, Greg KH wrote:
> > On Tue, Aug 04, 2009 at 01:24:24PM -0400, Valdis.Kletnieks@vt.edu wrote:
> > > On Tue, 04 Aug 2009 08:10:42 PDT, Greg KH said:
> > > > On Tue, Aug 04, 2009 at 03:21:29PM +0200, Pavel Machek wrote:
> > > > > On Mon 2009-08-03 17:10:57, Zhang Rui wrote:
> > > 
> > > > > > 	cat /sys/class/als/als0/mappings
> > > > > > 	???Illuminance	Adjustment
> > > > > > 	          0	       70
> > > > > > 	         10	       73
> > > > > > 	         80	       85
> > > > > > 	        300	      100
> > > > > > 	       1000	      150
> > > > > 
> > > > > There's one value per file for sysfs... You should definitely have th
e
> > > > > header. 
> > > > 
> > > > No, no "header", just don't do this, it's not allowed. Again,
> > > > one-value-per-sysfs-file is the rule, please do not violate it.
> > > 
> > > What's the intended sysfs solution here, then? Make 'mappings' a directory,
> > > and populate it with files called 0, 10, 80, 300, 1000, each with one number
> > > in them?
> > 
> > That's one acceptable solution.  Or how about files in this directory
> > called "mapping_0", "mapping_10", and so on, containing the adjustment
> > value?
> > 
> great. that's what I'm going to do.
> refreshed patch will be sent out later. :)

Hmm. whether to call the files mapping_0, mapping_10 etc or mapping/0, mapping/10
etc probably depends on how nailed-down the 0,10,80,300,100 is - are those
numbers carved in stone, or are we better off defining the API as "directory
mapping, and files for whatever values the hardware happens to cough up?"

Not a biggie either way, it's going to be a readdir()/filter/open()/close()
loop either way, just different details for the readdir() and entry filtering...

[-- Attachment #2: Type: application/pgp-signature, Size: 226 bytes --]

Re: [PATCH 0/3] Generic sysfs support for ACPI ALS and other ALS devices

[Zhang Rui]

On Thu, 2009-08-06 at 00:10 +0800, Valdis.Kletnieks@vt.edu wrote:
> On Wed, 05 Aug 2009 09:04:52 +0800, Zhang Rui said:
> > On Wed, 2009-08-05 at 01:36 +0800, Greg KH wrote:
> > > On Tue, Aug 04, 2009 at 01:24:24PM -0400, Valdis.Kletnieks@vt.edu wrote:
> > > > On Tue, 04 Aug 2009 08:10:42 PDT, Greg KH said:
> > > > > On Tue, Aug 04, 2009 at 03:21:29PM +0200, Pavel Machek wrote:
> > > > > > On Mon 2009-08-03 17:10:57, Zhang Rui wrote:
> > > > 
> > > > > > > 	cat /sys/class/als/als0/mappings
> > > > > > > 	???Illuminance	Adjustment
> > > > > > > 	          0	       70
> > > > > > > 	         10	       73
> > > > > > > 	         80	       85
> > > > > > > 	        300	      100
> > > > > > > 	       1000	      150
> > > > > > 
> > > > > > There's one value per file for sysfs... You should definitely have th
> e
> > > > > > header. 
> > > > > 
> > > > > No, no "header", just don't do this, it's not allowed. Again,
> > > > > one-value-per-sysfs-file is the rule, please do not violate it.
> > > > 
> > > > What's the intended sysfs solution here, then? Make 'mappings' a directory,
> > > > and populate it with files called 0, 10, 80, 300, 1000, each with one number
> > > > in them?
> > > 
> > > That's one acceptable solution.  Or how about files in this directory
> > > called "mapping_0", "mapping_10", and so on, containing the adjustment
> > > value?
> > > 
> > great. that's what I'm going to do.
> > refreshed patch will be sent out later. :)
> 
> Hmm. whether to call the files mapping_0, mapping_10 etc or mapping/0, mapping/10
> etc probably depends on how nailed-down the 0,10,80,300,100 is - are those
> numbers carved in stone, 

no, these values can even change at run time.

> or are we better off defining the API as "directory
> mapping, and files for whatever values the hardware happens to cough up?"
> 
hmm, I'd prefer mappings0/{illuminance, adjustment},
mappings1/{illuminance, adjustment}...

in this case, all the illuminance/adjustment files can share the same
kobj_attribute.

thanks,
rui

Re: [PATCH 0/3] Generic sysfs support for ACPI ALS and other ALS devices

[Zhang Rui]

On Tue, 2009-08-04 at 23:10 +0800, Greg KH wrote:
> On Tue, Aug 04, 2009 at 03:21:29PM +0200, Pavel Machek wrote:
> > On Mon 2009-08-03 17:10:57, Zhang Rui wrote:
> > > Hi, all,
> > > 
> > > This is the patch set I made to introduce ACPI ALS device driver
> > > and a generic sysfs I/F for all the ALS devices, like ACPI ALS,
> > > platform ALS, etc.
> > > 
> > > Patch 01 introduces the ACPI ALS device driver.
> > > 
> > > Patch 02 introduces ALS sysfs class.
> > > 	Two sysfs I/F are created for each ALS device.
> > >   /sys/class/als/alsX/illuminance:
> > > 	the amount of light incident upon a specified surface area.
> > >   /sys/class/als/alsX/mappings:
> > > 	ambient light illuminance to display luminance mappings
> > > 	that can be used by an OS to calibrate its ambient light policy
> > > 	this is what I got on a test box:
> > > 	cat /sys/class/als/als0/mappings
> > > 	???Illuminance	Adjustment
> > > 	          0	       70
> > > 	         10	       73
> > > 	         80	       85
> > > 	        300	      100
> > > 	       1000	      150
> > 
> > There's one value per file for sysfs... You should definitely have the
> > header. 
> 
> No, no "header", just don't do this, it's not allowed. Again,
> one-value-per-sysfs-file is the rule, please do not violate it.
> 
Okay.
how about 
/sys/class/als/als0/mapping{0, 1, ...}/illuminance
and 
/sys/class/als/als0/mapping{0, 1, ...}/adjustment

> > Plus I'd say Documentation/ file is needed.
> 
> It's required for any new sysfs file being added to the kernel.
> 
Okay, I'll add the document file.

thanks,
rui

Re: [PATCH 0/3] Generic sysfs support for ACPI ALS and other ALS devices

[Zhang Rui]

On Tue, 2009-08-04 at 21:21 +0800, Pavel Machek wrote:
> On Mon 2009-08-03 17:10:57, Zhang Rui wrote:
> > Hi, all,
> > 
> > This is the patch set I made to introduce ACPI ALS device driver
> > and a generic sysfs I/F for all the ALS devices, like ACPI ALS,
> > platform ALS, etc.
> > 
> > Patch 01 introduces the ACPI ALS device driver.
> > 
> > Patch 02 introduces ALS sysfs class.
> > 	Two sysfs I/F are created for each ALS device.
> >   /sys/class/als/alsX/illuminance:
> > 	the amount of light incident upon a specified surface area.
> >   /sys/class/als/alsX/mappings:
> > 	ambient light illuminance to display luminance mappings
> > 	that can be used by an OS to calibrate its ambient light policy
> > 	this is what I got on a test box:
> > 	cat /sys/class/als/als0/mappings
> > 	???Illuminance	Adjustment
> > 	          0	       70
> > 	         10	       73
> > 	         80	       85
> > 	        300	      100
> > 	       1000	      150
> 
> There's one value per file for sysfs... You should definitely have the
> header. 
> 
> Is there chance to already return adjusted values, avoiding this
> uglyness?

No,
For a ALS policy, user space should set a brightness level as the base
point, i.e. the backlight when display adjustment is 100.
and then uses the adjustment values gotten from this mappings to
calculate the actual brightness level the display should be put in.

thanks,
rui

> 
> Plus I'd say Documentation/ file is needed.
> 
> 									Pavel

Re: [PATCH 0/3] Generic sysfs support for ACPI ALS and other ALS devices

[Pavel Machek]

On Wed 2009-08-05 09:02:50, Zhang Rui wrote:
> On Tue, 2009-08-04 at 21:21 +0800, Pavel Machek wrote:
> > On Mon 2009-08-03 17:10:57, Zhang Rui wrote:
> > > Hi, all,
> > > 
> > > This is the patch set I made to introduce ACPI ALS device driver
> > > and a generic sysfs I/F for all the ALS devices, like ACPI ALS,
> > > platform ALS, etc.
> > > 
> > > Patch 01 introduces the ACPI ALS device driver.
> > > 
> > > Patch 02 introduces ALS sysfs class.
> > > 	Two sysfs I/F are created for each ALS device.
> > >   /sys/class/als/alsX/illuminance:
> > > 	the amount of light incident upon a specified surface area.
> > >   /sys/class/als/alsX/mappings:
> > > 	ambient light illuminance to display luminance mappings
> > > 	that can be used by an OS to calibrate its ambient light policy
> > > 	this is what I got on a test box:
> > > 	cat /sys/class/als/als0/mappings
> > > 	???Illuminance	Adjustment
> > > 	          0	       70
> > > 	         10	       73
> > > 	         80	       85
> > > 	        300	      100
> > > 	       1000	      150
> > 
> > There's one value per file for sysfs... You should definitely have the
> > header. 
> > 
> > Is there chance to already return adjusted values, avoiding this
> > uglyness?
> 
> No,
> For a ALS policy, user space should set a brightness level as the base
> point, i.e. the backlight when display adjustment is 100.
> and then uses the adjustment values gotten from this mappings to
> calculate the actual brightness level the display should be put in.

Ok, so just make the code return only the "adjustement" -- userspace
does not really need to know "illuminance" and you can do the mapping
in the kernel AFAICT.
									Pavel

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [PATCH 0/3] Generic sysfs support for ACPI ALS and other ALS devices

[Zhang Rui]

On Thu, 2009-08-06 at 00:19 +0800, Pavel Machek wrote:
> On Wed 2009-08-05 09:02:50, Zhang Rui wrote:
> > On Tue, 2009-08-04 at 21:21 +0800, Pavel Machek wrote:
> > > On Mon 2009-08-03 17:10:57, Zhang Rui wrote:
> > > > Hi, all,
> > > > 
> > > > This is the patch set I made to introduce ACPI ALS device driver
> > > > and a generic sysfs I/F for all the ALS devices, like ACPI ALS,
> > > > platform ALS, etc.
> > > > 
> > > > Patch 01 introduces the ACPI ALS device driver.
> > > > 
> > > > Patch 02 introduces ALS sysfs class.
> > > > 	Two sysfs I/F are created for each ALS device.
> > > >   /sys/class/als/alsX/illuminance:
> > > > 	the amount of light incident upon a specified surface area.
> > > >   /sys/class/als/alsX/mappings:
> > > > 	ambient light illuminance to display luminance mappings
> > > > 	that can be used by an OS to calibrate its ambient light policy
> > > > 	this is what I got on a test box:
> > > > 	cat /sys/class/als/als0/mappings
> > > > 	???Illuminance	Adjustment
> > > > 	          0	       70
> > > > 	         10	       73
> > > > 	         80	       85
> > > > 	        300	      100
> > > > 	       1000	      150
> > > 
> > > There's one value per file for sysfs... You should definitely have the
> > > header. 
> > > 
> > > Is there chance to already return adjusted values, avoiding this
> > > uglyness?
> > 
> > No,
> > For a ALS policy, user space should set a brightness level as the base
> > point, i.e. the backlight when display adjustment is 100.
> > and then uses the adjustment values gotten from this mappings to
> > calculate the actual brightness level the display should be put in.
> 
> Ok, so just make the code return only the "adjustement" -- userspace
> does not really need to know "illuminance" and you can do the mapping
> in the kernel AFAICT.
> 			

ALS exports
1. the current ambient light illuminance
2. the mappings

Backlight device exports
1. the brightness levels.

User space needs to:
1. set a brightness level as the base point
2. read the current ambient light illuminance from ALS device
3. read the ambient light illuminance to display adjustment mappings
4. calculate a proper brightness level we should set.

For example, if we set brightness 6 as the default brightness, i.e. the
brightness level when ambient light illuminance is 300.
Now we walk outdoors, and the current illuminance is 1000,
then we should set the new_brightness_level to (6 * 150%)

thanks,
rui

Re: [PATCH 0/3] Generic sysfs support for ACPI ALS and other ALS devices

[Pavel Machek]

> > > No,
> > > For a ALS policy, user space should set a brightness level as the base
> > > point, i.e. the backlight when display adjustment is 100.
> > > and then uses the adjustment values gotten from this mappings to
> > > calculate the actual brightness level the display should be put in.
> > 
> > Ok, so just make the code return only the "adjustement" -- userspace
> > does not really need to know "illuminance" and you can do the mapping
> > in the kernel AFAICT.
> > 			
> 
> ALS exports
> 1. the current ambient light illuminance
> 2. the mappings
> 
> Backlight device exports
> 1. the brightness levels.
> 
> User space needs to:
> 1. set a brightness level as the base point
> 2. read the current ambient light illuminance from ALS device
> 3. read the ambient light illuminance to display adjustment mappings
> 4. calculate a proper brightness level we should set.
> 
> For example, if we set brightness 6 as the default brightness, i.e. the
> brightness level when ambient light illuminance is 300.
> Now we walk outdoors, and the current illuminance is 1000,
> then we should set the new_brightness_level to (6 * 150%)

Yes, so just export

ALS exports
1. the current ambient light illuminance
2. the current mapped ambient light illuminance

Backlight device exports
1. the brightness levels.
 
User space needs to:
1. set a brightness level as the base point
2. read the current mapped ambient light illuminance from ALS device
3. calculate a proper brightness level we should set.

You get

a) nicer interface
b) simpler userland
									Pavel

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [PATCH 0/3] Generic sysfs support for ACPI ALS and other ALS devices

[Zhang Rui]

On Thu, 2009-08-06 at 15:13 +0800, Pavel Machek wrote:
> > > > No,
> > > > For a ALS policy, user space should set a brightness level as the base
> > > > point, i.e. the backlight when display adjustment is 100.
> > > > and then uses the adjustment values gotten from this mappings to
> > > > calculate the actual brightness level the display should be put in.
> > > 
> > > Ok, so just make the code return only the "adjustement" -- userspace
> > > does not really need to know "illuminance" and you can do the mapping
> > > in the kernel AFAICT.
> > > 			
> > 
> > ALS exports
> > 1. the current ambient light illuminance
> > 2. the mappings
> > 
> > Backlight device exports
> > 1. the brightness levels.
> > 
> > User space needs to:
> > 1. set a brightness level as the base point
> > 2. read the current ambient light illuminance from ALS device
> > 3. read the ambient light illuminance to display adjustment mappings
> > 4. calculate a proper brightness level we should set.
> > 
> > For example, if we set brightness 6 as the default brightness, i.e. the
> > brightness level when ambient light illuminance is 300.
> > Now we walk outdoors, and the current illuminance is 1000,
> > then we should set the new_brightness_level to (6 * 150%)
> 
> Yes, so just export
> 
> ALS exports
> 1. the current ambient light illuminance
> 2. the current mapped ambient light illuminance
> 
In fact, only one attribute is enough.
The ALS driver can query the current illuminance, parse the mapping and
only export the display luminance adjustment values to user space.

> Backlight device exports
> 1. the brightness levels.
>  
> User space needs to:
> 1. set a brightness level as the base point
> 2. read the current mapped ambient light illuminance from ALS device
> 3. calculate a proper brightness level we should set.
> 
then user space just needs to
1. set a brightness level as the base point
2. get the display adjustment value and calculate a proper brightness
level at any time.

this is easier, but it makes me feel that this doesn't look like an
_ALS_ device any more.
We still should export some ALS properties to user space, shouldn't we?

Could you please look at the documentation about ALS sysfs class in the
patch I sent out just now, and comment on that one please? thanks!

thanks,
rui

Re: [PATCH 0/3] Generic sysfs support for ACPI ALS and other ALS devices

[Pavel Machek]

Hi!

> > Yes, so just export
> > 
> > ALS exports
> > 1. the current ambient light illuminance
> > 2. the current mapped ambient light illuminance
> > 
> In fact, only one attribute is enough.

Good. (I thought you would want to export the 1) too, mainly for debugging).

> The ALS driver can query the current illuminance, parse the mapping and
> only export the display luminance adjustment values to user space.

> then user space just needs to
> 1. set a brightness level as the base point
> 2. get the display adjustment value and calculate a proper brightness
> level at any time.
> 
> this is easier, but it makes me feel that this doesn't look like an
> _ALS_ device any more.
> We still should export some ALS properties to user space, shouldn't
> we?

Why? Just because ACPI specs is ugly does not mean we should make
Linux ugly, too. Please just use suggestion above.

> Could you please look at the documentation about ALS sysfs class in the
> patch I sent out just now, and comment on that one please? thanks!

I did. The interface is too ugly to live.
								Pavel

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [PATCH 0/3] Generic sysfs support for ACPI ALS and other ALS devices

[Zhang Rui]

On Thu, 2009-08-06 at 17:52 +0800, Pavel Machek wrote:
> Hi!
> 
> > > Yes, so just export
> > > 
> > > ALS exports
> > > 1. the current ambient light illuminance
> > > 2. the current mapped ambient light illuminance
> > > 
> > In fact, only one attribute is enough.
> 
> Good. (I thought you would want to export the 1) too, mainly for debugging).
> 
> > The ALS driver can query the current illuminance, parse the mapping and
> > only export the display luminance adjustment values to user space.
> 
> > then user space just needs to
> > 1. set a brightness level as the base point
> > 2. get the display adjustment value and calculate a proper brightness
> > level at any time.
> > 
> > this is easier, but it makes me feel that this doesn't look like an
> > _ALS_ device any more.
> > We still should export some ALS properties to user space, shouldn't
> > we?
> 
> Why? Just because ACPI specs is ugly does not mean we should make
> Linux ugly, too. Please just use suggestion above.
> 
> > Could you please look at the documentation about ALS sysfs class in the
> > patch I sent out just now, and comment on that one please? thanks!
> 
> I did. The interface is too ugly to live.

Hi, Pavel,

I tried to convert the ALS sysfs I/F to two attributes only, i.e.
illuminance and adjustment.
But I found several potential problems.
1. the illuminance to display adjustment mappings can not be convert to
a brightness level smoothly.
for example,
	illuminance	adjustment
1	600		70
2	900		100
3	1500		120
when the current illuminance is not one of the values listed in the
mappings, e.g. 750, the ALS driver don't have enough knowledge to get
the proper display adjustment, especially that a proper display
adjustment would be easy to select a proper brightness level.
We'd better leave this to user space, which is more flexible.
2. I don't know if there will be laptops exporting buggy mappings that
needs to be overridden some day, but the current sysfs I/F is easy for
expanding.
what do you think?

thanks,
rui



> 								Pavel
> 

Re: [PATCH 0/3] Generic sysfs support for ACPI ALS and other ALS devices

[Pavel Machek]

Hi!

> > > Could you please look at the documentation about ALS sysfs class in the
> > > patch I sent out just now, and comment on that one please? thanks!
> > 
> > I did. The interface is too ugly to live.
> 
> Hi, Pavel,
> 
> I tried to convert the ALS sysfs I/F to two attributes only, i.e.
> illuminance and adjustment.
> But I found several potential problems.
> 1. the illuminance to display adjustment mappings can not be convert to
> a brightness level smoothly.
> for example,
> 	illuminance	adjustment
> 1	600		70
> 2	900		100
> 3	1500		120
> when the current illuminance is not one of the values listed in the
> mappings, e.g. 750, the ALS driver don't have enough knowledge to get
> the proper display adjustment, especially that a proper display
> adjustment would be easy to select a proper brightness level.
> We'd better leave this to user space, which is more flexible.

Well, what interpolation does ACPI specs suggest to do? Maybe it is
easier to have linear interpolation in the kernel than to have ugly
20-file interface?

> 2. I don't know if there will be laptops exporting buggy mappings that
> needs to be overridden some day, but the current sysfs I/F is easy for
> expanding.
> what do you think?

If you have buggy laptop, fix the laptop. (You can still export the
raw values... and btw if you have buggy laptop and want to work around
it, maybe the workaround is easier/better done in kernel?)

								Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [PATCH 0/3] Generic sysfs support for ACPI ALS and other ALS devices

[Zhang Rui]

On Fri, 2009-08-21 at 19:51 +0800, Pavel Machek wrote:
> Hi!
> 
> > > > Could you please look at the documentation about ALS sysfs class in the
> > > > patch I sent out just now, and comment on that one please? thanks!
> > > 
> > > I did. The interface is too ugly to live.
> > 
> > Hi, Pavel,
> > 
> > I tried to convert the ALS sysfs I/F to two attributes only, i.e.
> > illuminance and adjustment.
> > But I found several potential problems.
> > 1. the illuminance to display adjustment mappings can not be convert to
> > a brightness level smoothly.
> > for example,
> > 	illuminance	adjustment
> > 1	600		70
> > 2	900		100
> > 3	1500		120
> > when the current illuminance is not one of the values listed in the
> > mappings, e.g. 750, the ALS driver don't have enough knowledge to get
> > the proper display adjustment, especially that a proper display
> > adjustment would be easy to select a proper brightness level.
> > We'd better leave this to user space, which is more flexible.
> 
> Well, what interpolation does ACPI specs suggest to do?

Well, the spec says that "Extrapolation of the values between
these points is OS-specific", and it uses a piecewise linear
approximation in an example.

>  Maybe it is
> easier to have linear interpolation in the kernel than to have ugly
> 20-file interface?

yes. we can do that.
refreshed patch will be sent out later.

thanks,
rui