usb: typec: tps6598x: handle block reads separately with plain-I2C adapters

PATCH New Heikki Krogerus 5 participants 25 messages Apr 25, 2018 – Feb 21, 2019

Subsystems (2)

USB USB Type-C class

lore

Share & tools

lore

Heikki Krogerus

author maintainer

2018-04-25 14:22 UTC #0 lore raw

PATCH v2 1/1 usb: typec: tps6598x: handle block reads separately with plain-I2C adapters

If the I2C adapter that the PD controller is attached to does not support SMBus protocol, the driver needs to handle block reads separately. The first byte returned in block read protocol will show the total number of bytes. It needs to be stripped away.

This is handled separately in the driver only because right now we have no way of requesting the used protocol with regmap-i2c. This is in practice a workaround for what is really a problem in regmap-i2c. The other option would have been to register custom regmap, or not use regmap at all, however, since the solution is very simple, I choose to use it in this case for convenience. It is easy to remove once we figure out how to handle this kind of cases in regmap-i2c.

Fixes 0a4c005bd171 ("usb: typec: driver for TI TPS6598x USB Power Delivery controllers")

Reviewed-by Guenter Roeck <[email protected]>

Signed-off-by Heikki Krogerus <[email protected]>

---
Changed since v1:
- Added sanity check
- Fixed also 2 byte reads

---
 drivers/usb/typec/tps6598x.c | 47 ++++++++++++++++++++++++++++++------
 1 file changed, 39 insertions(+), 8 deletions(-)

drivers/usb/typec/tps6598x.c +39 -8

index 8b8406867c02..4b4c8d271b27 100644
@@ -73,6 +73,7 @@ struct tps6598x {
 	struct device *dev;
 	struct regmap *regmap;
 	struct mutex lock; /* device lock */
+	u8 i2c_protocol:1;
 
 	struct typec_port *port;
 	struct typec_partner *partner;

@@ -80,19 +81,39 @@ struct tps6598x {
 	struct typec_capability typec_cap;
 };
 
+static int
+tps6598x_block_read(struct tps6598x *tps, u8 reg, void *val, size_t len)
+{
+	u8 data[len + 1];
+	int ret;
+
+	if (!tps->i2c_protocol)
+		return regmap_raw_read(tps->regmap, reg, val, len);
+
+	ret = regmap_raw_read(tps->regmap, reg, data, sizeof(data));
+	if (ret)
+		return ret;
+
+	if (data[0] < len)
+		return -EIO;
+
+	memcpy(val, &data[1], len);
+	return 0;
+}
+
 static inline int tps6598x_read16(struct tps6598x *tps, u8 reg, u16 *val)
 {
-	return regmap_raw_read(tps->regmap, reg, val, sizeof(u16));
+	return tps6598x_block_read(tps, reg, val, sizeof(u16));
 }
 
 static inline int tps6598x_read32(struct tps6598x *tps, u8 reg, u32 *val)
 {
-	return regmap_raw_read(tps->regmap, reg, val, sizeof(u32));
+	return tps6598x_block_read(tps, reg, val, sizeof(u32));
 }
 
 static inline int tps6598x_read64(struct tps6598x *tps, u8 reg, u64 *val)
 {
-	return regmap_raw_read(tps->regmap, reg, val, sizeof(u64));
+	return tps6598x_block_read(tps, reg, val, sizeof(u64));
 }
 
 static inline int tps6598x_write16(struct tps6598x *tps, u8 reg, u16 val)

@@ -121,8 +142,8 @@ static int tps6598x_read_partner_identity(struct tps6598x *tps)
 	struct tps6598x_rx_identity_reg id;
 	int ret;
 
-	ret = regmap_raw_read(tps->regmap, TPS_REG_RX_IDENTITY_SOP,
-			      &id, sizeof(id));
+	ret = tps6598x_block_read(tps, TPS_REG_RX_IDENTITY_SOP,
+				  &id, sizeof(id));
 	if (ret)
 		return ret;

@@ -224,13 +245,13 @@ static int tps6598x_exec_cmd(struct tps6598x *tps, const char *cmd,
 	} while (val);
 
 	if (out_len) {
-		ret = regmap_raw_read(tps->regmap, TPS_REG_DATA1,
-				      out_data, out_len);
+		ret = tps6598x_block_read(tps, TPS_REG_DATA1,
+					  out_data, out_len);
 		if (ret)
 			return ret;
 		val = out_data[0];
 	} else {
-		ret = regmap_read(tps->regmap, TPS_REG_DATA1, &val);
+		ret = tps6598x_block_read(tps, TPS_REG_DATA1, &val, sizeof(u8));
 		if (ret)
 			return ret;
 	}

@@ -385,6 +406,16 @@ static int tps6598x_probe(struct i2c_client *client)
 	if (!vid)
 		return -ENODEV;
 
+	/*
+	 * Checking can the adapter handle SMBus protocol. If it can not, the
+	 * driver needs to take care of block reads separately.
+	 *
+	 * FIXME: Testing with I2C_FUNC_I2C. regmap-i2c uses I2C protocol
+	 * unconditionally if the adapter has I2C_FUNC_I2C set.
+	 */
+	if (i2c_check_functionality(client->adapter, I2C_FUNC_I2C))
+		tps->i2c_protocol = true;
+
 	ret = tps6598x_read32(tps, TPS_REG_STATUS, &status);
 	if (ret < 0)
 		return ret;

Nikolaus Voss

2018-09-10 05:05 UTC #1 lore raw

PATCH 1/1 usb: typec: tps6598x: handle block writes separately with plain-I2C adapters

Commit 1a2f474d328f handles block _reads_ separately with plain-I2C adapters, but the problem described with regmap-i2c not handling SMBus block transfers (i.e. read and writes) correctly also exists with writes.

As workaround, this patch adds a block write function the same way 1a2f474d328f adds a block read function.

Fixes 1a2f474d328f ("usb: typec: tps6598x: handle block reads separately with plain-I2C adapters")

Fixes 0a4c005bd171 ("usb: typec: driver for TI TPS6598x USB Power Delivery controllers")

Signed-off-by Nikolaus Voss <[email protected]>

---
 drivers/usb/typec/tps6598x.c | 22 ++++++++++++++++++----
 1 file changed, 18 insertions(+), 4 deletions(-)

drivers/usb/typec/tps6598x.c +18 -4

index c84c8c189e90..57a3e6c5c175 100644
@@ -110,6 +110,20 @@ tps6598x_block_read(struct tps6598x *tps, u8 reg, void *val, size_t len)
 	return 0;
 }
 
+static int tps6598x_block_write(struct tps6598x *tps, u8 reg,
+				void *val, size_t len)
+{
+	u8 data[len + 1];
+
+	if (!tps->i2c_protocol)
+		return regmap_raw_write(tps->regmap, reg, val, len);
+
+	data[0] = len;
+	memcpy(&data[1], val, len);
+
+	return regmap_raw_write(tps->regmap, reg, data, sizeof(data));
+}
+
 static inline int tps6598x_read16(struct tps6598x *tps, u8 reg, u16 *val)
 {
 	return tps6598x_block_read(tps, reg, val, sizeof(u16));

@@ -127,23 +141,23 @@ static inline int tps6598x_read64(struct tps6598x *tps, u8 reg, u64 *val)
 
 static inline int tps6598x_write16(struct tps6598x *tps, u8 reg, u16 val)
 {
-	return regmap_raw_write(tps->regmap, reg, &val, sizeof(u16));
+	return tps6598x_block_write(tps, reg, &val, sizeof(u16));
 }
 
 static inline int tps6598x_write32(struct tps6598x *tps, u8 reg, u32 val)
 {
-	return regmap_raw_write(tps->regmap, reg, &val, sizeof(u32));
+	return tps6598x_block_write(tps, reg, &val, sizeof(u32));
 }
 
 static inline int tps6598x_write64(struct tps6598x *tps, u8 reg, u64 val)
 {
-	return regmap_raw_write(tps->regmap, reg, &val, sizeof(u64));
+	return tps6598x_block_write(tps, reg, &val, sizeof(u64));
 }
 
 static inline int
 tps6598x_write_4cc(struct tps6598x *tps, u8 reg, const char *val)
 {
-	return regmap_raw_write(tps->regmap, reg, &val, sizeof(u32));
+	return tps6598x_block_write(tps, reg, &val, sizeof(u32));
 }
 
 static int tps6598x_read_partner_identity(struct tps6598x *tps)

Heikki Krogerus

author maintainer

2019-02-20 12:14 UTC #2 lore raw

Re: [PATCH] usb: typec: tps6598x: handle block writes separately with plain-I2C adapters

Hi,

Quoted 58 lines · Nikolaus Voss

As workaround, this patch adds a block write function the same way 1a2f474d328f adds a block read function.

Fixes 1a2f474d328f ("usb: typec: tps6598x: handle block reads separately with plain-I2C adapters")

Fixes 0a4c005bd171 ("usb: typec: driver for TI TPS6598x USB Power Delivery controllers")

Signed-off-by Nikolaus Voss <[email protected]>

---
 drivers/usb/typec/tps6598x.c | 22 ++++++++++++++++++----
 1 file changed, 18 insertions(+), 4 deletions(-)

drivers/usb/typec/tps6598x.c +17 -3

index c84c8c189e90..57a3e6c5c175 100644
@@ -110,6 +110,20 @@ tps6598x_block_read(struct tps6598x *tps, u8 reg, void *val, size_t len)
 	return 0;
 }
 
+static int tps6598x_block_write(struct tps6598x *tps, u8 reg,
+				void *val, size_t len)
+{
+	u8 data[len + 1];
+
+	if (!tps->i2c_protocol)
+		return regmap_raw_write(tps->regmap, reg, val, len);
+
+	data[0] = len;
+	memcpy(&data[1], val, len);
+
+	return regmap_raw_write(tps->regmap, reg, data, sizeof(data));
+}
+
 static inline int tps6598x_read16(struct tps6598x *tps, u8 reg, u16 *val)
 {
 	return tps6598x_block_read(tps, reg, val, sizeof(u16));

@@ -127,23 +141,23 @@ static inline int tps6598x_read64(struct tps6598x *tps, u8 reg, u64 *val)
 
 static inline int tps6598x_write16(struct tps6598x *tps, u8 reg, u16 val)
 {
-	return regmap_raw_write(tps->regmap, reg, &val, sizeof(u16));
+	return tps6598x_block_write(tps, reg, &val, sizeof(u16));
 }
 
 static inline int tps6598x_write32(struct tps6598x *tps, u8 reg, u32 val)
 {
-	return regmap_raw_write(tps->regmap, reg, &val, sizeof(u32));
+	return tps6598x_block_write(tps, reg, &val, sizeof(u32));
 }
 
 static inline int tps6598x_write64(struct tps6598x *tps, u8 reg, u64 val)
 {
-	return regmap_raw_write(tps->regmap, reg, &val, sizeof(u64));
+	return tps6598x_block_write(tps, reg, &val, sizeof(u64));

drivers/usb/typec/tps6598x.c static inline int tps6598x_read64(struct tps6598x *tps, u8 reg, u64 *val) · line 163

Quoted 10 lines

drivers/usb/typec/tps6598x.c +1 -1

 }
 
 static inline int
 tps6598x_write_4cc(struct tps6598x *tps, u8 reg, const char *val)
 {
-	return regmap_raw_write(tps->regmap, reg, &val, sizeof(u32));
+	return tps6598x_block_write(tps, reg, &val, sizeof(u32));
 }
 
 static int tps6598x_read_partner_identity(struct tps6598x *tps)

You need to fix tps6598x_exec_cmd() as well.

Did you really send this last September? If you did, then the mail has been stuck somewhere for a long time.

thanks,

Nikolaus Voss

2019-02-20 12:56 UTC #3 lore raw

Re: [PATCH] usb: typec: tps6598x: handle block writes separately with plain-I2C adapters

Hi,

Quoted 70 lines · Heikki Krogerus

Hi,

Quoted 67 lines · Nikolaus Voss

As workaround, this patch adds a block write function the same way 1a2f474d328f adds a block read function.

Fixes 1a2f474d328f ("usb: typec: tps6598x: handle block reads separately with plain-I2C adapters")

Fixes 0a4c005bd171 ("usb: typec: driver for TI TPS6598x USB Power Delivery controllers")

Signed-off-by Nikolaus Voss <[email protected]>

---
 drivers/usb/typec/tps6598x.c | 22 ++++++++++++++++++----
 1 file changed, 18 insertions(+), 4 deletions(-)

drivers/usb/typec/tps6598x.c +18 -4

index c84c8c189e90..57a3e6c5c175 100644
@@ -110,6 +110,20 @@ tps6598x_block_read(struct tps6598x *tps, u8 reg, void *val, size_t len)
 	return 0;
 }
 
+static int tps6598x_block_write(struct tps6598x *tps, u8 reg,
+				void *val, size_t len)
+{
+	u8 data[len + 1];
+
+	if (!tps->i2c_protocol)
+		return regmap_raw_write(tps->regmap, reg, val, len);
+
+	data[0] = len;
+	memcpy(&data[1], val, len);
+
+	return regmap_raw_write(tps->regmap, reg, data, sizeof(data));
+}
+
 static inline int tps6598x_read16(struct tps6598x *tps, u8 reg, u16 *val)
 {
 	return tps6598x_block_read(tps, reg, val, sizeof(u16));

@@ -127,23 +141,23 @@ static inline int tps6598x_read64(struct tps6598x *tps, u8 reg, u64 *val)
 
 static inline int tps6598x_write16(struct tps6598x *tps, u8 reg, u16 val)
 {
-	return regmap_raw_write(tps->regmap, reg, &val, sizeof(u16));
+	return tps6598x_block_write(tps, reg, &val, sizeof(u16));
 }
 
 static inline int tps6598x_write32(struct tps6598x *tps, u8 reg, u32 val)
 {
-	return regmap_raw_write(tps->regmap, reg, &val, sizeof(u32));
+	return tps6598x_block_write(tps, reg, &val, sizeof(u32));
 }
 
 static inline int tps6598x_write64(struct tps6598x *tps, u8 reg, u64 val)
 {
-	return regmap_raw_write(tps->regmap, reg, &val, sizeof(u64));
+	return tps6598x_block_write(tps, reg, &val, sizeof(u64));
 }
 
 static inline int
 tps6598x_write_4cc(struct tps6598x *tps, u8 reg, const char *val)
 {
-	return regmap_raw_write(tps->regmap, reg, &val, sizeof(u32));
+	return tps6598x_block_write(tps, reg, &val, sizeof(u32));
 }

Quoted 3 lines

drivers/usb/typec/tps6598x.c static inline int tps6598x_read64(struct tps6598x *tps, u8 reg, u64 *val) · line 163
Quoted 1 line
drivers/usb/typec/tps6598x.c +0 -0
 static int tps6598x_read_partner_identity(struct tps6598x *tps)
You need to fix tps6598x_exec_cmd() as well.

Right, thanks. I will fix that.

Quoted 3 lines

Did you really send this last September? If you did, then the mail has been stuck somewhere for a long time.

It's been stuck with me, I forgot to send it ;-).

Nikolaus

Nikolaus Voss

2019-02-20 12:57 UTC #4 lore raw

PATCH v2 1/1 usb: typec: tps6598x: handle block writes separately with plain-I2C adapters

In reply to Heikki Krogerus

As workaround, this patch adds a block write function the same way 1a2f474d328f adds a block read function.

Fixes 1a2f474d328f ("usb: typec: tps6598x: handle block reads separately with plain-I2C adapters")

Fixes 0a4c005bd171 ("usb: typec: driver for TI TPS6598x USB Power Delivery controllers")

Signed-off-by Nikolaus Voss <[email protected]>

---
v2: fix tps6598x_exec_cmd also

---
 drivers/usb/typec/tps6598x.c | 26 ++++++++++++++++++++------
 1 file changed, 20 insertions(+), 6 deletions(-)

drivers/usb/typec/tps6598x.c +20 -6

index c84c8c189e90..c54b73fb2a2f 100644
@@ -110,6 +110,20 @@ tps6598x_block_read(struct tps6598x *tps, u8 reg, void *val, size_t len)
 	return 0;
 }
 
+static int tps6598x_block_write(struct tps6598x *tps, u8 reg,
+				void *val, size_t len)
+{
+	u8 data[len + 1];
+
+	if (!tps->i2c_protocol)
+		return regmap_raw_write(tps->regmap, reg, val, len);
+
+	data[0] = len;
+	memcpy(&data[1], val, len);
+
+	return regmap_raw_write(tps->regmap, reg, data, sizeof(data));
+}
+
 static inline int tps6598x_read16(struct tps6598x *tps, u8 reg, u16 *val)
 {
 	return tps6598x_block_read(tps, reg, val, sizeof(u16));

@@ -127,23 +141,23 @@ static inline int tps6598x_read64(struct tps6598x *tps, u8 reg, u64 *val)
 
 static inline int tps6598x_write16(struct tps6598x *tps, u8 reg, u16 val)
 {
-	return regmap_raw_write(tps->regmap, reg, &val, sizeof(u16));
+	return tps6598x_block_write(tps, reg, &val, sizeof(u16));
 }
 
 static inline int tps6598x_write32(struct tps6598x *tps, u8 reg, u32 val)
 {
-	return regmap_raw_write(tps->regmap, reg, &val, sizeof(u32));
+	return tps6598x_block_write(tps, reg, &val, sizeof(u32));
 }
 
 static inline int tps6598x_write64(struct tps6598x *tps, u8 reg, u64 val)
 {
-	return regmap_raw_write(tps->regmap, reg, &val, sizeof(u64));
+	return tps6598x_block_write(tps, reg, &val, sizeof(u64));
 }
 
 static inline int
 tps6598x_write_4cc(struct tps6598x *tps, u8 reg, const char *val)
 {
-	return regmap_raw_write(tps->regmap, reg, &val, sizeof(u32));
+	return tps6598x_block_write(tps, reg, &val, sizeof(u32));
 }
 
 static int tps6598x_read_partner_identity(struct tps6598x *tps)

@@ -229,8 +243,8 @@ static int tps6598x_exec_cmd(struct tps6598x *tps, const char *cmd,
 		return -EBUSY;
 
 	if (in_len) {
-		ret = regmap_raw_write(tps->regmap, TPS_REG_DATA1,
-				       in_data, in_len);
+		ret = tps6598x_block_write(tps, TPS_REG_DATA1,
+					   in_data, in_len);
 		if (ret)
 			return ret;
 	}

Heikki Krogerus

author maintainer

2019-02-20 13:30 UTC #5 lore raw

Re: [PATCHv2] usb: typec: tps6598x: handle block writes separately with plain-I2C adapters

Quoted 11 lines · Nikolaus Voss

Commit 1a2f474d328f handles block _reads_ separately with plain-I2C adapters, but the problem described with regmap-i2c not handling SMBus block transfers (i.e. read and writes) correctly also exists with writes.
As workaround, this patch adds a block write function the same way 1a2f474d328f adds a block read function.
Fixes 1a2f474d328f ("usb: typec: tps6598x: handle block reads separately with plain-I2C adapters")
Fixes 0a4c005bd171 ("usb: typec: driver for TI TPS6598x USB Power Delivery controllers")
Signed-off-by Nikolaus Voss <[email protected]>

You are missing a "from" line with address that matches your SoB address.

thanks,

Nikolaus Voss

2019-02-20 13:38 UTC #6 lore raw

Re: [PATCHv2] usb: typec: tps6598x: handle block writes separately with plain-I2C adapters

Quoted 9 lines · Heikki Krogerus

Quoted 8 lines · Nikolaus Voss
Commit 1a2f474d328f handles block _reads_ separately with plain-I2C adapters, but the problem described with regmap-i2c not handling SMBus block transfers (i.e. read and writes) correctly also exists with writes.
As workaround, this patch adds a block write function the same way 1a2f474d328f adds a block read function.

Quoted 6 lines

Quoted 3 lines
Fixes 1a2f474d328f ("usb: typec: tps6598x: handle block reads separately with plain-I2C adapters")
Fixes 0a4c005bd171 ("usb: typec: driver for TI TPS6598x USB Power Delivery controllers")
Signed-off-by Nikolaus Voss <[email protected]>
You are missing a "from" line with address that matches your SoB address.

That's because I currently cannot send patch mails from my company account as our MTA breaks diffs. You could add

Signed-off-by Nikolaus Voss <[email protected]>

Nikolaus

Heikki Krogerus

author maintainer

2019-02-20 14:14 UTC #7 lore raw

Re: [PATCHv2] usb: typec: tps6598x: handle block writes separately with plain-I2C adapters

Quoted 14 lines · Nikolaus Voss

Quoted 13 lines · Heikki Krogerus
Quoted 11 lines · Nikolaus Voss
Commit 1a2f474d328f handles block _reads_ separately with plain-I2C adapters, but the problem described with regmap-i2c not handling SMBus block transfers (i.e. read and writes) correctly also exists with writes.
As workaround, this patch adds a block write function the same way 1a2f474d328f adds a block read function.
Fixes 1a2f474d328f ("usb: typec: tps6598x: handle block reads separately with plain-I2C adapters")
Fixes 0a4c005bd171 ("usb: typec: driver for TI TPS6598x USB Power Delivery controllers")
Signed-off-by Nikolaus Voss <[email protected]>

Quoted 5 lines

Quoted 2 lines
You are missing a "from" line with address that matches your SoB address.
That's because I currently cannot send patch mails from my company account as our MTA breaks diffs.

I understand, but you can have a separate "From line" in your patch, i.e. you send the patch using one address, and have an extra "From line" (outside of the mail header) with another address.

That other From line will be interpreted as the author address, and it should match your SoB address.

Try something like this in a branch where this patch is the HEAD:

        % export MY_COMMIT=$(git show -s --pretty=%h HEAD)
        % git reset HEAD^
        % GIT_COMMITTER_IDENT='Nikolaus Voss <[email protected]>' \
          GIT_AUTHOR_IDENT='Nikolaus Voss <[email protected]>' \
          git commit -a -C $MY_COMMIT

Then:

% git format-patch HEAD^
% git send-email ...

thanks,

Heikki Krogerus

author maintainer

2019-02-20 14:30 UTC #8 lore raw

Re: [PATCHv2] usb: typec: tps6598x: handle block writes separately with plain-I2C adapters

Quoted 29 lines · Heikki Krogerus

Quoted 14 lines · Nikolaus Voss
Quoted 13 lines · Heikki Krogerus
Quoted 11 lines · Nikolaus Voss
Commit 1a2f474d328f handles block _reads_ separately with plain-I2C adapters, but the problem described with regmap-i2c not handling SMBus block transfers (i.e. read and writes) correctly also exists with writes.
As workaround, this patch adds a block write function the same way 1a2f474d328f adds a block read function.
Fixes 1a2f474d328f ("usb: typec: tps6598x: handle block reads separately with plain-I2C adapters")
Fixes 0a4c005bd171 ("usb: typec: driver for TI TPS6598x USB Power Delivery controllers")
Signed-off-by Nikolaus Voss <[email protected]>
Quoted 5 lines
Quoted 2 lines
You are missing a "from" line with address that matches your SoB address.
That's because I currently cannot send patch mails from my company account as our MTA breaks diffs.
I understand, but you can have a separate "From line" in your patch, i.e. you send the patch using one address, and have an extra "From line" (outside of the mail header) with another address.
That other From line will be interpreted as the author address, and it should match your SoB address.

Quoted 7 lines

Try something like this in a branch where this patch is the HEAD:

        % export MY_COMMIT=$(git show -s --pretty=%h HEAD)
        % git reset HEAD^
        % GIT_COMMITTER_IDENT='Nikolaus Voss <[email protected]>' \
          GIT_AUTHOR_IDENT='Nikolaus Voss <[email protected]>' \
          git commit -a -C $MY_COMMIT

Correction here:

        % GIT_COMMITTER_IDENT='Nikolaus Voss <[email protected]>' \
          GIT_AUTHOR_IDENT='Nikolaus Voss <[email protected]>' \
          git commit -a -C $MY_COMMIT --reset-author

That "--reset-author" was missing. Sorry for that.

thanks,

Nikolaus Voss

2019-02-20 15:08 UTC #9 lore raw

Re: [PATCHv2] usb: typec: tps6598x: handle block writes separately with plain-I2C adapters

Quoted 38 lines · Heikki Krogerus

Quoted 29 lines · Heikki Krogerus
Quoted 14 lines · Nikolaus Voss
Quoted 13 lines · Heikki Krogerus
Quoted 11 lines · Nikolaus Voss
Commit 1a2f474d328f handles block _reads_ separately with plain-I2C adapters, but the problem described with regmap-i2c not handling SMBus block transfers (i.e. read and writes) correctly also exists with writes.
As workaround, this patch adds a block write function the same way 1a2f474d328f adds a block read function.
Fixes 1a2f474d328f ("usb: typec: tps6598x: handle block reads separately with plain-I2C adapters")
Fixes 0a4c005bd171 ("usb: typec: driver for TI TPS6598x USB Power Delivery controllers")
Signed-off-by Nikolaus Voss <[email protected]>
Quoted 5 lines
Quoted 2 lines
You are missing a "from" line with address that matches your SoB address.
That's because I currently cannot send patch mails from my company account as our MTA breaks diffs.
I understand, but you can have a separate "From line" in your patch, i.e. you send the patch using one address, and have an extra "From line" (outside of the mail header) with another address.
That other From line will be interpreted as the author address, and it should match your SoB address.
Quoted 7 lines
Try something like this in a branch where this patch is the HEAD:
        % export MY_COMMIT=$(git show -s --pretty=%h HEAD)
        % git reset HEAD^
        % GIT_COMMITTER_IDENT='Nikolaus Voss <[email protected]>' \
          GIT_AUTHOR_IDENT='Nikolaus Voss <[email protected]>' \
          git commit -a -C $MY_COMMIT

Quoted 7 lines

Correction here:

       % GIT_COMMITTER_IDENT='Nikolaus Voss <[email protected]>' \
         GIT_AUTHOR_IDENT='Nikolaus Voss <[email protected]>' \
         git commit -a -C $MY_COMMIT --reset-author

That "--reset-author" was missing. Sorry for that.

Thanks, Heikki, I'll give it a try...

Nikolaus

Guenter Roeck

2019-02-20 14:45 UTC #10 lore raw

Re: [PATCHv2] usb: typec: tps6598x: handle block writes separately with plain-I2C adapters

In reply to Nikolaus Voss

Quoted 22 lines · Nikolaus Voss

Commit 1a2f474d328f handles block _reads_ separately with plain-I2C adapters, but the problem described with regmap-i2c not handling SMBus block transfers (i.e. read and writes) correctly also exists with writes.
As workaround, this patch adds a block write function the same way 1a2f474d328f adds a block read function.
Fixes 1a2f474d328f ("usb: typec: tps6598x: handle block reads separately with plain-I2C adapters")
Fixes 0a4c005bd171 ("usb: typec: driver for TI TPS6598x USB Power Delivery controllers")
Signed-off-by Nikolaus Voss <[email protected]>
---
v2: fix tps6598x_exec_cmd also
---
  drivers/usb/typec/tps6598x.c | 26 ++++++++++++++++++++------
  1 file changed, 20 insertions(+), 6 deletions(-)
drivers/usb/typec/tps6598x.c +0 -0
index c84c8c189e90..c54b73fb2a2f 100644

drivers/usb/typec/tps6598x.c tps6598x_block_read(struct tps6598x *tps, u8 reg, void *val, size_t len) · line 117

Quoted 8 lines

drivers/usb/typec/tps6598x.c +5 -0

@@ -110,6 +110,20 @@ tps6598x_block_read(struct tps6598x *tps, u8 reg, void *val, size_t len)
  	return 0;
  }
 
+static int tps6598x_block_write(struct tps6598x *tps, u8 reg,
+				void *val, size_t len)
+{
+	u8 data[len + 1];
+

You should use TPS_MAX_LEN + 1 here to avoid the variable length array. See upstream commit 0bb95f80a38f8 ("Makefile: Globally enable VLA warning") and 8d361fa2c29dc ("usb: typec: tps6598x: Remove VLA usage"). Not sure if the WARN_ON introduced by 8d361fa2c29dc is really needed; I dislike unnecessary runtime checks.

Guenter

Quoted 52 lines

drivers/usb/typec/tps6598x.c +15 -6

+	if (!tps->i2c_protocol)
+		return regmap_raw_write(tps->regmap, reg, val, len);
+
+	data[0] = len;
+	memcpy(&data[1], val, len);
+
+	return regmap_raw_write(tps->regmap, reg, data, sizeof(data));
+}
+
  static inline int tps6598x_read16(struct tps6598x *tps, u8 reg, u16 *val)
  {
  	return tps6598x_block_read(tps, reg, val, sizeof(u16));

@@ -127,23 +141,23 @@ static inline int tps6598x_read64(struct tps6598x *tps, u8 reg, u64 *val)
 
  static inline int tps6598x_write16(struct tps6598x *tps, u8 reg, u16 val)
  {
-	return regmap_raw_write(tps->regmap, reg, &val, sizeof(u16));
+	return tps6598x_block_write(tps, reg, &val, sizeof(u16));
  }
 
  static inline int tps6598x_write32(struct tps6598x *tps, u8 reg, u32 val)
  {
-	return regmap_raw_write(tps->regmap, reg, &val, sizeof(u32));
+	return tps6598x_block_write(tps, reg, &val, sizeof(u32));
  }
 
  static inline int tps6598x_write64(struct tps6598x *tps, u8 reg, u64 val)
  {
-	return regmap_raw_write(tps->regmap, reg, &val, sizeof(u64));
+	return tps6598x_block_write(tps, reg, &val, sizeof(u64));
  }
 
  static inline int
  tps6598x_write_4cc(struct tps6598x *tps, u8 reg, const char *val)
  {
-	return regmap_raw_write(tps->regmap, reg, &val, sizeof(u32));
+	return tps6598x_block_write(tps, reg, &val, sizeof(u32));
  }
 
  static int tps6598x_read_partner_identity(struct tps6598x *tps)

@@ -229,8 +243,8 @@ static int tps6598x_exec_cmd(struct tps6598x *tps, const char *cmd,
  		return -EBUSY;
 
  	if (in_len) {
-		ret = regmap_raw_write(tps->regmap, TPS_REG_DATA1,
-				       in_data, in_len);
+		ret = tps6598x_block_write(tps, TPS_REG_DATA1,
+					   in_data, in_len);
  		if (ret)
  			return ret;
  	}

Nikolaus Voss

2019-02-20 15:18 UTC #11 lore raw

Re: [PATCHv2] usb: typec: tps6598x: handle block writes separately with plain-I2C adapters

Quoted 31 lines · Guenter Roeck

Quoted 30 lines · Nikolaus Voss
Commit 1a2f474d328f handles block _reads_ separately with plain-I2C adapters, but the problem described with regmap-i2c not handling SMBus block transfers (i.e. read and writes) correctly also exists with writes.
As workaround, this patch adds a block write function the same way 1a2f474d328f adds a block read function.
Fixes 1a2f474d328f ("usb: typec: tps6598x: handle block reads separately
with plain-I2C adapters")
Fixes 0a4c005bd171 ("usb: typec: driver for TI TPS6598x USB Power Delivery
controllers")
Signed-off-by Nikolaus Voss <[email protected]>
---
v2: fix tps6598x_exec_cmd also
---
  drivers/usb/typec/tps6598x.c | 26 ++++++++++++++++++++------
  1 file changed, 20 insertions(+), 6 deletions(-)
drivers/usb/typec/tps6598x.c +2 -0
index c84c8c189e90..c54b73fb2a2f 100644
@@ -110,6 +110,20 @@ tps6598x_block_read(struct tps6598x *tps, u8 reg, void 
 *val, size_t len)
  	return 0;
  }
  +static int tps6598x_block_write(struct tps6598x *tps, u8 reg,
+				void *val, size_t len)
+{

Quoted 8 lines

drivers/usb/typec/tps6598x.c tps6598x_block_read(struct tps6598x *tps, u8 reg, void · line 117
Quoted 2 lines
drivers/usb/typec/tps6598x.c +2 -0
+	u8 data[len + 1];
+
You should use TPS_MAX_LEN + 1 here to avoid the variable length array. See upstream commit 0bb95f80a38f8 ("Makefile: Globally enable VLA warning") and 8d361fa2c29dc ("usb: typec: tps6598x: Remove VLA usage"). Not sure if the WARN_ON introduced by 8d361fa2c29dc is really needed; I dislike unnecessary runtime checks.

Thanks for the pointer, I fixed this...

Nikolaus

Greg Kroah-Hartman

maintainer

2019-02-20 15:02 UTC #12 lore raw

Re: [PATCHv2] usb: typec: tps6598x: handle block writes separately with plain-I2C adapters

In reply to Nikolaus Voss

Quoted 5 lines · Nikolaus Voss

Commit 1a2f474d328f handles block _reads_ separately with plain-I2C adapters, but the problem described with regmap-i2c not handling SMBus block transfers (i.e. read and writes) correctly also exists with writes.

Quoted 7 lines

As workaround, this patch adds a block write function the same way 1a2f474d328f adds a block read function.
Fixes 1a2f474d328f ("usb: typec: tps6598x: handle block reads separately with plain-I2C adapters")
Fixes 0a4c005bd171 ("usb: typec: driver for TI TPS6598x USB Power Delivery controllers")
Signed-off-by Nikolaus Voss <[email protected]>
---

As was pointed out, you have to have a From: that matches a signed-off-by somewhere here. If your company email systems is horrid and can not handle patches, then put the correct from: line as the first line of the commit message as the documentation says and all will be good.

Quoted 10 lines

v2: fix tps6598x_exec_cmd also

---
 drivers/usb/typec/tps6598x.c | 26 ++++++++++++++++++++------
 1 file changed, 20 insertions(+), 6 deletions(-)

drivers/usb/typec/tps6598x.c +0 -0

index c84c8c189e90..c54b73fb2a2f 100644

drivers/usb/typec/tps6598x.c tps6598x_block_read(struct tps6598x *tps, u8 reg, void *val, size_t len) · line 116

Quoted 7 lines

drivers/usb/typec/tps6598x.c +4 -0

@@ -110,6 +110,20 @@ tps6598x_block_read(struct tps6598x *tps, u8 reg, void *val, size_t len)
 	return 0;
 }
 
+static int tps6598x_block_write(struct tps6598x *tps, u8 reg,
+				void *val, size_t len)
+{
+	u8 data[len + 1];

I thought the build system now warned when you did this :(

thanks,

greg k-h

Nikolaus Voss

2019-02-20 15:22 UTC #13 lore raw

Re: [PATCHv2] usb: typec: tps6598x: handle block writes separately with plain-I2C adapters

Quoted 35 lines · Greg Kroah-Hartman

Quoted 5 lines · Nikolaus Voss
Commit 1a2f474d328f handles block _reads_ separately with plain-I2C adapters, but the problem described with regmap-i2c not handling SMBus block transfers (i.e. read and writes) correctly also exists with writes.
Quoted 7 lines
As workaround, this patch adds a block write function the same way 1a2f474d328f adds a block read function.
Fixes 1a2f474d328f ("usb: typec: tps6598x: handle block reads separately with plain-I2C adapters")
Fixes 0a4c005bd171 ("usb: typec: driver for TI TPS6598x USB Power Delivery controllers")
Signed-off-by Nikolaus Voss <[email protected]>
---
As was pointed out, you have to have a From: that matches a signed-off-by somewhere here. If your company email systems is horrid and can not handle patches, then put the correct from: line as the first line of the commit message as the documentation says and all will be good.
Quoted 13 lines
v2: fix tps6598x_exec_cmd also
---
 drivers/usb/typec/tps6598x.c | 26 ++++++++++++++++++++------
 1 file changed, 20 insertions(+), 6 deletions(-)
drivers/usb/typec/tps6598x.c +0 -0
index c84c8c189e90..c54b73fb2a2f 100644
@@ -110,6 +110,20 @@ tps6598x_block_read(struct tps6598x *tps, u8 reg, void *val, size_t len)
 	return 0;
 }
 

Quoted 6 lines

drivers/usb/typec/tps6598x.c tps6598x_block_read(struct tps6598x *tps, u8 reg, void *val, size_t len) · line 116
Quoted 4 lines
drivers/usb/typec/tps6598x.c +4 -0
+static int tps6598x_block_write(struct tps6598x *tps, u8 reg,
+				void *val, size_t len)
+{
+	u8 data[len + 1];
I thought the build system now warned when you did this :(

I must admit I'm developing on 4.19 stable series, so no warnings...

Nikolaus

Greg Kroah-Hartman

maintainer

2019-02-20 16:22 UTC #14 lore raw

Re: [PATCHv2] usb: typec: tps6598x: handle block writes separately with plain-I2C adapters

Quoted 13 lines · Nikolaus Voss

Quoted 13 lines

v2: fix tps6598x_exec_cmd also

---
 drivers/usb/typec/tps6598x.c | 26 ++++++++++++++++++++------
 1 file changed, 20 insertions(+), 6 deletions(-)

drivers/usb/typec/tps6598x.c +0 -0

index c84c8c189e90..c54b73fb2a2f 100644
@@ -110,6 +110,20 @@ tps6598x_block_read(struct tps6598x *tps, u8 reg, void *val, size_t len)
 	return 0;
 }

Quoted 8 lines

Quoted 6 lines
drivers/usb/typec/tps6598x.c tps6598x_block_read(struct tps6598x *tps, u8 reg, void *val, size_t len) · line 116
Quoted 4 lines
drivers/usb/typec/tps6598x.c +4 -0
+static int tps6598x_block_write(struct tps6598x *tps, u8 reg,
+				void *val, size_t len)
+{
+	u8 data[len + 1];
I thought the build system now warned when you did this :(
I must admit I'm developing on 4.19 stable series, so no warnings...

Ick, no, you are 6 months behind where the rest of us are :(

Always, at the very least, work off of Linus's tree. For best results, work off of linux-next.

thanks,

greg k-h

Nikolaus Voss

2019-02-21 08:37 UTC #15 lore raw

Re: [PATCHv2] usb: typec: tps6598x: handle block writes separately with plain-I2C adapters

Hi Greg,

Quoted 21 lines · Greg Kroah-Hartman

Quoted 20 lines · Nikolaus Voss
Quoted 19 lines
Quoted 10 lines
v2: fix tps6598x_exec_cmd also
---
 drivers/usb/typec/tps6598x.c | 26 ++++++++++++++++++++------
 1 file changed, 20 insertions(+), 6 deletions(-)
drivers/usb/typec/tps6598x.c +0 -0
index c84c8c189e90..c54b73fb2a2f 100644
drivers/usb/typec/tps6598x.c tps6598x_block_read(struct tps6598x *tps, u8 reg, void *val, size_t len) · line 116
Quoted 7 lines
drivers/usb/typec/tps6598x.c +4 -0
@@ -110,6 +110,20 @@ tps6598x_block_read(struct tps6598x *tps, u8 reg, void *val, size_t len)
 	return 0;
 }
 
+static int tps6598x_block_write(struct tps6598x *tps, u8 reg,
+				void *val, size_t len)
+{
+	u8 data[len + 1];
I thought the build system now warned when you did this :(

Quoted 6 lines

Quoted 1 line
I must admit I'm developing on 4.19 stable series, so no warnings...
Ick, no, you are 6 months behind where the rest of us are :(
Always, at the very least, work off of Linus's tree. For best results, work off of linux-next.

we are a medical device manufacturer and our prototypes run stable kernels because our main development goal is the patient therapy.

However, what I do check is that my patches apply cleanly onto Linus's tree and if I see any other changes of files my patch touches I rebase and compile.

I'll try to always do the last before I submit a patch in the future but testing can only be done on our prototype branch (with reasonable effort).

Nikolaus

Greg Kroah-Hartman

maintainer

2019-02-21 09:52 UTC #16 lore raw

Re: [PATCHv2] usb: typec: tps6598x: handle block writes separately with plain-I2C adapters

Quoted 26 lines · Nikolaus Voss

Hi Greg,
Quoted 23 lines · Greg Kroah-Hartman
Quoted 21 lines · Nikolaus Voss
Quoted 13 lines
Quoted 13 lines
v2: fix tps6598x_exec_cmd also
---
 drivers/usb/typec/tps6598x.c | 26 ++++++++++++++++++++------
 1 file changed, 20 insertions(+), 6 deletions(-)
drivers/usb/typec/tps6598x.c +0 -0
index c84c8c189e90..c54b73fb2a2f 100644
@@ -110,6 +110,20 @@ tps6598x_block_read(struct tps6598x *tps, u8 reg, void *val, size_t len)
 	return 0;
 }
 
Quoted 6 lines
drivers/usb/typec/tps6598x.c tps6598x_block_read(struct tps6598x *tps, u8 reg, void *val, size_t len) · line 116
Quoted 4 lines
drivers/usb/typec/tps6598x.c +4 -0
+static int tps6598x_block_write(struct tps6598x *tps, u8 reg,
+				void *val, size_t len)
+{
+	u8 data[len + 1];
I thought the build system now warned when you did this :(
I must admit I'm developing on 4.19 stable series, so no warnings...

Quoted 7 lines

Quoted 4 lines
Ick, no, you are 6 months behind where the rest of us are :(
Always, at the very least, work off of Linus's tree. For best results, work off of linux-next.
we are a medical device manufacturer and our prototypes run stable kernels because our main development goal is the patient therapy.

That's great, and fine, but testing on newer kernels is always a good idea, that way you are not over a year behind when you have to move to the next LTS release :)

thanks,

greg k-h

kbuild test robot

build bot

2019-02-21 10:40 UTC #17 lore raw

Re: [PATCHv2] usb: typec: tps6598x: handle block writes separately with plain-I2C adapters

In reply to Nikolaus Voss

Thank you for the patch! Perhaps something to improve:

Hi Nikolaus,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on usb/usb-testing]
[also build test WARNING on v5.0-rc4 next-20190220]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Nikolaus-Voss/usb-typec-tps6598x-handle-block-writes-separately-with-plain-I2C-adapters/20190221-165456
base:   https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/usb.git usb-testing
config: i386-randconfig-a0-201907 (attached as .config)
compiler: gcc-4.9 (Debian 4.9.4-2) 4.9.4
reproduce:
        # save the attached .config to linux build tree
        make ARCH=i386

All warnings (new ones prefixed by >>):

drivers/usb/typec/tps6598x.c: In function 'tps6598x_block_write':

>> drivers/usb/typec/tps6598x.c:132:2: warning: variable length array 'data' is used [-Wvla]
     u8 data[len + 1];
     ^

vim +/data +132 drivers/usb/typec/tps6598x.c

128
129 static int tps6598x_block_write(struct tps6598x *tps, u8 reg,
130 void *val, size_t len)
131 {

 > 132		u8 data[len + 1];

133
134 if (!tps->i2c_protocol)
135 return regmap_raw_write(tps->regmap, reg, val, len);
136
137 data[0] = len;
138 memcpy(&data[1], val, len);
139
140 return regmap_raw_write(tps->regmap, reg, data, sizeof(data));
141 }
142
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation

Nikolaus Voss

2019-02-20 15:11 UTC #18 lore raw

PATCH v3 1/1 usb: typec: tps6598x: handle block writes separately with plain-I2C adapters

In reply to Nikolaus Voss

From: Nikolaus Voss <[email protected]>

As workaround, this patch adds a block write function the same way 1a2f474d328f adds a block read function.

Fixes 1a2f474d328f ("usb: typec: tps6598x: handle block reads separately with plain-I2C adapters")

Fixes 0a4c005bd171 ("usb: typec: driver for TI TPS6598x USB Power Delivery controllers")

Signed-off-by Nikolaus Voss <[email protected]>

---
v2: fix tps6598x_exec_cmd also
v3: use fixed length for stack buffer

---
 drivers/usb/typec/tps6598x.c | 26 ++++++++++++++++++++------
 1 file changed, 20 insertions(+), 6 deletions(-)

drivers/usb/typec/tps6598x.c +20 -6

index c84c8c189e90..eb8046f87a54 100644
@@ -110,6 +110,20 @@ tps6598x_block_read(struct tps6598x *tps, u8 reg, void *val, size_t len)
 	return 0;
 }
 
+static int tps6598x_block_write(struct tps6598x *tps, u8 reg,
+				void *val, size_t len)
+{
+	u8 data[TPS_MAX_LEN + 1];
+
+	if (!tps->i2c_protocol)
+		return regmap_raw_write(tps->regmap, reg, val, len);
+
+	data[0] = len;
+	memcpy(&data[1], val, len);
+
+	return regmap_raw_write(tps->regmap, reg, data, sizeof(data));
+}
+
 static inline int tps6598x_read16(struct tps6598x *tps, u8 reg, u16 *val)
 {
 	return tps6598x_block_read(tps, reg, val, sizeof(u16));

@@ -127,23 +141,23 @@ static inline int tps6598x_read64(struct tps6598x *tps, u8 reg, u64 *val)
 
 static inline int tps6598x_write16(struct tps6598x *tps, u8 reg, u16 val)
 {
-	return regmap_raw_write(tps->regmap, reg, &val, sizeof(u16));
+	return tps6598x_block_write(tps, reg, &val, sizeof(u16));
 }
 
 static inline int tps6598x_write32(struct tps6598x *tps, u8 reg, u32 val)
 {
-	return regmap_raw_write(tps->regmap, reg, &val, sizeof(u32));
+	return tps6598x_block_write(tps, reg, &val, sizeof(u32));
 }
 
 static inline int tps6598x_write64(struct tps6598x *tps, u8 reg, u64 val)
 {
-	return regmap_raw_write(tps->regmap, reg, &val, sizeof(u64));
+	return tps6598x_block_write(tps, reg, &val, sizeof(u64));
 }
 
 static inline int
 tps6598x_write_4cc(struct tps6598x *tps, u8 reg, const char *val)
 {
-	return regmap_raw_write(tps->regmap, reg, &val, sizeof(u32));
+	return tps6598x_block_write(tps, reg, &val, sizeof(u32));
 }
 
 static int tps6598x_read_partner_identity(struct tps6598x *tps)

@@ -229,8 +243,8 @@ static int tps6598x_exec_cmd(struct tps6598x *tps, const char *cmd,
 		return -EBUSY;
 
 	if (in_len) {
-		ret = regmap_raw_write(tps->regmap, TPS_REG_DATA1,
-				       in_data, in_len);
+		ret = tps6598x_block_write(tps, TPS_REG_DATA1,
+					   in_data, in_len);
 		if (ret)
 			return ret;
 	}

Guenter Roeck

Reviewed-by on v3 1/1

2019-02-20 15:35 UTC #19 lore raw

Re: [PATCHv3] usb: typec: tps6598x: handle block writes separately with plain-I2C adapters

Quoted 7 lines · Nikolaus Voss

From: Nikolaus Voss <[email protected]>
Commit 1a2f474d328f handles block _reads_ separately with plain-I2C adapters, but the problem described with regmap-i2c not handling SMBus block transfers (i.e. read and writes) correctly also exists with writes.

Quoted 6 lines

As workaround, this patch adds a block write function the same way 1a2f474d328f adds a block read function.
Fixes 1a2f474d328f ("usb: typec: tps6598x: handle block reads separately with plain-I2C adapters")
Fixes 0a4c005bd171 ("usb: typec: driver for TI TPS6598x USB Power Delivery controllers")
Signed-off-by Nikolaus Voss <[email protected]>

Reviewed-by Guenter Roeck <[email protected]>

Note that tps6598x_exec_cmd() is only called with in_len == out_len == 0 and NULL data pointers. It might make sense to simplify the function and drop the unused parameters as well as the associated code.

Guenter

Quoted 72 lines

---
v2: fix tps6598x_exec_cmd also
v3: use fixed length for stack buffer

---
  drivers/usb/typec/tps6598x.c | 26 ++++++++++++++++++++------
  1 file changed, 20 insertions(+), 6 deletions(-)

drivers/usb/typec/tps6598x.c +20 -6

index c84c8c189e90..eb8046f87a54 100644
@@ -110,6 +110,20 @@ tps6598x_block_read(struct tps6598x *tps, u8 reg, void *val, size_t len)
  	return 0;
  }
 
+static int tps6598x_block_write(struct tps6598x *tps, u8 reg,
+				void *val, size_t len)
+{
+	u8 data[TPS_MAX_LEN + 1];
+
+	if (!tps->i2c_protocol)
+		return regmap_raw_write(tps->regmap, reg, val, len);
+
+	data[0] = len;
+	memcpy(&data[1], val, len);
+
+	return regmap_raw_write(tps->regmap, reg, data, sizeof(data));
+}
+
  static inline int tps6598x_read16(struct tps6598x *tps, u8 reg, u16 *val)
  {
  	return tps6598x_block_read(tps, reg, val, sizeof(u16));

@@ -127,23 +141,23 @@ static inline int tps6598x_read64(struct tps6598x *tps, u8 reg, u64 *val)
 
  static inline int tps6598x_write16(struct tps6598x *tps, u8 reg, u16 val)
  {
-	return regmap_raw_write(tps->regmap, reg, &val, sizeof(u16));
+	return tps6598x_block_write(tps, reg, &val, sizeof(u16));
  }
 
  static inline int tps6598x_write32(struct tps6598x *tps, u8 reg, u32 val)
  {
-	return regmap_raw_write(tps->regmap, reg, &val, sizeof(u32));
+	return tps6598x_block_write(tps, reg, &val, sizeof(u32));
  }
 
  static inline int tps6598x_write64(struct tps6598x *tps, u8 reg, u64 val)
  {
-	return regmap_raw_write(tps->regmap, reg, &val, sizeof(u64));
+	return tps6598x_block_write(tps, reg, &val, sizeof(u64));
  }
 
  static inline int
  tps6598x_write_4cc(struct tps6598x *tps, u8 reg, const char *val)
  {
-	return regmap_raw_write(tps->regmap, reg, &val, sizeof(u32));
+	return tps6598x_block_write(tps, reg, &val, sizeof(u32));
  }
 
  static int tps6598x_read_partner_identity(struct tps6598x *tps)

@@ -229,8 +243,8 @@ static int tps6598x_exec_cmd(struct tps6598x *tps, const char *cmd,
  		return -EBUSY;
 
  	if (in_len) {
-		ret = regmap_raw_write(tps->regmap, TPS_REG_DATA1,
-				       in_data, in_len);
+		ret = tps6598x_block_write(tps, TPS_REG_DATA1,
+					   in_data, in_len);
  		if (ret)
  			return ret;
  	}

Guenter Roeck

2019-02-20 15:36 UTC #20 lore raw

Re: [PATCHv3] usb: typec: tps6598x: handle block writes separately with plain-I2C adapters

Quoted 15 lines · Guenter Roeck

Quoted 7 lines · Nikolaus Voss
From: Nikolaus Voss <[email protected]>
Commit 1a2f474d328f handles block _reads_ separately with plain-I2C adapters, but the problem described with regmap-i2c not handling SMBus block transfers (i.e. read and writes) correctly also exists with writes.
Quoted 6 lines
As workaround, this patch adds a block write function the same way 1a2f474d328f adds a block read function.
Fixes 1a2f474d328f ("usb: typec: tps6598x: handle block reads separately with plain-I2C adapters")
Fixes 0a4c005bd171 ("usb: typec: driver for TI TPS6598x USB Power Delivery controllers")
Signed-off-by Nikolaus Voss <[email protected]>

Quoted 6 lines

Reviewed-by Guenter Roeck <[email protected]>
Note that tps6598x_exec_cmd() is only called with in_len == out_len == 0 and NULL data pointers. It might make sense to simplify the function and drop the unused parameters as well as the associated code.

Clarification: That would be a separate patch.

Guenter

Quoted 68 lines

Guenter

Quoted 65 lines

---
v2: fix tps6598x_exec_cmd also
v3: use fixed length for stack buffer
---
drivers/usb/typec/tps6598x.c | 26 ++++++++++++++++++++------

  1 file changed, 20 insertions(+), 6 deletions(-)

drivers/usb/typec/tps6598x.c +17 -3

index c84c8c189e90..eb8046f87a54 100644
@@ -110,6 +110,20 @@ tps6598x_block_read(struct tps6598x *tps, u8 reg, void *val, size_t len)
       return 0;
   }
+static int tps6598x_block_write(struct tps6598x *tps, u8 reg,
+                void *val, size_t len)
+{
+    u8 data[TPS_MAX_LEN + 1];
+
+    if (!tps->i2c_protocol)
+        return regmap_raw_write(tps->regmap, reg, val, len);
+
+    data[0] = len;
+    memcpy(&data[1], val, len);
+
+    return regmap_raw_write(tps->regmap, reg, data, sizeof(data));
+}
+
   static inline int tps6598x_read16(struct tps6598x *tps, u8 reg, u16 *val)
   {
       return tps6598x_block_read(tps, reg, val, sizeof(u16));

@@ -127,23 +141,23 @@ static inline int tps6598x_read64(struct tps6598x *tps, u8 reg, u64 *val)
   static inline int tps6598x_write16(struct tps6598x *tps, u8 reg, u16 val)
   {
-    return regmap_raw_write(tps->regmap, reg, &val, sizeof(u16));
+    return tps6598x_block_write(tps, reg, &val, sizeof(u16));
   }
   static inline int tps6598x_write32(struct tps6598x *tps, u8 reg, u32 val)
   {
-    return regmap_raw_write(tps->regmap, reg, &val, sizeof(u32));
+    return tps6598x_block_write(tps, reg, &val, sizeof(u32));
   }
   static inline int tps6598x_write64(struct tps6598x *tps, u8 reg, u64 val)
   {
-    return regmap_raw_write(tps->regmap, reg, &val, sizeof(u64));
+    return tps6598x_block_write(tps, reg, &val, sizeof(u64));
   }

static inline int
tps6598x_write_4cc(struct tps6598x *tps, u8 reg, const char *val) {
- return regmap_raw_write(tps->regmap, reg, &val, sizeof(u32)); + return tps6598x_block_write(tps, reg, &val, sizeof(u32)); }
static int tps6598x_read_partner_identity(struct tps6598x *tps)

drivers/usb/typec/tps6598x.c +2 -2

@@ -229,8 +243,8 @@ static int tps6598x_exec_cmd(struct tps6598x *tps, const char *cmd,
           return -EBUSY;
       if (in_len) {
-        ret = regmap_raw_write(tps->regmap, TPS_REG_DATA1,
-                       in_data, in_len);
+        ret = tps6598x_block_write(tps, TPS_REG_DATA1,
+                       in_data, in_len);
           if (ret)
               return ret;
       }

Nikolaus Voss

2019-02-21 08:41 UTC #21 lore raw

Re: [PATCHv3] usb: typec: tps6598x: handle block writes separately with plain-I2C adapters

In reply to Guenter Roeck

Hi Guenther,

Quoted 17 lines · Guenter Roeck

Quoted 7 lines · Nikolaus Voss
From: Nikolaus Voss <[email protected]>
Commit 1a2f474d328f handles block _reads_ separately with plain-I2C adapters, but the problem described with regmap-i2c not handling SMBus block transfers (i.e. read and writes) correctly also exists with writes.
Quoted 8 lines
As workaround, this patch adds a block write function the same way 1a2f474d328f adds a block read function.
Fixes 1a2f474d328f ("usb: typec: tps6598x: handle block reads separately
with plain-I2C adapters")
Fixes 0a4c005bd171 ("usb: typec: driver for TI TPS6598x USB Power Delivery
controllers")
Signed-off-by Nikolaus Voss <[email protected]>

Quoted 4 lines

Reviewed-by Guenter Roeck <[email protected]>
Note that tps6598x_exec_cmd() is only called with in_len == out_len == 0 and NULL data pointers.

That's probably why I didn't notice I missed patching tps6598x_exec_cmd() in spite of running and testing the driver for half a year ;-).

Thanks,
Nikolaus

Heikki Krogerus

author maintainer Acked-by on v3 1/1

2019-02-21 09:08 UTC #22 lore raw

Re: [PATCHv3] usb: typec: tps6598x: handle block writes separately with plain-I2C adapters

In reply to Nikolaus Voss

Quoted 7 lines · Nikolaus Voss

From: Nikolaus Voss <[email protected]>
Commit 1a2f474d328f handles block _reads_ separately with plain-I2C adapters, but the problem described with regmap-i2c not handling SMBus block transfers (i.e. read and writes) correctly also exists with writes.

Quoted 6 lines

As workaround, this patch adds a block write function the same way 1a2f474d328f adds a block read function.
Fixes 1a2f474d328f ("usb: typec: tps6598x: handle block reads separately with plain-I2C adapters")
Fixes 0a4c005bd171 ("usb: typec: driver for TI TPS6598x USB Power Delivery controllers")
Signed-off-by Nikolaus Voss <[email protected]>

Acked-by Heikki Krogerus <[email protected]>

Quoted 73 lines

---
v2: fix tps6598x_exec_cmd also
v3: use fixed length for stack buffer

---
 drivers/usb/typec/tps6598x.c | 26 ++++++++++++++++++++------
 1 file changed, 20 insertions(+), 6 deletions(-)

drivers/usb/typec/tps6598x.c +20 -6

index c84c8c189e90..eb8046f87a54 100644
@@ -110,6 +110,20 @@ tps6598x_block_read(struct tps6598x *tps, u8 reg, void *val, size_t len)
 	return 0;
 }
 
+static int tps6598x_block_write(struct tps6598x *tps, u8 reg,
+				void *val, size_t len)
+{
+	u8 data[TPS_MAX_LEN + 1];
+
+	if (!tps->i2c_protocol)
+		return regmap_raw_write(tps->regmap, reg, val, len);
+
+	data[0] = len;
+	memcpy(&data[1], val, len);
+
+	return regmap_raw_write(tps->regmap, reg, data, sizeof(data));
+}
+
 static inline int tps6598x_read16(struct tps6598x *tps, u8 reg, u16 *val)
 {
 	return tps6598x_block_read(tps, reg, val, sizeof(u16));

@@ -127,23 +141,23 @@ static inline int tps6598x_read64(struct tps6598x *tps, u8 reg, u64 *val)
 
 static inline int tps6598x_write16(struct tps6598x *tps, u8 reg, u16 val)
 {
-	return regmap_raw_write(tps->regmap, reg, &val, sizeof(u16));
+	return tps6598x_block_write(tps, reg, &val, sizeof(u16));
 }
 
 static inline int tps6598x_write32(struct tps6598x *tps, u8 reg, u32 val)
 {
-	return regmap_raw_write(tps->regmap, reg, &val, sizeof(u32));
+	return tps6598x_block_write(tps, reg, &val, sizeof(u32));
 }
 
 static inline int tps6598x_write64(struct tps6598x *tps, u8 reg, u64 val)
 {
-	return regmap_raw_write(tps->regmap, reg, &val, sizeof(u64));
+	return tps6598x_block_write(tps, reg, &val, sizeof(u64));
 }
 
 static inline int
 tps6598x_write_4cc(struct tps6598x *tps, u8 reg, const char *val)
 {
-	return regmap_raw_write(tps->regmap, reg, &val, sizeof(u32));
+	return tps6598x_block_write(tps, reg, &val, sizeof(u32));
 }
 
 static int tps6598x_read_partner_identity(struct tps6598x *tps)

@@ -229,8 +243,8 @@ static int tps6598x_exec_cmd(struct tps6598x *tps, const char *cmd,
 		return -EBUSY;
 
 	if (in_len) {
-		ret = regmap_raw_write(tps->regmap, TPS_REG_DATA1,
-				       in_data, in_len);
+		ret = tps6598x_block_write(tps, TPS_REG_DATA1,
+					   in_data, in_len);
 		if (ret)
 			return ret;
 	}

Signature · 1 line

2.17.1

thanks,

Nikolaus Voss

2019-02-21 09:42 UTC #23 lore raw

Re: [PATCHv3] usb: typec: tps6598x: handle block writes separately with plain-I2C adapters

Quoted 8 lines · Heikki Krogerus

Quoted 7 lines · Nikolaus Voss
From: Nikolaus Voss <[email protected]>
Commit 1a2f474d328f handles block _reads_ separately with plain-I2C adapters, but the problem described with regmap-i2c not handling SMBus block transfers (i.e. read and writes) correctly also exists with writes.

Quoted 8 lines

Quoted 6 lines
As workaround, this patch adds a block write function the same way 1a2f474d328f adds a block read function.
Fixes 1a2f474d328f ("usb: typec: tps6598x: handle block reads separately with plain-I2C adapters")
Fixes 0a4c005bd171 ("usb: typec: driver for TI TPS6598x USB Power Delivery controllers")
Signed-off-by Nikolaus Voss <[email protected]>
Acked-by Heikki Krogerus <[email protected]>

Heikki,

shall I respin the patch with tags?

Nikolaus

Greg Kroah-Hartman

maintainer

2019-02-21 09:52 UTC #24 lore raw

Re: [PATCHv3] usb: typec: tps6598x: handle block writes separately with plain-I2C adapters

Quoted 16 lines · Nikolaus Voss

Quoted 15 lines · Heikki Krogerus
Quoted 13 lines · Nikolaus Voss
From: Nikolaus Voss <[email protected]>
Commit 1a2f474d328f handles block _reads_ separately with plain-I2C adapters, but the problem described with regmap-i2c not handling SMBus block transfers (i.e. read and writes) correctly also exists with writes.
As workaround, this patch adds a block write function the same way 1a2f474d328f adds a block read function.
Fixes 1a2f474d328f ("usb: typec: tps6598x: handle block reads separately with plain-I2C adapters")
Fixes 0a4c005bd171 ("usb: typec: driver for TI TPS6598x USB Power Delivery controllers")
Signed-off-by Nikolaus Voss <[email protected]>

Quoted 5 lines

Quoted 1 line
Acked-by Heikki Krogerus <[email protected]>
Heikki,
shall I respin the patch with tags?

I can add it, thanks.

greg k-h