usb: typec: ucsi: add support for Cypress CCGx
PATCH v13 2/2 usb: typec: ucsi: add support for Cypress CCGx
Hi,
Quoted 51 lines · Ajay Gupta
Quoted 4 lines
Review
Quoted 1 line
(quoted diff) +1
-0+ /* i2c adapter (ccgx-ucsi) can read 4 byte max */By "i2c adapter" do you mean this Cypress CCGx controller, or the NVIDIA I2C host adapter?
It mean NVIDIA I2C host adapter with name "ccgx-ucsi"
Quoted 7 lines
Review
Quoted 3 lines
(quoted diff) +3
-0+ while (rem_len > 0) { + msgs[1].buf = &data[len - rem_len]; + rlen = min_t(u16, rem_len, 4);I guess this is where you check for that 4 bytes.
I'm guessing this limitation is for the NVIDIA I2C host adapter.
Correct
Quoted 1 line
If that is the case than this driver really should not care about it.
I got your point but need to handle this case gracefully.
I think best way to handle this is to add a runtime check to find I2C adapter's quirk and use
quirks->max_read_lenof the adapter. How does below look?(quoted diff) +7
-3@@ -247,6 +247,7 @@ struct ucsi_ccg { static int ccg_read(struct ucsi_ccg *uc, u16 rab, u8 *data, u32 len) { struct i2c_client *client = uc->client; + const struct i2c_adapter_quirks *quirks = client->adapter->quirks; unsigned char buf[2]; struct i2c_msg msgs[] = { {@@ -261,13 +262,16 @@ static int ccg_read(struct ucsi_ccg *uc, u16 rab, u8 *data, u32 len) .buf = data, }, }; - u32 rlen, rem_len = len; + u32 rlen, rem_len = len, max_read_len = len; int status; - /* i2c adapter (ccgx-ucsi) can read 4 byte max */ + /* check any max_read_len limitation on i2c adapter */ + if (quirks && quirks->max_read_len) + max_read_len = quirks->max_read_len; + while (rem_len > 0) { msgs[1].buf = &data[len - rem_len]; - rlen = min_t(u16, rem_len, 4); + rlen = min_t(u16, rem_len, max_read_len); msgs[1].len = rlen; put_unaligned_le16(rab, buf); status = i2c_transfer(client->adapter, msgs, ARRAY_SIZE(msgs));
Quoted 3 lines
Quoted 2 lines
We most likely need to use this driver on other platforms as well where the I2C host is something else.
Correct and above solution would not impact other I2C host.
I still didn't understand why can't this just be taken care of in your I2C host driver? Why can't you just read 4 bytes at a time in your master_xfer hook until you have received as much as the message is asking, and only after that return?
Quoted 93 lines
Quoted 83 lines
Review
Quoted 3 lines
(quoted diff) +3
-0+ msgs[1].len = rlen; + put_unaligned_le16(rab, buf); + status = i2c_transfer(client->adapter, msgs,
ARRAY_SIZE(msgs));Quoted 41 lines
(quoted diff) +41
-0+ if (status < 0) { + dev_err(uc->dev, "i2c_transfer failed %d\n", status); + return status; + } + rab += rlen; + rem_len -= rlen; + } + + return 0; +} + +static int ccg_write(struct ucsi_ccg *uc, u16 rab, u8 *data, u32 len) +{ + struct i2c_client *client = uc->client; + unsigned char *buf; + struct i2c_msg msgs[] = { + { + .addr = client->addr, + .flags = 0x0, + } + }; + int status; + + buf = kzalloc(len + sizeof(rab), GFP_KERNEL); + if (!buf) + return -ENOMEM; + + put_unaligned_le16(rab, buf); + memcpy(buf + sizeof(rab), data, len); + + msgs[0].len = len + sizeof(rab); + msgs[0].buf = buf; + + status = i2c_transfer(client->adapter, msgs, ARRAY_SIZE(msgs)); + if (status < 0) { + dev_err(uc->dev, "i2c_transfer failed %d\n", status); + kfree(buf); + return status; + } + + kfree(buf);Review
Quoted 10 lines
(quoted diff) +10
-0+ return 0; +} + +static int ucsi_ccg_init(struct ucsi_ccg *uc) { + unsigned int count = 10; + u8 data; + int status; + + data = CCGX_RAB_UCSI_CONTROL_STOP; + status = ccg_write(uc, CCGX_RAB_UCSI_CONTROL, &data,sizeof(data));
Review
Quoted 5 lines
(quoted diff) +5
-0+ if (status < 0) + return status; + + data = CCGX_RAB_UCSI_CONTROL_START; + status = ccg_write(uc, CCGX_RAB_UCSI_CONTROL, &data,sizeof(data));
Review
Quoted 5 lines
(quoted diff) +5
-0+ if (status < 0) + return status; + + /* + * Flush CCGx RESPONSE queue by acking interrupts. Above ucsicontrol
Review
Quoted 7 lines
(quoted diff) +7
-0+ * register write will push response which must be cleared. + */ + status = ccg_read(uc, CCGX_RAB_INTR_REG, &data, sizeof(data)); + if (status < 0) + return status; + do { + status = ccg_write(uc, CCGX_RAB_INTR_REG, &data,sizeof(data));
Review
Quoted 6 lines
(quoted diff) +6
-0+ if (status < 0) + return status; + + usleep_range(10000, 11000); + + status = ccg_read(uc, CCGX_RAB_INTR_REG, &data,sizeof(data));
Quoted 5 lines
Review
Quoted 3 lines
(quoted diff) +3
-0+ if (status < 0) + return status; + } while ((data != 0x00) && count--);What's the significance of that count?
It is like a retry count to clear interrupt status.
Quoted 1 line
Shouldn't you return
-ETIMEDOUTif count == 0?Yes. Good catch. Does the below fix looks ok?
Quoted 14 lines
do {
status = ccg_write(uc, CCGX_RAB_INTR_REG, &data, sizeof(data));
if (status < 0)
return status;
usleep_range(10000, 11000);
status = ccg_read(uc, CCGX_RAB_INTR_REG, &data, sizeof(data));
if (status < 0)
return status;
if (!data)
return 0;
} while (data && count--);Doesn't that condition break out of the loop immediately?
Quoted 187 lines
return -ETIMEDOUT;Quoted 185 lines
Something like:
... while (count--) status = ccg_read(uc, CCGX_RAB_INTR_REG, &data, sizeof(data)); if (status < 0) return status; if (!data) return 0; } return -ETIMEDOUT;Or does the count of 10 have some specific meaning?
Review
Quoted 8 lines
(quoted diff) +8
-0+} + +static int ucsi_ccg_send_data(struct ucsi_ccg *uc) { + u8 *ppm = (u8 *)uc->ppm.data; + int status; + u16 rab; + + rab = CCGX_RAB_UCSI_DATA_BLOCK(offsetof(struct ucsi_data,
message_out));Quoted 13 lines
(quoted diff) +13
-0+ status = ccg_write(uc, rab, ppm + + offsetof(struct ucsi_data, message_out), + sizeof(uc->ppm.data->message_out)); + if (status < 0) + return status; + + rab = CCGX_RAB_UCSI_DATA_BLOCK(offsetof(struct ucsi_data, ctrl)); + return ccg_write(uc, rab, ppm + offsetof(struct ucsi_data, ctrl), + sizeof(uc->ppm.data->ctrl)); +} + +static int ucsi_ccg_recv_data(struct ucsi_ccg *uc) { + u8 *ppm = (u8 *)uc->ppm.data;Review
Quoted 10 lines
(quoted diff) +10
-0+ int status; + u16 rab; + + rab = CCGX_RAB_UCSI_DATA_BLOCK(offsetof(struct ucsi_data, cci)); + status = ccg_read(uc, rab, ppm + offsetof(struct ucsi_data, cci), + sizeof(uc->ppm.data->cci)); + if (status < 0) + return status; + + rab = CCGX_RAB_UCSI_DATA_BLOCK(offsetof(struct ucsi_data,
message_in));Quoted 45 lines
(quoted diff) +45
-0+ return ccg_read(uc, rab, ppm + offsetof(struct ucsi_data, message_in), + sizeof(uc->ppm.data->message_in)); +} + +static int ucsi_ccg_ack_interrupt(struct ucsi_ccg *uc) { + int status; + unsigned char data; + + status = ccg_read(uc, CCGX_RAB_INTR_REG, &data, sizeof(data)); + if (status < 0) + return status; + + return ccg_write(uc, CCGX_RAB_INTR_REG, &data, sizeof(data)); } + +static int ucsi_ccg_sync(struct ucsi_ppm *ppm) { + struct ucsi_ccg *uc = container_of(ppm, struct ucsi_ccg, ppm); + int status; + + status = ucsi_ccg_recv_data(uc); + if (status < 0) + return status; + + /* ack interrupt to allow next command to run */ + return ucsi_ccg_ack_interrupt(uc); +} + +static int ucsi_ccg_cmd(struct ucsi_ppm *ppm, struct ucsi_control +*ctrl) { + struct ucsi_ccg *uc = container_of(ppm, struct ucsi_ccg, ppm); + + ppm->data->ctrl.raw_cmd = ctrl->raw_cmd; + return ucsi_ccg_send_data(uc); +} + +static irqreturn_t ccg_irq_handler(int irq, void *data) { + struct ucsi_ccg *uc = data; + + ucsi_notify(uc->ucsi); + + return IRQ_HANDLED; +} + +static int ucsi_ccg_probe(struct i2c_client *client, + const struct i2c_device_id *id) +{Review
Quoted 10 lines
(quoted diff) +10
-0+ struct device *dev = &client->dev; + struct ucsi_ccg *uc; + int status; + u16 rab; + + uc = devm_kzalloc(dev, sizeof(*uc), GFP_KERNEL); + if (!uc) + return -ENOMEM; + + uc->ppm.data = devm_kzalloc(dev, sizeof(struct ucsi_data),
GFP_KERNEL);Quoted 8 lines
(quoted diff) +8
-0+ if (!uc->ppm.data) + return -ENOMEM; + + uc->ppm.cmd = ucsi_ccg_cmd; + uc->ppm.sync = ucsi_ccg_sync; + uc->dev = dev; + uc->client = client; +Review
Quoted 10 lines
(quoted diff) +10
-0+ /* reset ccg device and initialize ucsi */ + status = ucsi_ccg_init(uc); + if (status < 0) { + dev_err(uc->dev, "ucsi_ccg_init failed - %d\n", status); + return status; + } + + uc->irq = client->irq; + + status = devm_request_threaded_irq(dev, uc->irq, NULL,
ccg_irq_handler,Review
Quoted 1 line
(quoted diff) +1
-0+ IRQF_ONESHOT |
IRQF_TRIGGER_HIGH,Review
Quoted 3 lines
(quoted diff) +3
-0+ dev_name(dev), uc); + if (status < 0) { + dev_err(uc->dev, "request_threaded_irq failed - %d\n",status);
Review
Quoted 10 lines
(quoted diff) +10
-0+ return status; + } + + uc->ucsi = ucsi_register_ppm(dev, &uc->ppm); + if (IS_ERR(uc->ucsi)) { + dev_err(uc->dev, "ucsi_register_ppm failed\n"); + return PTR_ERR(uc->ucsi); + } + + rab = CCGX_RAB_UCSI_DATA_BLOCK(offsetof(struct ucsi_data,version));
Quoted 30 lines
(quoted diff) +30
-0+ status = ccg_read(uc, rab, (u8 *)(uc->ppm.data) + + offsetof(struct ucsi_data, version), + sizeof(uc->ppm.data->version)); + if (status < 0) { + ucsi_unregister_ppm(uc->ucsi); + return status; + } + + i2c_set_clientdata(client, uc); + return 0; +} + +static int ucsi_ccg_remove(struct i2c_client *client) { + struct ucsi_ccg *uc = i2c_get_clientdata(client); + + ucsi_unregister_ppm(uc->ucsi); + + return 0; +} + +static const struct i2c_device_id ucsi_ccg_device_id[] = { + {"ccgx-ucsi", 0}, + {} +}; +MODULE_DEVICE_TABLE(i2c, ucsi_ccg_device_id); + +static struct i2c_driver ucsi_ccg_driver = { + .driver = { + .name = "ucsi_ccg", + },Review
Quoted 10 lines
(quoted diff) +10
-0+ .probe = ucsi_ccg_probe, + .remove = ucsi_ccg_remove, + .id_table = ucsi_ccg_device_id, +}; + +module_i2c_driver(ucsi_ccg_driver); + +MODULE_AUTHOR("Ajay Gupta <[email protected]>"); +MODULE_DESCRIPTION("UCSI driver for Cypress CCGx Type-C controller"); +MODULE_LICENSE("GPL v2");I'm still worried about how this driver works on other platforms. It just looks like you have written ccg_read/write() functions for only your I2C host.
Quoted 8 lines
Quoted 4 lines
I would feel much more comfortable with this if you for example used those i2c_smbus_read/write*() helpers instead of raw
i2c_transfer().
I would expect them to force you to write your i2c host driver, as well as this driver, in a more generic fashion.I2c_smbus_read/write*() will not work with Cypress CCGx controller since CCGx requires 2 byte of command for any read/write transaction. I2c_smbus_read/write*() APIs support only 1 byte of command.
OK, got it.
thanks,