Re: [PATCH 0/3] em28xx: add support for two buses on em2874 and upper

["Frank Schäfer" <fschaefer.oss@googlemail.com>]

Am 01.04.2013 21:22, schrieb Mauro Carvalho Chehab:
> Em Mon, 01 Apr 2013 19:14:03 +0200
> Frank Schäfer <fschaefer.oss@googlemail.com> escreveu:
>
>> Am 18.03.2013 22:22, schrieb Mauro Carvalho Chehab:
>>> Em Wed, 06 Mar 2013 18:44:07 +0100
>>> Frank Schäfer <fschaefer.oss@googlemail.com> escreveu:
>>>
>>>> Am 05.03.2013 16:43, schrieb Devin Heitmueller:
>>>>> 2013/3/5 Mauro Carvalho Chehab <mchehab@redhat.com>:
>>>>>> The em2874 chips and upper have 2 buses. On all known devices, bus 0 is
>>>>>> currently used only by eeprom, and bus 1 for the rest. Add support to
>>>>>> register both buses.
>>>>> Did you add a mutex to ensure that both buses cannot be used at the
>>>>> same time?  Because using the bus requires you to toggle a register
>>>>> (thus you cannot be using both busses at the same time), you cannot
>>>>> rely on the existing i2c adapter lock anymore.
>>>>>
>>>>> You don't want a situation where something is actively talking on bus
>>>>> 0, and then something else tries to talk on bus 1, flips the register
>>>>> bit and then the thread talking on bus 0 starts failing.
>>>>>
>>>>> Devin
>>>> Hmm... there are several writes to EM28XX_R06_I2C_CLK in em28xx-dvb...
>>>> See hauppauge_hvr930c_init(), terratec_h5_init() and
>>>> terratec_htc_stick_init().
>>>> These functions are called from em28xx_dvb_init() at module init.
>>>> Module init is async, so yes, this is (or could at least become) a
>>>> problem...
>>>>
>>>> I wonder if we can't simply remove all those writes to
>>>> EM28XX_R06_I2C_CLK from em28xx-dvb.
>>>> This is what the functions are doing:
>>>>
>>>> hauppauge_hvr930c_init()
>>>>     ...
>>>>     em28xx_write_reg(dev, EM28XX_R06_I2C_CLK, 0x40);
>>>>     msleep(10);
>>>>     em28xx_write_reg(dev, EM28XX_R06_I2C_CLK, 0x44);
>>>>     msleep(10);
>>>>     ... [init sequence for slave at address 0x82]
>>>>     em28xx_write_reg(dev, EM28XX_R06_I2C_CLK, 0x44);
>>>>     msleep(30);
>>>>     em28xx_write_reg(dev, EM28XX_R06_I2C_CLK, 0x45);
>>>>     msleep(10);
>>>>
>>>> terratec_h5_init():
>>>>     ...
>>>>     em28xx_write_reg(dev, EM28XX_R06_I2C_CLK, 0x40);
>>>>     msleep(10);
>>>>     em28xx_write_reg(dev, EM28XX_R06_I2C_CLK, 0x45);
>>>>     msleep(10);
>>>>     ...
>>>>
>>>> terratec_htc_stick_init()
>>>>     ...
>>>>     em28xx_write_reg(dev, EM28XX_R06_I2C_CLK, 0x40);
>>>>     msleep(10);
>>>>     em28xx_write_reg(dev, EM28XX_R06_I2C_CLK, 0x44);
>>>>     msleep(10);
>>>>     ...
>>>>
>>>> All three boards are using the following settings:
>>>>         .i2c_speed    = EM2874_I2C_SECONDARY_BUS_SELECT |
>>>> EM28XX_I2C_CLK_WAIT_ENABLE | EM28XX_I2C_FREQ_400_KHZ = 0x45
>>>>
>>>> So what these functions are doing is
>>>> - switch to bus A and do nothing fo 10ms
>>>> - overwrite board settings for reg 0x06 with a local value (clears
>>>> EM28XX_I2C_FREQ_400_KHZ permanently for the HTC-Stick !).
>>>>
>>>> I can test the HVR-930C next week.
>> Ok, I finally had the chance to test with a HVR-930C, but it seems my
>> device () is different.

I forgot to insert the device/model of the device I tested: it's 16009, B1F0

>> It has no slave device at i2c address 0x82, so I can't test this part.
>> Btw, which slave device is this ?
>>
>> Removing the temporary switches to bus A makes no difference (as expected).
>>
>>> There are some things there on the init sequence that can be
>>> cleaned/removed. Those sequences generally comes from observing
>>> what the original driver does. While it produces a working driver,
>>> in general, it is not optimized and part of the init sequence can
>>> be removed.
>> Do you want me to send patches to remove these writes ?
>> Which i2c speed settings do you suggest for the HVR-930C and the
>> HTC-Stick (board settings:
>> EM28XX_I2C_FREQ_400_KHZ, code overwrites it with EM28XX_I2C_FREQ_100_KHZ) ?
> Sure. The better would be to even remove the hauppauge_hvr930c_init()
> function, as this is just a hack, and use the setup via the em28xx-cards
> commented entries:
>
> 		.tuner_type   = TUNER_XC5000,
> 		.tuner_addr   = 0x41,
> 		.dvb_gpio     = hauppauge_930c_digital,
> 		.tuner_gpio   = hauppauge_930c_gpio,

Hmm... tuner address is 0x61 for the device I tested !
The register sequences in em28xx-cards.c also seem to be different to
the ones used in hauppauge_hvr930c_init() in em28xx-dvb.c...
Are you sure this will work for _all_ variants of the HVR-930C ?

I think it would be better if you would create those patches.
I really don't like writing patches without completely understanding the
code, not beeing able to test them and commit messages saying "Mauro
told me to do this"... ;)
You also didn't answer my question concerning the i2c speed settings. ;)

Regards,
Frank

>
> Regards,
> Mauro