| Message ID | 1247862937.10066.21.camel@palomino.walls.org (mailing list archive) |
|---|---|
| State | Accepted |
| Delegated to: | Douglas Landgraf |
| Headers | show |
Hi Andy, On Fri, 17 Jul 2009 16:35:37 -0400, Andy Walls wrote: > This patch augments the init data passed by bridge drivers to ir-kbd-i2c > so that the ir_type can be set explicitly and so ir-kbd-i2c internal > get_key functions can be reused without requiring symbols from > ir-kbd-i2c in the bridge driver. > > > Regards, > Andy Looks good. Minor suggestion below: > > diff -r d754a2d5a376 linux/drivers/media/video/ir-kbd-i2c.c > --- a/linux/drivers/media/video/ir-kbd-i2c.c Wed Jul 15 07:28:02 2009 -0300 > +++ b/linux/drivers/media/video/ir-kbd-i2c.c Fri Jul 17 16:05:28 2009 -0400 > @@ -478,7 +480,34 @@ > > ir_codes = init_data->ir_codes; > name = init_data->name; > + if (init_data->type) > + ir_type = init_data->type; > ir->get_key = init_data->get_key; > + switch (init_data->internal_get_key_func) { > + case IR_KBD_GET_KEY_PIXELVIEW: > + ir->get_key = get_key_pixelview; > + break; > + case IR_KBD_GET_KEY_PV951: > + ir->get_key = get_key_pv951; > + break; > + case IR_KBD_GET_KEY_HAUP: > + ir->get_key = get_key_haup; > + break; > + case IR_KBD_GET_KEY_KNC1: > + ir->get_key = get_key_knc1; > + break; > + case IR_KBD_GET_KEY_FUSIONHDTV: > + ir->get_key = get_key_fusionhdtv; > + break; > + case IR_KBD_GET_KEY_HAUP_XVR: > + ir->get_key = get_key_haup_xvr; > + break; > + case IR_KBD_GET_KEY_AVERMEDIA_CARDBUS: > + ir->get_key = get_key_avermedia_cardbus; > + break; > + default: > + break; > + } > } > > /* Make sure we are all setup before going on */ > diff -r d754a2d5a376 linux/include/media/ir-kbd-i2c.h > --- a/linux/include/media/ir-kbd-i2c.h Wed Jul 15 07:28:02 2009 -0300 > +++ b/linux/include/media/ir-kbd-i2c.h Fri Jul 17 16:05:28 2009 -0400 > @@ -24,10 +24,27 @@ > int (*get_key)(struct IR_i2c*, u32*, u32*); > }; > > +enum ir_kbd_get_key_fn { > + IR_KBD_GET_KEY_NONE = 0, As you never use IR_KBD_GET_KEY_NONE, you might as well not define it and start with IR_KBD_GET_KEY_PIXELVIEW = 1. This would have the added advantage that you could get rid of the "default" statement in the above switch, letting gcc warn you (or any other developer) if you ever add a new enum value and forget to handle it in ir_probe(). > + IR_KBD_GET_KEY_PIXELVIEW, > + IR_KBD_GET_KEY_PV951, > + IR_KBD_GET_KEY_HAUP, > + IR_KBD_GET_KEY_KNC1, > + IR_KBD_GET_KEY_FUSIONHDTV, > + IR_KBD_GET_KEY_HAUP_XVR, > + IR_KBD_GET_KEY_AVERMEDIA_CARDBUS, > +}; > + > /* Can be passed when instantiating an ir_video i2c device */ > struct IR_i2c_init_data { > IR_KEYTAB_TYPE *ir_codes; > const char *name; > + int type; /* IR_TYPE_RC5, IR_TYPE_PD, etc */ > + /* > + * Specify either a function pointer or a value indicating one of > + * ir_kbd_i2c's internal get_key functions > + */ > int (*get_key)(struct IR_i2c*, u32*, u32*); > + enum ir_kbd_get_key_fn internal_get_key_func; > }; > #endif
While you folks are looking into ir-kbd-i2c, perhaps one of you will fix the regressions introduced in 2.6.31-* ? The drive no longer detects/works with the I/R port on the Hauppauge PVR-250 cards, which is a user-visible regression. Cheers -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Mark, On Sun, 19 Jul 2009 08:52:09 -0400, Mark Lord wrote: > While you folks are looking into ir-kbd-i2c, > perhaps one of you will fix the regressions > introduced in 2.6.31-* ? > > The drive no longer detects/works with the I/R port on > the Hauppauge PVR-250 cards, which is a user-visible regression. This is bad. If there a bugzilla entry? If not, where can I read more details / get in touch with an affected user?
Jean Delvare wrote: > Hi Mark, > > On Sun, 19 Jul 2009 08:52:09 -0400, Mark Lord wrote: >> While you folks are looking into ir-kbd-i2c, >> perhaps one of you will fix the regressions >> introduced in 2.6.31-* ? >> >> The drive no longer detects/works with the I/R port on >> the Hauppauge PVR-250 cards, which is a user-visible regression. > > This is bad. If there a bugzilla entry? If not, where can I read more > details / get in touch with an affected user? .. I imagine there will be thousands of affected users once the kernel is released, but for now I'll volunteer as a guinea-pig. It is difficult to test with 2.6.31 on the system at present, though, because that kernel also breaks other things that the MythTV box relies on, and the system is in regular use as our only PVR. Right now, all I know is, that the PVR-250 IR port did not show up in /dev/input/ with 2.6.31 after loading ir_kbd_i2c. But it does show up there with all previous kernels going back to the 2.6.1x days. So, to keep the pain level reasonable, perhaps you could send some debugging patches, and I'll apply those, reconfigure the machine for 2.6.31 again, and collect some output for you. And also perhaps try a few things locally as well to speed up the process. Okay? Thanks -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Mark Lord wrote: > Jean Delvare wrote: >> Hi Mark, >> >> On Sun, 19 Jul 2009 08:52:09 -0400, Mark Lord wrote: >>> While you folks are looking into ir-kbd-i2c, >>> perhaps one of you will fix the regressions >>> introduced in 2.6.31-* ? >>> >>> The drive no longer detects/works with the I/R port on >>> the Hauppauge PVR-250 cards, which is a user-visible regression. >> >> This is bad. If there a bugzilla entry? If not, where can I read more >> details / get in touch with an affected user? > .. > > I imagine there will be thousands of affected users once the kernel > is released, but for now I'll volunteer as a guinea-pig. > > It is difficult to test with 2.6.31 on the system at present, though, > because that kernel also breaks other things that the MythTV box relies on, > and the system is in regular use as our only PVR. > > Right now, all I know is, that the PVR-250 IR port did not show up > in /dev/input/ with 2.6.31 after loading ir_kbd_i2c. But it does show > up there with all previous kernels going back to the 2.6.1x days. .. Actually, I meant to say that it does not show up in the output from the lsinput command, whereas it did show up there in all previous kernels. > So, to keep the pain level reasonable, perhaps you could send some > debugging patches, and I'll apply those, reconfigure the machine for > 2.6.31 again, and collect some output for you. And also perhaps try > a few things locally as well to speed up the process. > > Okay? > > Thanks > -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Mark Lord wrote: > Mark Lord wrote: >> Jean Delvare wrote: >>> Hi Mark, >>> >>> On Sun, 19 Jul 2009 08:52:09 -0400, Mark Lord wrote: >>>> While you folks are looking into ir-kbd-i2c, >>>> perhaps one of you will fix the regressions >>>> introduced in 2.6.31-* ? >>>> >>>> The drive no longer detects/works with the I/R port on >>>> the Hauppauge PVR-250 cards, which is a user-visible regression. >>> >>> This is bad. If there a bugzilla entry? If not, where can I read more >>> details / get in touch with an affected user? >> .. >> >> I imagine there will be thousands of affected users once the kernel >> is released, but for now I'll volunteer as a guinea-pig. >> >> It is difficult to test with 2.6.31 on the system at present, though, >> because that kernel also breaks other things that the MythTV box >> relies on, >> and the system is in regular use as our only PVR. >> >> Right now, all I know is, that the PVR-250 IR port did not show up >> in /dev/input/ with 2.6.31 after loading ir_kbd_i2c. But it does show >> up there with all previous kernels going back to the 2.6.1x days. > .. > > Actually, I meant to say that it does not show up in the output from > the lsinput command, whereas it did show up there in all previous kernels. > >> So, to keep the pain level reasonable, perhaps you could send some >> debugging patches, and I'll apply those, reconfigure the machine for >> 2.6.31 again, and collect some output for you. And also perhaps try >> a few things locally as well to speed up the process. .. I'm debugging various other b0rked things in 2.6.31 here right now, so I had a closer look at the Hauppauge I/R remote issue. The ir_kbd_i2c driver *does* still find it after all. But the difference is that the output from 'lsinput' has changed and no longer says "Hauppauge". Which prevents the application from finding the remote control in the same way as before. I'll hack the application code here now to use the new output, but I wonder what the the thousands of other users will do when they first try 2.6.31 after release ? Cheers -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Sun, 19 Jul 2009 09:17:30 -0400, Mark Lord wrote: > Mark Lord wrote: > > Jean Delvare wrote: > >> Hi Mark, > >> > >> On Sun, 19 Jul 2009 08:52:09 -0400, Mark Lord wrote: > >>> While you folks are looking into ir-kbd-i2c, > >>> perhaps one of you will fix the regressions > >>> introduced in 2.6.31-* ? > >>> > >>> The drive no longer detects/works with the I/R port on > >>> the Hauppauge PVR-250 cards, which is a user-visible regression. > >> > >> This is bad. If there a bugzilla entry? If not, where can I read more > >> details / get in touch with an affected user? > > .. > > > > I imagine there will be thousands of affected users once the kernel > > is released, but for now I'll volunteer as a guinea-pig. > > > > It is difficult to test with 2.6.31 on the system at present, though, > > because that kernel also breaks other things that the MythTV box relies on, > > and the system is in regular use as our only PVR. > > > > Right now, all I know is, that the PVR-250 IR port did not show up > > in /dev/input/ with 2.6.31 after loading ir_kbd_i2c. But it does show > > up there with all previous kernels going back to the 2.6.1x days. > .. > > Actually, I meant to say that it does not show up in the output from > the lsinput command, whereas it did show up there in all previous kernels. Never heard of lsinput, where does it come from? > > > So, to keep the pain level reasonable, perhaps you could send some > > debugging patches, and I'll apply those, reconfigure the machine for > > 2.6.31 again, and collect some output for you. And also perhaps try > > a few things locally as well to speed up the process. > > > > Okay? I'd need additional information first, otherwise I have no clue where to start debugging. Which you just sent in a further post, as I see, so I'll follow-up there...
On Sun, 19 Jul 2009 10:38:37 -0400, Mark Lord wrote: > I'm debugging various other b0rked things in 2.6.31 here right now, > so I had a closer look at the Hauppauge I/R remote issue. > > The ir_kbd_i2c driver *does* still find it after all. > But the difference is that the output from 'lsinput' has changed > and no longer says "Hauppauge". Which prevents the application from > finding the remote control in the same way as before. OK, thanks for the investigation. > I'll hack the application code here now to use the new output, > but I wonder what the the thousands of other users will do when > they first try 2.6.31 after release ? Where does lsinput get the string from? What exactly was it before, and what is it exactly in 2.6.31?
Jean Delvare wrote: > On Sun, 19 Jul 2009 10:38:37 -0400, Mark Lord wrote: >> I'm debugging various other b0rked things in 2.6.31 here right now, >> so I had a closer look at the Hauppauge I/R remote issue. >> >> The ir_kbd_i2c driver *does* still find it after all. >> But the difference is that the output from 'lsinput' has changed >> and no longer says "Hauppauge". Which prevents the application from >> finding the remote control in the same way as before. > > OK, thanks for the investigation. > >> I'll hack the application code here now to use the new output, >> but I wonder what the the thousands of other users will do when >> they first try 2.6.31 after release ? > > Where does lsinput get the string from? .. Here's a test program for you: #include <stdio.h> #include <stdlib.h> #include <unistd.h> #include <errno.h> #include <fcntl.h> #include <sys/ioctl.h> #include <linux/input.h> // Invoke with "/dev/input/event4" as argv[1] // // On 2.6.30, this gives the real name, eg. "i2c IR (Hauppauge)". // On 2.6.31, it simply gives "event4" as the "name". int main(int argc, char *argv[]) { char buf[32]; int fd, rc; fd = open(argv[1], O_RDONLY); if (fd == -1) { perror(argv[1]); exit(1); } rc = ioctl(fd,EVIOCGNAME(sizeof(buf)),buf); if (rc >= 0) fprintf(stderr," name : \"%.*s\"\n", rc, buf); return 0; } -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
(resending.. somebody trimmed linux-kernel from the CC: earlier) Jean Delvare wrote: > On Sun, 19 Jul 2009 10:38:37 -0400, Mark Lord wrote: >> I'm debugging various other b0rked things in 2.6.31 here right now, >> so I had a closer look at the Hauppauge I/R remote issue. >> >> The ir_kbd_i2c driver *does* still find it after all. >> But the difference is that the output from 'lsinput' has changed >> and no longer says "Hauppauge". Which prevents the application from >> finding the remote control in the same way as before. > > OK, thanks for the investigation. > >> I'll hack the application code here now to use the new output, >> but I wonder what the the thousands of other users will do when >> they first try 2.6.31 after release ? > > Where does lsinput get the string from? .. Here's a test program for you: #include <stdio.h> #include <stdlib.h> #include <unistd.h> #include <errno.h> #include <fcntl.h> #include <sys/ioctl.h> #include <linux/input.h> // Invoke with "/dev/input/event4" as argv[1] // // On 2.6.30, this gives the real name, eg. "i2c IR (Hauppauge)". // On 2.6.31, it simply gives "event4" as the "name". int main(int argc, char *argv[]) { char buf[32]; int fd, rc; fd = open(argv[1], O_RDONLY); if (fd == -1) { perror(argv[1]); exit(1); } rc = ioctl(fd,EVIOCGNAME(sizeof(buf)),buf); if (rc >= 0) fprintf(stderr," name : \"%.*s\"\n", rc, buf); return 0; } -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Sun, Jul 19, 2009 at 02:26:53PM -0400, Mark Lord wrote: > (resending.. somebody trimmed linux-kernel from the CC: earlier) > > Jean Delvare wrote: >> On Sun, 19 Jul 2009 10:38:37 -0400, Mark Lord wrote: >>> I'm debugging various other b0rked things in 2.6.31 here right now, >>> so I had a closer look at the Hauppauge I/R remote issue. >>> >>> The ir_kbd_i2c driver *does* still find it after all. >>> But the difference is that the output from 'lsinput' has changed >>> and no longer says "Hauppauge". Which prevents the application from >>> finding the remote control in the same way as before. >> >> OK, thanks for the investigation. >> >>> I'll hack the application code here now to use the new output, >>> but I wonder what the the thousands of other users will do when >>> they first try 2.6.31 after release ? >> >> Where does lsinput get the string from? > .. > > Here's a test program for you: > And I think have a fix for that, commit f936601471d1454dacbd3b2a961fd4d883090aeb in the for-linus branch of my tree.
On Sun, 2009-07-19 at 14:47 +0200, Jean Delvare wrote: > Hi Andy, > > On Fri, 17 Jul 2009 16:35:37 -0400, Andy Walls wrote: > > This patch augments the init data passed by bridge drivers to ir-kbd-i2c > > so that the ir_type can be set explicitly and so ir-kbd-i2c internal > > get_key functions can be reused without requiring symbols from > > ir-kbd-i2c in the bridge driver. > > > > > > Regards, > > Andy > > Looks good. Minor suggestion below: Jean, Thanks for the reply. My responses are inline. > > > > diff -r d754a2d5a376 linux/drivers/media/video/ir-kbd-i2c.c > > --- a/linux/drivers/media/video/ir-kbd-i2c.c Wed Jul 15 07:28:02 2009 -0300 > > +++ b/linux/drivers/media/video/ir-kbd-i2c.c Fri Jul 17 16:05:28 2009 -0400 > > @@ -478,7 +480,34 @@ > > > > ir_codes = init_data->ir_codes; > > name = init_data->name; > > + if (init_data->type) > > + ir_type = init_data->type; > > ir->get_key = init_data->get_key; > > + switch (init_data->internal_get_key_func) { > > + case IR_KBD_GET_KEY_PIXELVIEW: > > + ir->get_key = get_key_pixelview; > > + break; > > + case IR_KBD_GET_KEY_PV951: > > + ir->get_key = get_key_pv951; > > + break; > > + case IR_KBD_GET_KEY_HAUP: > > + ir->get_key = get_key_haup; > > + break; > > + case IR_KBD_GET_KEY_KNC1: > > + ir->get_key = get_key_knc1; > > + break; > > + case IR_KBD_GET_KEY_FUSIONHDTV: > > + ir->get_key = get_key_fusionhdtv; > > + break; > > + case IR_KBD_GET_KEY_HAUP_XVR: > > + ir->get_key = get_key_haup_xvr; > > + break; > > + case IR_KBD_GET_KEY_AVERMEDIA_CARDBUS: > > + ir->get_key = get_key_avermedia_cardbus; > > + break; > > + default: > > + break; > > + } > > } > > > > /* Make sure we are all setup before going on */ > > diff -r d754a2d5a376 linux/include/media/ir-kbd-i2c.h > > --- a/linux/include/media/ir-kbd-i2c.h Wed Jul 15 07:28:02 2009 -0300 > > +++ b/linux/include/media/ir-kbd-i2c.h Fri Jul 17 16:05:28 2009 -0400 > > @@ -24,10 +24,27 @@ > > int (*get_key)(struct IR_i2c*, u32*, u32*); > > }; > > > > +enum ir_kbd_get_key_fn { > > + IR_KBD_GET_KEY_NONE = 0, > > As you never use IR_KBD_GET_KEY_NONE, you might as well not define it > and start with IR_KBD_GET_KEY_PIXELVIEW = 1. This would have the added > advantage that you could get rid of the "default" statement in the > above switch, letting gcc warn you (or any other developer) if you ever > add a new enum value and forget to handle it in ir_probe(). >From gcc-4.0.1 docs: -Wswitch Warn whenever a switch statement has an index of enumerated type and lacks a case for one or more of the named codes of that enumeration. (The presence of a default label prevents this warning.) case labels outside the enumeration range also provoke warnings when this option is used. This warning is enabled by -Wall. Since a calling driver may provide a value of 0 via a memset, I'd choose keeping the enum label of IR_KBD_GET_KEY_NONE, add a case for it in the switch(), and remove the "default:" case. It just seems wrong to let drivers pass in 0 value for "internal_get_key_func" and the switch() neither have an explicit nor a "default:" case for it. (Maybe it's just the years of Ada programming that beat things like this into me...) My idea was that a driver would a. for a driver provided function, specify a pointer to the driver's function in "get_key" and set the "internal_get_key_func" field set to 0 (IR_KBD_GET_KEY_NONE) likely via memset(). b. for a ir-kbd-i2c provided function, specify a NULL pointer in "get_key", and use an enumerated value in "internal_get_key_func". If both are specified, the switch() will set to use the ir-kbd-i2c internal function, unless an invalid enum value was used. Regards, Andy > > + IR_KBD_GET_KEY_PIXELVIEW, > > + IR_KBD_GET_KEY_PV951, > > + IR_KBD_GET_KEY_HAUP, > > + IR_KBD_GET_KEY_KNC1, > > + IR_KBD_GET_KEY_FUSIONHDTV, > > + IR_KBD_GET_KEY_HAUP_XVR, > > + IR_KBD_GET_KEY_AVERMEDIA_CARDBUS, > > +}; > > + > > /* Can be passed when instantiating an ir_video i2c device */ > > struct IR_i2c_init_data { > > IR_KEYTAB_TYPE *ir_codes; > > const char *name; > > + int type; /* IR_TYPE_RC5, IR_TYPE_PD, etc */ > > + /* > > + * Specify either a function pointer or a value indicating one of > > + * ir_kbd_i2c's internal get_key functions > > + */ > > int (*get_key)(struct IR_i2c*, u32*, u32*); > > + enum ir_kbd_get_key_fn internal_get_key_func; > > }; > > #endif > > -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Andy, On Mon, 20 Jul 2009 20:07:50 -0400, Andy Walls wrote: > On Sun, 2009-07-19 at 14:47 +0200, Jean Delvare wrote: > > Hi Andy, > > > > On Fri, 17 Jul 2009 16:35:37 -0400, Andy Walls wrote: > > > This patch augments the init data passed by bridge drivers to ir-kbd-i2c > > > so that the ir_type can be set explicitly and so ir-kbd-i2c internal > > > get_key functions can be reused without requiring symbols from > > > ir-kbd-i2c in the bridge driver. > > > > > > > > > Regards, > > > Andy > > > > Looks good. Minor suggestion below: > > Jean, > > Thanks for the reply. My responses are inline. > > > > > > > diff -r d754a2d5a376 linux/drivers/media/video/ir-kbd-i2c.c > > > --- a/linux/drivers/media/video/ir-kbd-i2c.c Wed Jul 15 07:28:02 2009 -0300 > > > +++ b/linux/drivers/media/video/ir-kbd-i2c.c Fri Jul 17 16:05:28 2009 -0400 > > > @@ -478,7 +480,34 @@ > > > > > > ir_codes = init_data->ir_codes; > > > name = init_data->name; > > > + if (init_data->type) > > > + ir_type = init_data->type; > > > ir->get_key = init_data->get_key; > > > + switch (init_data->internal_get_key_func) { > > > + case IR_KBD_GET_KEY_PIXELVIEW: > > > + ir->get_key = get_key_pixelview; > > > + break; > > > + case IR_KBD_GET_KEY_PV951: > > > + ir->get_key = get_key_pv951; > > > + break; > > > + case IR_KBD_GET_KEY_HAUP: > > > + ir->get_key = get_key_haup; > > > + break; > > > + case IR_KBD_GET_KEY_KNC1: > > > + ir->get_key = get_key_knc1; > > > + break; > > > + case IR_KBD_GET_KEY_FUSIONHDTV: > > > + ir->get_key = get_key_fusionhdtv; > > > + break; > > > + case IR_KBD_GET_KEY_HAUP_XVR: > > > + ir->get_key = get_key_haup_xvr; > > > + break; > > > + case IR_KBD_GET_KEY_AVERMEDIA_CARDBUS: > > > + ir->get_key = get_key_avermedia_cardbus; > > > + break; > > > + default: > > > + break; > > > + } > > > } > > > > > > /* Make sure we are all setup before going on */ > > > diff -r d754a2d5a376 linux/include/media/ir-kbd-i2c.h > > > --- a/linux/include/media/ir-kbd-i2c.h Wed Jul 15 07:28:02 2009 -0300 > > > +++ b/linux/include/media/ir-kbd-i2c.h Fri Jul 17 16:05:28 2009 -0400 > > > @@ -24,10 +24,27 @@ > > > int (*get_key)(struct IR_i2c*, u32*, u32*); > > > }; > > > > > > +enum ir_kbd_get_key_fn { > > > + IR_KBD_GET_KEY_NONE = 0, > > > > As you never use IR_KBD_GET_KEY_NONE, you might as well not define it > > and start with IR_KBD_GET_KEY_PIXELVIEW = 1. This would have the added > > advantage that you could get rid of the "default" statement in the > > above switch, letting gcc warn you (or any other developer) if you ever > > add a new enum value and forget to handle it in ir_probe(). > > >From gcc-4.0.1 docs: > > -Wswitch > Warn whenever a switch statement has an index of enumerated type > and lacks a case for one or more of the named codes of that > enumeration. (The presence of a default label prevents this > warning.) case labels outside the enumeration range also provoke > warnings when this option is used. This warning is enabled by > -Wall. > > Since a calling driver may provide a value of 0 via a memset, I'd choose > keeping the enum label of IR_KBD_GET_KEY_NONE, add a case for it in the > switch(), and remove the "default:" case. Yes, that's fine with me too. You might want to rename IR_KBD_GET_KEY_NONE to IR_KBD_GET_KEY_CUSTOM then, and move ir->get_key = init_data->get_key; inside the switch. > It just seems wrong to let > drivers pass in 0 value for "internal_get_key_func" and the switch() > neither have an explicit nor a "default:" case for it. (Maybe it's just > the years of Ada programming that beat things like this into me...) > > My idea was that a driver would > > a. for a driver provided function, specify a pointer to the driver's > function in "get_key" and set the "internal_get_key_func" field set to 0 > (IR_KBD_GET_KEY_NONE) likely via memset(). > > b. for a ir-kbd-i2c provided function, specify a NULL pointer in > "get_key", and use an enumerated value in "internal_get_key_func". > > If both are specified, the switch() will set to use the ir-kbd-i2c > internal function, unless an invalid enum value was used. My key point was that the default case would prevent gcc from helping you. Any solution without the default case is thus fine with me.
diff -r d754a2d5a376 linux/drivers/media/video/ir-kbd-i2c.c --- a/linux/drivers/media/video/ir-kbd-i2c.c Wed Jul 15 07:28:02 2009 -0300 +++ b/linux/drivers/media/video/ir-kbd-i2c.c Fri Jul 17 16:05:28 2009 -0400 @@ -478,7 +480,34 @@ ir_codes = init_data->ir_codes; name = init_data->name; + if (init_data->type) + ir_type = init_data->type; ir->get_key = init_data->get_key; + switch (init_data->internal_get_key_func) { + case IR_KBD_GET_KEY_PIXELVIEW: + ir->get_key = get_key_pixelview; + break; + case IR_KBD_GET_KEY_PV951: + ir->get_key = get_key_pv951; + break; + case IR_KBD_GET_KEY_HAUP: + ir->get_key = get_key_haup; + break; + case IR_KBD_GET_KEY_KNC1: + ir->get_key = get_key_knc1; + break; + case IR_KBD_GET_KEY_FUSIONHDTV: + ir->get_key = get_key_fusionhdtv; + break; + case IR_KBD_GET_KEY_HAUP_XVR: + ir->get_key = get_key_haup_xvr; + break; + case IR_KBD_GET_KEY_AVERMEDIA_CARDBUS: + ir->get_key = get_key_avermedia_cardbus; + break; + default: + break; + } } /* Make sure we are all setup before going on */ diff -r d754a2d5a376 linux/include/media/ir-kbd-i2c.h --- a/linux/include/media/ir-kbd-i2c.h Wed Jul 15 07:28:02 2009 -0300 +++ b/linux/include/media/ir-kbd-i2c.h Fri Jul 17 16:05:28 2009 -0400 @@ -24,10 +24,27 @@ int (*get_key)(struct IR_i2c*, u32*, u32*); }; +enum ir_kbd_get_key_fn { + IR_KBD_GET_KEY_NONE = 0, + IR_KBD_GET_KEY_PIXELVIEW, + IR_KBD_GET_KEY_PV951, + IR_KBD_GET_KEY_HAUP, + IR_KBD_GET_KEY_KNC1, + IR_KBD_GET_KEY_FUSIONHDTV, + IR_KBD_GET_KEY_HAUP_XVR, + IR_KBD_GET_KEY_AVERMEDIA_CARDBUS, +}; + /* Can be passed when instantiating an ir_video i2c device */ struct IR_i2c_init_data { IR_KEYTAB_TYPE *ir_codes; const char *name; + int type; /* IR_TYPE_RC5, IR_TYPE_PD, etc */ + /* + * Specify either a function pointer or a value indicating one of + * ir_kbd_i2c's internal get_key functions + */ int (*get_key)(struct IR_i2c*, u32*, u32*); + enum ir_kbd_get_key_fn internal_get_key_func; }; #endif