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

[Tharunkumar.Pasumarthi]

On Tue, 2022-09-06 at 17:58 +0300, Andy Shevchenko wrote:
> + bits.h
> + types.h

Okay. I will check and include these header files.

> So, basically it's like this:
> 
>         void __iomem *bp = i2c->i2c_base + SMBUS_MST_BUF;
> 
>         if (slaveaddr)
>                 writeb(slaveaddr, bp++);
> 
>         if (buf)
>                 memcpy_toio(bp, buf, transferlen);
> 
> > +}

Okay. I will update code like this.

> ...
> 
> > +static void pci1xxxx_i2c_reset_counters(struct pci1xxxx_i2c *i2c)
> > +{
> > +     void __iomem *bp;
> 
> In all these helpers do like above:
> 
>         void __iomem *bp = i2c->i2c_base + SMBUS_CONTROL_REG_OFF;
> 
> It will unload code from duplicating noise.

Okay

> ...
> Why not positive conditional?
> 
>         if (ret == -EPERM) {
>                 ...
>         } else {
>                 ...
>         }
> 

Okay, I will change code like this.

> ...
> 
> > +     /*
> > +      * Enable Pullup for the SMB alert pin which is just used for
> 
> pull-up

Okay. I will modify like this.

> 
> > +             transferlen = min_t(u16, remainingbytes,
> > (u16)SMBUS_BUF_MAX_SIZE);
> 
> min_t (note 't' part) does type casting for you, no need to repeat this.
> 

okay.

> ...
> 
> > +             if ((count + transferlen >= total_len) &&
> > +                             (i2c->flags & I2C_FLAGS_STOP)) {
> 
> Broken indentation (align ( under ( in the second line)

Okay.

> 
> ...
> 
> > +             time_left = wait_for_completion_timeout
> > +                             (&i2c->i2c_xfer_done,
> 
> Why this on a separate line? Join them together.

Okay. I will join.

> > +             } else {
> > +                     memcpy_fromio(&buf[count], bp + SMBUS_MST_BUF,
> > transferlen);
> > +             }
> 
> Hmm... count in the second case because we may not write all in one transfer?

Yes. Harware supports transferring 128 bytes at a time.

> ...
> >              writeb(0x00, bp + SMB_CORE_CMD_REG_OFF1);
> 
> 0x00 --> 0

Okay.

> (below as well)

Okay.

> > +                     transferlen = min_t(u16, (u16)SMBUS_BUF_MAX_SIZE - 1,
> > +                                       remainingbytes);
> 
> No castings.

Okay.

> > +                     transferlen = min_t(u16, (u16)SMBUS_BUF_MAX_SIZE,
> > +                                       remainingbytes);
> 
> Ditto.

Okay.

> > +             if (remainingbytes <= transferlen &&
> > +                             (i2c->flags & I2C_FLAGS_STOP))
> 
> Indentation needs to be fixed.

Okay. I will fix indentation

> > +
> > +                     writeb(COMPLETION_MNAKX, i2c->i2c_base +
> > +                                             SMB_CORE_COMPLETION_REG_OFF3);
> 
>                         writeb(COMPLETION_MNAKX,
>                                i2c->i2c_base + SMB_CORE_COMPLETION_REG_OFF3);
> 
> looks better.

Okay. I will change code as suggested.

> 
> > +     .owner  = THIS_MODULE,
> > +     .name   = "Pci1xxxx I2C Adapter",
> 
> Wouldn't PCI1xxxx look better?
> 

Yes. I will use PCI1xxxx.

> > +     .algo   = &pci1xxxx_i2c_algo,
> > +};
> 
> ...
> 
> > +     device_set_wakeup_enable(dev, TRUE);
> > +     pci_wake_from_d3(pdev, TRUE);
> 
> What's TRUE? Why true can't be used?

I will use true.

> ...
> 
> > +     struct pci1xxxx_i2c *i2c;
> > +     struct device *dev;
> > +     int ret;
> > +
> > +     dev = &pdev->dev;
> 
> The idea is when we 100% know that the dereference is okay and can't fail, we
> move assignments like this to the definition block above, otherwise, when an
> additional check is needed, we keep the assignment closer to its first user
> (conditional).

Okay.

> > +     ret = pcim_iomap_regions(pdev, BIT(0), pci_name(pdev));
> > +     if (ret < 0)
> 
> > +             return -ENOMEM;
> 
> What's wrong with
> 
>                 return ret;
> 

Yes, return ret can be used. I will check and use it.



Thanks,
Tharun Kumar P