media: isl6421: add checks for current overflow
RFC PATCH 1/1 media: isl6421: add checks for current overflow
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.
--- drivers/media/dvb-frontends/isl6421.c | 72 +++++++++++++++++++++++++++++++++-- 1 file changed, 68 insertions(+), 4 deletions(-)
drivers/media/dvb-frontends/isl6421.c +68 -4
@@ -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_adapterreturn NULL ; } + isl6421->is_off = true; +/* install release callback */ fe->ops.release_sec = isl6421_release;
Signature · 1 line
2.13.3
Hi Mauro,
Just tried this on my card - unfortunately the patch as is doesn't work.
Quoted 84 lines · Mauro Carvalho Chehab
This Kaffeine's BZ:
https://bugs.kde.org/show_bug.cgi?id=374693affects 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 <[email protected]>--- drivers/media/dvb-frontends/isl6421.c | 72 +++++++++++++++++++++++++++++++++-- 1 file changed, 68 insertions(+), 4 deletions(-)drivers/media/dvb-frontends/isl6421.c +24
-4@@ -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
drivers/media/dvb-frontends/isl6421.c static int isl6421_set_voltage(struct dvb_frontend *fe, · line 99
Quoted 10 lines
drivers/media/dvb-frontends/isl6421.c +10
-0+* 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
Quoted 6 lines
drivers/media/dvb-frontends/isl6421.c +6
-0+ + 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 );
drivers/media/dvb-frontends/isl6421.c static int isl6421_set_voltage(struct dvb_frontend *fe, · line 115
Quoted 10 lines
drivers/media/dvb-frontends/isl6421.c +10
-0+ +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
Quoted 20 lines
drivers/media/dvb-frontends/isl6421.c +16
-0+ +/* 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)
drivers/media/dvb-frontends/isl6421.c struct dvb_frontend *isl6421_attach(struct dvb_frontend *fe, struct i2c_adapter · line 217
Quoted 8 lines
drivers/media/dvb-frontends/isl6421.c +2
-0@@ -148,6 +210,8 @@ struct dvb_frontend *isl6421_attach(struct dvb_frontend *fe, struct i2c_adapterreturn 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.
PATCH v2 1/1 media: isl6421: add checks for current overflow
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.
--- drivers/media/dvb-frontends/isl6421.c | 72 +++++++++++++++++++++++++++++++++-- 1 file changed, 68 insertions(+), 4 deletions(-)
drivers/media/dvb-frontends/isl6421.c +68 -4
@@ -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_adapterreturn NULL ; } + isl6421->is_off = true; +/* install release callback */ fe->ops.release_sec = isl6421_release;
Signature · 1 line
2.13.3
Hi Mauro,
Quoted 78 lines · Mauro Carvalho Chehab
This Kaffeine's BZ:
https://bugs.kde.org/show_bug.cgi?id=374693affects 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 <[email protected]>--- drivers/media/dvb-frontends/isl6421.c | 72 +++++++++++++++++++++++++++++++++-- 1 file changed, 68 insertions(+), 4 deletions(-)drivers/media/dvb-frontends/isl6421.c +21
-3@@ -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;
drivers/media/dvb-frontends/isl6421.c static int isl6421_set_voltage(struct dvb_frontend *fe, · line 93
Quoted 10 lines
drivers/media/dvb-frontends/isl6421.c +7
-1isl6421->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.
drivers/media/dvb-frontends/isl6421.c static int isl6421_set_voltage(struct dvb_frontend *fe, · line 101
Quoted 8 lines
drivers/media/dvb-frontends/isl6421.c +8
-0+ + 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?
drivers/media/dvb-frontends/isl6421.c static int isl6421_set_voltage(struct dvb_frontend *fe, · line 105
Quoted 4 lines
drivers/media/dvb-frontends/isl6421.c +4
-0+ +/* 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.
drivers/media/dvb-frontends/isl6421.c static int isl6421_set_voltage(struct dvb_frontend *fe, · line 115
Quoted 10 lines
drivers/media/dvb-frontends/isl6421.c +10
-0+ +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.
Quoted 20 lines
drivers/media/dvb-frontends/isl6421.c +16
-0+ +/* 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)
drivers/media/dvb-frontends/isl6421.c struct dvb_frontend *isl6421_attach(struct dvb_frontend *fe, struct i2c_adapter · line 217
Quoted 8 lines
drivers/media/dvb-frontends/isl6421.c +2
-0@@ -148,6 +210,8 @@ struct dvb_frontend *isl6421_attach(struct dvb_frontend *fe, struct i2c_adapterreturn 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.
Em Tue, 15 Aug 2017 20:51:09 +0100
Jemma Denson <[email protected]> escreveu:
Quoted 84 lines
Hi Mauro,
Quoted 80 lines · Mauro Carvalho Chehab
This Kaffeine's BZ:
https://bugs.kde.org/show_bug.cgi?id=374693affects 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 <[email protected]>--- drivers/media/dvb-frontends/isl6421.c | 72 +++++++++++++++++++++++++++++++++-- 1 file changed, 68 insertions(+), 4 deletions(-)drivers/media/dvb-frontends/isl6421.c +21
-3@@ -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;
Quoted 11 lines
drivers/media/dvb-frontends/isl6421.c static int isl6421_set_voltage(struct dvb_frontend *fe, · line 93
Quoted 8 lines
drivers/media/dvb-frontends/isl6421.c +7
-1-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 :-)
Quoted 10 lines
drivers/media/dvb-frontends/isl6421.c static int isl6421_set_voltage(struct dvb_frontend *fe, · line 101
Quoted 8 lines
drivers/media/dvb-frontends/isl6421.c +8
-0+ + 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.
Quoted 10 lines
drivers/media/dvb-frontends/isl6421.c static int isl6421_set_voltage(struct dvb_frontend *fe, · line 105
Quoted 5 lines
drivers/media/dvb-frontends/isl6421.c +4
-0+ +/* 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.
Quoted 3 lines
drivers/media/dvb-frontends/isl6421.c static int isl6421_set_voltage(struct dvb_frontend *fe, · line 108
Quoted 3 lines
drivers/media/dvb-frontends/isl6421.c +3
-0+ +if (isl6421->is_off && !is_off) { + isl6421->config &= ~ISL6421_EN1 ;
Again, this should be DCL.
Quoted 14 lines
drivers/media/dvb-frontends/isl6421.c static int isl6421_set_voltage(struct dvb_frontend *fe, · line 115
Quoted 7 lines
drivers/media/dvb-frontends/isl6421.c +7
-0+ + 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_offandis_offare 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?
Quoted 2 lines
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.
Quoted 24 lines
Quoted 24 lines
drivers/media/dvb-frontends/isl6421.c +16
-0+ +/* 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_adapterreturn NULL ; }
Quoted 8 lines
drivers/media/dvb-frontends/isl6421.c struct dvb_frontend *isl6421_attach(struct dvb_frontend *fe, struct i2c_adapter · line 217
Quoted 5 lines
drivers/media/dvb-frontends/isl6421.c +2
-0+ 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
Quoted 7 lines · Mauro Carvalho Chehab
Quoted 2 lines
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.
Quoted 7 lines · Jemma Denson
Quoted 6 lines · Mauro Carvalho Chehab
Quoted 2 lines
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
Quoted 7 lines
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.
Em Sun, 13 Aug 2017 19:47:37 +0100
Jemma Denson <[email protected]> escreveu:
Quoted 144 lines
Hi Mauro,
Just tried this on my card - unfortunately the patch as is doesn't work.
Quoted 84 lines · Mauro Carvalho Chehab
This Kaffeine's BZ:
https://bugs.kde.org/show_bug.cgi?id=374693affects 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 <[email protected]>--- drivers/media/dvb-frontends/isl6421.c | 72 +++++++++++++++++++++++++++++++++-- 1 file changed, 68 insertions(+), 4 deletions(-)drivers/media/dvb-frontends/isl6421.c +24
-4@@ -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 drivers/media/dvb-frontends/isl6421.c static int isl6421_set_voltage(struct dvb_frontend *fe, · line 99
Quoted 10 lines
drivers/media/dvb-frontends/isl6421.c +10
-0+* 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
Quoted 6 lines
drivers/media/dvb-frontends/isl6421.c +6
-0+ + 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 );drivers/media/dvb-frontends/isl6421.c static int isl6421_set_voltage(struct dvb_frontend *fe, · line 115
Quoted 10 lines
drivers/media/dvb-frontends/isl6421.c +10
-0+ +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
Quoted 23 lines
drivers/media/dvb-frontends/isl6421.c +16
-0+ +/* 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_adapterreturn NULL ; }
Quoted 10 lines
drivers/media/dvb-frontends/isl6421.c struct dvb_frontend *isl6421_attach(struct dvb_frontend *fe, struct i2c_adapter · line 217
Quoted 5 lines
drivers/media/dvb-frontends/isl6421.c +2
-0+ 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
PATCH v3 1/1 media: isl6421: add checks for current overflow
In reply to Mauro Carvalho ChehabThis 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.
--- drivers/media/dvb-frontends/isl6421.c | 77 +++++++++++++++++++++++++++++++++-- 1 file changed, 73 insertions(+), 4 deletions(-)
drivers/media/dvb-frontends/isl6421.c +73 -4
@@ -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_adapterreturn NULL ; } + isl6421->is_off = true; +/* install release callback */ fe->ops.release_sec = isl6421_release;
Signature · 1 line
2.13.4
PATCH v4 1/1 media: isl6421: add checks for current overflow
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
--- drivers/media/dvb-frontends/isl6421.c | 76 +++++++++++++++++++++++++++++++++-- 1 file changed, 72 insertions(+), 4 deletions(-)
drivers/media/dvb-frontends/isl6421.c +72 -4
@@ -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_adapterreturn NULL ; } + isl6421->is_off = true; +/* install release callback */ fe->ops.release_sec = isl6421_release;
Signature · 1 line
2.13.4