On 04.07.24 00:09, Andi Shyti wrote:
Hi Gerhard,
Hi Andi,
...
+static int ki2c_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[], int num)
+{
+ struct ki2c *ki2c = i2c_get_adapdata(adap);
+ int ret;
+
+ ret = ki2c_inuse_lock(ki2c);
+ if (ret < 0)
+ return ret;
+
+ for (int i = 0; i < num; i++) {
+ struct i2c_msg *m = &msgs[i];
+
+ if (i == 0)
+ ret = ki2c_start_addr(ki2c, m);
do we want to keep the error if we fail to receive the data ack?
Yes that is intended. Is there something wrong with forwarding the
error?
+ else
+ ret = ki2c_repstart_addr(ki2c, m);
+ if (ret < 0)
+ break;
+
+ if (m->flags & I2C_M_RD)
+ ret = ki2c_read(ki2c, m->buf, m->len);
+ else
+ ret = ki2c_write(ki2c, m->buf, m->len);
+ if (ret < 0)
+ break;
+ }
+
+ ki2c_stop(ki2c);
+
+ ki2c_inuse_unlock(ki2c);
+
+ return (ret < 0) ? ret : num;
+}
+
+static void ki2c_unregister_devices(struct ki2c *ki2c)
+{
+ for (int i = 0; i < ki2c->client_size; i++) {
+ struct i2c_client *client = ki2c->client[i];
+
+ if (client)
+ i2c_unregister_device(client);
+ }
+}
+
+static int ki2c_register_devices(struct ki2c *ki2c)
+{
+ struct i2c_board_info *info = ki2c->auxdev->info;
+
+ /* register all I2C devices from platform_data array */
+ for (int i = 0; i < ki2c->client_size; i++) {
Please declare the variable at the beginning of the function.
Did you run checkpatch.pl before sending?
Yes, multiple times, but it did not trigger. I will fix that
declaration.
+ struct i2c_client *client;
+ unsigned short const addr_list[2] = { info[i].addr,
+ I2C_CLIENT_END };
+
+ client = i2c_new_scanned_device(&ki2c->adapter, &info[i],
+ addr_list, NULL);
+ if (!IS_ERR(client)) {
+ ki2c->client[i] = client;
+ } else if (PTR_ERR(client) != -ENODEV) {
+ ki2c_unregister_devices(ki2c);
+
+ return PTR_ERR(client);
+ }
+ }
+
+ return 0;
+}
+
+static u32 ki2c_func(struct i2c_adapter *adap)
+{
+ return I2C_FUNC_I2C | I2C_FUNC_SMBUS_EMUL;
+}
+
+static const struct i2c_algorithm ki2c_algo = {
+ .master_xfer = ki2c_xfer,
+ .functionality = ki2c_func,
+};
+
+static int ki2c_probe(struct auxiliary_device *auxdev,
+ const struct auxiliary_device_id *id)
+{
+ struct device *dev = &auxdev->dev;
+ struct i2c_adapter *adap;
+ struct ki2c *ki2c;
+ int ret;
+
+ ki2c = devm_kzalloc(dev, sizeof(*ki2c), GFP_KERNEL);
+ if (!ki2c)
+ return -ENOMEM;
+ ki2c->auxdev = container_of(auxdev, struct keba_i2c_auxdev, auxdev);
+ ki2c->client = devm_kcalloc(dev, ki2c->auxdev->info_size,
+ sizeof(*ki2c->client), GFP_KERNEL);
+ if (!ki2c->client)
+ return -ENOMEM;
+ ki2c->client_size = ki2c->auxdev->info_size;
+ auxiliary_set_drvdata(auxdev, ki2c);
+
+ ki2c->base = devm_ioremap_resource(dev, &ki2c->auxdev->io);
+ if (IS_ERR(ki2c->base))
+ return PTR_ERR(ki2c->base);
+
+ /* enable controller */
+ iowrite8(KI2C_CONTROL_MEN, ki2c->base + KI2C_CONTROL_REG);
Can we enable at the end? What's the point for enabling the
device so early? (We also save a few goto's)
You are right, I will enable it as late as possible.
+ adap = &ki2c->adapter;
+ strscpy(adap->name, "KEBA I2C adapter", sizeof(adap->name));
+ adap->owner = THIS_MODULE;
+ adap->class = I2C_CLASS_HWMON;
+ adap->algo = &ki2c_algo;
+ adap->dev.parent = dev;
+
+ i2c_set_adapdata(adap, ki2c);
+
+ /* reset bus before probing I2C devices */
+ ret = ki2c_reset_bus(ki2c);
+ if (ret)
+ goto out_disable;
+
+ ret = i2c_add_adapter(adap);
Please use devm_i2c_add_adapter()
Another devm_ function I was not aware of. I will use it. Thanks
for the advice!
+ if (ret) {
+ dev_err(dev, "Failed to add adapter (%d)!\n", ret);
+ goto out_disable;
+ }
+
+ ret = ki2c_register_devices(ki2c);
+ if (ret) {
+ dev_err(dev, "Failed to register devices (%d)!\n", ret);
+ goto out_delete;
+ }
+
+ return 0;
+
+out_delete:
+ i2c_del_adapter(adap);
+out_disable:
+ iowrite8(0, ki2c->base + KI2C_CONTROL_REG);
Can we define Value '0'? KI2C_CONTROL_DISABLE, perhaps?
Will be done.
Shall both patches go over i2c-host-next or over char-misc?
Thank you for the review!
Gerhard
+ return ret;
+}
+
+static void ki2c_remove(struct auxiliary_device *auxdev)
+{
+ struct ki2c *ki2c = auxiliary_get_drvdata(auxdev);
+
+ ki2c_unregister_devices(ki2c);
+
+ i2c_del_adapter(&ki2c->adapter);
+
+ /* disable controller */
+ iowrite8(0, ki2c->base + KI2C_CONTROL_REG);
+
+ auxiliary_set_drvdata(auxdev, NULL);
+}
+
+static const struct auxiliary_device_id ki2c_devtype_aux[] = {
+ { .name = "keba.i2c" },
+ { },
+};
+MODULE_DEVICE_TABLE(auxiliary, ki2c_devtype_aux);
+
+static struct auxiliary_driver ki2c_driver_aux = {
+ .name = KI2C,
+ .id_table = ki2c_devtype_aux,
+ .probe = ki2c_probe,
+ .remove = ki2c_remove,
+};
+module_auxiliary_driver(ki2c_driver_aux);
+
+MODULE_AUTHOR("Gerhard Engleder <eg@xxxxxxxx>");
+MODULE_DESCRIPTION("KEBA I2C bus controller driver");
+MODULE_LICENSE("GPL");
--
2.39.2
