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@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
