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

STALE2020d

Revision rfc of 2 in this series.

9 messages, 2 authors, 2021-01-18 · open the first message on its own page

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

From: Wolfram Sang <wsa+renesas@sang-engineering.com>
Date: 2021-01-12 16:42:36

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.


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

 drivers/char/ipmi/ipmb_dev_int.c | 21 +++++++----------
 drivers/media/i2c/adv7511-v4l2.c | 40 +++++++++++---------------------
 drivers/media/i2c/adv7842.c      | 14 +----------
 3 files changed, 23 insertions(+), 52 deletions(-)

-- 
2.29.2

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

From: Wolfram Sang <wsa+renesas@sang-engineering.com>
Date: 2021-01-12 16:42:36

The version here is identical to the one in the I2C core, so use a
define to keep the original name within the driver but call the I2C core
function instead.

Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
---
 drivers/media/i2c/adv7842.c | 14 +-------------
 1 file changed, 1 insertion(+), 13 deletions(-)
diff --git a/drivers/media/i2c/adv7842.c b/drivers/media/i2c/adv7842.c
index 0855f648416d..6ed6bcd1d64d 100644
--- a/drivers/media/i2c/adv7842.c
+++ b/drivers/media/i2c/adv7842.c
@@ -343,19 +343,7 @@ 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);
-}
+#define adv_smbus_write_i2c_block_data i2c_smbus_write_i2c_block_data
 
 /* ----------------------------------------------------------------------- */
 
-- 
2.29.2

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

From: Hans Verkuil <hidden>
Date: 2021-01-18 10:20:11

On 12/01/2021 17:41, Wolfram Sang wrote:
quoted hunk
The version here is identical to the one in the I2C core, so use a
define to keep the original name within the driver but call the I2C core
function instead.

Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
---
 drivers/media/i2c/adv7842.c | 14 +-------------
 1 file changed, 1 insertion(+), 13 deletions(-)
diff --git a/drivers/media/i2c/adv7842.c b/drivers/media/i2c/adv7842.c
index 0855f648416d..6ed6bcd1d64d 100644
--- a/drivers/media/i2c/adv7842.c
+++ b/drivers/media/i2c/adv7842.c
@@ -343,19 +343,7 @@ 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);
-}
+#define adv_smbus_write_i2c_block_data i2c_smbus_write_i2c_block_data
I would prefer it if the driver just uses i2c_smbus_write_i2c_block_data
directly instead of relying on this define. It's used only 5 times, so
it should be a trivial change.

Regards,

	Hans
 
 /* ----------------------------------------------------------------------- */
 

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

From: Wolfram Sang <wsa+renesas@sang-engineering.com>
Date: 2021-01-18 09:47:31

quoted
+#define adv_smbus_write_i2c_block_data i2c_smbus_write_i2c_block_data
I would prefer it if the driver just uses i2c_smbus_write_i2c_block_data
directly instead of relying on this define. It's used only 5 times, so
it should be a trivial change.
Okay, will change it!

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

From: Wolfram Sang <wsa+renesas@sang-engineering.com>
Date: 2021-01-12 16:42:37

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. It
looks like it is expected that the sanity check for a correct CRC and
header will bail out later.

Use the block read from the I2C core and propagate a potential errno
further to the sanity check.

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

Note: we could now make the error checking even stronger by checking if
the number of received bytes is I2C_SMBUS_BLOCK_MAX. But to avoid
regressions, I kept the logic as is, i.e. only check for errno.

 drivers/media/i2c/adv7511-v4l2.c | 40 +++++++++++---------------------
 1 file changed, 14 insertions(+), 26 deletions(-)
diff --git a/drivers/media/i2c/adv7511-v4l2.c b/drivers/media/i2c/adv7511-v4l2.c
index a3161d709015..0150f76dc6a6 100644
--- a/drivers/media/i2c/adv7511-v4l2.c
+++ b/drivers/media/i2c/adv7511-v4l2.c
@@ -214,36 +214,24 @@ 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);
+	s32 len;
 	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) {
+		len = 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 (len < 0) {
+			v4l2_err(sd, "%s: i2c read error\n", __func__);
+			return len;
+		}
+	}
+
+	return 0;
 }
 
 static inline int adv7511_cec_read(struct v4l2_subdev *sd, u8 reg)
@@ -1668,20 +1656,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

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

From: Wolfram Sang <wsa+renesas@sang-engineering.com>
Date: 2021-01-12 16:45:22

On Tue, Jan 12, 2021 at 05:41:28PM +0100, 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. It
looks like it is expected that the sanity check for a correct CRC and
header will bail out later.

Use the block read from the I2C core and propagate a potential errno
further to the sanity check.

Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
$subject should say "adv7511", sorry!

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

From: Wolfram Sang <wsa+renesas@sang-engineering.com>
Date: 2021-01-12 19:42:31

+static int adv7511_edid_rd(struct v4l2_subdev *sd, uint16_t len, uint8_t *buf)
 {
 	struct adv7511_state *state = get_adv7511_state(sd);
+	s32 len;
And 'len' here shadows the function argument :( Ok, I need to resend
this patch. Still, looking forward to a comment if an approach like this
is acceptable.

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

From: Hans Verkuil <hidden>
Date: 2021-01-18 09:39:03

Hi Wolfram,

On 12/01/2021 17:41, 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. It
looks like it is expected that the sanity check for a correct CRC and
header will bail out later.
Nah, it's just a bug. It should have returned and checked the error code,
so your patch does the right thing.

Regards,

	Hans
quoted hunk
Use the block read from the I2C core and propagate a potential errno
further to the sanity check.

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

Note: we could now make the error checking even stronger by checking if
the number of received bytes is I2C_SMBUS_BLOCK_MAX. But to avoid
regressions, I kept the logic as is, i.e. only check for errno.

 drivers/media/i2c/adv7511-v4l2.c | 40 +++++++++++---------------------
 1 file changed, 14 insertions(+), 26 deletions(-)
diff --git a/drivers/media/i2c/adv7511-v4l2.c b/drivers/media/i2c/adv7511-v4l2.c
index a3161d709015..0150f76dc6a6 100644
--- a/drivers/media/i2c/adv7511-v4l2.c
+++ b/drivers/media/i2c/adv7511-v4l2.c
@@ -214,36 +214,24 @@ 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);
+	s32 len;
 	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) {
+		len = 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 (len < 0) {
+			v4l2_err(sd, "%s: i2c read error\n", __func__);
+			return len;
+		}
+	}
+
+	return 0;
 }
 
 static inline int adv7511_cec_read(struct v4l2_subdev *sd, u8 reg)
@@ -1668,20 +1656,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;

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

From: Wolfram Sang <wsa+renesas@sang-engineering.com>
Date: 2021-01-18 09:47:31

Hello Hans,

I hope you are well!
quoted
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. It
looks like it is expected that the sanity check for a correct CRC and
header will bail out later.
Nah, it's just a bug. It should have returned and checked the error code,
so your patch does the right thing.
I see. So, I will only update the commit message.

Thanks for the reviews!

All the best,

   Wolfram
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help