Re: [PATCH v2] i2c: add support for Zhaoxin I2C controller

[Andi Shyti <andi.shyti@kernel.org>]

Hi Hans,

On Mon, May 29, 2023 at 03:31:47PM +0800, Hans Hu wrote:
> Add Zhaoxin I2C controller driver. It provides the access to the i2c
> busses, which connects to the touchpad, eeprom, etc.
> 
> Zhaoxin I2C controller have two separate busses, so may accommodate up

/have/has/

> to two I2C adapters. Those adapters are listed in the ACPI namespace
> with the "IIC1D17" HID, and probed by a platform driver.
> 
> The driver works with IRQ mode, and supports basic I2C features. Flags
> I2C_AQ_NO_ZERO_LEN and I2C_AQ_COMB_WRITE_THEN_READ are used to limit
> the unsupported access.
> 
> Change since v1:
>   * remove some useless include files and sort the rest.
>   * use mmio bar distinguish host index.
>   * use pci-dev's name and i2c-dev's name rename adapter's name.
>   * remove some debug code, fix some code style issue.
>   Link: https://lore.kernel.org/all/20230504060043.13155-1-hanshu-oc@zhaoxin.com/
> 
> Signed-off-by: Hans Hu <hanshu-oc@zhaoxin.com>

[...]

> +#define set_byte(r, d)           iowrite8(d, r+IICDATA2IIC)
> +#define get_byte(r)              ioread8(r+IICDATA2CPU)
> +#define is_ready(r)              (ioread8(r+IICSR)&READY)
> +#define is_nack(r)               (ioread8(r+IICSR)&RCV_NACK)
> +#define get_irq_status(r)        ioread8(r+IICISR)
> +#define get_reversion(r)         ioread8(r+IICREV)
> +#define clear_irq_status(r)      iowrite8(IRQ_STS_MASK, r+IICISR)
> +#define set_fifo_byte(r, d)      iowrite8(d, r+IICHTDR)
> +#define get_fifo_byte(r)         ioread8(r+IICHRDR)
> +#define set_fifo_wr_len(r, d)    iowrite8(d, r+IICHTLR)
> +#define set_fifo_rd_len(r, d)    iowrite8(d, r+IICHRLR)
> +#define get_fifo_wr_cnt(r)       ioread8(r+IICHWCNTR)
> +#define get_fifo_rd_cnt(r)       ioread8(r+IICHRCNTR)
> +#define master_regs_reset(r)     iowrite8(MST_RST|0x41, r+IICCR_L)
> +#define set_dynamic_clock(r, d)  iowrite8(d, r+IICMCR)
> +#define get_dynamic_clock(r)     (ioread8(r+IICMCR)&DYCLK_EN)
> +#define stop_write_byte(r)       iowrite8(TX_END|0x41, r+IICCR_L)
> +#define get_fstp_value(r)        ioread8(r+IICTR_FSTP)
> +#define enable_fifo_mode(r)      iowrite8(FIFO_EN, r+IICCR_H)
> +#define enable_byte_mode(r)      iowrite8(0, r+IICCR_H)

I guess checkpatch would give error here, please put some space
between '+'.

[...]

> +static void zxi2c_set_bus_speed(struct zxi2c *i2c)
> +{
> +	u8 i, count;
> +	const u32 *params = NULL;
> +
> +	count = ARRAY_SIZE(speed_params_table);
> +	for (i = 0; i < count; i++) {
> +		if (speed_params_table[i][0] == i2c->speed) {
> +			params = speed_params_table[i];
> +			break;
> +		}
> +	}
> +
> +	iowrite8(params[1], i2c->regs + IICTCR);
> +	if (abs(i2c->fstp - params[2]) > 0x10) {
> +		/* if BIOS setting value far from golden value,
> +		 * use golden value and warn user
> +		 */
> +		dev_warn_once(i2c->dev, "speed:%d, fstp:0x%x, golden:0x%x\n",
> +				i2c->speed, i2c->fstp, params[2]);
> +		iowrite8(params[2], i2c->regs + IICTR_FSTP);
> +	} else
> +		iowrite8(i2c->fstp, i2c->regs + IICTR_FSTP);

please use checkpatch.pl before sending the patch, you're missing
the brackets here.

> +	iowrite8(CLKSEL_50M, i2c->regs + IICCS);
> +	iowrite8(0xff, i2c->regs + IICTR_SCLTP);
> +
> +	/* for Hs-mode, use 0000 1000 as master code */
> +	if (i2c->speed == I2C_MAX_HIGH_SPEED_MODE_FREQ)
> +		iowrite8(0x08, i2c->regs + IIC_MST_CODE);
> +}

[...]

> +static irqreturn_t zxi2c_irq_handle(int irq, void *dev_id)
> +{
> +	struct zxi2c *i2c = (struct zxi2c *)dev_id;
> +	void __iomem *regs = i2c->regs;
> +	u8 status = get_irq_status(regs);
> +
> +	if ((status & IRQ_STS_MASK) == 0)
> +		return IRQ_NONE;

unlikely?

> +	if (status & IRQ_SCL_TIMEOUT)
> +		dev_warn(i2c->dev, "timeout(HW), ID: 0x%X\n", i2c->addr);
> +
> +	if (status & IRQ_STS_ADDRNACK) {
> +		dev_dbg(i2c->dev, "addr NACK, ID: 0x%X\n", i2c->addr);
> +	} else if (status & IRQ_STS_BYTEEND) {
> +		i2c->byte_left--;
> +		if (!i2c->is_read) {
> +			if (is_nack(regs)) {
> +				status = IRQ_STS_BYTENACK;
> +				i2c->byte_left++;
> +				dev_err(i2c->dev, "data NACK, ID: 0x%X\n",
> +					i2c->addr);
> +			} else if (i2c->byte_left == 0 && i2c->is_last_msg) {
> +				stop_write_byte(regs);
> +			}
> +		}
> +	}
> +
> +	i2c->event = status;
> +	clear_irq_status(regs);
> +	wake_up(&i2c->waitq);
> +
> +	return IRQ_HANDLED;
> +}
> +
> +static int zxi2c_wait_event(struct zxi2c *i2c, u8 event)
> +{
> +	int timeout;
> +
> +	timeout = wait_event_interruptible_timeout(i2c->waitq,
> +			i2c->event != 0,
> +			msecs_to_jiffies(ZXI2C_TIMEOUT));
> +
> +	if (i2c->event & event)
> +		return 0;

is this valid even when "timeout == 0"?

> +	if (timeout == 0) {
> +		dev_err(i2c->dev, "timeout(SW), ID: 0x%X\n", i2c->addr);
> +		/* Clock streching timeout, do recovery */
> +		if (!is_nack(i2c->regs))
> +			dev_err(i2c->dev, "device hang? reset, ID: 0x%X\n",
> +				i2c->addr);
> +
> +		master_regs_reset(i2c->regs);
> +		zxi2c_set_bus_speed(i2c);
> +	}
> +	return -ENODEV;

is ENODEV the right error code? Maybe -EIO?

> +}
> +
> +static int zxi2c_byte_xfer(struct zxi2c *i2c, struct i2c_msg *msgs, int num)
> +{
> +	u16 i, finished;
> +	int error;

please move this error declaration inside the for() to make it
clear where it's used (and possibly all the variables used only
inside the for()).

> +	u8 index, ret = 0;

this function returns an integer, but ret is u8. Please, declare
ret as int.

> +	struct i2c_msg *msg;
> +	void __iomem *regs = i2c->regs;
> +
> +	clear_irq_status(regs);
> +	enable_byte_mode(regs);
> +	zxi2c_enable_irq(regs, IRQ_EN_BYTEEND, true);
> +
> +	for (index = 0; index < num; index++) {
> +		msg = msgs + index;
> +		i2c->addr = msg->addr;
> +		i2c->is_read = !!(msg->flags & I2C_M_RD);
> +		i2c->byte_left = i2c->len = msg->len;
> +
> +		zxi2c_set_wr(regs, i2c->is_read);
> +		if (i2c->is_read) {
> +			zxi2c_prepare_next_read(regs, i2c->byte_left);
> +			zxi2c_start(i2c);
> +			/* create restart for non-first msg */
> +			if (index)
> +				zxi2c_continue(i2c);
> +
> +			for (i = 1; i <= msg->len; i++) {
> +				error = zxi2c_wait_event(i2c, IRQ_STS_BYTEEND);
> +				if (error)
> +					break;
> +
> +				msg->buf[i - 1] = get_byte(regs);
> +				if (i2c->byte_left == 0)
> +					break;
> +
> +				zxi2c_prepare_next_read(regs, i2c->byte_left);
> +				zxi2c_continue(i2c);
> +			}
> +		} else {
> +			set_byte(regs, msg->buf[0]);
> +			/* mark whether this is the last msg */
> +			i2c->is_last_msg = index == !!(num - 1);
> +			zxi2c_start(i2c);
> +			/* create restart for non-first msg */
> +			if (index)
> +				zxi2c_continue(i2c);
> +
> +			for (i = 1; i <= msg->len; i++) {
> +				error = zxi2c_wait_event(i2c, IRQ_STS_BYTEEND);
> +				if (error)
> +					break;
> +
> +				if (i2c->byte_left == 0)
> +					break;
> +				set_byte(regs, msg->buf[i]);
> +				zxi2c_continue(i2c);
> +			}
> +		}
> +
> +		if (error) {
> +			/* check if NACK during transmitting */
> +			finished = msg->len - i2c->byte_left;
> +			if (finished)
> +				dev_err(i2c->dev,
> +					"%s: %s finished %d bytes: %*ph\n",
> +					__func__,
> +					i2c->is_read ? "read" : "write",
> +					finished, finished, msg->buf);
> +			return error;
> +		}
> +		ret++;
> +	}
> +
> +	zxi2c_enable_irq(regs, IRQ_EN_BYTEEND, false);
> +	return ret;
> +}

[...]

> +static int zxi2c_probe(struct platform_device *pdev)
> +{
> +	int error;
> +	struct zxi2c *i2c;
> +	struct pci_dev *pci;
> +	struct device *dev;
> +
> +	/* make sure this is zhaoxin platform */
> +	dev = pdev->dev.parent;
> +	if (dev && dev_is_pci(dev)) {
> +		pci = to_pci_dev(dev);
> +		if (pci->vendor != 0x1d17 || pci->device != 0x1001)
> +			return -ENODEV;
> +	} else
> +		return -ENODEV;

please use brackets here (and checkpatch.pl before sending the
patch).

> +	i2c = devm_kzalloc(&pdev->dev, sizeof(*i2c), GFP_KERNEL);
> +	if (IS_ERR(i2c)) {
> +		dev_err(&pdev->dev, "devm_kzalloc failed\n");
> +		return -ENOMEM;

please use dev_err_probe() here and everywhere you are printing
and returning an error.

> +	}
> +
> +	i2c->pci = pci;
> +	i2c->dev = &pdev->dev;
> +	error = zxi2c_parse_resources(i2c);
> +	if (error)
> +		return error;
> +
> +	platform_set_drvdata(pdev, (void *)i2c);
> +
> +	if (devm_request_irq(&pdev->dev, i2c->irq, zxi2c_irq_handle,
> +			     IRQF_SHARED, pdev->name, i2c)) {
> +		dev_err(i2c->dev, "IRQ%d request failed\n", i2c->irq);
> +		return -ENODEV;

dev_err_probe()

> +	}
> +
> +	init_waitqueue_head(&i2c->waitq);
> +
> +	i2c->adap.owner = THIS_MODULE;
> +	i2c->adap.algo = &zxi2c_algorithm;
> +	i2c->adap.retries = 2;
> +	i2c->adap.quirks = &zxi2c_quirks;
> +	i2c->adap.dev.parent = &pdev->dev;
> +	ACPI_COMPANION_SET(&i2c->adap.dev, ACPI_COMPANION(&pdev->dev));
> +	snprintf(i2c->adap.name, sizeof(i2c->adap.name),
> +		 "%s-%s", dev_name(&pci->dev), dev_name(i2c->dev));
> +	i2c_set_adapdata(&i2c->adap, i2c);
> +
> +	error = i2c_add_adapter(&i2c->adap);
> +	if (unlikely(error)) {
> +		dev_err(i2c->dev, "failed to register, %d\n", error);
> +		return error;

dev_err_probe()

Andi

> +	}
> +
> +	dev_info(i2c->dev, "adapter /dev/i2c-%d registered. hw version %x\n",
> +		 i2c->adap.nr, i2c->hrv);
> +
> +	return 0;
> +}