[PATCH] staging: media: av7110: fix sp8870 initialization and hardware communication

[PATCH] staging: media: av7110: fix sp8870 initialization and hardware communication

[Punnay299]

Address multiple correctness issues in the sp8870 demodulator:
- Fix BER reporting in sp8870_read_ber() by correctly ORing the upper bits.
- Set state->initialised only after successful firmware upload in sp8870_init() to allow recovery from transient I2C failures.
- Catch partial I2C transfers (err == 0) in sp8870_firmware_upload() and return -EREMOTEIO to prevent silent firmware corruption.

Signed-off-by: Punnay299 <punnaysharma805@gmail.com>
---
 drivers/staging/media/av7110/sp8870.c | 16 ++++++++++------
 1 file changed, 10 insertions(+), 6 deletions(-)

diff --git a/drivers/staging/media/av7110/sp8870.c b/drivers/staging/media/av7110/sp8870.c
index 29fb4934c..def2a7f0c 100644
--- a/drivers/staging/media/av7110/sp8870.c
+++ b/drivers/staging/media/av7110/sp8870.c
@@ -68,7 +68,8 @@ static int sp8870_writereg(struct sp8870_state *state, u16 reg, u16 data)
 
 	err = i2c_transfer(state->i2c, &msg, 1);
 	if (err != 1) {
-		dprintk("writereg error (err == %i, reg == 0x%02x, data == 0x%02x)\n", err, reg, data);
+		dprintk("writereg error (err == %i, reg == 0x%02x, data == 0x%02x)\n",
+			err, reg, data);
 		return -EREMOTEIO;
 	}
 
@@ -135,7 +136,7 @@ static int sp8870_firmware_upload(struct sp8870_state *state, const struct firmw
 		if (err != 1) {
 			pr_err("%s(): firmware upload failed!\n", __func__);
 			pr_err("%s(): i2c error (err == %i)\n", __func__, err);
-			return err;
+			return err < 0 ? err : -EREMOTEIO;
 		}
 		fw_pos += tx_len;
 	}
@@ -315,7 +316,6 @@ static int sp8870_init(struct dvb_frontend *fe)
 	sp8870_wake_up(state);
 	if (state->initialised)
 		return 0;
-	state->initialised = 1;
 
 	dprintk("initialising frontend...\n");
 
@@ -353,6 +353,8 @@ static int sp8870_init(struct dvb_frontend *fe)
 	sp8870_writereg(state, 0x0D00, 0x010);
 	sp8870_writereg(state, 0x0D01, 0x000);
 
+	state->initialised = 1;
+
 	return 0;
 }
 
@@ -401,7 +403,7 @@ static int sp8870_read_ber(struct dvb_frontend *fe, u32 *ber)
 	if (ret < 0)
 		return -EIO;
 
-	tmp = ret << 6;
+	tmp |= ret << 6;
 	if (tmp >= 0x3FFF0)
 		tmp = ~0;
 
@@ -511,7 +513,8 @@ static int sp8870_set_frontend(struct dvb_frontend *fe)
 		if (valid) {
 			if (trials > 1) {
 				pr_info("%s(): firmware lockup!!!\n", __func__);
-				pr_info("%s(): recovered after %i trial(s))\n",  __func__, trials - 1);
+				pr_info("%s(): recovered after %i trial(s))\n",
+					__func__, trials - 1);
 				lockups++;
 			}
 		}
@@ -530,7 +533,8 @@ static int sp8870_sleep(struct dvb_frontend *fe)
 	return sp8870_writereg(state, 0xC18, 0x000);
 }
 
-static int sp8870_get_tune_settings(struct dvb_frontend *fe, struct dvb_frontend_tune_settings *fesettings)
+static int sp8870_get_tune_settings(struct dvb_frontend *fe,
+				    struct dvb_frontend_tune_settings *fesettings)
 {
 	fesettings->min_delay_ms = 350;
 	fesettings->step_size = 0;
-- 
2.55.0

Re: [PATCH] staging: media: av7110: fix sp8870 initialization and hardware communication

[Dan Carpenter]

On Fri, Aug 07, 2026 at 09:31:49PM +0530, Punnay299 wrote:
> Address multiple correctness issues in the sp8870 demodulator:
> - Fix BER reporting in sp8870_read_ber() by correctly ORing the upper bits.
> - Set state->initialised only after successful firmware upload in sp8870_init() to allow recovery from transient I2C failures.

The AI is too vague on this.  It needs to be spelled out more
specifically.

> - Catch partial I2C transfers (err == 0) in sp8870_firmware_upload() and return -EREMOTEIO to prevent silent firmware corruption.

Run your patch through checkpatch.
Do one thing per patch instead of three unrelated things.
If it's a real bug it needs a Fixes tag.

> 
> Signed-off-by: Punnay299 <punnaysharma805@gmail.com>

Use your real name.

> ---
>  drivers/staging/media/av7110/sp8870.c | 16 ++++++++++------
>  1 file changed, 10 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/staging/media/av7110/sp8870.c b/drivers/staging/media/av7110/sp8870.c
> index 29fb4934c..def2a7f0c 100644
> --- a/drivers/staging/media/av7110/sp8870.c
> +++ b/drivers/staging/media/av7110/sp8870.c
> @@ -68,7 +68,8 @@ static int sp8870_writereg(struct sp8870_state *state, u16 reg, u16 data)
>  
>  	err = i2c_transfer(state->i2c, &msg, 1);
>  	if (err != 1) {
> -		dprintk("writereg error (err == %i, reg == 0x%02x, data == 0x%02x)\n", err, reg, data);
> +		dprintk("writereg error (err == %i, reg == 0x%02x, data == 0x%02x)\n",
> +			err, reg, data);

Don't make unrelated whitespace changes.

>  		return -EREMOTEIO;
>  	}
>  
> @@ -135,7 +136,7 @@ static int sp8870_firmware_upload(struct sp8870_state *state, const struct firmw
>  		if (err != 1) {
>  			pr_err("%s(): firmware upload failed!\n", __func__);
>  			pr_err("%s(): i2c error (err == %i)\n", __func__, err);
> -			return err;
> +			return err < 0 ? err : -EREMOTEIO;

I'm pretty sure i2c_transfer() can't return zero in this case.
It feels like this would be a bug in i2c_transfer().

>  		}
>  		fw_pos += tx_len;
>  	}
> @@ -315,7 +316,6 @@ static int sp8870_init(struct dvb_frontend *fe)
>  	sp8870_wake_up(state);
>  	if (state->initialised)
>  		return 0;
> -	state->initialised = 1;
>  
>  	dprintk("initialising frontend...\n");
>  
> @@ -353,6 +353,8 @@ static int sp8870_init(struct dvb_frontend *fe)
>  	sp8870_writereg(state, 0x0D00, 0x010);
>  	sp8870_writereg(state, 0x0D01, 0x000);
>  
> +	state->initialised = 1;
> +
>  	return 0;
>  }
>  
> @@ -401,7 +403,7 @@ static int sp8870_read_ber(struct dvb_frontend *fe, u32 *ber)
>  	if (ret < 0)
>  		return -EIO;
>  
> -	tmp = ret << 6;
> +	tmp |= ret << 6;
>  	if (tmp >= 0x3FFF0)
>  		tmp = ~0;
>  

This feels like it probably is a Fix.  It would be better if it could
be tested.

regards,
dan carpenter