saa7134: Fix IR support of some ASUS TV-FM 7135 variants

From: Jean Delvare <[email protected]>
Subject: saa7134: Fix IR support of some ASUS TV-FM 7135 variants

Some variants of the ASUS TV-FM 7135 are handled as the ASUSTeK P7131 Analog (card=146). However, by the time we find out, some card-specific initialization is missed. In particular, the fact that the IR is GPIO-based. Set it when we change the card type.

We also have to move the initialization of IR until after the card number has been changed. I hope that this won't cause any problem.

Quoted 10 lines · Jean Delvare

From: Jean Delvare <[email protected]>
Subject: saa7134: Fix IR support of some ASUS TV-FM 7135 variants

Some variants of the ASUS TV-FM 7135 are handled as the ASUSTeK P7131 Analog (card=146). However, by the time we find out, some card-specific initialization is missed. In particular, the fact that the IR is GPIO-based. Set it when we change the card type.

We also have to move the initialization of IR until after the card number has been changed. I hope that this won't cause any problem.

Hi Jean,

Moving the initialization will likely cause regressions. The reason why there are two init codes there were due to the way the old i2c code used to work. This got fixed after the i2c rework, but it caused regressions on that time.

The proper way would be to just muve the IR initialization on this board from init1 to init2, instead of changing it for all other devices.

cheers,
Mauro

Am Freitag, den 29.01.2010, 13:40 -0200 schrieb Mauro Carvalho Chehab:

Quoted 17 lines

Quoted 10 lines · Jean Delvare

From: Jean Delvare <[email protected]>
Subject: saa7134: Fix IR support of some ASUS TV-FM 7135 variants

Some variants of the ASUS TV-FM 7135 are handled as the ASUSTeK P7131 Analog (card=146). However, by the time we find out, some card-specific initialization is missed. In particular, the fact that the IR is GPIO-based. Set it when we change the card type.

We also have to move the initialization of IR until after the card number has been changed. I hope that this won't cause any problem.

Hi Jean,

Moving the initialization will likely cause regressions. The reason why there are two init codes there were due to the way the old i2c code used to work. This got fixed after the i2c rework, but it caused regressions on that time.

Quoted 2 lines

The proper way would be to just muve the IR initialization on this board from init1 to init2, instead of changing it for all other devices.

Mauro, I do agree with you that it is likely better to go a way with minimum chances for regressions, also given the current testing base and that only this single card is involved..

Do we end up with something card specific in core code here? After all, we know this is a no go.

Hartmut and me thought back and forth on how to deal with it for quite some while, unfortunately Hartmut is not present currently on the list, but he voted for to have a separate entry for that card finally too.

What we seem to have now is:

1. We don't know, if Jean's patch really would cause regressions, but it is likely hard to get all the testing done. No problems with a FlyVideo3000 gpio remote at the time Roman suggested it, but I had not any i2c remote that time ...

(...)
Given what is also in the cruft for bttv, I would not care too much for that single card on that now also ancient driver, just print what the user can do to escape and any google would find it quickly too. For Asus it is a unique problem on that driver so far.

I should have some time on Sunday afternoon for testing, if we should go that way.

Hi Mauro, Hermann,

Quoted 18 lines · hermann pitton

Am Freitag, den 29.01.2010, 13:40 -0200 schrieb Mauro Carvalho Chehab:

Quoted 17 lines

Quoted 10 lines · Jean Delvare

From: Jean Delvare <[email protected]>
Subject: saa7134: Fix IR support of some ASUS TV-FM 7135 variants

Some variants of the ASUS TV-FM 7135 are handled as the ASUSTeK P7131 Analog (card=146). However, by the time we find out, some card-specific initialization is missed. In particular, the fact that the IR is GPIO-based. Set it when we change the card type.

We also have to move the initialization of IR until after the card number has been changed. I hope that this won't cause any problem.

Hi Jean,

Moving the initialization will likely cause regressions. The reason why there are two init codes there were due to the way the old i2c code used to work. This got fixed after the i2c rework, but it caused regressions on that time.

I don't think there is any problem with having two init sequences. You need the EEPROM to identify some devices, you need I2C support to access the EEPROM, and you need some initialization to get I2C operational.

This doesn't mean that some adjustments to the exact sequence aren't possible. In particular, I don't see what else can depend on IR being initialized, so I can't really see what harm can be done in moving IR init code _later_ in the sequence. Looking at saa7134_input_init1(), I see that it is essentially setting up software parameters in the saa7134_dev structure, there is almost no hardware access. Only for a few cards, we change a couple bits in registers GPIO_GPMODE* and GPIO_GPSTATUS*. I honestly can't see how doing this _later_ in the init sequence could be a problem.

Quoted 2 lines

Quoted 2 lines

The proper way would be to just muve the IR initialization on this board from init1 to init2, instead of changing it for all other devices.

Hmm, OK. I think it's wrong, but I'm not the one to decide. Patch below.

Quoted 3 lines

Mauro, I do agree with you that it is likely better to go a way with minimum chances for regressions, also given the current testing base and that only this single card is involved..

I admit I am very surprised that we apparently can't get anyone to test changes to a driver that supports 176 different models of TV cards :(

Quoted 7 lines

Do we end up with something card specific in core code here? After all, we know this is a no go.

Hartmut and me thought back and forth on how to deal with it for quite some while, unfortunately Hartmut is not present currently on the list, but he voted for to have a separate entry for that card finally too.

Quoted 6 lines

What we seem to have now is:

1. We don't know, if Jean's patch really would cause regressions, but it is likely hard to get all the testing done. No problems with a FlyVideo3000 gpio remote at the time Roman suggested it, but I had not any i2c remote that time ...

I doubt it matters, given that saa7134_input_init1() only cares about GPIO-based IR:

int saa7134_input_init1(struct saa7134_dev *dev)

{
	(...)
	if (dev->has_remote != SAA7134_REMOTE_GPIO)
		return -ENODEV;

So the moving the call to this function should have no effect on boards with I2C-based IR.

Quoted 5 lines

(...)
Given what is also in the cruft for bttv, I would not care too much for that single card on that now also ancient driver, just print what the user can do to escape and any google would find it quickly too. For Asus it is a unique problem on that driver so far.

This isn't how we're going to make Linux popular.

Quoted 2 lines

I should have some time on Sunday afternoon for testing, if we should go that way.

Any testing you can provide is very welcome, thanks.

* * * * *

From: Jean Delvare <[email protected]>
Subject: saa7134: Fix IR support of some ASUS TV-FM 7135 variants

Some variants of the ASUS TV-FM 7135 are handled as the ASUSTeK P7131 Analog (card=146). However, by the time we find out, some card-specific initialization is missed. In particular, the fact that the IR is GPIO-based. Set it when we change the card type, and run saa7134_input_init1().

Signed-off-by Jean Delvare <[email protected]>

Cc Daro <[email protected]>

Cc Roman Kellner <[email protected]>

---
 linux/drivers/media/video/saa7134/saa7134-cards.c |    5 +++++
 1 file changed, 5 insertions(+)

linux/drivers/media/video/saa7134/saa7134-cards.c +0 -0

@@ -7299,6 +7299,11 @@ int saa7134_board_init2(struct saa7134_d
 		       printk(KERN_INFO "%s: P7131 analog only, using "

For now, I only faked a P7131 Dual with a broken IR receiver on a 2.6.29 with recent, you can see that gpio 0x40000 doesn't go high, but your patch should enable the remote on that P7131 analog only.

Hi Hermann,

Quoted 3 lines · hermann pitton

For now, I only faked a P7131 Dual with a broken IR receiver on a 2.6.29 with recent, you can see that gpio 0x40000 doesn't go high, but your patch should enable the remote on that P7131 analog only.

I'm not sure why you had to fake anything? What I'd like to know is simply if my first patch had any negative effect on other cards.

Hi Jean,

Am Montag, den 01.02.2010, 10:56 +0100 schrieb Jean Delvare:

Quoted 7 lines

Hi Hermann,

Quoted 3 lines · hermann pitton

For now, I only faked a P7131 Dual with a broken IR receiver on a 2.6.29 with recent, you can see that gpio 0x40000 doesn't go high, but your patch should enable the remote on that P7131 analog only.

Quoted 2 lines

I'm not sure why you had to fake anything? What I'd like to know is simply if my first patch had any negative effect on other cards.

because I simply don't have that Asus My Cinema analog only in question.

To recap, you previously announced a patch, tested by Daro, claiming to get the remote up under auto detection for that device and I told you having some doubts on it.

Mauro prefers to have a fix for that single card in need for now.

Since nobody else cares, "For now", see above, I can confirm that your last patch for that single device should work to get IR up with auto detection in delay after we change the card such late with eeprom detection.

The meaning of that byte in use here is unknown to me, we should avoid such as much we can! It can turn out to be only some pseudo service.

If your call for testers on your previous attempt, really reaches some for some reason, I'm with you, but for now I have to keep the car operable within all such snow.

Hi Hermann,

Quoted 9 lines · hermann pitton

Hi Jean,

Am Montag, den 01.02.2010, 10:56 +0100 schrieb Jean Delvare:

Quoted 6 lines

Hi Hermann,

Quoted 3 lines · hermann pitton

For now, I only faked a P7131 Dual with a broken IR receiver on a 2.6.29 with recent, you can see that gpio 0x40000 doesn't go high, but your patch should enable the remote on that P7131 analog only.

Quoted 7 lines

Quoted 2 lines

I'm not sure why you had to fake anything? What I'd like to know is simply if my first patch had any negative effect on other cards.

because I simply don't have that Asus My Cinema analog only in question.

To recap, you previously announced a patch, tested by Daro, claiming to get the remote up under auto detection for that device and I told you having some doubts on it.

My first patch was not actually tested by Daro. What he tested was loading the driver with card=146. At first I thought it was equivalent, but since then I have realized it wasn't. That's the reason why the "Tested-by:" was turned into a mere "Cc:" on my second and third patches.

Quoted 10 lines

Mauro prefers to have a fix for that single card in need for now.

Since nobody else cares, "For now", see above, I can confirm that your last patch for that single device should work to get IR up with auto detection in delay after we change the card such late with eeprom detection.

The meaning of that byte in use here is unknown to me, we should avoid such as much we can! It can turn out to be only some pseudo service.

Quoted 3 lines

If your call for testers on your previous attempt, really reaches some for some reason, I'm with you, but for now I have to keep the car operable within all such snow.

That I understand. What I don't understand is: if you have a SAA7134-based card, why don't you test my second patch (the one moving the call to saa7134_input_init1 to saa7134_hwinit2) on it, without faking anything? This would be a first, useful data point.

From all those entries, there are 15 boards that are listed on both init1 and init2:

The init1 code has 107 boards listed:

SAA7134_BOARD_10MOONSTVMASTER3
SAA7134_BOARD_ADS_DUO_CARDBUS_PTV331
SAA7134_BOARD_ASUST
SAA7134_BOARD_AVACSSMARTTV
SAA7134_BOARD_AVERMEDIA_305
SAA7134_BOARD_AVERMEDIA_307
SAA7134_BOARD_AVERMEDIA_777
SAA7134_BOARD_AVERMEDIA_A169_B
SAA7134_BOARD_AVERMEDIA_A16AR
SAA7134_BOARD_AVERMEDIA_A16D
SAA7134_BOARD_AVERMEDIA_A700_HYBRID
SAA7134_BOARD_AVERMEDIA_A700_PRO
SAA7134_BOARD_AVERMEDIA_CARDBUS
SAA7134_BOARD_AVERMEDIA_CARDBUS_501
SAA7134_BOARD_AVERMEDIA_CARDBUS_506
SAA7134_BOARD_AVERMEDIA_GO_007_FM
SAA7134_BOARD_AVERMEDIA_GO_007_FM_PLUS
SAA7134_BOARD_AVERMEDIA_M102
SAA7134_BOARD_AVERMEDIA_M103
SAA7134_BOARD_AVERMEDIA_M115
SAA7134_BOARD_AVERMEDIA_M135A
SAA7134_BOARD_AVERMEDIA_STUDIO_305
SAA7134_BOARD_AVERMEDIA_STUDIO_307
SAA7134_BOARD_AVERMEDIA_STUDIO_505
SAA7134_BOARD_AVERMEDIA_STUDIO_507
SAA7134_BOARD_BEHOLD_401
SAA7134_BOARD_BEHOLD_403
SAA7134_BOARD_BEHOLD_403FM
SAA7134_BOARD_BEHOLD_405
SAA7134_BOARD_BEHOLD_405FM
SAA7134_BOARD_BEHOLD_407
SAA7134_BOARD_BEHOLD_407FM
SAA7134_BOARD_BEHOLD_409
SAA7134_BOARD_BEHOLD_409FM
SAA7134_BOARD_BEHOLD_505FM
SAA7134_BOARD_BEHOLD_505RDS_MK3
SAA7134_BOARD_BEHOLD_505RDS_MK5
SAA7134_BOARD_BEHOLD_507_9FM
SAA7134_BOARD_BEHOLD_507RDS_MK3
SAA7134_BOARD_BEHOLD_507RDS_MK5
SAA7134_BOARD_BEHOLD_607FM_MK3
SAA7134_BOARD_BEHOLD_607FM_MK5
SAA7134_BOARD_BEHOLD_607RDS_MK3
SAA7134_BOARD_BEHOLD_607RDS_MK5
SAA7134_BOARD_BEHOLD_609FM_MK3
SAA7134_BOARD_BEHOLD_609FM_MK5
SAA7134_BOARD_BEHOLD_609RDS_MK3
SAA7134_BOARD_BEHOLD_609RDS_MK5
SAA7134_BOARD_BEHOLD_COLUMBUS_TVFM
SAA7134_BOARD_BEHOLD_H6
SAA7134_BOARD_BEHOLD_M6
SAA7134_BOARD_BEHOLD_M63
SAA7134_BOARD_BEHOLD_M6_EXTRA
SAA7134_BOARD_BEHOLD_X7
SAA7134_BOARD_CINERGY400
SAA7134_BOARD_CINERGY400_CARDBUS
SAA7134_BOARD_CINERGY600
SAA7134_BOARD_CINERGY600_MK3
SAA7134_BOARD_ECS_TVP3XP
SAA7134_BOARD_ECS_TVP3XP_4CB5
SAA7134_BOARD_ECS_TVP3XP_4CB6
SAA7134_BOARD_ENCORE_ENLTV
SAA7134_BOARD_ENCORE_ENLTV_FM
SAA7134_BOARD_ENCORE_ENLTV_FM53
SAA7134_BOARD_FLYDVBS_LR300
SAA7134_BOARD_FLYDVBTDUO
SAA7134_BOARD_FLYDVBT_DUO_CARDBUS
SAA7134_BOARD_FLYDVBT_HYBRID_CARDBUS
SAA7134_BOARD_FLYDVBT_LR301
SAA7134_BOARD_FLYTVPLATINUM_FM
SAA7134_BOARD_FLYTVPLATINUM_MINI2
SAA7134_BOARD_FLYVIDEO2000
SAA7134_BOARD_FLYVIDEO3000
SAA7134_BOARD_FLYVIDEO3000_NTSC
SAA7134_BOARD_GENIUS_TVGO_A11MCE
SAA7134_BOARD_GOTVIEW_7135
SAA7134_BOARD_HAUPPAUGE_HVR1110
SAA7134_BOARD_HAUPPAUGE_HVR1120
SAA7134_BOARD_HAUPPAUGE_HVR1150
SAA7134_BOARD_KWORLD_PLUS_TV_ANALOG
SAA7134_BOARD_KWORLD_TERMINATOR
SAA7134_BOARD_KWORLD_VSTREAM_XPERT
SAA7134_BOARD_KWORLD_XPERT
SAA7134_BOARD_LEADTEK_WINFAST_DTV1000S
SAA7134_BOARD_MANLI_MTV001
SAA7134_BOARD_MANLI_MTV002
SAA7134_BOARD_MD2819
SAA7134_BOARD_MD5044
SAA7134_BOARD_MONSTERTV_MOBILE
SAA7134_BOARD_MSI_TVATANYWHERE_PLUS
SAA7134_BOARD_PINNACLE_300I_DVBT_PAL
SAA7134_BOARD_PINNACLE_PCTV_110
SAA7134_BOARD_PINNACLE_PCTV_310
SAA7134_BOARD_PROTEUS_2309
SAA7134_BOARD_REAL_ANGEL_220
SAA7134_BOARD_ROVERMEDIA_LINK_PRO_FM
SAA7134_BOARD_RTD_VFG7350
SAA7134_BOARD_SABRENT_SBTTVFM
SAA7134_BOARD_SEDNA_PC_TV_CARDBUS
SAA7134_BOARD_UPMOST_PURPLE_TV
SAA7134_BOARD_VIDEOMATE_DVBT_200
SAA7134_BOARD_VIDEOMATE_DVBT_200A
SAA7134_BOARD_VIDEOMATE_DVBT_300
SAA7134_BOARD_VIDEOMATE_GOLD_PLUS
SAA7134_BOARD_VIDEOMATE_S350
SAA7134_BOARD_VIDEOMATE_TV_GOLD_PLUSII
SAA7134_BOARD_VIDEOMATE_TV_PVR

The init2 code has 32 boards listed:

SAA7134_BOARD_ADS_DUO_CARDBUS_PTV331
SAA7134_BOARD_ADS_INSTANT_HDTV_PCI
SAA7134_BOARD_ASUS_EUROPA2_HYBRID
SAA7134_BOARD_ASUS_EUROPA_HYBRID
SAA7134_BOARD_ASUST
SAA7134_BOARD_AVERMEDIA_CARDBUS_501
SAA7134_BOARD_AVERMEDIA_SUPER_007
SAA7134_BOARD_BEHOLD_COLUMBUS_TVFM
SAA7134_BOARD_BMK_MPEX_NOTUNER
SAA7134_BOARD_BMK_MPEX_TUNER
SAA7134_BOARD_CINERGY_HT_PCI
SAA7134_BOARD_CINERGY_HT_PCMCIA
SAA7134_BOARD_CREATIX_CTX953
SAA7134_BOARD_FLYDVBT_HYBRID_CARDBUS
SAA7134_BOARD_FLYDVB_TRIO
SAA7134_BOARD_HAUPPAUGE_HVR1110
SAA7134_BOARD_HAUPPAUGE_HVR1120
SAA7134_BOARD_HAUPPAUGE_HVR1150
SAA7134_BOARD_KWORLD_ATSC110
SAA7134_BOARD_KWORLD_DVBT_210
SAA7134_BOARD_MD7134
SAA7134_BOARD_MEDION_MD8800_QUADRO
SAA7134_BOARD_PHILIPS_EUROPA
SAA7134_BOARD_PHILIPS_SNAKE
SAA7134_BOARD_PHILIPS_TIGER
SAA7134_BOARD_PHILIPS_TIGER_S
SAA7134_BOARD_PINNACLE_PCTV_310
SAA7134_BOARD_TEVION_DVBT_220RF
SAA7134_BOARD_TWINHAN_DTV_DVB_3056
SAA7134_BOARD_VIDEOMATE_DVBT_200
SAA7134_BOARD_VIDEOMATE_DVBT_200A
SAA7134_BOARD_VIDEOMATE_DVBT_300

From all those entries, there are 15 boards that are listed on both init1 and init2:

SAA7134_BOARD_ADS_DUO_CARDBUS_PTV331
SAA7134_BOARD_ASUST
SAA7134_BOARD_AVERMEDIA_CARDBUS_501
SAA7134_BOARD_BEHOLD_COLUMBUS_TVFM
SAA7134_BOARD_BMK_MPEX_NOTUNER
SAA7134_BOARD_BMK_MPEX_TUNER
SAA7134_BOARD_FLYDVBT_HYBRID_CARDBUS
SAA7134_BOARD_HAUPPAUGE_HVR1110
SAA7134_BOARD_HAUPPAUGE_HVR1120
SAA7134_BOARD_HAUPPAUGE_HVR1150
SAA7134_BOARD_PHILIPS_TIGER_S
SAA7134_BOARD_PINNACLE_PCTV_310
SAA7134_BOARD_VIDEOMATE_DVBT_200
SAA7134_BOARD_VIDEOMATE_DVBT_200A
SAA7134_BOARD_VIDEOMATE_DVBT_300

So, there are a large set of boards that will be affected by this change.

Your premise that the init1 only cares about GPIO IR is not true. It does contain the GPIO initializations to reset, turn on devices and enable i2c bridges for those devices. Eventually, on a few boards, the gpio's are only due to IR, but this is an exception.

The current code does that:

        saa7134_board_init1(dev);
        saa7134_hwinit1(dev);
        msleep(100);
        saa7134_i2c_register(dev);
        saa7134_board_init2(dev);
        saa7134_hwinit2(dev);

What happens is that the saa7134_board_init1(dev) code has lots of gpio codes, and most of those code is needed in order to enable i2c bridges or to turn on/reset some chips that would otherwise be on OFF or undefined state. Without the gpio code, done by init1, you may not be able to read eeprom or to detect the devices as needed by saa7134_board_init2().

That's why I'm saying that, if in the specific case of the board you're dealing with, you're sure that an unknown GPIO state won't affect the code at saa7134_hwinit1() and at saa7134_i2c_register(), then you can move the GPIO code for this board to init2.

Moving things between init1 and init2 are very tricky and requires testing on all the affected boards. So a change like what your patch proposed would require a test of all the 107 boards that are on init1. I bet you'll break several of them with this change.

Hi Mauro,

Sorry for the late answer. I'm tracking so many things in parallel...

Quoted 5 lines

What happens is that the saa7134_board_init1(dev) code has lots of gpio codes, and most of those code is needed in order to enable i2c bridges or to turn on/reset some chips that would otherwise be on OFF or undefined state. Without the gpio code, done by init1, you may not be able to read eeprom or to detect the devices as needed by saa7134_board_init2().

I don't think I ever said otherwise. I never said that init1 as a whole only cared about GPIO IR. That's what I said of function saa7134_input_init1() and only this function!

My first proposed patch didn't move all of init1 to init2. It was only moving saa7134_input_init1 (which doesn't yet have a counterpart in init2), and it is moving it from the end of init1 to the beginning of init2, so the movement isn't as big as it may sound. The steps saa7134_input_init1() is moving over are, in order:
* saa7134_hw_enable1
* request_irq
* saa7134_i2c_register

So my point was that none of these 3 functions seemed to be dependent on saa7134_input_init1() having been called beforehand. Which is why I strongly question the fact that moving saa7134_input_init1() _after_ these 3 other initialization steps can have any negative effect. I wouldn't have claimed that moving saa7134_input_init1() _earlier_ in the init sequence would be safe, because there's nothing obvious about that. But moving it forward where I had put it did not seem terrific.

I really would like a few users of this driver to just give it a try and report, because it seems a much more elegant fix to the bug at hand, than the workaround you'd accept instead.

Quoted 4 lines

That's why I'm saying that, if in the specific case of the board you're dealing with, you're sure that an unknown GPIO state won't affect the code at saa7134_hwinit1() and at saa7134_i2c_register(), then you can move the GPIO code for this board to init2.

Quoted 3 lines

Moving things between init1 and init2 are very tricky and requires testing on all the affected boards. So a change like what your patch proposed would require a test of all the 107 boards that are on init1. I bet you'll break several of them with this change.

Under the assumption that saa7134_hwinit1() only touches GPIOs connected to IR receivers (and it certainly looks like this to me) I fail to see how these pins not being initialized could have any effect on non-IR code.

Now, please don't get me wrong. I don't have any of these devices. I've posted that patch because a user incidentally pointed me to a problem and I had an idea how it could be fixed. I prefer my initial proposal because it looks better in the long run. If you don't like it and prefer the second proposal even though I think it's more of a workaround than a proper fix, it's really up to you. You're maintaining the subsystem and I am not, so you're the one deciding.

From my side, I just want to make sure that we won't add any regressions with other boards.

Quoted 4 lines · Jean Delvare

Under the assumption that saa7134_hwinit1() only touches GPIOs connected to IR receivers (and it certainly looks like this to me) I fail to see how these pins not being initialized could have any effect on non-IR code.

Now, i suspect that you're messing things again: are you referring to saa7134_hwinit1() or to saa7134_input_init1()?

I suspect that you're talking about moving saa7134_input_init1(), since saa7134_hwinit1() has the muted and spinlock inits. It also has the setups for video, vbi and mpeg. So, moving it require more care.

Quoted 9 lines

Quoted 4 lines · Jean Delvare

Under the assumption that saa7134_hwinit1() only touches GPIOs connected to IR receivers (and it certainly looks like this to me) I fail to see how these pins not being initialized could have any effect on non-IR code.

Now, i suspect that you're messing things again: are you referring to saa7134_hwinit1() or to saa7134_input_init1()?

Quoted 3 lines

I suspect that you're talking about moving saa7134_input_init1(), since saa7134_hwinit1() has the muted and spinlock inits. It also has the setups for video, vbi and mpeg. So, moving it require more care.

Err, you're right, I meant saa7134_input_init1() and not saa7134_hwinit1(), copy-and-paste error. Sorry for adding more confusion where it really wasn't needed...

Hi,

Am Mittwoch, den 10.02.2010, 20:36 +0100 schrieb Jean Delvare:

Quoted 10 lines · Mauro Carvalho Chehab

Quoted 9 lines

Quoted 4 lines · Jean Delvare

Under the assumption that saa7134_hwinit1() only touches GPIOs connected to IR receivers (and it certainly looks like this to me) I fail to see how these pins not being initialized could have any effect on non-IR code.

Now, i suspect that you're messing things again: are you referring to saa7134_hwinit1() or to saa7134_input_init1()?

Quoted 8 lines

Quoted 3 lines

I suspect that you're talking about moving saa7134_input_init1(), since saa7134_hwinit1() has the muted and spinlock inits. It also has the setups for video, vbi and mpeg. So, moving it require more care.

Err, you're right, I meant saa7134_input_init1() and not saa7134_hwinit1(), copy-and-paste error. Sorry for adding more confusion where it really wasn't needed...

both attempts of Jean will work.

If we are only talking about moving input_init, only that Jean did suggest initially, it should work, since only some GPIOs for enabling remote chips are affected.

I can give the crappy tester, but don't have such a remote, but should not be a problem to trigger the GPIOs later.

Cheers,
Hermann

Am Donnerstag, den 11.02.2010, 01:58 +0100 schrieb hermann pitton:

Quoted 28 lines

Hi,

Am Mittwoch, den 10.02.2010, 20:36 +0100 schrieb Jean Delvare:

Quoted 10 lines · Mauro Carvalho Chehab

Quoted 9 lines

Quoted 4 lines · Jean Delvare

Under the assumption that saa7134_hwinit1() only touches GPIOs connected to IR receivers (and it certainly looks like this to me) I fail to see how these pins not being initialized could have any effect on non-IR code.

Now, i suspect that you're messing things again: are you referring to saa7134_hwinit1() or to saa7134_input_init1()?

Quoted 8 lines

Quoted 3 lines

I suspect that you're talking about moving saa7134_input_init1(), since saa7134_hwinit1() has the muted and spinlock inits. It also has the setups for video, vbi and mpeg. So, moving it require more care.

Err, you're right, I meant saa7134_input_init1() and not saa7134_hwinit1(), copy-and-paste error. Sorry for adding more confusion where it really wasn't needed...

both attempts of Jean will work.

If we are only talking about moving input_init, only that Jean did suggest initially, it should work, since only some GPIOs for enabling remote chips are affected.

Quoted 6 lines

I can give the crappy tester, but don't have such a remote, but should not be a problem to trigger the GPIOs later.

Cheers,
Hermann

Hi Jean,

I did test your patch, only following Roman's initial patch already known, on eight different cards for now, also with three slightly different remotes and it does not have any negative impact.

Please consider, that it is only about that single card for now and a per card solution is enough.

I strongly remind, that we should not rely on unknown eeprom bytes, as told previously and should not expand such into any direction.

If we make progress there, we should change it for all cards, but again, what had happened on the m$ drivers previously is not encouraging to do it without any need.

To do it per card in need for now seems enough "service" to me.

If more such should come, unlikely on that driver, I would at first deny auto detection support, since they are breaking rules.

The problem likely will time out very soon.

Cheers,
Hermann

Are you still waiting for Daro's report?

As said, I would prefer to see all OEMs _not_ following Philips/NXP eeprom rules running into their own trash on GNU/Linux too.

Then we have facts.

That is much better than to provide a golden cloud for them. At least I won't help to debug such later ...

If you did not manage to decipher all OEM eeprom content already, just let's go with the per card solution for now.

Are you aware, that my intention is _not_ to spread the use of random and potentially invalid eeprom content for some sort of such auto detection?

The other solution is not lost and in mind, if we should need to come back to it and are in details. Preferably the OEMs should take the responsibility for such.

We can see, that even those always doing best on it, can't provide the missing informations for different LNA stuff after the Hauppauge/Pinnacle merge until now.

If you claim to know it better, please share with us.

Quoted 1 line

Are you still waiting for Daro's report?

Yes, I am still waiting. Unfortunately neither Daro nor Roman reported any test result so far. Now, if we go for my second patch, I guess we might as well apply it right now. It only affects this one card (Asus P7131 analog), and it was broken so far, so I don't think my patch can do any bad.

Quoted 19 lines

As said, I would prefer to see all OEMs _not_ following Philips/NXP eeprom rules running into their own trash on GNU/Linux too.

Then we have facts.

That is much better than to provide a golden cloud for them. At least I won't help to debug such later ...

If you did not manage to decipher all OEM eeprom content already, just let's go with the per card solution for now.

Are you aware, that my intention is _not_ to spread the use of random and potentially invalid eeprom content for some sort of such auto detection?

The other solution is not lost and in mind, if we should need to come back to it and are in details. Preferably the OEMs should take the responsibility for such.

Quoted 5 lines

We can see, that even those always doing best on it, can't provide the missing informations for different LNA stuff after the Hauppauge/Pinnacle merge until now.

If you claim to know it better, please share with us.

I'm not claiming anything, and to be honest, I have no idea what you're talking about.

Hi Hermann,

Quoted 10 lines · hermann pitton

Hi Jean,

Am Montag, den 01.02.2010, 10:56 +0100 schrieb Jean Delvare:

Quoted 7 lines

Hi Hermann,

Quoted 3 lines · hermann pitton

For now, I only faked a P7131 Dual with a broken IR receiver on a 2.6.29 with recent, you can see that gpio 0x40000 doesn't go high, but your patch should enable the remote on that P7131 analog only.

Quoted 8 lines

Quoted 2 lines

I'm not sure why you had to fake anything? What I'd like to know is simply if my first patch had any negative effect on other cards.

because I simply don't have that Asus My Cinema analog only in question.

To recap, you previously announced a patch, tested by Daro, claiming to get the remote up under auto detection for that device and I told you having some doubts on it.

My first patch was not actually tested by Daro. What he tested was loading the driver with card=146. At first I thought it was equivalent, but since then I have realized it wasn't. That's the reason why the "Tested-by:" was turned into a mere "Cc:" on my second and third patches.

Quoted 10 lines

Mauro prefers to have a fix for that single card in need for now.

Since nobody else cares, "For now", see above, I can confirm that your last patch for that single device should work to get IR up with auto detection in delay after we change the card such late with eeprom detection.

The meaning of that byte in use here is unknown to me, we should avoid such as much we can! It can turn out to be only some pseudo service.

Quoted 3 lines

If your call for testers on your previous attempt, really reaches some for some reason, I'm with you, but for now I have to keep the car operable within all such snow.

That I understand. What I don't understand is: if you have a SAA7134-based card, why don't you test my second patch (the one moving the call to saa7134_input_init1 to saa7134_hwinit2) on it, without faking anything? This would be a first, useful data point.

Hi Jean, Mauro and all,

Am Dienstag, den 02.02.2010, 08:54 +0100 schrieb Jean Delvare:

Quoted 42 lines

Hi Hermann,

Quoted 12 lines · hermann pitton

Hi Jean,

Am Montag, den 01.02.2010, 10:56 +0100 schrieb Jean Delvare:

Quoted 8 lines

Hi Hermann,

Quoted 4 lines · hermann pitton

For now, I only faked a P7131 Dual with a broken IR receiver on a 2.6.29 with recent, you can see that gpio 0x40000 doesn't go high, but your patch should enable the remote on that P7131 analog only.

Quoted 9 lines

Quoted 3 lines

I'm not sure why you had to fake anything? What I'd like to know is simply if my first patch had any negative effect on other cards.

because I simply don't have that Asus My Cinema analog only in question.

To recap, you previously announced a patch, tested by Daro, claiming to get the remote up under auto detection for that device and I told you having some doubts on it.

My first patch was not actually tested by Daro. What he tested was loading the driver with card=146. At first I thought it was equivalent, but since then I have realized it wasn't. That's the reason why the "Tested-by:" was turned into a mere "Cc:" on my second and third patches.

Quoted 10 lines

Mauro prefers to have a fix for that single card in need for now.

Since nobody else cares, "For now", see above, I can confirm that your last patch for that single device should work to get IR up with auto detection in delay after we change the card such late with eeprom detection.

The meaning of that byte in use here is unknown to me, we should avoid such as much we can! It can turn out to be only some pseudo service.

Quoted 10 lines

Quoted 4 lines

If your call for testers on your previous attempt, really reaches some for some reason, I'm with you, but for now I have to keep the car operable within all such snow.

That I understand. What I don't understand is: if you have a SAA7134-based card, why don't you test my second patch (the one moving the call to saa7134_input_init1 to saa7134_hwinit2) on it, without faking anything? This would be a first, useful data point.

sorry, the snow fall did not stop and we will need trucks next day to get it out of town. No place left.

I did not reread any single line of code until now, but told you that Roman has tested a equivalent patch on his P7131_ANALOG already and I can confirm that it also had no side effects on a FlyVideo3000 card=2.

For now, I would at least need some time to see, if input_init can be decoupled from all other hardware init, what you seem to suggest, and looking closer to Mauro's concerns.

Thought you are asking for some test with a i2c remote next to confirm your analysis there. No such card in any machine currently, but can be done.

Cheers,
Hermann

If some tests on my machine could be helpfull just let me know.

From: Jean Delvare <[email protected]>
Subject: saa7134: Fix IR support of some ASUS TV-FM 7135 variants

Some variants of the ASUS TV-FM 7135 are handled as the ASUSTeK P7131 Analog (card=146). However, by the time we find out, some card-specific initialization is missed. In particular, the fact that the IR is GPIO-based. Set it when we change the card type, and run saa7134_input_init1().

Signed-off-by Jean Delvare <[email protected]>

Cc Daro <[email protected]>

Cc Roman Kellner <[email protected]>

---
 linux/drivers/media/video/saa7134/saa7134-cards.c |    5 +++++
 1 file changed, 5 insertions(+)

linux/drivers/media/video/saa7134/saa7134-cards.c +0 -0

@@ -7299,6 +7299,11 @@ int saa7134_board_init2(struct saa7134_d
 		       printk(KERN_INFO "%s: P7131 analog only, using "

Quoted 20 lines

From: Jean Delvare <[email protected]>
Subject: saa7134: Fix IR support of some ASUS TV-FM 7135 variants

Some variants of the ASUS TV-FM 7135 are handled as the ASUSTeK P7131 Analog (card=146). However, by the time we find out, some card-specific initialization is missed. In particular, the fact that the IR is GPIO-based. Set it when we change the card type, and run saa7134_input_init1().

Signed-off-by Jean Delvare <[email protected]>

Cc Daro <[email protected]>

Cc Roman Kellner <[email protected]>

---
 linux/drivers/media/video/saa7134/saa7134-cards.c |    5 +++++
 1 file changed, 5 insertions(+)

linux/drivers/media/video/saa7134/saa7134-cards.c +0 -0

@@ -7299,6 +7299,11 @@ int saa7134_board_init2(struct saa7134_d
 		       printk(KERN_INFO "%s: P7131 analog only, using "

linux/drivers/media/video/saa7134/saa7134-cards.c int saa7134_board_init2(struct saa7134_d · line 7309

Quoted 10 lines

linux/drivers/media/video/saa7134/saa7134-cards.c +5 -0

 						       "entry of %s\n",
 		       dev->name, saa7134_boards[dev->board].name);
+
+			/* IR init has already happened for other cards, so
+			 * we have to catch up. */
+			dev->has_remote = SAA7134_REMOTE_GPIO;
+			saa7134_input_init1(dev);
 	       }
 	       break;
 	case SAA7134_BOARD_HAUPPAUGE_HVR1150:

This version of your patch makes sense to me.

This logic will only apply for board SAA7134_BOARD_ASUSTeK_P7131_ANALOG, so, provided that someone with this board test it, I'm OK with it.

Had Roman or Daro already test it?