[PATCH] isl6421.c - added optional features: tone control and temporary diseqc overcurrent

[PATCH] isl6421.c - added optional features: tone control and temporary diseqc overcurrent

[HoP]

[-- Attachment #1: Type: text/plain, Size: 838 bytes --]

Hi,

this is my first kernel patch, so all comments are welcome.

Attached patch adds two optional (so, disabled by default
and therefore could not break any compatibility) features:

1, tone_control=1
When enabled, ISL6421 overrides frontend's tone control
function (fe->ops.set_tone) by its own one.

2, overcurrent_enable=1
When enabled, overcurrent protection is disabled during
sending diseqc command. Such option is usable when ISL6421
catch overcurrent threshold and starts limiting output.
Note: protection is disabled only during sending
of diseqc command, until next set_tone() usage.
What typically means only max up to few hundreds of ms.
WARNING: overcurrent_enable=1 is dangerous
and can damage your device. Use with care
and only if you really know what you do.

/Honza

Signed-off-by: Jan Petrous <jpetrous@gmail.com>
---

[-- Attachment #2: isl6421-tonectrl_overcurr.patch --]
[-- Type: text/x-patch, Size: 4757 bytes --]

diff -r f6680fa8e7ec linux/drivers/media/dvb/frontends/isl6421.c
--- a/linux/drivers/media/dvb/frontends/isl6421.c	Tue Oct 20 00:08:05 2009 +0900
+++ b/linux/drivers/media/dvb/frontends/isl6421.c	Sun Oct 25 00:59:46 2009 +0200
@@ -3,6 +3,9 @@
  *
  * Copyright (C) 2006 Andrew de Quincey
  * Copyright (C) 2006 Oliver Endriss
+ * Copyright (C) 2009 Ales Jurik and Jan Petrous (added optional 22k tone 
+ *                    support and temporary diseqc overcurrent enable until 
+ *                    next command - set voltage or tone)
  *
  * This program is free software; you can redistribute it and/or
  * modify it under the terms of the GNU General Public License
@@ -35,12 +38,23 @@
 #include "dvb_frontend.h"
 #include "isl6421.h"
 
+static int tone_control = 0;
+module_param(tone_control, int, S_IRUGO);
+MODULE_PARM_DESC(tone_control, "Set ISL6421 to control 22kHz tone");
+
+static int overcurrent_enable = 0;
+module_param(overcurrent_enable, int, S_IRUGO);
+MODULE_PARM_DESC(overcurrent_enable, "Set ISL6421 to temporary enable overcurrent "
+		"when diseqc command is active");
+
 struct isl6421 {
 	u8			config;
 	u8			override_or;
 	u8			override_and;
 	struct i2c_adapter	*i2c;
 	u8			i2c_addr;
+	int (*diseqc_send_master_cmd_orig)(struct dvb_frontend* fe,
+			struct dvb_diseqc_master_cmd* cmd);
 };
 
 static int isl6421_set_voltage(struct dvb_frontend *fe, fe_sec_voltage_t voltage)
@@ -60,6 +74,53 @@
 		break;
 	case SEC_VOLTAGE_18:
 		isl6421->config |= (ISL6421_EN1 | ISL6421_VSEL1);
+		break;
+	default:
+		return -EINVAL;
+	};
+
+	isl6421->config |= isl6421->override_or;
+	isl6421->config &= isl6421->override_and;
+
+	return (i2c_transfer(isl6421->i2c, &msg, 1) == 1) ? 0 : -EIO;
+}
+
+static int isl6421_send_diseqc(struct dvb_frontend *fe,
+				struct dvb_diseqc_master_cmd *cmd)
+{
+	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) };
+
+	isl6421->config |= ISL6421_DCL;
+
+	isl6421->config |= isl6421->override_or;
+	isl6421->config &= isl6421->override_and;
+
+	if (i2c_transfer(isl6421->i2c, &msg, 1) != 1) return -EIO;
+	
+	isl6421->config &= ~ISL6421_DCL;
+	
+	return isl6421->diseqc_send_master_cmd_orig(fe, cmd);
+}
+
+static int isl6421_set_tone(struct dvb_frontend *fe, fe_sec_tone_mode_t tone)
+{
+	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) };
+
+	isl6421->config &= ~(ISL6421_ENT1);
+
+	printk(KERN_INFO "%s: %s\n", __func__, ((tone == SEC_TONE_OFF) ? "Off" : "On"));
+
+	switch(tone) {
+	case SEC_TONE_ON:
+		isl6421->config |= ISL6421_ENT1;
+		break;
+	case SEC_TONE_OFF:
 		break;
 	default:
 		return -EINVAL;
@@ -91,18 +152,26 @@
 
 static void isl6421_release(struct dvb_frontend *fe)
 {
+	struct isl6421 *isl6421 = (struct isl6421 *) fe->sec_priv;
+
 	/* power off */
 	isl6421_set_voltage(fe, SEC_VOLTAGE_OFF);
+
+	if(overcurrent_enable)
+		fe->ops.diseqc_send_master_cmd =
+			isl6421->diseqc_send_master_cmd_orig;
 
 	/* free */
 	kfree(fe->sec_priv);
 	fe->sec_priv = NULL;
 }
 
-struct dvb_frontend *isl6421_attach(struct dvb_frontend *fe, struct i2c_adapter *i2c, u8 i2c_addr,
-		   u8 override_set, u8 override_clear)
+struct dvb_frontend *isl6421_attach(struct dvb_frontend *fe,
+		struct i2c_adapter *i2c, u8 i2c_addr, u8 override_set,
+		u8 override_clear)
 {
 	struct isl6421 *isl6421 = kmalloc(sizeof(struct isl6421), GFP_KERNEL);
+
 	if (!isl6421)
 		return NULL;
 
@@ -131,6 +200,31 @@
 	/* override frontend ops */
 	fe->ops.set_voltage = isl6421_set_voltage;
 	fe->ops.enable_high_lnb_voltage = isl6421_enable_high_lnb_voltage;
+	if(tone_control)
+		fe->ops.set_tone = isl6421_set_tone;
+	if(overcurrent_enable) {
+		if((override_set & ISL6421_DCL) ||
+				(override_clear & ISL6421_DCL)) {
+		/* there is no sense to use overcurrent_enable with DCL bit set
+		 * in any override byte */
+			if(override_set & ISL6421_DCL)
+				printk(KERN_WARNING "ISL6421 overcurrent_enable"
+						" with DCL bit in override_set,"
+						" overcurrent_enable ignored\n");
+			if(override_clear & ISL6421_DCL)
+				printk(KERN_WARNING "ISL6421 overcurrent_enable"
+						" with DCL bit in override_clear,"
+						" overcurrent_enable ignored\n");
+		} else {
+			isl6421->diseqc_send_master_cmd_orig =
+				fe->ops.diseqc_send_master_cmd;
+			fe->ops.diseqc_send_master_cmd = isl6421_send_diseqc;
+		}
+	}
+
+	printk(KERN_INFO "ISL6421 attached on addr=%x %s %s\n", i2c_addr,
+			overcurrent_enable ? "(overcurrent_enable overwritten)" : "",
+			tone_control ? "(tone_control overwritten)" : "");
 
 	return fe;
 }

Re: [PATCH] isl6421.c - added optional features: tone control and temporary diseqc overcurrent

[Mauro Carvalho Chehab]

HoP escreveu:
> Hi,
>
> this is my first kernel patch, so all comments are welcome.
>   
First of all, please check all your patches with checkpatch, to be sure
that they don't have any CodingStyle troubles. There are some on your
patch (the better is to read README.patches for more info useful
for developers).
> Attached patch adds two optional (so, disabled by default
> and therefore could not break any compatibility) features:
>
> 1, tone_control=1
> When enabled, ISL6421 overrides frontend's tone control
> function (fe->ops.set_tone) by its own one.
>   
On your comments, the better is to describe why someone would need
to use such option. You should also add a quick hint about that at the
option description.
> 2, overcurrent_enable=1
> When enabled, overcurrent protection is disabled during
> sending diseqc command. Such option is usable when ISL6421
> catch overcurrent threshold and starts limiting output.
> Note: protection is disabled only during sending
> of diseqc command, until next set_tone() usage.
> What typically means only max up to few hundreds of ms.
> WARNING: overcurrent_enable=1 is dangerous
> and can damage your device. Use with care
> and only if you really know what you do.
>   
I'm not sure if it is a good idea to have this... Why/when someone would 
need this?

If we go ahead and add this one, you should add a notice about it at the 
parameter.
I would also print a big WARNING message at the dmesg if the module were 
loaded
with this option turned on.
> /Honza
>
> Signed-off-by: Jan Petrous <jpetrous@gmail.com>
> ---
>   

Re: [PATCH] isl6421.c - added optional features: tone control and temporary diseqc overcurrent

[HoP]

[-- Attachment #1: Type: text/plain, Size: 2820 bytes --]

Hi Mauro,

thank you for your valued hints. I'm commenting inside
message:

> First of all, please check all your patches with checkpatch, to be sure
> that they don't have any CodingStyle troubles. There are some on your
> patch (the better is to read README.patches for more info useful
> for developers).

Did checkpatch testing and has fixed all errors/warnings except
of 3 warning regarding longer line (all 3 lines has exactly
one char over 80, so I guess it should not bother much).
Of course if this rule is a must, then I can fix that also).

>>
>> Attached patch adds two optional (so, disabled by default
>> and therefore could not break any compatibility) features:
>>
>> 1, tone_control=1
>> When enabled, ISL6421 overrides frontend's tone control
>> function (fe->ops.set_tone) by its own one.
>>
>
> On your comments, the better is to describe why someone would need
> to use such option. You should also add a quick hint about that at the
> option description.

Well, I'm not sure I can make some good hint why such option can
be useful by someone. I can only say that isl6121 has possibility
to drive 22k tone, so why not enable usage of it?

Of course, we made such code because we were using exactly
this way of 22k control in our device.

>>
>> 2, overcurrent_enable=1
>> When enabled, overcurrent protection is disabled during
>> sending diseqc command. Such option is usable when ISL6421
>> catch overcurrent threshold and starts limiting output.
>> Note: protection is disabled only during sending
>> of diseqc command, until next set_tone() usage.
>> What typically means only max up to few hundreds of ms.
>> WARNING: overcurrent_enable=1 is dangerous
>> and can damage your device. Use with care
>> and only if you really know what you do.
>>
>
> I'm not sure if it is a good idea to have this... Why/when someone would
> need this?
>

I know that it is a bit dangerous option, so I can understand you can
don't like it :)

But I would like to note again - such way of using is permitted
by datasheet (otherwise it would not be even possible to enable it)
and we learnt when used correctly (it is enabled only within diseqc
sequence), it boost rotor moving or fixes using some "power-eating"
diseqc switches.

If you still feel it is better to not support bit strange mode, then
I can live with "#if 0" commented out blocks or adding some
kernel config option with something like ISL6421_ENABLE_OVERCURRENT
or so.

> If we go ahead and add this one, you should add a notice about it at the
> parameter.
> I would also print a big WARNING message at the dmesg if the module were
> loaded
> with this option turned on.

Added some WARNING printing to dmesg when option is enabled.

Regards

/Honza

---

Signed-off-by: Jan Petrous <jpetrous@gmail.com>
Signed-off-by: Ales Jurik <ajurik@quick.cz>

[-- Attachment #2: isl6421-tonectrl_overcurr.patch --]
[-- Type: text/x-patch, Size: 4877 bytes --]

diff -r 9d9bc92d7c33 drivers/media/dvb/frontends/isl6421.c
--- a/drivers/media/dvb/frontends/isl6421.c	Sat Sep 19 12:48:44 2009 +0200
+++ b/drivers/media/dvb/frontends/isl6421.c	Tue Nov 03 23:23:05 2009 +0100
@@ -3,6 +3,9 @@
  *
  * Copyright (C) 2006 Andrew de Quincey
  * Copyright (C) 2006 Oliver Endriss
+ * Copyright (C) 2009 Ales Jurik and Jan Petrous (added optional 22k tone
+ *                    support and temporary diseqc overcurrent enable until
+ *                    next command - set voltage or tone)
  *
  * This program is free software; you can redistribute it and/or
  * modify it under the terms of the GNU General Public License
@@ -35,12 +38,23 @@
 #include "dvb_frontend.h"
 #include "isl6421.h"
 
+static int tone_control;
+module_param(tone_control, int, S_IRUGO);
+MODULE_PARM_DESC(tone_control, "Set ISL6421 to control 22kHz tone");
+
+static int overcurrent_enable;
+module_param(overcurrent_enable, int, S_IRUGO);
+MODULE_PARM_DESC(overcurrent_enable, "Set ISL6421 to temporary enable "
+		"overcurrent when diseqc command is active");
+
 struct isl6421 {
 	u8			config;
 	u8			override_or;
 	u8			override_and;
 	struct i2c_adapter	*i2c;
 	u8			i2c_addr;
+	int (*diseqc_send_master_cmd_orig)(struct dvb_frontend *fe,
+			struct dvb_diseqc_master_cmd *cmd);
 };
 
 static int isl6421_set_voltage(struct dvb_frontend *fe, fe_sec_voltage_t voltage)
@@ -60,6 +74,55 @@ static int isl6421_set_voltage(struct dv
 		break;
 	case SEC_VOLTAGE_18:
 		isl6421->config |= (ISL6421_EN1 | ISL6421_VSEL1);
+		break;
+	default:
+		return -EINVAL;
+	};
+
+	isl6421->config |= isl6421->override_or;
+	isl6421->config &= isl6421->override_and;
+
+	return (i2c_transfer(isl6421->i2c, &msg, 1) == 1) ? 0 : -EIO;
+}
+
+static int isl6421_send_diseqc(struct dvb_frontend *fe,
+				struct dvb_diseqc_master_cmd *cmd)
+{
+	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) };
+
+	isl6421->config |= ISL6421_DCL;
+
+	isl6421->config |= isl6421->override_or;
+	isl6421->config &= isl6421->override_and;
+
+	if (i2c_transfer(isl6421->i2c, &msg, 1) != 1)
+		return -EIO;
+
+	isl6421->config &= ~ISL6421_DCL;
+
+	return isl6421->diseqc_send_master_cmd_orig(fe, cmd);
+}
+
+static int isl6421_set_tone(struct dvb_frontend *fe, fe_sec_tone_mode_t tone)
+{
+	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) };
+
+	isl6421->config &= ~(ISL6421_ENT1);
+
+	printk(KERN_INFO "%s: %s\n", __func__, ((tone == SEC_TONE_OFF) ?
+				"Off" : "On"));
+
+	switch (tone) {
+	case SEC_TONE_ON:
+		isl6421->config |= ISL6421_ENT1;
+		break;
+	case SEC_TONE_OFF:
 		break;
 	default:
 		return -EINVAL;
@@ -91,18 +154,26 @@ static int isl6421_enable_high_lnb_volta
 
 static void isl6421_release(struct dvb_frontend *fe)
 {
+	struct isl6421 *isl6421 = (struct isl6421 *) fe->sec_priv;
+
 	/* power off */
 	isl6421_set_voltage(fe, SEC_VOLTAGE_OFF);
+
+	if (overcurrent_enable)
+		fe->ops.diseqc_send_master_cmd =
+			isl6421->diseqc_send_master_cmd_orig;
 
 	/* free */
 	kfree(fe->sec_priv);
 	fe->sec_priv = NULL;
 }
 
-struct dvb_frontend *isl6421_attach(struct dvb_frontend *fe, struct i2c_adapter *i2c, u8 i2c_addr,
-		   u8 override_set, u8 override_clear)
+struct dvb_frontend *isl6421_attach(struct dvb_frontend *fe,
+		struct i2c_adapter *i2c, u8 i2c_addr, u8 override_set,
+		u8 override_clear)
 {
 	struct isl6421 *isl6421 = kmalloc(sizeof(struct isl6421), GFP_KERNEL);
+
 	if (!isl6421)
 		return NULL;
 
@@ -131,6 +202,33 @@ struct dvb_frontend *isl6421_attach(stru
 	/* override frontend ops */
 	fe->ops.set_voltage = isl6421_set_voltage;
 	fe->ops.enable_high_lnb_voltage = isl6421_enable_high_lnb_voltage;
+	if (tone_control)
+		fe->ops.set_tone = isl6421_set_tone;
+
+	printk(KERN_INFO "ISL6421 attached on addr=%x\n", i2c_addr);
+
+	if (overcurrent_enable) {
+		if ((override_set & ISL6421_DCL) ||
+				(override_clear & ISL6421_DCL)) {
+			/* there is no sense to use overcurrent_enable
+			 * with DCL bit set in any override byte */
+			if (override_set & ISL6421_DCL)
+				printk(KERN_WARNING "ISL6421 overcurrent_enable"
+						" with DCL bit in override_set,"
+						" overcurrent_enable ignored\n");
+			if (override_clear & ISL6421_DCL)
+				printk(KERN_WARNING "ISL6421 overcurrent_enable"
+						" with DCL bit in override_clear,"
+						" overcurrent_enable ignored\n");
+		} else {
+			printk(KERN_WARNING "ISL6421 overcurrent_enable "
+					" activated. WARNING: it can be "
+					" dangerous for your hardware!");
+			isl6421->diseqc_send_master_cmd_orig =
+				fe->ops.diseqc_send_master_cmd;
+			fe->ops.diseqc_send_master_cmd = isl6421_send_diseqc;
+		}
+	}
 
 	return fe;
 }

Re: [PATCH] isl6421.c - added optional features: tone control and temporary diseqc overcurrent

[hermann pitton]

Hi Honza,

Am Mittwoch, den 04.11.2009, 00:10 +0100 schrieb HoP:
> Hi Mauro,
> 
> thank you for your valued hints. I'm commenting inside
> message:
> 
> > First of all, please check all your patches with checkpatch, to be sure
> > that they don't have any CodingStyle troubles. There are some on your
> > patch (the better is to read README.patches for more info useful
> > for developers).
> 
> Did checkpatch testing and has fixed all errors/warnings except
> of 3 warning regarding longer line (all 3 lines has exactly
> one char over 80, so I guess it should not bother much).
> Of course if this rule is a must, then I can fix that also).
> 
> >>
> >> Attached patch adds two optional (so, disabled by default
> >> and therefore could not break any compatibility) features:
> >>
> >> 1, tone_control=1
> >> When enabled, ISL6421 overrides frontend's tone control
> >> function (fe->ops.set_tone) by its own one.
> >>
> >
> > On your comments, the better is to describe why someone would need
> > to use such option. You should also add a quick hint about that at the
> > option description.
> 
> Well, I'm not sure I can make some good hint why such option can
> be useful by someone. I can only say that isl6121 has possibility
> to drive 22k tone, so why not enable usage of it?

well, we have much more experienced guys than me here on that, but it
should be device specific then.

> Of course, we made such code because we were using exactly
> this way of 22k control in our device.

So the demod can't do it or just free choice?

> >>
> >> 2, overcurrent_enable=1
> >> When enabled, overcurrent protection is disabled during
> >> sending diseqc command. Such option is usable when ISL6421
> >> catch overcurrent threshold and starts limiting output.
> >> Note: protection is disabled only during sending
> >> of diseqc command, until next set_tone() usage.
> >> What typically means only max up to few hundreds of ms.
> >> WARNING: overcurrent_enable=1 is dangerous
> >> and can damage your device. Use with care
> >> and only if you really know what you do.
> >>
> >
> > I'm not sure if it is a good idea to have this... Why/when someone would
> > need this?
> >
> 
> I know that it is a bit dangerous option, so I can understand you can
> don't like it :)
> 
> But I would like to note again - such way of using is permitted
> by datasheet (otherwise it would not be even possible to enable it)
> and we learnt when used correctly (it is enabled only within diseqc
> sequence), it boost rotor moving or fixes using some "power-eating"
> diseqc switches.
> 
> If you still feel it is better to not support bit strange mode, then
> I can live with "#if 0" commented out blocks or adding some
> kernel config option with something like ISL6421_ENABLE_OVERCURRENT
> or so.

Question is, can you melt down some chip with it or not?

If you can, stay away, since this was not in the scope earlier.

Cheers,
Hermann

> > If we go ahead and add this one, you should add a notice about it at the
> > parameter.
> > I would also print a big WARNING message at the dmesg if the module were
> > loaded
> > with this option turned on.
> 
> Added some WARNING printing to dmesg when option is enabled.
> 
> Regards
> 
> /Honza
> 
> ---
> 
> Signed-off-by: Jan Petrous <jpetrous@gmail.com>
> Signed-off-by: Ales Jurik <ajurik@quick.cz>

Re: [PATCH] isl6421.c - added optional features: tone control and temporary diseqc overcurrent

[HoP]

Hi Hermann,

>> >>
>> >> Attached patch adds two optional (so, disabled by default
>> >> and therefore could not break any compatibility) features:
>> >>
>> >> 1, tone_control=1
>> >> When enabled, ISL6421 overrides frontend's tone control
>> >> function (fe->ops.set_tone) by its own one.
>> >>
>> >
>> > On your comments, the better is to describe why someone would need
>> > to use such option. You should also add a quick hint about that at the
>> > option description.
>>
>> Well, I'm not sure I can make some good hint why such option can
>> be useful by someone. I can only say that isl6121 has possibility
>> to drive 22k tone, so why not enable usage of it?
>
> well, we have much more experienced guys than me here on that, but it
> should be device specific then.
>
>> Of course, we made such code because we were using exactly
>> this way of 22k control in our device.
>
> So the demod can't do it or just free choice?
>

Well, more detailed Ales can speak about it, he is "hw guy" here :)
Anyway, regardless reason of choice important is that isl6421
can be used this way and, may be even more important, it is
used (and works correctly) in our hardware.

I understand it can be a bit non-usual way of usage, but as
I said, it works for us :)

>> >>
>> >> 2, overcurrent_enable=1
>> >> When enabled, overcurrent protection is disabled during
>> >> sending diseqc command. Such option is usable when ISL6421
>> >> catch overcurrent threshold and starts limiting output.
>> >> Note: protection is disabled only during sending
>> >> of diseqc command, until next set_tone() usage.
>> >> What typically means only max up to few hundreds of ms.
>> >> WARNING: overcurrent_enable=1 is dangerous
>> >> and can damage your device. Use with care
>> >> and only if you really know what you do.
>> >>
>> >
>> > I'm not sure if it is a good idea to have this... Why/when someone would
>> > need this?
>> >
>>
>> I know that it is a bit dangerous option, so I can understand you can
>> don't like it :)
>>
>> But I would like to note again - such way of using is permitted
>> by datasheet (otherwise it would not be even possible to enable it)
>> and we learnt when used correctly (it is enabled only within diseqc
>> sequence), it boost rotor moving or fixes using some "power-eating"
>> diseqc switches.
>>
>> If you still feel it is better to not support bit strange mode, then
>> I can live with "#if 0" commented out blocks or adding some
>> kernel config option with something like ISL6421_ENABLE_OVERCURRENT
>> or so.
>
> Question is, can you melt down some chip with it or not?
>
> If you can, stay away, since this was not in the scope earlier.
>

We have tested it with few devices (both rotor and diseqc switches)
and have not ran in any damage yet.

TBH, I'm writing about possibility of damage only because
of understanding that if I disable overcurrent safeguard I
can imagine it can end up bad way. But not tested on our side.

Regards

/Honza

Re: [PATCH] isl6421.c - added optional features: tone control and temporary diseqc overcurrent

[Ales Jurik]

On Wednesday 04 of November 2009, HoP wrote:
> Hi Hermann,
> 
> >> >> Attached patch adds two optional (so, disabled by default
> >> >> and therefore could not break any compatibility) features:
> >> >>
> >> >> 1, tone_control=1
> >> >> When enabled, ISL6421 overrides frontend's tone control
> >> >> function (fe->ops.set_tone) by its own one.
> >> >
> >> > On your comments, the better is to describe why someone would need
> >> > to use such option. You should also add a quick hint about that at the
> >> > option description.
> >>
> >> Well, I'm not sure I can make some good hint why such option can
> >> be useful by someone. I can only say that isl6121 has possibility
> >> to drive 22k tone, so why not enable usage of it?
> >
> > well, we have much more experienced guys than me here on that, but it
> > should be device specific then.
> >
> >> Of course, we made such code because we were using exactly
> >> this way of 22k control in our device.
> >
> > So the demod can't do it or just free choice?
> 
> Well, more detailed Ales can speak about it, he is "hw guy" here :)
> Anyway, regardless reason of choice important is that isl6421
> can be used this way and, may be even more important, it is
> used (and works correctly) in our hardware.
> 
> I understand it can be a bit non-usual way of usage, but as
> I said, it works for us :)
When using isl6421 it is one possible way how to modulate LNB voltage with 
22kHz tone. The interesting is that if frontend is capable to support such 
function it doesn't need any additional hw.
> 
> >> >> 2, overcurrent_enable=1
> >> >> When enabled, overcurrent protection is disabled during
> >> >> sending diseqc command. Such option is usable when ISL6421
> >> >> catch overcurrent threshold and starts limiting output.
> >> >> Note: protection is disabled only during sending
> >> >> of diseqc command, until next set_tone() usage.
> >> >> What typically means only max up to few hundreds of ms.
> >> >> WARNING: overcurrent_enable=1 is dangerous
> >> >> and can damage your device. Use with care
> >> >> and only if you really know what you do.
> >> >
> >> > I'm not sure if it is a good idea to have this... Why/when someone
> >> > would need this?
> >>
> >> I know that it is a bit dangerous option, so I can understand you can
> >> don't like it :)
> >>
> >> But I would like to note again - such way of using is permitted
> >> by datasheet (otherwise it would not be even possible to enable it)
> >> and we learnt when used correctly (it is enabled only within diseqc
> >> sequence), it boost rotor moving or fixes using some "power-eating"
> >> diseqc switches.
> >>
> >> If you still feel it is better to not support bit strange mode, then
> >> I can live with "#if 0" commented out blocks or adding some
> >> kernel config option with something like ISL6421_ENABLE_OVERCURRENT
> >> or so.
> >
> > Question is, can you melt down some chip with it or not?
> >
> > If you can, stay away, since this was not in the scope earlier.
> 
> We have tested it with few devices (both rotor and diseqc switches)
> and have not ran in any damage yet.
> 
> TBH, I'm writing about possibility of damage only because
> of understanding that if I disable overcurrent safeguard I
> can imagine it can end up bad way. But not tested on our side.
> 
> Regards
> 
> /Honza
> 
This is used in way I hope it was supposed to by designers of the chip. The 
current to LNB is in real not "unlimited", it is limited by hw design (sense 
resistor in FET circuit). So not isl6421 nor connected FET should be damaged 
even when short circuit appears in antenna connection, but as in most of cases 
this feature is not needed we add it as optional parameter.

Of course hw designer should take care of power dissipation from the circuit.

Let me cite the isl6421 datasheet:

"However, there could be some cases in which a highly                                                               
capacitive load on the output may cause a difficult start-up,
when the dynamic protection is chosen. This can be solved
by initiating a power start-up in static mode (DCL = HIGH)
and then switching to the dynamic mode (DCL = LOW) after
a chosen amount of time. When in static mode, the OLF bit
goes HIGH when the current clamp limit is reached and
returns LOW when the overload condition is cleared. The
OLF bit will be LOW at the end of initial power-on soft-start."

This is exactly situation in which we find ourselves when testing our hw with 
cascade of diseqc switch and diseqc motor. The proposed patch and activating 
of temporarily disabling the dynamic current limitations solved this problem 
perfectly.

BR,

Ales

Re: [PATCH] isl6421.c - added optional features: tone control and temporary diseqc overcurrent

[Mauro Carvalho Chehab]

Hi Ales and HoP,

Em Wed, 4 Nov 2009 09:35:51 +0100
Ales Jurik <ajurik@quick.cz> escreveu:

> On Wednesday 04 of November 2009, HoP wrote:
> > Hi Hermann,
> > 
> > >> >> Attached patch adds two optional (so, disabled by default
> > >> >> and therefore could not break any compatibility) features:
> > >> >>
> > >> >> 1, tone_control=1
> > >> >> When enabled, ISL6421 overrides frontend's tone control
> > >> >> function (fe->ops.set_tone) by its own one.
> > >> >
> > >> > On your comments, the better is to describe why someone would need
> > >> > to use such option. You should also add a quick hint about that at the
> > >> > option description.
> > >>
> > >> Well, I'm not sure I can make some good hint why such option can
> > >> be useful by someone. I can only say that isl6121 has possibility
> > >> to drive 22k tone, so why not enable usage of it?
> > >
> > > well, we have much more experienced guys than me here on that, but it
> > > should be device specific then.
> > >
> > >> Of course, we made such code because we were using exactly
> > >> this way of 22k control in our device.
> > >
> > > So the demod can't do it or just free choice?
> > 
> > Well, more detailed Ales can speak about it, he is "hw guy" here :)
> > Anyway, regardless reason of choice important is that isl6421
> > can be used this way and, may be even more important, it is
> > used (and works correctly) in our hardware.
> > 
> > I understand it can be a bit non-usual way of usage, but as
> > I said, it works for us :)
> When using isl6421 it is one possible way how to modulate LNB voltage with 
> 22kHz tone. The interesting is that if frontend is capable to support such 
> function it doesn't need any additional hw.
> > 
> > >> >> 2, overcurrent_enable=1
> > >> >> When enabled, overcurrent protection is disabled during
> > >> >> sending diseqc command. Such option is usable when ISL6421
> > >> >> catch overcurrent threshold and starts limiting output.
> > >> >> Note: protection is disabled only during sending
> > >> >> of diseqc command, until next set_tone() usage.
> > >> >> What typically means only max up to few hundreds of ms.
> > >> >> WARNING: overcurrent_enable=1 is dangerous
> > >> >> and can damage your device. Use with care
> > >> >> and only if you really know what you do.
> > >> >
> > >> > I'm not sure if it is a good idea to have this... Why/when someone
> > >> > would need this?
> > >>
> > >> I know that it is a bit dangerous option, so I can understand you can
> > >> don't like it :)
> > >>
> > >> But I would like to note again - such way of using is permitted
> > >> by datasheet (otherwise it would not be even possible to enable it)
> > >> and we learnt when used correctly (it is enabled only within diseqc
> > >> sequence), it boost rotor moving or fixes using some "power-eating"
> > >> diseqc switches.
> > >>
> > >> If you still feel it is better to not support bit strange mode, then
> > >> I can live with "#if 0" commented out blocks or adding some
> > >> kernel config option with something like ISL6421_ENABLE_OVERCURRENT
> > >> or so.
> > >
> > > Question is, can you melt down some chip with it or not?
> > >
> > > If you can, stay away, since this was not in the scope earlier.
> > 
> > We have tested it with few devices (both rotor and diseqc switches)
> > and have not ran in any damage yet.
> > 
> > TBH, I'm writing about possibility of damage only because
> > of understanding that if I disable overcurrent safeguard I
> > can imagine it can end up bad way. But not tested on our side.
> > 
> > Regards
> > 
> > /Honza
> > 
> This is used in way I hope it was supposed to by designers of the chip. The 
> current to LNB is in real not "unlimited", it is limited by hw design (sense 
> resistor in FET circuit). So not isl6421 nor connected FET should be damaged 
> even when short circuit appears in antenna connection, but as in most of cases 
> this feature is not needed we add it as optional parameter.
> 
> Of course hw designer should take care of power dissipation from the circuit.
> 
> Let me cite the isl6421 datasheet:
> 
> "However, there could be some cases in which a highly                                                               
> capacitive load on the output may cause a difficult start-up,
> when the dynamic protection is chosen. This can be solved
> by initiating a power start-up in static mode (DCL = HIGH)
> and then switching to the dynamic mode (DCL = LOW) after
> a chosen amount of time. When in static mode, the OLF bit
> goes HIGH when the current clamp limit is reached and
> returns LOW when the overload condition is cleared. The
> OLF bit will be LOW at the end of initial power-on soft-start."
> 
> This is exactly situation in which we find ourselves when testing our hw with 
> cascade of diseqc switch and diseqc motor. The proposed patch and activating 
> of temporarily disabling the dynamic current limitations solved this problem 
> perfectly.

Your arguments make sense for me, but not as a patch for adding two extra
parameters that any user can try to enable as a trial to make their board work.

It should be, instead, mapped as two parameters at the frontend structure, and
sent us together with the driver (or with the patch for an existing driver)
that needs such features.

In the specific case of disabling the software limits, in favor of a hardware
protection, it should be clearly documented at the corresponding parameter, at
the frontend private struct that such usage requires a proper hardware protection
to avoid damages to isl6421.

Currently, isl6421 doesn't have such structs, but you can take a look at isl6423.h
for an example.

It should be something like:

struct isl6421_config {
	/* Enable DISEqC tone control mode */
	bool tone_control;		

	/*
	 * Disable isl6421 overcurrent protection.
	 *
	 * WARNING: Don't disable the protection unless you are 100% sure about
	 * 	    what you're doing, otherwise you may damage your board.
	 *	    Only a few designs require to disable the protection, since
	 *	    the hardware designer opted to use a hardware protection instead
	 */
	bool disable_overcurrent_protection;

	/*
	 * The current existing arguments - let's put them at the 
	 * config struct - adding a proper description for them 
	 */
	
	/* foo */
	u8 override_set;

	/* bar */
	u8 override_clear;
};

And the attach function will now be:

extern struct dvb_frontend *isl6421_attach(struct dvb_frontend *fe,
                                           struct i2c_adapter *i2c,
                                           const struct isl6421_config *config);


-- 

Cheers,
Mauro