[RFC 0/5] Introduce i3c device userspace interface

[RFC 0/5] Introduce i3c device userspace interface

[Vitor Soares]

For today there is no way to use i3c devices from user space and
the introduction of such API will help developers during the i3c device
or i3c host controllers development.

The i3cdev module is highly based on i2c-dev and yet I tried to address
the concerns raised in [1].

NOTES:
- The i3cdev dynamically request an unused major number.

- The i3c devices are dynamically exposed/removed from dev/ folder based
  on if they have a device driver bound to it.

- For now, the module exposes i3c devices without device driver on
  dev/i3c-<bus>-<pid>, but we can change the path to
  dev/bus/i3c/<bus>-<pid> or dev/i3c/<bus>-<pid>.

- As in the i2c subsystem, here it is exposed the i3c_priv_xfer to
  userspace. I tried to use a dedicated structure as in spidev but I don't
  see any obvious advantage.

- Since the i3c API only exposes i3c_priv_xfer to devices, for now, the
  module just makes use of one ioctl(). This can change in the future with
  the introduction hdr commands or by the need of exposing some CCC
  commands to the device API (private contract between master-slave).
  Regarding the i3c device info, some information is already available
  through sysfs. We can add more device attributes to expose more
  information or add a dedicated ioctl() request for that purpose or both.

- Similar to i2c, I have also created a tool that you can find in [2]
  for testing purposes. If you have some time available I would appreciate
  your feedback about it as well.

[1] https://lkml.org/lkml/2018/11/15/853
[2] https://github.com/vitor-soares-snps/i3c-tools.git

Vitor Soares (5):
  i3c: master: export i3c_masterdev_type
  i3c: master: export i3c_bus_type symbol
  i3c: device: expose transfer strutures to uapi
  i3c: master: add i3c_for_each_dev helper
  i3c: add i3cdev module to expose i3c dev in /dev

 drivers/i3c/Kconfig             |  15 ++
 drivers/i3c/Makefile            |   1 +
 drivers/i3c/i3cdev.c            | 438 ++++++++++++++++++++++++++++++++++++++++
 drivers/i3c/internals.h         |   2 +
 drivers/i3c/master.c            |  16 +-
 include/linux/i3c/device.h      |  54 +----
 include/uapi/linux/i3c/device.h |  66 ++++++
 include/uapi/linux/i3c/i3cdev.h |  27 +++
 8 files changed, 565 insertions(+), 54 deletions(-)
 create mode 100644 drivers/i3c/i3cdev.c
 create mode 100644 include/uapi/linux/i3c/device.h
 create mode 100644 include/uapi/linux/i3c/i3cdev.h

-- 
2.7.4


_______________________________________________
linux-i3c mailing list
linux-i3c@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-i3c

[RFC 1/5] i3c: master: export i3c_masterdev_type

[Vitor Soares]

Export i3c_masterdev_type symbol so i3cdev module can verify if an
i3c device is a master device.

Signed-off-by: Vitor Soares <vitor.soares@synopsys.com>
---
 drivers/i3c/internals.h | 1 +
 drivers/i3c/master.c    | 3 ++-
 2 files changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/i3c/internals.h b/drivers/i3c/internals.h
index 86b7b44..bc062e8 100644
--- a/drivers/i3c/internals.h
+++ b/drivers/i3c/internals.h
@@ -11,6 +11,7 @@
 #include <linux/i3c/master.h>
 
 extern struct bus_type i3c_bus_type;
+extern const struct device_type i3c_masterdev_type;
 
 void i3c_bus_normaluse_lock(struct i3c_bus *bus);
 void i3c_bus_normaluse_unlock(struct i3c_bus *bus);
diff --git a/drivers/i3c/master.c b/drivers/i3c/master.c
index 0436916..a1fb5f7 100644
--- a/drivers/i3c/master.c
+++ b/drivers/i3c/master.c
@@ -523,9 +523,10 @@ static void i3c_masterdev_release(struct device *dev)
 	of_node_put(dev->of_node);
 }
 
-static const struct device_type i3c_masterdev_type = {
+const struct device_type i3c_masterdev_type = {
 	.groups	= i3c_masterdev_groups,
 };
+EXPORT_SYMBOL_GPL(i3c_masterdev_type);
 
 int i3c_bus_set_mode(struct i3c_bus *i3cbus, enum i3c_bus_mode mode,
 		     unsigned long max_i2c_scl_rate)
-- 
2.7.4


_______________________________________________
linux-i3c mailing list
linux-i3c@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-i3c

[RFC 2/5] i3c: master: export i3c_bus_type symbol

[Vitor Soares]

Export i3c_bus_type symbol so i3cdev can register a notifier chain
for the i3c bus.

Signed-off-by: Vitor Soares <vitor.soares@synopsys.com>
---
 drivers/i3c/master.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/i3c/master.c b/drivers/i3c/master.c
index a1fb5f7..a9df276 100644
--- a/drivers/i3c/master.c
+++ b/drivers/i3c/master.c
@@ -321,6 +321,7 @@ struct bus_type i3c_bus_type = {
 	.probe = i3c_device_probe,
 	.remove = i3c_device_remove,
 };
+EXPORT_SYMBOL_GPL(i3c_bus_type);
 
 static enum i3c_addr_slot_status
 i3c_bus_get_addr_slot_status(struct i3c_bus *bus, u16 addr)
-- 
2.7.4


_______________________________________________
linux-i3c mailing list
linux-i3c@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-i3c

[RFC 3/5] i3c: device: expose transfer strutures to uapi

[Vitor Soares]

Move i3c transfer related structures to uapi, so they can be access
by userspace.

Signed-off-by: Vitor Soares <vitor.soares@synopsys.com>
---
 include/linux/i3c/device.h      | 54 +--------------------------------
 include/uapi/linux/i3c/device.h | 66 +++++++++++++++++++++++++++++++++++++++++
 2 files changed, 67 insertions(+), 53 deletions(-)
 create mode 100644 include/uapi/linux/i3c/device.h

diff --git a/include/linux/i3c/device.h b/include/linux/i3c/device.h
index de102e4..1df05c2 100644
--- a/include/linux/i3c/device.h
+++ b/include/linux/i3c/device.h
@@ -14,59 +14,7 @@
 #include <linux/kconfig.h>
 #include <linux/mod_devicetable.h>
 #include <linux/module.h>
-
-/**
- * enum i3c_error_code - I3C error codes
- *
- * These are the standard error codes as defined by the I3C specification.
- * When -EIO is returned by the i3c_device_do_priv_xfers() or
- * i3c_device_send_hdr_cmds() one can check the error code in
- * &struct_i3c_priv_xfer.err or &struct i3c_hdr_cmd.err to get a better idea of
- * what went wrong.
- *
- * @I3C_ERROR_UNKNOWN: unknown error, usually means the error is not I3C
- *		       related
- * @I3C_ERROR_M0: M0 error
- * @I3C_ERROR_M1: M1 error
- * @I3C_ERROR_M2: M2 error
- */
-enum i3c_error_code {
-	I3C_ERROR_UNKNOWN = 0,
-	I3C_ERROR_M0 = 1,
-	I3C_ERROR_M1,
-	I3C_ERROR_M2,
-};
-
-/**
- * enum i3c_hdr_mode - HDR mode ids
- * @I3C_HDR_DDR: DDR mode
- * @I3C_HDR_TSP: TSP mode
- * @I3C_HDR_TSL: TSL mode
- */
-enum i3c_hdr_mode {
-	I3C_HDR_DDR,
-	I3C_HDR_TSP,
-	I3C_HDR_TSL,
-};
-
-/**
- * struct i3c_priv_xfer - I3C SDR private transfer
- * @rnw: encodes the transfer direction. true for a read, false for a write
- * @len: transfer length in bytes of the transfer
- * @data: input/output buffer
- * @data.in: input buffer. Must point to a DMA-able buffer
- * @data.out: output buffer. Must point to a DMA-able buffer
- * @err: I3C error code
- */
-struct i3c_priv_xfer {
-	u8 rnw;
-	u16 len;
-	union {
-		void *in;
-		const void *out;
-	} data;
-	enum i3c_error_code err;
-};
+#include <uapi/linux/i3c/device.h>
 
 /**
  * enum i3c_dcr - I3C DCR values
diff --git a/include/uapi/linux/i3c/device.h b/include/uapi/linux/i3c/device.h
new file mode 100644
index 0000000..edbb14d
--- /dev/null
+++ b/include/uapi/linux/i3c/device.h
@@ -0,0 +1,66 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * Copyright (c) 2019 Synopsys, Inc. and/or its affiliates.
+ *
+ * Author: Vitor Soares <vitor.soares@synopsys.com>
+ */
+
+#ifndef _UAPI_LINUX_I3C_DEVICE_H
+#define _UAPI_LINUX_I3C_DEVICE_H
+
+#include <linux/types.h>
+
+/**
+ * enum i3c_error_code - I3C error codes
+ *
+ * These are the standard error codes as defined by the I3C specification.
+ * When -EIO is returned by the i3c_device_do_priv_xfers() or
+ * i3c_device_send_hdr_cmds() one can check the error code in
+ * &struct_i3c_priv_xfer.err or &struct i3c_hdr_cmd.err to get a better idea of
+ * what went wrong.
+ *
+ * @I3C_ERROR_UNKNOWN: unknown error, usually means the error is not I3C
+ *		       related
+ * @I3C_ERROR_M0: M0 error
+ * @I3C_ERROR_M1: M1 error
+ * @I3C_ERROR_M2: M2 error
+ */
+enum i3c_error_code {
+	I3C_ERROR_UNKNOWN = 0,
+	I3C_ERROR_M0 = 1,
+	I3C_ERROR_M1,
+	I3C_ERROR_M2,
+};
+
+/**
+ * enum i3c_hdr_mode - HDR mode ids
+ * @I3C_HDR_DDR: DDR mode
+ * @I3C_HDR_TSP: TSP mode
+ * @I3C_HDR_TSL: TSL mode
+ */
+enum i3c_hdr_mode {
+	I3C_HDR_DDR,
+	I3C_HDR_TSP,
+	I3C_HDR_TSL,
+};
+
+/**
+ * struct i3c_priv_xfer - I3C SDR private transfer
+ * @rnw: encodes the transfer direction. true for a read, false for a write
+ * @len: transfer length in bytes of the transfer
+ * @data: input/output buffer
+ * @data.in: input buffer. Must point to a DMA-able buffer
+ * @data.out: output buffer. Must point to a DMA-able buffer
+ * @err: I3C error code
+ */
+struct i3c_priv_xfer {
+	u8 rnw;
+	u16 len;
+	union {
+		void *in;
+		const void *out;
+	} data;
+	enum i3c_error_code err;
+};
+
+#endif /* _UAPI_LINUX_I3C_DEVICE_H */
-- 
2.7.4


_______________________________________________
linux-i3c mailing list
linux-i3c@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-i3c

[RFC 4/5] i3c: master: add i3c_for_each_dev helper

[Vitor Soares]

Introduce i3c_for_each_dev(), an i3c device iterator for use by i3cdev.

Signed-off-by: Vitor Soares <vitor.soares@synopsys.com>
---
 drivers/i3c/internals.h |  1 +
 drivers/i3c/master.c    | 12 ++++++++++++
 2 files changed, 13 insertions(+)

diff --git a/drivers/i3c/internals.h b/drivers/i3c/internals.h
index bc062e8..a6deedf 100644
--- a/drivers/i3c/internals.h
+++ b/drivers/i3c/internals.h
@@ -24,4 +24,5 @@ int i3c_dev_enable_ibi_locked(struct i3c_dev_desc *dev);
 int i3c_dev_request_ibi_locked(struct i3c_dev_desc *dev,
 			       const struct i3c_ibi_setup *req);
 void i3c_dev_free_ibi_locked(struct i3c_dev_desc *dev);
+int i3c_for_each_dev(void *data, int (*fn)(struct device *, void *));
 #endif /* I3C_INTERNAL_H */
diff --git a/drivers/i3c/master.c b/drivers/i3c/master.c
index a9df276..cc41efe 100644
--- a/drivers/i3c/master.c
+++ b/drivers/i3c/master.c
@@ -2640,6 +2640,18 @@ void i3c_dev_free_ibi_locked(struct i3c_dev_desc *dev)
 	dev->ibi = NULL;
 }
 
+int i3c_for_each_dev(void *data, int (*fn)(struct device *, void *))
+{
+	int res;
+
+	mutex_lock(&i3c_core_lock);
+	res = bus_for_each_dev(&i3c_bus_type, NULL, data, fn);
+	mutex_unlock(&i3c_core_lock);
+
+	return res;
+}
+EXPORT_SYMBOL_GPL(i3c_for_each_dev);
+
 static int __init i3c_init(void)
 {
 	return bus_register(&i3c_bus_type);
-- 
2.7.4


_______________________________________________
linux-i3c mailing list
linux-i3c@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-i3c

[RFC 5/5] i3c: add i3cdev module to expose i3c dev in /dev

[Vitor Soares]

This patch adds user-mode support to I3C SDR transfers.

The module is based on i2c-dev.c with the following features:
  - expose on /dev the i3c devices dynamically based on if they have
    a device driver bound.
  - Dynamically allocate the char device Major number.

Signed-off-by: Vitor Soares <vitor.soares@synopsys.com>
---
 drivers/i3c/Kconfig             |  15 ++
 drivers/i3c/Makefile            |   1 +
 drivers/i3c/i3cdev.c            | 438 ++++++++++++++++++++++++++++++++++++++++
 include/uapi/linux/i3c/i3cdev.h |  27 +++
 4 files changed, 481 insertions(+)
 create mode 100644 drivers/i3c/i3cdev.c
 create mode 100644 include/uapi/linux/i3c/i3cdev.h

diff --git a/drivers/i3c/Kconfig b/drivers/i3c/Kconfig
index 30a4415..0164276 100644
--- a/drivers/i3c/Kconfig
+++ b/drivers/i3c/Kconfig
@@ -20,5 +20,20 @@ menuconfig I3C
 	  will be called i3c.
 
 if I3C
+
+config I3CDEV
+	tristate "I3C device interface"
+	depends on I3C
+	help
+	  Say Y here to use i3c-* device files, usually found in the /dev
+	  directory on your system.  They make it possible to have user-space
+	  programs use the I3C devices.
+
+	  This support is also available as a module.  If so, the module
+	  will be called i3cdev.
+
+	  Note that this application programming interface is EXPERIMENTAL
+	  and hence SUBJECT TO CHANGE WITHOUT NOTICE while it stabilizes.
+
 source "drivers/i3c/master/Kconfig"
 endif # I3C
diff --git a/drivers/i3c/Makefile b/drivers/i3c/Makefile
index 11982ef..606d422 100644
--- a/drivers/i3c/Makefile
+++ b/drivers/i3c/Makefile
@@ -1,4 +1,5 @@
 # SPDX-License-Identifier: GPL-2.0
 i3c-y				:= device.o master.o
 obj-$(CONFIG_I3C)		+= i3c.o
+obj-$(CONFIG_I3CDEV)		+= i3cdev.o
 obj-$(CONFIG_I3C)		+= master/
diff --git a/drivers/i3c/i3cdev.c b/drivers/i3c/i3cdev.c
new file mode 100644
index 0000000..4d4b83c
--- /dev/null
+++ b/drivers/i3c/i3cdev.c
@@ -0,0 +1,438 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (c) 2019 Synopsys, Inc. and/or its affiliates.
+ *
+ * Author: Vitor Soares <soares@synopsys.com>
+ */
+
+#include <linux/cdev.h>
+#include <linux/compat.h>
+#include <linux/device.h>
+#include <linux/fs.h>
+#include <linux/init.h>
+#include <linux/jiffies.h>
+#include <linux/kernel.h>
+#include <linux/list.h>
+#include <linux/module.h>
+#include <linux/notifier.h>
+#include <linux/slab.h>
+#include <linux/uaccess.h>
+
+#include <linux/i3c/i3cdev.h>
+
+#include "internals.h"
+
+#define I3C_MINORS	MINORMASK
+#define N_I3C_MINORS	16 /* For now */
+
+static DECLARE_BITMAP(minors, N_I3C_MINORS);
+
+struct i3cdev_data {
+	struct list_head list;
+	struct i3c_device *i3c;
+	struct cdev cdev;
+	struct device *dev;
+	dev_t devt;
+};
+
+static dev_t i3cdev_number; /* Alloted device number */
+
+static LIST_HEAD(i3cdev_list);
+static DEFINE_SPINLOCK(i3cdev_list_lock);
+
+static struct i3cdev_data *i3cdev_get_by_minor(unsigned int minor)
+{
+	struct i3cdev_data *i3cdev;
+
+	spin_lock(&i3cdev_list_lock);
+	list_for_each_entry(i3cdev, &i3cdev_list, list) {
+		if (MINOR(i3cdev->devt) == minor)
+			goto found;
+	}
+
+	i3cdev = NULL;
+
+found:
+	spin_unlock(&i3cdev_list_lock);
+	return i3cdev;
+}
+
+static struct i3cdev_data *i3cdev_get_by_i3c(struct i3c_device *i3c)
+{
+	struct i3cdev_data *i3cdev;
+
+	spin_lock(&i3cdev_list_lock);
+	list_for_each_entry(i3cdev, &i3cdev_list, list) {
+		if (i3cdev->i3c == i3c)
+			goto found;
+	}
+
+	i3cdev = NULL;
+
+found:
+	spin_unlock(&i3cdev_list_lock);
+	return i3cdev;
+}
+
+static struct i3cdev_data *get_free_i3cdev(struct i3c_device *i3c)
+{
+	struct i3cdev_data *i3cdev;
+	unsigned long minor;
+
+	minor = find_first_zero_bit(minors, N_I3C_MINORS);
+	if (minor >= N_I3C_MINORS) {
+		pr_err("i3cdev: no minor number available!\n");
+		return ERR_PTR(-ENODEV);
+	}
+
+	i3cdev = kzalloc(sizeof(*i3cdev), GFP_KERNEL);
+	if (!i3cdev)
+		return ERR_PTR(-ENOMEM);
+
+	i3cdev->i3c = i3c;
+	i3cdev->devt = MKDEV(MAJOR(i3cdev_number), minor);
+	set_bit(minor, minors);
+
+	spin_lock(&i3cdev_list_lock);
+	list_add_tail(&i3cdev->list, &i3cdev_list);
+	spin_unlock(&i3cdev_list_lock);
+
+	return i3cdev;
+}
+
+static void put_i3cdev(struct i3cdev_data *i3cdev)
+{
+	spin_lock(&i3cdev_list_lock);
+	list_del(&i3cdev->list);
+	spin_unlock(&i3cdev_list_lock);
+	kfree(i3cdev);
+}
+
+static ssize_t
+i3cdev_read(struct file *file, char __user *buf, size_t count, loff_t *f_pos)
+{
+	struct i3c_device *i3c = file->private_data;
+	struct i3c_priv_xfer xfers = {
+		.rnw = true,
+		.len = count,
+	};
+	char *tmp;
+	int ret;
+
+	tmp = kzalloc(count, GFP_KERNEL);
+	if (!tmp)
+		return -ENOMEM;
+
+	xfers.data.in = tmp;
+
+	dev_dbg(&i3c->dev, "Reading %zu bytes.\n", count);
+
+	ret = i3c_device_do_priv_xfers(i3c, &xfers, 1);
+	if (!ret)
+		ret = copy_to_user(buf, tmp, count) ? -EFAULT : ret;
+
+	kfree(tmp);
+	return ret;
+}
+
+static ssize_t
+i3cdev_write(struct file *file, const char __user *buf, size_t count,
+	     loff_t *f_pos)
+{
+	struct i3c_device *i3c = file->private_data;
+	struct i3c_priv_xfer xfers = {
+		.rnw = false,
+		.len = count,
+	};
+	char *tmp;
+	int ret;
+
+	tmp = memdup_user(buf, count);
+	if (IS_ERR(tmp))
+		return PTR_ERR(tmp);
+
+	xfers.data.out = tmp;
+
+	dev_dbg(&i3c->dev, "Writing %zu bytes.\n", count);
+
+	ret = i3c_device_do_priv_xfers(i3c, &xfers, 1);
+	kfree(tmp);
+	return (!ret) ? count : ret;
+}
+
+static int
+i3cdev_do_priv_xfer(struct i3c_device *dev, struct i3c_priv_xfer *xfers,
+		    unsigned int nxfers)
+{
+	void __user **data_ptrs;
+	unsigned int i;
+	int ret = 0;
+
+	data_ptrs = kmalloc_array(nxfers, sizeof(*data_ptrs), GFP_KERNEL);
+	if (!data_ptrs)
+		return -ENOMEM;
+
+	for (i = 0; i < nxfers; i++) {
+		if (xfers[i].rnw) {
+			data_ptrs[i] = (void __user *)xfers[i].data.in;
+			xfers[i].data.in = memdup_user(data_ptrs[i],
+						       xfers[i].len);
+			if (IS_ERR(xfers[i].data.in)) {
+				ret = PTR_ERR(xfers[i].data.in);
+				break;
+			}
+		} else {
+			data_ptrs[i] = (void __user *)xfers[i].data.out;
+			xfers[i].data.out = memdup_user(data_ptrs[i],
+							xfers[i].len);
+			if (IS_ERR(xfers[i].data.out)) {
+				ret = PTR_ERR(xfers[i].data.out);
+				break;
+			}
+		}
+	}
+
+	if (ret < 0) {
+		unsigned int j;
+
+		for (j = 0; j < i; ++j) {
+			if (xfers[i].rnw)
+				kfree(xfers[i].data.in);
+			else
+				kfree(xfers[i].data.out);
+		}
+
+		kfree(data_ptrs);
+		return ret;
+	}
+
+	ret = i3c_device_do_priv_xfers(dev, xfers, nxfers);
+	while (i-- > 0) {
+		if (ret >= 0 && xfers[i].rnw) {
+			if (copy_to_user(data_ptrs[i], xfers[i].data.in,
+					 xfers[i].len))
+				ret = -EFAULT;
+		}
+
+		if (xfers[i].rnw)
+			kfree(xfers[i].data.in);
+		else
+			kfree(xfers[i].data.out);
+	}
+
+	kfree(data_ptrs);
+	return ret;
+}
+
+static int
+i3cdev_ioc_priv_xfer(struct i3c_device *i3c,
+		     struct i3c_ioc_priv_xfer __user *u_ioc_xfers)
+{
+	struct i3c_ioc_priv_xfer k_ioc_xfer;
+	struct i3c_priv_xfer *xfers;
+	int ret;
+
+	if (copy_from_user(&k_ioc_xfer, u_ioc_xfers, sizeof(k_ioc_xfer)))
+		return -EFAULT;
+
+	xfers = memdup_user(k_ioc_xfer.xfers,
+			    k_ioc_xfer.nxfers * sizeof(struct i3c_priv_xfer));
+	if (IS_ERR(xfers))
+		return PTR_ERR(xfers);
+
+	ret = i3cdev_do_priv_xfer(i3c, xfers, k_ioc_xfer.nxfers);
+	kfree(xfers);
+
+	return ret;
+}
+
+static long
+i3cdev_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
+{
+	struct i3c_device *i3c = file->private_data;
+
+	dev_dbg(&i3c->dev, "ioctl, cmd=0x%02x, arg=0x%02lx\n", cmd, arg);
+
+	if (_IOC_TYPE(cmd) != I3C_DEV_IOC_MAGIC)
+		return -ENOTTY;
+
+	if (cmd == I3C_IOC_PRIV_XFER)
+		return i3cdev_ioc_priv_xfer(i3c,
+					(struct i3c_ioc_priv_xfer __user *)arg);
+
+	return 0;
+}
+
+static int i3cdev_open(struct inode *inode, struct file *file)
+{
+	unsigned int minor = iminor(inode);
+	struct i3cdev_data *i3cdev;
+
+	i3cdev = i3cdev_get_by_minor(minor);
+	if (!i3cdev)
+		return -ENODEV;
+
+	file->private_data = i3cdev->i3c;
+
+	return 0;
+}
+
+static int i3cdev_release(struct inode *inode, struct file *file)
+{
+	file->private_data = NULL;
+
+	return 0;
+}
+
+static const struct file_operations i3cdev_fops = {
+	.owner		= THIS_MODULE,
+	.read		= i3cdev_read,
+	.write		= i3cdev_write,
+	.unlocked_ioctl	= i3cdev_ioctl,
+	.open		= i3cdev_open,
+	.release	= i3cdev_release,
+};
+
+/* ------------------------------------------------------------------------- */
+
+static struct class *i3cdev_class;
+
+static int i3cdev_attach(struct device *dev, void *dummy)
+{
+	struct i3c_device *i3c;
+	struct i3cdev_data *i3cdev;
+	int res;
+
+	if (dev->type == &i3c_masterdev_type || dev->driver)
+		return 0;
+
+	i3c = dev_to_i3cdev(dev);
+
+	/* Get a device */
+	i3cdev = get_free_i3cdev(i3c);
+	if (IS_ERR(i3cdev))
+		return PTR_ERR(i3cdev);
+
+	cdev_init(&i3cdev->cdev, &i3cdev_fops);
+	i3cdev->cdev.owner = THIS_MODULE;
+	res = cdev_add(&i3cdev->cdev, i3cdev->devt, 1);
+	if (res)
+		goto error_cdev;
+
+	/* register this i3c device with the driver core */
+	i3cdev->dev = device_create(i3cdev_class, &i3c->dev,
+				    i3cdev->devt, NULL,
+				    "i3c-%s", dev_name(&i3c->dev));
+	if (IS_ERR(i3cdev->dev)) {
+		res = PTR_ERR(i3cdev->dev);
+		goto error;
+	}
+	pr_debug("i3c-cdev: I3C device [%s] registered as minor %d\n",
+		 dev_name(&i3c->dev), MINOR(i3cdev->devt));
+	return 0;
+
+error:
+	cdev_del(&i3cdev->cdev);
+error_cdev:
+	put_i3cdev(i3cdev);
+	return res;
+}
+
+static int i3cdev_detach(struct device *dev, void *dummy)
+{
+	struct i3c_device *i3c;
+	struct i3cdev_data *i3cdev;
+
+	if (dev->type == &i3c_masterdev_type)
+		return 0;
+
+	i3c = dev_to_i3cdev(dev);
+
+	i3cdev = i3cdev_get_by_i3c(i3c);
+	if (!i3cdev)
+		return 0;
+
+	clear_bit(MINOR(i3cdev->devt), minors);
+	cdev_del(&i3cdev->cdev);
+	device_destroy(i3cdev_class, i3cdev->devt);
+	put_i3cdev(i3cdev);
+
+	pr_debug("i3c-busdev: bus [%s] unregistered\n",
+		 dev_name(&i3c->dev));
+
+	return 0;
+}
+
+static int i3cdev_notifier_call(struct notifier_block *nb,
+				unsigned long action,
+				void *data)
+{
+	struct device *dev = data;
+
+	switch (action) {
+	case BUS_NOTIFY_ADD_DEVICE:
+	case BUS_NOTIFY_UNBOUND_DRIVER:
+		return i3cdev_attach(dev, NULL);
+	case BUS_NOTIFY_DEL_DEVICE:
+	case BUS_NOTIFY_BOUND_DRIVER:
+		return i3cdev_detach(dev, NULL);
+	}
+
+	return 0;
+}
+
+static struct notifier_block i3c_notifier = {
+	.notifier_call = i3cdev_notifier_call,
+};
+
+static int __init i3cdev_init(void)
+{
+	int res;
+
+	pr_info("i3c /dev entries driver\n");
+
+	/* Dynamically request unused major number */
+	res = alloc_chrdev_region(&i3cdev_number, 0, N_I3C_MINORS, "i3c");
+	if (res)
+		goto out;
+
+	/* Create a classe to populate sysfs entries*/
+	i3cdev_class = class_create(THIS_MODULE, "i3c-dev");
+	if (IS_ERR(i3cdev_class)) {
+		res = PTR_ERR(i3cdev_class);
+		goto out_unreg_chrdev;
+	}
+
+	/* Keep track of busses which have devices to add or remove later */
+	res = bus_register_notifier(&i3c_bus_type, &i3c_notifier);
+	if (res)
+		goto out_unreg_class;
+
+	/* Bind to already existing device without driver right away */
+	i3c_for_each_dev(NULL, i3cdev_attach);
+
+	return 0;
+
+out_unreg_class:
+	class_destroy(i3cdev_class);
+out_unreg_chrdev:
+	unregister_chrdev_region(i3cdev_number, I3C_MINORS);
+out:
+	pr_err("%s: Driver Initialisation failed\n", __FILE__);
+	return res;
+}
+
+static void __exit i3cdev_exit(void)
+{
+	bus_unregister_notifier(&i3c_bus_type, &i3c_notifier);
+	i3c_for_each_dev(NULL, i3cdev_detach);
+	class_destroy(i3cdev_class);
+	unregister_chrdev_region(i3cdev_number, I3C_MINORS);
+}
+
+MODULE_AUTHOR("Vitor Soares <soares@synopsys.com>");
+MODULE_DESCRIPTION("I3C /dev entries driver");
+MODULE_LICENSE("GPL");
+
+module_init(i3cdev_init);
+module_exit(i3cdev_exit);
diff --git a/include/uapi/linux/i3c/i3cdev.h b/include/uapi/linux/i3c/i3cdev.h
new file mode 100644
index 0000000..4030043
--- /dev/null
+++ b/include/uapi/linux/i3c/i3cdev.h
@@ -0,0 +1,27 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * Copyright (c) 2019 Synopsys, Inc. and/or its affiliates.
+ *
+ * Author: Vitor Soares <vitor.soares@synopsys.com>
+ */
+
+#ifndef _UAPI_I3C_DEV_H_
+#define _UAPI_I3C_DEV_H_
+
+#include <linux/types.h>
+#include <linux/ioctl.h>
+
+/* IOCTL commands */
+#define I3C_DEV_IOC_MAGIC	0x07
+
+struct i3c_ioc_priv_xfer {
+	struct i3c_priv_xfer __user *xfers;	/* pointers to i3c_priv_xfer */
+	__u32 nxfers;				/* number of i3c_priv_xfer */
+};
+
+#define I3C_IOC_PRIV_XFER	\
+	_IOW(I3C_DEV_IOC_MAGIC, 30, struct i3c_ioc_priv_xfer)
+
+#define  I3C_IOC_PRIV_XFER_MAX_MSGS	42
+
+#endif
-- 
2.7.4


_______________________________________________
linux-i3c mailing list
linux-i3c@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-i3c

Re: [RFC 5/5] i3c: add i3cdev module to expose i3c dev in /dev

[Arnd Bergmann]

On Tue, Dec 10, 2019 at 4:37 PM Vitor Soares <Vitor.Soares@synopsys.com> wrote:
>
> +/* IOCTL commands */
> +#define I3C_DEV_IOC_MAGIC      0x07
> +
> +struct i3c_ioc_priv_xfer {
> +       struct i3c_priv_xfer __user *xfers;     /* pointers to i3c_priv_xfer */
> +       __u32 nxfers;                           /* number of i3c_priv_xfer */
> +};
> +
> +#define I3C_IOC_PRIV_XFER      \
> +       _IOW(I3C_DEV_IOC_MAGIC, 30, struct i3c_ioc_priv_xfer)
> +
> +#define  I3C_IOC_PRIV_XFER_MAX_MSGS    42

This is not a great data structure for UAPI, please see
https://git.kernel.org/pub/scm/linux/kernel/git/arnd/playground.git/tree/Documentation/core-api/ioctl.rst?h=compat-ioctl-endgame&id=927324b7900ee9b877691a8b237e272fabb21bf5

for some background. I'm planning to submit that documentation for
mainline integration soon.

     Arnd

_______________________________________________
linux-i3c mailing list
linux-i3c@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-i3c

RE: [RFC 5/5] i3c: add i3cdev module to expose i3c dev in /dev

[Vitor Soares]

Hi Arnd,

From: Arnd Bergmann <arnd@arndb.de>
Date: Tue, Dec 10, 2019 at 17:51:14

> On Tue, Dec 10, 2019 at 4:37 PM Vitor Soares <Vitor.Soares@synopsys.com> wrote:
> >
> > +/* IOCTL commands */
> > +#define I3C_DEV_IOC_MAGIC      0x07
> > +
> > +struct i3c_ioc_priv_xfer {
> > +       struct i3c_priv_xfer __user *xfers;     /* pointers to i3c_priv_xfer */
> > +       __u32 nxfers;                           /* number of i3c_priv_xfer */
> > +};
> > +
> > +#define I3C_IOC_PRIV_XFER      \
> > +       _IOW(I3C_DEV_IOC_MAGIC, 30, struct i3c_ioc_priv_xfer)
> > +
> > +#define  I3C_IOC_PRIV_XFER_MAX_MSGS    42
> 
> This is not a great data structure for UAPI, please see
> https://urldefense.proofpoint.com/v2/url?u=https-3A__git.kernel.org_pub_scm_linux_kernel_git_arnd_playground.git_tree_Documentation_core-2Dapi_ioctl.rst-3Fh-3Dcompat-2Dioctl-2Dendgame-26id-3D927324b7900ee9b877691a8b237e272fabb21bf5&d=DwIBaQ&c=DPL6_X_6JkXFx7AXWqB0tg&r=qVuU64u9x77Y0Kd0PhDK_lpxFgg6PK9PateHwjb_DY0&m=5Q9WjK0o93NR7DQ9NM6So6mfdgpNnZnSaP8qMpgaC7E&s=LzzjrUQAG8fx5jkVyK73dBDrahNAvk09Cxxlx3KOiXI&e= 
> 
> for some background. I'm planning to submit that documentation for
> mainline integration soon.
> 
>      Arnd

Thanks for sharing the document.

My understanding is that I should use a data structure like the struct 
spi_ioc_transfer, with this I may also use the same ioctl command 
definition. Am I right?
In the documentation you also refer the compact_ioctl() and It is not 
clear to me if the compact_ioctl() is mandatory in this case. Should I 
implement it as well?

Best regards,
Vitor Soares
_______________________________________________
linux-i3c mailing list
linux-i3c@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-i3c

Re: [RFC 5/5] i3c: add i3cdev module to expose i3c dev in /dev

[Arnd Bergmann]

On Tue, Dec 10, 2019 at 8:15 PM Vitor Soares <Vitor.Soares@synopsys.com> wrote:
> From: Arnd Bergmann <arnd@arndb.de>
> Date: Tue, Dec 10, 2019 at 17:51:14
>
> > On Tue, Dec 10, 2019 at 4:37 PM Vitor Soares <Vitor.Soares@synopsys.com> wrote:
> > >
> > > +/* IOCTL commands */
> > > +#define I3C_DEV_IOC_MAGIC      0x07
> > > +
> > > +struct i3c_ioc_priv_xfer {
> > > +       struct i3c_priv_xfer __user *xfers;     /* pointers to i3c_priv_xfer */
> > > +       __u32 nxfers;                           /* number of i3c_priv_xfer */
> > > +};
> > > +
> > > +#define I3C_IOC_PRIV_XFER      \
> > > +       _IOW(I3C_DEV_IOC_MAGIC, 30, struct i3c_ioc_priv_xfer)
> > > +
> > > +#define  I3C_IOC_PRIV_XFER_MAX_MSGS    42
> >
> > This is not a great data structure for UAPI, please see
> > https://urldefense.proofpoint.com/v2/url?u=https-3A__git.kernel.org_pub_scm_linux_kernel_git_arnd_playground.git_tree_Documentation_core-2Dapi_ioctl.rst-3Fh-3Dcompat-2Dioctl-2Dendgame-26id-3D927324b7900ee9b877691a8b237e272fabb21bf5&d=DwIBaQ&c=DPL6_X_6JkXFx7AXWqB0tg&r=qVuU64u9x77Y0Kd0PhDK_lpxFgg6PK9PateHwjb_DY0&m=5Q9WjK0o93NR7DQ9NM6So6mfdgpNnZnSaP8qMpgaC7E&s=LzzjrUQAG8fx5jkVyK73dBDrahNAvk09Cxxlx3KOiXI&e=
> >
> > for some background. I'm planning to submit that documentation for
> > mainline integration soon.
> >
> >      Arnd
>
> Thanks for sharing the document.
>
> My understanding is that I should use a data structure like the struct
> spi_ioc_transfer, with this I may also use the same ioctl command
> definition. Am I right?

Yes, that would be an example of a structure that follows the best
practices from my document. It is still rather complex, so if you
can make it any simpler, that would be ideal.

> In the documentation you also refer the compact_ioctl() and It is not
> clear to me if the compact_ioctl() is mandatory in this case. Should I
> implement it as well?

If the structure is defined like that, you just need to set
".compat_ioctl=compat_ptr_ioctl," in the file_operations structure
and it will work, but you cannot skip that step.

As your interface is basically just read/write based, I wonder
if there is a way to completely avoid the ioctl and instead
use io_submit() as the primary interface.

      Arnd

_______________________________________________
linux-i3c mailing list
linux-i3c@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-i3c

RE: [RFC 5/5] i3c: add i3cdev module to expose i3c dev in /dev

[Vitor Soares]

Hi Arnd,

From: Arnd Bergmann <arnd@arndb.de>
Date: Tue, Dec 10, 2019 at 19:37:14

> On Tue, Dec 10, 2019 at 8:15 PM Vitor Soares <Vitor.Soares@synopsys.com> wrote:
> > From: Arnd Bergmann <arnd@arndb.de>
> > Date: Tue, Dec 10, 2019 at 17:51:14
> >
> > > On Tue, Dec 10, 2019 at 4:37 PM Vitor Soares <Vitor.Soares@synopsys.com> wrote:
> > > >
> > > > +/* IOCTL commands */
> > > > +#define I3C_DEV_IOC_MAGIC      0x07
> > > > +
> > > > +struct i3c_ioc_priv_xfer {
> > > > +       struct i3c_priv_xfer __user *xfers;     /* pointers to i3c_priv_xfer */
> > > > +       __u32 nxfers;                           /* number of i3c_priv_xfer */
> > > > +};
> > > > +
> > > > +#define I3C_IOC_PRIV_XFER      \
> > > > +       _IOW(I3C_DEV_IOC_MAGIC, 30, struct i3c_ioc_priv_xfer)
> > > > +
> > > > +#define  I3C_IOC_PRIV_XFER_MAX_MSGS    42
> > >
> > > This is not a great data structure for UAPI, please see
> > > https://urldefense.proofpoint.com/v2/url?u=https-3A__git.kernel.org_pub_scm_linux_kernel_git_arnd_playground.git_tree_Documentation_core-2Dapi_ioctl.rst-3Fh-3Dcompat-2Dioctl-2Dendgame-26id-3D927324b7900ee9b877691a8b237e272fabb21bf5&d=DwIBaQ&c=DPL6_X_6JkXFx7AXWqB0tg&r=qVuU64u9x77Y0Kd0PhDK_lpxFgg6PK9PateHwjb_DY0&m=5Q9WjK0o93NR7DQ9NM6So6mfdgpNnZnSaP8qMpgaC7E&s=LzzjrUQAG8fx5jkVyK73dBDrahNAvk09Cxxlx3KOiXI&e=
> > >
> > > for some background. I'm planning to submit that documentation for
> > > mainline integration soon.
> > >
> > >      Arnd
> >
> > Thanks for sharing the document.
> >
> > My understanding is that I should use a data structure like the struct
> > spi_ioc_transfer, with this I may also use the same ioctl command
> > definition. Am I right?
> 
> Yes, that would be an example of a structure that follows the best
> practices from my document. It is still rather complex, so if you
> can make it any simpler, that would be ideal.

I will try to do that.

> 
> > In the documentation you also refer the compact_ioctl() and It is not
> > clear to me if the compact_ioctl() is mandatory in this case. Should I
> > implement it as well?
> 
> If the structure is defined like that, you just need to set
> ".compat_ioctl=compat_ptr_ioctl," in the file_operations structure
> and it will work, but you cannot skip that step.

Thanks, now I know that is mandatory 😊.

> 
> As your interface is basically just read/write based, I wonder
> if there is a way to completely avoid the ioctl and instead
> use io_submit() as the primary interface.
> 
>       Arnd

I confess that I wasn't familiar with io_submit() until now and went 
straightway for the ioctl() approach.
So far, my understanding is that io_submit() will call .write or .read of 
i3cdev module depending on the iocb command. if so, we won't be able to 
do a repeated start between a multiple iocb in the same list, right?

Apart from this private read/write need, another requirement that leads 
me to use the ioctl() was:
- When we support HDR command in i3c subsystem we can expand the ioctl() 
command to support it.
- For now, device API doesn't expose CCC commands but some of them are 
used for a private contract between master and device and we likely need 
that support in the future as well.

Thanks,
Best regards,
Vitor Soares

_______________________________________________
linux-i3c mailing list
linux-i3c@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-i3c

Re: [RFC 5/5] i3c: add i3cdev module to expose i3c dev in /dev

[Arnd Bergmann]

On Wed, Dec 11, 2019 at 4:07 PM Vitor Soares <Vitor.Soares@synopsys.com> wrote:
> From: Arnd Bergmann <arnd@arndb.de>
> Date: Tue, Dec 10, 2019 at 19:37:14
> > On Tue, Dec 10, 2019 at 8:15 PM Vitor Soares <Vitor.Soares@synopsys.com> wrote:
> > > From: Arnd Bergmann <arnd@arndb.de>
> > > Date: Tue, Dec 10, 2019 at 17:51:14
> > > > On Tue, Dec 10, 2019 at 4:37 PM Vitor Soares <Vitor.Soares@synopsys.com> wrote:
> > As your interface is basically just read/write based, I wonder
> > if there is a way to completely avoid the ioctl and instead
> > use io_submit() as the primary interface.
>
> I confess that I wasn't familiar with io_submit() until now and went
> straightway for the ioctl() approach.
> So far, my understanding is that io_submit() will call .write or .read of
> i3cdev module depending on the iocb command. if so, we won't be able to
> do a repeated start between a multiple iocb in the same list, right?

I'm not sure what you mean with "repeated start", but it's definitely
possible that io_submit() is not a useful interface for i3c. The main
advantage would be that it avoids creating a complex ioctl command.

> Apart from this private read/write need, another requirement that leads
> me to use the ioctl() was:
> - When we support HDR command in i3c subsystem we can expand the ioctl()
> command to support it.
> - For now, device API doesn't expose CCC commands but some of them are
> used for a private contract between master and device and we likely need
> that support in the future as well.

I think you could still have both the io_submit() interface for basic I/O
(if you can get it to do what you want), plus an ioctl interface for more
complex interactions.

     Arnd

_______________________________________________
linux-i3c mailing list
linux-i3c@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-i3c

Re: [RFC 3/5] i3c: device: expose transfer strutures to uapi

[Greg KH]

On Tue, Dec 10, 2019 at 04:37:31PM +0100, Vitor Soares wrote:
> --- /dev/null
> +++ b/include/uapi/linux/i3c/device.h
> @@ -0,0 +1,66 @@
> +/* SPDX-License-Identifier: GPL-2.0 */

Not a valid uapi SPDX license :(


> +/*
> + * Copyright (c) 2019 Synopsys, Inc. and/or its affiliates.
> + *
> + * Author: Vitor Soares <vitor.soares@synopsys.com>
> + */
> +
> +#ifndef _UAPI_LINUX_I3C_DEVICE_H
> +#define _UAPI_LINUX_I3C_DEVICE_H
> +
> +#include <linux/types.h>
> +
> +/**
> + * enum i3c_error_code - I3C error codes
> + *
> + * These are the standard error codes as defined by the I3C specification.
> + * When -EIO is returned by the i3c_device_do_priv_xfers() or
> + * i3c_device_send_hdr_cmds() one can check the error code in
> + * &struct_i3c_priv_xfer.err or &struct i3c_hdr_cmd.err to get a better idea of
> + * what went wrong.
> + *
> + * @I3C_ERROR_UNKNOWN: unknown error, usually means the error is not I3C
> + *		       related
> + * @I3C_ERROR_M0: M0 error
> + * @I3C_ERROR_M1: M1 error
> + * @I3C_ERROR_M2: M2 error
> + */
> +enum i3c_error_code {
> +	I3C_ERROR_UNKNOWN = 0,
> +	I3C_ERROR_M0 = 1,
> +	I3C_ERROR_M1,
> +	I3C_ERROR_M2,

You have to specify all of these.

> +};
> +
> +/**
> + * enum i3c_hdr_mode - HDR mode ids
> + * @I3C_HDR_DDR: DDR mode
> + * @I3C_HDR_TSP: TSP mode
> + * @I3C_HDR_TSL: TSL mode
> + */
> +enum i3c_hdr_mode {
> +	I3C_HDR_DDR,
> +	I3C_HDR_TSP,
> +	I3C_HDR_TSL,

same here.


> +};
> +
> +/**
> + * struct i3c_priv_xfer - I3C SDR private transfer
> + * @rnw: encodes the transfer direction. true for a read, false for a write
> + * @len: transfer length in bytes of the transfer
> + * @data: input/output buffer
> + * @data.in: input buffer. Must point to a DMA-able buffer
> + * @data.out: output buffer. Must point to a DMA-able buffer
> + * @err: I3C error code
> + */
> +struct i3c_priv_xfer {
> +	u8 rnw;
> +	u16 len;
> +	union {
> +		void *in;
> +		const void *out;
> +	} data;
> +	enum i3c_error_code err;

Ick, that's a horrid user/kernel api structure that will not work at
all.

Please fix.

thanks,

greg k-h

_______________________________________________
linux-i3c mailing list
linux-i3c@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-i3c

Re: [RFC 5/5] i3c: add i3cdev module to expose i3c dev in /dev

[Greg KH]

On Tue, Dec 10, 2019 at 04:37:33PM +0100, Vitor Soares wrote:
> +static int __init i3cdev_init(void)
> +{
> +	int res;
> +
> +	pr_info("i3c /dev entries driver\n");

Please remove debugging information, kernel code should be quiet unless
something goes wrong.

> +	/* Dynamically request unused major number */
> +	res = alloc_chrdev_region(&i3cdev_number, 0, N_I3C_MINORS, "i3c");

Do you really need a whole major, or will a few minors work?

thanks,

greg k-h

_______________________________________________
linux-i3c mailing list
linux-i3c@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-i3c

Re: [RFC 5/5] i3c: add i3cdev module to expose i3c dev in /dev

[Greg KH]

On Tue, Dec 10, 2019 at 04:37:33PM +0100, Vitor Soares wrote:
> +static struct i3cdev_data *i3cdev_get_by_minor(unsigned int minor)

Why?  Why not just embed the structure in the cdev if you really need
it?

> +static struct i3cdev_data *get_free_i3cdev(struct i3c_device *i3c)
> +{
> +	struct i3cdev_data *i3cdev;
> +	unsigned long minor;
> +
> +	minor = find_first_zero_bit(minors, N_I3C_MINORS);

No locking, fun!!!

:(

Why not use an idr instead, that is what it is there for.  Don't try to
roll your own.

thanks,

greg k-h

_______________________________________________
linux-i3c mailing list
linux-i3c@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-i3c

RE: [RFC 3/5] i3c: device: expose transfer strutures to uapi

[Vitor Soares]

HI Greg,

From: Greg KH <gregkh@linuxfoundation.org>
Date: Thu, Dec 12, 2019 at 14:42:33

> On Tue, Dec 10, 2019 at 04:37:31PM +0100, Vitor Soares wrote:
> > --- /dev/null
> > +++ b/include/uapi/linux/i3c/device.h
> > @@ -0,0 +1,66 @@
> > +/* SPDX-License-Identifier: GPL-2.0 */
> 
> Not a valid uapi SPDX license :(

Sorry, I missed that one. I already got the right SPDX.

> 
> 
> > +/*
> > + * Copyright (c) 2019 Synopsys, Inc. and/or its affiliates.
> > + *
> > + * Author: Vitor Soares <vitor.soares@synopsys.com>
> > + */
> > +
> > +#ifndef _UAPI_LINUX_I3C_DEVICE_H
> > +#define _UAPI_LINUX_I3C_DEVICE_H
> > +
> > +#include <linux/types.h>
> > +
> > +/**
> > + * enum i3c_error_code - I3C error codes
> > + *
> > + * These are the standard error codes as defined by the I3C specification.
> > + * When -EIO is returned by the i3c_device_do_priv_xfers() or
> > + * i3c_device_send_hdr_cmds() one can check the error code in
> > + * &struct_i3c_priv_xfer.err or &struct i3c_hdr_cmd.err to get a better idea of
> > + * what went wrong.
> > + *
> > + * @I3C_ERROR_UNKNOWN: unknown error, usually means the error is not I3C
> > + *		       related
> > + * @I3C_ERROR_M0: M0 error
> > + * @I3C_ERROR_M1: M1 error
> > + * @I3C_ERROR_M2: M2 error
> > + */
> > +enum i3c_error_code {
> > +	I3C_ERROR_UNKNOWN = 0,
> > +	I3C_ERROR_M0 = 1,
> > +	I3C_ERROR_M1,
> > +	I3C_ERROR_M2,
> 
> You have to specify all of these.
> 
> > +};
> > +
> > +/**
> > + * enum i3c_hdr_mode - HDR mode ids
> > + * @I3C_HDR_DDR: DDR mode
> > + * @I3C_HDR_TSP: TSP mode
> > + * @I3C_HDR_TSL: TSL mode
> > + */
> > +enum i3c_hdr_mode {
> > +	I3C_HDR_DDR,
> > +	I3C_HDR_TSP,
> > +	I3C_HDR_TSL,
> 
> same here.
> 
> 
> > +};
> > +
> > +/**
> > + * struct i3c_priv_xfer - I3C SDR private transfer
> > + * @rnw: encodes the transfer direction. true for a read, false for a write
> > + * @len: transfer length in bytes of the transfer
> > + * @data: input/output buffer
> > + * @data.in: input buffer. Must point to a DMA-able buffer
> > + * @data.out: output buffer. Must point to a DMA-able buffer
> > + * @err: I3C error code
> > + */
> > +struct i3c_priv_xfer {
> > +	u8 rnw;
> > +	u16 len;
> > +	union {
> > +		void *in;
> > +		const void *out;
> > +	} data;
> > +	enum i3c_error_code err;
> 
> Ick, that's a horrid user/kernel api structure that will not work at
> all.
> 
> Please fix.
> 
> thanks,
> 
> greg k-h

Thanks for your feedback, I will address it in next version.

Best regards,
Vitor Soares

_______________________________________________
linux-i3c mailing list
linux-i3c@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-i3c

RE: [RFC 5/5] i3c: add i3cdev module to expose i3c dev in /dev

[Vitor Soares]

Hi Greg,

From: Greg KH <gregkh@linuxfoundation.org>
Date: Thu, Dec 12, 2019 at 14:44:59

> On Tue, Dec 10, 2019 at 04:37:33PM +0100, Vitor Soares wrote:
> > +static int __init i3cdev_init(void)
> > +{
> > +	int res;
> > +
> > +	pr_info("i3c /dev entries driver\n");
> 
> Please remove debugging information, kernel code should be quiet unless
> something goes wrong.

I will remove it.

> 
> > +	/* Dynamically request unused major number */
> > +	res = alloc_chrdev_region(&i3cdev_number, 0, N_I3C_MINORS, "i3c");
> 
> Do you really need a whole major, or will a few minors work?
> 
> thanks,
> 
> greg k-h

I'm reserving one per device. What do you suggest?

Best regards,
Vitor Soares

_______________________________________________
linux-i3c mailing list
linux-i3c@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-i3c

Re: [RFC 5/5] i3c: add i3cdev module to expose i3c dev in /dev

[Greg KH]

On Thu, Dec 12, 2019 at 02:56:56PM +0000, Vitor Soares wrote:
> Hi Greg,
> 
> From: Greg KH <gregkh@linuxfoundation.org>
> Date: Thu, Dec 12, 2019 at 14:44:59
> 
> > On Tue, Dec 10, 2019 at 04:37:33PM +0100, Vitor Soares wrote:
> > > +static int __init i3cdev_init(void)
> > > +{
> > > +	int res;
> > > +
> > > +	pr_info("i3c /dev entries driver\n");
> > 
> > Please remove debugging information, kernel code should be quiet unless
> > something goes wrong.
> 
> I will remove it.
> 
> > 
> > > +	/* Dynamically request unused major number */
> > > +	res = alloc_chrdev_region(&i3cdev_number, 0, N_I3C_MINORS, "i3c");
> > 
> > Do you really need a whole major, or will a few minors work?
> > 
> > thanks,
> > 
> > greg k-h
> 
> I'm reserving one per device. What do you suggest?

How many devices do you have in a system?

_______________________________________________
linux-i3c mailing list
linux-i3c@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-i3c

RE: [RFC 5/5] i3c: add i3cdev module to expose i3c dev in /dev

[Vitor Soares]

From: Greg KH <gregkh@linuxfoundation.org>
Date: Thu, Dec 12, 2019 at 16:00:45

> On Thu, Dec 12, 2019 at 02:56:56PM +0000, Vitor Soares wrote:
> > Hi Greg,
> > 
> > From: Greg KH <gregkh@linuxfoundation.org>
> > Date: Thu, Dec 12, 2019 at 14:44:59
> > 
> > > On Tue, Dec 10, 2019 at 04:37:33PM +0100, Vitor Soares wrote:
> > > > +static int __init i3cdev_init(void)
> > > > +{
> > > > +	int res;
> > > > +
> > > > +	pr_info("i3c /dev entries driver\n");
> > > 
> > > Please remove debugging information, kernel code should be quiet unless
> > > something goes wrong.
> > 
> > I will remove it.
> > 
> > > 
> > > > +	/* Dynamically request unused major number */
> > > > +	res = alloc_chrdev_region(&i3cdev_number, 0, N_I3C_MINORS, "i3c");
> > > 
> > > Do you really need a whole major, or will a few minors work?
> > > 
> > > thanks,
> > > 
> > > greg k-h
> > 
> > I'm reserving one per device. What do you suggest?
> 
> How many devices do you have in a system?

According to MIPI I3C spec, the maximum number of devices on a bus will 
depend on trace length, bus load, and clock requirements.

Frankly, I don't know what is the best compromise because it depends from 
system to system and the end-use of it. In my case, I started developing 
this to help me during the HC controller driver development.
Even If I choose a fixed major, I wouldn't know in which one i3c fits.

Best regards,
Vitor Soares



_______________________________________________
linux-i3c mailing list
linux-i3c@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-i3c

Re: [RFC 5/5] i3c: add i3cdev module to expose i3c dev in /dev

[Greg KH]

On Thu, Dec 12, 2019 at 05:25:28PM +0000, Vitor Soares wrote:
> From: Greg KH <gregkh@linuxfoundation.org>
> Date: Thu, Dec 12, 2019 at 16:00:45
> 
> > On Thu, Dec 12, 2019 at 02:56:56PM +0000, Vitor Soares wrote:
> > > Hi Greg,
> > > 
> > > From: Greg KH <gregkh@linuxfoundation.org>
> > > Date: Thu, Dec 12, 2019 at 14:44:59
> > > 
> > > > On Tue, Dec 10, 2019 at 04:37:33PM +0100, Vitor Soares wrote:
> > > > > +static int __init i3cdev_init(void)
> > > > > +{
> > > > > +	int res;
> > > > > +
> > > > > +	pr_info("i3c /dev entries driver\n");
> > > > 
> > > > Please remove debugging information, kernel code should be quiet unless
> > > > something goes wrong.
> > > 
> > > I will remove it.
> > > 
> > > > 
> > > > > +	/* Dynamically request unused major number */
> > > > > +	res = alloc_chrdev_region(&i3cdev_number, 0, N_I3C_MINORS, "i3c");
> > > > 
> > > > Do you really need a whole major, or will a few minors work?
> > > > 
> > > > thanks,
> > > > 
> > > > greg k-h
> > > 
> > > I'm reserving one per device. What do you suggest?
> > 
> > How many devices do you have in a system?
> 
> According to MIPI I3C spec, the maximum number of devices on a bus will 
> depend on trace length, bus load, and clock requirements.
> 
> Frankly, I don't know what is the best compromise because it depends from 
> system to system and the end-use of it. In my case, I started developing 
> this to help me during the HC controller driver development.
> Even If I choose a fixed major, I wouldn't know in which one i3c fits.

Ok, a full major will be fine, I was worried you only wanted 1 or 2
minor numbers, which would mean you could have used a misc device
instead.

This is fine.

thanks,

greg k-h

_______________________________________________
linux-i3c mailing list
linux-i3c@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-i3c

RE: [RFC 5/5] i3c: add i3cdev module to expose i3c dev in /dev

[Vitor Soares]

Hi Arnd,

From: Arnd Bergmann <arnd@arndb.de>
Date: Tue, Dec 10, 2019 at 17:51:14

> On Tue, Dec 10, 2019 at 4:37 PM Vitor Soares <Vitor.Soares@synopsys.com> wrote:
> >
> > +/* IOCTL commands */
> > +#define I3C_DEV_IOC_MAGIC      0x07
> > +
> > +struct i3c_ioc_priv_xfer {
> > +       struct i3c_priv_xfer __user *xfers;     /* pointers to i3c_priv_xfer */
> > +       __u32 nxfers;                           /* number of i3c_priv_xfer */
> > +};
> > +
> > +#define I3C_IOC_PRIV_XFER      \
> > +       _IOW(I3C_DEV_IOC_MAGIC, 30, struct i3c_ioc_priv_xfer)
> > +
> > +#define  I3C_IOC_PRIV_XFER_MAX_MSGS    42
> 
> This is not a great data structure for UAPI, please see
> https://urldefense.proofpoint.com/v2/url?u=https-3A__git.kernel.org_pub_scm_linux_kernel_git_arnd_playground.git_tree_Documentation_core-2Dapi_ioctl.rst-3Fh-3Dcompat-2Dioctl-2Dendgame-26id-3D927324b7900ee9b877691a8b237e272fabb21bf5&d=DwIBaQ&c=DPL6_X_6JkXFx7AXWqB0tg&r=qVuU64u9x77Y0Kd0PhDK_lpxFgg6PK9PateHwjb_DY0&m=5Q9WjK0o93NR7DQ9NM6So6mfdgpNnZnSaP8qMpgaC7E&s=LzzjrUQAG8fx5jkVyK73dBDrahNAvk09Cxxlx3KOiXI&e= 
> 
> for some background. I'm planning to submit that documentation for
> mainline integration soon.
> 
>      Arnd

I was checking other code examples for this and found that some authors 
add extra reserved fields for future use. Should I do the same?
 
I have some doubt too if it is ok to use the same ioctl command approach 
as in spidev:

#define SPI_MSGSIZE(N) \
  	((((N)*(sizeof (struct spi_ioc_transfer))) < (1 << _IOC_SIZEBITS)) \
		? ((N)*(sizeof (struct spi_ioc_transfer))) : 0)
#define SPI_IOC_MESSAGE(N) _IOW(SPI_IOC_MAGIC, 0, char[SPI_MSGSIZE(N)])

Or the best is to use another structure to embedded N transfers like in 
this patch.

Thanks in advance for your help.
Best regards,
Vitor Soares
_______________________________________________
linux-i3c mailing list
linux-i3c@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-i3c