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 <alessan...@radicati.net> escribió: > > On Sat, Apr 9, 2016 at 7:07 PM, Antti Palosaari <cr...@iki.fi> wrote: > > > > > > On 04/09/2016 07:11 PM, Alessandro Radicati wrote: > >> > >> On Sat, Apr 9, 2016 at 4:25 PM, Antti Palosaari <cr...@iki.fi> wrote: > >>> > >>> On 04/09/2016 11:13 AM, Alessandro Radicati wrote: > >>>> > >>>> > >>>> On Sat, Apr 9, 2016 at 4:17 AM, Antti Palosaari <cr...@iki.fi> wrote: > >>>>> > >>>>> > >>>>> On 04/09/2016 04:52 AM, Alessandro Radicati wrote: > >>>>>> > >>>>>> > >>>>>> > >>>>>> On Sat, Apr 9, 2016 at 3:22 AM, Antti Palosaari <cr...@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 majord...@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html
