Re: [linux-usb-devel] [PATCH] Driver for Apple Cinema Display

2006-05-27 Thread Michael Hanselmann 
Hello Andrew

On Fri, May 26, 2006 at 01:16:30PM -0700, Andrew Morton wrote:
  +   up(pdata-bd-sem);
  +   retval = appledisplay_bl_get_brightness(pdata-bd);
  +   if (retval = 0)
  +   pdata-bd-props-brightness = retval;
  +   down(pdata-bd-sem);

 Gee it's odd to go upping a semaphore on entry to a schedule_work()
 handler.  What's going on here?

As Benjamin wrote, I mixed up down() and up(). The patch below fixes
this. All other comments were due to that.

  +config USB_APPLEDISPLAY

 I have such a display - shall try to remember to test the driver, thanks.

Thanks!

Are complete patches okay for you or would you like to have diffes
against the earlier patch?

Greets,
Michael

---
diff -Nrup --exclude-from linux-exclude-from 
linux-2.6.17-rc5.orig/drivers/usb/Makefile linux-2.6.17-rc5/drivers/usb/Makefile
--- linux-2.6.17-rc5.orig/drivers/usb/Makefile  2006-05-25 21:14:36.0 
+0200
+++ linux-2.6.17-rc5/drivers/usb/Makefile   2006-05-25 23:34:25.0 
+0200
@@ -61,6 +61,7 @@ obj-$(CONFIG_USB_TEST)+= misc/
 obj-$(CONFIG_USB_USS720)   += misc/
 obj-$(CONFIG_USB_PHIDGETSERVO) += misc/
 obj-$(CONFIG_USB_SISUSBVGA)+= misc/
+obj-$(CONFIG_USB_APPLEDISPLAY) += misc/
 
 obj-$(CONFIG_USB_ATM)  += atm/
 obj-$(CONFIG_USB_SPEEDTOUCH)   += atm/
diff -Nrup --exclude-from linux-exclude-from 
linux-2.6.17-rc5.orig/drivers/usb/misc/appledisplay.c 
linux-2.6.17-rc5/drivers/usb/misc/appledisplay.c
--- linux-2.6.17-rc5.orig/drivers/usb/misc/appledisplay.c   1970-01-01 
01:00:00.0 +0100
+++ linux-2.6.17-rc5/drivers/usb/misc/appledisplay.c2006-05-27 
13:23:35.0 +0200
@@ -0,0 +1,380 @@
+/*
+ * Apple Cinema Display driver
+ *
+ * Copyright (C) 2006  Michael Hanselmann ([EMAIL PROTECTED])
+ *
+ * Thanks to Caskey L. Dickson for his work with acdctl.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, 
USA.
+ */
+
+#include linux/config.h
+#include linux/kernel.h
+#include linux/errno.h
+#include linux/init.h
+#include linux/module.h
+#include linux/usb.h
+#include linux/backlight.h
+#include linux/timer.h
+#include linux/workqueue.h
+#include asm/atomic.h
+#include asm/semaphore.h
+
+#define APPLE_VENDOR_ID0x05AC
+
+#define USB_REQ_GET_REPORT 0x01
+#define USB_REQ_SET_REPORT 0x09
+
+#define ACD_USB_TIMEOUT250
+
+#define ACD_USB_EDID   0x0302
+#define ACD_USB_BRIGHTNESS 0x0310
+
+#define ACD_BTN_NONE   0
+#define ACD_BTN_BRIGHT_UP  3
+#define ACD_BTN_BRIGHT_DOWN4
+
+#define ACD_URB_BUFFER_LEN 2
+#define ACD_MSG_BUFFER_LEN 2
+
+#define APPLEDISPLAY_DEVICE(prod)  \
+   .match_flags = USB_DEVICE_ID_MATCH_DEVICE | \
+  USB_DEVICE_ID_MATCH_INT_CLASS |  \
+  USB_DEVICE_ID_MATCH_INT_PROTOCOL,\
+   .idVendor = APPLE_VENDOR_ID,\
+   .idProduct = (prod),\
+   .bInterfaceClass = USB_CLASS_HID,   \
+   .bInterfaceProtocol = 0x00
+
+/* table of devices that work with this driver */
+static struct usb_device_id appledisplay_table [] = {
+   { APPLEDISPLAY_DEVICE(0x9218) },
+   { APPLEDISPLAY_DEVICE(0x9219) },
+   { APPLEDISPLAY_DEVICE(0x921d) },
+
+   /* Terminating entry */
+   { }
+};
+MODULE_DEVICE_TABLE(usb, appledisplay_table);
+
+/* Structure to hold all of our device specific stuff */
+struct appledisplay {
+   struct usb_device* udev;/* usb device */
+   struct urb* urb;/* usb request block */
+   struct backlight_device* bd;/* backlight device */
+   char *urbdata;  /* interrupt URB data buffer */
+   char *msgdata;  /* control message data buffer */
+
+   struct work_struct work;
+   int button_pressed;
+   spinlock_t lock;
+};
+
+static atomic_t count_displays = ATOMIC_INIT(0);
+static struct workqueue_struct *wq;
+
+static void appledisplay_complete(struct urb* urb, struct pt_regs* regs)
+{
+   struct appledisplay *pdata = urb-context;
+   unsigned long flags;
+   int retval;
+
+   switch (urb-status) {
+   case 0: 
+   /* success */
+   break;
+   case

Re: [linux-usb-devel] [PATCH] Driver for Apple Cinema Display

2006-05-27 Thread Christian Iversen 
On Saturday 27 May 2006 13:30, Michael Hanselmann wrote:
 Hello Andrew

 On Fri, May 26, 2006 at 01:16:30PM -0700, Andrew Morton wrote:
   + up(pdata-bd-sem);
   + retval = appledisplay_bl_get_brightness(pdata-bd);
   + if (retval = 0)
   + pdata-bd-props-brightness = retval;
   + down(pdata-bd-sem);
 
  Gee it's odd to go upping a semaphore on entry to a schedule_work()
  handler.  What's going on here?

 As Benjamin wrote, I mixed up down() and up(). The patch below fixes
 this. All other comments were due to that.

Also, HZ / 8 is not the right way to specify 125ms. It just happens to be 
right in the default desktop config where HZ == 1000.

-- 
Regards,
Christian Iversen


---
All the advantages of Linux Managed Hosting--Without the Cost and Risk!
Fully trained technicians. The highest number of Red Hat certifications in
the hosting industry. Fanatical Support. Click to learn more
http://sel.as-us.falkag.net/sel?cmd=lnkkid=107521bid=248729dat=121642
___
linux-usb-devel@lists.sourceforge.net
To unsubscribe, use the last form field at:
https://lists.sourceforge.net/lists/listinfo/linux-usb-devel

Re: [linux-usb-devel] [PATCH] Driver for Apple Cinema Display

2006-05-27 Thread Christian Iversen 
On Saturday 27 May 2006 13:36, Christian Iversen wrote:
 On Saturday 27 May 2006 13:30, Michael Hanselmann wrote:
  Hello Andrew
 
  On Fri, May 26, 2006 at 01:16:30PM -0700, Andrew Morton wrote:
+   up(pdata-bd-sem);
+   retval = appledisplay_bl_get_brightness(pdata-bd);
+   if (retval = 0)
+   pdata-bd-props-brightness = retval;
+   down(pdata-bd-sem);
  
   Gee it's odd to go upping a semaphore on entry to a schedule_work()
   handler.  What's going on here?
 
  As Benjamin wrote, I mixed up down() and up(). The patch below fixes
  this. All other comments were due to that.

 Also, HZ / 8 is not the right way to specify 125ms. It just happens to be
 right in the default desktop config where HZ == 1000.

On second thought, maybe I got that wrong. I don't know the 
schedule_delayed_work() function. Can anyone sort this out?

-- 
Regards,
Christian Iversen


---
All the advantages of Linux Managed Hosting--Without the Cost and Risk!
Fully trained technicians. The highest number of Red Hat certifications in
the hosting industry. Fanatical Support. Click to learn more
http://sel.as-us.falkag.net/sel?cmd=lnkkid=107521bid=248729dat=121642
___
linux-usb-devel@lists.sourceforge.net
To unsubscribe, use the last form field at:
https://lists.sourceforge.net/lists/listinfo/linux-usb-devel

[linux-usb-devel] [PATCH] Driver for Apple Cinema Display

2006-05-26 Thread Michael Hanselmann 
This is a driver to control the brightness of an Apple Cinema Display
over USB. It updates the local brightness value if the user presses a
button on the display.

Please apply to -mm for testing if nothing major is broken.

Signed-off-by: Michael Hanselmann [EMAIL PROTECTED]

---
diff -Nrup --exclude-from linux-exclude-from 
linux-2.6.17-rc5.orig/drivers/usb/Makefile linux-2.6.17-rc5/drivers/usb/Makefile
--- linux-2.6.17-rc5.orig/drivers/usb/Makefile  2006-05-25 21:14:36.0 
+0200
+++ linux-2.6.17-rc5/drivers/usb/Makefile   2006-05-25 23:34:25.0 
+0200
@@ -61,6 +61,7 @@ obj-$(CONFIG_USB_TEST)+= misc/
 obj-$(CONFIG_USB_USS720)   += misc/
 obj-$(CONFIG_USB_PHIDGETSERVO) += misc/
 obj-$(CONFIG_USB_SISUSBVGA)+= misc/
+obj-$(CONFIG_USB_APPLEDISPLAY) += misc/
 
 obj-$(CONFIG_USB_ATM)  += atm/
 obj-$(CONFIG_USB_SPEEDTOUCH)   += atm/
diff -Nrup --exclude-from linux-exclude-from 
linux-2.6.17-rc5.orig/drivers/usb/misc/appledisplay.c 
linux-2.6.17-rc5/drivers/usb/misc/appledisplay.c
--- linux-2.6.17-rc5.orig/drivers/usb/misc/appledisplay.c   1970-01-01 
01:00:00.0 +0100
+++ linux-2.6.17-rc5/drivers/usb/misc/appledisplay.c2006-05-26 
10:51:20.0 +0200
@@ -0,0 +1,368 @@
+/*
+ * Apple Cinema Display driver
+ *
+ * Copyright (C) 2006  Michael Hanselmann ([EMAIL PROTECTED])
+ *
+ * Thanks to Caskey L. Dickson for his work with acdctl.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, 
USA.
+ */
+
+#include linux/config.h
+#include linux/kernel.h
+#include linux/errno.h
+#include linux/init.h
+#include linux/module.h
+#include linux/usb.h
+#include linux/backlight.h
+#include linux/timer.h
+#include linux/workqueue.h
+#include asm/atomic.h
+#include asm/semaphore.h
+
+#define APPLE_VENDOR_ID0x05AC
+
+#define USB_REQ_GET_REPORT 0x01
+#define USB_REQ_SET_REPORT 0x09
+
+#define ACD_USB_TIMEOUT250
+
+#define ACD_USB_EDID   0x0302
+#define ACD_USB_BRIGHTNESS 0x0310
+
+#define ACD_BTN_NONE   0
+#define ACD_BTN_BRIGHT_UP  3
+#define ACD_BTN_BRIGHT_DOWN4
+
+#define APPLEDISPLAY_DEVICE(prod)  \
+   .match_flags = USB_DEVICE_ID_MATCH_DEVICE | \
+  USB_DEVICE_ID_MATCH_INT_CLASS |  \
+  USB_DEVICE_ID_MATCH_INT_PROTOCOL,\
+   .idVendor = APPLE_VENDOR_ID,\
+   .idProduct = (prod),\
+   .bInterfaceClass = USB_CLASS_HID,   \
+   .bInterfaceProtocol = 0x00
+
+/* table of devices that work with this driver */
+static struct usb_device_id appledisplay_table [] = {
+   { APPLEDISPLAY_DEVICE(0x9218) },
+   { APPLEDISPLAY_DEVICE(0x9219) },
+   { APPLEDISPLAY_DEVICE(0x921d) },
+
+   /* Terminating entry */
+   { }
+};
+MODULE_DEVICE_TABLE(usb, appledisplay_table);
+
+/* Structure to hold all of our device specific stuff */
+struct appledisplay {
+   struct usb_device* udev;/* usb device */
+   struct urb* urb;/* usb request block */
+   struct backlight_device* bd;/* backlight device */
+   char *data;
+   int datalen;
+   struct work_struct work;
+   int button_pressed;
+   spinlock_t lock;
+};
+
+static atomic_t count_displays = ATOMIC_INIT(0);
+static struct workqueue_struct *wq;
+
+static void appledisplay_complete(struct urb* urb, struct pt_regs* regs)
+{
+   struct appledisplay *pdata = urb-context;
+   unsigned long flags;
+   int retval;
+
+   switch (urb-status) {
+   case 0: 
+   /* success */
+   break;
+   case -EOVERFLOW:
+   printk(appletouch: OVERFLOW with data 
+   length %d, actual length is %d\n,
+   pdata-datalen, pdata-urb-actual_length);
+   case -ECONNRESET:
+   case -ENOENT:
+   case -ESHUTDOWN:
+   /* This urb is terminated, clean up */
+   dbg(%s - urb shutting down with status: %d,
+   __FUNCTION__, urb-status);
+   return;
+   default:
+   dbg(%s - nonzero urb status received: %d,
+   __FUNCTION__, urb-status);
+   goto

Re: [linux-usb-devel] [PATCH] Driver for Apple Cinema Display

2006-05-26 Thread Oliver Neukum 
Am Freitag, 26. Mai 2006 11:15 schrieb Michael Hanselmann:
 +static int appledisplay_bl_update_status(struct backlight_device *bd)
 +{
 +   struct appledisplay *pdata = class_get_devdata(bd-class_dev);
 +   char buffer[2];
 +   int retval;
 +
 +   buffer[0] = 0x10;
 +   buffer[1] = bd-props-brightness;
 +
 +   retval = usb_control_msg(
 +   pdata-udev,
 +   usb_sndctrlpipe(pdata-udev, 0),
 +   USB_REQ_SET_REPORT,
 +   USB_DIR_OUT | USB_TYPE_CLASS | USB_RECIP_INTERFACE,
 +   ACD_USB_BRIGHTNESS,
 +   0,
 +   buffer,
 +   sizeof(buffer),
 +   250);
 +
 +   return retval;
 +}

DMA on the stack. You must not do this. Allocate the buffer with kmalloc.

Regards
Oliver
Ym…杽©íj¬¡òâžìLjv yÑè²Ø§h­†‹­¶‚¢ËZÔb²An–\­­¨§yÛ^râr§±8^†(!zËgºfÞ®‡ÑyÑڵǫ¶'âq«b¢{žØ^†‹-ŠxÛ¬¶¼…jv­‰Æ¥Jêi¢»B–'$¶‰^j¹æ¢·¡¶ÚþÇ¥jˬ}©dj
  Þ·û•É

Re: [linux-usb-devel] [PATCH] Driver for Apple Cinema Display

2006-05-26 Thread Oliver Neukum 
Am Freitag, 26. Mai 2006 11:15 schrieb Michael Hanselmann:
 +   if (usb_submit_urb(pdata-urb, GFP_ATOMIC)) {
 +   retval = -EIO;

GFP_KERNEL will do.

Regards
Oliver


---
All the advantages of Linux Managed Hosting--Without the Cost and Risk!
Fully trained technicians. The highest number of Red Hat certifications in
the hosting industry. Fanatical Support. Click to learn more
http://sel.as-us.falkag.net/sel?cmd=lnkkid7521bid$8729dat1642
___
linux-usb-devel@lists.sourceforge.net
To unsubscribe, use the last form field at:
https://lists.sourceforge.net/lists/listinfo/linux-usb-devel

Re: [linux-usb-devel] [PATCH] Driver for Apple Cinema Display

2006-05-26 Thread Oliver Neukum 
Am Freitag, 26. Mai 2006 11:15 schrieb Michael Hanselmann:
 +static void appledisplay_disconnect(struct usb_interface *iface)
 +{
 +   struct appledisplay *pdata = usb_get_intfdata(iface);
 +
 +   if (pdata) {
 +   cancel_delayed_work(pdata-work);
 +   usb_kill_urb(pdata-urb);
 +   backlight_device_unregister(pdata-bd);

This is a race condition. As the URB's completion handler can queue new
work, you must kill the URB before you cancel work.

Regards
Oliver


---
All the advantages of Linux Managed Hosting--Without the Cost and Risk!
Fully trained technicians. The highest number of Red Hat certifications in
the hosting industry. Fanatical Support. Click to learn more
http://sel.as-us.falkag.net/sel?cmd=lnkkid7521bid$8729dat1642
___
linux-usb-devel@lists.sourceforge.net
To unsubscribe, use the last form field at:
https://lists.sourceforge.net/lists/listinfo/linux-usb-devel

Re: [linux-usb-devel] [PATCH] Driver for Apple Cinema Display

2006-05-26 Thread Oliver Neukum 
Am Freitag, 26. Mai 2006 11:15 schrieb Michael Hanselmann:
 +static int __init appledisplay_init(void)
 +{
 +   wq = create_singlethread_workqueue(appledisplay);
 +   BUG_ON(!wq);

That's not a BUG.

Regards
Oliver


---
All the advantages of Linux Managed Hosting--Without the Cost and Risk!
Fully trained technicians. The highest number of Red Hat certifications in
the hosting industry. Fanatical Support. Click to learn more
http://sel.as-us.falkag.net/sel?cmd=lnkkid7521bid$8729dat1642
___
linux-usb-devel@lists.sourceforge.net
To unsubscribe, use the last form field at:
https://lists.sourceforge.net/lists/listinfo/linux-usb-devel

Re: [linux-usb-devel] [PATCH] Driver for Apple Cinema Display

2006-05-26 Thread Michael Hanselmann 
Hello Oliver

On Fri, May 26, 2006 at 11:29:14AM +0200, Oliver Neukum wrote:
  +   char buffer[2];

 DMA on the stack. You must not do this. Allocate the buffer with kmalloc.

This and the other errors are fixed in the patch below.

Thanks,
Michael

---
diff -Nrup --exclude-from linux-exclude-from 
linux-2.6.17-rc5.orig/drivers/usb/Makefile linux-2.6.17-rc5/drivers/usb/Makefile
--- linux-2.6.17-rc5.orig/drivers/usb/Makefile  2006-05-25 21:14:36.0 
+0200
+++ linux-2.6.17-rc5/drivers/usb/Makefile   2006-05-25 23:34:25.0 
+0200
@@ -61,6 +61,7 @@ obj-$(CONFIG_USB_TEST)+= misc/
 obj-$(CONFIG_USB_USS720)   += misc/
 obj-$(CONFIG_USB_PHIDGETSERVO) += misc/
 obj-$(CONFIG_USB_SISUSBVGA)+= misc/
+obj-$(CONFIG_USB_APPLEDISPLAY) += misc/
 
 obj-$(CONFIG_USB_ATM)  += atm/
 obj-$(CONFIG_USB_SPEEDTOUCH)   += atm/
diff -Nrup --exclude-from linux-exclude-from 
linux-2.6.17-rc5.orig/drivers/usb/misc/appledisplay.c 
linux-2.6.17-rc5/drivers/usb/misc/appledisplay.c
--- linux-2.6.17-rc5.orig/drivers/usb/misc/appledisplay.c   1970-01-01 
01:00:00.0 +0100
+++ linux-2.6.17-rc5/drivers/usb/misc/appledisplay.c2006-05-26 
13:33:42.0 +0200
@@ -0,0 +1,380 @@
+/*
+ * Apple Cinema Display driver
+ *
+ * Copyright (C) 2006  Michael Hanselmann ([EMAIL PROTECTED])
+ *
+ * Thanks to Caskey L. Dickson for his work with acdctl.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, 
USA.
+ */
+
+#include linux/config.h
+#include linux/kernel.h
+#include linux/errno.h
+#include linux/init.h
+#include linux/module.h
+#include linux/usb.h
+#include linux/backlight.h
+#include linux/timer.h
+#include linux/workqueue.h
+#include asm/atomic.h
+#include asm/semaphore.h
+
+#define APPLE_VENDOR_ID0x05AC
+
+#define USB_REQ_GET_REPORT 0x01
+#define USB_REQ_SET_REPORT 0x09
+
+#define ACD_USB_TIMEOUT250
+
+#define ACD_USB_EDID   0x0302
+#define ACD_USB_BRIGHTNESS 0x0310
+
+#define ACD_BTN_NONE   0
+#define ACD_BTN_BRIGHT_UP  3
+#define ACD_BTN_BRIGHT_DOWN4
+
+#define ACD_URB_BUFFER_LEN 2
+#define ACD_MSG_BUFFER_LEN 2
+
+#define APPLEDISPLAY_DEVICE(prod)  \
+   .match_flags = USB_DEVICE_ID_MATCH_DEVICE | \
+  USB_DEVICE_ID_MATCH_INT_CLASS |  \
+  USB_DEVICE_ID_MATCH_INT_PROTOCOL,\
+   .idVendor = APPLE_VENDOR_ID,\
+   .idProduct = (prod),\
+   .bInterfaceClass = USB_CLASS_HID,   \
+   .bInterfaceProtocol = 0x00
+
+/* table of devices that work with this driver */
+static struct usb_device_id appledisplay_table [] = {
+   { APPLEDISPLAY_DEVICE(0x9218) },
+   { APPLEDISPLAY_DEVICE(0x9219) },
+   { APPLEDISPLAY_DEVICE(0x921d) },
+
+   /* Terminating entry */
+   { }
+};
+MODULE_DEVICE_TABLE(usb, appledisplay_table);
+
+/* Structure to hold all of our device specific stuff */
+struct appledisplay {
+   struct usb_device* udev;/* usb device */
+   struct urb* urb;/* usb request block */
+   struct backlight_device* bd;/* backlight device */
+   char *urbdata;  /* interrupt URB data buffer */
+   char *msgdata;  /* control message data buffer */
+
+   struct work_struct work;
+   int button_pressed;
+   spinlock_t lock;
+};
+
+static atomic_t count_displays = ATOMIC_INIT(0);
+static struct workqueue_struct *wq;
+
+static void appledisplay_complete(struct urb* urb, struct pt_regs* regs)
+{
+   struct appledisplay *pdata = urb-context;
+   unsigned long flags;
+   int retval;
+
+   switch (urb-status) {
+   case 0: 
+   /* success */
+   break;
+   case -EOVERFLOW:
+   printk(appletouch: OVERFLOW with data 
+   length %d, actual length is %d\n,
+   ACD_URB_BUFFER_LEN, pdata-urb-actual_length);
+   case -ECONNRESET:
+   case -ENOENT:
+   case -ESHUTDOWN:
+   /* This urb is terminated, clean up */
+   dbg(%s - urb shutting down with status: %d,
+   __FUNCTION__, urb-status);
+

Re: [linux-usb-devel] [PATCH] Driver for Apple Cinema Display

2006-05-26 Thread Andrew Morton 
Michael Hanselmann [EMAIL PROTECTED] wrote:

 +#include asm/semaphore.h

backlight_device would appear to need mutex conversion.

Then again, maybe we take backlight_device.sem recursively - I do recall
some pretty wild happenings in there.

 +static void appledisplay_work(void *private)
 +{
 + struct appledisplay *pdata = private;
 + int retval;
 +
 + up(pdata-bd-sem);
 + retval = appledisplay_bl_get_brightness(pdata-bd);
 + if (retval = 0)
 + pdata-bd-props-brightness = retval;
 + down(pdata-bd-sem);
 +
 + /* Poll again in about 125ms if there's still a button pressed */
 + if (pdata-button_pressed)
 + schedule_delayed_work(pdata-work, HZ / 8);
 +}

Gee it's odd to go upping a semaphore on entry to a schedule_work()
handler.  What's going on here?


 +static int appledisplay_probe(struct usb_interface *iface,
 + const struct usb_device_id *id)
 +{

 ..

 + /* Try to get brightness */
 + up(pdata-bd-sem);
 + brightness = appledisplay_bl_get_brightness(pdata-bd);
 + down(pdata-bd-sem);

This looks fishy too.  appledisplay_bl_get_brightness() doesn't appear to
take that semaphore, so why drop the lock?


 + if (brightness  0) {
 + retval = brightness;
 + err(appledisplay: Error while getting initial brightness: %d, 
 retval);
 + goto error;
 + }
 +
 + /* Set brightness in backlight device */
 + up(pdata-bd-sem);
 + pdata-bd-props-brightness = brightness;
 + down(pdata-bd-sem);

Ditto.  If it's attempting to synchronise against appledisplay_work() in
some way then I'd have thought it was racy..

 +config USB_APPLEDISPLAY

I have such a display - shall try to remember to test the driver, thanks.


---
All the advantages of Linux Managed Hosting--Without the Cost and Risk!
Fully trained technicians. The highest number of Red Hat certifications in
the hosting industry. Fanatical Support. Click to learn more
http://sel.as-us.falkag.net/sel?cmd=lnkkid=107521bid=248729dat=121642
___
linux-usb-devel@lists.sourceforge.net
To unsubscribe, use the last form field at:
https://lists.sourceforge.net/lists/listinfo/linux-usb-devel

Re: [linux-usb-devel] [PATCH] Driver for Apple Cinema Display

2006-05-26 Thread Benjamin Herrenschmidt 

  +   /* Set brightness in backlight device */
  +   up(pdata-bd-sem);
  +   pdata-bd-props-brightness = brightness;
  +   down(pdata-bd-sem);
 
 Ditto.  If it's attempting to synchronise against appledisplay_work() in
 some way then I'd have thought it was racy..

Heh, smells to me like Michael mixed them up ;) 

Ben.




---
All the advantages of Linux Managed Hosting--Without the Cost and Risk!
Fully trained technicians. The highest number of Red Hat certifications in
the hosting industry. Fanatical Support. Click to learn more
http://sel.as-us.falkag.net/sel?cmd=lnkkid=107521bid=248729dat=121642
___
linux-usb-devel@lists.sourceforge.net
To unsubscribe, use the last form field at:
https://lists.sourceforge.net/lists/listinfo/linux-usb-devel