| Message ID | 20190822053452.20168-1-crope@iki.fi (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | gl861: re-implement i2c adapter logic | expand |
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
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
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
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
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@<I2C addr XX>
40 03 00 30 fe 00 01 00 >>> (XX << 1) | 1
2. write another read command to tuner@<I2C addr YY>
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
diff --git a/drivers/media/usb/dvb-usb-v2/gl861.c b/drivers/media/usb/dvb-usb-v2/gl861.c index b784d9da1a82..b8358cd2e4b7 100644 --- a/drivers/media/usb/dvb-usb-v2/gl861.c +++ b/drivers/media/usb/dvb-usb-v2/gl861.c @@ -14,6 +14,144 @@ DVB_DEFINE_MOD_OPT_ADAPTER_NR(adapter_nr); +struct gl861 { + /* USB control message buffer */ + u8 buf[16]; + + struct i2c_adapter *demod_sub_i2c; + struct i2c_client *i2c_client_demod; + struct i2c_client *i2c_client_tuner; + struct i2c_adapter tuner_adap; +}; + +#define CMD_WRITE_SHORT 0x01 +#define CMD_READ 0x02 +#define CMD_WRITE 0x03 + +static int gl861_ctrl_msg(struct dvb_usb_device *d, u8 request, u16 value, + u16 index, void *data, u16 size) +{ + struct gl861 *ctx = d_to_priv(d); + struct usb_interface *intf = d->intf; + int ret; + unsigned int pipe; + u8 requesttype; + + mutex_lock(&d->usb_mutex); + + switch (request) { + case CMD_WRITE_SHORT: + pipe = usb_sndctrlpipe(d->udev, 0); + requesttype = USB_TYPE_VENDOR | USB_DIR_OUT; + break; + case CMD_READ: + pipe = usb_rcvctrlpipe(d->udev, 0); + requesttype = USB_TYPE_VENDOR | USB_DIR_IN; + break; + case CMD_WRITE: + pipe = usb_sndctrlpipe(d->udev, 0); + requesttype = USB_TYPE_VENDOR | USB_DIR_OUT; + memcpy(ctx->buf, data, size); + break; + default: + ret = -EINVAL; + goto err_mutex_unlock; + } + + ret = usb_control_msg(d->udev, pipe, request, requesttype, value, + index, ctx->buf, size, 200); + dev_dbg(&intf->dev, "%d | %02x %02x %*ph %*ph %*ph %s %*ph\n", + ret, requesttype, request, 2, &value, 2, &index, 2, &size, + (requesttype & USB_DIR_IN) ? "<<<" : ">>>", size, ctx->buf); + if (ret < 0) + goto err_mutex_unlock; + + if (request == CMD_READ) + memcpy(data, ctx->buf, size); + + usleep_range(1000, 2000); /* Avoid I2C errors */ + + mutex_unlock(&d->usb_mutex); + + return 0; + +err_mutex_unlock: + mutex_unlock(&d->usb_mutex); + dev_dbg(&intf->dev, "failed %d\n", ret); + return ret; +} + +static int gl861_i2c_master_xfer(struct i2c_adapter *adap, struct i2c_msg msg[], + int num) +{ + struct dvb_usb_device *d = i2c_get_adapdata(adap); + struct usb_interface *intf = d->intf; + struct gl861 *ctx = d_to_priv(d); + int ret; + u8 request, *data; + u16 value, index, size; + + /* XXX: I2C adapter maximum data lengths are not tested */ + if (num == 1 && !(msg[0].flags & I2C_M_RD)) { + /* I2C write */ + if (msg[0].len < 2 || msg[0].len > sizeof(ctx->buf)) { + ret = -EOPNOTSUPP; + goto err; + } + + value = (msg[0].addr << 1) << 8; + index = msg[0].buf[0]; + + if (msg[0].len == 2) { + request = CMD_WRITE_SHORT; + value |= msg[0].buf[1]; + size = 0; + data = NULL; + } else { + request = CMD_WRITE; + size = msg[0].len - 1; + data = &msg[0].buf[1]; + } + + ret = gl861_ctrl_msg(d, request, value, index, data, size); + } else if (num == 2 && !(msg[0].flags & I2C_M_RD) && + (msg[1].flags & I2C_M_RD)) { + /* I2C write + read */ + if (msg[0].len > 1 || msg[1].len > sizeof(ctx->buf)) { + ret = -EOPNOTSUPP; + goto err; + } + + value = (msg[0].addr << 1) << 8; + index = msg[0].buf[0]; + request = CMD_READ; + + ret = gl861_ctrl_msg(d, request, value, index, + msg[1].buf, msg[1].len); + } else { + /* Unsupported I2C message */ + dev_dbg(&intf->dev, "unknown i2c msg, num %u\n", num); + ret = -EOPNOTSUPP; + } + if (ret) + goto err; + + return num; +err: + dev_dbg(&intf->dev, "failed %d\n", ret); + return ret; +} + +static u32 gl861_i2c_functionality(struct i2c_adapter *adapter) +{ + return I2C_FUNC_I2C; +} + +static struct i2c_algorithm gl861_i2c_algo = { + .master_xfer = gl861_i2c_master_xfer, + .functionality = gl861_i2c_functionality, +}; + static int gl861_i2c_msg(struct dvb_usb_device *d, u8 addr, u8 *wbuf, u16 wlen, u8 *rbuf, u16 rlen) { @@ -63,46 +201,6 @@ static int gl861_i2c_msg(struct dvb_usb_device *d, u8 addr, return ret; } -/* I2C */ -static int gl861_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg msg[], - int num) -{ - struct dvb_usb_device *d = i2c_get_adapdata(adap); - int i; - - if (num > 2) - return -EINVAL; - - if (mutex_lock_interruptible(&d->i2c_mutex) < 0) - return -EAGAIN; - - for (i = 0; i < num; i++) { - /* write/read request */ - if (i+1 < num && (msg[i+1].flags & I2C_M_RD)) { - if (gl861_i2c_msg(d, msg[i].addr, msg[i].buf, - msg[i].len, msg[i+1].buf, msg[i+1].len) < 0) - break; - i++; - } else - if (gl861_i2c_msg(d, msg[i].addr, msg[i].buf, - msg[i].len, NULL, 0) < 0) - break; - } - - mutex_unlock(&d->i2c_mutex); - return i; -} - -static u32 gl861_i2c_func(struct i2c_adapter *adapter) -{ - return I2C_FUNC_I2C; -} - -static struct i2c_algorithm gl861_i2c_algo = { - .master_xfer = gl861_i2c_xfer, - .functionality = gl861_i2c_func, -}; - /* Callbacks for DVB USB */ static struct zl10353_config gl861_zl10353_config = { .demod_address = 0x0f, @@ -149,6 +247,8 @@ static struct dvb_usb_device_properties gl861_props = { .owner = THIS_MODULE, .adapter_nr = adapter_nr, + .size_of_priv = sizeof(struct gl861), + .i2c_algo = &gl861_i2c_algo, .frontend_attach = gl861_frontend_attach, .tuner_attach = gl861_tuner_attach, @@ -166,14 +266,6 @@ static struct dvb_usb_device_properties gl861_props = { /* * For Friio */ - -struct friio_priv { - struct i2c_adapter *demod_sub_i2c; - struct i2c_client *i2c_client_demod; - struct i2c_client *i2c_client_tuner; - struct i2c_adapter tuner_adap; -}; - struct friio_config { struct i2c_board_info demod_info; struct tc90522_config demod_cfg; @@ -242,7 +334,7 @@ gl861_i2c_read_ex(struct dvb_usb_device *d, u8 addr, u8 *rbuf, u16 rlen) static int friio_i2c_tuner_read(struct dvb_usb_device *d, struct i2c_msg *msg) { - struct friio_priv *priv; + struct gl861 *priv; u8 addr; priv = d_to_priv(d); @@ -255,7 +347,7 @@ friio_i2c_tuner_write(struct dvb_usb_device *d, struct i2c_msg *msg) { u8 *buf; int ret; - struct friio_priv *priv; + struct gl861 *priv; priv = d_to_priv(d); @@ -308,7 +400,7 @@ static int friio_tuner_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg msg[], static struct i2c_algorithm friio_tuner_i2c_algo = { .master_xfer = friio_tuner_i2c_xfer, - .functionality = gl861_i2c_func, + .functionality = gl861_i2c_functionality, }; /* GPIO control in Friio */ @@ -488,7 +580,7 @@ static int friio_frontend_attach(struct dvb_usb_adapter *adap) struct dvb_usb_device *d; struct tc90522_config cfg; struct i2c_client *cl; - struct friio_priv *priv; + struct gl861 *priv; info = &friio_config.demod_info; d = adap_to_d(adap); @@ -513,7 +605,7 @@ static int friio_frontend_attach(struct dvb_usb_adapter *adap) static int friio_frontend_detach(struct dvb_usb_adapter *adap) { - struct friio_priv *priv; + struct gl861 *priv; priv = adap_to_priv(adap); i2c_del_adapter(&priv->tuner_adap); @@ -526,7 +618,7 @@ static int friio_tuner_attach(struct dvb_usb_adapter *adap) const struct i2c_board_info *info; struct dvb_pll_config cfg; struct i2c_client *cl; - struct friio_priv *priv; + struct gl861 *priv; priv = adap_to_priv(adap); info = &friio_config.tuner_info; @@ -543,7 +635,7 @@ static int friio_tuner_attach(struct dvb_usb_adapter *adap) static int friio_tuner_detach(struct dvb_usb_adapter *adap) { - struct friio_priv *priv; + struct gl861 *priv; priv = adap_to_priv(adap); dvb_module_release(priv->i2c_client_tuner); @@ -554,7 +646,7 @@ static int friio_init(struct dvb_usb_device *d) { int i; int ret; - struct friio_priv *priv; + struct gl861 *priv; static const u8 demod_init[][2] = { {0x01, 0x40}, {0x04, 0x38}, {0x05, 0x40}, {0x07, 0x40}, @@ -606,7 +698,7 @@ static struct dvb_usb_device_properties friio_props = { .owner = THIS_MODULE, .adapter_nr = adapter_nr, - .size_of_priv = sizeof(struct friio_priv), + .size_of_priv = sizeof(struct gl861), .i2c_algo = &gl861_i2c_algo, .power_ctrl = friio_power_ctrl,
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. Cc: Akihiro TSUKADA <tskd08@gmail.com> Signed-off-by: Antti Palosaari <crope@iki.fi> --- drivers/media/usb/dvb-usb-v2/gl861.c | 206 +++++++++++++++++++-------- 1 file changed, 149 insertions(+), 57 deletions(-)