Messages in this thread | | | Date | Fri, 22 May 2020 15:27:17 +0300 | From | Andy Shevchenko <> | Subject | Re: [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
| |