All of lore.kernel.org
 help / color / mirror / Atom feed
* Re: AVerMedia HD Volar (A867) AF9035 + MXL5007T driver issues
@ 2016-04-08 23:13 Jose Alberto Reguero
  0 siblings, 0 replies; 18+ messages in thread
From: Jose Alberto Reguero @ 2016-04-08 23:13 UTC (permalink / raw)
  To: Alessandro Radicati; +Cc: linux-media, Antti Palosaari

I made a patch long time ago, but it was not accepted.

https://patchwork.linuxtv.org/patch/16242/

Jose Alberto

El 06/04/2016 01:00, Alessandro Radicati <alessandro@radicati.net> escribió:
>
> On Wed, Apr 6, 2016 at 12:33 AM, Antti Palosaari <crope@iki.fi> wrote: 
> > I found one stick having AF9035 + MXL5007T. It is HP branded A867, so it 
> > should be similar. It seems to work all three 12.13.15.0 6.20.15.0 
> > firmwares: 
> > http://palosaari.fi/linux/v4l-dvb/firmware/af9035/ 
> > 
> > mxl5007t 5-0060: creating new instance 
> > mxl5007t_get_chip_id: unknown rev (3f) 
> > mxl5007t_get_chip_id: MxL5007T detected @ 5-0060 
> > 
> > That is what AF9035 reports (with debug) as a chip version: 
> > dvb_usb_af9035: prechip_version=00 chip_version=03 chip_type=3802 
> > 
> > 
> > Do you have different chip version? 
> > 
>
> I have a Sky Italy DVB stick with the same chip version.  I see that 
> you get the 0x3f response as well... that should be fixed by the I2C 
> patch I proposed.  However, your stick seems to handle the read 
> properly and process subsequent I2C commands - something that doesn't 
> happen with mine.  The vendor drivers in linux and windows never seem 
> issue the USB I2C commands to read from the tuner.  I'll test with 
> other firmware versions to see if something changes. 
>
> Regards, 
> Alessandro 
> -- 
> To unsubscribe from this list: send the line "unsubscribe linux-media" in 
> the body of a message to majordomo@vger.kernel.org 
> More majordomo info at  http://vger.kernel.org/majordomo-info.html 

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: AVerMedia HD Volar (A867) AF9035 + MXL5007T driver issues
@ 2016-04-10 21:30 Jose Alberto Reguero
  0 siblings, 0 replies; 18+ messages in thread
From: Jose Alberto Reguero @ 2016-04-10 21:30 UTC (permalink / raw)
  To: Alessandro Radicati; +Cc: Antti Palosaari, linux-media

I have the problem with a avermedia twinstart.
07ca:0825
With first hardware revision the read works, but second hardware revision read don’t work.

Jose Alberto

El 09/04/2016 19:38, Alessandro Radicati <alessandro@radicati.net> escribió:
>
> On Sat, Apr 9, 2016 at 7:07 PM, Antti Palosaari <crope@iki.fi> wrote: 
> > 
> > 
> > On 04/09/2016 07:11 PM, Alessandro Radicati wrote: 
> >> 
> >> On Sat, Apr 9, 2016 at 4:25 PM, Antti Palosaari <crope@iki.fi> wrote: 
> >>> 
> >>> On 04/09/2016 11:13 AM, Alessandro Radicati wrote: 
> >>>> 
> >>>> 
> >>>> On Sat, Apr 9, 2016 at 4:17 AM, Antti Palosaari <crope@iki.fi> wrote: 
> >>>>> 
> >>>>> 
> >>>>> On 04/09/2016 04:52 AM, Alessandro Radicati wrote: 
> >>>>>> 
> >>>>>> 
> >>>>>> 
> >>>>>> On Sat, Apr 9, 2016 at 3:22 AM, Antti Palosaari <crope@iki.fi> wrote: 
> >>>>>>> 
> >>>>>>> 
> >>>>>>> 
> >>>>>>> Here is patches to test: 
> >>>>>>> http://git.linuxtv.org/anttip/media_tree.git/log/?h=af9035 
> >>>>>>> 
> >>>>>> 
> >>>>>> I've done this already in my testing, and it works for getting a 
> >>>>>> correct chip_id response, but only because it's avoiding the issue 
> >>>>>> with the write/read case in the af9035 driver.  Don't have an 
> >>>>>> af9015... perhaps there's a similar issue with that code or we are 
> >>>>>> dealing with two separate issues since af9035 never does a repeated 
> >>>>>> start? 
> >>>>> 
> >>>>> 
> >>>>> 
> >>>>> 
> >>>>> I am pretty sure mxl5007t requires stop between read and write. Usually 
> >>>>> chips are not caring too much if it is repeated start or not, whilst 
> >>>>> datasheets are often register read is S Wr S Rw P. 
> >>>>> 
> >>>>> Even af9035 i2c adapter implementation implements repeated start wrong, 
> >>>>> I 
> >>>> 
> >>>> 
> >>>> 
> >>>> Where does the assumption that CMD_I2C_RD should issue a repeated 
> >>>> start sequence come from?  From the datasheet?  Maybe it was never 
> >>>> intended as repeated start.  Perhaps if there is another stick  with 
> >>>> mxl5007t and a chip that does repeated start, we can put this to bed. 
> >>> 
> >>> 
> >>> 
> >>> Assumption was coming from it just does it as a single USB transaction. 
> >>> Datasheet says there is no repeated start. And kernel I2C API says all 
> >>> messages send using single i2c_transfer() should be send with repeated 
> >>> start, so now it is violating it, but that's not the biggest problem... 
> >>> 
> >> 
> >> Unfortunately there is no way around that problem, but at least it 
> >> means that you can reduce the whole function to just read and write 
> >> since at the I2C level nothing changes. 
> >> 
> >>>>> would not like to add anymore hacks there. It is currently ugly and 
> >>>>> complex 
> >>>> 
> >>>> 
> >>>> 
> >>>> Bugfix != hack.  Don't see how putting the register address in the 
> >>>> address fields is a hack (perhaps semantics around the fact that 0xFB 
> >>>> is not really part of the address?); this is the only and intended way 
> >>>> to use that command for write/read. 
> >>> 
> >>> 
> >>> 
> >>> I did bunch of testing and find it is really wrong. Dumped out registers 
> >>> from some tuner chips and those seems to be mostly off by one. 
> >>> 
> >>> I think that skeleton is correct way (and it ends about same you did) 
> >>> if (msg[0].len == 0) // probe message, payload 0 
> >>>    buf[0] = msg[0].len; 
> >>>    buf[1] = msg[0].addr << 1; 
> >>>    buf[2] = 0x00; /* reg addr len */ 
> >>>    buf[3] = 0x00; /* reg addr MSB */ 
> >>>    buf[4] = 0x00; /* reg addr LSB */ 
> >>> else if (msg[0].len == 1) 
> >>>    buf[0] = msg[0].len; 
> >>>    buf[1] = msg[0].addr << 1; 
> >>>    buf[2] = 1; /* reg addr len */ 
> >>>    buf[3] = 0x00; /* reg addr MSB */ 
> >>>    buf[4] = msg[0].buf[0]; /* reg addr LSB */ 
> >>> else if (msg[0].len == 2) 
> >>>    buf[0] = msg[0].len; 
> >>>    buf[1] = msg[0].addr << 1; 
> >>>    buf[2] = 2; /* reg addr len */ 
> >>>    buf[3] = msg[0].buf[0]; /* reg addr MSB */ 
> >>>    buf[4] = msg[0].buf[1]; /* reg addr LSB */ 
> >>> else 
> >>>    buf[0] = msg[0].len; 
> >>>    buf[1] = msg[0].addr << 1; 
> >>>    buf[2] = 2; /* reg addr len */ 
> >>>    buf[3] = msg[0].buf[0]; /* reg addr MSB */ 
> >>>    buf[4] = msg[0].buf[1]; /* reg addr LSB */ 
> >>>    memcpy(&buf[5], msg[2].buf, msg[0].len - 2); 
> >>> 
> >> 
> >> Yes, this is the same, except I kept the original behavior when write 
> >> len > 2.  Hence with my patch the I2C bus would only see a read 
> >> transaction.  With the above, you would write the first two bytes and 
> >> ignore the rest, then read.  This may be worse than just doing a read 
> >> because if a future tuner reg read setup/address is > 2 then you may 
> >> get into a strange situation.  If that case needs to be addressed, 
> >> then might as well get rid of the single write/read usb transaction 
> >> and just support write or read. 
> > 
> > 
> > Last else branch should do it - but no idea if it works at all and none of 
> > tuners are using it and it is very unlikely there will never be. 
> > 
> > It is easy to test, but I suspect if you write S Wr[11 11 12 13] P S Rw P it 
> > will return value from register 13 on a case chip supports writing multiple 
> > registers using reg address auto-increment as usually. 
> > 
>
> Point is the USB read command ignores anything in the memcpy, so you 
> cant write more than 2 bytes.  Your example results in S Wr 11 11 P S 
> Rd xx P.  My patch left this with previous behavior so it would just 
> do S Rd xx P. 
>
> > 
> >>>>> as hell. I should be re-written totally in any case. Those tuner I2C 
> >>>>> adapters should be moved to demod. Demod has 1 I2C adapter. USB-bridge 
> >>>>> has 2 
> >>>>> adapters, one for each demod. 
> >>>>> 
> >>>> 
> >>>> Agreed that it can be refactored and improved.  Also to support n 
> >>>> transactions with a simple while loop and only issuing single writes 
> >>>> and reads.  Only downside would be increased USB traffic for 2 
> >>>> commands vs 1 - hence negligible. 
> >>> 
> >>> 
> >>> 
> >>> there is i2c_adapter_quirks nowadays for these adapters which could do 
> >>> only 
> >>> limited set of commands. 
> >>> include/linux/i2c.h 
> >> 
> >> 
> >> Perhaps just supporting write or read can be done with: 
> >> 
> >> struct i2c_adapter_quirks just_rw = { 
> >> .flags=0, 
> >> .max_num_msgs=1, 
> >> .max_write_len=40, 
> >> .max_read_len=40, 
> >> }; 
> >> 
> >> Otherwise as is: 
> >> 
> >> struct i2c_adapter_quirks as_is = { 
> >> .flags=I2C_AQ_COMB_WRITE_THEN_READ, 
> >> .max_num_msgs=2, 
> >> .max_write_len=40, 
> >> .max_read_len=40, 
> >> .max_comb_1st_msg_len=2, 
> >> .max_comb_2nd_msg_len=40, 
> >> }; 
> >> 
> >>> 
> >>> In my understanding that is how those chips are wired: 
> >>> +---------------+     +--------+ 
> >>> | I2C adapter-1 | --> | eeprom | 
> >>> +---------------+     +--------+ 
> >>> +---------------+     +---------+     +---------+ 
> >>> | I2C adapter-2 | --> | demod-1 | --> | tuner-1 | 
> >>> +---------------+     +---------+     +---------+ 
> >>> +---------------+     +---------+     +---------+ 
> >>> | I2C adapter-3 | --> | demod-2 | --> | tuner-2 | 
> >>> +---------------+     +---------+     +---------+ 
> >>> 
> >> 
> >> I just have one demod, but as a clue, the address you provided to set 
> >> the tuner I2C speed is named like this in the OEM linux driver: 
> >> 
> >> p_reg_lnk2ofdm_data_63_56 
> >> 
> >>>>> I have to find out af9015 datasheets and check how it is there. But I 
> >>>>> still 
> >>>>> remember one case where I implemented one FX2 firmware and that same 
> >>>>> issues 
> >>>>> raises there as well. 
> >>>>> 
> >>>>>>> After that both af9015+mxl5007t and af9035+mxl5007t started working. 
> >>>>>>> Earlier 
> >>>>>>> both were returning bogus values for chip id read. 
> >>>>>>> 
> >>>>>>> Also I am interested to known which kind of communication there is 
> >>>>>>> actually 
> >>>>>>> seen on I2C bus? 
> >>>>>> 
> >>>>>> 
> >>>>>> 
> >>>>>> 
> >>>>>> With this or the patch I proposed, you see exactly what you expect on 
> >>>>>> the I2C bus with repeated stops, as detailed in my previous mails. 
> >>>>> 
> >>>>> 
> >>>>> 
> >>>>> 
> >>>>> So it is good? 
> >>>>> 
> >>>> 
> >>>> Yes, I2C looks good. 
> >>>> 
> >>>>>>> If it starts working then have to find out way to fix it properly so 
> >>>>>>> that 
> >>>>>>> any earlier device didn't broke. 
> >>>>>>> 
> >>>>>> 
> >>>>>> I hope that by now I've made abundantly clear that my mxl5007t locks 
> >>>>>> up after *any* read.  It doesn't matter if we are reading the correct 
> >>>>>> register after any of the proposed patches. 
> >>>>> 
> >>>>> 
> >>>>> 
> >>>>> 
> >>>>> So it still locks up after any read after the chip id read? And does 
> >>>>> not 
> >>>>> work then? On my devices I can add multiple mxl5007t_get_chip_id() 
> >>>>> calls 
> >>>>> and 
> >>>>> all are returning correct values. 
> >>>>> 
> >>>>41920172 
> >>>> No, as mentioned before, it locks up at the end of any read command. 
> >>>> Including the chip_id.  The firmware is not aware of the issue and 
> >>>> wont complain until the next I2C transaction. 
> >>> 
> >>> 
> >>> 
> >>> Maybe I2C speed is too fast? 
> >>> I tested with my device it failed when I increased speed to 850kHz. 
> >>> 640kHz 
> >>> was working. I am not sure which is default speed and driver didn't 
> >>> change 
> >>> it. Just try to dropping it to 142kHz (0x12). 
> >>> Speed is calculated using that formula (0x12 in that case is register 
> >>> value): 
> >>> octave:36> 1000000 / (24.4 * 16 * 0x12) 
> >>> ans =  142.304189435337 
> >>> 
> >>> These are related registers: 
> >>> /* I2C master bus 2 clock speed 300k */ 
> >>> ret = af9035_wr_reg(d, 0x00f6a7, 0x07); 
> >>> /* I2C master bus 1,3 clock speed 300k */ 
> >>> ret = af9035_wr_reg(d, 0x00f103, 0x07); 
> >>> 
> >>> Just add some good place before tuner attach like 
> >>> af9035_frontend_attach(). 
> >>> 
> >> 
> >> Found that the default value is 0x00 and results in ~97KHz SCL 
> >> frequency.  Tested up to 0x3C which I measured to ~42KHz, but the bus 
> >> still locks up.  Doesn't seem like speed is the problem. 
> >> 
> >>>>> Could you test what happens if you use that CMD_GENERIC_I2C_WR + 
> >>>>> CMD_GENERIC_I2C_RD ? I suspect it is lower level I2C xfer than those 
> >>>>> CMD_I2C_RD + CMD_I2C_WR, which are likely somehow handled by demod 
> >>>>> core. 
> >>>>> 
> >>>> 
> >>>> I will test, but the issue is either electrical or with the state of 
> >>>> the mxl5007t.  I2C bus looks good from AF9035 side once the bug in the 
> >>>> above is patched. 
> >>> 
> >>> 
> >>> 
> >>> If dropping I2C speed does not help then I cannot imagine any other fix 
> >>> than 
> >>> adding mxl5007t driver option which disables problematic reads *or* add 
> >>> some 
> >>> hack to af9035 i2c adapter implementation which fakes required 
> >>> problematic 
> >>> commands ones that looks "good". 
> >>> 
> >> 
> >> Unless there is a specific state in which the mxl5007t must be in for 
> >> you to issue a read, I really don't know what else could be wrong. 
> >> Would be nice to know if this issue happens with other demods to 
> >> further justify the "no_probe" fix in the mxl5007t driver. 
> > 
> > 
> > For me it works even device is ~same. It could be just some hw issues, too 
> > noisy bus or like that. Maybe different PCB revision. 
> > 
> > My device ID is 07ca:0337, yours different. I think it is best add some 
> > quirk to af9035 i2c-adapter that it looks USB ID and returns fake values to 
> > mxl5007t driver in order to work-around issue. As thumb of rule all device 
> > specifics hacks should be added to interface driver leaving chip drivers 
> > hack free. So add some glue there and that's it. I cannot discover any 
> > better fix currently. 
> > 
>
> Indeed mine is 07ca:a867, will propose a patch. 
> -- 
> To unsubscribe from this list: send the line "unsubscribe linux-media" in 
> the body of a message to majordomo@vger.kernel.org 
> More majordomo info at  http://vger.kernel.org/majordomo-info.html 

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: AVerMedia HD Volar (A867) AF9035 + MXL5007T driver issues
  2016-04-09 17:07                 ` Antti Palosaari
@ 2016-04-09 17:38                   ` Alessandro Radicati
  0 siblings, 0 replies; 18+ messages in thread
From: Alessandro Radicati @ 2016-04-09 17:38 UTC (permalink / raw)
  To: Antti Palosaari; +Cc: Jose Alberto Reguero, linux-media

On Sat, Apr 9, 2016 at 7:07 PM, Antti Palosaari <crope@iki.fi> wrote:
>
>
> On 04/09/2016 07:11 PM, Alessandro Radicati wrote:
>>
>> On Sat, Apr 9, 2016 at 4:25 PM, Antti Palosaari <crope@iki.fi> wrote:
>>>
>>> On 04/09/2016 11:13 AM, Alessandro Radicati wrote:
>>>>
>>>>
>>>> On Sat, Apr 9, 2016 at 4:17 AM, Antti Palosaari <crope@iki.fi> wrote:
>>>>>
>>>>>
>>>>> On 04/09/2016 04:52 AM, Alessandro Radicati wrote:
>>>>>>
>>>>>>
>>>>>>
>>>>>> On Sat, Apr 9, 2016 at 3:22 AM, Antti Palosaari <crope@iki.fi> wrote:
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> Here is patches to test:
>>>>>>> http://git.linuxtv.org/anttip/media_tree.git/log/?h=af9035
>>>>>>>
>>>>>>
>>>>>> I've done this already in my testing, and it works for getting a
>>>>>> correct chip_id response, but only because it's avoiding the issue
>>>>>> with the write/read case in the af9035 driver.  Don't have an
>>>>>> af9015... perhaps there's a similar issue with that code or we are
>>>>>> dealing with two separate issues since af9035 never does a repeated
>>>>>> start?
>>>>>
>>>>>
>>>>>
>>>>>
>>>>> I am pretty sure mxl5007t requires stop between read and write. Usually
>>>>> chips are not caring too much if it is repeated start or not, whilst
>>>>> datasheets are often register read is S Wr S Rw P.
>>>>>
>>>>> Even af9035 i2c adapter implementation implements repeated start wrong,
>>>>> I
>>>>
>>>>
>>>>
>>>> Where does the assumption that CMD_I2C_RD should issue a repeated
>>>> start sequence come from?  From the datasheet?  Maybe it was never
>>>> intended as repeated start.  Perhaps if there is another stick  with
>>>> mxl5007t and a chip that does repeated start, we can put this to bed.
>>>
>>>
>>>
>>> Assumption was coming from it just does it as a single USB transaction.
>>> Datasheet says there is no repeated start. And kernel I2C API says all
>>> messages send using single i2c_transfer() should be send with repeated
>>> start, so now it is violating it, but that's not the biggest problem...
>>>
>>
>> Unfortunately there is no way around that problem, but at least it
>> means that you can reduce the whole function to just read and write
>> since at the I2C level nothing changes.
>>
>>>>> would not like to add anymore hacks there. It is currently ugly and
>>>>> complex
>>>>
>>>>
>>>>
>>>> Bugfix != hack.  Don't see how putting the register address in the
>>>> address fields is a hack (perhaps semantics around the fact that 0xFB
>>>> is not really part of the address?); this is the only and intended way
>>>> to use that command for write/read.
>>>
>>>
>>>
>>> I did bunch of testing and find it is really wrong. Dumped out registers
>>> from some tuner chips and those seems to be mostly off by one.
>>>
>>> I think that skeleton is correct way (and it ends about same you did)
>>> if (msg[0].len == 0) // probe message, payload 0
>>>    buf[0] = msg[0].len;
>>>    buf[1] = msg[0].addr << 1;
>>>    buf[2] = 0x00; /* reg addr len */
>>>    buf[3] = 0x00; /* reg addr MSB */
>>>    buf[4] = 0x00; /* reg addr LSB */
>>> else if (msg[0].len == 1)
>>>    buf[0] = msg[0].len;
>>>    buf[1] = msg[0].addr << 1;
>>>    buf[2] = 1; /* reg addr len */
>>>    buf[3] = 0x00; /* reg addr MSB */
>>>    buf[4] = msg[0].buf[0]; /* reg addr LSB */
>>> else if (msg[0].len == 2)
>>>    buf[0] = msg[0].len;
>>>    buf[1] = msg[0].addr << 1;
>>>    buf[2] = 2; /* reg addr len */
>>>    buf[3] = msg[0].buf[0]; /* reg addr MSB */
>>>    buf[4] = msg[0].buf[1]; /* reg addr LSB */
>>> else
>>>    buf[0] = msg[0].len;
>>>    buf[1] = msg[0].addr << 1;
>>>    buf[2] = 2; /* reg addr len */
>>>    buf[3] = msg[0].buf[0]; /* reg addr MSB */
>>>    buf[4] = msg[0].buf[1]; /* reg addr LSB */
>>>    memcpy(&buf[5], msg[2].buf, msg[0].len - 2);
>>>
>>
>> Yes, this is the same, except I kept the original behavior when write
>> len > 2.  Hence with my patch the I2C bus would only see a read
>> transaction.  With the above, you would write the first two bytes and
>> ignore the rest, then read.  This may be worse than just doing a read
>> because if a future tuner reg read setup/address is > 2 then you may
>> get into a strange situation.  If that case needs to be addressed,
>> then might as well get rid of the single write/read usb transaction
>> and just support write or read.
>
>
> Last else branch should do it - but no idea if it works at all and none of
> tuners are using it and it is very unlikely there will never be.
>
> It is easy to test, but I suspect if you write S Wr[11 11 12 13] P S Rw P it
> will return value from register 13 on a case chip supports writing multiple
> registers using reg address auto-increment as usually.
>

Point is the USB read command ignores anything in the memcpy, so you
cant write more than 2 bytes.  Your example results in S Wr 11 11 P S
Rd xx P.  My patch left this with previous behavior so it would just
do S Rd xx P.

>
>>>>> as hell. I should be re-written totally in any case. Those tuner I2C
>>>>> adapters should be moved to demod. Demod has 1 I2C adapter. USB-bridge
>>>>> has 2
>>>>> adapters, one for each demod.
>>>>>
>>>>
>>>> Agreed that it can be refactored and improved.  Also to support n
>>>> transactions with a simple while loop and only issuing single writes
>>>> and reads.  Only downside would be increased USB traffic for 2
>>>> commands vs 1 - hence negligible.
>>>
>>>
>>>
>>> there is i2c_adapter_quirks nowadays for these adapters which could do
>>> only
>>> limited set of commands.
>>> include/linux/i2c.h
>>
>>
>> Perhaps just supporting write or read can be done with:
>>
>> struct i2c_adapter_quirks just_rw = {
>> .flags=0,
>> .max_num_msgs=1,
>> .max_write_len=40,
>> .max_read_len=40,
>> };
>>
>> Otherwise as is:
>>
>> struct i2c_adapter_quirks as_is = {
>> .flags=I2C_AQ_COMB_WRITE_THEN_READ,
>> .max_num_msgs=2,
>> .max_write_len=40,
>> .max_read_len=40,
>> .max_comb_1st_msg_len=2,
>> .max_comb_2nd_msg_len=40,
>> };
>>
>>>
>>> In my understanding that is how those chips are wired:
>>> +---------------+     +--------+
>>> | I2C adapter-1 | --> | eeprom |
>>> +---------------+     +--------+
>>> +---------------+     +---------+     +---------+
>>> | I2C adapter-2 | --> | demod-1 | --> | tuner-1 |
>>> +---------------+     +---------+     +---------+
>>> +---------------+     +---------+     +---------+
>>> | I2C adapter-3 | --> | demod-2 | --> | tuner-2 |
>>> +---------------+     +---------+     +---------+
>>>
>>
>> I just have one demod, but as a clue, the address you provided to set
>> the tuner I2C speed is named like this in the OEM linux driver:
>>
>> p_reg_lnk2ofdm_data_63_56
>>
>>>>> I have to find out af9015 datasheets and check how it is there. But I
>>>>> still
>>>>> remember one case where I implemented one FX2 firmware and that same
>>>>> issues
>>>>> raises there as well.
>>>>>
>>>>>>> After that both af9015+mxl5007t and af9035+mxl5007t started working.
>>>>>>> Earlier
>>>>>>> both were returning bogus values for chip id read.
>>>>>>>
>>>>>>> Also I am interested to known which kind of communication there is
>>>>>>> actually
>>>>>>> seen on I2C bus?
>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>> With this or the patch I proposed, you see exactly what you expect on
>>>>>> the I2C bus with repeated stops, as detailed in my previous mails.
>>>>>
>>>>>
>>>>>
>>>>>
>>>>> So it is good?
>>>>>
>>>>
>>>> Yes, I2C looks good.
>>>>
>>>>>>> If it starts working then have to find out way to fix it properly so
>>>>>>> that
>>>>>>> any earlier device didn't broke.
>>>>>>>
>>>>>>
>>>>>> I hope that by now I've made abundantly clear that my mxl5007t locks
>>>>>> up after *any* read.  It doesn't matter if we are reading the correct
>>>>>> register after any of the proposed patches.
>>>>>
>>>>>
>>>>>
>>>>>
>>>>> So it still locks up after any read after the chip id read? And does
>>>>> not
>>>>> work then? On my devices I can add multiple mxl5007t_get_chip_id()
>>>>> calls
>>>>> and
>>>>> all are returning correct values.
>>>>>
>>>>41920172
>>>> No, as mentioned before, it locks up at the end of any read command.
>>>> Including the chip_id.  The firmware is not aware of the issue and
>>>> wont complain until the next I2C transaction.
>>>
>>>
>>>
>>> Maybe I2C speed is too fast?
>>> I tested with my device it failed when I increased speed to 850kHz.
>>> 640kHz
>>> was working. I am not sure which is default speed and driver didn't
>>> change
>>> it. Just try to dropping it to 142kHz (0x12).
>>> Speed is calculated using that formula (0x12 in that case is register
>>> value):
>>> octave:36> 1000000 / (24.4 * 16 * 0x12)
>>> ans =  142.304189435337
>>>
>>> These are related registers:
>>> /* I2C master bus 2 clock speed 300k */
>>> ret = af9035_wr_reg(d, 0x00f6a7, 0x07);
>>> /* I2C master bus 1,3 clock speed 300k */
>>> ret = af9035_wr_reg(d, 0x00f103, 0x07);
>>>
>>> Just add some good place before tuner attach like
>>> af9035_frontend_attach().
>>>
>>
>> Found that the default value is 0x00 and results in ~97KHz SCL
>> frequency.  Tested up to 0x3C which I measured to ~42KHz, but the bus
>> still locks up.  Doesn't seem like speed is the problem.
>>
>>>>> Could you test what happens if you use that CMD_GENERIC_I2C_WR +
>>>>> CMD_GENERIC_I2C_RD ? I suspect it is lower level I2C xfer than those
>>>>> CMD_I2C_RD + CMD_I2C_WR, which are likely somehow handled by demod
>>>>> core.
>>>>>
>>>>
>>>> I will test, but the issue is either electrical or with the state of
>>>> the mxl5007t.  I2C bus looks good from AF9035 side once the bug in the
>>>> above is patched.
>>>
>>>
>>>
>>> If dropping I2C speed does not help then I cannot imagine any other fix
>>> than
>>> adding mxl5007t driver option which disables problematic reads *or* add
>>> some
>>> hack to af9035 i2c adapter implementation which fakes required
>>> problematic
>>> commands ones that looks "good".
>>>
>>
>> Unless there is a specific state in which the mxl5007t must be in for
>> you to issue a read, I really don't know what else could be wrong.
>> Would be nice to know if this issue happens with other demods to
>> further justify the "no_probe" fix in the mxl5007t driver.
>
>
> For me it works even device is ~same. It could be just some hw issues, too
> noisy bus or like that. Maybe different PCB revision.
>
> My device ID is 07ca:0337, yours different. I think it is best add some
> quirk to af9035 i2c-adapter that it looks USB ID and returns fake values to
> mxl5007t driver in order to work-around issue. As thumb of rule all device
> specifics hacks should be added to interface driver leaving chip drivers
> hack free. So add some glue there and that's it. I cannot discover any
> better fix currently.
>

Indeed mine is 07ca:a867, will propose a patch.

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: AVerMedia HD Volar (A867) AF9035 + MXL5007T driver issues
  2016-04-09 16:11               ` Alessandro Radicati
@ 2016-04-09 17:07                 ` Antti Palosaari
  2016-04-09 17:38                   ` Alessandro Radicati
  0 siblings, 1 reply; 18+ messages in thread
From: Antti Palosaari @ 2016-04-09 17:07 UTC (permalink / raw)
  To: Alessandro Radicati; +Cc: Jose Alberto Reguero, linux-media



On 04/09/2016 07:11 PM, Alessandro Radicati wrote:
> On Sat, Apr 9, 2016 at 4:25 PM, Antti Palosaari <crope@iki.fi> wrote:
>> On 04/09/2016 11:13 AM, Alessandro Radicati wrote:
>>>
>>> On Sat, Apr 9, 2016 at 4:17 AM, Antti Palosaari <crope@iki.fi> wrote:
>>>>
>>>> On 04/09/2016 04:52 AM, Alessandro Radicati wrote:
>>>>>
>>>>>
>>>>> On Sat, Apr 9, 2016 at 3:22 AM, Antti Palosaari <crope@iki.fi> wrote:
>>>>>>
>>>>>>
>>>>>> Here is patches to test:
>>>>>> http://git.linuxtv.org/anttip/media_tree.git/log/?h=af9035
>>>>>>
>>>>>
>>>>> I've done this already in my testing, and it works for getting a
>>>>> correct chip_id response, but only because it's avoiding the issue
>>>>> with the write/read case in the af9035 driver.  Don't have an
>>>>> af9015... perhaps there's a similar issue with that code or we are
>>>>> dealing with two separate issues since af9035 never does a repeated
>>>>> start?
>>>>
>>>>
>>>>
>>>> I am pretty sure mxl5007t requires stop between read and write. Usually
>>>> chips are not caring too much if it is repeated start or not, whilst
>>>> datasheets are often register read is S Wr S Rw P.
>>>>
>>>> Even af9035 i2c adapter implementation implements repeated start wrong, I
>>>
>>>
>>> Where does the assumption that CMD_I2C_RD should issue a repeated
>>> start sequence come from?  From the datasheet?  Maybe it was never
>>> intended as repeated start.  Perhaps if there is another stick  with
>>> mxl5007t and a chip that does repeated start, we can put this to bed.
>>
>>
>> Assumption was coming from it just does it as a single USB transaction.
>> Datasheet says there is no repeated start. And kernel I2C API says all
>> messages send using single i2c_transfer() should be send with repeated
>> start, so now it is violating it, but that's not the biggest problem...
>>
>
> Unfortunately there is no way around that problem, but at least it
> means that you can reduce the whole function to just read and write
> since at the I2C level nothing changes.
>
>>>> would not like to add anymore hacks there. It is currently ugly and
>>>> complex
>>>
>>>
>>> Bugfix != hack.  Don't see how putting the register address in the
>>> address fields is a hack (perhaps semantics around the fact that 0xFB
>>> is not really part of the address?); this is the only and intended way
>>> to use that command for write/read.
>>
>>
>> I did bunch of testing and find it is really wrong. Dumped out registers
>> from some tuner chips and those seems to be mostly off by one.
>>
>> I think that skeleton is correct way (and it ends about same you did)
>> if (msg[0].len == 0) // probe message, payload 0
>>    buf[0] = msg[0].len;
>>    buf[1] = msg[0].addr << 1;
>>    buf[2] = 0x00; /* reg addr len */
>>    buf[3] = 0x00; /* reg addr MSB */
>>    buf[4] = 0x00; /* reg addr LSB */
>> else if (msg[0].len == 1)
>>    buf[0] = msg[0].len;
>>    buf[1] = msg[0].addr << 1;
>>    buf[2] = 1; /* reg addr len */
>>    buf[3] = 0x00; /* reg addr MSB */
>>    buf[4] = msg[0].buf[0]; /* reg addr LSB */
>> else if (msg[0].len == 2)
>>    buf[0] = msg[0].len;
>>    buf[1] = msg[0].addr << 1;
>>    buf[2] = 2; /* reg addr len */
>>    buf[3] = msg[0].buf[0]; /* reg addr MSB */
>>    buf[4] = msg[0].buf[1]; /* reg addr LSB */
>> else
>>    buf[0] = msg[0].len;
>>    buf[1] = msg[0].addr << 1;
>>    buf[2] = 2; /* reg addr len */
>>    buf[3] = msg[0].buf[0]; /* reg addr MSB */
>>    buf[4] = msg[0].buf[1]; /* reg addr LSB */
>>    memcpy(&buf[5], msg[2].buf, msg[0].len - 2);
>>
>
> Yes, this is the same, except I kept the original behavior when write
> len > 2.  Hence with my patch the I2C bus would only see a read
> transaction.  With the above, you would write the first two bytes and
> ignore the rest, then read.  This may be worse than just doing a read
> because if a future tuner reg read setup/address is > 2 then you may
> get into a strange situation.  If that case needs to be addressed,
> then might as well get rid of the single write/read usb transaction
> and just support write or read.

Last else branch should do it - but no idea if it works at all and none 
of tuners are using it and it is very unlikely there will never be.

It is easy to test, but I suspect if you write S Wr[11 11 12 13] P S Rw 
P it will return value from register 13 on a case chip supports writing 
multiple registers using reg address auto-increment as usually.

>>>> as hell. I should be re-written totally in any case. Those tuner I2C
>>>> adapters should be moved to demod. Demod has 1 I2C adapter. USB-bridge
>>>> has 2
>>>> adapters, one for each demod.
>>>>
>>>
>>> Agreed that it can be refactored and improved.  Also to support n
>>> transactions with a simple while loop and only issuing single writes
>>> and reads.  Only downside would be increased USB traffic for 2
>>> commands vs 1 - hence negligible.
>>
>>
>> there is i2c_adapter_quirks nowadays for these adapters which could do only
>> limited set of commands.
>> include/linux/i2c.h
>
> Perhaps just supporting write or read can be done with:
>
> struct i2c_adapter_quirks just_rw = {
> .flags=0,
> .max_num_msgs=1,
> .max_write_len=40,
> .max_read_len=40,
> };
>
> Otherwise as is:
>
> struct i2c_adapter_quirks as_is = {
> .flags=I2C_AQ_COMB_WRITE_THEN_READ,
> .max_num_msgs=2,
> .max_write_len=40,
> .max_read_len=40,
> .max_comb_1st_msg_len=2,
> .max_comb_2nd_msg_len=40,
> };
>
>>
>> In my understanding that is how those chips are wired:
>> +---------------+     +--------+
>> | I2C adapter-1 | --> | eeprom |
>> +---------------+     +--------+
>> +---------------+     +---------+     +---------+
>> | I2C adapter-2 | --> | demod-1 | --> | tuner-1 |
>> +---------------+     +---------+     +---------+
>> +---------------+     +---------+     +---------+
>> | I2C adapter-3 | --> | demod-2 | --> | tuner-2 |
>> +---------------+     +---------+     +---------+
>>
>
> I just have one demod, but as a clue, the address you provided to set
> the tuner I2C speed is named like this in the OEM linux driver:
>
> p_reg_lnk2ofdm_data_63_56
>
>>>> I have to find out af9015 datasheets and check how it is there. But I
>>>> still
>>>> remember one case where I implemented one FX2 firmware and that same
>>>> issues
>>>> raises there as well.
>>>>
>>>>>> After that both af9015+mxl5007t and af9035+mxl5007t started working.
>>>>>> Earlier
>>>>>> both were returning bogus values for chip id read.
>>>>>>
>>>>>> Also I am interested to known which kind of communication there is
>>>>>> actually
>>>>>> seen on I2C bus?
>>>>>
>>>>>
>>>>>
>>>>> With this or the patch I proposed, you see exactly what you expect on
>>>>> the I2C bus with repeated stops, as detailed in my previous mails.
>>>>
>>>>
>>>>
>>>> So it is good?
>>>>
>>>
>>> Yes, I2C looks good.
>>>
>>>>>> If it starts working then have to find out way to fix it properly so
>>>>>> that
>>>>>> any earlier device didn't broke.
>>>>>>
>>>>>
>>>>> I hope that by now I've made abundantly clear that my mxl5007t locks
>>>>> up after *any* read.  It doesn't matter if we are reading the correct
>>>>> register after any of the proposed patches.
>>>>
>>>>
>>>>
>>>> So it still locks up after any read after the chip id read? And does not
>>>> work then? On my devices I can add multiple mxl5007t_get_chip_id() calls
>>>> and
>>>> all are returning correct values.
>>>>
>>>
>>> No, as mentioned before, it locks up at the end of any read command.
>>> Including the chip_id.  The firmware is not aware of the issue and
>>> wont complain until the next I2C transaction.
>>
>>
>> Maybe I2C speed is too fast?
>> I tested with my device it failed when I increased speed to 850kHz. 640kHz
>> was working. I am not sure which is default speed and driver didn't change
>> it. Just try to dropping it to 142kHz (0x12).
>> Speed is calculated using that formula (0x12 in that case is register
>> value):
>> octave:36> 1000000 / (24.4 * 16 * 0x12)
>> ans =  142.304189435337
>>
>> These are related registers:
>> /* I2C master bus 2 clock speed 300k */
>> ret = af9035_wr_reg(d, 0x00f6a7, 0x07);
>> /* I2C master bus 1,3 clock speed 300k */
>> ret = af9035_wr_reg(d, 0x00f103, 0x07);
>>
>> Just add some good place before tuner attach like af9035_frontend_attach().
>>
>
> Found that the default value is 0x00 and results in ~97KHz SCL
> frequency.  Tested up to 0x3C which I measured to ~42KHz, but the bus
> still locks up.  Doesn't seem like speed is the problem.
>
>>>> Could you test what happens if you use that CMD_GENERIC_I2C_WR +
>>>> CMD_GENERIC_I2C_RD ? I suspect it is lower level I2C xfer than those
>>>> CMD_I2C_RD + CMD_I2C_WR, which are likely somehow handled by demod core.
>>>>
>>>
>>> I will test, but the issue is either electrical or with the state of
>>> the mxl5007t.  I2C bus looks good from AF9035 side once the bug in the
>>> above is patched.
>>
>>
>> If dropping I2C speed does not help then I cannot imagine any other fix than
>> adding mxl5007t driver option which disables problematic reads *or* add some
>> hack to af9035 i2c adapter implementation which fakes required problematic
>> commands ones that looks "good".
>>
>
> Unless there is a specific state in which the mxl5007t must be in for
> you to issue a read, I really don't know what else could be wrong.
> Would be nice to know if this issue happens with other demods to
> further justify the "no_probe" fix in the mxl5007t driver.

For me it works even device is ~same. It could be just some hw issues, 
too noisy bus or like that. Maybe different PCB revision.

My device ID is 07ca:0337, yours different. I think it is best add some 
quirk to af9035 i2c-adapter that it looks USB ID and returns fake values 
to mxl5007t driver in order to work-around issue. As thumb of rule all 
device specifics hacks should be added to interface driver leaving chip 
drivers hack free. So add some glue there and that's it. I cannot 
discover any better fix currently.

regards
Antti

-- 
http://palosaari.fi/

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: AVerMedia HD Volar (A867) AF9035 + MXL5007T driver issues
  2016-04-09 14:25             ` Antti Palosaari
@ 2016-04-09 16:11               ` Alessandro Radicati
  2016-04-09 17:07                 ` Antti Palosaari
  0 siblings, 1 reply; 18+ messages in thread
From: Alessandro Radicati @ 2016-04-09 16:11 UTC (permalink / raw)
  To: Antti Palosaari; +Cc: Jose Alberto Reguero, linux-media

On Sat, Apr 9, 2016 at 4:25 PM, Antti Palosaari <crope@iki.fi> wrote:
> On 04/09/2016 11:13 AM, Alessandro Radicati wrote:
>>
>> On Sat, Apr 9, 2016 at 4:17 AM, Antti Palosaari <crope@iki.fi> wrote:
>>>
>>> On 04/09/2016 04:52 AM, Alessandro Radicati wrote:
>>>>
>>>>
>>>> On Sat, Apr 9, 2016 at 3:22 AM, Antti Palosaari <crope@iki.fi> wrote:
>>>>>
>>>>>
>>>>> Here is patches to test:
>>>>> http://git.linuxtv.org/anttip/media_tree.git/log/?h=af9035
>>>>>
>>>>
>>>> I've done this already in my testing, and it works for getting a
>>>> correct chip_id response, but only because it's avoiding the issue
>>>> with the write/read case in the af9035 driver.  Don't have an
>>>> af9015... perhaps there's a similar issue with that code or we are
>>>> dealing with two separate issues since af9035 never does a repeated
>>>> start?
>>>
>>>
>>>
>>> I am pretty sure mxl5007t requires stop between read and write. Usually
>>> chips are not caring too much if it is repeated start or not, whilst
>>> datasheets are often register read is S Wr S Rw P.
>>>
>>> Even af9035 i2c adapter implementation implements repeated start wrong, I
>>
>>
>> Where does the assumption that CMD_I2C_RD should issue a repeated
>> start sequence come from?  From the datasheet?  Maybe it was never
>> intended as repeated start.  Perhaps if there is another stick  with
>> mxl5007t and a chip that does repeated start, we can put this to bed.
>
>
> Assumption was coming from it just does it as a single USB transaction.
> Datasheet says there is no repeated start. And kernel I2C API says all
> messages send using single i2c_transfer() should be send with repeated
> start, so now it is violating it, but that's not the biggest problem...
>

Unfortunately there is no way around that problem, but at least it
means that you can reduce the whole function to just read and write
since at the I2C level nothing changes.

>>> would not like to add anymore hacks there. It is currently ugly and
>>> complex
>>
>>
>> Bugfix != hack.  Don't see how putting the register address in the
>> address fields is a hack (perhaps semantics around the fact that 0xFB
>> is not really part of the address?); this is the only and intended way
>> to use that command for write/read.
>
>
> I did bunch of testing and find it is really wrong. Dumped out registers
> from some tuner chips and those seems to be mostly off by one.
>
> I think that skeleton is correct way (and it ends about same you did)
> if (msg[0].len == 0) // probe message, payload 0
>   buf[0] = msg[0].len;
>   buf[1] = msg[0].addr << 1;
>   buf[2] = 0x00; /* reg addr len */
>   buf[3] = 0x00; /* reg addr MSB */
>   buf[4] = 0x00; /* reg addr LSB */
> else if (msg[0].len == 1)
>   buf[0] = msg[0].len;
>   buf[1] = msg[0].addr << 1;
>   buf[2] = 1; /* reg addr len */
>   buf[3] = 0x00; /* reg addr MSB */
>   buf[4] = msg[0].buf[0]; /* reg addr LSB */
> else if (msg[0].len == 2)
>   buf[0] = msg[0].len;
>   buf[1] = msg[0].addr << 1;
>   buf[2] = 2; /* reg addr len */
>   buf[3] = msg[0].buf[0]; /* reg addr MSB */
>   buf[4] = msg[0].buf[1]; /* reg addr LSB */
> else
>   buf[0] = msg[0].len;
>   buf[1] = msg[0].addr << 1;
>   buf[2] = 2; /* reg addr len */
>   buf[3] = msg[0].buf[0]; /* reg addr MSB */
>   buf[4] = msg[0].buf[1]; /* reg addr LSB */
>   memcpy(&buf[5], msg[2].buf, msg[0].len - 2);
>

Yes, this is the same, except I kept the original behavior when write
len > 2.  Hence with my patch the I2C bus would only see a read
transaction.  With the above, you would write the first two bytes and
ignore the rest, then read.  This may be worse than just doing a read
because if a future tuner reg read setup/address is > 2 then you may
get into a strange situation.  If that case needs to be addressed,
then might as well get rid of the single write/read usb transaction
and just support write or read.

>
>>> as hell. I should be re-written totally in any case. Those tuner I2C
>>> adapters should be moved to demod. Demod has 1 I2C adapter. USB-bridge
>>> has 2
>>> adapters, one for each demod.
>>>
>>
>> Agreed that it can be refactored and improved.  Also to support n
>> transactions with a simple while loop and only issuing single writes
>> and reads.  Only downside would be increased USB traffic for 2
>> commands vs 1 - hence negligible.
>
>
> there is i2c_adapter_quirks nowadays for these adapters which could do only
> limited set of commands.
> include/linux/i2c.h

Perhaps just supporting write or read can be done with:

struct i2c_adapter_quirks just_rw = {
.flags=0,
.max_num_msgs=1,
.max_write_len=40,
.max_read_len=40,
};

Otherwise as is:

struct i2c_adapter_quirks as_is = {
.flags=I2C_AQ_COMB_WRITE_THEN_READ,
.max_num_msgs=2,
.max_write_len=40,
.max_read_len=40,
.max_comb_1st_msg_len=2,
.max_comb_2nd_msg_len=40,
};

>
> In my understanding that is how those chips are wired:
> +---------------+     +--------+
> | I2C adapter-1 | --> | eeprom |
> +---------------+     +--------+
> +---------------+     +---------+     +---------+
> | I2C adapter-2 | --> | demod-1 | --> | tuner-1 |
> +---------------+     +---------+     +---------+
> +---------------+     +---------+     +---------+
> | I2C adapter-3 | --> | demod-2 | --> | tuner-2 |
> +---------------+     +---------+     +---------+
>

I just have one demod, but as a clue, the address you provided to set
the tuner I2C speed is named like this in the OEM linux driver:

p_reg_lnk2ofdm_data_63_56

>>> I have to find out af9015 datasheets and check how it is there. But I
>>> still
>>> remember one case where I implemented one FX2 firmware and that same
>>> issues
>>> raises there as well.
>>>
>>>>> After that both af9015+mxl5007t and af9035+mxl5007t started working.
>>>>> Earlier
>>>>> both were returning bogus values for chip id read.
>>>>>
>>>>> Also I am interested to known which kind of communication there is
>>>>> actually
>>>>> seen on I2C bus?
>>>>
>>>>
>>>>
>>>> With this or the patch I proposed, you see exactly what you expect on
>>>> the I2C bus with repeated stops, as detailed in my previous mails.
>>>
>>>
>>>
>>> So it is good?
>>>
>>
>> Yes, I2C looks good.
>>
>>>>> If it starts working then have to find out way to fix it properly so
>>>>> that
>>>>> any earlier device didn't broke.
>>>>>
>>>>
>>>> I hope that by now I've made abundantly clear that my mxl5007t locks
>>>> up after *any* read.  It doesn't matter if we are reading the correct
>>>> register after any of the proposed patches.
>>>
>>>
>>>
>>> So it still locks up after any read after the chip id read? And does not
>>> work then? On my devices I can add multiple mxl5007t_get_chip_id() calls
>>> and
>>> all are returning correct values.
>>>
>>
>> No, as mentioned before, it locks up at the end of any read command.
>> Including the chip_id.  The firmware is not aware of the issue and
>> wont complain until the next I2C transaction.
>
>
> Maybe I2C speed is too fast?
> I tested with my device it failed when I increased speed to 850kHz. 640kHz
> was working. I am not sure which is default speed and driver didn't change
> it. Just try to dropping it to 142kHz (0x12).
> Speed is calculated using that formula (0x12 in that case is register
> value):
> octave:36> 1000000 / (24.4 * 16 * 0x12)
> ans =  142.304189435337
>
> These are related registers:
> /* I2C master bus 2 clock speed 300k */
> ret = af9035_wr_reg(d, 0x00f6a7, 0x07);
> /* I2C master bus 1,3 clock speed 300k */
> ret = af9035_wr_reg(d, 0x00f103, 0x07);
>
> Just add some good place before tuner attach like af9035_frontend_attach().
>

Found that the default value is 0x00 and results in ~97KHz SCL
frequency.  Tested up to 0x3C which I measured to ~42KHz, but the bus
still locks up.  Doesn't seem like speed is the problem.

>>> Could you test what happens if you use that CMD_GENERIC_I2C_WR +
>>> CMD_GENERIC_I2C_RD ? I suspect it is lower level I2C xfer than those
>>> CMD_I2C_RD + CMD_I2C_WR, which are likely somehow handled by demod core.
>>>
>>
>> I will test, but the issue is either electrical or with the state of
>> the mxl5007t.  I2C bus looks good from AF9035 side once the bug in the
>> above is patched.
>
>
> If dropping I2C speed does not help then I cannot imagine any other fix than
> adding mxl5007t driver option which disables problematic reads *or* add some
> hack to af9035 i2c adapter implementation which fakes required problematic
> commands ones that looks "good".
>

Unless there is a specific state in which the mxl5007t must be in for
you to issue a read, I really don't know what else could be wrong.
Would be nice to know if this issue happens with other demods to
further justify the "no_probe" fix in the mxl5007t driver.

Regards,
Alessandro

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: AVerMedia HD Volar (A867) AF9035 + MXL5007T driver issues
  2016-04-09  8:13           ` Alessandro Radicati
@ 2016-04-09 14:25             ` Antti Palosaari
  2016-04-09 16:11               ` Alessandro Radicati
  0 siblings, 1 reply; 18+ messages in thread
From: Antti Palosaari @ 2016-04-09 14:25 UTC (permalink / raw)
  To: Alessandro Radicati; +Cc: Jose Alberto Reguero, linux-media

On 04/09/2016 11:13 AM, Alessandro Radicati wrote:
> On Sat, Apr 9, 2016 at 4:17 AM, Antti Palosaari <crope@iki.fi> wrote:
>> On 04/09/2016 04:52 AM, Alessandro Radicati wrote:
>>>
>>> On Sat, Apr 9, 2016 at 3:22 AM, Antti Palosaari <crope@iki.fi> wrote:
>>>>
>>>> Here is patches to test:
>>>> http://git.linuxtv.org/anttip/media_tree.git/log/?h=af9035
>>>>
>>>
>>> I've done this already in my testing, and it works for getting a
>>> correct chip_id response, but only because it's avoiding the issue
>>> with the write/read case in the af9035 driver.  Don't have an
>>> af9015... perhaps there's a similar issue with that code or we are
>>> dealing with two separate issues since af9035 never does a repeated
>>> start?
>>
>>
>> I am pretty sure mxl5007t requires stop between read and write. Usually
>> chips are not caring too much if it is repeated start or not, whilst
>> datasheets are often register read is S Wr S Rw P.
>>
>> Even af9035 i2c adapter implementation implements repeated start wrong, I
>
> Where does the assumption that CMD_I2C_RD should issue a repeated
> start sequence come from?  From the datasheet?  Maybe it was never
> intended as repeated start.  Perhaps if there is another stick  with
> mxl5007t and a chip that does repeated start, we can put this to bed.

Assumption was coming from it just does it as a single USB transaction. 
Datasheet says there is no repeated start. And kernel I2C API says all 
messages send using single i2c_transfer() should be send with repeated 
start, so now it is violating it, but that's not the biggest problem...

>> would not like to add anymore hacks there. It is currently ugly and complex
>
> Bugfix != hack.  Don't see how putting the register address in the
> address fields is a hack (perhaps semantics around the fact that 0xFB
> is not really part of the address?); this is the only and intended way
> to use that command for write/read.

I did bunch of testing and find it is really wrong. Dumped out registers 
from some tuner chips and those seems to be mostly off by one.

I think that skeleton is correct way (and it ends about same you did)
if (msg[0].len == 0) // probe message, payload 0
   buf[0] = msg[0].len;
   buf[1] = msg[0].addr << 1;
   buf[2] = 0x00; /* reg addr len */
   buf[3] = 0x00; /* reg addr MSB */
   buf[4] = 0x00; /* reg addr LSB */
else if (msg[0].len == 1)
   buf[0] = msg[0].len;
   buf[1] = msg[0].addr << 1;
   buf[2] = 1; /* reg addr len */
   buf[3] = 0x00; /* reg addr MSB */
   buf[4] = msg[0].buf[0]; /* reg addr LSB */
else if (msg[0].len == 2)
   buf[0] = msg[0].len;
   buf[1] = msg[0].addr << 1;
   buf[2] = 2; /* reg addr len */
   buf[3] = msg[0].buf[0]; /* reg addr MSB */
   buf[4] = msg[0].buf[1]; /* reg addr LSB */
else
   buf[0] = msg[0].len;
   buf[1] = msg[0].addr << 1;
   buf[2] = 2; /* reg addr len */
   buf[3] = msg[0].buf[0]; /* reg addr MSB */
   buf[4] = msg[0].buf[1]; /* reg addr LSB */
   memcpy(&buf[5], msg[2].buf, msg[0].len - 2);


>> as hell. I should be re-written totally in any case. Those tuner I2C
>> adapters should be moved to demod. Demod has 1 I2C adapter. USB-bridge has 2
>> adapters, one for each demod.
>>
>
> Agreed that it can be refactored and improved.  Also to support n
> transactions with a simple while loop and only issuing single writes
> and reads.  Only downside would be increased USB traffic for 2
> commands vs 1 - hence negligible.

there is i2c_adapter_quirks nowadays for these adapters which could do 
only limited set of commands.
include/linux/i2c.h

In my understanding that is how those chips are wired:
+---------------+     +--------+
| I2C adapter-1 | --> | eeprom |
+---------------+     +--------+
+---------------+     +---------+     +---------+
| I2C adapter-2 | --> | demod-1 | --> | tuner-1 |
+---------------+     +---------+     +---------+
+---------------+     +---------+     +---------+
| I2C adapter-3 | --> | demod-2 | --> | tuner-2 |
+---------------+     +---------+     +---------+

>> I have to find out af9015 datasheets and check how it is there. But I still
>> remember one case where I implemented one FX2 firmware and that same issues
>> raises there as well.
>>
>>>> After that both af9015+mxl5007t and af9035+mxl5007t started working.
>>>> Earlier
>>>> both were returning bogus values for chip id read.
>>>>
>>>> Also I am interested to known which kind of communication there is
>>>> actually
>>>> seen on I2C bus?
>>>
>>>
>>> With this or the patch I proposed, you see exactly what you expect on
>>> the I2C bus with repeated stops, as detailed in my previous mails.
>>
>>
>> So it is good?
>>
>
> Yes, I2C looks good.
>
>>>> If it starts working then have to find out way to fix it properly so that
>>>> any earlier device didn't broke.
>>>>
>>>
>>> I hope that by now I've made abundantly clear that my mxl5007t locks
>>> up after *any* read.  It doesn't matter if we are reading the correct
>>> register after any of the proposed patches.
>>
>>
>> So it still locks up after any read after the chip id read? And does not
>> work then? On my devices I can add multiple mxl5007t_get_chip_id() calls and
>> all are returning correct values.
>>
>
> No, as mentioned before, it locks up at the end of any read command.
> Including the chip_id.  The firmware is not aware of the issue and
> wont complain until the next I2C transaction.

Maybe I2C speed is too fast?
I tested with my device it failed when I increased speed to 850kHz. 
640kHz was working. I am not sure which is default speed and driver 
didn't change it. Just try to dropping it to 142kHz (0x12).
Speed is calculated using that formula (0x12 in that case is register 
value):
octave:36> 1000000 / (24.4 * 16 * 0x12)
ans =  142.304189435337

These are related registers:
/* I2C master bus 2 clock speed 300k */
ret = af9035_wr_reg(d, 0x00f6a7, 0x07);
/* I2C master bus 1,3 clock speed 300k */
ret = af9035_wr_reg(d, 0x00f103, 0x07);

Just add some good place before tuner attach like af9035_frontend_attach().

>> Could you test what happens if you use that CMD_GENERIC_I2C_WR +
>> CMD_GENERIC_I2C_RD ? I suspect it is lower level I2C xfer than those
>> CMD_I2C_RD + CMD_I2C_WR, which are likely somehow handled by demod core.
>>
>
> I will test, but the issue is either electrical or with the state of
> the mxl5007t.  I2C bus looks good from AF9035 side once the bug in the
> above is patched.

If dropping I2C speed does not help then I cannot imagine any other fix 
than adding mxl5007t driver option which disables problematic reads *or* 
add some hack to af9035 i2c adapter implementation which fakes required 
problematic commands ones that looks "good".

PS. I get af9015 working also just implementing i2c adapter so that it 
uses 2 byte long addresses when i2c msg write len is 2 - it is not 
problem anymore.

regards
Antti

-- 
http://palosaari.fi/

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: AVerMedia HD Volar (A867) AF9035 + MXL5007T driver issues
  2016-04-09  2:17         ` Antti Palosaari
@ 2016-04-09  8:13           ` Alessandro Radicati
  2016-04-09 14:25             ` Antti Palosaari
  0 siblings, 1 reply; 18+ messages in thread
From: Alessandro Radicati @ 2016-04-09  8:13 UTC (permalink / raw)
  To: Antti Palosaari; +Cc: Jose Alberto Reguero, linux-media

On Sat, Apr 9, 2016 at 4:17 AM, Antti Palosaari <crope@iki.fi> wrote:
> On 04/09/2016 04:52 AM, Alessandro Radicati wrote:
>>
>> On Sat, Apr 9, 2016 at 3:22 AM, Antti Palosaari <crope@iki.fi> wrote:
>>>
>>> Here is patches to test:
>>> http://git.linuxtv.org/anttip/media_tree.git/log/?h=af9035
>>>
>>
>> I've done this already in my testing, and it works for getting a
>> correct chip_id response, but only because it's avoiding the issue
>> with the write/read case in the af9035 driver.  Don't have an
>> af9015... perhaps there's a similar issue with that code or we are
>> dealing with two separate issues since af9035 never does a repeated
>> start?
>
>
> I am pretty sure mxl5007t requires stop between read and write. Usually
> chips are not caring too much if it is repeated start or not, whilst
> datasheets are often register read is S Wr S Rw P.
>
> Even af9035 i2c adapter implementation implements repeated start wrong, I

Where does the assumption that CMD_I2C_RD should issue a repeated
start sequence come from?  From the datasheet?  Maybe it was never
intended as repeated start.  Perhaps if there is another stick  with
mxl5007t and a chip that does repeated start, we can put this to bed.

> would not like to add anymore hacks there. It is currently ugly and complex

Bugfix != hack.  Don't see how putting the register address in the
address fields is a hack (perhaps semantics around the fact that 0xFB
is not really part of the address?); this is the only and intended way
to use that command for write/read.

> as hell. I should be re-written totally in any case. Those tuner I2C
> adapters should be moved to demod. Demod has 1 I2C adapter. USB-bridge has 2
> adapters, one for each demod.
>

Agreed that it can be refactored and improved.  Also to support n
transactions with a simple while loop and only issuing single writes
and reads.  Only downside would be increased USB traffic for 2
commands vs 1 - hence negligible.

> I have to find out af9015 datasheets and check how it is there. But I still
> remember one case where I implemented one FX2 firmware and that same issues
> raises there as well.
>
>>> After that both af9015+mxl5007t and af9035+mxl5007t started working.
>>> Earlier
>>> both were returning bogus values for chip id read.
>>>
>>> Also I am interested to known which kind of communication there is
>>> actually
>>> seen on I2C bus?
>>
>>
>> With this or the patch I proposed, you see exactly what you expect on
>> the I2C bus with repeated stops, as detailed in my previous mails.
>
>
> So it is good?
>

Yes, I2C looks good.

>>> If it starts working then have to find out way to fix it properly so that
>>> any earlier device didn't broke.
>>>
>>
>> I hope that by now I've made abundantly clear that my mxl5007t locks
>> up after *any* read.  It doesn't matter if we are reading the correct
>> register after any of the proposed patches.
>
>
> So it still locks up after any read after the chip id read? And does not
> work then? On my devices I can add multiple mxl5007t_get_chip_id() calls and
> all are returning correct values.
>

No, as mentioned before, it locks up at the end of any read command.
Including the chip_id.  The firmware is not aware of the issue and
wont complain until the next I2C transaction.

> Could you test what happens if you use that CMD_GENERIC_I2C_WR +
> CMD_GENERIC_I2C_RD ? I suspect it is lower level I2C xfer than those
> CMD_I2C_RD + CMD_I2C_WR, which are likely somehow handled by demod core.
>

I will test, but the issue is either electrical or with the state of
the mxl5007t.  I2C bus looks good from AF9035 side once the bug in the
above is patched.

Regards,
Alessandro

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: AVerMedia HD Volar (A867) AF9035 + MXL5007T driver issues
  2016-04-09  1:52       ` Alessandro Radicati
@ 2016-04-09  2:17         ` Antti Palosaari
  2016-04-09  8:13           ` Alessandro Radicati
  0 siblings, 1 reply; 18+ messages in thread
From: Antti Palosaari @ 2016-04-09  2:17 UTC (permalink / raw)
  To: Alessandro Radicati; +Cc: Jose Alberto Reguero, linux-media

On 04/09/2016 04:52 AM, Alessandro Radicati wrote:
> On Sat, Apr 9, 2016 at 3:22 AM, Antti Palosaari <crope@iki.fi> wrote:
>> Here is patches to test:
>> http://git.linuxtv.org/anttip/media_tree.git/log/?h=af9035
>>
>
> I've done this already in my testing, and it works for getting a
> correct chip_id response, but only because it's avoiding the issue
> with the write/read case in the af9035 driver.  Don't have an
> af9015... perhaps there's a similar issue with that code or we are
> dealing with two separate issues since af9035 never does a repeated
> start?

I am pretty sure mxl5007t requires stop between read and write. Usually 
chips are not caring too much if it is repeated start or not, whilst 
datasheets are often register read is S Wr S Rw P.

Even af9035 i2c adapter implementation implements repeated start wrong, 
I would not like to add anymore hacks there. It is currently ugly and 
complex as hell. I should be re-written totally in any case. Those tuner 
I2C adapters should be moved to demod. Demod has 1 I2C adapter. 
USB-bridge has 2 adapters, one for each demod.

I have to find out af9015 datasheets and check how it is there. But I 
still remember one case where I implemented one FX2 firmware and that 
same issues raises there as well.

>> After that both af9015+mxl5007t and af9035+mxl5007t started working. Earlier
>> both were returning bogus values for chip id read.
>>
>> Also I am interested to known which kind of communication there is actually
>> seen on I2C bus?
>
> With this or the patch I proposed, you see exactly what you expect on
> the I2C bus with repeated stops, as detailed in my previous mails.

So it is good?

>> If it starts working then have to find out way to fix it properly so that
>> any earlier device didn't broke.
>>
>
> I hope that by now I've made abundantly clear that my mxl5007t locks
> up after *any* read.  It doesn't matter if we are reading the correct
> register after any of the proposed patches.

So it still locks up after any read after the chip id read? And does not 
work then? On my devices I can add multiple mxl5007t_get_chip_id() calls 
and all are returning correct values.

Could you test what happens if you use that CMD_GENERIC_I2C_WR + 
CMD_GENERIC_I2C_RD ? I suspect it is lower level I2C xfer than those 
CMD_I2C_RD + CMD_I2C_WR, which are likely somehow handled by demod core.

regards
Antti

-- 
http://palosaari.fi/

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: AVerMedia HD Volar (A867) AF9035 + MXL5007T driver issues
  2016-04-09  1:22     ` Antti Palosaari
@ 2016-04-09  1:52       ` Alessandro Radicati
  2016-04-09  2:17         ` Antti Palosaari
  0 siblings, 1 reply; 18+ messages in thread
From: Alessandro Radicati @ 2016-04-09  1:52 UTC (permalink / raw)
  To: Antti Palosaari; +Cc: Jose Alberto Reguero, linux-media

On Sat, Apr 9, 2016 at 3:22 AM, Antti Palosaari <crope@iki.fi> wrote:
> Here is patches to test:
> http://git.linuxtv.org/anttip/media_tree.git/log/?h=af9035
>

I've done this already in my testing, and it works for getting a
correct chip_id response, but only because it's avoiding the issue
with the write/read case in the af9035 driver.  Don't have an
af9015... perhaps there's a similar issue with that code or we are
dealing with two separate issues since af9035 never does a repeated
start?

> After that both af9015+mxl5007t and af9035+mxl5007t started working. Earlier
> both were returning bogus values for chip id read.
>
> Also I am interested to known which kind of communication there is actually
> seen on I2C bus?

With this or the patch I proposed, you see exactly what you expect on
the I2C bus with repeated stops, as detailed in my previous mails.

>
> If it starts working then have to find out way to fix it properly so that
> any earlier device didn't broke.
>

I hope that by now I've made abundantly clear that my mxl5007t locks
up after *any* read.  It doesn't matter if we are reading the correct
register after any of the proposed patches.

Thanks,
Alessandro

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: AVerMedia HD Volar (A867) AF9035 + MXL5007T driver issues
  2016-04-09  0:50   ` Antti Palosaari
  2016-04-09  1:22     ` Antti Palosaari
@ 2016-04-09  1:29     ` Alessandro Radicati
  1 sibling, 0 replies; 18+ messages in thread
From: Alessandro Radicati @ 2016-04-09  1:29 UTC (permalink / raw)
  To: Antti Palosaari; +Cc: Jose Alberto Reguero, linux-media

Antti,
AF9035 I2C write/read is fine with the patch I proposed, like your
middle case.  This issue is not specific to MXL5007T; it's something I
caught sniffing the I2C bus with a logic analyzer and stands on it's
own.  I implemented it this way because the driver specifically
implements that write/read command, and it has the least impact by
trying to keep as much of the original behavior as possible.  BTW,
this command does not issue a repeated start on the bus.  It doesn't
work because the firmware ignores anything after the USB header for
the read command, so address fields must be used.

I apologize if i didn't make this more clear, but the real issue is
that after an I2C read (patch or unpatched - correct or incorrect),
the mxl5007t locks up.  Again, I verified this via logic analyzer on
the I2C bus.  So we need a way to avoid issuing reads to the mxl5007t,
unless someone has other suggestions to find the root cause.  It seems
that with your hardware this issue does not occur - but it does with
mine, Jose and others.  Perhaps we can dump and compare the eeprom?
However, it seems like a lot of work for just one printk.

Regards,
Alessandro

On Sat, Apr 9, 2016 at 2:50 AM, Antti Palosaari <crope@iki.fi> wrote:
> uh, how it could be so hard?
>
> I just made few tests and found 3 ways to read it. OK, one is that
> Alessandro already pointed out and I don't feel it correct. But those 2 are
> one for look. CMD_I2C_WR / CMD_I2C_RD with 1st priority, then
> CMD_GENERIC_I2C_WR / CMD_GENERIC_I2C_RD....
>
> {
> u8 buf[MAX_XFER_SIZE];
> struct usb_req req = {0, 0, 0, buf, 0, buf};
> #if 0
> req.cmd = CMD_GENERIC_I2C_WR;
> req.wlen = 3 + 2;
> req.rlen = 0;
> buf[0] = 2; // write len
> buf[1] = 0x02; /* I2C bus */ // NOK 3, 1, 0
> buf[2] = 0x60 << 1; // I2C addr
> buf[3] = 0xfb; /* reg addr MSB */
> buf[4] = 0xd9; /* reg addr LSB */
> ret = af9035_ctrl_msg(d, &req);
> dev_dbg(&d->udev->dev, "1mxl5007t %02x\n", 0);
>
> req.cmd = CMD_GENERIC_I2C_RD;
> req.wlen = 3;
> req.rlen = 1;
> buf[0] = 1; // read len
> buf[1] = 0x02; /* I2C bus */ // NOK 3, 1, 0
> buf[2] = 0x60 << 1; // I2C addr
> ret = af9035_ctrl_msg(d, &req);
> dev_dbg(&d->udev->dev, "1mxl5007t %02x\n", buf[0]);
> #endif
>
> #if 0
> req.cmd = CMD_I2C_RD;
> req.wlen = 5;
> req.rlen = 1;
> buf[0] = 1; // read len
> buf[1] = 0x60 << 1; // I2C addr
> buf[2] = 2; /* reg addr len */
> buf[3] = 0xfb; /* reg addr MSB */
> buf[4] = 0xd9; /* reg addr LSB */
> ret = af9035_ctrl_msg(d, &req);
> dev_dbg(&d->udev->dev, "4mxl5007t %02x\n", buf[0]);
> #endif
>
> #if 1
> req.cmd = CMD_I2C_WR;
> req.wlen = 7;
> req.rlen = 0;
> buf[0] = 2; // write len msg[0].len;
> buf[1] = 0x60 << 1; // I2C addr
> buf[2] = 0x00; /* reg addr len */
> buf[3] = 0x00; /* reg addr MSB */
> buf[4] = 0x00; /* reg addr LSB */
> buf[5] = 0xfb;
> buf[6] = 0xd9;
> ret = af9035_ctrl_msg(d, &req);
> dev_dbg(&d->udev->dev, "9mxl5007t %02x\n", buf[0]);
>
> req.cmd = CMD_I2C_RD;
> req.wlen = 5;
> req.rlen = 1;
> buf[0] = 1; // read len
> buf[1] = 0x60 << 1; // I2C addr
> buf[2] = 0x00; /* reg addr len */
> buf[3] = 0x00; /* reg addr MSB */
> buf[4] = 0x00; /* reg addr LSB */
> ret = af9035_ctrl_msg(d, &req);
> dev_dbg(&d->udev->dev, "9mxl5007t %02x\n", buf[0]);
> #endif
>
> }
>
>
>
>
> On 04/09/2016 02:59 AM, Alessandro Radicati wrote:
>>
>> Jose, Antti,
>> The no_probe option or similar is the only fix I could find (in fact i
>> was going to propose a similar patch to what you have).  I've tried
>> all combinations of firmware and also tried issuing the read command
>> to the tuner in different states (e.g. sleep, just after soft/hard
>> reset) to no avail.  I've modified AverMedia's linux driver to probe
>> as well, and the same thing happens.  I found the following behavior
>> in further testing:
>>
>> - I can arbitrarily read as many bytes as I want from any valid
>> register and the tuner will continue responding until the af9035
>> issues the expected NAK to signal the end of the read so that the
>> mxl5007t can release the bus.  The bus doesn't get released and it
>> stays stuck either high or low indefinitely so subsequent I2C commands
>> fail.
>> - Hard reset of the tuner by cycling af9035 GPIOH12 seems like the
>> only way to recover.  So mxl5007t is probably at fault.  Perhaps I2C
>> speed is too fast (SCL cycles at ~100KHz)?  Faulty hardware design of
>> the usb stick?
>> - Doesn't seem like the OEM drivers ever issue I2C read commands.
>> Maybe it's a known issue to them.
>>
>> I'm pretty much out of ideas to test.  Suggestions are welcome.
>> Otherwise I'll try to push through a patch for just "no_probe".
>>
>> Thanks,
>> Alessandro
>>
>> On Sat, Apr 9, 2016 at 1:13 AM, Jose Alberto Reguero
>> <jareguero@telefonica.net> wrote:
>>>
>>> I made a patch long time ago, but it was not accepted.
>>>
>>> https://patchwork.linuxtv.org/patch/16242/
>>>
>>> Jose Alberto
>>>
>>> El 06/04/2016 01:00, Alessandro Radicati <alessandro@radicati.net>
>>> escribió:
>>>>
>>>>
>>>> On Wed, Apr 6, 2016 at 12:33 AM, Antti Palosaari <crope@iki.fi> wrote:
>>>>>
>>>>> I found one stick having AF9035 + MXL5007T. It is HP branded A867, so
>>>>> it
>>>>> should be similar. It seems to work all three 12.13.15.0 6.20.15.0
>>>>> firmwares:
>>>>> http://palosaari.fi/linux/v4l-dvb/firmware/af9035/
>>>>>
>>>>> mxl5007t 5-0060: creating new instance
>>>>> mxl5007t_get_chip_id: unknown rev (3f)
>>>>> mxl5007t_get_chip_id: MxL5007T detected @ 5-0060
>>>>>
>>>>> That is what AF9035 reports (with debug) as a chip version:
>>>>> dvb_usb_af9035: prechip_version=00 chip_version=03 chip_type=3802
>>>>>
>>>>>
>>>>> Do you have different chip version?
>>>>>
>>>>
>>>> I have a Sky Italy DVB stick with the same chip version.  I see that
>>>> you get the 0x3f response as well... that should be fixed by the I2C
>>>> patch I proposed.  However, your stick seems to handle the read
>>>> properly and process subsequent I2C commands - something that doesn't
>>>> happen with mine.  The vendor drivers in linux and windows never seem
>>>> issue the USB I2C commands to read from the tuner.  I'll test with
>>>> other firmware versions to see if something changes.
>>>>
>>>> Regards,
>>>> Alessandro
>>>> --
>>>> To unsubscribe from this list: send the line "unsubscribe linux-media"
>>>> in
>>>> the body of a message to majordomo@vger.kernel.org
>>>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>
>
> --
> http://palosaari.fi/

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: AVerMedia HD Volar (A867) AF9035 + MXL5007T driver issues
  2016-04-09  0:50   ` Antti Palosaari
@ 2016-04-09  1:22     ` Antti Palosaari
  2016-04-09  1:52       ` Alessandro Radicati
  2016-04-09  1:29     ` Alessandro Radicati
  1 sibling, 1 reply; 18+ messages in thread
From: Antti Palosaari @ 2016-04-09  1:22 UTC (permalink / raw)
  To: Alessandro Radicati, Jose Alberto Reguero; +Cc: linux-media

Here is patches to test:
http://git.linuxtv.org/anttip/media_tree.git/log/?h=af9035

After that both af9015+mxl5007t and af9035+mxl5007t started working. 
Earlier both were returning bogus values for chip id read.

Also I am interested to known which kind of communication there is 
actually seen on I2C bus?

If it starts working then have to find out way to fix it properly so 
that any earlier device didn't broke.

regards
Antti

On 04/09/2016 03:50 AM, Antti Palosaari wrote:
> uh, how it could be so hard?
>
> I just made few tests and found 3 ways to read it. OK, one is that
> Alessandro already pointed out and I don't feel it correct. But those 2
> are one for look. CMD_I2C_WR / CMD_I2C_RD with 1st priority, then
> CMD_GENERIC_I2C_WR / CMD_GENERIC_I2C_RD....
>
> {
> u8 buf[MAX_XFER_SIZE];
> struct usb_req req = {0, 0, 0, buf, 0, buf};
> #if 0
> req.cmd = CMD_GENERIC_I2C_WR;
> req.wlen = 3 + 2;
> req.rlen = 0;
> buf[0] = 2; // write len
> buf[1] = 0x02; /* I2C bus */ // NOK 3, 1, 0
> buf[2] = 0x60 << 1; // I2C addr
> buf[3] = 0xfb; /* reg addr MSB */
> buf[4] = 0xd9; /* reg addr LSB */
> ret = af9035_ctrl_msg(d, &req);
> dev_dbg(&d->udev->dev, "1mxl5007t %02x\n", 0);
>
> req.cmd = CMD_GENERIC_I2C_RD;
> req.wlen = 3;
> req.rlen = 1;
> buf[0] = 1; // read len
> buf[1] = 0x02; /* I2C bus */ // NOK 3, 1, 0
> buf[2] = 0x60 << 1; // I2C addr
> ret = af9035_ctrl_msg(d, &req);
> dev_dbg(&d->udev->dev, "1mxl5007t %02x\n", buf[0]);
> #endif
>
> #if 0
> req.cmd = CMD_I2C_RD;
> req.wlen = 5;
> req.rlen = 1;
> buf[0] = 1; // read len
> buf[1] = 0x60 << 1; // I2C addr
> buf[2] = 2; /* reg addr len */
> buf[3] = 0xfb; /* reg addr MSB */
> buf[4] = 0xd9; /* reg addr LSB */
> ret = af9035_ctrl_msg(d, &req);
> dev_dbg(&d->udev->dev, "4mxl5007t %02x\n", buf[0]);
> #endif
>
> #if 1
> req.cmd = CMD_I2C_WR;
> req.wlen = 7;
> req.rlen = 0;
> buf[0] = 2; // write len msg[0].len;
> buf[1] = 0x60 << 1; // I2C addr
> buf[2] = 0x00; /* reg addr len */
> buf[3] = 0x00; /* reg addr MSB */
> buf[4] = 0x00; /* reg addr LSB */
> buf[5] = 0xfb;
> buf[6] = 0xd9;
> ret = af9035_ctrl_msg(d, &req);
> dev_dbg(&d->udev->dev, "9mxl5007t %02x\n", buf[0]);
>
> req.cmd = CMD_I2C_RD;
> req.wlen = 5;
> req.rlen = 1;
> buf[0] = 1; // read len
> buf[1] = 0x60 << 1; // I2C addr
> buf[2] = 0x00; /* reg addr len */
> buf[3] = 0x00; /* reg addr MSB */
> buf[4] = 0x00; /* reg addr LSB */
> ret = af9035_ctrl_msg(d, &req);
> dev_dbg(&d->udev->dev, "9mxl5007t %02x\n", buf[0]);
> #endif
> }
>
>
>
>
> On 04/09/2016 02:59 AM, Alessandro Radicati wrote:
>> Jose, Antti,
>> The no_probe option or similar is the only fix I could find (in fact i
>> was going to propose a similar patch to what you have).  I've tried
>> all combinations of firmware and also tried issuing the read command
>> to the tuner in different states (e.g. sleep, just after soft/hard
>> reset) to no avail.  I've modified AverMedia's linux driver to probe
>> as well, and the same thing happens.  I found the following behavior
>> in further testing:
>>
>> - I can arbitrarily read as many bytes as I want from any valid
>> register and the tuner will continue responding until the af9035
>> issues the expected NAK to signal the end of the read so that the
>> mxl5007t can release the bus.  The bus doesn't get released and it
>> stays stuck either high or low indefinitely so subsequent I2C commands
>> fail.
>> - Hard reset of the tuner by cycling af9035 GPIOH12 seems like the
>> only way to recover.  So mxl5007t is probably at fault.  Perhaps I2C
>> speed is too fast (SCL cycles at ~100KHz)?  Faulty hardware design of
>> the usb stick?
>> - Doesn't seem like the OEM drivers ever issue I2C read commands.
>> Maybe it's a known issue to them.
>>
>> I'm pretty much out of ideas to test.  Suggestions are welcome.
>> Otherwise I'll try to push through a patch for just "no_probe".
>>
>> Thanks,
>> Alessandro
>>
>> On Sat, Apr 9, 2016 at 1:13 AM, Jose Alberto Reguero
>> <jareguero@telefonica.net> wrote:
>>> I made a patch long time ago, but it was not accepted.
>>>
>>> https://patchwork.linuxtv.org/patch/16242/
>>>
>>> Jose Alberto
>>>
>>> El 06/04/2016 01:00, Alessandro Radicati <alessandro@radicati.net>
>>> escribió:
>>>>
>>>> On Wed, Apr 6, 2016 at 12:33 AM, Antti Palosaari <crope@iki.fi> wrote:
>>>>> I found one stick having AF9035 + MXL5007T. It is HP branded A867,
>>>>> so it
>>>>> should be similar. It seems to work all three 12.13.15.0 6.20.15.0
>>>>> firmwares:
>>>>> http://palosaari.fi/linux/v4l-dvb/firmware/af9035/
>>>>>
>>>>> mxl5007t 5-0060: creating new instance
>>>>> mxl5007t_get_chip_id: unknown rev (3f)
>>>>> mxl5007t_get_chip_id: MxL5007T detected @ 5-0060
>>>>>
>>>>> That is what AF9035 reports (with debug) as a chip version:
>>>>> dvb_usb_af9035: prechip_version=00 chip_version=03 chip_type=3802
>>>>>
>>>>>
>>>>> Do you have different chip version?
>>>>>
>>>>
>>>> I have a Sky Italy DVB stick with the same chip version.  I see that
>>>> you get the 0x3f response as well... that should be fixed by the I2C
>>>> patch I proposed.  However, your stick seems to handle the read
>>>> properly and process subsequent I2C commands - something that doesn't
>>>> happen with mine.  The vendor drivers in linux and windows never seem
>>>> issue the USB I2C commands to read from the tuner.  I'll test with
>>>> other firmware versions to see if something changes.
>>>>
>>>> Regards,
>>>> Alessandro
>>>> --
>>>> To unsubscribe from this list: send the line "unsubscribe
>>>> linux-media" in
>>>> the body of a message to majordomo@vger.kernel.org
>>>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

-- 
http://palosaari.fi/

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: AVerMedia HD Volar (A867) AF9035 + MXL5007T driver issues
  2016-04-08 23:59 ` Alessandro Radicati
@ 2016-04-09  0:50   ` Antti Palosaari
  2016-04-09  1:22     ` Antti Palosaari
  2016-04-09  1:29     ` Alessandro Radicati
  0 siblings, 2 replies; 18+ messages in thread
From: Antti Palosaari @ 2016-04-09  0:50 UTC (permalink / raw)
  To: Alessandro Radicati, Jose Alberto Reguero; +Cc: linux-media

uh, how it could be so hard?

I just made few tests and found 3 ways to read it. OK, one is that 
Alessandro already pointed out and I don't feel it correct. But those 2 
are one for look. CMD_I2C_WR / CMD_I2C_RD with 1st priority, then 
CMD_GENERIC_I2C_WR / CMD_GENERIC_I2C_RD....

{
u8 buf[MAX_XFER_SIZE];
struct usb_req req = {0, 0, 0, buf, 0, buf};
#if 0
req.cmd = CMD_GENERIC_I2C_WR;
req.wlen = 3 + 2;
req.rlen = 0;
buf[0] = 2; // write len
buf[1] = 0x02; /* I2C bus */ // NOK 3, 1, 0
buf[2] = 0x60 << 1; // I2C addr
buf[3] = 0xfb; /* reg addr MSB */
buf[4] = 0xd9; /* reg addr LSB */
ret = af9035_ctrl_msg(d, &req);
dev_dbg(&d->udev->dev, "1mxl5007t %02x\n", 0);

req.cmd = CMD_GENERIC_I2C_RD;
req.wlen = 3;
req.rlen = 1;
buf[0] = 1; // read len
buf[1] = 0x02; /* I2C bus */ // NOK 3, 1, 0
buf[2] = 0x60 << 1; // I2C addr
ret = af9035_ctrl_msg(d, &req);
dev_dbg(&d->udev->dev, "1mxl5007t %02x\n", buf[0]);
#endif

#if 0
req.cmd = CMD_I2C_RD;
req.wlen = 5;
req.rlen = 1;
buf[0] = 1; // read len
buf[1] = 0x60 << 1; // I2C addr
buf[2] = 2; /* reg addr len */
buf[3] = 0xfb; /* reg addr MSB */
buf[4] = 0xd9; /* reg addr LSB */
ret = af9035_ctrl_msg(d, &req);
dev_dbg(&d->udev->dev, "4mxl5007t %02x\n", buf[0]);
#endif

#if 1
req.cmd = CMD_I2C_WR;
req.wlen = 7;
req.rlen = 0;
buf[0] = 2; // write len msg[0].len;
buf[1] = 0x60 << 1; // I2C addr
buf[2] = 0x00; /* reg addr len */
buf[3] = 0x00; /* reg addr MSB */
buf[4] = 0x00; /* reg addr LSB */
buf[5] = 0xfb;
buf[6] = 0xd9;
ret = af9035_ctrl_msg(d, &req);
dev_dbg(&d->udev->dev, "9mxl5007t %02x\n", buf[0]);

req.cmd = CMD_I2C_RD;
req.wlen = 5;
req.rlen = 1;
buf[0] = 1; // read len
buf[1] = 0x60 << 1; // I2C addr
buf[2] = 0x00; /* reg addr len */
buf[3] = 0x00; /* reg addr MSB */
buf[4] = 0x00; /* reg addr LSB */
ret = af9035_ctrl_msg(d, &req);
dev_dbg(&d->udev->dev, "9mxl5007t %02x\n", buf[0]);
#endif
}




On 04/09/2016 02:59 AM, Alessandro Radicati wrote:
> Jose, Antti,
> The no_probe option or similar is the only fix I could find (in fact i
> was going to propose a similar patch to what you have).  I've tried
> all combinations of firmware and also tried issuing the read command
> to the tuner in different states (e.g. sleep, just after soft/hard
> reset) to no avail.  I've modified AverMedia's linux driver to probe
> as well, and the same thing happens.  I found the following behavior
> in further testing:
>
> - I can arbitrarily read as many bytes as I want from any valid
> register and the tuner will continue responding until the af9035
> issues the expected NAK to signal the end of the read so that the
> mxl5007t can release the bus.  The bus doesn't get released and it
> stays stuck either high or low indefinitely so subsequent I2C commands
> fail.
> - Hard reset of the tuner by cycling af9035 GPIOH12 seems like the
> only way to recover.  So mxl5007t is probably at fault.  Perhaps I2C
> speed is too fast (SCL cycles at ~100KHz)?  Faulty hardware design of
> the usb stick?
> - Doesn't seem like the OEM drivers ever issue I2C read commands.
> Maybe it's a known issue to them.
>
> I'm pretty much out of ideas to test.  Suggestions are welcome.
> Otherwise I'll try to push through a patch for just "no_probe".
>
> Thanks,
> Alessandro
>
> On Sat, Apr 9, 2016 at 1:13 AM, Jose Alberto Reguero
> <jareguero@telefonica.net> wrote:
>> I made a patch long time ago, but it was not accepted.
>>
>> https://patchwork.linuxtv.org/patch/16242/
>>
>> Jose Alberto
>>
>> El 06/04/2016 01:00, Alessandro Radicati <alessandro@radicati.net> escribió:
>>>
>>> On Wed, Apr 6, 2016 at 12:33 AM, Antti Palosaari <crope@iki.fi> wrote:
>>>> I found one stick having AF9035 + MXL5007T. It is HP branded A867, so it
>>>> should be similar. It seems to work all three 12.13.15.0 6.20.15.0
>>>> firmwares:
>>>> http://palosaari.fi/linux/v4l-dvb/firmware/af9035/
>>>>
>>>> mxl5007t 5-0060: creating new instance
>>>> mxl5007t_get_chip_id: unknown rev (3f)
>>>> mxl5007t_get_chip_id: MxL5007T detected @ 5-0060
>>>>
>>>> That is what AF9035 reports (with debug) as a chip version:
>>>> dvb_usb_af9035: prechip_version=00 chip_version=03 chip_type=3802
>>>>
>>>>
>>>> Do you have different chip version?
>>>>
>>>
>>> I have a Sky Italy DVB stick with the same chip version.  I see that
>>> you get the 0x3f response as well... that should be fixed by the I2C
>>> patch I proposed.  However, your stick seems to handle the read
>>> properly and process subsequent I2C commands - something that doesn't
>>> happen with mine.  The vendor drivers in linux and windows never seem
>>> issue the USB I2C commands to read from the tuner.  I'll test with
>>> other firmware versions to see if something changes.
>>>
>>> Regards,
>>> Alessandro
>>> --
>>> To unsubscribe from this list: send the line "unsubscribe linux-media" in
>>> the body of a message to majordomo@vger.kernel.org
>>> More majordomo info at  http://vger.kernel.org/majordomo-info.html

-- 
http://palosaari.fi/

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: AVerMedia HD Volar (A867) AF9035 + MXL5007T driver issues
       [not found] <57083b12.ec3ec20a.eed91.1ea1SMTPIN_ADDED_BROKEN@mx.google.com>
@ 2016-04-08 23:59 ` Alessandro Radicati
  2016-04-09  0:50   ` Antti Palosaari
  0 siblings, 1 reply; 18+ messages in thread
From: Alessandro Radicati @ 2016-04-08 23:59 UTC (permalink / raw)
  To: Jose Alberto Reguero; +Cc: linux-media, Antti Palosaari

Jose, Antti,
The no_probe option or similar is the only fix I could find (in fact i
was going to propose a similar patch to what you have).  I've tried
all combinations of firmware and also tried issuing the read command
to the tuner in different states (e.g. sleep, just after soft/hard
reset) to no avail.  I've modified AverMedia's linux driver to probe
as well, and the same thing happens.  I found the following behavior
in further testing:

- I can arbitrarily read as many bytes as I want from any valid
register and the tuner will continue responding until the af9035
issues the expected NAK to signal the end of the read so that the
mxl5007t can release the bus.  The bus doesn't get released and it
stays stuck either high or low indefinitely so subsequent I2C commands
fail.
- Hard reset of the tuner by cycling af9035 GPIOH12 seems like the
only way to recover.  So mxl5007t is probably at fault.  Perhaps I2C
speed is too fast (SCL cycles at ~100KHz)?  Faulty hardware design of
the usb stick?
- Doesn't seem like the OEM drivers ever issue I2C read commands.
Maybe it's a known issue to them.

I'm pretty much out of ideas to test.  Suggestions are welcome.
Otherwise I'll try to push through a patch for just "no_probe".

Thanks,
Alessandro

On Sat, Apr 9, 2016 at 1:13 AM, Jose Alberto Reguero
<jareguero@telefonica.net> wrote:
> I made a patch long time ago, but it was not accepted.
>
> https://patchwork.linuxtv.org/patch/16242/
>
> Jose Alberto
>
> El 06/04/2016 01:00, Alessandro Radicati <alessandro@radicati.net> escribió:
>>
>> On Wed, Apr 6, 2016 at 12:33 AM, Antti Palosaari <crope@iki.fi> wrote:
>> > I found one stick having AF9035 + MXL5007T. It is HP branded A867, so it
>> > should be similar. It seems to work all three 12.13.15.0 6.20.15.0
>> > firmwares:
>> > http://palosaari.fi/linux/v4l-dvb/firmware/af9035/
>> >
>> > mxl5007t 5-0060: creating new instance
>> > mxl5007t_get_chip_id: unknown rev (3f)
>> > mxl5007t_get_chip_id: MxL5007T detected @ 5-0060
>> >
>> > That is what AF9035 reports (with debug) as a chip version:
>> > dvb_usb_af9035: prechip_version=00 chip_version=03 chip_type=3802
>> >
>> >
>> > Do you have different chip version?
>> >
>>
>> I have a Sky Italy DVB stick with the same chip version.  I see that
>> you get the 0x3f response as well... that should be fixed by the I2C
>> patch I proposed.  However, your stick seems to handle the read
>> properly and process subsequent I2C commands - something that doesn't
>> happen with mine.  The vendor drivers in linux and windows never seem
>> issue the USB I2C commands to read from the tuner.  I'll test with
>> other firmware versions to see if something changes.
>>
>> Regards,
>> Alessandro
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-media" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: AVerMedia HD Volar (A867) AF9035 + MXL5007T driver issues
  2016-04-05 22:33     ` Antti Palosaari
@ 2016-04-05 23:00       ` Alessandro Radicati
  0 siblings, 0 replies; 18+ messages in thread
From: Alessandro Radicati @ 2016-04-05 23:00 UTC (permalink / raw)
  To: Antti Palosaari; +Cc: linux-media

On Wed, Apr 6, 2016 at 12:33 AM, Antti Palosaari <crope@iki.fi> wrote:
> I found one stick having AF9035 + MXL5007T. It is HP branded A867, so it
> should be similar. It seems to work all three 12.13.15.0 6.20.15.0
> firmwares:
> http://palosaari.fi/linux/v4l-dvb/firmware/af9035/
>
> mxl5007t 5-0060: creating new instance
> mxl5007t_get_chip_id: unknown rev (3f)
> mxl5007t_get_chip_id: MxL5007T detected @ 5-0060
>
> That is what AF9035 reports (with debug) as a chip version:
> dvb_usb_af9035: prechip_version=00 chip_version=03 chip_type=3802
>
>
> Do you have different chip version?
>

I have a Sky Italy DVB stick with the same chip version.  I see that
you get the 0x3f response as well... that should be fixed by the I2C
patch I proposed.  However, your stick seems to handle the read
properly and process subsequent I2C commands - something that doesn't
happen with mine.  The vendor drivers in linux and windows never seem
issue the USB I2C commands to read from the tuner.  I'll test with
other firmware versions to see if something changes.

Regards,
Alessandro

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: AVerMedia HD Volar (A867) AF9035 + MXL5007T driver issues
  2016-04-05 19:34   ` Alessandro Radicati
@ 2016-04-05 22:33     ` Antti Palosaari
  2016-04-05 23:00       ` Alessandro Radicati
  0 siblings, 1 reply; 18+ messages in thread
From: Antti Palosaari @ 2016-04-05 22:33 UTC (permalink / raw)
  To: Alessandro Radicati; +Cc: linux-media

I found one stick having AF9035 + MXL5007T. It is HP branded A867, so it 
should be similar. It seems to work all three 12.13.15.0 6.20.15.0 
firmwares:
http://palosaari.fi/linux/v4l-dvb/firmware/af9035/

mxl5007t 5-0060: creating new instance
mxl5007t_get_chip_id: unknown rev (3f)
mxl5007t_get_chip_id: MxL5007T detected @ 5-0060

That is what AF9035 reports (with debug) as a chip version:
dvb_usb_af9035: prechip_version=00 chip_version=03 chip_type=3802


Do you have different chip version?

regards
Antti

-- 
http://palosaari.fi/

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: AVerMedia HD Volar (A867) AF9035 + MXL5007T driver issues
  2016-04-05 18:15 ` Antti Palosaari
@ 2016-04-05 19:34   ` Alessandro Radicati
  2016-04-05 22:33     ` Antti Palosaari
  0 siblings, 1 reply; 18+ messages in thread
From: Alessandro Radicati @ 2016-04-05 19:34 UTC (permalink / raw)
  To: Antti Palosaari; +Cc: linux-media

On Tue, Apr 5, 2016 at 8:15 PM, Antti Palosaari <crope@iki.fi> wrote:
> On 04/02/2016 01:44 PM, Alessandro Radicati wrote:
>>
>> Hi,
>> In trying to understand why my DVB USB tuner doesn't work with stock
>> kernel drivers (4.2.0), I decided to pull out my logic analyser and
>> sniff the I2C bus between the AF9035 and MXL5007T.  I seem to have
>> uncovered a couple of issues:
>>
>> 1) Attach fails because MXL5007T driver I2C soft reset fails.  This is
>> due to the preceding chip id read request that seems to hang the I2C
>> bus and cause subsequent I2C commands to fail.
>
>
> In my understanding MXL5007T register read is done in a two transactions.
> First you should write wanted register to register 0xfb. After that single
> byte read from the chip returns value for register configured to 0xfb.
> Write+read with repeated start is not supported and driver is buggy as it
> request that which usually leads to bogus value.
>

I'm not sure if it supports repeated start or not, but in reality the
AF9035 firmware will not perform a repeated start sequence.  I.e. a
single read USB command results in two separate I2C transactions (s
write ... p + s read ... p).  So using the specific I2C tuner read usb
command (with address fields) is the same as using two separate write
and read commands.  I've only tested default firmware version: LINK
12.13.15.0 - OFDM 6.20.15.0

>
>> 2) AF9035 driver I2C master xfer incorrectly implements "Write read"
>> case.  The FW expects register address fields to be used to send the
>> I2C writes for register selection.  The current implementation ignores
>> these fields and the result is that only an I2C read is issued.
>> Therefore the "0x3f" returned by the MXL5007T chip id query is not
>> from the expected register.  This is what is seen on the I2C bus:
>>
>> S | Read 0x60 + ACK | 0x3F + NAK | ...
>>
>> After which SDA is held low for ~6sec; reason for subsequent commands
>> failing.
>
>
> You should use "S | W | P | S | R | P", no "S | W | S | R | P" == write read
> with repeated start condition.
>

This is what I sniffed on the bus without any modifications to the
driver.  My intent was to show that there is no write transaction at
all.

>> 3) After modifying the AF9035 driver to fix point 2 and use the
>> register address field, the following is seen on the I2C bus:
>>
>> S | Write 0x60 + ACK | 0xFB + ACK | 0xD9 + ACK | P
>> S | Read 0x60 + ACK | 0x14 + NAK | ...
>
>
> That is correct sequence. You are trying to read more than 1 byte and it
> fails?
>

This is after I modified the driver to use the register address
fields.  Sequence looks good except for the missing P after the NAK,
but this is due to something holding SDA low or an issue with the
AF9035.  I've tested this reading other registers, but same behavior.
I've also tried using two separate write read commands, exact same I2C
sequence and same result.  I also hacked the driver to read two bytes
instead of one.  Interestingly I got 0x14 twice as expected, but again
the bus hangs after the NAK that marks the end of the read.

>> This time we get an expected response, but the I2C bus still hangs
>> with SDA held low and no Stop sequence.  It seems that the MXL5007T is
>> holding SDA low since the AF9035 happily cycles SCL trying to execute
>> the subsequent writes.  Without a solution to this, it seems that
>> avoiding the I2C read is the best way to have the driver work
>> correctly.  There are no other tuner reads so point 2 above becomes
>> moot for at least this device.
>>
>> Does anyone have any insight on the MXL5007T chip ID command and
>> whether it should be issued in certain conditions?  Any suggestions on
>> how to resolve this given the above?
>
>
> I tried to fix it earlier:
> http://www.spinics.net/lists/linux-media/msg62264.html
>
> Feel free to fix! It should not be very hard as you could even sniff data
> from the I2C bus directly. I don't have any AF9035+MXL5007T device, but I
> have tested it with older AF9015+MXL5007T.
>

So there are two problems:
1) AF9035 I2C master read function needs to use the register address
fields.  This is a trivial fix and I've done a patch.  However I'm
unsure of the consequences this may have with other tuners.  I can
only guess that tuner reads are almost never done or are not
important, otherwise this would have been caught earlier.  Again, this
may be something related to this specific firmware, but unlikely.
2) I2C bus hangs after any MXL5007t I2C read is performed.  I have no
idea why this happens, or who is the culprit.  I'm happy to test out
any suggestions.

Thanks,
Alessandro

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: AVerMedia HD Volar (A867) AF9035 + MXL5007T driver issues
  2016-04-02 10:44 Alessandro Radicati
@ 2016-04-05 18:15 ` Antti Palosaari
  2016-04-05 19:34   ` Alessandro Radicati
  0 siblings, 1 reply; 18+ messages in thread
From: Antti Palosaari @ 2016-04-05 18:15 UTC (permalink / raw)
  To: Alessandro Radicati, linux-media

On 04/02/2016 01:44 PM, Alessandro Radicati wrote:
> Hi,
> In trying to understand why my DVB USB tuner doesn't work with stock
> kernel drivers (4.2.0), I decided to pull out my logic analyser and
> sniff the I2C bus between the AF9035 and MXL5007T.  I seem to have
> uncovered a couple of issues:
>
> 1) Attach fails because MXL5007T driver I2C soft reset fails.  This is
> due to the preceding chip id read request that seems to hang the I2C
> bus and cause subsequent I2C commands to fail.

In my understanding MXL5007T register read is done in a two 
transactions. First you should write wanted register to register 0xfb. 
After that single byte read from the chip returns value for register 
configured to 0xfb. Write+read with repeated start is not supported and 
driver is buggy as it request that which usually leads to bogus value.


> 2) AF9035 driver I2C master xfer incorrectly implements "Write read"
> case.  The FW expects register address fields to be used to send the
> I2C writes for register selection.  The current implementation ignores
> these fields and the result is that only an I2C read is issued.
> Therefore the "0x3f" returned by the MXL5007T chip id query is not
> from the expected register.  This is what is seen on the I2C bus:
>
> S | Read 0x60 + ACK | 0x3F + NAK | ...
>
> After which SDA is held low for ~6sec; reason for subsequent commands failing.

You should use "S | W | P | S | R | P", no "S | W | S | R | P" == write 
read with repeated start condition.

> 3) After modifying the AF9035 driver to fix point 2 and use the
> register address field, the following is seen on the I2C bus:
>
> S | Write 0x60 + ACK | 0xFB + ACK | 0xD9 + ACK | P
> S | Read 0x60 + ACK | 0x14 + NAK | ...

That is correct sequence. You are trying to read more than 1 byte and it 
fails?

> This time we get an expected response, but the I2C bus still hangs
> with SDA held low and no Stop sequence.  It seems that the MXL5007T is
> holding SDA low since the AF9035 happily cycles SCL trying to execute
> the subsequent writes.  Without a solution to this, it seems that
> avoiding the I2C read is the best way to have the driver work
> correctly.  There are no other tuner reads so point 2 above becomes
> moot for at least this device.
>
> Does anyone have any insight on the MXL5007T chip ID command and
> whether it should be issued in certain conditions?  Any suggestions on
> how to resolve this given the above?

I tried to fix it earlier:
http://www.spinics.net/lists/linux-media/msg62264.html

Feel free to fix! It should not be very hard as you could even sniff 
data from the I2C bus directly. I don't have any AF9035+MXL5007T device, 
but I have tested it with older AF9015+MXL5007T.

regards
Antti

-- 
http://palosaari.fi/

^ permalink raw reply	[flat|nested] 18+ messages in thread

* AVerMedia HD Volar (A867) AF9035 + MXL5007T driver issues
@ 2016-04-02 10:44 Alessandro Radicati
  2016-04-05 18:15 ` Antti Palosaari
  0 siblings, 1 reply; 18+ messages in thread
From: Alessandro Radicati @ 2016-04-02 10:44 UTC (permalink / raw)
  To: linux-media

Hi,
In trying to understand why my DVB USB tuner doesn't work with stock
kernel drivers (4.2.0), I decided to pull out my logic analyser and
sniff the I2C bus between the AF9035 and MXL5007T.  I seem to have
uncovered a couple of issues:

1) Attach fails because MXL5007T driver I2C soft reset fails.  This is
due to the preceding chip id read request that seems to hang the I2C
bus and cause subsequent I2C commands to fail.

2) AF9035 driver I2C master xfer incorrectly implements "Write read"
case.  The FW expects register address fields to be used to send the
I2C writes for register selection.  The current implementation ignores
these fields and the result is that only an I2C read is issued.
Therefore the "0x3f" returned by the MXL5007T chip id query is not
from the expected register.  This is what is seen on the I2C bus:

S | Read 0x60 + ACK | 0x3F + NAK | ...

After which SDA is held low for ~6sec; reason for subsequent commands failing.

3) After modifying the AF9035 driver to fix point 2 and use the
register address field, the following is seen on the I2C bus:

S | Write 0x60 + ACK | 0xFB + ACK | 0xD9 + ACK | P
S | Read 0x60 + ACK | 0x14 + NAK | ...

This time we get an expected response, but the I2C bus still hangs
with SDA held low and no Stop sequence.  It seems that the MXL5007T is
holding SDA low since the AF9035 happily cycles SCL trying to execute
the subsequent writes.  Without a solution to this, it seems that
avoiding the I2C read is the best way to have the driver work
correctly.  There are no other tuner reads so point 2 above becomes
moot for at least this device.

Does anyone have any insight on the MXL5007T chip ID command and
whether it should be issued in certain conditions?  Any suggestions on
how to resolve this given the above?

Regards,
Alessandro Radicati

^ permalink raw reply	[flat|nested] 18+ messages in thread

end of thread, other threads:[~2016-04-10 21:31 UTC | newest]

Thread overview: 18+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-04-08 23:13 AVerMedia HD Volar (A867) AF9035 + MXL5007T driver issues Jose Alberto Reguero
  -- strict thread matches above, loose matches on Subject: below --
2016-04-10 21:30 Jose Alberto Reguero
     [not found] <57083b12.ec3ec20a.eed91.1ea1SMTPIN_ADDED_BROKEN@mx.google.com>
2016-04-08 23:59 ` Alessandro Radicati
2016-04-09  0:50   ` Antti Palosaari
2016-04-09  1:22     ` Antti Palosaari
2016-04-09  1:52       ` Alessandro Radicati
2016-04-09  2:17         ` Antti Palosaari
2016-04-09  8:13           ` Alessandro Radicati
2016-04-09 14:25             ` Antti Palosaari
2016-04-09 16:11               ` Alessandro Radicati
2016-04-09 17:07                 ` Antti Palosaari
2016-04-09 17:38                   ` Alessandro Radicati
2016-04-09  1:29     ` Alessandro Radicati
2016-04-02 10:44 Alessandro Radicati
2016-04-05 18:15 ` Antti Palosaari
2016-04-05 19:34   ` Alessandro Radicati
2016-04-05 22:33     ` Antti Palosaari
2016-04-05 23:00       ` Alessandro Radicati

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.