* [PATCH RFC] media: isl6421: add checks for current overflow @ 2017-08-13 12:10 Mauro Carvalho Chehab 2017-08-13 18:47 ` Jemma Denson 2017-08-17 19:36 ` [PATCH v3] " Jemma Denson 0 siblings, 2 replies; 10+ messages in thread From: Mauro Carvalho Chehab @ 2017-08-13 12:10 UTC (permalink / raw) To: Linux Media Mailing List Cc: Mauro Carvalho Chehab, Mauro Carvalho Chehab, Patrick Boettcher This Kaffeine's BZ: https://bugs.kde.org/show_bug.cgi?id=374693 affects SkyStar S2 PCI DVB-S/S2 rev 3.3 device. It could be due to a Kernel bug. While checking the Isil 6421, comparing with its manual, available at: http://www.intersil.com/content/dam/Intersil/documents/isl6/isl6421a.pdf It was noticed that, if the output load is highly capacitive, a different approach is recomended when energizing the LNBf. Also, it is possible to detect if a current overload is happening, by checking an special flag. Add support for it. Compile-tested only. Signed-off-by: Mauro Carvalho Chehab <mchehab@s-opensource.com> --- drivers/media/dvb-frontends/isl6421.c | 72 +++++++++++++++++++++++++++++++++-- 1 file changed, 68 insertions(+), 4 deletions(-) diff --git a/drivers/media/dvb-frontends/isl6421.c b/drivers/media/dvb-frontends/isl6421.c index 838b42771a05..47a7e559cef2 100644 --- a/drivers/media/dvb-frontends/isl6421.c +++ b/drivers/media/dvb-frontends/isl6421.c @@ -38,25 +38,43 @@ struct isl6421 { u8 override_and; struct i2c_adapter *i2c; u8 i2c_addr; + bool is_off; }; static int isl6421_set_voltage(struct dvb_frontend *fe, enum fe_sec_voltage voltage) { + int ret; + u8 buf; + bool is_off; struct isl6421 *isl6421 = (struct isl6421 *) fe->sec_priv; - struct i2c_msg msg = { .addr = isl6421->i2c_addr, .flags = 0, - .buf = &isl6421->config, - .len = sizeof(isl6421->config) }; + struct i2c_msg msg[2] = { + { + .addr = isl6421->i2c_addr, + .flags = 0, + .buf = &isl6421->config, + .len = 1, + }, { + .addr = isl6421->i2c_addr, + .flags = I2C_M_RD, + .buf = &buf, + .len = 1, + } + + }; isl6421->config &= ~(ISL6421_VSEL1 | ISL6421_EN1); switch(voltage) { case SEC_VOLTAGE_OFF: + is_off = true; break; case SEC_VOLTAGE_13: + is_off = false; isl6421->config |= ISL6421_EN1; break; case SEC_VOLTAGE_18: + is_off = false; isl6421->config |= (ISL6421_EN1 | ISL6421_VSEL1); break; default: @@ -66,7 +84,51 @@ static int isl6421_set_voltage(struct dvb_frontend *fe, isl6421->config |= isl6421->override_or; isl6421->config &= isl6421->override_and; - return (i2c_transfer(isl6421->i2c, &msg, 1) == 1) ? 0 : -EIO; + /* + * If LNBf were not powered on, disable dynamic current limit, as, + * according with datasheet, highly capacitive load on the output may + * cause a difficult start-up. + */ + if (isl6421->is_off && !is_off) + isl6421->config |= ISL6421_EN1; + + ret = i2c_transfer(isl6421->i2c, msg, 2); + if (ret < 0) + return ret; + if (ret != 1) + return -EIO; + + isl6421->is_off = is_off; + + /* On overflow, the device will try again after 900 ms (typically) */ + if (isl6421->is_off && (buf & ISL6421_OLF1)) + msleep(1000); + + if (isl6421->is_off && !is_off) { + isl6421->config &= ~ISL6421_EN1; + + ret = i2c_transfer(isl6421->i2c, msg, 2); + if (ret < 0) + return ret; + if (ret != 1) + return -EIO; + } + + /* Check if overload flag is active. If so, disable power */ + if (buf & ISL6421_OLF1) { + isl6421->config &= ~(ISL6421_VSEL1 | ISL6421_EN1); + ret = i2c_transfer(isl6421->i2c, msg, 1); + if (ret < 0) + return ret; + if (ret != 1) + return -EIO; + isl6421->is_off = true; + + dev_warn(&isl6421->i2c->dev, + "Overload current detected. disabling LNBf power\n"); + return -EINVAL; + } + return 0; } static int isl6421_enable_high_lnb_voltage(struct dvb_frontend *fe, long arg) @@ -148,6 +210,8 @@ struct dvb_frontend *isl6421_attach(struct dvb_frontend *fe, struct i2c_adapter return NULL; } + isl6421->is_off = true; + /* install release callback */ fe->ops.release_sec = isl6421_release; -- 2.13.3 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH RFC] media: isl6421: add checks for current overflow 2017-08-13 12:10 [PATCH RFC] media: isl6421: add checks for current overflow Mauro Carvalho Chehab @ 2017-08-13 18:47 ` Jemma Denson 2017-08-13 19:35 ` [PATCH v2] " Mauro Carvalho Chehab 2017-08-13 19:40 ` [PATCH RFC] " Mauro Carvalho Chehab 2017-08-17 19:36 ` [PATCH v3] " Jemma Denson 1 sibling, 2 replies; 10+ messages in thread From: Jemma Denson @ 2017-08-13 18:47 UTC (permalink / raw) To: Mauro Carvalho Chehab, Linux Media Mailing List Cc: Mauro Carvalho Chehab, Patrick Boettcher Hi Mauro, Just tried this on my card - unfortunately the patch as is doesn't work. On 13/08/17 13:10, Mauro Carvalho Chehab wrote: > This Kaffeine's BZ: > https://bugs.kde.org/show_bug.cgi?id=374693 > > affects SkyStar S2 PCI DVB-S/S2 rev 3.3 device. It could be due to > a Kernel bug. > > While checking the Isil 6421, comparing with its manual, available at: > > http://www.intersil.com/content/dam/Intersil/documents/isl6/isl6421a.pdf > > It was noticed that, if the output load is highly capacitive, a different approach > is recomended when energizing the LNBf. > > Also, it is possible to detect if a current overload is happening, by checking an > special flag. > > Add support for it. > > Compile-tested only. > > Signed-off-by: Mauro Carvalho Chehab <mchehab@s-opensource.com> > --- > drivers/media/dvb-frontends/isl6421.c | 72 +++++++++++++++++++++++++++++++++-- > 1 file changed, 68 insertions(+), 4 deletions(-) > > diff --git a/drivers/media/dvb-frontends/isl6421.c b/drivers/media/dvb-frontends/isl6421.c > index 838b42771a05..47a7e559cef2 100644 > --- a/drivers/media/dvb-frontends/isl6421.c > +++ b/drivers/media/dvb-frontends/isl6421.c > @@ -38,25 +38,43 @@ struct isl6421 { > u8 override_and; > struct i2c_adapter *i2c; > u8 i2c_addr; > + bool is_off; > }; > > static int isl6421_set_voltage(struct dvb_frontend *fe, > enum fe_sec_voltage voltage) > { > + int ret; > + u8 buf; > + bool is_off; > struct isl6421 *isl6421 = (struct isl6421 *) fe->sec_priv; > - struct i2c_msg msg = { .addr = isl6421->i2c_addr, .flags = 0, > - .buf = &isl6421->config, > - .len = sizeof(isl6421->config) }; > + struct i2c_msg msg[2] = { > + { > + .addr = isl6421->i2c_addr, > + .flags = 0, > + .buf = &isl6421->config, > + .len = 1, > + }, { > + .addr = isl6421->i2c_addr, > + .flags = I2C_M_RD, > + .buf = &buf, > + .len = 1, > + } > + > + }; > > isl6421->config &= ~(ISL6421_VSEL1 | ISL6421_EN1); > > switch(voltage) { > case SEC_VOLTAGE_OFF: > + is_off = true; > break; > case SEC_VOLTAGE_13: > + is_off = false; > isl6421->config |= ISL6421_EN1; > break; > case SEC_VOLTAGE_18: > + is_off = false; > isl6421->config |= (ISL6421_EN1 | ISL6421_VSEL1); > break; > default: > @@ -66,7 +84,51 @@ static int isl6421_set_voltage(struct dvb_frontend *fe, > isl6421->config |= isl6421->override_or; > isl6421->config &= isl6421->override_and; > > - return (i2c_transfer(isl6421->i2c, &msg, 1) == 1) ? 0 : -EIO; > + /* > + * If LNBf were not powered on, disable dynamic current limit, as, > + * according with datasheet, highly capacitive load on the output may > + * cause a difficult start-up. > + */ > + if (isl6421->is_off && !is_off) > + isl6421->config |= ISL6421_EN1; > + > + ret = i2c_transfer(isl6421->i2c, msg, 2); > + if (ret < 0) > + return ret; > + if (ret != 1) > + return -EIO; This needs to check ret != 2 > + > + isl6421->is_off = is_off; > + > + /* On overflow, the device will try again after 900 ms (typically) */ > + if (isl6421->is_off && (buf & ISL6421_OLF1)) > + msleep(1000); > + > + if (isl6421->is_off && !is_off) { > + isl6421->config &= ~ISL6421_EN1; > + > + ret = i2c_transfer(isl6421->i2c, msg, 2); > + if (ret < 0) > + return ret; > + if (ret != 1) > + return -EIO; > + } Same again > + > + /* Check if overload flag is active. If so, disable power */ > + if (buf & ISL6421_OLF1) { > + isl6421->config &= ~(ISL6421_VSEL1 | ISL6421_EN1); > + ret = i2c_transfer(isl6421->i2c, msg, 1); > + if (ret < 0) > + return ret; > + if (ret != 1) > + return -EIO; > + isl6421->is_off = true; > + > + dev_warn(&isl6421->i2c->dev, > + "Overload current detected. disabling LNBf power\n"); > + return -EINVAL; > + } > + return 0; > } > > static int isl6421_enable_high_lnb_voltage(struct dvb_frontend *fe, long arg) > @@ -148,6 +210,8 @@ struct dvb_frontend *isl6421_attach(struct dvb_frontend *fe, struct i2c_adapter > return NULL; > } > > + isl6421->is_off = true; > + > /* install release callback */ > fe->ops.release_sec = isl6421_release; > Once I changed these it worked fine - I can still tune in with this patch even switching from H to V. I've no idea if this fixes the bug logged in kaffeine though as I haven't seen it. Jemma. ^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH v2] media: isl6421: add checks for current overflow 2017-08-13 18:47 ` Jemma Denson @ 2017-08-13 19:35 ` Mauro Carvalho Chehab 2017-08-15 19:51 ` Jemma Denson 2017-08-13 19:40 ` [PATCH RFC] " Mauro Carvalho Chehab 1 sibling, 1 reply; 10+ messages in thread From: Mauro Carvalho Chehab @ 2017-08-13 19:35 UTC (permalink / raw) To: Linux Media Mailing List Cc: Mauro Carvalho Chehab, Mauro Carvalho Chehab, Sakari Ailus This Kaffeine's BZ: https://bugs.kde.org/show_bug.cgi?id=374693 affects SkyStar S2 PCI DVB-S/S2 rev 3.3 device. It could be due to a Kernel bug. While checking the Isil 6421, comparing with its manual, available at: http://www.intersil.com/content/dam/Intersil/documents/isl6/isl6421a.pdf It was noticed that, if the output load is highly capacitive, a different approach is recomended when energizing the LNBf. Also, it is possible to detect if a current overload is happening, by checking an special flag. Add support for it. Compile-tested only. Signed-off-by: Mauro Carvalho Chehab <mchehab@s-opensource.com> --- drivers/media/dvb-frontends/isl6421.c | 72 +++++++++++++++++++++++++++++++++-- 1 file changed, 68 insertions(+), 4 deletions(-) diff --git a/drivers/media/dvb-frontends/isl6421.c b/drivers/media/dvb-frontends/isl6421.c index 838b42771a05..b04d56ad4ce8 100644 --- a/drivers/media/dvb-frontends/isl6421.c +++ b/drivers/media/dvb-frontends/isl6421.c @@ -38,25 +38,43 @@ struct isl6421 { u8 override_and; struct i2c_adapter *i2c; u8 i2c_addr; + bool is_off; }; static int isl6421_set_voltage(struct dvb_frontend *fe, enum fe_sec_voltage voltage) { + int ret; + u8 buf; + bool is_off; struct isl6421 *isl6421 = (struct isl6421 *) fe->sec_priv; - struct i2c_msg msg = { .addr = isl6421->i2c_addr, .flags = 0, - .buf = &isl6421->config, - .len = sizeof(isl6421->config) }; + struct i2c_msg msg[2] = { + { + .addr = isl6421->i2c_addr, + .flags = 0, + .buf = &isl6421->config, + .len = 1, + }, { + .addr = isl6421->i2c_addr, + .flags = I2C_M_RD, + .buf = &buf, + .len = 1, + } + + }; isl6421->config &= ~(ISL6421_VSEL1 | ISL6421_EN1); switch(voltage) { case SEC_VOLTAGE_OFF: + is_off = true; break; case SEC_VOLTAGE_13: + is_off = false; isl6421->config |= ISL6421_EN1; break; case SEC_VOLTAGE_18: + is_off = false; isl6421->config |= (ISL6421_EN1 | ISL6421_VSEL1); break; default: @@ -66,7 +84,51 @@ static int isl6421_set_voltage(struct dvb_frontend *fe, isl6421->config |= isl6421->override_or; isl6421->config &= isl6421->override_and; - return (i2c_transfer(isl6421->i2c, &msg, 1) == 1) ? 0 : -EIO; + /* + * If LNBf were not powered on, disable dynamic current limit, as, + * according with datasheet, highly capacitive load on the output may + * cause a difficult start-up. + */ + if (isl6421->is_off && !is_off) + isl6421->config |= ISL6421_EN1; + + ret = i2c_transfer(isl6421->i2c, msg, 2); + if (ret < 0) + return ret; + if (ret != 2) + return -EIO; + + isl6421->is_off = is_off; + + /* On overflow, the device will try again after 900 ms (typically) */ + if (isl6421->is_off && (buf & ISL6421_OLF1)) + msleep(1000); + + if (isl6421->is_off && !is_off) { + isl6421->config &= ~ISL6421_EN1; + + ret = i2c_transfer(isl6421->i2c, msg, 2); + if (ret < 0) + return ret; + if (ret != 2) + return -EIO; + } + + /* Check if overload flag is active. If so, disable power */ + if (buf & ISL6421_OLF1) { + isl6421->config &= ~(ISL6421_VSEL1 | ISL6421_EN1); + ret = i2c_transfer(isl6421->i2c, msg, 1); + if (ret < 0) + return ret; + if (ret != 1) + return -EIO; + isl6421->is_off = true; + + dev_warn(&isl6421->i2c->dev, + "Overload current detected. disabling LNBf power\n"); + return -EINVAL; + } + return 0; } static int isl6421_enable_high_lnb_voltage(struct dvb_frontend *fe, long arg) @@ -148,6 +210,8 @@ struct dvb_frontend *isl6421_attach(struct dvb_frontend *fe, struct i2c_adapter return NULL; } + isl6421->is_off = true; + /* install release callback */ fe->ops.release_sec = isl6421_release; -- 2.13.3 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH v2] media: isl6421: add checks for current overflow 2017-08-13 19:35 ` [PATCH v2] " Mauro Carvalho Chehab @ 2017-08-15 19:51 ` Jemma Denson 2017-08-16 9:42 ` Mauro Carvalho Chehab 0 siblings, 1 reply; 10+ messages in thread From: Jemma Denson @ 2017-08-15 19:51 UTC (permalink / raw) To: Mauro Carvalho Chehab, Linux Media Mailing List Cc: Mauro Carvalho Chehab, Sakari Ailus Hi Mauro, On 13/08/17 20:35, Mauro Carvalho Chehab wrote: > This Kaffeine's BZ: > https://bugs.kde.org/show_bug.cgi?id=374693 > > affects SkyStar S2 PCI DVB-S/S2 rev 3.3 device. It could be due to > a Kernel bug. > > While checking the Isil 6421, comparing with its manual, available at: > > http://www.intersil.com/content/dam/Intersil/documents/isl6/isl6421a.pdf > > It was noticed that, if the output load is highly capacitive, a different approach > is recomended when energizing the LNBf. > > Also, it is possible to detect if a current overload is happening, by checking an > special flag. > > Add support for it. > > Compile-tested only. > > Signed-off-by: Mauro Carvalho Chehab <mchehab@s-opensource.com> > --- > drivers/media/dvb-frontends/isl6421.c | 72 +++++++++++++++++++++++++++++++++-- > 1 file changed, 68 insertions(+), 4 deletions(-) > > diff --git a/drivers/media/dvb-frontends/isl6421.c b/drivers/media/dvb-frontends/isl6421.c > index 838b42771a05..b04d56ad4ce8 100644 > --- a/drivers/media/dvb-frontends/isl6421.c > +++ b/drivers/media/dvb-frontends/isl6421.c > @@ -38,25 +38,43 @@ struct isl6421 { > u8 override_and; > struct i2c_adapter *i2c; > u8 i2c_addr; > + bool is_off; > }; > > static int isl6421_set_voltage(struct dvb_frontend *fe, > enum fe_sec_voltage voltage) > { > + int ret; > + u8 buf; > + bool is_off; > struct isl6421 *isl6421 = (struct isl6421 *) fe->sec_priv; > - struct i2c_msg msg = { .addr = isl6421->i2c_addr, .flags = 0, > - .buf = &isl6421->config, > - .len = sizeof(isl6421->config) }; > + struct i2c_msg msg[2] = { > + { > + .addr = isl6421->i2c_addr, > + .flags = 0, > + .buf = &isl6421->config, > + .len = 1, > + }, { > + .addr = isl6421->i2c_addr, > + .flags = I2C_M_RD, > + .buf = &buf, > + .len = 1, > + } > + > + }; > > isl6421->config &= ~(ISL6421_VSEL1 | ISL6421_EN1); > > switch(voltage) { > case SEC_VOLTAGE_OFF: > + is_off = true; > break; > case SEC_VOLTAGE_13: > + is_off = false; > isl6421->config |= ISL6421_EN1; > break; > case SEC_VOLTAGE_18: > + is_off = false; > isl6421->config |= (ISL6421_EN1 | ISL6421_VSEL1); > break; > default: > @@ -66,7 +84,51 @@ static int isl6421_set_voltage(struct dvb_frontend *fe, > isl6421->config |= isl6421->override_or; > isl6421->config &= isl6421->override_and; > > - return (i2c_transfer(isl6421->i2c, &msg, 1) == 1) ? 0 : -EIO; > + /* > + * If LNBf were not powered on, disable dynamic current limit, as, > + * according with datasheet, highly capacitive load on the output may > + * cause a difficult start-up. > + */ > + if (isl6421->is_off && !is_off) > + isl6421->config |= ISL6421_EN1; Checking the datasheet I think we need to be setting DCL high instead. EN1 is already set anyway. > + > + ret = i2c_transfer(isl6421->i2c, msg, 2); > + if (ret < 0) > + return ret; > + if (ret != 2) > + return -EIO; > + > + isl6421->is_off = is_off; Is this in the right place? > + > + /* On overflow, the device will try again after 900 ms (typically) */ > + if (isl6421->is_off && (buf & ISL6421_OLF1)) > + msleep(1000); 1000ms does only cover one cycle of OFF then ON - the device will keep cycling 900ms off then 20ms on until overflow is cleared so it might take longer but adding the code to support longer is probably not worth it. Waiting one cycle is better than the current none anyway. > + > + if (isl6421->is_off && !is_off) { > + isl6421->config &= ~ISL6421_EN1; > + > + ret = i2c_transfer(isl6421->i2c, msg, 2); > + if (ret < 0) > + return ret; > + if (ret != 2) > + return -EIO; > + } Does this if statement ever match? isl6421->is_off and is_off are the same value at this point. I presume this is supposed to be re-enabling DCL so would again also need that bit instead of EN1. We might also need a little delay before turning it on - the datasheet mentions a "chosen amount of time". I've no idea what's appropriate here, 200ms? A simpler if statement might be just (isl6421->config & ISL6421_DCL), we just need to check DCL was set high above. > + > + /* Check if overload flag is active. If so, disable power */ > + if (buf & ISL6421_OLF1) { > + isl6421->config &= ~(ISL6421_VSEL1 | ISL6421_EN1); > + ret = i2c_transfer(isl6421->i2c, msg, 1); > + if (ret < 0) > + return ret; > + if (ret != 1) > + return -EIO; > + isl6421->is_off = true; > + > + dev_warn(&isl6421->i2c->dev, > + "Overload current detected. disabling LNBf power\n"); > + return -EINVAL; > + } > + return 0; > } > > static int isl6421_enable_high_lnb_voltage(struct dvb_frontend *fe, long arg) > @@ -148,6 +210,8 @@ struct dvb_frontend *isl6421_attach(struct dvb_frontend *fe, struct i2c_adapter > return NULL; > } > > + isl6421->is_off = true; > + > /* install release callback */ > fe->ops.release_sec = isl6421_release; > I've just tested both your v2 patch and changes I'm suggesting above; both work fine on my setup. Do you want me to send a v3? Jemma. ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2] media: isl6421: add checks for current overflow 2017-08-15 19:51 ` Jemma Denson @ 2017-08-16 9:42 ` Mauro Carvalho Chehab 2017-08-17 19:50 ` Jemma Denson 0 siblings, 1 reply; 10+ messages in thread From: Mauro Carvalho Chehab @ 2017-08-16 9:42 UTC (permalink / raw) To: Jemma Denson Cc: Linux Media Mailing List, Mauro Carvalho Chehab, Sakari Ailus Em Tue, 15 Aug 2017 20:51:09 +0100 Jemma Denson <jdenson@gmail.com> escreveu: > Hi Mauro, > > On 13/08/17 20:35, Mauro Carvalho Chehab wrote: > > > This Kaffeine's BZ: > > https://bugs.kde.org/show_bug.cgi?id=374693 > > > > affects SkyStar S2 PCI DVB-S/S2 rev 3.3 device. It could be due to > > a Kernel bug. > > > > While checking the Isil 6421, comparing with its manual, available at: > > > > http://www.intersil.com/content/dam/Intersil/documents/isl6/isl6421a.pdf > > > > It was noticed that, if the output load is highly capacitive, a different approach > > is recomended when energizing the LNBf. > > > > Also, it is possible to detect if a current overload is happening, by checking an > > special flag. > > > > Add support for it. > > > > Compile-tested only. > > > > Signed-off-by: Mauro Carvalho Chehab <mchehab@s-opensource.com> > > --- > > drivers/media/dvb-frontends/isl6421.c | 72 +++++++++++++++++++++++++++++++++-- > > 1 file changed, 68 insertions(+), 4 deletions(-) > > > > diff --git a/drivers/media/dvb-frontends/isl6421.c b/drivers/media/dvb-frontends/isl6421.c > > index 838b42771a05..b04d56ad4ce8 100644 > > --- a/drivers/media/dvb-frontends/isl6421.c > > +++ b/drivers/media/dvb-frontends/isl6421.c > > @@ -38,25 +38,43 @@ struct isl6421 { > > u8 override_and; > > struct i2c_adapter *i2c; > > u8 i2c_addr; > > + bool is_off; > > }; > > > > static int isl6421_set_voltage(struct dvb_frontend *fe, > > enum fe_sec_voltage voltage) > > { > > + int ret; > > + u8 buf; > > + bool is_off; > > struct isl6421 *isl6421 = (struct isl6421 *) fe->sec_priv; > > - struct i2c_msg msg = { .addr = isl6421->i2c_addr, .flags = 0, > > - .buf = &isl6421->config, > > - .len = sizeof(isl6421->config) }; > > + struct i2c_msg msg[2] = { > > + { > > + .addr = isl6421->i2c_addr, > > + .flags = 0, > > + .buf = &isl6421->config, > > + .len = 1, > > + }, { > > + .addr = isl6421->i2c_addr, > > + .flags = I2C_M_RD, > > + .buf = &buf, > > + .len = 1, > > + } > > + > > + }; > > > > isl6421->config &= ~(ISL6421_VSEL1 | ISL6421_EN1); > > > > switch(voltage) { > > case SEC_VOLTAGE_OFF: > > + is_off = true; > > break; > > case SEC_VOLTAGE_13: > > + is_off = false; > > isl6421->config |= ISL6421_EN1; > > break; > > case SEC_VOLTAGE_18: > > + is_off = false; > > isl6421->config |= (ISL6421_EN1 | ISL6421_VSEL1); > > break; > > default: > > @@ -66,7 +84,51 @@ static int isl6421_set_voltage(struct dvb_frontend *fe, > > isl6421->config |= isl6421->override_or; > > isl6421->config &= isl6421->override_and; > > > > - return (i2c_transfer(isl6421->i2c, &msg, 1) == 1) ? 0 : -EIO; > > + /* > > + * If LNBf were not powered on, disable dynamic current limit, as, > > + * according with datasheet, highly capacitive load on the output may > > + * cause a difficult start-up. > > + */ > > + if (isl6421->is_off && !is_off) > > + isl6421->config |= ISL6421_EN1; > > Checking the datasheet I think we need to be setting DCL high instead. EN1 is > already set anyway. Yes, that was the idea. Not sure what happened here :-) > > + > > + ret = i2c_transfer(isl6421->i2c, msg, 2); > > + if (ret < 0) > > + return ret; > > + if (ret != 2) > > + return -EIO; > > + > > + isl6421->is_off = is_off; > > Is this in the right place? On my first internal versions, this used to be below, but I guess the best is to place it here, because, if the next i2c_transfer fail, this would still reflect the current state. > > > + > > + /* On overflow, the device will try again after 900 ms (typically) */ > > + if (isl6421->is_off && (buf & ISL6421_OLF1)) > > + msleep(1000); > > 1000ms does only cover one cycle of OFF then ON - the device will keep cycling > 900ms off then 20ms on until overflow is cleared so it might take longer but > adding the code to support longer is probably not worth it. Waiting one cycle > is better than the current none anyway. Yes, I know it could wait for more time here, but not sure if it would be worth doing it. The problem is that 1000ms is already a lot of time, and if this gets too long, userspace may any giving up of tuning, anyway. > > + > > + if (isl6421->is_off && !is_off) { > > + isl6421->config &= ~ISL6421_EN1; Again, this should be DCL. > > + > > + ret = i2c_transfer(isl6421->i2c, msg, 2); > > + if (ret < 0) > > + return ret; > > + if (ret != 2) > > + return -EIO; > > + } > > Does this if statement ever match? isl6421->is_off and is_off are the same value > at this point. I presume this is supposed to be re-enabling DCL so would again > also need that bit instead of EN1. We might also need a little delay before > turning it on - the datasheet mentions a "chosen amount of time". I've no idea > what's appropriate here, 200ms? > > A simpler if statement might be just (isl6421->config & ISL6421_DCL), we just > need to check DCL was set high above. It used to, on my original version, where I was setting config->is_off only at the end :-) Yeah, checking for DCL is a way more reliable. > > > + > > + /* Check if overload flag is active. If so, disable power */ > > + if (buf & ISL6421_OLF1) { > > + isl6421->config &= ~(ISL6421_VSEL1 | ISL6421_EN1); > > + ret = i2c_transfer(isl6421->i2c, msg, 1); > > + if (ret < 0) > > + return ret; > > + if (ret != 1) > > + return -EIO; > > + isl6421->is_off = true; > > + > > + dev_warn(&isl6421->i2c->dev, > > + "Overload current detected. disabling LNBf power\n"); > > + return -EINVAL; > > + } > > + return 0; > > } > > > > static int isl6421_enable_high_lnb_voltage(struct dvb_frontend *fe, long arg) > > @@ -148,6 +210,8 @@ struct dvb_frontend *isl6421_attach(struct dvb_frontend *fe, struct i2c_adapter > > return NULL; > > } > > > > + isl6421->is_off = true; > > + > > /* install release callback */ > > fe->ops.release_sec = isl6421_release; > > > > I've just tested both your v2 patch and changes I'm suggesting above; both work > fine on my setup. Do you want me to send a v3? Yeah, sure! I'm currently in travel, returning only on Friday, and I don't have the hardware to test. So, if you can send it, I'd appreciate :-) Cheers, Mauro ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2] media: isl6421: add checks for current overflow 2017-08-16 9:42 ` Mauro Carvalho Chehab @ 2017-08-17 19:50 ` Jemma Denson 2017-08-19 11:44 ` Jemma Denson 0 siblings, 1 reply; 10+ messages in thread From: Jemma Denson @ 2017-08-17 19:50 UTC (permalink / raw) To: Mauro Carvalho Chehab Cc: Linux Media Mailing List, Mauro Carvalho Chehab, Sakari Ailus On 16/08/17 10:42, Mauro Carvalho Chehab wrote: >> I've just tested both your v2 patch and changes I'm suggesting above; both work >> fine on my setup. Do you want me to send a v3? > Yeah, sure! I'm currently in travel, returning only on Friday, and I don't > have the hardware to test. So, if you can send it, I'd appreciate :-) > > Cheers, > Mauro Ok, just sent. The if statements ended up being a bit complicated, but I added checking if the DCL bit was being overridden (it is by several cards under cx88), only pausing for a second if DCL was in use as the datasheet suggested that's only done in that mode, and also skipped checking overflow if the device was set to off. The latter should cover overflow somehow being picked up during attach and causing the attach to fail. Unlikely to happen but we shouldn't fail on what could be a transient issue. Jemma. ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2] media: isl6421: add checks for current overflow 2017-08-17 19:50 ` Jemma Denson @ 2017-08-19 11:44 ` Jemma Denson 0 siblings, 0 replies; 10+ messages in thread From: Jemma Denson @ 2017-08-19 11:44 UTC (permalink / raw) To: Mauro Carvalho Chehab Cc: Linux Media Mailing List, Mauro Carvalho Chehab, Sakari Ailus On 17/08/17 20:50, Jemma Denson wrote: > On 16/08/17 10:42, Mauro Carvalho Chehab wrote: >>> I've just tested both your v2 patch and changes I'm suggesting above; both work >>> fine on my setup. Do you want me to send a v3? >> Yeah, sure! I'm currently in travel, returning only on Friday, and I don't >> have the hardware to test. So, if you can send it, I'd appreciate :-) >> Cheers, >> Mauro > Ok, just sent. The if statements ended up being a bit complicated, but I added checking > if the DCL bit was being overridden (it is by several cards under cx88), only pausing > for a second if DCL was in use as the datasheet suggested that's only done in that mode, > and also skipped checking overflow if the device was set to off. > The latter should cover overflow somehow being picked up during attach and causing the > attach to fail. Unlikely to happen but we shouldn't fail on what could be a transient > issue. Sorry, time for a v4! I wasn't happy with adding a permanent 200ms pause so I've reverted that back to how you had it before. The pause before re-enabling dcl is only really needed if it needs time to settle down and so reverting back to always doing a 1000ms on overload should cover it. Jemma. ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH RFC] media: isl6421: add checks for current overflow 2017-08-13 18:47 ` Jemma Denson 2017-08-13 19:35 ` [PATCH v2] " Mauro Carvalho Chehab @ 2017-08-13 19:40 ` Mauro Carvalho Chehab 1 sibling, 0 replies; 10+ messages in thread From: Mauro Carvalho Chehab @ 2017-08-13 19:40 UTC (permalink / raw) To: Jemma Denson Cc: Linux Media Mailing List, Mauro Carvalho Chehab, Patrick Boettcher Em Sun, 13 Aug 2017 19:47:37 +0100 Jemma Denson <jdenson@gmail.com> escreveu: > Hi Mauro, > > Just tried this on my card - unfortunately the patch as is doesn't work. > > On 13/08/17 13:10, Mauro Carvalho Chehab wrote: > > This Kaffeine's BZ: > > https://bugs.kde.org/show_bug.cgi?id=374693 > > > > affects SkyStar S2 PCI DVB-S/S2 rev 3.3 device. It could be due to > > a Kernel bug. > > > > While checking the Isil 6421, comparing with its manual, available at: > > > > http://www.intersil.com/content/dam/Intersil/documents/isl6/isl6421a.pdf > > > > It was noticed that, if the output load is highly capacitive, a different approach > > is recomended when energizing the LNBf. > > > > Also, it is possible to detect if a current overload is happening, by checking an > > special flag. > > > > Add support for it. > > > > Compile-tested only. > > > > Signed-off-by: Mauro Carvalho Chehab <mchehab@s-opensource.com> > > --- > > drivers/media/dvb-frontends/isl6421.c | 72 +++++++++++++++++++++++++++++++++-- > > 1 file changed, 68 insertions(+), 4 deletions(-) > > > > diff --git a/drivers/media/dvb-frontends/isl6421.c b/drivers/media/dvb-frontends/isl6421.c > > index 838b42771a05..47a7e559cef2 100644 > > --- a/drivers/media/dvb-frontends/isl6421.c > > +++ b/drivers/media/dvb-frontends/isl6421.c > > @@ -38,25 +38,43 @@ struct isl6421 { > > u8 override_and; > > struct i2c_adapter *i2c; > > u8 i2c_addr; > > + bool is_off; > > }; > > > > static int isl6421_set_voltage(struct dvb_frontend *fe, > > enum fe_sec_voltage voltage) > > { > > + int ret; > > + u8 buf; > > + bool is_off; > > struct isl6421 *isl6421 = (struct isl6421 *) fe->sec_priv; > > - struct i2c_msg msg = { .addr = isl6421->i2c_addr, .flags = 0, > > - .buf = &isl6421->config, > > - .len = sizeof(isl6421->config) }; > > + struct i2c_msg msg[2] = { > > + { > > + .addr = isl6421->i2c_addr, > > + .flags = 0, > > + .buf = &isl6421->config, > > + .len = 1, > > + }, { > > + .addr = isl6421->i2c_addr, > > + .flags = I2C_M_RD, > > + .buf = &buf, > > + .len = 1, > > + } > > + > > + }; > > > > isl6421->config &= ~(ISL6421_VSEL1 | ISL6421_EN1); > > > > switch(voltage) { > > case SEC_VOLTAGE_OFF: > > + is_off = true; > > break; > > case SEC_VOLTAGE_13: > > + is_off = false; > > isl6421->config |= ISL6421_EN1; > > break; > > case SEC_VOLTAGE_18: > > + is_off = false; > > isl6421->config |= (ISL6421_EN1 | ISL6421_VSEL1); > > break; > > default: > > @@ -66,7 +84,51 @@ static int isl6421_set_voltage(struct dvb_frontend *fe, > > isl6421->config |= isl6421->override_or; > > isl6421->config &= isl6421->override_and; > > > > - return (i2c_transfer(isl6421->i2c, &msg, 1) == 1) ? 0 : -EIO; > > + /* > > + * If LNBf were not powered on, disable dynamic current limit, as, > > + * according with datasheet, highly capacitive load on the output may > > + * cause a difficult start-up. > > + */ > > + if (isl6421->is_off && !is_off) > > + isl6421->config |= ISL6421_EN1; > > + > > + ret = i2c_transfer(isl6421->i2c, msg, 2); > > + if (ret < 0) > > + return ret; > > + if (ret != 1) > > + return -EIO; > > This needs to check ret != 2 > > > + > > + isl6421->is_off = is_off; > > + > > + /* On overflow, the device will try again after 900 ms (typically) */ > > + if (isl6421->is_off && (buf & ISL6421_OLF1)) > > + msleep(1000); > > + > > + if (isl6421->is_off && !is_off) { > > + isl6421->config &= ~ISL6421_EN1; > > + > > + ret = i2c_transfer(isl6421->i2c, msg, 2); > > + if (ret < 0) > > + return ret; > > + if (ret != 1) > > + return -EIO; > > + } > > Same again > > > + > > + /* Check if overload flag is active. If so, disable power */ > > + if (buf & ISL6421_OLF1) { > > + isl6421->config &= ~(ISL6421_VSEL1 | ISL6421_EN1); > > + ret = i2c_transfer(isl6421->i2c, msg, 1); > > + if (ret < 0) > > + return ret; > > + if (ret != 1) > > + return -EIO; > > + isl6421->is_off = true; > > + > > + dev_warn(&isl6421->i2c->dev, > > + "Overload current detected. disabling LNBf power\n"); > > + return -EINVAL; > > + } > > + return 0; > > } > > > > static int isl6421_enable_high_lnb_voltage(struct dvb_frontend *fe, long arg) > > @@ -148,6 +210,8 @@ struct dvb_frontend *isl6421_attach(struct dvb_frontend *fe, struct i2c_adapter > > return NULL; > > } > > > > + isl6421->is_off = true; > > + > > /* install release callback */ > > fe->ops.release_sec = isl6421_release; > > > > > Once I changed these it worked fine - I can still tune in with this > patch even switching from H to V. I've no idea if this fixes the bug > logged in kaffeine though as I haven't seen it. Hi Jemma, Thanks for testing! Just sent a v2 with the fixes you pointed. Please reply it with a tested-by. IMHO, detecting current overflow is a good improvement for the driver. I suspect that, in the case of Kaffeine's bug, the issue could also be due to a very long cabling. So, I'm preparing another patch that will let the user to optionally ask for a higher voltage (Isil 6421 allows selecting 14V/19V instead of the standard 13V/18V). Thanks, Mauro ^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH v3] media: isl6421: add checks for current overflow 2017-08-13 12:10 [PATCH RFC] media: isl6421: add checks for current overflow Mauro Carvalho Chehab 2017-08-13 18:47 ` Jemma Denson @ 2017-08-17 19:36 ` Jemma Denson 2017-08-19 11:38 ` [PATCH v4] " Jemma Denson 1 sibling, 1 reply; 10+ messages in thread From: Jemma Denson @ 2017-08-17 19:36 UTC (permalink / raw) To: Linux Media Mailing List Cc: Mauro Carvalho Chehab, Patrick Boettcher, Mauro Carvalho Chehab This Kaffeine's BZ: https://bugs.kde.org/show_bug.cgi?id=374693 affects SkyStar S2 PCI DVB-S/S2 rev 3.3 device. It could be due to a Kernel bug. While checking the Isil 6421, comparing with its manual, available at: http://www.intersil.com/content/dam/Intersil/documents/isl6/isl6421a.pdf It was noticed that, if the output load is highly capacitive, a different approach is recomended when energizing the LNBf. Also, it is possible to detect if a current overload is happening, by checking an special flag. Add support for it. Tested on Skystar S2. Changes respect override_or option so should still work fine on cx88 based cards which disable dynamic current limit. Signed-off-by: Mauro Carvalho Chehab <mchehab@s-opensource.com> Signed-off-by: Jemma Denson <jdenson@gmail.com> --- drivers/media/dvb-frontends/isl6421.c | 77 +++++++++++++++++++++++++++++++++-- 1 file changed, 73 insertions(+), 4 deletions(-) diff --git a/drivers/media/dvb-frontends/isl6421.c b/drivers/media/dvb-frontends/isl6421.c index 838b42771a05..f58590fe71f5 100644 --- a/drivers/media/dvb-frontends/isl6421.c +++ b/drivers/media/dvb-frontends/isl6421.c @@ -38,35 +38,102 @@ struct isl6421 { u8 override_and; struct i2c_adapter *i2c; u8 i2c_addr; + bool is_off; }; static int isl6421_set_voltage(struct dvb_frontend *fe, enum fe_sec_voltage voltage) { + int ret; + u8 buf; + bool is_off; struct isl6421 *isl6421 = (struct isl6421 *) fe->sec_priv; - struct i2c_msg msg = { .addr = isl6421->i2c_addr, .flags = 0, - .buf = &isl6421->config, - .len = sizeof(isl6421->config) }; + struct i2c_msg msg[2] = { + { + .addr = isl6421->i2c_addr, + .flags = 0, + .buf = &isl6421->config, + .len = 1, + }, { + .addr = isl6421->i2c_addr, + .flags = I2C_M_RD, + .buf = &buf, + .len = 1, + } + + }; isl6421->config &= ~(ISL6421_VSEL1 | ISL6421_EN1); switch(voltage) { case SEC_VOLTAGE_OFF: + is_off = true; break; case SEC_VOLTAGE_13: + is_off = false; isl6421->config |= ISL6421_EN1; break; case SEC_VOLTAGE_18: + is_off = false; isl6421->config |= (ISL6421_EN1 | ISL6421_VSEL1); break; default: return -EINVAL; } + /* + * If LNBf were not powered on, disable dynamic current limit, as, + * according with datasheet, highly capacitive load on the output may + * cause a difficult start-up. + */ + if (isl6421->is_off && !is_off) + isl6421->config |= ISL6421_DCL; + isl6421->config |= isl6421->override_or; isl6421->config &= isl6421->override_and; - return (i2c_transfer(isl6421->i2c, &msg, 1) == 1) ? 0 : -EIO; + ret = i2c_transfer(isl6421->i2c, msg, 2); + if (ret < 0) + return ret; + if (ret != 2) + return -EIO; + + /* Store off status now incase future commands fail */ + isl6421->is_off = is_off; + + /* On overflow, the device will try again after 900 ms (typically) */ + if (!(isl6421->config & ISL6421_DCL) && (buf & ISL6421_OLF1)) + msleep(1000); + + /* Re-enable dynamic current limit after a certain amount of time */ + if ((isl6421->config & ISL6421_DCL) && + !(isl6421->override_or & ISL6421_DCL)) { + msleep(200); + isl6421->config &= ~ISL6421_DCL; + + ret = i2c_transfer(isl6421->i2c, msg, 2); + if (ret < 0) + return ret; + if (ret != 2) + return -EIO; + } + + /* Check if overload flag is active. If so, disable power */ + if (!is_off && (buf & ISL6421_OLF1)) { + isl6421->config &= ~(ISL6421_VSEL1 | ISL6421_EN1); + ret = i2c_transfer(isl6421->i2c, msg, 1); + if (ret < 0) + return ret; + if (ret != 1) + return -EIO; + isl6421->is_off = true; + + dev_warn(&isl6421->i2c->dev, + "Overload current detected. disabling LNBf power\n"); + return -EINVAL; + } + + return 0; } static int isl6421_enable_high_lnb_voltage(struct dvb_frontend *fe, long arg) @@ -148,6 +215,8 @@ struct dvb_frontend *isl6421_attach(struct dvb_frontend *fe, struct i2c_adapter return NULL; } + isl6421->is_off = true; + /* install release callback */ fe->ops.release_sec = isl6421_release; -- 2.13.4 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH v4] media: isl6421: add checks for current overflow 2017-08-17 19:36 ` [PATCH v3] " Jemma Denson @ 2017-08-19 11:38 ` Jemma Denson 0 siblings, 0 replies; 10+ messages in thread From: Jemma Denson @ 2017-08-19 11:38 UTC (permalink / raw) To: Linux Media Mailing List Cc: Mauro Carvalho Chehab, Patrick Boettcher, Sakari Ailus, Mauro Carvalho Chehab This Kaffeine's BZ: https://bugs.kde.org/show_bug.cgi?id=374693 affects SkyStar S2 PCI DVB-S/S2 rev 3.3 device. It could be due to a Kernel bug. While checking the Isil 6421, comparing with its manual, available at: http://www.intersil.com/content/dam/Intersil/documents/isl6/isl6421a.pdf It was noticed that, if the output load is highly capacitive, a different approach is recomended when energizing the LNBf. Also, it is possible to detect if a current overload is happening, by checking an special flag. Add support for it. Tested on Skystar S2. Changes respect override_or option so should still work fine on cx88 based cards which disable dynamic current limit. Changes since v1: v2 - fixed incorrect checking of i2c return values v3 - fix if logic to check if dcl needs re-enabling - respect override_or values which aim to disable dcl - only do long sleep on overload if dcl enabled - add short sleep before re-enabling dcl - only check overload and potentially return EINVAL if device is on v4 - revert v3 sleep logic changes to remove tuning delays Signed-off-by: Mauro Carvalho Chehab <mchehab@s-opensource.com> Signed-off-by: Jemma Denson <jdenson@gmail.com> --- drivers/media/dvb-frontends/isl6421.c | 76 +++++++++++++++++++++++++++++++++-- 1 file changed, 72 insertions(+), 4 deletions(-) diff --git a/drivers/media/dvb-frontends/isl6421.c b/drivers/media/dvb-frontends/isl6421.c index 838b42771a05..3f3487887672 100644 --- a/drivers/media/dvb-frontends/isl6421.c +++ b/drivers/media/dvb-frontends/isl6421.c @@ -38,35 +38,101 @@ struct isl6421 { u8 override_and; struct i2c_adapter *i2c; u8 i2c_addr; + bool is_off; }; static int isl6421_set_voltage(struct dvb_frontend *fe, enum fe_sec_voltage voltage) { + int ret; + u8 buf; + bool is_off; struct isl6421 *isl6421 = (struct isl6421 *) fe->sec_priv; - struct i2c_msg msg = { .addr = isl6421->i2c_addr, .flags = 0, - .buf = &isl6421->config, - .len = sizeof(isl6421->config) }; + struct i2c_msg msg[2] = { + { + .addr = isl6421->i2c_addr, + .flags = 0, + .buf = &isl6421->config, + .len = 1, + }, { + .addr = isl6421->i2c_addr, + .flags = I2C_M_RD, + .buf = &buf, + .len = 1, + } + + }; isl6421->config &= ~(ISL6421_VSEL1 | ISL6421_EN1); switch(voltage) { case SEC_VOLTAGE_OFF: + is_off = true; break; case SEC_VOLTAGE_13: + is_off = false; isl6421->config |= ISL6421_EN1; break; case SEC_VOLTAGE_18: + is_off = false; isl6421->config |= (ISL6421_EN1 | ISL6421_VSEL1); break; default: return -EINVAL; } + /* + * If LNBf were not powered on, disable dynamic current limit, as, + * according with datasheet, highly capacitive load on the output may + * cause a difficult start-up. + */ + if (isl6421->is_off && !is_off) + isl6421->config |= ISL6421_DCL; + isl6421->config |= isl6421->override_or; isl6421->config &= isl6421->override_and; - return (i2c_transfer(isl6421->i2c, &msg, 1) == 1) ? 0 : -EIO; + ret = i2c_transfer(isl6421->i2c, msg, 2); + if (ret < 0) + return ret; + if (ret != 2) + return -EIO; + + /* Store off status now incase future commands fail */ + isl6421->is_off = is_off; + + /* On overflow, the device will try again after 900 ms (typically) */ + if (!is_off && (buf & ISL6421_OLF1)) + msleep(1000); + + /* Re-enable dynamic current limit */ + if ((isl6421->config & ISL6421_DCL) && + !(isl6421->override_or & ISL6421_DCL)) { + isl6421->config &= ~ISL6421_DCL; + + ret = i2c_transfer(isl6421->i2c, msg, 2); + if (ret < 0) + return ret; + if (ret != 2) + return -EIO; + } + + /* Check if overload flag is active. If so, disable power */ + if (!is_off && (buf & ISL6421_OLF1)) { + isl6421->config &= ~(ISL6421_VSEL1 | ISL6421_EN1); + ret = i2c_transfer(isl6421->i2c, msg, 1); + if (ret < 0) + return ret; + if (ret != 1) + return -EIO; + isl6421->is_off = true; + + dev_warn(&isl6421->i2c->dev, + "Overload current detected. disabling LNBf power\n"); + return -EINVAL; + } + + return 0; } static int isl6421_enable_high_lnb_voltage(struct dvb_frontend *fe, long arg) @@ -148,6 +214,8 @@ struct dvb_frontend *isl6421_attach(struct dvb_frontend *fe, struct i2c_adapter return NULL; } + isl6421->is_off = true; + /* install release callback */ fe->ops.release_sec = isl6421_release; -- 2.13.4 ^ permalink raw reply related [flat|nested] 10+ messages in thread
end of thread, other threads:[~2017-08-19 11:44 UTC | newest] Thread overview: 10+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2017-08-13 12:10 [PATCH RFC] media: isl6421: add checks for current overflow Mauro Carvalho Chehab 2017-08-13 18:47 ` Jemma Denson 2017-08-13 19:35 ` [PATCH v2] " Mauro Carvalho Chehab 2017-08-15 19:51 ` Jemma Denson 2017-08-16 9:42 ` Mauro Carvalho Chehab 2017-08-17 19:50 ` Jemma Denson 2017-08-19 11:44 ` Jemma Denson 2017-08-13 19:40 ` [PATCH RFC] " Mauro Carvalho Chehab 2017-08-17 19:36 ` [PATCH v3] " Jemma Denson 2017-08-19 11:38 ` [PATCH v4] " Jemma Denson
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.