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



_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel