Re: [PATCH v3 3/7] i2c: Add Nuvoton NCT6694 I2C support
From: Andi Shyti <andi.shyti@kernel.org>
Date: 2024-12-26 00:43:16
Also in:
linux-can, linux-gpio, linux-hwmon, linux-i2c, linux-rtc, linux-watchdog, lkml
Hi Ming,
+// SPDX-License-Identifier: GPL-2.0 +/* + * Nuvoton NCT6694 I2C adapter driver based on USB interface. + * + * Copyright (C) 2024 Nuvoton Technology Corp. + */ + +#include <linux/i2c.h> +#include <linux/kernel.h> +#include <linux/mfd/core.h> +#include <linux/mfd/nct6694.h> +#include <linux/module.h> +#include <linux/platform_device.h> + +/* Host interface */
What does it mean "Host interface"?
+#define NCT6694_I2C_MOD 0x03 + +/* Message Channel*/ +/* Command 00h */
This comments are meaningless, either make them clearer or remove them.
+#define NCT6694_I2C_CMD0_OFFSET 0x0000 /* OFFSET = SEL|CMD */
I find this comment quite meaningless. Can you please make it clearer?
+#define NCT6694_I2C_CMD0_LEN 0x90
+
+enum i2c_baudrate {
+ I2C_BR_25K = 0,
+ I2C_BR_50K,
+ I2C_BR_100K,
+ I2C_BR_200K,
+ I2C_BR_400K,
+ I2C_BR_800K,
+ I2C_BR_1M
+};
+
+struct __packed nct6694_i2c_cmd0 {
+ u8 port;
+ u8 br;
+ u8 addr;
+ u8 w_cnt;
+ u8 r_cnt;
+ u8 rsv[11];
+ u8 write_data[0x40];
+ u8 read_data[0x40];
+};
+
+struct nct6694_i2c_data {
+ struct nct6694 *nct6694;
+ struct i2c_adapter adapter;
+ unsigned char *xmit_buf;why isn't this a nct6694_i2c_cmd0 type?
+ unsigned char port;
+ unsigned char br;
+};
+
+static int nct6694_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs, int num)
+{
+ struct nct6694_i2c_data *data = adap->algo_data;
+ struct nct6694_i2c_cmd0 *cmd = (struct nct6694_i2c_cmd0 *)data->xmit_buf;
+ int ret, i;
+
+ for (i = 0; i < num ; i++) {
+ struct i2c_msg *msg_temp = &msgs[i];
+
+ memset(data->xmit_buf, 0, sizeof(struct nct6694_i2c_cmd0));
+
+ if (msg_temp->len > 64)
+ return -EPROTO;
+ cmd->port = data->port;
+ cmd->br = data->br;
+ cmd->addr = i2c_8bit_addr_from_msg(msg_temp);
+ if (msg_temp->flags & I2C_M_RD) {
+ cmd->r_cnt = msg_temp->len;
+ ret = nct6694_write_msg(data->nct6694, NCT6694_I2C_MOD,
+ NCT6694_I2C_CMD0_OFFSET,
+ NCT6694_I2C_CMD0_LEN,
+ cmd);
+ if (ret < 0)
+ return 0;why not return ret?
+
+ memcpy(msg_temp->buf, cmd->read_data, msg_temp->len);
+ } else {
+ cmd->w_cnt = msg_temp->len;
+ memcpy(cmd->write_data, msg_temp->buf, msg_temp->len);
+ ret = nct6694_write_msg(data->nct6694, NCT6694_I2C_MOD,
+ NCT6694_I2C_CMD0_OFFSET,
+ NCT6694_I2C_CMD0_LEN,
+ cmd);
+ if (ret < 0)
+ return 0;
+ }
+ }
+
+ return num;
+}
+
+static u32 nct6694_func(struct i2c_adapter *adapter)
+{
+ return (I2C_FUNC_I2C | I2C_FUNC_SMBUS_EMUL);parenthesis are not needed.
+}
...
+static struct platform_driver nct6694_i2c_driver = {
+ .driver = {
+ .name = "nct6694-i2c",
+ },
+ .probe = nct6694_i2c_probe,
+ .remove = nct6694_i2c_remove,
+};
+
+module_platform_driver(nct6694_i2c_driver);what I meant in v1 is to try using module_auxiliary_driver(). Check, e.g., i2c-ljca.c or i2c-keba.c. Andi
+MODULE_DESCRIPTION("USB-I2C adapter driver for NCT6694");
+MODULE_AUTHOR("Ming Yu [off-list ref]");
+MODULE_LICENSE("GPL");
+MODULE_ALIAS("platform:nct6694-i2c");
--
2.34.1