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

[<Tharunkumar.Pasumarthi@microchip.com>]

On Thu, 2022-09-01 at 21:47 +0300, Andy Shevchenko wrote:
> 
> 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.

Okay. I will update in all the places.

> ...
> 
> > +/*
> 
> If it's a kernel doc, make it kernel doc.

This need not be kernel doc. I will add comments within structure.

> > + * struct pci1xxxx_i2c - private structure for the I2C controller
> > + * @i2c_xfer_done: used for synchronisation between foreground & isr
> ...
> 
> > +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?

Okay

> ...
> 
> > +static int release_sys_lock(void __iomem *addr)
> 
> Ditto.

Okay


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

Yes, buf will be passed as NULL in some cases. So, this check is required.

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

Okay. I will take care of this.

> ...
> 
> > +             regval &=  ~(I2C_INPUT_EN | I2C_OUTPUT_EN);
> 
> Extra space.
> 

Okay. I will remove extra spaces.

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

Okay

> ...
> > +                             pci1xxxx_i2c_set_readm(i2c, true);
> 
> > +
> 
> We don't need useless blank lines.

Okay. I will take care of this.

> 
> > +                     buf[0] = readb(i2c->i2c_base +
> > +                                   SMBUS_MST_BUF);
> 
> Why not on one line?

Okay. I will update.

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

Yes, Hardware supports this kind of access.

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

This handling takes care of special case when system is put to suspend when i2c
transfer is progress in driver. We will wait until transfer completes.

> > 
> > +     pci_wake_from_d3(pdev, FALSE);
> 
> What's FALSE and why false can't be used?

Okay, I will use false.

> ...
> > +static SIMPLE_DEV_PM_OPS(pci1xxxx_i2c_pm_ops, pci1xxxx_i2c_suspend,
> > +                      pci1xxxx_i2c_resume);
> 
> Use new macro which starts with DEFINE prefix.

Okay

> ...
> Missed period.
> 

Okay. I will take care of this.

> > +      */
> 
> > +
> 
> Useless blank line.

Okay, I will take care of this.

> 
> > +
> > +     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);

I am not getting. Are you suggesting to change API name to
devm_pci1xxxx_i2c_shutdown?


> With
> 
>         struct device *dev = &pdev->dev;
> 
> you may have some lines of code neater and shorter.

Okay. I will take care in all places.

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

I am not getting this comment. Can you kindly explain more.

> > +             return ret;
> 
> After fixing above, convert the error messages to use
> 
>         return dev_err_probe(...);
> 
> pattern.
> 

Okay.


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

I am not getting this comment also.


Thanks,
Tharun Kumar P