* [PATCH] HID: cp2112: add I2C mode @ 2014-06-29 6:14 Antonio Borneo 2014-07-07 13:57 ` Benjamin Tissoires 0 siblings, 1 reply; 6+ messages in thread From: Antonio Borneo @ 2014-06-29 6:14 UTC (permalink / raw) To: Jiri Kosina, linux-input, David Barksdale; +Cc: linux-kernel, Antonio Borneo cp2112 supports single I2C read/write transactions. It can't combine I2C transactions. Add master_xfer, using similar code flow as for smbus_xfer. Signed-off-by: Antonio Borneo <borneo.antonio@gmail.com> --- drivers/hid/hid-cp2112.c | 102 ++++++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 101 insertions(+), 1 deletion(-) diff --git a/drivers/hid/hid-cp2112.c b/drivers/hid/hid-cp2112.c index 3952d90..1260a8a 100644 --- a/drivers/hid/hid-cp2112.c +++ b/drivers/hid/hid-cp2112.c @@ -429,6 +429,104 @@ static int cp2112_write_req(void *buf, u8 slave_address, u8 command, u8 *data, return data_length + 4; } +static int cp2112_i2c_write_req(void *buf, u8 slave_address, u8 *data, + u8 data_length) +{ + struct cp2112_write_req_report *report = buf; + + if (data_length > sizeof(report->data)) + return -EINVAL; + + report->report = CP2112_DATA_WRITE_REQUEST; + report->slave_address = slave_address << 1; + report->length = data_length; + memcpy(report->data, data, data_length); + return data_length + 3; +} + +static int cp2112_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs, + int num) +{ + struct cp2112_device *dev = (struct cp2112_device *)adap->algo_data; + struct hid_device *hdev = dev->hdev; + u8 buf[64]; + ssize_t count; + unsigned int retries; + int ret; + + hid_dbg(hdev, "I2C %d messages\n", num); + + if (num != 1) { + hid_err(hdev, + "Multi-message I2C transactions not supported\n"); + return -EOPNOTSUPP; + } + + if (msgs->flags & I2C_M_RD) + count = cp2112_read_req(buf, msgs->addr, msgs->len); + else + count = cp2112_i2c_write_req(buf, msgs->addr, msgs->buf, + msgs->len); + + if (count < 0) + return count; + + ret = hid_hw_power(hdev, PM_HINT_FULLON); + if (ret < 0) { + hid_err(hdev, "power management error: %d\n", ret); + return ret; + } + + ret = cp2112_hid_output(hdev, buf, count, HID_OUTPUT_REPORT); + if (ret < 0) { + hid_warn(hdev, "Error starting transaction: %d\n", ret); + goto power_normal; + } + + for (retries = 0; retries < XFER_STATUS_RETRIES; ++retries) { + ret = cp2112_xfer_status(dev); + if (-EBUSY == ret) + continue; + if (ret < 0) + goto power_normal; + break; + } + + if (XFER_STATUS_RETRIES <= retries) { + hid_warn(hdev, "Transfer timed out, cancelling.\n"); + buf[0] = CP2112_CANCEL_TRANSFER; + buf[1] = 0x01; + + ret = cp2112_hid_output(hdev, buf, 2, HID_OUTPUT_REPORT); + if (ret < 0) + hid_warn(hdev, "Error cancelling transaction: %d\n", + ret); + + ret = -ETIMEDOUT; + goto power_normal; + } + + if (!(msgs->flags & I2C_M_RD)) { + ret = 0; + goto power_normal; + } + + ret = cp2112_read(dev, msgs->buf, msgs->len); + if (ret < 0) + goto power_normal; + if (ret != msgs->len) { + hid_warn(hdev, "short read: %d < %d\n", ret, msgs->len); + ret = -EIO; + goto power_normal; + } + + ret = 0; +power_normal: + hid_hw_power(hdev, PM_HINT_NORMAL); + hid_dbg(hdev, "I2C transfer finished: %d\n", ret); + return (ret) ? ret : 1; +} + static int cp2112_xfer(struct i2c_adapter *adap, u16 addr, unsigned short flags, char read_write, u8 command, int size, union i2c_smbus_data *data) @@ -595,7 +693,8 @@ power_normal: static u32 cp2112_functionality(struct i2c_adapter *adap) { - return I2C_FUNC_SMBUS_BYTE | + return I2C_FUNC_I2C | + I2C_FUNC_SMBUS_BYTE | I2C_FUNC_SMBUS_BYTE_DATA | I2C_FUNC_SMBUS_WORD_DATA | I2C_FUNC_SMBUS_BLOCK_DATA | @@ -605,6 +704,7 @@ static u32 cp2112_functionality(struct i2c_adapter *adap) } static const struct i2c_algorithm smbus_algorithm = { + .master_xfer = cp2112_i2c_xfer, .smbus_xfer = cp2112_xfer, .functionality = cp2112_functionality, }; -- 2.0.1 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] HID: cp2112: add I2C mode 2014-06-29 6:14 [PATCH] HID: cp2112: add I2C mode Antonio Borneo @ 2014-07-07 13:57 ` Benjamin Tissoires 2014-07-07 15:32 ` Antonio Borneo 0 siblings, 1 reply; 6+ messages in thread From: Benjamin Tissoires @ 2014-07-07 13:57 UTC (permalink / raw) To: Antonio Borneo Cc: Jiri Kosina, linux-input, David Barksdale, linux-kernel@vger.kernel.org Hi Antonio, On Sun, Jun 29, 2014 at 2:14 AM, Antonio Borneo <borneo.antonio@gmail.com> wrote: > cp2112 supports single I2C read/write transactions. > It can't combine I2C transactions. > > Add master_xfer, using similar code flow as for smbus_xfer. Thanks for taking the time to implement it. I wanted to add this functionality, but did not found the time to finalize/submit the patches. So here is a review. > > Signed-off-by: Antonio Borneo <borneo.antonio@gmail.com> > --- > drivers/hid/hid-cp2112.c | 102 ++++++++++++++++++++++++++++++++++++++++++++++- > 1 file changed, 101 insertions(+), 1 deletion(-) > > diff --git a/drivers/hid/hid-cp2112.c b/drivers/hid/hid-cp2112.c > index 3952d90..1260a8a 100644 > --- a/drivers/hid/hid-cp2112.c > +++ b/drivers/hid/hid-cp2112.c > @@ -429,6 +429,104 @@ static int cp2112_write_req(void *buf, u8 slave_address, u8 command, u8 *data, > return data_length + 4; > } > > +static int cp2112_i2c_write_req(void *buf, u8 slave_address, u8 *data, > + u8 data_length) > +{ > + struct cp2112_write_req_report *report = buf; > + > + if (data_length > sizeof(report->data)) > + return -EINVAL; > + > + report->report = CP2112_DATA_WRITE_REQUEST; > + report->slave_address = slave_address << 1; > + report->length = data_length; > + memcpy(report->data, data, data_length); > + return data_length + 3; > +} > + > +static int cp2112_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs, > + int num) > +{ > + struct cp2112_device *dev = (struct cp2112_device *)adap->algo_data; > + struct hid_device *hdev = dev->hdev; > + u8 buf[64]; > + ssize_t count; > + unsigned int retries; > + int ret; > + > + hid_dbg(hdev, "I2C %d messages\n", num); > + > + if (num != 1) { > + hid_err(hdev, > + "Multi-message I2C transactions not supported\n"); This is a shame. You can just externalize the following block (in pseudo code): { - read/write_req - hid_output - xfer_status - read} and then just use a for loop to call this sequence for each incoming message. > + return -EOPNOTSUPP; > + } > + > + if (msgs->flags & I2C_M_RD) > + count = cp2112_read_req(buf, msgs->addr, msgs->len); > + else > + count = cp2112_i2c_write_req(buf, msgs->addr, msgs->buf, > + msgs->len); > + > + if (count < 0) > + return count; > + > + ret = hid_hw_power(hdev, PM_HINT_FULLON); > + if (ret < 0) { > + hid_err(hdev, "power management error: %d\n", ret); > + return ret; > + } > + > + ret = cp2112_hid_output(hdev, buf, count, HID_OUTPUT_REPORT); > + if (ret < 0) { > + hid_warn(hdev, "Error starting transaction: %d\n", ret); > + goto power_normal; > + } > + > + for (retries = 0; retries < XFER_STATUS_RETRIES; ++retries) { > + ret = cp2112_xfer_status(dev); > + if (-EBUSY == ret) > + continue; > + if (ret < 0) > + goto power_normal; > + break; > + } > + > + if (XFER_STATUS_RETRIES <= retries) { > + hid_warn(hdev, "Transfer timed out, cancelling.\n"); > + buf[0] = CP2112_CANCEL_TRANSFER; > + buf[1] = 0x01; > + > + ret = cp2112_hid_output(hdev, buf, 2, HID_OUTPUT_REPORT); > + if (ret < 0) > + hid_warn(hdev, "Error cancelling transaction: %d\n", > + ret); > + > + ret = -ETIMEDOUT; > + goto power_normal; > + } > + > + if (!(msgs->flags & I2C_M_RD)) { > + ret = 0; > + goto power_normal; > + } > + > + ret = cp2112_read(dev, msgs->buf, msgs->len); > + if (ret < 0) > + goto power_normal; > + if (ret != msgs->len) { > + hid_warn(hdev, "short read: %d < %d\n", ret, msgs->len); > + ret = -EIO; > + goto power_normal; > + } > + > + ret = 0; ret = num; > +power_normal: > + hid_hw_power(hdev, PM_HINT_NORMAL); > + hid_dbg(hdev, "I2C transfer finished: %d\n", ret); > + return (ret) ? ret : 1; and "return ret;" > +} > + > static int cp2112_xfer(struct i2c_adapter *adap, u16 addr, > unsigned short flags, char read_write, u8 command, > int size, union i2c_smbus_data *data) > @@ -595,7 +693,8 @@ power_normal: > > static u32 cp2112_functionality(struct i2c_adapter *adap) > { > - return I2C_FUNC_SMBUS_BYTE | > + return I2C_FUNC_I2C | > + I2C_FUNC_SMBUS_BYTE | > I2C_FUNC_SMBUS_BYTE_DATA | > I2C_FUNC_SMBUS_WORD_DATA | > I2C_FUNC_SMBUS_BLOCK_DATA | > @@ -605,6 +704,7 @@ static u32 cp2112_functionality(struct i2c_adapter *adap) > } > > static const struct i2c_algorithm smbus_algorithm = { > + .master_xfer = cp2112_i2c_xfer, > .smbus_xfer = cp2112_xfer, > .functionality = cp2112_functionality, > }; > -- > 2.0.1 > Cheers, Benjamin ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] HID: cp2112: add I2C mode 2014-07-07 13:57 ` Benjamin Tissoires @ 2014-07-07 15:32 ` Antonio Borneo 2014-07-07 15:52 ` Benjamin Tissoires 0 siblings, 1 reply; 6+ messages in thread From: Antonio Borneo @ 2014-07-07 15:32 UTC (permalink / raw) To: Benjamin Tissoires Cc: Jiri Kosina, linux-input, David Barksdale, linux-kernel@vger.kernel.org On Mon, Jul 7, 2014 at 9:57 PM, Benjamin Tissoires <benjamin.tissoires@gmail.com> wrote: > Hi Antonio, > > On Sun, Jun 29, 2014 at 2:14 AM, Antonio Borneo > <borneo.antonio@gmail.com> wrote: >> cp2112 supports single I2C read/write transactions. >> It can't combine I2C transactions. >> >> Add master_xfer, using similar code flow as for smbus_xfer. > > Thanks for taking the time to implement it. I wanted to add this > functionality, but did not found the time to finalize/submit the > patches. > So here is a review. Ciao Benjamin, thanks for the review. My comments inline below >> >> Signed-off-by: Antonio Borneo <borneo.antonio@gmail.com> >> --- >> drivers/hid/hid-cp2112.c | 102 ++++++++++++++++++++++++++++++++++++++++++++++- >> 1 file changed, 101 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/hid/hid-cp2112.c b/drivers/hid/hid-cp2112.c >> index 3952d90..1260a8a 100644 >> --- a/drivers/hid/hid-cp2112.c >> +++ b/drivers/hid/hid-cp2112.c >> @@ -429,6 +429,104 @@ static int cp2112_write_req(void *buf, u8 slave_address, u8 command, u8 *data, >> return data_length + 4; >> } >> >> +static int cp2112_i2c_write_req(void *buf, u8 slave_address, u8 *data, >> + u8 data_length) >> +{ >> + struct cp2112_write_req_report *report = buf; >> + >> + if (data_length > sizeof(report->data)) >> + return -EINVAL; >> + >> + report->report = CP2112_DATA_WRITE_REQUEST; >> + report->slave_address = slave_address << 1; >> + report->length = data_length; >> + memcpy(report->data, data, data_length); >> + return data_length + 3; >> +} >> + >> +static int cp2112_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs, >> + int num) >> +{ >> + struct cp2112_device *dev = (struct cp2112_device *)adap->algo_data; >> + struct hid_device *hdev = dev->hdev; >> + u8 buf[64]; >> + ssize_t count; >> + unsigned int retries; >> + int ret; >> + >> + hid_dbg(hdev, "I2C %d messages\n", num); >> + >> + if (num != 1) { >> + hid_err(hdev, >> + "Multi-message I2C transactions not supported\n"); > > This is a shame. You can just externalize the following block (in pseudo code): > { - read/write_req > - hid_output > - xfer_status > - read} > > and then just use a for loop to call this sequence for each incoming message. The parameter "num" is for sending a "combined I2C transaction" in which a series of messages is sent "without" stop-bits between the messages. You can get full description searching for "i2c_transfer" in - Documentation/i2c/writing-clients - Documentation/i2c/i2c-protocol Since CP2112 does not support this feature, I return EOPNOTSUPP as is done in drivers/i2c/busses/i2c-powermac.c If I implement the loop, as you suggest, CP2112 will just send a sequence of independent messages, each with its own stop bit. Probably the error message could be more explicit by mentioning the word "combined". I lazily copied the same message from i2c-powermac think it's clear enough. > >> + return -EOPNOTSUPP; >> + } >> + >> + if (msgs->flags & I2C_M_RD) >> + count = cp2112_read_req(buf, msgs->addr, msgs->len); >> + else >> + count = cp2112_i2c_write_req(buf, msgs->addr, msgs->buf, >> + msgs->len); >> + >> + if (count < 0) >> + return count; >> + >> + ret = hid_hw_power(hdev, PM_HINT_FULLON); >> + if (ret < 0) { >> + hid_err(hdev, "power management error: %d\n", ret); >> + return ret; >> + } >> + >> + ret = cp2112_hid_output(hdev, buf, count, HID_OUTPUT_REPORT); >> + if (ret < 0) { >> + hid_warn(hdev, "Error starting transaction: %d\n", ret); >> + goto power_normal; >> + } >> + >> + for (retries = 0; retries < XFER_STATUS_RETRIES; ++retries) { >> + ret = cp2112_xfer_status(dev); >> + if (-EBUSY == ret) >> + continue; >> + if (ret < 0) >> + goto power_normal; >> + break; >> + } >> + >> + if (XFER_STATUS_RETRIES <= retries) { >> + hid_warn(hdev, "Transfer timed out, cancelling.\n"); >> + buf[0] = CP2112_CANCEL_TRANSFER; >> + buf[1] = 0x01; >> + >> + ret = cp2112_hid_output(hdev, buf, 2, HID_OUTPUT_REPORT); >> + if (ret < 0) >> + hid_warn(hdev, "Error cancelling transaction: %d\n", >> + ret); >> + >> + ret = -ETIMEDOUT; >> + goto power_normal; >> + } >> + >> + if (!(msgs->flags & I2C_M_RD)) { >> + ret = 0; >> + goto power_normal; >> + } >> + >> + ret = cp2112_read(dev, msgs->buf, msgs->len); >> + if (ret < 0) >> + goto power_normal; >> + if (ret != msgs->len) { >> + hid_warn(hdev, "short read: %d < %d\n", ret, msgs->len); >> + ret = -EIO; >> + goto power_normal; >> + } >> + >> + ret = 0; > > ret = num; > >> +power_normal: >> + hid_hw_power(hdev, PM_HINT_NORMAL); >> + hid_dbg(hdev, "I2C transfer finished: %d\n", ret); >> + return (ret) ? ret : 1; > > and "return ret;" Not sure I catch what you mean, here. Please confirm my understanding. >From a comment inside include/linux/i2c.h /* master_xfer should return the number of messages successfully processed, or a negative value on error */ In the code above I set "ret" to an error number or to zero. Then return the error number or the number of messages (equal to 1, as explained above). Do you prefer I set "ret" directly to 1 instead of 0? I can send a V2 for this. In this case I would not change the debug message above that prints "ret" and I will just print the current value for "ret". Best Regards, Antonio > >> +} >> + >> static int cp2112_xfer(struct i2c_adapter *adap, u16 addr, >> unsigned short flags, char read_write, u8 command, >> int size, union i2c_smbus_data *data) >> @@ -595,7 +693,8 @@ power_normal: >> >> static u32 cp2112_functionality(struct i2c_adapter *adap) >> { >> - return I2C_FUNC_SMBUS_BYTE | >> + return I2C_FUNC_I2C | >> + I2C_FUNC_SMBUS_BYTE | >> I2C_FUNC_SMBUS_BYTE_DATA | >> I2C_FUNC_SMBUS_WORD_DATA | >> I2C_FUNC_SMBUS_BLOCK_DATA | >> @@ -605,6 +704,7 @@ static u32 cp2112_functionality(struct i2c_adapter *adap) >> } >> >> static const struct i2c_algorithm smbus_algorithm = { >> + .master_xfer = cp2112_i2c_xfer, >> .smbus_xfer = cp2112_xfer, >> .functionality = cp2112_functionality, >> }; >> -- >> 2.0.1 >> > > Cheers, > Benjamin ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] HID: cp2112: add I2C mode 2014-07-07 15:32 ` Antonio Borneo @ 2014-07-07 15:52 ` Benjamin Tissoires 2014-07-07 23:25 ` [PATCH v2] " Antonio Borneo 0 siblings, 1 reply; 6+ messages in thread From: Benjamin Tissoires @ 2014-07-07 15:52 UTC (permalink / raw) To: Antonio Borneo Cc: Jiri Kosina, linux-input, David Barksdale, linux-kernel@vger.kernel.org On Mon, Jul 7, 2014 at 11:32 AM, Antonio Borneo <borneo.antonio@gmail.com> wrote: > On Mon, Jul 7, 2014 at 9:57 PM, Benjamin Tissoires > <benjamin.tissoires@gmail.com> wrote: >> Hi Antonio, >> >> On Sun, Jun 29, 2014 at 2:14 AM, Antonio Borneo >> <borneo.antonio@gmail.com> wrote: >>> cp2112 supports single I2C read/write transactions. >>> It can't combine I2C transactions. >>> >>> Add master_xfer, using similar code flow as for smbus_xfer. >> >> Thanks for taking the time to implement it. I wanted to add this >> functionality, but did not found the time to finalize/submit the >> patches. >> So here is a review. > > Ciao Benjamin, > > thanks for the review. > My comments inline below > >>> >>> Signed-off-by: Antonio Borneo <borneo.antonio@gmail.com> >>> --- >>> drivers/hid/hid-cp2112.c | 102 ++++++++++++++++++++++++++++++++++++++++++++++- >>> 1 file changed, 101 insertions(+), 1 deletion(-) >>> >>> diff --git a/drivers/hid/hid-cp2112.c b/drivers/hid/hid-cp2112.c >>> index 3952d90..1260a8a 100644 >>> --- a/drivers/hid/hid-cp2112.c >>> +++ b/drivers/hid/hid-cp2112.c >>> @@ -429,6 +429,104 @@ static int cp2112_write_req(void *buf, u8 slave_address, u8 command, u8 *data, >>> return data_length + 4; >>> } >>> >>> +static int cp2112_i2c_write_req(void *buf, u8 slave_address, u8 *data, >>> + u8 data_length) >>> +{ >>> + struct cp2112_write_req_report *report = buf; >>> + >>> + if (data_length > sizeof(report->data)) >>> + return -EINVAL; >>> + >>> + report->report = CP2112_DATA_WRITE_REQUEST; >>> + report->slave_address = slave_address << 1; >>> + report->length = data_length; >>> + memcpy(report->data, data, data_length); >>> + return data_length + 3; >>> +} >>> + >>> +static int cp2112_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs, >>> + int num) >>> +{ >>> + struct cp2112_device *dev = (struct cp2112_device *)adap->algo_data; >>> + struct hid_device *hdev = dev->hdev; >>> + u8 buf[64]; >>> + ssize_t count; >>> + unsigned int retries; >>> + int ret; >>> + >>> + hid_dbg(hdev, "I2C %d messages\n", num); >>> + >>> + if (num != 1) { >>> + hid_err(hdev, >>> + "Multi-message I2C transactions not supported\n"); >> >> This is a shame. You can just externalize the following block (in pseudo code): >> { - read/write_req >> - hid_output >> - xfer_status >> - read} >> >> and then just use a for loop to call this sequence for each incoming message. > > The parameter "num" is for sending a "combined I2C transaction" in which > a series of messages is sent "without" stop-bits between the messages. > You can get full description searching for "i2c_transfer" in > - Documentation/i2c/writing-clients > - Documentation/i2c/i2c-protocol Alright, this convince me. > Since CP2112 does not support this feature, I return EOPNOTSUPP as is done in > drivers/i2c/busses/i2c-powermac.c > > If I implement the loop, as you suggest, CP2112 will just send a > sequence of independent messages, each with its own stop bit. > > Probably the error message could be more explicit by mentioning the > word "combined". > I lazily copied the same message from i2c-powermac think it's clear enough. No, I think the error message is fine. I just thought that we could "emulate" the combined transaction protocol by sending a bunch of single transactions. Given that the protocol is explicit regarding the stop, it is better not to emulate it and use the implementation you are proposing. > >> >>> + return -EOPNOTSUPP; >>> + } >>> + >>> + if (msgs->flags & I2C_M_RD) >>> + count = cp2112_read_req(buf, msgs->addr, msgs->len); >>> + else >>> + count = cp2112_i2c_write_req(buf, msgs->addr, msgs->buf, >>> + msgs->len); >>> + >>> + if (count < 0) >>> + return count; >>> + >>> + ret = hid_hw_power(hdev, PM_HINT_FULLON); >>> + if (ret < 0) { >>> + hid_err(hdev, "power management error: %d\n", ret); >>> + return ret; >>> + } >>> + >>> + ret = cp2112_hid_output(hdev, buf, count, HID_OUTPUT_REPORT); >>> + if (ret < 0) { >>> + hid_warn(hdev, "Error starting transaction: %d\n", ret); >>> + goto power_normal; >>> + } >>> + >>> + for (retries = 0; retries < XFER_STATUS_RETRIES; ++retries) { >>> + ret = cp2112_xfer_status(dev); >>> + if (-EBUSY == ret) >>> + continue; >>> + if (ret < 0) >>> + goto power_normal; >>> + break; >>> + } >>> + >>> + if (XFER_STATUS_RETRIES <= retries) { >>> + hid_warn(hdev, "Transfer timed out, cancelling.\n"); >>> + buf[0] = CP2112_CANCEL_TRANSFER; >>> + buf[1] = 0x01; >>> + >>> + ret = cp2112_hid_output(hdev, buf, 2, HID_OUTPUT_REPORT); >>> + if (ret < 0) >>> + hid_warn(hdev, "Error cancelling transaction: %d\n", >>> + ret); >>> + >>> + ret = -ETIMEDOUT; >>> + goto power_normal; >>> + } >>> + >>> + if (!(msgs->flags & I2C_M_RD)) { >>> + ret = 0; >>> + goto power_normal; >>> + } >>> + >>> + ret = cp2112_read(dev, msgs->buf, msgs->len); >>> + if (ret < 0) >>> + goto power_normal; >>> + if (ret != msgs->len) { >>> + hid_warn(hdev, "short read: %d < %d\n", ret, msgs->len); >>> + ret = -EIO; >>> + goto power_normal; >>> + } >>> + >>> + ret = 0; >> >> ret = num; >> >>> +power_normal: >>> + hid_hw_power(hdev, PM_HINT_NORMAL); >>> + hid_dbg(hdev, "I2C transfer finished: %d\n", ret); >>> + return (ret) ? ret : 1; >> >> and "return ret;" > > Not sure I catch what you mean, here. Please confirm my understanding. > > From a comment inside include/linux/i2c.h > /* master_xfer should return the number of messages successfully > processed, or a negative value on error */ > > In the code above I set "ret" to an error number or to zero. > Then return the error number or the number of messages (equal to 1, as > explained above). > > Do you prefer I set "ret" directly to 1 instead of 0? Yep, just set ret to 1 instead of 0 (both at the end and right after the I2C_M_RD test). Then, ret will contain exactly either the number of messages processed, or the error. > > I can send a V2 for this. > In this case I would not change the debug message above that prints > "ret" and I will just print the current value for "ret". Yes, IMO, that makes more sense to print the exit value instead of changing it after. If you just do this small nitpicks in the V2, you can add my "Reviewed-by: Benjamin TIssoires <benjamin.tissoires@redhat.com>" directly in your submission. Cheers, Benjamin > > Best Regards, > Antonio > >> >>> +} >>> + >>> static int cp2112_xfer(struct i2c_adapter *adap, u16 addr, >>> unsigned short flags, char read_write, u8 command, >>> int size, union i2c_smbus_data *data) >>> @@ -595,7 +693,8 @@ power_normal: >>> >>> static u32 cp2112_functionality(struct i2c_adapter *adap) >>> { >>> - return I2C_FUNC_SMBUS_BYTE | >>> + return I2C_FUNC_I2C | >>> + I2C_FUNC_SMBUS_BYTE | >>> I2C_FUNC_SMBUS_BYTE_DATA | >>> I2C_FUNC_SMBUS_WORD_DATA | >>> I2C_FUNC_SMBUS_BLOCK_DATA | >>> @@ -605,6 +704,7 @@ static u32 cp2112_functionality(struct i2c_adapter *adap) >>> } >>> >>> static const struct i2c_algorithm smbus_algorithm = { >>> + .master_xfer = cp2112_i2c_xfer, >>> .smbus_xfer = cp2112_xfer, >>> .functionality = cp2112_functionality, >>> }; >>> -- >>> 2.0.1 >>> >> >> Cheers, >> Benjamin ^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH v2] HID: cp2112: add I2C mode 2014-07-07 15:52 ` Benjamin Tissoires @ 2014-07-07 23:25 ` Antonio Borneo 2014-07-29 9:38 ` Jiri Kosina 0 siblings, 1 reply; 6+ messages in thread From: Antonio Borneo @ 2014-07-07 23:25 UTC (permalink / raw) To: Jiri Kosina, linux-input Cc: David Barksdale, linux-kernel, Benjamin Tissoires, Antonio Borneo cp2112 supports single I2C read/write transactions. It can't combine I2C transactions. Add master_xfer, using similar code flow as for smbus_xfer. Signed-off-by: Antonio Borneo <borneo.antonio@gmail.com> Reviewed-by: Benjamin Tissoires <benjamin.tissoires@redhat.com> --- ChangeLog v1->v2: - In cp2112_i2c_xfer() set variable "ret" directly with the expected return value: either an error number or the number of transferred messages. --- drivers/hid/hid-cp2112.c | 103 ++++++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 102 insertions(+), 1 deletion(-) diff --git a/drivers/hid/hid-cp2112.c b/drivers/hid/hid-cp2112.c index 3952d90..a822db5 100644 --- a/drivers/hid/hid-cp2112.c +++ b/drivers/hid/hid-cp2112.c @@ -429,6 +429,105 @@ static int cp2112_write_req(void *buf, u8 slave_address, u8 command, u8 *data, return data_length + 4; } +static int cp2112_i2c_write_req(void *buf, u8 slave_address, u8 *data, + u8 data_length) +{ + struct cp2112_write_req_report *report = buf; + + if (data_length > sizeof(report->data)) + return -EINVAL; + + report->report = CP2112_DATA_WRITE_REQUEST; + report->slave_address = slave_address << 1; + report->length = data_length; + memcpy(report->data, data, data_length); + return data_length + 3; +} + +static int cp2112_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs, + int num) +{ + struct cp2112_device *dev = (struct cp2112_device *)adap->algo_data; + struct hid_device *hdev = dev->hdev; + u8 buf[64]; + ssize_t count; + unsigned int retries; + int ret; + + hid_dbg(hdev, "I2C %d messages\n", num); + + if (num != 1) { + hid_err(hdev, + "Multi-message I2C transactions not supported\n"); + return -EOPNOTSUPP; + } + + if (msgs->flags & I2C_M_RD) + count = cp2112_read_req(buf, msgs->addr, msgs->len); + else + count = cp2112_i2c_write_req(buf, msgs->addr, msgs->buf, + msgs->len); + + if (count < 0) + return count; + + ret = hid_hw_power(hdev, PM_HINT_FULLON); + if (ret < 0) { + hid_err(hdev, "power management error: %d\n", ret); + return ret; + } + + ret = cp2112_hid_output(hdev, buf, count, HID_OUTPUT_REPORT); + if (ret < 0) { + hid_warn(hdev, "Error starting transaction: %d\n", ret); + goto power_normal; + } + + for (retries = 0; retries < XFER_STATUS_RETRIES; ++retries) { + ret = cp2112_xfer_status(dev); + if (-EBUSY == ret) + continue; + if (ret < 0) + goto power_normal; + break; + } + + if (XFER_STATUS_RETRIES <= retries) { + hid_warn(hdev, "Transfer timed out, cancelling.\n"); + buf[0] = CP2112_CANCEL_TRANSFER; + buf[1] = 0x01; + + ret = cp2112_hid_output(hdev, buf, 2, HID_OUTPUT_REPORT); + if (ret < 0) + hid_warn(hdev, "Error cancelling transaction: %d\n", + ret); + + ret = -ETIMEDOUT; + goto power_normal; + } + + if (!(msgs->flags & I2C_M_RD)) + goto finish; + + ret = cp2112_read(dev, msgs->buf, msgs->len); + if (ret < 0) + goto power_normal; + if (ret != msgs->len) { + hid_warn(hdev, "short read: %d < %d\n", ret, msgs->len); + ret = -EIO; + goto power_normal; + } + +finish: + /* return the number of transferred messages */ + ret = 1; + +power_normal: + hid_hw_power(hdev, PM_HINT_NORMAL); + hid_dbg(hdev, "I2C transfer finished: %d\n", ret); + return ret; +} + static int cp2112_xfer(struct i2c_adapter *adap, u16 addr, unsigned short flags, char read_write, u8 command, int size, union i2c_smbus_data *data) @@ -595,7 +694,8 @@ power_normal: static u32 cp2112_functionality(struct i2c_adapter *adap) { - return I2C_FUNC_SMBUS_BYTE | + return I2C_FUNC_I2C | + I2C_FUNC_SMBUS_BYTE | I2C_FUNC_SMBUS_BYTE_DATA | I2C_FUNC_SMBUS_WORD_DATA | I2C_FUNC_SMBUS_BLOCK_DATA | @@ -605,6 +705,7 @@ static u32 cp2112_functionality(struct i2c_adapter *adap) } static const struct i2c_algorithm smbus_algorithm = { + .master_xfer = cp2112_i2c_xfer, .smbus_xfer = cp2112_xfer, .functionality = cp2112_functionality, }; -- 2.0.1 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH v2] HID: cp2112: add I2C mode 2014-07-07 23:25 ` [PATCH v2] " Antonio Borneo @ 2014-07-29 9:38 ` Jiri Kosina 0 siblings, 0 replies; 6+ messages in thread From: Jiri Kosina @ 2014-07-29 9:38 UTC (permalink / raw) To: Antonio Borneo Cc: linux-input, David Barksdale, linux-kernel, Benjamin Tissoires On Tue, 8 Jul 2014, Antonio Borneo wrote: > cp2112 supports single I2C read/write transactions. > It can't combine I2C transactions. > > Add master_xfer, using similar code flow as for smbus_xfer. > > Signed-off-by: Antonio Borneo <borneo.antonio@gmail.com> > Reviewed-by: Benjamin Tissoires <benjamin.tissoires@redhat.com> > --- > ChangeLog v1->v2: > - In cp2112_i2c_xfer() set variable "ret" directly with the > expected return value: either an error number or the number > of transferred messages. Applied, thanks. -- Jiri Kosina SUSE Labs ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2014-07-29 9:38 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2014-06-29 6:14 [PATCH] HID: cp2112: add I2C mode Antonio Borneo 2014-07-07 13:57 ` Benjamin Tissoires 2014-07-07 15:32 ` Antonio Borneo 2014-07-07 15:52 ` Benjamin Tissoires 2014-07-07 23:25 ` [PATCH v2] " Antonio Borneo 2014-07-29 9:38 ` Jiri Kosina
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox