[1/3] ir-kbd-i2c: Allow use of ir-kdb-i2c internal get_key funcs and set ir_type

  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

 




--
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 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.