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@xxxxxxxxxxxxxxx>
> > > > ---
> > > > 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