[PATCH 1/2] Add USB device ID for Apple Cinema Display 23in 2007

[PATCH 1/2] Add USB device ID for Apple Cinema Display 23in 2007

[pancho horrillo]

Hi!

$ lsusb -v
Bus 001 Device 008: ID 05ac:921c Apple, Inc. 
Device Descriptor:
  bLength                18
  bDescriptorType         1
  bcdUSB               1.10
  bDeviceClass            0 (Defined at Interface level)
  bDeviceSubClass         0 
  bDeviceProtocol         0 
  bMaxPacketSize0         8
  idVendor           0x05ac Apple, Inc.
  idProduct          0x921c 
  bcdDevice            1.15
  iManufacturer           1 
  iProduct                2 
  iSerial                 0 
  bNumConfigurations      1
  Configuration Descriptor:
    bLength                 9
    bDescriptorType         2
    wTotalLength           34
    bNumInterfaces          1
    bConfigurationValue     1
    iConfiguration          0 
    bmAttributes         0xe0
      Self Powered
      Remote Wakeup
    MaxPower                2mA
    Interface Descriptor:
      bLength                 9
      bDescriptorType         4
      bInterfaceNumber        0
      bAlternateSetting       0
      bNumEndpoints           1
      bInterfaceClass         3 Human Interface Device
      bInterfaceSubClass      0 No Subclass
      bInterfaceProtocol      0 None
      iInterface              0 
        HID Device Descriptor:
          bLength                 9
          bDescriptorType        33
          bcdHID               1.11
          bCountryCode            0 Not supported
          bNumDescriptors         1
          bDescriptorType        34 Report
          wDescriptorLength      92
         Report Descriptors: 
           ** UNAVAILABLE **
      Endpoint Descriptor:
        bLength                 7
        bDescriptorType         5
        bEndpointAddress     0x81  EP 1 IN
        bmAttributes            3
          Transfer Type            Interrupt
          Synch Type               None
          Usage Type               Data
        wMaxPacketSize     0x0008  1x 8 bytes
        bInterval              16



(Against linux-2.6.31.6)

Signed-off-by: pancho horrillo <pancho@pancho.name>

diff -purN a/drivers/usb/misc/appledisplay.c b/drivers/usb/misc/appledisplay.c
--- a/drivers/usb/misc/appledisplay.c	2009-11-10 01:32:31.000000000 +0100
+++ b/drivers/usb/misc/appledisplay.c	2009-12-02 20:01:25.000000000 +0100
@@ -60,6 +60,7 @@
 static struct usb_device_id appledisplay_table [] = {
 	{ APPLEDISPLAY_DEVICE(0x9218) },
 	{ APPLEDISPLAY_DEVICE(0x9219) },
+	{ APPLEDISPLAY_DEVICE(0x921c) },
 	{ APPLEDISPLAY_DEVICE(0x921d) },
 
 	/* Terminating entry */

-- 
pancho horrillo

To be conscious that
you are ignorant is a great step
to knowledge.

		Benjamin Disraeli

Re: [PATCH 2/2] Fix a bug on appledisplay.c regarding signedness

[pancho horrillo]

Hi!

bug description:  If the brightness level on the apple display is 128 or
higher, the driver will complain on startup:

"Error while getting initial brightness: -128"

Actual                  Driver
Brightness level        Reported value
0                       0
...                     ...
126                     126
127                     127
128                     -128 (aha! :-)
129                     -127
...                     ...
255                     -1

Also, the reported brightness via /sys interface will present the same
inconsistency.

I realized that this was simply because the monitor transmits its data
as an unsigned char (uint8_t), and in the code it is treated as a signed
char.

I consulted Caskey L. Dickson's acdctl sources to ascertain this.  He
used uint8_t there.   I also checked the usb_* functions definitions to be
sure that I was not second-guessing them.  Yep, they accept (void *) in
the relevant parameters, so it's up to us to decide the type of the
payload.

I've tested it and works like a charm now.

I have emailed the author of the driver, Michael Hanselmann, and he said:
On Wed, Dec 02, 2009 at 05:14:26PM +0000, Michael Hanselmann wrote:
[...]
> If I remember correctly, Linux on PowerPC uses unsigned chars by
> default. I never tested the driver on a non-PowerPC platform and hence
> may never have seen this.


Please, review and push the patch, if you deem it adequate.

Thanks a bunch!

Signed-off-by: pancho horrillo <pancho@pancho.name>

diff -purN a/drivers/usb/misc/appledisplay.c b/drivers/usb/misc/appledisplay.c
--- a/drivers/usb/misc/appledisplay.c	2009-11-10 01:32:31.000000000 +0100
+++ b/drivers/usb/misc/appledisplay.c	2009-12-02 20:05:00.000000000 +0100
@@ -72,8 +72,8 @@ 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 */
+	uint8_t *urbdata;		/* interrupt URB data buffer */
+	uint8_t *msgdata;		/* control message data buffer */
 
 	struct delayed_work work;
 	int button_pressed;

-- 
pancho horrillo

To be conscious that
you are ignorant is a great step
to knowledge.

		Benjamin Disraeli

Re: [PATCH 2/2] Fix a bug on appledisplay.c regarding signedness

[Sergei Shtylyov]

Hello.

pancho horrillo wrote:

> Please, review and push the patch, if you deem it adequate.

> Thanks a bunch!

> Signed-off-by: pancho horrillo <pancho@pancho.name>

> diff -purN a/drivers/usb/misc/appledisplay.c b/drivers/usb/misc/appledisplay.c
> --- a/drivers/usb/misc/appledisplay.c	2009-11-10 01:32:31.000000000 +0100
> +++ b/drivers/usb/misc/appledisplay.c	2009-12-02 20:05:00.000000000 +0100
> @@ -72,8 +72,8 @@ 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 */
> +	uint8_t *urbdata;		/* interrupt URB data buffer */
> +	uint8_t *msgdata;		/* control message data buffer */

    That 'uint8_t' is for userspace, use 'u8' instead.

WBR, Sergei

Re: [PATCH 2/2] Fix a bug on appledisplay.c regarding signedness

[pancho horrillo]

On Wed, Dec 02, 2009 at 10:38:27PM +0300, Sergei Shtylyov wrote:
> Hello.
> 
> pancho horrillo wrote:
> 
> >Please, review and push the patch, if you deem it adequate.
> 
> >Thanks a bunch!
> 
>    That 'uint8_t' is for userspace, use 'u8' instead.
> 
> WBR, Sergei

Thanks for the coaching, Sergei!

BTW, there are about 4863 of uint8_t in the kernel sources :-)

Cheers,

pancho.

Signed-off-by: pancho horrillo <pancho@pancho.name>

diff -purN a/drivers/usb/misc/appledisplay.c b/drivers/usb/misc/appledisplay.c
--- a/drivers/usb/misc/appledisplay.c	2009-11-10 01:32:31.000000000 +0100
+++ b/drivers/usb/misc/appledisplay.c	2009-12-02 20:05:00.000000000 +0100
@@ -72,8 +72,8 @@ 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 */
+	u8 *urbdata;			/* interrupt URB data buffer */
+	u8 *msgdata;			/* control message data buffer */
 
 	struct delayed_work work;
 	int button_pressed;
-- 
pancho horrillo

To be conscious that
you are ignorant is a great step
to knowledge.

		Benjamin Disraeli

Re: [PATCH 2/2] Fix a bug on appledisplay.c regarding signedness

[Greg KH]

On Wed, Dec 02, 2009 at 09:44:21PM +0100, pancho horrillo wrote:
> On Wed, Dec 02, 2009 at 10:38:27PM +0300, Sergei Shtylyov wrote:
> > Hello.
> > 
> > pancho horrillo wrote:
> > 
> > >Please, review and push the patch, if you deem it adequate.
> > 
> > >Thanks a bunch!
> > 
> >    That 'uint8_t' is for userspace, use 'u8' instead.
> > 
> > WBR, Sergei
> 
> Thanks for the coaching, Sergei!
> 
> BTW, there are about 4863 of uint8_t in the kernel sources :-)

And please don't try to add any new ones :)

thanks,

greg k-h

Re: [PATCH 2/2] Fix a bug on appledisplay.c regarding signedness

[Greg KH]

On Wed, Dec 02, 2009 at 09:44:21PM +0100, pancho horrillo wrote:
> On Wed, Dec 02, 2009 at 10:38:27PM +0300, Sergei Shtylyov wrote:
> > Hello.
> > 
> > pancho horrillo wrote:
> > 
> > >Please, review and push the patch, if you deem it adequate.
> > 
> > >Thanks a bunch!
> > 
> >    That 'uint8_t' is for userspace, use 'u8' instead.
> > 
> > WBR, Sergei
> 
> Thanks for the coaching, Sergei!
> 
> BTW, there are about 4863 of uint8_t in the kernel sources :-)
> 
> Cheers,
> 
> pancho.
> 
> Signed-off-by: pancho horrillo <pancho@pancho.name>

Care to resend this one with the proper changelog comment so that I can
apply it?

thanks,

greg k-h

Re: [PATCH 2/2] Fix a bug on appledisplay.c regarding signedness

[pancho horrillo]

On Tue, Dec 22, 2009 at 11:29:37AM -0800, Greg KH wrote:
[...]
> Care to resend this one with the proper changelog comment so that I can
> apply it?
> 
Sure.  Here it goes...  Please let me know if more editing is required.



brightness status is reported by the Apple Cinema Displays as an
'unsigned char' (u8) value, but the code used 'char' instead.

Note that he driver was developed on the PowerPC architecture,
where the two types are synonymous, which is not always the case.

Fixed that.  Otherwise the driver will interpret brightness
levels > 127 as negative, and fail to load.

Signed-off-by: pancho horrillo <pancho@pancho.name>
---
 drivers/usb/misc/appledisplay.c |    4 +-
 1 file changed, 2 insertions(+), 2 deletions(-)

diff -purN a/drivers/usb/misc/appledisplay.c b/drivers/usb/misc/appledisplay.c
--- a/drivers/usb/misc/appledisplay.c	2009-11-10 01:32:31.000000000 +0100
+++ b/drivers/usb/misc/appledisplay.c	2009-12-02 20:05:00.000000000 +0100
@@ -72,8 +72,8 @@ 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 */
+	u8 *urbdata;			/* interrupt URB data buffer */
+	u8 *msgdata;			/* control message data buffer */
 
 	struct delayed_work work;
 	int button_pressed;



> thanks,
really appreciated.  This is my very first (and tiny) contribution to
the linux kernel, even though I've been a user since the 1.2.x series,
back in '96.

> greg k-h

Happy hacking!

pancho.

-- 
pancho horrillo

To be conscious that
you are ignorant is a great step
to knowledge.

		Benjamin Disraeli