[REVIEW] ivtv, ir-kbd-i2c: Explicit IR support for the AVerTV M116 for newer kernels

[REVIEW] ivtv, ir-kbd-i2c: Explicit IR support for the AVerTV M116 for newer kernels

[Andy Walls]

Aleksandr and Jean,

Zdrastvoitye & Bonjour,

To support the AVerMedia M166's IR microcontroller in ivtv and
ir-kbd-i2c with the new i2c binding model, I have added 3 changesets in

	http://linuxtv.org/hg/~awalls/ivtv

01/03: ivtv: Defer legacy I2C IR probing until after setup of known I2C devices
http://linuxtv.org/hg/~awalls/ivtv?cmd=changeset;node=3d243437f046

02/03: ivtv: Add explicit IR controller initialization for the AVerTV M116
http://linuxtv.org/hg/~awalls/ivtv?cmd=changeset;node=0127ed2ea55b

03/03: ir-kbd-i2c: Add support for the AVerTV M116 with the new binding model
http://linuxtv.org/hg/~awalls/ivtv?cmd=changeset;node=c10e0d5d895c


 ir-kbd-i2c.c       |    1 
 ivtv/ivtv-cards.c  |    3 -
 ivtv/ivtv-cards.h  |   35 +++++++-------
 ivtv/ivtv-driver.c |    3 +
 ivtv/ivtv-i2c.c    |  128 ++++++++++++++++++++++++++++++++++++++---------------
 ivtv/ivtv-i2c.h    |    1 
 6 files changed, 118 insertions(+), 53 deletions(-)

I cannot really test them as I still am using an older kernel.  Could
you please review, and test them if possible?

Change 01/03 actually fixes a problem I inadvertantly let slip by for
ivtv in newer kernels, because I missed it in my initial review.  In
ivtv, we should really only do IR chip probing after all other known I2C
devices on a card are registered.

Regards,
Andy

Re: [REVIEW] ivtv, ir-kbd-i2c: Explicit IR support for the AVerTV M116 for newer kernels

[Aleksandr V. Piskunov]

On Sat, Oct 03, 2009 at 11:44:20AM -0400, Andy Walls wrote:
> Aleksandr and Jean,
> 
> Zdrastvoitye & Bonjour,
> 
> To support the AVerMedia M166's IR microcontroller in ivtv and
> ir-kbd-i2c with the new i2c binding model, I have added 3 changesets in
> 
> 	http://linuxtv.org/hg/~awalls/ivtv
> 
> 01/03: ivtv: Defer legacy I2C IR probing until after setup of known I2C devices
> http://linuxtv.org/hg/~awalls/ivtv?cmd=changeset;node=3d243437f046
> 
> 02/03: ivtv: Add explicit IR controller initialization for the AVerTV M116
> http://linuxtv.org/hg/~awalls/ivtv?cmd=changeset;node=0127ed2ea55b
> 
> 03/03: ir-kbd-i2c: Add support for the AVerTV M116 with the new binding model
> http://linuxtv.org/hg/~awalls/ivtv?cmd=changeset;node=c10e0d5d895c
> 
> 
> I cannot really test them as I still am using an older kernel.  Could
> you please review, and test them if possible?
> 

Thank you, Andy! Much more elegant solution than simply pounding 0x40 on every ivtv
board.

Tested on 2.6.30.8, one of Ubuntu mainline kernel builds.

ivtv-i2c part works, ivtv_i2c_new_ir() gets called, according to /sys/bus/i2c
device @ 0x40 gets a name ir_rx_em78p153s_ave.

Now according to my (very) limited understanding of new binding model, ir-kbd-i2c
should attach to this device by its name. Somehow it doesn't, ir-kbd-i2c gets loaded
silently without doing anything.

Re: [REVIEW] ivtv, ir-kbd-i2c: Explicit IR support for the AVerTV M116 for newer kernels

[Jean Delvare]

On Sun, 4 Oct 2009 11:31:39 +0300, Aleksandr V. Piskunov wrote:
> Tested on 2.6.30.8, one of Ubuntu mainline kernel builds.
> 
> ivtv-i2c part works, ivtv_i2c_new_ir() gets called, according to /sys/bus/i2c
> device @ 0x40 gets a name ir_rx_em78p153s_ave.
> 
> Now according to my (very) limited understanding of new binding model, ir-kbd-i2c
> should attach to this device by its name. Somehow it doesn't, ir-kbd-i2c gets loaded
> silently without doing anything.

Change the device name to a shorter string (e.g. "ir_rx_em78p153s").
You're hitting the i2c client name length limit. More details about
this in the details reply I'm writing right now.

-- 
Jean Delvare

Re: [REVIEW] ivtv, ir-kbd-i2c: Explicit IR support for the AVerTV M116 for newer kernels

[Aleksandr V. Piskunov]

On Sun, Oct 04, 2009 at 10:44:52AM +0200, Jean Delvare wrote:
> On Sun, 4 Oct 2009 11:31:39 +0300, Aleksandr V. Piskunov wrote:
> > Tested on 2.6.30.8, one of Ubuntu mainline kernel builds.
> > 
> > ivtv-i2c part works, ivtv_i2c_new_ir() gets called, according to /sys/bus/i2c
> > device @ 0x40 gets a name ir_rx_em78p153s_ave.
> > 
> > Now according to my (very) limited understanding of new binding model, ir-kbd-i2c
> > should attach to this device by its name. Somehow it doesn't, ir-kbd-i2c gets loaded
> > silently without doing anything.
> 
> Change the device name to a shorter string (e.g. "ir_rx_em78p153s").
> You're hitting the i2c client name length limit. More details about
> this in the details reply I'm writing right now.

Thanks, it works now. ir-kbd-i2c picks up the short name, input device is created, remote
works.

Another place where truncation occurs is name field in em78p153s_aver_ir_init_data 
("ivtv-CX23416 EM78P153S AVerMedia"). Actual input device ends up with a name
"i2c IR (ivtv-CX23416 EM78P153S ", limited by char name[32] in IR_i2c struct.

IMHO actual name of resulting input device should be readable by end-user. Perhaps it should
include the short name of the card itself, or model/color of remote control itself if several
revisions exist, etc.

Re: [REVIEW] ivtv, ir-kbd-i2c: Explicit IR support for the AVerTV M116 for newer kernels

[Andy Walls]

On Sun, 2009-10-04 at 12:26 +0300, Aleksandr V. Piskunov wrote:
> On Sun, Oct 04, 2009 at 10:44:52AM +0200, Jean Delvare wrote:
> > On Sun, 4 Oct 2009 11:31:39 +0300, Aleksandr V. Piskunov wrote:
> > > Tested on 2.6.30.8, one of Ubuntu mainline kernel builds.
> > > 
> > > ivtv-i2c part works, ivtv_i2c_new_ir() gets called, according to /sys/bus/i2c
> > > device @ 0x40 gets a name ir_rx_em78p153s_ave.
> > > 
> > > Now according to my (very) limited understanding of new binding model, ir-kbd-i2c
> > > should attach to this device by its name. Somehow it doesn't, ir-kbd-i2c gets loaded
> > > silently without doing anything.
> > 
> > Change the device name to a shorter string (e.g. "ir_rx_em78p153s").
> > You're hitting the i2c client name length limit. More details about
> > this in the details reply I'm writing right now.
> 
> Thanks, it works now. ir-kbd-i2c picks up the short name, input device is created, remote
> works.
> 
> Another place where truncation occurs is name field in em78p153s_aver_ir_init_data 
> ("ivtv-CX23416 EM78P153S AVerMedia"). Actual input device ends up with a name
> "i2c IR (ivtv-CX23416 EM78P153S ", limited by char name[32] in IR_i2c struct.

I'm naive here.  What applications actually show this string or use it?
For what purposes do applications use it?


> IMHO actual name of resulting input device should be readable by end-user. Perhaps it should
> include the short name of the card itself, or model/color of remote control itself if several
> revisions exist, etc.

The em28xx driver uses things like:

	i2c IR (EM28XX Terratec)
	i2c IR (EM28XX Pinnacle PCTV)

The saa7134 driver uses thing like:
	
	Pinnacle PCTV
	Purple TV
	MSI TV@nywhere Plus
	HVR 1110
	BeholdTV

The cx18 driver (i.e. me) uses:

	CX23418 Z8F0811 Hauppauge


I appear to be user-unfriendly. ;)  I guess I like knowing what devices
are precisely involved, as it helps me when I need to troubleshoot.  I
agree that it doesn't help the normal user in day to day operations.

I will change the names to something more card specific.  It could end
up slightly misleading in the long run.  A single card entry in
ivtv-cards.c can describe multiple card variants, but gives all those
variants the same "name" whether or not the consumer retail names are
the same.


Regards,
Andy

Re: [REVIEW] ivtv, ir-kbd-i2c: Explicit IR support for the AVerTV M116 for newer kernels

[Aleksandr V. Piskunov]

On Sun, Oct 04, 2009 at 04:41:01PM -0400, Andy Walls wrote:
> On Sun, 2009-10-04 at 12:26 +0300, Aleksandr V. Piskunov wrote:
> > On Sun, Oct 04, 2009 at 10:44:52AM +0200, Jean Delvare wrote:
> > > On Sun, 4 Oct 2009 11:31:39 +0300, Aleksandr V. Piskunov wrote:
> > > > Tested on 2.6.30.8, one of Ubuntu mainline kernel builds.
> > > > 
> > > > ivtv-i2c part works, ivtv_i2c_new_ir() gets called, according to /sys/bus/i2c
> > > > device @ 0x40 gets a name ir_rx_em78p153s_ave.
> > > > 
> > > > Now according to my (very) limited understanding of new binding model, ir-kbd-i2c
> > > > should attach to this device by its name. Somehow it doesn't, ir-kbd-i2c gets loaded
> > > > silently without doing anything.
> > > 
> > > Change the device name to a shorter string (e.g. "ir_rx_em78p153s").
> > > You're hitting the i2c client name length limit. More details about
> > > this in the details reply I'm writing right now.
> > 
> > Thanks, it works now. ir-kbd-i2c picks up the short name, input device is created, remote
> > works.
> > 
> > Another place where truncation occurs is name field in em78p153s_aver_ir_init_data 
> > ("ivtv-CX23416 EM78P153S AVerMedia"). Actual input device ends up with a name
> > "i2c IR (ivtv-CX23416 EM78P153S ", limited by char name[32] in IR_i2c struct.
> 
> I'm naive here.  What applications actually show this string or use it?
> For what purposes do applications use it?

Mmm, from the top of my head
1) evtest, using it to test remotes and other input devices.
2) gnome-device-manager, displays a tree of hardware devices with details

I hope when the time comes for a media controller stuff, applications will be able to link
all the related hardware, including boards IR device and show this information to end-user.
There it becomes a little bit more important to have a readable and meaningful name.

> > IMHO actual name of resulting input device should be readable by end-user. Perhaps it should
> > include the short name of the card itself, or model/color of remote control itself if several
> > revisions exist, etc.
> 
> The em28xx driver uses things like:
> 
> 	i2c IR (EM28XX Terratec)
> 	i2c IR (EM28XX Pinnacle PCTV)
> 
> The saa7134 driver uses thing like:
> 	
> 	Pinnacle PCTV
> 	Purple TV
> 	MSI TV@nywhere Plus
> 	HVR 1110
> 	BeholdTV
> 
> The cx18 driver (i.e. me) uses:
> 
> 	CX23418 Z8F0811 Hauppauge
> 
> 
> I appear to be user-unfriendly. ;)  I guess I like knowing what devices
> are precisely involved, as it helps me when I need to troubleshoot.  I
> agree that it doesn't help the normal user in day to day operations.
> 
> I will change the names to something more card specific.  It could end
> up slightly misleading in the long run.  A single card entry in
> ivtv-cards.c can describe multiple card variants, but gives all those
> variants the same "name" whether or not the consumer retail names are
> the same.
> 
> 
> Regards,
> Andy
> 
> 

Re: [REVIEW] ivtv, ir-kbd-i2c: Explicit IR support for the AVerTV M116 for newer kernels

[Andy Walls]

On Sun, 2009-10-04 at 11:31 +0300, Aleksandr V. Piskunov wrote:
> On Sat, Oct 03, 2009 at 11:44:20AM -0400, Andy Walls wrote:
> > Aleksandr and Jean,
> > 
> > Zdrastvoitye & Bonjour,
> > 
> > To support the AVerMedia M166's IR microcontroller in ivtv and
> > ir-kbd-i2c with the new i2c binding model, I have added 3 changesets in
> > 
> > 	http://linuxtv.org/hg/~awalls/ivtv
> > 
> > 01/03: ivtv: Defer legacy I2C IR probing until after setup of known I2C devices
> > http://linuxtv.org/hg/~awalls/ivtv?cmd=changeset;node=3d243437f046
> > 
> > 02/03: ivtv: Add explicit IR controller initialization for the AVerTV M116
> > http://linuxtv.org/hg/~awalls/ivtv?cmd=changeset;node=0127ed2ea55b
> > 
> > 03/03: ir-kbd-i2c: Add support for the AVerTV M116 with the new binding model
> > http://linuxtv.org/hg/~awalls/ivtv?cmd=changeset;node=c10e0d5d895c
> > 
> > 
> > I cannot really test them as I still am using an older kernel.  Could
> > you please review, and test them if possible?
> > 
> 
> Thank you, Andy! Much more elegant solution than simply pounding 0x40 on every ivtv
> board.

Thank you.  Of course as Jean has pointed out, I have some things to
clean up.

> Tested on 2.6.30.8, one of Ubuntu mainline kernel builds.

Thank you for testing.

> ivtv-i2c part works, ivtv_i2c_new_ir() gets called, according to /sys/bus/i2c
> device @ 0x40 gets a name ir_rx_em78p153s_ave.
> 
> Now according to my (very) limited understanding of new binding model, ir-kbd-i2c
> should attach to this device by its name. Somehow it doesn't, ir-kbd-i2c gets loaded
> silently without doing anything.

As you probably know, the truncated name cannot be matched.


Regards,
Andy

Re: [REVIEW] ivtv, ir-kbd-i2c: Explicit IR support for the AVerTV M116 for newer kernels

[Jean Delvare]

Hi Andy,

On Sat, 03 Oct 2009 11:44:20 -0400, Andy Walls wrote:
> Aleksandr and Jean,
> 
> Zdrastvoitye & Bonjour,
> 
> To support the AVerMedia M166's IR microcontroller in ivtv and
> ir-kbd-i2c with the new i2c binding model, I have added 3 changesets in
> 
> 	http://linuxtv.org/hg/~awalls/ivtv
> 
> 01/03: ivtv: Defer legacy I2C IR probing until after setup of known I2C devices
> http://linuxtv.org/hg/~awalls/ivtv?cmd=changeset;node=3d243437f046
> 
> 02/03: ivtv: Add explicit IR controller initialization for the AVerTV M116
> http://linuxtv.org/hg/~awalls/ivtv?cmd=changeset;node=0127ed2ea55b
> 
> 03/03: ir-kbd-i2c: Add support for the AVerTV M116 with the new binding model
> http://linuxtv.org/hg/~awalls/ivtv?cmd=changeset;node=c10e0d5d895c
> 
> 
>  ir-kbd-i2c.c       |    1 
>  ivtv/ivtv-cards.c  |    3 -
>  ivtv/ivtv-cards.h  |   35 +++++++-------
>  ivtv/ivtv-driver.c |    3 +
>  ivtv/ivtv-i2c.c    |  128 ++++++++++++++++++++++++++++++++++++++---------------
>  ivtv/ivtv-i2c.h    |    1 
>  6 files changed, 118 insertions(+), 53 deletions(-)
> 
> I cannot really test them as I still am using an older kernel.  Could
> you please review, and test them if possible?

I can't test as I don't have the hardware. But have reviewed your code.

> Change 01/03 actually fixes a problem I inadvertantly let slip by for
> ivtv in newer kernels, because I missed it in my initial review.  In
> ivtv, we should really only do IR chip probing after all other known I2C
> devices on a card are registered.

Conceptually this sounds totally right, indeed. I think this could even
be improved, by skipping IR device probing altogether if a known IR
device has already instantiated. Something like:

#define IVTV_HW_IR_ANY	(IVTV_HW_EM78P153S_IR_RX_AVER)

	/* probe for legacy IR controllers that aren't in card definitions */
	if (!(ivtv->card->hw_all & IVTV_HW_IR_ANY))
		ivtv_i2c_new_ir_legacy(itv);

Comments on the code itself:

Patch #1:

> --- a/linux/drivers/media/video/ivtv/ivtv-i2c.c	Sat Sep 26 13:45:03 2009 -0300
> +++ b/linux/drivers/media/video/ivtv/ivtv-i2c.c	Sat Oct 03 10:28:55 2009 -0400
> @@ -153,6 +153,45 @@
>  	"gpio",
>  };
>  
> +#if LINUX_VERSION_CODE >= KERNEL_VERSION(2, 6, 30)
> +/* Instantiate the IR receiver device using probing -- undesirable */
> +int ivtv_i2c_new_ir_legacy(struct ivtv *itv)
> +{
> +	struct i2c_board_info info;
> +	/*
> +	 * The external IR receiver is at i2c address 0x34.
> +	 * The internal IR receiver is at i2c address 0x30.
> +	 *
> +	 * In theory, both can be fitted, and Hauppauge suggests an external
> +	 * overrides an internal.  That's why we probe 0x1a (~0x34) first. CB
> +	 *
> +	 * Some of these addresses we probe may collide with other i2c address
> +	 * allocations, so this function must be called after all other i2c
> +	 * devices we care about are registered.
> +	 */
> +	const unsigned short addr_list[] = {
> +		0x1a,	/* Hauppauge IR external - collides with WM8739 */
> +		0x18,	/* Hauppauge IR internal */
> +		0x71,	/* Hauppauge IR (PVR150) */
> +		0x64,	/* Pixelview IR */
> +		0x30,	/* KNC ONE IR */
> +		0x6b,	/* Adaptec IR */
> +		I2C_CLIENT_END
> +	};
> +
> +	memset(&info, 0, sizeof(struct i2c_board_info));
> +	strlcpy(info.type, "ir_video", I2C_NAME_SIZE);
> +	return i2c_new_probed_device(&itv->i2c_adap, &info, addr_list) == NULL
> +								       ? -1 : 0;

Why don't you just return the i2c client instead? This is less work,
and you don't use the returned value currently anyway.

> +}
> +#else
> +int ivtv_i2c_new_ir_legacy(struct ivtv *itv)
> +{
> +	/* ir-kbd-i2c does the legacy I2C IR controller probe for old kernels*/

Missing space before end of comment.

> +	return -1;
> +}
> +#endif
> +
>  int ivtv_i2c_register(struct ivtv *itv, unsigned idx)
>  {
>  	struct v4l2_subdev *sd;

Patch #2:

> # HG changeset patch
> # User Andy Walls <awalls@radix.net>
> # Date 1254583380 14400
> # Node ID 0127ed2ea55b215cc17660a698a19ac990117a46
> # Parent  3d243437f04695220d92a0e906f287968e56d328
> ivtv: Add explicit IR controller initialization for the AVerTV M116
> 
> From: Andy Walls <awalls@radix.net>
> 
> Priority: normal
> 
> Signed-off-by: Andy Walls <awalls@radix.net>
> 
> --- a/linux/drivers/media/video/ivtv/ivtv-cards.c	Sat Oct 03 10:28:55 2009 -0400
> +++ b/linux/drivers/media/video/ivtv/ivtv-cards.c	Sat Oct 03 11:23:00 2009 -0400
> @@ -955,7 +955,8 @@
>  	.hw_video = IVTV_HW_CX25840,
>  	.hw_audio = IVTV_HW_CX25840,
>  	.hw_audio_ctrl = IVTV_HW_CX25840,
> -	.hw_all = IVTV_HW_CX25840 | IVTV_HW_TUNER | IVTV_HW_WM8739,
> +	.hw_all = IVTV_HW_CX25840 | IVTV_HW_TUNER | IVTV_HW_WM8739 |
> +		  IVTV_HW_EM78P153S_IR_RX_AVER,
>  	.video_inputs = {
>  		{ IVTV_CARD_INPUT_VID_TUNER,  0, CX25840_COMPOSITE2 },
>  		{ IVTV_CARD_INPUT_SVIDEO1,    1, CX25840_SVIDEO3    },
> --- a/linux/drivers/media/video/ivtv/ivtv-cards.h	Sat Oct 03 10:28:55 2009 -0400
> +++ b/linux/drivers/media/video/ivtv/ivtv-cards.h	Sat Oct 03 11:23:00 2009 -0400
> @@ -87,23 +87,24 @@
>  #define IVTV_PCI_ID_GOTVIEW1		0xffac
>  #define IVTV_PCI_ID_GOTVIEW2 		0xffad
>  
> -/* hardware flags, no gaps allowed, IVTV_HW_GPIO must always be last */
> -#define IVTV_HW_CX25840   (1 << 0)
> -#define IVTV_HW_SAA7115   (1 << 1)
> -#define IVTV_HW_SAA7127   (1 << 2)
> -#define IVTV_HW_MSP34XX   (1 << 3)
> -#define IVTV_HW_TUNER     (1 << 4)
> -#define IVTV_HW_WM8775    (1 << 5)
> -#define IVTV_HW_CS53L32A  (1 << 6)
> -#define IVTV_HW_TVEEPROM  (1 << 7)
> -#define IVTV_HW_SAA7114   (1 << 8)
> -#define IVTV_HW_UPD64031A (1 << 9)
> -#define IVTV_HW_UPD6408X  (1 << 10)
> -#define IVTV_HW_SAA717X   (1 << 11)
> -#define IVTV_HW_WM8739    (1 << 12)
> -#define IVTV_HW_VP27SMPX  (1 << 13)
> -#define IVTV_HW_M52790    (1 << 14)
> -#define IVTV_HW_GPIO      (1 << 15)
> +/* hardware flags, no gaps allowed */
> +#define IVTV_HW_CX25840			(1 << 0)
> +#define IVTV_HW_SAA7115			(1 << 1)
> +#define IVTV_HW_SAA7127			(1 << 2)
> +#define IVTV_HW_MSP34XX			(1 << 3)
> +#define IVTV_HW_TUNER			(1 << 4)
> +#define IVTV_HW_WM8775			(1 << 5)
> +#define IVTV_HW_CS53L32A		(1 << 6)
> +#define IVTV_HW_TVEEPROM		(1 << 7)
> +#define IVTV_HW_SAA7114			(1 << 8)
> +#define IVTV_HW_UPD64031A		(1 << 9)
> +#define IVTV_HW_UPD6408X		(1 << 10)
> +#define IVTV_HW_SAA717X			(1 << 11)
> +#define IVTV_HW_WM8739			(1 << 12)
> +#define IVTV_HW_VP27SMPX		(1 << 13)
> +#define IVTV_HW_M52790			(1 << 14)
> +#define IVTV_HW_GPIO			(1 << 15)
> +#define IVTV_HW_EM78P153S_IR_RX_AVER	(1 << 16)
>  
>  #define IVTV_HW_SAA711X   (IVTV_HW_SAA7115 | IVTV_HW_SAA7114)
>  
> --- a/linux/drivers/media/video/ivtv/ivtv-i2c.c	Sat Oct 03 10:28:55 2009 -0400
> +++ b/linux/drivers/media/video/ivtv/ivtv-i2c.c	Sat Oct 03 11:23:00 2009 -0400
> @@ -63,6 +63,9 @@
>  #include "ivtv-cards.h"
>  #include "ivtv-gpio.h"
>  #include "ivtv-i2c.h"
> +#if LINUX_VERSION_CODE >= KERNEL_VERSION(2, 6, 30)
> +#include <media/ir-kbd-i2c.h>
> +#endif
>  
>  /* i2c implementation for cx23415/6 chip, ivtv project.
>   * Author: Kevin Thayer (nufan_wfk at yahoo.com)
> @@ -88,6 +91,7 @@
>  #define IVTV_UPD64083_I2C_ADDR 		0x5c
>  #define IVTV_VP27SMPX_I2C_ADDR      	0x5b
>  #define IVTV_M52790_I2C_ADDR      	0x48
> +#define IVTV_EM78P153S_IR_RX_I2C_ADDR	0x40
>  
>  /* This array should match the IVTV_HW_ defines */
>  static const u8 hw_addrs[] = {
> @@ -106,7 +110,8 @@
>  	IVTV_WM8739_I2C_ADDR,
>  	IVTV_VP27SMPX_I2C_ADDR,
>  	IVTV_M52790_I2C_ADDR,
> -	0 		/* IVTV_HW_GPIO dummy driver ID */
> +	0,				/* IVTV_HW_GPIO dummy driver ID */
> +	IVTV_EM78P153S_IR_RX_I2C_ADDR	/* IVTV_HW_EM78P153S_IR_RX_AVER */
>  };

I suspect Hans put the GPIO at the end on purpose, I'm not sure why you
want to change this? It makes your patch slightly larger.

>  
>  /* This array should match the IVTV_HW_ defines */
> @@ -126,7 +131,8 @@
>  	"wm8739",
>  	"vp27smpx",
>  	"m52790",
> -	NULL
> +	NULL,
> +	NULL		/* IVTV_HW_EM78P153S_IR_RX_AVER */
>  };
>  
>  /* This array should match the IVTV_HW_ defines */
> @@ -151,9 +157,38 @@
>  	"vp27smpx",
>  	"m52790",
>  	"gpio",
> +	"ir_rx_em78p153s_aver",

This exceeds the maximum length for I2C client names (19 chars max.) So
your patch won't work. I could make the name field slightly larger (say
23 chars) if really needed, but I'd rather have you simply use shorter
names.

>  };
>  
>  #if LINUX_VERSION_CODE >= KERNEL_VERSION(2, 6, 30)
> +static const struct IR_i2c_init_data em78p153s_aver_ir_init_data = {
> +	.ir_codes = &ir_codes_avermedia_cardbus_table,
> +	.internal_get_key_func = IR_KBD_GET_KEY_AVERMEDIA_CARDBUS,
> +	.type = IR_TYPE_OTHER,
> +	.name = "ivtv-CX23416 EM78P153S AVerMedia",
> +};
> +
> +static int ivtv_i2c_new_ir(struct i2c_adapter *adap, u32 hw, const char *type,
> +			   u8 addr)
> +{
> +	struct i2c_board_info info;
> +	unsigned short addr_list[2] = { addr, I2C_CLIENT_END };
> +
> +	memset(&info, 0, sizeof(struct i2c_board_info));
> +	strlcpy(info.type, type, I2C_NAME_SIZE);
> +
> +	/* Our default information for ir-kbd-i2c.c to use */
> +	switch (hw) {
> +	case IVTV_HW_EM78P153S_IR_RX_AVER:
> +		info.platform_data = (void *) &em78p153s_aver_ir_init_data;

Useless cast. You never need to cast to void *.

> +		break;
> +	default:
> +		break;

Useless statement.

> +	}
> +
> +	return i2c_new_probed_device(adap, &info, addr_list) == NULL ? -1 : 0;

I don't really get why you use i2c_new_probed_device() instead of the
cheaper i2c_new_device(). Are you not 100% certain that the IR device
is present on this model?

> +}
> +
>  /* Instantiate the IR receiver device using probing -- undesirable */
>  int ivtv_i2c_new_ir_legacy(struct ivtv *itv)
>  {
> @@ -185,9 +220,15 @@
>  								       ? -1 : 0;
>  }
>  #else
> +/* ir-kbd-i2c does the legacy I2C IR controller probe for old kernels*/

Missing space at end of comment.

> +static int ivtv_i2c_new_ir(struct i2c_adapter *adap, u32 hw, const char *type,
> +			   u8 addr)
> +{
> +	return -1;
> +}
> +
>  int ivtv_i2c_new_ir_legacy(struct ivtv *itv)
>  {
> -	/* ir-kbd-i2c does the legacy I2C IR controller probe for old kernels*/
>  	return -1;
>  }
>  #endif
> @@ -221,8 +262,15 @@
>  			sd->grp_id = 1 << idx;
>  		return sd ? 0 : -1;
>  	}
> +
> +	if (hw & IVTV_HW_EM78P153S_IR_RX_AVER)

Maybe use IVTV_HW_IR_ANY as I defined earlier? Otherwise you'll have to
modify the code with each new remote control you add.

> +		return ivtv_i2c_new_ir(adap, hw, type, hw_addrs[idx]);
> +
> +	/* Is it not an I2C device or one we do not wish to register? */
>  	if (!hw_addrs[idx])
>  		return -1;
> +
> +	/* It's an I2C device other than an analog tuner or IR chip */
>  	if (hw == IVTV_HW_UPD64031A || hw == IVTV_HW_UPD6408X) {
>  		sd = v4l2_i2c_new_subdev(&itv->v4l2_dev,
>  				adap, mod, type, 0, I2C_ADDRS(hw_addrs[idx]));

Patch #3.

> --- a/linux/drivers/media/video/ir-kbd-i2c.c	Sat Oct 03 11:23:00 2009 -0400
> +++ b/linux/drivers/media/video/ir-kbd-i2c.c	Sat Oct 03 11:27:19 2009 -0400
> @@ -730,6 +730,7 @@
>  	{ "ir_video", 0 },
>  	/* IR device specific entries should be added here */
>  	{ "ir_rx_z8f0811_haup", 0 },
> +	{ "ir_rx_em78p153s_aver", 0 },
>  	{ }
>  };
>  

I think we need to discuss this. I don't really see the point of adding
new entries if the ir-kbd-i2c driver doesn't do anything about it. This
makes device probing slower with no benefit. As long as you pass device
information with all the details, the ir-kbd-i2c driver won't care
about the device name.

So the question is, where are we going with the ir-kbd-i2c driver? Are
we happy with the current model where bridge drivers pass IR device
information? Or do we want to move to a model where they just pass a
device name and ir-kbd-i2c maps names to device information? In the
latter case, it makes sense to have many i2c_device_id entries in
ir-kbd-i2c, but in the former case it doesn't.

I guess the answer depends in part on how common IR devices and remote
controls are across adapters. If the same IR device is used on many
adapters then it makes some sense to move the definitions into
ir-kbd-i2c. But if devices are heavily adapter-dependent, and moving
the definitions into ir-kbd-i2c doesn't allow for any code refactoring,
then I don't quite see the point.

-- 
Jean Delvare

Re: [REVIEW] ivtv, ir-kbd-i2c: Explicit IR support for the AVerTV M116 for newer kernels

[Aleksandr V. Piskunov]

> Patch #3.
> 
> > --- a/linux/drivers/media/video/ir-kbd-i2c.c	Sat Oct 03 11:23:00 2009 -0400
> > +++ b/linux/drivers/media/video/ir-kbd-i2c.c	Sat Oct 03 11:27:19 2009 -0400
> > @@ -730,6 +730,7 @@
> >  	{ "ir_video", 0 },
> >  	/* IR device specific entries should be added here */
> >  	{ "ir_rx_z8f0811_haup", 0 },
> > +	{ "ir_rx_em78p153s_aver", 0 },
> >  	{ }
> >  };
> >  
> 
> I think we need to discuss this. I don't really see the point of adding
> new entries if the ir-kbd-i2c driver doesn't do anything about it. This
> makes device probing slower with no benefit. As long as you pass device
> information with all the details, the ir-kbd-i2c driver won't care
> about the device name.
> 
> So the question is, where are we going with the ir-kbd-i2c driver? Are
> we happy with the current model where bridge drivers pass IR device
> information? Or do we want to move to a model where they just pass a
> device name and ir-kbd-i2c maps names to device information? In the
> latter case, it makes sense to have many i2c_device_id entries in
> ir-kbd-i2c, but in the former case it doesn't.
> 
> I guess the answer depends in part on how common IR devices and remote
> controls are across adapters. If the same IR device is used on many
> adapters then it makes some sense to move the definitions into
> ir-kbd-i2c. But if devices are heavily adapter-dependent, and moving
> the definitions into ir-kbd-i2c doesn't allow for any code refactoring,
> then I don't quite see the point.

I believe when it comes to TV cards, IR RX devices and remotes are mostly
vendor-dependent thing, hardware design is usually reused in different
products of same vendor.

Did some research into Avermedia IR stuff while trying to get my M116 working.

For example during last ~10 years Avermedia had 6(?) remote unit models,
covering entire range of its TV cards (models FP/KH, HR/KJ, HV, JX, KS, KV).
Model name is clearly printed on the remote near the vendor label. There is
no clear connection or logic in how remotes are assigned to product. Analog
USB Volar AX stick has a bulky JX remote, while the more advanced hybrid
Volar HX variant of same stick has a smaller/cheaper HR/KJ remote.

How IR RX part is handled also seems to be device specific, but generally
there are several designs being repeated on different models.

Like the above mentioned 8-bit controller design, it either Elan EM78P153S
or Sonix SN8P2501A on I2C bus, or perhaps other compatible 8-bit controller:
* AVerTV Cardbus Plus (Cardbus, saa713x, SN8P2501A)
  http://www.ixbt.com/monitor/images/aver-cardbus-plus/inside-front.jpg
* AVerTV USB2.0 Plus (USB, tvp5150, SN8P2501A) (not supported?)
  http://www.ixbt.com/monitor/images/aver-usb20-plus/inside-front.jpg
* AVerTV MCE 116 Plus (PCI, ivtv, EM78P153S or SN8P2501A)
  http://www.ixbt.com/monitor/images/aver-m116-plus/aver-m116-plus-front.jpg
* AVerTV Studio 709 (PCI, saa713x, EM78P153S)
  http://www.ixbt.com/monitor/images/aver-709/aver-709-front.jpg

So different buses, different bridge drivers, same IR RX controller design.

Re: [REVIEW] ivtv, ir-kbd-i2c: Explicit IR support for the AVerTV M116 for newer kernels

[Andy Walls]

On Sun, 2009-10-04 at 14:33 +0300, Aleksandr V. Piskunov wrote:
> > Patch #3.
> > 
> > > --- a/linux/drivers/media/video/ir-kbd-i2c.c	Sat Oct 03 11:23:00 2009 -0400
> > > +++ b/linux/drivers/media/video/ir-kbd-i2c.c	Sat Oct 03 11:27:19 2009 -0400
> > > @@ -730,6 +730,7 @@
> > >  	{ "ir_video", 0 },
> > >  	/* IR device specific entries should be added here */
> > >  	{ "ir_rx_z8f0811_haup", 0 },
> > > +	{ "ir_rx_em78p153s_aver", 0 },
> > >  	{ }
> > >  };
> > >  
> > 
> > I think we need to discuss this. I don't really see the point of adding
> > new entries if the ir-kbd-i2c driver doesn't do anything about it. This
> > makes device probing slower with no benefit. As long as you pass device
> > information with all the details, the ir-kbd-i2c driver won't care
> > about the device name.
> > 
> > So the question is, where are we going with the ir-kbd-i2c driver? Are
> > we happy with the current model where bridge drivers pass IR device
> > information? Or do we want to move to a model where they just pass a
> > device name and ir-kbd-i2c maps names to device information? In the
> > latter case, it makes sense to have many i2c_device_id entries in
> > ir-kbd-i2c, but in the former case it doesn't.
> > 
> > I guess the answer depends in part on how common IR devices and remote
> > controls are across adapters. If the same IR device is used on many
> > adapters then it makes some sense to move the definitions into
> > ir-kbd-i2c. But if devices are heavily adapter-dependent, and moving
> > the definitions into ir-kbd-i2c doesn't allow for any code refactoring,
> > then I don't quite see the point.
> 
> I believe when it comes to TV cards, IR RX devices and remotes are mostly
> vendor-dependent thing, hardware design is usually reused in different
> products of same vendor.
> 
> Did some research into Avermedia IR stuff while trying to get my M116 working.
> 
> For example during last ~10 years Avermedia had 6(?) remote unit models,
> covering entire range of its TV cards (models FP/KH, HR/KJ, HV, JX, KS, KV).
> Model name is clearly printed on the remote near the vendor label. There is
> no clear connection or logic in how remotes are assigned to product. Analog
> USB Volar AX stick has a bulky JX remote, while the more advanced hybrid
> Volar HX variant of same stick has a smaller/cheaper HR/KJ remote.
> 
> How IR RX part is handled also seems to be device specific, but generally
> there are several designs being repeated on different models.
> 
> Like the above mentioned 8-bit controller design, it either Elan EM78P153S
> or Sonix SN8P2501A on I2C bus, or perhaps other compatible 8-bit controller:
> * AVerTV Cardbus Plus (Cardbus, saa713x, SN8P2501A)
>   http://www.ixbt.com/monitor/images/aver-cardbus-plus/inside-front.jpg
> * AVerTV USB2.0 Plus (USB, tvp5150, SN8P2501A) (not supported?)
>   http://www.ixbt.com/monitor/images/aver-usb20-plus/inside-front.jpg
> * AVerTV MCE 116 Plus (PCI, ivtv, EM78P153S or SN8P2501A)
>   http://www.ixbt.com/monitor/images/aver-m116-plus/aver-m116-plus-front.jpg
> * AVerTV Studio 709 (PCI, saa713x, EM78P153S)
>   http://www.ixbt.com/monitor/images/aver-709/aver-709-front.jpg
> 
> So different buses, different bridge drivers, same IR RX controller design.

Aleksandr,

Good research!


Hauppauge did the same sort of thing with the

	PVR-150 (later models): PCI, ivtv, Z8F0811
	HVR-1600: PCI, cx18, Z8F0811
	HD-PVR: USB, hdpvr, Z8F0811

Also, I think the PVR-500, HVR-1200/1250/1700/1800 units may use this
microcontroller as well.

I have no idea about commonality of the remotes. 

So commonality really only happens within the products a vendor
supports.  What remains common appears to be:

- Microcontroller
- Vendor loaded microcontroller firmware
- Presence of an IR transmitter along with the receiver.

Regards,
Andy

Re: [REVIEW] ivtv, ir-kbd-i2c: Explicit IR support for the AVerTV M116 for newer kernels

[Andy Walls]

On Sun, 2009-10-04 at 10:54 +0200, Jean Delvare wrote:
> Hi Andy,
> 
> On Sat, 03 Oct 2009 11:44:20 -0400, Andy Walls wrote:
> > Aleksandr and Jean,

> > Change 01/03 actually fixes a problem I inadvertantly let slip by for
> > ivtv in newer kernels, because I missed it in my initial review.  In
> > ivtv, we should really only do IR chip probing after all other known I2C
> > devices on a card are registered.
> 
> Conceptually this sounds totally right, indeed. I think this could even
> be improved, by skipping IR device probing altogether if a known IR
> device has already instantiated. Something like:
> 
> #define IVTV_HW_IR_ANY	(IVTV_HW_EM78P153S_IR_RX_AVER)

Yes, I was planning on defining something like this in the long run.


> 	/* probe for legacy IR controllers that aren't in card definitions */
> 	if (!(ivtv->card->hw_all & IVTV_HW_IR_ANY))
> 		ivtv_i2c_new_ir_legacy(itv);

But I hadn't thought about doing that: good idea.

> Comments on the code itself:
> 
> Patch #1:
> 
> > --- a/linux/drivers/media/video/ivtv/ivtv-i2c.c	Sat Sep 26 13:45:03 2009 -0300
> > +++ b/linux/drivers/media/video/ivtv/ivtv-i2c.c	Sat Oct 03 10:28:55 2009 -0400
> > @@ -153,6 +153,45 @@
> >  	"gpio",
> >  };
> >  
> > +#if LINUX_VERSION_CODE >= KERNEL_VERSION(2, 6, 30)
> > +/* Instantiate the IR receiver device using probing -- undesirable */
> > +int ivtv_i2c_new_ir_legacy(struct ivtv *itv)
> > +{
> > +	struct i2c_board_info info;
> > +	/*
> > +	 * The external IR receiver is at i2c address 0x34.
> > +	 * The internal IR receiver is at i2c address 0x30.
> > +	 *
> > +	 * In theory, both can be fitted, and Hauppauge suggests an external
> > +	 * overrides an internal.  That's why we probe 0x1a (~0x34) first. CB
> > +	 *
> > +	 * Some of these addresses we probe may collide with other i2c address
> > +	 * allocations, so this function must be called after all other i2c
> > +	 * devices we care about are registered.
> > +	 */
> > +	const unsigned short addr_list[] = {
> > +		0x1a,	/* Hauppauge IR external - collides with WM8739 */
> > +		0x18,	/* Hauppauge IR internal */
> > +		0x71,	/* Hauppauge IR (PVR150) */
> > +		0x64,	/* Pixelview IR */
> > +		0x30,	/* KNC ONE IR */
> > +		0x6b,	/* Adaptec IR */
> > +		I2C_CLIENT_END
> > +	};
> > +
> > +	memset(&info, 0, sizeof(struct i2c_board_info));
> > +	strlcpy(info.type, "ir_video", I2C_NAME_SIZE);
> > +	return i2c_new_probed_device(&itv->i2c_adap, &info, addr_list) == NULL
> > +								       ? -1 : 0;
> 
> Why don't you just return the i2c client instead? This is less work,
> and you don't use the returned value currently anyway.

I can, but I have no real use for that value either.  What you see here
was a variation of something I did in the cx18 driver.

Honestly I'd like to wrap all these i2C IR controller's into a
v4l2_subdev upon detection and start treating them that way.  (I have
not thought through the step to get there though.)  Ultimately I'd like
access to any type of IR controller device within the bridge drivers to
be through a v4l2_subdev.  Then the bridge drivers can present a more
uniform interface to IR controllers to other modules via a v4l2_device
interface.  Big plans, little time...


> > +}
> > +#else
> > +int ivtv_i2c_new_ir_legacy(struct ivtv *itv)
> > +{
> > +	/* ir-kbd-i2c does the legacy I2C IR controller probe for old kernels*/
> 
> Missing space before end of comment.

80 character limit.  Still readable.  But it's moot since I move the
comment in a later change where I have more characters on a line, so I
can fix it.  (I wonder if I did fix it...?)


> > +	return -1;
> > +}
> > +#endif
> > +
> >  int ivtv_i2c_register(struct ivtv *itv, unsigned idx)
> >  {
> >  	struct v4l2_subdev *sd;


> Patch #2:
> 
> > # HG changeset patch
> > # User Andy Walls <awalls@radix.net>
> > # Date 1254583380 14400
> > # Node ID 0127ed2ea55b215cc17660a698a19ac990117a46
> > # Parent  3d243437f04695220d92a0e906f287968e56d328
> > ivtv: Add explicit IR controller initialization for the AVerTV M116
> > 
> > From: Andy Walls <awalls@radix.net>
> > 
> > Priority: normal
> > 
> > Signed-off-by: Andy Walls <awalls@radix.net>
> > 
> > --- a/linux/drivers/media/video/ivtv/ivtv-cards.c	Sat Oct 03 10:28:55 2009 -0400
> > +++ b/linux/drivers/media/video/ivtv/ivtv-cards.c	Sat Oct 03 11:23:00 2009 -0400
> > @@ -955,7 +955,8 @@
> >  	.hw_video = IVTV_HW_CX25840,
> >  	.hw_audio = IVTV_HW_CX25840,
> >  	.hw_audio_ctrl = IVTV_HW_CX25840,
> > -	.hw_all = IVTV_HW_CX25840 | IVTV_HW_TUNER | IVTV_HW_WM8739,
> > +	.hw_all = IVTV_HW_CX25840 | IVTV_HW_TUNER | IVTV_HW_WM8739 |
> > +		  IVTV_HW_EM78P153S_IR_RX_AVER,
> >  	.video_inputs = {
> >  		{ IVTV_CARD_INPUT_VID_TUNER,  0, CX25840_COMPOSITE2 },
> >  		{ IVTV_CARD_INPUT_SVIDEO1,    1, CX25840_SVIDEO3    },
> > --- a/linux/drivers/media/video/ivtv/ivtv-cards.h	Sat Oct 03 10:28:55 2009 -0400
> > +++ b/linux/drivers/media/video/ivtv/ivtv-cards.h	Sat Oct 03 11:23:00 2009 -0400
> > @@ -87,23 +87,24 @@
> >  #define IVTV_PCI_ID_GOTVIEW1		0xffac
> >  #define IVTV_PCI_ID_GOTVIEW2 		0xffad
> >  
> > -/* hardware flags, no gaps allowed, IVTV_HW_GPIO must always be last */
> > -#define IVTV_HW_CX25840   (1 << 0)
> > -#define IVTV_HW_SAA7115   (1 << 1)
> > -#define IVTV_HW_SAA7127   (1 << 2)
> > -#define IVTV_HW_MSP34XX   (1 << 3)
> > -#define IVTV_HW_TUNER     (1 << 4)
> > -#define IVTV_HW_WM8775    (1 << 5)
> > -#define IVTV_HW_CS53L32A  (1 << 6)
> > -#define IVTV_HW_TVEEPROM  (1 << 7)
> > -#define IVTV_HW_SAA7114   (1 << 8)
> > -#define IVTV_HW_UPD64031A (1 << 9)
> > -#define IVTV_HW_UPD6408X  (1 << 10)
> > -#define IVTV_HW_SAA717X   (1 << 11)
> > -#define IVTV_HW_WM8739    (1 << 12)
> > -#define IVTV_HW_VP27SMPX  (1 << 13)
> > -#define IVTV_HW_M52790    (1 << 14)
> > -#define IVTV_HW_GPIO      (1 << 15)
> > +/* hardware flags, no gaps allowed */
> > +#define IVTV_HW_CX25840			(1 << 0)
> > +#define IVTV_HW_SAA7115			(1 << 1)
> > +#define IVTV_HW_SAA7127			(1 << 2)
> > +#define IVTV_HW_MSP34XX			(1 << 3)
> > +#define IVTV_HW_TUNER			(1 << 4)
> > +#define IVTV_HW_WM8775			(1 << 5)
> > +#define IVTV_HW_CS53L32A		(1 << 6)
> > +#define IVTV_HW_TVEEPROM		(1 << 7)
> > +#define IVTV_HW_SAA7114			(1 << 8)
> > +#define IVTV_HW_UPD64031A		(1 << 9)
> > +#define IVTV_HW_UPD6408X		(1 << 10)
> > +#define IVTV_HW_SAA717X			(1 << 11)
> > +#define IVTV_HW_WM8739			(1 << 12)
> > +#define IVTV_HW_VP27SMPX		(1 << 13)
> > +#define IVTV_HW_M52790			(1 << 14)
> > +#define IVTV_HW_GPIO			(1 << 15)
> > +#define IVTV_HW_EM78P153S_IR_RX_AVER	(1 << 16)
> >  
> >  #define IVTV_HW_SAA711X   (IVTV_HW_SAA7115 | IVTV_HW_SAA7114)
> >  
> > --- a/linux/drivers/media/video/ivtv/ivtv-i2c.c	Sat Oct 03 10:28:55 2009 -0400
> > +++ b/linux/drivers/media/video/ivtv/ivtv-i2c.c	Sat Oct 03 11:23:00 2009 -0400
> > @@ -63,6 +63,9 @@
> >  #include "ivtv-cards.h"
> >  #include "ivtv-gpio.h"
> >  #include "ivtv-i2c.h"
> > +#if LINUX_VERSION_CODE >= KERNEL_VERSION(2, 6, 30)
> > +#include <media/ir-kbd-i2c.h>
> > +#endif
> >  
> >  /* i2c implementation for cx23415/6 chip, ivtv project.
> >   * Author: Kevin Thayer (nufan_wfk at yahoo.com)
> > @@ -88,6 +91,7 @@
> >  #define IVTV_UPD64083_I2C_ADDR 		0x5c
> >  #define IVTV_VP27SMPX_I2C_ADDR      	0x5b
> >  #define IVTV_M52790_I2C_ADDR      	0x48
> > +#define IVTV_EM78P153S_IR_RX_I2C_ADDR	0x40
> >  
> >  /* This array should match the IVTV_HW_ defines */
> >  static const u8 hw_addrs[] = {
> > @@ -106,7 +110,8 @@
> >  	IVTV_WM8739_I2C_ADDR,
> >  	IVTV_VP27SMPX_I2C_ADDR,
> >  	IVTV_M52790_I2C_ADDR,
> > -	0 		/* IVTV_HW_GPIO dummy driver ID */
> > +	0,				/* IVTV_HW_GPIO dummy driver ID */
> > +	IVTV_EM78P153S_IR_RX_I2C_ADDR	/* IVTV_HW_EM78P153S_IR_RX_AVER */
> >  };
> 
> I suspect Hans put the GPIO at the end on purpose, I'm not sure why you
> want to change this? It makes your patch slightly larger.

I hate relying on implcit ordering and implicit positional markers.  I
checked the reasons why GPIO had to be last, and there was only one
reason left in the ivtv code: a redundant sanity check on the compiled
in arrays when starting up the ivtv modules.

Previously, I believe there were other reasons why it had to be last,
but Hans' changes over time have gradually phased out those requirements
if I am remembering things correctly.



> >  
> >  /* This array should match the IVTV_HW_ defines */
> > @@ -126,7 +131,8 @@
> >  	"wm8739",
> >  	"vp27smpx",
> >  	"m52790",
> > -	NULL
> > +	NULL,
> > +	NULL		/* IVTV_HW_EM78P153S_IR_RX_AVER */
> >  };
> >  
> >  /* This array should match the IVTV_HW_ defines */
> > @@ -151,9 +157,38 @@
> >  	"vp27smpx",
> >  	"m52790",
> >  	"gpio",
> > +	"ir_rx_em78p153s_aver",
> 
> This exceeds the maximum length for I2C client names (19 chars max.) So
> your patch won't work. I could make the name field slightly larger (say
> 23 chars) if really needed, but I'd rather have you simply use shorter
> names.

I'll use shorter names.  I was trying to be maintain some uniqueness.
The bridge driver has the knowledge of the exact chip and nothing else
does unless the bridge exposes it somehow.  It seemed like a good way to
expose the knowledge.


> >  };
> >  
> >  #if LINUX_VERSION_CODE >= KERNEL_VERSION(2, 6, 30)
> > +static const struct IR_i2c_init_data em78p153s_aver_ir_init_data = {
> > +	.ir_codes = &ir_codes_avermedia_cardbus_table,
> > +	.internal_get_key_func = IR_KBD_GET_KEY_AVERMEDIA_CARDBUS,
> > +	.type = IR_TYPE_OTHER,
> > +	.name = "ivtv-CX23416 EM78P153S AVerMedia",
> > +};
> > +
> > +static int ivtv_i2c_new_ir(struct i2c_adapter *adap, u32 hw, const char *type,
> > +			   u8 addr)
> > +{
> > +	struct i2c_board_info info;
> > +	unsigned short addr_list[2] = { addr, I2C_CLIENT_END };
> > +
> > +	memset(&info, 0, sizeof(struct i2c_board_info));
> > +	strlcpy(info.type, type, I2C_NAME_SIZE);
> > +
> > +	/* Our default information for ir-kbd-i2c.c to use */
> > +	switch (hw) {
> > +	case IVTV_HW_EM78P153S_IR_RX_AVER:
> > +		info.platform_data = (void *) &em78p153s_aver_ir_init_data;
> 
> Useless cast. You never need to cast to void *.

The compiler gripes because the "const" gets discarded; Mauro asked me
to fix it in cx18 previously.  I could have cast it to the proper type,
but then it wouldn't have fit in 80 columns.

(void *) wasn't "useless", it kept gcc, checkpatch, Mauro and me happy.
Now I guess I'll have to break the assignment to be over two lines. :(



> > +		break;
> > +	default:
> > +		break;
> 
> Useless statement.

True.


> > +	}
> > +
> > +	return i2c_new_probed_device(adap, &info, addr_list) == NULL ? -1 : 0;
> 
> I don't really get why you use i2c_new_probed_device() instead of the
> cheaper i2c_new_device(). Are you not 100% certain that the IR device
> is present on this model?

I cut and paste from cx18 for the HVR-1600.  There the IR controller may
be present or not depending on HVR-1600 vs HVR-1600MCE so probing made
sense.

I have no idea about the AverTV M116.

I know the PVR-150 vs. PVR-150MCE supported by ivtv has the same
situation as the HVR-1600 under cx18.  Eventually I'll use this function
for the PVR-150 models as well.



> > +}
> > +
> >  /* Instantiate the IR receiver device using probing -- undesirable */
> >  int ivtv_i2c_new_ir_legacy(struct ivtv *itv)
> >  {
> > @@ -185,9 +220,15 @@
> >  								       ? -1 : 0;
> >  }
> >  #else
> > +/* ir-kbd-i2c does the legacy I2C IR controller probe for old kernels*/
> 
> Missing space at end of comment.

I'll fix it.

> > +static int ivtv_i2c_new_ir(struct i2c_adapter *adap, u32 hw, const char *type,
> > +			   u8 addr)
> > +{
> > +	return -1;
> > +}
> > +
> >  int ivtv_i2c_new_ir_legacy(struct ivtv *itv)
> >  {
> > -	/* ir-kbd-i2c does the legacy I2C IR controller probe for old kernels*/
> >  	return -1;
> >  }
> >  #endif
> > @@ -221,8 +262,15 @@
> >  			sd->grp_id = 1 << idx;
> >  		return sd ? 0 : -1;
> >  	}
> > +
> > +	if (hw & IVTV_HW_EM78P153S_IR_RX_AVER)
> 
> Maybe use IVTV_HW_IR_ANY as I defined earlier? Otherwise you'll have to
> modify the code with each new remote control you add.

Good idea.



> > +		return ivtv_i2c_new_ir(adap, hw, type, hw_addrs[idx]);
> > +
> > +	/* Is it not an I2C device or one we do not wish to register? */
> >  	if (!hw_addrs[idx])
> >  		return -1;
> > +
> > +	/* It's an I2C device other than an analog tuner or IR chip */
> >  	if (hw == IVTV_HW_UPD64031A || hw == IVTV_HW_UPD6408X) {
> >  		sd = v4l2_i2c_new_subdev(&itv->v4l2_dev,
> >  				adap, mod, type, 0, I2C_ADDRS(hw_addrs[idx]));
> 
> Patch #3.
> 
> > --- a/linux/drivers/media/video/ir-kbd-i2c.c	Sat Oct 03 11:23:00 2009 -0400
> > +++ b/linux/drivers/media/video/ir-kbd-i2c.c	Sat Oct 03 11:27:19 2009 -0400
> > @@ -730,6 +730,7 @@
> >  	{ "ir_video", 0 },
> >  	/* IR device specific entries should be added here */
> >  	{ "ir_rx_z8f0811_haup", 0 },
> > +	{ "ir_rx_em78p153s_aver", 0 },
> >  	{ }
> >  };
> >  
> 
> I think we need to discuss this. I don't really see the point of adding
> new entries if the ir-kbd-i2c driver doesn't do anything about it. This
> makes device probing slower with no benefit. As long as you pass device
> information with all the details, the ir-kbd-i2c driver won't care
> about the device name.

I though a matching name was required for ir-kbd-i2c to bind to the IR
controller deivce.  I personally don't like the "ir_video" name as it is
a bit too generic, but then again I don't know whwre that name is
visible outside the kernel.  My plan was to have rather specific names,
so LIRC in the future could know automatically how to handle some of
these devices without the user trying to guess what an "ir_video" device
was as that name supplied no information to LIRC or the user.


> So the question is, where are we going with the ir-kbd-i2c driver? Are
> we happy with the current model where bridge drivers pass IR device
> information?

I think I would prever the brdige driver pass a v4l2_device object and
then provide an internal API for ir-kbd-i2c to query.  (See below).

Maybe even better would for ir-kbd-i2c to iterate over all v4l_device
objects in the system calling an interal IR API asking for information
about any IR device that is a child of that v4l_device.


>  Or do we want to move to a model where they just pass a
> device name and ir-kbd-i2c maps names to device information? In the
> latter case, it makes sense to have many i2c_device_id entries in
> ir-kbd-i2c, but in the former case it doesn't.
> 
> I guess the answer depends in part on how common IR devices and remote
> controls are across adapters. If the same IR device is used on many
> adapters then it makes some sense to move the definitions into
> ir-kbd-i2c.

That would be the case for the Z8F0811 loaded with firmware from
Zilog/Hauppauge.  I'm not sure of any other device with a lot of
commonality.  I would imagine that commonality would never cross vendor
lines.

>  But if devices are heavily adapter-dependent, and moving
> the definitions into ir-kbd-i2c doesn't allow for any code refactoring,
> then I don't quite see the point.

My decisions are based on thinking on a larger scope than just
ir-kbd-i2c:

1. More than just I2C connected IR devices, but also GPIO IR and IR
hardware like what is built into the CX2388x, CX2584x, and CX23418
chips.

2. More than just receivers, but also blasters.

3. More than just ir-kbd-i2c as an input solution; i.e. LIRC.  My
personal opinion is that ir-kbd-i2c and other v4l-dvb IR input handling
is a basic solutuion.  It "teaches the bear to dance", but I'd like to
"teach the bear to dance well".

4. More flexible selection and support of remotes.  I want to be able to
use any RC-5 remote with an adapter if necessary, not just the one that
came with the adapter.   Some of the I2C microcontrollers may not allow
this (maybe some do) due to the "address" in the RC-5 code.  GPIO and
CX2388x etc. IR controllers certainly can handle the possibility of
alternate remotes and alternate protocols like RC-6 Mode 6A used in MCE
remotes.  

5. I'd like to create a uniform interface for LIRC modules or any other
kernel modules to access bridge driver provided IR via an internal API
implemented in v4l2_device somehow.  (Just an idea with no real details
yet.)



So after mentally working my way through all that, I guess I would like
the details to move out of bridge drivers somewhat and into the modules
that handle IR input and output: ir-kbd-i2c and LIRC modules.  In that
case, the verbose names should give enough information for ir-kbd-i2c
and LIRC to do something sensible.  I think it is appropriate for the
bridge driver to provide hints about the default remote for a card.  I
would prefer ir-kbd-i2c and/or LIRC call an API accessable with a bridge
driver's v4l2_device to query for hints for the default remote or
additional information.

Right now the bridge drivers are essentially fixing the remote and
protocol used for a card.

Those are just my thoughts and ramblings.

Thanks very much for the review and comments.


Regards,
Andy

Re: [REVIEW] ivtv, ir-kbd-i2c: Explicit IR support for the AVerTV M116 for newer kernels

[Jean Delvare]

Hi Andy,

On Sun, 04 Oct 2009 16:11:32 -0400, Andy Walls wrote:
> On Sun, 2009-10-04 at 10:54 +0200, Jean Delvare wrote:
> > On Sat, 03 Oct 2009 11:44:20 -0400, Andy Walls wrote:
> > >  /* This array should match the IVTV_HW_ defines */
> > > @@ -126,7 +131,8 @@
> > >  	"wm8739",
> > >  	"vp27smpx",
> > >  	"m52790",
> > > -	NULL
> > > +	NULL,
> > > +	NULL		/* IVTV_HW_EM78P153S_IR_RX_AVER */
> > >  };
> > >  
> > >  /* This array should match the IVTV_HW_ defines */
> > > @@ -151,9 +157,38 @@
> > >  	"vp27smpx",
> > >  	"m52790",
> > >  	"gpio",
> > > +	"ir_rx_em78p153s_aver",
> > 
> > This exceeds the maximum length for I2C client names (19 chars max.) So
> > your patch won't work. I could make the name field slightly larger (say
> > 23 chars) if really needed, but I'd rather have you simply use shorter
> > names.
> 
> I'll use shorter names.  I was trying to be maintain some uniqueness.
> The bridge driver has the knowledge of the exact chip and nothing else
> does unless the bridge exposes it somehow.  It seemed like a good way to
> expose the knowledge.

The knowledge is already exposed through the platform data attached to
the instantiated i2c device (.ir_codes, .internal_get_key_func, .type
and .name). The i2c client name isn't used by the ir-kbd-i2c driver to
do anything useful.

> > > +static int ivtv_i2c_new_ir(struct i2c_adapter *adap, u32 hw, const char *type,
> > > +			   u8 addr)
> > > +{
> > > +	struct i2c_board_info info;
> > > +	unsigned short addr_list[2] = { addr, I2C_CLIENT_END };
> > > +
> > > +	memset(&info, 0, sizeof(struct i2c_board_info));
> > > +	strlcpy(info.type, type, I2C_NAME_SIZE);
> > > +
> > > +	/* Our default information for ir-kbd-i2c.c to use */
> > > +	switch (hw) {
> > > +	case IVTV_HW_EM78P153S_IR_RX_AVER:
> > > +		info.platform_data = (void *) &em78p153s_aver_ir_init_data;
> > 
> > Useless cast. You never need to cast to void *.
> 
> The compiler gripes because the "const" gets discarded; Mauro asked me
> to fix it in cx18 previously.  I could have cast it to the proper type,
> but then it wouldn't have fit in 80 columns.
> 
> (void *) wasn't "useless", it kept gcc, checkpatch, Mauro and me happy.
> Now I guess I'll have to break the assignment to be over two lines. :(

Ah, good point, I had missed it. Well basically this means that you're
not supposed to pass const data structures as platform data. So maybe
you'd rather follow the approach used in the saa7134 and em28xx drivers.

> > > --- a/linux/drivers/media/video/ir-kbd-i2c.c	Sat Oct 03 11:23:00 2009 -0400
> > > +++ b/linux/drivers/media/video/ir-kbd-i2c.c	Sat Oct 03 11:27:19 2009 -0400
> > > @@ -730,6 +730,7 @@
> > >  	{ "ir_video", 0 },
> > >  	/* IR device specific entries should be added here */
> > >  	{ "ir_rx_z8f0811_haup", 0 },
> > > +	{ "ir_rx_em78p153s_aver", 0 },
> > >  	{ }
> > >  };
> > >  
> > 
> > I think we need to discuss this. I don't really see the point of adding
> > new entries if the ir-kbd-i2c driver doesn't do anything about it. This
> > makes device probing slower with no benefit. As long as you pass device
> > information with all the details, the ir-kbd-i2c driver won't care
> > about the device name.
> 
> I though a matching name was required for ir-kbd-i2c to bind to the IR
> controller deivce.  I personally don't like the "ir_video" name as it is
> a bit too generic, but then again I don't know whwre that name is
> visible outside the kernel.  My plan was to have rather specific names,
> so LIRC in the future could know automatically how to handle some of
> these devices without the user trying to guess what an "ir_video" device
> was as that name supplied no information to LIRC or the user.

The name is visible in sysfs as the client's name attribute. But no
user-space application should rely on this. If a user-space application
should use a name string, that should be the _input_ name and not the
i2c client name. For the simple reason that IR devices don't have to be
I2C devices.

The i2c device name is merely used for device-driver matching. For this
purpose, "ir_video" works just fine. As I said before, there is a point
in defining other names if it allows the ir-kbd-i2c driver to make
decisions by itself, or if you envision to move support for a specific
device to a separate driver as some point. If not then you're making
things more complex with zero benefit.

I'd like to add that, IMHO, LIRC shouldn't have to care about this at
all. The name should be purely informative. I have experimented in the
past with user-space trying to do device-specific handling based on a
name string. This is what we did in libsensors 2. This ended up being a
totally unmaintainable mess, where each new kernel had to be followed
by updated user-space. This was a pain and you really shouldn't go in
this direction. For libsensors 3, we've defined a clean sysfs
interface, which describes the functionality of each supported device,
so the library doesn't do any name-driver processing. Very easy to
maintain.

So if you want a piece of advice: either handle all device-specific
things in the kernel, or in user-space, but don't do half in the kernel
and half in user-space, because you will have a horrible interface
in-between.

> > So the question is, where are we going with the ir-kbd-i2c driver? Are
> > we happy with the current model where bridge drivers pass IR device
> > information?
> 
> I think I would prever the brdige driver pass a v4l2_device object and
> then provide an internal API for ir-kbd-i2c to query.  (See below).
> 
> Maybe even better would for ir-kbd-i2c to iterate over all v4l_device
> objects in the system calling an interal IR API asking for information
> about any IR device that is a child of that v4l_device.

I am not too familiar with the v4l2 internals so I can't really comment
on this.

> >  Or do we want to move to a model where they just pass a
> > device name and ir-kbd-i2c maps names to device information? In the
> > latter case, it makes sense to have many i2c_device_id entries in
> > ir-kbd-i2c, but in the former case it doesn't.
> > 
> > I guess the answer depends in part on how common IR devices and remote
> > controls are across adapters. If the same IR device is used on many
> > adapters then it makes some sense to move the definitions into
> > ir-kbd-i2c.
> 
> That would be the case for the Z8F0811 loaded with firmware from
> Zilog/Hauppauge.  I'm not sure of any other device with a lot of
> commonality.  I would imagine that commonality would never cross vendor
> lines.
> 
> >  But if devices are heavily adapter-dependent, and moving
> > the definitions into ir-kbd-i2c doesn't allow for any code refactoring,
> > then I don't quite see the point.
> 
> My decisions are based on thinking on a larger scope than just
> ir-kbd-i2c:
> 
> 1. More than just I2C connected IR devices, but also GPIO IR and IR
> hardware like what is built into the CX2388x, CX2584x, and CX23418
> chips.
> 
> 2. More than just receivers, but also blasters.
> 
> 3. More than just ir-kbd-i2c as an input solution; i.e. LIRC.  My
> personal opinion is that ir-kbd-i2c and other v4l-dvb IR input handling
> is a basic solutuion.  It "teaches the bear to dance", but I'd like to
> "teach the bear to dance well".
> 
> 4. More flexible selection and support of remotes.  I want to be able to
> use any RC-5 remote with an adapter if necessary, not just the one that
> came with the adapter.   Some of the I2C microcontrollers may not allow
> this (maybe some do) due to the "address" in the RC-5 code.  GPIO and
> CX2388x etc. IR controllers certainly can handle the possibility of
> alternate remotes and alternate protocols like RC-6 Mode 6A used in MCE
> remotes.  
> 
> 5. I'd like to create a uniform interface for LIRC modules or any other
> kernel modules to access bridge driver provided IR via an internal API
> implemented in v4l2_device somehow.  (Just an idea with no real details
> yet.)
> 
> 
> 
> So after mentally working my way through all that, I guess I would like
> the details to move out of bridge drivers somewhat and into the modules
> that handle IR input and output: ir-kbd-i2c and LIRC modules.  In that
> case, the verbose names should give enough information for ir-kbd-i2c
> and LIRC to do something sensible.  I think it is appropriate for the
> bridge driver to provide hints about the default remote for a card.  I
> would prefer ir-kbd-i2c and/or LIRC call an API accessable with a bridge
> driver's v4l2_device to query for hints for the default remote or
> additional information.

This all goes well beyond the point I was initially attempting to
discuss. But you certainly have a much better vision of this area than
I do.

-- 
Jean Delvare

Re: [REVIEW] ivtv, ir-kbd-i2c: Explicit IR support for the AVerTV M116 for newer kernels

[Aleksandr V. Piskunov]

On Sat, Oct 03, 2009 at 11:44:20AM -0400, Andy Walls wrote:
> Aleksandr and Jean,
> 
> Zdrastvoitye & Bonjour,
> 
> To support the AVerMedia M166's IR microcontroller in ivtv and
> ir-kbd-i2c with the new i2c binding model, I have added 3 changesets in
> 
> 	http://linuxtv.org/hg/~awalls/ivtv
> 

Now the last step to the decent support of M116 remote.

I spent hours banging my head against the wall, controller just doesn't
give a stable keypresses, skips a lot of them. Increasing polling interval
from default 100 ms to 400-500 ms helps a bit, but it only masks the
problem. Decreasing polling interval below 50ms makes it skip virtually
90% of keypresses.

Basicly during the I2C operation that reads scancode, controller seems
to stop processing input from IR sensor, resulting a loss of keypress.

So the solution(?) I found was to decrease the udelay in
ivtv_i2c_algo_template from 10 to 5. Guess it just doubles the frequency
of ivtv i2c bus or something like that. Problem went away, IR controller
is now working as expected.

So question is:
1) Is it ok to decrease udelay for this board?
2) If yes, how to do it right?

Re: [REVIEW] ivtv, ir-kbd-i2c: Explicit IR support for the AVerTV M116 for newer kernels

[Andy Walls]

On Mon, 2009-10-05 at 01:23 +0300, Aleksandr V. Piskunov wrote:
> On Sat, Oct 03, 2009 at 11:44:20AM -0400, Andy Walls wrote:
> > Aleksandr and Jean,
> > 
> > Zdrastvoitye & Bonjour,
> > 
> > To support the AVerMedia M166's IR microcontroller in ivtv and
> > ir-kbd-i2c with the new i2c binding model, I have added 3 changesets in
> > 
> > 	http://linuxtv.org/hg/~awalls/ivtv
> > 
> 
> Now the last step to the decent support of M116 remote.
> 
> I spent hours banging my head against the wall, controller just doesn't
> give a stable keypresses, skips a lot of them. Increasing polling interval
> from default 100 ms to 400-500 ms helps a bit, but it only masks the
> problem. Decreasing polling interval below 50ms makes it skip virtually
> 90% of keypresses.
> 
> Basicly during the I2C operation that reads scancode, controller seems
> to stop processing input from IR sensor, resulting a loss of keypress.
> 
> So the solution(?) I found was to decrease the udelay in
> ivtv_i2c_algo_template from 10 to 5. Guess it just doubles the frequency
> of ivtv i2c bus or something like that. Problem went away, IR controller
> is now working as expected.

That's a long standing error in the ivtv driver.  It ran the I2C bus at
1/(2*10 usec) = 50 kHz instead of the standard 100 kHz.

Technically any I2C device should be able to handle clock rates down to
about DC IIRC; so there must be a bug in the IR microcontroller
implementation.

Also the CX23416 errantly marks its PCI register space as cacheable
which is probably wrong (see lspci output).  This may also be
interfering with proper I2C operation with i2c_algo_bit depedning on the
PCI bridges in your system.

> 
> So question is:
> 1) Is it ok to decrease udelay for this board?

Sure, I think.  It would actually run the ivtv I2C bus at the nominal
clock rate specified by the I2C specification.

I never had any reason to change it, as I feared causing regressions in
many well tested boards.


> 2) If yes, how to do it right?

Try:

# modprobe ivtv newi2c=1

to see if that works first. 


Regards,
Andy

Re: [REVIEW] ivtv, ir-kbd-i2c: Explicit IR support for the AVerTV M116 for newer kernels

[Jean Delvare]

On Sun, 04 Oct 2009 21:54:37 -0400, Andy Walls wrote:
> On Mon, 2009-10-05 at 01:23 +0300, Aleksandr V. Piskunov wrote:
> > So the solution(?) I found was to decrease the udelay in
> > ivtv_i2c_algo_template from 10 to 5. Guess it just doubles the frequency
> > of ivtv i2c bus or something like that. Problem went away, IR controller
> > is now working as expected.
> 
> That's a long standing error in the ivtv driver.  It ran the I2C bus at
> 1/(2*10 usec) = 50 kHz instead of the standard 100 kHz.
> 
> Technically any I2C device should be able to handle clock rates down to
> about DC IIRC; so there must be a bug in the IR microcontroller
> implementation.
> 
> Also the CX23416 errantly marks its PCI register space as cacheable
> which is probably wrong (see lspci output).  This may also be
> interfering with proper I2C operation with i2c_algo_bit depedning on the
> PCI bridges in your system.
> 
> > 
> > So question is:
> > 1) Is it ok to decrease udelay for this board?
> 
> Sure, I think.  It would actually run the ivtv I2C bus at the nominal
> clock rate specified by the I2C specification.

FWIW, 100 kHz isn't the "nominal" I2C clock rate, but the maximum clock
rate for normal I2C. It is perfectly valid to run I2C buses as lower
clock frequencies. I don't even think there is a minimum for I2C (but
there is a minimum of 10 kHz for SMBus.)

But of course different hardware implementations may not fully cover
the standard I2C or SMBus frequency range, and it is possible that a TV
adapter manufacturer designed its hardware to run the I2C bus at a
fixed frequency and we have to use that frequency to make the adapter
happy.

> I never had any reason to change it, as I feared causing regressions in
> many well tested boards.

This is a possibility, indeed. But for obvious performance reasons, I'd
rather use 100 kHz as the default, and let boards override it with a
lower frequency of their choice if needed. Obviously this provides an
easy improvement path, where each board can be tested separately and
I2C bus frequency bumped from 50 kHz to 100 kHz after some good testing.

Some boards might even support fast I2C, up to 400 kHz but limited to
250 kHz by the i2c-algo-bit implementation.

-- 
Jean Delvare

Re: [REVIEW] ivtv, ir-kbd-i2c: Explicit IR support for the AVerTV M116 for newer kernels

[Andy Walls]

On Mon, 2009-10-05 at 10:29 +0200, Jean Delvare wrote:
> On Sun, 04 Oct 2009 21:54:37 -0400, Andy Walls wrote:
> > On Mon, 2009-10-05 at 01:23 +0300, Aleksandr V. Piskunov wrote:

> > > 
> > > So question is:
> > > 1) Is it ok to decrease udelay for this board?
> > 
> > Sure, I think.  It would actually run the ivtv I2C bus at the nominal
> > clock rate specified by the I2C specification.
> 
> FWIW, 100 kHz isn't the "nominal" I2C clock rate, but the maximum clock
> rate for normal I2C. It is perfectly valid to run I2C buses as lower
> clock frequencies. I don't even think there is a minimum for I2C (but
> there is a minimum of 10 kHz for SMBus.)

Ah, thanks.  I was too lazy to go read my copy of the spec.


> But of course different hardware implementations may not fully cover
> the standard I2C or SMBus frequency range, and it is possible that a TV
> adapter manufacturer designed its hardware to run the I2C bus at a
> fixed frequency and we have to use that frequency to make the adapter
> happy.

This is very plausible for a microcontroller implementation of an I2C
slave, which is the case here.


> > I never had any reason to change it, as I feared causing regressions in
> > many well tested boards.
> 
> This is a possibility, indeed. But for obvious performance reasons, I'd
> rather use 100 kHz as the default, and let boards override it with a
> lower frequency of their choice if needed. Obviously this provides an
> easy improvement path, where each board can be tested separately and
> I2C bus frequency bumped from 50 kHz to 100 kHz after some good testing.
> 
> Some boards might even support fast I2C, up to 400 kHz but limited to
> 250 kHz by the i2c-algo-bit implementation.

I can add a module option to ivtv for I2C clock rate.  It may take a few
days.

Regards,
Andy

Re: [REVIEW] ivtv, ir-kbd-i2c: Explicit IR support for the AVerTV M116 for newer kernels

[Aleksandr V. Piskunov]

> > Basicly during the I2C operation that reads scancode, controller seems
> > to stop processing input from IR sensor, resulting a loss of keypress.
> > 
> > So the solution(?) I found was to decrease the udelay in
> > ivtv_i2c_algo_template from 10 to 5. Guess it just doubles the frequency
> > of ivtv i2c bus or something like that. Problem went away, IR controller
> > is now working as expected.
> 
> That's a long standing error in the ivtv driver.  It ran the I2C bus at
> 1/(2*10 usec) = 50 kHz instead of the standard 100 kHz.
> 
> Technically any I2C device should be able to handle clock rates down to
> about DC IIRC; so there must be a bug in the IR microcontroller
> implementation.
> 
> Also the CX23416 errantly marks its PCI register space as cacheable
> which is probably wrong (see lspci output).  This may also be
> interfering with proper I2C operation with i2c_algo_bit depedning on the
> PCI bridges in your system.
> 
> > 
> > So question is:
> > 1) Is it ok to decrease udelay for this board?
> 
> Sure, I think.  It would actually run the ivtv I2C bus at the nominal
> clock rate specified by the I2C specification.
> 
> I never had any reason to change it, as I feared causing regressions in
> many well tested boards.
> 
> 
> > 2) If yes, how to do it right?
> 
> Try:
> 
> # modprobe ivtv newi2c=1
> 
> to see if that works first. 
> 

udelay=10, newi2c=0  => BAD
udelay=10, newi2c=1  => BAD
udelay=5,  newi2c=0  => OK
udelay=5,  newi2c=1  => BAD


newi2c=1 also throws some log messages, not sure if its ok or not.

Oct  5 11:41:16 moon kernel: [45430.916449] ivtv: Start initialization, version 1.4.1
Oct  5 11:41:16 moon kernel: [45430.916618] ivtv0: Initializing card 0
Oct  5 11:41:16 moon kernel: [45430.916628] ivtv0: Autodetected AVerTV MCE 116 Plus card (cx23416 based)
Oct  5 11:41:16 moon kernel: [45430.918887] ivtv 0000:03:06.0: PCI INT A -> GSI 20 (level, low) -> IRQ 20
Oct  5 11:41:16 moon kernel: [45430.919229] ivtv0:  i2c: i2c init
Oct  5 11:41:16 moon kernel: [45430.919234] ivtv0:  i2c: setting scl and sda to 1
Oct  5 11:41:16 moon kernel: [45430.937745] cx25840 0-0044: cx25843-23 found @ 0x88 (ivtv i2c driver #0)
Oct  5 11:41:16 moon kernel: [45430.949145] ivtv0:  i2c: Slave did not ack
Oct  5 11:41:16 moon kernel: [45430.951628] ivtv0:  i2c: Slave did not ack
Oct  5 11:41:16 moon kernel: [45430.954191] ivtv0:  i2c: Slave did not ack
Oct  5 11:41:16 moon kernel: [45430.956724] ivtv0:  i2c: Slave did not ack
Oct  5 11:41:16 moon kernel: [45430.959211] ivtv0:  i2c: Slave did not ack
Oct  5 11:41:16 moon kernel: [45430.961749] ivtv0:  i2c: Slave did not ack
Oct  5 11:41:16 moon kernel: [45430.964236] ivtv0:  i2c: Slave did not ack
Oct  5 11:41:16 moon kernel: [45430.966722] ivtv0:  i2c: Slave did not ack
Oct  5 11:41:16 moon kernel: [45430.966786] ivtv0:  i2c: i2c write to 43 failed
Oct  5 11:41:16 moon kernel: [45430.971106] tuner 0-0061: chip found @ 0xc2 (ivtv i2c driver #0)
Oct  5 11:41:16 moon kernel: [45430.974404] wm8739 0-001a: chip found @ 0x34 (ivtv i2c driver #0)
Oct  5 11:41:16 moon kernel: [45430.986328] ivtv0:  i2c: Slave did not ack
Oct  5 11:41:16 moon kernel: [45430.988871] ivtv0:  i2c: Slave did not ack
Oct  5 11:41:16 moon kernel: [45430.991355] ivtv0:  i2c: Slave did not ack
Oct  5 11:41:16 moon kernel: [45430.993904] ivtv0:  i2c: Slave did not ack
Oct  5 11:41:16 moon kernel: [45430.996427] ivtv0:  i2c: Slave did not ack
Oct  5 11:41:16 moon kernel: [45430.998938] ivtv0:  i2c: Slave did not ack
Oct  5 11:41:16 moon kernel: [45431.001477] ivtv0:  i2c: Slave did not ack
Oct  5 11:41:16 moon kernel: [45431.003968] ivtv0:  i2c: Slave did not ack
Oct  5 11:41:16 moon kernel: [45431.004053] ivtv0:  i2c: i2c write to 18 failed
Oct  5 11:41:16 moon kernel: [45431.011333] ivtv0:  i2c: Slave did not ack
Oct  5 11:41:16 moon kernel: [45431.013883] ivtv0:  i2c: Slave did not ack
Oct  5 11:41:16 moon kernel: [45431.016418] ivtv0:  i2c: Slave did not ack
Oct  5 11:41:16 moon kernel: [45431.018911] ivtv0:  i2c: Slave did not ack
Oct  5 11:41:16 moon kernel: [45431.021463] ivtv0:  i2c: Slave did not ack
Oct  5 11:41:16 moon kernel: [45431.023937] ivtv0:  i2c: Slave did not ack
Oct  5 11:41:16 moon kernel: [45431.026478] ivtv0:  i2c: Slave did not ack
Oct  5 11:41:16 moon kernel: [45431.028998] ivtv0:  i2c: Slave did not ack
Oct  5 11:41:16 moon kernel: [45431.029063] ivtv0:  i2c: i2c write to 71 failed
Oct  5 11:41:16 moon kernel: [45431.031468] ivtv0:  i2c: Slave did not ack
....

Re: [REVIEW] ivtv, ir-kbd-i2c: Explicit IR support for the AVerTV M116 for newer kernels

[Jean Delvare]

On Mon, 5 Oct 2009 11:50:31 +0300, Aleksandr V. Piskunov wrote:
> > Try:
> > 
> > # modprobe ivtv newi2c=1
> > 
> > to see if that works first. 
> > 
> 
> udelay=10, newi2c=0  => BAD
> udelay=10, newi2c=1  => BAD
> udelay=5,  newi2c=0  => OK
> udelay=5,  newi2c=1  => BAD

The udelay value is only used by i2c-algo-bit, not newi2c, so the last
test was not needed.

> newi2c=1 also throws some log messages, not sure if its ok or not.
> 
> Oct  5 11:41:16 moon kernel: [45430.916449] ivtv: Start initialization, version 1.4.1
> Oct  5 11:41:16 moon kernel: [45430.916618] ivtv0: Initializing card 0
> Oct  5 11:41:16 moon kernel: [45430.916628] ivtv0: Autodetected AVerTV MCE 116 Plus card (cx23416 based)
> Oct  5 11:41:16 moon kernel: [45430.918887] ivtv 0000:03:06.0: PCI INT A -> GSI 20 (level, low) -> IRQ 20
> Oct  5 11:41:16 moon kernel: [45430.919229] ivtv0:  i2c: i2c init
> Oct  5 11:41:16 moon kernel: [45430.919234] ivtv0:  i2c: setting scl and sda to 1
> Oct  5 11:41:16 moon kernel: [45430.937745] cx25840 0-0044: cx25843-23 found @ 0x88 (ivtv i2c driver #0)
> Oct  5 11:41:16 moon kernel: [45430.949145] ivtv0:  i2c: Slave did not ack
> Oct  5 11:41:16 moon kernel: [45430.951628] ivtv0:  i2c: Slave did not ack
> Oct  5 11:41:16 moon kernel: [45430.954191] ivtv0:  i2c: Slave did not ack
> Oct  5 11:41:16 moon kernel: [45430.956724] ivtv0:  i2c: Slave did not ack
> Oct  5 11:41:16 moon kernel: [45430.959211] ivtv0:  i2c: Slave did not ack
> Oct  5 11:41:16 moon kernel: [45430.961749] ivtv0:  i2c: Slave did not ack
> Oct  5 11:41:16 moon kernel: [45430.964236] ivtv0:  i2c: Slave did not ack
> Oct  5 11:41:16 moon kernel: [45430.966722] ivtv0:  i2c: Slave did not ack
> Oct  5 11:41:16 moon kernel: [45430.966786] ivtv0:  i2c: i2c write to 43 failed
> Oct  5 11:41:16 moon kernel: [45430.971106] tuner 0-0061: chip found @ 0xc2 (ivtv i2c driver #0)
> Oct  5 11:41:16 moon kernel: [45430.974404] wm8739 0-001a: chip found @ 0x34 (ivtv i2c driver #0)
> Oct  5 11:41:16 moon kernel: [45430.986328] ivtv0:  i2c: Slave did not ack
> Oct  5 11:41:16 moon kernel: [45430.988871] ivtv0:  i2c: Slave did not ack
> Oct  5 11:41:16 moon kernel: [45430.991355] ivtv0:  i2c: Slave did not ack
> Oct  5 11:41:16 moon kernel: [45430.993904] ivtv0:  i2c: Slave did not ack
> Oct  5 11:41:16 moon kernel: [45430.996427] ivtv0:  i2c: Slave did not ack
> Oct  5 11:41:16 moon kernel: [45430.998938] ivtv0:  i2c: Slave did not ack
> Oct  5 11:41:16 moon kernel: [45431.001477] ivtv0:  i2c: Slave did not ack
> Oct  5 11:41:16 moon kernel: [45431.003968] ivtv0:  i2c: Slave did not ack
> Oct  5 11:41:16 moon kernel: [45431.004053] ivtv0:  i2c: i2c write to 18 failed
> Oct  5 11:41:16 moon kernel: [45431.011333] ivtv0:  i2c: Slave did not ack
> Oct  5 11:41:16 moon kernel: [45431.013883] ivtv0:  i2c: Slave did not ack
> Oct  5 11:41:16 moon kernel: [45431.016418] ivtv0:  i2c: Slave did not ack
> Oct  5 11:41:16 moon kernel: [45431.018911] ivtv0:  i2c: Slave did not ack
> Oct  5 11:41:16 moon kernel: [45431.021463] ivtv0:  i2c: Slave did not ack
> Oct  5 11:41:16 moon kernel: [45431.023937] ivtv0:  i2c: Slave did not ack
> Oct  5 11:41:16 moon kernel: [45431.026478] ivtv0:  i2c: Slave did not ack
> Oct  5 11:41:16 moon kernel: [45431.028998] ivtv0:  i2c: Slave did not ack
> Oct  5 11:41:16 moon kernel: [45431.029063] ivtv0:  i2c: i2c write to 71 failed
> Oct  5 11:41:16 moon kernel: [45431.031468] ivtv0:  i2c: Slave did not ack
> ....

That would be I2C probe attempts such as the ones done by ir-kbd-i2c.
Nothing to be afraid of.

-- 
Jean Delvare

Re: [REVIEW] ivtv, ir-kbd-i2c: Explicit IR support for the AVerTV M116 for newer kernels

[Aleksandr V. Piskunov]

On Mon, Oct 05, 2009 at 11:04:02AM +0200, Jean Delvare wrote:
> On Mon, 5 Oct 2009 11:50:31 +0300, Aleksandr V. Piskunov wrote:
> > > Try:
> > > 
> > > # modprobe ivtv newi2c=1
> > > 
> > > to see if that works first. 
> > > 
> > 
> > udelay=10, newi2c=0  => BAD
> > udelay=10, newi2c=1  => BAD
> > udelay=5,  newi2c=0  => OK
> > udelay=5,  newi2c=1  => BAD
> 
> The udelay value is only used by i2c-algo-bit, not newi2c, so the last
> test was not needed.
> 

Yup, also tried udelay=4, IR controller handles it without problems,
though cx25840 and xc2028 doesn't seem to like the 125 KHz frequency,
refusing to communicate. xc2028 even stopped responding, requiring a cold
reboot.

So for M116 board, the most stable combination seems to be 100 KHz i2c bus
and 150ms polling delay (up from 100 default). With this combination
I can quickly press 1234567890 on remote and driver gets the combination
without any losses.

Re: [REVIEW] ivtv, ir-kbd-i2c: Explicit IR support for the AVerTV M116 for newer kernels

[Devin Heitmueller]

On Mon, Oct 5, 2009 at 6:02 AM, Aleksandr V. Piskunov
<aleksandr.v.piskunov@gmail.com> wrote:
> Yup, also tried udelay=4, IR controller handles it without problems,
> though cx25840 and xc2028 doesn't seem to like the 125 KHz frequency,
> refusing to communicate. xc2028 even stopped responding, requiring a cold
> reboot.

The i2c maximum clock rate for xc3028 is 100 KHz.  Nobody should ever
be running it at anything higher.

Devin

-- 
Devin J. Heitmueller - Kernel Labs
http://www.kernellabs.com

Re: [REVIEW] ivtv, ir-kbd-i2c: Explicit IR support for the AVerTV M116 for newer kernels

[Oldrich Jedlicka]

On Monday 05 of October 2009 at 10:50:31, Aleksandr V. Piskunov wrote:
> > > Basicly during the I2C operation that reads scancode, controller seems
> > > to stop processing input from IR sensor, resulting a loss of keypress.
> > >
> > > So the solution(?) I found was to decrease the udelay in
> > > ivtv_i2c_algo_template from 10 to 5. Guess it just doubles the
> > > frequency of ivtv i2c bus or something like that. Problem went away, IR
> > > controller is now working as expected.
> >
> > That's a long standing error in the ivtv driver.  It ran the I2C bus at
> > 1/(2*10 usec) = 50 kHz instead of the standard 100 kHz.
> >
> > Technically any I2C device should be able to handle clock rates down to
> > about DC IIRC; so there must be a bug in the IR microcontroller
> > implementation.
> >
> > Also the CX23416 errantly marks its PCI register space as cacheable
> > which is probably wrong (see lspci output).  This may also be
> > interfering with proper I2C operation with i2c_algo_bit depedning on the
> > PCI bridges in your system.
> >
> > > So question is:
> > > 1) Is it ok to decrease udelay for this board?
> >
> > Sure, I think.  It would actually run the ivtv I2C bus at the nominal
> > clock rate specified by the I2C specification.
> >
> > I never had any reason to change it, as I feared causing regressions in
> > many well tested boards.
> >
> > > 2) If yes, how to do it right?
> >
> > Try:
> >
> > # modprobe ivtv newi2c=1
> >
> > to see if that works first.
>
> udelay=10, newi2c=0  => BAD
> udelay=10, newi2c=1  => BAD
> udelay=5,  newi2c=0  => OK
> udelay=5,  newi2c=1  => BAD
>
>
> newi2c=1 also throws some log messages, not sure if its ok or not.

Hi Aleksandr,

I had a look at the ivtv newi2c implementation and it doesn't have any good 
timing source. The only delay there is 5 times read of the clock line (SCL) 
and waiting for the clock/data line to go high or low.

The implementation assumes that the slave is generating sufficient clock line 
slow-down (by holding the clock line down) and that it is enough to have data 
line stable for very short time (5 times read of the clock line).

I'm not surprised that with newi2c=1 you see "Slave did not ack", because the 
clock just isn't correct. The generic i2c-algo-bit is doing much better job 
here.

Regards,
Oldrich.


> Oct  5 11:41:16 moon kernel: [45430.916449] ivtv: Start initialization,
> version 1.4.1 Oct  5 11:41:16 moon kernel: [45430.916618] ivtv0:
> Initializing card 0 Oct  5 11:41:16 moon kernel: [45430.916628] ivtv0:
> Autodetected AVerTV MCE 116 Plus card (cx23416 based) Oct  5 11:41:16 moon
> kernel: [45430.918887] ivtv 0000:03:06.0: PCI INT A -> GSI 20 (level, low)
> -> IRQ 20 Oct  5 11:41:16 moon kernel: [45430.919229] ivtv0:  i2c: i2c init
> Oct  5 11:41:16 moon kernel: [45430.919234] ivtv0:  i2c: setting scl and
> sda to 1 Oct  5 11:41:16 moon kernel: [45430.937745] cx25840 0-0044:
> cx25843-23 found @ 0x88 (ivtv i2c driver #0) Oct  5 11:41:16 moon kernel:
> [45430.949145] ivtv0:  i2c: Slave did not ack Oct  5 11:41:16 moon kernel:
> [45430.951628] ivtv0:  i2c: Slave did not ack Oct  5 11:41:16 moon kernel:
> [45430.954191] ivtv0:  i2c: Slave did not ack Oct  5 11:41:16 moon kernel:
> [45430.956724] ivtv0:  i2c: Slave did not ack Oct  5 11:41:16 moon kernel:
> [45430.959211] ivtv0:  i2c: Slave did not ack Oct  5 11:41:16 moon kernel:
> [45430.961749] ivtv0:  i2c: Slave did not ack Oct  5 11:41:16 moon kernel:
> [45430.964236] ivtv0:  i2c: Slave did not ack Oct  5 11:41:16 moon kernel:
> [45430.966722] ivtv0:  i2c: Slave did not ack Oct  5 11:41:16 moon kernel:
> [45430.966786] ivtv0:  i2c: i2c write to 43 failed Oct  5 11:41:16 moon
> kernel: [45430.971106] tuner 0-0061: chip found @ 0xc2 (ivtv i2c driver #0)
> Oct  5 11:41:16 moon kernel: [45430.974404] wm8739 0-001a: chip found @
> 0x34 (ivtv i2c driver #0) Oct  5 11:41:16 moon kernel: [45430.986328]
> ivtv0:  i2c: Slave did not ack Oct  5 11:41:16 moon kernel: [45430.988871]
> ivtv0:  i2c: Slave did not ack Oct  5 11:41:16 moon kernel: [45430.991355]
> ivtv0:  i2c: Slave did not ack Oct  5 11:41:16 moon kernel: [45430.993904]
> ivtv0:  i2c: Slave did not ack Oct  5 11:41:16 moon kernel: [45430.996427]
> ivtv0:  i2c: Slave did not ack Oct  5 11:41:16 moon kernel: [45430.998938]
> ivtv0:  i2c: Slave did not ack Oct  5 11:41:16 moon kernel: [45431.001477]
> ivtv0:  i2c: Slave did not ack Oct  5 11:41:16 moon kernel: [45431.003968]
> ivtv0:  i2c: Slave did not ack Oct  5 11:41:16 moon kernel: [45431.004053]
> ivtv0:  i2c: i2c write to 18 failed Oct  5 11:41:16 moon kernel:
> [45431.011333] ivtv0:  i2c: Slave did not ack Oct  5 11:41:16 moon kernel:
> [45431.013883] ivtv0:  i2c: Slave did not ack Oct  5 11:41:16 moon kernel:
> [45431.016418] ivtv0:  i2c: Slave did not ack Oct  5 11:41:16 moon kernel:
> [45431.018911] ivtv0:  i2c: Slave did not ack Oct  5 11:41:16 moon kernel:
> [45431.021463] ivtv0:  i2c: Slave did not ack Oct  5 11:41:16 moon kernel:
> [45431.023937] ivtv0:  i2c: Slave did not ack Oct  5 11:41:16 moon kernel:
> [45431.026478] ivtv0:  i2c: Slave did not ack Oct  5 11:41:16 moon kernel:
> [45431.028998] ivtv0:  i2c: Slave did not ack Oct  5 11:41:16 moon kernel:
> [45431.029063] ivtv0:  i2c: i2c write to 71 failed Oct  5 11:41:16 moon
> kernel: [45431.031468] ivtv0:  i2c: Slave did not ack ....

Re: [REVIEW] ivtv, ir-kbd-i2c: Explicit IR support for the AVerTV M116 for newer kernels

[Oldrich Jedlicka]

On Monday 05 of October 2009 at 00:23:47, Aleksandr V. Piskunov wrote:
> On Sat, Oct 03, 2009 at 11:44:20AM -0400, Andy Walls wrote:
> > Aleksandr and Jean,
> >
> > Zdrastvoitye & Bonjour,
> >
> > To support the AVerMedia M166's IR microcontroller in ivtv and
> > ir-kbd-i2c with the new i2c binding model, I have added 3 changesets in
> >
> > 	http://linuxtv.org/hg/~awalls/ivtv
>
> Now the last step to the decent support of M116 remote.
>
> I spent hours banging my head against the wall, controller just doesn't
> give a stable keypresses, skips a lot of them. Increasing polling interval
> from default 100 ms to 400-500 ms helps a bit, but it only masks the
> problem. Decreasing polling interval below 50ms makes it skip virtually
> 90% of keypresses.
>
> Basicly during the I2C operation that reads scancode, controller seems
> to stop processing input from IR sensor, resulting a loss of keypress.

Hi Aleksandr,

Just a side note. If your M166 has the same remote control chip as my CardBus 
Plus/Hybrid (I2C address 0x40), then I have to say it is very fragile. From 
my experience it doesn't like probing (empty read), when reading the value it 
doesn't like interruptions (you have to write the address and read 
immediately). So I wouldn't be surprised if it doesn't work under some other 
conditions.

Regards,
Oldrich.

>
> So the solution(?) I found was to decrease the udelay in
> ivtv_i2c_algo_template from 10 to 5. Guess it just doubles the frequency
> of ivtv i2c bus or something like that. Problem went away, IR controller
> is now working as expected.
>
> So question is:
> 1) Is it ok to decrease udelay for this board?
> 2) If yes, how to do it right?