[PATCH 0/4] Some IR fixes for I2C devices on em28xx

[PATCH 0/4] Some IR fixes for I2C devices on em28xx

[Mauro Carvalho Chehab]

Frank pointed that IR was not working with I2C devices. So, I took some
time to fix them.

Tested with Hauppauge WinTV USB2.

Mauro Carvalho Chehab (4):
  [media] em28xx: initialize button/I2C IR earlier
  [media] em28xx: autoload em28xx-rc if the device has an I2C IR
  [media] em28xx: simplify IR names on I2C devices
  [media] em28xx: tell ir-kbd-i2c that WinTV uses an RC5 protocol

 drivers/media/usb/em28xx/em28xx-cards.c |  2 +-
 drivers/media/usb/em28xx/em28xx-input.c | 29 ++++++++++++++++-------------
 2 files changed, 17 insertions(+), 14 deletions(-)

-- 
1.7.11.7

[PATCH 1/4] [media] em28xx: initialize button/I2C IR earlier

[Mauro Carvalho Chehab]

The em28xx-input is used by 3 different types of input devices:
	- devices with buttons (like cameras and grabber devices);
	- devices with I2C remotes;
	- em2860 or latter chips with RC support embedded.
When the device has an I2C remote, all it needs to do is to call
the proper I2C driver (ir-i2c-kbd), passing the proper data to
it, and just leave the code.
Also, button devices have its own init code that doesn't depend on
having an IR or not (as a general rule, they don't have).
So, move its init code to fix bugs introduced by earlier patches
that prevent them to work.

Signed-off-by: Mauro Carvalho Chehab <mchehab@redhat.com>
---
 drivers/media/usb/em28xx/em28xx-input.c | 20 +++++++++++---------
 1 file changed, 11 insertions(+), 9 deletions(-)

diff --git a/drivers/media/usb/em28xx/em28xx-input.c b/drivers/media/usb/em28xx/em28xx-input.c
index 3598221..2a1b3d2 100644
--- a/drivers/media/usb/em28xx/em28xx-input.c
+++ b/drivers/media/usb/em28xx/em28xx-input.c
@@ -590,6 +590,17 @@ static int em28xx_ir_init(struct em28xx *dev)
 	int err = -ENOMEM;
 	u64 rc_type;
 
+	if (dev->board.has_snapshot_button)
+		em28xx_register_snapshot_button(dev);
+
+	if (dev->board.has_ir_i2c) {
+		em28xx_register_i2c_ir(dev);
+#if defined(CONFIG_MODULES) && defined(MODULE)
+		request_module("ir-kbd-i2c");
+#endif
+		return 0;
+	}
+
 	if (dev->board.ir_codes == NULL) {
 		/* No remote control support */
 		em28xx_warn("Remote control support is not available for "
@@ -663,15 +674,6 @@ static int em28xx_ir_init(struct em28xx *dev)
 	if (err)
 		goto error;
 
-	em28xx_register_i2c_ir(dev);
-
-#if defined(CONFIG_MODULES) && defined(MODULE)
-	if (dev->board.has_ir_i2c)
-		request_module("ir-kbd-i2c");
-#endif
-	if (dev->board.has_snapshot_button)
-		em28xx_register_snapshot_button(dev);
-
 	return 0;
 
 error:
-- 
1.7.11.7

[PATCH 2/4] [media] em28xx: autoload em28xx-rc if the device has an I2C IR

[Mauro Carvalho Chehab]

If the device has an I2C IR, em28xx-rc should be loaded by default,
except if the user explicitly requested to not load, via modprobe
option.

Signed-off-by: Mauro Carvalho Chehab <mchehab@redhat.com>
---
 drivers/media/usb/em28xx/em28xx-cards.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/media/usb/em28xx/em28xx-cards.c b/drivers/media/usb/em28xx/em28xx-cards.c
index f5cac47..e17be07 100644
--- a/drivers/media/usb/em28xx/em28xx-cards.c
+++ b/drivers/media/usb/em28xx/em28xx-cards.c
@@ -2912,7 +2912,7 @@ static void request_module_async(struct work_struct *work)
 
 	if (dev->board.has_dvb)
 		request_module("em28xx-dvb");
-	if (dev->board.ir_codes && !disable_ir)
+	if ((dev->board.ir_codes || dev->board.has_ir_i2c) && !disable_ir)
 		request_module("em28xx-rc");
 #endif /* CONFIG_MODULES */
 }
-- 
1.7.11.7

[PATCH 3/4] [media] em28xx: simplify IR names on I2C devices

[Mauro Carvalho Chehab]

The ir-i2c-kbd already adds I2C IR before the name. The way it is,
the devices are named as:
	"i2c IR (i2c IR (EM2840 Hauppaug"
With is ugly and incorrect. After this patch, it is now properly
displayed as:
	"i2c IR (WinTV USB2)"

Signed-off-by: Mauro Carvalho Chehab <mchehab@redhat.com>
---
 drivers/media/usb/em28xx/em28xx-input.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/media/usb/em28xx/em28xx-input.c b/drivers/media/usb/em28xx/em28xx-input.c
index 2a1b3d2..ebbb0aa 100644
--- a/drivers/media/usb/em28xx/em28xx-input.c
+++ b/drivers/media/usb/em28xx/em28xx-input.c
@@ -472,22 +472,22 @@ static void em28xx_register_i2c_ir(struct em28xx *dev)
 	case EM2820_BOARD_TERRATEC_CINERGY_250:
 		dev->init_data.ir_codes = RC_MAP_EM_TERRATEC;
 		dev->init_data.get_key = em28xx_get_key_terratec;
-		dev->init_data.name = "i2c IR (EM28XX Terratec)";
+		dev->init_data.name = "Terratec Cinergy 200/250";
 		break;
 	case EM2820_BOARD_PINNACLE_USB_2:
 		dev->init_data.ir_codes = RC_MAP_PINNACLE_GREY;
 		dev->init_data.get_key = em28xx_get_key_pinnacle_usb_grey;
-		dev->init_data.name = "i2c IR (EM28XX Pinnacle PCTV)";
+		dev->init_data.name = "Pinnacle USB2";
 		break;
 	case EM2820_BOARD_HAUPPAUGE_WINTV_USB_2:
 		dev->init_data.ir_codes = RC_MAP_HAUPPAUGE;
 		dev->init_data.get_key = em28xx_get_key_em_haup;
-		dev->init_data.name = "i2c IR (EM2840 Hauppauge)";
+		dev->init_data.name = "WinTV USB2";
 		break;
 	case EM2820_BOARD_LEADTEK_WINFAST_USBII_DELUXE:
 		dev->init_data.ir_codes = RC_MAP_WINFAST_USBII_DELUXE;
 		dev->init_data.get_key = em28xx_get_key_winfast_usbii_deluxe;
-		dev->init_data.name = "i2c IR (EM2820 Winfast TV USBII Deluxe)";
+		dev->init_data.name = "Winfast TV USBII Deluxe";
 		break;
 	}
 
-- 
1.7.11.7

[PATCH 4/4] [media] em28xx: tell ir-kbd-i2c that WinTV uses an RC5 protocol

[Mauro Carvalho Chehab]

Signed-off-by: Mauro Carvalho Chehab <mchehab@redhat.com>
---
 drivers/media/usb/em28xx/em28xx-input.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/media/usb/em28xx/em28xx-input.c b/drivers/media/usb/em28xx/em28xx-input.c
index ebbb0aa..5b3292c 100644
--- a/drivers/media/usb/em28xx/em28xx-input.c
+++ b/drivers/media/usb/em28xx/em28xx-input.c
@@ -483,6 +483,7 @@ static void em28xx_register_i2c_ir(struct em28xx *dev)
 		dev->init_data.ir_codes = RC_MAP_HAUPPAUGE;
 		dev->init_data.get_key = em28xx_get_key_em_haup;
 		dev->init_data.name = "WinTV USB2";
+		dev->init_data.type = RC_BIT_RC5;
 		break;
 	case EM2820_BOARD_LEADTEK_WINFAST_USBII_DELUXE:
 		dev->init_data.ir_codes = RC_MAP_WINFAST_USBII_DELUXE;
-- 
1.7.11.7

Re: [PATCH 0/4] Some IR fixes for I2C devices on em28xx

[Frank Schäfer]

Am 04.01.2013 22:15, schrieb Mauro Carvalho Chehab:
> Frank pointed that IR was not working with I2C devices. So, I took some
> time to fix them.
>
> Tested with Hauppauge WinTV USB2.
>
> Mauro Carvalho Chehab (4):
>   [media] em28xx: initialize button/I2C IR earlier
>   [media] em28xx: autoload em28xx-rc if the device has an I2C IR
>   [media] em28xx: simplify IR names on I2C devices
>   [media] em28xx: tell ir-kbd-i2c that WinTV uses an RC5 protocol
>
>  drivers/media/usb/em28xx/em28xx-cards.c |  2 +-
>  drivers/media/usb/em28xx/em28xx-input.c | 29 ++++++++++++++++-------------
>  2 files changed, 17 insertions(+), 14 deletions(-)
>

While these patches make I2C IR remote controls working again, they
leave several issues unaddressed which should really be fixed:
1) the i2c client isn't unregistered on module unload. This was the
reason for patch 2 in my series. There is also a FIXME comment about
this in em28xx_release_resources() (although this is the wrong place to
do it).
2) there is no error checking in em28xx_register_i2c_ir().
em28xx_ir_init should really bail out if no i2c device is found.
3) All RC maps should be assigned at the same place, no matter if the
receiver/demodulator is built in or external. Spreading them over the
code is inconsistent and makes the code bug prone.
4) the list of known i2c devices in em28xx-i2c.c misses client address
0x3e >> 1 = 0x1f. See client list in em28xx_register_i2c_ir().
5) there should be a warning message for the case that we call
ir-kbd-i2c with an unknown rc device.
6) because we use our own key polling functions with ir-kbd-i2c, we
should also select the polling interval value manually. That makes
things consistent and avoids confusion.

The rest is a matter of taste / prefered code layout. I'm fine with it.

Regards,
Frank

Re: [PATCH 0/4] Some IR fixes for I2C devices on em28xx

[Frank Schäfer]

Am 05.01.2013 14:22, schrieb Frank Schäfer:
> Am 04.01.2013 22:15, schrieb Mauro Carvalho Chehab:
>> Frank pointed that IR was not working with I2C devices. So, I took some
>> time to fix them.
>>
>> Tested with Hauppauge WinTV USB2.
>>
>> Mauro Carvalho Chehab (4):
>>   [media] em28xx: initialize button/I2C IR earlier
>>   [media] em28xx: autoload em28xx-rc if the device has an I2C IR
>>   [media] em28xx: simplify IR names on I2C devices
>>   [media] em28xx: tell ir-kbd-i2c that WinTV uses an RC5 protocol
>>
>>  drivers/media/usb/em28xx/em28xx-cards.c |  2 +-
>>  drivers/media/usb/em28xx/em28xx-input.c | 29 ++++++++++++++++-------------
>>  2 files changed, 17 insertions(+), 14 deletions(-)
>>
> While these patches make I2C IR remote controls working again, they
> leave several issues unaddressed which should really be fixed:
> 1) the i2c client isn't unregistered on module unload. This was the
> reason for patch 2 in my series. There is also a FIXME comment about
> this in em28xx_release_resources() (although this is the wrong place to
> do it).
> 2) there is no error checking in em28xx_register_i2c_ir().
> em28xx_ir_init should really bail out if no i2c device is found.
> 3) All RC maps should be assigned at the same place, no matter if the
> receiver/demodulator is built in or external. Spreading them over the
> code is inconsistent and makes the code bug prone.
> 4) the list of known i2c devices in em28xx-i2c.c misses client address
> 0x3e >> 1 = 0x1f. See client list in em28xx_register_i2c_ir().
> 5) there should be a warning message for the case that we call
> ir-kbd-i2c with an unknown rc device.
> 6) because we use our own key polling functions with ir-kbd-i2c, we
> should also select the polling interval value manually. That makes
> things consistent and avoids confusion.
>
> The rest is a matter of taste / prefered code layout. I'm fine with it.
>
> Regards,
> Frank

It seems like already applied them... :(

While I certainly appreciate patches beeing applied as soon as possible,
I think there should really be a chance to review them before this happens.
Especially when the changes are non-trivial and someone else has posted
patches addressing the same issues before (other contributers might feel
offended ;) ).

Care to fix these issues ?

Regards,
Frank

Re: [PATCH 0/4] Some IR fixes for I2C devices on em28xx

[Mauro Carvalho Chehab]

Em Sat, 05 Jan 2013 14:42:10 +0100
Frank Schäfer <fschaefer.oss@googlemail.com> escreveu:

> Am 05.01.2013 14:22, schrieb Frank Schäfer:
> > Am 04.01.2013 22:15, schrieb Mauro Carvalho Chehab:
> >> Frank pointed that IR was not working with I2C devices. So, I took some
> >> time to fix them.
> >>
> >> Tested with Hauppauge WinTV USB2.
> >>
> >> Mauro Carvalho Chehab (4):
> >>   [media] em28xx: initialize button/I2C IR earlier
> >>   [media] em28xx: autoload em28xx-rc if the device has an I2C IR
> >>   [media] em28xx: simplify IR names on I2C devices
> >>   [media] em28xx: tell ir-kbd-i2c that WinTV uses an RC5 protocol
> >>
> >>  drivers/media/usb/em28xx/em28xx-cards.c |  2 +-
> >>  drivers/media/usb/em28xx/em28xx-input.c | 29 ++++++++++++++++-------------
> >>  2 files changed, 17 insertions(+), 14 deletions(-)
> >>
> > While these patches make I2C IR remote controls working again, they
> > leave several issues unaddressed which should really be fixed:
> > 1) the i2c client isn't unregistered on module unload. This was the
> > reason for patch 2 in my series. There is also a FIXME comment about
> > this in em28xx_release_resources() (although this is the wrong place to
> > do it).
> > 2) there is no error checking in em28xx_register_i2c_ir().
> > em28xx_ir_init should really bail out if no i2c device is found.
> > 3) All RC maps should be assigned at the same place, no matter if the
> > receiver/demodulator is built in or external. Spreading them over the
> > code is inconsistent and makes the code bug prone.
> > 4) the list of known i2c devices in em28xx-i2c.c misses client address
> > 0x3e >> 1 = 0x1f. See client list in em28xx_register_i2c_ir().
> > 5) there should be a warning message for the case that we call
> > ir-kbd-i2c with an unknown rc device.
> > 6) because we use our own key polling functions with ir-kbd-i2c, we
> > should also select the polling interval value manually. That makes
> > things consistent and avoids confusion.
> >
> > The rest is a matter of taste / prefered code layout. I'm fine with it.
> >
> > Regards,
> > Frank
> 
> It seems like already applied them... :(
> 
> While I certainly appreciate patches beeing applied as soon as possible,
> I think there should really be a chance to review them before this happens.
> Especially when the changes are non-trivial and someone else has posted
> patches addressing the same issues before (other contributers might feel
> offended ;) ).

All the 4 applied patches are really trivial:
	- patch 1: just reorder existing code;
	- patch 2: one-line patch adding another condition to an existing if;
	- patch 3: pure string rename;
	- patch 4: one line patch properly reporting the RC5 protocol on WinTV.

Also, my time is very limited, especially when I need to test a driver, as
I need to allocate a bigger time window. On such cases, I just reorder the
patches to to apply all of them at the same time, to optimize my time.

Also, both Devin and you are working right now at the same driver, and you
both have pending work. Merging the patches quicker helps to avoid merge
conflicts.

Regards,
Mauro

Re: [PATCH 0/4] Some IR fixes for I2C devices on em28xx

[Frank Schäfer]

Am 05.01.2013 16:35, schrieb Mauro Carvalho Chehab:
> Em Sat, 05 Jan 2013 14:42:10 +0100
> Frank Schäfer <fschaefer.oss@googlemail.com> escreveu:
>
>> Am 05.01.2013 14:22, schrieb Frank Schäfer:
>>> Am 04.01.2013 22:15, schrieb Mauro Carvalho Chehab:
>>>> Frank pointed that IR was not working with I2C devices. So, I took some
>>>> time to fix them.
>>>>
>>>> Tested with Hauppauge WinTV USB2.
>>>>
>>>> Mauro Carvalho Chehab (4):
>>>>   [media] em28xx: initialize button/I2C IR earlier
>>>>   [media] em28xx: autoload em28xx-rc if the device has an I2C IR
>>>>   [media] em28xx: simplify IR names on I2C devices
>>>>   [media] em28xx: tell ir-kbd-i2c that WinTV uses an RC5 protocol
>>>>
>>>>  drivers/media/usb/em28xx/em28xx-cards.c |  2 +-
>>>>  drivers/media/usb/em28xx/em28xx-input.c | 29 ++++++++++++++++-------------
>>>>  2 files changed, 17 insertions(+), 14 deletions(-)
>>>>
>>> While these patches make I2C IR remote controls working again, they
>>> leave several issues unaddressed which should really be fixed:
>>> 1) the i2c client isn't unregistered on module unload. This was the
>>> reason for patch 2 in my series. There is also a FIXME comment about
>>> this in em28xx_release_resources() (although this is the wrong place to
>>> do it).
>>> 2) there is no error checking in em28xx_register_i2c_ir().
>>> em28xx_ir_init should really bail out if no i2c device is found.
>>> 3) All RC maps should be assigned at the same place, no matter if the
>>> receiver/demodulator is built in or external. Spreading them over the
>>> code is inconsistent and makes the code bug prone.
>>> 4) the list of known i2c devices in em28xx-i2c.c misses client address
>>> 0x3e >> 1 = 0x1f. See client list in em28xx_register_i2c_ir().
>>> 5) there should be a warning message for the case that we call
>>> ir-kbd-i2c with an unknown rc device.
>>> 6) because we use our own key polling functions with ir-kbd-i2c, we
>>> should also select the polling interval value manually. That makes
>>> things consistent and avoids confusion.
>>>
>>> The rest is a matter of taste / prefered code layout. I'm fine with it.
>>>
>>> Regards,
>>> Frank
>> It seems like already applied them... :(
>>
>> While I certainly appreciate patches beeing applied as soon as possible,
>> I think there should really be a chance to review them before this happens.
>> Especially when the changes are non-trivial and someone else has posted
>> patches addressing the same issues before (other contributers might feel
>> offended ;) ).
> All the 4 applied patches are really trivial:
> 	- patch 1: just reorder existing code;
> 	- patch 2: one-line patch adding another condition to an existing if;
> 	- patch 3: pure string rename;
> 	- patch 4: one line patch properly reporting the RC5 protocol on WinTV.

Just because a patch "just reorders existing code" or "just changes a
single line" it's not automatically trivial.
I'm sure you have seen more cases than me in which it were patches like
this who caused big trouble. ;)

And especially in cases where the changes are under discussion (which I
would say is the case when someone else has posted patches addressing
the same issues before) there should be a minimum chance to react on them.
Isn't that what you would expect from others, too ? ;)

Apart from that, there are also lots of other 'trivial' patches rotting
at patchwork or bugzilla...

> Also, my time is very limited, especially when I need to test a driver, as
> I need to allocate a bigger time window. On such cases, I just reorder the
> patches to to apply all of them at the same time, to optimize my time.

Yeah, I understand your time problems and I really appreciate patches
beeing applied as soon as possible (after they have been reviewed).
But delaying a patch for a few days really shouldn't cause too much
extra work.

> Also, both Devin and you are working right now at the same driver, and you
> both have pending work. Merging the patches quicker helps to avoid merge
> conflicts.

100% agreement, although I don't think these patches are causing any
problems here.

Regards,
Frank

> Regards,
> Mauro

Re: [PATCH 0/4] Some IR fixes for I2C devices on em28xx

[Mauro Carvalho Chehab]

Em Sun, 06 Jan 2013 21:26:46 +0100
Frank Schäfer <fschaefer.oss@googlemail.com> escreveu:

> Am 05.01.2013 16:35, schrieb Mauro Carvalho Chehab:
> > Em Sat, 05 Jan 2013 14:42:10 +0100
> > Frank Schäfer <fschaefer.oss@googlemail.com> escreveu:
> >
> >> Am 05.01.2013 14:22, schrieb Frank Schäfer:
> >>> Am 04.01.2013 22:15, schrieb Mauro Carvalho Chehab:
> >>>> Frank pointed that IR was not working with I2C devices. So, I took some
> >>>> time to fix them.
> >>>>
> >>>> Tested with Hauppauge WinTV USB2.
> >>>>
> >>>> Mauro Carvalho Chehab (4):
> >>>>   [media] em28xx: initialize button/I2C IR earlier
> >>>>   [media] em28xx: autoload em28xx-rc if the device has an I2C IR
> >>>>   [media] em28xx: simplify IR names on I2C devices
> >>>>   [media] em28xx: tell ir-kbd-i2c that WinTV uses an RC5 protocol
> >>>>
> >>>>  drivers/media/usb/em28xx/em28xx-cards.c |  2 +-
> >>>>  drivers/media/usb/em28xx/em28xx-input.c | 29 ++++++++++++++++-------------
> >>>>  2 files changed, 17 insertions(+), 14 deletions(-)
> >>>>
> >>> While these patches make I2C IR remote controls working again, they
> >>> leave several issues unaddressed which should really be fixed:
> >>> 1) the i2c client isn't unregistered on module unload. This was the
> >>> reason for patch 2 in my series. There is also a FIXME comment about
> >>> this in em28xx_release_resources() (although this is the wrong place to
> >>> do it).
> >>> 2) there is no error checking in em28xx_register_i2c_ir().
> >>> em28xx_ir_init should really bail out if no i2c device is found.
> >>> 3) All RC maps should be assigned at the same place, no matter if the
> >>> receiver/demodulator is built in or external. Spreading them over the
> >>> code is inconsistent and makes the code bug prone.
> >>> 4) the list of known i2c devices in em28xx-i2c.c misses client address
> >>> 0x3e >> 1 = 0x1f. See client list in em28xx_register_i2c_ir().
> >>> 5) there should be a warning message for the case that we call
> >>> ir-kbd-i2c with an unknown rc device.
> >>> 6) because we use our own key polling functions with ir-kbd-i2c, we
> >>> should also select the polling interval value manually. That makes
> >>> things consistent and avoids confusion.
> >>>
> >>> The rest is a matter of taste / prefered code layout. I'm fine with it.
> >>>
> >>> Regards,
> >>> Frank
> >> It seems like already applied them... :(
> >>
> >> While I certainly appreciate patches beeing applied as soon as possible,
> >> I think there should really be a chance to review them before this happens.
> >> Especially when the changes are non-trivial and someone else has posted
> >> patches addressing the same issues before (other contributers might feel
> >> offended ;) ).
> > All the 4 applied patches are really trivial:
> > 	- patch 1: just reorder existing code;
> > 	- patch 2: one-line patch adding another condition to an existing if;
> > 	- patch 3: pure string rename;
> > 	- patch 4: one line patch properly reporting the RC5 protocol on WinTV.
> 
> Just because a patch "just reorders existing code" or "just changes a
> single line" it's not automatically trivial.

True, but this is not the case of all the above ones: all of them are obvious,
and do what are described there.

> I'm sure you have seen more cases than me in which it were patches like
> this who caused big trouble. ;)

Trivial patches are known to cause troubles, just like any other code change.

The thing is that trivial patches are:
	1) easy to review, especially when properly described;
	2) when they're wrong, easy to fix;
	3) easy to get them accepted/merged at stable kernels.

In any case, if you found any breakage caused by the above patches, feel
free to send a patch fixing it.

If you're concerned with other things that aren't there, send in separate.

The rule is one patch by each logical change.

> Yeah, I understand your time problems and I really appreciate patches
> beeing applied as soon as possible (after they have been reviewed).
> But delaying a patch for a few days really shouldn't cause too much
> extra work.

It causes, as my main work is unrelated to drivers/media maintenance.
So, it may take weeks for me to be able to get a time slice big enough
for installing a kernel on my test machines and re-test some code.

Cheers,
Mauro

Re: [PATCH 0/4] Some IR fixes for I2C devices on em28xx

[Mauro Carvalho Chehab]

Em Sat, 05 Jan 2013 14:22:08 +0100
Frank Schäfer <fschaefer.oss@googlemail.com> escreveu:

> Am 04.01.2013 22:15, schrieb Mauro Carvalho Chehab:
> > Frank pointed that IR was not working with I2C devices. So, I took some
> > time to fix them.
> >
> > Tested with Hauppauge WinTV USB2.
> >
> > Mauro Carvalho Chehab (4):
> >   [media] em28xx: initialize button/I2C IR earlier
> >   [media] em28xx: autoload em28xx-rc if the device has an I2C IR
> >   [media] em28xx: simplify IR names on I2C devices
> >   [media] em28xx: tell ir-kbd-i2c that WinTV uses an RC5 protocol
> >
> >  drivers/media/usb/em28xx/em28xx-cards.c |  2 +-
> >  drivers/media/usb/em28xx/em28xx-input.c | 29 ++++++++++++++++-------------
> >  2 files changed, 17 insertions(+), 14 deletions(-)
> >
> 
> While these patches make I2C IR remote controls working again, they
> leave several issues unaddressed which should really be fixed:
> 1) the i2c client isn't unregistered on module unload. This was the
> reason for patch 2 in my series. There is also a FIXME comment about
> this in em28xx_release_resources() (although this is the wrong place to
> do it).

AFAIKT, this is not really needed, as the I2C clients are unregistered
when the I2C bus is unregistered.

So, a device disconnect will release it. Also, an em28xx driver unload.

The only difference might be if just ir-kbd-i2c and em28xx-rc are
unloaded, but em28xx is still loaded, but I think that, even on this
case, calling the .release code for an I2C bus will release it.

So, I don't see any need for such patch. I might be wrong, of course, but,
in order to proof that a release code is needed, you'll need to check if
some memory are lost after module load/unload.

> 2) there is no error checking in em28xx_register_i2c_ir().
> em28xx_ir_init should really bail out if no i2c device is found.

A failure to initialize IR should not be fatal for the driver, as the
rest of the hardware still works.

Also, there's no way to warrant that the I2C code is actually running,
as ir-i2c-kbd may not even be compiled.

So, returning 0 there doesn't mean that IR is working.

> 3) All RC maps should be assigned at the same place, no matter if the
> receiver/demodulator is built in or external. Spreading them over the
> code is inconsistent and makes the code bug prone.

I don't agree. It is better to keep RC maps for those devices together
with the RC protocol setting, get_key config, etc. At boards config,
it is very easy to identify I2C IR's, as there's an special field there
to mark those devices (has_ir_i2c). So, if the board has_ir_i2c, the
IR config is inside em28xx-input. That's the same logic that it is
there for has_dvb: if this field is true, the DVB specifics is inside
em28xx-dvb.

> 4) the list of known i2c devices in em28xx-i2c.c misses client address
> 0x3e >> 1 = 0x1f. See client list in em28xx_register_i2c_ir().

Ok. Separate patch, please.

> 5) there should be a warning message for the case that we call
> ir-kbd-i2c with an unknown rc device.

Why? All boards with has_ir_i2c have entries there. I double-checked.
Adding will just bloat the code with no reason. We just need to take
care if we get a patch adding I2C IR support for an old card, to be
sure that data is filled on both places.

Considering that we don't receive any IR I2C code for several years,
and that newer devices won't use that part of the code, it seems highly
unlikely that such code would be ever used.

> 6) because we use our own key polling functions with ir-kbd-i2c, we
> should also select the polling interval value manually. That makes
> things consistent and avoids confusion.

I disagree. The polling interval is mainly dictated by the RC protocol
used (e. g. the minimal time for a repeat code) and by the speed that 
users can type things. It is typically ~100 ms everywhere, except when
there are some exceptional cases, like GPIO polling.

Regards,
Mauro

Re: [PATCH 0/4] Some IR fixes for I2C devices on em28xx

[Frank Schäfer]

Am 05.01.2013 16:06, schrieb Mauro Carvalho Chehab:
> Em Sat, 05 Jan 2013 14:22:08 +0100
> Frank Schäfer <fschaefer.oss@googlemail.com> escreveu:
>
>> Am 04.01.2013 22:15, schrieb Mauro Carvalho Chehab:
>>> Frank pointed that IR was not working with I2C devices. So, I took some
>>> time to fix them.
>>>
>>> Tested with Hauppauge WinTV USB2.
>>>
>>> Mauro Carvalho Chehab (4):
>>>   [media] em28xx: initialize button/I2C IR earlier
>>>   [media] em28xx: autoload em28xx-rc if the device has an I2C IR
>>>   [media] em28xx: simplify IR names on I2C devices
>>>   [media] em28xx: tell ir-kbd-i2c that WinTV uses an RC5 protocol
>>>
>>>  drivers/media/usb/em28xx/em28xx-cards.c |  2 +-
>>>  drivers/media/usb/em28xx/em28xx-input.c | 29 ++++++++++++++++-------------
>>>  2 files changed, 17 insertions(+), 14 deletions(-)
>>>
>> While these patches make I2C IR remote controls working again, they
>> leave several issues unaddressed which should really be fixed:
>> 1) the i2c client isn't unregistered on module unload. This was the
>> reason for patch 2 in my series. There is also a FIXME comment about
>> this in em28xx_release_resources() (although this is the wrong place to
>> do it).
> AFAIKT, this is not really needed, as the I2C clients are unregistered
> when the I2C bus is unregistered.
>
> So, a device disconnect will release it. Also, an em28xx driver unload.
>
> The only difference might be if just ir-kbd-i2c and em28xx-rc are
> unloaded, but em28xx is still loaded, but I think that, even on this
> case, calling the .release code for an I2C bus will release it.
>
> So, I don't see any need for such patch. I might be wrong, of course, but,
> in order to proof that a release code is needed, you'll need to check if
> some memory are lost after module load/unload.

Mauro, just because code luckily 'works' in the current constellation,
it isn't necessarily good code.
It's this kind of issues that can easily cause regressions if the code
changes somewhere else.

em28xx-input registers the i2c device, so it should unregister it on
uninit/close/unload, too.
Pretty simple and easy to fix (+5 - 2 = 3 additional lines of code).

>> 2) there is no error checking in em28xx_register_i2c_ir().
>> em28xx_ir_init should really bail out if no i2c device is found.
> A failure to initialize IR should not be fatal for the driver, as the
> rest of the hardware still works.

I'm talking about em28xx-input and the RC part only. Of course not the
whole driver.
Do you really want to load module ir-kbd-i2c even though there is no
device ?


> Also, there's no way to warrant that the I2C code is actually running,
> as ir-i2c-kbd may not even be compiled.
>
> So, returning 0 there doesn't mean that IR is working.

You can check the success of request_module.

The whole thing is really easy to fix, I fail to see why you don't want
to do it.

>> 3) All RC maps should be assigned at the same place, no matter if the
>> receiver/demodulator is built in or external. Spreading them over the
>> code is inconsistent and makes the code bug prone.
> I don't agree. It is better to keep RC maps for those devices together
> with the RC protocol setting, get_key config, etc. At boards config,
> it is very easy to identify I2C IR's, as there's an special field there
> to mark those devices (has_ir_i2c). So, if the board has_ir_i2c, the
> IR config is inside em28xx-input. 

... which is exactly what made it so easy to cause this regression !!!

It's not obvious for programmers that no RC map has to be specified for
i2c RCs in the board data.
It's also not obvious that em28xx-input silently overwrites the rc-map
assigned at board level.
In general, it's not obvious that two completely different code areas
have to be touched for these devices.
That's why we really should avoid those board specific code parts spread
all over the driver as much as possible.
In case of the RC map it's really easy.

I also fail to see what you would loose in em28xx-input. We would still
assign the RC map to dev->init_data.
If you prefer seeing the used RC map in the em28xx-input code directly,
then the same should apply for devices with built-in IR
recceiver/decoder (which means moving all rc-map assignments to
em28xx-input).
You could also get rid of field ir_codes this way.

> That's the same logic that it is
> there for has_dvb: if this field is true, the DVB specifics is inside
> em28xx-dvb.

Different case.
Avoiding board specific code parts there likely isn't possible (and
reasonable), although it should be the goal.

>> 4) the list of known i2c devices in em28xx-i2c.c misses client address
>> 0x3e >> 1 = 0x1f. See client list in em28xx_register_i2c_ir().
> Ok. Separate patch, please.

Coming soon.

>> 5) there should be a warning message for the case that we call
>> ir-kbd-i2c with an unknown rc device.
> Why? All boards with has_ir_i2c have entries there. I double-checked.
> Adding will just bloat the code with no reason. We just need to take
> care if we get a patch adding I2C IR support for an old card, to be
> sure that data is filled on both places.
>
> Considering that we don't receive any IR I2C code for several years,
> and that newer devices won't use that part of the code, it seems highly
> unlikely that such code would be ever used.

Yes, this is indeed more a theoretical thing.
I think good code should have such a warning, but that's also matter of
taste.
Talking about code bloating seems a bit exaggerated to me, we are
talking about 1 or 2 extra lines of code.


>> 6) because we use our own key polling functions with ir-kbd-i2c, we
>> should also select the polling interval value manually. That makes
>> things consistent and avoids confusion.
> I disagree. The polling interval is mainly dictated by the RC protocol
> used (e. g. the minimal time for a repeat code) and by the speed that 
> users can type things. It is typically ~100 ms everywhere, except when
> there are some exceptional cases, like GPIO polling.

Yepp, and that's the responsibility of the driver. em28xx-input and not
ir-kbd-i2c !
The driver that supplies the key polling functions.

> Regards,
> Mauro


Mauro,
I would like to repeat what I've already said above: just because code
'works' in a specific constellation, it isn't necessarily good code.
The fact that this code has been suffering from a big fat regression for
a long time should really draw our attention !
So let's try to avoid this happening again by fixing the issues the code
has, so that it becomes much more robust.
I'm offering to send patches.

Regards,
Frank

Re: [PATCH 0/4] Some IR fixes for I2C devices on em28xx

[Mauro Carvalho Chehab]

Em Sun, 06 Jan 2013 21:20:31 +0100
Frank Schäfer <fschaefer.oss@googlemail.com> escreveu:

> Am 05.01.2013 16:06, schrieb Mauro Carvalho Chehab:
> > Em Sat, 05 Jan 2013 14:22:08 +0100
> > Frank Schäfer <fschaefer.oss@googlemail.com> escreveu:
> >
> >> Am 04.01.2013 22:15, schrieb Mauro Carvalho Chehab:
> >>> Frank pointed that IR was not working with I2C devices. So, I took some
> >>> time to fix them.
> >>>
> >>> Tested with Hauppauge WinTV USB2.
> >>>
> >>> Mauro Carvalho Chehab (4):
> >>>   [media] em28xx: initialize button/I2C IR earlier
> >>>   [media] em28xx: autoload em28xx-rc if the device has an I2C IR
> >>>   [media] em28xx: simplify IR names on I2C devices
> >>>   [media] em28xx: tell ir-kbd-i2c that WinTV uses an RC5 protocol
> >>>
> >>>  drivers/media/usb/em28xx/em28xx-cards.c |  2 +-
> >>>  drivers/media/usb/em28xx/em28xx-input.c | 29 ++++++++++++++++-------------
> >>>  2 files changed, 17 insertions(+), 14 deletions(-)
> >>>
> >> While these patches make I2C IR remote controls working again, they
> >> leave several issues unaddressed which should really be fixed:
> >> 1) the i2c client isn't unregistered on module unload. This was the
> >> reason for patch 2 in my series. There is also a FIXME comment about
> >> this in em28xx_release_resources() (although this is the wrong place to
> >> do it).
> > AFAIKT, this is not really needed, as the I2C clients are unregistered
> > when the I2C bus is unregistered.
> >
> > So, a device disconnect will release it. Also, an em28xx driver unload.
> >
> > The only difference might be if just ir-kbd-i2c and em28xx-rc are
> > unloaded, but em28xx is still loaded, but I think that, even on this
> > case, calling the .release code for an I2C bus will release it.
> >
> > So, I don't see any need for such patch. I might be wrong, of course, but,
> > in order to proof that a release code is needed, you'll need to check if
> > some memory are lost after module load/unload.
> 
> Mauro, just because code luckily 'works' in the current constellation,
> it isn't necessarily good code.

It doesn't luckly 'works'. It is rock solid. There were really few bug
reports for ir-kbd-i2c during all those years.

> It's this kind of issues that can easily cause regressions if the code
> changes somewhere else.

Nah. What causes regression is touching on a code that works for no
good reason and without enough testing.

Btw, I was told that audio on HVR-950 seemed to stop working.

Not sure what broke it, but, as I tested it some time ago, I suspect
was due to one of those recent patches (v4l2-compliance, vb2 or your
patches - we need to bisect to discover what broke it). None of those
touched on em28xx-alsa directly, but perhaps one of the patches had
a bad side effect.

> em28xx-input registers the i2c device, so it should unregister it on
> uninit/close/unload, too.
> Pretty simple and easy to fix (+5 - 2 = 3 additional lines of code).

Fair enough. Please send us a patch, after enough testing.

> >> 2) there is no error checking in em28xx_register_i2c_ir().
> >> em28xx_ir_init should really bail out if no i2c device is found.
> > A failure to initialize IR should not be fatal for the driver, as the
> > rest of the hardware still works.
> 
> I'm talking about em28xx-input and the RC part only. Of course not the
> whole driver.
> Do you really want to load module ir-kbd-i2c even though there is no
> device ?
> 
> 
> > Also, there's no way to warrant that the I2C code is actually running,
> > as ir-i2c-kbd may not even be compiled.
> >
> > So, returning 0 there doesn't mean that IR is working.
> 
> You can check the success of request_module.

> The whole thing is really easy to fix, I fail to see why you don't want
> to do it.

Ok, such change may make sense, but only as a separate patch, and not as
a big "fix" patch that does something else.

> 
> >> 3) All RC maps should be assigned at the same place, no matter if the
> >> receiver/demodulator is built in or external. Spreading them over the
> >> code is inconsistent and makes the code bug prone.
> > I don't agree. It is better to keep RC maps for those devices together
> > with the RC protocol setting, get_key config, etc. At boards config,
> > it is very easy to identify I2C IR's, as there's an special field there
> > to mark those devices (has_ir_i2c). So, if the board has_ir_i2c, the
> > IR config is inside em28xx-input. 
> 
> ... which is exactly what made it so easy to cause this regression !!!
> 
> It's not obvious for programmers that no RC map has to be specified for
> i2c RCs in the board data.
> It's also not obvious that em28xx-input silently overwrites the rc-map
> assigned at board level.
> In general, it's not obvious that two completely different code areas
> have to be touched for these devices.
> That's why we really should avoid those board specific code parts spread
> all over the driver as much as possible.
> In case of the RC map it's really easy.
> 
> I also fail to see what you would loose in em28xx-input. We would still
> assign the RC map to dev->init_data.
> If you prefer seeing the used RC map in the em28xx-input code directly,
> then the same should apply for devices with built-in IR
> recceiver/decoder (which means moving all rc-map assignments to
> em28xx-input).
> You could also get rid of field ir_codes this way.

I don't agree with the changes you're proposing, for the already explained
reasons. Your arguments are, IMHO, weak, as there are really few devices
with I2C IRs, and I doubt that we'll have a sudden burst of patches
adding new I2C IRs.

> 
> > That's the same logic that it is
> > there for has_dvb: if this field is true, the DVB specifics is inside
> > em28xx-dvb.
> 
> Different case.
> Avoiding board specific code parts there likely isn't possible (and
> reasonable), although it should be the goal.

It is possible. We did it on tm6000. Yet, just like IR's, there are
board-specific functions to initialize DVB device-specific code.

The thing is: on both I2C IR and DVB, you can't specify completely a
board at em28xx cards structure. So, the best is to put everything
board specific related to DVB and RC inside their own .c code, than
to spread it into em28xx cards struct and em28xx-input/em28xx-dvb.

> >> 4) the list of known i2c devices in em28xx-i2c.c misses client address
> >> 0x3e >> 1 = 0x1f. See client list in em28xx_register_i2c_ir().
> > Ok. Separate patch, please.
> 
> Coming soon.
> 
> >> 5) there should be a warning message for the case that we call
> >> ir-kbd-i2c with an unknown rc device.
> > Why? All boards with has_ir_i2c have entries there. I double-checked.
> > Adding will just bloat the code with no reason. We just need to take
> > care if we get a patch adding I2C IR support for an old card, to be
> > sure that data is filled on both places.
> >
> > Considering that we don't receive any IR I2C code for several years,
> > and that newer devices won't use that part of the code, it seems highly
> > unlikely that such code would be ever used.
> 
> Yes, this is indeed more a theoretical thing.
> I think good code should have such a warning, but that's also matter of
> taste.
> Talking about code bloating seems a bit exaggerated to me, we are
> talking about 1 or 2 extra lines of code.
> 
> 
> >> 6) because we use our own key polling functions with ir-kbd-i2c, we
> >> should also select the polling interval value manually. That makes
> >> things consistent and avoids confusion.
> > I disagree. The polling interval is mainly dictated by the RC protocol
> > used (e. g. the minimal time for a repeat code) and by the speed that 
> > users can type things. It is typically ~100 ms everywhere, except when
> > there are some exceptional cases, like GPIO polling.
> 
> Yepp, and that's the responsibility of the driver. em28xx-input and not
> ir-kbd-i2c !
> The driver that supplies the key polling functions.

Again, I disagree.

> 
> > Regards,
> > Mauro
> 
> 
> Mauro,
> I would like to repeat what I've already said above: just because code
> 'works' in a specific constellation, it isn't necessarily good code.
> The fact that this code has been suffering from a big fat regression for
> a long time should really draw our attention !
> So let's try to avoid this happening again by fixing the issues the code
> has, so that it becomes much more robust.
> I'm offering to send patches.

The regression is there just because it is a code that most people don't
use or even care: only very few boards have buttons; only 4 boards use
I2C IRs. Also, because developers didn't test the new code with those
very old hardware (6 years or more) that use I2C IR's.

In practice, very very few people use a TV card with a hardware IR decoder
for remote controllers. They prefer to just disable that code, either at
driver's init or at compile time.

Why? Simple: most people just use their keyboards, as the capture board
is inside their desktop machines.

The ones that use the TV devices like a TV VDR hardware very likely prefer
to buy an mceusb compatible hardware that it is a way more flexible,
being capable of working with other protocols, use hardware IR decoders
and to send IR codes to other devices.

Regards,
Mauro

Re: [PATCH 0/4] Some IR fixes for I2C devices on em28xx

[Frank Schäfer]

Am 07.01.2013 18:04, schrieb Mauro Carvalho Chehab:
> Em Sun, 06 Jan 2013 21:20:31 +0100
> Frank Schäfer <fschaefer.oss@googlemail.com> escreveu:
>
>> Am 05.01.2013 16:06, schrieb Mauro Carvalho Chehab:
>>> Em Sat, 05 Jan 2013 14:22:08 +0100
>>> Frank Schäfer <fschaefer.oss@googlemail.com> escreveu:
>>>
>>>> Am 04.01.2013 22:15, schrieb Mauro Carvalho Chehab:
>>>>> Frank pointed that IR was not working with I2C devices. So, I took some
>>>>> time to fix them.
>>>>>
>>>>> Tested with Hauppauge WinTV USB2.
>>>>>
>>>>> Mauro Carvalho Chehab (4):
>>>>>   [media] em28xx: initialize button/I2C IR earlier
>>>>>   [media] em28xx: autoload em28xx-rc if the device has an I2C IR
>>>>>   [media] em28xx: simplify IR names on I2C devices
>>>>>   [media] em28xx: tell ir-kbd-i2c that WinTV uses an RC5 protocol
>>>>>
>>>>>  drivers/media/usb/em28xx/em28xx-cards.c |  2 +-
>>>>>  drivers/media/usb/em28xx/em28xx-input.c | 29 ++++++++++++++++-------------
>>>>>  2 files changed, 17 insertions(+), 14 deletions(-)
>>>>>
>>>> While these patches make I2C IR remote controls working again, they
>>>> leave several issues unaddressed which should really be fixed:
>>>> 1) the i2c client isn't unregistered on module unload. This was the
>>>> reason for patch 2 in my series. There is also a FIXME comment about
>>>> this in em28xx_release_resources() (although this is the wrong place to
>>>> do it).
>>> AFAIKT, this is not really needed, as the I2C clients are unregistered
>>> when the I2C bus is unregistered.
>>>
>>> So, a device disconnect will release it. Also, an em28xx driver unload.
>>>
>>> The only difference might be if just ir-kbd-i2c and em28xx-rc are
>>> unloaded, but em28xx is still loaded, but I think that, even on this
>>> case, calling the .release code for an I2C bus will release it.
>>>
>>> So, I don't see any need for such patch. I might be wrong, of course, but,
>>> in order to proof that a release code is needed, you'll need to check if
>>> some memory are lost after module load/unload.
>> Mauro, just because code luckily 'works' in the current constellation,
>> it isn't necessarily good code.
> It doesn't luckly 'works'. It is rock solid. 

I disagree here.

> There were really few bug
> reports for ir-kbd-i2c during all those years.

Yeah, but not because the code is so good. ;)
Getting no bug reports doesn't mean that the code is good quality / bug
free (in fact is was broken for a long time).
The 3 main reasons are
1) noone uses the hardware (because it's old, because it's completely
broken, ...)
2) noone want's to report bugs (because they don't know where, think
it's a waste of time, ...)
3) the bugs are reported somewhere else (distros) but not processed /
forwared properly

>> It's this kind of issues that can easily cause regressions if the code
>> changes somewhere else.
> Nah. What causes regression is touching on a code that works for no
> good reason and without enough testing.
>
> Btw, I was told that audio on HVR-950 seemed to stop working.

Yes, and there was another similar bug report recently...

> Not sure what broke it, but, as I tested it some time ago, I suspect
> was due to one of those recent patches (v4l2-compliance, vb2 or your
> patches - we need to bisect to discover what broke it). 

I don't think it was one of those changes.
AFAICS, the bug reports arrived before thes patches were applied.

> None of those
> touched on em28xx-alsa directly, but perhaps one of the patches had
> a bad side effect.

I've done a quick test with my devices but can't reproduce it. And I
didn't look into the audio code too deep yet.
We should ask for further details.


[...]

>>>> 3) All RC maps should be assigned at the same place, no matter if the
>>>> receiver/demodulator is built in or external. Spreading them over the
>>>> code is inconsistent and makes the code bug prone.
>>> I don't agree. It is better to keep RC maps for those devices together
>>> with the RC protocol setting, get_key config, etc. At boards config,
>>> it is very easy to identify I2C IR's, as there's an special field there
>>> to mark those devices (has_ir_i2c). So, if the board has_ir_i2c, the
>>> IR config is inside em28xx-input. 
>> ... which is exactly what made it so easy to cause this regression !!!
>>
>> It's not obvious for programmers that no RC map has to be specified for
>> i2c RCs in the board data.
>> It's also not obvious that em28xx-input silently overwrites the rc-map
>> assigned at board level.
>> In general, it's not obvious that two completely different code areas
>> have to be touched for these devices.
>> That's why we really should avoid those board specific code parts spread
>> all over the driver as much as possible.
>> In case of the RC map it's really easy.
>>
>> I also fail to see what you would loose in em28xx-input. We would still
>> assign the RC map to dev->init_data.
>> If you prefer seeing the used RC map in the em28xx-input code directly,
>> then the same should apply for devices with built-in IR
>> recceiver/decoder (which means moving all rc-map assignments to
>> em28xx-input).
>> You could also get rid of field ir_codes this way.
> I don't agree with the changes you're proposing, for the already explained
> reasons. Your arguments are, IMHO, weak, as there are really few devices
> with I2C IRs, and I doubt that we'll have a sudden burst of patches
> adding new I2C IRs.
...
>
>>
>>> That's the same logic that it is
>>> there for has_dvb: if this field is true, the DVB specifics is inside
>>> em28xx-dvb.
>> Different case.
>> Avoiding board specific code parts there likely isn't possible (and
>> reasonable), although it should be the goal.
> It is possible. We did it on tm6000. Yet, just like IR's, there are
> board-specific functions to initialize DVB device-specific code.
>
> The thing is: on both I2C IR and DVB, you can't specify completely a
> board at em28xx cards structure.

It seems like you still don't understand me. :(
The explanation is a few lines above (and in the parts you skipped). We
don't disagree here.

The ir_codes field is already there an we use it. I don't want to change
that (remove it) and I assume you agree here.
I just don't want to specify the RC maps that should be used for a board
at two completely different code locations without a real need. And I
can't see the need here.
IMHO, this is bad coding style and - more important - it's dangerous.

Take a look at the fix you've applied:

> diff --git a/drivers/media/usb/em28xx/em28xx-cards.c b/drivers/media/usb/em28xx/em28xx-cards.c
> index f5cac47..e17be07 100644
> --- a/drivers/media/usb/em28xx/em28xx-cards.c
> +++ b/drivers/media/usb/em28xx/em28xx-cards.c
> @@ -2912,7 +2912,7 @@ static void request_module_async(struct work_struct *work)
>  
>  	if (dev->board.has_dvb)
>  		request_module("em28xx-dvb");
> -	if (dev->board.ir_codes && !disable_ir)
> +	if ((dev->board.ir_codes || dev->board.has_ir_i2c) && !disable_ir)
>  		request_module("em28xx-rc");
>  #endif /* CONFIG_MODULES */
>  }

You see what had happened ? ;)


>>
>> Mauro,
>> I would like to repeat what I've already said above: just because code
>> 'works' in a specific constellation, it isn't necessarily good code.
>> The fact that this code has been suffering from a big fat regression for
>> a long time should really draw our attention !
>> So let's try to avoid this happening again by fixing the issues the code
>> has, so that it becomes much more robust.
>> I'm offering to send patches.
> The regression is there just because it is a code that most people don't
> use or even care: only very few boards have buttons; only 4 boards use
> I2C IRs. Also, because developers didn't test the new code with those
> very old hardware (6 years or more) that use I2C IR's.

Yeah, sure, that's the main problem. Likely because they didn't have
this legacy hardware anymore for testing.
+1 reason for making the code bullet proof ! ;)

Regards,
Frank