lkml.org 
[lkml]   [2020]   [May]   [22]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH v13 2/3] i2c: npcm7xx: Add Nuvoton NPCM I2C controller driver
    On Fri, May 22, 2020 at 02:33:11PM +0300, Tali Perry wrote:
    > Add Nuvoton NPCM BMC I2C controller driver.

    I thought we are waiting for Wolfram finishing his review...


    In any case see couple of comments below.

    ...


    > +#ifdef CONFIG_DEBUG_FS

    Now, do we need the rest of DEBUG_FS guards?

    > + if (status) {
    > + if (bus->rec_fail_cnt == ULLONG_MAX) {
    > + dev_dbg(bus->dev, "rec_fail_cnt reach max, reset to 0");

    > + bus->rec_fail_cnt = 0;

    It's redundant, since we will anyway roll over when incrementing.
    https://stackoverflow.com/q/18195715/2511795

    > + }
    > + bus->rec_fail_cnt++;
    > + } else {
    > + if (bus->rec_succ_cnt == ULLONG_MAX) {
    > + dev_dbg(bus->dev, "rec_succ_cnt reach max, reset to 0");

    > + bus->rec_succ_cnt = 0;

    Ditto.

    > + }
    > + bus->rec_succ_cnt++;
    > + }
    > +#endif

    ...

    > +static int npcm_i2c_remove_bus(struct platform_device *pdev)
    > +{
    > + unsigned long lock_flags;
    > + struct npcm_i2c *bus = platform_get_drvdata(pdev);
    > +
    > + spin_lock_irqsave(&bus->lock, lock_flags);
    > + npcm_i2c_disable(bus);
    > + spin_unlock_irqrestore(&bus->lock, lock_flags);
    > + i2c_del_adapter(&bus->adap);

    > + debugfs_remove_recursive(bus->debugfs);

    This should be in reversed order, i.e. you inited last in ->probe(), thus
    should remove first in ->remove().

    > + return 0;
    > +}

    ...

    > +static int __init npcm_i2c_init(void)
    > +{
    > + struct dentry *dir;
    > +
    > + dir = debugfs_create_dir("i2c", NULL);

    > + if (IS_ERR_OR_NULL(dir))

    IS_ERR() is redundant. And NULL already being checked inside i2c_init_debugfs()
    or how do you call it?

    > + return 0;
    > +
    > + npcm_i2c_debugfs_dir = dir;
    > + return 0;
    > +}
    > +
    > +static void __exit npcm_i2c_exit(void)
    > +{
    > + debugfs_remove_recursive(npcm_i2c_debugfs_dir);
    > +}

    > +
    > +module_init(npcm_i2c_init);
    > +module_exit(npcm_i2c_exit);

    Slightly better to attach to the respective function, like other macros above
    do.

    --
    With Best Regards,
    Andy Shevchenko


    \
     
     \ /
      Last update: 2020-05-22 14:28    [W:2.609 / U:0.032 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site