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
