Hi Peter, > >> This seems to behave rather strange for len > 4, and I do not see > >> anything preventing that from happening. > > Actually the check is in ccg_read() of the client driver at > > https://marc.info/?l=linux-usb&m=153618608301206&w=2 > > > >> Am I missing something, or do you need to add a quirk for max_read_len? > > I will add a check in this function as well. > > No, you should add a pointer to a struct i2c_adapter_quirks, with > max_read_len set I think. At least that's how e.g. i2c-qup.c does it. Ok, will fix in next version. > >>> + break; > >>> + mutex_lock(&i2cd->mutex); > >>> + for (i = 0; i < num; i++) { > >> > >> I don't get how this loop works. You do not seem to always start with a > start. > >> E.g., if the first message is I2C_M_RD, i2c_read() is called before > >> i2c_start(). Is that correct? > > That’s correct and it is because i2_read() itself does the START and STOP. > > Well, in that case, you don't fully support combined I2C transactions. > You cannot e.g. generate this from Documentation/i2c/i2c-protocol > > S Addr Rd [A] [Data] NA S Addr Wr [A] Data [A] P > > By your description, all reads are terminated by a stop, and that is a quirk. I > think you need to add some of the I2C_AQ_COMB* flags to the above > mentioned struct i2c_adapter_quirks. Ok, will add those quirks flags. Also will modify to have implicit STOP after last write message. > >> Also, if a message has I2C_M_STOP but not I2C_M_RD, the call to > >> i2c_stop() happens before the call the i2c_write(). What's up with that? > > Client driver sends I2C_M_STOP along with every write message. > > Why is it certain that the client driver in 2/2 is the only client of this adapter? > If that's really fundamentally the case, and it can't change for whatever > reason, then I think these things should be mentioned somewhere. There can be other client. I will update the driver with quirks and implicit STOP after last write message. Thanks Ajay -- nvpublic -- > >> I would expect an i2c_start() before the loop or first in the loop, > >> and a > >> i2c_stop() after the loop. > > I2c_read() function itself takes care of it. > > > >> As is, the stop after the loop is only called on error. > > To be exact, whenever there is error in i2c_write(). > > > >> In addition, I would expect messages that have I2C_M_STOP to trigger > >> an > >> i2c_stop() call *after* the message, even if the message is not last in the > loop. > > Yes, its like that for all write messages. > > > >> What am I missing? > >> > >>> + if (msgs[i].flags & I2C_M_RD) { > >>> + status = i2c_read(i2cd, msgs[i].buf, msgs[i].len); > >>> + if (status < 0) { > >>> + dev_err(dev, "i2c_read error %x", status); > >>> + goto unlock; > >>> + } > >>> + i2cd->do_start = true; > >>> + } else if (msgs[i].flags & I2C_M_STOP) { > >>> + status = i2c_stop(i2cd); > >>> + if (status < 0) { > >>> + dev_err(dev, "i2c_stop error %x", status); > >>> + goto unlock; > >>> + } > >>> + i2cd->do_start = true; > >>> + } else { > >>> + if (i2cd->do_start) { > >>> + status = i2c_start(i2cd, msgs[i].addr); > >>> + if (status < 0) { > >>> + dev_err(dev, "i2c_start error %x", > >>> + status); > >>> + goto unlock; > >>> + } > >>> + status = i2c_write(i2cd, msgs[i].addr << 1); > >>> + if (status < 0) { > >>> + dev_err(dev, "i2c_write error %x", > >>> + status); > >>> + goto stop; > >>> + } > >>> + i2cd->do_start = false; > >>> + } > >>> + for (j = 0; j < msgs[i].len; j++) { > >>> + status = i2c_write(i2cd, *(msgs[i].buf + j)); > >>> + if (status < 0) { > >>> + dev_err(dev, "i2c_write error %x", > >>> + status); > >>> + goto stop; > >>> + } > >>> + } > >>> + } > >>> + } > >>> + status = i; > >>> + goto unlock; > > <here> > > > >>> +stop: > >>> + status = i2c_stop(i2cd); > >>> + if (status < 0) > >>> + dev_err(dev, "i2c_stop error %x", status); > >> > >> Hmm, every time you call one of i2c_start, i2c_read, i2c_write or > >> i2c_stop you check for error and issue a generic dev_err message. I > >> think you should move these error messages into the functions instead > >> or repeating them for every use. > > Ok, will fix. > > > >>> +unlock: > >>> + mutex_unlock(&i2cd->mutex); > >>> + return status; > >> > >> Here you return status, which seems to be zero when everything is fine. > >> That is incorrect for sure; you should return num on success. > > When everything is fine then status is written with number of messages > > "i". Please see above. > > Right you are, I missed that assignment. Sorry... > > Cheers, > Peter > > > Thanks > > Ajay > > > > -- > > nvpublic > > -- > > > >> Cheers, > >> Peter > >> > >>> +} > >>> + > >>> +static u32 gpu_i2c_functionality(struct i2c_adapter *adap) { > >>> + return I2C_FUNC_I2C | I2C_FUNC_SMBUS_EMUL; } > >>> + > >>> +static const struct i2c_algorithm gpu_i2c_algorithm = { > >>> + .master_xfer = gpu_i2c_master_xfer, > >>> + .functionality = gpu_i2c_functionality, > >>> +}; > >>> + > >>> +/* > >>> + * This driver is for Nvidia GPU cards with USB Type-C interface. > >>> + * We want to identify the cards using vendor ID and class code > >>> +only > >>> + * to avoid dependency of adding product id for any new card which > >>> + * requires this driver. > >>> + * Currently there is no class code defined for UCSI device over > >>> +PCI > >>> + * so using UNKNOWN class for now and it will be updated when UCSI > >>> + * over PCI gets a class code. > >>> + * There is no other NVIDIA cards with UNKNOWN class code. Even if > >>> +the > >>> + * driver gets loaded for an undesired card then eventually > >>> +i2c_read() > >>> + * (initiated from UCSI i2c_client) will timeout or UCSI commands > >>> +will > >>> + * timeout. > >>> + */ > >>> +#define PCI_CLASS_SERIAL_UNKNOWN 0x0c80 > >>> +static const struct pci_device_id gpu_i2c_ids[] = { > >>> + { PCI_VENDOR_ID_NVIDIA, PCI_ANY_ID, PCI_ANY_ID, PCI_ANY_ID, > >>> + PCI_CLASS_SERIAL_UNKNOWN << 8, 0xffffff00}, > >>> + { } > >>> +}; > >>> +MODULE_DEVICE_TABLE(pci, gpu_i2c_ids); > >>> + > >>> +static int gpu_populate_client(struct gpu_i2c_dev *i2cd, int irq) { > >>> + static struct i2c_board_info gpu_ccgx_ucsi = { > >>> + I2C_BOARD_INFO("ccgx-ucsi", 0x8), > >>> + }; > >>> + > >>> + gpu_ccgx_ucsi.irq = irq; > >>> + i2cd->client = i2c_new_device(&i2cd->adapter, &gpu_ccgx_ucsi); > >>> + if (!i2cd->client) { > >>> + dev_err(i2cd->dev, "i2c_new_device failed\n"); > >>> + return -ENODEV; > >>> + } > >>> + return 0; > >>> +} > >>> + > >>> +static int gpu_i2c_probe(struct pci_dev *pdev, const struct > >>> +pci_device_id *id) { > >>> + struct gpu_i2c_dev *i2cd; > >>> + int status; > >>> + > >>> + i2cd = devm_kzalloc(&pdev->dev, sizeof(struct gpu_i2c_dev), > >> GFP_KERNEL); > >>> + if (!i2cd) > >>> + return -ENOMEM; > >>> + > >>> + i2cd->dev = &pdev->dev; > >>> + dev_set_drvdata(&pdev->dev, i2cd); > >>> + > >>> + status = pcim_enable_device(pdev); > >>> + if (status < 0) { > >>> + dev_err(&pdev->dev, "pcim_enable_device failed %d\n", > >> status); > >>> + return status; > >>> + } > >>> + > >>> + pci_set_master(pdev); > >>> + > >>> + i2cd->regs = pcim_iomap(pdev, 0, 0); > >>> + if (!i2cd->regs) { > >>> + dev_err(&pdev->dev, "pcim_iomap failed\n"); > >>> + return -ENOMEM; > >>> + } > >>> + > >>> + status = pci_alloc_irq_vectors(pdev, 1, 1, PCI_IRQ_MSI); > >>> + if (status < 0) { > >>> + dev_err(&pdev->dev, "pci_alloc_irq_vectors err %d\n", > >> status); > >>> + return status; > >>> + } > >>> + > >>> + i2cd->do_start = true; > >>> + mutex_init(&i2cd->mutex); > >>> + enable_i2c_bus(i2cd); > >>> + > >>> + i2c_set_adapdata(&i2cd->adapter, i2cd); > >>> + i2cd->adapter.owner = THIS_MODULE; > >>> + strlcpy(i2cd->adapter.name, "NVIDIA GPU I2C adapter", > >>> + sizeof(i2cd->adapter.name)); > >>> + i2cd->adapter.algo = &gpu_i2c_algorithm; > >>> + i2cd->adapter.dev.parent = &pdev->dev; > >>> + status = i2c_add_adapter(&i2cd->adapter); > >>> + if (status < 0) { > >>> + dev_err(&pdev->dev, "i2c_add_adapter failed %d\n", status); > >>> + goto free_irq_vectors; > >>> + } > >>> + > >>> + status = gpu_populate_client(i2cd, pdev->irq); > >>> + if (status < 0) { > >>> + dev_err(&pdev->dev, "gpu_populate_client failed %d\n", > >> status); > >>> + goto del_adapter; > >>> + } > >>> + > >>> + return 0; > >>> + > >>> +del_adapter: > >>> + i2c_del_adapter(&i2cd->adapter); > >>> +free_irq_vectors: > >>> + pci_free_irq_vectors(pdev); > >>> + return status; > >>> +} > >>> + > >>> +static void gpu_i2c_remove(struct pci_dev *pdev) { > >>> + struct gpu_i2c_dev *i2cd = dev_get_drvdata(&pdev->dev); > >>> + > >>> + i2c_del_adapter(&i2cd->adapter); > >>> + pci_free_irq_vectors(pdev); > >>> +} > >>> + > >>> +static int gpu_i2c_resume(struct device *dev) { > >>> + struct gpu_i2c_dev *i2cd = dev_get_drvdata(dev); > >>> + > >>> + enable_i2c_bus(i2cd); > >>> + return 0; > >>> +} > >>> + > >>> +static int gpu_i2c_idle(struct device *dev) { > >>> + struct gpu_i2c_dev *i2cd = dev_get_drvdata(dev); > >>> + > >>> + if (!mutex_trylock(&i2cd->mutex)) { > >>> + dev_info(dev, "-EBUSY\n"); > >>> + return -EBUSY; > >>> + } > >>> + mutex_unlock(&i2cd->mutex); > >>> + > >>> + return 0; > >>> +} > >>> + > >>> +UNIVERSAL_DEV_PM_OPS(gpu_i2c_driver_pm, NULL, gpu_i2c_resume, > >>> +gpu_i2c_idle); > >>> + > >>> +static struct pci_driver gpu_i2c_driver = { > >>> + .name = "nvidia-gpu", > >>> + .id_table = gpu_i2c_ids, > >>> + .probe = gpu_i2c_probe, > >>> + .remove = gpu_i2c_remove, > >>> + .driver = { > >>> + .pm = &gpu_i2c_driver_pm, > >>> + }, > >>> +}; > >>> + > >>> +module_pci_driver(gpu_i2c_driver); > >>> + > >>> +MODULE_AUTHOR("Ajay Gupta <ajayg@xxxxxxxxxx>"); > >>> +MODULE_DESCRIPTION("Nvidia GPU I2C controller Driver"); > >>> +MODULE_LICENSE("GPL v2"); > >>> > >
