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.
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/
--
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
