On 2018-09-10 19:32, Ajay Gupta wrote:
>>> +static int ccg_read(struct ucsi_ccg *uc, u16 rab, u8 *data, u32 len)
>>> +{
>>> + struct i2c_client *client = uc->client;
>>> + unsigned char buf[2];
>>> + struct i2c_msg msgs[] = {
>>> + {
>>> + .addr = client->addr,
>>> + .flags = 0x0,
>>> + .len = 0x2,
>>
>> sizeof(buf)?
> ok
>>
>>> + .buf = buf,
>>> + },
>>> + {
>>> + .addr = client->addr,
>>> + .flags = I2C_M_RD,
>>> + .buf = data,
>>> + },
>>> + };
>>> + u32 rlen, rem_len = len;
>>> + int status;
>>> +
>>
>> If your target I2C adapter had supported larger reads, this would have been a
>> single xfer instead of the loop, correct? I think this deserves a comment, and
>> perhaps e.g. the eeprom drivers should be examined to see how they handle
>> deficient I2C adapters (there is a module_param named io_limit in the at24
>> driver). Because it is a little bit sad to penalise all users just because you have
>> an adapter with limitations.
>> Or is this driver tied to that adapter?
>> Anyway, I'm satisfied with a comment, as I don't care all that much.
> I am thinking of moving the loop to the i2c driver.
No, that will not work. The i2c driver cannot know how the splitting works,
since how to split transfers will typically be different for different drivers
needing large reads.
You will have to do the split here in this driver.
>>> + while (rem_len > 0) {
>>> + msgs[1].buf = &data[len - rem_len];
>>> + rlen = min_t(u16, rem_len, 4);
>>> + msgs[1].len = rlen;
>>> + put_unaligned_le16(rab, buf);
>>> + status = i2c_transfer(client->adapter, msgs,
>> ARRAY_SIZE(msgs));
>>> + 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[2];
>>> + struct i2c_msg msgs[] = {
>>> + {
>>> + .addr = client->addr,
>>> + .flags = 0x0,
>>> + .len = 0x2,
>>
>> sizeof(buf)?
> ok
>>
>>> + .buf = buf,
>>> + },
>>> + {
>>> + .addr = client->addr,
>>> + .flags = 0x0,
>>> + .buf = data,
>>> + .len = len,
>>> + },
>>> + };
>>> + int status;
>>> +
>>> + put_unaligned_le16(rab, buf);
>>> + status = i2c_transfer(client->adapter, msgs, ARRAY_SIZE(msgs));
>>> + if (status < 0) {
>>> + dev_err(uc->dev, "i2c_transfer failed %d\n", status);
>>> + return status;
>>> + }
>>> +
>>> + return 0;
>>> +}
>>> +
>>> +static int ucsi_ccg_init(struct ucsi_ccg *uc) {
>>> + struct device *dev = uc->dev;
>>> + unsigned int count = 10;
>>> + u8 data[64];
>>> + int status;
>>> +
>>> + status = ccg_read(uc, CCGX_I2C_RAB_DEVICE_MODE, data,
>> sizeof(data));
>>> + if (status < 0)
>>> + return status;
>>> +
>>> + dev_dbg(dev, "Silicon id %2ph", data +
>> CCGX_I2C_RAB_READ_SILICON_ID);
>>> + dev_dbg(dev, "FW1 version %8ph\n", data +
>> CCGX_I2C_RAB_FW1_VERSION);
>>> + dev_dbg(dev, "FW2 version %8ph\n", data +
>> CCGX_I2C_RAB_FW2_VERSION);
>>> +
>>> + data[0] = CCGX_I2C_RAB_UCSI_CONTROL_STOP;
>>> + status = ccg_write(uc, CCGX_I2C_RAB_UCSI_CONTROL, data, 0x1);
>>> + if (status < 0)
>>> + return status;
>>> +
>>> + data[0] = CCGX_I2C_RAB_UCSI_CONTROL_START;
>>> + status = ccg_write(uc, CCGX_I2C_RAB_UCSI_CONTROL, data, 0x1);
>>> + if (status < 0)
>>> + return status;
>>> +
>>> + /*
>>> + * Flush CCGx RESPONSE queue by acking interrupts
>>> + * - above ucsi control register write will push response
>>> + * which must be flushed
>>> + * - affects f/w update which reads response register
>>> + */
>>> + data[0] = 0xff;
>>> + do {
>>> + status = ccg_write(uc, CCGX_I2C_RAB_INTR_REG, data, 0x1);
>>> + if (status < 0)
>>> + return status;
>>> +
>>> + usleep_range(10000, 11000);
>>> +
>>> + status = ccg_read(uc, CCGX_I2C_RAB_INTR_REG, data, 0x1);
>>> + if (status < 0)
>>> + return status;
>>> + } while ((data[0] != 0x00) && count--);
>>> +
>>> + return 0;
>>> +}
>>> +
>>> +static int ucsi_ccg_send_data(struct ucsi_ccg *uc) {
>>> + unsigned char buf1[USBC_MSG_OUT_SIZE];
>>> + unsigned char buf2[USBC_CONTROL_SIZE];
>>> + int status;
>>> + u16 rab;
>>> +
>>> + memcpy(buf1, (u8 *)(uc->ppm.data) + USBC_MSG_OUT_OFFSET,
>> sizeof(buf1));
>>> + memcpy(buf2, (u8 *)(uc->ppm.data) + USBC_CONTROL_OFFSET,
>>> +sizeof(buf2));
>>
>> Hmm, now that I see what this function does, instead of just seeing a bunch of
>> magic numbers, I wonder why you make copies instead of feeding the correct
>> section of the ppm.data buffer directly to ccg_write, like you do below for
>> recv?
> Ok, will fix.
Hmm, now that I see this again, it makes me wonder why you complained about
copying the buffer to fix the misunderstanding of the i2c_transfer interface,
when you already copy the buffer in the first place?
>>> +
>>> + rab = CCGX_I2C_RAB_UCSI_DATA_BLOCK(USBC_MSG_OUT_OFFSET);
>>> + status = ccg_write(uc, rab, buf1, sizeof(buf1));
>>> + if (status < 0)
>>> + return status;
>>> +
>>> + rab = CCGX_I2C_RAB_UCSI_DATA_BLOCK(USBC_CONTROL_OFFSET);
>>> + return ccg_write(uc, rab, buf2, sizeof(buf2)); }
>>> +
>>> +static int ucsi_ccg_recv_data(struct ucsi_ccg *uc) {
>>> + u8 *ppm = (u8 *)uc->ppm.data;
>>> + int status;
>>> + u16 rab;
>>> +
>>> + rab = CCGX_I2C_RAB_UCSI_DATA_BLOCK(USBC_CCI_OFFSET);
>>> + status = ccg_read(uc, rab, ppm + USBC_CCI_OFFSET, USBC_CCI_SIZE);
>>> + if (status < 0)
>>> + return status;
>>> +
>>> + rab = CCGX_I2C_RAB_UCSI_DATA_BLOCK(USBC_MSG_IN_OFFSET);
>>> + return ccg_read(uc, rab, ppm + USBC_MSG_IN_OFFSET,
>>> +USBC_MSG_IN_SIZE); }
>>> +
>>> +static int ucsi_ccg_ack_interrupt(struct ucsi_ccg *uc) {
>>> + int status;
>>> + unsigned char buf[1] = {0x0};
>>
>> The initializer can be dropped.
> ok
>>
>>> +
>>> + status = ccg_read(uc, CCGX_I2C_RAB_INTR_REG, buf, 0x1);
>>
>> sizeof(buf)?
> ok
>>
>>> + if (status < 0)
>>> + return status;
>>> +
>>> + return ccg_write(uc, CCGX_I2C_RAB_INTR_REG, buf, 0x1);
>>
>> sizeof(buf)?
> ok
>>
>>> +}
>>> +
>>> +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)
>>> +{
>>> + 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);
>>> + if (!uc->ppm.data)
>>> + return -ENOMEM;
>>
>> Wait a minute, ppm.data is allocated as a struct? And it's __packed! So, it's
>> clearly intended to match something real. I didn't notice that before, but that
>> means that all the new offsets and sizes defined in v10 are available with
>> offsetof() and sizeof() which would be much neater than a bunch of defines.
>> Sorry for not catching this earlier!
>>
>> See below for an example.
> Sure.
>>
>>> +
>>> + uc->ppm.cmd = ucsi_ccg_cmd;
>>> + uc->ppm.sync = ucsi_ccg_sync;
>>> + uc->dev = dev;
>>> + uc->client = client;
>>> +
>>> + /* 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,
>>> + IRQF_ONESHOT |
>> IRQF_TRIGGER_HIGH,
>>> + dev_name(dev), uc);
>>> + if (status < 0) {
>>> + dev_err(uc->dev, "request_threaded_irq failed - %d\n",
>> status);
>>> + 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_I2C_RAB_UCSI_DATA_BLOCK(USBC_VERSION_OFFSET);
>>> + status = ccg_read(uc, rab, (u8 *)(uc->ppm.data) +
>> USBC_VERSION_OFFSET,
>>> + USBC_VERSION_SIZE);
>>
>> E.g.
>> rab = CCGX_I2C_RAB_UCSI_DATA_BLOCK(offsetof(struct ucsi_data,
>> version));
>> status = ccg_read(uc, rab, (u8 *)&uc->ppm.data->version,
>> sizeof(uc->ppm.data->version));
>>
>> Hmm, but this highlights that you are not doing any endian conversion of the
>> fields in that struct as you read/write it.
>
>> Do you need to in case you have an endian mismatch?
> Looks like don't need it. I have tested it and it works as is.
Yeah, but have you tested the driver on a machine with the other byte-sex?
Cheers,
Peter
> Thanks
> Ajay
>
> --
> nvpublic
> --
>>
>> Cheers,
>> Peter
>>
>>> + 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",
>>> + },
>>> + .probe = ucsi_ccg_probe,
>>> + .remove = ucsi_ccg_remove,
>>> + .id_table = ucsi_ccg_device_id,
>>> +};
>>> +
>>> +module_i2c_driver(ucsi_ccg_driver);
>>> +
>>> +MODULE_AUTHOR("Ajay Gupta <ajayg@xxxxxxxxxx>");
>>> +MODULE_DESCRIPTION("UCSI driver for Cypress CCGx Type-C controller");
>>> +MODULE_LICENSE("GPL v2");
>>>
>
