[PATCH 0/5] LG Electronics LGDT3306A DVB frontend driver cleanups

[PATCH 0/5] LG Electronics LGDT3306A DVB frontend driver cleanups

[Sergey Shtylyov]

First, there was Svace (static analysis tool) reporting many assignments
(mainly of the results of the register read/write functions) done but the
resulting values ignored. Then, while looking at the code, I came up with
a few more patches...

The patches are against the next branch of the linuxtv.org/media.git repo
plus this patch (posted earlier this week):

https://lore.kernel.org/all/20260316204455.63426-1-s.shtylyov@auroraos.dev/

Sergey Shtylyov (5):
  media: lgdt3306a: drop useless assignments
  media: lgdt3306a: drop pointless register read in lgdt3306a_set_qam()
  media: lgdt3306a: drop useless variables in lgdt3306a_set_inversion*()
  media: lgdt3306a: use *switch* statement in lgdt3306a_init()
  media: lgdt3306a: streamline lgdt3306a_read_{ber,ucblocks}()

 drivers/media/dvb-frontends/lgdt3306a.c | 138 ++++++++++++------------
 1 file changed, 66 insertions(+), 72 deletions(-)

-- 
2.53.0

[PATCH 1/5] media: lgdt3306a: drop useless assignments

[Sergey Shtylyov]

The driver assigns the result of lgdt3306a_write_reg() (and some other
functions) to the local variable ret but then often completely ignores
its value. Drop such useless assignments...

Found by Linux Verification Center (linuxtesting.org) with the Svace static
analysis tool.

Signed-off-by: Sergey Shtylyov <s.shtylyov@auroraos.dev>
---
 drivers/media/dvb-frontends/lgdt3306a.c | 102 ++++++++++++------------
 1 file changed, 51 insertions(+), 51 deletions(-)

diff --git a/drivers/media/dvb-frontends/lgdt3306a.c b/drivers/media/dvb-frontends/lgdt3306a.c
index 03e598d73d9f..cde9f3edae63 100644
--- a/drivers/media/dvb-frontends/lgdt3306a.c
+++ b/drivers/media/dvb-frontends/lgdt3306a.c
@@ -436,24 +436,24 @@ static int lgdt3306a_set_vsb(struct lgdt3306a_state *state)
 		goto fail;
 
 	/* AICCFIXFREQ0 NT N-1(Video rejection) */
-	ret = lgdt3306a_write_reg(state, 0x002e, 0x00);
-	ret = lgdt3306a_write_reg(state, 0x002f, 0x00);
-	ret = lgdt3306a_write_reg(state, 0x0030, 0x00);
+	lgdt3306a_write_reg(state, 0x002e, 0x00);
+	lgdt3306a_write_reg(state, 0x002f, 0x00);
+	lgdt3306a_write_reg(state, 0x0030, 0x00);
 
 	/* AICCFIXFREQ1 NT N-1(Audio rejection) */
-	ret = lgdt3306a_write_reg(state, 0x002b, 0x00);
-	ret = lgdt3306a_write_reg(state, 0x002c, 0x00);
-	ret = lgdt3306a_write_reg(state, 0x002d, 0x00);
+	lgdt3306a_write_reg(state, 0x002b, 0x00);
+	lgdt3306a_write_reg(state, 0x002c, 0x00);
+	lgdt3306a_write_reg(state, 0x002d, 0x00);
 
 	/* AICCFIXFREQ2 NT Co-Channel(Video rejection) */
-	ret = lgdt3306a_write_reg(state, 0x0028, 0x00);
-	ret = lgdt3306a_write_reg(state, 0x0029, 0x00);
-	ret = lgdt3306a_write_reg(state, 0x002a, 0x00);
+	lgdt3306a_write_reg(state, 0x0028, 0x00);
+	lgdt3306a_write_reg(state, 0x0029, 0x00);
+	lgdt3306a_write_reg(state, 0x002a, 0x00);
 
 	/* AICCFIXFREQ3 NT Co-Channel(Audio rejection) */
-	ret = lgdt3306a_write_reg(state, 0x0025, 0x00);
-	ret = lgdt3306a_write_reg(state, 0x0026, 0x00);
-	ret = lgdt3306a_write_reg(state, 0x0027, 0x00);
+	lgdt3306a_write_reg(state, 0x0025, 0x00);
+	lgdt3306a_write_reg(state, 0x0026, 0x00);
+	lgdt3306a_write_reg(state, 0x0027, 0x00);
 
 #else
 	/* FGR - this works well for HVR-1955,1975 */
@@ -464,24 +464,24 @@ static int lgdt3306a_set_vsb(struct lgdt3306a_state *state)
 		goto fail;
 
 	/* AICCFIXFREQ0 NT N-1(Video rejection) */
-	ret = lgdt3306a_write_reg(state, 0x002e, 0x5A);
-	ret = lgdt3306a_write_reg(state, 0x002f, 0x00);
-	ret = lgdt3306a_write_reg(state, 0x0030, 0x00);
+	lgdt3306a_write_reg(state, 0x002e, 0x5A);
+	lgdt3306a_write_reg(state, 0x002f, 0x00);
+	lgdt3306a_write_reg(state, 0x0030, 0x00);
 
 	/* AICCFIXFREQ1 NT N-1(Audio rejection) */
-	ret = lgdt3306a_write_reg(state, 0x002b, 0x36);
-	ret = lgdt3306a_write_reg(state, 0x002c, 0x00);
-	ret = lgdt3306a_write_reg(state, 0x002d, 0x00);
+	lgdt3306a_write_reg(state, 0x002b, 0x36);
+	lgdt3306a_write_reg(state, 0x002c, 0x00);
+	lgdt3306a_write_reg(state, 0x002d, 0x00);
 
 	/* AICCFIXFREQ2 NT Co-Channel(Video rejection) */
-	ret = lgdt3306a_write_reg(state, 0x0028, 0x2A);
-	ret = lgdt3306a_write_reg(state, 0x0029, 0x00);
-	ret = lgdt3306a_write_reg(state, 0x002a, 0x00);
+	lgdt3306a_write_reg(state, 0x0028, 0x2A);
+	lgdt3306a_write_reg(state, 0x0029, 0x00);
+	lgdt3306a_write_reg(state, 0x002a, 0x00);
 
 	/* AICCFIXFREQ3 NT Co-Channel(Audio rejection) */
-	ret = lgdt3306a_write_reg(state, 0x0025, 0x06);
-	ret = lgdt3306a_write_reg(state, 0x0026, 0x00);
-	ret = lgdt3306a_write_reg(state, 0x0027, 0x00);
+	lgdt3306a_write_reg(state, 0x0025, 0x06);
+	lgdt3306a_write_reg(state, 0x0026, 0x00);
+	lgdt3306a_write_reg(state, 0x0027, 0x00);
 #endif
 
 	ret = lgdt3306a_read_reg(state, 0x001e, &val);
@@ -489,72 +489,72 @@ static int lgdt3306a_set_vsb(struct lgdt3306a_state *state)
 		goto fail;
 	val &= 0x0f;
 	val |= 0xa0;
-	ret = lgdt3306a_write_reg(state, 0x001e, val);
+	lgdt3306a_write_reg(state, 0x001e, val);
 
-	ret = lgdt3306a_write_reg(state, 0x0022, 0x08);
+	lgdt3306a_write_reg(state, 0x0022, 0x08);
 
-	ret = lgdt3306a_write_reg(state, 0x0023, 0xFF);
+	lgdt3306a_write_reg(state, 0x0023, 0xFF);
 
 	ret = lgdt3306a_read_reg(state, 0x211f, &val);
 	if (lg_chkerr(ret))
 		goto fail;
 	val &= 0xef;
-	ret = lgdt3306a_write_reg(state, 0x211f, val);
+	lgdt3306a_write_reg(state, 0x211f, val);
 
-	ret = lgdt3306a_write_reg(state, 0x2173, 0x01);
+	lgdt3306a_write_reg(state, 0x2173, 0x01);
 
 	ret = lgdt3306a_read_reg(state, 0x1061, &val);
 	if (lg_chkerr(ret))
 		goto fail;
 	val &= 0xf8;
 	val |= 0x04;
-	ret = lgdt3306a_write_reg(state, 0x1061, val);
+	lgdt3306a_write_reg(state, 0x1061, val);
 
 	ret = lgdt3306a_read_reg(state, 0x103d, &val);
 	if (lg_chkerr(ret))
 		goto fail;
 	val &= 0xcf;
-	ret = lgdt3306a_write_reg(state, 0x103d, val);
+	lgdt3306a_write_reg(state, 0x103d, val);
 
-	ret = lgdt3306a_write_reg(state, 0x2122, 0x40);
+	lgdt3306a_write_reg(state, 0x2122, 0x40);
 
 	ret = lgdt3306a_read_reg(state, 0x2141, &val);
 	if (lg_chkerr(ret))
 		goto fail;
 	val &= 0x3f;
-	ret = lgdt3306a_write_reg(state, 0x2141, val);
+	lgdt3306a_write_reg(state, 0x2141, val);
 
 	ret = lgdt3306a_read_reg(state, 0x2135, &val);
 	if (lg_chkerr(ret))
 		goto fail;
 	val &= 0x0f;
 	val |= 0x70;
-	ret = lgdt3306a_write_reg(state, 0x2135, val);
+	lgdt3306a_write_reg(state, 0x2135, val);
 
 	ret = lgdt3306a_read_reg(state, 0x0003, &val);
 	if (lg_chkerr(ret))
 		goto fail;
 	val &= 0xf7;
-	ret = lgdt3306a_write_reg(state, 0x0003, val);
+	lgdt3306a_write_reg(state, 0x0003, val);
 
 	ret = lgdt3306a_read_reg(state, 0x001c, &val);
 	if (lg_chkerr(ret))
 		goto fail;
 	val &= 0x7f;
-	ret = lgdt3306a_write_reg(state, 0x001c, val);
+	lgdt3306a_write_reg(state, 0x001c, val);
 
 	/* 6. EQ step size */
 	ret = lgdt3306a_read_reg(state, 0x2179, &val);
 	if (lg_chkerr(ret))
 		goto fail;
 	val &= 0xf8;
-	ret = lgdt3306a_write_reg(state, 0x2179, val);
+	lgdt3306a_write_reg(state, 0x2179, val);
 
 	ret = lgdt3306a_read_reg(state, 0x217a, &val);
 	if (lg_chkerr(ret))
 		goto fail;
 	val &= 0xf8;
-	ret = lgdt3306a_write_reg(state, 0x217a, val);
+	lgdt3306a_write_reg(state, 0x217a, val);
 
 	/* 7. Reset */
 	ret = lgdt3306a_soft_reset(state);
@@ -959,16 +959,16 @@ static int lgdt3306a_init(struct dvb_frontend *fe)
 		pr_err("Bad xtalMHz=%d\n", state->cfg->xtalMHz);
 	}
 #if 0
-	ret = lgdt3306a_write_reg(state, 0x000e, 0x00);
-	ret = lgdt3306a_write_reg(state, 0x000f, 0x00);
+	lgdt3306a_write_reg(state, 0x000e, 0x00);
+	lgdt3306a_write_reg(state, 0x000f, 0x00);
 #endif
 
 	/* 9. Center frequency of input signal of ADC */
-	ret = lgdt3306a_write_reg(state, 0x0010, 0x34); /* 3.25MHz */
-	ret = lgdt3306a_write_reg(state, 0x0011, 0x00);
+	lgdt3306a_write_reg(state, 0x0010, 0x34); /* 3.25MHz */
+	lgdt3306a_write_reg(state, 0x0011, 0x00);
 
 	/* 10. Fixed gain error value */
-	ret = lgdt3306a_write_reg(state, 0x0014, 0); /* gain error=0 */
+	lgdt3306a_write_reg(state, 0x0014, 0); /* gain error=0 */
 
 	/* 10a. VSB TR BW gear shift initial step */
 	ret = lgdt3306a_read_reg(state, 0x103c, &val);
@@ -976,7 +976,7 @@ static int lgdt3306a_init(struct dvb_frontend *fe)
 		goto fail;
 	val &= 0x0f;
 	val |= 0x20; /* SAMGSAUTOSTL_V[3:0] = 2 */
-	ret = lgdt3306a_write_reg(state, 0x103c, val);
+	lgdt3306a_write_reg(state, 0x103c, val);
 
 	/* 10b. Timing offset calibration in low temperature for VSB */
 	ret = lgdt3306a_read_reg(state, 0x103d, &val);
@@ -984,7 +984,7 @@ static int lgdt3306a_init(struct dvb_frontend *fe)
 		goto fail;
 	val &= 0xfc;
 	val |= 0x03;
-	ret = lgdt3306a_write_reg(state, 0x103d, val);
+	lgdt3306a_write_reg(state, 0x103d, val);
 
 	/* 10c. Timing offset calibration in low temperature for QAM */
 	ret = lgdt3306a_read_reg(state, 0x1036, &val);
@@ -992,30 +992,30 @@ static int lgdt3306a_init(struct dvb_frontend *fe)
 		goto fail;
 	val &= 0xf0;
 	val |= 0x0c;
-	ret = lgdt3306a_write_reg(state, 0x1036, val);
+	lgdt3306a_write_reg(state, 0x1036, val);
 
 	/* 11. Using the imaginary part of CIR in CIR loading */
 	ret = lgdt3306a_read_reg(state, 0x211f, &val);
 	if (lg_chkerr(ret))
 		goto fail;
 	val &= 0xef; /* do not use imaginary of CIR */
-	ret = lgdt3306a_write_reg(state, 0x211f, val);
+	lgdt3306a_write_reg(state, 0x211f, val);
 
 	/* 12. Control of no signal detector function */
 	ret = lgdt3306a_read_reg(state, 0x2849, &val);
 	if (lg_chkerr(ret))
 		goto fail;
 	val &= 0xef; /* NOUSENOSIGDET=0, enable no signal detector */
-	ret = lgdt3306a_write_reg(state, 0x2849, val);
+	lgdt3306a_write_reg(state, 0x2849, val);
 
 	/* FGR - put demod in some known mode */
-	ret = lgdt3306a_set_vsb(state);
+	lgdt3306a_set_vsb(state);
 
 	/* 13. TP stream format */
-	ret = lgdt3306a_mpeg_mode(state, state->cfg->mpeg_mode);
+	lgdt3306a_mpeg_mode(state, state->cfg->mpeg_mode);
 
 	/* 14. disable output buses */
-	ret = lgdt3306a_mpeg_tristate(state, 1);
+	lgdt3306a_mpeg_tristate(state, 1);
 
 	/* 15. Sleep (in reset) */
 	ret = lgdt3306a_sleep(state);
-- 
2.53.0

[PATCH 2/5] media: lgdt3306a: drop pointless register read in lgdt3306a_set_qam()

[Sergey Shtylyov]

In lgdt3306a_set_qam(), the driver reads register 0x0024 but after that it
ANDs the value read with 0 which renders the read pointless (I don't think
it has any side effect). Drop the pointless read, just writing 0x00 to the
register...

Signed-off-by: Sergey Shtylyov <s.shtylyov@auroraos.dev>
---
 drivers/media/dvb-frontends/lgdt3306a.c | 6 +-----
 1 file changed, 1 insertion(+), 5 deletions(-)

diff --git a/drivers/media/dvb-frontends/lgdt3306a.c b/drivers/media/dvb-frontends/lgdt3306a.c
index cde9f3edae63..458ff198e659 100644
--- a/drivers/media/dvb-frontends/lgdt3306a.c
+++ b/drivers/media/dvb-frontends/lgdt3306a.c
@@ -637,11 +637,7 @@ static int lgdt3306a_set_qam(struct lgdt3306a_state *state, int modulation)
 		goto fail;
 
 	/* 5. No AICC operation in QAM mode */
-	ret = lgdt3306a_read_reg(state, 0x0024, &val);
-	if (lg_chkerr(ret))
-		goto fail;
-	val &= 0x00;
-	ret = lgdt3306a_write_reg(state, 0x0024, val);
+	ret = lgdt3306a_write_reg(state, 0x0024, 0x00);
 	if (lg_chkerr(ret))
 		goto fail;
 
-- 
2.53.0

[PATCH 3/5] media: lgdt3306a: drop useless variables in lgdt3306a_set_inversion*()

[Sergey Shtylyov]

In lgdt3306a_set_inversion{,_auto}(), the local variables serve little
purpose -- get rid of them. While at it, fix up the indentation of the
functions' parameters...

Signed-off-by: Sergey Shtylyov <s.shtylyov@auroraos.dev>
---
 drivers/media/dvb-frontends/lgdt3306a.c | 14 ++++----------
 1 file changed, 4 insertions(+), 10 deletions(-)

diff --git a/drivers/media/dvb-frontends/lgdt3306a.c b/drivers/media/dvb-frontends/lgdt3306a.c
index 458ff198e659..d586f9ca6a86 100644
--- a/drivers/media/dvb-frontends/lgdt3306a.c
+++ b/drivers/media/dvb-frontends/lgdt3306a.c
@@ -731,26 +731,20 @@ static int lgdt3306a_agc_setup(struct lgdt3306a_state *state,
 /* ------------------------------------------------------------------------ */
 
 static int lgdt3306a_set_inversion(struct lgdt3306a_state *state,
-				       int inversion)
+				   int inversion)
 {
-	int ret;
-
 	dbg_info("(%d)\n", inversion);
 
-	ret = lgdt3306a_set_reg_bit(state, 0x0002, 2, inversion ? 1 : 0);
-	return ret;
+	return lgdt3306a_set_reg_bit(state, 0x0002, 2, inversion ? 1 : 0);
 }
 
 static int lgdt3306a_set_inversion_auto(struct lgdt3306a_state *state,
-				       int enabled)
+					int enabled)
 {
-	int ret;
-
 	dbg_info("(%d)\n", enabled);
 
 	/* 0=Manual 1=Auto(QAM only) - SPECINVAUTO=0x04 */
-	ret = lgdt3306a_set_reg_bit(state, 0x0002, 3, enabled);
-	return ret;
+	return lgdt3306a_set_reg_bit(state, 0x0002, 3, enabled);
 }
 
 static int lgdt3306a_set_if(struct lgdt3306a_state *state,
-- 
2.53.0

[PATCH 4/5] media: lgdt3306a: use *switch* statement in lgdt3306a_init()

[Sergey Shtylyov]

In lgdt3306a_init(), there's a chain of state->cfg->xtalMHz comparisons --
the *switch* statement clearly fits better there...

Signed-off-by: Sergey Shtylyov <s.shtylyov@auroraos.dev>
---
 drivers/media/dvb-frontends/lgdt3306a.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/drivers/media/dvb-frontends/lgdt3306a.c b/drivers/media/dvb-frontends/lgdt3306a.c
index d586f9ca6a86..98af9c0f3757 100644
--- a/drivers/media/dvb-frontends/lgdt3306a.c
+++ b/drivers/media/dvb-frontends/lgdt3306a.c
@@ -898,7 +898,8 @@ static int lgdt3306a_init(struct dvb_frontend *fe)
 	if (lg_chkerr(ret))
 		goto fail;
 
-	if (state->cfg->xtalMHz == 24) {	/* 24MHz */
+	switch (state->cfg->xtalMHz) {
+	case 24: /* 24MHz */
 		/* 7. Frequency for PLL output(0x2564 for 192MHz for 24MHz) */
 		ret = lgdt3306a_read_reg(state, 0x0005, &val);
 		if (lg_chkerr(ret))
@@ -921,8 +922,8 @@ static int lgdt3306a_init(struct dvb_frontend *fe)
 		ret = lgdt3306a_write_reg(state, 0x000d, val);
 		if (lg_chkerr(ret))
 			goto fail;
-
-	} else if (state->cfg->xtalMHz == 25) { /* 25MHz */
+		break;
+	case 25: /* 25MHz */
 		/* 7. Frequency for PLL output */
 		ret = lgdt3306a_read_reg(state, 0x0005, &val);
 		if (lg_chkerr(ret))
@@ -945,7 +946,8 @@ static int lgdt3306a_init(struct dvb_frontend *fe)
 		ret = lgdt3306a_write_reg(state, 0x000d, val);
 		if (lg_chkerr(ret))
 			goto fail;
-	} else {
+		break;
+	default:
 		pr_err("Bad xtalMHz=%d\n", state->cfg->xtalMHz);
 	}
 #if 0
-- 
2.53.0

[PATCH 5/5] media: lgdt3306a: streamline lgdt3306a_read_{ber,ucblocks}()

[Sergey Shtylyov]

In lgdt3306a_read_{ber,ucblocks}(), there's writing of 0 at the passed
{ber,ucblocks} pointer before writing the real value (behind #if 1) --
this makes little sense, and the 1st write gets discarded by gcc when
generating object code.  Enclosing the zero-write with #if 0 and then
replacing #if 1 with #else makes a lot more sense...

Signed-off-by: Sergey Shtylyov <s.shtylyov@auroraos.dev>
---
 drivers/media/dvb-frontends/lgdt3306a.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/media/dvb-frontends/lgdt3306a.c b/drivers/media/dvb-frontends/lgdt3306a.c
index 98af9c0f3757..c118ab36c343 100644
--- a/drivers/media/dvb-frontends/lgdt3306a.c
+++ b/drivers/media/dvb-frontends/lgdt3306a.c
@@ -1745,8 +1745,9 @@ static int lgdt3306a_read_ber(struct dvb_frontend *fe, u32 *ber)
 	struct lgdt3306a_state *state = fe->demodulator_priv;
 	u32 tmp;
 
+#if 0
 	*ber = 0;
-#if 1
+#else
 	/* FGR - FIXME - I don't know what value is expected by dvb_core
 	 * what is the scale of the value?? */
 	tmp =              read_reg(state, 0x00fc); /* NBERVALUE[24-31] */
@@ -1763,8 +1764,9 @@ static int lgdt3306a_read_ucblocks(struct dvb_frontend *fe, u32 *ucblocks)
 {
 	struct lgdt3306a_state *state = fe->demodulator_priv;
 
+#if 0
 	*ucblocks = 0;
-#if 1
+#else
 	/* FGR - FIXME - I don't know what value is expected by dvb_core
 	 * what happens when value wraps? */
 	*ucblocks = read_reg(state, 0x00f4); /* TPIFTPERRCNT[0-7] */
-- 
2.53.0

Re: [0/5] LG Electronics LGDT3306A DVB frontend driver cleanups

[Sergey Shtylyov]

On 3/22/26 12:04 AM, Patchwork Integration wrote:

> Thanks for your patches! Unfortunately the Media CI robot has not been
> able to test them.
> 
> Make sure that the whole series 20260321190143.21123-1-s.shtylyov@auroraos.dev is
> available at lore. And that it can be cherry-picked on top the "next"
> branch of "https://gitlab.freedesktop.org/linux-media/media-committers.git".

   I have explicitly said in the cover letter that this series of cleanups was done
atop of this fix [0]. I now realize that I should have probably added [RFC] to the
subjects...

[0] https://lore.kernel.org/all/20260316204455.63426-1-s.shtylyov@auroraos.dev/

[...]

MBR, Sergey