Hi Gerhard,
...
> + ret = readb_poll_timeout(ki2c->base + KI2C_STATUS_REG,
> + sts, (sts & KI2C_STATUS_IN_USE) == 0,
> + KI2C_INUSE_SLEEP_US, KI2C_INUSE_TIMEOUT_US);
> + if (ret != 0)
if (ret)
> + dev_warn(&ki2c->auxdev->auxdev.dev, "%s err!\n", __func__);
...
> +/*
> + * Resetting bus bitwise is done by checking SDA and applying clock cycles as
> + * long as SDA is low. 9 clock cycles are applied at most.
> + *
> + * Clock cycles are generated and ndelay() determines the duration of clock
> + * cycles. Generated clock rate is 100 KHz and so duration of both clock levels
> + * is: delay in ns = (10^6 / 100) / 2
> + */
> +#define KI2C_RECOVERY_CLK_CNT 9
we can have
#define KI2C_RECOVERY_CLK_CNT 9 * 2
and...
> +#define KI2C_RECOVERY_NDELAY 5000
... use udelay()
#define KI2C_RECOVERY_UDELAY 5
I think it's a bit easier to read later.
> +static int ki2c_reset_bus_bitwise(struct ki2c *ki2c)
> +{
> + int count = 0;
> + int val = 1;
> + int ret = 0;
> +
> + /* disable I2C controller (MEN = 0) to get direct access to SCL/SDA */
> + iowrite8(0, ki2c->base + KI2C_CONTROL_REG);
> +
> + /* generate clock cycles */
> + ki2c_set_scl(ki2c, val);
> + ndelay(KI2C_RECOVERY_NDELAY);
> + while (count++ < KI2C_RECOVERY_CLK_CNT * 2) {
incrementing inside a while? sounds like a for :-)
for (count = 0; count++ < KI2C_RECOVERY_CLK_CNT * 2; count++)
> + if (val) {
...
> +static int ki2c_repstart_addr(struct ki2c *ki2c, struct i2c_msg *m)
> +{
> + int ret;
> +
> + /* repeated start and write is not supported */
> + if ((m->flags & I2C_M_RD) == 0) {
> + dev_warn(&ki2c->auxdev->auxdev.dev,
you are returning an error but printing a warning. Should the
print level be aligned with the returning behaviour? Otherwise,
if this has a debugging purpose, just use dev_dbg().
> + "Repeated start not supported for writes\n");
> + return -EINVAL;
> + }
> +
> + /* send repeated start */
> + iowrite8(KI2C_CONTROL_MEN | KI2C_CONTROL_MSTA | KI2C_CONTROL_RSTA,
> + ki2c->base + KI2C_CONTROL_REG);
> +
> + ret = ki2c_wait_for_mcf(ki2c);
> + if (ret < 0) {
> + dev_warn(&ki2c->auxdev->auxdev.dev,
> + "%s wait for MCF err at 0x%02x!\n", __func__, m->addr);
> + return ret;
> + }
...
> + for (int i = 0; i < len; i++) {
please, do not declare inside the for(), it's strange that
checkpatch doesn't warn about this.
> + ret = ki2c_wait_for_data(ki2c);
> + if (ret < 0)
> + return ret;
> +
> + /* send tx-nack after transfer of last byte */
> + if (i == len - 2)
> + iowrite8(KI2C_CONTROL_MEN | KI2C_CONTROL_MSTA | KI2C_CONTROL_TXAK,
> + ki2c->base + KI2C_CONTROL_REG);
> +
> + /*
> + * switch to TX on last byte, so that reading DATA-register
> + * does not trigger another read transfer.
> + */
> + if (i == len - 1)
else if
> + iowrite8(KI2C_CONTROL_MEN | KI2C_CONTROL_MSTA | KI2C_CONTROL_MTX,
...
> + return (ret < 0) ? ret : num;
no need for parenthesis here.
> +}
...
> + unsigned short const addr_list[2] = { info[i].addr,
> + I2C_CLIENT_END };
> +
> + client = i2c_new_scanned_device(&ki2c->adapter, &info[i],
> + addr_list, NULL);
so this comes with a known client's list. Stupid question, have
you checked the address range is OK for this use?
> + if (!IS_ERR(client)) {
> + ki2c->client[i] = client;
> + } else if (PTR_ERR(client) != -ENODEV) {
if you set here ki2c->client_size = i, you avoid unregistering
devices that are not registered... just a micro optimization, not
a binding comment.
> + ki2c_unregister_devices(ki2c);
...
> + /* reset bus before probing I2C devices */
> + ret = ki2c_reset_bus(ki2c);
> + if (ret)
> + goto out;
We can still have the enabling and the reset at the end, I don't
seen any interaction with the hardware.
> + ret = devm_i2c_add_adapter(dev, adap);
> + if (ret) {
> + dev_err(dev, "Failed to add adapter (%d)!\n", ret);
> + goto out;
> + }
> +
> + ret = ki2c_register_devices(ki2c);
> + if (ret) {
> + dev_err(dev, "Failed to register devices (%d)!\n", ret);
> + goto out;
> + }
> +
> + return 0;
> +
> +out:
> + iowrite8(KI2C_CONTROL_DISABLE, ki2c->base + KI2C_CONTROL_REG);
> + return ret;
> +}
...
> +static const struct auxiliary_device_id ki2c_devtype_aux[] = {
> + { .name = "keba.i2c" },
> + { },
Please, remove the comma here, there has been recently a patch
from Wolfram doing it on all the i2c drivers[*]
Thanks,
Andi
[*] 11bfff4913e4 ("i2c: don't use ',' after delimiters")
