[PATCH RFC] media: isl6421: add checks for current overflow

[PATCH RFC] media: isl6421: add checks for current overflow

[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.

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

Re: [PATCH RFC] media: isl6421: add checks for current overflow

[Jemma Denson]

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.

[PATCH v2] media: isl6421: add checks for current overflow

[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.

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

Re: [PATCH RFC] media: isl6421: add checks for current overflow

[Mauro Carvalho Chehab]

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

Re: [PATCH v2] media: isl6421: add checks for current overflow

[Jemma Denson]

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.

Re: [PATCH v2] media: isl6421: add checks for current overflow

[Mauro Carvalho Chehab]

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

[PATCH v3] media: isl6421: add checks for current overflow

[Jemma Denson]

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

Re: [PATCH v2] media: isl6421: add checks for current overflow

[Jemma Denson]

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.

[PATCH v4] media: isl6421: add checks for current overflow

[Jemma Denson]

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

Re: [PATCH v2] media: isl6421: add checks for current overflow

[Jemma Denson]

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.