diff mbox series

[v3,3/7] i2c: Add Nuvoton NCT6694 I2C support

Message ID 20241210104524.2466586-4-tmyu0@nuvoton.com (mailing list archive)
State Not Applicable
Headers show
Series Add Nuvoton NCT6694 MFD drivers | expand

Checks

Context Check Description
netdev/series_format success Posting correctly formatted
netdev/tree_selection success Guessed tree name to be net-next, async
netdev/ynl success Generated files up to date; no warnings/errors; no diff in generated;
netdev/fixes_present success Fixes tag not required for -next series
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 0 this patch: 0
netdev/build_tools success No tools touched, skip
netdev/cc_maintainers success CCed 3 of 3 maintainers
netdev/build_clang success Errors and warnings before: 1 this patch: 1
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/deprecated_api success None detected
netdev/check_selftest success No net selftest shell script
netdev/verify_fixes success No Fixes tag
netdev/build_allmodconfig_warn success Errors and warnings before: 5 this patch: 5
netdev/checkpatch warning WARNING: From:/Signed-off-by: email address mismatch: 'From: Ming Yu <a0282524688@gmail.com>' != 'Signed-off-by: Ming Yu <tmyu0@nuvoton.com>' WARNING: line length of 81 exceeds 80 columns
netdev/build_clang_rust success No Rust files in patch. Skipping build
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/source_inline success Was 0 now: 0
netdev/contest success net-next-2024-12-11--03-01 (tests: 763)

Commit Message

Ming Yu Dec. 10, 2024, 10:45 a.m. UTC 
This driver supports I2C adapter functionality for NCT6694 MFD
device based on USB interface, each I2C controller use default
baudrate(100K).

Signed-off-by: Ming Yu <tmyu0@nuvoton.com>
---
 MAINTAINERS                      |   1 +
 drivers/i2c/busses/Kconfig       |  10 ++
 drivers/i2c/busses/Makefile      |   1 +
 drivers/i2c/busses/i2c-nct6694.c | 153 +++++++++++++++++++++++++++++++
 4 files changed, 165 insertions(+)
 create mode 100644 drivers/i2c/busses/i2c-nct6694.c

Comments

Andi Shyti Dec. 26, 2024, 12:43 a.m. UTC | #1 
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 <tmyu0@nuvoton.com>");
> +MODULE_LICENSE("GPL");
> +MODULE_ALIAS("platform:nct6694-i2c");
> -- 
> 2.34.1
>
Ming Yu Dec. 26, 2024, 2:06 a.m. UTC | #2 
Dear Andi,

Thank you for your comments,

Andi Shyti <andi.shyti@kernel.org> 於 2024年12月26日 週四 上午8:43寫道：
>
> > +#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?
>

I have already updated these structures and comments following
suggestions from other reviewers, and I plan to include the changes in
the next patch submission.

> > +#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?
>

Fix it in v4.

> > +     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?
>

Fix it in v4.

> > +
> > +                     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.
>

Fix it in v4.

> > +}
>
> ...
>
> > +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.
>

I think the NCT6694  is an MCU-based device, and the current
implementation is as an MFD driver. Are you suggesting it should
instead be implemented as an auxiliary device driver? If so, would
that mean all related drivers need to be revised accordingly?

Best regards,
Ming
Andi Shyti Dec. 26, 2024, 9:25 a.m. UTC | #3 
Hi Ming,

> > > +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.
> 
> I think the NCT6694  is an MCU-based device, and the current
> implementation is as an MFD driver. Are you suggesting it should
> instead be implemented as an auxiliary device driver? If so, would
> that mean all related drivers need to be revised accordingly?

No worries, module_platform_driver() is also fine.

Andi
diff mbox series

Patch

diff --git a/MAINTAINERS b/MAINTAINERS
index 6688c5c470b7..a190f2b08fa3 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -16546,6 +16546,7 @@  M:	Ming Yu <tmyu0@nuvoton.com>
 L:	linux-kernel@vger.kernel.org
 S:	Supported
 F:	drivers/gpio/gpio-nct6694.c
+F:	drivers/i2c/busses/i2c-nct6694.c
 F:	drivers/mfd/nct6694.c
 F:	include/linux/mfd/nct6694.h
 
diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
index 4977abcd7c46..1962cf1e71f9 100644
--- a/drivers/i2c/busses/Kconfig
+++ b/drivers/i2c/busses/Kconfig
@@ -1325,6 +1325,16 @@  config I2C_LJCA
 	  This driver can also be built as a module.  If so, the module
 	  will be called i2c-ljca.
 
+config I2C_NCT6694
+	tristate "Nuvoton NCT6694 I2C adapter support"
+	depends on MFD_NCT6694
+	help
+	  If you say yes to this option, support will be included for Nuvoton
+	  NCT6694, a USB to I2C interface.
+
+	  This driver can also be built as a module. If so, the module will
+	  be called i2c-nct6694.
+
 config I2C_CP2615
 	tristate "Silicon Labs CP2615 USB sound card and I2C adapter"
 	depends on USB
diff --git a/drivers/i2c/busses/Makefile b/drivers/i2c/busses/Makefile
index a6bcbf2febcf..6d2fd8e56569 100644
--- a/drivers/i2c/busses/Makefile
+++ b/drivers/i2c/busses/Makefile
@@ -135,6 +135,7 @@  obj-$(CONFIG_I2C_GXP)		+= i2c-gxp.o
 obj-$(CONFIG_I2C_DIOLAN_U2C)	+= i2c-diolan-u2c.o
 obj-$(CONFIG_I2C_DLN2)		+= i2c-dln2.o
 obj-$(CONFIG_I2C_LJCA)		+= i2c-ljca.o
+obj-$(CONFIG_I2C_NCT6694)	+= i2c-nct6694.o
 obj-$(CONFIG_I2C_CP2615) += i2c-cp2615.o
 obj-$(CONFIG_I2C_PARPORT)	+= i2c-parport.o
 obj-$(CONFIG_I2C_PCI1XXXX)	+= i2c-mchp-pci1xxxx.o
diff --git a/drivers/i2c/busses/i2c-nct6694.c b/drivers/i2c/busses/i2c-nct6694.c
new file mode 100644
index 000000000000..d35e1ea3521f
--- /dev/null
+++ b/drivers/i2c/busses/i2c-nct6694.c
@@ -0,0 +1,153 @@ 
+// 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 */
+#define NCT6694_I2C_MOD		0x03
+
+/* Message Channel*/
+/* Command 00h */
+#define NCT6694_I2C_CMD0_OFFSET	0x0000	/* OFFSET = SEL|CMD */
+#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;
+	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;
+
+			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);
+}
+
+static const struct i2c_algorithm algorithm = {
+	.master_xfer = nct6694_xfer,
+	.functionality = nct6694_func,
+};
+
+static int nct6694_i2c_probe(struct platform_device *pdev)
+{
+	const struct mfd_cell *cell = mfd_get_cell(pdev);
+	struct nct6694 *nct6694 = dev_get_drvdata(pdev->dev.parent);
+	struct nct6694_i2c_data *data;
+
+	data = devm_kzalloc(&pdev->dev, sizeof(*data), GFP_KERNEL);
+	if (!data)
+		return -ENOMEM;
+
+	data->xmit_buf = devm_kcalloc(&pdev->dev, NCT6694_MAX_PACKET_SZ,
+				      sizeof(unsigned char), GFP_KERNEL);
+	if (!data->xmit_buf)
+		return -ENOMEM;
+
+	data->nct6694 = nct6694;
+	data->port = cell->id;
+	data->br = I2C_BR_100K;
+
+	sprintf(data->adapter.name, "NCT6694 I2C Adapter %d", cell->id);
+	data->adapter.owner = THIS_MODULE;
+	data->adapter.algo = &algorithm;
+	data->adapter.dev.parent = &pdev->dev;
+	data->adapter.algo_data = data;
+
+	platform_set_drvdata(pdev, data);
+
+	return i2c_add_adapter(&data->adapter);
+}
+
+static void nct6694_i2c_remove(struct platform_device *pdev)
+{
+	struct nct6694_i2c_data *data = platform_get_drvdata(pdev);
+
+	i2c_del_adapter(&data->adapter);
+}
+
+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);
+
+MODULE_DESCRIPTION("USB-I2C adapter driver for NCT6694");
+MODULE_AUTHOR("Ming Yu <tmyu0@nuvoton.com>");
+MODULE_LICENSE("GPL");
+MODULE_ALIAS("platform:nct6694-i2c");