diff mbox

cx24117: Fix/enhance set_voltage function.

Message ID 1381568085-2407-1-git-send-email-ljalvs@gmail.com (mailing list archive)
State New, archived
Headers show

Commit Message

Luis Alves Oct. 12, 2013, 8:54 a.m. UTC 
Hi,

On this patch:
Added a few defines to describe what every constant in the set_voltage function.
Added the description to the CX24117 GPIO control commands.
Moved the GPIODIR setup to the initfe function.

Regards,
Luis


Signed-off-by: Luis Alves <ljalvs@gmail.com>
---
 drivers/media/dvb-frontends/cx24117.c |   60 +++++++++++++++++++++------------
 1 file changed, 38 insertions(+), 22 deletions(-)

Comments

Michael Ira Krufky Oct. 14, 2013, 8:40 p.m. UTC | #1 
On Sat, Oct 12, 2013 at 4:54 AM, Luis Alves <ljalvs@gmail.com> wrote:
> Hi,
>
> On this patch:
> Added a few defines to describe what every constant in the set_voltage function.
> Added the description to the CX24117 GPIO control commands.
> Moved the GPIODIR setup to the initfe function.

Luis,

It is generally not preferred to send multiple changes in a single
patch, even if the changes themselves are small.

I know it's not such a huge patch, but, it is preferred for each patch
to contain only one single change, provided that the patch remains
atomic in nature.  I don't think it would be such a problem to merge
these changes as-is, but I do think it would be better to try to
enforce the idea of "one change per atomic patch" when possible.

Can you re-spin this into two (or three) smaller patches?

The first patch should handle the cosmetics, "Added a few defines to
describe every constant in the set_voltage function" & "Added the
description to the CX24117 GPIO control commands" ...  As you see,
these two bits change the code slightly but do not alter the behavior
of the driver.

The final patch should be the one that does alter the driver's
behavior, "Moved the GPIODIR setup to the initfe function"

Doing this can potentially help to quicken the review & merge process,
while also enhancing the readability of the change history within the
kernel.

Cheers,

Mike Krufky
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/drivers/media/dvb-frontends/cx24117.c b/drivers/media/dvb-frontends/cx24117.c
index 476b422..3c7523b 100644
--- a/drivers/media/dvb-frontends/cx24117.c
+++ b/drivers/media/dvb-frontends/cx24117.c
@@ -129,6 +129,14 @@ 
 #define CX24117_DISEQC_MINI_A (0)
 #define CX24117_DISEQC_MINI_B (1)
 
+/* LNB voltage enable GPIO pins */
+#define CX24117_DEMOD0_LNBDCPIN (1 << 4)
+#define CX24117_DEMOD1_LNBDCPIN (1 << 5)
+
+/* Demod to LNB mapping */
+#define CX24117_DEMOD0_LNB	(1)
+#define CX24117_DEMOD1_LNB	(0)
+
 
 #define CX24117_PNE	(0) /* 0 disabled / 2 enabled */
 #define CX24117_OCC	(1) /* 0 disabled / 1 enabled */
@@ -142,6 +150,8 @@  enum cmds {
 	CMD_LNBSEND     = 0x21, /* Formerly CMD_SEND_DISEQC */
 	CMD_LNBDCLEVEL  = 0x22,
 	CMD_SET_TONE    = 0x23,
+	CMD_GPIODIR	= 0x32,
+	CMD_GPIOOUT	= 0x33,
 	CMD_UPDFWVERS   = 0x35,
 	CMD_TUNERSLEEP  = 0x36,
 };
@@ -891,7 +901,8 @@  static int cx24117_set_voltage(struct dvb_frontend *fe,
 	struct cx24117_state *state = fe->demodulator_priv;
 	struct cx24117_cmd cmd;
 	int ret;
-	u8 reg = (state->demod == 0) ? 0x10 : 0x20;
+	u8 pin = (state->demod == 0) ?
+		CX24117_DEMOD0_LNBDCPIN : CX24117_DEMOD1_LNBDCPIN;
 
 	dev_dbg(&state->priv->i2c->dev, "%s() demod%d %s\n",
 		__func__, state->demod,
@@ -899,26 +910,18 @@  static int cx24117_set_voltage(struct dvb_frontend *fe,
 		voltage == SEC_VOLTAGE_18 ? "SEC_VOLTAGE_18" :
 		"SEC_VOLTAGE_OFF");
 
-	/* CMD 32 */
-	cmd.args[0] = 0x32;
-	cmd.args[1] = reg;
-	cmd.args[2] = reg;
-	cmd.len = 3;
-	ret = cx24117_cmd_execute(fe, &cmd);
-	if (ret)
-		return ret;
-
 	if ((voltage == SEC_VOLTAGE_13) ||
 	    (voltage == SEC_VOLTAGE_18)) {
-		/* CMD 33 */
-		cmd.args[0] = 0x33;
-		cmd.args[1] = reg;
-		cmd.args[2] = reg;
+		/* Turn on LNB DC voltage */
+		cmd.args[0] = CMD_GPIOOUT;
+		cmd.args[1] = pin;	/* level */
+		cmd.args[2] = pin;	/* mask */
 		cmd.len = 3;
 		ret = cx24117_cmd_execute(fe, &cmd);
 		if (ret != 0)
 			return ret;
 
+		/* Wait for any pending diseqc TX */
 		ret = cx24117_wait_for_lnb(fe);
 		if (ret != 0)
 			return ret;
@@ -926,22 +929,25 @@  static int cx24117_set_voltage(struct dvb_frontend *fe,
 		/* Wait for voltage/min repeat delay */
 		msleep(100);
 
-		/* CMD 22 - CMD_LNBDCLEVEL */
+		/* Set DC level (0=13V 1=18V) */
 		cmd.args[0] = CMD_LNBDCLEVEL;
-		cmd.args[1] = state->demod ? 0 : 1;
-		cmd.args[2] = (voltage == SEC_VOLTAGE_18 ? 0x01 : 0x00);
+		cmd.args[1] = (state->demod == 0) ?
+			CX24117_DEMOD0_LNB : CX24117_DEMOD1_LNB;
+		cmd.args[2] = (voltage == SEC_VOLTAGE_18 ? 1 : 0);
 		cmd.len = 3;
+		ret = cx24117_cmd_execute(fe, &cmd);
 
 		/* Min delay time before DiSEqC send */
 		msleep(20);
 	} else {
-		cmd.args[0] = 0x33;
-		cmd.args[1] = 0x00;
-		cmd.args[2] = reg;
+		/* Turn off LNB DC voltage */
+		cmd.args[0] = CMD_GPIOOUT;
+		cmd.args[1] = 0;	/* level */
+		cmd.args[2] = pin;	/* mask */
 		cmd.len = 3;
+		ret = cx24117_cmd_execute(fe, &cmd);
 	}
-
-	return cx24117_cmd_execute(fe, &cmd);
+	return ret;
 }
 
 static int cx24117_set_tone(struct dvb_frontend *fe,
@@ -1260,6 +1266,16 @@  static int cx24117_initfe(struct dvb_frontend *fe)
 	cmd.args[2] = CX24117_OCC;
 	cmd.len = 3;
 	ret = cx24117_cmd_execute_nolock(fe, &cmd);
+	if (ret != 0)
+		goto exit;
+
+	/* Setup cx24117 GPIO direction */
+	/* These pins turn on/off LNB DC voltage */
+	cmd.args[0] = CMD_GPIODIR;
+	cmd.args[1] = CX24117_DEMOD0_LNBDCPIN | CX24117_DEMOD1_LNBDCPIN;
+	cmd.args[2] = CX24117_DEMOD0_LNBDCPIN | CX24117_DEMOD1_LNBDCPIN;
+	cmd.len = 3;
+	ret = cx24117_cmd_execute_nolock(fe, &cmd);
 
 exit:
 	mutex_unlock(&state->priv->fe_lock);