Messages in this thread | | | Date | Wed, 20 May 2020 13:24:52 +0300 | From | Andy Shevchenko <> | Subject | Re: [PATCH v11 2/3] i2c: npcm7xx: Add Nuvoton NPCM I2C controller driver |
| |
On Wed, May 20, 2020 at 12:51:12PM +0300, Tali Perry wrote: > Add Nuvoton NPCM BMC I2C controller driver.
...
> +#ifdef CONFIG_DEBUG_FS
Why?!
> +#include <linux/debugfs.h> > +#endif
...
> +/* Status of one I2C module */ > +struct npcm_i2c { > + struct i2c_adapter adap;
> + struct device *dev;
Isn't it adap.dev->parent?
> +};
...
> +static void npcm_i2c_master_abort(struct npcm_i2c *bus) > +{ > + /* Only current master is allowed to issue a stop condition */
> + if (npcm_i2c_is_master(bus)) {
if (!npcm_i2c_is_master(bus)) return;
?
> + npcm_i2c_eob_int(bus, true); > + npcm_i2c_master_stop(bus); > + npcm_i2c_clear_master_status(bus); > + } > +}
...
> +/* SDA status is set - TX or RX, master */ > +static void npcm_i2c_irq_handle_sda(struct npcm_i2c *bus, u8 i2cst) > +{ > + u8 fif_cts;
> + if (bus->state == I2C_IDLE) { > + if (npcm_i2c_is_master(bus)) {
if (a) { if (b) { ... } }
==
if (a && b) { ... }
Check whole code for such pattern.
> + } > + > + /* SDA interrupt, after start\restart */ > + } else { > + if (NPCM_I2CST_XMIT & i2cst) { > + bus->operation = I2C_WRITE_OPER; > + npcm_i2c_irq_master_handler_write(bus); > + } else { > + bus->operation = I2C_READ_OPER; > + npcm_i2c_irq_master_handler_read(bus); > + } > + } > +}
...
> + } > +
+ /* 1MHz */ ?
> + else if (bus_freq_hz <= I2C_MAX_FAST_MODE_PLUS_FREQ) {
> + } > + > + /* Frequency larger than 1 MHZ is not supported */ > + else > + return -EINVAL;
...
> + // master and slave modes share a single irq.
It's again being inconsistent with comment style. Choose one and fix all comments accordingly (SPDX is another story, though)
...
> +static int i2c_debugfs_get(void *data, u64 *val) > +{ > + *val = *(u64 *)(data); > + return 0; > +} > +DEFINE_DEBUGFS_ATTRIBUTE(i2c_debugfs_ops, i2c_debugfs_get, NULL, "0x%02llx\n");
Why not to use debugfs_create_u64(), or how is it called?
> +static void i2c_init_debugfs(struct platform_device *pdev, struct npcm_i2c *bus) > +{ > + if (!npcm_i2c_debugfs_dir) > + return; > +
> + if (!pdev || !bus) > + return;
How is it possible?
> + bus->debugfs = debugfs_create_dir(dev_name(&pdev->dev), > + npcm_i2c_debugfs_dir); > + if (IS_ERR_OR_NULL(bus->debugfs)) { > + bus->debugfs = NULL; > + return; > + }
struct dentry *d;
d = create(...); if (IS_ERR_OR_NULL(d)) return;
bus->... = d;
> + > + debugfs_create_file("ber_count", 0444, bus->debugfs, > + &bus->ber_count, > + &i2c_debugfs_ops); > + > + debugfs_create_file("rec_succ_count", 0444, bus->debugfs, > + &bus->rec_succ_count, > + &i2c_debugfs_ops); > + > + debugfs_create_file("rec_fail_count", 0444, bus->debugfs, > + &bus->rec_fail_count, > + &i2c_debugfs_ops); > + > + debugfs_create_file("nack_count", 0444, bus->debugfs, > + &bus->nack_count, > + &i2c_debugfs_ops); > + > + debugfs_create_file("timeout_count", 0444, bus->debugfs, > + &bus->timeout_count, > + &i2c_debugfs_ops); > +}
...
> +#ifdef CONFIG_DEBUG_FS
Why?!
> + i2c_init_debugfs(pdev, bus); > +#endif
...
> +#ifdef CONFIG_DEBUG_FS
Ditto.
> + debugfs_remove_recursive(bus->debugfs); > +#endif
> +static int __init npcm_i2c_init(void) > +{
> + npcm_i2c_debugfs_dir = debugfs_create_dir("i2c", NULL);
You didn't compile this with !CONFIG_DEBUG_FS?
> + if (IS_ERR_OR_NULL(npcm_i2c_debugfs_dir)) { > + pr_warn("i2c init of debugfs failed\n"); > + npcm_i2c_debugfs_dir = NULL; > + }
See above for the better pattern. Why do you need noisy warning? What does it say to user? Can they use device or not?
> + return 0; > +}
-- With Best Regards, Andy Shevchenko
| |