On Thu, Sep 01, 2022 at 07:06:26AM +0530, Tharun Kumar P wrote:
> Microchip pci1xxxx is an unmanaged PCIe3.1a Switch for Consumer,
> Industrial and Automotive applications. This switch has multiple
> downstream ports. In one of the Switch's Downstream port, there
> is a multifunction endpoint for peripherals which includes an I2C
> host controller. The I2C function in the endpoint operates at 100KHz,
> 400KHz and 1 MHz and has buffer depth of 128 bytes.
> This patch provides the I2C controller driver for the I2C function
> of the switch.
...
> +#define SMB_IDLE_SCALING_100KHZ ((FAIR_IDLE_DELAY_100KHZ_TICKS << 16) | \
> + FAIR_BUS_IDLE_MIN_100KHZ_TICKS)
This kind of indentation harder to read than
#define SMB_IDLE_SCALING_100KHZ \
((FAIR_IDLE_DELAY_100KHZ_TICKS << 16) | FAIR_BUS_IDLE_MIN_100KHZ_TICKS)
Ditto for other similar cases.
...
> +/*
If it's a kernel doc, make it kernel doc.
> + * struct pci1xxxx_i2c - private structure for the I2C controller
> + * @i2c_xfer_done: used for synchronisation between foreground & isr
> + * @i2c_xfer_in_progress: boolean to indicate whether a transfer is in
> + * progress or not
> + * @adap: I2C adapter instance
> + * @i2c_base: pci base address of the I2C controller
> + * @freq: frequency of I2C transfer
> + * @flags: internal flags to store transfer conditions
> + */
...
> +static int set_sys_lock(void __iomem *addr)
Why not to take pointer to your private structure and offset instead of address
and calc the address here?
...
> +static int release_sys_lock(void __iomem *addr)
Ditto.
...
> +static void pci1xxxx_i2c_buffer_write(struct pci1xxxx_i2c *i2c, u8 slaveaddr,
> + u8 transferlen, unsigned char *buf)
> +{
> + if (slaveaddr) {
> + writeb(slaveaddr, i2c->i2c_base + SMBUS_MST_BUF);
> + if (buf)
> + memcpy_toio((i2c->i2c_base + SMBUS_MST_BUF + 1), buf, transferlen);
> + } else {
> + if (buf)
> + memcpy_toio((i2c->i2c_base + SMBUS_MST_BUF), buf, transferlen);
> + }
Why do you need buf checks? Is your code can shoot itself in the foot?
> +}
...
> + regval &= ~(intr_msk);
> + regval |= (I2C_INPUT_EN | I2C_OUTPUT_EN);
> + regval |= (I2C_INPUT_EN | I2C_OUTPUT_EN);
And in a plenty places you add extra parentheses. Reread your code and drop
them and in some cases (I will show below an example) move code to shorter
amount of LoCs.
...
> + regval &= ~(I2C_INPUT_EN | I2C_OUTPUT_EN);
Extra space.
...
> + pci1xxxx_i2c_config_high_level_intr(i2c, (I2C_BUF_MSTR_INTR_MASK),
> + true);
Why parentheses? Why it can't be one line?
There are more examples like this. Fix them all.
...
> + if (i2c->flags & I2C_FLAGS_SMB_BLK_READ)
> + pci1xxxx_i2c_set_readm(i2c, true);
> +
We don't need useless blank lines.
> + } else {
> + pci1xxxx_i2c_set_count(i2c, 0, 0, transferlen);
> +
> + pci1xxxx_i2c_config_asr(i2c, false);
> +
> + pci1xxxx_i2c_clear_flags(i2c);
> +
> + pci1xxxx_i2c_set_transfer_dir(i2c, I2C_DIRN_READ);
> + }
...
> + if (i2c->flags & I2C_FLAGS_SMB_BLK_READ) {
> + buf[0] = readb(i2c->i2c_base +
> + SMBUS_MST_BUF);
Why not on one line?
> + read_count = buf[0];
> + memcpy_fromio(&buf[1], (i2c->i2c_base +
> + SMBUS_MST_BUF + 1),
> + read_count);
> + } else {
> + memcpy_fromio(&buf[count], (i2c->i2c_base +
> + SMBUS_MST_BUF), transferlen);
> + }
These accessors may copy from 1 to 4 bytes at a time. Does your hardware
supports this kind of accesses?
...
> +static int pci1xxxx_i2c_suspend(struct device *dev)
> +{
> + struct pci1xxxx_i2c *i2c = dev_get_drvdata(dev);
> + struct pci_dev *pdev = to_pci_dev(dev);
> + u32 regval;
> +
> + i2c_mark_adapter_suspended(&i2c->adap);
> +
> + while ((i2c->i2c_xfer_in_progress))
> + msleep(20);
Each long sleep (20 ms is quite long) has to be explained. But this entire loop
looks like a band-aid of lack of IRQ or so. Why do you need to poll?
> + pci1xxxx_i2c_config_high_level_intr(i2c,
> + SMBALERT_WAKE_INTR_MASK,
> + true);
> +
> + /*
> + * Enable the PERST_DIS bit to mask the PERST from
> + * resetting the core registers
> + */
> + regval = readl(i2c->i2c_base + SMBUS_RESET_REG);
> + regval |= PERI_SMBUS_D3_RESET_DIS;
> + writel(regval, i2c->i2c_base + SMBUS_RESET_REG);
> + /* Enable PCI wake in the PMCSR register */
> + device_set_wakeup_enable(dev, TRUE);
> + pci_wake_from_d3(pdev, TRUE);
> +
> + return 0;
> +}
...
> + pci_wake_from_d3(pdev, FALSE);
What's FALSE and why false can't be used?
...
> +static SIMPLE_DEV_PM_OPS(pci1xxxx_i2c_pm_ops, pci1xxxx_i2c_suspend,
> + pci1xxxx_i2c_resume);
Use new macro which starts with DEFINE prefix.
...
> + /*
> + * We are getting the base address of the SMB core. SMB core uses
> + * BAR0 and size is 32K
Missed period.
> + */
> +
Useless blank line.
> + ret = pcim_iomap_regions(pdev, BIT(0), pci_name(pdev));
> + if (ret < 0)
> + return -ENOMEM;
> + i2c->i2c_base = pcim_iomap_table(pdev)[0];
> +
> + init_completion(&i2c->i2c_xfer_done);
> +
> + pci1xxxx_i2c_init(i2c);
Here you need to wrap pci1xxxx_i2c_shutdown() to be devm_. See below.
> + ret = pci_alloc_irq_vectors(pdev, 1, 1, PCI_IRQ_ALL_TYPES);
> + if (ret < 0) {
> + pci1xxxx_i2c_shutdown(i2c);
> + return ret;
> + }
> +
> + /* Register the isr. We are not using any isr flags here. */
> + ret = devm_request_irq(&pdev->dev, pci_irq_vector(pdev, 0),
With
struct device *dev = &pdev->dev;
you may have some lines of code neater and shorter.
> + pci1xxxx_i2c_isr, PCI1XXXX_IRQ_FLAGS,
> + pci_name(pdev), i2c);
> + if (ret) {
> + pci1xxxx_i2c_shutdown(i2c);
> + return ret;
> + }
> +
> + i2c->adap = pci1xxxx_i2c_ops;
> + i2c->adap.class = I2C_CLASS_SPD;
> + i2c->adap.dev.parent = &pdev->dev;
> +
> + snprintf(i2c->adap.name, sizeof(i2c->adap.name),
> + "MCHP PCI1xxxx i2c adapter at %s", pci_name(pdev));
> +
> + i2c_set_adapdata(&i2c->adap, i2c);
> +
> + ret = devm_i2c_add_adapter(&pdev->dev, &i2c->adap);
> + if (ret) {
> + dev_err(&pdev->dev, "i2c add adapter failed = %d\n", ret);
> + pci1xxxx_i2c_shutdown(i2c);
You can't mix devm_ and non-devm_ in such manner. It's asking for troubles at
->remove() or unbind stages.
> + return ret;
After fixing above, convert the error messages to use
return dev_err_probe(...);
pattern.
> + }
...
> +static void pci1xxxx_i2c_remove_pci(struct pci_dev *pdev)
> +{
> + struct pci1xxxx_i2c *i2c = pci_get_drvdata(pdev);
> +
> + pci1xxxx_i2c_shutdown(i2c);
> +}
This will be gone.
--
With Best Regards,
Andy Shevchenko
