From: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
To: Boris Brezillon <boris.brezillon@free-electrons.com>
Cc: Wolfram Sang <wsa@the-dreams.de>,
linux-i2c@vger.kernel.org, Jonathan Corbet <corbet@lwn.net>,
linux-doc@vger.kernel.org, Arnd Bergmann <arnd@arndb.de>,
Przemyslaw Sroka <psroka@cadence.com>,
Arkadiusz Golec <agolec@cadence.com>,
Alan Douglas <adouglas@cadence.com>,
Bartosz Folta <bfolta@cadence.com>, Damian Kos <dkos@cadence.com>,
Alicja Jurasik-Urbaniak <alicja@cadence.com>,
Jan Kotas <jank@cadence.com>,
Cyprian Wronka <cwronka@cadence.com>,
Alexandre Belloni <alexandre.belloni@free-electrons.com>,
Thomas Petazzoni <thomas.petazzoni@free-electrons.com>,
Nishanth Menon <nm@ti.com>, Rob Herring <robh+dt@kernel.org>,
Pawel Moll <pawel.moll@arm.com>,
Mark Rutland <mark.rutland@arm.com>,
Ian Campbell <ijc+devicetree@hellion.org.uk>,
Kumar Gala <galak@co>
Subject: Re: [RFC 2/5] i3c: Add core I3C infrastructure
Date: Mon, 31 Jul 2017 18:40:21 -0700 [thread overview]
Message-ID: <20170801014021.GA20004@kroah.com> (raw)
In-Reply-To: <1501518290-5723-3-git-send-email-boris.brezillon@free-electrons.com>
On Mon, Jul 31, 2017 at 06:24:47PM +0200, Boris Brezillon wrote:
> Add core infrastructure to support I3C in Linux and document it.
>
> This infrastructure is not complete yet and will be extended over
> time.
>
> There are a few design choices that are worth mentioning because they
> impact the way I3C device drivers can interact with their devices:
>
> - all functions used to send I3C/I2C frames must be called in
> non-atomic context. Mainly done this way to ease implementation, but
> this is still open to discussion. Please let me know if you think it's
> worth considering an asynchronous model here
> - the bus element is a separate object and is not implicitly described
> by the master (as done in I2C). The reason is that I want to be able
> to handle multiple master connected to the same bus and visible to
> Linux.
> In this situation, we should only have one instance of the device and
> not one per master, and sharing the bus object would be part of the
> solution to gracefully handle this case.
> I'm not sure we will ever need to deal with multiple masters
> controlling the same bus and exposed under Linux, but separating the
> bus and master concept is pretty easy, hence the decision to do it
> like that.
> The other benefit of separating the bus and master concepts is that
> master devices appear under the bus directory in sysfs.
> - I2C backward compatibility has been designed to be transparent to I2C
> drivers and the I2C subsystem. The I3C master just registers an I2C
> adapter which creates a new I2C bus. I'd say that, from a
> representation PoV it's not ideal because what should appear as a
> single I3C bus exposing I3C and I2C devices here appears as 2
> different busses connected to each other through the parenting (the
> I3C master is the parent of the I2C and I3C busses).
> On the other hand, I don't see a better solution if we want something
> that is not invasive.
> - the whole API is exposed through a single header file (i3c.h), but I'm
> seriously considering the option of splitting the I3C driver/user API
> and the I3C master one, mainly to hide I3C core internals and restrict
> what I3C users can do to a limited set of functionalities (send
> I3C/I2C frames to a specific device and that's all).
>
> Missing features in this preliminary version:
> - no support for IBI (In Band Interrupts). This is something I'm working
> on, and I'm still unsure how to represent it: an irqchip or a
> completely independent representation that would be I3C specific.
> Right now, I'm more inclined to go for the irqchip approach, since
> this is something people are used to deal with already.
> - no Hot Join support, which is similar to hotplug
> - no support for multi-master and the associated concepts (mastership
> handover, support for secondary masters, ...)
> - I2C devices can only be described using DT because this is the only
> use case I have. However, the framework can easily be extended with
> ACPI and board info support
> - I3C slave framework. This has been completely omitted, but shouldn't
> have a huge impact on the I3C framework because I3C slaves don't see
> the whole bus, it's only about handling master requests and generating
> IBIs. Some of the struct, constant and enum definitions could be
> shared, but most of the I3C slave framework logic will be different
>
> Signed-off-by: Boris Brezillon <boris.brezillon@free-electrons.com>
> ---
> Documentation/i3c/conf.py | 10 +
> Documentation/i3c/device-driver-api.rst | 7 +
> Documentation/i3c/index.rst | 9 +
> Documentation/i3c/master-driver-api.rst | 8 +
> Documentation/i3c/protocol.rst | 199 +++++
> Documentation/index.rst | 1 +
> drivers/Kconfig | 2 +
> drivers/Makefile | 2 +-
> drivers/i3c/Kconfig | 24 +
> drivers/i3c/Makefile | 3 +
> drivers/i3c/core.c | 532 ++++++++++++++
> drivers/i3c/device.c | 138 ++++
> drivers/i3c/internals.h | 45 ++
> drivers/i3c/master.c | 1225 +++++++++++++++++++++++++++++++
> drivers/i3c/master/Kconfig | 0
> drivers/i3c/master/Makefile | 0
> include/linux/i3c/ccc.h | 389 ++++++++++
> include/linux/i3c/device.h | 212 ++++++
> include/linux/i3c/master.h | 453 ++++++++++++
> include/linux/mod_devicetable.h | 15 +
> 20 files changed, 3273 insertions(+), 1 deletion(-)
Any chance you can break the documentation out from this patch to make
it smaller and a bit simpler to review?
Here's a few random review comments, I have only glanced at this, not
done any real reading of this at all...
> +menu "I3C support"
> +
> +config I3C
> + tristate "I3C support"
> + ---help---
> + I3C (pronounce: I-cube-C) is a serial protocol standardized by the
> + MIPI alliance.
> +
> + It's supposed to be backward compatible with I2C while providing
> + support for high speed transfers and native interrupt support
> + without the need for extra pins.
> +
> + The I3C protocol also standardizes the slave device types and is
> + mainly design to communicate with sensors.
> +
> + If you want I3C support, you should say Y here and also to the
> + specific driver for your bus adapter(s) below.
> +
> + This I3C support can also be built as a module. If so, the module
> + will be called i3c.
> +
> +source "drivers/i3c/master/Kconfig"
Don't source unless i3c is enabled, right?
> +
> +endmenu
> diff --git a/drivers/i3c/Makefile b/drivers/i3c/Makefile
> new file mode 100644
> index 000000000000..0605a275f47b
> --- /dev/null
> +++ b/drivers/i3c/Makefile
> @@ -0,0 +1,3 @@
> +i3c-y := core.o device.o master.o
> +obj-$(CONFIG_I3C) += i3c.o
> +obj-$(CONFIG_I3C) += master/
> diff --git a/drivers/i3c/core.c b/drivers/i3c/core.c
> new file mode 100644
> index 000000000000..c000fb458547
> --- /dev/null
> +++ b/drivers/i3c/core.c
> @@ -0,0 +1,532 @@
> +/*
> + * Copyright (C) 2017 Cadence Design Systems Inc.
> + *
> + * Author: Boris Brezillon <boris.brezillon@free-electrons.com>
> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms of the GNU General Public License as published by the Free
> + * Software Foundation; either version 2 of the License, or (at your option)
> + * any later version.
I have to ask, do you really mean "or any later version"?
> +static DEFINE_IDR(i3c_bus_idr);
You never clean this up when the module goes away :(
> +static DEFINE_MUTEX(i3c_core_lock);
> +
> +void i3c_bus_lock(struct i3c_bus *bus, bool exclusive)
> +{
> + if (exclusive)
> + down_write(&bus->lock);
> + else
> + down_read(&bus->lock);
> +}
The "exclusive" flag is odd, and messy, and hard to understand, don't
you agree? And have you measured the difference in using a rw lock over
a normal mutex and found it to be faster? If not, just use a normal
mutex, it's simpler and almost always better in the end.
> +
> +void i3c_bus_unlock(struct i3c_bus *bus, bool exclusive)
> +{
> + if (exclusive)
> + up_write(&bus->lock);
> + else
> + up_read(&bus->lock);
> +}
> +
> +static ssize_t bcr_show(struct device *dev,
> + struct device_attribute *da,
> + char *buf)
> +{
> + struct i3c_device *i3cdev = dev_to_i3cdev(dev);
> + struct i3c_bus *bus = i3c_device_get_bus(i3cdev);
> + ssize_t ret;
> +
> + i3c_bus_lock(bus, false);
> + ret = sprintf(buf, "%x\n", i3cdev->info.bcr);
> + i3c_bus_unlock(bus, false);
> +
> + return ret;
> +}
> +static DEVICE_ATTR_RO(bcr);
> +
> +static ssize_t dcr_show(struct device *dev,
> + struct device_attribute *da,
> + char *buf)
> +{
> + struct i3c_device *i3cdev = dev_to_i3cdev(dev);
> + struct i3c_bus *bus = i3c_device_get_bus(i3cdev);
> + ssize_t ret;
> +
> + i3c_bus_lock(bus, false);
> + ret = sprintf(buf, "%x\n", i3cdev->info.dcr);
> + i3c_bus_unlock(bus, false);
> +
> + return ret;
> +}
> +static DEVICE_ATTR_RO(dcr);
> +
> +static ssize_t pid_show(struct device *dev,
> + struct device_attribute *da,
> + char *buf)
> +{
> + struct i3c_device *i3cdev = dev_to_i3cdev(dev);
> + struct i3c_bus *bus = i3c_device_get_bus(i3cdev);
> + ssize_t ret;
> +
> + i3c_bus_lock(bus, false);
> + ret = sprintf(buf, "%llx\n", i3cdev->info.pid);
> + i3c_bus_unlock(bus, false);
> +
> + return ret;
> +}
> +static DEVICE_ATTR_RO(pid);
No Documentation/ABI entries for all of these sysfs files?
> +
> +static ssize_t address_show(struct device *dev,
> + struct device_attribute *da,
> + char *buf)
> +{
> + struct i3c_device *i3cdev = dev_to_i3cdev(dev);
> + struct i3c_bus *bus = i3c_device_get_bus(i3cdev);
> + ssize_t ret;
> +
> + i3c_bus_lock(bus, false);
> + ret = sprintf(buf, "%02x\n", i3cdev->info.dyn_addr);
> + i3c_bus_unlock(bus, false);
> +
> + return ret;
> +}
> +static DEVICE_ATTR_RO(address);
> +
> +static const char * const hdrcap_strings[] = {
> + "hdr-ddr", "hdr-tsp", "hdr-tsl",
> +};
> +
> +static ssize_t hdrcap_show(struct device *dev,
> + struct device_attribute *da,
> + char *buf)
> +{
> + struct i3c_device *i3cdev = dev_to_i3cdev(dev);
> + struct i3c_bus *bus = i3c_device_get_bus(i3cdev);
> + unsigned long caps = i3cdev->info.hdr_cap;
> + ssize_t offset = 0, ret;
> + int mode;
> +
> + i3c_bus_lock(bus, false);
> + for_each_set_bit(mode, &caps, 8) {
> + if (mode >= ARRAY_SIZE(hdrcap_strings))
> + break;
> +
> + if (!hdrcap_strings[mode])
> + continue;
> +
> + ret = sprintf(buf + offset, "%s\n", hdrcap_strings[mode]);
Multiple lines in a single sysfs file? No.
> + if (ret < 0)
> + goto out;
> +
> + offset += ret;
> + }
> + ret = offset;
> +
> +out:
> + i3c_bus_unlock(bus, false);
> +
> + return ret;
> +}
> +static DEVICE_ATTR_RO(hdrcap);
> +
> +static struct attribute *i3c_device_attrs[] = {
> + &dev_attr_bcr.attr,
> + &dev_attr_dcr.attr,
> + &dev_attr_pid.attr,
> + &dev_attr_address.attr,
> + &dev_attr_hdrcap.attr,
> + NULL,
> +};
> +
> +static const struct attribute_group i3c_device_group = {
> + .attrs = i3c_device_attrs,
> +};
> +
> +static const struct attribute_group *i3c_device_groups[] = {
> + &i3c_device_group,
> + NULL,
> +};
ATTRIBUTE_GROUPS()?
> +
> +static int i3c_device_uevent(struct device *dev, struct kobj_uevent_env *env)
> +{
> + struct i3c_device *i3cdev = dev_to_i3cdev(dev);
> + u16 manuf = I3C_PID_MANUF_ID(i3cdev->info.pid);
> + u16 part = I3C_PID_PART_ID(i3cdev->info.pid);
> + u16 ext = I3C_PID_EXTRA_INFO(i3cdev->info.pid);
> +
> + if (I3C_PID_RND_LOWER_32BITS(i3cdev->info.pid))
> + return add_uevent_var(env, "MODALIAS=i3c:dcr%02Xmanuf%04X",
> + i3cdev->info.dcr, manuf);
> +
> + return add_uevent_var(env,
> + "MODALIAS=i3c:dcr%02Xmanuf%04Xpart%04xext%04x",
> + i3cdev->info.dcr, manuf, part, ext);
> +}
> +
> +const struct device_type i3c_device_type = {
> + .groups = i3c_device_groups,
> + .uevent = i3c_device_uevent,
> +};
No release type? Oh that's bad bad bad and implies you have never
removed a device from your system as the kernel would have complained
loudly at you.
> +
> +static const struct attribute_group *i3c_master_groups[] = {
> + &i3c_device_group,
> + NULL,
> +};
ATTRIBUTE_GROUPS()?
> +
> +const struct device_type i3c_master_type = {
> + .groups = i3c_master_groups,
> +};
> +
> +static const char * const i3c_bus_mode_strings[] = {
> + [I3C_BUS_MODE_PURE] = "pure",
> + [I3C_BUS_MODE_MIXED_FAST] = "mixed-fast",
> + [I3C_BUS_MODE_MIXED_SLOW] = "mixed-slow",
> +};
> +
> +static ssize_t mode_show(struct device *dev,
> + struct device_attribute *da,
> + char *buf)
> +{
> + struct i3c_bus *i3cbus = container_of(dev, struct i3c_bus, dev);
> + ssize_t ret;
> +
> + i3c_bus_lock(i3cbus, false);
> + if (i3cbus->mode < 0 ||
> + i3cbus->mode > ARRAY_SIZE(i3c_bus_mode_strings) ||
> + !i3c_bus_mode_strings[i3cbus->mode])
> + ret = sprintf(buf, "unknown\n");
> + else
> + ret = sprintf(buf, "%s\n", i3c_bus_mode_strings[i3cbus->mode]);
> + i3c_bus_unlock(i3cbus, false);
> +
> + return ret;
> +}
> +static DEVICE_ATTR_RO(mode);
> +
> +static ssize_t current_master_show(struct device *dev,
> + struct device_attribute *da,
> + char *buf)
> +{
> + struct i3c_bus *i3cbus = container_of(dev, struct i3c_bus, dev);
> + ssize_t ret;
> +
> + i3c_bus_lock(i3cbus, false);
> + ret = sprintf(buf, "%s\n", dev_name(&i3cbus->cur_master->dev));
> + i3c_bus_unlock(i3cbus, false);
> +
> + return ret;
> +}
> +static DEVICE_ATTR_RO(current_master);
> +
> +static ssize_t i3c_scl_frequency_show(struct device *dev,
> + struct device_attribute *da,
> + char *buf)
> +{
> + struct i3c_bus *i3cbus = container_of(dev, struct i3c_bus, dev);
> + ssize_t ret;
> +
> + i3c_bus_lock(i3cbus, false);
> + ret = sprintf(buf, "%ld\n", i3cbus->scl_rate.i3c);
> + i3c_bus_unlock(i3cbus, false);
> +
> + return ret;
> +}
> +static DEVICE_ATTR_RO(i3c_scl_frequency);
> +
> +static ssize_t i2c_scl_frequency_show(struct device *dev,
> + struct device_attribute *da,
> + char *buf)
> +{
> + struct i3c_bus *i3cbus = container_of(dev, struct i3c_bus, dev);
> + ssize_t ret;
> +
> + i3c_bus_lock(i3cbus, false);
> + ret = sprintf(buf, "%ld\n", i3cbus->scl_rate.i2c);
> + i3c_bus_unlock(i3cbus, false);
> +
> + return ret;
> +}
> +static DEVICE_ATTR_RO(i2c_scl_frequency);
> +
> +static struct attribute *i3c_busdev_attrs[] = {
> + &dev_attr_mode.attr,
> + &dev_attr_current_master.attr,
> + &dev_attr_i3c_scl_frequency.attr,
> + &dev_attr_i2c_scl_frequency.attr,
> + NULL,
> +};
> +ATTRIBUTE_GROUPS(i3c_busdev);
Yeah, you used it here!
that's all the time I have right now...
thanks,
greg k-h
next prev parent reply other threads:[~2017-08-01 1:40 UTC|newest]
Thread overview: 48+ messages / expand[flat|nested] mbox.gz Atom feed top
2017-07-31 16:24 [RFC 0/5] Add I3C subsystem Boris Brezillon
2017-07-31 16:24 ` [RFC 1/5] i2c: Export of_i2c_get_board_info() Boris Brezillon
2017-07-31 16:24 ` [RFC 2/5] i3c: Add core I3C infrastructure Boris Brezillon
2017-07-31 19:17 ` Wolfram Sang
2017-07-31 20:46 ` Boris Brezillon
2017-07-31 20:16 ` Arnd Bergmann
[not found] ` <CAK8P3a06GoMdKdn=3Cq0FUwYnjGX0oG+FQLjxfiasVDpbonWRw-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
2017-07-31 21:15 ` Boris Brezillon
2017-07-31 21:32 ` Peter Rosin
2017-07-31 21:42 ` Wolfram Sang
2017-08-01 16:47 ` Andrew F. Davis
2017-08-01 17:27 ` Wolfram Sang
2017-08-01 21:47 ` Boris Brezillon
2017-08-02 10:21 ` Wolfram Sang
2017-08-01 12:00 ` Arnd Bergmann
2017-08-01 12:29 ` Boris Brezillon
2017-08-01 13:11 ` Arnd Bergmann
2017-08-01 13:34 ` Boris Brezillon
2017-08-01 13:58 ` Boris Brezillon
2017-08-01 14:22 ` Arnd Bergmann
2017-08-01 15:14 ` Boris Brezillon
2017-08-01 20:16 ` Arnd Bergmann
2017-08-01 14:12 ` Wolfram Sang
2017-08-01 14:48 ` Boris Brezillon
2017-08-01 15:01 ` Wolfram Sang
2017-08-01 15:20 ` Boris Brezillon
2017-08-03 8:03 ` Boris Brezillon
2017-08-16 21:03 ` Geert Uytterhoeven
2017-08-17 7:48 ` Boris Brezillon
2017-08-01 1:40 ` Greg Kroah-Hartman [this message]
[not found] ` <20170801014021.GA20004-U8xfFu+wG4EAvxtiuMwx3w@public.gmane.org>
2017-08-01 10:48 ` Boris Brezillon
2017-08-01 17:51 ` Greg Kroah-Hartman
2017-08-01 21:30 ` Boris Brezillon
2017-08-02 0:54 ` Greg Kroah-Hartman
2017-08-02 2:13 ` Greg Kroah-Hartman
[not found] ` <20170802021327.GB23033-U8xfFu+wG4EAvxtiuMwx3w@public.gmane.org>
2017-12-13 16:20 ` Boris Brezillon
2017-12-13 16:51 ` Greg Kroah-Hartman
2017-08-17 9:03 ` Linus Walleij
2017-08-17 9:28 ` Boris Brezillon
[not found] ` <1501518290-5723-1-git-send-email-boris.brezillon-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
2017-07-31 16:24 ` [RFC 3/5] dt-bindings: i3c: Document core bindings Boris Brezillon
2017-08-09 23:43 ` Rob Herring
2017-08-10 8:49 ` Boris Brezillon
2017-07-31 16:24 ` [RFC 4/5] i3c: master: Add driver for Cadence IP Boris Brezillon
2017-07-31 16:24 ` [RFC 5/5] dt-bindings: i3c: Document Cadence I3C master bindings Boris Brezillon
2017-07-31 19:17 ` [RFC 0/5] Add I3C subsystem Wolfram Sang
2017-07-31 20:40 ` Boris Brezillon
2017-07-31 20:47 ` Wolfram Sang
2017-12-12 19:58 ` Boris Brezillon
2017-12-12 22:01 ` Wolfram Sang
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20170801014021.GA20004@kroah.com \
--to=gregkh@linuxfoundation.org \
--cc=adouglas@cadence.com \
--cc=agolec@cadence.com \
--cc=alexandre.belloni@free-electrons.com \
--cc=alicja@cadence.com \
--cc=arnd@arndb.de \
--cc=bfolta@cadence.com \
--cc=boris.brezillon@free-electrons.com \
--cc=corbet@lwn.net \
--cc=cwronka@cadence.com \
--cc=dkos@cadence.com \
--cc=galak@co \
--cc=ijc+devicetree@hellion.org.uk \
--cc=jank@cadence.com \
--cc=linux-doc@vger.kernel.org \
--cc=linux-i2c@vger.kernel.org \
--cc=mark.rutland@arm.com \
--cc=nm@ti.com \
--cc=pawel.moll@arm.com \
--cc=psroka@cadence.com \
--cc=robh+dt@kernel.org \
--cc=thomas.petazzoni@free-electrons.com \
--cc=wsa@the-dreams.de \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).