Wolfram,
I am sending you the i2c-slave-mqueue driver.
Apparently Haiyue had to move on to another project and
does not have cycles to continue with the comments on this
driver after some time waiting for feedback,
that is essentially why I took over.
Here is a small changelog from V5 to V6:
- Added DT support for probing via Device Tree
- Docummented DT bindings
- Documented sysfs ABI
- Small fixes on wording and Kconfig entries.
Haiyue's V5: https://lkml.org/lkml/2018/4/23/835
BR,
Eduardo Valentin (2):
dt-bindings: i2c: document bindings for i2c-slave-mqueue
Documentation: ABI: Add i2c-slave-mqueue sysfs documentation
Haiyue Wang (1):
i2c: slave-mqueue: add a slave backend to receive and queue messages
.../sysfs-bus-i2c-devices-slave-mqueue | 10 +
.../bindings/i2c/i2c-slave-mqueue.txt | 34 +++
Documentation/i2c/slave-mqueue-backend.rst | 124 ++++++++++
MAINTAINERS | 8 +
drivers/i2c/Kconfig | 25 +++
drivers/i2c/Makefile | 1 +
drivers/i2c/i2c-slave-mqueue.c | 211 ++++++++++++++++++
7 files changed, 413 insertions(+)
create mode 100644 Documentation/ABI/testing/sysfs-bus-i2c-devices-slave-mqueue
create mode 100644 Documentation/devicetree/bindings/i2c/i2c-slave-mqueue.txt
create mode 100644 Documentation/i2c/slave-mqueue-backend.rst
create mode 100644 drivers/i2c/i2c-slave-mqueue.c
--
2.21.0
Document the i2c-slave-mqueue binding by adding
descriptor, required properties, and example.
Cc: Rob Herring <[email protected]>
Cc: Mark Rutland <[email protected]>
Cc: Wolfram Sang <[email protected]>
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
Signed-off-by: Eduardo Valentin <[email protected]>
---
.../bindings/i2c/i2c-slave-mqueue.txt | 34 +++++++++++++++++++
1 file changed, 34 insertions(+)
create mode 100644 Documentation/devicetree/bindings/i2c/i2c-slave-mqueue.txt
diff --git a/Documentation/devicetree/bindings/i2c/i2c-slave-mqueue.txt b/Documentation/devicetree/bindings/i2c/i2c-slave-mqueue.txt
new file mode 100644
index 000000000000..eb1881a4fc0e
--- /dev/null
+++ b/Documentation/devicetree/bindings/i2c/i2c-slave-mqueue.txt
@@ -0,0 +1,34 @@
+===============================================
+Device Tree for I2C slave message queue backend
+===============================================
+
+Some protocols over I2C/SMBus are designed for bi-directional transferring
+messages by using I2C Master Write protocol. This requires that both sides
+of the communication have slave addresses.
+
+This I2C slave mqueue (message queue) is used to receive and queue
+messages from the remote i2c intelligent device; and it will add the target
+slave address (with R/W# bit is always 0) into the message at the first byte.
+
+Links
+----
+`Intelligent Platform Management Bus
+Communications Protocol Specification
+<https://www.intel.com/content/dam/www/public/us/en/documents/product-briefs/ipmp-spec-v1.0.pdf>`_
+
+`Management Component Transport Protocol (MCTP)
+SMBus/I2C Transport Binding Specification
+<https://www.dmtf.org/sites/default/files/standards/documents/DSP0237_1.1.0.pdf>`_
+
+Required Properties:
+- compatible : should be "i2c-slave-mqueue"
+- reg : slave address
+
+Example:
+
+i2c {
+ slave_mqueue: i2c-slave-mqueue {
+ compatible = "i2c-slave-mqueue";
+ reg = <0x10>;
+ };
+};
--
2.21.0
From: Haiyue Wang <[email protected]>
Some protocols over I2C are designed for bi-directional transferring
messages by using I2C Master Write protocol. Like the MCTP (Management
Component Transport Protocol) and IPMB (Intelligent Platform Management
Bus), they both require that the userspace can receive messages from
I2C dirvers under slave mode.
This new slave mqueue backend is used to receive and queue messages, it
will exposes these messages to userspace by sysfs bin file.
Note: DT interface and a couple of minor fixes here and there
by Eduardo, so I kept the original authorship here.
Cc: Rob Herring <[email protected]>
Cc: Mark Rutland <[email protected]>
Cc: Wolfram Sang <[email protected]>
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
Signed-off-by: Haiyue Wang <[email protected]>
Signed-off-by: Eduardo Valentin <[email protected]>
---
Documentation/i2c/slave-mqueue-backend.rst | 124 ++++++++++++
MAINTAINERS | 8 +
drivers/i2c/Kconfig | 25 +++
drivers/i2c/Makefile | 1 +
drivers/i2c/i2c-slave-mqueue.c | 211 +++++++++++++++++++++
5 files changed, 369 insertions(+)
create mode 100644 Documentation/i2c/slave-mqueue-backend.rst
create mode 100644 drivers/i2c/i2c-slave-mqueue.c
diff --git a/Documentation/i2c/slave-mqueue-backend.rst b/Documentation/i2c/slave-mqueue-backend.rst
new file mode 100644
index 000000000000..376dff998fa3
--- /dev/null
+++ b/Documentation/i2c/slave-mqueue-backend.rst
@@ -0,0 +1,124 @@
+.. SPDX-License-Identifier: GPL-2.0
+
+=====================================
+Linux I2C slave message queue backend
+=====================================
+
+:Author: Haiyue Wang <[email protected]>
+
+Some protocols over I2C/SMBus are designed for bi-directional transferring
+messages by using I2C Master Write protocol. This requires that both sides
+of the communication have slave addresses.
+
+Like MCTP (Management Component Transport Protocol) and IPMB (Intelligent
+Platform Management Bus), they both require that the userspace can receive
+messages from i2c drivers under slave mode.
+
+This I2C slave mqueue (message queue) backend is used to receive and queue
+messages from the remote i2c intelligent device; and it will add the target
+slave address (with R/W# bit is always 0) into the message at the first byte,
+so that userspace can use this byte to dispatch the messages into different
+handling modules. Also, like IPMB, the address byte is in its message format,
+it needs it to do checksum.
+
+For messages are time related, so this backend will flush the oldest message
+to queue the newest one.
+
+Link
+----
+`Intelligent Platform Management Bus
+Communications Protocol Specification
+<https://www.intel.com/content/dam/www/public/us/en/documents/product-briefs/ipmp-spec-v1.0.pdf>`_
+
+`Management Component Transport Protocol (MCTP)
+SMBus/I2C Transport Binding Specification
+<https://www.dmtf.org/sites/default/files/standards/documents/DSP0237_1.1.0.pdf>`_
+
+How to use
+----------
+For example, the I2C5 bus has slave address 0x10, the below command will create
+the related message queue interface:
+
+ echo slave-mqueue 0x1010 > /sys/bus/i2c/devices/i2c-5/new_device
+
+Then you can dump the messages like this:
+
+ hexdump -C /sys/bus/i2c/devices/5-1010/slave-mqueue
+
+Code Example
+------------
+*Note: call 'lseek' before 'read', this is a requirement from kernfs' design.*
+
+::
+
+ #include <sys/types.h>
+ #include <sys/stat.h>
+ #include <unistd.h>
+ #include <poll.h>
+ #include <time.h>
+ #include <fcntl.h>
+ #include <stdio.h>
+
+ int main(int argc, char *argv[])
+ {
+ int i, r;
+ struct pollfd pfd;
+ struct timespec ts;
+ unsigned char data[256];
+
+ pfd.fd = open(argv[1], O_RDONLY | O_NONBLOCK);
+ if (pfd.fd < 0)
+ return -1;
+
+ pfd.events = POLLPRI;
+
+ while (1) {
+ r = poll(&pfd, 1, 5000);
+
+ if (r < 0)
+ break;
+
+ if (r == 0 || !(pfd.revents & POLLPRI))
+ continue;
+
+ lseek(pfd.fd, 0, SEEK_SET);
+ r = read(pfd.fd, data, sizeof(data));
+ if (r <= 0)
+ continue;
+
+ clock_gettime(CLOCK_MONOTONIC, &ts);
+ printf("[%ld.%.9ld] :", ts.tv_sec, ts.tv_nsec);
+ for (i = 0; i < r; i++)
+ printf(" %02x", data[i]);
+ printf("\n");
+ }
+
+ close(pfd.fd);
+
+ return 0;
+ }
+
+Result
+------
+*./a.out "/sys/bus/i2c/devices/5-1010/slave-mqueue"*
+
+::
+
+ [10183.232500449] : 20 18 c8 2c 78 01 5b
+ [10183.479358348] : 20 18 c8 2c 78 01 5b
+ [10183.726556812] : 20 18 c8 2c 78 01 5b
+ [10183.972605863] : 20 18 c8 2c 78 01 5b
+ [10184.220124772] : 20 18 c8 2c 78 01 5b
+ [10184.467764166] : 20 18 c8 2c 78 01 5b
+ [10193.233421784] : 20 18 c8 2c 7c 01 57
+ [10193.480273460] : 20 18 c8 2c 7c 01 57
+ [10193.726788733] : 20 18 c8 2c 7c 01 57
+ [10193.972781945] : 20 18 c8 2c 7c 01 57
+ [10194.220487360] : 20 18 c8 2c 7c 01 57
+ [10194.468089259] : 20 18 c8 2c 7c 01 57
+ [10203.233433099] : 20 18 c8 2c 80 01 53
+ [10203.481058715] : 20 18 c8 2c 80 01 53
+ [10203.727610472] : 20 18 c8 2c 80 01 53
+ [10203.974044856] : 20 18 c8 2c 80 01 53
+ [10204.220734634] : 20 18 c8 2c 80 01 53
+ [10204.468461664] : 20 18 c8 2c 80 01 53
diff --git a/MAINTAINERS b/MAINTAINERS
index 429c6c624861..bca17c3398b2 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -7445,6 +7445,14 @@ L: [email protected]
S: Maintained
F: drivers/i2c/i2c-stub.c
+I2C SLAVE MQUEUE DRIVER
+M: Eduardo Valentin <[email protected]>
+L: [email protected]
+S: Maintained
+F: drivers/i2c/i2c-slave-mqueue.c
+F: Documentation/i2c/slave-mqueue-backend.rst
+F: Documentation/devicetree/bindings/i2c/i2c-slave-mqueue.txt
+
I3C SUBSYSTEM
M: Boris Brezillon <[email protected]>
L: [email protected]
diff --git a/drivers/i2c/Kconfig b/drivers/i2c/Kconfig
index abedd55a1264..f335924936ae 100644
--- a/drivers/i2c/Kconfig
+++ b/drivers/i2c/Kconfig
@@ -119,6 +119,31 @@ if I2C_SLAVE
config I2C_SLAVE_EEPROM
tristate "I2C eeprom slave driver"
+config I2C_SLAVE_MQUEUE
+ tristate "I2C mqueue (message queue) slave driver"
+ help
+ Some protocols over I2C are designed for bi-directional transferring
+ messages by using I2C Master Write protocol. This driver is used to
+ receive and queue messages from the remote I2C device.
+
+ Userspace can get the messages by reading sysfs file that this driver
+ exposes.
+
+ This support is also available as a module. If so, the module will be
+ called i2c-slave-mqueue.
+
+config I2C_SLAVE_MQUEUE_MESSAGE_SIZE
+ int "The message size of I2C mqueue slave"
+ depends on I2C_SLAVE_MQUEUE
+ default 120
+
+config I2C_SLAVE_MQUEUE_QUEUE_SIZE
+ int "The queue size of I2C mqueue slave"
+ depends on I2C_SLAVE_MQUEUE
+ default 32
+ help
+ This number MUST be power of 2.
+
endif
config I2C_DEBUG_CORE
diff --git a/drivers/i2c/Makefile b/drivers/i2c/Makefile
index bed6ba63c983..9a31bc75a446 100644
--- a/drivers/i2c/Makefile
+++ b/drivers/i2c/Makefile
@@ -16,5 +16,6 @@ obj-$(CONFIG_I2C_MUX) += i2c-mux.o
obj-y += algos/ busses/ muxes/
obj-$(CONFIG_I2C_STUB) += i2c-stub.o
obj-$(CONFIG_I2C_SLAVE_EEPROM) += i2c-slave-eeprom.o
+obj-$(CONFIG_I2C_SLAVE_MQUEUE) += i2c-slave-mqueue.o
ccflags-$(CONFIG_I2C_DEBUG_CORE) := -DDEBUG
diff --git a/drivers/i2c/i2c-slave-mqueue.c b/drivers/i2c/i2c-slave-mqueue.c
new file mode 100644
index 000000000000..235099549d40
--- /dev/null
+++ b/drivers/i2c/i2c-slave-mqueue.c
@@ -0,0 +1,211 @@
+// SPDX-License-Identifier: GPL-2.0
+// Copyright (c) 2017 - 2018, Intel Corporation.
+
+#include <linux/i2c.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/slab.h>
+#include <linux/spinlock.h>
+#include <linux/sysfs.h>
+
+#define MQ_MSGBUF_SIZE CONFIG_I2C_SLAVE_MQUEUE_MESSAGE_SIZE
+#define MQ_QUEUE_SIZE CONFIG_I2C_SLAVE_MQUEUE_QUEUE_SIZE
+#define MQ_QUEUE_NEXT(x) (((x) + 1) & (MQ_QUEUE_SIZE - 1))
+
+struct mq_msg {
+ int len;
+ u8 *buf;
+};
+
+struct mq_queue {
+ struct bin_attribute bin;
+ struct kernfs_node *kn;
+
+ spinlock_t lock; /* spinlock for queue index handling */
+ int in;
+ int out;
+
+ struct mq_msg *curr;
+ int truncated; /* drop current if truncated */
+ struct mq_msg queue[MQ_QUEUE_SIZE];
+};
+
+static int i2c_slave_mqueue_callback(struct i2c_client *client,
+ enum i2c_slave_event event, u8 *val)
+{
+ struct mq_queue *mq = i2c_get_clientdata(client);
+ struct mq_msg *msg = mq->curr;
+ int ret = 0;
+
+ switch (event) {
+ case I2C_SLAVE_WRITE_REQUESTED:
+ mq->truncated = 0;
+
+ msg->len = 1;
+ msg->buf[0] = client->addr << 1;
+ break;
+
+ case I2C_SLAVE_WRITE_RECEIVED:
+ if (msg->len < MQ_MSGBUF_SIZE) {
+ msg->buf[msg->len++] = *val;
+ } else {
+ dev_err(&client->dev, "message is truncated!\n");
+ mq->truncated = 1;
+ ret = -EINVAL;
+ }
+ break;
+
+ case I2C_SLAVE_STOP:
+ if (unlikely(mq->truncated || msg->len < 2))
+ break;
+
+ spin_lock(&mq->lock);
+ mq->in = MQ_QUEUE_NEXT(mq->in);
+ mq->curr = &mq->queue[mq->in];
+ mq->curr->len = 0;
+
+ /* Flush the oldest message */
+ if (mq->out == mq->in)
+ mq->out = MQ_QUEUE_NEXT(mq->out);
+ spin_unlock(&mq->lock);
+
+ kernfs_notify(mq->kn);
+ break;
+
+ default:
+ *val = 0xFF;
+ break;
+ }
+
+ return ret;
+}
+
+static ssize_t i2c_slave_mqueue_bin_read(struct file *filp,
+ struct kobject *kobj,
+ struct bin_attribute *attr,
+ char *buf, loff_t pos, size_t count)
+{
+ struct mq_queue *mq;
+ struct mq_msg *msg;
+ unsigned long flags;
+ bool more = false;
+ ssize_t ret = 0;
+
+ mq = dev_get_drvdata(container_of(kobj, struct device, kobj));
+
+ spin_lock_irqsave(&mq->lock, flags);
+ if (mq->out != mq->in) {
+ msg = &mq->queue[mq->out];
+
+ if (msg->len <= count) {
+ ret = msg->len;
+ memcpy(buf, msg->buf, ret);
+ } else {
+ ret = -EOVERFLOW; /* Drop this HUGE one. */
+ }
+
+ mq->out = MQ_QUEUE_NEXT(mq->out);
+ if (mq->out != mq->in)
+ more = true;
+ }
+ spin_unlock_irqrestore(&mq->lock, flags);
+
+ if (more)
+ kernfs_notify(mq->kn);
+
+ return ret;
+}
+
+static int i2c_slave_mqueue_probe(struct i2c_client *client,
+ const struct i2c_device_id *id)
+{
+ struct device *dev = &client->dev;
+ struct mq_queue *mq;
+ int ret, i;
+ void *buf;
+
+ mq = devm_kzalloc(dev, sizeof(*mq), GFP_KERNEL);
+ if (!mq)
+ return -ENOMEM;
+
+ BUILD_BUG_ON(!is_power_of_2(MQ_QUEUE_SIZE));
+
+ buf = devm_kmalloc_array(dev, MQ_QUEUE_SIZE, MQ_MSGBUF_SIZE,
+ GFP_KERNEL);
+ if (!buf)
+ return -ENOMEM;
+
+ for (i = 0; i < MQ_QUEUE_SIZE; i++)
+ mq->queue[i].buf = buf + i * MQ_MSGBUF_SIZE;
+
+ i2c_set_clientdata(client, mq);
+
+ spin_lock_init(&mq->lock);
+ mq->curr = &mq->queue[0];
+
+ sysfs_bin_attr_init(&mq->bin);
+ mq->bin.attr.name = "slave-mqueue";
+ mq->bin.attr.mode = 0400;
+ mq->bin.read = i2c_slave_mqueue_bin_read;
+ mq->bin.size = MQ_MSGBUF_SIZE * MQ_QUEUE_SIZE;
+
+ ret = sysfs_create_bin_file(&dev->kobj, &mq->bin);
+ if (ret)
+ return ret;
+
+ mq->kn = kernfs_find_and_get(dev->kobj.sd, mq->bin.attr.name);
+ if (!mq->kn) {
+ sysfs_remove_bin_file(&dev->kobj, &mq->bin);
+ return -EFAULT;
+ }
+
+ ret = i2c_slave_register(client, i2c_slave_mqueue_callback);
+ if (ret) {
+ kernfs_put(mq->kn);
+ sysfs_remove_bin_file(&dev->kobj, &mq->bin);
+ return ret;
+ }
+
+ return 0;
+}
+
+static int i2c_slave_mqueue_remove(struct i2c_client *client)
+{
+ struct mq_queue *mq = i2c_get_clientdata(client);
+
+ i2c_slave_unregister(client);
+
+ kernfs_put(mq->kn);
+ sysfs_remove_bin_file(&client->dev.kobj, &mq->bin);
+
+ return 0;
+}
+
+static const struct i2c_device_id i2c_slave_mqueue_id[] = {
+ { "slave-mqueue", 0 },
+ { }
+};
+MODULE_DEVICE_TABLE(i2c, i2c_slave_mqueue_id);
+
+static const struct of_device_id i2c_slave_mqueue_of_match[] = {
+ {
+ .compatible = "i2c-slave-mqueue",
+ },
+ { },
+};
+
+static struct i2c_driver i2c_slave_mqueue_driver = {
+ .driver = {
+ .name = "i2c-slave-mqueue",
+ .of_match_table = of_match_ptr(i2c_slave_mqueue_of_match),
+ },
+ .probe = i2c_slave_mqueue_probe,
+ .remove = i2c_slave_mqueue_remove,
+ .id_table = i2c_slave_mqueue_id,
+};
+module_i2c_driver(i2c_slave_mqueue_driver);
+
+MODULE_LICENSE("GPL v2");
+MODULE_AUTHOR("Haiyue Wang <[email protected]>");
+MODULE_DESCRIPTION("I2C slave mode for receiving and queuing messages");
--
2.21.0
Document the slave-mqueue sysfs attribute used by
the i2c-slave-mqueue driver.
Cc: Rob Herring <[email protected]>
Cc: Mark Rutland <[email protected]>
Cc: Wolfram Sang <[email protected]>
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
Signed-off-by: Eduardo Valentin <[email protected]>
---
.../ABI/testing/sysfs-bus-i2c-devices-slave-mqueue | 10 ++++++++++
1 file changed, 10 insertions(+)
create mode 100644 Documentation/ABI/testing/sysfs-bus-i2c-devices-slave-mqueue
diff --git a/Documentation/ABI/testing/sysfs-bus-i2c-devices-slave-mqueue b/Documentation/ABI/testing/sysfs-bus-i2c-devices-slave-mqueue
new file mode 100644
index 000000000000..28318108ce85
--- /dev/null
+++ b/Documentation/ABI/testing/sysfs-bus-i2c-devices-slave-mqueue
@@ -0,0 +1,10 @@
+What: /sys/bus/i2c/devices/*/slave-mqueue
+Date: May 2019
+KernelVersion: 5.2
+Contact: Eduardo Valentin <[email protected]>
+Description:
+ Reading to this file will return exactly one message,
+ when available, of the i2c-slave-mqueue device attached
+ to that bus. Userspace can also poll on this file to
+ get notified when new messages are available.
+Users: i2c-slave-mqueue driver
--
2.21.0
On Thu, May 30, 2019 at 09:33:46PM -0700, Eduardo Valentin wrote:
> From: Haiyue Wang <[email protected]>
>
> Some protocols over I2C are designed for bi-directional transferring
> messages by using I2C Master Write protocol. Like the MCTP (Management
> Component Transport Protocol) and IPMB (Intelligent Platform Management
> Bus), they both require that the userspace can receive messages from
> I2C dirvers under slave mode.
>
> This new slave mqueue backend is used to receive and queue messages, it
> will exposes these messages to userspace by sysfs bin file.
>
> Note: DT interface and a couple of minor fixes here and there
> by Eduardo, so I kept the original authorship here.
> +#define MQ_MSGBUF_SIZE CONFIG_I2C_SLAVE_MQUEUE_MESSAGE_SIZE
> +#define MQ_QUEUE_SIZE CONFIG_I2C_SLAVE_MQUEUE_QUEUE_SIZE
> +#define MQ_QUEUE_NEXT(x) (((x) + 1) & (MQ_QUEUE_SIZE - 1))
Also possible ((x + 1) % ..._SIZE)
> + mq = dev_get_drvdata(container_of(kobj, struct device, kobj));
kobj_to_dev()
> +static int i2c_slave_mqueue_probe(struct i2c_client *client,
> + const struct i2c_device_id *id)
> +{
> + struct device *dev = &client->dev;
> + struct mq_queue *mq;
> + int ret, i;
> + void *buf;
> +
> + mq = devm_kzalloc(dev, sizeof(*mq), GFP_KERNEL);
> + if (!mq)
> + return -ENOMEM;
> +
> + BUILD_BUG_ON(!is_power_of_2(MQ_QUEUE_SIZE));
Perhaps start function with this kind of assertions?
> +
> + buf = devm_kmalloc_array(dev, MQ_QUEUE_SIZE, MQ_MSGBUF_SIZE,
> + GFP_KERNEL);
> + if (!buf)
> + return -ENOMEM;
> +
> + for (i = 0; i < MQ_QUEUE_SIZE; i++)
> + mq->queue[i].buf = buf + i * MQ_MSGBUF_SIZE;
Just wondering if kfifo API can bring an advantage here?
> + return 0;
> +}
> +static const struct of_device_id i2c_slave_mqueue_of_match[] = {
> + {
> + .compatible = "i2c-slave-mqueue",
> + },
> + { },
No need for comma here.
> +};
> +
> +static struct i2c_driver i2c_slave_mqueue_driver = {
> + .driver = {
> + .name = "i2c-slave-mqueue",
> + .of_match_table = of_match_ptr(i2c_slave_mqueue_of_match),
Wouldn't compiler warn you due to unused data?
Perhaps drop of_match_ptr() for good...
> + },
> + .probe = i2c_slave_mqueue_probe,
> + .remove = i2c_slave_mqueue_remove,
> + .id_table = i2c_slave_mqueue_id,
> +};
--
With Best Regards,
Andy Shevchenko
Hey Andry,
Long time no seeing :-)
On Tue, Jun 04, 2019 at 08:16:11PM +0300, Andy Shevchenko wrote:
> On Thu, May 30, 2019 at 09:33:46PM -0700, Eduardo Valentin wrote:
> > From: Haiyue Wang <[email protected]>
> >
> > Some protocols over I2C are designed for bi-directional transferring
> > messages by using I2C Master Write protocol. Like the MCTP (Management
> > Component Transport Protocol) and IPMB (Intelligent Platform Management
> > Bus), they both require that the userspace can receive messages from
> > I2C dirvers under slave mode.
> >
> > This new slave mqueue backend is used to receive and queue messages, it
> > will exposes these messages to userspace by sysfs bin file.
> >
> > Note: DT interface and a couple of minor fixes here and there
> > by Eduardo, so I kept the original authorship here.
>
> > +#define MQ_MSGBUF_SIZE CONFIG_I2C_SLAVE_MQUEUE_MESSAGE_SIZE
> > +#define MQ_QUEUE_SIZE CONFIG_I2C_SLAVE_MQUEUE_QUEUE_SIZE
>
> > +#define MQ_QUEUE_NEXT(x) (((x) + 1) & (MQ_QUEUE_SIZE - 1))
>
> Also possible ((x + 1) % ..._SIZE)
Right.. but I suppose the original idea is to avoid divisions on the hotpath.
So, I am actually fine with the limitation of only using power of 2.
>
> > + mq = dev_get_drvdata(container_of(kobj, struct device, kobj));
>
> kobj_to_dev()
Well, yeah, I guess this is a nit, but I can add that in case of a real need for a v7.
>
> > +static int i2c_slave_mqueue_probe(struct i2c_client *client,
> > + const struct i2c_device_id *id)
> > +{
> > + struct device *dev = &client->dev;
> > + struct mq_queue *mq;
> > + int ret, i;
> > + void *buf;
> > +
> > + mq = devm_kzalloc(dev, sizeof(*mq), GFP_KERNEL);
> > + if (!mq)
> > + return -ENOMEM;
> > +
>
> > + BUILD_BUG_ON(!is_power_of_2(MQ_QUEUE_SIZE));
>
> Perhaps start function with this kind of assertions?
>
same here, in case I see a huge ask for a v7, I can move this up.
> > +
> > + buf = devm_kmalloc_array(dev, MQ_QUEUE_SIZE, MQ_MSGBUF_SIZE,
> > + GFP_KERNEL);
> > + if (!buf)
> > + return -ENOMEM;
> > +
> > + for (i = 0; i < MQ_QUEUE_SIZE; i++)
> > + mq->queue[i].buf = buf + i * MQ_MSGBUF_SIZE;
>
>
> Just wondering if kfifo API can bring an advantage here?
>
Well, then again, I suppose the idea is simplify here, not if we need to go
kfifo as the Protocol on top of this is perfectly fine with the current
discipline of just having a simple drop of older messages.
> > + return 0;
> > +}
>
> > +static const struct of_device_id i2c_slave_mqueue_of_match[] = {
> > + {
> > + .compatible = "i2c-slave-mqueue",
> > + },
>
> > + { },
>
> No need for comma here.
It does not hurt to have it either :-)
>
> > +};
>
> > +
> > +static struct i2c_driver i2c_slave_mqueue_driver = {
> > + .driver = {
> > + .name = "i2c-slave-mqueue",
>
> > + .of_match_table = of_match_ptr(i2c_slave_mqueue_of_match),
>
> Wouldn't compiler warn you due to unused data?
> Perhaps drop of_match_ptr() for good...
Not sure what you meant here. I dont see any compiler warning.
Also, of_match_ptr seams to be well spread in the kernel.
>
> > + },
> > + .probe = i2c_slave_mqueue_probe,
> > + .remove = i2c_slave_mqueue_remove,
> > + .id_table = i2c_slave_mqueue_id,
> > +};
>
> --
> With Best Regards,
> Andy Shevchenko
>
>
--
All the best,
Eduardo Valentin
On Wed, Jun 5, 2019 at 6:30 AM Eduardo Valentin <[email protected]> wrote:
>
> Hey Andry,
>
> Long time no seeing :-)
True!
> > > +#define MQ_MSGBUF_SIZE CONFIG_I2C_SLAVE_MQUEUE_MESSAGE_SIZE
> > > +#define MQ_QUEUE_SIZE CONFIG_I2C_SLAVE_MQUEUE_QUEUE_SIZE
> >
> > > +#define MQ_QUEUE_NEXT(x) (((x) + 1) & (MQ_QUEUE_SIZE - 1))
> >
> > Also possible ((x + 1) % ..._SIZE)
>
> Right.. but I suppose the original idea is to avoid divisions on the hotpath.
>
> So, I am actually fine with the limitation of only using power of 2.
The original code implies that anyway, so, my proposal doesn't
restrict it any farther.
> > > + {
> > > + .compatible = "i2c-slave-mqueue",
> > > + },
> >
> > > + { },
> >
> > No need for comma here.
>
> It does not hurt to have it either :-)
It's just a protection against some weird cases of adding entries
behind the terminator.
> > > + .of_match_table = of_match_ptr(i2c_slave_mqueue_of_match),
> >
> > Wouldn't compiler warn you due to unused data?
> > Perhaps drop of_match_ptr() for good...
>
>
> Not sure what you meant here. I dont see any compiler warning.
> Also, of_match_ptr seams to be well spread in the kernel.
If this will be compiled with CONFIG_OF=n...
Though I didn't check all dependencies to see if it even possible. In
any case of_match_ptr() is redundant in both cases here.
Either you need to protect i2c_slave_mqueue_of_match with #ifdef
CONFIG_OF, or drop the macro use.
P.S. Taking into account the last part, I would wait for v7 with that
fixed followed by fixing other nits.
--
With Best Regards,
Andy Shevchenko
On Wed, Jun 05, 2019 at 11:25:39AM +0300, Andy Shevchenko wrote:
> On Wed, Jun 5, 2019 at 6:30 AM Eduardo Valentin <[email protected]> wrote:
> >
> > Hey Andry,
> >
> > Long time no seeing :-)
>
> True!
>
>
> > > > +#define MQ_MSGBUF_SIZE CONFIG_I2C_SLAVE_MQUEUE_MESSAGE_SIZE
> > > > +#define MQ_QUEUE_SIZE CONFIG_I2C_SLAVE_MQUEUE_QUEUE_SIZE
> > >
> > > > +#define MQ_QUEUE_NEXT(x) (((x) + 1) & (MQ_QUEUE_SIZE - 1))
> > >
> > > Also possible ((x + 1) % ..._SIZE)
> >
> > Right.. but I suppose the original idea is to avoid divisions on the hotpath.
> >
> > So, I am actually fine with the limitation of only using power of 2.
>
> The original code implies that anyway, so, my proposal doesn't
> restrict it any farther.
Well, yes, but the point is you would be switching from a simple AND (&) operation
to a division...
I am keeping the power of 2 dep so that we can keep this with a simple &.
> > > > + {
> > > > + .compatible = "i2c-slave-mqueue",
> > > > + },
> > >
> > > > + { },
> > >
> > > No need for comma here.
> >
> > It does not hurt to have it either :-)
>
> It's just a protection against some weird cases of adding entries
> behind the terminator.
Fair..
>
> > > > + .of_match_table = of_match_ptr(i2c_slave_mqueue_of_match),
> > >
> > > Wouldn't compiler warn you due to unused data?
> > > Perhaps drop of_match_ptr() for good...
> >
> >
> > Not sure what you meant here. I dont see any compiler warning.
> > Also, of_match_ptr seams to be well spread in the kernel.
>
> If this will be compiled with CONFIG_OF=n...
I see.. I obviously did not test with that config..
> Though I didn't check all dependencies to see if it even possible. In
> any case of_match_ptr() is redundant in both cases here.
> Either you need to protect i2c_slave_mqueue_of_match with #ifdef
> CONFIG_OF, or drop the macro use.
I will wrap it into CONFIG_OF..
>
> P.S. Taking into account the last part, I would wait for v7 with that
> fixed followed by fixing other nits.
I agree, the warn on CONFIG_OF=n is enough to spin out an extra version.
I will include the other nits too.
>
> --
> With Best Regards,
> Andy Shevchenko
--
All the best,
Eduardo Valentin
On Wed, Jun 5, 2019 at 5:32 PM Eduardo Valentin <[email protected]> wrote:
> On Wed, Jun 05, 2019 at 11:25:39AM +0300, Andy Shevchenko wrote:
> > On Wed, Jun 5, 2019 at 6:30 AM Eduardo Valentin <[email protected]> wrote:
> Well, yes, but the point is you would be switching from a simple AND (&) operation
> to a division...
>
> I am keeping the power of 2 dep so that we can keep this with a simple &.
Works for me.
> > > > > + .of_match_table = of_match_ptr(i2c_slave_mqueue_of_match),
> > > >
> > > > Wouldn't compiler warn you due to unused data?
> > > > Perhaps drop of_match_ptr() for good...
> > >
> > > Not sure what you meant here. I dont see any compiler warning.
> > > Also, of_match_ptr seams to be well spread in the kernel.
> >
> > If this will be compiled with CONFIG_OF=n...
>
> I see.. I obviously did not test with that config..
>
> > Though I didn't check all dependencies to see if it even possible. In
> > any case of_match_ptr() is redundant in both cases here.
> > Either you need to protect i2c_slave_mqueue_of_match with #ifdef
> > CONFIG_OF, or drop the macro use.
>
> I will wrap it into CONFIG_OF..
Would be this expected to work in the case of CONFIG_OF=n?
If no, why to introduce ugly #ifdef:s and additional macros?
Wouldn't be better to have
depends on OF || COMPILE_TEST
?
--
With Best Regards,
Andy Shevchenko
On Wed, Jun 05, 2019 at 06:20:37PM +0300, Andy Shevchenko wrote:
> On Wed, Jun 5, 2019 at 5:32 PM Eduardo Valentin <[email protected]> wrote:
> > On Wed, Jun 05, 2019 at 11:25:39AM +0300, Andy Shevchenko wrote:
> > > On Wed, Jun 5, 2019 at 6:30 AM Eduardo Valentin <[email protected]> wrote:
>
> > Well, yes, but the point is you would be switching from a simple AND (&) operation
> > to a division...
> >
> > I am keeping the power of 2 dep so that we can keep this with a simple &.
>
> Works for me.
>
> > > > > > + .of_match_table = of_match_ptr(i2c_slave_mqueue_of_match),
> > > > >
> > > > > Wouldn't compiler warn you due to unused data?
> > > > > Perhaps drop of_match_ptr() for good...
> > > >
> > > > Not sure what you meant here. I dont see any compiler warning.
> > > > Also, of_match_ptr seams to be well spread in the kernel.
> > >
> > > If this will be compiled with CONFIG_OF=n...
> >
> > I see.. I obviously did not test with that config..
> >
> > > Though I didn't check all dependencies to see if it even possible. In
> > > any case of_match_ptr() is redundant in both cases here.
> > > Either you need to protect i2c_slave_mqueue_of_match with #ifdef
> > > CONFIG_OF, or drop the macro use.
> >
> > I will wrap it into CONFIG_OF..
>
> Would be this expected to work in the case of CONFIG_OF=n?
> If no, why to introduce ugly #ifdef:s and additional macros?
I do hate those too...
> Wouldn't be better to have
> depends on OF || COMPILE_TEST
Well, technically, the original author had a case for using this
without CONFIG_OF. That is why I did not force here to be a strong
dependency on CONFIG_OF. So, I guess in this case the driver will
work properly in both cases if we:
+#ifdef CONFIG_OF
+static const struct of_device_id i2c_slave_mqueue_of_match[] = {
+ {
+ .compatible = "i2c-slave-mqueue",
+ },
+ { /* sentinel */ }
+};
+MODULE_DEVICE_TABLE(of, i2c_slave_mqueue_of_match);
+#endif
+
+static struct i2c_driver i2c_slave_mqueue_driver = {
+ .driver = {
+ .name = "i2c-slave-mqueue",
+ .of_match_table = of_match_ptr(i2c_slave_mqueue_of_match),
+ },
+ .probe = i2c_slave_mqueue_probe,
+ .remove = i2c_slave_mqueue_remove,
+ .id_table = i2c_slave_mqueue_id,
+};
The above is a well stablish pattern across the drivers.
> ?
>
> --
> With Best Regards,
> Andy Shevchenko
--
All the best,
Eduardo Valentin
On Wed, Jun 5, 2019 at 6:31 PM Eduardo Valentin <[email protected]> wrote:
> On Wed, Jun 05, 2019 at 06:20:37PM +0300, Andy Shevchenko wrote:
> > On Wed, Jun 5, 2019 at 5:32 PM Eduardo Valentin <[email protected]> wrote:
> > > On Wed, Jun 05, 2019 at 11:25:39AM +0300, Andy Shevchenko wrote:
> > > > On Wed, Jun 5, 2019 at 6:30 AM Eduardo Valentin <[email protected]> wrote:
> > > > > > > + .of_match_table = of_match_ptr(i2c_slave_mqueue_of_match),
> > > > > >
> > > > > > Wouldn't compiler warn you due to unused data?
> > > > > > Perhaps drop of_match_ptr() for good...
> > > > >
> > > > > Not sure what you meant here. I dont see any compiler warning.
> > > > > Also, of_match_ptr seams to be well spread in the kernel.
> > > >
> > > > If this will be compiled with CONFIG_OF=n...
> > >
> > > I see.. I obviously did not test with that config..
> > >
> > > > Though I didn't check all dependencies to see if it even possible. In
> > > > any case of_match_ptr() is redundant in both cases here.
> > > > Either you need to protect i2c_slave_mqueue_of_match with #ifdef
> > > > CONFIG_OF, or drop the macro use.
> > >
> > > I will wrap it into CONFIG_OF..
> >
> > Would be this expected to work in the case of CONFIG_OF=n?
> > If no, why to introduce ugly #ifdef:s and additional macros?
>
> I do hate those too...
>
> > Wouldn't be better to have
> > depends on OF || COMPILE_TEST
>
> Well, technically, the original author had a case for using this
> without CONFIG_OF. That is why I did not force here to be a strong
> dependency on CONFIG_OF. So, I guess in this case the driver will
> work properly in both cases if we:
>
> +#ifdef CONFIG_OF
> +static const struct of_device_id i2c_slave_mqueue_of_match[] = {
> + {
> + .compatible = "i2c-slave-mqueue",
> + },
> + { /* sentinel */ }
> +};
> +MODULE_DEVICE_TABLE(of, i2c_slave_mqueue_of_match);
> +#endif
> +
> +static struct i2c_driver i2c_slave_mqueue_driver = {
> + .driver = {
> + .name = "i2c-slave-mqueue",
> + .of_match_table = of_match_ptr(i2c_slave_mqueue_of_match),
> + },
> + .probe = i2c_slave_mqueue_probe,
> + .remove = i2c_slave_mqueue_remove,
> + .id_table = i2c_slave_mqueue_id,
> +};
>
> The above is a well stablish pattern across the drivers.
My point here that you may achieve the same by simple dropping of_match_ptr().
P.S. Many of the drivers just old enough and not being simplified due
to pointless churn there, but for new drivers we may avoid it.
--
With Best Regards,
Andy Shevchenko