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 <[email protected]>
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
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 <[email protected]>
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
Hello.
pancho horrillo wrote:
> Please, review and push the patch, if you deem it adequate.
> Thanks a bunch!
> Signed-off-by: pancho horrillo <[email protected]>
> 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
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 <[email protected]>
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
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
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 <[email protected]>
Care to resend this one with the proper changelog comment so that I can
apply it?
thanks,
greg k-h
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 <[email protected]>
---
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