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;
Message-ID: <8712b866-af22-d9ac-fd84-7d77c2628682@linaro.org>
Date:   Tue, 23 Aug 2022 13:31:02 +0300
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101
 Thunderbird/91.12.0
Subject: Re: [PATCH RFC i2c-master] i2c: microchip: pci1xxxx: Add driver for
 I2C host controller in multifunction endpoint of pci1xxxx switch
Content-Language: en-US
To:     Tharun Kumar P <tharunkumar.pasumarthi@microchip.com>,
        linux-i2c@vger.kernel.org, linux-kernel@vger.kernel.org,
        wsa@kernel.org
Cc:     andriy.shevchenko@linux.intel.com, 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
References: <20220823145603.2606475-1-tharunkumar.pasumarthi@microchip.com>
From:   Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
In-Reply-To: <20220823145603.2606475-1-tharunkumar.pasumarthi@microchip.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
Precedence: bulk

On 23/08/2022 17:56, 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 endpoint
> of the switch.

(...)

> +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);
> +
> +	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 regs
> +	 */

Use Linux coding style comments. Everywhere...

(...)

> +
> +static int pci1xxxx_i2c_probe_pci(struct pci_dev *pdev,
> +				  const struct pci_device_id *ent)
> +{
> +	struct pci1xxxx_i2c *i2c;
> +	int ret;
> +
> +	i2c = devm_kzalloc(&pdev->dev, sizeof(*i2c), GFP_KERNEL);
> +	if (!i2c)
> +		return -ENOMEM;
> +
> +	pci_set_drvdata(pdev, i2c);
> +
> +	i2c->i2c_xfer_in_progress = false;
> +
> +	ret = pcim_enable_device(pdev);
> +	if (ret)
> +		return ret;
> +
> +	pci_set_master(pdev);
> +
> +	/* we are getting the base address of the SMB core. SMB core uses
> +	 * BAR0 and 32K is the size here pci_resource_len returns 32K by
> +	 * reading BAR0
> +	 */
> +
> +	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);
> +
> +	dev_info(&pdev->dev, "i2c clock freq: %d\n", i2c->freq);

That's not a helpful print. Don't pollute dmesg.

> +
> +	ret = pci_alloc_irq_vectors(pdev, 1, 1, PCI_IRQ_ALL_TYPES);
> +	if (ret < 0)
> +		return ret;
> +
> +	/*Register the isr. we are not using any isr flags here.*/

Use Linux coding style comments. Everywhere...

> +	ret = devm_request_irq(&pdev->dev, pci_irq_vector(pdev, 0),
> +			       pci1xxxx_i2c_isr, PCI1XXXX_IRQ_FLAGS,
> +			       pci_name(pdev), i2c);
> +	if (ret)
> +		goto err_free_region;
> +
> +	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 = i2c_add_adapter(&i2c->adap);
> +	if (ret) {
> +		dev_err(&pdev->dev, "i2c add adapter failed = %d\n", ret);
> +		pci1xxxx_i2c_shutdown(i2c);

Why do you call here pci1xxxx_i2c_shutdown() but not in previous error path?

> +		goto err_free_region;
> +	}
> +
> +	return 0;
> +
> +err_free_region:
> +	pci_free_irq_vectors(pdev);
> +	return ret;
> +}
> +
> +static void pci1xxxx_i2c_remove_pci(struct pci_dev *pdev)
> +{
> +	struct pci1xxxx_i2c *i2c = pci_get_drvdata(pdev);

No need for pci_free_irq_vectors()?

> +
> +	i2c_del_adapter(&i2c->adap);
> +	pci1xxxx_i2c_shutdown(i2c);
> +}
> +

Best regards,
Krzysztof