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@xxxxxxxxx>

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