[PATCH 0/2] usb: typec: fixes

[PATCH 0/2] usb: typec: fixes

[Heikki Krogerus]

Hi,

I got two separate fixes here. First one will fix an issue with ucsi where the
driver may timeout if EC is under heavy load, and the second an issue with
tps6598x when used with plain I2C adapters.


Thanks,


Heikki Krogerus (2):
  usb: typec: tps6598x: handle block reads separately with plain-I2C
    adapters
  usb: typec: ucsi: Increase command completion timeout value

 drivers/usb/typec/tps6598x.c  | 42 +++++++++++++++++++++++++++++------
 drivers/usb/typec/ucsi/ucsi.c |  2 +-
 2 files changed, 36 insertions(+), 8 deletions(-)

-- 
2.17.0

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

[Heikki Krogerus]

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")
Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
---
 drivers/usb/typec/tps6598x.c | 42 ++++++++++++++++++++++++++++++------
 1 file changed, 35 insertions(+), 7 deletions(-)

diff --git a/drivers/usb/typec/tps6598x.c b/drivers/usb/typec/tps6598x.c
index 8b8406867c02..82f09cd9792d 100644
--- a/drivers/usb/typec/tps6598x.c
+++ b/drivers/usb/typec/tps6598x.c
@@ -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,6 +81,23 @@ struct tps6598x {
 	struct typec_capability typec_cap;
 };
 
+static int
+tps6598x_block_read(struct tps6598x *tps, u8 reg, void *val, ssize_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;
+
+	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));
@@ -87,12 +105,12 @@ static inline int tps6598x_read16(struct tps6598x *tps, u8 reg, u16 *val)
 
 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 +139,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 +242,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 +403,16 @@ static int tps6598x_probe(struct i2c_client *client)
 	if (!vid)
 		return -ENODEV;
 
+	/*
+	 * Checking can the adapter handle SMBus protocol. If if 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;
-- 
2.17.0

[1/2] usb: typec: tps6598x: handle block reads separately with plain-I2C adapters

[Heikki Krogerus]

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")
Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
---
 drivers/usb/typec/tps6598x.c | 42 ++++++++++++++++++++++++++++++------
 1 file changed, 35 insertions(+), 7 deletions(-)

diff --git a/drivers/usb/typec/tps6598x.c b/drivers/usb/typec/tps6598x.c
index 8b8406867c02..82f09cd9792d 100644
--- a/drivers/usb/typec/tps6598x.c
+++ b/drivers/usb/typec/tps6598x.c
@@ -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,6 +81,23 @@ struct tps6598x {
 	struct typec_capability typec_cap;
 };
 
+static int
+tps6598x_block_read(struct tps6598x *tps, u8 reg, void *val, ssize_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;
+
+	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));
@@ -87,12 +105,12 @@ static inline int tps6598x_read16(struct tps6598x *tps, u8 reg, u16 *val)
 
 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 +139,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 +242,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 +403,16 @@ static int tps6598x_probe(struct i2c_client *client)
 	if (!vid)
 		return -ENODEV;
 
+	/*
+	 * Checking can the adapter handle SMBus protocol. If if 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;

[PATCH 2/2] usb: typec: ucsi: Increase command completion timeout value

[Heikki Krogerus]

On some boards, under heavy load, the EC firmware is
unable to complete commands even in one second. Increasing
the command completion timeout value to five seconds.

Reported-by: Quanxian Wang <quanxian.wang@intel.com>
Fixes: c1b0bc2dabfa ("usb: typec: Add support for UCSI interface")
Cc: <stable@vger.kernel.org>
Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
---
 drivers/usb/typec/ucsi/ucsi.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/usb/typec/ucsi/ucsi.c b/drivers/usb/typec/ucsi/ucsi.c
index bf0977fbd100..bd5cca5632b3 100644
--- a/drivers/usb/typec/ucsi/ucsi.c
+++ b/drivers/usb/typec/ucsi/ucsi.c
@@ -28,7 +28,7 @@
  * difficult to estimate the time it takes for the system to process the command
  * before it is actually passed to the PPM.
  */
-#define UCSI_TIMEOUT_MS		1000
+#define UCSI_TIMEOUT_MS		5000
 
 /*
  * UCSI_SWAP_TIMEOUT_MS - Timeout for role swap requests
-- 
2.17.0

[2/2] usb: typec: ucsi: Increase command completion timeout value

[Heikki Krogerus]

On some boards, under heavy load, the EC firmware is
unable to complete commands even in one second. Increasing
the command completion timeout value to five seconds.

Reported-by: Quanxian Wang <quanxian.wang@intel.com>
Fixes: c1b0bc2dabfa ("usb: typec: Add support for UCSI interface")
Cc: <stable@vger.kernel.org>
Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
---
 drivers/usb/typec/ucsi/ucsi.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/usb/typec/ucsi/ucsi.c b/drivers/usb/typec/ucsi/ucsi.c
index bf0977fbd100..bd5cca5632b3 100644
--- a/drivers/usb/typec/ucsi/ucsi.c
+++ b/drivers/usb/typec/ucsi/ucsi.c
@@ -28,7 +28,7 @@
  * difficult to estimate the time it takes for the system to process the command
  * before it is actually passed to the PPM.
  */
-#define UCSI_TIMEOUT_MS		1000
+#define UCSI_TIMEOUT_MS		5000
 
 /*
  * UCSI_SWAP_TIMEOUT_MS - Timeout for role swap requests

Re: [PATCH 1/2] usb: typec: tps6598x: handle block reads separately with plain-I2C adapters

[Guenter Roeck]

On Wed, Apr 18, 2018 at 03:34:09PM +0300, Heikki Krogerus wrote:
> 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")
> Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> ---
>  drivers/usb/typec/tps6598x.c | 42 ++++++++++++++++++++++++++++++------
>  1 file changed, 35 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/usb/typec/tps6598x.c b/drivers/usb/typec/tps6598x.c
> index 8b8406867c02..82f09cd9792d 100644
> --- a/drivers/usb/typec/tps6598x.c
> +++ b/drivers/usb/typec/tps6598x.c
> @@ -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,6 +81,23 @@ struct tps6598x {
>  	struct typec_capability typec_cap;
>  };
>  
> +static int
> +tps6598x_block_read(struct tps6598x *tps, u8 reg, void *val, ssize_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;
> +

Sanity check ?
	if (data[0] != len)
		return -Esomething;

Other than that,

Reviewed-by: Guenter Roeck <linux@roeck-us.net>

> +	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));
> @@ -87,12 +105,12 @@ static inline int tps6598x_read16(struct tps6598x *tps, u8 reg, u16 *val)
>  
>  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 +139,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 +242,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 +403,16 @@ static int tps6598x_probe(struct i2c_client *client)
>  	if (!vid)
>  		return -ENODEV;
>  
> +	/*
> +	 * Checking can the adapter handle SMBus protocol. If if 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;
> -- 
> 2.17.0
> 

[1/2] usb: typec: tps6598x: handle block reads separately with plain-I2C adapters

[Guenter Roeck]

On Wed, Apr 18, 2018 at 03:34:09PM +0300, Heikki Krogerus wrote:
> 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")
> Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> ---
>  drivers/usb/typec/tps6598x.c | 42 ++++++++++++++++++++++++++++++------
>  1 file changed, 35 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/usb/typec/tps6598x.c b/drivers/usb/typec/tps6598x.c
> index 8b8406867c02..82f09cd9792d 100644
> --- a/drivers/usb/typec/tps6598x.c
> +++ b/drivers/usb/typec/tps6598x.c
> @@ -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,6 +81,23 @@ struct tps6598x {
>  	struct typec_capability typec_cap;
>  };
>  
> +static int
> +tps6598x_block_read(struct tps6598x *tps, u8 reg, void *val, ssize_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;
> +

Sanity check ?
	if (data[0] != len)
		return -Esomething;

Other than that,

Reviewed-by: Guenter Roeck <linux@roeck-us.net>

> +	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));
> @@ -87,12 +105,12 @@ static inline int tps6598x_read16(struct tps6598x *tps, u8 reg, u16 *val)
>  
>  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 +139,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 +242,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 +403,16 @@ static int tps6598x_probe(struct i2c_client *client)
>  	if (!vid)
>  		return -ENODEV;
>  
> +	/*
> +	 * Checking can the adapter handle SMBus protocol. If if 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;
> -- 
> 2.17.0
>
---
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/2] usb: typec: tps6598x: handle block reads separately with plain-I2C adapters

[Greg Kroah-Hartman]

On Fri, Apr 20, 2018 at 10:26:08AM -0700, Guenter Roeck wrote:
> On Wed, Apr 18, 2018 at 03:34:09PM +0300, Heikki Krogerus wrote:
> > 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")
> > Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> > ---
> >  drivers/usb/typec/tps6598x.c | 42 ++++++++++++++++++++++++++++++------
> >  1 file changed, 35 insertions(+), 7 deletions(-)
> > 
> > diff --git a/drivers/usb/typec/tps6598x.c b/drivers/usb/typec/tps6598x.c
> > index 8b8406867c02..82f09cd9792d 100644
> > --- a/drivers/usb/typec/tps6598x.c
> > +++ b/drivers/usb/typec/tps6598x.c
> > @@ -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,6 +81,23 @@ struct tps6598x {
> >  	struct typec_capability typec_cap;
> >  };
> >  
> > +static int
> > +tps6598x_block_read(struct tps6598x *tps, u8 reg, void *val, ssize_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;
> > +
> 
> Sanity check ?
> 	if (data[0] != len)
> 		return -Esomething;
> 
> Other than that,
> 
> Reviewed-by: Guenter Roeck <linux@roeck-us.net>

I'll wait for a v2 of this to apply it.

thanks,

greg k-h

[1/2] usb: typec: tps6598x: handle block reads separately with plain-I2C adapters

[Greg Kroah-Hartman]

On Fri, Apr 20, 2018 at 10:26:08AM -0700, Guenter Roeck wrote:
> On Wed, Apr 18, 2018 at 03:34:09PM +0300, Heikki Krogerus wrote:
> > 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")
> > Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> > ---
> >  drivers/usb/typec/tps6598x.c | 42 ++++++++++++++++++++++++++++++------
> >  1 file changed, 35 insertions(+), 7 deletions(-)
> > 
> > diff --git a/drivers/usb/typec/tps6598x.c b/drivers/usb/typec/tps6598x.c
> > index 8b8406867c02..82f09cd9792d 100644
> > --- a/drivers/usb/typec/tps6598x.c
> > +++ b/drivers/usb/typec/tps6598x.c
> > @@ -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,6 +81,23 @@ struct tps6598x {
> >  	struct typec_capability typec_cap;
> >  };
> >  
> > +static int
> > +tps6598x_block_read(struct tps6598x *tps, u8 reg, void *val, ssize_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;
> > +
> 
> Sanity check ?
> 	if (data[0] != len)
> 		return -Esomething;
> 
> Other than that,
> 
> Reviewed-by: Guenter Roeck <linux@roeck-us.net>

I'll wait for a v2 of this to apply it.

thanks,

greg k-h
---
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/2] usb: typec: tps6598x: handle block reads separately with plain-I2C adapters

[Heikki Krogerus]

On Fri, Apr 20, 2018 at 10:26:08AM -0700, Guenter Roeck wrote:
> On Wed, Apr 18, 2018 at 03:34:09PM +0300, Heikki Krogerus wrote:
> > 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")
> > Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> > ---
> >  drivers/usb/typec/tps6598x.c | 42 ++++++++++++++++++++++++++++++------
> >  1 file changed, 35 insertions(+), 7 deletions(-)
> > 
> > diff --git a/drivers/usb/typec/tps6598x.c b/drivers/usb/typec/tps6598x.c
> > index 8b8406867c02..82f09cd9792d 100644
> > --- a/drivers/usb/typec/tps6598x.c
> > +++ b/drivers/usb/typec/tps6598x.c
> > @@ -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,6 +81,23 @@ struct tps6598x {
> >  	struct typec_capability typec_cap;
> >  };
> >  
> > +static int
> > +tps6598x_block_read(struct tps6598x *tps, u8 reg, void *val, ssize_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;
> > +
> 
> Sanity check ?
> 	if (data[0] != len)
> 		return -Esomething;

No. Then we would not even need the len parameter. The idea is to
allow reading a number of bytes specified by caller, regardless of the
maximum size of the block.


Thanks,

-- 
heikki

[1/2] usb: typec: tps6598x: handle block reads separately with plain-I2C adapters

[Heikki Krogerus]

On Fri, Apr 20, 2018 at 10:26:08AM -0700, Guenter Roeck wrote:
> On Wed, Apr 18, 2018 at 03:34:09PM +0300, Heikki Krogerus wrote:
> > 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")
> > Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> > ---
> >  drivers/usb/typec/tps6598x.c | 42 ++++++++++++++++++++++++++++++------
> >  1 file changed, 35 insertions(+), 7 deletions(-)
> > 
> > diff --git a/drivers/usb/typec/tps6598x.c b/drivers/usb/typec/tps6598x.c
> > index 8b8406867c02..82f09cd9792d 100644
> > --- a/drivers/usb/typec/tps6598x.c
> > +++ b/drivers/usb/typec/tps6598x.c
> > @@ -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,6 +81,23 @@ struct tps6598x {
> >  	struct typec_capability typec_cap;
> >  };
> >  
> > +static int
> > +tps6598x_block_read(struct tps6598x *tps, u8 reg, void *val, ssize_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;
> > +
> 
> Sanity check ?
> 	if (data[0] != len)
> 		return -Esomething;

No. Then we would not even need the len parameter. The idea is to
allow reading a number of bytes specified by caller, regardless of the
maximum size of the block.


Thanks,

Re: [PATCH 1/2] usb: typec: tps6598x: handle block reads separately with plain-I2C adapters

[Guenter Roeck]

On Mon, Apr 23, 2018 at 11:03:09AM +0300, Heikki Krogerus wrote:
> On Fri, Apr 20, 2018 at 10:26:08AM -0700, Guenter Roeck wrote:
> > On Wed, Apr 18, 2018 at 03:34:09PM +0300, Heikki Krogerus wrote:
> > > 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")
> > > Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> > > ---
> > >  drivers/usb/typec/tps6598x.c | 42 ++++++++++++++++++++++++++++++------
> > >  1 file changed, 35 insertions(+), 7 deletions(-)
> > > 
> > > diff --git a/drivers/usb/typec/tps6598x.c b/drivers/usb/typec/tps6598x.c
> > > index 8b8406867c02..82f09cd9792d 100644
> > > --- a/drivers/usb/typec/tps6598x.c
> > > +++ b/drivers/usb/typec/tps6598x.c
> > > @@ -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,6 +81,23 @@ struct tps6598x {
> > >  	struct typec_capability typec_cap;
> > >  };
> > >  
> > > +static int
> > > +tps6598x_block_read(struct tps6598x *tps, u8 reg, void *val, ssize_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;
> > > +
> > 
> > Sanity check ?
> > 	if (data[0] != len)
> > 		return -Esomething;
> 
> No. Then we would not even need the len parameter. The idea is to
> allow reading a number of bytes specified by caller, regardless of the
> maximum size of the block.
> 
If I2C_FUNC_I2C is not supported but I2C_FUNC_SMBUS_I2C_BLOCK is, the
regmap core will use i2c_smbus_read_i2c_block_data() and validate the
return length. It will return -EIO if it does not match. That seems
inconsistent. Also, I am not sure how you know that at least the minimum
required number of bytes is returned if the number of bytes requested
is larger than the number of bytes returned by the chip. Am I missing
something ?

Also, I notice that your patch does not touch tps6598x_read16(). Yet,
according to "TPS65981, TPS65982, and TPS65986 Host Interface Technical
Reference Manual", it appears that the 2-byte command used (0x3f) does
support/use block commands. Is that an oversight or on purpose ?

Thanks,
Guenter

[1/2] usb: typec: tps6598x: handle block reads separately with plain-I2C adapters

[Guenter Roeck]

On Mon, Apr 23, 2018 at 11:03:09AM +0300, Heikki Krogerus wrote:
> On Fri, Apr 20, 2018 at 10:26:08AM -0700, Guenter Roeck wrote:
> > On Wed, Apr 18, 2018 at 03:34:09PM +0300, Heikki Krogerus wrote:
> > > 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")
> > > Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> > > ---
> > >  drivers/usb/typec/tps6598x.c | 42 ++++++++++++++++++++++++++++++------
> > >  1 file changed, 35 insertions(+), 7 deletions(-)
> > > 
> > > diff --git a/drivers/usb/typec/tps6598x.c b/drivers/usb/typec/tps6598x.c
> > > index 8b8406867c02..82f09cd9792d 100644
> > > --- a/drivers/usb/typec/tps6598x.c
> > > +++ b/drivers/usb/typec/tps6598x.c
> > > @@ -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,6 +81,23 @@ struct tps6598x {
> > >  	struct typec_capability typec_cap;
> > >  };
> > >  
> > > +static int
> > > +tps6598x_block_read(struct tps6598x *tps, u8 reg, void *val, ssize_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;
> > > +
> > 
> > Sanity check ?
> > 	if (data[0] != len)
> > 		return -Esomething;
> 
> No. Then we would not even need the len parameter. The idea is to
> allow reading a number of bytes specified by caller, regardless of the
> maximum size of the block.
> 
If I2C_FUNC_I2C is not supported but I2C_FUNC_SMBUS_I2C_BLOCK is, the
regmap core will use i2c_smbus_read_i2c_block_data() and validate the
return length. It will return -EIO if it does not match. That seems
inconsistent. Also, I am not sure how you know that at least the minimum
required number of bytes is returned if the number of bytes requested
is larger than the number of bytes returned by the chip. Am I missing
something ?

Also, I notice that your patch does not touch tps6598x_read16(). Yet,
according to "TPS65981, TPS65982, and TPS65986 Host Interface Technical
Reference Manual", it appears that the 2-byte command used (0x3f) does
support/use block commands. Is that an oversight or on purpose ?

Thanks,
Guenter
---
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/2] usb: typec: tps6598x: handle block reads separately with plain-I2C adapters

[Heikki Krogerus]

On Mon, Apr 23, 2018 at 09:43:57AM -0700, Guenter Roeck wrote:
> On Mon, Apr 23, 2018 at 11:03:09AM +0300, Heikki Krogerus wrote:
> > On Fri, Apr 20, 2018 at 10:26:08AM -0700, Guenter Roeck wrote:
> > > On Wed, Apr 18, 2018 at 03:34:09PM +0300, Heikki Krogerus wrote:
> > > > 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")
> > > > Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> > > > ---
> > > >  drivers/usb/typec/tps6598x.c | 42 ++++++++++++++++++++++++++++++------
> > > >  1 file changed, 35 insertions(+), 7 deletions(-)
> > > > 
> > > > diff --git a/drivers/usb/typec/tps6598x.c b/drivers/usb/typec/tps6598x.c
> > > > index 8b8406867c02..82f09cd9792d 100644
> > > > --- a/drivers/usb/typec/tps6598x.c
> > > > +++ b/drivers/usb/typec/tps6598x.c
> > > > @@ -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,6 +81,23 @@ struct tps6598x {
> > > >  	struct typec_capability typec_cap;
> > > >  };
> > > >  
> > > > +static int
> > > > +tps6598x_block_read(struct tps6598x *tps, u8 reg, void *val, ssize_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;
> > > > +
> > > 
> > > Sanity check ?
> > > 	if (data[0] != len)
> > > 		return -Esomething;
> > 
> > No. Then we would not even need the len parameter. The idea is to
> > allow reading a number of bytes specified by caller, regardless of the
> > maximum size of the block.
> > 
> If I2C_FUNC_I2C is not supported but I2C_FUNC_SMBUS_I2C_BLOCK is, the
> regmap core will use i2c_smbus_read_i2c_block_data() and validate the
> return length. It will return -EIO if it does not match. That seems
> inconsistent.

For some reason I though that regmap-i2c supports
I2C_FUNC_SMBUS_BLOCK_DATA functionality instead of
I2C_FUNC_SMBUS_I2C_BLOCK_DATA. I stand corrected.

> Also, I am not sure how you know that at least the minimum
> required number of bytes is returned if the number of bytes requested
> is larger than the number of bytes returned by the chip. Am I missing
> something ?

If the slave chip returns less bytes then what we are asking from it,
the adapter driver should return an error, timeout most likely, no? Or
did I misunderstood the question?

In any case, I will add some sanity checks. I just wanted to make sure
we don't add useless checks.

> Also, I notice that your patch does not touch tps6598x_read16(). Yet,
> according to "TPS65981, TPS65982, and TPS65986 Host Interface Technical
> Reference Manual", it appears that the 2-byte command used (0x3f) does
> support/use block commands. Is that an oversight or on purpose ?

I did not change it because in my test I did not see the byte count in
the first byte, but I'm questioning myself now... I need to re-check
it. I may have done that test with wrong platform.


Thanks Guenter,

-- 
heikki

[1/2] usb: typec: tps6598x: handle block reads separately with plain-I2C adapters

[Heikki Krogerus]

On Mon, Apr 23, 2018 at 09:43:57AM -0700, Guenter Roeck wrote:
> On Mon, Apr 23, 2018 at 11:03:09AM +0300, Heikki Krogerus wrote:
> > On Fri, Apr 20, 2018 at 10:26:08AM -0700, Guenter Roeck wrote:
> > > On Wed, Apr 18, 2018 at 03:34:09PM +0300, Heikki Krogerus wrote:
> > > > 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")
> > > > Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> > > > ---
> > > >  drivers/usb/typec/tps6598x.c | 42 ++++++++++++++++++++++++++++++------
> > > >  1 file changed, 35 insertions(+), 7 deletions(-)
> > > > 
> > > > diff --git a/drivers/usb/typec/tps6598x.c b/drivers/usb/typec/tps6598x.c
> > > > index 8b8406867c02..82f09cd9792d 100644
> > > > --- a/drivers/usb/typec/tps6598x.c
> > > > +++ b/drivers/usb/typec/tps6598x.c
> > > > @@ -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,6 +81,23 @@ struct tps6598x {
> > > >  	struct typec_capability typec_cap;
> > > >  };
> > > >  
> > > > +static int
> > > > +tps6598x_block_read(struct tps6598x *tps, u8 reg, void *val, ssize_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;
> > > > +
> > > 
> > > Sanity check ?
> > > 	if (data[0] != len)
> > > 		return -Esomething;
> > 
> > No. Then we would not even need the len parameter. The idea is to
> > allow reading a number of bytes specified by caller, regardless of the
> > maximum size of the block.
> > 
> If I2C_FUNC_I2C is not supported but I2C_FUNC_SMBUS_I2C_BLOCK is, the
> regmap core will use i2c_smbus_read_i2c_block_data() and validate the
> return length. It will return -EIO if it does not match. That seems
> inconsistent.

For some reason I though that regmap-i2c supports
I2C_FUNC_SMBUS_BLOCK_DATA functionality instead of
I2C_FUNC_SMBUS_I2C_BLOCK_DATA. I stand corrected.

> Also, I am not sure how you know that at least the minimum
> required number of bytes is returned if the number of bytes requested
> is larger than the number of bytes returned by the chip. Am I missing
> something ?

If the slave chip returns less bytes then what we are asking from it,
the adapter driver should return an error, timeout most likely, no? Or
did I misunderstood the question?

In any case, I will add some sanity checks. I just wanted to make sure
we don't add useless checks.

> Also, I notice that your patch does not touch tps6598x_read16(). Yet,
> according to "TPS65981, TPS65982, and TPS65986 Host Interface Technical
> Reference Manual", it appears that the 2-byte command used (0x3f) does
> support/use block commands. Is that an oversight or on purpose ?

I did not change it because in my test I did not see the byte count in
the first byte, but I'm questioning myself now... I need to re-check
it. I may have done that test with wrong platform.


Thanks Guenter,

Re: [PATCH 1/2] usb: typec: tps6598x: handle block reads separately with plain-I2C adapters

[Guenter Roeck]

On 04/24/2018 04:46 AM, Heikki Krogerus wrote:
> On Mon, Apr 23, 2018 at 09:43:57AM -0700, Guenter Roeck wrote:
>> On Mon, Apr 23, 2018 at 11:03:09AM +0300, Heikki Krogerus wrote:
>>> On Fri, Apr 20, 2018 at 10:26:08AM -0700, Guenter Roeck wrote:
>>>> On Wed, Apr 18, 2018 at 03:34:09PM +0300, Heikki Krogerus wrote:
>>>>> 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")
>>>>> Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
>>>>> ---
>>>>>   drivers/usb/typec/tps6598x.c | 42 ++++++++++++++++++++++++++++++------
>>>>>   1 file changed, 35 insertions(+), 7 deletions(-)
>>>>>
>>>>> diff --git a/drivers/usb/typec/tps6598x.c b/drivers/usb/typec/tps6598x.c
>>>>> index 8b8406867c02..82f09cd9792d 100644
>>>>> --- a/drivers/usb/typec/tps6598x.c
>>>>> +++ b/drivers/usb/typec/tps6598x.c
>>>>> @@ -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,6 +81,23 @@ struct tps6598x {
>>>>>   	struct typec_capability typec_cap;
>>>>>   };
>>>>>   
>>>>> +static int
>>>>> +tps6598x_block_read(struct tps6598x *tps, u8 reg, void *val, ssize_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;
>>>>> +
>>>>
>>>> Sanity check ?
>>>> 	if (data[0] != len)
>>>> 		return -Esomething;
>>>
>>> No. Then we would not even need the len parameter. The idea is to
>>> allow reading a number of bytes specified by caller, regardless of the
>>> maximum size of the block.
>>>
>> If I2C_FUNC_I2C is not supported but I2C_FUNC_SMBUS_I2C_BLOCK is, the
>> regmap core will use i2c_smbus_read_i2c_block_data() and validate the
>> return length. It will return -EIO if it does not match. That seems
>> inconsistent.
> 
> For some reason I though that regmap-i2c supports
> I2C_FUNC_SMBUS_BLOCK_DATA functionality instead of
> I2C_FUNC_SMBUS_I2C_BLOCK_DATA. I stand corrected.
> 
>> Also, I am not sure how you know that at least the minimum
>> required number of bytes is returned if the number of bytes requested
>> is larger than the number of bytes returned by the chip. Am I missing
>> something ?
> 
> If the slave chip returns less bytes then what we are asking from it,
> the adapter driver should return an error, timeout most likely, no? Or
> did I misunderstood the question?
> 

The chip should return the number of bytes it has available and then NAK
the transfer. The chip driver should then report the number of bytes read
to its caller. This is why i2c_smbus_read_block_data() and friends return
not just 0/-errno, but the number of bytes read.

Guenter

> In any case, I will add some sanity checks. I just wanted to make sure
> we don't add useless checks.
> 
>> Also, I notice that your patch does not touch tps6598x_read16(). Yet,
>> according to "TPS65981, TPS65982, and TPS65986 Host Interface Technical
>> Reference Manual", it appears that the 2-byte command used (0x3f) does
>> support/use block commands. Is that an oversight or on purpose ?
> 
> I did not change it because in my test I did not see the byte count in
> the first byte, but I'm questioning myself now... I need to re-check
> it. I may have done that test with wrong platform.
> 
> 
> Thanks Guenter,
> 

[1/2] usb: typec: tps6598x: handle block reads separately with plain-I2C adapters

[Guenter Roeck]

On 04/24/2018 04:46 AM, Heikki Krogerus wrote:
> On Mon, Apr 23, 2018 at 09:43:57AM -0700, Guenter Roeck wrote:
>> On Mon, Apr 23, 2018 at 11:03:09AM +0300, Heikki Krogerus wrote:
>>> On Fri, Apr 20, 2018 at 10:26:08AM -0700, Guenter Roeck wrote:
>>>> On Wed, Apr 18, 2018 at 03:34:09PM +0300, Heikki Krogerus wrote:
>>>>> 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")
>>>>> Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
>>>>> ---
>>>>>   drivers/usb/typec/tps6598x.c | 42 ++++++++++++++++++++++++++++++------
>>>>>   1 file changed, 35 insertions(+), 7 deletions(-)
>>>>>
>>>>> diff --git a/drivers/usb/typec/tps6598x.c b/drivers/usb/typec/tps6598x.c
>>>>> index 8b8406867c02..82f09cd9792d 100644
>>>>> --- a/drivers/usb/typec/tps6598x.c
>>>>> +++ b/drivers/usb/typec/tps6598x.c
>>>>> @@ -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,6 +81,23 @@ struct tps6598x {
>>>>>   	struct typec_capability typec_cap;
>>>>>   };
>>>>>   
>>>>> +static int
>>>>> +tps6598x_block_read(struct tps6598x *tps, u8 reg, void *val, ssize_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;
>>>>> +
>>>>
>>>> Sanity check ?
>>>> 	if (data[0] != len)
>>>> 		return -Esomething;
>>>
>>> No. Then we would not even need the len parameter. The idea is to
>>> allow reading a number of bytes specified by caller, regardless of the
>>> maximum size of the block.
>>>
>> If I2C_FUNC_I2C is not supported but I2C_FUNC_SMBUS_I2C_BLOCK is, the
>> regmap core will use i2c_smbus_read_i2c_block_data() and validate the
>> return length. It will return -EIO if it does not match. That seems
>> inconsistent.
> 
> For some reason I though that regmap-i2c supports
> I2C_FUNC_SMBUS_BLOCK_DATA functionality instead of
> I2C_FUNC_SMBUS_I2C_BLOCK_DATA. I stand corrected.
> 
>> Also, I am not sure how you know that at least the minimum
>> required number of bytes is returned if the number of bytes requested
>> is larger than the number of bytes returned by the chip. Am I missing
>> something ?
> 
> If the slave chip returns less bytes then what we are asking from it,
> the adapter driver should return an error, timeout most likely, no? Or
> did I misunderstood the question?
> 

The chip should return the number of bytes it has available and then NAK
the transfer. The chip driver should then report the number of bytes read
to its caller. This is why i2c_smbus_read_block_data() and friends return
not just 0/-errno, but the number of bytes read.

Guenter

> In any case, I will add some sanity checks. I just wanted to make sure
> we don't add useless checks.
> 
>> Also, I notice that your patch does not touch tps6598x_read16(). Yet,
>> according to "TPS65981, TPS65982, and TPS65986 Host Interface Technical
>> Reference Manual", it appears that the 2-byte command used (0x3f) does
>> support/use block commands. Is that an oversight or on purpose ?
> 
> I did not change it because in my test I did not see the byte count in
> the first byte, but I'm questioning myself now... I need to re-check
> it. I may have done that test with wrong platform.
> 
> 
> Thanks Guenter,
>
---
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/2] usb: typec: tps6598x: handle block reads separately with plain-I2C adapters

[Heikki Krogerus]

On Tue, Apr 24, 2018 at 06:02:56AM -0700, Guenter Roeck wrote:
> The chip should return the number of bytes it has available and then NAK
> the transfer. The chip driver should then report the number of bytes read
> to its caller. This is why i2c_smbus_read_block_data() and friends return
> not just 0/-errno, but the number of bytes read.

Thanks for the info.


Br,

-- 
heikki

[1/2] usb: typec: tps6598x: handle block reads separately with plain-I2C adapters

[Heikki Krogerus]

On Tue, Apr 24, 2018 at 06:02:56AM -0700, Guenter Roeck wrote:
> The chip should return the number of bytes it has available and then NAK
> the transfer. The chip driver should then report the number of bytes read
> to its caller. This is why i2c_smbus_read_block_data() and friends return
> not just 0/-errno, but the number of bytes read.

Thanks for the info.


Br,