Re: [PATCH] gl861: re-implement i2c adapter logic
Hi, thanks for the patch.
> See updated patch on ml.
This raises two concerns for me:
[1]. Clients must split an I2C transaction of one read into two,
releasing the lock between them.
They might be interrupted (for example) by other read transaction
to another tuner, (or by other, un-related I2C to the demod),
and get wrong result.
ex.
1. write a read command to tuner@
40 03 00 30 fe 00 01 00 >>> (XX << 1) | 1
2. write another read command to tuner@
40 03 00 30 fe 00 01 00 >>> (YY << 1) | 1
3. read the result from tuner@XX
C0 02 00 30 00 01 01 00 <<< ??? # which one?
4. read the result from tuner@YY
C0 02 00 30 00 01 01 00 <<< ??? # right/valid answer?
[2]. There are (fairly common) use-cases of 2-Bytes, non-short writing,
which is not seemed to be supported by this patch.
ex. tuner read log from my previous post. (no tuner register)
40 03 00 30 fe 00 01 00 >>> c1 # addr:0x18, buf = {0xfe, 0xc1}
And friio itself also uses those 2-Bytes non-short msgs in its reset,
to control some peripherals other than demod/tuners.
ex. gl861.c::friio_reset()::line 422
40 03 00 12 03 00 01 00 >>> 80 # addr:0x09, buf = {0x03, 0x80}
But It may be possible to get around all these concerns.
For [1], demod usually does not share tuner I2C access like this.
For example, tc90522 supports multiple tuner input but has separate
demod blocks for each input with distinct I2C address.
For [2], 'short write' version might work still. (not sure/verified)
(40 02 c1 30 fe 00 00 00 >>>)
And friio can use gl861_ctrl_msg() directly instead.
So, anyway I will try to re-structure i2c in friio
and test/verify it.
Regards,
Akihiro
Re: [PATCH] gl861: re-implement i2c adapter logic
On 8/24/19 2:33 AM, Antti Palosaari wrote: On 8/23/19 8:28 PM, Akihiro TSUKADA wrote: Hi, thanks for the example patch. Here is debug log I tested multibyte i2c writes using zl10353 demod. All returned bytes are not same, but it due to write only register bits I think. dvb_usb_gl861 1-13:1.0: 1 | c0 02 00 1e 50 00 01 00 <<< 03 dvb_usb_gl861 1-13:1.0: 1 | c0 02 00 1e 51 00 01 00 <<< 44 dvb_usb_gl861 1-13:1.0: 1 | c0 02 00 1e 52 00 01 00 <<< 46 dvb_usb_gl861 1-13:1.0: 1 | c0 02 00 1e 53 00 01 00 <<< 15 dvb_usb_gl861 1-13:1.0: 1 | c0 02 00 1e 54 00 01 00 <<< 0f dvb_usb_gl861 1-13:1.0: 5 | 40 03 00 1e 50 00 05 00 >>> 0c 77 aa bb cc dvb_usb_gl861 1-13:1.0: 1 | c0 02 00 1e 50 00 01 00 <<< 0c dvb_usb_gl861 1-13:1.0: 1 | c0 02 00 1e 51 00 01 00 <<< 77 dvb_usb_gl861 1-13:1.0: 1 | c0 02 00 1e 52 00 01 00 <<< aa dvb_usb_gl861 1-13:1.0: 1 | c0 02 00 1e 53 00 01 00 <<< 3b dvb_usb_gl861 1-13:1.0: 1 | c0 02 00 1e 54 00 01 00 <<< 4c Now if you look your tuner i2c implementation... buf[0] = msg->addr << 1; memcpy(buf + 1, msg->buf, msg->len); ret = usb_control_msg(d->udev, usb_sndctrlpipe(d->udev, 0), GL861_REQ_I2C_RAW, GL861_WRITE, priv->i2c_client_demod->addr << (8 + 1), 0xFE, buf, msg->len + 1, 2000); ...it translates same. Log of an 1-byte read from tuner in Friio looks like the following: (re-formatted from my past post: https://patchwork.linuxtv.org/comment/92946/ ) 40 03 00 30 fe 00 01 00 >>> c1 # command a read from the tuner@0x60 (hence 0xc1) c0 02 00 30 00 01 01 00 <<< 7c # get the result (return value: 0x7c) so, - One read is composed of *two* USB messages. (note that friio_tuner_i2c_xfer() does NOT combine the two I2C messages of one read, and issues separate USB message for each, contrary to gl861_i2c_master_xfer()). - The second USB message uses CMD_READ but 'index'(demod register addr) value exceeds 8bit (0x0100), thus cannot use the normal gl861_i2c_master_xfer() as is. It looks to me different. It looks just read command done with 2 separate I2C messages (look I2C specs REPEATED START vs. STOP START). OK, I will add support for bulk I2C READs for adapter too, no problem. See updated patch on ml. Tested it quickly against qt1010 tuner and results are expected: dvb_usb_gl861 1-14:1.0: 0 | 40 01 1a 1e 62 00 00 00 >>> dvb_usb_gl861 1-14:1.0: 1 | c0 02 00 c4 29 00 01 00 <<< 39 dvb_usb_gl861 1-14:1.0: 0 | 40 03 00 c4 29 00 00 00 >>> dvb_usb_gl861 1-14:1.0: 1 | c0 02 00 c4 00 01 01 00 <<< 39 dvb_usb_gl861 1-14:1.0: 1 | c0 02 00 c4 00 01 01 00 <<< 39 dvb_usb_gl861 1-14:1.0: 1 | c0 02 00 c4 00 01 01 00 <<< 39 dvb_usb_gl861 1-14:1.0: 1 | c0 02 00 c4 00 01 01 00 <<< 39 dvb_usb_gl861 1-14:1.0: 0 | 40 01 0a 1e 62 00 00 00 >>> Register 29 is likely chip id and its value is always 39. So it first makes normal write+write to that register which sets and leaves chip registers address counter to that. After that each plain I2C read request gives 39 which is correct content for that register. Antti -- http://palosaari.fi/
Re: [PATCH] gl861: re-implement i2c adapter logic
On 8/23/19 8:28 PM, Akihiro TSUKADA wrote: Hi, thanks for the example patch. Here is debug log I tested multibyte i2c writes using zl10353 demod. All returned bytes are not same, but it due to write only register bits I think. dvb_usb_gl861 1-13:1.0: 1 | c0 02 00 1e 50 00 01 00 <<< 03 dvb_usb_gl861 1-13:1.0: 1 | c0 02 00 1e 51 00 01 00 <<< 44 dvb_usb_gl861 1-13:1.0: 1 | c0 02 00 1e 52 00 01 00 <<< 46 dvb_usb_gl861 1-13:1.0: 1 | c0 02 00 1e 53 00 01 00 <<< 15 dvb_usb_gl861 1-13:1.0: 1 | c0 02 00 1e 54 00 01 00 <<< 0f dvb_usb_gl861 1-13:1.0: 5 | 40 03 00 1e 50 00 05 00 >>> 0c 77 aa bb cc dvb_usb_gl861 1-13:1.0: 1 | c0 02 00 1e 50 00 01 00 <<< 0c dvb_usb_gl861 1-13:1.0: 1 | c0 02 00 1e 51 00 01 00 <<< 77 dvb_usb_gl861 1-13:1.0: 1 | c0 02 00 1e 52 00 01 00 <<< aa dvb_usb_gl861 1-13:1.0: 1 | c0 02 00 1e 53 00 01 00 <<< 3b dvb_usb_gl861 1-13:1.0: 1 | c0 02 00 1e 54 00 01 00 <<< 4c Now if you look your tuner i2c implementation... buf[0] = msg->addr << 1; memcpy(buf + 1, msg->buf, msg->len); ret = usb_control_msg(d->udev, usb_sndctrlpipe(d->udev, 0), GL861_REQ_I2C_RAW, GL861_WRITE, priv->i2c_client_demod->addr << (8 + 1), 0xFE, buf, msg->len + 1, 2000); ...it translates same. Log of an 1-byte read from tuner in Friio looks like the following: (re-formatted from my past post: https://patchwork.linuxtv.org/comment/92946/ ) 40 03 00 30 fe 00 01 00 >>> c1 # command a read from the tuner@0x60 (hence 0xc1) c0 02 00 30 00 01 01 00 <<< 7c # get the result (return value: 0x7c) so, - One read is composed of *two* USB messages. (note that friio_tuner_i2c_xfer() does NOT combine the two I2C messages of one read, and issues separate USB message for each, contrary to gl861_i2c_master_xfer()). - The second USB message uses CMD_READ but 'index'(demod register addr) value exceeds 8bit (0x0100), thus cannot use the normal gl861_i2c_master_xfer() as is. It looks to me different. It looks just read command done with 2 separate I2C messages (look I2C specs REPEATED START vs. STOP START). OK, I will add support for bulk I2C READs for adapter too, no problem. Antti -- http://palosaari.fi/
Re: [PATCH] gl861: re-implement i2c adapter logic
Hi, thanks for the example patch. > Here is debug log I tested multibyte i2c writes using zl10353 demod. All > returned bytes are not same, but it due to write only register bits I > think. > > dvb_usb_gl861 1-13:1.0: 1 | c0 02 00 1e 50 00 01 00 <<< 03 > dvb_usb_gl861 1-13:1.0: 1 | c0 02 00 1e 51 00 01 00 <<< 44 > dvb_usb_gl861 1-13:1.0: 1 | c0 02 00 1e 52 00 01 00 <<< 46 > dvb_usb_gl861 1-13:1.0: 1 | c0 02 00 1e 53 00 01 00 <<< 15 > dvb_usb_gl861 1-13:1.0: 1 | c0 02 00 1e 54 00 01 00 <<< 0f > dvb_usb_gl861 1-13:1.0: 5 | 40 03 00 1e 50 00 05 00 >>> 0c 77 aa bb cc > dvb_usb_gl861 1-13:1.0: 1 | c0 02 00 1e 50 00 01 00 <<< 0c > dvb_usb_gl861 1-13:1.0: 1 | c0 02 00 1e 51 00 01 00 <<< 77 > dvb_usb_gl861 1-13:1.0: 1 | c0 02 00 1e 52 00 01 00 <<< aa > dvb_usb_gl861 1-13:1.0: 1 | c0 02 00 1e 53 00 01 00 <<< 3b > dvb_usb_gl861 1-13:1.0: 1 | c0 02 00 1e 54 00 01 00 <<< 4c > > > Now if you look your tuner i2c implementation... > > buf[0] = msg->addr << 1; > memcpy(buf + 1, msg->buf, msg->len); > ret = usb_control_msg(d->udev, usb_sndctrlpipe(d->udev, 0), > GL861_REQ_I2C_RAW, GL861_WRITE, > priv->i2c_client_demod->addr << (8 + 1), 0xFE, buf, msg->len + 1, 2000); > > ...it translates same. Log of an 1-byte read from tuner in Friio looks like the following: (re-formatted from my past post: https://patchwork.linuxtv.org/comment/92946/ ) 40 03 00 30 fe 00 01 00 >>> c1 # command a read from the tuner@0x60 (hence 0xc1) c0 02 00 30 00 01 01 00 <<< 7c # get the result (return value: 0x7c) so, - One read is composed of *two* USB messages. (note that friio_tuner_i2c_xfer() does NOT combine the two I2C messages of one read, and issues separate USB message for each, contrary to gl861_i2c_master_xfer()). - The second USB message uses CMD_READ but 'index'(demod register addr) value exceeds 8bit (0x0100), thus cannot use the normal gl861_i2c_master_xfer() as is. It looks to me different. Regards, Akihiro
Re: [PATCH] gl861: re-implement i2c adapter logic
On 8/22/19 8:34 AM, Antti Palosaari wrote: Device I2C adapter is capable of writing and reading large messages. For I2C writes there is 2 methods: simple for max 2 byte messages and usb_control_msg() with payload data for larger I2C messages. Add I2C adapter logic which selects suitable method according to message size. Here is debug log I tested multibyte i2c writes using zl10353 demod. All returned bytes are not same, but it due to write only register bits I think. dvb_usb_gl861 1-13:1.0: 1 | c0 02 00 1e 50 00 01 00 <<< 03 dvb_usb_gl861 1-13:1.0: 1 | c0 02 00 1e 51 00 01 00 <<< 44 dvb_usb_gl861 1-13:1.0: 1 | c0 02 00 1e 52 00 01 00 <<< 46 dvb_usb_gl861 1-13:1.0: 1 | c0 02 00 1e 53 00 01 00 <<< 15 dvb_usb_gl861 1-13:1.0: 1 | c0 02 00 1e 54 00 01 00 <<< 0f dvb_usb_gl861 1-13:1.0: 5 | 40 03 00 1e 50 00 05 00 >>> 0c 77 aa bb cc dvb_usb_gl861 1-13:1.0: 1 | c0 02 00 1e 50 00 01 00 <<< 0c dvb_usb_gl861 1-13:1.0: 1 | c0 02 00 1e 51 00 01 00 <<< 77 dvb_usb_gl861 1-13:1.0: 1 | c0 02 00 1e 52 00 01 00 <<< aa dvb_usb_gl861 1-13:1.0: 1 | c0 02 00 1e 53 00 01 00 <<< 3b dvb_usb_gl861 1-13:1.0: 1 | c0 02 00 1e 54 00 01 00 <<< 4c Now if you look your tuner i2c implementation... buf[0] = msg->addr << 1; memcpy(buf + 1, msg->buf, msg->len); ret = usb_control_msg(d->udev, usb_sndctrlpipe(d->udev, 0), GL861_REQ_I2C_RAW, GL861_WRITE, priv->i2c_client_demod->addr << (8 + 1), 0xFE, buf, msg->len + 1, 2000); ...it translates same. It writes i2c message to demod which; byte0 0xfe, demod register/cmd/mailbox for tuner i2c bus byte1 tuner i2c address byte2-n tuner i2c data Antti -- http://palosaari.fi/
