Re: [PATCH v12 2/3] i2c: npcm7xx: Add Nuvoton NPCM I2C controller driver

[Wolfram Sang]

On Thu, May 21, 2020 at 02:09:09PM +0300, Tali Perry wrote:
> Add Nuvoton NPCM BMC I2C controller driver.
> 
> Signed-off-by: Tali Perry <tali.perry1@xxxxxxxxx>

This is a very complex driver, so I can really comment only about high
level things. Thank you very much for keeping at it!

My code checkers say:

CHECKPATCH:
CHECK: usleep_range is preferred over udelay; see Documentation/timers/timers-howto.rst
#1210: FILE: drivers/i2c/busses/i2c-npcm7xx.c:1161:
+			udelay(200);

(a few of them)

GCC:
  CC      drivers/i2c/busses/i2c-npcm7xx.o
drivers/i2c/busses/i2c-npcm7xx.c: In function ânpcm_i2c_resetâ:
drivers/i2c/busses/i2c-npcm7xx.c:521:5: warning: variable âi2cctl2â set but not used [-Wunused-but-set-variable]


> +/* Status of one I2C module */
> +struct npcm_i2c {
> +	struct i2c_adapter adap;
> +	struct device *dev;
> +	unsigned char __iomem *reg;
> +	spinlock_t lock;   /* IRQ synchronization */
> +	struct completion cmd_complete;
> +	int irq;
> +	int cmd_err;
> +	struct i2c_msg *msgs;
> +	int msgs_num;
> +	int num;
> +	u32 apb_clk;
> +	struct i2c_bus_recovery_info rinfo;
> +	enum i2c_state state;
> +	enum i2c_oper operation;
> +	enum i2c_mode master_or_slave;
> +	enum i2c_state_ind stop_ind;
> +	u8 dest_addr;
> +	u8 *rd_buf;
> +	u16 rd_size;
> +	u16 rd_ind;
> +	u8 *wr_buf;
> +	u16 wr_size;
> +	u16 wr_ind;
> +	bool fifo_use;
> +	u16 PEC_mask; /* PEC bit mask per slave address */
> +	bool PEC_use;
> +	bool read_block_use;
> +	u8 int_cnt;

What is this for? It is written to but never read.

> +	u32 clk_period_us;

Not used? Seems this struct could need some cleaning up.

> +	unsigned long int_time_stamp;
> +	unsigned long bus_freq; /* in kHz */
> +	u32 xmits;
> +#ifdef CONFIG_DEBUG_FS
> +	struct dentry *debugfs; /* debugfs device directory */
> +	u64 ber_cnt;
> +	u64 rec_succ_cnt;
> +	u64 rec_fail_cnt;
> +	u64 nack_cnt;
> +	u64 timeout_cnt;
> +#endif
> +};
> +

...

> +static inline u16 npcm_i2c_get_index(struct npcm_i2c *bus)
> +{
> +	if (bus->operation == I2C_READ_OPER)
> +		return bus->rd_ind;
> +	if (bus->operation == I2C_WRITE_OPER)
> +		return bus->wr_ind;
> +	return 0;

I2C_NO_OPER?

...

> +/* recovery using bit banging functionality of the module */
> +static int npcm_i2c_recovery_init(struct i2c_adapter *_adap)
> +{
> +	struct npcm_i2c *bus = container_of(_adap, struct npcm_i2c, adap);
> +	struct i2c_bus_recovery_info *rinfo = &bus->rinfo;
> +
> +	rinfo->recover_bus = npcm_i2c_recovery_tgclk;
> +	rinfo->prepare_recovery = NULL;
> +	rinfo->unprepare_recovery = NULL;
> +	rinfo->set_scl = NULL;
> +	rinfo->set_sda = NULL;

'bus' is kzalloced, so no need for these NULLs.

What I wonder more, though, is if you can't populate {set|get}_{scl|sda}
and use the internal i2c_generic_scl_recovery()? Are there any issues
with it?

> +
> +	dev_dbg(bus->dev, "init i2c recovery using TGCLK\n");

There is no error path here, so I think this message is not useful.
Means also this function could be 'void'.

> +
> +	rinfo->get_scl = npcm_i2c_get_SCL;
> +	rinfo->get_sda = npcm_i2c_get_SDA;

Not needed when you have a custom function.

> +
> +	_adap->bus_recovery_info = rinfo;
> +
> +	return 0;
> +}
> +

...

> +static int npcm_i2c_master_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs,
> +				int num)
> +{
> +	struct npcm_i2c *bus = container_of(adap, struct npcm_i2c, adap);
> +	struct i2c_msg *msg0, *msg1;
> +	unsigned long time_left, flags;
> +	u16 nwrite, nread;
> +	u8 *write_data, *read_data;
> +	u8 slave_addr;
> +	int timeout;
> +	int ret = 0;
> +	bool read_block = false;
> +	bool read_PEC = false;
> +	u8 bus_busy;
> +	unsigned long timeout_usec;
> +
> +	if (bus->state == I2C_DISABLE) {
> +		dev_err(bus->dev, "I2C%d module is disabled", bus->num);
> +		return -EINVAL;
> +	}
> +
> +	if (num > 2 || num < 1) {
> +		dev_err(bus->dev, "I2C cmd not supported num of msgs=%d", num);
> +		return -EINVAL;
> +	}

Since you have an 'i2c_adapter_quirks' struct filled, the core will
I2C check that for you.

> +
> +	msg0 = &msgs[0];
> +	slave_addr = msg0->addr;
> +	if (msg0->flags & I2C_M_RD) { /* read */
> +		if (num == 2) {
> +			dev_err(bus->dev, "num=2 but 1st msg rd instead of wr");
> +			return -EINVAL;

Ditto.

> +		}
> +		nwrite = 0;
> +		write_data = NULL;
> +		read_data = msg0->buf;
> +		if (msg0->flags & I2C_M_RECV_LEN) {
> +			nread = 1;
> +			read_block = true;
> +			if (msg0->flags & I2C_CLIENT_PEC)
> +				read_PEC = true;
> +		} else {
> +			nread = msg0->len;
> +		}
> +	} else { /* write */
> +		nwrite = msg0->len;
> +		write_data = msg0->buf;
> +		nread = 0;
> +		read_data = NULL;
> +		if (num == 2) {
> +			msg1 = &msgs[1];
> +			read_data = msg1->buf;
> +			if (slave_addr != msg1->addr) {
> +				dev_err(bus->dev,
> +					"SA==%02x but msg1->addr==%02x\n",
> +				       slave_addr, msg1->addr);
> +				return -EINVAL;

Ditto.

> +			}
> +			if ((msg1->flags & I2C_M_RD) == 0) {
> +				dev_err(bus->dev,
> +					"num = 2 but both msg are write.\n");
> +				return -EINVAL;
> +			}

Ditto.

> +			if (msg1->flags & I2C_M_RECV_LEN) {
> +				nread = 1;
> +				read_block = true;
> +				if (msg1->flags & I2C_CLIENT_PEC)
> +					read_PEC = true;
> +			} else {
> +				nread = msg1->len;
> +				read_block = false;
> +			}
> +		}
> +	}
> +
> +	/* Adaptive TimeOut: astimated time in usec  + 100% margin */
> +	timeout_usec = (2 * 10000 / bus->bus_freq) * (2 + nread + nwrite);
> +	timeout = max(msecs_to_jiffies(35), usecs_to_jiffies(timeout_usec));
> +	if (nwrite >= 32 * 1024 ||  nread >= 32 * 1024) {
> +		dev_err(bus->dev, "i2c%d buffer too big\n", bus->num);
> +		return -EINVAL;
> +	}

Ditto.

> +
> +	time_left = jiffies + msecs_to_jiffies(DEFAULT_STALL_COUNT) + 1;
> +	do {
> +		/*
> +		 * we must clear slave address immediately when the bus is not
> +		 * busy, so we spinlock it, but we don't keep the lock for the
> +		 * entire while since it is too long.
> +		 */
> +		spin_lock_irqsave(&bus->lock, flags);
> +		bus_busy = ioread8(bus->reg + NPCM_I2CCST) & NPCM_I2CCST_BB;
> +		spin_unlock_irqrestore(&bus->lock, flags);
> +
> +	} while (time_is_after_jiffies(time_left) && bus_busy);
> +
> +	if (bus_busy) {
> +		iowrite8(NPCM_I2CCST_BB, bus->reg + NPCM_I2CCST);
> +		npcm_i2c_reset(bus);
> +		i2c_recover_bus(adap);
> +		return -EAGAIN;
> +	}
> +
> +	npcm_i2c_init_params(bus);
> +	bus->dest_addr = slave_addr;
> +	bus->msgs = msgs;
> +	bus->msgs_num = num;
> +	bus->cmd_err = 0;
> +	bus->read_block_use = read_block;
> +
> +	reinit_completion(&bus->cmd_complete);
> +	if (!npcm_i2c_master_start_xmit(bus, slave_addr, nwrite, nread,
> +					write_data, read_data, read_PEC,
> +					read_block))
> +		ret = -EBUSY;
> +
> +	if (ret != -EBUSY) {
> +		time_left = wait_for_completion_timeout(&bus->cmd_complete,
> +							timeout);
> +
> +		if (time_left == 0) {
> +#ifdef CONFIG_DEBUG_FS
> +			if (bus->timeout_cnt == ULLONG_MAX) {
> +				dev_dbg(bus->dev,
> +					"timeout_cnt reach max, reset to 0");
> +				bus->timeout_cnt = 0;
> +			}
> +			bus->timeout_cnt++;
> +#endif
> +			if (bus->master_or_slave == I2C_MASTER) {
> +				i2c_recover_bus(adap);
> +				bus->cmd_err = -EIO;
> +				bus->state = I2C_IDLE;
> +			}
> +		}
> +	}
> +	ret = bus->cmd_err;
> +
> +	/* if there was BER, check if need to recover the bus: */
> +	if (bus->cmd_err == -EAGAIN)
> +		ret = i2c_recover_bus(adap);
> +
> +	return bus->cmd_err;
> +}
> +
> +static u32 npcm_i2c_functionality(struct i2c_adapter *adap)
> +{
> +	return I2C_FUNC_I2C |
> +	       I2C_FUNC_SMBUS_EMUL |
> +	       I2C_FUNC_SMBUS_BLOCK_DATA |
> +	       I2C_FUNC_SMBUS_PEC |
> +	       I2C_FUNC_SLAVE;
> +}
> +
> +static const struct i2c_adapter_quirks npcm_i2c_quirks = {
> +	.max_read_len = 32768,
> +	.max_write_len = 32768,

These limits are for simple reads/writes with num_msgs == 1. If you have
limits also for num_msgs == 2, then you also need to fill
'max_comb_1st_msg_len' and 'max_comb_2nd_msg_len'. (Because for some HW
these are different values then)

> +	.max_num_msgs = 2,

You can drop this because I2C_AQ_COMB_WRITE_THEN_READ implies it.

> +	.flags = I2C_AQ_COMB_WRITE_THEN_READ,
> +};
> +
> +static const struct i2c_algorithm npcm_i2c_algo = {
> +	.master_xfer = npcm_i2c_master_xfer,
> +	.functionality = npcm_i2c_functionality,
> +};
> +
> +#ifdef CONFIG_DEBUG_FS
> +/* i2c debugfs directory: used to keep health monitor of i2c devices */
> +static struct dentry *npcm_i2c_debugfs_dir;
> +
> +static void i2c_init_debugfs(struct platform_device *pdev, struct npcm_i2c *bus)
> +{
> +	struct dentry *d;
> +
> +	if (!npcm_i2c_debugfs_dir)
> +		return;
> +
> +	d = debugfs_create_dir(dev_name(&pdev->dev), npcm_i2c_debugfs_dir);
> +	if (IS_ERR_OR_NULL(d))
> +		return;
> +
> +	debugfs_create_u64("ber_cnt", 0444, d, &bus->ber_cnt);
> +	debugfs_create_u64("nack_cnt", 0444, d, &bus->nack_cnt);
> +	debugfs_create_u64("rec_succ_cnt", 0444, d, &bus->rec_succ_cnt);
> +	debugfs_create_u64("rec_fail_cnt", 0444, d, &bus->rec_fail_cnt);
> +	debugfs_create_u64("timeout_cnt", 0444, d, &bus->timeout_cnt);
> +
> +	bus->debugfs = d;
> +}
> +#else
> +static void i2c_init_debugfs(struct platform_device *pdev, struct npcm_i2c *bus)
> +{
> +}
> +#endif
> +
> +static int  npcm_i2c_probe_bus(struct platform_device *pdev)
> +{
> +	struct npcm_i2c *bus;
> +	struct i2c_adapter *adap;
> +	struct clk *i2c_clk;
> +	static struct regmap *gcr_regmap;
> +	static struct regmap *clk_regmap;
> +	int ret;
> +	int num;
> +
> +	bus = devm_kzalloc(&pdev->dev, sizeof(*bus), GFP_KERNEL);
> +	if (!bus)
> +		return -ENOMEM;
> +
> +	bus->dev = &pdev->dev;
> +
> +	num = of_alias_get_id(pdev->dev.of_node, "i2c");
> +	bus->num = num;

Why not assigning it directly and save the 'num' variable?

> +	/* core clk must be acquired to calculate module timing settings */
> +	i2c_clk = devm_clk_get(&pdev->dev, NULL);
> +	if (IS_ERR(i2c_clk))
> +		return PTR_ERR(i2c_clk);
> +	bus->apb_clk = clk_get_rate(i2c_clk);
> +
> +	gcr_regmap = syscon_regmap_lookup_by_compatible("nuvoton,npcm750-gcr");
> +	if (IS_ERR(gcr_regmap))
> +		return IS_ERR(gcr_regmap);
> +	regmap_write(gcr_regmap, NPCM_I2CSEGCTL, NPCM_I2CSEGCTL_INIT_VAL);
> +
> +	clk_regmap = syscon_regmap_lookup_by_compatible("nuvoton,npcm750-clk");
> +	if (IS_ERR(clk_regmap))
> +		return IS_ERR(clk_regmap);
> +
> +	bus->reg = devm_platform_ioremap_resource(pdev, 0);
> +	if (IS_ERR(bus->reg))
> +		return PTR_ERR((bus)->reg);
> +
> +	spin_lock_init(&bus->lock);
> +	init_completion(&bus->cmd_complete);
> +
> +	adap = &bus->adap;
> +	adap->owner = THIS_MODULE;
> +	adap->class = I2C_CLASS_HWMON | I2C_CLASS_SPD | I2C_CLIENT_SLAVE;

Since you have a DT compatible, you won't need classes. Just drop them.

> +	adap->retries = 3;
> +	adap->timeout = HZ;
> +	adap->algo = &npcm_i2c_algo;
> +	adap->quirks = &npcm_i2c_quirks;
> +	adap->algo_data = bus;
> +	adap->dev.parent = &pdev->dev;
> +	adap->dev.of_node = pdev->dev.of_node;
> +	adap->nr = pdev->id;
> +
> +	bus->irq = platform_get_irq(pdev, 0);
> +	if (bus->irq < 0)
> +		return bus->irq;
> +
> +	ret = devm_request_irq(bus->dev, bus->irq, npcm_i2c_bus_irq, 0,
> +			       dev_name(bus->dev), bus);
> +	if (ret)
> +		return ret;
> +
> +	ret = __npcm_i2c_init(bus, pdev);
> +	if (ret)
> +		return ret;
> +
> +	ret = npcm_i2c_recovery_init(adap);
> +	if (ret)
> +		return ret;
> +
> +	i2c_set_adapdata(adap, bus);
> +
> +	snprintf(bus->adap.name, sizeof(bus->adap.name), "Nuvoton i2c");

Maybe you want to add something more specific in case you have multiple
instances of this driver at runtime.

> +	ret = i2c_add_numbered_adapter(&bus->adap);
> +	if (ret) {
> +		dev_err(&pdev->dev, "failed to add numbered adapter %d\n", ret);

The I2C core will print warnings for you.

> +		return ret;
> +	}
> +	platform_set_drvdata(pdev, bus);
> +
> +	i2c_init_debugfs(pdev, bus);
> +	return 0;
> +}
> +
...

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

Okay, the GPIO fault injector could also need such a directory. I will
add this to the core. And then send an incremental patch for your
driver.

> +	if (IS_ERR_OR_NULL(dir))
> +		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);
> +#endif
> +
> +MODULE_AUTHOR("Avi Fishman <avi.fishman@xxxxxxxxx>");
> +MODULE_AUTHOR("Tali Perry <tali.perry@xxxxxxxxxxx>");
> +MODULE_AUTHOR("Tyrone Ting <kfting@xxxxxxxxxxx>");
> +MODULE_DESCRIPTION("Nuvoton I2C Bus Driver");
> +MODULE_LICENSE("GPL v2");
> +MODULE_VERSION("0.1.3");
> +
> -- 
> 2.22.0
>

Attachment: signature.asc
Description: PGP signature