On 08.08.24 02:57, Andi Shyti wrote:
Hi Gerhard,
Hi Andi,
...
+ 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)
Done.
+ 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
Done.
and...
+#define KI2C_RECOVERY_NDELAY 5000
... use udelay()
#define KI2C_RECOVERY_UDELAY 5
I think it's a bit easier to read later.
Done.
+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++)
Done.
+ 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().
You are right, changed all dev_warn() to dev_err().
+ "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.
Done for all for loops.
+ 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
Done.
+ iowrite8(KI2C_CONTROL_MEN | KI2C_CONTROL_MSTA | KI2C_CONTROL_MTX,
...
+ return (ret < 0) ? ret : num;
no need for parenthesis here.
Done.
+}
...
+ 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?
Yes, the addresses are 0x4c (EMC1403) and 0x50 to 0x56 (EEPROMs).
All between 0x08 and 0x77. All devices are working/found.
+ 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.
Done.
+ 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.
At least ki2c_register_device() does interact with the hardware. It uses
i2c_new_scanned_device() which probes the devices.
IMHO the device shall be ready to operate when devm_i2c_add_adapter()
is called. Because the user space can trigger I2C action as soon as
devm_i2c_add_adapter() is done. So enabling and reset after
devm_i2c_add_adapter() would be a potential race against user space I2C
actions. Is that correct?
+ 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[*]
Done.
Thanks,
Andi
[*] 11bfff4913e4 ("i2c: don't use ',' after delimiters")
Thank you for your detailed review!
Gerhard
