lkml.org 
[lkml]   [2022]   [Sep]   [1]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH v2 i2c-master] i2c: microchip: pci1xxxx: Add driver for I2C host controller in multifunction endpoint of pci1xxxx switch
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


\
 
 \ /
  Last update: 2022-09-01 20:49    [W:0.070 / U:0.348 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site