| Message ID | 20170111100819.2190-1-oleg@kaa.org.ua (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
> diff --git a/drivers/media/usb/cx231xx/cx231xx-i2c.c b/drivers/media/usb/cx231xx/cx231xx-i2c.c > index 35e9acf..60412ec 100644 > --- a/drivers/media/usb/cx231xx/cx231xx-i2c.c > +++ b/drivers/media/usb/cx231xx/cx231xx-i2c.c > @@ -171,6 +171,43 @@ static int cx231xx_i2c_send_bytes(struct i2c_adapter *i2c_adap, > bus->i2c_nostop = 0; > bus->i2c_reserve = 0; > > + } else if (dev->model == CX231XX_BOARD_EVROMEDIA_FULL_HYBRID_FULLHD > + && msg->addr == dev->tuner_addr > + && msg->len > 4) { > + /* special case for Evromedia USB Full Hybrid Full HD tuner chip */ > + size = msg->len; > + saddr_len = 1; > + > + /* adjust the length to correct length */ > + size -= saddr_len; > + > + buf_ptr = (u8*)(msg->buf + 1); > + > + do { > + /* prepare xfer_data struct */ > + req_data.dev_addr = msg->addr; > + req_data.direction = msg->flags; > + req_data.saddr_len = saddr_len; > + req_data.saddr_dat = msg->buf[0]; > + req_data.buf_size = size > 4 ? 4 : size; > + req_data.p_buffer = (u8*)(buf_ptr + loop * 4); > + > + bus->i2c_nostop = (size > 4) ? 1 : 0; > + bus->i2c_reserve = (loop == 0) ? 0 : 1; > + > + /* usb send command */ > + status = dev->cx231xx_send_usb_command(bus, &req_data); > + loop++; > + > + if (size >= 4) { > + size -= 4; > + } else { > + size = 0; > + } > + } while (size > 0); > + > + bus->i2c_nostop = 0; > + bus->i2c_reserve = 0; > } else { /* regular case */ > > /* prepare xfer_data struct */ If the i2c functionality is broken in some way, I suggest its fixed first, along with a correct patch description, as a separate piece of work. Lets not group this in with a board profile. Almost certainly we should never see a "if board == X" in any i2c implementation without proper discussion.
On 11.01.17 15:53, Steven Toth wrote: >> diff --git a/drivers/media/usb/cx231xx/cx231xx-i2c.c b/drivers/media/usb/cx231xx/cx231xx-i2c.c >> index 35e9acf..60412ec 100644 >> --- a/drivers/media/usb/cx231xx/cx231xx-i2c.c >> +++ b/drivers/media/usb/cx231xx/cx231xx-i2c.c >> @@ -171,6 +171,43 @@ static int cx231xx_i2c_send_bytes(struct i2c_adapter *i2c_adap, >> bus->i2c_nostop = 0; >> bus->i2c_reserve = 0; >> >> + } else if (dev->model == CX231XX_BOARD_EVROMEDIA_FULL_HYBRID_FULLHD >> + && msg->addr == dev->tuner_addr >> + && msg->len > 4) { >> + /* special case for Evromedia USB Full Hybrid Full HD tuner chip */ >> + size = msg->len; >> + saddr_len = 1; >> + >> + /* adjust the length to correct length */ >> + size -= saddr_len; >> + >> + buf_ptr = (u8*)(msg->buf + 1); >> + >> + do { >> + /* prepare xfer_data struct */ >> + req_data.dev_addr = msg->addr; >> + req_data.direction = msg->flags; >> + req_data.saddr_len = saddr_len; >> + req_data.saddr_dat = msg->buf[0]; >> + req_data.buf_size = size > 4 ? 4 : size; >> + req_data.p_buffer = (u8*)(buf_ptr + loop * 4); >> + >> + bus->i2c_nostop = (size > 4) ? 1 : 0; >> + bus->i2c_reserve = (loop == 0) ? 0 : 1; >> + >> + /* usb send command */ >> + status = dev->cx231xx_send_usb_command(bus, &req_data); >> + loop++; >> + >> + if (size >= 4) { >> + size -= 4; >> + } else { >> + size = 0; >> + } >> + } while (size > 0); >> + >> + bus->i2c_nostop = 0; >> + bus->i2c_reserve = 0; >> } else { /* regular case */ >> >> /* prepare xfer_data struct */ > If the i2c functionality is broken in some way, I suggest its fixed > first, along with a correct patch description, as a separate piece of > work. Lets not group this in with a board profile. > > Almost certainly we should never see a "if board == X" in any i2c > implementation without proper discussion. > I'm interested in accepting this patch :) What I should do?
On Wed, Jan 11, 2017 at 9:00 AM, Oleh Kravchenko <oleg@kaa.org.ua> wrote: > On 11.01.17 15:53, Steven Toth wrote: >>> diff --git a/drivers/media/usb/cx231xx/cx231xx-i2c.c b/drivers/media/usb/cx231xx/cx231xx-i2c.c >>> index 35e9acf..60412ec 100644 >>> --- a/drivers/media/usb/cx231xx/cx231xx-i2c.c >>> +++ b/drivers/media/usb/cx231xx/cx231xx-i2c.c >>> @@ -171,6 +171,43 @@ static int cx231xx_i2c_send_bytes(struct i2c_adapter *i2c_adap, >>> bus->i2c_nostop = 0; >>> bus->i2c_reserve = 0; >>> >>> + } else if (dev->model == CX231XX_BOARD_EVROMEDIA_FULL_HYBRID_FULLHD >>> + && msg->addr == dev->tuner_addr >>> + && msg->len > 4) { >>> + /* special case for Evromedia USB Full Hybrid Full HD tuner chip */ >>> + size = msg->len; >>> + saddr_len = 1; >>> + >>> + /* adjust the length to correct length */ >>> + size -= saddr_len; >>> + >>> + buf_ptr = (u8*)(msg->buf + 1); >>> + >>> + do { >>> + /* prepare xfer_data struct */ >>> + req_data.dev_addr = msg->addr; >>> + req_data.direction = msg->flags; >>> + req_data.saddr_len = saddr_len; >>> + req_data.saddr_dat = msg->buf[0]; >>> + req_data.buf_size = size > 4 ? 4 : size; >>> + req_data.p_buffer = (u8*)(buf_ptr + loop * 4); >>> + >>> + bus->i2c_nostop = (size > 4) ? 1 : 0; >>> + bus->i2c_reserve = (loop == 0) ? 0 : 1; >>> + >>> + /* usb send command */ >>> + status = dev->cx231xx_send_usb_command(bus, &req_data); >>> + loop++; >>> + >>> + if (size >= 4) { >>> + size -= 4; >>> + } else { >>> + size = 0; >>> + } >>> + } while (size > 0); >>> + >>> + bus->i2c_nostop = 0; >>> + bus->i2c_reserve = 0; >>> } else { /* regular case */ >>> >>> /* prepare xfer_data struct */ >> If the i2c functionality is broken in some way, I suggest its fixed >> first, along with a correct patch description, as a separate piece of >> work. Lets not group this in with a board profile. >> >> Almost certainly we should never see a "if board == X" in any i2c >> implementation without proper discussion. >> > I'm interested in accepting this patch :) What I should do? Lets start the conversion with what's wrong with the current implementation that prevents it from working with the new design. Lets discuss the I2C needs of the parts you need to communicate with, and why the current design is broken. Then, lets think about a better solution. A BOARD == X is the wrong solution to the problem.
Hello!
So, I tried to split i2c send messages to 5, 8, 16 bytes - that cause error:
cx231xx 3-1:1.1: cx231xx_send_usb_command: failed with status --32
This problem occurred only with Si2158 tuner,
demodulator Si2168 eats up to 15 bytes per i2c message.
I think problem in cx231xx_send_vendor_cmd() function.
It's split reading, but not writing.
PS:
I use USB sniffer to capture Windows traffic, please find it in attachment.
Sorry, in last mail I forget to compress it.
On Wed, Jan 11, 2017 at 9:39 AM, Oleh Kravchenko <oleg@kaa.org.ua> wrote: > Hello! > > So, I tried to split i2c send messages to 5, 8, 16 bytes - that cause error: > cx231xx 3-1:1.1: cx231xx_send_usb_command: failed with status --32 > > This problem occurred only with Si2158 tuner, > demodulator Si2168 eats up to 15 bytes per i2c message. > > I think problem in cx231xx_send_vendor_cmd() function. > It's split reading, but not writing. > > PS: I use USB sniffer to capture traffic, please find it in attachment. Do you have a logic analyzer so you can inspect the I2C bus state? I'm speaking as someone who's done significant reverse engineering projects on many pieces of hardware, you'll get a much better understanding of platform specific I2C problems if you can see the bus directly. Any such issues tend to propagate upwards through micro-controller interfaces (cx231xx) and its much more obvious to deal with the controller problem when the state of the bus is absolutely known. I have no objections to a I2C bus controller implementation splitting reads and writes in a uniform and generic way, assuming windows is doing the same thing, providing that approach applies to all attached devices. We don't want board specific hacks infecting the various drivers......... unless we're absolutely clear as to why...... and we don't want to see I2C controller limitations infecting I2C demod/tuner drivers where at all possible. If its true that send_vendor_cmd() splits reads but not writes, fix the function to be uniform for all boards. Don't do boards specific overrides.
Hello! Dear users of tv-tuners based cx231xx. Could you please connect tuner to PC and send to me output of "lsusb -v" command? I would like to clarify hardware revision, stored in bcdDevice usb descriptor. Thank you! -- 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
diff --git a/drivers/media/usb/cx231xx/Kconfig b/drivers/media/usb/cx231xx/Kconfig index 0cced3e..58de80b 100644 --- a/drivers/media/usb/cx231xx/Kconfig +++ b/drivers/media/usb/cx231xx/Kconfig @@ -50,6 +50,7 @@ config VIDEO_CX231XX_DVB select DVB_LGDT3306A if MEDIA_SUBDRV_AUTOSELECT select DVB_TDA18271C2DD if MEDIA_SUBDRV_AUTOSELECT select DVB_SI2165 if MEDIA_SUBDRV_AUTOSELECT + select DVB_SI2168 if MEDIA_SUBDRV_AUTOSELECT select MEDIA_TUNER_SI2157 if MEDIA_SUBDRV_AUTOSELECT ---help--- diff --git a/drivers/media/usb/cx231xx/cx231xx-cards.c b/drivers/media/usb/cx231xx/cx231xx-cards.c index 36bc254..f730fdb 100644 --- a/drivers/media/usb/cx231xx/cx231xx-cards.c +++ b/drivers/media/usb/cx231xx/cx231xx-cards.c @@ -841,6 +841,33 @@ struct cx231xx_board cx231xx_boards[] = { .gpio = NULL, } }, }, + [CX231XX_BOARD_EVROMEDIA_FULL_HYBRID_FULLHD] = { + .name = "Evromedia USB Full Hybrid Full HD", + .tuner_type = TUNER_ABSENT, + .demod_addr = 0x64, /* 0xc8 >> 1 */ + .demod_i2c_master = I2C_1_MUX_3, + .has_dvb = 1, + .ir_i2c_master = I2C_0, + .norm = V4L2_STD_PAL, + .output_mode = OUT_MODE_VIP11, + .tuner_addr = 0x60, /* 0xc0 >> 1 */ + .tuner_i2c_master = I2C_2, + .input = {{ + .type = CX231XX_VMUX_TELEVISION, + .vmux = 0, + .amux = CX231XX_AMUX_VIDEO, + }, { + .type = CX231XX_VMUX_COMPOSITE1, + .vmux = CX231XX_VIN_2_1, + .amux = CX231XX_AMUX_LINE_IN, + }, { + .type = CX231XX_VMUX_SVIDEO, + .vmux = CX231XX_VIN_1_1 | + (CX231XX_VIN_1_2 << 8) | + CX25840_SVIDEO_ON, + .amux = CX231XX_AMUX_LINE_IN, + } }, + }, }; const unsigned int cx231xx_bcount = ARRAY_SIZE(cx231xx_boards); @@ -908,6 +935,8 @@ struct usb_device_id cx231xx_id_table[] = { .driver_info = CX231XX_BOARD_OTG102}, {USB_DEVICE(USB_VID_TERRATEC, 0x00a6), .driver_info = CX231XX_BOARD_TERRATEC_GRABBY}, + {USB_DEVICE(0x1b80, 0xd3b2), + .driver_info = CX231XX_BOARD_EVROMEDIA_FULL_HYBRID_FULLHD}, {}, }; diff --git a/drivers/media/usb/cx231xx/cx231xx-dvb.c b/drivers/media/usb/cx231xx/cx231xx-dvb.c index 1417515..cf75c0a 100644 --- a/drivers/media/usb/cx231xx/cx231xx-dvb.c +++ b/drivers/media/usb/cx231xx/cx231xx-dvb.c @@ -33,6 +33,7 @@ #include "s5h1411.h" #include "lgdt3305.h" #include "si2165.h" +#include "si2168.h" #include "mb86a20s.h" #include "si2157.h" #include "lgdt3306a.h" @@ -949,6 +950,75 @@ static int dvb_init(struct cx231xx *dev) &pv_tda18271_config); break; + case CX231XX_BOARD_EVROMEDIA_FULL_HYBRID_FULLHD: + { + struct si2157_config si2157_config = {}; + struct si2168_config si2168_config = {}; + struct i2c_board_info info = {}; + struct i2c_client *client; + struct i2c_adapter *adapter; + + /* attach demodulator chip */ + si2168_config.ts_mode = SI2168_TS_SERIAL; /* from *.inf file */ + si2168_config.fe = &dev->dvb->frontend; + si2168_config.i2c_adapter = &adapter; + si2168_config.ts_clock_inv = true; + + strlcpy(info.type, "si2168", sizeof(info.type)); + info.addr = dev->board.demod_addr; + info.platform_data = &si2168_config; + + request_module(info.type); + client = i2c_new_device(demod_i2c, &info); + + if (client == NULL || client->dev.driver == NULL) { + result = -ENODEV; + goto out_free; + } + + if (!try_module_get(client->dev.driver->owner)) { + i2c_unregister_device(client); + result = -ENODEV; + goto out_free; + } + + dvb->i2c_client_demod = client; + + /* attach tuner chip */ + si2157_config.fe = dev->dvb->frontend; +#ifdef CONFIG_MEDIA_CONTROLLER_DVB + si2157_config.mdev = dev->media_dev; +#endif + si2157_config.if_port = 1; + si2157_config.inversion = false; + + memset(&info, 0, sizeof(info)); + strlcpy(info.type, "si2157", sizeof(info.type)); + info.addr = dev->board.tuner_addr; + info.platform_data = &si2157_config; + + request_module(info.type); + client = i2c_new_device(tuner_i2c, &info); + + if (client == NULL || client->dev.driver == NULL) { + module_put(dvb->i2c_client_demod->dev.driver->owner); + i2c_unregister_device(dvb->i2c_client_demod); + result = -ENODEV; + goto out_free; + } + + if (!try_module_get(client->dev.driver->owner)) { + i2c_unregister_device(client); + module_put(dvb->i2c_client_demod->dev.driver->owner); + i2c_unregister_device(dvb->i2c_client_demod); + result = -ENODEV; + goto out_free; + } + + dev->cx231xx_reset_analog_tuner = NULL; + dev->dvb->i2c_client_tuner = client; + break; + } default: dev_err(dev->dev, "%s/2: The frontend of your DVB/ATSC card isn't supported yet\n", diff --git a/drivers/media/usb/cx231xx/cx231xx-i2c.c b/drivers/media/usb/cx231xx/cx231xx-i2c.c index 35e9acf..60412ec 100644 --- a/drivers/media/usb/cx231xx/cx231xx-i2c.c +++ b/drivers/media/usb/cx231xx/cx231xx-i2c.c @@ -171,6 +171,43 @@ static int cx231xx_i2c_send_bytes(struct i2c_adapter *i2c_adap, bus->i2c_nostop = 0; bus->i2c_reserve = 0; + } else if (dev->model == CX231XX_BOARD_EVROMEDIA_FULL_HYBRID_FULLHD + && msg->addr == dev->tuner_addr + && msg->len > 4) { + /* special case for Evromedia USB Full Hybrid Full HD tuner chip */ + size = msg->len; + saddr_len = 1; + + /* adjust the length to correct length */ + size -= saddr_len; + + buf_ptr = (u8*)(msg->buf + 1); + + do { + /* prepare xfer_data struct */ + req_data.dev_addr = msg->addr; + req_data.direction = msg->flags; + req_data.saddr_len = saddr_len; + req_data.saddr_dat = msg->buf[0]; + req_data.buf_size = size > 4 ? 4 : size; + req_data.p_buffer = (u8*)(buf_ptr + loop * 4); + + bus->i2c_nostop = (size > 4) ? 1 : 0; + bus->i2c_reserve = (loop == 0) ? 0 : 1; + + /* usb send command */ + status = dev->cx231xx_send_usb_command(bus, &req_data); + loop++; + + if (size >= 4) { + size -= 4; + } else { + size = 0; + } + } while (size > 0); + + bus->i2c_nostop = 0; + bus->i2c_reserve = 0; } else { /* regular case */ /* prepare xfer_data struct */ diff --git a/drivers/media/usb/cx231xx/cx231xx.h b/drivers/media/usb/cx231xx/cx231xx.h index 90c8676..d9792ea 100644 --- a/drivers/media/usb/cx231xx/cx231xx.h +++ b/drivers/media/usb/cx231xx/cx231xx.h @@ -78,6 +78,7 @@ #define CX231XX_BOARD_HAUPPAUGE_930C_HD_1114xx 20 #define CX231XX_BOARD_HAUPPAUGE_955Q 21 #define CX231XX_BOARD_TERRATEC_GRABBY 22 +#define CX231XX_BOARD_EVROMEDIA_FULL_HYBRID_FULLHD 23 /* Limits minimum and default number of buffers */ #define CX231XX_MIN_BUF 4