* [PATCHv2] usb: typec: tps6598x: handle block reads separately with plain-I2C adapters @ 2018-04-25 14:22 Heikki Krogerus 2018-09-10 5:05 ` Nikolaus Voss 0 siblings, 1 reply; 49+ messages in thread From: Heikki Krogerus @ 2018-04-25 14:22 UTC (permalink / raw) To: Greg Kroah-Hartman; +Cc: Guenter Roeck, linux-usb 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 <linux@roeck-us.net> Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com> --- Changed since v1: - Added sanity check - Fixed also 2 byte reads --- drivers/usb/typec/tps6598x.c | 47 ++++++++++++++++++++++++++++++------ 1 file changed, 39 insertions(+), 8 deletions(-) diff --git a/drivers/usb/typec/tps6598x.c b/drivers/usb/typec/tps6598x.c index 8b8406867c02..4b4c8d271b27 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,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; ^ permalink raw reply related [flat|nested] 49+ messages in thread
* [PATCH] usb: typec: tps6598x: handle block writes separately with plain-I2C adapters @ 2018-09-10 5:05 ` Nikolaus Voss 0 siblings, 0 replies; 49+ messages in thread From: Nikolaus Voss @ 2018-09-10 5:05 UTC (permalink / raw) To: Heikki Krogerus, Greg Kroah-Hartman, Guenter Roeck Cc: linux-usb, linux-kernel, 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 <nikolaus.voss@loewensteinmedical.de> --- drivers/usb/typec/tps6598x.c | 22 ++++++++++++++++++---- 1 file changed, 18 insertions(+), 4 deletions(-) diff --git a/drivers/usb/typec/tps6598x.c b/drivers/usb/typec/tps6598x.c index c84c8c189e90..57a3e6c5c175 100644 --- a/drivers/usb/typec/tps6598x.c +++ b/drivers/usb/typec/tps6598x.c @@ -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) -- 2.17.1 ^ permalink raw reply related [flat|nested] 49+ messages in thread
* usb: typec: tps6598x: handle block writes separately with plain-I2C adapters @ 2018-09-10 5:05 ` Nikolaus Voss 0 siblings, 0 replies; 49+ messages in thread From: Nikolaus Voss @ 2018-09-10 5:05 UTC (permalink / raw) To: Heikki Krogerus, Greg Kroah-Hartman, Guenter Roeck Cc: linux-usb, linux-kernel, 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 <nikolaus.voss@loewensteinmedical.de> --- drivers/usb/typec/tps6598x.c | 22 ++++++++++++++++++---- 1 file changed, 18 insertions(+), 4 deletions(-) diff --git a/drivers/usb/typec/tps6598x.c b/drivers/usb/typec/tps6598x.c index c84c8c189e90..57a3e6c5c175 100644 --- a/drivers/usb/typec/tps6598x.c +++ b/drivers/usb/typec/tps6598x.c @@ -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) ^ permalink raw reply related [flat|nested] 49+ messages in thread
* Re: [PATCH] usb: typec: tps6598x: handle block writes separately with plain-I2C adapters @ 2019-02-20 12:14 ` Heikki Krogerus 0 siblings, 0 replies; 49+ messages in thread From: Heikki Krogerus @ 2019-02-20 12:14 UTC (permalink / raw) To: Nikolaus Voss Cc: Greg Kroah-Hartman, Guenter Roeck, linux-usb, linux-kernel, nikolaus.voss Hi, On Mon, Sep 10, 2018 at 07:05:01AM +0200, Nikolaus Voss wrote: > 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 <nikolaus.voss@loewensteinmedical.de> > --- > drivers/usb/typec/tps6598x.c | 22 ++++++++++++++++++---- > 1 file changed, 18 insertions(+), 4 deletions(-) > > diff --git a/drivers/usb/typec/tps6598x.c b/drivers/usb/typec/tps6598x.c > index c84c8c189e90..57a3e6c5c175 100644 > --- a/drivers/usb/typec/tps6598x.c > +++ b/drivers/usb/typec/tps6598x.c > @@ -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) 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, -- heikki ^ permalink raw reply [flat|nested] 49+ messages in thread
* usb: typec: tps6598x: handle block writes separately with plain-I2C adapters @ 2019-02-20 12:14 ` Heikki Krogerus 0 siblings, 0 replies; 49+ messages in thread From: Heikki Krogerus @ 2019-02-20 12:14 UTC (permalink / raw) To: Nikolaus Voss Cc: Greg Kroah-Hartman, Guenter Roeck, linux-usb, linux-kernel, nikolaus.voss Hi, On Mon, Sep 10, 2018 at 07:05:01AM +0200, Nikolaus Voss wrote: > 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 <nikolaus.voss@loewensteinmedical.de> > --- > drivers/usb/typec/tps6598x.c | 22 ++++++++++++++++++---- > 1 file changed, 18 insertions(+), 4 deletions(-) > > diff --git a/drivers/usb/typec/tps6598x.c b/drivers/usb/typec/tps6598x.c > index c84c8c189e90..57a3e6c5c175 100644 > --- a/drivers/usb/typec/tps6598x.c > +++ b/drivers/usb/typec/tps6598x.c > @@ -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) 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, ^ permalink raw reply [flat|nested] 49+ messages in thread
* Re: [PATCH] usb: typec: tps6598x: handle block writes separately with plain-I2C adapters @ 2019-02-20 12:56 ` Nikolaus Voss 0 siblings, 0 replies; 49+ messages in thread From: Nikolaus Voss @ 2019-02-20 12:56 UTC (permalink / raw) To: Heikki Krogerus Cc: Greg Kroah-Hartman, Guenter Roeck, linux-usb, linux-kernel Hi, On Wed, 20 Feb 2019, Heikki Krogerus wrote: > Hi, > > On Mon, Sep 10, 2018 at 07:05:01AM +0200, Nikolaus Voss wrote: >> 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 <nikolaus.voss@loewensteinmedical.de> >> --- >> drivers/usb/typec/tps6598x.c | 22 ++++++++++++++++++---- >> 1 file changed, 18 insertions(+), 4 deletions(-) >> >> diff --git a/drivers/usb/typec/tps6598x.c b/drivers/usb/typec/tps6598x.c >> index c84c8c189e90..57a3e6c5c175 100644 >> --- a/drivers/usb/typec/tps6598x.c >> +++ b/drivers/usb/typec/tps6598x.c >> @@ -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) > > You need to fix tps6598x_exec_cmd() as well. Right, thanks. I will fix that. > > 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 ^ permalink raw reply [flat|nested] 49+ messages in thread
* usb: typec: tps6598x: handle block writes separately with plain-I2C adapters @ 2019-02-20 12:56 ` Nikolaus Voss 0 siblings, 0 replies; 49+ messages in thread From: Nikolaus Voss @ 2019-02-20 12:56 UTC (permalink / raw) To: Heikki Krogerus Cc: Greg Kroah-Hartman, Guenter Roeck, linux-usb, linux-kernel Hi, On Wed, 20 Feb 2019, Heikki Krogerus wrote: > Hi, > > On Mon, Sep 10, 2018 at 07:05:01AM +0200, Nikolaus Voss wrote: >> 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 <nikolaus.voss@loewensteinmedical.de> >> --- >> drivers/usb/typec/tps6598x.c | 22 ++++++++++++++++++---- >> 1 file changed, 18 insertions(+), 4 deletions(-) >> >> diff --git a/drivers/usb/typec/tps6598x.c b/drivers/usb/typec/tps6598x.c >> index c84c8c189e90..57a3e6c5c175 100644 >> --- a/drivers/usb/typec/tps6598x.c >> +++ b/drivers/usb/typec/tps6598x.c >> @@ -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) > > You need to fix tps6598x_exec_cmd() as well. Right, thanks. I will fix that. > > 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 ^ permalink raw reply [flat|nested] 49+ messages in thread
* [PATCHv2] usb: typec: tps6598x: handle block writes separately with plain-I2C adapters @ 2019-02-20 12:57 ` Nikolaus Voss 0 siblings, 0 replies; 49+ messages in thread From: Nikolaus Voss @ 2019-02-20 12:57 UTC (permalink / raw) To: Heikki Krogerus, Greg Kroah-Hartman, Guenter Roeck Cc: linux-usb, linux-kernel, 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 <nikolaus.voss@loewensteinmedical.de> --- v2: fix tps6598x_exec_cmd also --- drivers/usb/typec/tps6598x.c | 26 ++++++++++++++++++++------ 1 file changed, 20 insertions(+), 6 deletions(-) diff --git a/drivers/usb/typec/tps6598x.c b/drivers/usb/typec/tps6598x.c index c84c8c189e90..c54b73fb2a2f 100644 --- a/drivers/usb/typec/tps6598x.c +++ b/drivers/usb/typec/tps6598x.c @@ -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; } -- 2.17.1 ^ permalink raw reply related [flat|nested] 49+ messages in thread
* [PATCHv2] usb: typec: tps6598x: handle block writes separately with plain-I2C adapters @ 2019-02-20 12:57 ` Nikolaus Voss 0 siblings, 0 replies; 49+ messages in thread From: Nikolaus Voss @ 2019-02-20 12:57 UTC (permalink / raw) To: Heikki Krogerus, Greg Kroah-Hartman, Guenter Roeck Cc: linux-usb, linux-kernel, 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 <nikolaus.voss@loewensteinmedical.de> --- v2: fix tps6598x_exec_cmd also --- drivers/usb/typec/tps6598x.c | 26 ++++++++++++++++++++------ 1 file changed, 20 insertions(+), 6 deletions(-) diff --git a/drivers/usb/typec/tps6598x.c b/drivers/usb/typec/tps6598x.c index c84c8c189e90..c54b73fb2a2f 100644 --- a/drivers/usb/typec/tps6598x.c +++ b/drivers/usb/typec/tps6598x.c @@ -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; } ^ permalink raw reply related [flat|nested] 49+ messages in thread
* Re: [PATCHv2] usb: typec: tps6598x: handle block writes separately with plain-I2C adapters @ 2019-02-20 13:30 ` Heikki Krogerus 0 siblings, 0 replies; 49+ messages in thread From: Heikki Krogerus @ 2019-02-20 13:30 UTC (permalink / raw) To: Nikolaus Voss Cc: Greg Kroah-Hartman, Guenter Roeck, linux-usb, linux-kernel, nikolaus.voss On Wed, Feb 20, 2019 at 01:57:30PM +0100, Nikolaus Voss wrote: > 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 <nikolaus.voss@loewensteinmedical.de> You are missing a "from" line with address that matches your SoB address. thanks, -- heikki ^ permalink raw reply [flat|nested] 49+ messages in thread
* [PATCHv2] usb: typec: tps6598x: handle block writes separately with plain-I2C adapters @ 2019-02-20 13:30 ` Heikki Krogerus 0 siblings, 0 replies; 49+ messages in thread From: Heikki Krogerus @ 2019-02-20 13:30 UTC (permalink / raw) To: Nikolaus Voss Cc: Greg Kroah-Hartman, Guenter Roeck, linux-usb, linux-kernel, nikolaus.voss On Wed, Feb 20, 2019 at 01:57:30PM +0100, Nikolaus Voss wrote: > 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 <nikolaus.voss@loewensteinmedical.de> You are missing a "from" line with address that matches your SoB address. thanks, ^ permalink raw reply [flat|nested] 49+ messages in thread
* Re: [PATCHv2] usb: typec: tps6598x: handle block writes separately with plain-I2C adapters @ 2019-02-20 13:38 ` Nikolaus Voss 0 siblings, 0 replies; 49+ messages in thread From: Nikolaus Voss @ 2019-02-20 13:38 UTC (permalink / raw) To: Heikki Krogerus Cc: Greg Kroah-Hartman, Guenter Roeck, linux-usb, linux-kernel On Wed, 20 Feb 2019, Heikki Krogerus wrote: > On Wed, Feb 20, 2019 at 01:57:30PM +0100, Nikolaus Voss wrote: >> 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 <nikolaus.voss@loewensteinmedical.de> > > 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 <nv@vosn.de> Nikolaus ^ permalink raw reply [flat|nested] 49+ messages in thread
* [PATCHv2] usb: typec: tps6598x: handle block writes separately with plain-I2C adapters @ 2019-02-20 13:38 ` Nikolaus Voss 0 siblings, 0 replies; 49+ messages in thread From: Nikolaus Voss @ 2019-02-20 13:38 UTC (permalink / raw) To: Heikki Krogerus Cc: Greg Kroah-Hartman, Guenter Roeck, linux-usb, linux-kernel On Wed, 20 Feb 2019, Heikki Krogerus wrote: > On Wed, Feb 20, 2019 at 01:57:30PM +0100, Nikolaus Voss wrote: >> 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 <nikolaus.voss@loewensteinmedical.de> > > 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 <nv@vosn.de> Nikolaus ^ permalink raw reply [flat|nested] 49+ messages in thread
* Re: [PATCHv2] usb: typec: tps6598x: handle block writes separately with plain-I2C adapters @ 2019-02-20 14:14 ` Heikki Krogerus 0 siblings, 0 replies; 49+ messages in thread From: Heikki Krogerus @ 2019-02-20 14:14 UTC (permalink / raw) To: Nikolaus Voss; +Cc: Greg Kroah-Hartman, Guenter Roeck, linux-usb, linux-kernel On Wed, Feb 20, 2019 at 02:38:47PM +0100, Nikolaus Voss wrote: > On Wed, 20 Feb 2019, Heikki Krogerus wrote: > > On Wed, Feb 20, 2019 at 01:57:30PM +0100, Nikolaus Voss wrote: > > > 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 <nikolaus.voss@loewensteinmedical.de> > > > > 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 <nikolaus.voss@loewensteinmedical.de>' \ GIT_AUTHOR_IDENT='Nikolaus Voss <nikolaus.voss@loewensteinmedical.de>' \ git commit -a -C $MY_COMMIT Then: % git format-patch HEAD^ % git send-email ... thanks, -- heikki ^ permalink raw reply [flat|nested] 49+ messages in thread
* [PATCHv2] usb: typec: tps6598x: handle block writes separately with plain-I2C adapters @ 2019-02-20 14:14 ` Heikki Krogerus 0 siblings, 0 replies; 49+ messages in thread From: Heikki Krogerus @ 2019-02-20 14:14 UTC (permalink / raw) To: Nikolaus Voss; +Cc: Greg Kroah-Hartman, Guenter Roeck, linux-usb, linux-kernel On Wed, Feb 20, 2019 at 02:38:47PM +0100, Nikolaus Voss wrote: > On Wed, 20 Feb 2019, Heikki Krogerus wrote: > > On Wed, Feb 20, 2019 at 01:57:30PM +0100, Nikolaus Voss wrote: > > > 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 <nikolaus.voss@loewensteinmedical.de> > > > > 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 <nikolaus.voss@loewensteinmedical.de>' \ GIT_AUTHOR_IDENT='Nikolaus Voss <nikolaus.voss@loewensteinmedical.de>' \ git commit -a -C $MY_COMMIT Then: % git format-patch HEAD^ % git send-email ... thanks, ^ permalink raw reply [flat|nested] 49+ messages in thread
* Re: [PATCHv2] usb: typec: tps6598x: handle block writes separately with plain-I2C adapters @ 2019-02-20 14:30 ` Heikki Krogerus 0 siblings, 0 replies; 49+ messages in thread From: Heikki Krogerus @ 2019-02-20 14:30 UTC (permalink / raw) To: Nikolaus Voss; +Cc: Greg Kroah-Hartman, Guenter Roeck, linux-usb, linux-kernel On Wed, Feb 20, 2019 at 04:14:23PM +0200, Heikki Krogerus wrote: > On Wed, Feb 20, 2019 at 02:38:47PM +0100, Nikolaus Voss wrote: > > On Wed, 20 Feb 2019, Heikki Krogerus wrote: > > > On Wed, Feb 20, 2019 at 01:57:30PM +0100, Nikolaus Voss wrote: > > > > 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 <nikolaus.voss@loewensteinmedical.de> > > > > > > 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 <nikolaus.voss@loewensteinmedical.de>' \ > GIT_AUTHOR_IDENT='Nikolaus Voss <nikolaus.voss@loewensteinmedical.de>' \ > git commit -a -C $MY_COMMIT Correction here: % GIT_COMMITTER_IDENT='Nikolaus Voss <nikolaus.voss@loewensteinmedical.de>' \ GIT_AUTHOR_IDENT='Nikolaus Voss <nikolaus.voss@loewensteinmedical.de>' \ git commit -a -C $MY_COMMIT --reset-author That "--reset-author" was missing. Sorry for that. thanks, -- heikki ^ permalink raw reply [flat|nested] 49+ messages in thread
* [PATCHv2] usb: typec: tps6598x: handle block writes separately with plain-I2C adapters @ 2019-02-20 14:30 ` Heikki Krogerus 0 siblings, 0 replies; 49+ messages in thread From: Heikki Krogerus @ 2019-02-20 14:30 UTC (permalink / raw) To: Nikolaus Voss; +Cc: Greg Kroah-Hartman, Guenter Roeck, linux-usb, linux-kernel On Wed, Feb 20, 2019 at 04:14:23PM +0200, Heikki Krogerus wrote: > On Wed, Feb 20, 2019 at 02:38:47PM +0100, Nikolaus Voss wrote: > > On Wed, 20 Feb 2019, Heikki Krogerus wrote: > > > On Wed, Feb 20, 2019 at 01:57:30PM +0100, Nikolaus Voss wrote: > > > > 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 <nikolaus.voss@loewensteinmedical.de> > > > > > > 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 <nikolaus.voss@loewensteinmedical.de>' \ > GIT_AUTHOR_IDENT='Nikolaus Voss <nikolaus.voss@loewensteinmedical.de>' \ > git commit -a -C $MY_COMMIT Correction here: % GIT_COMMITTER_IDENT='Nikolaus Voss <nikolaus.voss@loewensteinmedical.de>' \ GIT_AUTHOR_IDENT='Nikolaus Voss <nikolaus.voss@loewensteinmedical.de>' \ git commit -a -C $MY_COMMIT --reset-author That "--reset-author" was missing. Sorry for that. thanks, ^ permalink raw reply [flat|nested] 49+ messages in thread
* Re: [PATCHv2] usb: typec: tps6598x: handle block writes separately with plain-I2C adapters @ 2019-02-20 15:08 ` Nikolaus Voss 0 siblings, 0 replies; 49+ messages in thread From: Nikolaus Voss @ 2019-02-20 15:08 UTC (permalink / raw) To: Heikki Krogerus Cc: Greg Kroah-Hartman, Guenter Roeck, linux-usb, linux-kernel On Wed, 20 Feb 2019, Heikki Krogerus wrote: > On Wed, Feb 20, 2019 at 04:14:23PM +0200, Heikki Krogerus wrote: >> On Wed, Feb 20, 2019 at 02:38:47PM +0100, Nikolaus Voss wrote: >>> On Wed, 20 Feb 2019, Heikki Krogerus wrote: >>>> On Wed, Feb 20, 2019 at 01:57:30PM +0100, Nikolaus Voss wrote: >>>>> 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 <nikolaus.voss@loewensteinmedical.de> >>>> >>>> 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 <nikolaus.voss@loewensteinmedical.de>' \ >> GIT_AUTHOR_IDENT='Nikolaus Voss <nikolaus.voss@loewensteinmedical.de>' \ >> git commit -a -C $MY_COMMIT > > Correction here: > > % GIT_COMMITTER_IDENT='Nikolaus Voss <nikolaus.voss@loewensteinmedical.de>' \ > GIT_AUTHOR_IDENT='Nikolaus Voss <nikolaus.voss@loewensteinmedical.de>' \ > 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 ^ permalink raw reply [flat|nested] 49+ messages in thread
* [PATCHv2] usb: typec: tps6598x: handle block writes separately with plain-I2C adapters @ 2019-02-20 15:08 ` Nikolaus Voss 0 siblings, 0 replies; 49+ messages in thread From: Nikolaus Voss @ 2019-02-20 15:08 UTC (permalink / raw) To: Heikki Krogerus Cc: Greg Kroah-Hartman, Guenter Roeck, linux-usb, linux-kernel On Wed, 20 Feb 2019, Heikki Krogerus wrote: > On Wed, Feb 20, 2019 at 04:14:23PM +0200, Heikki Krogerus wrote: >> On Wed, Feb 20, 2019 at 02:38:47PM +0100, Nikolaus Voss wrote: >>> On Wed, 20 Feb 2019, Heikki Krogerus wrote: >>>> On Wed, Feb 20, 2019 at 01:57:30PM +0100, Nikolaus Voss wrote: >>>>> 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 <nikolaus.voss@loewensteinmedical.de> >>>> >>>> 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 <nikolaus.voss@loewensteinmedical.de>' \ >> GIT_AUTHOR_IDENT='Nikolaus Voss <nikolaus.voss@loewensteinmedical.de>' \ >> git commit -a -C $MY_COMMIT > > Correction here: > > % GIT_COMMITTER_IDENT='Nikolaus Voss <nikolaus.voss@loewensteinmedical.de>' \ > GIT_AUTHOR_IDENT='Nikolaus Voss <nikolaus.voss@loewensteinmedical.de>' \ > 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 ^ permalink raw reply [flat|nested] 49+ messages in thread
* Re: [PATCHv2] usb: typec: tps6598x: handle block writes separately with plain-I2C adapters @ 2019-02-20 14:45 ` Guenter Roeck 0 siblings, 0 replies; 49+ messages in thread From: Guenter Roeck @ 2019-02-20 14:45 UTC (permalink / raw) To: Nikolaus Voss, Heikki Krogerus, Greg Kroah-Hartman Cc: linux-usb, linux-kernel, nikolaus.voss On 2/20/19 4:57 AM, Nikolaus Voss wrote: > 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 <nikolaus.voss@loewensteinmedical.de> > --- > v2: fix tps6598x_exec_cmd also > --- > drivers/usb/typec/tps6598x.c | 26 ++++++++++++++++++++------ > 1 file changed, 20 insertions(+), 6 deletions(-) > > diff --git a/drivers/usb/typec/tps6598x.c b/drivers/usb/typec/tps6598x.c > index c84c8c189e90..c54b73fb2a2f 100644 > --- a/drivers/usb/typec/tps6598x.c > +++ b/drivers/usb/typec/tps6598x.c > @@ -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 > + 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; > } > ^ permalink raw reply [flat|nested] 49+ messages in thread
* [PATCHv2] usb: typec: tps6598x: handle block writes separately with plain-I2C adapters @ 2019-02-20 14:45 ` Guenter Roeck 0 siblings, 0 replies; 49+ messages in thread From: Guenter Roeck @ 2019-02-20 14:45 UTC (permalink / raw) To: Nikolaus Voss, Heikki Krogerus, Greg Kroah-Hartman Cc: linux-usb, linux-kernel, nikolaus.voss On 2/20/19 4:57 AM, Nikolaus Voss wrote: > 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 <nikolaus.voss@loewensteinmedical.de> > --- > v2: fix tps6598x_exec_cmd also > --- > drivers/usb/typec/tps6598x.c | 26 ++++++++++++++++++++------ > 1 file changed, 20 insertions(+), 6 deletions(-) > > diff --git a/drivers/usb/typec/tps6598x.c b/drivers/usb/typec/tps6598x.c > index c84c8c189e90..c54b73fb2a2f 100644 > --- a/drivers/usb/typec/tps6598x.c > +++ b/drivers/usb/typec/tps6598x.c > @@ -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 > + 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; > } > ^ permalink raw reply [flat|nested] 49+ messages in thread
* Re: [PATCHv2] usb: typec: tps6598x: handle block writes separately with plain-I2C adapters @ 2019-02-20 15:18 ` Nikolaus Voss 0 siblings, 0 replies; 49+ messages in thread From: Nikolaus Voss @ 2019-02-20 15:18 UTC (permalink / raw) To: Guenter Roeck Cc: Heikki Krogerus, Greg Kroah-Hartman, linux-usb, linux-kernel On Wed, 20 Feb 2019, Guenter Roeck wrote: > On 2/20/19 4:57 AM, Nikolaus Voss wrote: >> 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 <nikolaus.voss@loewensteinmedical.de> >> --- >> v2: fix tps6598x_exec_cmd also >> --- >> drivers/usb/typec/tps6598x.c | 26 ++++++++++++++++++++------ >> 1 file changed, 20 insertions(+), 6 deletions(-) >> >> diff --git a/drivers/usb/typec/tps6598x.c b/drivers/usb/typec/tps6598x.c >> index c84c8c189e90..c54b73fb2a2f 100644 >> --- a/drivers/usb/typec/tps6598x.c >> +++ b/drivers/usb/typec/tps6598x.c >> @@ -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. Thanks for the pointer, I fixed this... Nikolaus ^ permalink raw reply [flat|nested] 49+ messages in thread
* [PATCHv2] usb: typec: tps6598x: handle block writes separately with plain-I2C adapters @ 2019-02-20 15:18 ` Nikolaus Voss 0 siblings, 0 replies; 49+ messages in thread From: Nikolaus Voss @ 2019-02-20 15:18 UTC (permalink / raw) To: Guenter Roeck Cc: Heikki Krogerus, Greg Kroah-Hartman, linux-usb, linux-kernel On Wed, 20 Feb 2019, Guenter Roeck wrote: > On 2/20/19 4:57 AM, Nikolaus Voss wrote: >> 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 <nikolaus.voss@loewensteinmedical.de> >> --- >> v2: fix tps6598x_exec_cmd also >> --- >> drivers/usb/typec/tps6598x.c | 26 ++++++++++++++++++++------ >> 1 file changed, 20 insertions(+), 6 deletions(-) >> >> diff --git a/drivers/usb/typec/tps6598x.c b/drivers/usb/typec/tps6598x.c >> index c84c8c189e90..c54b73fb2a2f 100644 >> --- a/drivers/usb/typec/tps6598x.c >> +++ b/drivers/usb/typec/tps6598x.c >> @@ -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. Thanks for the pointer, I fixed this... Nikolaus ^ permalink raw reply [flat|nested] 49+ messages in thread
* Re: [PATCHv2] usb: typec: tps6598x: handle block writes separately with plain-I2C adapters @ 2019-02-20 15:02 ` Greg Kroah-Hartman 0 siblings, 0 replies; 49+ messages in thread From: Greg Kroah-Hartman @ 2019-02-20 15:02 UTC (permalink / raw) To: Nikolaus Voss Cc: Heikki Krogerus, Guenter Roeck, linux-usb, linux-kernel, nikolaus.voss On Wed, Feb 20, 2019 at 01:57:30PM +0100, Nikolaus Voss wrote: > 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 <nikolaus.voss@loewensteinmedical.de> > --- 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. > v2: fix tps6598x_exec_cmd also > --- > drivers/usb/typec/tps6598x.c | 26 ++++++++++++++++++++------ > 1 file changed, 20 insertions(+), 6 deletions(-) > > diff --git a/drivers/usb/typec/tps6598x.c b/drivers/usb/typec/tps6598x.c > index c84c8c189e90..c54b73fb2a2f 100644 > --- a/drivers/usb/typec/tps6598x.c > +++ b/drivers/usb/typec/tps6598x.c > @@ -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 ^ permalink raw reply [flat|nested] 49+ messages in thread
* [PATCHv2] usb: typec: tps6598x: handle block writes separately with plain-I2C adapters @ 2019-02-20 15:02 ` Greg Kroah-Hartman 0 siblings, 0 replies; 49+ messages in thread From: Greg Kroah-Hartman @ 2019-02-20 15:02 UTC (permalink / raw) To: Nikolaus Voss Cc: Heikki Krogerus, Guenter Roeck, linux-usb, linux-kernel, nikolaus.voss On Wed, Feb 20, 2019 at 01:57:30PM +0100, Nikolaus Voss wrote: > 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 <nikolaus.voss@loewensteinmedical.de> > --- 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. > v2: fix tps6598x_exec_cmd also > --- > drivers/usb/typec/tps6598x.c | 26 ++++++++++++++++++++------ > 1 file changed, 20 insertions(+), 6 deletions(-) > > diff --git a/drivers/usb/typec/tps6598x.c b/drivers/usb/typec/tps6598x.c > index c84c8c189e90..c54b73fb2a2f 100644 > --- a/drivers/usb/typec/tps6598x.c > +++ b/drivers/usb/typec/tps6598x.c > @@ -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 ^ permalink raw reply [flat|nested] 49+ messages in thread
* Re: [PATCHv2] usb: typec: tps6598x: handle block writes separately with plain-I2C adapters @ 2019-02-20 15:22 ` Nikolaus Voss 0 siblings, 0 replies; 49+ messages in thread From: Nikolaus Voss @ 2019-02-20 15:22 UTC (permalink / raw) To: Greg Kroah-Hartman Cc: Heikki Krogerus, Guenter Roeck, linux-usb, linux-kernel On Wed, 20 Feb 2019, Greg Kroah-Hartman wrote: > On Wed, Feb 20, 2019 at 01:57:30PM +0100, Nikolaus Voss wrote: >> 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 <nikolaus.voss@loewensteinmedical.de> >> --- > > 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. > > > >> v2: fix tps6598x_exec_cmd also >> --- >> drivers/usb/typec/tps6598x.c | 26 ++++++++++++++++++++------ >> 1 file changed, 20 insertions(+), 6 deletions(-) >> >> diff --git a/drivers/usb/typec/tps6598x.c b/drivers/usb/typec/tps6598x.c >> index c84c8c189e90..c54b73fb2a2f 100644 >> --- a/drivers/usb/typec/tps6598x.c >> +++ b/drivers/usb/typec/tps6598x.c >> @@ -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 :( I must admit I'm developing on 4.19 stable series, so no warnings... Nikolaus ^ permalink raw reply [flat|nested] 49+ messages in thread
* [PATCHv2] usb: typec: tps6598x: handle block writes separately with plain-I2C adapters @ 2019-02-20 15:22 ` Nikolaus Voss 0 siblings, 0 replies; 49+ messages in thread From: Nikolaus Voss @ 2019-02-20 15:22 UTC (permalink / raw) To: Greg Kroah-Hartman Cc: Heikki Krogerus, Guenter Roeck, linux-usb, linux-kernel On Wed, 20 Feb 2019, Greg Kroah-Hartman wrote: > On Wed, Feb 20, 2019 at 01:57:30PM +0100, Nikolaus Voss wrote: >> 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 <nikolaus.voss@loewensteinmedical.de> >> --- > > 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. > > > >> v2: fix tps6598x_exec_cmd also >> --- >> drivers/usb/typec/tps6598x.c | 26 ++++++++++++++++++++------ >> 1 file changed, 20 insertions(+), 6 deletions(-) >> >> diff --git a/drivers/usb/typec/tps6598x.c b/drivers/usb/typec/tps6598x.c >> index c84c8c189e90..c54b73fb2a2f 100644 >> --- a/drivers/usb/typec/tps6598x.c >> +++ b/drivers/usb/typec/tps6598x.c >> @@ -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 :( I must admit I'm developing on 4.19 stable series, so no warnings... Nikolaus ^ permalink raw reply [flat|nested] 49+ messages in thread
* Re: [PATCHv2] usb: typec: tps6598x: handle block writes separately with plain-I2C adapters @ 2019-02-20 16:22 ` Greg Kroah-Hartman 0 siblings, 0 replies; 49+ messages in thread From: Greg Kroah-Hartman @ 2019-02-20 16:22 UTC (permalink / raw) To: Nikolaus Voss; +Cc: Heikki Krogerus, Guenter Roeck, linux-usb, linux-kernel On Wed, Feb 20, 2019 at 04:22:00PM +0100, Nikolaus Voss wrote: > > > v2: fix tps6598x_exec_cmd also > > > --- > > > drivers/usb/typec/tps6598x.c | 26 ++++++++++++++++++++------ > > > 1 file changed, 20 insertions(+), 6 deletions(-) > > > > > > diff --git a/drivers/usb/typec/tps6598x.c b/drivers/usb/typec/tps6598x.c > > > index c84c8c189e90..c54b73fb2a2f 100644 > > > --- a/drivers/usb/typec/tps6598x.c > > > +++ b/drivers/usb/typec/tps6598x.c > > > @@ -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 :( > > 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 ^ permalink raw reply [flat|nested] 49+ messages in thread
* [PATCHv2] usb: typec: tps6598x: handle block writes separately with plain-I2C adapters @ 2019-02-20 16:22 ` Greg Kroah-Hartman 0 siblings, 0 replies; 49+ messages in thread From: Greg Kroah-Hartman @ 2019-02-20 16:22 UTC (permalink / raw) To: Nikolaus Voss; +Cc: Heikki Krogerus, Guenter Roeck, linux-usb, linux-kernel On Wed, Feb 20, 2019 at 04:22:00PM +0100, Nikolaus Voss wrote: > > > v2: fix tps6598x_exec_cmd also > > > --- > > > drivers/usb/typec/tps6598x.c | 26 ++++++++++++++++++++------ > > > 1 file changed, 20 insertions(+), 6 deletions(-) > > > > > > diff --git a/drivers/usb/typec/tps6598x.c b/drivers/usb/typec/tps6598x.c > > > index c84c8c189e90..c54b73fb2a2f 100644 > > > --- a/drivers/usb/typec/tps6598x.c > > > +++ b/drivers/usb/typec/tps6598x.c > > > @@ -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 :( > > 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 ^ permalink raw reply [flat|nested] 49+ messages in thread
* Re: [PATCHv2] usb: typec: tps6598x: handle block writes separately with plain-I2C adapters @ 2019-02-21 8:37 ` Nikolaus Voss 0 siblings, 0 replies; 49+ messages in thread From: Nikolaus Voss @ 2019-02-21 8:37 UTC (permalink / raw) To: Greg Kroah-Hartman Cc: Heikki Krogerus, Guenter Roeck, linux-usb, linux-kernel Hi Greg, On Wed, 20 Feb 2019, Greg Kroah-Hartman wrote: > On Wed, Feb 20, 2019 at 04:22:00PM +0100, Nikolaus Voss wrote: >>>> v2: fix tps6598x_exec_cmd also >>>> --- >>>> drivers/usb/typec/tps6598x.c | 26 ++++++++++++++++++++------ >>>> 1 file changed, 20 insertions(+), 6 deletions(-) >>>> >>>> diff --git a/drivers/usb/typec/tps6598x.c b/drivers/usb/typec/tps6598x.c >>>> index c84c8c189e90..c54b73fb2a2f 100644 >>>> --- a/drivers/usb/typec/tps6598x.c >>>> +++ b/drivers/usb/typec/tps6598x.c >>>> @@ -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 :( >> >> 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 ^ permalink raw reply [flat|nested] 49+ messages in thread
* [PATCHv2] usb: typec: tps6598x: handle block writes separately with plain-I2C adapters @ 2019-02-21 8:37 ` Nikolaus Voss 0 siblings, 0 replies; 49+ messages in thread From: Nikolaus Voss @ 2019-02-21 8:37 UTC (permalink / raw) To: Greg Kroah-Hartman Cc: Heikki Krogerus, Guenter Roeck, linux-usb, linux-kernel Hi Greg, On Wed, 20 Feb 2019, Greg Kroah-Hartman wrote: > On Wed, Feb 20, 2019 at 04:22:00PM +0100, Nikolaus Voss wrote: >>>> v2: fix tps6598x_exec_cmd also >>>> --- >>>> drivers/usb/typec/tps6598x.c | 26 ++++++++++++++++++++------ >>>> 1 file changed, 20 insertions(+), 6 deletions(-) >>>> >>>> diff --git a/drivers/usb/typec/tps6598x.c b/drivers/usb/typec/tps6598x.c >>>> index c84c8c189e90..c54b73fb2a2f 100644 >>>> --- a/drivers/usb/typec/tps6598x.c >>>> +++ b/drivers/usb/typec/tps6598x.c >>>> @@ -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 :( >> >> 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 ^ permalink raw reply [flat|nested] 49+ messages in thread
* Re: [PATCHv2] usb: typec: tps6598x: handle block writes separately with plain-I2C adapters @ 2019-02-21 9:52 ` Greg Kroah-Hartman 0 siblings, 0 replies; 49+ messages in thread From: Greg Kroah-Hartman @ 2019-02-21 9:52 UTC (permalink / raw) To: Nikolaus Voss; +Cc: Heikki Krogerus, Guenter Roeck, linux-usb, linux-kernel On Thu, Feb 21, 2019 at 09:37:33AM +0100, Nikolaus Voss wrote: > Hi Greg, > > On Wed, 20 Feb 2019, Greg Kroah-Hartman wrote: > > On Wed, Feb 20, 2019 at 04:22:00PM +0100, Nikolaus Voss wrote: > > > > > v2: fix tps6598x_exec_cmd also > > > > > --- > > > > > drivers/usb/typec/tps6598x.c | 26 ++++++++++++++++++++------ > > > > > 1 file changed, 20 insertions(+), 6 deletions(-) > > > > > > > > > > diff --git a/drivers/usb/typec/tps6598x.c b/drivers/usb/typec/tps6598x.c > > > > > index c84c8c189e90..c54b73fb2a2f 100644 > > > > > --- a/drivers/usb/typec/tps6598x.c > > > > > +++ b/drivers/usb/typec/tps6598x.c > > > > > @@ -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 :( > > > > > > 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. 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 ^ permalink raw reply [flat|nested] 49+ messages in thread
* [PATCHv2] usb: typec: tps6598x: handle block writes separately with plain-I2C adapters @ 2019-02-21 9:52 ` Greg Kroah-Hartman 0 siblings, 0 replies; 49+ messages in thread From: Greg Kroah-Hartman @ 2019-02-21 9:52 UTC (permalink / raw) To: Nikolaus Voss; +Cc: Heikki Krogerus, Guenter Roeck, linux-usb, linux-kernel On Thu, Feb 21, 2019 at 09:37:33AM +0100, Nikolaus Voss wrote: > Hi Greg, > > On Wed, 20 Feb 2019, Greg Kroah-Hartman wrote: > > On Wed, Feb 20, 2019 at 04:22:00PM +0100, Nikolaus Voss wrote: > > > > > v2: fix tps6598x_exec_cmd also > > > > > --- > > > > > drivers/usb/typec/tps6598x.c | 26 ++++++++++++++++++++------ > > > > > 1 file changed, 20 insertions(+), 6 deletions(-) > > > > > > > > > > diff --git a/drivers/usb/typec/tps6598x.c b/drivers/usb/typec/tps6598x.c > > > > > index c84c8c189e90..c54b73fb2a2f 100644 > > > > > --- a/drivers/usb/typec/tps6598x.c > > > > > +++ b/drivers/usb/typec/tps6598x.c > > > > > @@ -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 :( > > > > > > 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. 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 ^ permalink raw reply [flat|nested] 49+ messages in thread
* Re: [PATCHv2] usb: typec: tps6598x: handle block writes separately with plain-I2C adapters @ 2019-02-21 10:40 ` kbuild test robot 0 siblings, 0 replies; 49+ messages in thread From: kbuild test robot @ 2019-02-21 10:40 UTC (permalink / raw) To: Nikolaus Voss Cc: kbuild-all, Heikki Krogerus, Greg Kroah-Hartman, Guenter Roeck, linux-usb, linux-kernel, nikolaus.voss [-- Attachment #1: Type: text/plain, Size: 1544 bytes --] 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 [-- Attachment #2: .config.gz --] [-- Type: application/gzip, Size: 31125 bytes --] ^ permalink raw reply [flat|nested] 49+ messages in thread
* [PATCHv2] usb: typec: tps6598x: handle block writes separately with plain-I2C adapters @ 2019-02-21 10:40 ` kbuild test robot 0 siblings, 0 replies; 49+ messages in thread From: kbuild test robot @ 2019-02-21 10:40 UTC (permalink / raw) To: Nikolaus Voss Cc: kbuild-all, Heikki Krogerus, Greg Kroah-Hartman, Guenter Roeck, linux-usb, linux-kernel, nikolaus.voss 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 ^ permalink raw reply [flat|nested] 49+ messages in thread
* [PATCHv3] usb: typec: tps6598x: handle block writes separately with plain-I2C adapters @ 2019-02-20 15:11 ` Nikolaus Voss 0 siblings, 0 replies; 49+ messages in thread From: Nikolaus Voss @ 2019-02-20 15:11 UTC (permalink / raw) To: Heikki Krogerus, Greg Kroah-Hartman, Guenter Roeck Cc: linux-usb, linux-kernel, Nikolaus Voss From: Nikolaus Voss <nikolaus.voss@loewensteinmedical.de> 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 <nikolaus.voss@loewensteinmedical.de> --- 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(-) diff --git a/drivers/usb/typec/tps6598x.c b/drivers/usb/typec/tps6598x.c index c84c8c189e90..eb8046f87a54 100644 --- a/drivers/usb/typec/tps6598x.c +++ b/drivers/usb/typec/tps6598x.c @@ -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; } -- 2.17.1 ^ permalink raw reply related [flat|nested] 49+ messages in thread
* [PATCHv3] usb: typec: tps6598x: handle block writes separately with plain-I2C adapters @ 2019-02-20 15:11 ` Nikolaus Voss 0 siblings, 0 replies; 49+ messages in thread From: Nikolaus Voss @ 2019-02-20 15:11 UTC (permalink / raw) To: Heikki Krogerus, Greg Kroah-Hartman, Guenter Roeck Cc: linux-usb, linux-kernel, Nikolaus Voss From: Nikolaus Voss <nikolaus.voss@loewensteinmedical.de> 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 <nikolaus.voss@loewensteinmedical.de> --- 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(-) diff --git a/drivers/usb/typec/tps6598x.c b/drivers/usb/typec/tps6598x.c index c84c8c189e90..eb8046f87a54 100644 --- a/drivers/usb/typec/tps6598x.c +++ b/drivers/usb/typec/tps6598x.c @@ -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; } ^ permalink raw reply related [flat|nested] 49+ messages in thread
* Re: [PATCHv3] usb: typec: tps6598x: handle block writes separately with plain-I2C adapters @ 2019-02-20 15:35 ` Guenter Roeck 0 siblings, 0 replies; 49+ messages in thread From: Guenter Roeck @ 2019-02-20 15:35 UTC (permalink / raw) To: Nikolaus Voss, Heikki Krogerus, Greg Kroah-Hartman Cc: linux-usb, linux-kernel, Nikolaus Voss On 2/20/19 7:11 AM, Nikolaus Voss wrote: > From: Nikolaus Voss <nikolaus.voss@loewensteinmedical.de> > > 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 <nikolaus.voss@loewensteinmedical.de> Reviewed-by: Guenter Roeck <linux@roeck-us.net> 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 > --- > 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(-) > > diff --git a/drivers/usb/typec/tps6598x.c b/drivers/usb/typec/tps6598x.c > index c84c8c189e90..eb8046f87a54 100644 > --- a/drivers/usb/typec/tps6598x.c > +++ b/drivers/usb/typec/tps6598x.c > @@ -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; > } > ^ permalink raw reply [flat|nested] 49+ messages in thread
* [PATCHv3] usb: typec: tps6598x: handle block writes separately with plain-I2C adapters @ 2019-02-20 15:35 ` Guenter Roeck 0 siblings, 0 replies; 49+ messages in thread From: Guenter Roeck @ 2019-02-20 15:35 UTC (permalink / raw) To: Nikolaus Voss, Heikki Krogerus, Greg Kroah-Hartman Cc: linux-usb, linux-kernel, Nikolaus Voss On 2/20/19 7:11 AM, Nikolaus Voss wrote: > From: Nikolaus Voss <nikolaus.voss@loewensteinmedical.de> > > 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 <nikolaus.voss@loewensteinmedical.de> Reviewed-by: Guenter Roeck <linux@roeck-us.net> 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 > --- > 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(-) > > diff --git a/drivers/usb/typec/tps6598x.c b/drivers/usb/typec/tps6598x.c > index c84c8c189e90..eb8046f87a54 100644 > --- a/drivers/usb/typec/tps6598x.c > +++ b/drivers/usb/typec/tps6598x.c > @@ -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; > } > ^ permalink raw reply [flat|nested] 49+ messages in thread
* Re: [PATCHv3] usb: typec: tps6598x: handle block writes separately with plain-I2C adapters @ 2019-02-20 15:36 ` Guenter Roeck 0 siblings, 0 replies; 49+ messages in thread From: Guenter Roeck @ 2019-02-20 15:36 UTC (permalink / raw) To: Nikolaus Voss, Heikki Krogerus, Greg Kroah-Hartman Cc: linux-usb, linux-kernel, Nikolaus Voss On 2/20/19 7:35 AM, Guenter Roeck wrote: > On 2/20/19 7:11 AM, Nikolaus Voss wrote: >> From: Nikolaus Voss <nikolaus.voss@loewensteinmedical.de> >> >> 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 <nikolaus.voss@loewensteinmedical.de> > > Reviewed-by: Guenter Roeck <linux@roeck-us.net> > > 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 > Guenter > >> --- >> 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(-) >> >> diff --git a/drivers/usb/typec/tps6598x.c b/drivers/usb/typec/tps6598x.c >> index c84c8c189e90..eb8046f87a54 100644 >> --- a/drivers/usb/typec/tps6598x.c >> +++ b/drivers/usb/typec/tps6598x.c >> @@ -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; >> } >> > ^ permalink raw reply [flat|nested] 49+ messages in thread
* [PATCHv3] usb: typec: tps6598x: handle block writes separately with plain-I2C adapters @ 2019-02-20 15:36 ` Guenter Roeck 0 siblings, 0 replies; 49+ messages in thread From: Guenter Roeck @ 2019-02-20 15:36 UTC (permalink / raw) To: Nikolaus Voss, Heikki Krogerus, Greg Kroah-Hartman Cc: linux-usb, linux-kernel, Nikolaus Voss On 2/20/19 7:35 AM, Guenter Roeck wrote: > On 2/20/19 7:11 AM, Nikolaus Voss wrote: >> From: Nikolaus Voss <nikolaus.voss@loewensteinmedical.de> >> >> 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 <nikolaus.voss@loewensteinmedical.de> > > Reviewed-by: Guenter Roeck <linux@roeck-us.net> > > 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 > Guenter > >> --- >> 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(-) >> >> diff --git a/drivers/usb/typec/tps6598x.c b/drivers/usb/typec/tps6598x.c >> index c84c8c189e90..eb8046f87a54 100644 >> --- a/drivers/usb/typec/tps6598x.c >> +++ b/drivers/usb/typec/tps6598x.c >> @@ -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; >> } >> > ^ permalink raw reply [flat|nested] 49+ messages in thread
* Re: [PATCHv3] usb: typec: tps6598x: handle block writes separately with plain-I2C adapters @ 2019-02-21 8:41 ` Nikolaus Voss 0 siblings, 0 replies; 49+ messages in thread From: Nikolaus Voss @ 2019-02-21 8:41 UTC (permalink / raw) To: Guenter Roeck Cc: Heikki Krogerus, Greg Kroah-Hartman, linux-usb, linux-kernel Hi Guenther, On Wed, 20 Feb 2019, Guenter Roeck wrote: > On 2/20/19 7:11 AM, Nikolaus Voss wrote: >> From: Nikolaus Voss <nikolaus.voss@loewensteinmedical.de> >> >> 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 <nikolaus.voss@loewensteinmedical.de> > > Reviewed-by: Guenter Roeck <linux@roeck-us.net> > > 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 ^ permalink raw reply [flat|nested] 49+ messages in thread
* [PATCHv3] usb: typec: tps6598x: handle block writes separately with plain-I2C adapters @ 2019-02-21 8:41 ` Nikolaus Voss 0 siblings, 0 replies; 49+ messages in thread From: Nikolaus Voss @ 2019-02-21 8:41 UTC (permalink / raw) To: Guenter Roeck Cc: Heikki Krogerus, Greg Kroah-Hartman, linux-usb, linux-kernel Hi Guenther, On Wed, 20 Feb 2019, Guenter Roeck wrote: > On 2/20/19 7:11 AM, Nikolaus Voss wrote: >> From: Nikolaus Voss <nikolaus.voss@loewensteinmedical.de> >> >> 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 <nikolaus.voss@loewensteinmedical.de> > > Reviewed-by: Guenter Roeck <linux@roeck-us.net> > > 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 ^ permalink raw reply [flat|nested] 49+ messages in thread
* Re: [PATCHv3] usb: typec: tps6598x: handle block writes separately with plain-I2C adapters @ 2019-02-21 9:08 ` Heikki Krogerus 0 siblings, 0 replies; 49+ messages in thread From: Heikki Krogerus @ 2019-02-21 9:08 UTC (permalink / raw) To: Nikolaus Voss Cc: Greg Kroah-Hartman, Guenter Roeck, linux-usb, linux-kernel, Nikolaus Voss On Wed, Feb 20, 2019 at 04:11:38PM +0100, Nikolaus Voss wrote: > From: Nikolaus Voss <nikolaus.voss@loewensteinmedical.de> > > 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 <nikolaus.voss@loewensteinmedical.de> Acked-by: Heikki Krogerus <heikki.krogerus@linux.intel.com> > --- > 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(-) > > diff --git a/drivers/usb/typec/tps6598x.c b/drivers/usb/typec/tps6598x.c > index c84c8c189e90..eb8046f87a54 100644 > --- a/drivers/usb/typec/tps6598x.c > +++ b/drivers/usb/typec/tps6598x.c > @@ -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; > } > -- > 2.17.1 thanks, -- heikki ^ permalink raw reply [flat|nested] 49+ messages in thread
* [PATCHv3] usb: typec: tps6598x: handle block writes separately with plain-I2C adapters @ 2019-02-21 9:08 ` Heikki Krogerus 0 siblings, 0 replies; 49+ messages in thread From: Heikki Krogerus @ 2019-02-21 9:08 UTC (permalink / raw) To: Nikolaus Voss Cc: Greg Kroah-Hartman, Guenter Roeck, linux-usb, linux-kernel, Nikolaus Voss On Wed, Feb 20, 2019 at 04:11:38PM +0100, Nikolaus Voss wrote: > From: Nikolaus Voss <nikolaus.voss@loewensteinmedical.de> > > 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 <nikolaus.voss@loewensteinmedical.de> Acked-by: Heikki Krogerus <heikki.krogerus@linux.intel.com> > --- > 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(-) > > diff --git a/drivers/usb/typec/tps6598x.c b/drivers/usb/typec/tps6598x.c > index c84c8c189e90..eb8046f87a54 100644 > --- a/drivers/usb/typec/tps6598x.c > +++ b/drivers/usb/typec/tps6598x.c > @@ -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; > } > -- > 2.17.1 thanks, ^ permalink raw reply [flat|nested] 49+ messages in thread
* Re: [PATCHv3] usb: typec: tps6598x: handle block writes separately with plain-I2C adapters @ 2019-02-21 9:42 ` Nikolaus Voss 0 siblings, 0 replies; 49+ messages in thread From: Nikolaus Voss @ 2019-02-21 9:42 UTC (permalink / raw) To: Heikki Krogerus Cc: Greg Kroah-Hartman, Guenter Roeck, linux-usb, linux-kernel On Thu, 21 Feb 2019, Heikki Krogerus wrote: > On Wed, Feb 20, 2019 at 04:11:38PM +0100, Nikolaus Voss wrote: >> From: Nikolaus Voss <nikolaus.voss@loewensteinmedical.de> >> >> 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 <nikolaus.voss@loewensteinmedical.de> > > Acked-by: Heikki Krogerus <heikki.krogerus@linux.intel.com> Heikki, shall I respin the patch with tags? Nikolaus ^ permalink raw reply [flat|nested] 49+ messages in thread
* [PATCHv3] usb: typec: tps6598x: handle block writes separately with plain-I2C adapters @ 2019-02-21 9:42 ` Nikolaus Voss 0 siblings, 0 replies; 49+ messages in thread From: Nikolaus Voss @ 2019-02-21 9:42 UTC (permalink / raw) To: Heikki Krogerus Cc: Greg Kroah-Hartman, Guenter Roeck, linux-usb, linux-kernel On Thu, 21 Feb 2019, Heikki Krogerus wrote: > On Wed, Feb 20, 2019 at 04:11:38PM +0100, Nikolaus Voss wrote: >> From: Nikolaus Voss <nikolaus.voss@loewensteinmedical.de> >> >> 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 <nikolaus.voss@loewensteinmedical.de> > > Acked-by: Heikki Krogerus <heikki.krogerus@linux.intel.com> Heikki, shall I respin the patch with tags? Nikolaus ^ permalink raw reply [flat|nested] 49+ messages in thread
* Re: [PATCHv3] usb: typec: tps6598x: handle block writes separately with plain-I2C adapters @ 2019-02-21 9:52 ` Greg Kroah-Hartman 0 siblings, 0 replies; 49+ messages in thread From: Greg Kroah-Hartman @ 2019-02-21 9:52 UTC (permalink / raw) To: Nikolaus Voss; +Cc: Heikki Krogerus, Guenter Roeck, linux-usb, linux-kernel On Thu, Feb 21, 2019 at 10:42:07AM +0100, Nikolaus Voss wrote: > On Thu, 21 Feb 2019, Heikki Krogerus wrote: > > On Wed, Feb 20, 2019 at 04:11:38PM +0100, Nikolaus Voss wrote: > > > From: Nikolaus Voss <nikolaus.voss@loewensteinmedical.de> > > > > > > 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 <nikolaus.voss@loewensteinmedical.de> > > > > Acked-by: Heikki Krogerus <heikki.krogerus@linux.intel.com> > > Heikki, > > shall I respin the patch with tags? I can add it, thanks. greg k-h ^ permalink raw reply [flat|nested] 49+ messages in thread
* [PATCHv3] usb: typec: tps6598x: handle block writes separately with plain-I2C adapters @ 2019-02-21 9:52 ` Greg Kroah-Hartman 0 siblings, 0 replies; 49+ messages in thread From: Greg Kroah-Hartman @ 2019-02-21 9:52 UTC (permalink / raw) To: Nikolaus Voss; +Cc: Heikki Krogerus, Guenter Roeck, linux-usb, linux-kernel On Thu, Feb 21, 2019 at 10:42:07AM +0100, Nikolaus Voss wrote: > On Thu, 21 Feb 2019, Heikki Krogerus wrote: > > On Wed, Feb 20, 2019 at 04:11:38PM +0100, Nikolaus Voss wrote: > > > From: Nikolaus Voss <nikolaus.voss@loewensteinmedical.de> > > > > > > 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 <nikolaus.voss@loewensteinmedical.de> > > > > Acked-by: Heikki Krogerus <heikki.krogerus@linux.intel.com> > > Heikki, > > shall I respin the patch with tags? I can add it, thanks. greg k-h ^ permalink raw reply [flat|nested] 49+ messages in thread
end of thread, other threads:[~2019-02-21 10:41 UTC | newest] Thread overview: 49+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2018-04-25 14:22 [PATCHv2] usb: typec: tps6598x: handle block reads separately with plain-I2C adapters Heikki Krogerus 2018-09-10 5:05 ` [PATCH] usb: typec: tps6598x: handle block writes " Nikolaus Voss 2018-09-10 5:05 ` Nikolaus Voss 2019-02-20 12:14 ` [PATCH] " Heikki Krogerus 2019-02-20 12:14 ` Heikki Krogerus 2019-02-20 12:56 ` [PATCH] " Nikolaus Voss 2019-02-20 12:56 ` Nikolaus Voss 2019-02-20 12:57 ` [PATCHv2] " Nikolaus Voss 2019-02-20 12:57 ` Nikolaus Voss 2019-02-20 13:30 ` Heikki Krogerus 2019-02-20 13:30 ` Heikki Krogerus 2019-02-20 13:38 ` Nikolaus Voss 2019-02-20 13:38 ` Nikolaus Voss 2019-02-20 14:14 ` Heikki Krogerus 2019-02-20 14:14 ` Heikki Krogerus 2019-02-20 14:30 ` Heikki Krogerus 2019-02-20 14:30 ` Heikki Krogerus 2019-02-20 15:08 ` Nikolaus Voss 2019-02-20 15:08 ` Nikolaus Voss 2019-02-20 14:45 ` Guenter Roeck 2019-02-20 14:45 ` Guenter Roeck 2019-02-20 15:18 ` Nikolaus Voss 2019-02-20 15:18 ` Nikolaus Voss 2019-02-20 15:02 ` Greg Kroah-Hartman 2019-02-20 15:02 ` Greg Kroah-Hartman 2019-02-20 15:22 ` Nikolaus Voss 2019-02-20 15:22 ` Nikolaus Voss 2019-02-20 16:22 ` Greg Kroah-Hartman 2019-02-20 16:22 ` Greg Kroah-Hartman 2019-02-21 8:37 ` Nikolaus Voss 2019-02-21 8:37 ` Nikolaus Voss 2019-02-21 9:52 ` Greg Kroah-Hartman 2019-02-21 9:52 ` Greg Kroah-Hartman 2019-02-21 10:40 ` kbuild test robot 2019-02-21 10:40 ` kbuild test robot 2019-02-20 15:11 ` [PATCHv3] " Nikolaus Voss 2019-02-20 15:11 ` Nikolaus Voss 2019-02-20 15:35 ` Guenter Roeck 2019-02-20 15:35 ` Guenter Roeck 2019-02-20 15:36 ` Guenter Roeck 2019-02-20 15:36 ` Guenter Roeck 2019-02-21 8:41 ` Nikolaus Voss 2019-02-21 8:41 ` Nikolaus Voss 2019-02-21 9:08 ` Heikki Krogerus 2019-02-21 9:08 ` Heikki Krogerus 2019-02-21 9:42 ` Nikolaus Voss 2019-02-21 9:42 ` Nikolaus Voss 2019-02-21 9:52 ` Greg Kroah-Hartman 2019-02-21 9:52 ` Greg Kroah-Hartman
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.