| Message ID | 20191002141359.30166-2-gonsolo@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | si2157: Add support for Logilink VG0022A. | expand |
On Wed, Oct 02, 2019 at 04:13:59PM +0200, Gon Solo wrote: You need a message and a Signed-off-by: here. > --- > drivers/media/tuners/si2157.c | 68 +++++++++++++++++---------- > drivers/media/tuners/si2157_priv.h | 1 + > drivers/media/usb/dvb-usb-v2/af9035.c | 47 ++++++++++++++++++ > 3 files changed, 90 insertions(+), 26 deletions(-) > > diff --git a/drivers/media/tuners/si2157.c b/drivers/media/tuners/si2157.c > index e87040d6eca7..8f9df2485d51 100644 > --- a/drivers/media/tuners/si2157.c > +++ b/drivers/media/tuners/si2157.c > @@ -66,6 +66,24 @@ static int si2157_cmd_execute(struct i2c_client *client, struct si2157_cmd *cmd) > return ret; > } > > +static int si2157_power_up(struct si2157_dev *dev, struct i2c_client *client) > +{ > + struct si2157_cmd cmd; > + > + if (dev->chiptype == SI2157_CHIPTYPE_SI2146) { > + memcpy(cmd.args, "\xc0\x05\x01\x00\x00\x0b\x00\x00\x01", 9); > + cmd.wlen = 9; > + } else if (dev->chiptype == SI2157_CHIPTYPE_SI2141) { > + memcpy(cmd.args, "\xc0\x00\x0d\x0e\x00\x01\x01\x01\x01\x03", 10); > + cmd.wlen = 10; > + } else { > + memcpy(cmd.args, "\xc0\x00\x0c\x00\x00\x01\x01\x01\x01\x01\x01\x02\x00\x00\x01", 15); > + cmd.wlen = 15; > + } > + cmd.rlen = 1; > + return si2157_cmd_execute(client, &cmd); > +} > + > static int si2157_init(struct dvb_frontend *fe) > { > struct i2c_client *client = fe->tuner_priv; > @@ -75,7 +93,7 @@ static int si2157_init(struct dvb_frontend *fe) > struct si2157_cmd cmd; > const struct firmware *fw; > const char *fw_name; > - unsigned int uitmp, chip_id; > + unsigned int uitmp; > > dev_dbg(&client->dev, "\n"); > > @@ -93,19 +111,7 @@ static int si2157_init(struct dvb_frontend *fe) > if (uitmp == dev->if_frequency / 1000) > goto warm; > > - /* power up */ > - if (dev->chiptype == SI2157_CHIPTYPE_SI2146) { > - memcpy(cmd.args, "\xc0\x05\x01\x00\x00\x0b\x00\x00\x01", 9); > - cmd.wlen = 9; > - } else if (dev->chiptype == SI2157_CHIPTYPE_SI2141) { > - memcpy(cmd.args, "\xc0\x00\x0d\x0e\x00\x01\x01\x01\x01\x03", 10); > - cmd.wlen = 10; > - } else { > - memcpy(cmd.args, "\xc0\x00\x0c\x00\x00\x01\x01\x01\x01\x01\x01\x02\x00\x00\x01", 15); > - cmd.wlen = 15; > - } > - cmd.rlen = 1; > - ret = si2157_cmd_execute(client, &cmd); > + ret = si2157_power_up(dev, client); > if (ret) > goto err; > > @@ -118,17 +124,6 @@ static int si2157_init(struct dvb_frontend *fe) > goto err; > } > > - /* query chip revision */ > - memcpy(cmd.args, "\x02", 1); > - cmd.wlen = 1; > - cmd.rlen = 13; > - ret = si2157_cmd_execute(client, &cmd); > - if (ret) > - goto err; > - > - chip_id = cmd.args[1] << 24 | cmd.args[2] << 16 | cmd.args[3] << 8 | > - cmd.args[4] << 0; > - > #define SI2177_A30 ('A' << 24 | 77 << 16 | '3' << 8 | '0' << 0) > #define SI2158_A20 ('A' << 24 | 58 << 16 | '2' << 8 | '0' << 0) > #define SI2148_A20 ('A' << 24 | 48 << 16 | '2' << 8 | '0' << 0) > @@ -137,7 +132,7 @@ static int si2157_init(struct dvb_frontend *fe) > #define SI2146_A10 ('A' << 24 | 46 << 16 | '1' << 8 | '0' << 0) > #define SI2141_A10 ('A' << 24 | 41 << 16 | '1' << 8 | '0' << 0) > > - switch (chip_id) { > + switch (dev->chip_id) { > case SI2158_A20: > case SI2148_A20: > fw_name = SI2158_A20_FIRMWARE; > @@ -456,6 +451,27 @@ static int si2157_probe(struct i2c_client *client, > memcpy(&fe->ops.tuner_ops, &si2157_ops, sizeof(struct dvb_tuner_ops)); > fe->tuner_priv = client; > > + ret = si2157_power_up(dev, client); > + if (ret) > + goto err; > + /* query chip revision */ > + /* hack: do it here because after the si2168 gets 0101, commands will > + * still be executed here but no result I don't understand. What problem are you seeing here? Why can't you do a query chip revision first? This needs resolving of course. > + */ > + memcpy(cmd.args, "\x02", 1); > + cmd.wlen = 1; > + cmd.rlen = 13; > + ret = si2157_cmd_execute(client, &cmd); > + if (ret) > + goto err_kfree; > + dev->chip_id = cmd.args[1] << 24 | > + cmd.args[2] << 16 | > + cmd.args[3] << 8 | > + cmd.args[4] << 0; > + dev_info(&client->dev, "found a 'Silicon Labs Si21%d-%c%c%c'\n", > + cmd.args[2], cmd.args[1], cmd.args[3], cmd.args[4]); > + > + > #ifdef CONFIG_MEDIA_CONTROLLER > if (cfg->mdev) { > dev->mdev = cfg->mdev; > diff --git a/drivers/media/tuners/si2157_priv.h b/drivers/media/tuners/si2157_priv.h > index 2bda903358da..9ab7c88b01b4 100644 > --- a/drivers/media/tuners/si2157_priv.h > +++ b/drivers/media/tuners/si2157_priv.h > @@ -28,6 +28,7 @@ struct si2157_dev { > u8 chiptype; > u8 if_port; > u32 if_frequency; > + u32 chip_id; > struct delayed_work stat_work; > > #if defined(CONFIG_MEDIA_CONTROLLER) > diff --git a/drivers/media/usb/dvb-usb-v2/af9035.c b/drivers/media/usb/dvb-usb-v2/af9035.c > index 3afd18733614..83e243df59b9 100644 > --- a/drivers/media/usb/dvb-usb-v2/af9035.c > +++ b/drivers/media/usb/dvb-usb-v2/af9035.c > @@ -1246,6 +1246,51 @@ static int it930x_frontend_attach(struct dvb_usb_adapter *adap) > > msleep(200); > > + if (le16_to_cpu(d->udev->descriptor.idVendor) == USB_VID_DEXATEK) { > + > + ret = af9035_wr_reg_mask(d, 0xd8b7, 0x01, 0x01); > + if (ret < 0) > + goto err; > + > + /* I2C master bus 2 clock speed 300k */ > + ret = af9035_wr_reg(d, 0x00f6a7, 0x07); > + if (ret < 0) > + goto err; > + > + /* I2C master bus 1,3 clock speed 300k */ > + ret = af9035_wr_reg(d, 0x00f103, 0x07); > + if (ret < 0) > + goto err; > + > + /* set gpio11 low */ > + ret = af9035_wr_reg_mask(d, 0xd8d4, 0x01, 0x01); > + if (ret < 0) > + goto err; > + > + ret = af9035_wr_reg_mask(d, 0xd8d5, 0x01, 0x01); > + if (ret < 0) > + goto err; > + > + ret = af9035_wr_reg_mask(d, 0xd8d3, 0x01, 0x01); > + if (ret < 0) > + goto err; > + > + /* Tuner enable using gpiot2_en, gpiot2_on and gpiot2_o (reset) */ > + ret = af9035_wr_reg_mask(d, 0xd8b8, 0x01, 0x01); > + if (ret < 0) > + goto err; > + > + ret = af9035_wr_reg_mask(d, 0xd8b9, 0x01, 0x01); > + if (ret < 0) > + goto err; > + > + ret = af9035_wr_reg_mask(d, 0xd8b7, 0x00, 0x01); > + if (ret < 0) > + goto err; > + > + msleep(200); > + } > + > ret = af9035_wr_reg_mask(d, 0xd8b7, 0x01, 0x01); > if (ret < 0) > goto err; > @@ -2119,6 +2164,8 @@ static const struct usb_device_id af9035_id_table[] = { > /* IT930x devices */ > { DVB_USB_DEVICE(USB_VID_ITETECH, USB_PID_ITETECH_IT9303, > &it930x_props, "ITE 9303 Generic", NULL) }, > + { DVB_USB_DEVICE(USB_VID_DEXATEK, 0x0100, > + &it930x_props, "Logilink VG0022A", NULL) }, > { DVB_USB_DEVICE(USB_VID_AVERMEDIA, USB_PID_AVERMEDIA_TD310, > &it930x_props, "AVerMedia TD310 DVB-T2", NULL) }, > { } > -- > 2.20.1
Hi! > You need a message and a Signed-off-by: here. Ok, I'll try to get that right the next time. > > + ret = si2157_power_up(dev, client); > > + if (ret) > > + goto err; > > + /* query chip revision */ > > + /* hack: do it here because after the si2168 gets 0101, commands will > > + * still be executed here but no result > > I don't understand. What problem are you seeing here? Why can't you do a > query chip revision first? This was explained here: https://lkml.org/lkml/2017/3/15/778. To quote: If the si2157 is behind a e.g. si2168, the si2157 will at least in some situations not be readable after the si268 got the command 0101. It still accepts commands but the answer is just ffffff. So read the chip id before that so the information is not lost. The following line in kernel output is a symptome of that problem: si2157 7-0063: unknown chip version Si21255-\xffffffff\xffffffff\xffffffff > This needs resolving of course. I hope so. :) g
Em Wed, 2 Oct 2019 16:13:59 +0200 Gon Solo <gonsolo@gmail.com> escreveu: All patches should have a description at the beginning of the e-mail body, even the trivial ones. You also forgot to add your Signed-off-by. > --- > drivers/media/tuners/si2157.c | 68 +++++++++++++++++---------- > drivers/media/tuners/si2157_priv.h | 1 + > drivers/media/usb/dvb-usb-v2/af9035.c | 47 ++++++++++++++++++ > 3 files changed, 90 insertions(+), 26 deletions(-) > > diff --git a/drivers/media/tuners/si2157.c b/drivers/media/tuners/si2157.c > index e87040d6eca7..8f9df2485d51 100644 > --- a/drivers/media/tuners/si2157.c > +++ b/drivers/media/tuners/si2157.c > @@ -66,6 +66,24 @@ static int si2157_cmd_execute(struct i2c_client *client, struct si2157_cmd *cmd) > return ret; > } > > +static int si2157_power_up(struct si2157_dev *dev, struct i2c_client *client) > +{ > + struct si2157_cmd cmd; > + > + if (dev->chiptype == SI2157_CHIPTYPE_SI2146) { > + memcpy(cmd.args, "\xc0\x05\x01\x00\x00\x0b\x00\x00\x01", 9); > + cmd.wlen = 9; > + } else if (dev->chiptype == SI2157_CHIPTYPE_SI2141) { > + memcpy(cmd.args, "\xc0\x00\x0d\x0e\x00\x01\x01\x01\x01\x03", 10); > + cmd.wlen = 10; > + } else { > + memcpy(cmd.args, "\xc0\x00\x0c\x00\x00\x01\x01\x01\x01\x01\x01\x02\x00\x00\x01", 15); > + cmd.wlen = 15; > + } > + cmd.rlen = 1; > + return si2157_cmd_execute(client, &cmd); > +} > + > static int si2157_init(struct dvb_frontend *fe) > { > struct i2c_client *client = fe->tuner_priv; > @@ -75,7 +93,7 @@ static int si2157_init(struct dvb_frontend *fe) > struct si2157_cmd cmd; > const struct firmware *fw; > const char *fw_name; > - unsigned int uitmp, chip_id; > + unsigned int uitmp; > > dev_dbg(&client->dev, "\n"); > > @@ -93,19 +111,7 @@ static int si2157_init(struct dvb_frontend *fe) > if (uitmp == dev->if_frequency / 1000) > goto warm; > > - /* power up */ > - if (dev->chiptype == SI2157_CHIPTYPE_SI2146) { > - memcpy(cmd.args, "\xc0\x05\x01\x00\x00\x0b\x00\x00\x01", 9); > - cmd.wlen = 9; > - } else if (dev->chiptype == SI2157_CHIPTYPE_SI2141) { > - memcpy(cmd.args, "\xc0\x00\x0d\x0e\x00\x01\x01\x01\x01\x03", 10); > - cmd.wlen = 10; > - } else { > - memcpy(cmd.args, "\xc0\x00\x0c\x00\x00\x01\x01\x01\x01\x01\x01\x02\x00\x00\x01", 15); > - cmd.wlen = 15; > - } > - cmd.rlen = 1; > - ret = si2157_cmd_execute(client, &cmd); > + ret = si2157_power_up(dev, client); > if (ret) > goto err; Ok, you're moving the power-op code to a function. That's OK, but the rule is one patch per functional change. So, the first patch in this series should be the one moving the power up code to a separate function, e. g. the e-mail would be something like: Subject: [PATCH 1/3] media: si2157: move power up code to a function On some devices, we need to power up the device on other places, so move the code to a separate function. Signed-off-by: your name <your@email> ... > > @@ -118,17 +124,6 @@ static int si2157_init(struct dvb_frontend *fe) > goto err; > } > > - /* query chip revision */ > - memcpy(cmd.args, "\x02", 1); > - cmd.wlen = 1; > - cmd.rlen = 13; > - ret = si2157_cmd_execute(client, &cmd); > - if (ret) > - goto err; > - > - chip_id = cmd.args[1] << 24 | cmd.args[2] << 16 | cmd.args[3] << 8 | > - cmd.args[4] << 0; > - > #define SI2177_A30 ('A' << 24 | 77 << 16 | '3' << 8 | '0' << 0) > #define SI2158_A20 ('A' << 24 | 58 << 16 | '2' << 8 | '0' << 0) > #define SI2148_A20 ('A' << 24 | 48 << 16 | '2' << 8 | '0' << 0) > @@ -137,7 +132,7 @@ static int si2157_init(struct dvb_frontend *fe) > #define SI2146_A10 ('A' << 24 | 46 << 16 | '1' << 8 | '0' << 0) > #define SI2141_A10 ('A' << 24 | 41 << 16 | '1' << 8 | '0' << 0) > > - switch (chip_id) { > + switch (dev->chip_id) { > case SI2158_A20: > case SI2148_A20: > fw_name = SI2158_A20_FIRMWARE; > @@ -456,6 +451,27 @@ static int si2157_probe(struct i2c_client *client, > memcpy(&fe->ops.tuner_ops, &si2157_ops, sizeof(struct dvb_tuner_ops)); > fe->tuner_priv = client; > > + ret = si2157_power_up(dev, client); > + if (ret) > + goto err; > + /* query chip revision */ > + /* hack: do it here because after the si2168 gets 0101, commands will > + * still be executed here but no result > + */ > + memcpy(cmd.args, "\x02", 1); > + cmd.wlen = 1; > + cmd.rlen = 13; > + ret = si2157_cmd_execute(client, &cmd); > + if (ret) > + goto err_kfree; > + dev->chip_id = cmd.args[1] << 24 | > + cmd.args[2] << 16 | > + cmd.args[3] << 8 | > + cmd.args[4] << 0; > + dev_info(&client->dev, "found a 'Silicon Labs Si21%d-%c%c%c'\n", > + cmd.args[2], cmd.args[1], cmd.args[3], cmd.args[4]); > + > + Why the extra blank line? The above code seems to be a separate issue and should be handled in separate. I suspect, however, that the issue is actually at the bridge driver. You should probably open the I2C gate before being able to talk with it. > #ifdef CONFIG_MEDIA_CONTROLLER > if (cfg->mdev) { > dev->mdev = cfg->mdev; > diff --git a/drivers/media/tuners/si2157_priv.h b/drivers/media/tuners/si2157_priv.h > index 2bda903358da..9ab7c88b01b4 100644 > --- a/drivers/media/tuners/si2157_priv.h > +++ b/drivers/media/tuners/si2157_priv.h > @@ -28,6 +28,7 @@ struct si2157_dev { > u8 chiptype; > u8 if_port; > u32 if_frequency; > + u32 chip_id; > struct delayed_work stat_work; > > #if defined(CONFIG_MEDIA_CONTROLLER) > diff --git a/drivers/media/usb/dvb-usb-v2/af9035.c b/drivers/media/usb/dvb-usb-v2/af9035.c > index 3afd18733614..83e243df59b9 100644 > --- a/drivers/media/usb/dvb-usb-v2/af9035.c > +++ b/drivers/media/usb/dvb-usb-v2/af9035.c > @@ -1246,6 +1246,51 @@ static int it930x_frontend_attach(struct dvb_usb_adapter *adap) > > msleep(200); > > + if (le16_to_cpu(d->udev->descriptor.idVendor) == USB_VID_DEXATEK) { > + > + ret = af9035_wr_reg_mask(d, 0xd8b7, 0x01, 0x01); > + if (ret < 0) > + goto err; > + > + /* I2C master bus 2 clock speed 300k */ > + ret = af9035_wr_reg(d, 0x00f6a7, 0x07); > + if (ret < 0) > + goto err; > + > + /* I2C master bus 1,3 clock speed 300k */ > + ret = af9035_wr_reg(d, 0x00f103, 0x07); > + if (ret < 0) > + goto err; > + > + /* set gpio11 low */ > + ret = af9035_wr_reg_mask(d, 0xd8d4, 0x01, 0x01); > + if (ret < 0) > + goto err; > + > + ret = af9035_wr_reg_mask(d, 0xd8d5, 0x01, 0x01); > + if (ret < 0) > + goto err; > + > + ret = af9035_wr_reg_mask(d, 0xd8d3, 0x01, 0x01); > + if (ret < 0) > + goto err; > + > + /* Tuner enable using gpiot2_en, gpiot2_on and gpiot2_o (reset) */ > + ret = af9035_wr_reg_mask(d, 0xd8b8, 0x01, 0x01); > + if (ret < 0) > + goto err; > + > + ret = af9035_wr_reg_mask(d, 0xd8b9, 0x01, 0x01); > + if (ret < 0) > + goto err; > + > + ret = af9035_wr_reg_mask(d, 0xd8b7, 0x00, 0x01); > + if (ret < 0) > + goto err; > + > + msleep(200); Hmm.... you are setting bit 1 of 0xd8b7 to 0 here... > + } > + > ret = af9035_wr_reg_mask(d, 0xd8b7, 0x01, 0x01); > if (ret < 0) > goto err; Then setting it to 1 here. Is this a reset pin? If so, perhaps you need to do something like: ret = af9035_wr_reg_mask(d, 0xd8b7, 0x00, 0x01); if (ret < 0) goto err; msleep(200); ret = af9035_wr_reg_mask(d, 0xd8b7, 0x01, 0x01); if (ret < 0) goto err; msleep(200); In order to wait for the reset to finish and be able to talk with the tuner. > @@ -2119,6 +2164,8 @@ static const struct usb_device_id af9035_id_table[] = { > /* IT930x devices */ > { DVB_USB_DEVICE(USB_VID_ITETECH, USB_PID_ITETECH_IT9303, > &it930x_props, "ITE 9303 Generic", NULL) }, > + { DVB_USB_DEVICE(USB_VID_DEXATEK, 0x0100, > + &it930x_props, "Logilink VG0022A", NULL) }, > { DVB_USB_DEVICE(USB_VID_AVERMEDIA, USB_PID_AVERMEDIA_TD310, > &it930x_props, "AVerMedia TD310 DVB-T2", NULL) }, > { } Thanks, Mauro
On Wed, Oct 02, 2019 at 04:44:24PM +0200, Gonsolo wrote: > Hi! > > > You need a message and a Signed-off-by: here. > > Ok, I'll try to get that right the next time. > > > > + ret = si2157_power_up(dev, client); > > > + if (ret) > > > + goto err; > > > + /* query chip revision */ > > > + /* hack: do it here because after the si2168 gets 0101, commands will > > > + * still be executed here but no result > > > > I don't understand. What problem are you seeing here? Why can't you do a > > query chip revision first? > > This was explained here: https://lkml.org/lkml/2017/3/15/778. To quote: Antti has some great suggestions in that thread: https://lkml.org/lkml/2017/5/24/245 Also note https://lkml.org/lkml/2017/5/26/357 if you have access to a logic analyser. Sean
Hi! > Antti has some great suggestions in that thread: > https://lkml.org/lkml/2017/5/24/245 > > Also note https://lkml.org/lkml/2017/5/26/357 if you have access to a > logic analyser. I read that thread. Unfortunately I'm not a hardware engineer nor do I have access to a logic analyser, just the device and a remote hope not to keep my custom 4.13 kernel forever or to have to buy a new DVB T2 device. :( In the above thread it is mentioned that even the Windows driver receives the ffff's so maybe it is a hardware bug? I'd love to see this driver upstream but I have no idea how to proceed. Any suggestions?
Hi all. On 10/2/19 5:21 PM, Gonsolo wrote: > Hi! > >> Antti has some great suggestions in that thread: >> https://lkml.org/lkml/2017/5/24/245 >> >> Also note https://lkml.org/lkml/2017/5/26/357 if you have access to a >> logic analyser. > I read that thread. Unfortunately I'm not a hardware engineer nor do I > have access to a logic analyser, just the device and a remote hope not > to keep my custom 4.13 kernel forever or to have to buy a new DVB T2 > device. :( > In the above thread it is mentioned that even the Windows driver > receives the ffff's so maybe it is a hardware bug? Looks like it is: http://lkml.iu.edu/hypermail/linux/kernel/1710.3/03205.html > > I'd love to see this driver upstream but I have no idea how to > proceed. Any suggestions? >
Em Wed, 2 Oct 2019 19:23:02 +0200 JP <jp@jpvw.nl> escreveu: > Hi all. > > On 10/2/19 5:21 PM, Gonsolo wrote: > > Hi! > > > >> Antti has some great suggestions in that thread: > >> https://lkml.org/lkml/2017/5/24/245 > >> > >> Also note https://lkml.org/lkml/2017/5/26/357 if you have access to a > >> logic analyser. > > I read that thread. Unfortunately I'm not a hardware engineer nor do I > > have access to a logic analyser, just the device and a remote hope not > > to keep my custom 4.13 kernel forever or to have to buy a new DVB T2 > > device. :( > > In the above thread it is mentioned that even the Windows driver > > receives the ffff's so maybe it is a hardware bug? > Looks like it is: > http://lkml.iu.edu/hypermail/linux/kernel/1710.3/03205.html Hmm... changing the pull-up register will very likely change the timings. There's a logic at the driver that changes the I2C bus speed to 300k (with is non-standard): /* I2C master bus 2 clock speed 300k */ ret = af9035_wr_reg(d, 0x00f6a7, 0x07); if (ret < 0) goto err; /* I2C master bus 1,3 clock speed 300k */ ret = af9035_wr_reg(d, 0x00f103, 0x07); if (ret < 0) goto err; Perhaps if we reduce the bus speed to 100k, the device will work without the hacks. I don't have af9035 datasheets. Perhaps Antti could shed some light about how to change the speed to 100k, but on a quick search at the Internet, I found this: https://lore.kernel.org/linux-media/1312539895.2763.33.camel@Jason-Linux/ With has a comment that explains how the I2C speed is calculated on those ITE devices: #define p_reg_one_cycle_counter_tuner 0xF103 /* Define I2C master speed, the default value 0x07 means 366KHz (1000000000 / (24.4 * 16 * User_I2C_SPEED)). */ #define User_I2C_SPEED 0x07 error = it9135_write_reg(data, 0, PROCESSOR_LINK, p_reg_one_cycle_counter_tuner, User_I2C_SPEED); So, in summary, the value for the I2C speed register is given by: I2C_speed_register = (1000000000 / (24.4 * 16 * I2C_speed)) So, for 100 kbps, the I2C speed register should be set with a value close to ~25,6. Doing the reverse math, we have: I2C_speed_register = 25 -> I2C_speed = 102,46 kbps I2C_speed_register = 26 -> I2C_speed = 98,52 kbps So, if we do: /* I2C master bus 2 clock speed ~100k */ ret = af9035_wr_reg(d, 0x00f6a7, 26); if (ret < 0) goto err; /* I2C master bus 1,3 clock speed ~100k */ ret = af9035_wr_reg(d, 0x00f103, 26); if (ret < 0) goto err; The bus speed will reduce to 98,52 kbps, with is a typical nominal value for I2C tuners and other devices. With that, the device should probably work fine without needing to replace the pull up resistor. Ok, tuner firmware load would be ~3,7 times slower, but this is something that we do just once need a firmware). All other I2C messages are short enough to not cause any real difference. Could you please test the enclosing patch and see if, with that, you can remove the hacks you added for the tuner probe to work? Regards, Mauro [PATCH] media: af9035: slow down I2C bus speed on it930x devices We have a few reports about tuner probing instability with some it930x devices. As it is better safe than sorry, let's slow down the I2C, using the formula found at an old patch: https://lore.kernel.org/linux-media/1312539895.2763.33.camel@Jason-Linux/ Signed-off-by: Mauro Carvalho Chehab <mchehab+samsung@kernel.org> diff --git a/drivers/media/usb/dvb-usb-v2/af9035.c b/drivers/media/usb/dvb-usb-v2/af9035.c index 3afd18733614..df2c75b84be8 100644 --- a/drivers/media/usb/dvb-usb-v2/af9035.c +++ b/drivers/media/usb/dvb-usb-v2/af9035.c @@ -1197,6 +1197,9 @@ static int af9035_frontend_attach(struct dvb_usb_adapter *adap) return ret; } +/* I2C speed register = (1000000000 / (24.4 * 16 * I2C_speed)) */ +#define I2C_SPEED_REGISTER 26 + static int it930x_frontend_attach(struct dvb_usb_adapter *adap) { struct state *state = adap_to_priv(adap); @@ -1208,13 +1211,13 @@ static int it930x_frontend_attach(struct dvb_usb_adapter *adap) dev_dbg(&intf->dev, "adap->id=%d\n", adap->id); - /* I2C master bus 2 clock speed 300k */ - ret = af9035_wr_reg(d, 0x00f6a7, 0x07); + /* I2C master bus 2 clock speed ~100k */ + ret = af9035_wr_reg(d, 0x00f6a7, I2C_SPEED_REGISTER); if (ret < 0) goto err; - /* I2C master bus 1,3 clock speed 300k */ - ret = af9035_wr_reg(d, 0x00f103, 0x07); + /* I2C master bus 1,3 clock speed ~100k */ + ret = af9035_wr_reg(d, 0x00f103, I2C_SPEED_REGISTER); if (ret < 0) goto err;
Hi! > Could you please test the enclosing patch and see if, with that, you > can remove the hacks you added for the tuner probe to work? I tested again on a vanilla media_tree with Mauro's patch attached. Doesn't work. Dmesg output: [ 0.788387] kernel: usb 1-1: new high-speed USB device number 2 using ehci-pci [ 0.792384] kernel: usb 2-1: new high-speed USB device number 2 using xhci_hcd [ 0.944937] kernel: usb 2-1: New USB device found, idVendor=1d19, idProduct=0100, bcdDevice= 1.00 [ 0.944939] kernel: usb 2-1: New USB device strings: Mfr=1, Product=2, SerialNumber=3 [ 0.944940] kernel: usb 2-1: Product: TS Aggregator [ 0.944941] kernel: usb 2-1: Manufacturer: ITE Tech., Inc. [ 0.944942] kernel: usb 2-1: SerialNumber: AF0102020700001 I then also used the following (additional) patch: @@ -2119,6 +2122,8 @@ static const struct usb_device_id af9035_id_table[] = { /* IT930x devices */ { DVB_USB_DEVICE(USB_VID_ITETECH, USB_PID_ITETECH_IT9303, &it930x_props, "ITE 9303 Generic", NULL) }, + { DVB_USB_DEVICE(USB_VID_DEXATEK, 0x0100, + &it930x_props, "Logilink VG0022A", NULL) }, { DVB_USB_DEVICE(USB_VID_AVERMEDIA, USB_PID_AVERMEDIA_TD310, &it930x_props, "AVerMedia TD310 DVB-T2", NULL) }, { } Which gives the following output: [ 5.380989] si2168 1-0067: Silicon Labs Si2168-B40 successfully identified [ 5.380991] si2168 1-0067: firmware version: B 4.0.2 [ 5.381013] usb 2-1: DVB: registering adapter 0 frontend 0 (Silicon Labs Si2168)... [ 5.381018] dvbdev: dvb_create_media_entity: media entity 'Silicon Labs Si2168' registered. [ 5.390058] checking generic (e0000000 410000) vs hw (e0000000 10000000) [ 5.390062] fb0: switching to inteldrmfb from EFI VGA [ 5.390268] Console: switching to colour dummy device 80x25 [ 5.390281] i915 0000:00:02.0: vgaarb: deactivate vga console [ 5.393438] si2157 2-0063: Silicon Labs Si2147/2148/2157/2158 successfully attached But when I try to use VLC I get the following: [ 457.677363] si2168 1-0067: downloading firmware from file 'dvb-demod-si2168-b40-01.fw' [ 458.631034] si2168 1-0067: firmware version: B 4.0.11 [ 458.650309] si2157 2-0063: unknown chip version Si21255-\xff\xff\xff Now I'm trying other timings. Thanks, g
Hi! Boot time: > [ 5.380991] si2168 1-0067: firmware version: B 4.0.2 When starting VLC: > [ 457.677363] si2168 1-0067: downloading firmware from file > 'dvb-demod-si2168-b40-01.fw' > [ 458.631034] si2168 1-0067: firmware version: B 4.0.11 > [ 458.650309] si2157 2-0063: unknown chip version Si21255-\xff\xff\xff There are two different firmware versions, 4.0.2 and 4.0.11. Is that expected?
Em Thu, 3 Oct 2019 12:13:33 +0200 Gonsolo <gonsolo@gmail.com> escreveu: > Hi! > > > Could you please test the enclosing patch and see if, with that, you > > can remove the hacks you added for the tuner probe to work? > > I tested again on a vanilla media_tree with Mauro's patch attached. > Doesn't work. Dmesg output: > > [ 0.788387] kernel: usb 1-1: new high-speed USB device number 2 > using ehci-pci > [ 0.792384] kernel: usb 2-1: new high-speed USB device number 2 > using xhci_hcd > [ 0.944937] kernel: usb 2-1: New USB device found, idVendor=1d19, > idProduct=0100, bcdDevice= 1.00 > [ 0.944939] kernel: usb 2-1: New USB device strings: Mfr=1, > Product=2, SerialNumber=3 > [ 0.944940] kernel: usb 2-1: Product: TS Aggregator > [ 0.944941] kernel: usb 2-1: Manufacturer: ITE Tech., Inc. > [ 0.944942] kernel: usb 2-1: SerialNumber: AF0102020700001 > > I then also used the following (additional) patch: > > @@ -2119,6 +2122,8 @@ static const struct usb_device_id af9035_id_table[] = { > /* IT930x devices */ > { DVB_USB_DEVICE(USB_VID_ITETECH, USB_PID_ITETECH_IT9303, > &it930x_props, "ITE 9303 Generic", NULL) }, > + { DVB_USB_DEVICE(USB_VID_DEXATEK, 0x0100, > + &it930x_props, "Logilink VG0022A", NULL) }, > { DVB_USB_DEVICE(USB_VID_AVERMEDIA, USB_PID_AVERMEDIA_TD310, > &it930x_props, "AVerMedia TD310 DVB-T2", NULL) }, > { } > > Which gives the following output: > > [ 5.380989] si2168 1-0067: Silicon Labs Si2168-B40 successfully identified > [ 5.380991] si2168 1-0067: firmware version: B 4.0.2 > [ 5.381013] usb 2-1: DVB: registering adapter 0 frontend 0 (Silicon > Labs Si2168)... > [ 5.381018] dvbdev: dvb_create_media_entity: media entity 'Silicon > Labs Si2168' registered. > [ 5.390058] checking generic (e0000000 410000) vs hw (e0000000 10000000) > [ 5.390062] fb0: switching to inteldrmfb from EFI VGA > [ 5.390268] Console: switching to colour dummy device 80x25 > [ 5.390281] i915 0000:00:02.0: vgaarb: deactivate vga console > [ 5.393438] si2157 2-0063: Silicon Labs Si2147/2148/2157/2158 > successfully attached Ok. It sounds that the issues you're facing are indeed related to timing conditions. > > But when I try to use VLC I get the following: > > [ 457.677363] si2168 1-0067: downloading firmware from file > 'dvb-demod-si2168-b40-01.fw' > [ 458.631034] si2168 1-0067: firmware version: B 4.0.11 > [ 458.650309] si2157 2-0063: unknown chip version Si21255-\xff\xff\xff > > Now I'm trying other timings. Returning 0xff, 0xff, 0xff, ... usually means that the tuner chip didn't respond in time. This could indicate: 1) The device requires more time to go to sane state after firmware load and/or device reset/power up; 2) Tuner may be using I2C clock stretching, but the bridge doesn't support it. 3) The clock used at the I2C bus is still too high; 4) The tuner is hidden by an I2C gate. I think that using the standard I2C bus clock of 100kbps should be enough. I2C clock stretching seems to be unlikely for a command that it is just getting the device's version. What seems more likely is that this device may need some time after firmware load to start working. So, I would add a msleep() somewhere after the firmware update. Thanks, Mauro
Em Thu, 3 Oct 2019 12:57:50 +0200 Gonsolo <gonsolo@gmail.com> escreveu: > Hi! > > Boot time: > > > [ 5.380991] si2168 1-0067: firmware version: B 4.0.2 > > When starting VLC: > > > [ 457.677363] si2168 1-0067: downloading firmware from file > > 'dvb-demod-si2168-b40-01.fw' > > [ 458.631034] si2168 1-0067: firmware version: B 4.0.11 > > [ 458.650309] si2157 2-0063: unknown chip version Si21255-\xff\xff\xff > > There are two different firmware versions, 4.0.2 and 4.0.11. Is that expected? It means that there's a firmware stored at the device's eeprom (version 4.0.2). When the driver starts, it downloads a newer firmware from the file dvb-demod-si2168-b40-01.fw. Btw, could you please try the enclosed hack and post the results? Thanks, Mauro diff --git a/drivers/media/tuners/si2157.c b/drivers/media/tuners/si2157.c index e87040d6eca7..3ccfd602934b 100644 --- a/drivers/media/tuners/si2157.c +++ b/drivers/media/tuners/si2157.c @@ -76,6 +76,7 @@ static int si2157_init(struct dvb_frontend *fe) const struct firmware *fw; const char *fw_name; unsigned int uitmp, chip_id; + int i; dev_dbg(&client->dev, "\n"); @@ -118,16 +119,32 @@ static int si2157_init(struct dvb_frontend *fe) goto err; } - /* query chip revision */ - memcpy(cmd.args, "\x02", 1); - cmd.wlen = 1; - cmd.rlen = 13; - ret = si2157_cmd_execute(client, &cmd); - if (ret) - goto err; + for (i = 0; i < 10; i++) { + /* query chip revision */ + memcpy(cmd.args, "\x02", 1); + cmd.wlen = 1; + cmd.rlen = 13; + ret = si2157_cmd_execute(client, &cmd); + if (ret) + goto err; + + chip_id = cmd.args[1] << 24 | cmd.args[2] << 16 | cmd.args[3] << 8 | + cmd.args[4] << 0; - chip_id = cmd.args[1] << 24 | cmd.args[2] << 16 | cmd.args[3] << 8 | - cmd.args[4] << 0; + if (chip_id != 0xffffffff) + break; + + msleep(10); + } + + if (i) + dev_info(&client->dev, "Needed to wait %i ms to get chip version", i * 10); + + if (chip_id == 0xffffffff) { + dev_err(&client->dev, "Unable to retrieve chip version\n"); + ret = -EINVAL; + goto err; + } #define SI2177_A30 ('A' << 24 | 77 << 16 | '3' << 8 | '0' << 0) #define SI2158_A20 ('A' << 24 | 58 << 16 | '2' << 8 | '0' << 0)
Hi! > It means that there's a firmware stored at the device's eeprom > (version 4.0.2). When the driver starts, it downloads a newer firmware > from the file dvb-demod-si2168-b40-01.fw. Thanks for the explanation. > Btw, could you please try the enclosed hack and post the results? Will do in a second. FWIW, this hack worked: diff --git a/drivers/media/tuners/si2157.c b/drivers/media/tuners/si2157.c index e87040d6eca7..28a3a4f1640e 100644 --- a/drivers/media/tuners/si2157.c +++ b/drivers/media/tuners/si2157.c @@ -136,6 +136,7 @@ static int si2157_init(struct dvb_frontend *fe) #define SI2147_A30 ('A' << 24 | 47 << 16 | '3' << 8 | '0' << 0) #define SI2146_A10 ('A' << 24 | 46 << 16 | '1' << 8 | '0' << 0) #define SI2141_A10 ('A' << 24 | 41 << 16 | '1' << 8 | '0' << 0) + #define GONZO (255 << 24 | 255 << 16 | 255 << 8 | 255 << 0) switch (chip_id) { case SI2158_A20: @@ -148,6 +149,10 @@ static int si2157_init(struct dvb_frontend *fe) case SI2177_A30: fw_name = SI2157_A30_FIRMWARE; break; + case GONZO: + dev_info(&client->dev, "trying null\n"); + fw_name = NULL; + break; case SI2157_A30: case SI2147_A30: case SI2146_A10:
Hi!
> Btw, could you please try the enclosed hack and post the results?
[ 210.178948] si2168 1-0067: downloading firmware from file 'dvb-demod-si2168-b40-01.fw'
[ 212.404011] si2168 1-0067: firmware version: B 4.0.25
[ 212.656467] si2157 2-0063: Needed to wait 100 ms to get chip version
[ 212.656470] si2157 2-0063: Unable to retrieve chip version
:(
g
Em Thu, 3 Oct 2019 14:01:43 +0200 Gon Solo <gonsolo@gmail.com> escreveu: > Hi! > > > > Btw, could you please try the enclosed hack and post the results? > > [ 210.178948] si2168 1-0067: downloading firmware from file 'dvb-demod-si2168-b40-01.fw' > [ 212.404011] si2168 1-0067: firmware version: B 4.0.25 > [ 212.656467] si2157 2-0063: Needed to wait 100 ms to get chip version > [ 212.656470] si2157 2-0063: Unable to retrieve chip version well, you could try to increase the timeout - although 100 ms seems a lot of time to me. Thanks, Mauro
> well, you could try to increase the timeout - although 100 ms seems a lot > of time to me. I tried 5s, still no change. Would it be possible to include my patch, possibly with a warning like "bogus chip version, trying with no firmware"? g
Em Thu, 3 Oct 2019 13:41:23 +0200 Gonsolo <gonsolo@gmail.com> escreveu: > Hi! > > > It means that there's a firmware stored at the device's eeprom > > (version 4.0.2). When the driver starts, it downloads a newer firmware > > from the file dvb-demod-si2168-b40-01.fw. > > Thanks for the explanation. > > > Btw, could you please try the enclosed hack and post the results? > > Will do in a second. > > FWIW, this hack worked: > > diff --git a/drivers/media/tuners/si2157.c b/drivers/media/tuners/si2157.c > index e87040d6eca7..28a3a4f1640e 100644 > --- a/drivers/media/tuners/si2157.c > +++ b/drivers/media/tuners/si2157.c > @@ -136,6 +136,7 @@ static int si2157_init(struct dvb_frontend *fe) > #define SI2147_A30 ('A' << 24 | 47 << 16 | '3' << 8 | '0' << 0) > #define SI2146_A10 ('A' << 24 | 46 << 16 | '1' << 8 | '0' << 0) > #define SI2141_A10 ('A' << 24 | 41 << 16 | '1' << 8 | '0' << 0) > + #define GONZO (255 << 24 | 255 << 16 | 255 << 8 | 255 << 0) > > switch (chip_id) { > case SI2158_A20: > @@ -148,6 +149,10 @@ static int si2157_init(struct dvb_frontend *fe) > case SI2177_A30: > fw_name = SI2157_A30_FIRMWARE; > break; > + case GONZO: > + dev_info(&client->dev, "trying null\n"); > + fw_name = NULL; > + break; > case SI2157_A30: > case SI2147_A30: > case SI2146_A10: What does it print with this hack? Also, could you get the SI version after the reset code at skip_fw_download, just after retrieving si2157 firmware version? Thanks, Mauro
Em Thu, 3 Oct 2019 09:49:04 -0300 Mauro Carvalho Chehab <mchehab+samsung@kernel.org> escreveu: > Em Thu, 3 Oct 2019 13:41:23 +0200 > Gonsolo <gonsolo@gmail.com> escreveu: > > > Hi! > > > > > It means that there's a firmware stored at the device's eeprom > > > (version 4.0.2). When the driver starts, it downloads a newer firmware > > > from the file dvb-demod-si2168-b40-01.fw. > > > > Thanks for the explanation. > > > > > Btw, could you please try the enclosed hack and post the results? > > > > Will do in a second. > > > > FWIW, this hack worked: > > > > diff --git a/drivers/media/tuners/si2157.c b/drivers/media/tuners/si2157.c > > index e87040d6eca7..28a3a4f1640e 100644 > > --- a/drivers/media/tuners/si2157.c > > +++ b/drivers/media/tuners/si2157.c > > @@ -136,6 +136,7 @@ static int si2157_init(struct dvb_frontend *fe) > > #define SI2147_A30 ('A' << 24 | 47 << 16 | '3' << 8 | '0' << 0) > > #define SI2146_A10 ('A' << 24 | 46 << 16 | '1' << 8 | '0' << 0) > > #define SI2141_A10 ('A' << 24 | 41 << 16 | '1' << 8 | '0' << 0) > > + #define GONZO (255 << 24 | 255 << 16 | 255 << 8 | 255 << 0) > > > > switch (chip_id) { > > case SI2158_A20: > > @@ -148,6 +149,10 @@ static int si2157_init(struct dvb_frontend *fe) > > case SI2177_A30: > > fw_name = SI2157_A30_FIRMWARE; > > break; > > + case GONZO: > > + dev_info(&client->dev, "trying null\n"); > > + fw_name = NULL; > > + break; > > case SI2157_A30: > > case SI2147_A30: > > case SI2146_A10: > > What does it print with this hack? > > Also, could you get the SI version after the reset code at > skip_fw_download, just after retrieving si2157 firmware version? Maybe something like this would make it work? diff --git a/drivers/media/tuners/si2157.c b/drivers/media/tuners/si2157.c index e87040d6eca7..86d945fd50b9 100644 --- a/drivers/media/tuners/si2157.c +++ b/drivers/media/tuners/si2157.c @@ -129,6 +129,28 @@ static int si2157_init(struct dvb_frontend *fe) chip_id = cmd.args[1] << 24 | cmd.args[2] << 16 | cmd.args[3] << 8 | cmd.args[4] << 0; + if (chip_id == 0xffffffff) { + /* reboot the tuner */ + memcpy(cmd.args, "\x01\x01", 2); + cmd.wlen = 2; + cmd.rlen = 1; + ret = si2157_cmd_execute(client, &cmd); + if (ret) + goto err; + + /* query chip revision */ + memcpy(cmd.args, "\x02", 1); + cmd.wlen = 1; + cmd.rlen = 13; + ret = si2157_cmd_execute(client, &cmd); + if (ret) + goto err; + + chip_id = cmd.args[1] << 24 | cmd.args[2] << 16 | cmd.args[3] << 8 | + cmd.args[4] << 0; + + } + #define SI2177_A30 ('A' << 24 | 77 << 16 | '3' << 8 | '0' << 0) #define SI2158_A20 ('A' << 24 | 58 << 16 | '2' << 8 | '0' << 0) #define SI2148_A20 ('A' << 24 | 48 << 16 | '2' << 8 | '0' << 0) Thanks, Mauro
> Maybe something like this would make it work?
Nope. :(
[ 47.371022] si2168 1-0067: downloading firmware from file 'dvb-demod-si2168-b40-01.fw'
[ 48.632824] si2168 1-0067: firmware version: B 4.0.25
[ 48.671268] si2157 2-0063: unknown chip version Si21255-\xff\xff\xff
g
Hi!
I tried downloading a new firmware via
case SI_BOGUS:
- dev_info(&client->dev, "Bogus chip version, trying
with no firmware\n");
- fw_name = NULL;
+ dev_info(&client->dev, "Bogus chip version, trying
with new firmware\n");
+ fw_name = SI2157_A30_FIRMWARE;
break;
which I downloaded from
+ //
https://github.com/CoreELEC/dvb-firmware/blob/master/firmware/dvb-tuner-si2157-a30-01.fw
resulting in
[ 209.312086] si2168 1-0067: downloading firmware from file
'dvb-demod-si2168-b40-01.fw'
[ 211.535097] si2168 1-0067: firmware version: B 4.0.25
[ 211.554938] si2157 2-0063: Bogus chip version, trying with new firmware
[ 211.554944] si2157 2-0063: found a 'Silicon Labs Si21255-\xff\xff\xff'
[ 211.557978] si2157 2-0063: downloading firmware from file
'dvb-tuner-si2157-a30-01.fw'
[ 215.739092] si2157 2-0063: rebooting tuner...
[ 215.755271] si2157 2-0063: querying firmware version...
[ 215.760756] si2157 2-0063: firmware version: \xff.\xff.255
. So even with a new firmware the queried numbers are bogus.
g
Em Thu, 3 Oct 2019 15:53:30 +0200 Gonsolo <gonsolo@gmail.com> escreveu: > Hi! > > I tried downloading a new firmware via > > case SI_BOGUS: > - dev_info(&client->dev, "Bogus chip version, trying > with no firmware\n"); > - fw_name = NULL; > + dev_info(&client->dev, "Bogus chip version, trying > with new firmware\n"); > + fw_name = SI2157_A30_FIRMWARE; > break; > > which I downloaded from > > + // > https://github.com/CoreELEC/dvb-firmware/blob/master/firmware/dvb-tuner-si2157-a30-01.fw > > resulting in > > [ 209.312086] si2168 1-0067: downloading firmware from file > 'dvb-demod-si2168-b40-01.fw' > [ 211.535097] si2168 1-0067: firmware version: B 4.0.25 > [ 211.554938] si2157 2-0063: Bogus chip version, trying with new firmware > [ 211.554944] si2157 2-0063: found a 'Silicon Labs Si21255-\xff\xff\xff' > [ 211.557978] si2157 2-0063: downloading firmware from file > 'dvb-tuner-si2157-a30-01.fw' > [ 215.739092] si2157 2-0063: rebooting tuner... > [ 215.755271] si2157 2-0063: querying firmware version... > [ 215.760756] si2157 2-0063: firmware version: \xff.\xff.255 > > . So even with a new firmware the queried numbers are bogus. Try to reduce the bus speed to 10kbps > > g Thanks, Mauro
> Try to reduce the bus speed to 10kbps
Nope:
#define I2C_SPEED_REGISTER 260 // ~10k
[ 117.860961] si2168 1-0067: downloading firmware from file
'dvb-demod-si2168-b40-01.fw'
[ 118.958355] si2168 1-0067: firmware version: B 4.0.25
[ 118.968882] si2157 2-0063: Bogus chip version, trying with new firmware
[ 118.968888] si2157 2-0063: found a 'Silicon Labs Si21255-\xff\xff\xff'
[ 118.972005] si2157 2-0063: downloading firmware from file
'dvb-tuner-si2157-a30-01.fw'
[ 121.154130] si2157 2-0063: rebooting tuner...
[ 121.169626] si2157 2-0063: querying firmware version...
[ 121.172799] si2157 2-0063: firmware version: \xff.\xff.255
[ 121.172803] si2157 2-0063: querying chip revision...
[ 121.175911] si2157 2-0063: chip revision: 255.255.255.255
g
> So, I would add a msleep() somewhere after the firmware update.
I tried that to no avail:
release_firmware(fw);
+ msleep(1000);
[ 107.903918] si2157 2-0063: firmware version: \xff.\xff.255
[ 107.903920] si2157 2-0063: querying chip revision...
[ 107.906970] si2157 2-0063: chip revision: 255.255.255.255
Em Thu, 3 Oct 2019 17:00:08 +0200 Gonsolo <gonsolo@gmail.com> escreveu: > > So, I would add a msleep() somewhere after the firmware update. > > I tried that to no avail: > > release_firmware(fw); > + msleep(1000); > > [ 107.903918] si2157 2-0063: firmware version: \xff.\xff.255 > [ 107.903920] si2157 2-0063: querying chip revision... > [ 107.906970] si2157 2-0063: chip revision: 255.255.255.255 > With the original patch you proposed, what are the logs? Thanks, Mauro
> With the original patch you proposed, what are the logs?
Which one do you mean? That one:
+ case SI_BOGUS:
+ dev_info(&client->dev, "Bogus chip version, trying
with no firmware\n");
+ fw_name = NULL;
+ break;
With this patch VLC is running but the chip revision number and
firmware version are still bogus.
Which means if you receive 0xffffffff you can force no firmware and it runs.
> With the original patch you proposed, what are the logs?
With the following patch applied to media_tree master:
diff --git a/drivers/media/tuners/si2157.c b/drivers/media/tuners/si2157.c
index e87040d6eca7..4c1ab0b6876a 100644
--- a/drivers/media/tuners/si2157.c
+++ b/drivers/media/tuners/si2157.c
@@ -129,13 +129,14 @@ static int si2157_init(struct dvb_frontend *fe)
chip_id = cmd.args[1] << 24 | cmd.args[2] << 16 | cmd.args[3] << 8 |
cmd.args[4] << 0;
- #define SI2177_A30 ('A' << 24 | 77 << 16 | '3' << 8 | '0' << 0)
- #define SI2158_A20 ('A' << 24 | 58 << 16 | '2' << 8 | '0' << 0)
- #define SI2148_A20 ('A' << 24 | 48 << 16 | '2' << 8 | '0' << 0)
- #define SI2157_A30 ('A' << 24 | 57 << 16 | '3' << 8 | '0' << 0)
- #define SI2147_A30 ('A' << 24 | 47 << 16 | '3' << 8 | '0' << 0)
- #define SI2146_A10 ('A' << 24 | 46 << 16 | '1' << 8 | '0' << 0)
- #define SI2141_A10 ('A' << 24 | 41 << 16 | '1' << 8 | '0' << 0)
+ #define SI2177_A30 ('A' << 24 | 77 << 16 | '3' << 8 | '0' << 0)
+ #define SI2158_A20 ('A' << 24 | 58 << 16 | '2' << 8 | '0' << 0)
+ #define SI2148_A20 ('A' << 24 | 48 << 16 | '2' << 8 | '0' << 0)
+ #define SI2157_A30 ('A' << 24 | 57 << 16 | '3' << 8 | '0' << 0)
+ #define SI2147_A30 ('A' << 24 | 47 << 16 | '3' << 8 | '0' << 0)
+ #define SI2146_A10 ('A' << 24 | 46 << 16 | '1' << 8 | '0' << 0)
+ #define SI2141_A10 ('A' << 24 | 41 << 16 | '1' << 8 | '0' << 0)
+ #define SI_BOGUS (255 << 24 | 255 << 16 | 255 << 8 | 255 << 0)
switch (chip_id) {
case SI2158_A20:
@@ -148,6 +149,10 @@ static int si2157_init(struct dvb_frontend *fe)
case SI2177_A30:
fw_name = SI2157_A30_FIRMWARE;
break;
+ case SI_BOGUS:
+ dev_info(&client->dev, "Bogus chip version, trying with no firmware\n");
+ fw_name = NULL;
+ break;
case SI2157_A30:
case SI2147_A30:
case SI2146_A10:
@@ -225,6 +230,7 @@ static int si2157_init(struct dvb_frontend *fe)
dev_info(&client->dev, "firmware version: %c.%c.%d\n",
cmd.args[6], cmd.args[7], cmd.args[8]);
warm:
/* init statistics in order signal app which are supported */
c->strength.len = 1;
diff --git a/drivers/media/usb/dvb-usb-v2/af9035.c b/drivers/media/usb/dvb-usb-v2/af9035.c
index 3afd18733614..a8d59cf06b1e 100644
--- a/drivers/media/usb/dvb-usb-v2/af9035.c
+++ b/drivers/media/usb/dvb-usb-v2/af9035.c
@@ -1197,6 +1197,11 @@ static int af9035_frontend_attach(struct dvb_usb_adapter *adap)
return ret;
}
+/* I2C speed register = (1000000000 / (24.4 * 16 * I2C_speed))
+ * 7 equals ~400k, 26 ~100k and 260 ~10k.
+ * */
+#define I2C_SPEED_REGISTER 7
+
static int it930x_frontend_attach(struct dvb_usb_adapter *adap)
{
struct state *state = adap_to_priv(adap);
@@ -1208,13 +1213,13 @@ static int it930x_frontend_attach(struct dvb_usb_adapter *adap)
dev_dbg(&intf->dev, "adap->id=%d\n", adap->id);
- /* I2C master bus 2 clock speed 300k */
- ret = af9035_wr_reg(d, 0x00f6a7, 0x07);
+ /* I2C master bus 2 clock speed */
+ ret = af9035_wr_reg(d, 0x00f6a7, I2C_SPEED_REGISTER);
if (ret < 0)
goto err;
- /* I2C master bus 1,3 clock speed 300k */
- ret = af9035_wr_reg(d, 0x00f103, 0x07);
+ /* I2C master bus 1,3 clock speed */
+ ret = af9035_wr_reg(d, 0x00f103, I2C_SPEED_REGISTER);
if (ret < 0)
goto err;
@@ -2119,6 +2124,8 @@ static const struct usb_device_id af9035_id_table[] = {
/* IT930x devices */
{ DVB_USB_DEVICE(USB_VID_ITETECH, USB_PID_ITETECH_IT9303,
&it930x_props, "ITE 9303 Generic", NULL) },
+ { DVB_USB_DEVICE(USB_VID_DEXATEK, 0x0100,
+ &it930x_props, "Logilink VG0022A", NULL) },
{ DVB_USB_DEVICE(USB_VID_AVERMEDIA, USB_PID_AVERMEDIA_TD310,
&it930x_props, "AVerMedia TD310 DVB-T2", NULL) },
{ }
the Messages at boot time are
[ 4.262882] si2168 1-0067: Silicon Labs Si2168-B40 successfully identified
[ 4.262884] si2168 1-0067: firmware version: B 4.0.2
[ 4.262902] usb 2-1: DVB: registering adapter 0 frontend 0 (Silicon Labs Si2168)...
[ 4.262908] dvbdev: dvb_create_media_entity: media entity 'Silicon Labs Si2168' registered.
[ 4.289776] si2157 2-0063: Silicon Labs Si2147/2148/2157/2158 successfully attached
and the messages when running vlc (successfully) are
[ 486.537128] si2168 1-0067: downloading firmware from file 'dvb-demod-si2168-b40-01.fw'
[ 487.795436] si2168 1-0067: firmware version: B 4.0.25
[ 487.807614] si2157 2-0063: Bogus chip version, trying with no firmware
[ 487.807618] si2157 2-0063: found a 'Silicon Labs Si21255-\xff\xff\xff'
[ 487.833876] si2157 2-0063: firmware version: \xff.\xff.255
g
Em Thu, 3 Oct 2019 18:03:36 +0200 Gon Solo <gonsolo@gmail.com> escreveu: > > With the original patch you proposed, what are the logs? > > With the following patch applied to media_tree master: > > diff --git a/drivers/media/tuners/si2157.c b/drivers/media/tuners/si2157.c > index e87040d6eca7..4c1ab0b6876a 100644 > --- a/drivers/media/tuners/si2157.c > +++ b/drivers/media/tuners/si2157.c > @@ -129,13 +129,14 @@ static int si2157_init(struct dvb_frontend *fe) > chip_id = cmd.args[1] << 24 | cmd.args[2] << 16 | cmd.args[3] << 8 | > cmd.args[4] << 0; > > - #define SI2177_A30 ('A' << 24 | 77 << 16 | '3' << 8 | '0' << 0) > - #define SI2158_A20 ('A' << 24 | 58 << 16 | '2' << 8 | '0' << 0) > - #define SI2148_A20 ('A' << 24 | 48 << 16 | '2' << 8 | '0' << 0) > - #define SI2157_A30 ('A' << 24 | 57 << 16 | '3' << 8 | '0' << 0) > - #define SI2147_A30 ('A' << 24 | 47 << 16 | '3' << 8 | '0' << 0) > - #define SI2146_A10 ('A' << 24 | 46 << 16 | '1' << 8 | '0' << 0) > - #define SI2141_A10 ('A' << 24 | 41 << 16 | '1' << 8 | '0' << 0) > + #define SI2177_A30 ('A' << 24 | 77 << 16 | '3' << 8 | '0' << 0) > + #define SI2158_A20 ('A' << 24 | 58 << 16 | '2' << 8 | '0' << 0) > + #define SI2148_A20 ('A' << 24 | 48 << 16 | '2' << 8 | '0' << 0) > + #define SI2157_A30 ('A' << 24 | 57 << 16 | '3' << 8 | '0' << 0) > + #define SI2147_A30 ('A' << 24 | 47 << 16 | '3' << 8 | '0' << 0) > + #define SI2146_A10 ('A' << 24 | 46 << 16 | '1' << 8 | '0' << 0) > + #define SI2141_A10 ('A' << 24 | 41 << 16 | '1' << 8 | '0' << 0) > + #define SI_BOGUS (255 << 24 | 255 << 16 | 255 << 8 | 255 << 0) > > switch (chip_id) { > case SI2158_A20: > @@ -148,6 +149,10 @@ static int si2157_init(struct dvb_frontend *fe) > case SI2177_A30: > fw_name = SI2157_A30_FIRMWARE; > break; > + case SI_BOGUS: > + dev_info(&client->dev, "Bogus chip version, trying with no firmware\n"); > + fw_name = NULL; > + break; > case SI2157_A30: > case SI2147_A30: > case SI2146_A10: > @@ -225,6 +230,7 @@ static int si2157_init(struct dvb_frontend *fe) > > dev_info(&client->dev, "firmware version: %c.%c.%d\n", > cmd.args[6], cmd.args[7], cmd.args[8]); > warm: > /* init statistics in order signal app which are supported */ > c->strength.len = 1; > diff --git a/drivers/media/usb/dvb-usb-v2/af9035.c b/drivers/media/usb/dvb-usb-v2/af9035.c > index 3afd18733614..a8d59cf06b1e 100644 > --- a/drivers/media/usb/dvb-usb-v2/af9035.c > +++ b/drivers/media/usb/dvb-usb-v2/af9035.c > @@ -1197,6 +1197,11 @@ static int af9035_frontend_attach(struct dvb_usb_adapter *adap) > return ret; > } > > +/* I2C speed register = (1000000000 / (24.4 * 16 * I2C_speed)) > + * 7 equals ~400k, 26 ~100k and 260 ~10k. > + * */ > +#define I2C_SPEED_REGISTER 7 > + > static int it930x_frontend_attach(struct dvb_usb_adapter *adap) > { > struct state *state = adap_to_priv(adap); > @@ -1208,13 +1213,13 @@ static int it930x_frontend_attach(struct dvb_usb_adapter *adap) > > dev_dbg(&intf->dev, "adap->id=%d\n", adap->id); > > - /* I2C master bus 2 clock speed 300k */ > - ret = af9035_wr_reg(d, 0x00f6a7, 0x07); > + /* I2C master bus 2 clock speed */ > + ret = af9035_wr_reg(d, 0x00f6a7, I2C_SPEED_REGISTER); > if (ret < 0) > goto err; > > - /* I2C master bus 1,3 clock speed 300k */ > - ret = af9035_wr_reg(d, 0x00f103, 0x07); > + /* I2C master bus 1,3 clock speed */ > + ret = af9035_wr_reg(d, 0x00f103, I2C_SPEED_REGISTER); > if (ret < 0) > goto err; > > @@ -2119,6 +2124,8 @@ static const struct usb_device_id af9035_id_table[] = { > /* IT930x devices */ > { DVB_USB_DEVICE(USB_VID_ITETECH, USB_PID_ITETECH_IT9303, > &it930x_props, "ITE 9303 Generic", NULL) }, > + { DVB_USB_DEVICE(USB_VID_DEXATEK, 0x0100, > + &it930x_props, "Logilink VG0022A", NULL) }, > { DVB_USB_DEVICE(USB_VID_AVERMEDIA, USB_PID_AVERMEDIA_TD310, > &it930x_props, "AVerMedia TD310 DVB-T2", NULL) }, > { } > > the Messages at boot time are > > [ 4.262882] si2168 1-0067: Silicon Labs Si2168-B40 successfully identified > [ 4.262884] si2168 1-0067: firmware version: B 4.0.2 > [ 4.262902] usb 2-1: DVB: registering adapter 0 frontend 0 (Silicon Labs Si2168)... > [ 4.262908] dvbdev: dvb_create_media_entity: media entity 'Silicon Labs Si2168' registered. > [ 4.289776] si2157 2-0063: Silicon Labs Si2147/2148/2157/2158 successfully attached > > and the messages when running vlc (successfully) are > > [ 486.537128] si2168 1-0067: downloading firmware from file 'dvb-demod-si2168-b40-01.fw' > [ 487.795436] si2168 1-0067: firmware version: B 4.0.25 > [ 487.807614] si2157 2-0063: Bogus chip version, trying with no firmware > [ 487.807618] si2157 2-0063: found a 'Silicon Labs Si21255-\xff\xff\xff' > [ 487.833876] si2157 2-0063: firmware version: \xff.\xff.255 No, I mean with the first patch you sent to the ML, with the powerup hack. Thanks, Mauro
> No, I mean with the first patch you sent to the ML, with the powerup > hack. Boot time: [ 4.653257] si2168 1-0067: Silicon Labs Si2168-B40 successfully identified [ 4.653259] si2168 1-0067: firmware version: B 4.0.2 [ 4.653279] usb 2-1: DVB: registering adapter 0 frontend 0 (Silicon Labs Si2168)... [ 4.653284] dvbdev: dvb_create_media_entity: media entity 'Silicon Labs Si2168' registered. ... [ 4.694785] si2157 2-0063: found a 'Silicon Labs Si2147-A30' [ 4.694787] si2157 2-0063: Silicon Labs Si2147/2148/2157/2158 successfully attached [ 4.717814] usb 2-1: dvb_usb_v2: 'Logilink VG0022A' successfully initialized and connected [ 4.717880] usbcore: registered new interface driver dvb_usb_af9035 VLC time: [ 175.490609] si2168 1-0067: downloading firmware from file 'dvb-demod-si2168-b40-01.fw' [ 176.746585] si2168 1-0067: firmware version: B 4.0.25 [ 176.781570] si2157 2-0063: firmware version: \xff.\xff.255 g
Em Thu, 3 Oct 2019 18:23:26 +0200 Gon Solo <gonsolo@gmail.com> escreveu: > > No, I mean with the first patch you sent to the ML, with the powerup > > hack. > > Boot time: > > [ 4.653257] si2168 1-0067: Silicon Labs Si2168-B40 successfully identified > [ 4.653259] si2168 1-0067: firmware version: B 4.0.2 > [ 4.653279] usb 2-1: DVB: registering adapter 0 frontend 0 (Silicon Labs Si2168)... > [ 4.653284] dvbdev: dvb_create_media_entity: media entity 'Silicon Labs Si2168' registered. > ... > [ 4.694785] si2157 2-0063: found a 'Silicon Labs Si2147-A30' > [ 4.694787] si2157 2-0063: Silicon Labs Si2147/2148/2157/2158 successfully attached > [ 4.717814] usb 2-1: dvb_usb_v2: 'Logilink VG0022A' successfully initialized and connected > [ 4.717880] usbcore: registered new interface driver dvb_usb_af9035 > > VLC time: > > [ 175.490609] si2168 1-0067: downloading firmware from file 'dvb-demod-si2168-b40-01.fw' > [ 176.746585] si2168 1-0067: firmware version: B 4.0.25 > [ 176.781570] si2157 2-0063: firmware version: \xff.\xff.255 Weird... it sounds that, after si2168 has its firmware updated, it starts interfering at si2157. Perhaps there's a bug at si2168 I2C gate mux logic. Are you using a new Kernel like 5.2? I guess the best is to enable the debug logs in order to see what's happening on both cases. You can enable all debug messages (after loading the modules) with: # for i in $(cat /sys/kernel/debug/dynamic_debug/control |grep -E '(si21|af9035)' |cut -d' ' -f 1); do echo "file $i +p" >>/sys/kernel/debug/dynamic_debug/control; done You could also try to disable the firmware upload at si2168 and see if the si2157 reads will succeed. Thanks, Mauro
> Weird... it sounds that, after si2168 has its firmware updated, it > starts interfering at si2157. Perhaps there's a bug at si2168 I2C > gate mux logic. Are you using a new Kernel like 5.2? Everything is based on git://linuxtv.org/media_tree.git which is at 5.4-rc1 right now. > I guess the best is to enable the debug logs in order to see what's > happening on both cases. > > You can enable all debug messages (after loading the modules) with: > > # for i in $(cat /sys/kernel/debug/dynamic_debug/control |grep -E '(si21|af9035)' |cut -d' ' -f 1); do echo "file $i +p" >>/sys/kernel/debug/dynamic_debug/control; done I'll give that a try. > You could also try to disable the firmware upload at si2168 and see > if the si2157 reads will succeed. That too. Thanks for the advice.
> You could also try to disable the firmware upload at si2168 and see > if the si2157 reads will succeed. I disabled the upload and while vlc wasn't working anymore I got the following messages: [ 168.196656] si2157 2-0063: found a 'Silicon Labs Si2147-A30' [ 168.223009] si2157 2-0063: firmware version: 3.0.5 It really seems that the firmware upload is the culprit. g
On 10/3/19 8:32 PM, Gon Solo wrote: >> You could also try to disable the firmware upload at si2168 and see >> if the si2157 reads will succeed. > I disabled the upload and while vlc wasn't working anymore I got the > following messages: > > [ 168.196656] si2157 2-0063: found a 'Silicon Labs Si2147-A30' > [ 168.223009] si2157 2-0063: firmware version: 3.0.5 > > > It really seems that the firmware upload is the culprit. try other firmware? http://palosaari.fi/linux/v4l-dvb/firmware/Si2168/ > > g > >
> try other firmware? > http://palosaari.fi/linux/v4l-dvb/firmware/Si2168/ I already downloaded version 4.0.25 from there. -rw-r--r-- 1 root root 6,8K Apr 5 2018 /lib/firmware/dvb-demod-si2168-b40-01.fw.gonzo -rw-rw-r-- 1 gonsolo gonsolo 16K Okt 3 13:08 /lib/firmware/dvb-demod-si2168-b40-01.fw No difference.
I also tried to add a msleep(1000) after the si2168 firmware upload; no difference. g
> try other firmware? > http://palosaari.fi/linux/v4l-dvb/firmware/Si2168/ I tried all of them. No difference.
Em Thu, 3 Oct 2019 21:19:16 +0200 Gonsolo <gonsolo@gmail.com> escreveu: > > try other firmware? > > http://palosaari.fi/linux/v4l-dvb/firmware/Si2168/ > > I tried all of them. No difference. Maybe the vendor of this device wrote a different firmware. That happens. Thanks, Mauro
From si2168.c:808:
/* Sometimes firmware returns bogus value */
if (utmp1 == 0xffff)
utmp1 = 0;
Maybe we can include my "bogus" hack to get at least Logilink running.
Maybe with an info message to tell users what is going on.
g
Em Thu, 3 Oct 2019 16:39:14 -0300 Mauro Carvalho Chehab <mchehab+samsung@kernel.org> escreveu: > Em Thu, 3 Oct 2019 21:19:16 +0200 > Gonsolo <gonsolo@gmail.com> escreveu: > > > > try other firmware? > > > http://palosaari.fi/linux/v4l-dvb/firmware/Si2168/ > > > > I tried all of them. No difference. > > Maybe the vendor of this device wrote a different firmware. That happens. Two additional comments: 1) The firmware file is likely at the Windows driver for this device (probably using a different format). It should be possible to get it from there. 2) Another possibility would be to add a way to tell the si2168 driver to not try to load a firmware, using the original one. That would require adding a field at si2168_config to allow signalizing to it that it should not try to load a firmware file, and add a quirk at the af9035 that would set such flag for Logilink VG0022A. Option (1) is the best one. Thanks, Mauro
> 1) The firmware file is likely at the Windows driver for this device > (probably using a different format). It should be possible to get > it from there. If you tell me how I'm willing to do this. :) > 2) Another possibility would be to add a way to tell the si2168 driver > to not try to load a firmware, using the original one. That would > require adding a field at si2168_config to allow signalizing to it > that it should not try to load a firmware file, and add a quirk at > the af9035 that would set such flag for Logilink VG0022A. I don't get this. Which firmware, si2168 or si2157? I'm still for option 3: If there is a bogus chip revision number it's likely the VG0022A and we can safely set fw to NULL, in which case everything works. All already working devices will continue to work as before. With a low probability there are other devices that will return 0xffff but a) they didn't work until now and b) they receive a clear message that they return bogus numbers and this works just for the VG0022A, in which case this hardware can be tested. At last, *my* VG0022A will work without a custom kernel which I'm a big fan of. :)) Are there any counterarguments except that it is not the cleanest solution in the universe? ;)
Em Thu, 3 Oct 2019 21:40:28 +0200 Gonsolo <gonsolo@gmail.com> escreveu: > From si2168.c:808: > /* Sometimes firmware returns bogus value */ > if (utmp1 == 0xffff) > utmp1 = 0; Huh? are you using the upstream Kernel? the above code is at line 215! Please always use the upstream code when sending patches. > > Maybe we can include my "bogus" hack to get at least Logilink running. > Maybe with an info message to tell users what is going on. > > g Thanks, Mauro
> Huh? are you using the upstream Kernel? the above code is at line 215! > Please always use the upstream code when sending patches. Sorry, I was confused by my vi line: "drivers/media/dvb-frontends/si2168.c" 808 lines --26%-- 212,1-8 25%" Twelve hours behind this screen. I think I have to have a walk in the forest right now. :)
Em Thu, 3 Oct 2019 21:51:35 +0200 Gonsolo <gonsolo@gmail.com> escreveu: > > 1) The firmware file is likely at the Windows driver for this device > > (probably using a different format). It should be possible to get > > it from there. > > If you tell me how I'm willing to do this. :) I don't know. I was not the one that extracted the firmware. I guess Antti did it. I suspect that there are some comments about that in the past at the ML. seek at lore.kernel.org. > > > 2) Another possibility would be to add a way to tell the si2168 driver > > to not try to load a firmware, using the original one. That would > > require adding a field at si2168_config to allow signalizing to it > > that it should not try to load a firmware file, and add a quirk at > > the af9035 that would set such flag for Logilink VG0022A. > > I don't get this. Which firmware, si2168 or si2157? The one that it is causing the problem. If I understood well, the culprit was the si2168 firmware. > I'm still for option 3: If there is a bogus chip revision number it's > likely the VG0022A and we can safely set fw to NULL, in which case > everything works. > All already working devices will continue to work as before. > With a low probability there are other devices that will return 0xffff > but a) they didn't work until now and b) they receive a clear message > that they return bogus numbers and this works just for the VG0022A, in > which case this hardware can be tested. > At last, *my* VG0022A will work without a custom kernel which I'm a > big fan of. :)) > > Are there any counterarguments except that it is not the cleanest > solution in the universe? ;) That's a really bad solution. Returning 0xff is what happens when things go wrong during I2C transfers. Several problems can cause it, including device misfunction. Every time someone comes with a patch trying to ignore it, things go sideways for other devices (existing or future ones). Ignoring errors is always a bad idea. Also, it is a very bad idea to load a firmware that it is not compatible with a device. There are even cases of devices that were burned due to that[1]. [1] XCeive has two versions of 3028/2028 chipsets. One with a "normal power" and a "low power" version. If the firmware for the "normal power" (version 2.7) is used at the "low power" chip (with requires version 3.6), it makes the chipset hotter and reduces a lot the lifetime of the tuner. Thanks, Mauro
> I don't know. I was not the one that extracted the firmware. I guess > Antti did it. Ok. > That's a really bad solution. Returning 0xff is what happens when > things go wrong during I2C transfers. Several problems can cause it, > including device misfunction. Every time someone comes with a patch > trying to ignore it, things go sideways for other devices (existing > or future ones). > > Ignoring errors is always a bad idea. I understand. Anyway, I have to give up for now. Maybe I will have some time in the future to come back to this or somebody else can use the information in this thread. :( Thanks for your time, I appreciate that very much. It was worth a try. :)
On 10/3/19 10:03 PM, Mauro Carvalho Chehab wrote: > Em Thu, 3 Oct 2019 21:51:35 +0200 > Gonsolo <gonsolo@gmail.com> escreveu: > >>> 1) The firmware file is likely at the Windows driver for this device >>> (probably using a different format). It should be possible to get >>> it from there. >> If you tell me how I'm willing to do this. :) > I don't know. I was not the one that extracted the firmware. I guess > Antti did it. > > I suspect that there are some comments about that in the past at the > ML. seek at lore.kernel.org. > >>> 2) Another possibility would be to add a way to tell the si2168 driver >>> to not try to load a firmware, using the original one. That would >>> require adding a field at si2168_config to allow signalizing to it >>> that it should not try to load a firmware file, and add a quirk at >>> the af9035 that would set such flag for Logilink VG0022A. >> I don't get this. Which firmware, si2168 or si2157? > The one that it is causing the problem. If I understood well, the > culprit was the si2168 firmware. > >> I'm still for option 3: If there is a bogus chip revision number it's >> likely the VG0022A and we can safely set fw to NULL, in which case >> everything works. >> All already working devices will continue to work as before. >> With a low probability there are other devices that will return 0xffff >> but a) they didn't work until now and b) they receive a clear message >> that they return bogus numbers and this works just for the VG0022A, in >> which case this hardware can be tested. >> At last, *my* VG0022A will work without a custom kernel which I'm a >> big fan of. :)) >> >> Are there any counterarguments except that it is not the cleanest >> solution in the universe? ;) > That's a really bad solution. Returning 0xff is what happens when > things go wrong during I2C transfers. Several problems can cause it, > including device misfunction. Every time someone comes with a patch > trying to ignore it, things go sideways for other devices (existing > or future ones). > > Ignoring errors is always a bad idea. add module param say 'gonso_hack_vg0022a' if true, act on error by setting a flag if this flag is set don't load firmware Jan Pieter.
Em Fri, 4 Oct 2019 13:50:43 +0200 JP <jp@jpvw.nl> escreveu: > On 10/3/19 10:03 PM, Mauro Carvalho Chehab wrote: > > Em Thu, 3 Oct 2019 21:51:35 +0200 > > Gonsolo <gonsolo@gmail.com> escreveu: > > > >>> 1) The firmware file is likely at the Windows driver for this device > >>> (probably using a different format). It should be possible to get > >>> it from there. > >> If you tell me how I'm willing to do this. :) > > I don't know. I was not the one that extracted the firmware. I guess > > Antti did it. > > > > I suspect that there are some comments about that in the past at the > > ML. seek at lore.kernel.org. > > > >>> 2) Another possibility would be to add a way to tell the si2168 driver > >>> to not try to load a firmware, using the original one. That would > >>> require adding a field at si2168_config to allow signalizing to it > >>> that it should not try to load a firmware file, and add a quirk at > >>> the af9035 that would set such flag for Logilink VG0022A. > >> I don't get this. Which firmware, si2168 or si2157? > > The one that it is causing the problem. If I understood well, the > > culprit was the si2168 firmware. > > > >> I'm still for option 3: If there is a bogus chip revision number it's > >> likely the VG0022A and we can safely set fw to NULL, in which case > >> everything works. > >> All already working devices will continue to work as before. > >> With a low probability there are other devices that will return 0xffff > >> but a) they didn't work until now and b) they receive a clear message > >> that they return bogus numbers and this works just for the VG0022A, in > >> which case this hardware can be tested. > >> At last, *my* VG0022A will work without a custom kernel which I'm a > >> big fan of. :)) > >> > >> Are there any counterarguments except that it is not the cleanest > >> solution in the universe? ;) > > That's a really bad solution. Returning 0xff is what happens when > > things go wrong during I2C transfers. Several problems can cause it, > > including device misfunction. Every time someone comes with a patch > > trying to ignore it, things go sideways for other devices (existing > > or future ones). > > > > Ignoring errors is always a bad idea. > add module param say 'gonso_hack_vg0022a' > if true, act on error by setting a flag > if this flag is set don't load firmware Adding a module param should be the last resort, only when there's no way for the driver to autodetect. Making af9035 to detect vg0022a is quite simple. Considering this device's entry: { DVB_USB_DEVICE(USB_VID_DEXATEK, 0x0100, &it930x_props, "Logilink VG0022A", NULL) }, the check, at af9035 would be: if (le16_to_cpu(d->udev->descriptor.idVendor) == USB_VID_DEXATEK && le16_to_cpu(d->udev->descriptor.idProduct) == 0x0100) /* do something to disable firmware load */ So, no need to add any load time parameter. It should be noticed that a change just at af9035 won't work, as the firmware is updated by si2168 driver. So, the caller code needs to pass a config parameter to si2168 driver. Thanks, Mauro
On 10/4/19 2:08 PM, Mauro Carvalho Chehab wrote: > Em Fri, 4 Oct 2019 13:50:43 +0200 > JP <jp@jpvw.nl> escreveu: > >> On 10/3/19 10:03 PM, Mauro Carvalho Chehab wrote: >>> Em Thu, 3 Oct 2019 21:51:35 +0200 >>> Gonsolo <gonsolo@gmail.com> escreveu: >>> >>>>> 1) The firmware file is likely at the Windows driver for this device >>>>> (probably using a different format). It should be possible to get >>>>> it from there. >>>> If you tell me how I'm willing to do this. :) >>> I don't know. I was not the one that extracted the firmware. I guess >>> Antti did it. >>> >>> I suspect that there are some comments about that in the past at the >>> ML. seek at lore.kernel.org. >>> >>>>> 2) Another possibility would be to add a way to tell the si2168 driver >>>>> to not try to load a firmware, using the original one. That would >>>>> require adding a field at si2168_config to allow signalizing to it >>>>> that it should not try to load a firmware file, and add a quirk at >>>>> the af9035 that would set such flag for Logilink VG0022A. >>>> I don't get this. Which firmware, si2168 or si2157? >>> The one that it is causing the problem. If I understood well, the >>> culprit was the si2168 firmware. >>> >>>> I'm still for option 3: If there is a bogus chip revision number it's >>>> likely the VG0022A and we can safely set fw to NULL, in which case >>>> everything works. >>>> All already working devices will continue to work as before. >>>> With a low probability there are other devices that will return 0xffff >>>> but a) they didn't work until now and b) they receive a clear message >>>> that they return bogus numbers and this works just for the VG0022A, in >>>> which case this hardware can be tested. >>>> At last, *my* VG0022A will work without a custom kernel which I'm a >>>> big fan of. :)) >>>> >>>> Are there any counterarguments except that it is not the cleanest >>>> solution in the universe? ;) >>> That's a really bad solution. Returning 0xff is what happens when >>> things go wrong during I2C transfers. Several problems can cause it, >>> including device misfunction. Every time someone comes with a patch >>> trying to ignore it, things go sideways for other devices (existing >>> or future ones). >>> >>> Ignoring errors is always a bad idea. >> add module param say 'gonso_hack_vg0022a' >> if true, act on error by setting a flag >> if this flag is set don't load firmware > Adding a module param should be the last resort, only when there's > no way for the driver to autodetect. Remember the guy reported the hw fix? Could be that only some receiver units are affected. Therefore the module param. The hw fix was original 4k7 and 10k added. That looks like 3k3 total and all 3 chips on the bus work. 10k per chip. Now Gon reported that said bus works with 2 chips active on a faulty device with 4k7 resistor, which is 2 times 10k. It looks same hw error to me. > Making af9035 to detect vg0022a is quite simple. > > Considering this device's entry: > > { DVB_USB_DEVICE(USB_VID_DEXATEK, 0x0100, > &it930x_props, "Logilink VG0022A", NULL) }, > > the check, at af9035 would be: > > if (le16_to_cpu(d->udev->descriptor.idVendor) == USB_VID_DEXATEK && > le16_to_cpu(d->udev->descriptor.idProduct) == 0x0100) > /* do something to disable firmware load */ > > So, no need to add any load time parameter. > > It should be noticed that a change just at af9035 won't work, as the > firmware is updated by si2168 driver. So, the caller code needs to > pass a config parameter to si2168 driver. If it is a failing pull-up resistor on only some individual receiver units, this seems overkill to me. In my proposal I did not realized this change in the demod driver was needed. > Thanks, > Mauro > Thank you.
Em Fri, 4 Oct 2019 15:50:18 +0200 JP <jp@jpvw.nl> escreveu: > On 10/4/19 2:08 PM, Mauro Carvalho Chehab wrote: > > Em Fri, 4 Oct 2019 13:50:43 +0200 > > JP <jp@jpvw.nl> escreveu: > > > >> On 10/3/19 10:03 PM, Mauro Carvalho Chehab wrote: > >>> Em Thu, 3 Oct 2019 21:51:35 +0200 > >>> Gonsolo <gonsolo@gmail.com> escreveu: > >>> > >>>>> 1) The firmware file is likely at the Windows driver for this device > >>>>> (probably using a different format). It should be possible to get > >>>>> it from there. > >>>> If you tell me how I'm willing to do this. :) > >>> I don't know. I was not the one that extracted the firmware. I guess > >>> Antti did it. > >>> > >>> I suspect that there are some comments about that in the past at the > >>> ML. seek at lore.kernel.org. > >>> > >>>>> 2) Another possibility would be to add a way to tell the si2168 driver > >>>>> to not try to load a firmware, using the original one. That would > >>>>> require adding a field at si2168_config to allow signalizing to it > >>>>> that it should not try to load a firmware file, and add a quirk at > >>>>> the af9035 that would set such flag for Logilink VG0022A. > >>>> I don't get this. Which firmware, si2168 or si2157? > >>> The one that it is causing the problem. If I understood well, the > >>> culprit was the si2168 firmware. > >>> > >>>> I'm still for option 3: If there is a bogus chip revision number it's > >>>> likely the VG0022A and we can safely set fw to NULL, in which case > >>>> everything works. > >>>> All already working devices will continue to work as before. > >>>> With a low probability there are other devices that will return 0xffff > >>>> but a) they didn't work until now and b) they receive a clear message > >>>> that they return bogus numbers and this works just for the VG0022A, in > >>>> which case this hardware can be tested. > >>>> At last, *my* VG0022A will work without a custom kernel which I'm a > >>>> big fan of. :)) > >>>> > >>>> Are there any counterarguments except that it is not the cleanest > >>>> solution in the universe? ;) > >>> That's a really bad solution. Returning 0xff is what happens when > >>> things go wrong during I2C transfers. Several problems can cause it, > >>> including device misfunction. Every time someone comes with a patch > >>> trying to ignore it, things go sideways for other devices (existing > >>> or future ones). > >>> > >>> Ignoring errors is always a bad idea. > >> add module param say 'gonso_hack_vg0022a' > >> if true, act on error by setting a flag > >> if this flag is set don't load firmware > > Adding a module param should be the last resort, only when there's > > no way for the driver to autodetect. > Remember the guy reported the hw fix? Could be that > only some receiver units are affected. Therefore the > module param. > > The hw fix was original 4k7 and 10k added. That looks > like 3k3 total and all 3 chips on the bus work. 10k per > chip. Now Gon reported that said bus works with 2 chips > active on a faulty device with 4k7 resistor, which is 2 > times 10k. It looks same hw error to me. I'm not so sure. From the reports from Gonsolo, in the case of this specific issue with VG0022A, the device is not unstable. It is just that it works fine with the vendor-provided firmware, while it breaks with the new one. We don't know that the same thing would happen with the original reported bug. The only way to be sure would be to obtain the same hardware from the original post and test it to check if the device has issues without replacing the resistor. Even the original reporter can't help, as his device was modified, and the issue won't be there anymore. Btw, if we end by noticing this bug happening on other it931x device models, we could simply disable firmware load for all of them, but we need more tests and reports before changing the behavior for other models, as older firmwares may have other problems. > > Making af9035 to detect vg0022a is quite simple. > > > > Considering this device's entry: > > > > { DVB_USB_DEVICE(USB_VID_DEXATEK, 0x0100, > > &it930x_props, "Logilink VG0022A", NULL) }, > > > > the check, at af9035 would be: > > > > if (le16_to_cpu(d->udev->descriptor.idVendor) == USB_VID_DEXATEK && > > le16_to_cpu(d->udev->descriptor.idProduct) == 0x0100) > > /* do something to disable firmware load */ > > > > So, no need to add any load time parameter. > > > > It should be noticed that a change just at af9035 won't work, as the > > firmware is updated by si2168 driver. So, the caller code needs to > > pass a config parameter to si2168 driver. > If it is a failing pull-up resistor on only some individual receiver > units, this seems overkill to me. In my proposal I did not realized > this change in the demod driver was needed. Agreed, but we have no means to know that until someone buys other units of the VG0022A and do tests with and without the patch. Thanks, Mauro
diff --git a/drivers/media/tuners/si2157.c b/drivers/media/tuners/si2157.c index e87040d6eca7..8f9df2485d51 100644 --- a/drivers/media/tuners/si2157.c +++ b/drivers/media/tuners/si2157.c @@ -66,6 +66,24 @@ static int si2157_cmd_execute(struct i2c_client *client, struct si2157_cmd *cmd) return ret; } +static int si2157_power_up(struct si2157_dev *dev, struct i2c_client *client) +{ + struct si2157_cmd cmd; + + if (dev->chiptype == SI2157_CHIPTYPE_SI2146) { + memcpy(cmd.args, "\xc0\x05\x01\x00\x00\x0b\x00\x00\x01", 9); + cmd.wlen = 9; + } else if (dev->chiptype == SI2157_CHIPTYPE_SI2141) { + memcpy(cmd.args, "\xc0\x00\x0d\x0e\x00\x01\x01\x01\x01\x03", 10); + cmd.wlen = 10; + } else { + memcpy(cmd.args, "\xc0\x00\x0c\x00\x00\x01\x01\x01\x01\x01\x01\x02\x00\x00\x01", 15); + cmd.wlen = 15; + } + cmd.rlen = 1; + return si2157_cmd_execute(client, &cmd); +} + static int si2157_init(struct dvb_frontend *fe) { struct i2c_client *client = fe->tuner_priv; @@ -75,7 +93,7 @@ static int si2157_init(struct dvb_frontend *fe) struct si2157_cmd cmd; const struct firmware *fw; const char *fw_name; - unsigned int uitmp, chip_id; + unsigned int uitmp; dev_dbg(&client->dev, "\n"); @@ -93,19 +111,7 @@ static int si2157_init(struct dvb_frontend *fe) if (uitmp == dev->if_frequency / 1000) goto warm; - /* power up */ - if (dev->chiptype == SI2157_CHIPTYPE_SI2146) { - memcpy(cmd.args, "\xc0\x05\x01\x00\x00\x0b\x00\x00\x01", 9); - cmd.wlen = 9; - } else if (dev->chiptype == SI2157_CHIPTYPE_SI2141) { - memcpy(cmd.args, "\xc0\x00\x0d\x0e\x00\x01\x01\x01\x01\x03", 10); - cmd.wlen = 10; - } else { - memcpy(cmd.args, "\xc0\x00\x0c\x00\x00\x01\x01\x01\x01\x01\x01\x02\x00\x00\x01", 15); - cmd.wlen = 15; - } - cmd.rlen = 1; - ret = si2157_cmd_execute(client, &cmd); + ret = si2157_power_up(dev, client); if (ret) goto err; @@ -118,17 +124,6 @@ static int si2157_init(struct dvb_frontend *fe) goto err; } - /* query chip revision */ - memcpy(cmd.args, "\x02", 1); - cmd.wlen = 1; - cmd.rlen = 13; - ret = si2157_cmd_execute(client, &cmd); - if (ret) - goto err; - - chip_id = cmd.args[1] << 24 | cmd.args[2] << 16 | cmd.args[3] << 8 | - cmd.args[4] << 0; - #define SI2177_A30 ('A' << 24 | 77 << 16 | '3' << 8 | '0' << 0) #define SI2158_A20 ('A' << 24 | 58 << 16 | '2' << 8 | '0' << 0) #define SI2148_A20 ('A' << 24 | 48 << 16 | '2' << 8 | '0' << 0) @@ -137,7 +132,7 @@ static int si2157_init(struct dvb_frontend *fe) #define SI2146_A10 ('A' << 24 | 46 << 16 | '1' << 8 | '0' << 0) #define SI2141_A10 ('A' << 24 | 41 << 16 | '1' << 8 | '0' << 0) - switch (chip_id) { + switch (dev->chip_id) { case SI2158_A20: case SI2148_A20: fw_name = SI2158_A20_FIRMWARE; @@ -456,6 +451,27 @@ static int si2157_probe(struct i2c_client *client, memcpy(&fe->ops.tuner_ops, &si2157_ops, sizeof(struct dvb_tuner_ops)); fe->tuner_priv = client; + ret = si2157_power_up(dev, client); + if (ret) + goto err; + /* query chip revision */ + /* hack: do it here because after the si2168 gets 0101, commands will + * still be executed here but no result + */ + memcpy(cmd.args, "\x02", 1); + cmd.wlen = 1; + cmd.rlen = 13; + ret = si2157_cmd_execute(client, &cmd); + if (ret) + goto err_kfree; + dev->chip_id = cmd.args[1] << 24 | + cmd.args[2] << 16 | + cmd.args[3] << 8 | + cmd.args[4] << 0; + dev_info(&client->dev, "found a 'Silicon Labs Si21%d-%c%c%c'\n", + cmd.args[2], cmd.args[1], cmd.args[3], cmd.args[4]); + + #ifdef CONFIG_MEDIA_CONTROLLER if (cfg->mdev) { dev->mdev = cfg->mdev; diff --git a/drivers/media/tuners/si2157_priv.h b/drivers/media/tuners/si2157_priv.h index 2bda903358da..9ab7c88b01b4 100644 --- a/drivers/media/tuners/si2157_priv.h +++ b/drivers/media/tuners/si2157_priv.h @@ -28,6 +28,7 @@ struct si2157_dev { u8 chiptype; u8 if_port; u32 if_frequency; + u32 chip_id; struct delayed_work stat_work; #if defined(CONFIG_MEDIA_CONTROLLER) diff --git a/drivers/media/usb/dvb-usb-v2/af9035.c b/drivers/media/usb/dvb-usb-v2/af9035.c index 3afd18733614..83e243df59b9 100644 --- a/drivers/media/usb/dvb-usb-v2/af9035.c +++ b/drivers/media/usb/dvb-usb-v2/af9035.c @@ -1246,6 +1246,51 @@ static int it930x_frontend_attach(struct dvb_usb_adapter *adap) msleep(200); + if (le16_to_cpu(d->udev->descriptor.idVendor) == USB_VID_DEXATEK) { + + ret = af9035_wr_reg_mask(d, 0xd8b7, 0x01, 0x01); + if (ret < 0) + goto err; + + /* I2C master bus 2 clock speed 300k */ + ret = af9035_wr_reg(d, 0x00f6a7, 0x07); + if (ret < 0) + goto err; + + /* I2C master bus 1,3 clock speed 300k */ + ret = af9035_wr_reg(d, 0x00f103, 0x07); + if (ret < 0) + goto err; + + /* set gpio11 low */ + ret = af9035_wr_reg_mask(d, 0xd8d4, 0x01, 0x01); + if (ret < 0) + goto err; + + ret = af9035_wr_reg_mask(d, 0xd8d5, 0x01, 0x01); + if (ret < 0) + goto err; + + ret = af9035_wr_reg_mask(d, 0xd8d3, 0x01, 0x01); + if (ret < 0) + goto err; + + /* Tuner enable using gpiot2_en, gpiot2_on and gpiot2_o (reset) */ + ret = af9035_wr_reg_mask(d, 0xd8b8, 0x01, 0x01); + if (ret < 0) + goto err; + + ret = af9035_wr_reg_mask(d, 0xd8b9, 0x01, 0x01); + if (ret < 0) + goto err; + + ret = af9035_wr_reg_mask(d, 0xd8b7, 0x00, 0x01); + if (ret < 0) + goto err; + + msleep(200); + } + ret = af9035_wr_reg_mask(d, 0xd8b7, 0x01, 0x01); if (ret < 0) goto err; @@ -2119,6 +2164,8 @@ static const struct usb_device_id af9035_id_table[] = { /* IT930x devices */ { DVB_USB_DEVICE(USB_VID_ITETECH, USB_PID_ITETECH_IT9303, &it930x_props, "ITE 9303 Generic", NULL) }, + { DVB_USB_DEVICE(USB_VID_DEXATEK, 0x0100, + &it930x_props, "Logilink VG0022A", NULL) }, { DVB_USB_DEVICE(USB_VID_AVERMEDIA, USB_PID_AVERMEDIA_TD310, &it930x_props, "AVerMedia TD310 DVB-T2", NULL) }, { }