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

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

Hi Hans,

On Wed, Jun 14, 2023 at 03:34:33PM +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 has two separate busses, so may accommodate up
> 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 v4:
>   * delete platform check in probe()
>   * move config interface under ACPI in Kconfig file
>   * add irq disable when access error
>   * fix some trivial issues
>   Link: https://lore.kernel.org/all/20230609031625.6928-1-hanshu-oc@zhaoxin.com/

Looks very good, almost ready for r-b, I have few little nitpicks
though, that are more a matter of style.

[...]

> +static const u32 zxi2c_speed_params_table[][3] = {
> +	/* speed, ZXI2C_TCR, ZXI2C_FSTP, ZXI2C_CS, ZXI2C_SCLTP */
> +	{I2C_MAX_STANDARD_MODE_FREQ, 0, ZXI2C_GOLDEN_FSTP_100K},
> +	{I2C_MAX_FAST_MODE_FREQ, ZXI2C_FAST_SEL, ZXI2C_GOLDEN_FSTP_400K},
> +	{I2C_MAX_FAST_MODE_PLUS_FREQ, ZXI2C_FAST_SEL, ZXI2C_GOLDEN_FSTP_1M},
> +	{I2C_MAX_HIGH_SPEED_MODE_FREQ, ZXI2C_HS_SEL, ZXI2C_GOLDEN_FSTP_3400K},

can you leave a space here between brackets:

{ I2C_MAX_FAST_MODE_PLUS_FREQ, ZXI2C_FAST_SEL, ZXI2C_GOLDEN_FSTP_1M },

> +};
> +
> +static void zxi2c_set_bus_speed(struct zxi2c *i2c)
> +{
> +	u8 i, count;
> +	const u32 *params = NULL;
> +
> +	count = ARRAY_SIZE(zxi2c_speed_params_table);
> +	for (i = 0; i < count; i++) {
> +		if (zxi2c_speed_params_table[i][0] == i2c->speed) {
> +			params = zxi2c_speed_params_table[i];
> +			break;
> +		}
> +	}

the brackets around the for() are not necessary

> +	iowrite8(params[1], i2c->regs + ZXI2C_TCR);
> +	if (abs(i2c->fstp - params[2]) > 0x10) {
> +		/* if BIOS setting value far from golden value,
> +		 * use golden value and warn user
> +		 */

the comment should be

	/*
	 * 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 + ZXI2C_FSTP);
> +	} else {
> +		iowrite8(i2c->fstp, i2c->regs + ZXI2C_FSTP);
> +	}

[...]

> +	if (num == 1 && msgs->len <= ZXI2C_FIFO_SIZE && msgs->len >= 3)
> +		err = zxi2c_fifo_xfer(i2c, msgs);
> +	else
> +		err = zxi2c_byte_xfer(i2c, msgs, num);
> +
> +	zxi2c_enable_irq(i2c->regs, ZXI2C_EN_FIFOEND | ZXI2C_EN_BYTEEND, false);

can you add a comment here to explain that interrupts have been
enabled inside the xfer functions? Otherwise someone might wonder
why this line.

> +	return err;
> +}

[...]

> +static const struct acpi_device_id zxi2c_acpi_match[] = {
> +	{"IIC1D17", 0},
> +	{},

the comma is not needed and please leave a space

	{ "IIC1D17", 0 },
	{ }

With the little things above, you can add:

Reviewed-by: Andi Shyti <andi.shyti@kernel.org> 

Thank you!
Andi

> +};
> +MODULE_DEVICE_TABLE(acpi, zxi2c_acpi_match);