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@xxxxxxxxxxx/ 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@xxxxxxxxxx> Thank you! Andi > +}; > +MODULE_DEVICE_TABLE(acpi, zxi2c_acpi_match);
