Re: [PATCH 2/3] i2c: thunderx: Add support for High speed mode

From: Andi Shyti
Date: Sat Jun 10 2023 - 08:45:44 EST


Hi Suneel and Piyush,

[...]

> @@ -61,10 +61,19 @@ static int octeon_i2c_wait(struct octeon_i2c *i2c)
> return octeon_i2c_test_iflg(i2c) ? 0 : -ETIMEDOUT;
> }
>
> - i2c->int_enable(i2c);
> - time_left = wait_event_timeout(i2c->queue, octeon_i2c_test_iflg(i2c),
> - i2c->adap.timeout);
> - i2c->int_disable(i2c);
> + if (IS_LS_FREQ(i2c->twsi_freq)) {
> + i2c->int_enable(i2c);
> + time_left = wait_event_timeout(i2c->queue,
> + octeon_i2c_test_iflg(i2c),
> + i2c->adap.timeout);
> + i2c->int_disable(i2c);
> + } else {
> + time_left = 1000; /* 1ms */
> + do {
> + if (time_left--)
> + __udelay(1);

Are you sure you want to wait 1ms with __udelay(). This is a bit
dsruptive, can we use a more relaxed waiting method?

> + } while (!octeon_i2c_test_iflg(i2c) && time_left);
> + }

[...]

> * Find divisors to produce target frequency, start with large delta
> * to cover wider range of divisors, note thp = TCLK half period.
> */
> - int thp = 0x18, mdiv = 2, ndiv = 0, delta_hz = huge_delta;
> + int ds = 10, thp = 0x18, mdiv = 2, ndiv = 0, delta_hz = huge_delta;

unsigned?

[...]

> + if (octeon_i2c_is_otx2(to_pci_dev(i2c->dev))) {
> + u64 mode;
> +
> + mode = __raw_readq(i2c->twsi_base + MODE(i2c));
> + /* Set REFCLK_SRC and HS_MODE in TWSX_MODE register */
> + if (!IS_LS_FREQ(i2c->twsi_freq))
> + mode |= BIT(4) | BIT(0);
> + else
> + mode &= ~(BIT(4) | BIT(0));

would be nice to have this defined and with some meaning as
comment.

> + octeon_i2c_writeq_flush(mode, i2c->twsi_base + MODE(i2c));
> + }

Robert, any comment here?

Thanks,
Andi