Re: [PATCH RFC i2c-master] i2c: microchip: pci1xxxx: Add driver for I2C host controller in multifunction endpoint of pci1xxxx switch

[Tharunkumar.Pasumarthi]

On Tue, 2022-08-23 at 13:31 +0300, Krzysztof Kozlowski wrote:
> 
> 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...

Okay. I will fix Linux coding style for comments throughout the file in upcoming
patch.

> (...)
> 
> > +
> > +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.

I will remove this print

> > +
> > +     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...

Okay

> > +     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?

pci1xxxx_i2c_shutdown API will reset the registers that are set as part of
pci1xxxx_i2c_init API. I will update this API and also include this API in
failure case of pci_alloc_irq_vectors as well as devm_request_irq

> > +             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()?
> 

I will add this API in pci1xxxx_i2c_remove_pci

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

Thank you,
Tharun Kumar P