Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Thu, 1 Sep 2022 21:47:17 +0300
From:   Andy Shevchenko <andriy.shevchenko@linux.intel.com>
To:     Tharun Kumar P <tharunkumar.pasumarthi@microchip.com>
Cc:     linux-i2c@vger.kernel.org, linux-kernel@vger.kernel.org,
        wsa@kernel.org, krzk@kernel.org, jarkko.nikula@linux.intel.com,
        robh@kernel.org, semen.protsenko@linaro.org, sven@svenpeter.dev,
        jsd@semihalf.com, rafal@milecki.pl, olof@lixom.net, arnd@arndb.de,
        UNGLinuxDriver@microchip.com
Subject: Re: [PATCH v2 i2c-master] i2c: microchip: pci1xxxx: Add driver for
 I2C host controller in multifunction endpoint of pci1xxxx switch
Message-ID: <YxD+NTWok2vkYos/@smile.fi.intel.com>
References: <20220901013626.2213100-1-tharunkumar.pasumarthi@microchip.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20220901013626.2213100-1-tharunkumar.pasumarthi@microchip.com>
Organization: Intel Finland Oy - BIC 0357606-4 - Westendinkatu 7, 02160 Espoo
Precedence: bulk

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