[PATCH 0/3] treewide: remove open coded SMBus block transfers

[PATCH 0/3] treewide: remove open coded SMBus block transfers

[Wolfram Sang]

The bigger picture is that I want to extend the maximum block length for
SMBus block transfers from 32 (SMBus2) to 255 (SMBus3). That needs some
cleanups and refactoring first. To make that easier, it would be helpful
if all in-kernel users would call the helper functions of the I2C core
for SMBus block transfers and not open code it via the generic
smbus_xfer.

This series converts the three users doing that. I don't have the
hardware, so these patches are only build tested. Please let me know
what you think.

Changes since RFC:
* addressed review comments, see individual patches

Wolfram Sang (3):
  media: i2c: adv7842: remove open coded version of SMBus block write
  media: i2c: adv7511: remove open coded version of SMBus block read
  ipmi: remove open coded version of SMBus block write

 drivers/char/ipmi/ipmb_dev_int.c | 24 +++++++++----------
 drivers/media/i2c/adv7511-v4l2.c | 41 ++++++++++++--------------------
 drivers/media/i2c/adv7842.c      | 24 ++++---------------
 3 files changed, 32 insertions(+), 57 deletions(-)

-- 
2.29.2

[PATCH 1/3] media: i2c: adv7842: remove open coded version of SMBus block write

[Wolfram Sang]

The version here is identical to the one in the I2C core, so use the
latter version directly.

Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
---

Changes since RFC:
* skip define, use i2c_smbus_write_i2c_block_data directly

 drivers/media/i2c/adv7842.c | 24 +++++-------------------
 1 file changed, 5 insertions(+), 19 deletions(-)

diff --git a/drivers/media/i2c/adv7842.c b/drivers/media/i2c/adv7842.c
index 0855f648416d..e0629d5ef17b 100644
--- a/drivers/media/i2c/adv7842.c
+++ b/drivers/media/i2c/adv7842.c
@@ -343,20 +343,6 @@ static void adv_smbus_write_byte_no_check(struct i2c_client *client,
 		       I2C_SMBUS_BYTE_DATA, &data);
 }
 
-static s32 adv_smbus_write_i2c_block_data(struct i2c_client *client,
-				  u8 command, unsigned length, const u8 *values)
-{
-	union i2c_smbus_data data;
-
-	if (length > I2C_SMBUS_BLOCK_MAX)
-		length = I2C_SMBUS_BLOCK_MAX;
-	data.block[0] = length;
-	memcpy(data.block + 1, values, length);
-	return i2c_smbus_xfer(client->adapter, client->addr, client->flags,
-			      I2C_SMBUS_WRITE, command,
-			      I2C_SMBUS_I2C_BLOCK_DATA, &data);
-}
-
 /* ----------------------------------------------------------------------- */
 
 static inline int io_read(struct v4l2_subdev *sd, u8 reg)
@@ -741,7 +727,7 @@ static int edid_write_vga_segment(struct v4l2_subdev *sd)
 	rep_write_and_or(sd, 0x77, 0xef, 0x10);
 
 	for (i = 0; !err && i < 256; i += I2C_SMBUS_BLOCK_MAX)
-		err = adv_smbus_write_i2c_block_data(state->i2c_edid, i,
+		err = i2c_smbus_write_i2c_block_data(state->i2c_edid, i,
 					     I2C_SMBUS_BLOCK_MAX, val + i);
 	if (err)
 		return err;
@@ -807,7 +793,7 @@ static int edid_write_hdmi_segment(struct v4l2_subdev *sd, u8 port)
 	rep_write_and_or(sd, 0x77, 0xef, 0x00);
 
 	for (i = 0; !err && i < 256; i += I2C_SMBUS_BLOCK_MAX)
-		err = adv_smbus_write_i2c_block_data(state->i2c_edid, i,
+		err = i2c_smbus_write_i2c_block_data(state->i2c_edid, i,
 						     I2C_SMBUS_BLOCK_MAX, edid + i);
 	if (err)
 		return err;
@@ -1079,7 +1065,7 @@ static void configure_custom_video_timings(struct v4l2_subdev *sd,
 		/* Should only be set in auto-graphics mode [REF_02, p. 91-92] */
 		/* setup PLL_DIV_MAN_EN and PLL_DIV_RATIO */
 		/* IO-map reg. 0x16 and 0x17 should be written in sequence */
-		if (adv_smbus_write_i2c_block_data(client, 0x16, 2, pll)) {
+		if (i2c_smbus_write_i2c_block_data(client, 0x16, 2, pll)) {
 			v4l2_err(sd, "writing to reg 0x16 and 0x17 failed\n");
 			break;
 		}
@@ -1135,7 +1121,7 @@ static void adv7842_set_offset(struct v4l2_subdev *sd, bool auto_offset, u16 off
 	offset_buf[3] = offset_c & 0x0ff;
 
 	/* Registers must be written in this order with no i2c access in between */
-	if (adv_smbus_write_i2c_block_data(state->i2c_cp, 0x77, 4, offset_buf))
+	if (i2c_smbus_write_i2c_block_data(state->i2c_cp, 0x77, 4, offset_buf))
 		v4l2_err(sd, "%s: i2c error writing to CP reg 0x77, 0x78, 0x79, 0x7a\n", __func__);
 }
 
@@ -1164,7 +1150,7 @@ static void adv7842_set_gain(struct v4l2_subdev *sd, bool auto_gain, u16 gain_a,
 	gain_buf[3] = ((gain_c & 0x0ff));
 
 	/* Registers must be written in this order with no i2c access in between */
-	if (adv_smbus_write_i2c_block_data(state->i2c_cp, 0x73, 4, gain_buf))
+	if (i2c_smbus_write_i2c_block_data(state->i2c_cp, 0x73, 4, gain_buf))
 		v4l2_err(sd, "%s: i2c error writing to CP reg 0x73, 0x74, 0x75, 0x76\n", __func__);
 }
 
-- 
2.29.2

[PATCH 2/3] media: i2c: adv7511: remove open coded version of SMBus block read

[Wolfram Sang]

The open coded version differs from the one in the core in one way: the
buffer will be always copied back, even when the transfer failed. Be
more robust: use the block read from the I2C core and propagate a
potential errno further to the sanity checks.

Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
---

Changes since RFC:
* removed wrong assumption from the commit message

 drivers/media/i2c/adv7511-v4l2.c | 41 ++++++++++++--------------------
 1 file changed, 15 insertions(+), 26 deletions(-)

diff --git a/drivers/media/i2c/adv7511-v4l2.c b/drivers/media/i2c/adv7511-v4l2.c
index a3161d709015..441bafa743a6 100644
--- a/drivers/media/i2c/adv7511-v4l2.c
+++ b/drivers/media/i2c/adv7511-v4l2.c
@@ -214,36 +214,25 @@ static inline void adv7511_wr_and_or(struct v4l2_subdev *sd, u8 reg, u8 clr_mask
 	adv7511_wr(sd, reg, (adv7511_rd(sd, reg) & clr_mask) | val_mask);
 }
 
-static int adv_smbus_read_i2c_block_data(struct i2c_client *client,
-					 u8 command, unsigned length, u8 *values)
-{
-	union i2c_smbus_data data;
-	int ret;
-
-	if (length > I2C_SMBUS_BLOCK_MAX)
-		length = I2C_SMBUS_BLOCK_MAX;
-	data.block[0] = length;
-
-	ret = i2c_smbus_xfer(client->adapter, client->addr, client->flags,
-			     I2C_SMBUS_READ, command,
-			     I2C_SMBUS_I2C_BLOCK_DATA, &data);
-	memcpy(values, data.block + 1, length);
-	return ret;
-}
-
-static void adv7511_edid_rd(struct v4l2_subdev *sd, uint16_t len, uint8_t *buf)
+static int adv7511_edid_rd(struct v4l2_subdev *sd, uint16_t len, uint8_t *buf)
 {
 	struct adv7511_state *state = get_adv7511_state(sd);
 	int i;
-	int err = 0;
 
 	v4l2_dbg(1, debug, sd, "%s:\n", __func__);
 
-	for (i = 0; !err && i < len; i += I2C_SMBUS_BLOCK_MAX)
-		err = adv_smbus_read_i2c_block_data(state->i2c_edid, i,
+	for (i = 0; i < len; i += I2C_SMBUS_BLOCK_MAX) {
+		s32 ret;
+
+		ret = i2c_smbus_read_i2c_block_data(state->i2c_edid, i,
 						    I2C_SMBUS_BLOCK_MAX, buf + i);
-	if (err)
-		v4l2_err(sd, "%s: i2c read error\n", __func__);
+		if (ret < 0) {
+			v4l2_err(sd, "%s: i2c read error\n", __func__);
+			return ret;
+		}
+	}
+
+	return 0;
 }
 
 static inline int adv7511_cec_read(struct v4l2_subdev *sd, u8 reg)
@@ -1668,20 +1657,20 @@ static bool adv7511_check_edid_status(struct v4l2_subdev *sd)
 	if (edidRdy & MASK_ADV7511_EDID_RDY) {
 		int segment = adv7511_rd(sd, 0xc4);
 		struct adv7511_edid_detect ed;
+		int err;
 
 		if (segment >= EDID_MAX_SEGM) {
 			v4l2_err(sd, "edid segment number too big\n");
 			return false;
 		}
 		v4l2_dbg(1, debug, sd, "%s: got segment %d\n", __func__, segment);
-		adv7511_edid_rd(sd, 256, &state->edid.data[segment * 256]);
+		err = adv7511_edid_rd(sd, 256, &state->edid.data[segment * 256]);
 		adv7511_dbg_dump_edid(2, debug, sd, segment, &state->edid.data[segment * 256]);
 		if (segment == 0) {
 			state->edid.blocks = state->edid.data[0x7e] + 1;
 			v4l2_dbg(1, debug, sd, "%s: %d blocks in total\n", __func__, state->edid.blocks);
 		}
-		if (!edid_verify_crc(sd, segment) ||
-		    !edid_verify_header(sd, segment)) {
+		if (err < 0 || !edid_verify_crc(sd, segment) || !edid_verify_header(sd, segment)) {
 			/* edid crc error, force reread of edid segment */
 			v4l2_err(sd, "%s: edid crc or header error\n", __func__);
 			state->have_monitor = false;
-- 
2.29.2

[PATCH 3/3] ipmi: remove open coded version of SMBus block write

[Wolfram Sang]

The block-write function of the core was not used because there was no
client-struct to use. However, in this case it seems apropriate to use a
temporary client struct. Because we are answering a request we recieved
when being a client ourselves. So, convert the code to use a temporary
client and use the block-write function of the I2C core.

Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
---

Changes since RFC:
* don't use stack but kmemdup. No complaints from buildbots

 drivers/char/ipmi/ipmb_dev_int.c | 24 ++++++++++++------------
 1 file changed, 12 insertions(+), 12 deletions(-)

diff --git a/drivers/char/ipmi/ipmb_dev_int.c b/drivers/char/ipmi/ipmb_dev_int.c
index 382b28f1cf2f..49b8f22fdcf0 100644
--- a/drivers/char/ipmi/ipmb_dev_int.c
+++ b/drivers/char/ipmi/ipmb_dev_int.c
@@ -137,7 +137,7 @@ static ssize_t ipmb_write(struct file *file, const char __user *buf,
 {
 	struct ipmb_dev *ipmb_dev = to_ipmb_dev(file);
 	u8 rq_sa, netf_rq_lun, msg_len;
-	union i2c_smbus_data data;
+	struct i2c_client *temp_client;
 	u8 msg[MAX_MSG_LEN];
 	ssize_t ret;
 
@@ -160,21 +160,21 @@ static ssize_t ipmb_write(struct file *file, const char __user *buf,
 	}
 
 	/*
-	 * subtract rq_sa and netf_rq_lun from the length of the msg passed to
-	 * i2c_smbus_xfer
+	 * subtract rq_sa and netf_rq_lun from the length of the msg. Fill the
+	 * temporary client. Note that its use is an exception for IPMI.
 	 */
 	msg_len = msg[IPMB_MSG_LEN_IDX] - SMBUS_MSG_HEADER_LENGTH;
-	if (msg_len > I2C_SMBUS_BLOCK_MAX)
-		msg_len = I2C_SMBUS_BLOCK_MAX;
+	temp_client = kmemdup(ipmb_dev->client, sizeof(*temp_client), GFP_KERNEL);
+	if (!temp_client)
+		return -ENOMEM;
+
+	temp_client->addr = rq_sa;
 
-	data.block[0] = msg_len;
-	memcpy(&data.block[1], msg + SMBUS_MSG_IDX_OFFSET, msg_len);
-	ret = i2c_smbus_xfer(ipmb_dev->client->adapter, rq_sa,
-			     ipmb_dev->client->flags,
-			     I2C_SMBUS_WRITE, netf_rq_lun,
-			     I2C_SMBUS_BLOCK_DATA, &data);
+	ret = i2c_smbus_write_block_data(temp_client, netf_rq_lun, msg_len,
+					 msg + SMBUS_MSG_IDX_OFFSET);
+	kfree(temp_client);
 
-	return ret ? : count;
+	return ret < 0 ? ret : count;
 }
 
 static __poll_t ipmb_poll(struct file *file, poll_table *wait)
-- 
2.29.2

Re: [PATCH 1/3] media: i2c: adv7842: remove open coded version of SMBus block write

[Hans Verkuil]

On 19/01/2021 10:39, Wolfram Sang wrote:
> The version here is identical to the one in the I2C core, so use the
> latter version directly.
> 
> Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>

Reviewed-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>

Looks good to me!

	Hans

> ---
> 
> Changes since RFC:
> * skip define, use i2c_smbus_write_i2c_block_data directly
> 
>  drivers/media/i2c/adv7842.c | 24 +++++-------------------
>  1 file changed, 5 insertions(+), 19 deletions(-)
> 
> diff --git a/drivers/media/i2c/adv7842.c b/drivers/media/i2c/adv7842.c
> index 0855f648416d..e0629d5ef17b 100644
> --- a/drivers/media/i2c/adv7842.c
> +++ b/drivers/media/i2c/adv7842.c
> @@ -343,20 +343,6 @@ static void adv_smbus_write_byte_no_check(struct i2c_client *client,
>  		       I2C_SMBUS_BYTE_DATA, &data);
>  }
>  
> -static s32 adv_smbus_write_i2c_block_data(struct i2c_client *client,
> -				  u8 command, unsigned length, const u8 *values)
> -{
> -	union i2c_smbus_data data;
> -
> -	if (length > I2C_SMBUS_BLOCK_MAX)
> -		length = I2C_SMBUS_BLOCK_MAX;
> -	data.block[0] = length;
> -	memcpy(data.block + 1, values, length);
> -	return i2c_smbus_xfer(client->adapter, client->addr, client->flags,
> -			      I2C_SMBUS_WRITE, command,
> -			      I2C_SMBUS_I2C_BLOCK_DATA, &data);
> -}
> -
>  /* ----------------------------------------------------------------------- */
>  
>  static inline int io_read(struct v4l2_subdev *sd, u8 reg)
> @@ -741,7 +727,7 @@ static int edid_write_vga_segment(struct v4l2_subdev *sd)
>  	rep_write_and_or(sd, 0x77, 0xef, 0x10);
>  
>  	for (i = 0; !err && i < 256; i += I2C_SMBUS_BLOCK_MAX)
> -		err = adv_smbus_write_i2c_block_data(state->i2c_edid, i,
> +		err = i2c_smbus_write_i2c_block_data(state->i2c_edid, i,
>  					     I2C_SMBUS_BLOCK_MAX, val + i);
>  	if (err)
>  		return err;
> @@ -807,7 +793,7 @@ static int edid_write_hdmi_segment(struct v4l2_subdev *sd, u8 port)
>  	rep_write_and_or(sd, 0x77, 0xef, 0x00);
>  
>  	for (i = 0; !err && i < 256; i += I2C_SMBUS_BLOCK_MAX)
> -		err = adv_smbus_write_i2c_block_data(state->i2c_edid, i,
> +		err = i2c_smbus_write_i2c_block_data(state->i2c_edid, i,
>  						     I2C_SMBUS_BLOCK_MAX, edid + i);
>  	if (err)
>  		return err;
> @@ -1079,7 +1065,7 @@ static void configure_custom_video_timings(struct v4l2_subdev *sd,
>  		/* Should only be set in auto-graphics mode [REF_02, p. 91-92] */
>  		/* setup PLL_DIV_MAN_EN and PLL_DIV_RATIO */
>  		/* IO-map reg. 0x16 and 0x17 should be written in sequence */
> -		if (adv_smbus_write_i2c_block_data(client, 0x16, 2, pll)) {
> +		if (i2c_smbus_write_i2c_block_data(client, 0x16, 2, pll)) {
>  			v4l2_err(sd, "writing to reg 0x16 and 0x17 failed\n");
>  			break;
>  		}
> @@ -1135,7 +1121,7 @@ static void adv7842_set_offset(struct v4l2_subdev *sd, bool auto_offset, u16 off
>  	offset_buf[3] = offset_c & 0x0ff;
>  
>  	/* Registers must be written in this order with no i2c access in between */
> -	if (adv_smbus_write_i2c_block_data(state->i2c_cp, 0x77, 4, offset_buf))
> +	if (i2c_smbus_write_i2c_block_data(state->i2c_cp, 0x77, 4, offset_buf))
>  		v4l2_err(sd, "%s: i2c error writing to CP reg 0x77, 0x78, 0x79, 0x7a\n", __func__);
>  }
>  
> @@ -1164,7 +1150,7 @@ static void adv7842_set_gain(struct v4l2_subdev *sd, bool auto_gain, u16 gain_a,
>  	gain_buf[3] = ((gain_c & 0x0ff));
>  
>  	/* Registers must be written in this order with no i2c access in between */
> -	if (adv_smbus_write_i2c_block_data(state->i2c_cp, 0x73, 4, gain_buf))
> +	if (i2c_smbus_write_i2c_block_data(state->i2c_cp, 0x73, 4, gain_buf))
>  		v4l2_err(sd, "%s: i2c error writing to CP reg 0x73, 0x74, 0x75, 0x76\n", __func__);
>  }
>  
> 

Re: [PATCH 2/3] media: i2c: adv7511: remove open coded version of SMBus block read

[Hans Verkuil]

On 19/01/2021 10:39, Wolfram Sang wrote:
> The open coded version differs from the one in the core in one way: the
> buffer will be always copied back, even when the transfer failed. Be
> more robust: use the block read from the I2C core and propagate a
> potential errno further to the sanity checks.
> 
> Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
> ---
> 
> Changes since RFC:
> * removed wrong assumption from the commit message
> 
>  drivers/media/i2c/adv7511-v4l2.c | 41 ++++++++++++--------------------
>  1 file changed, 15 insertions(+), 26 deletions(-)
> 
> diff --git a/drivers/media/i2c/adv7511-v4l2.c b/drivers/media/i2c/adv7511-v4l2.c
> index a3161d709015..441bafa743a6 100644
> --- a/drivers/media/i2c/adv7511-v4l2.c
> +++ b/drivers/media/i2c/adv7511-v4l2.c
> @@ -214,36 +214,25 @@ static inline void adv7511_wr_and_or(struct v4l2_subdev *sd, u8 reg, u8 clr_mask
>  	adv7511_wr(sd, reg, (adv7511_rd(sd, reg) & clr_mask) | val_mask);
>  }
>  
> -static int adv_smbus_read_i2c_block_data(struct i2c_client *client,
> -					 u8 command, unsigned length, u8 *values)
> -{
> -	union i2c_smbus_data data;
> -	int ret;
> -
> -	if (length > I2C_SMBUS_BLOCK_MAX)
> -		length = I2C_SMBUS_BLOCK_MAX;
> -	data.block[0] = length;
> -
> -	ret = i2c_smbus_xfer(client->adapter, client->addr, client->flags,
> -			     I2C_SMBUS_READ, command,
> -			     I2C_SMBUS_I2C_BLOCK_DATA, &data);
> -	memcpy(values, data.block + 1, length);
> -	return ret;
> -}
> -
> -static void adv7511_edid_rd(struct v4l2_subdev *sd, uint16_t len, uint8_t *buf)
> +static int adv7511_edid_rd(struct v4l2_subdev *sd, uint16_t len, uint8_t *buf)
>  {
>  	struct adv7511_state *state = get_adv7511_state(sd);
>  	int i;
> -	int err = 0;
>  
>  	v4l2_dbg(1, debug, sd, "%s:\n", __func__);
>  
> -	for (i = 0; !err && i < len; i += I2C_SMBUS_BLOCK_MAX)
> -		err = adv_smbus_read_i2c_block_data(state->i2c_edid, i,
> +	for (i = 0; i < len; i += I2C_SMBUS_BLOCK_MAX) {
> +		s32 ret;
> +
> +		ret = i2c_smbus_read_i2c_block_data(state->i2c_edid, i,
>  						    I2C_SMBUS_BLOCK_MAX, buf + i);
> -	if (err)
> -		v4l2_err(sd, "%s: i2c read error\n", __func__);
> +		if (ret < 0) {
> +			v4l2_err(sd, "%s: i2c read error\n", __func__);
> +			return ret;
> +		}
> +	}
> +
> +	return 0;
>  }
>  
>  static inline int adv7511_cec_read(struct v4l2_subdev *sd, u8 reg)
> @@ -1668,20 +1657,20 @@ static bool adv7511_check_edid_status(struct v4l2_subdev *sd)
>  	if (edidRdy & MASK_ADV7511_EDID_RDY) {
>  		int segment = adv7511_rd(sd, 0xc4);
>  		struct adv7511_edid_detect ed;
> +		int err;
>  
>  		if (segment >= EDID_MAX_SEGM) {
>  			v4l2_err(sd, "edid segment number too big\n");
>  			return false;
>  		}
>  		v4l2_dbg(1, debug, sd, "%s: got segment %d\n", __func__, segment);
> -		adv7511_edid_rd(sd, 256, &state->edid.data[segment * 256]);
> +		err = adv7511_edid_rd(sd, 256, &state->edid.data[segment * 256]);
>  		adv7511_dbg_dump_edid(2, debug, sd, segment, &state->edid.data[segment * 256]);

Only call adv7511_dbg_dump_edid if err >= 0.

>  		if (segment == 0) {

Change condition to: err >= 0 && segment == 0

>  			state->edid.blocks = state->edid.data[0x7e] + 1;
>  			v4l2_dbg(1, debug, sd, "%s: %d blocks in total\n", __func__, state->edid.blocks);
>  		}
> -		if (!edid_verify_crc(sd, segment) ||
> -		    !edid_verify_header(sd, segment)) {
> +		if (err < 0 || !edid_verify_crc(sd, segment) || !edid_verify_header(sd, segment)) {
>  			/* edid crc error, force reread of edid segment */

Hmm, this comment is a bit out of date. Change to:

			/*
			 * Couldn't read EDID or EDID has invalid content.
			 * Force reread of EDID segment.
			 */

>  			v4l2_err(sd, "%s: edid crc or header error\n", __func__);

Only show this message if err >= 0. For err < 0 the adv7511_edid_rd() already
logs an error.

>  			state->have_monitor = false;
> 

Regards,

	Hans

Re: [PATCH 2/3] media: i2c: adv7511: remove open coded version of SMBus block read

[Wolfram Sang]

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

Hi Hans,

> > -		adv7511_edid_rd(sd, 256, &state->edid.data[segment * 256]);
> > +		err = adv7511_edid_rd(sd, 256, &state->edid.data[segment * 256]);
> >  		adv7511_dbg_dump_edid(2, debug, sd, segment, &state->edid.data[segment * 256]);
> 
> Only call adv7511_dbg_dump_edid if err >= 0.

Yes.

> 
> >  		if (segment == 0) {
> 
> Change condition to: err >= 0 && segment == 0

Yes.

> 
> >  			state->edid.blocks = state->edid.data[0x7e] + 1;
> >  			v4l2_dbg(1, debug, sd, "%s: %d blocks in total\n", __func__, state->edid.blocks);
> >  		}

So I guarded the above block with an 'if (!err)' clause.
adv7511_edid_rd() returns either 0 or errno, so we can take the above
simpler condition.



> > -		if (!edid_verify_crc(sd, segment) ||
> > -		    !edid_verify_header(sd, segment)) {
> > +		if (err < 0 || !edid_verify_crc(sd, segment) || !edid_verify_header(sd, segment)) {
> >  			/* edid crc error, force reread of edid segment */
> 
> Hmm, this comment is a bit out of date. Change to:
> 
> 			/*
> 			 * Couldn't read EDID or EDID has invalid content.
> 			 * Force reread of EDID segment.
> 			 */

I updated the comment but kept it a oneliner.

> 
> >  			v4l2_err(sd, "%s: edid crc or header error\n", __func__);
> 
> Only show this message if err >= 0. For err < 0 the adv7511_edid_rd() already
> logs an error.

Yes. I used 'if (!err)' again here.

Will resend in a minute, thanks for the review!

All the best,

   Wolfram


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]