[RFC, patch] Toshiba LCD support, now also sony, panasonic, asus and ibm

[Richard Hughes]

On Tue, 2005-08-30 at 10:02 -0400, David Zeuthen wrote:
> On Tue, 2005-08-30 at 01:42 +0100, Richard Hughes wrote:
> > On Mon, 2005-08-29 at 18:45 +0100, Richard Hughes wrote:
> > > Guys, Girls.
> > > 
> > > Here is a patch to add Toshiba LCD support into HAL.
> > > 
> > > Please review and comment.
> > 
> > With lots of help from the #ubuntu-devel guys, I've added sony,
> > panasonic, asus and ibm to the patch. And I've had lots of reports of
> > success, which  is always nice.
> > 
> > I've also added support for the method: SetLCDBrightness ()
> > 
> > To test do something like the following:
> > 
> > dbus-send --system --print-reply --dest=org.freedesktop.Hal \
> > /org/freedesktop/Hal/devices/acpi_toshiba \
> 
> Care to paste the output of lshal for this device object?

udi = '/org/freedesktop/Hal/devices/acpi_toshiba'
  info.udi = '/org/freedesktop/Hal/devices/acpi_toshiba'  (string)
  org.freedesktop.Hal.Device.SystemPowerManagement.method_execpaths =
{'hal-system-power-set-brightness'} (string list)
  org.freedesktop.Hal.Device.SystemPowerManagement.method_signatures =
{'i'} (string list)
  org.freedesktop.Hal.Device.SystemPowerManagement.method_names =
{'SetLCDBrightness'} (string list)
  info.interfaces = {'org.freedesktop.Hal.Device.SystemPowerManagement'}
(string list)
  linux.hotplug_type = 4  (0x4)  (int)
  info.capabilities = {'lcdpanel'} (string list)
  lcdpanel.brightness.percent = 62  (0x3e)  (int)
  lcdpanel.brightness.levels = 8  (0x8)  (int)
  lcdpanel.brightness.raw = 5  (0x5)  (int)
  lcdpanel.acpi_method = 'toshiba'  (string)
  info.product = 'Toshiba LCD Panel'  (string)
  info.category = 'lcdpanel'  (string)
  info.parent = '/org/freedesktop/Hal/devices/computer'  (string)
  linux.acpi_type = 4  (0x4)  (int)
  linux.acpi_path = '/proc/acpi/toshiba'  (string)

> > org.freedesktop.Hal.Device.SystemPowerManagement.SetLCDBrightness \
> > int32:2
> 
> Hmm, bad choice of interface name; this has nothing to do with
> powermanagement. Suggest

Tenuous, I could argue. :-)

>  org.freedesktop.Hal.Device.LCDPanel

What about something more generic?

org.freedesktop.Hal.Device.Display

> instead.
> 
> + <entry><literal>lcdpanel.brightness.raw</literal> (int)</entry>
> + <entry><literal>lcdpanel.brightness.percent</literal> (int)</entry>
> + <entry><literal>lcdpanel.brightness.levels</literal> (int)</entry>
> 
> Not ideal names IMO... Suggest
> 
>  .num_levels
>  .current_level   # can assume {0, 1, 2, ..., num-levels - 1}

Sure, will change. Should I keep the .brightness prefix?

> +		 * echo "level {0..15}" > /proc/acpi/ibm/brightness
> 
> This is not true on my system with the latest ibm-acpi code :-).
> 
>  [root at daxter ibm]# cat /proc/acpi/ibm/brightness
>  level:          7
>  commands:       up, down
>  commands:       level <level> (<level> is 0-7)

Ohh I see. I was looking at the initial patch to lkml. I'll change this
now.

> Suggest to double-check all the code for vendor modules... 

Sure, I've got lots of ubuntu volunteers waiting as I speak!

> You also want to patch hal.conf.in so only the user at the console can
> invoke these methods (and please test that this actually works using a
> dummy user).

Sure, will do.

> > Please have a look and tell me what you think.
> 
> Apart from that I think this looks good. Care to clean this up and send
> a new patch? 

No problem.

> Btw, there were some comments a few weeks back on this list... did you
> take these into account?

I think so, if anyone's got any comments then please speak up!

Thanks,

Richard.