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.kroge...@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 -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html