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. 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