* [PATCH v7 08/12] iio: multiplexer: new iio category and iio-mux driver
From: Peter Rosin @ 2017-01-04 12:16 UTC (permalink / raw)
To: linux-kernel
Cc: Peter Rosin, Wolfram Sang, Rob Herring, Mark Rutland,
Jonathan Cameron, Hartmut Knaack, Lars-Peter Clausen,
Peter Meerwald-Stadler, Jonathan Corbet, Arnd Bergmann,
Greg Kroah-Hartman, linux-i2c, devicetree, linux-iio, linux-doc
In-Reply-To: <1483532187-28494-1-git-send-email-peda@axentia.se>
When a multiplexer changes how an iio device behaves (for example
by feeding different signals to an ADC), this driver can be used
to create one virtual iio channel for each multiplexer state.
Depends on the generic multiplexer subsystem.
Cache any ext_info values from the parent iio channel, creating a private
copy of the ext_info attributes for each multiplexer state/channel.
Reviewed-by: Jonathan Cameron <jic23@kernel.org>
Signed-off-by: Peter Rosin <peda@axentia.se>
---
MAINTAINERS | 1 +
drivers/iio/Kconfig | 1 +
drivers/iio/Makefile | 1 +
drivers/iio/multiplexer/Kconfig | 18 ++
drivers/iio/multiplexer/Makefile | 6 +
drivers/iio/multiplexer/iio-mux.c | 456 ++++++++++++++++++++++++++++++++++++++
6 files changed, 483 insertions(+)
create mode 100644 drivers/iio/multiplexer/Kconfig
create mode 100644 drivers/iio/multiplexer/Makefile
create mode 100644 drivers/iio/multiplexer/iio-mux.c
diff --git a/MAINTAINERS b/MAINTAINERS
index 91775f2be209..ff28ae01ebd1 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -6279,6 +6279,7 @@ M: Peter Rosin <peda@axentia.se>
L: linux-iio@vger.kernel.org
S: Maintained
F: Documentation/devicetree/bindings/iio/multiplexer/iio-mux.txt
+F: drivers/iio/multiplexer/iio-mux.c
IIO SUBSYSTEM AND DRIVERS
M: Jonathan Cameron <jic23@kernel.org>
diff --git a/drivers/iio/Kconfig b/drivers/iio/Kconfig
index a918270d6f54..b3c8c6ef0dff 100644
--- a/drivers/iio/Kconfig
+++ b/drivers/iio/Kconfig
@@ -83,6 +83,7 @@ source "drivers/iio/humidity/Kconfig"
source "drivers/iio/imu/Kconfig"
source "drivers/iio/light/Kconfig"
source "drivers/iio/magnetometer/Kconfig"
+source "drivers/iio/multiplexer/Kconfig"
source "drivers/iio/orientation/Kconfig"
if IIO_TRIGGER
source "drivers/iio/trigger/Kconfig"
diff --git a/drivers/iio/Makefile b/drivers/iio/Makefile
index 33fa4026f92c..93c769cd99bf 100644
--- a/drivers/iio/Makefile
+++ b/drivers/iio/Makefile
@@ -28,6 +28,7 @@ obj-y += humidity/
obj-y += imu/
obj-y += light/
obj-y += magnetometer/
+obj-y += multiplexer/
obj-y += orientation/
obj-y += potentiometer/
obj-y += potentiostat/
diff --git a/drivers/iio/multiplexer/Kconfig b/drivers/iio/multiplexer/Kconfig
new file mode 100644
index 000000000000..70a044510686
--- /dev/null
+++ b/drivers/iio/multiplexer/Kconfig
@@ -0,0 +1,18 @@
+#
+# Multiplexer drivers
+#
+# When adding new entries keep the list in alphabetical order
+
+menu "Multiplexers"
+
+config IIO_MUX
+ tristate "IIO multiplexer driver"
+ select MULTIPLEXER
+ depends on OF
+ help
+ Say yes here to build support for the IIO multiplexer.
+
+ To compile this driver as a module, choose M here: the
+ module will be called iio-mux.
+
+endmenu
diff --git a/drivers/iio/multiplexer/Makefile b/drivers/iio/multiplexer/Makefile
new file mode 100644
index 000000000000..68be3c4abd07
--- /dev/null
+++ b/drivers/iio/multiplexer/Makefile
@@ -0,0 +1,6 @@
+#
+# Makefile for industrial I/O multiplexer drivers
+#
+
+# When adding new entries keep the list in alphabetical order
+obj-$(CONFIG_IIO_MUX) += iio-mux.o
diff --git a/drivers/iio/multiplexer/iio-mux.c b/drivers/iio/multiplexer/iio-mux.c
new file mode 100644
index 000000000000..60054be8962a
--- /dev/null
+++ b/drivers/iio/multiplexer/iio-mux.c
@@ -0,0 +1,456 @@
+/*
+ * IIO multiplexer driver
+ *
+ * Copyright (C) 2016 Axentia Technologies AB
+ *
+ * Author: Peter Rosin <peda@axentia.se>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+
+#include <linux/err.h>
+#include <linux/iio/consumer.h>
+#include <linux/iio/iio.h>
+#include <linux/module.h>
+#include <linux/mutex.h>
+#include <linux/mux.h>
+#include <linux/of.h>
+#include <linux/platform_device.h>
+
+struct mux_ext_info_cache {
+ char *data;
+ size_t size;
+};
+
+struct mux_child {
+ struct mux_ext_info_cache *ext_info_cache;
+};
+
+struct mux {
+ int cached_state;
+ struct mux_control *control;
+ struct iio_channel *parent;
+ struct iio_dev *indio_dev;
+ struct iio_chan_spec *chan;
+ struct iio_chan_spec_ext_info *ext_info;
+ struct mux_child *child;
+};
+
+static int iio_mux_select(struct mux *mux, int idx)
+{
+ struct mux_child *child = &mux->child[idx];
+ struct iio_chan_spec const *chan = &mux->chan[idx];
+ int ret;
+ int i;
+
+ ret = mux_control_select(mux->control, chan->channel);
+ if (ret < 0) {
+ mux->cached_state = -1;
+ return ret;
+ }
+
+ if (mux->cached_state == chan->channel)
+ return 0;
+
+ if (chan->ext_info) {
+ for (i = 0; chan->ext_info[i].name; ++i) {
+ const char *attr = chan->ext_info[i].name;
+ struct mux_ext_info_cache *cache;
+
+ cache = &child->ext_info_cache[i];
+
+ if (cache->size < 0)
+ continue;
+
+ ret = iio_write_channel_ext_info(mux->parent, attr,
+ cache->data,
+ cache->size);
+
+ if (ret < 0) {
+ mux_control_deselect(mux->control);
+ mux->cached_state = -1;
+ return ret;
+ }
+ }
+ }
+ mux->cached_state = chan->channel;
+
+ return 0;
+}
+
+static void iio_mux_deselect(struct mux *mux)
+{
+ mux_control_deselect(mux->control);
+}
+
+static int mux_read_raw(struct iio_dev *indio_dev,
+ struct iio_chan_spec const *chan,
+ int *val, int *val2, long mask)
+{
+ struct mux *mux = iio_priv(indio_dev);
+ int idx = chan - mux->chan;
+ int ret;
+
+ ret = iio_mux_select(mux, idx);
+ if (ret < 0)
+ return ret;
+
+ switch (mask) {
+ case IIO_CHAN_INFO_RAW:
+ ret = iio_read_channel_raw(mux->parent, val);
+ break;
+
+ case IIO_CHAN_INFO_SCALE:
+ ret = iio_read_channel_scale(mux->parent, val, val2);
+ break;
+
+ default:
+ ret = -EINVAL;
+ }
+
+ iio_mux_deselect(mux);
+
+ return ret;
+}
+
+static int mux_read_avail(struct iio_dev *indio_dev,
+ struct iio_chan_spec const *chan,
+ const int **vals, int *type, int *length,
+ long mask)
+{
+ struct mux *mux = iio_priv(indio_dev);
+ int idx = chan - mux->chan;
+ int ret;
+
+ ret = iio_mux_select(mux, idx);
+ if (ret < 0)
+ return ret;
+
+ switch (mask) {
+ case IIO_CHAN_INFO_RAW:
+ *type = IIO_VAL_INT;
+ ret = iio_read_avail_channel_raw(mux->parent, vals, length);
+ break;
+
+ default:
+ ret = -EINVAL;
+ }
+
+ iio_mux_deselect(mux);
+
+ return ret;
+}
+
+static int mux_write_raw(struct iio_dev *indio_dev,
+ struct iio_chan_spec const *chan,
+ int val, int val2, long mask)
+{
+ struct mux *mux = iio_priv(indio_dev);
+ int idx = chan - mux->chan;
+ int ret;
+
+ ret = iio_mux_select(mux, idx);
+ if (ret < 0)
+ return ret;
+
+ switch (mask) {
+ case IIO_CHAN_INFO_RAW:
+ ret = iio_write_channel_raw(mux->parent, val);
+ break;
+
+ default:
+ ret = -EINVAL;
+ }
+
+ iio_mux_deselect(mux);
+
+ return ret;
+}
+
+static const struct iio_info mux_info = {
+ .read_raw = mux_read_raw,
+ .read_avail = mux_read_avail,
+ .write_raw = mux_write_raw,
+ .driver_module = THIS_MODULE,
+};
+
+static ssize_t mux_read_ext_info(struct iio_dev *indio_dev, uintptr_t private,
+ struct iio_chan_spec const *chan, char *buf)
+{
+ struct mux *mux = iio_priv(indio_dev);
+ int idx = chan - mux->chan;
+ ssize_t ret;
+
+ ret = iio_mux_select(mux, idx);
+ if (ret < 0)
+ return ret;
+
+ ret = iio_read_channel_ext_info(mux->parent,
+ mux->ext_info[private].name,
+ buf);
+
+ iio_mux_deselect(mux);
+
+ return ret;
+}
+
+static ssize_t mux_write_ext_info(struct iio_dev *indio_dev, uintptr_t private,
+ struct iio_chan_spec const *chan,
+ const char *buf, size_t len)
+{
+ struct device *dev = indio_dev->dev.parent;
+ struct mux *mux = iio_priv(indio_dev);
+ int idx = chan - mux->chan;
+ char *new;
+ ssize_t ret;
+
+ ret = iio_mux_select(mux, idx);
+ if (ret < 0)
+ return ret;
+
+ new = devm_kmemdup(dev, buf, len + 1, GFP_KERNEL);
+ if (!new) {
+ iio_mux_deselect(mux);
+ return -ENOMEM;
+ }
+
+ new[len] = 0;
+
+ ret = iio_write_channel_ext_info(mux->parent,
+ mux->ext_info[private].name,
+ buf, len);
+ if (ret < 0) {
+ iio_mux_deselect(mux);
+ devm_kfree(dev, new);
+ return ret;
+ }
+
+ devm_kfree(dev, mux->child[idx].ext_info_cache[private].data);
+ mux->child[idx].ext_info_cache[private].data = new;
+ mux->child[idx].ext_info_cache[private].size = len;
+
+ iio_mux_deselect(mux);
+
+ return ret;
+}
+
+static int mux_configure_channel(struct device *dev, struct mux *mux,
+ u32 state, const char *label, int idx)
+{
+ struct mux_child *child = &mux->child[idx];
+ struct iio_chan_spec *chan = &mux->chan[idx];
+ struct iio_chan_spec const *pchan = mux->parent->channel;
+ char *page = NULL;
+ int num_ext_info;
+ int i;
+ int ret;
+
+ chan->indexed = 1;
+ chan->output = pchan->output;
+ chan->datasheet_name = label;
+ chan->ext_info = mux->ext_info;
+
+ ret = iio_get_channel_type(mux->parent, &chan->type);
+ if (ret < 0) {
+ dev_err(dev, "failed to get parent channel type\n");
+ return ret;
+ }
+
+ if (iio_channel_has_info(pchan, IIO_CHAN_INFO_RAW))
+ chan->info_mask_separate |= BIT(IIO_CHAN_INFO_RAW);
+ if (iio_channel_has_info(pchan, IIO_CHAN_INFO_SCALE))
+ chan->info_mask_separate |= BIT(IIO_CHAN_INFO_SCALE);
+
+ if (iio_channel_has_available(pchan, IIO_CHAN_INFO_RAW))
+ chan->info_mask_separate_available |= BIT(IIO_CHAN_INFO_RAW);
+
+ if (state >= mux->control->states) {
+ dev_err(dev, "too many channels\n");
+ return -EINVAL;
+ }
+
+ chan->channel = state;
+
+ num_ext_info = iio_get_channel_ext_info_count(mux->parent);
+ if (num_ext_info) {
+ page = devm_kzalloc(dev, PAGE_SIZE, GFP_KERNEL);
+ if (!page)
+ return -ENOMEM;
+ }
+ child->ext_info_cache = devm_kzalloc(dev,
+ sizeof(*child->ext_info_cache) *
+ num_ext_info, GFP_KERNEL);
+ for (i = 0; i < num_ext_info; ++i) {
+ child->ext_info_cache[i].size = -1;
+
+ if (!pchan->ext_info[i].write)
+ continue;
+ if (!pchan->ext_info[i].read)
+ continue;
+
+ ret = iio_read_channel_ext_info(mux->parent,
+ mux->ext_info[i].name,
+ page);
+ if (ret < 0) {
+ dev_err(dev, "failed to get ext_info '%s'\n",
+ pchan->ext_info[i].name);
+ return ret;
+ }
+ if (ret >= PAGE_SIZE) {
+ dev_err(dev, "too large ext_info '%s'\n",
+ pchan->ext_info[i].name);
+ return -EINVAL;
+ }
+
+ child->ext_info_cache[i].data = devm_kmemdup(dev, page, ret + 1,
+ GFP_KERNEL);
+ child->ext_info_cache[i].data[ret] = 0;
+ child->ext_info_cache[i].size = ret;
+ }
+
+ if (page)
+ devm_kfree(dev, page);
+
+ return 0;
+}
+
+/*
+ * Same as of_property_for_each_string(), but also keeps track of the
+ * index of each string.
+ */
+#define of_property_for_each_string_index(np, propname, prop, s, i) \
+ for (prop = of_find_property(np, propname, NULL), \
+ s = of_prop_next_string(prop, NULL), \
+ i = 0; \
+ s; \
+ s = of_prop_next_string(prop, s), \
+ i++)
+
+static int mux_probe(struct platform_device *pdev)
+{
+ struct device *dev = &pdev->dev;
+ struct device_node *np = pdev->dev.of_node;
+ struct iio_dev *indio_dev;
+ struct iio_channel *parent;
+ struct mux *mux;
+ struct property *prop;
+ const char *label;
+ u32 state;
+ int sizeof_ext_info;
+ int children;
+ int sizeof_priv;
+ int i;
+ int ret;
+
+ if (!np)
+ return -ENODEV;
+
+ parent = devm_iio_channel_get(dev, "parent");
+ if (IS_ERR(parent)) {
+ if (PTR_ERR(parent) != -EPROBE_DEFER)
+ dev_err(dev, "failed to get parent channel\n");
+ return PTR_ERR(parent);
+ }
+
+ sizeof_ext_info = iio_get_channel_ext_info_count(parent);
+ if (sizeof_ext_info) {
+ sizeof_ext_info += 1; /* one extra entry for the sentinel */
+ sizeof_ext_info *= sizeof(*mux->ext_info);
+ }
+
+ children = 0;
+ of_property_for_each_string(np, "channels", prop, label) {
+ if (*label)
+ children++;
+ }
+ if (children <= 0) {
+ dev_err(dev, "not even a single child\n");
+ return -EINVAL;
+ }
+
+ sizeof_priv = sizeof(*mux);
+ sizeof_priv += sizeof(*mux->child) * children;
+ sizeof_priv += sizeof(*mux->chan) * children;
+ sizeof_priv += sizeof_ext_info;
+
+ indio_dev = devm_iio_device_alloc(dev, sizeof_priv);
+ if (!indio_dev)
+ return -ENOMEM;
+
+ mux = iio_priv(indio_dev);
+ mux->child = (struct mux_child *)(mux + 1);
+ mux->chan = (struct iio_chan_spec *)(mux->child + children);
+
+ platform_set_drvdata(pdev, indio_dev);
+
+ mux->parent = parent;
+ mux->cached_state = -1;
+
+ indio_dev->name = dev_name(dev);
+ indio_dev->dev.parent = dev;
+ indio_dev->info = &mux_info;
+ indio_dev->modes = INDIO_DIRECT_MODE;
+ indio_dev->channels = mux->chan;
+ indio_dev->num_channels = children;
+ if (sizeof_ext_info) {
+ mux->ext_info = devm_kmemdup(dev,
+ parent->channel->ext_info,
+ sizeof_ext_info, GFP_KERNEL);
+ if (!mux->ext_info)
+ return -ENOMEM;
+
+ for (i = 0; mux->ext_info[i].name; ++i) {
+ if (parent->channel->ext_info[i].read)
+ mux->ext_info[i].read = mux_read_ext_info;
+ if (parent->channel->ext_info[i].write)
+ mux->ext_info[i].write = mux_write_ext_info;
+ mux->ext_info[i].private = i;
+ }
+ }
+
+ mux->control = devm_mux_control_get(dev, NULL);
+ if (IS_ERR(mux->control)) {
+ if (PTR_ERR(mux->control) != -EPROBE_DEFER)
+ dev_err(dev, "failed to get control-mux\n");
+ return PTR_ERR(mux->control);
+ }
+
+ i = 0;
+ of_property_for_each_string_index(np, "channels", prop, label, state) {
+ if (!*label)
+ continue;
+
+ ret = mux_configure_channel(dev, mux, state, label, i++);
+ if (ret < 0)
+ return ret;
+ }
+
+ ret = devm_iio_device_register(dev, indio_dev);
+ if (ret) {
+ dev_err(dev, "failed to register iio device\n");
+ return ret;
+ }
+
+ return 0;
+}
+
+static const struct of_device_id mux_match[] = {
+ { .compatible = "io-channel-mux" },
+ { /* sentinel */ }
+};
+MODULE_DEVICE_TABLE(of, mux_match);
+
+static struct platform_driver mux_driver = {
+ .probe = mux_probe,
+ .driver = {
+ .name = "iio-mux",
+ .of_match_table = mux_match,
+ },
+};
+module_platform_driver(mux_driver);
+
+MODULE_DESCRIPTION("IIO multiplexer driver");
+MODULE_AUTHOR("Peter Rosin <peda@axentia.se>");
+MODULE_LICENSE("GPL v2");
--
2.1.4
^ permalink raw reply related
* [PATCH v7 07/12] dt-bindings: iio: iio-mux: document iio-mux bindings
From: Peter Rosin @ 2017-01-04 12:16 UTC (permalink / raw)
To: linux-kernel
Cc: Peter Rosin, Wolfram Sang, Rob Herring, Mark Rutland,
Jonathan Cameron, Hartmut Knaack, Lars-Peter Clausen,
Peter Meerwald-Stadler, Jonathan Corbet, Arnd Bergmann,
Greg Kroah-Hartman, linux-i2c, devicetree, linux-iio, linux-doc
In-Reply-To: <1483532187-28494-1-git-send-email-peda@axentia.se>
Signed-off-by: Peter Rosin <peda@axentia.se>
---
.../bindings/iio/multiplexer/io-channel-mux.txt | 39 ++++++++++++++++++++++
MAINTAINERS | 6 ++++
2 files changed, 45 insertions(+)
create mode 100644 Documentation/devicetree/bindings/iio/multiplexer/io-channel-mux.txt
diff --git a/Documentation/devicetree/bindings/iio/multiplexer/io-channel-mux.txt b/Documentation/devicetree/bindings/iio/multiplexer/io-channel-mux.txt
new file mode 100644
index 000000000000..c82794002595
--- /dev/null
+++ b/Documentation/devicetree/bindings/iio/multiplexer/io-channel-mux.txt
@@ -0,0 +1,39 @@
+I/O channel multiplexer bindings
+
+If a multiplexer is used to select which hardware signal is fed to
+e.g. an ADC channel, these bindings describe that situation.
+
+Required properties:
+- compatible : "io-channel-mux"
+- io-channels : Channel node of the parent channel that has multiplexed
+ input.
+- io-channel-names : Should be "parent".
+- #address-cells = <1>;
+- #size-cells = <0>;
+- mux-controls : Mux controller node to use for operating the mux
+- channels : List of strings, labeling the mux controller states.
+
+For each non-empty string in the channels property, an io-channel will
+be created. The number of this io-channel is the same as the index into
+the list of strings in the channels property, and also matches the mux
+controller state. The mux controller state is described in
+../mux/mux-controller.txt
+
+Example:
+ mux: mux-controller {
+ compatible = "mux-gpio";
+ #mux-control-cells = <0>;
+
+ mux-gpios = <&pioA 0 GPIO_ACTIVE_HIGH>,
+ <&pioA 1 GPIO_ACTIVE_HIGH>;
+ };
+
+ adc-mux {
+ compatible = "io-channel-mux";
+ io-channels = <&adc 0>;
+ io-channel-names = "parent";
+
+ mux-controls = <&mux>;
+
+ channels = "sync", "in", "system-regulator";
+ };
diff --git a/MAINTAINERS b/MAINTAINERS
index ebe96f3e25a0..91775f2be209 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -6274,6 +6274,12 @@ F: Documentation/ABI/testing/sysfs-bus-iio-adc-envelope-detector
F: Documentation/devicetree/bindings/iio/adc/envelope-detector.txt
F: drivers/iio/adc/envelope-detector.c
+IIO MULTIPLEXER
+M: Peter Rosin <peda@axentia.se>
+L: linux-iio@vger.kernel.org
+S: Maintained
+F: Documentation/devicetree/bindings/iio/multiplexer/iio-mux.txt
+
IIO SUBSYSTEM AND DRIVERS
M: Jonathan Cameron <jic23@kernel.org>
R: Hartmut Knaack <knaack.h@gmx.de>
--
2.1.4
^ permalink raw reply related
* [PATCH v7 06/12] iio: inkern: api for manipulating ext_info of iio channels
From: Peter Rosin @ 2017-01-04 12:16 UTC (permalink / raw)
To: linux-kernel
Cc: Peter Rosin, Wolfram Sang, Rob Herring, Mark Rutland,
Jonathan Cameron, Hartmut Knaack, Lars-Peter Clausen,
Peter Meerwald-Stadler, Jonathan Corbet, Arnd Bergmann,
Greg Kroah-Hartman, linux-i2c, devicetree, linux-iio, linux-doc
In-Reply-To: <1483532187-28494-1-git-send-email-peda@axentia.se>
Extend the inkern api with functions for reading and writing ext_info
of iio channels.
Acked-by: Jonathan Cameron <jic23@kernel.org>
Signed-off-by: Peter Rosin <peda@axentia.se>
---
drivers/iio/inkern.c | 60 ++++++++++++++++++++++++++++++++++++++++++++
include/linux/iio/consumer.h | 37 +++++++++++++++++++++++++++
2 files changed, 97 insertions(+)
diff --git a/drivers/iio/inkern.c b/drivers/iio/inkern.c
index b0f4630a163f..4848b8129e6c 100644
--- a/drivers/iio/inkern.c
+++ b/drivers/iio/inkern.c
@@ -863,3 +863,63 @@ int iio_write_channel_raw(struct iio_channel *chan, int val)
return ret;
}
EXPORT_SYMBOL_GPL(iio_write_channel_raw);
+
+unsigned int iio_get_channel_ext_info_count(struct iio_channel *chan)
+{
+ const struct iio_chan_spec_ext_info *ext_info;
+ unsigned int i = 0;
+
+ if (!chan->channel->ext_info)
+ return i;
+
+ for (ext_info = chan->channel->ext_info; ext_info->name; ext_info++)
+ ++i;
+
+ return i;
+}
+EXPORT_SYMBOL_GPL(iio_get_channel_ext_info_count);
+
+static const struct iio_chan_spec_ext_info *iio_lookup_ext_info(
+ const struct iio_channel *chan,
+ const char *attr)
+{
+ const struct iio_chan_spec_ext_info *ext_info;
+
+ if (!chan->channel->ext_info)
+ return NULL;
+
+ for (ext_info = chan->channel->ext_info; ext_info->name; ++ext_info) {
+ if (!strcmp(attr, ext_info->name))
+ return ext_info;
+ }
+
+ return NULL;
+}
+
+ssize_t iio_read_channel_ext_info(struct iio_channel *chan,
+ const char *attr, char *buf)
+{
+ const struct iio_chan_spec_ext_info *ext_info;
+
+ ext_info = iio_lookup_ext_info(chan, attr);
+ if (!ext_info)
+ return -EINVAL;
+
+ return ext_info->read(chan->indio_dev, ext_info->private,
+ chan->channel, buf);
+}
+EXPORT_SYMBOL_GPL(iio_read_channel_ext_info);
+
+ssize_t iio_write_channel_ext_info(struct iio_channel *chan, const char *attr,
+ const char *buf, size_t len)
+{
+ const struct iio_chan_spec_ext_info *ext_info;
+
+ ext_info = iio_lookup_ext_info(chan, attr);
+ if (!ext_info)
+ return -EINVAL;
+
+ return ext_info->write(chan->indio_dev, ext_info->private,
+ chan->channel, buf, len);
+}
+EXPORT_SYMBOL_GPL(iio_write_channel_ext_info);
diff --git a/include/linux/iio/consumer.h b/include/linux/iio/consumer.h
index 47eeec3218b5..5e347a9805fd 100644
--- a/include/linux/iio/consumer.h
+++ b/include/linux/iio/consumer.h
@@ -312,4 +312,41 @@ int iio_read_channel_scale(struct iio_channel *chan, int *val,
int iio_convert_raw_to_processed(struct iio_channel *chan, int raw,
int *processed, unsigned int scale);
+/**
+ * iio_get_channel_ext_info_count() - get number of ext_info attributes
+ * connected to the channel.
+ * @chan: The channel being queried
+ *
+ * Returns the number of ext_info attributes
+ */
+unsigned int iio_get_channel_ext_info_count(struct iio_channel *chan);
+
+/**
+ * iio_read_channel_ext_info() - read ext_info attribute from a given channel
+ * @chan: The channel being queried.
+ * @attr: The ext_info attribute to read.
+ * @buf: Where to store the attribute value. Assumed to hold
+ * at least PAGE_SIZE bytes.
+ *
+ * Returns the number of bytes written to buf (perhaps w/o zero termination;
+ * it need not even be a string), or an error code.
+ */
+ssize_t iio_read_channel_ext_info(struct iio_channel *chan,
+ const char *attr, char *buf);
+
+/**
+ * iio_write_channel_ext_info() - write ext_info attribute from a given channel
+ * @chan: The channel being queried.
+ * @attr: The ext_info attribute to read.
+ * @buf: The new attribute value. Strings needs to be zero-
+ * terminated, but the terminator should not be included
+ * in the below len.
+ * @len: The size of the new attribute value.
+ *
+ * Returns the number of accepted bytes, which should be the same as len.
+ * An error code can also be returned.
+ */
+ssize_t iio_write_channel_ext_info(struct iio_channel *chan, const char *attr,
+ const char *buf, size_t len);
+
#endif
--
2.1.4
^ permalink raw reply related
* [PATCH v7 05/12] mux: support simplified bindings for single-user gpio mux
From: Peter Rosin @ 2017-01-04 12:16 UTC (permalink / raw)
To: linux-kernel
Cc: Peter Rosin, Wolfram Sang, Rob Herring, Mark Rutland,
Jonathan Cameron, Hartmut Knaack, Lars-Peter Clausen,
Peter Meerwald-Stadler, Jonathan Corbet, Arnd Bergmann,
Greg Kroah-Hartman, linux-i2c, devicetree, linux-iio, linux-doc
In-Reply-To: <1483532187-28494-1-git-send-email-peda@axentia.se>
Signed-off-by: Peter Rosin <peda@axentia.se>
---
drivers/mux/mux-core.c | 81 ++++++++++++++++++++++++++++++++++++++++++++++++--
drivers/mux/mux-gpio.c | 56 ++--------------------------------
include/linux/mux.h | 7 +++++
3 files changed, 89 insertions(+), 55 deletions(-)
diff --git a/drivers/mux/mux-core.c b/drivers/mux/mux-core.c
index 21da15a264ad..d887ae1c0e55 100644
--- a/drivers/mux/mux-core.c
+++ b/drivers/mux/mux-core.c
@@ -15,6 +15,7 @@
#include <linux/device.h>
#include <linux/err.h>
#include <linux/idr.h>
+#include <linux/gpio/consumer.h>
#include <linux/module.h>
#include <linux/mux.h>
#include <linux/of.h>
@@ -288,6 +289,63 @@ static struct mux_chip *of_find_mux_chip_by_node(struct device_node *np)
return dev ? to_mux_chip(dev) : NULL;
}
+#ifdef CONFIG_MUX_GPIO
+
+static int mux_gpio_set(struct mux_control *mux, int state)
+{
+ struct mux_gpio *mux_gpio = mux_chip_priv(mux->chip);
+ int i;
+
+ for (i = 0; i < mux_gpio->gpios->ndescs; i++)
+ mux_gpio->val[i] = (state >> i) & 1;
+
+ gpiod_set_array_value_cansleep(mux_gpio->gpios->ndescs,
+ mux_gpio->gpios->desc,
+ mux_gpio->val);
+
+ return 0;
+}
+
+static const struct mux_control_ops mux_gpio_ops = {
+ .set = mux_gpio_set,
+};
+
+struct mux_chip *mux_gpio_alloc(struct device *dev)
+{
+ struct mux_chip *mux_chip;
+ struct mux_gpio *mux_gpio;
+ int pins;
+ int ret;
+
+ pins = gpiod_count(dev, "mux");
+ if (pins < 0)
+ return ERR_PTR(pins);
+
+ mux_chip = devm_mux_chip_alloc(dev, 1, sizeof(*mux_gpio) +
+ pins * sizeof(*mux_gpio->val));
+ if (!mux_chip)
+ return ERR_PTR(-ENOMEM);
+
+ mux_gpio = mux_chip_priv(mux_chip);
+ mux_gpio->val = (int *)(mux_gpio + 1);
+ mux_chip->ops = &mux_gpio_ops;
+
+ mux_gpio->gpios = devm_gpiod_get_array(dev, "mux", GPIOD_OUT_LOW);
+ if (IS_ERR(mux_gpio->gpios)) {
+ ret = PTR_ERR(mux_gpio->gpios);
+ if (ret != -EPROBE_DEFER)
+ dev_err(dev, "failed to get gpios\n");
+ return ERR_PTR(ret);
+ }
+ WARN_ON(pins != mux_gpio->gpios->ndescs);
+ mux_chip->mux->states = 1 << pins;
+
+ return mux_chip;
+}
+EXPORT_SYMBOL_GPL(mux_gpio_alloc);
+
+#endif /* CONFIG_MUX_GPIO */
+
struct mux_control *mux_control_get(struct device *dev, const char *mux_name)
{
struct device_node *np = dev->of_node;
@@ -307,9 +365,28 @@ struct mux_control *mux_control_get(struct device *dev, const char *mux_name)
ret = of_parse_phandle_with_args(np,
"mux-controls", "#mux-control-cells",
index, &args);
+
+#ifdef CONFIG_MUX_GPIO
+ if (ret == -ENOENT && !mux_name && gpiod_count(dev, "mux") > 0) {
+ mux_chip = mux_gpio_alloc(dev);
+ if (!IS_ERR(mux_chip)) {
+ ret = devm_mux_chip_register(dev, mux_chip);
+ if (ret < 0) {
+ dev_err(dev, "failed to register mux-chip\n");
+ return ERR_PTR(ret);
+ }
+ get_device(&mux_chip->dev);
+ return mux_chip->mux;
+ }
+
+ ret = PTR_ERR(mux_chip);
+ }
+#endif
+
if (ret) {
- dev_err(dev, "%s: failed to get mux-control %s(%i)\n",
- np->full_name, mux_name ?: "", index);
+ if (ret != -EPROBE_DEFER)
+ dev_err(dev, "%s: failed to get mux-control %s(%i)\n",
+ np->full_name, mux_name ?: "", index);
return ERR_PTR(ret);
}
diff --git a/drivers/mux/mux-gpio.c b/drivers/mux/mux-gpio.c
index 76b52bc63470..8a7bfbc0c4bb 100644
--- a/drivers/mux/mux-gpio.c
+++ b/drivers/mux/mux-gpio.c
@@ -11,37 +11,12 @@
*/
#include <linux/err.h>
-#include <linux/gpio/consumer.h>
#include <linux/module.h>
#include <linux/mux.h>
#include <linux/of_platform.h>
#include <linux/platform_device.h>
#include <linux/property.h>
-struct mux_gpio {
- struct gpio_descs *gpios;
- int *val;
-};
-
-static int mux_gpio_set(struct mux_control *mux, int state)
-{
- struct mux_gpio *mux_gpio = mux_chip_priv(mux->chip);
- int i;
-
- for (i = 0; i < mux_gpio->gpios->ndescs; i++)
- mux_gpio->val[i] = (state >> i) & 1;
-
- gpiod_set_array_value_cansleep(mux_gpio->gpios->ndescs,
- mux_gpio->gpios->desc,
- mux_gpio->val);
-
- return 0;
-}
-
-static const struct mux_control_ops mux_gpio_ops = {
- .set = mux_gpio_set,
-};
-
static const struct of_device_id mux_gpio_dt_ids[] = {
{ .compatible = "mux-gpio", },
{ /* sentinel */ }
@@ -51,38 +26,13 @@ MODULE_DEVICE_TABLE(of, mux_gpio_dt_ids);
static int mux_gpio_probe(struct platform_device *pdev)
{
struct device *dev = &pdev->dev;
- struct device_node *np = dev->of_node;
struct mux_chip *mux_chip;
- struct mux_gpio *mux_gpio;
- int pins;
u32 idle_state;
int ret;
- if (!np)
- return -ENODEV;
-
- pins = gpiod_count(dev, "mux");
- if (pins < 0)
- return pins;
-
- mux_chip = devm_mux_chip_alloc(dev, 1, sizeof(*mux_gpio) +
- pins * sizeof(*mux_gpio->val));
- if (!mux_chip)
- return -ENOMEM;
-
- mux_gpio = mux_chip_priv(mux_chip);
- mux_gpio->val = (int *)(mux_gpio + 1);
- mux_chip->ops = &mux_gpio_ops;
-
- mux_gpio->gpios = devm_gpiod_get_array(dev, "mux", GPIOD_OUT_LOW);
- if (IS_ERR(mux_gpio->gpios)) {
- ret = PTR_ERR(mux_gpio->gpios);
- if (ret != -EPROBE_DEFER)
- dev_err(dev, "failed to get gpios\n");
- return ret;
- }
- WARN_ON(pins != mux_gpio->gpios->ndescs);
- mux_chip->mux->states = 1 << pins;
+ mux_chip = mux_gpio_alloc(dev);
+ if (IS_ERR(mux_chip))
+ return PTR_ERR(mux_chip);
ret = device_property_read_u32(dev, "idle-state", &idle_state);
if (ret >= 0) {
diff --git a/include/linux/mux.h b/include/linux/mux.h
index 3b9439927f11..3bfee23cfb8c 100644
--- a/include/linux/mux.h
+++ b/include/linux/mux.h
@@ -241,4 +241,11 @@ struct mux_control *devm_mux_control_get(struct device *dev,
*/
void devm_mux_control_put(struct device *dev, struct mux_control *mux);
+struct mux_gpio {
+ struct gpio_descs *gpios;
+ int *val;
+};
+
+struct mux_chip *mux_gpio_alloc(struct device *dev);
+
#endif /* _LINUX_MUX_H */
--
2.1.4
^ permalink raw reply related
* [PATCH v7 04/12] dt-bindings: simplified bindings for single-user gpio mux
From: Peter Rosin @ 2017-01-04 12:16 UTC (permalink / raw)
To: linux-kernel
Cc: Peter Rosin, Wolfram Sang, Rob Herring, Mark Rutland,
Jonathan Cameron, Hartmut Knaack, Lars-Peter Clausen,
Peter Meerwald-Stadler, Jonathan Corbet, Arnd Bergmann,
Greg Kroah-Hartman, linux-i2c, devicetree, linux-iio, linux-doc
In-Reply-To: <1483532187-28494-1-git-send-email-peda@axentia.se>
Signed-off-by: Peter Rosin <peda@axentia.se>
---
.../devicetree/bindings/mux/mux-controller.txt | 26 ++++++++++++++++++++++
1 file changed, 26 insertions(+)
diff --git a/Documentation/devicetree/bindings/mux/mux-controller.txt b/Documentation/devicetree/bindings/mux/mux-controller.txt
index 42b2177e5ae1..4e89df8b2392 100644
--- a/Documentation/devicetree/bindings/mux/mux-controller.txt
+++ b/Documentation/devicetree/bindings/mux/mux-controller.txt
@@ -125,3 +125,29 @@ An example mux controller might look like this:
reg = <0x50>;
#mux-control-cells = <1>;
};
+
+
+Combinded controller and consumer of a GPIO mux
+-----------------------------------------------
+
+For the common case of a single consumer of a GPIO controlled mux, there is
+a simplified binding which will instantiate an implicit mux controller. Just
+specify a mux-gpios property with the same interpretation as in mux-gpio.txt.
+Note that other properties described in mux-gpio.txt are not available in
+this simplified form and that the mux controller is unnamed. If you need
+more than one mux controller, a shared mux controller or if you need a
+specific idle-state, use the more flexible binding with the mux controller
+in its own node.
+
+Example:
+
+ adc-mux {
+ compatible = "io-channel-mux";
+ io-channels = <&adc 0>;
+ io-channel-names = "parent";
+
+ mux-gpios = <&pioA 0 GPIO_ACTIVE_HIGH>,
+ <&pioA 1 GPIO_ACTIVE_HIGH>;
+
+ channels = "sync-1", "in", "out", "sync-2";
+ };
--
2.1.4
^ permalink raw reply related
* [PATCH v7 03/12] mux: minimal mux subsystem and gpio-based mux controller
From: Peter Rosin @ 2017-01-04 12:16 UTC (permalink / raw)
To: linux-kernel-u79uwXL29TY76Z2rM5mHXA
Cc: Peter Rosin, Wolfram Sang, Rob Herring, Mark Rutland,
Jonathan Cameron, Hartmut Knaack, Lars-Peter Clausen,
Peter Meerwald-Stadler, Jonathan Corbet, Arnd Bergmann,
Greg Kroah-Hartman, linux-i2c-u79uwXL29TY76Z2rM5mHXA,
devicetree-u79uwXL29TY76Z2rM5mHXA,
linux-iio-u79uwXL29TY76Z2rM5mHXA,
linux-doc-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <1483532187-28494-1-git-send-email-peda-koto5C5qi+TLoDKTGw+V6w@public.gmane.org>
Add a new minimalistic subsystem that handles multiplexer controllers.
When multiplexers are used in various places in the kernel, and the
same multiplexer controller can be used for several independent things,
there should be one place to implement support for said multiplexer
controller.
A single multiplexer controller can also be used to control several
parallel multiplexers, that are in turn used by different subsystems
in the kernel, leading to a need to coordinate multiplexer accesses.
The multiplexer subsystem handles this coordination.
This new mux controller subsystem initially comes with a single backend
driver that controls gpio based multiplexers. Even though not needed by
this initial driver, the mux controller subsystem is prepared to handle
chips with multiple (independent) mux controllers.
Signed-off-by: Peter Rosin <peda-koto5C5qi+TLoDKTGw+V6w@public.gmane.org>
---
Documentation/driver-model/devres.txt | 8 +
MAINTAINERS | 2 +
drivers/Kconfig | 2 +
drivers/Makefile | 1 +
drivers/mux/Kconfig | 33 +++
drivers/mux/Makefile | 6 +
drivers/mux/mux-core.c | 398 ++++++++++++++++++++++++++++++++++
drivers/mux/mux-gpio.c | 120 ++++++++++
include/linux/mux.h | 244 +++++++++++++++++++++
9 files changed, 814 insertions(+)
create mode 100644 drivers/mux/Kconfig
create mode 100644 drivers/mux/Makefile
create mode 100644 drivers/mux/mux-core.c
create mode 100644 drivers/mux/mux-gpio.c
create mode 100644 include/linux/mux.h
diff --git a/Documentation/driver-model/devres.txt b/Documentation/driver-model/devres.txt
index dc51fb024190..1e9ae701a587 100644
--- a/Documentation/driver-model/devres.txt
+++ b/Documentation/driver-model/devres.txt
@@ -332,6 +332,14 @@ MEM
MFD
devm_mfd_add_devices()
+MUX
+ devm_mux_chip_alloc()
+ devm_mux_chip_free()
+ devm_mux_chip_register()
+ devm_mux_chip_unregister()
+ devm_mux_control_get()
+ devm_mux_control_put()
+
PER-CPU MEM
devm_alloc_percpu()
devm_free_percpu()
diff --git a/MAINTAINERS b/MAINTAINERS
index 32abef2b6d05..ebe96f3e25a0 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -8442,6 +8442,8 @@ MULTIPLEXER SUBSYSTEM
M: Peter Rosin <peda-koto5C5qi+TLoDKTGw+V6w@public.gmane.org>
S: Maintained
F: Documentation/devicetree/bindings/mux/
+F: include/linux/mux.h
+F: drivers/mux/
MULTISOUND SOUND DRIVER
M: Andrew Veliath <andrewtv-Jdbf3xiKgS8@public.gmane.org>
diff --git a/drivers/Kconfig b/drivers/Kconfig
index e1e2066cecdb..993aeb65affa 100644
--- a/drivers/Kconfig
+++ b/drivers/Kconfig
@@ -76,6 +76,8 @@ source "drivers/hwmon/Kconfig"
source "drivers/thermal/Kconfig"
+source "drivers/mux/Kconfig"
+
source "drivers/watchdog/Kconfig"
source "drivers/ssb/Kconfig"
diff --git a/drivers/Makefile b/drivers/Makefile
index 060026a02f59..d089baa57965 100644
--- a/drivers/Makefile
+++ b/drivers/Makefile
@@ -112,6 +112,7 @@ obj-$(CONFIG_W1) += w1/
obj-y += power/
obj-$(CONFIG_HWMON) += hwmon/
obj-$(CONFIG_THERMAL) += thermal/
+obj-$(CONFIG_MULTIPLEXER) += mux/
obj-$(CONFIG_WATCHDOG) += watchdog/
obj-$(CONFIG_MD) += md/
obj-$(CONFIG_BT) += bluetooth/
diff --git a/drivers/mux/Kconfig b/drivers/mux/Kconfig
new file mode 100644
index 000000000000..de5a35ffe7af
--- /dev/null
+++ b/drivers/mux/Kconfig
@@ -0,0 +1,33 @@
+#
+# Multiplexer devices
+#
+
+menuconfig MULTIPLEXER
+ bool "Multiplexer subsystem"
+ help
+ Multiplexer controller subsystem. Multiplexers are used in a
+ variety of settings, and this subsystem abstracts their use
+ so that the rest of the kernel sees a common interface. When
+ multiple parallel multiplexers are controlled by one single
+ multiplexer controller, this subsystem also coordinates the
+ multiplexer accesses.
+
+ If unsure, say no.
+
+if MULTIPLEXER
+
+config MUX_GPIO
+ tristate "GPIO-controlled Multiplexer"
+ depends on OF && GPIOLIB
+ help
+ GPIO-controlled Multiplexer controller.
+
+ The driver builds a single multiplexer controller using a number
+ of gpio pins. For N pins, there will be 2^N possible multiplexer
+ states. The GPIO pins can be connected (by the hardware) to several
+ multiplexers, which in that case will be operated in parallel.
+
+ To compile this driver as a module, choose M here: the module will
+ be called mux-gpio.
+
+endif
diff --git a/drivers/mux/Makefile b/drivers/mux/Makefile
new file mode 100644
index 000000000000..facc43da3648
--- /dev/null
+++ b/drivers/mux/Makefile
@@ -0,0 +1,6 @@
+#
+# Makefile for multiplexer devices.
+#
+
+obj-$(CONFIG_MULTIPLEXER) += mux-core.o
+obj-$(CONFIG_MUX_GPIO) += mux-gpio.o
diff --git a/drivers/mux/mux-core.c b/drivers/mux/mux-core.c
new file mode 100644
index 000000000000..21da15a264ad
--- /dev/null
+++ b/drivers/mux/mux-core.c
@@ -0,0 +1,398 @@
+/*
+ * Multiplexer subsystem
+ *
+ * Copyright (C) 2016 Axentia Technologies AB
+ *
+ * Author: Peter Rosin <peda-koto5C5qi+TLoDKTGw+V6w@public.gmane.org>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+
+#define pr_fmt(fmt) "mux-core: " fmt
+
+#include <linux/device.h>
+#include <linux/err.h>
+#include <linux/idr.h>
+#include <linux/module.h>
+#include <linux/mux.h>
+#include <linux/of.h>
+#include <linux/of_platform.h>
+#include <linux/slab.h>
+
+static struct class mux_class = {
+ .name = "mux",
+ .owner = THIS_MODULE,
+};
+
+static int __init mux_init(void)
+{
+ return class_register(&mux_class);
+}
+
+static DEFINE_IDA(mux_ida);
+
+static void mux_chip_release(struct device *dev)
+{
+ struct mux_chip *mux_chip = to_mux_chip(dev);
+
+ ida_simple_remove(&mux_ida, mux_chip->id);
+ kfree(mux_chip);
+}
+
+static struct device_type mux_type = {
+ .name = "mux-chip",
+ .release = mux_chip_release,
+};
+
+struct mux_chip *mux_chip_alloc(struct device *dev,
+ unsigned int controllers, size_t sizeof_priv)
+{
+ struct mux_chip *mux_chip;
+ int i;
+
+ if (!dev || !controllers)
+ return NULL;
+
+ mux_chip = kzalloc(sizeof(*mux_chip) +
+ controllers * sizeof(*mux_chip->mux) +
+ sizeof_priv, GFP_KERNEL);
+ if (!mux_chip)
+ return NULL;
+
+ mux_chip->mux = (struct mux_control *)(mux_chip + 1);
+ mux_chip->dev.class = &mux_class;
+ mux_chip->dev.type = &mux_type;
+ mux_chip->dev.parent = dev;
+ mux_chip->dev.of_node = dev->of_node;
+ dev_set_drvdata(&mux_chip->dev, mux_chip);
+
+ mux_chip->id = ida_simple_get(&mux_ida, 0, 0, GFP_KERNEL);
+ if (mux_chip->id < 0) {
+ pr_err("muxchipX failed to get a device id\n");
+ kfree(mux_chip);
+ return NULL;
+ }
+ dev_set_name(&mux_chip->dev, "muxchip%d", mux_chip->id);
+
+ mux_chip->controllers = controllers;
+ for (i = 0; i < controllers; ++i) {
+ struct mux_control *mux = &mux_chip->mux[i];
+
+ mux->chip = mux_chip;
+ init_rwsem(&mux->lock);
+ mux->cached_state = -1;
+ mux->idle_state = -1;
+ }
+
+ device_initialize(&mux_chip->dev);
+
+ return mux_chip;
+}
+EXPORT_SYMBOL_GPL(mux_chip_alloc);
+
+static int mux_control_set(struct mux_control *mux, int state)
+{
+ int ret = mux->chip->ops->set(mux, state);
+
+ mux->cached_state = ret < 0 ? -1 : state;
+
+ return ret;
+}
+
+int mux_chip_register(struct mux_chip *mux_chip)
+{
+ int i;
+ int ret;
+
+ for (i = 0; i < mux_chip->controllers; ++i) {
+ struct mux_control *mux = &mux_chip->mux[i];
+
+ if (mux->idle_state == mux->cached_state)
+ continue;
+
+ ret = mux_control_set(mux, mux->idle_state);
+ if (ret < 0)
+ return ret;
+ }
+
+ return device_add(&mux_chip->dev);
+}
+EXPORT_SYMBOL_GPL(mux_chip_register);
+
+void mux_chip_unregister(struct mux_chip *mux_chip)
+{
+ device_del(&mux_chip->dev);
+}
+EXPORT_SYMBOL_GPL(mux_chip_unregister);
+
+void mux_chip_free(struct mux_chip *mux_chip)
+{
+ if (!mux_chip)
+ return;
+
+ put_device(&mux_chip->dev);
+}
+EXPORT_SYMBOL_GPL(mux_chip_free);
+
+static void devm_mux_chip_release(struct device *dev, void *res)
+{
+ struct mux_chip *mux_chip = *(struct mux_chip **)res;
+
+ mux_chip_free(mux_chip);
+}
+
+struct mux_chip *devm_mux_chip_alloc(struct device *dev,
+ unsigned int controllers,
+ size_t sizeof_priv)
+{
+ struct mux_chip **ptr, *mux_chip;
+
+ ptr = devres_alloc(devm_mux_chip_release, sizeof(*ptr), GFP_KERNEL);
+ if (!ptr)
+ return ERR_PTR(-ENOMEM);
+
+ mux_chip = mux_chip_alloc(dev, controllers, sizeof_priv);
+ if (IS_ERR(mux_chip)) {
+ devres_free(ptr);
+ return mux_chip;
+ }
+
+ *ptr = mux_chip;
+ devres_add(dev, ptr);
+
+ return mux_chip;
+}
+EXPORT_SYMBOL_GPL(devm_mux_chip_alloc);
+
+static int devm_mux_chip_match(struct device *dev, void *res, void *data)
+{
+ struct mux_chip **r = res;
+
+ if (!r || !*r) {
+ WARN_ON(!r || !*r);
+ return 0;
+ }
+
+ return *r == data;
+}
+
+void devm_mux_chip_free(struct device *dev, struct mux_chip *mux_chip)
+{
+ WARN_ON(devres_release(dev, devm_mux_chip_release,
+ devm_mux_chip_match, mux_chip));
+}
+EXPORT_SYMBOL_GPL(devm_mux_chip_free);
+
+static void devm_mux_chip_reg_release(struct device *dev, void *res)
+{
+ struct mux_chip *mux_chip = *(struct mux_chip **)res;
+
+ mux_chip_unregister(mux_chip);
+}
+
+int devm_mux_chip_register(struct device *dev,
+ struct mux_chip *mux_chip)
+{
+ struct mux_chip **ptr;
+ int res;
+
+ ptr = devres_alloc(devm_mux_chip_reg_release, sizeof(*ptr), GFP_KERNEL);
+ if (!ptr)
+ return -ENOMEM;
+
+ res = mux_chip_register(mux_chip);
+ if (res) {
+ devres_free(ptr);
+ return res;
+ }
+
+ *ptr = mux_chip;
+ devres_add(dev, ptr);
+
+ return res;
+}
+EXPORT_SYMBOL_GPL(devm_mux_chip_register);
+
+void devm_mux_chip_unregister(struct device *dev, struct mux_chip *mux_chip)
+{
+ WARN_ON(devres_release(dev, devm_mux_chip_reg_release,
+ devm_mux_chip_match, mux_chip));
+}
+EXPORT_SYMBOL_GPL(devm_mux_chip_unregister);
+
+int mux_control_select(struct mux_control *mux, int state)
+{
+ int ret;
+
+ if (down_read_trylock(&mux->lock)) {
+ if (mux->cached_state == state)
+ return 0;
+
+ /* Sigh, the mux needs updating... */
+ up_read(&mux->lock);
+ }
+
+ /* ...or it's just contended. */
+ down_write(&mux->lock);
+
+ if (mux->cached_state == state) {
+ /*
+ * Hmmm, someone else changed the mux to my liking.
+ * That makes me wonder how long I waited for nothing?
+ */
+ downgrade_write(&mux->lock);
+ return 0;
+ }
+
+ ret = mux_control_set(mux, state);
+ if (ret < 0) {
+ if (mux->idle_state != -1)
+ mux_control_set(mux, mux->idle_state);
+
+ up_write(&mux->lock);
+ return ret;
+ }
+
+ downgrade_write(&mux->lock);
+
+ return 1;
+}
+EXPORT_SYMBOL_GPL(mux_control_select);
+
+int mux_control_deselect(struct mux_control *mux)
+{
+ int ret = 0;
+
+ if (mux->idle_state != -1 && mux->cached_state != mux->idle_state)
+ ret = mux_control_set(mux, mux->idle_state);
+
+ up_read(&mux->lock);
+
+ return ret;
+}
+EXPORT_SYMBOL_GPL(mux_control_deselect);
+
+static int of_dev_node_match(struct device *dev, const void *data)
+{
+ return dev->of_node == data;
+}
+
+static struct mux_chip *of_find_mux_chip_by_node(struct device_node *np)
+{
+ struct device *dev;
+
+ dev = class_find_device(&mux_class, NULL, np, of_dev_node_match);
+
+ return dev ? to_mux_chip(dev) : NULL;
+}
+
+struct mux_control *mux_control_get(struct device *dev, const char *mux_name)
+{
+ struct device_node *np = dev->of_node;
+ struct of_phandle_args args;
+ struct mux_chip *mux_chip;
+ unsigned int controller;
+ int index = 0;
+ int ret;
+
+ if (mux_name) {
+ index = of_property_match_string(np, "mux-control-names",
+ mux_name);
+ if (index < 0)
+ return ERR_PTR(index);
+ }
+
+ ret = of_parse_phandle_with_args(np,
+ "mux-controls", "#mux-control-cells",
+ index, &args);
+ if (ret) {
+ dev_err(dev, "%s: failed to get mux-control %s(%i)\n",
+ np->full_name, mux_name ?: "", index);
+ return ERR_PTR(ret);
+ }
+
+ mux_chip = of_find_mux_chip_by_node(args.np);
+ of_node_put(args.np);
+ if (!mux_chip)
+ return ERR_PTR(-EPROBE_DEFER);
+
+ if (args.args_count > 1 ||
+ (!args.args_count && (mux_chip->controllers > 1))) {
+ dev_err(dev, "%s: wrong #mux-control-cells for %s\n",
+ np->full_name, args.np->full_name);
+ return ERR_PTR(-EINVAL);
+ }
+
+ controller = 0;
+ if (args.args_count)
+ controller = args.args[0];
+
+ if (controller >= mux_chip->controllers)
+ return ERR_PTR(-EINVAL);
+
+ get_device(&mux_chip->dev);
+ return &mux_chip->mux[controller];
+}
+EXPORT_SYMBOL_GPL(mux_control_get);
+
+void mux_control_put(struct mux_control *mux)
+{
+ put_device(&mux->chip->dev);
+}
+EXPORT_SYMBOL_GPL(mux_control_put);
+
+static void devm_mux_control_release(struct device *dev, void *res)
+{
+ struct mux_control *mux = *(struct mux_control **)res;
+
+ mux_control_put(mux);
+}
+
+struct mux_control *devm_mux_control_get(struct device *dev,
+ const char *mux_name)
+{
+ struct mux_control **ptr, *mux;
+
+ ptr = devres_alloc(devm_mux_control_release, sizeof(*ptr), GFP_KERNEL);
+ if (!ptr)
+ return ERR_PTR(-ENOMEM);
+
+ mux = mux_control_get(dev, mux_name);
+ if (IS_ERR(mux)) {
+ devres_free(ptr);
+ return mux;
+ }
+
+ *ptr = mux;
+ devres_add(dev, ptr);
+
+ return mux;
+}
+EXPORT_SYMBOL_GPL(devm_mux_control_get);
+
+static int devm_mux_control_match(struct device *dev, void *res, void *data)
+{
+ struct mux_control **r = res;
+
+ if (!r || !*r) {
+ WARN_ON(!r || !*r);
+ return 0;
+ }
+
+ return *r == data;
+}
+
+void devm_mux_control_put(struct device *dev, struct mux_control *mux)
+{
+ WARN_ON(devres_release(dev, devm_mux_control_release,
+ devm_mux_control_match, mux));
+}
+EXPORT_SYMBOL_GPL(devm_mux_control_put);
+
+subsys_initcall(mux_init);
+
+MODULE_DESCRIPTION("Multiplexer subsystem");
+MODULE_AUTHOR("Peter Rosin <peda-koto5C5qi+TLoDKTGw+V6w@public.gmane.org");
+MODULE_LICENSE("GPL v2");
diff --git a/drivers/mux/mux-gpio.c b/drivers/mux/mux-gpio.c
new file mode 100644
index 000000000000..76b52bc63470
--- /dev/null
+++ b/drivers/mux/mux-gpio.c
@@ -0,0 +1,120 @@
+/*
+ * GPIO-controlled multiplexer driver
+ *
+ * Copyright (C) 2016 Axentia Technologies AB
+ *
+ * Author: Peter Rosin <peda-koto5C5qi+TLoDKTGw+V6w@public.gmane.org>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+
+#include <linux/err.h>
+#include <linux/gpio/consumer.h>
+#include <linux/module.h>
+#include <linux/mux.h>
+#include <linux/of_platform.h>
+#include <linux/platform_device.h>
+#include <linux/property.h>
+
+struct mux_gpio {
+ struct gpio_descs *gpios;
+ int *val;
+};
+
+static int mux_gpio_set(struct mux_control *mux, int state)
+{
+ struct mux_gpio *mux_gpio = mux_chip_priv(mux->chip);
+ int i;
+
+ for (i = 0; i < mux_gpio->gpios->ndescs; i++)
+ mux_gpio->val[i] = (state >> i) & 1;
+
+ gpiod_set_array_value_cansleep(mux_gpio->gpios->ndescs,
+ mux_gpio->gpios->desc,
+ mux_gpio->val);
+
+ return 0;
+}
+
+static const struct mux_control_ops mux_gpio_ops = {
+ .set = mux_gpio_set,
+};
+
+static const struct of_device_id mux_gpio_dt_ids[] = {
+ { .compatible = "mux-gpio", },
+ { /* sentinel */ }
+};
+MODULE_DEVICE_TABLE(of, mux_gpio_dt_ids);
+
+static int mux_gpio_probe(struct platform_device *pdev)
+{
+ struct device *dev = &pdev->dev;
+ struct device_node *np = dev->of_node;
+ struct mux_chip *mux_chip;
+ struct mux_gpio *mux_gpio;
+ int pins;
+ u32 idle_state;
+ int ret;
+
+ if (!np)
+ return -ENODEV;
+
+ pins = gpiod_count(dev, "mux");
+ if (pins < 0)
+ return pins;
+
+ mux_chip = devm_mux_chip_alloc(dev, 1, sizeof(*mux_gpio) +
+ pins * sizeof(*mux_gpio->val));
+ if (!mux_chip)
+ return -ENOMEM;
+
+ mux_gpio = mux_chip_priv(mux_chip);
+ mux_gpio->val = (int *)(mux_gpio + 1);
+ mux_chip->ops = &mux_gpio_ops;
+
+ mux_gpio->gpios = devm_gpiod_get_array(dev, "mux", GPIOD_OUT_LOW);
+ if (IS_ERR(mux_gpio->gpios)) {
+ ret = PTR_ERR(mux_gpio->gpios);
+ if (ret != -EPROBE_DEFER)
+ dev_err(dev, "failed to get gpios\n");
+ return ret;
+ }
+ WARN_ON(pins != mux_gpio->gpios->ndescs);
+ mux_chip->mux->states = 1 << pins;
+
+ ret = device_property_read_u32(dev, "idle-state", &idle_state);
+ if (ret >= 0) {
+ if (idle_state >= mux_chip->mux->states) {
+ dev_err(dev, "invalid idle-state %u\n", idle_state);
+ return -EINVAL;
+ }
+
+ mux_chip->mux->idle_state = idle_state;
+ }
+
+ ret = devm_mux_chip_register(dev, mux_chip);
+ if (ret < 0) {
+ dev_err(dev, "failed to register mux-chip\n");
+ return ret;
+ }
+
+ dev_info(dev, "%u-way mux-controller registered\n",
+ mux_chip->mux->states);
+
+ return 0;
+}
+
+static struct platform_driver mux_gpio_driver = {
+ .driver = {
+ .name = "mux-gpio",
+ .of_match_table = of_match_ptr(mux_gpio_dt_ids),
+ },
+ .probe = mux_gpio_probe,
+};
+module_platform_driver(mux_gpio_driver);
+
+MODULE_DESCRIPTION("GPIO-controlled multiplexer driver");
+MODULE_AUTHOR("Peter Rosin <peda-koto5C5qi+TLoDKTGw+V6w@public.gmane.org");
+MODULE_LICENSE("GPL v2");
diff --git a/include/linux/mux.h b/include/linux/mux.h
new file mode 100644
index 000000000000..3b9439927f11
--- /dev/null
+++ b/include/linux/mux.h
@@ -0,0 +1,244 @@
+/*
+ * mux.h - definitions for the multiplexer interface
+ *
+ * Copyright (C) 2016 Axentia Technologies AB
+ *
+ * Author: Peter Rosin <peda-koto5C5qi+TLoDKTGw+V6w@public.gmane.org>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+
+#ifndef _LINUX_MUX_H
+#define _LINUX_MUX_H
+
+#include <linux/device.h>
+#include <linux/rwsem.h>
+
+struct mux_chip;
+struct mux_control;
+struct platform_device;
+
+struct mux_control_ops {
+ int (*set)(struct mux_control *mux, int state);
+};
+
+/**
+ * struct mux_control - Represents a mux controller.
+ * @lock: Protects the mux controller state.
+ * @chip: The mux chip that is handling this mux controller.
+ * @states: The number of mux controller states.
+ * @cached_state: The current mux controller state, or -1 if none.
+ * @idle_state: The mux controller state to use when inactive, or -1
+ * for none.
+ */
+struct mux_control {
+ struct rw_semaphore lock; /* protects the state of the mux */
+
+ struct mux_chip *chip;
+
+ unsigned int states;
+ int cached_state;
+ int idle_state;
+};
+
+/**
+ * struct mux_chip - Represents a chip holding mux controllers.
+ * @controllers: Number of mux controllers handled by the chip.
+ * @mux: Array of mux controllers that is handled.
+ * @dev: Device structure.
+ * @id: Used to identify the device internally.
+ * @ops: Mux controller operations.
+ */
+struct mux_chip {
+ unsigned int controllers;
+ struct mux_control *mux;
+ struct device dev;
+ int id;
+
+ const struct mux_control_ops *ops;
+};
+
+#define to_mux_chip(x) container_of((x), struct mux_chip, dev)
+
+/**
+ * mux_chip_priv() - Get the extra memory reserved by mux_chip_alloc().
+ * @mux_chip: The mux-chip to get the private memory from.
+ *
+ * Return: Pointer to the private memory reserved by the allocator.
+ */
+static inline void *mux_chip_priv(struct mux_chip *mux_chip)
+{
+ return &mux_chip->mux[mux_chip->controllers];
+}
+
+/**
+ * mux_chip_alloc() - Allocate a mux-chip.
+ * @dev: The parent device implementing the mux interface.
+ * @controllers: The number of mux controllers to allocate for this chip.
+ * @sizeof_priv: Size of extra memory area for private use by the caller.
+ *
+ * Return: A pointer to the new mux-chip, NULL on failure.
+ */
+struct mux_chip *mux_chip_alloc(struct device *dev,
+ unsigned int controllers, size_t sizeof_priv);
+
+/**
+ * mux_chip_register() - Register a mux-chip, thus readying the controllers
+ * for use.
+ * @mux_chip: The mux-chip to register.
+ *
+ * Do not retry registration of the same mux-chip on failure. You should
+ * instead put it away with mux_chip_free() and allocate a new one, if you
+ * for some reason would like to retry registration.
+ *
+ * Return: Zero on success or a negative errno on error.
+ */
+int mux_chip_register(struct mux_chip *mux_chip);
+
+/**
+ * mux_chip_unregister() - Take the mux-chip off-line.
+ * @mux_chip: The mux-chip to unregister.
+ *
+ * mux_chip_unregister() reverses the effects of mux_chip_register().
+ * But not completely, you should not try to call mux_chip_register()
+ * on a mux-chip that has been registered before.
+ */
+void mux_chip_unregister(struct mux_chip *mux_chip);
+
+/**
+ * mux_chip_free() - Free the mux-chip for good.
+ * @mux_chip: The mux-chip to free.
+ *
+ * mux_chip_free() reverses the effects of mux_chip_alloc().
+ */
+void mux_chip_free(struct mux_chip *mux_chip);
+
+/**
+ * devm_mux_chip_alloc() - Resource-managed version of mux_chip_alloc().
+ * @dev: The parent device implementing the mux interface.
+ * @controllers: The number of mux controllers to allocate for this chip.
+ * @sizeof_priv: Size of extra memory area for private use by the caller.
+ *
+ * See mux_chip_alloc() for more details.
+ *
+ * Return: A pointer to the new mux-chip, NULL on failure.
+ */
+struct mux_chip *devm_mux_chip_alloc(struct device *dev,
+ unsigned int controllers,
+ size_t sizeof_priv);
+
+/**
+ * devm_mux_chip_register() - Resource-managed version mux_chip_register().
+ * @dev: The parent device implementing the mux interface.
+ * @mux_chip: The mux-chip to register.
+ *
+ * See mux_chip_register() for more details.
+ *
+ * Return: Zero on success or a negative errno on error.
+ */
+int devm_mux_chip_register(struct device *dev, struct mux_chip *mux_chip);
+
+/**
+ * devm_mux_chip_unregister() - Resource-managed version mux_chip_unregister().
+ * @dev: The device that originally registered the mux-chip.
+ * @mux_chip: The mux-chip to unregister.
+ *
+ * See mux_chip_unregister() for more details.
+ *
+ * Note that you do not normally need to call this function.
+ */
+void devm_mux_chip_unregister(struct device *dev, struct mux_chip *mux_chip);
+
+/**
+ * devm_mux_chip_free() - Resource-managed version mux_chip_free().
+ * @dev: The device that originally got the mux-chip.
+ * @mux_chip: The mux-chip to free.
+ *
+ * See mux_chip_free() for more details.
+ *
+ * Note that you do not normally need to call this function.
+ */
+void devm_mux_chip_free(struct device *dev, struct mux_chip *mux_chip);
+
+/**
+ * mux_control_select() - Select the given multiplexer state.
+ * @mux: The mux-control to request a change of state from.
+ * @state: The new requested state.
+ *
+ * Make sure to call mux_control_deselect() when the operation is complete and
+ * the mux-control is free for others to use, but do not call
+ * mux_control_deselect() if mux_control_select() fails.
+ *
+ * Return: 0 if the requested state was already active, or 1 it the
+ * mux-control state was changed to the requested state. Or a negavive
+ * errno on error.
+ *
+ * Note that the difference in return value of zero or one is of
+ * questionable value; especially if the mux-control has several independent
+ * consumers, which is something the consumers should perhaps not be making
+ * assumptions about.
+ */
+int mux_control_select(struct mux_control *mux, int state);
+
+/**
+ * mux_control_deselect() - Deselect the previously selected multiplexer state.
+ * @mux: The mux-control to deselect.
+ *
+ * Return: 0 on success and a negative errno on error. An error can only
+ * occur if the mux has an idle state. Note that even if an error occurs, the
+ * mux-control is unlocked for others to access.
+ */
+int mux_control_deselect(struct mux_control *mux);
+
+/**
+ * mux_control_get_index() - Get the index of the given mux controller
+ * @mux: The mux-control to the the index for.
+ *
+ * Return: The index of the mux controller within the mux chip the mux
+ * controller is a part of.
+ */
+static inline unsigned int mux_control_get_index(struct mux_control *mux)
+{
+ return mux - mux->chip->mux;
+}
+
+/**
+ * mux_control_get() - Get the mux-control for a device.
+ * @dev: The device that needs a mux-control.
+ * @mux_name: The name identifying the mux-control.
+ *
+ * Return: A pointer to the mux-control, or an ERR_PTR with a negative errno.
+ */
+struct mux_control *mux_control_get(struct device *dev, const char *mux_name);
+
+/**
+ * mux_control_put() - Put away the mux-control for good.
+ * @mux: The mux-control to put away.
+ *
+ * mux_control_put() reverses the effects of mux_control_get().
+ */
+void mux_control_put(struct mux_control *mux);
+
+/**
+ * devm_mux_control_get() - Get the mux-control for a device, with resource
+ * management.
+ * @dev: The device that needs a mux-control.
+ * @mux_name: The name identifying the mux-control.
+ *
+ * Return: Pointer to the mux-control, or an ERR_PTR with a negative errno.
+ */
+struct mux_control *devm_mux_control_get(struct device *dev,
+ const char *mux_name);
+
+/**
+ * devm_mux_control_put() - Resource-managed version mux_control_put().
+ * @dev: The device that originally got the mux-control.
+ * @mux: The mux-control to put away.
+ *
+ * Note that you do not normally need to call this function.
+ */
+void devm_mux_control_put(struct device *dev, struct mux_control *mux);
+
+#endif /* _LINUX_MUX_H */
--
2.1.4
^ permalink raw reply related
* [PATCH v7 02/12] dt-bindings: document devicetree bindings for mux-controllers and mux-gpio
From: Peter Rosin @ 2017-01-04 12:16 UTC (permalink / raw)
To: linux-kernel
Cc: Peter Rosin, Wolfram Sang, Rob Herring, Mark Rutland,
Jonathan Cameron, Hartmut Knaack, Lars-Peter Clausen,
Peter Meerwald-Stadler, Jonathan Corbet, Arnd Bergmann,
Greg Kroah-Hartman, linux-i2c, devicetree, linux-iio, linux-doc
In-Reply-To: <1483532187-28494-1-git-send-email-peda@axentia.se>
Acked-by: Jonathan Cameron <jic23@kernel.org>
Signed-off-by: Peter Rosin <peda@axentia.se>
---
.../devicetree/bindings/mux/mux-controller.txt | 127 +++++++++++++++++++++
Documentation/devicetree/bindings/mux/mux-gpio.txt | 68 +++++++++++
MAINTAINERS | 5 +
3 files changed, 200 insertions(+)
create mode 100644 Documentation/devicetree/bindings/mux/mux-controller.txt
create mode 100644 Documentation/devicetree/bindings/mux/mux-gpio.txt
diff --git a/Documentation/devicetree/bindings/mux/mux-controller.txt b/Documentation/devicetree/bindings/mux/mux-controller.txt
new file mode 100644
index 000000000000..42b2177e5ae1
--- /dev/null
+++ b/Documentation/devicetree/bindings/mux/mux-controller.txt
@@ -0,0 +1,127 @@
+Common multiplexer controller bindings
+======================================
+
+A multiplexer (or mux) controller will have one, or several, consumer devices
+that uses the mux controller. Thus, a mux controller can possibly control
+several parallel multiplexers. Presumably there will be at least one
+multiplexer needed by each consumer, but a single mux controller can of course
+control several multiplexers for a single consumer.
+
+A mux controller provides a number of states to its consumers, and the state
+space is a simple zero-based enumeration. I.e. 0-1 for a 2-way multiplexer,
+0-7 for an 8-way multiplexer, etc.
+
+
+Consumers
+---------
+
+Mux controller consumers should specify a list of mux controllers that they
+want to use with a property containing a 'mux-ctrl-list':
+
+ mux-ctrl-list ::= <single-mux-ctrl> [mux-ctrl-list]
+ single-mux-ctrl ::= <mux-ctrl-phandle> [mux-ctrl-specifier]
+ mux-ctrl-phandle : phandle to mux controller node
+ mux-ctrl-specifier : array of #mux-control-cells specifying the
+ given mux controller (controller specific)
+
+Mux controller properties should be named "mux-controls". The exact meaning of
+each mux controller property must be documented in the device tree binding for
+each consumer. An optional property "mux-control-names" may contain a list of
+strings to label each of the mux controllers listed in the "mux-controls"
+property.
+
+Drivers for devices that use more than a single mux controller can use the
+"mux-control-names" property to map the name of the requested mux controller
+to an index into the list given by the "mux-controls" property.
+
+mux-ctrl-specifier typically encodes the chip-relative mux controller number.
+If the mux controller chip only provides a single mux controller, the
+mux-ctrl-specifier can typically be left out.
+
+Example:
+
+ /* One consumer of a 2-way mux controller (one GPIO-line) */
+ mux: mux-controller {
+ compatible = "mux-gpio";
+ #mux-control-cells = <0>;
+
+ mux-gpios = <&pioA 0 GPIO_ACTIVE_HIGH>;
+ };
+
+ adc-mux {
+ compatible = "io-channel-mux";
+ io-channels = <&adc 0>;
+ io-channel-names = "parent";
+ mux-controls = <&mux>;
+ mux-control-names = "adc";
+
+ channels = "sync", "in";
+ };
+
+Note that in the example above, specifying the "mux-control-names" is redundant
+because there is only one mux controller in the list.
+
+ /*
+ * Two consumers (one for an ADC line and one for an i2c bus) of
+ * parallel 4-way multiplexers controlled by the same two GPIO-lines.
+ */
+ mux: mux-controller {
+ compatible = "mux-gpio";
+ #mux-control-cells = <0>;
+
+ mux-gpios = <&pioA 0 GPIO_ACTIVE_HIGH>,
+ <&pioA 1 GPIO_ACTIVE_HIGH>;
+ };
+
+ adc-mux {
+ compatible = "io-channel-mux";
+ io-channels = <&adc 0>;
+ io-channel-names = "parent";
+ mux-controls = <&mux>;
+
+ channels = "sync-1", "in", "out", "sync-2";
+ };
+
+ i2c-mux {
+ compatible = "i2c-mux-simple,mux-locked";
+ i2c-parent = <&i2c1>;
+ mux-controls = <&mux>;
+
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ i2c@0 {
+ reg = <0>;
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ ssd1307: oled@3c {
+ /* ... */
+ };
+ };
+
+ i2c@3 {
+ reg = <3>;
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ pca9555: pca9555@20 {
+ /* ... */
+ };
+ };
+ };
+
+
+Mux controller nodes
+--------------------
+
+Mux controller nodes must specify the number of cells used for the
+specifier using the '#mux-control-cells' property.
+
+An example mux controller might look like this:
+
+ mux: adg792a@50 {
+ compatible = "adi,adg792a";
+ reg = <0x50>;
+ #mux-control-cells = <1>;
+ };
diff --git a/Documentation/devicetree/bindings/mux/mux-gpio.txt b/Documentation/devicetree/bindings/mux/mux-gpio.txt
new file mode 100644
index 000000000000..8f6bda6f9d78
--- /dev/null
+++ b/Documentation/devicetree/bindings/mux/mux-gpio.txt
@@ -0,0 +1,68 @@
+GPIO-based multiplexer controller bindings
+
+Define what GPIO pins are used to control a multiplexer. Or several
+multiplexers, if the same pins control more than one multiplexer.
+
+Required properties:
+- compatible : "mux-gpio"
+- mux-gpios : list of gpios used to control the multiplexer, least
+ significant bit first.
+- #mux-control-cells : <0>
+* Standard mux-controller bindings as decribed in mux-controller.txt
+
+Optional properties:
+- idle-state : if present, the state the mux will have when idle.
+
+The multiplexer state is defined as the number represented by the
+multiplexer GPIO pins, where the first pin is the least significant
+bit. An active pin is a binary 1, an inactive pin is a binary 0.
+
+Example:
+
+ mux: mux-controller {
+ compatible = "mux-gpio";
+ #mux-control-cells = <0>;
+
+ mux-gpios = <&pioA 0 GPIO_ACTIVE_HIGH>,
+ <&pioA 1 GPIO_ACTIVE_HIGH>;
+ };
+
+ adc-mux {
+ compatible = "io-channel-mux";
+ io-channels = <&adc 0>;
+ io-channel-names = "parent";
+
+ mux-controls = <&mux>;
+
+ channels = "sync-1", "in", "out", "sync-2";
+ };
+
+ i2c-mux {
+ compatible = "i2c-mux-simple,mux-locked";
+ i2c-parent = <&i2c1>;
+
+ mux-controls = <&mux>;
+
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ i2c@0 {
+ reg = <0>;
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ ssd1307: oled@3c {
+ /* ... */
+ };
+ };
+
+ i2c@3 {
+ reg = <3>;
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ pca9555: pca9555@20 {
+ /* ... */
+ };
+ };
+ };
diff --git a/MAINTAINERS b/MAINTAINERS
index bb2a665fcd02..32abef2b6d05 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -8438,6 +8438,11 @@ S: Orphan
F: drivers/mmc/host/mmc_spi.c
F: include/linux/spi/mmc_spi.h
+MULTIPLEXER SUBSYSTEM
+M: Peter Rosin <peda@axentia.se>
+S: Maintained
+F: Documentation/devicetree/bindings/mux/
+
MULTISOUND SOUND DRIVER
M: Andrew Veliath <andrewtv@usa.net>
S: Maintained
--
2.1.4
^ permalink raw reply related
* [PATCH v7 01/12] devres: trivial whitespace fix
From: Peter Rosin @ 2017-01-04 12:16 UTC (permalink / raw)
To: linux-kernel-u79uwXL29TY76Z2rM5mHXA
Cc: Peter Rosin, Wolfram Sang, Rob Herring, Mark Rutland,
Jonathan Cameron, Hartmut Knaack, Lars-Peter Clausen,
Peter Meerwald-Stadler, Jonathan Corbet, Arnd Bergmann,
Greg Kroah-Hartman, linux-i2c-u79uwXL29TY76Z2rM5mHXA,
devicetree-u79uwXL29TY76Z2rM5mHXA,
linux-iio-u79uwXL29TY76Z2rM5mHXA,
linux-doc-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <1483532187-28494-1-git-send-email-peda-koto5C5qi+TLoDKTGw+V6w@public.gmane.org>
Signed-off-by: Peter Rosin <peda-koto5C5qi+TLoDKTGw+V6w@public.gmane.org>
---
Documentation/driver-model/devres.txt | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/Documentation/driver-model/devres.txt b/Documentation/driver-model/devres.txt
index ca9d1eb46bc0..dc51fb024190 100644
--- a/Documentation/driver-model/devres.txt
+++ b/Documentation/driver-model/devres.txt
@@ -330,7 +330,7 @@ MEM
devm_kzalloc()
MFD
- devm_mfd_add_devices()
+ devm_mfd_add_devices()
PER-CPU MEM
devm_alloc_percpu()
--
2.1.4
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply related
* [PATCH v7 00/12] mux controller abstraction and iio/i2c muxes
From: Peter Rosin @ 2017-01-04 12:16 UTC (permalink / raw)
To: linux-kernel-u79uwXL29TY76Z2rM5mHXA
Cc: Peter Rosin, Wolfram Sang, Rob Herring, Mark Rutland,
Jonathan Cameron, Hartmut Knaack, Lars-Peter Clausen,
Peter Meerwald-Stadler, Jonathan Corbet, Arnd Bergmann,
Greg Kroah-Hartman, linux-i2c-u79uwXL29TY76Z2rM5mHXA,
devicetree-u79uwXL29TY76Z2rM5mHXA,
linux-iio-u79uwXL29TY76Z2rM5mHXA,
linux-doc-u79uwXL29TY76Z2rM5mHXA
Hi!
v6 -> v7 changes
- move from drivers/misc/mux-* to drivers/mux/
[I will remove Cc:s to Arnd and Greg for v8, when/if that happens]
- add managed versions of mux_chip_alloc and mux_chip_register
- use the above in mux-gpio.c and mux-adg792a.c
- also use them to support a simplified binding of a gpio mux for the common
case where there is a single consumer (and no specific idle requirements)
- new binding for describing idle behaviour of mux-adg792a
- add bindings for the gpo-pins on the mux-adg792g
- use device_property_read_u32 instead of of_property_read_u32 in mux-gpio.c
- rename iio mux compatible to io-channel-mux (was iio-mux)
- remove linuxism in the bindings (it was mentioning a function name)
- add missing quote in the example in the io-channel-mux binding
- factor out preparatory cleanup of devres docs to its own patch
- add blank line in mux_chip_free
- use SIMPLE_{PARENT,MUX}_LOCKED instead of magic numbers {0,1} in
i2c-mux-simple.c
- add some acks and a reviewed-by from Jonathan
v5 -> v6 changes
- fix stupidity in mux_chip_priv, mux_gpio_remove and adg792a_remove.
- change the devicetree bindings for the iio-mux to use a list of strings
(channels property) instead of a list children.
v4 -> v5 changes
- remove support for fancier dt layouts and go back to the phandle
approach from v2 and before, killing the horrible non-working
refcounting crap from v4 and avoiding a bunch of life-time issues
in v3.
- introduce the concept of a mux-chip, that can hold one or more
mux-controllers (inspired by the pwm subsystem).
- add dt #mux-control-cells property needed to get to the desired
mux controller if a mux chip provides more than one.
- take away the option to build the mux-core as a module.
- if the mux controller has an idle state, make sure the mux controller
is set up in the idle state initially (when it should be idle).
- do not use a variable length array on the stack in mux_gpio_set to
temporarily store the gpio state, preallocate space instead.
- fix resource leak on one failure path in mux_gpio_probe.
- driver for Analog Devices ADG792A/G, literally the first mux chip
I found on the Internet with an i2c interface (that was not a
dedicated i2c multiplexer like PCA9547) which I used to verify
that the abstractions in the mux core are up to the task. Untested,
just proof of concept that at least looks pretty and compiles...
- various touch-ups.
v3 -> v4 changes
- rebased onto next-20161122 (depends on recent _available iio changes).
- added support for having the mux-controller in a child node of a
mux-consumer if it is a sole consumer, to hopefully even further satisfy
the complaint from Rob (and later Lars-Peter) about dt complexity.
- the above came at the cost of some rather horrible refcounting code,
please review and suggest how it should be done...
- changed to register a device class instead of a bus.
- pass in the parent device into mux_control_alloc and require less
work from mux-control drivers.
- changed device names from mux:control%d to mux%d
- move kernel-doc from mux-core.c to mux.h (and add some bits).
- give the gpio driver a chance to update all mux pins at once.
- factor out iio ext_info lookup into new helper function. /Lars-Peter
- use an unsigned type for the iio ext_info count. /Lars-Peter
- unified "brag strings" in the file headers.
v2 -> v3 changes
- have the mux-controller in the parent node of any mux-controller consumer,
to hopefully satisfy complaint from Rob about dt complexity.
- improve commit message of the mux subsystem commit, making it more
general, as requested by Jonathan.
- remove priv member from struct mux_control and calculate it on the
fly. /Jonathan
- make the function comments in mux-core.c kernel doc. /Jonathan
- add devm_mux_control_* to Documentation/driver.model/devres.txt. /Jonathan
- add common dt bindings for mux-controllers, refer to them from the
mux-gpio bindings. /Rob
- clarify how the gpio pins map to the mux state. /Rob
- separate CONFIG_ variables for the mux core and the mux gpio driver.
- improve Kconfig help texts.
- make CONFIG_MUX_GPIO depend on CONFIG_GPIOLIB.
- keep track of the number of mux states in the mux core.
- since the iio channel number is used as mux state, it was possible
to drop the state member from the mux_child struct.
- cleanup dt bindings for i2c-mux-simple, it had some of copy-paste
problems from ots origin (i2c-mux-gpio).
- select the mux control subsystem in config for the i2c-mux-simple driver.
- add entries to MAINTAINERS and my sign-off, I'm now satisfied and know
nothing in this to be ashamed of.
v1 -> v2 changes
- fixup export of mux_control_put reported by kbuild
- drop devicetree iio-ext-info property as noted by Lars-Peter,
and replace the functionality by exposing all ext_info
attributes of the parent channel for each of the muxed
channels. A cache on top of that and each muxed channel
gets its own view of the ext_info of the parent channel.
- implement idle-state for muxes
- clear out the cache on failure in order to force a mux
update on the following use
- cleanup the probe of i2c-mux-simple driver
- fix a bug in the i2c-mux-simple driver, where failure in
the selection of the mux caused a deadlock when the mux
was later unconditionally deselected.
I have a piece of hardware that is using the same 3 GPIO pins
to control four 8-way muxes. Three of them control ADC lines
to an ADS1015 chip with an iio driver, and the last one
controls the SDA line of an i2c bus. We have some deployed
code to handle this, but you do not want to see it or ever
hear about it. I'm not sure why I even mention it. Anyway,
the situation has nagged me to no end for quite some time.
So, after first getting more intimate with the i2c muxing code
and later discovering the drivers/iio/inkern.c file and
writing a couple of drivers making use of it, I came up with
what I think is an acceptable solution; add a generic mux
controller driver (and subsystem) that is shared between all
instances, and combine that with an iio mux driver and a new
generic i2c mux driver. The new i2c mux I called "simple"
since it is only hooking the i2c muxing and the new mux
controller (much like the alsa simple card driver does for ASoC).
One thing that I would like to do, but don't see a solution
for, is to move the mux control code that is present in
various drivers in drivers/i2c/muxes to this new minimalistic
muxing subsystem, thus converting all present i2c muxes (but
perhaps not gates and arbitrators) to be i2c-mux-simple muxes.
I'm using an rwsem to lock a mux, but that isn't really a
perfect fit. Is there a better locking primitive that I don't
know about that fits better? I had a mutex at one point, but
that didn't allow any concurrent accesses at all. At least
the rwsem allows concurrent access as long as all users
agree on the mux state, but I suspect that the rwsem will
degrade to the mutex situation pretty quickly if there is
any contention.
Also, the "mux" name feels a bit ambitious, there are many muxes
in the world, and this tiny bit of code is probably not good
enough to be a nice fit for all...
Cheers,
peda
Peter Rosin (12):
devres: trivial whitespace fix
dt-bindings: document devicetree bindings for mux-controllers and
mux-gpio
mux: minimal mux subsystem and gpio-based mux controller
dt-bindings: simplified bindings for single-user gpio mux
mux: support simplified bindings for single-user gpio mux
iio: inkern: api for manipulating ext_info of iio channels
dt-bindings: iio: iio-mux: document iio-mux bindings
iio: multiplexer: new iio category and iio-mux driver
dt-bindings: i2c: i2c-mux-simple: document i2c-mux-simple bindings
i2c: i2c-mux-simple: new driver
dt-bindings: mux-adg792a: document devicetree bindings for ADG792A/G
mux
mux: adg792a: add mux controller driver for ADG792A/G
.../devicetree/bindings/i2c/i2c-mux-simple.txt | 81 ++++
.../bindings/iio/multiplexer/io-channel-mux.txt | 39 ++
.../devicetree/bindings/mux/mux-adg792a.txt | 79 ++++
.../devicetree/bindings/mux/mux-controller.txt | 153 +++++++
Documentation/devicetree/bindings/mux/mux-gpio.txt | 68 +++
Documentation/driver-model/devres.txt | 10 +-
MAINTAINERS | 14 +
drivers/Kconfig | 2 +
drivers/Makefile | 1 +
drivers/i2c/muxes/Kconfig | 13 +
drivers/i2c/muxes/Makefile | 1 +
drivers/i2c/muxes/i2c-mux-simple.c | 182 ++++++++
drivers/iio/Kconfig | 1 +
drivers/iio/Makefile | 1 +
drivers/iio/inkern.c | 60 +++
drivers/iio/multiplexer/Kconfig | 18 +
drivers/iio/multiplexer/Makefile | 6 +
drivers/iio/multiplexer/iio-mux.c | 456 ++++++++++++++++++++
drivers/mux/Kconfig | 45 ++
drivers/mux/Makefile | 7 +
drivers/mux/mux-adg792a.c | 169 ++++++++
drivers/mux/mux-core.c | 475 +++++++++++++++++++++
drivers/mux/mux-gpio.c | 70 +++
include/linux/iio/consumer.h | 37 ++
include/linux/mux.h | 251 +++++++++++
25 files changed, 2238 insertions(+), 1 deletion(-)
create mode 100644 Documentation/devicetree/bindings/i2c/i2c-mux-simple.txt
create mode 100644 Documentation/devicetree/bindings/iio/multiplexer/io-channel-mux.txt
create mode 100644 Documentation/devicetree/bindings/mux/mux-adg792a.txt
create mode 100644 Documentation/devicetree/bindings/mux/mux-controller.txt
create mode 100644 Documentation/devicetree/bindings/mux/mux-gpio.txt
create mode 100644 drivers/i2c/muxes/i2c-mux-simple.c
create mode 100644 drivers/iio/multiplexer/Kconfig
create mode 100644 drivers/iio/multiplexer/Makefile
create mode 100644 drivers/iio/multiplexer/iio-mux.c
create mode 100644 drivers/mux/Kconfig
create mode 100644 drivers/mux/Makefile
create mode 100644 drivers/mux/mux-adg792a.c
create mode 100644 drivers/mux/mux-core.c
create mode 100644 drivers/mux/mux-gpio.c
create mode 100644 include/linux/mux.h
--
2.1.4
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH] i2c: i801: Register optional lis3lv02d i2c device on Dell machines
From: Pali Rohár @ 2017-01-04 12:00 UTC (permalink / raw)
To: Benjamin Tissoires
Cc: Dmitry Torokhov, Michał Kępień, Jean Delvare,
Wolfram Sang, Steven Honeyman, Valdis.Kletnieks, Jochen Eisinger,
Gabriele Mazzotta, Andy Lutomirski, Mario_Limonciello, Alex Hung,
Takashi Iwai, linux-i2c, linux-kernel, platform-driver-x86
In-Reply-To: <20170104112223.GE3499@pali>
On Wednesday 04 January 2017 12:22:23 Pali Rohár wrote:
> On Wednesday 04 January 2017 11:32:33 Benjamin Tissoires wrote:
> > On Jan 04 2017 or thereabouts, Pali Rohár wrote:
> > > On Wednesday 04 January 2017 11:13:06 Benjamin Tissoires wrote:
> > > > On Jan 04 2017 or thereabouts, Pali Rohár wrote:
> > > > > On Wednesday 04 January 2017 10:05:22 Benjamin Tissoires wrote:
> > > > > > On Jan 04 2017 or thereabouts, Pali Rohár wrote:
> > > > > > > On Tuesday 03 January 2017 12:59:37 Dmitry Torokhov wrote:
> > > > > > > > On Tue, Jan 03, 2017 at 09:39:13PM +0100, Pali Rohár wrote:
> > > > > > > > > On Tuesday 03 January 2017 21:24:18 Dmitry Torokhov wrote:
> > > > > > > > > > On Tue, Jan 03, 2017 at 09:05:51PM +0100, Pali Rohár wrote:
> > > > > > > > > > > On Tuesday 03 January 2017 20:48:12 Dmitry Torokhov wrote:
> > > > > > > > > > > > On Tue, Jan 03, 2017 at 07:50:17PM +0100, Pali Rohár wrote:
> > > > > > > > > > > > > On Tuesday 03 January 2017 19:38:43 Dmitry Torokhov wrote:
> > > > > > > > > > > > > > On Tue, Jan 03, 2017 at 10:06:41AM +0100, Benjamin Tissoires
> > > > > > > > > > > > > >
> > > > > > > > > > > > > > wrote:
> > > > > > > > > > > > > > > On Dec 29 2016 or thereabouts, Pali Rohár wrote:
> > > > > > > > > > > > > > > > On Thursday 29 December 2016 22:09:32 Michał Kępień wrote:
> > > > > > > > > > > > > > > > > > On Thursday 29 December 2016 14:47:19 Michał Kępień
> > > > > > > > > > > > > > > > > > wrote:
> > > > > > > > > > > > > > > > > > > > On Thursday 29 December 2016 09:29:36 Michał
> > > > > > > > > > > > > > > > > > > > Kępień
> > > > > > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > > > > > wrote:
> > > > > > > > > > > > > > > > > > > > > > Dell platform team told us that some (DMI
> > > > > > > > > > > > > > > > > > > > > > whitelisted) Dell Latitude machines have ST
> > > > > > > > > > > > > > > > > > > > > > microelectronics accelerometer at i2c address
> > > > > > > > > > > > > > > > > > > > > > 0x29. That i2c address is not specified in
> > > > > > > > > > > > > > > > > > > > > > DMI or ACPI, so runtime detection without
> > > > > > > > > > > > > > > > > > > > > > whitelist which is below is not possible.
> > > > > > > > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > > > > > > > Presence of that ST microelectronics
> > > > > > > > > > > > > > > > > > > > > > accelerometer is verified by existence of
> > > > > > > > > > > > > > > > > > > > > > SMO88xx ACPI device which represent that
> > > > > > > > > > > > > > > > > > > > > > accelerometer. Unfortunately without i2c
> > > > > > > > > > > > > > > > > > > > > > address.
> > > > > > > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > > > > > > This part of the commit message sounded a bit
> > > > > > > > > > > > > > > > > > > > > confusing to me at first because there is
> > > > > > > > > > > > > > > > > > > > > already an ACPI driver which handles SMO88xx
> > > > > > > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > > > > > > devices (dell-smo8800). My understanding is
> > > > > > > > > > > > > > > > > > > > > that:
> > > > > > > > > > > > > > > > > > > > > * the purpose of this patch is to expose a
> > > > > > > > > > > > > > > > > > > > > richer interface (as
> > > > > > > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > > > > > > provided by lis3lv02d) to these devices on
> > > > > > > > > > > > > > > > > > > > > some machines,
> > > > > > > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > > > > > > * on whitelisted machines, dell-smo8800 and
> > > > > > > > > > > > > > > > > > > > > lis3lv02d can work
> > > > > > > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > > > > > > simultaneously (even though dell-smo8800
> > > > > > > > > > > > > > > > > > > > > effectively duplicates the work that
> > > > > > > > > > > > > > > > > > > > > lis3lv02d does).
> > > > > > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > > > > > No. dell-smo8800 reads from ACPI irq number and
> > > > > > > > > > > > > > > > > > > > exports /dev/freefall device which notify
> > > > > > > > > > > > > > > > > > > > userspace about falls. lis3lv02d is i2c driver
> > > > > > > > > > > > > > > > > > > > which exports axes of accelerometer. Additionaly
> > > > > > > > > > > > > > > > > > > > lis3lv02d can export also /dev/freefall if
> > > > > > > > > > > > > > > > > > > > registerer of i2c device provides irq number --
> > > > > > > > > > > > > > > > > > > > which is not case of this patch.
> > > > > > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > > > > > So both drivers are doing different things and
> > > > > > > > > > > > > > > > > > > > both are useful.
> > > > > > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > > > > > IIRC both dell-smo8800 and lis3lv02d represent
> > > > > > > > > > > > > > > > > > > > one HW device (that ST microelectronics
> > > > > > > > > > > > > > > > > > > > accelerometer) but due to complicated HW
> > > > > > > > > > > > > > > > > > > > abstraction and layers on Dell laptops it is
> > > > > > > > > > > > > > > > > > > > handled by two drivers, one ACPI and one i2c.
> > > > > > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > > > > > Yes, in ideal world irq number should be passed
> > > > > > > > > > > > > > > > > > > > to lis3lv02d driver and that would export whole
> > > > > > > > > > > > > > > > > > > > device (with /dev/freefall too), but due to HW
> > > > > > > > > > > > > > > > > > > > abstraction it is too much complicated...
> > > > > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > > > > Why? AFAICT, all that is required to pass that IRQ
> > > > > > > > > > > > > > > > > > > number all the way down to lis3lv02d is to set the
> > > > > > > > > > > > > > > > > > > irq field of the struct i2c_board_info you are
> > > > > > > > > > > > > > > > > > > passing to i2c_new_device(). And you can extract
> > > > > > > > > > > > > > > > > > > that IRQ number e.g. in
> > > > > > > > > > > > > > > > > > > check_acpi_smo88xx_device(). However, you would
> > > > > > > > > > > > > > > > > > > then need to make sure dell-smo8800 does not
> > > > > > > > > > > > > > > > > > > attempt to request the same IRQ on whitelisted
> > > > > > > > > > > > > > > > > > > machines. This got me thinking about a way to
> > > > > > > > > > > > > > > > > > > somehow incorporate your changes into dell-smo8800
> > > > > > > > > > > > > > > > > > > using Wolfram's bus_notifier suggestion, but I do
> > > > > > > > > > > > > > > > > > > not have a working solution for now. What is
> > > > > > > > > > > > > > > > > > > tempting about this approach is that you would not
> > > > > > > > > > > > > > > > > > > have to scan the ACPI namespace in search of
> > > > > > > > > > > > > > > > > > > SMO88xx devices, because smo8800_add() is
> > > > > > > > > > > > > > > > > > > automatically called for them. However, I fear that
> > > > > > > > > > > > > > > > > > > the resulting solution may be more complicated than
> > > > > > > > > > > > > > > > > > > the one you submitted.
> > > > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > > > Then we need to deal with lot of problems. Order of
> > > > > > > > > > > > > > > > > > loading .ko modules is undefined. Binding devices to
> > > > > > > > > > > > > > > > > > drivers registered by .ko module is also in "random"
> > > > > > > > > > > > > > > > > > order. At any time any of those .ko module can be
> > > > > > > > > > > > > > > > > > unloaded or at least device unbind (via sysfs) from
> > > > > > > > > > > > > > > > > > driver... And there can be some pathological
> > > > > > > > > > > > > > > > > > situation (thanks to adding ACPI layer as Andy
> > > > > > > > > > > > > > > > > > pointed) that there will be more SMO88xx devices in
> > > > > > > > > > > > > > > > > > ACPI. Plus you can compile kernel with and without
> > > > > > > > > > > > > > > > > > those modules and also you can blacklist loading
> > > > > > > > > > > > > > > > > > them (so compile time check is not enough). And
> > > > > > > > > > > > > > > > > > still some correct message notifier must be used.
> > > > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > > > I think such solution is much much more complicated,
> > > > > > > > > > > > > > > > > > there are lot of combinations of kernel configuration
> > > > > > > > > > > > > > > > > > and available dell devices...
> > > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > > I tried a few more things, but ultimately failed to
> > > > > > > > > > > > > > > > > find a nice way to implement this.
> > > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > > Another issue popped up, though. Linus' master branch
> > > > > > > > > > > > > > > > > contains a recent commit by Benjamin Tissoires (CC'ed),
> > > > > > > > > > > > > > > > > 4d5538f5882a ("i2c: use an IRQ to report Host Notify
> > > > > > > > > > > > > > > > > events, not alert") which breaks your patch. The
> > > > > > > > > > > > > > > > > reason for that is that lis3lv02d relies on the i2c
> > > > > > > > > > > > > > > > > client's IRQ being 0 to detect that it should not
> > > > > > > > > > > > > > > > > create /dev/freefall.
> > > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > > Benjamin's patch causes the Host Notify IRQ to be
> > > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > > assigned to the i2c client your patch creates, thus
> > > > > > > > > > > > > > > > > causing lis3lv02d to create /dev/freefall, which in
> > > > > > > > > > > > > > > > > turn conflicts with dell-smo8800 which is trying to
> > > > > > > > > > > > > > > > > create /dev/freefall itself.
> > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > So 4d5538f5882a is breaking lis3lv02d driver...
> > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > Apologies for that.
> > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > I could easily fix this by adding a kernel API to know
> > > > > > > > > > > > > > > whether the provided irq is from Host Notify or if it was
> > > > > > > > > > > > > > > coming from an actual declaration. However, I have no idea
> > > > > > > > > > > > > > > how many other drivers would require this (hopefully only
> > > > > > > > > > > > > > > this one).
> > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > One other solution would be to reserve the Host Notify IRQ
> > > > > > > > > > > > > > > and let the actual drivers that need it to set it, but
> > > > > > > > > > > > > > > this was not the best solution according to Dmitri. On my
> > > > > > > > > > > > > > > side, I am not entirely against this given that it's a
> > > > > > > > > > > > > > > chip feature, so the driver should be able to know that
> > > > > > > > > > > > > > > it's available.
> > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > Dmitri, Wolfram, Jean, any preferences?
> > > > > > > > > > > > > >
> > > > > > > > > > > > > > I read this:
> > > > > > > > > > > > > >
> > > > > > > > > > > > > > "IIRC both dell-smo8800 and lis3lv02d represent one HW device
> > > > > > > > > > > > > > (that ST microelectronics accelerometer) but due to
> > > > > > > > > > > > > > complicated HW abstraction and layers on Dell laptops it is
> > > > > > > > > > > > > > handled by two drivers, one ACPI and one i2c."
> > > > > > > > > > > > > >
> > > > > > > > > > > > > > and that is the core of the issue. You have 2 drivers
> > > > > > > > > > > > > > fighting over the same device. Fix this and it will all
> > > > > > > > > > > > > > work.
> > > > > > > > > > > > >
> > > > > > > > > > > > > With my current implementation (which I sent in this patch),
> > > > > > > > > > > > > they are not fighting.
> > > > > > > > > > > > >
> > > > > > > > > > > > > dell-smo8800 exports /dev/freefall (and nothing more) and
> > > > > > > > > > > > > lis3lv02d only accelerometer device as lis3lv02d driver does
> > > > > > > > > > > > > not get IRQ number in platform data.
> > > > > > > > > > > > >
> > > > > > > > > > > > > > As far as I can see hp_accel instantiates lis3lv02d and
> > > > > > > > > > > > > > accesses it via ACPI methods, can the same be done for Dell?
> > > > > > > > > > > > >
> > > > > > > > > > > > > No, Dell does not have any ACPI methods. And as I wrote in ACPI
> > > > > > > > > > > > > or DMI is even not i2c address of device, so it needs to be
> > > > > > > > > > > > > specified in code itself.
> > > > > > > > > > > > >
> > > > > > > > > > > > > Really there is no other way... :-(
> > > > > > > > > > > >
> > > > > > > > > > > > Sure there is:
> > > > > > > > > > > >
> > > > > > > > > > > > 1. dell-smo8800 instantiates I2C device as "dell-smo8800-accel".
> > > > > > > > > > > > 2. dell-smo8800 provides read/write functions for lis3lv02d that
> > > > > > > > > > > > simply forward requests to dell-smo8800-accel i2c client.
> > > > > > > > > > > > 3. dell-smo8800 instantiates lis3lv02d instance like hp_accel
> > > > > > > > > > > > does.
> > > > > > > > > > >
> > > > > > > > > > > Sorry, but I do not understand how you mean it... Why to provides
> > > > > > > > > > > new read/write i2c functions which are already implemented by
> > > > > > > > > > > i2c-i801 bus and lis3lv02d i2c driver?
> > > > > > > > > >
> > > > > > > > > > Because that would allow you to avoid clashes with i2c creating
> > > > > > > > > > interrupt mapping for client residing on host-notify-capable
> > > > > > > > > > controller.
> > > > > > > > > >
> > > > > > > > > > > > Alternatively, can lis3lv02d be tasked to create /dev/freefall?
> > > > > > > > > > >
> > > > > > > > > > > If i2c_board_info contains IRQ then lis3lv02d create /dev/freefall
> > > > > > > > > > > device.
> > > > > > > > > > >
> > > > > > > > > > > But... what is problem with current implementation? Accelerometer
> > > > > > > > > > > HW provides two functions:
> > > > > > > > > > >
> > > > > > > > > > > 1) 3 axes reports
> > > > > > > > > > > 2) Disk freefall detection
> > > > > > > > > > >
> > > > > > > > > > > And 1) is handled by i2c driver lis3lv02d and 2) is by
> > > > > > > > > > > dell-smo8800. Both functions are independent here.
> > > > > > > > > > >
> > > > > > > > > > > I think you just trying to complicate this situation even more to
> > > > > > > > > > > be more complicated as currently is.
> > > > > > > > > >
> > > > > > > > > > Because this apparently does not work for you, does it?
> > > > > > > > >
> > > > > > > > > It is working fine. I do not see any problem.
> > > > > > > > >
> > > > > > > > > > In general,
> > > > > > > > > > if you want the same hardware be handled by 2 different drivers you
> > > > > > > > > > are going to have bad time.
> > > > > > > > >
> > > > > > > > > Yes, but in this case half of device is ACPI based and other half i2c
> > > > > > > > > based. This is problem of ACPI and Dell design.
> > > > > > > > >
> > > > > > > > > > It seems to be that /dev/freefall in dell-smo8800 and lis3lv02d are
> > > > > > > > > > the same, right?
> > > > > > > > >
> > > > > > > > > Yes. I understand that clean solution is to have one driver which
> > > > > > > > > provides everything.
> > > > > > > > >
> > > > > > > > > But because half of data are ACPI and half i2c, you still needs to
> > > > > > > > > create two drivers (one ACPI and one i2c). You can put both drivers into
> > > > > > > > > one .ko module, but still these will be two drivers due to how ACPI and
> > > > > > > > > i2c linux abstractions are different.
> > > > > > > > >
> > > > > > > > > > So, instead of having 2 drivers split the
> > > > > > > > > > functionality, can you forego registering smo8800 ACPI driver on
> > > > > > > > > > your whitelisted boxes and instead instantiate full i2c client
> > > > > > > > > > device with properly assigned both address and IRQ and let lis3lv02d
> > > > > > > > > > handle it (providing both accelerometer data and /dev/freefall)?
> > > > > > > > >
> > > > > > > > > With Michał we already discussed about it, see emails. Basically you can
> > > > > > > > > enable/disable kernel modules at compile time or blacklist at runtime
> > > > > > > > > (or even chose what will be compiled into vmlinux and what as external
> > > > > > > > > .ko module).
> > > > > > > >
> > > > > > > > This can be solved with a bit of Kconfig/IS_ENABLED() code.
> > > > > > > >
> > > > > > > > > Some distributions blacklist i2c-i801.ko module... And
> > > > > > > >
> > > > > > > > Any particular reason for that?
> > > > > > > >
> > > > > > > > > there can be also problem with initialization of i2c-i801 driver (fix is
> > > > > > > > > in commit a7ae81952cda, but does not have to work at every time!). So
> > > > > > > > > that move on whitelisted machines can potentially cause disappearance of
> > > > > > > > > /dev/freefall and users will not have hdd protection which is currently
> > > > > > > > > working.
> > > > > > > >
> > > > > > > > Well, I gave you 2 possible solutions (roll your own i2c read/write,
> > > > > > > > forward them to i2c client) or have faith in your implementation and let
> > > > > > > > lis3lv02d handle it.
> > > > > > > >
> > > > > > > > The 3rd one is to possibly add a flag to disable host notify to IRQ
> > > > > > > > mapping for given client (if Wolfram/Jean OK with it).
> > > > > > > >
> > > > > > > > Oh, the 4th one: change the irq in lis3lv02d.h to be "int" and change
> > > > > > > > the check in lis3lv02d.c to be "lis->irq <= 0" and instantiate your
> > > > > > > > i2c_client with board_info->irq = -1.
> > > > > > > >
> > > > > > > > Pick whichever you prefer.
> > > > > > > >
> > > > > > > > By the way, what do you need accelerometer for on these devices? They
> > > > > > > > don't appear to be tablets that could use one...
> > > > > > >
> > > > > > > Ah, you are talking about problem that after 4d5538f5882a lis3lv02d will
> > > > > > > not work... I thought that discussion is about different mechanism how
> > > > > > > to implement bus registration notification to smo8800 driver (or
> > > > > > > different solution to not have registration in i801).
> > > > > > >
> > > > > >
> > > > > > Just because I am not sure I got everything right, could you confirm
> > > > > > that:
> > > > > > - in the current upstream tree, the dell-smo8800 driver is now broken
> > > > > > after 4d5538f5882a (i2c: use an IRQ to report Host Notify events, not
> > > > > > alert)
> > > > >
> > > > > No, dell-smo8800 it is working fine. It is fully independent from i2c
> > > > > and lis3lv02d. It is pure ACPI driver which does not share anything with
> > > > > i2c.
> > > > >
> > > > > > - this series adds an extra lis3lv02d on some machines and you have
> > > > > > problem fighting for the irq (but this is not upstream yet).
> > > > >
> > > > > Yes, this series (not merged yet) adds extra lis3lv02d device but is not
> > > > > working because of 4d5538f5882a.
> > > > >
> > > > > > The extra lis3lv02d node is added from dell-smo8800
> > > > >
> > > > > No, dell-smo8800 does not add new node in this patch.
> > > > >
> > > > > > If the first point is not correct (by default, dell-smo8800 will not be
> > > > > > loaded at the same time than lis3lv02d), then it's a design issue with
> > > > > > the interactions between those 2 drivers.
> > > > >
> > > > > No, there is no interactions between these two drivers (dell-smo8800 and
> > > > > lis3lv02d). dell-smo8800 is pure ACPI driver and exports just
> > > > > /dev/freefall device based on IRQ (and nothing more).
> > > > >
> > > > > And lis3lv02d in *current* configuration in this patch exports only
> > > > > accelerometer input device, not /dev/freefall. It does not use IRQ.
> > > > > (Just there is problem with 4d5538f5882a which tells lis3lv02d IRQ
> > > > > number which is not freefall report, therefore lis3lv02d does not work).
> > > > >
> > > > > To make it clear, ST Accelerometer provides two operations:
> > > > > * report free fall
> > > > > * report 3 axes
> > > > >
> > > > > Free fall is reported by IRQ, state of 3 axes via i2c bus. Free fall IRQ
> > > > > is handled by dell-smo8800, state of 3 axes via i2c lis3lv02d driver.
> > > > >
> > > > > lis3lv02d can handle also free fall IRQ is platform i2c data provides
> > > > > IRQ number for it -- but this is not case in our Dell configuration. But
> > > > > commit 4d5538f5882a inject some IRQ number to lis3lv02d driver which is
> > > > > not free fall detection and so is breaking lis3lv02 driver. In our Dell
> > > > > configuration (by this patch) there should be no IRQ number.
> > > > >
> > > > > It is clear now?
> > > >
> > > > I think I am starting to understand the problem.
> > > >
> > > > Currently (upstream tree), 4d5538f5882a doesn't break anything.
> > >
> > > I cannot prove it... lis3lv02d i2c driver itself uses this IRQ logic
> > > (missing = no freefall) and there could be already hardware/boards which
> > > register lis3lv02d i2c device without IRQ number. And definitions could
> > > be also in device tree and so outside of kernel sources...
> > >
> > > So there can be potentially break. But at least not case of Dell.
> > >
> > > > On the
> > > > mentioned Dell platforms, the dell-smo8800 gets loaded and provides
> > > > /dev/freefall. lis3lv02d is not loaded so everything just works.
> > >
> > > Yes.
> > >
> > > > The problem comes from this patch which doesn't set the irq in
> > > > i2c_board_info and so i2c_core sets the IRQ to Host Notify. I think
> > > > Dmitri already gave you the solution: set the irq to -1 (or -ENOENT)
> > > > in i2c_board_info in your patch and only forward it to
> > > > lis3lv02d_init_device() only if it is positive (valid).
> > >
> > > Now I understood that Dmitri means. For this Dell platforms it is OK,
> > > but we have a problem for device tree platforms. Normally in device tree
> > > if device does not support something you just do not specify it. But
> > > with this approach you need to explicitly specify IRQ to -1 in device
> > > tree. And I think this is really not a clean solution.
> > >
>
> Here I was talking about other platforms/devices (not this Dell one).
>
> > No. DT platforms won't have an issue: they don't change anything, and
> > there will be a new /dev/freefall misc device for the platforms that
>
> And this is wrong! There should not be any /dev/freefall device
> connected with signal host notify. /dev/freefall is for hardware which
> supports free fall hdd detection.
>
> > don't have the irq set *and* if the device is on a Host Notify capable
> > adapter, currently only i2c-i801.
>
> I understood, but in future more bus drivers could support host notify.
This is really fragile. lis3lv02d will empty IRQ (0) will work on some
i2c buses and on some not. I would really expect that i2c driver
(lis3lv02d) will work in same way with any i2c bus driver. And not that
on i801 will acts differently as on e.g. omap3 i2c bus.
> This is basically incorrect if we use one property for two different
> things (host notify and hdd free fall).
>
> For me it looks like that we should separate these two things into two
> IRQ properties. It is really wrong design if one property is used for
> two different things.
>
> > But given that they are DT and not
> > ACPI, this won't conflict with dell-smo8800, so there won't be any
> > errors, just a dangling unused device node.
>
> Yes, for upcoming Dell in this patch (with IRQ -1) it is not a problem.
>
> > This approach is IMO the best if you want to have this in the kernel.
> >
> > Cheers,
> > Benjamin
>
--
Pali Rohár
pali.rohar@gmail.com
^ permalink raw reply
* Re: [PATCH] i2c: i801: Register optional lis3lv02d i2c device on Dell machines
From: Pali Rohár @ 2017-01-04 11:35 UTC (permalink / raw)
To: Benjamin Tissoires
Cc: Andy Shevchenko, Dmitry Torokhov, Michał Kępień,
Jean Delvare, Wolfram Sang, Steven Honeyman, Valdis Kletnieks,
Jochen Eisinger, Gabriele Mazzotta, Andy Lutomirski,
Mario Limonciello, Alex Hung, Takashi Iwai, linux-i2c,
linux-kernel@vger.kernel.org, Platform Driver
In-Reply-To: <20170104102544.GS5767@mail.corp.redhat.com>
On Wednesday 04 January 2017 11:25:44 Benjamin Tissoires wrote:
> On Jan 04 2017 or thereabouts, Andy Shevchenko wrote:
> > On Tue, Jan 3, 2017 at 10:39 PM, Pali Rohár <pali.rohar@gmail.com> wrote:
> > >
> > > With Michał we already discussed about it, see emails. Basically you can
> > > enable/disable kernel modules at compile time or blacklist at runtime
> > > (or even chose what will be compiled into vmlinux and what as external
> > > .ko module). Some distributions blacklist i2c-i801.ko module...
> >
> > But you understand that any of compile/not compile is not an option, right?
> > The case which we face will be both of them, if possible, will be
> > compiled as modules.
> >
> > Blacklisting means making your problem the actual user's one. Not good.
> >
> > > And
> > > there can be also problem with initialization of i2c-i801 driver (fix is
> > > in commit a7ae81952cda, but does not have to work at every time!). So
> > > that move on whitelisted machines can potentially cause disappearance of
> > > /dev/freefall and users will not have hdd protection which is currently
> > > working.
> >
>
> I am seeing the same issues with psmouse and SMBus touchpads. The PS/2
> device knows about the availability of a better but unlisted device at
> the ACPI level.
>
> The way I solved this to not have to deal with compile/not compile and
> runtime errors is the same way Wolfram told you about: bus notifiers.
> I also use an intermediate platform driver to not add i2c dependency on
> psmouse.
>
> For you the solution would be:
> - In dell-smo8800, after checking the whitelist, add a platform driver
> "dell-lis3lv02d-platform", and add in the platform_data the I2C address
> of the chip.
> - create a new driver dell-lis3lv02d-platform.ko which listens for the
> i2c bus creation and registers the lis3lv02d I2C node when it sees a
> matching adapter. (see [1] for my solution)
> - in dell-lis3lv02d-platform.ko make sure to set the irq to -ENOENT so
> that lis3lv02d.ko doesn't create /dev/freefall which will still be
> handled by ACPI.
>
> How does that sound?
Yes, something like this was already suggested. But it is more
complicated as my approach and less error prone... See my notes in
previous emails.
My current path (after fixing IRQ to -1) is smaller more intuitive and
do not introduce new complicated parts like bus notifier and new "fake"
i2c driver...
> Cheers,
> Benjamin
>
> [1] https://github.com/bentiss/linux/blob/synaptics-rmi4-v4.9-rc7+/drivers/input/rmi4/rmi_platform.c
--
Pali Rohár
pali.rohar@gmail.com
^ permalink raw reply
* Re: [PATCH] i2c: i801: Register optional lis3lv02d i2c device on Dell machines
From: Pali Rohár @ 2017-01-04 11:22 UTC (permalink / raw)
To: Benjamin Tissoires
Cc: Dmitry Torokhov, Michał Kępień, Jean Delvare,
Wolfram Sang, Steven Honeyman, Valdis.Kletnieks, Jochen Eisinger,
Gabriele Mazzotta, Andy Lutomirski, Mario_Limonciello, Alex Hung,
Takashi Iwai, linux-i2c, linux-kernel, platform-driver-x86
In-Reply-To: <20170104103233.GT5767@mail.corp.redhat.com>
On Wednesday 04 January 2017 11:32:33 Benjamin Tissoires wrote:
> On Jan 04 2017 or thereabouts, Pali Rohár wrote:
> > On Wednesday 04 January 2017 11:13:06 Benjamin Tissoires wrote:
> > > On Jan 04 2017 or thereabouts, Pali Rohár wrote:
> > > > On Wednesday 04 January 2017 10:05:22 Benjamin Tissoires wrote:
> > > > > On Jan 04 2017 or thereabouts, Pali Rohár wrote:
> > > > > > On Tuesday 03 January 2017 12:59:37 Dmitry Torokhov wrote:
> > > > > > > On Tue, Jan 03, 2017 at 09:39:13PM +0100, Pali Rohár wrote:
> > > > > > > > On Tuesday 03 January 2017 21:24:18 Dmitry Torokhov wrote:
> > > > > > > > > On Tue, Jan 03, 2017 at 09:05:51PM +0100, Pali Rohár wrote:
> > > > > > > > > > On Tuesday 03 January 2017 20:48:12 Dmitry Torokhov wrote:
> > > > > > > > > > > On Tue, Jan 03, 2017 at 07:50:17PM +0100, Pali Rohár wrote:
> > > > > > > > > > > > On Tuesday 03 January 2017 19:38:43 Dmitry Torokhov wrote:
> > > > > > > > > > > > > On Tue, Jan 03, 2017 at 10:06:41AM +0100, Benjamin Tissoires
> > > > > > > > > > > > >
> > > > > > > > > > > > > wrote:
> > > > > > > > > > > > > > On Dec 29 2016 or thereabouts, Pali Rohár wrote:
> > > > > > > > > > > > > > > On Thursday 29 December 2016 22:09:32 Michał Kępień wrote:
> > > > > > > > > > > > > > > > > On Thursday 29 December 2016 14:47:19 Michał Kępień
> > > > > > > > > > > > > > > > > wrote:
> > > > > > > > > > > > > > > > > > > On Thursday 29 December 2016 09:29:36 Michał
> > > > > > > > > > > > > > > > > > > Kępień
> > > > > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > > > > wrote:
> > > > > > > > > > > > > > > > > > > > > Dell platform team told us that some (DMI
> > > > > > > > > > > > > > > > > > > > > whitelisted) Dell Latitude machines have ST
> > > > > > > > > > > > > > > > > > > > > microelectronics accelerometer at i2c address
> > > > > > > > > > > > > > > > > > > > > 0x29. That i2c address is not specified in
> > > > > > > > > > > > > > > > > > > > > DMI or ACPI, so runtime detection without
> > > > > > > > > > > > > > > > > > > > > whitelist which is below is not possible.
> > > > > > > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > > > > > > Presence of that ST microelectronics
> > > > > > > > > > > > > > > > > > > > > accelerometer is verified by existence of
> > > > > > > > > > > > > > > > > > > > > SMO88xx ACPI device which represent that
> > > > > > > > > > > > > > > > > > > > > accelerometer. Unfortunately without i2c
> > > > > > > > > > > > > > > > > > > > > address.
> > > > > > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > > > > > This part of the commit message sounded a bit
> > > > > > > > > > > > > > > > > > > > confusing to me at first because there is
> > > > > > > > > > > > > > > > > > > > already an ACPI driver which handles SMO88xx
> > > > > > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > > > > > devices (dell-smo8800). My understanding is
> > > > > > > > > > > > > > > > > > > > that:
> > > > > > > > > > > > > > > > > > > > * the purpose of this patch is to expose a
> > > > > > > > > > > > > > > > > > > > richer interface (as
> > > > > > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > > > > > provided by lis3lv02d) to these devices on
> > > > > > > > > > > > > > > > > > > > some machines,
> > > > > > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > > > > > * on whitelisted machines, dell-smo8800 and
> > > > > > > > > > > > > > > > > > > > lis3lv02d can work
> > > > > > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > > > > > simultaneously (even though dell-smo8800
> > > > > > > > > > > > > > > > > > > > effectively duplicates the work that
> > > > > > > > > > > > > > > > > > > > lis3lv02d does).
> > > > > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > > > > No. dell-smo8800 reads from ACPI irq number and
> > > > > > > > > > > > > > > > > > > exports /dev/freefall device which notify
> > > > > > > > > > > > > > > > > > > userspace about falls. lis3lv02d is i2c driver
> > > > > > > > > > > > > > > > > > > which exports axes of accelerometer. Additionaly
> > > > > > > > > > > > > > > > > > > lis3lv02d can export also /dev/freefall if
> > > > > > > > > > > > > > > > > > > registerer of i2c device provides irq number --
> > > > > > > > > > > > > > > > > > > which is not case of this patch.
> > > > > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > > > > So both drivers are doing different things and
> > > > > > > > > > > > > > > > > > > both are useful.
> > > > > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > > > > IIRC both dell-smo8800 and lis3lv02d represent
> > > > > > > > > > > > > > > > > > > one HW device (that ST microelectronics
> > > > > > > > > > > > > > > > > > > accelerometer) but due to complicated HW
> > > > > > > > > > > > > > > > > > > abstraction and layers on Dell laptops it is
> > > > > > > > > > > > > > > > > > > handled by two drivers, one ACPI and one i2c.
> > > > > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > > > > Yes, in ideal world irq number should be passed
> > > > > > > > > > > > > > > > > > > to lis3lv02d driver and that would export whole
> > > > > > > > > > > > > > > > > > > device (with /dev/freefall too), but due to HW
> > > > > > > > > > > > > > > > > > > abstraction it is too much complicated...
> > > > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > > > Why? AFAICT, all that is required to pass that IRQ
> > > > > > > > > > > > > > > > > > number all the way down to lis3lv02d is to set the
> > > > > > > > > > > > > > > > > > irq field of the struct i2c_board_info you are
> > > > > > > > > > > > > > > > > > passing to i2c_new_device(). And you can extract
> > > > > > > > > > > > > > > > > > that IRQ number e.g. in
> > > > > > > > > > > > > > > > > > check_acpi_smo88xx_device(). However, you would
> > > > > > > > > > > > > > > > > > then need to make sure dell-smo8800 does not
> > > > > > > > > > > > > > > > > > attempt to request the same IRQ on whitelisted
> > > > > > > > > > > > > > > > > > machines. This got me thinking about a way to
> > > > > > > > > > > > > > > > > > somehow incorporate your changes into dell-smo8800
> > > > > > > > > > > > > > > > > > using Wolfram's bus_notifier suggestion, but I do
> > > > > > > > > > > > > > > > > > not have a working solution for now. What is
> > > > > > > > > > > > > > > > > > tempting about this approach is that you would not
> > > > > > > > > > > > > > > > > > have to scan the ACPI namespace in search of
> > > > > > > > > > > > > > > > > > SMO88xx devices, because smo8800_add() is
> > > > > > > > > > > > > > > > > > automatically called for them. However, I fear that
> > > > > > > > > > > > > > > > > > the resulting solution may be more complicated than
> > > > > > > > > > > > > > > > > > the one you submitted.
> > > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > > Then we need to deal with lot of problems. Order of
> > > > > > > > > > > > > > > > > loading .ko modules is undefined. Binding devices to
> > > > > > > > > > > > > > > > > drivers registered by .ko module is also in "random"
> > > > > > > > > > > > > > > > > order. At any time any of those .ko module can be
> > > > > > > > > > > > > > > > > unloaded or at least device unbind (via sysfs) from
> > > > > > > > > > > > > > > > > driver... And there can be some pathological
> > > > > > > > > > > > > > > > > situation (thanks to adding ACPI layer as Andy
> > > > > > > > > > > > > > > > > pointed) that there will be more SMO88xx devices in
> > > > > > > > > > > > > > > > > ACPI. Plus you can compile kernel with and without
> > > > > > > > > > > > > > > > > those modules and also you can blacklist loading
> > > > > > > > > > > > > > > > > them (so compile time check is not enough). And
> > > > > > > > > > > > > > > > > still some correct message notifier must be used.
> > > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > > I think such solution is much much more complicated,
> > > > > > > > > > > > > > > > > there are lot of combinations of kernel configuration
> > > > > > > > > > > > > > > > > and available dell devices...
> > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > I tried a few more things, but ultimately failed to
> > > > > > > > > > > > > > > > find a nice way to implement this.
> > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > Another issue popped up, though. Linus' master branch
> > > > > > > > > > > > > > > > contains a recent commit by Benjamin Tissoires (CC'ed),
> > > > > > > > > > > > > > > > 4d5538f5882a ("i2c: use an IRQ to report Host Notify
> > > > > > > > > > > > > > > > events, not alert") which breaks your patch. The
> > > > > > > > > > > > > > > > reason for that is that lis3lv02d relies on the i2c
> > > > > > > > > > > > > > > > client's IRQ being 0 to detect that it should not
> > > > > > > > > > > > > > > > create /dev/freefall.
> > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > Benjamin's patch causes the Host Notify IRQ to be
> > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > assigned to the i2c client your patch creates, thus
> > > > > > > > > > > > > > > > causing lis3lv02d to create /dev/freefall, which in
> > > > > > > > > > > > > > > > turn conflicts with dell-smo8800 which is trying to
> > > > > > > > > > > > > > > > create /dev/freefall itself.
> > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > So 4d5538f5882a is breaking lis3lv02d driver...
> > > > > > > > > > > > > >
> > > > > > > > > > > > > > Apologies for that.
> > > > > > > > > > > > > >
> > > > > > > > > > > > > > I could easily fix this by adding a kernel API to know
> > > > > > > > > > > > > > whether the provided irq is from Host Notify or if it was
> > > > > > > > > > > > > > coming from an actual declaration. However, I have no idea
> > > > > > > > > > > > > > how many other drivers would require this (hopefully only
> > > > > > > > > > > > > > this one).
> > > > > > > > > > > > > >
> > > > > > > > > > > > > > One other solution would be to reserve the Host Notify IRQ
> > > > > > > > > > > > > > and let the actual drivers that need it to set it, but
> > > > > > > > > > > > > > this was not the best solution according to Dmitri. On my
> > > > > > > > > > > > > > side, I am not entirely against this given that it's a
> > > > > > > > > > > > > > chip feature, so the driver should be able to know that
> > > > > > > > > > > > > > it's available.
> > > > > > > > > > > > > >
> > > > > > > > > > > > > > Dmitri, Wolfram, Jean, any preferences?
> > > > > > > > > > > > >
> > > > > > > > > > > > > I read this:
> > > > > > > > > > > > >
> > > > > > > > > > > > > "IIRC both dell-smo8800 and lis3lv02d represent one HW device
> > > > > > > > > > > > > (that ST microelectronics accelerometer) but due to
> > > > > > > > > > > > > complicated HW abstraction and layers on Dell laptops it is
> > > > > > > > > > > > > handled by two drivers, one ACPI and one i2c."
> > > > > > > > > > > > >
> > > > > > > > > > > > > and that is the core of the issue. You have 2 drivers
> > > > > > > > > > > > > fighting over the same device. Fix this and it will all
> > > > > > > > > > > > > work.
> > > > > > > > > > > >
> > > > > > > > > > > > With my current implementation (which I sent in this patch),
> > > > > > > > > > > > they are not fighting.
> > > > > > > > > > > >
> > > > > > > > > > > > dell-smo8800 exports /dev/freefall (and nothing more) and
> > > > > > > > > > > > lis3lv02d only accelerometer device as lis3lv02d driver does
> > > > > > > > > > > > not get IRQ number in platform data.
> > > > > > > > > > > >
> > > > > > > > > > > > > As far as I can see hp_accel instantiates lis3lv02d and
> > > > > > > > > > > > > accesses it via ACPI methods, can the same be done for Dell?
> > > > > > > > > > > >
> > > > > > > > > > > > No, Dell does not have any ACPI methods. And as I wrote in ACPI
> > > > > > > > > > > > or DMI is even not i2c address of device, so it needs to be
> > > > > > > > > > > > specified in code itself.
> > > > > > > > > > > >
> > > > > > > > > > > > Really there is no other way... :-(
> > > > > > > > > > >
> > > > > > > > > > > Sure there is:
> > > > > > > > > > >
> > > > > > > > > > > 1. dell-smo8800 instantiates I2C device as "dell-smo8800-accel".
> > > > > > > > > > > 2. dell-smo8800 provides read/write functions for lis3lv02d that
> > > > > > > > > > > simply forward requests to dell-smo8800-accel i2c client.
> > > > > > > > > > > 3. dell-smo8800 instantiates lis3lv02d instance like hp_accel
> > > > > > > > > > > does.
> > > > > > > > > >
> > > > > > > > > > Sorry, but I do not understand how you mean it... Why to provides
> > > > > > > > > > new read/write i2c functions which are already implemented by
> > > > > > > > > > i2c-i801 bus and lis3lv02d i2c driver?
> > > > > > > > >
> > > > > > > > > Because that would allow you to avoid clashes with i2c creating
> > > > > > > > > interrupt mapping for client residing on host-notify-capable
> > > > > > > > > controller.
> > > > > > > > >
> > > > > > > > > > > Alternatively, can lis3lv02d be tasked to create /dev/freefall?
> > > > > > > > > >
> > > > > > > > > > If i2c_board_info contains IRQ then lis3lv02d create /dev/freefall
> > > > > > > > > > device.
> > > > > > > > > >
> > > > > > > > > > But... what is problem with current implementation? Accelerometer
> > > > > > > > > > HW provides two functions:
> > > > > > > > > >
> > > > > > > > > > 1) 3 axes reports
> > > > > > > > > > 2) Disk freefall detection
> > > > > > > > > >
> > > > > > > > > > And 1) is handled by i2c driver lis3lv02d and 2) is by
> > > > > > > > > > dell-smo8800. Both functions are independent here.
> > > > > > > > > >
> > > > > > > > > > I think you just trying to complicate this situation even more to
> > > > > > > > > > be more complicated as currently is.
> > > > > > > > >
> > > > > > > > > Because this apparently does not work for you, does it?
> > > > > > > >
> > > > > > > > It is working fine. I do not see any problem.
> > > > > > > >
> > > > > > > > > In general,
> > > > > > > > > if you want the same hardware be handled by 2 different drivers you
> > > > > > > > > are going to have bad time.
> > > > > > > >
> > > > > > > > Yes, but in this case half of device is ACPI based and other half i2c
> > > > > > > > based. This is problem of ACPI and Dell design.
> > > > > > > >
> > > > > > > > > It seems to be that /dev/freefall in dell-smo8800 and lis3lv02d are
> > > > > > > > > the same, right?
> > > > > > > >
> > > > > > > > Yes. I understand that clean solution is to have one driver which
> > > > > > > > provides everything.
> > > > > > > >
> > > > > > > > But because half of data are ACPI and half i2c, you still needs to
> > > > > > > > create two drivers (one ACPI and one i2c). You can put both drivers into
> > > > > > > > one .ko module, but still these will be two drivers due to how ACPI and
> > > > > > > > i2c linux abstractions are different.
> > > > > > > >
> > > > > > > > > So, instead of having 2 drivers split the
> > > > > > > > > functionality, can you forego registering smo8800 ACPI driver on
> > > > > > > > > your whitelisted boxes and instead instantiate full i2c client
> > > > > > > > > device with properly assigned both address and IRQ and let lis3lv02d
> > > > > > > > > handle it (providing both accelerometer data and /dev/freefall)?
> > > > > > > >
> > > > > > > > With Michał we already discussed about it, see emails. Basically you can
> > > > > > > > enable/disable kernel modules at compile time or blacklist at runtime
> > > > > > > > (or even chose what will be compiled into vmlinux and what as external
> > > > > > > > .ko module).
> > > > > > >
> > > > > > > This can be solved with a bit of Kconfig/IS_ENABLED() code.
> > > > > > >
> > > > > > > > Some distributions blacklist i2c-i801.ko module... And
> > > > > > >
> > > > > > > Any particular reason for that?
> > > > > > >
> > > > > > > > there can be also problem with initialization of i2c-i801 driver (fix is
> > > > > > > > in commit a7ae81952cda, but does not have to work at every time!). So
> > > > > > > > that move on whitelisted machines can potentially cause disappearance of
> > > > > > > > /dev/freefall and users will not have hdd protection which is currently
> > > > > > > > working.
> > > > > > >
> > > > > > > Well, I gave you 2 possible solutions (roll your own i2c read/write,
> > > > > > > forward them to i2c client) or have faith in your implementation and let
> > > > > > > lis3lv02d handle it.
> > > > > > >
> > > > > > > The 3rd one is to possibly add a flag to disable host notify to IRQ
> > > > > > > mapping for given client (if Wolfram/Jean OK with it).
> > > > > > >
> > > > > > > Oh, the 4th one: change the irq in lis3lv02d.h to be "int" and change
> > > > > > > the check in lis3lv02d.c to be "lis->irq <= 0" and instantiate your
> > > > > > > i2c_client with board_info->irq = -1.
> > > > > > >
> > > > > > > Pick whichever you prefer.
> > > > > > >
> > > > > > > By the way, what do you need accelerometer for on these devices? They
> > > > > > > don't appear to be tablets that could use one...
> > > > > >
> > > > > > Ah, you are talking about problem that after 4d5538f5882a lis3lv02d will
> > > > > > not work... I thought that discussion is about different mechanism how
> > > > > > to implement bus registration notification to smo8800 driver (or
> > > > > > different solution to not have registration in i801).
> > > > > >
> > > > >
> > > > > Just because I am not sure I got everything right, could you confirm
> > > > > that:
> > > > > - in the current upstream tree, the dell-smo8800 driver is now broken
> > > > > after 4d5538f5882a (i2c: use an IRQ to report Host Notify events, not
> > > > > alert)
> > > >
> > > > No, dell-smo8800 it is working fine. It is fully independent from i2c
> > > > and lis3lv02d. It is pure ACPI driver which does not share anything with
> > > > i2c.
> > > >
> > > > > - this series adds an extra lis3lv02d on some machines and you have
> > > > > problem fighting for the irq (but this is not upstream yet).
> > > >
> > > > Yes, this series (not merged yet) adds extra lis3lv02d device but is not
> > > > working because of 4d5538f5882a.
> > > >
> > > > > The extra lis3lv02d node is added from dell-smo8800
> > > >
> > > > No, dell-smo8800 does not add new node in this patch.
> > > >
> > > > > If the first point is not correct (by default, dell-smo8800 will not be
> > > > > loaded at the same time than lis3lv02d), then it's a design issue with
> > > > > the interactions between those 2 drivers.
> > > >
> > > > No, there is no interactions between these two drivers (dell-smo8800 and
> > > > lis3lv02d). dell-smo8800 is pure ACPI driver and exports just
> > > > /dev/freefall device based on IRQ (and nothing more).
> > > >
> > > > And lis3lv02d in *current* configuration in this patch exports only
> > > > accelerometer input device, not /dev/freefall. It does not use IRQ.
> > > > (Just there is problem with 4d5538f5882a which tells lis3lv02d IRQ
> > > > number which is not freefall report, therefore lis3lv02d does not work).
> > > >
> > > > To make it clear, ST Accelerometer provides two operations:
> > > > * report free fall
> > > > * report 3 axes
> > > >
> > > > Free fall is reported by IRQ, state of 3 axes via i2c bus. Free fall IRQ
> > > > is handled by dell-smo8800, state of 3 axes via i2c lis3lv02d driver.
> > > >
> > > > lis3lv02d can handle also free fall IRQ is platform i2c data provides
> > > > IRQ number for it -- but this is not case in our Dell configuration. But
> > > > commit 4d5538f5882a inject some IRQ number to lis3lv02d driver which is
> > > > not free fall detection and so is breaking lis3lv02 driver. In our Dell
> > > > configuration (by this patch) there should be no IRQ number.
> > > >
> > > > It is clear now?
> > >
> > > I think I am starting to understand the problem.
> > >
> > > Currently (upstream tree), 4d5538f5882a doesn't break anything.
> >
> > I cannot prove it... lis3lv02d i2c driver itself uses this IRQ logic
> > (missing = no freefall) and there could be already hardware/boards which
> > register lis3lv02d i2c device without IRQ number. And definitions could
> > be also in device tree and so outside of kernel sources...
> >
> > So there can be potentially break. But at least not case of Dell.
> >
> > > On the
> > > mentioned Dell platforms, the dell-smo8800 gets loaded and provides
> > > /dev/freefall. lis3lv02d is not loaded so everything just works.
> >
> > Yes.
> >
> > > The problem comes from this patch which doesn't set the irq in
> > > i2c_board_info and so i2c_core sets the IRQ to Host Notify. I think
> > > Dmitri already gave you the solution: set the irq to -1 (or -ENOENT)
> > > in i2c_board_info in your patch and only forward it to
> > > lis3lv02d_init_device() only if it is positive (valid).
> >
> > Now I understood that Dmitri means. For this Dell platforms it is OK,
> > but we have a problem for device tree platforms. Normally in device tree
> > if device does not support something you just do not specify it. But
> > with this approach you need to explicitly specify IRQ to -1 in device
> > tree. And I think this is really not a clean solution.
> >
Here I was talking about other platforms/devices (not this Dell one).
> No. DT platforms won't have an issue: they don't change anything, and
> there will be a new /dev/freefall misc device for the platforms that
And this is wrong! There should not be any /dev/freefall device
connected with signal host notify. /dev/freefall is for hardware which
supports free fall hdd detection.
> don't have the irq set *and* if the device is on a Host Notify capable
> adapter, currently only i2c-i801.
I understood, but in future more bus drivers could support host notify.
This is basically incorrect if we use one property for two different
things (host notify and hdd free fall).
For me it looks like that we should separate these two things into two
IRQ properties. It is really wrong design if one property is used for
two different things.
> But given that they are DT and not
> ACPI, this won't conflict with dell-smo8800, so there won't be any
> errors, just a dangling unused device node.
Yes, for upcoming Dell in this patch (with IRQ -1) it is not a problem.
> This approach is IMO the best if you want to have this in the kernel.
>
> Cheers,
> Benjamin
--
Pali Rohár
pali.rohar@gmail.com
^ permalink raw reply
* Re: [PATCH] i2c: i801: Register optional lis3lv02d i2c device on Dell machines
From: Benjamin Tissoires @ 2017-01-04 10:32 UTC (permalink / raw)
To: Pali Rohár
Cc: Dmitry Torokhov, Michał Kępień, Jean Delvare,
Wolfram Sang, Steven Honeyman, Valdis.Kletnieks, Jochen Eisinger,
Gabriele Mazzotta, Andy Lutomirski, Mario_Limonciello, Alex Hung,
Takashi Iwai, linux-i2c, linux-kernel, platform-driver-x86
In-Reply-To: <20170104102118.GD3499@pali>
On Jan 04 2017 or thereabouts, Pali Rohár wrote:
> On Wednesday 04 January 2017 11:13:06 Benjamin Tissoires wrote:
> > On Jan 04 2017 or thereabouts, Pali Rohár wrote:
> > > On Wednesday 04 January 2017 10:05:22 Benjamin Tissoires wrote:
> > > > On Jan 04 2017 or thereabouts, Pali Rohár wrote:
> > > > > On Tuesday 03 January 2017 12:59:37 Dmitry Torokhov wrote:
> > > > > > On Tue, Jan 03, 2017 at 09:39:13PM +0100, Pali Rohár wrote:
> > > > > > > On Tuesday 03 January 2017 21:24:18 Dmitry Torokhov wrote:
> > > > > > > > On Tue, Jan 03, 2017 at 09:05:51PM +0100, Pali Rohár wrote:
> > > > > > > > > On Tuesday 03 January 2017 20:48:12 Dmitry Torokhov wrote:
> > > > > > > > > > On Tue, Jan 03, 2017 at 07:50:17PM +0100, Pali Rohár wrote:
> > > > > > > > > > > On Tuesday 03 January 2017 19:38:43 Dmitry Torokhov wrote:
> > > > > > > > > > > > On Tue, Jan 03, 2017 at 10:06:41AM +0100, Benjamin Tissoires
> > > > > > > > > > > >
> > > > > > > > > > > > wrote:
> > > > > > > > > > > > > On Dec 29 2016 or thereabouts, Pali Rohár wrote:
> > > > > > > > > > > > > > On Thursday 29 December 2016 22:09:32 Michał Kępień wrote:
> > > > > > > > > > > > > > > > On Thursday 29 December 2016 14:47:19 Michał Kępień
> > > > > > > > > > > > > > > > wrote:
> > > > > > > > > > > > > > > > > > On Thursday 29 December 2016 09:29:36 Michał
> > > > > > > > > > > > > > > > > > Kępień
> > > > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > > > wrote:
> > > > > > > > > > > > > > > > > > > > Dell platform team told us that some (DMI
> > > > > > > > > > > > > > > > > > > > whitelisted) Dell Latitude machines have ST
> > > > > > > > > > > > > > > > > > > > microelectronics accelerometer at i2c address
> > > > > > > > > > > > > > > > > > > > 0x29. That i2c address is not specified in
> > > > > > > > > > > > > > > > > > > > DMI or ACPI, so runtime detection without
> > > > > > > > > > > > > > > > > > > > whitelist which is below is not possible.
> > > > > > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > > > > > Presence of that ST microelectronics
> > > > > > > > > > > > > > > > > > > > accelerometer is verified by existence of
> > > > > > > > > > > > > > > > > > > > SMO88xx ACPI device which represent that
> > > > > > > > > > > > > > > > > > > > accelerometer. Unfortunately without i2c
> > > > > > > > > > > > > > > > > > > > address.
> > > > > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > > > > This part of the commit message sounded a bit
> > > > > > > > > > > > > > > > > > > confusing to me at first because there is
> > > > > > > > > > > > > > > > > > > already an ACPI driver which handles SMO88xx
> > > > > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > > > > devices (dell-smo8800). My understanding is
> > > > > > > > > > > > > > > > > > > that:
> > > > > > > > > > > > > > > > > > > * the purpose of this patch is to expose a
> > > > > > > > > > > > > > > > > > > richer interface (as
> > > > > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > > > > provided by lis3lv02d) to these devices on
> > > > > > > > > > > > > > > > > > > some machines,
> > > > > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > > > > * on whitelisted machines, dell-smo8800 and
> > > > > > > > > > > > > > > > > > > lis3lv02d can work
> > > > > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > > > > simultaneously (even though dell-smo8800
> > > > > > > > > > > > > > > > > > > effectively duplicates the work that
> > > > > > > > > > > > > > > > > > > lis3lv02d does).
> > > > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > > > No. dell-smo8800 reads from ACPI irq number and
> > > > > > > > > > > > > > > > > > exports /dev/freefall device which notify
> > > > > > > > > > > > > > > > > > userspace about falls. lis3lv02d is i2c driver
> > > > > > > > > > > > > > > > > > which exports axes of accelerometer. Additionaly
> > > > > > > > > > > > > > > > > > lis3lv02d can export also /dev/freefall if
> > > > > > > > > > > > > > > > > > registerer of i2c device provides irq number --
> > > > > > > > > > > > > > > > > > which is not case of this patch.
> > > > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > > > So both drivers are doing different things and
> > > > > > > > > > > > > > > > > > both are useful.
> > > > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > > > IIRC both dell-smo8800 and lis3lv02d represent
> > > > > > > > > > > > > > > > > > one HW device (that ST microelectronics
> > > > > > > > > > > > > > > > > > accelerometer) but due to complicated HW
> > > > > > > > > > > > > > > > > > abstraction and layers on Dell laptops it is
> > > > > > > > > > > > > > > > > > handled by two drivers, one ACPI and one i2c.
> > > > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > > > Yes, in ideal world irq number should be passed
> > > > > > > > > > > > > > > > > > to lis3lv02d driver and that would export whole
> > > > > > > > > > > > > > > > > > device (with /dev/freefall too), but due to HW
> > > > > > > > > > > > > > > > > > abstraction it is too much complicated...
> > > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > > Why? AFAICT, all that is required to pass that IRQ
> > > > > > > > > > > > > > > > > number all the way down to lis3lv02d is to set the
> > > > > > > > > > > > > > > > > irq field of the struct i2c_board_info you are
> > > > > > > > > > > > > > > > > passing to i2c_new_device(). And you can extract
> > > > > > > > > > > > > > > > > that IRQ number e.g. in
> > > > > > > > > > > > > > > > > check_acpi_smo88xx_device(). However, you would
> > > > > > > > > > > > > > > > > then need to make sure dell-smo8800 does not
> > > > > > > > > > > > > > > > > attempt to request the same IRQ on whitelisted
> > > > > > > > > > > > > > > > > machines. This got me thinking about a way to
> > > > > > > > > > > > > > > > > somehow incorporate your changes into dell-smo8800
> > > > > > > > > > > > > > > > > using Wolfram's bus_notifier suggestion, but I do
> > > > > > > > > > > > > > > > > not have a working solution for now. What is
> > > > > > > > > > > > > > > > > tempting about this approach is that you would not
> > > > > > > > > > > > > > > > > have to scan the ACPI namespace in search of
> > > > > > > > > > > > > > > > > SMO88xx devices, because smo8800_add() is
> > > > > > > > > > > > > > > > > automatically called for them. However, I fear that
> > > > > > > > > > > > > > > > > the resulting solution may be more complicated than
> > > > > > > > > > > > > > > > > the one you submitted.
> > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > Then we need to deal with lot of problems. Order of
> > > > > > > > > > > > > > > > loading .ko modules is undefined. Binding devices to
> > > > > > > > > > > > > > > > drivers registered by .ko module is also in "random"
> > > > > > > > > > > > > > > > order. At any time any of those .ko module can be
> > > > > > > > > > > > > > > > unloaded or at least device unbind (via sysfs) from
> > > > > > > > > > > > > > > > driver... And there can be some pathological
> > > > > > > > > > > > > > > > situation (thanks to adding ACPI layer as Andy
> > > > > > > > > > > > > > > > pointed) that there will be more SMO88xx devices in
> > > > > > > > > > > > > > > > ACPI. Plus you can compile kernel with and without
> > > > > > > > > > > > > > > > those modules and also you can blacklist loading
> > > > > > > > > > > > > > > > them (so compile time check is not enough). And
> > > > > > > > > > > > > > > > still some correct message notifier must be used.
> > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > I think such solution is much much more complicated,
> > > > > > > > > > > > > > > > there are lot of combinations of kernel configuration
> > > > > > > > > > > > > > > > and available dell devices...
> > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > I tried a few more things, but ultimately failed to
> > > > > > > > > > > > > > > find a nice way to implement this.
> > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > Another issue popped up, though. Linus' master branch
> > > > > > > > > > > > > > > contains a recent commit by Benjamin Tissoires (CC'ed),
> > > > > > > > > > > > > > > 4d5538f5882a ("i2c: use an IRQ to report Host Notify
> > > > > > > > > > > > > > > events, not alert") which breaks your patch. The
> > > > > > > > > > > > > > > reason for that is that lis3lv02d relies on the i2c
> > > > > > > > > > > > > > > client's IRQ being 0 to detect that it should not
> > > > > > > > > > > > > > > create /dev/freefall.
> > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > Benjamin's patch causes the Host Notify IRQ to be
> > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > assigned to the i2c client your patch creates, thus
> > > > > > > > > > > > > > > causing lis3lv02d to create /dev/freefall, which in
> > > > > > > > > > > > > > > turn conflicts with dell-smo8800 which is trying to
> > > > > > > > > > > > > > > create /dev/freefall itself.
> > > > > > > > > > > > > >
> > > > > > > > > > > > > > So 4d5538f5882a is breaking lis3lv02d driver...
> > > > > > > > > > > > >
> > > > > > > > > > > > > Apologies for that.
> > > > > > > > > > > > >
> > > > > > > > > > > > > I could easily fix this by adding a kernel API to know
> > > > > > > > > > > > > whether the provided irq is from Host Notify or if it was
> > > > > > > > > > > > > coming from an actual declaration. However, I have no idea
> > > > > > > > > > > > > how many other drivers would require this (hopefully only
> > > > > > > > > > > > > this one).
> > > > > > > > > > > > >
> > > > > > > > > > > > > One other solution would be to reserve the Host Notify IRQ
> > > > > > > > > > > > > and let the actual drivers that need it to set it, but
> > > > > > > > > > > > > this was not the best solution according to Dmitri. On my
> > > > > > > > > > > > > side, I am not entirely against this given that it's a
> > > > > > > > > > > > > chip feature, so the driver should be able to know that
> > > > > > > > > > > > > it's available.
> > > > > > > > > > > > >
> > > > > > > > > > > > > Dmitri, Wolfram, Jean, any preferences?
> > > > > > > > > > > >
> > > > > > > > > > > > I read this:
> > > > > > > > > > > >
> > > > > > > > > > > > "IIRC both dell-smo8800 and lis3lv02d represent one HW device
> > > > > > > > > > > > (that ST microelectronics accelerometer) but due to
> > > > > > > > > > > > complicated HW abstraction and layers on Dell laptops it is
> > > > > > > > > > > > handled by two drivers, one ACPI and one i2c."
> > > > > > > > > > > >
> > > > > > > > > > > > and that is the core of the issue. You have 2 drivers
> > > > > > > > > > > > fighting over the same device. Fix this and it will all
> > > > > > > > > > > > work.
> > > > > > > > > > >
> > > > > > > > > > > With my current implementation (which I sent in this patch),
> > > > > > > > > > > they are not fighting.
> > > > > > > > > > >
> > > > > > > > > > > dell-smo8800 exports /dev/freefall (and nothing more) and
> > > > > > > > > > > lis3lv02d only accelerometer device as lis3lv02d driver does
> > > > > > > > > > > not get IRQ number in platform data.
> > > > > > > > > > >
> > > > > > > > > > > > As far as I can see hp_accel instantiates lis3lv02d and
> > > > > > > > > > > > accesses it via ACPI methods, can the same be done for Dell?
> > > > > > > > > > >
> > > > > > > > > > > No, Dell does not have any ACPI methods. And as I wrote in ACPI
> > > > > > > > > > > or DMI is even not i2c address of device, so it needs to be
> > > > > > > > > > > specified in code itself.
> > > > > > > > > > >
> > > > > > > > > > > Really there is no other way... :-(
> > > > > > > > > >
> > > > > > > > > > Sure there is:
> > > > > > > > > >
> > > > > > > > > > 1. dell-smo8800 instantiates I2C device as "dell-smo8800-accel".
> > > > > > > > > > 2. dell-smo8800 provides read/write functions for lis3lv02d that
> > > > > > > > > > simply forward requests to dell-smo8800-accel i2c client.
> > > > > > > > > > 3. dell-smo8800 instantiates lis3lv02d instance like hp_accel
> > > > > > > > > > does.
> > > > > > > > >
> > > > > > > > > Sorry, but I do not understand how you mean it... Why to provides
> > > > > > > > > new read/write i2c functions which are already implemented by
> > > > > > > > > i2c-i801 bus and lis3lv02d i2c driver?
> > > > > > > >
> > > > > > > > Because that would allow you to avoid clashes with i2c creating
> > > > > > > > interrupt mapping for client residing on host-notify-capable
> > > > > > > > controller.
> > > > > > > >
> > > > > > > > > > Alternatively, can lis3lv02d be tasked to create /dev/freefall?
> > > > > > > > >
> > > > > > > > > If i2c_board_info contains IRQ then lis3lv02d create /dev/freefall
> > > > > > > > > device.
> > > > > > > > >
> > > > > > > > > But... what is problem with current implementation? Accelerometer
> > > > > > > > > HW provides two functions:
> > > > > > > > >
> > > > > > > > > 1) 3 axes reports
> > > > > > > > > 2) Disk freefall detection
> > > > > > > > >
> > > > > > > > > And 1) is handled by i2c driver lis3lv02d and 2) is by
> > > > > > > > > dell-smo8800. Both functions are independent here.
> > > > > > > > >
> > > > > > > > > I think you just trying to complicate this situation even more to
> > > > > > > > > be more complicated as currently is.
> > > > > > > >
> > > > > > > > Because this apparently does not work for you, does it?
> > > > > > >
> > > > > > > It is working fine. I do not see any problem.
> > > > > > >
> > > > > > > > In general,
> > > > > > > > if you want the same hardware be handled by 2 different drivers you
> > > > > > > > are going to have bad time.
> > > > > > >
> > > > > > > Yes, but in this case half of device is ACPI based and other half i2c
> > > > > > > based. This is problem of ACPI and Dell design.
> > > > > > >
> > > > > > > > It seems to be that /dev/freefall in dell-smo8800 and lis3lv02d are
> > > > > > > > the same, right?
> > > > > > >
> > > > > > > Yes. I understand that clean solution is to have one driver which
> > > > > > > provides everything.
> > > > > > >
> > > > > > > But because half of data are ACPI and half i2c, you still needs to
> > > > > > > create two drivers (one ACPI and one i2c). You can put both drivers into
> > > > > > > one .ko module, but still these will be two drivers due to how ACPI and
> > > > > > > i2c linux abstractions are different.
> > > > > > >
> > > > > > > > So, instead of having 2 drivers split the
> > > > > > > > functionality, can you forego registering smo8800 ACPI driver on
> > > > > > > > your whitelisted boxes and instead instantiate full i2c client
> > > > > > > > device with properly assigned both address and IRQ and let lis3lv02d
> > > > > > > > handle it (providing both accelerometer data and /dev/freefall)?
> > > > > > >
> > > > > > > With Michał we already discussed about it, see emails. Basically you can
> > > > > > > enable/disable kernel modules at compile time or blacklist at runtime
> > > > > > > (or even chose what will be compiled into vmlinux and what as external
> > > > > > > .ko module).
> > > > > >
> > > > > > This can be solved with a bit of Kconfig/IS_ENABLED() code.
> > > > > >
> > > > > > > Some distributions blacklist i2c-i801.ko module... And
> > > > > >
> > > > > > Any particular reason for that?
> > > > > >
> > > > > > > there can be also problem with initialization of i2c-i801 driver (fix is
> > > > > > > in commit a7ae81952cda, but does not have to work at every time!). So
> > > > > > > that move on whitelisted machines can potentially cause disappearance of
> > > > > > > /dev/freefall and users will not have hdd protection which is currently
> > > > > > > working.
> > > > > >
> > > > > > Well, I gave you 2 possible solutions (roll your own i2c read/write,
> > > > > > forward them to i2c client) or have faith in your implementation and let
> > > > > > lis3lv02d handle it.
> > > > > >
> > > > > > The 3rd one is to possibly add a flag to disable host notify to IRQ
> > > > > > mapping for given client (if Wolfram/Jean OK with it).
> > > > > >
> > > > > > Oh, the 4th one: change the irq in lis3lv02d.h to be "int" and change
> > > > > > the check in lis3lv02d.c to be "lis->irq <= 0" and instantiate your
> > > > > > i2c_client with board_info->irq = -1.
> > > > > >
> > > > > > Pick whichever you prefer.
> > > > > >
> > > > > > By the way, what do you need accelerometer for on these devices? They
> > > > > > don't appear to be tablets that could use one...
> > > > >
> > > > > Ah, you are talking about problem that after 4d5538f5882a lis3lv02d will
> > > > > not work... I thought that discussion is about different mechanism how
> > > > > to implement bus registration notification to smo8800 driver (or
> > > > > different solution to not have registration in i801).
> > > > >
> > > >
> > > > Just because I am not sure I got everything right, could you confirm
> > > > that:
> > > > - in the current upstream tree, the dell-smo8800 driver is now broken
> > > > after 4d5538f5882a (i2c: use an IRQ to report Host Notify events, not
> > > > alert)
> > >
> > > No, dell-smo8800 it is working fine. It is fully independent from i2c
> > > and lis3lv02d. It is pure ACPI driver which does not share anything with
> > > i2c.
> > >
> > > > - this series adds an extra lis3lv02d on some machines and you have
> > > > problem fighting for the irq (but this is not upstream yet).
> > >
> > > Yes, this series (not merged yet) adds extra lis3lv02d device but is not
> > > working because of 4d5538f5882a.
> > >
> > > > The extra lis3lv02d node is added from dell-smo8800
> > >
> > > No, dell-smo8800 does not add new node in this patch.
> > >
> > > > If the first point is not correct (by default, dell-smo8800 will not be
> > > > loaded at the same time than lis3lv02d), then it's a design issue with
> > > > the interactions between those 2 drivers.
> > >
> > > No, there is no interactions between these two drivers (dell-smo8800 and
> > > lis3lv02d). dell-smo8800 is pure ACPI driver and exports just
> > > /dev/freefall device based on IRQ (and nothing more).
> > >
> > > And lis3lv02d in *current* configuration in this patch exports only
> > > accelerometer input device, not /dev/freefall. It does not use IRQ.
> > > (Just there is problem with 4d5538f5882a which tells lis3lv02d IRQ
> > > number which is not freefall report, therefore lis3lv02d does not work).
> > >
> > > To make it clear, ST Accelerometer provides two operations:
> > > * report free fall
> > > * report 3 axes
> > >
> > > Free fall is reported by IRQ, state of 3 axes via i2c bus. Free fall IRQ
> > > is handled by dell-smo8800, state of 3 axes via i2c lis3lv02d driver.
> > >
> > > lis3lv02d can handle also free fall IRQ is platform i2c data provides
> > > IRQ number for it -- but this is not case in our Dell configuration. But
> > > commit 4d5538f5882a inject some IRQ number to lis3lv02d driver which is
> > > not free fall detection and so is breaking lis3lv02 driver. In our Dell
> > > configuration (by this patch) there should be no IRQ number.
> > >
> > > It is clear now?
> >
> > I think I am starting to understand the problem.
> >
> > Currently (upstream tree), 4d5538f5882a doesn't break anything.
>
> I cannot prove it... lis3lv02d i2c driver itself uses this IRQ logic
> (missing = no freefall) and there could be already hardware/boards which
> register lis3lv02d i2c device without IRQ number. And definitions could
> be also in device tree and so outside of kernel sources...
>
> So there can be potentially break. But at least not case of Dell.
>
> > On the
> > mentioned Dell platforms, the dell-smo8800 gets loaded and provides
> > /dev/freefall. lis3lv02d is not loaded so everything just works.
>
> Yes.
>
> > The problem comes from this patch which doesn't set the irq in
> > i2c_board_info and so i2c_core sets the IRQ to Host Notify. I think
> > Dmitri already gave you the solution: set the irq to -1 (or -ENOENT)
> > in i2c_board_info in your patch and only forward it to
> > lis3lv02d_init_device() only if it is positive (valid).
>
> Now I understood that Dmitri means. For this Dell platforms it is OK,
> but we have a problem for device tree platforms. Normally in device tree
> if device does not support something you just do not specify it. But
> with this approach you need to explicitly specify IRQ to -1 in device
> tree. And I think this is really not a clean solution.
>
No. DT platforms won't have an issue: they don't change anything, and
there will be a new /dev/freefall misc device for the platforms that
don't have the irq set *and* if the device is on a Host Notify capable
adapter, currently only i2c-i801. But given that they are DT and not
ACPI, this won't conflict with dell-smo8800, so there won't be any
errors, just a dangling unused device node.
This approach is IMO the best if you want to have this in the kernel.
Cheers,
Benjamin
^ permalink raw reply
* Re: [PATCH] i2c: i801: Register optional lis3lv02d i2c device on Dell machines
From: Pali Rohár @ 2017-01-04 10:21 UTC (permalink / raw)
To: Benjamin Tissoires
Cc: Dmitry Torokhov, Michał Kępień, Jean Delvare,
Wolfram Sang, Steven Honeyman, Valdis.Kletnieks, Jochen Eisinger,
Gabriele Mazzotta, Andy Lutomirski, Mario_Limonciello, Alex Hung,
Takashi Iwai, linux-i2c, linux-kernel, platform-driver-x86
In-Reply-To: <20170104101306.GR5767@mail.corp.redhat.com>
On Wednesday 04 January 2017 11:13:06 Benjamin Tissoires wrote:
> On Jan 04 2017 or thereabouts, Pali Rohár wrote:
> > On Wednesday 04 January 2017 10:05:22 Benjamin Tissoires wrote:
> > > On Jan 04 2017 or thereabouts, Pali Rohár wrote:
> > > > On Tuesday 03 January 2017 12:59:37 Dmitry Torokhov wrote:
> > > > > On Tue, Jan 03, 2017 at 09:39:13PM +0100, Pali Rohár wrote:
> > > > > > On Tuesday 03 January 2017 21:24:18 Dmitry Torokhov wrote:
> > > > > > > On Tue, Jan 03, 2017 at 09:05:51PM +0100, Pali Rohár wrote:
> > > > > > > > On Tuesday 03 January 2017 20:48:12 Dmitry Torokhov wrote:
> > > > > > > > > On Tue, Jan 03, 2017 at 07:50:17PM +0100, Pali Rohár wrote:
> > > > > > > > > > On Tuesday 03 January 2017 19:38:43 Dmitry Torokhov wrote:
> > > > > > > > > > > On Tue, Jan 03, 2017 at 10:06:41AM +0100, Benjamin Tissoires
> > > > > > > > > > >
> > > > > > > > > > > wrote:
> > > > > > > > > > > > On Dec 29 2016 or thereabouts, Pali Rohár wrote:
> > > > > > > > > > > > > On Thursday 29 December 2016 22:09:32 Michał Kępień wrote:
> > > > > > > > > > > > > > > On Thursday 29 December 2016 14:47:19 Michał Kępień
> > > > > > > > > > > > > > > wrote:
> > > > > > > > > > > > > > > > > On Thursday 29 December 2016 09:29:36 Michał
> > > > > > > > > > > > > > > > > Kępień
> > > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > > wrote:
> > > > > > > > > > > > > > > > > > > Dell platform team told us that some (DMI
> > > > > > > > > > > > > > > > > > > whitelisted) Dell Latitude machines have ST
> > > > > > > > > > > > > > > > > > > microelectronics accelerometer at i2c address
> > > > > > > > > > > > > > > > > > > 0x29. That i2c address is not specified in
> > > > > > > > > > > > > > > > > > > DMI or ACPI, so runtime detection without
> > > > > > > > > > > > > > > > > > > whitelist which is below is not possible.
> > > > > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > > > > Presence of that ST microelectronics
> > > > > > > > > > > > > > > > > > > accelerometer is verified by existence of
> > > > > > > > > > > > > > > > > > > SMO88xx ACPI device which represent that
> > > > > > > > > > > > > > > > > > > accelerometer. Unfortunately without i2c
> > > > > > > > > > > > > > > > > > > address.
> > > > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > > > This part of the commit message sounded a bit
> > > > > > > > > > > > > > > > > > confusing to me at first because there is
> > > > > > > > > > > > > > > > > > already an ACPI driver which handles SMO88xx
> > > > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > > > devices (dell-smo8800). My understanding is
> > > > > > > > > > > > > > > > > > that:
> > > > > > > > > > > > > > > > > > * the purpose of this patch is to expose a
> > > > > > > > > > > > > > > > > > richer interface (as
> > > > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > > > provided by lis3lv02d) to these devices on
> > > > > > > > > > > > > > > > > > some machines,
> > > > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > > > * on whitelisted machines, dell-smo8800 and
> > > > > > > > > > > > > > > > > > lis3lv02d can work
> > > > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > > > simultaneously (even though dell-smo8800
> > > > > > > > > > > > > > > > > > effectively duplicates the work that
> > > > > > > > > > > > > > > > > > lis3lv02d does).
> > > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > > No. dell-smo8800 reads from ACPI irq number and
> > > > > > > > > > > > > > > > > exports /dev/freefall device which notify
> > > > > > > > > > > > > > > > > userspace about falls. lis3lv02d is i2c driver
> > > > > > > > > > > > > > > > > which exports axes of accelerometer. Additionaly
> > > > > > > > > > > > > > > > > lis3lv02d can export also /dev/freefall if
> > > > > > > > > > > > > > > > > registerer of i2c device provides irq number --
> > > > > > > > > > > > > > > > > which is not case of this patch.
> > > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > > So both drivers are doing different things and
> > > > > > > > > > > > > > > > > both are useful.
> > > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > > IIRC both dell-smo8800 and lis3lv02d represent
> > > > > > > > > > > > > > > > > one HW device (that ST microelectronics
> > > > > > > > > > > > > > > > > accelerometer) but due to complicated HW
> > > > > > > > > > > > > > > > > abstraction and layers on Dell laptops it is
> > > > > > > > > > > > > > > > > handled by two drivers, one ACPI and one i2c.
> > > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > > Yes, in ideal world irq number should be passed
> > > > > > > > > > > > > > > > > to lis3lv02d driver and that would export whole
> > > > > > > > > > > > > > > > > device (with /dev/freefall too), but due to HW
> > > > > > > > > > > > > > > > > abstraction it is too much complicated...
> > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > Why? AFAICT, all that is required to pass that IRQ
> > > > > > > > > > > > > > > > number all the way down to lis3lv02d is to set the
> > > > > > > > > > > > > > > > irq field of the struct i2c_board_info you are
> > > > > > > > > > > > > > > > passing to i2c_new_device(). And you can extract
> > > > > > > > > > > > > > > > that IRQ number e.g. in
> > > > > > > > > > > > > > > > check_acpi_smo88xx_device(). However, you would
> > > > > > > > > > > > > > > > then need to make sure dell-smo8800 does not
> > > > > > > > > > > > > > > > attempt to request the same IRQ on whitelisted
> > > > > > > > > > > > > > > > machines. This got me thinking about a way to
> > > > > > > > > > > > > > > > somehow incorporate your changes into dell-smo8800
> > > > > > > > > > > > > > > > using Wolfram's bus_notifier suggestion, but I do
> > > > > > > > > > > > > > > > not have a working solution for now. What is
> > > > > > > > > > > > > > > > tempting about this approach is that you would not
> > > > > > > > > > > > > > > > have to scan the ACPI namespace in search of
> > > > > > > > > > > > > > > > SMO88xx devices, because smo8800_add() is
> > > > > > > > > > > > > > > > automatically called for them. However, I fear that
> > > > > > > > > > > > > > > > the resulting solution may be more complicated than
> > > > > > > > > > > > > > > > the one you submitted.
> > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > Then we need to deal with lot of problems. Order of
> > > > > > > > > > > > > > > loading .ko modules is undefined. Binding devices to
> > > > > > > > > > > > > > > drivers registered by .ko module is also in "random"
> > > > > > > > > > > > > > > order. At any time any of those .ko module can be
> > > > > > > > > > > > > > > unloaded or at least device unbind (via sysfs) from
> > > > > > > > > > > > > > > driver... And there can be some pathological
> > > > > > > > > > > > > > > situation (thanks to adding ACPI layer as Andy
> > > > > > > > > > > > > > > pointed) that there will be more SMO88xx devices in
> > > > > > > > > > > > > > > ACPI. Plus you can compile kernel with and without
> > > > > > > > > > > > > > > those modules and also you can blacklist loading
> > > > > > > > > > > > > > > them (so compile time check is not enough). And
> > > > > > > > > > > > > > > still some correct message notifier must be used.
> > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > I think such solution is much much more complicated,
> > > > > > > > > > > > > > > there are lot of combinations of kernel configuration
> > > > > > > > > > > > > > > and available dell devices...
> > > > > > > > > > > > > >
> > > > > > > > > > > > > > I tried a few more things, but ultimately failed to
> > > > > > > > > > > > > > find a nice way to implement this.
> > > > > > > > > > > > > >
> > > > > > > > > > > > > > Another issue popped up, though. Linus' master branch
> > > > > > > > > > > > > > contains a recent commit by Benjamin Tissoires (CC'ed),
> > > > > > > > > > > > > > 4d5538f5882a ("i2c: use an IRQ to report Host Notify
> > > > > > > > > > > > > > events, not alert") which breaks your patch. The
> > > > > > > > > > > > > > reason for that is that lis3lv02d relies on the i2c
> > > > > > > > > > > > > > client's IRQ being 0 to detect that it should not
> > > > > > > > > > > > > > create /dev/freefall.
> > > > > > > > > > > > > >
> > > > > > > > > > > > > > Benjamin's patch causes the Host Notify IRQ to be
> > > > > > > > > > > > > >
> > > > > > > > > > > > > > assigned to the i2c client your patch creates, thus
> > > > > > > > > > > > > > causing lis3lv02d to create /dev/freefall, which in
> > > > > > > > > > > > > > turn conflicts with dell-smo8800 which is trying to
> > > > > > > > > > > > > > create /dev/freefall itself.
> > > > > > > > > > > > >
> > > > > > > > > > > > > So 4d5538f5882a is breaking lis3lv02d driver...
> > > > > > > > > > > >
> > > > > > > > > > > > Apologies for that.
> > > > > > > > > > > >
> > > > > > > > > > > > I could easily fix this by adding a kernel API to know
> > > > > > > > > > > > whether the provided irq is from Host Notify or if it was
> > > > > > > > > > > > coming from an actual declaration. However, I have no idea
> > > > > > > > > > > > how many other drivers would require this (hopefully only
> > > > > > > > > > > > this one).
> > > > > > > > > > > >
> > > > > > > > > > > > One other solution would be to reserve the Host Notify IRQ
> > > > > > > > > > > > and let the actual drivers that need it to set it, but
> > > > > > > > > > > > this was not the best solution according to Dmitri. On my
> > > > > > > > > > > > side, I am not entirely against this given that it's a
> > > > > > > > > > > > chip feature, so the driver should be able to know that
> > > > > > > > > > > > it's available.
> > > > > > > > > > > >
> > > > > > > > > > > > Dmitri, Wolfram, Jean, any preferences?
> > > > > > > > > > >
> > > > > > > > > > > I read this:
> > > > > > > > > > >
> > > > > > > > > > > "IIRC both dell-smo8800 and lis3lv02d represent one HW device
> > > > > > > > > > > (that ST microelectronics accelerometer) but due to
> > > > > > > > > > > complicated HW abstraction and layers on Dell laptops it is
> > > > > > > > > > > handled by two drivers, one ACPI and one i2c."
> > > > > > > > > > >
> > > > > > > > > > > and that is the core of the issue. You have 2 drivers
> > > > > > > > > > > fighting over the same device. Fix this and it will all
> > > > > > > > > > > work.
> > > > > > > > > >
> > > > > > > > > > With my current implementation (which I sent in this patch),
> > > > > > > > > > they are not fighting.
> > > > > > > > > >
> > > > > > > > > > dell-smo8800 exports /dev/freefall (and nothing more) and
> > > > > > > > > > lis3lv02d only accelerometer device as lis3lv02d driver does
> > > > > > > > > > not get IRQ number in platform data.
> > > > > > > > > >
> > > > > > > > > > > As far as I can see hp_accel instantiates lis3lv02d and
> > > > > > > > > > > accesses it via ACPI methods, can the same be done for Dell?
> > > > > > > > > >
> > > > > > > > > > No, Dell does not have any ACPI methods. And as I wrote in ACPI
> > > > > > > > > > or DMI is even not i2c address of device, so it needs to be
> > > > > > > > > > specified in code itself.
> > > > > > > > > >
> > > > > > > > > > Really there is no other way... :-(
> > > > > > > > >
> > > > > > > > > Sure there is:
> > > > > > > > >
> > > > > > > > > 1. dell-smo8800 instantiates I2C device as "dell-smo8800-accel".
> > > > > > > > > 2. dell-smo8800 provides read/write functions for lis3lv02d that
> > > > > > > > > simply forward requests to dell-smo8800-accel i2c client.
> > > > > > > > > 3. dell-smo8800 instantiates lis3lv02d instance like hp_accel
> > > > > > > > > does.
> > > > > > > >
> > > > > > > > Sorry, but I do not understand how you mean it... Why to provides
> > > > > > > > new read/write i2c functions which are already implemented by
> > > > > > > > i2c-i801 bus and lis3lv02d i2c driver?
> > > > > > >
> > > > > > > Because that would allow you to avoid clashes with i2c creating
> > > > > > > interrupt mapping for client residing on host-notify-capable
> > > > > > > controller.
> > > > > > >
> > > > > > > > > Alternatively, can lis3lv02d be tasked to create /dev/freefall?
> > > > > > > >
> > > > > > > > If i2c_board_info contains IRQ then lis3lv02d create /dev/freefall
> > > > > > > > device.
> > > > > > > >
> > > > > > > > But... what is problem with current implementation? Accelerometer
> > > > > > > > HW provides two functions:
> > > > > > > >
> > > > > > > > 1) 3 axes reports
> > > > > > > > 2) Disk freefall detection
> > > > > > > >
> > > > > > > > And 1) is handled by i2c driver lis3lv02d and 2) is by
> > > > > > > > dell-smo8800. Both functions are independent here.
> > > > > > > >
> > > > > > > > I think you just trying to complicate this situation even more to
> > > > > > > > be more complicated as currently is.
> > > > > > >
> > > > > > > Because this apparently does not work for you, does it?
> > > > > >
> > > > > > It is working fine. I do not see any problem.
> > > > > >
> > > > > > > In general,
> > > > > > > if you want the same hardware be handled by 2 different drivers you
> > > > > > > are going to have bad time.
> > > > > >
> > > > > > Yes, but in this case half of device is ACPI based and other half i2c
> > > > > > based. This is problem of ACPI and Dell design.
> > > > > >
> > > > > > > It seems to be that /dev/freefall in dell-smo8800 and lis3lv02d are
> > > > > > > the same, right?
> > > > > >
> > > > > > Yes. I understand that clean solution is to have one driver which
> > > > > > provides everything.
> > > > > >
> > > > > > But because half of data are ACPI and half i2c, you still needs to
> > > > > > create two drivers (one ACPI and one i2c). You can put both drivers into
> > > > > > one .ko module, but still these will be two drivers due to how ACPI and
> > > > > > i2c linux abstractions are different.
> > > > > >
> > > > > > > So, instead of having 2 drivers split the
> > > > > > > functionality, can you forego registering smo8800 ACPI driver on
> > > > > > > your whitelisted boxes and instead instantiate full i2c client
> > > > > > > device with properly assigned both address and IRQ and let lis3lv02d
> > > > > > > handle it (providing both accelerometer data and /dev/freefall)?
> > > > > >
> > > > > > With Michał we already discussed about it, see emails. Basically you can
> > > > > > enable/disable kernel modules at compile time or blacklist at runtime
> > > > > > (or even chose what will be compiled into vmlinux and what as external
> > > > > > .ko module).
> > > > >
> > > > > This can be solved with a bit of Kconfig/IS_ENABLED() code.
> > > > >
> > > > > > Some distributions blacklist i2c-i801.ko module... And
> > > > >
> > > > > Any particular reason for that?
> > > > >
> > > > > > there can be also problem with initialization of i2c-i801 driver (fix is
> > > > > > in commit a7ae81952cda, but does not have to work at every time!). So
> > > > > > that move on whitelisted machines can potentially cause disappearance of
> > > > > > /dev/freefall and users will not have hdd protection which is currently
> > > > > > working.
> > > > >
> > > > > Well, I gave you 2 possible solutions (roll your own i2c read/write,
> > > > > forward them to i2c client) or have faith in your implementation and let
> > > > > lis3lv02d handle it.
> > > > >
> > > > > The 3rd one is to possibly add a flag to disable host notify to IRQ
> > > > > mapping for given client (if Wolfram/Jean OK with it).
> > > > >
> > > > > Oh, the 4th one: change the irq in lis3lv02d.h to be "int" and change
> > > > > the check in lis3lv02d.c to be "lis->irq <= 0" and instantiate your
> > > > > i2c_client with board_info->irq = -1.
> > > > >
> > > > > Pick whichever you prefer.
> > > > >
> > > > > By the way, what do you need accelerometer for on these devices? They
> > > > > don't appear to be tablets that could use one...
> > > >
> > > > Ah, you are talking about problem that after 4d5538f5882a lis3lv02d will
> > > > not work... I thought that discussion is about different mechanism how
> > > > to implement bus registration notification to smo8800 driver (or
> > > > different solution to not have registration in i801).
> > > >
> > >
> > > Just because I am not sure I got everything right, could you confirm
> > > that:
> > > - in the current upstream tree, the dell-smo8800 driver is now broken
> > > after 4d5538f5882a (i2c: use an IRQ to report Host Notify events, not
> > > alert)
> >
> > No, dell-smo8800 it is working fine. It is fully independent from i2c
> > and lis3lv02d. It is pure ACPI driver which does not share anything with
> > i2c.
> >
> > > - this series adds an extra lis3lv02d on some machines and you have
> > > problem fighting for the irq (but this is not upstream yet).
> >
> > Yes, this series (not merged yet) adds extra lis3lv02d device but is not
> > working because of 4d5538f5882a.
> >
> > > The extra lis3lv02d node is added from dell-smo8800
> >
> > No, dell-smo8800 does not add new node in this patch.
> >
> > > If the first point is not correct (by default, dell-smo8800 will not be
> > > loaded at the same time than lis3lv02d), then it's a design issue with
> > > the interactions between those 2 drivers.
> >
> > No, there is no interactions between these two drivers (dell-smo8800 and
> > lis3lv02d). dell-smo8800 is pure ACPI driver and exports just
> > /dev/freefall device based on IRQ (and nothing more).
> >
> > And lis3lv02d in *current* configuration in this patch exports only
> > accelerometer input device, not /dev/freefall. It does not use IRQ.
> > (Just there is problem with 4d5538f5882a which tells lis3lv02d IRQ
> > number which is not freefall report, therefore lis3lv02d does not work).
> >
> > To make it clear, ST Accelerometer provides two operations:
> > * report free fall
> > * report 3 axes
> >
> > Free fall is reported by IRQ, state of 3 axes via i2c bus. Free fall IRQ
> > is handled by dell-smo8800, state of 3 axes via i2c lis3lv02d driver.
> >
> > lis3lv02d can handle also free fall IRQ is platform i2c data provides
> > IRQ number for it -- but this is not case in our Dell configuration. But
> > commit 4d5538f5882a inject some IRQ number to lis3lv02d driver which is
> > not free fall detection and so is breaking lis3lv02 driver. In our Dell
> > configuration (by this patch) there should be no IRQ number.
> >
> > It is clear now?
>
> I think I am starting to understand the problem.
>
> Currently (upstream tree), 4d5538f5882a doesn't break anything.
I cannot prove it... lis3lv02d i2c driver itself uses this IRQ logic
(missing = no freefall) and there could be already hardware/boards which
register lis3lv02d i2c device without IRQ number. And definitions could
be also in device tree and so outside of kernel sources...
So there can be potentially break. But at least not case of Dell.
> On the
> mentioned Dell platforms, the dell-smo8800 gets loaded and provides
> /dev/freefall. lis3lv02d is not loaded so everything just works.
Yes.
> The problem comes from this patch which doesn't set the irq in
> i2c_board_info and so i2c_core sets the IRQ to Host Notify. I think
> Dmitri already gave you the solution: set the irq to -1 (or -ENOENT)
> in i2c_board_info in your patch and only forward it to
> lis3lv02d_init_device() only if it is positive (valid).
Now I understood that Dmitri means. For this Dell platforms it is OK,
but we have a problem for device tree platforms. Normally in device tree
if device does not support something you just do not specify it. But
with this approach you need to explicitly specify IRQ to -1 in device
tree. And I think this is really not a clean solution.
> So, to me 4d5538f5882a doesn't introduce a regression so we should keep
> it in its current state.
>
> Cheers,
> Benjamin
>
> >
> > > If the first point is correct because ACPI declares both devices, then
> > > there is an urgent fix to propose to not enable Host Notify by default
> > > on Host Notifier capable adapters. (even though the design between the
> > > 2 drivers is wrong, it's considered as a regression).
> > >
> > > Cheers,
> > > Benjamin
> >
> > --
> > Pali Rohár
> > pali.rohar@gmail.com
--
Pali Rohár
pali.rohar@gmail.com
^ permalink raw reply
* Re: [PATCH] i2c: i801: Register optional lis3lv02d i2c device on Dell machines
From: Benjamin Tissoires @ 2017-01-04 10:25 UTC (permalink / raw)
To: Andy Shevchenko
Cc: Pali Rohár, Dmitry Torokhov, Michał Kępień,
Jean Delvare, Wolfram Sang, Steven Honeyman, Valdis Kletnieks,
Jochen Eisinger, Gabriele Mazzotta, Andy Lutomirski,
Mario Limonciello, Alex Hung, Takashi Iwai, linux-i2c,
linux-kernel@vger.kernel.org, Platform Driver
In-Reply-To: <CAHp75Vd2WQRHiVpqCeeNEdcup4peD_28SF1--cRr+uqAk-yY0g@mail.gmail.com>
On Jan 04 2017 or thereabouts, Andy Shevchenko wrote:
> On Tue, Jan 3, 2017 at 10:39 PM, Pali Rohár <pali.rohar@gmail.com> wrote:
> >
> > With Michał we already discussed about it, see emails. Basically you can
> > enable/disable kernel modules at compile time or blacklist at runtime
> > (or even chose what will be compiled into vmlinux and what as external
> > .ko module). Some distributions blacklist i2c-i801.ko module...
>
> But you understand that any of compile/not compile is not an option, right?
> The case which we face will be both of them, if possible, will be
> compiled as modules.
>
> Blacklisting means making your problem the actual user's one. Not good.
>
> > And
> > there can be also problem with initialization of i2c-i801 driver (fix is
> > in commit a7ae81952cda, but does not have to work at every time!). So
> > that move on whitelisted machines can potentially cause disappearance of
> > /dev/freefall and users will not have hdd protection which is currently
> > working.
>
I am seeing the same issues with psmouse and SMBus touchpads. The PS/2
device knows about the availability of a better but unlisted device at
the ACPI level.
The way I solved this to not have to deal with compile/not compile and
runtime errors is the same way Wolfram told you about: bus notifiers.
I also use an intermediate platform driver to not add i2c dependency on
psmouse.
For you the solution would be:
- In dell-smo8800, after checking the whitelist, add a platform driver
"dell-lis3lv02d-platform", and add in the platform_data the I2C address
of the chip.
- create a new driver dell-lis3lv02d-platform.ko which listens for the
i2c bus creation and registers the lis3lv02d I2C node when it sees a
matching adapter. (see [1] for my solution)
- in dell-lis3lv02d-platform.ko make sure to set the irq to -ENOENT so
that lis3lv02d.ko doesn't create /dev/freefall which will still be
handled by ACPI.
How does that sound?
Cheers,
Benjamin
[1] https://github.com/bentiss/linux/blob/synaptics-rmi4-v4.9-rc7+/drivers/input/rmi4/rmi_platform.c
^ permalink raw reply
* Re: [PATCH v4 00/14] ARM: dts: r8a779x: use demuxer for I2C
From: Wolfram Sang @ 2017-01-04 10:25 UTC (permalink / raw)
To: Niklas Söderlund
Cc: Simon Horman, Wolfram Sang, linux-renesas-soc, linux-i2c
In-Reply-To: <20161110153016.GJ26304@bigcity.dyn.berto.se>
[-- Attachment #1: Type: text/plain, Size: 657 bytes --]
Bump...
> Yes, this is a bug in the rcar-vin driver which is addressed in the Gen3
> patches. However I'm not sure those patches will make it to v4.10, not
> much review from the V4L2 side yet (Geert and Sergei have had a few
> comments so there will at lest be one more iteration).
Seems your predictions were correct. I still get:
* OOPS with renesas/dt-for-v4.11 and these patches on top
* no OOPS with renesas-drivers/master and these patches on top
So, it seems to me that we still should wait with the HDMI related
patches of this series until the rcar-vin-gen3 branch is in -next.
D'accord everyone?
Regards,
Wolfram
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply
* Re: [PATCH] i2c: i801: Register optional lis3lv02d i2c device on Dell machines
From: Jean Delvare @ 2017-01-04 10:18 UTC (permalink / raw)
To: Benjamin Tissoires
Cc: Pali Rohár, Michał Kępień, Wolfram Sang,
Steven Honeyman, Valdis.Kletnieks, Jochen Eisinger,
Gabriele Mazzotta, Andy Lutomirski, Mario_Limonciello, Alex Hung,
Takashi Iwai, linux-i2c, linux-kernel, platform-driver-x86,
Dmitry Torokhov
In-Reply-To: <20170103090641.GH5767@mail.corp.redhat.com>
On Tue, 3 Jan 2017 10:06:41 +0100, Benjamin Tissoires wrote:
> On Dec 29 2016 or thereabouts, Pali Rohár wrote:
> > So 4d5538f5882a is breaking lis3lv02d driver...
>
> Apologies for that.
>
> I could easily fix this by adding a kernel API to know whether the
> provided irq is from Host Notify or if it was coming from an actual
> declaration. However, I have no idea how many other drivers would
> require this (hopefully only this one).
>
> One other solution would be to reserve the Host Notify IRQ and let the
> actual drivers that need it to set it, but this was not the best
> solution according to Dmitri. On my side, I am not entirely against this
> given that it's a chip feature, so the driver should be able to know
> that it's available.
>
> Dmitri, Wolfram, Jean, any preferences?
I did not look at your work so I do not feel qualified to give advice
on the proper way. I trust you to make the right decision :-)
--
Jean Delvare
SUSE L3 Support
^ permalink raw reply
* Re: [PATCH] i2c: i801: Register optional lis3lv02d i2c device on Dell machines
From: Benjamin Tissoires @ 2017-01-04 10:13 UTC (permalink / raw)
To: Pali Rohár
Cc: Dmitry Torokhov, Michał Kępień, Jean Delvare,
Wolfram Sang, Steven Honeyman, Valdis.Kletnieks, Jochen Eisinger,
Gabriele Mazzotta, Andy Lutomirski, Mario_Limonciello, Alex Hung,
Takashi Iwai, linux-i2c, linux-kernel, platform-driver-x86
In-Reply-To: <20170104091845.GB3499@pali>
On Jan 04 2017 or thereabouts, Pali Rohár wrote:
> On Wednesday 04 January 2017 10:05:22 Benjamin Tissoires wrote:
> > On Jan 04 2017 or thereabouts, Pali Rohár wrote:
> > > On Tuesday 03 January 2017 12:59:37 Dmitry Torokhov wrote:
> > > > On Tue, Jan 03, 2017 at 09:39:13PM +0100, Pali Rohár wrote:
> > > > > On Tuesday 03 January 2017 21:24:18 Dmitry Torokhov wrote:
> > > > > > On Tue, Jan 03, 2017 at 09:05:51PM +0100, Pali Rohár wrote:
> > > > > > > On Tuesday 03 January 2017 20:48:12 Dmitry Torokhov wrote:
> > > > > > > > On Tue, Jan 03, 2017 at 07:50:17PM +0100, Pali Rohár wrote:
> > > > > > > > > On Tuesday 03 January 2017 19:38:43 Dmitry Torokhov wrote:
> > > > > > > > > > On Tue, Jan 03, 2017 at 10:06:41AM +0100, Benjamin Tissoires
> > > > > > > > > >
> > > > > > > > > > wrote:
> > > > > > > > > > > On Dec 29 2016 or thereabouts, Pali Rohár wrote:
> > > > > > > > > > > > On Thursday 29 December 2016 22:09:32 Michał Kępień wrote:
> > > > > > > > > > > > > > On Thursday 29 December 2016 14:47:19 Michał Kępień
> > > > > > > > > > > > > > wrote:
> > > > > > > > > > > > > > > > On Thursday 29 December 2016 09:29:36 Michał
> > > > > > > > > > > > > > > > Kępień
> > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > wrote:
> > > > > > > > > > > > > > > > > > Dell platform team told us that some (DMI
> > > > > > > > > > > > > > > > > > whitelisted) Dell Latitude machines have ST
> > > > > > > > > > > > > > > > > > microelectronics accelerometer at i2c address
> > > > > > > > > > > > > > > > > > 0x29. That i2c address is not specified in
> > > > > > > > > > > > > > > > > > DMI or ACPI, so runtime detection without
> > > > > > > > > > > > > > > > > > whitelist which is below is not possible.
> > > > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > > > Presence of that ST microelectronics
> > > > > > > > > > > > > > > > > > accelerometer is verified by existence of
> > > > > > > > > > > > > > > > > > SMO88xx ACPI device which represent that
> > > > > > > > > > > > > > > > > > accelerometer. Unfortunately without i2c
> > > > > > > > > > > > > > > > > > address.
> > > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > > This part of the commit message sounded a bit
> > > > > > > > > > > > > > > > > confusing to me at first because there is
> > > > > > > > > > > > > > > > > already an ACPI driver which handles SMO88xx
> > > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > > devices (dell-smo8800). My understanding is
> > > > > > > > > > > > > > > > > that:
> > > > > > > > > > > > > > > > > * the purpose of this patch is to expose a
> > > > > > > > > > > > > > > > > richer interface (as
> > > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > > provided by lis3lv02d) to these devices on
> > > > > > > > > > > > > > > > > some machines,
> > > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > > * on whitelisted machines, dell-smo8800 and
> > > > > > > > > > > > > > > > > lis3lv02d can work
> > > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > > simultaneously (even though dell-smo8800
> > > > > > > > > > > > > > > > > effectively duplicates the work that
> > > > > > > > > > > > > > > > > lis3lv02d does).
> > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > No. dell-smo8800 reads from ACPI irq number and
> > > > > > > > > > > > > > > > exports /dev/freefall device which notify
> > > > > > > > > > > > > > > > userspace about falls. lis3lv02d is i2c driver
> > > > > > > > > > > > > > > > which exports axes of accelerometer. Additionaly
> > > > > > > > > > > > > > > > lis3lv02d can export also /dev/freefall if
> > > > > > > > > > > > > > > > registerer of i2c device provides irq number --
> > > > > > > > > > > > > > > > which is not case of this patch.
> > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > So both drivers are doing different things and
> > > > > > > > > > > > > > > > both are useful.
> > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > IIRC both dell-smo8800 and lis3lv02d represent
> > > > > > > > > > > > > > > > one HW device (that ST microelectronics
> > > > > > > > > > > > > > > > accelerometer) but due to complicated HW
> > > > > > > > > > > > > > > > abstraction and layers on Dell laptops it is
> > > > > > > > > > > > > > > > handled by two drivers, one ACPI and one i2c.
> > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > Yes, in ideal world irq number should be passed
> > > > > > > > > > > > > > > > to lis3lv02d driver and that would export whole
> > > > > > > > > > > > > > > > device (with /dev/freefall too), but due to HW
> > > > > > > > > > > > > > > > abstraction it is too much complicated...
> > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > Why? AFAICT, all that is required to pass that IRQ
> > > > > > > > > > > > > > > number all the way down to lis3lv02d is to set the
> > > > > > > > > > > > > > > irq field of the struct i2c_board_info you are
> > > > > > > > > > > > > > > passing to i2c_new_device(). And you can extract
> > > > > > > > > > > > > > > that IRQ number e.g. in
> > > > > > > > > > > > > > > check_acpi_smo88xx_device(). However, you would
> > > > > > > > > > > > > > > then need to make sure dell-smo8800 does not
> > > > > > > > > > > > > > > attempt to request the same IRQ on whitelisted
> > > > > > > > > > > > > > > machines. This got me thinking about a way to
> > > > > > > > > > > > > > > somehow incorporate your changes into dell-smo8800
> > > > > > > > > > > > > > > using Wolfram's bus_notifier suggestion, but I do
> > > > > > > > > > > > > > > not have a working solution for now. What is
> > > > > > > > > > > > > > > tempting about this approach is that you would not
> > > > > > > > > > > > > > > have to scan the ACPI namespace in search of
> > > > > > > > > > > > > > > SMO88xx devices, because smo8800_add() is
> > > > > > > > > > > > > > > automatically called for them. However, I fear that
> > > > > > > > > > > > > > > the resulting solution may be more complicated than
> > > > > > > > > > > > > > > the one you submitted.
> > > > > > > > > > > > > >
> > > > > > > > > > > > > > Then we need to deal with lot of problems. Order of
> > > > > > > > > > > > > > loading .ko modules is undefined. Binding devices to
> > > > > > > > > > > > > > drivers registered by .ko module is also in "random"
> > > > > > > > > > > > > > order. At any time any of those .ko module can be
> > > > > > > > > > > > > > unloaded or at least device unbind (via sysfs) from
> > > > > > > > > > > > > > driver... And there can be some pathological
> > > > > > > > > > > > > > situation (thanks to adding ACPI layer as Andy
> > > > > > > > > > > > > > pointed) that there will be more SMO88xx devices in
> > > > > > > > > > > > > > ACPI. Plus you can compile kernel with and without
> > > > > > > > > > > > > > those modules and also you can blacklist loading
> > > > > > > > > > > > > > them (so compile time check is not enough). And
> > > > > > > > > > > > > > still some correct message notifier must be used.
> > > > > > > > > > > > > >
> > > > > > > > > > > > > > I think such solution is much much more complicated,
> > > > > > > > > > > > > > there are lot of combinations of kernel configuration
> > > > > > > > > > > > > > and available dell devices...
> > > > > > > > > > > > >
> > > > > > > > > > > > > I tried a few more things, but ultimately failed to
> > > > > > > > > > > > > find a nice way to implement this.
> > > > > > > > > > > > >
> > > > > > > > > > > > > Another issue popped up, though. Linus' master branch
> > > > > > > > > > > > > contains a recent commit by Benjamin Tissoires (CC'ed),
> > > > > > > > > > > > > 4d5538f5882a ("i2c: use an IRQ to report Host Notify
> > > > > > > > > > > > > events, not alert") which breaks your patch. The
> > > > > > > > > > > > > reason for that is that lis3lv02d relies on the i2c
> > > > > > > > > > > > > client's IRQ being 0 to detect that it should not
> > > > > > > > > > > > > create /dev/freefall.
> > > > > > > > > > > > >
> > > > > > > > > > > > > Benjamin's patch causes the Host Notify IRQ to be
> > > > > > > > > > > > >
> > > > > > > > > > > > > assigned to the i2c client your patch creates, thus
> > > > > > > > > > > > > causing lis3lv02d to create /dev/freefall, which in
> > > > > > > > > > > > > turn conflicts with dell-smo8800 which is trying to
> > > > > > > > > > > > > create /dev/freefall itself.
> > > > > > > > > > > >
> > > > > > > > > > > > So 4d5538f5882a is breaking lis3lv02d driver...
> > > > > > > > > > >
> > > > > > > > > > > Apologies for that.
> > > > > > > > > > >
> > > > > > > > > > > I could easily fix this by adding a kernel API to know
> > > > > > > > > > > whether the provided irq is from Host Notify or if it was
> > > > > > > > > > > coming from an actual declaration. However, I have no idea
> > > > > > > > > > > how many other drivers would require this (hopefully only
> > > > > > > > > > > this one).
> > > > > > > > > > >
> > > > > > > > > > > One other solution would be to reserve the Host Notify IRQ
> > > > > > > > > > > and let the actual drivers that need it to set it, but
> > > > > > > > > > > this was not the best solution according to Dmitri. On my
> > > > > > > > > > > side, I am not entirely against this given that it's a
> > > > > > > > > > > chip feature, so the driver should be able to know that
> > > > > > > > > > > it's available.
> > > > > > > > > > >
> > > > > > > > > > > Dmitri, Wolfram, Jean, any preferences?
> > > > > > > > > >
> > > > > > > > > > I read this:
> > > > > > > > > >
> > > > > > > > > > "IIRC both dell-smo8800 and lis3lv02d represent one HW device
> > > > > > > > > > (that ST microelectronics accelerometer) but due to
> > > > > > > > > > complicated HW abstraction and layers on Dell laptops it is
> > > > > > > > > > handled by two drivers, one ACPI and one i2c."
> > > > > > > > > >
> > > > > > > > > > and that is the core of the issue. You have 2 drivers
> > > > > > > > > > fighting over the same device. Fix this and it will all
> > > > > > > > > > work.
> > > > > > > > >
> > > > > > > > > With my current implementation (which I sent in this patch),
> > > > > > > > > they are not fighting.
> > > > > > > > >
> > > > > > > > > dell-smo8800 exports /dev/freefall (and nothing more) and
> > > > > > > > > lis3lv02d only accelerometer device as lis3lv02d driver does
> > > > > > > > > not get IRQ number in platform data.
> > > > > > > > >
> > > > > > > > > > As far as I can see hp_accel instantiates lis3lv02d and
> > > > > > > > > > accesses it via ACPI methods, can the same be done for Dell?
> > > > > > > > >
> > > > > > > > > No, Dell does not have any ACPI methods. And as I wrote in ACPI
> > > > > > > > > or DMI is even not i2c address of device, so it needs to be
> > > > > > > > > specified in code itself.
> > > > > > > > >
> > > > > > > > > Really there is no other way... :-(
> > > > > > > >
> > > > > > > > Sure there is:
> > > > > > > >
> > > > > > > > 1. dell-smo8800 instantiates I2C device as "dell-smo8800-accel".
> > > > > > > > 2. dell-smo8800 provides read/write functions for lis3lv02d that
> > > > > > > > simply forward requests to dell-smo8800-accel i2c client.
> > > > > > > > 3. dell-smo8800 instantiates lis3lv02d instance like hp_accel
> > > > > > > > does.
> > > > > > >
> > > > > > > Sorry, but I do not understand how you mean it... Why to provides
> > > > > > > new read/write i2c functions which are already implemented by
> > > > > > > i2c-i801 bus and lis3lv02d i2c driver?
> > > > > >
> > > > > > Because that would allow you to avoid clashes with i2c creating
> > > > > > interrupt mapping for client residing on host-notify-capable
> > > > > > controller.
> > > > > >
> > > > > > > > Alternatively, can lis3lv02d be tasked to create /dev/freefall?
> > > > > > >
> > > > > > > If i2c_board_info contains IRQ then lis3lv02d create /dev/freefall
> > > > > > > device.
> > > > > > >
> > > > > > > But... what is problem with current implementation? Accelerometer
> > > > > > > HW provides two functions:
> > > > > > >
> > > > > > > 1) 3 axes reports
> > > > > > > 2) Disk freefall detection
> > > > > > >
> > > > > > > And 1) is handled by i2c driver lis3lv02d and 2) is by
> > > > > > > dell-smo8800. Both functions are independent here.
> > > > > > >
> > > > > > > I think you just trying to complicate this situation even more to
> > > > > > > be more complicated as currently is.
> > > > > >
> > > > > > Because this apparently does not work for you, does it?
> > > > >
> > > > > It is working fine. I do not see any problem.
> > > > >
> > > > > > In general,
> > > > > > if you want the same hardware be handled by 2 different drivers you
> > > > > > are going to have bad time.
> > > > >
> > > > > Yes, but in this case half of device is ACPI based and other half i2c
> > > > > based. This is problem of ACPI and Dell design.
> > > > >
> > > > > > It seems to be that /dev/freefall in dell-smo8800 and lis3lv02d are
> > > > > > the same, right?
> > > > >
> > > > > Yes. I understand that clean solution is to have one driver which
> > > > > provides everything.
> > > > >
> > > > > But because half of data are ACPI and half i2c, you still needs to
> > > > > create two drivers (one ACPI and one i2c). You can put both drivers into
> > > > > one .ko module, but still these will be two drivers due to how ACPI and
> > > > > i2c linux abstractions are different.
> > > > >
> > > > > > So, instead of having 2 drivers split the
> > > > > > functionality, can you forego registering smo8800 ACPI driver on
> > > > > > your whitelisted boxes and instead instantiate full i2c client
> > > > > > device with properly assigned both address and IRQ and let lis3lv02d
> > > > > > handle it (providing both accelerometer data and /dev/freefall)?
> > > > >
> > > > > With Michał we already discussed about it, see emails. Basically you can
> > > > > enable/disable kernel modules at compile time or blacklist at runtime
> > > > > (or even chose what will be compiled into vmlinux and what as external
> > > > > .ko module).
> > > >
> > > > This can be solved with a bit of Kconfig/IS_ENABLED() code.
> > > >
> > > > > Some distributions blacklist i2c-i801.ko module... And
> > > >
> > > > Any particular reason for that?
> > > >
> > > > > there can be also problem with initialization of i2c-i801 driver (fix is
> > > > > in commit a7ae81952cda, but does not have to work at every time!). So
> > > > > that move on whitelisted machines can potentially cause disappearance of
> > > > > /dev/freefall and users will not have hdd protection which is currently
> > > > > working.
> > > >
> > > > Well, I gave you 2 possible solutions (roll your own i2c read/write,
> > > > forward them to i2c client) or have faith in your implementation and let
> > > > lis3lv02d handle it.
> > > >
> > > > The 3rd one is to possibly add a flag to disable host notify to IRQ
> > > > mapping for given client (if Wolfram/Jean OK with it).
> > > >
> > > > Oh, the 4th one: change the irq in lis3lv02d.h to be "int" and change
> > > > the check in lis3lv02d.c to be "lis->irq <= 0" and instantiate your
> > > > i2c_client with board_info->irq = -1.
> > > >
> > > > Pick whichever you prefer.
> > > >
> > > > By the way, what do you need accelerometer for on these devices? They
> > > > don't appear to be tablets that could use one...
> > >
> > > Ah, you are talking about problem that after 4d5538f5882a lis3lv02d will
> > > not work... I thought that discussion is about different mechanism how
> > > to implement bus registration notification to smo8800 driver (or
> > > different solution to not have registration in i801).
> > >
> >
> > Just because I am not sure I got everything right, could you confirm
> > that:
> > - in the current upstream tree, the dell-smo8800 driver is now broken
> > after 4d5538f5882a (i2c: use an IRQ to report Host Notify events, not
> > alert)
>
> No, dell-smo8800 it is working fine. It is fully independent from i2c
> and lis3lv02d. It is pure ACPI driver which does not share anything with
> i2c.
>
> > - this series adds an extra lis3lv02d on some machines and you have
> > problem fighting for the irq (but this is not upstream yet).
>
> Yes, this series (not merged yet) adds extra lis3lv02d device but is not
> working because of 4d5538f5882a.
>
> > The extra lis3lv02d node is added from dell-smo8800
>
> No, dell-smo8800 does not add new node in this patch.
>
> > If the first point is not correct (by default, dell-smo8800 will not be
> > loaded at the same time than lis3lv02d), then it's a design issue with
> > the interactions between those 2 drivers.
>
> No, there is no interactions between these two drivers (dell-smo8800 and
> lis3lv02d). dell-smo8800 is pure ACPI driver and exports just
> /dev/freefall device based on IRQ (and nothing more).
>
> And lis3lv02d in *current* configuration in this patch exports only
> accelerometer input device, not /dev/freefall. It does not use IRQ.
> (Just there is problem with 4d5538f5882a which tells lis3lv02d IRQ
> number which is not freefall report, therefore lis3lv02d does not work).
>
> To make it clear, ST Accelerometer provides two operations:
> * report free fall
> * report 3 axes
>
> Free fall is reported by IRQ, state of 3 axes via i2c bus. Free fall IRQ
> is handled by dell-smo8800, state of 3 axes via i2c lis3lv02d driver.
>
> lis3lv02d can handle also free fall IRQ is platform i2c data provides
> IRQ number for it -- but this is not case in our Dell configuration. But
> commit 4d5538f5882a inject some IRQ number to lis3lv02d driver which is
> not free fall detection and so is breaking lis3lv02 driver. In our Dell
> configuration (by this patch) there should be no IRQ number.
>
> It is clear now?
I think I am starting to understand the problem.
Currently (upstream tree), 4d5538f5882a doesn't break anything. On the
mentioned Dell platforms, the dell-smo8800 gets loaded and provides
/dev/freefall. lis3lv02d is not loaded so everything just works.
The problem comes from this patch which doesn't set the irq in
i2c_board_info and so i2c_core sets the IRQ to Host Notify. I think
Dmitri already gave you the solution: set the irq to -1 (or -ENOENT)
in i2c_board_info in your patch and only forward it to
lis3lv02d_init_device() only if it is positive (valid).
So, to me 4d5538f5882a doesn't introduce a regression so we should keep
it in its current state.
Cheers,
Benjamin
>
> > If the first point is correct because ACPI declares both devices, then
> > there is an urgent fix to propose to not enable Host Notify by default
> > on Host Notifier capable adapters. (even though the design between the
> > 2 drivers is wrong, it's considered as a regression).
> >
> > Cheers,
> > Benjamin
>
> --
> Pali Rohár
> pali.rohar@gmail.com
^ permalink raw reply
* Re: [PATCH] i2c: i801: Register optional lis3lv02d i2c device on Dell machines
From: Andy Shevchenko @ 2017-01-04 9:53 UTC (permalink / raw)
To: Pali Rohár
Cc: Dmitry Torokhov, Benjamin Tissoires, Michał Kępień,
Jean Delvare, Wolfram Sang, Steven Honeyman, Valdis Kletnieks,
Jochen Eisinger, Gabriele Mazzotta, Andy Lutomirski,
Mario Limonciello, Alex Hung, Takashi Iwai, linux-i2c,
linux-kernel@vger.kernel.org, Platform Driver
In-Reply-To: <201701032139.13061@pali>
On Tue, Jan 3, 2017 at 10:39 PM, Pali Rohár <pali.rohar@gmail.com> wrote:
>
> With Michał we already discussed about it, see emails. Basically you can
> enable/disable kernel modules at compile time or blacklist at runtime
> (or even chose what will be compiled into vmlinux and what as external
> .ko module). Some distributions blacklist i2c-i801.ko module...
But you understand that any of compile/not compile is not an option, right?
The case which we face will be both of them, if possible, will be
compiled as modules.
Blacklisting means making your problem the actual user's one. Not good.
> And
> there can be also problem with initialization of i2c-i801 driver (fix is
> in commit a7ae81952cda, but does not have to work at every time!). So
> that move on whitelisted machines can potentially cause disappearance of
> /dev/freefall and users will not have hdd protection which is currently
> working.
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply
* Re: [PATCH] i2c: i801: Register optional lis3lv02d i2c device on Dell machines
From: Jean Delvare @ 2017-01-04 9:45 UTC (permalink / raw)
To: Wolfram Sang
Cc: Pali Rohár, Steven Honeyman, valdis.kletnieks,
Jochen Eisinger, Gabriele Mazzotta, Andy Lutomirski,
mario_limonciello, Alex Hung, kernel, Takashi Iwai, linux-i2c,
linux-kernel, platform-driver-x86
In-Reply-To: <6acf42bfb16eaa0e69ca322eb0c0853d@the-dreams.de>
On Wed, 28 Dec 2016 15:02:52 +0100, Wolfram Sang wrote:
> On Tue, Dec 27, 2016 at 02:51:01PM +0100, Pali Rohár wrote:
> > i2c_new_device() with lis3lv02d for i801 i2c bus needs to be called
> > after initializing i2c-i801 bus driver.
> >
> > I have no idea how to do it (properly) outside of i2c-i801.c file.
>
> I once used bus_notifiers to achieve something similar. You could check
> arch/arm/mach-shmobile/regulator-quirk-rcar-gen2.c to see an action
> triggered once a client device got added, but you could act on another
> action like BUS_NOTIFY_BOUND_DRIVER. I used exactly that, too, somewhen
> somewhere. Haven't checked if that helps here, too. And since we have
> a
> precedence (Fujitsu case), I'll leave it to Jean who is the maintainer
> of this driver.
I don't have a strong opinion on the matter, whatever works is fine
with me.
--
Jean Delvare
SUSE L3 Support
^ permalink raw reply
* Re: [PATCH] i2c: i801: Register optional lis3lv02d i2c device on Dell machines
From: Pali Rohár @ 2017-01-04 9:25 UTC (permalink / raw)
To: Benjamin Tissoires
Cc: Dmitry Torokhov, Michał Kępień, Jean Delvare,
Wolfram Sang, Steven Honeyman, Valdis.Kletnieks, Jochen Eisinger,
Gabriele Mazzotta, Andy Lutomirski, Mario_Limonciello, Alex Hung,
Takashi Iwai, linux-i2c, linux-kernel, platform-driver-x86
In-Reply-To: <20170104091338.GO5767@mail.corp.redhat.com>
On Wednesday 04 January 2017 10:13:38 Benjamin Tissoires wrote:
> On Jan 03 2017 or thereabouts, Dmitry Torokhov wrote:
> > On Tue, Jan 03, 2017 at 10:29:34PM +0100, Benjamin Tissoires wrote:
> > > On Jan 03 2017 or thereabouts, Dmitry Torokhov wrote:
> > >
> > > > Yet another option: can we add a new flag to i2c_board_info controlling
> > > > whether we want to enable/disable wiring up host notify interrupt?
> > >
> > > That should be fairly easy to implement. For now, given that only Elan
> > > and Synaptics are the one in need for Host Notify, it could be better to
> > > request the Host Notify IRQ instead of disabling it unconditionally
> > > (which would make the current yet 8 years old lis3lv02d driver happy
> > > again).
> >
> > I like that we have it done in i2c core instead of having drivers
> > implementing it individually. Since you are saying that handling host
> > notify is property of the slave/driver maybe we should be adding a flag
> > to the *i2c_driver* structure to let i2c core that we want to have host
> > notify mapped as interrupt if "native" interrupt is not supplied by the
> > platform code?
>
> I don't think this is a good idea. It's still a property of the I2C
> device, not the driver. It's crappy under Windows, but that doesn't
> prevent us to do the right thing.
>
> I think the idea of having it at the i2c_board_info level is the good
> one. It's a property of the device node and it wouldn't hurt me to have
> a device tree property for that too (not entering the DT field now).
> There is no ACPI prop for it too, but I wouldn't be surprised if it
> comes in a later revision. The advantage of having it turned on
> unconditionally is that we can instantiate it from userspace without
> breaking the sysfs ABI.
>
> Note that in the 2 uses I have seen so far of Host Notify, in both cases
> I need to instantiate the I2C device from an other driver (psmouse) so I
> can control the content of i2c_board_info.
If I understand it correctly, there is problem that i2c lis3lv02d driver
needs to get IRQ number for freefall and in current structure you pass
host notify IRQ number.
It means that one property in lis3lv02d driver is used for two things:
free fall IRQ and host notify IRQ. So the only way how to fix it is to
use two different IRQ properties. Probably free fall IRQ is lis3lv02d
specific and should be in platform data structure, not in generic
i2c_board_info shared across all i2c drivers?
--
Pali Rohár
pali.rohar@gmail.com
^ permalink raw reply
* [Patch V1] i2c: imx-lpi2c: add VLLS mode support
From: Gao Pan @ 2017-01-04 9:20 UTC (permalink / raw)
To: wsa, wsa-dev; +Cc: linux-i2c, pandy.gao, frank.li, fugang.duan
When system enters VLLS mode, module power is turned off. As a result,
all registers are reset to HW default value. After exiting VLLS mode,
registers are still in default mode. As a result, the pinctrl settings
are incorrect, which will affect the module function.
The patch recovers the pinctrl setting when exit VLLS mode.
Signed-off-by: Gao Pan <pandy.gao@nxp.com>
---
drivers/i2c/busses/i2c-imx-lpi2c.c | 22 ++++++++++++++++++++++
1 file changed, 22 insertions(+)
diff --git a/drivers/i2c/busses/i2c-imx-lpi2c.c b/drivers/i2c/busses/i2c-imx-lpi2c.c
index c62b7cd..e528c1d1 100644
--- a/drivers/i2c/busses/i2c-imx-lpi2c.c
+++ b/drivers/i2c/busses/i2c-imx-lpi2c.c
@@ -636,12 +636,34 @@ static int lpi2c_imx_remove(struct platform_device *pdev)
return 0;
}
+#ifdef CONFIG_PM_SLEEP
+static int lpi2c_imx_suspend(struct device *dev)
+{
+ pinctrl_pm_select_sleep_state(dev);
+
+ return 0;
+}
+
+static int lpi2c_imx_resume(struct device *dev)
+{
+ pinctrl_pm_select_default_state(dev);
+
+ return 0;
+}
+
+static SIMPLE_DEV_PM_OPS(imx_lpi2c_pm, lpi2c_imx_suspend, lpi2c_imx_resume);
+#define IMX_LPI2C_PM (&imx_lpi2c_pm)
+#else
+#define IMX_LPI2C_PM NULL
+#endif
+
static struct platform_driver lpi2c_imx_driver = {
.probe = lpi2c_imx_probe,
.remove = lpi2c_imx_remove,
.driver = {
.name = DRIVER_NAME,
.of_match_table = lpi2c_imx_of_match,
+ .pm = IMX_LPI2C_PM,
},
};
--
1.9.1
^ permalink raw reply related
* Re: [PATCH] i2c: i801: Register optional lis3lv02d i2c device on Dell machines
From: Pali Rohár @ 2017-01-04 9:18 UTC (permalink / raw)
To: Benjamin Tissoires
Cc: Dmitry Torokhov, Michał Kępień, Jean Delvare,
Wolfram Sang, Steven Honeyman, Valdis.Kletnieks, Jochen Eisinger,
Gabriele Mazzotta, Andy Lutomirski, Mario_Limonciello, Alex Hung,
Takashi Iwai, linux-i2c, linux-kernel, platform-driver-x86
In-Reply-To: <20170104090522.GN5767@mail.corp.redhat.com>
On Wednesday 04 January 2017 10:05:22 Benjamin Tissoires wrote:
> On Jan 04 2017 or thereabouts, Pali Rohár wrote:
> > On Tuesday 03 January 2017 12:59:37 Dmitry Torokhov wrote:
> > > On Tue, Jan 03, 2017 at 09:39:13PM +0100, Pali Rohár wrote:
> > > > On Tuesday 03 January 2017 21:24:18 Dmitry Torokhov wrote:
> > > > > On Tue, Jan 03, 2017 at 09:05:51PM +0100, Pali Rohár wrote:
> > > > > > On Tuesday 03 January 2017 20:48:12 Dmitry Torokhov wrote:
> > > > > > > On Tue, Jan 03, 2017 at 07:50:17PM +0100, Pali Rohár wrote:
> > > > > > > > On Tuesday 03 January 2017 19:38:43 Dmitry Torokhov wrote:
> > > > > > > > > On Tue, Jan 03, 2017 at 10:06:41AM +0100, Benjamin Tissoires
> > > > > > > > >
> > > > > > > > > wrote:
> > > > > > > > > > On Dec 29 2016 or thereabouts, Pali Rohár wrote:
> > > > > > > > > > > On Thursday 29 December 2016 22:09:32 Michał Kępień wrote:
> > > > > > > > > > > > > On Thursday 29 December 2016 14:47:19 Michał Kępień
> > > > > > > > > > > > > wrote:
> > > > > > > > > > > > > > > On Thursday 29 December 2016 09:29:36 Michał
> > > > > > > > > > > > > > > Kępień
> > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > wrote:
> > > > > > > > > > > > > > > > > Dell platform team told us that some (DMI
> > > > > > > > > > > > > > > > > whitelisted) Dell Latitude machines have ST
> > > > > > > > > > > > > > > > > microelectronics accelerometer at i2c address
> > > > > > > > > > > > > > > > > 0x29. That i2c address is not specified in
> > > > > > > > > > > > > > > > > DMI or ACPI, so runtime detection without
> > > > > > > > > > > > > > > > > whitelist which is below is not possible.
> > > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > > Presence of that ST microelectronics
> > > > > > > > > > > > > > > > > accelerometer is verified by existence of
> > > > > > > > > > > > > > > > > SMO88xx ACPI device which represent that
> > > > > > > > > > > > > > > > > accelerometer. Unfortunately without i2c
> > > > > > > > > > > > > > > > > address.
> > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > This part of the commit message sounded a bit
> > > > > > > > > > > > > > > > confusing to me at first because there is
> > > > > > > > > > > > > > > > already an ACPI driver which handles SMO88xx
> > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > devices (dell-smo8800). My understanding is
> > > > > > > > > > > > > > > > that:
> > > > > > > > > > > > > > > > * the purpose of this patch is to expose a
> > > > > > > > > > > > > > > > richer interface (as
> > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > provided by lis3lv02d) to these devices on
> > > > > > > > > > > > > > > > some machines,
> > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > * on whitelisted machines, dell-smo8800 and
> > > > > > > > > > > > > > > > lis3lv02d can work
> > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > simultaneously (even though dell-smo8800
> > > > > > > > > > > > > > > > effectively duplicates the work that
> > > > > > > > > > > > > > > > lis3lv02d does).
> > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > No. dell-smo8800 reads from ACPI irq number and
> > > > > > > > > > > > > > > exports /dev/freefall device which notify
> > > > > > > > > > > > > > > userspace about falls. lis3lv02d is i2c driver
> > > > > > > > > > > > > > > which exports axes of accelerometer. Additionaly
> > > > > > > > > > > > > > > lis3lv02d can export also /dev/freefall if
> > > > > > > > > > > > > > > registerer of i2c device provides irq number --
> > > > > > > > > > > > > > > which is not case of this patch.
> > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > So both drivers are doing different things and
> > > > > > > > > > > > > > > both are useful.
> > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > IIRC both dell-smo8800 and lis3lv02d represent
> > > > > > > > > > > > > > > one HW device (that ST microelectronics
> > > > > > > > > > > > > > > accelerometer) but due to complicated HW
> > > > > > > > > > > > > > > abstraction and layers on Dell laptops it is
> > > > > > > > > > > > > > > handled by two drivers, one ACPI and one i2c.
> > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > Yes, in ideal world irq number should be passed
> > > > > > > > > > > > > > > to lis3lv02d driver and that would export whole
> > > > > > > > > > > > > > > device (with /dev/freefall too), but due to HW
> > > > > > > > > > > > > > > abstraction it is too much complicated...
> > > > > > > > > > > > > >
> > > > > > > > > > > > > > Why? AFAICT, all that is required to pass that IRQ
> > > > > > > > > > > > > > number all the way down to lis3lv02d is to set the
> > > > > > > > > > > > > > irq field of the struct i2c_board_info you are
> > > > > > > > > > > > > > passing to i2c_new_device(). And you can extract
> > > > > > > > > > > > > > that IRQ number e.g. in
> > > > > > > > > > > > > > check_acpi_smo88xx_device(). However, you would
> > > > > > > > > > > > > > then need to make sure dell-smo8800 does not
> > > > > > > > > > > > > > attempt to request the same IRQ on whitelisted
> > > > > > > > > > > > > > machines. This got me thinking about a way to
> > > > > > > > > > > > > > somehow incorporate your changes into dell-smo8800
> > > > > > > > > > > > > > using Wolfram's bus_notifier suggestion, but I do
> > > > > > > > > > > > > > not have a working solution for now. What is
> > > > > > > > > > > > > > tempting about this approach is that you would not
> > > > > > > > > > > > > > have to scan the ACPI namespace in search of
> > > > > > > > > > > > > > SMO88xx devices, because smo8800_add() is
> > > > > > > > > > > > > > automatically called for them. However, I fear that
> > > > > > > > > > > > > > the resulting solution may be more complicated than
> > > > > > > > > > > > > > the one you submitted.
> > > > > > > > > > > > >
> > > > > > > > > > > > > Then we need to deal with lot of problems. Order of
> > > > > > > > > > > > > loading .ko modules is undefined. Binding devices to
> > > > > > > > > > > > > drivers registered by .ko module is also in "random"
> > > > > > > > > > > > > order. At any time any of those .ko module can be
> > > > > > > > > > > > > unloaded or at least device unbind (via sysfs) from
> > > > > > > > > > > > > driver... And there can be some pathological
> > > > > > > > > > > > > situation (thanks to adding ACPI layer as Andy
> > > > > > > > > > > > > pointed) that there will be more SMO88xx devices in
> > > > > > > > > > > > > ACPI. Plus you can compile kernel with and without
> > > > > > > > > > > > > those modules and also you can blacklist loading
> > > > > > > > > > > > > them (so compile time check is not enough). And
> > > > > > > > > > > > > still some correct message notifier must be used.
> > > > > > > > > > > > >
> > > > > > > > > > > > > I think such solution is much much more complicated,
> > > > > > > > > > > > > there are lot of combinations of kernel configuration
> > > > > > > > > > > > > and available dell devices...
> > > > > > > > > > > >
> > > > > > > > > > > > I tried a few more things, but ultimately failed to
> > > > > > > > > > > > find a nice way to implement this.
> > > > > > > > > > > >
> > > > > > > > > > > > Another issue popped up, though. Linus' master branch
> > > > > > > > > > > > contains a recent commit by Benjamin Tissoires (CC'ed),
> > > > > > > > > > > > 4d5538f5882a ("i2c: use an IRQ to report Host Notify
> > > > > > > > > > > > events, not alert") which breaks your patch. The
> > > > > > > > > > > > reason for that is that lis3lv02d relies on the i2c
> > > > > > > > > > > > client's IRQ being 0 to detect that it should not
> > > > > > > > > > > > create /dev/freefall.
> > > > > > > > > > > >
> > > > > > > > > > > > Benjamin's patch causes the Host Notify IRQ to be
> > > > > > > > > > > >
> > > > > > > > > > > > assigned to the i2c client your patch creates, thus
> > > > > > > > > > > > causing lis3lv02d to create /dev/freefall, which in
> > > > > > > > > > > > turn conflicts with dell-smo8800 which is trying to
> > > > > > > > > > > > create /dev/freefall itself.
> > > > > > > > > > >
> > > > > > > > > > > So 4d5538f5882a is breaking lis3lv02d driver...
> > > > > > > > > >
> > > > > > > > > > Apologies for that.
> > > > > > > > > >
> > > > > > > > > > I could easily fix this by adding a kernel API to know
> > > > > > > > > > whether the provided irq is from Host Notify or if it was
> > > > > > > > > > coming from an actual declaration. However, I have no idea
> > > > > > > > > > how many other drivers would require this (hopefully only
> > > > > > > > > > this one).
> > > > > > > > > >
> > > > > > > > > > One other solution would be to reserve the Host Notify IRQ
> > > > > > > > > > and let the actual drivers that need it to set it, but
> > > > > > > > > > this was not the best solution according to Dmitri. On my
> > > > > > > > > > side, I am not entirely against this given that it's a
> > > > > > > > > > chip feature, so the driver should be able to know that
> > > > > > > > > > it's available.
> > > > > > > > > >
> > > > > > > > > > Dmitri, Wolfram, Jean, any preferences?
> > > > > > > > >
> > > > > > > > > I read this:
> > > > > > > > >
> > > > > > > > > "IIRC both dell-smo8800 and lis3lv02d represent one HW device
> > > > > > > > > (that ST microelectronics accelerometer) but due to
> > > > > > > > > complicated HW abstraction and layers on Dell laptops it is
> > > > > > > > > handled by two drivers, one ACPI and one i2c."
> > > > > > > > >
> > > > > > > > > and that is the core of the issue. You have 2 drivers
> > > > > > > > > fighting over the same device. Fix this and it will all
> > > > > > > > > work.
> > > > > > > >
> > > > > > > > With my current implementation (which I sent in this patch),
> > > > > > > > they are not fighting.
> > > > > > > >
> > > > > > > > dell-smo8800 exports /dev/freefall (and nothing more) and
> > > > > > > > lis3lv02d only accelerometer device as lis3lv02d driver does
> > > > > > > > not get IRQ number in platform data.
> > > > > > > >
> > > > > > > > > As far as I can see hp_accel instantiates lis3lv02d and
> > > > > > > > > accesses it via ACPI methods, can the same be done for Dell?
> > > > > > > >
> > > > > > > > No, Dell does not have any ACPI methods. And as I wrote in ACPI
> > > > > > > > or DMI is even not i2c address of device, so it needs to be
> > > > > > > > specified in code itself.
> > > > > > > >
> > > > > > > > Really there is no other way... :-(
> > > > > > >
> > > > > > > Sure there is:
> > > > > > >
> > > > > > > 1. dell-smo8800 instantiates I2C device as "dell-smo8800-accel".
> > > > > > > 2. dell-smo8800 provides read/write functions for lis3lv02d that
> > > > > > > simply forward requests to dell-smo8800-accel i2c client.
> > > > > > > 3. dell-smo8800 instantiates lis3lv02d instance like hp_accel
> > > > > > > does.
> > > > > >
> > > > > > Sorry, but I do not understand how you mean it... Why to provides
> > > > > > new read/write i2c functions which are already implemented by
> > > > > > i2c-i801 bus and lis3lv02d i2c driver?
> > > > >
> > > > > Because that would allow you to avoid clashes with i2c creating
> > > > > interrupt mapping for client residing on host-notify-capable
> > > > > controller.
> > > > >
> > > > > > > Alternatively, can lis3lv02d be tasked to create /dev/freefall?
> > > > > >
> > > > > > If i2c_board_info contains IRQ then lis3lv02d create /dev/freefall
> > > > > > device.
> > > > > >
> > > > > > But... what is problem with current implementation? Accelerometer
> > > > > > HW provides two functions:
> > > > > >
> > > > > > 1) 3 axes reports
> > > > > > 2) Disk freefall detection
> > > > > >
> > > > > > And 1) is handled by i2c driver lis3lv02d and 2) is by
> > > > > > dell-smo8800. Both functions are independent here.
> > > > > >
> > > > > > I think you just trying to complicate this situation even more to
> > > > > > be more complicated as currently is.
> > > > >
> > > > > Because this apparently does not work for you, does it?
> > > >
> > > > It is working fine. I do not see any problem.
> > > >
> > > > > In general,
> > > > > if you want the same hardware be handled by 2 different drivers you
> > > > > are going to have bad time.
> > > >
> > > > Yes, but in this case half of device is ACPI based and other half i2c
> > > > based. This is problem of ACPI and Dell design.
> > > >
> > > > > It seems to be that /dev/freefall in dell-smo8800 and lis3lv02d are
> > > > > the same, right?
> > > >
> > > > Yes. I understand that clean solution is to have one driver which
> > > > provides everything.
> > > >
> > > > But because half of data are ACPI and half i2c, you still needs to
> > > > create two drivers (one ACPI and one i2c). You can put both drivers into
> > > > one .ko module, but still these will be two drivers due to how ACPI and
> > > > i2c linux abstractions are different.
> > > >
> > > > > So, instead of having 2 drivers split the
> > > > > functionality, can you forego registering smo8800 ACPI driver on
> > > > > your whitelisted boxes and instead instantiate full i2c client
> > > > > device with properly assigned both address and IRQ and let lis3lv02d
> > > > > handle it (providing both accelerometer data and /dev/freefall)?
> > > >
> > > > With Michał we already discussed about it, see emails. Basically you can
> > > > enable/disable kernel modules at compile time or blacklist at runtime
> > > > (or even chose what will be compiled into vmlinux and what as external
> > > > .ko module).
> > >
> > > This can be solved with a bit of Kconfig/IS_ENABLED() code.
> > >
> > > > Some distributions blacklist i2c-i801.ko module... And
> > >
> > > Any particular reason for that?
> > >
> > > > there can be also problem with initialization of i2c-i801 driver (fix is
> > > > in commit a7ae81952cda, but does not have to work at every time!). So
> > > > that move on whitelisted machines can potentially cause disappearance of
> > > > /dev/freefall and users will not have hdd protection which is currently
> > > > working.
> > >
> > > Well, I gave you 2 possible solutions (roll your own i2c read/write,
> > > forward them to i2c client) or have faith in your implementation and let
> > > lis3lv02d handle it.
> > >
> > > The 3rd one is to possibly add a flag to disable host notify to IRQ
> > > mapping for given client (if Wolfram/Jean OK with it).
> > >
> > > Oh, the 4th one: change the irq in lis3lv02d.h to be "int" and change
> > > the check in lis3lv02d.c to be "lis->irq <= 0" and instantiate your
> > > i2c_client with board_info->irq = -1.
> > >
> > > Pick whichever you prefer.
> > >
> > > By the way, what do you need accelerometer for on these devices? They
> > > don't appear to be tablets that could use one...
> >
> > Ah, you are talking about problem that after 4d5538f5882a lis3lv02d will
> > not work... I thought that discussion is about different mechanism how
> > to implement bus registration notification to smo8800 driver (or
> > different solution to not have registration in i801).
> >
>
> Just because I am not sure I got everything right, could you confirm
> that:
> - in the current upstream tree, the dell-smo8800 driver is now broken
> after 4d5538f5882a (i2c: use an IRQ to report Host Notify events, not
> alert)
No, dell-smo8800 it is working fine. It is fully independent from i2c
and lis3lv02d. It is pure ACPI driver which does not share anything with
i2c.
> - this series adds an extra lis3lv02d on some machines and you have
> problem fighting for the irq (but this is not upstream yet).
Yes, this series (not merged yet) adds extra lis3lv02d device but is not
working because of 4d5538f5882a.
> The extra lis3lv02d node is added from dell-smo8800
No, dell-smo8800 does not add new node in this patch.
> If the first point is not correct (by default, dell-smo8800 will not be
> loaded at the same time than lis3lv02d), then it's a design issue with
> the interactions between those 2 drivers.
No, there is no interactions between these two drivers (dell-smo8800 and
lis3lv02d). dell-smo8800 is pure ACPI driver and exports just
/dev/freefall device based on IRQ (and nothing more).
And lis3lv02d in *current* configuration in this patch exports only
accelerometer input device, not /dev/freefall. It does not use IRQ.
(Just there is problem with 4d5538f5882a which tells lis3lv02d IRQ
number which is not freefall report, therefore lis3lv02d does not work).
To make it clear, ST Accelerometer provides two operations:
* report free fall
* report 3 axes
Free fall is reported by IRQ, state of 3 axes via i2c bus. Free fall IRQ
is handled by dell-smo8800, state of 3 axes via i2c lis3lv02d driver.
lis3lv02d can handle also free fall IRQ is platform i2c data provides
IRQ number for it -- but this is not case in our Dell configuration. But
commit 4d5538f5882a inject some IRQ number to lis3lv02d driver which is
not free fall detection and so is breaking lis3lv02 driver. In our Dell
configuration (by this patch) there should be no IRQ number.
It is clear now?
> If the first point is correct because ACPI declares both devices, then
> there is an urgent fix to propose to not enable Host Notify by default
> on Host Notifier capable adapters. (even though the design between the
> 2 drivers is wrong, it's considered as a regression).
>
> Cheers,
> Benjamin
--
Pali Rohár
pali.rohar@gmail.com
^ permalink raw reply
* Re: [PATCH] i2c: i801: Register optional lis3lv02d i2c device on Dell machines
From: Benjamin Tissoires @ 2017-01-04 9:13 UTC (permalink / raw)
To: Dmitry Torokhov
Cc: Pali Rohár, Michał Kępień, Jean Delvare,
Wolfram Sang, Steven Honeyman, Valdis.Kletnieks, Jochen Eisinger,
Gabriele Mazzotta, Andy Lutomirski, Mario_Limonciello, Alex Hung,
Takashi Iwai, linux-i2c, linux-kernel, platform-driver-x86
In-Reply-To: <20170104063642.GA20545@dtor-ws>
On Jan 03 2017 or thereabouts, Dmitry Torokhov wrote:
> On Tue, Jan 03, 2017 at 10:29:34PM +0100, Benjamin Tissoires wrote:
> > On Jan 03 2017 or thereabouts, Dmitry Torokhov wrote:
> >
> > > Yet another option: can we add a new flag to i2c_board_info controlling
> > > whether we want to enable/disable wiring up host notify interrupt?
> >
> > That should be fairly easy to implement. For now, given that only Elan
> > and Synaptics are the one in need for Host Notify, it could be better to
> > request the Host Notify IRQ instead of disabling it unconditionally
> > (which would make the current yet 8 years old lis3lv02d driver happy
> > again).
>
> I like that we have it done in i2c core instead of having drivers
> implementing it individually. Since you are saying that handling host
> notify is property of the slave/driver maybe we should be adding a flag
> to the *i2c_driver* structure to let i2c core that we want to have host
> notify mapped as interrupt if "native" interrupt is not supplied by the
> platform code?
I don't think this is a good idea. It's still a property of the I2C
device, not the driver. It's crappy under Windows, but that doesn't
prevent us to do the right thing.
I think the idea of having it at the i2c_board_info level is the good
one. It's a property of the device node and it wouldn't hurt me to have
a device tree property for that too (not entering the DT field now).
There is no ACPI prop for it too, but I wouldn't be surprised if it
comes in a later revision. The advantage of having it turned on
unconditionally is that we can instantiate it from userspace without
breaking the sysfs ABI.
Note that in the 2 uses I have seen so far of Host Notify, in both cases
I need to instantiate the I2C device from an other driver (psmouse) so I
can control the content of i2c_board_info.
Cheers,
Benjamin
^ permalink raw reply
* Re: [PATCH] i2c: i801: Register optional lis3lv02d i2c device on Dell machines
From: Benjamin Tissoires @ 2017-01-04 9:05 UTC (permalink / raw)
To: Pali Rohár
Cc: Dmitry Torokhov, Michał Kępień, Jean Delvare,
Wolfram Sang, Steven Honeyman, Valdis.Kletnieks, Jochen Eisinger,
Gabriele Mazzotta, Andy Lutomirski, Mario_Limonciello, Alex Hung,
Takashi Iwai, linux-i2c, linux-kernel, platform-driver-x86
In-Reply-To: <20170104081804.GA3499@pali>
On Jan 04 2017 or thereabouts, Pali Rohár wrote:
> On Tuesday 03 January 2017 12:59:37 Dmitry Torokhov wrote:
> > On Tue, Jan 03, 2017 at 09:39:13PM +0100, Pali Rohár wrote:
> > > On Tuesday 03 January 2017 21:24:18 Dmitry Torokhov wrote:
> > > > On Tue, Jan 03, 2017 at 09:05:51PM +0100, Pali Rohár wrote:
> > > > > On Tuesday 03 January 2017 20:48:12 Dmitry Torokhov wrote:
> > > > > > On Tue, Jan 03, 2017 at 07:50:17PM +0100, Pali Rohár wrote:
> > > > > > > On Tuesday 03 January 2017 19:38:43 Dmitry Torokhov wrote:
> > > > > > > > On Tue, Jan 03, 2017 at 10:06:41AM +0100, Benjamin Tissoires
> > > > > > > >
> > > > > > > > wrote:
> > > > > > > > > On Dec 29 2016 or thereabouts, Pali Rohár wrote:
> > > > > > > > > > On Thursday 29 December 2016 22:09:32 Michał Kępień wrote:
> > > > > > > > > > > > On Thursday 29 December 2016 14:47:19 Michał Kępień
> > > > > > > > > > > > wrote:
> > > > > > > > > > > > > > On Thursday 29 December 2016 09:29:36 Michał
> > > > > > > > > > > > > > Kępień
> > > > > > > > > > > > > >
> > > > > > > > > > > > > > wrote:
> > > > > > > > > > > > > > > > Dell platform team told us that some (DMI
> > > > > > > > > > > > > > > > whitelisted) Dell Latitude machines have ST
> > > > > > > > > > > > > > > > microelectronics accelerometer at i2c address
> > > > > > > > > > > > > > > > 0x29. That i2c address is not specified in
> > > > > > > > > > > > > > > > DMI or ACPI, so runtime detection without
> > > > > > > > > > > > > > > > whitelist which is below is not possible.
> > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > Presence of that ST microelectronics
> > > > > > > > > > > > > > > > accelerometer is verified by existence of
> > > > > > > > > > > > > > > > SMO88xx ACPI device which represent that
> > > > > > > > > > > > > > > > accelerometer. Unfortunately without i2c
> > > > > > > > > > > > > > > > address.
> > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > This part of the commit message sounded a bit
> > > > > > > > > > > > > > > confusing to me at first because there is
> > > > > > > > > > > > > > > already an ACPI driver which handles SMO88xx
> > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > devices (dell-smo8800). My understanding is
> > > > > > > > > > > > > > > that:
> > > > > > > > > > > > > > > * the purpose of this patch is to expose a
> > > > > > > > > > > > > > > richer interface (as
> > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > provided by lis3lv02d) to these devices on
> > > > > > > > > > > > > > > some machines,
> > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > * on whitelisted machines, dell-smo8800 and
> > > > > > > > > > > > > > > lis3lv02d can work
> > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > simultaneously (even though dell-smo8800
> > > > > > > > > > > > > > > effectively duplicates the work that
> > > > > > > > > > > > > > > lis3lv02d does).
> > > > > > > > > > > > > >
> > > > > > > > > > > > > > No. dell-smo8800 reads from ACPI irq number and
> > > > > > > > > > > > > > exports /dev/freefall device which notify
> > > > > > > > > > > > > > userspace about falls. lis3lv02d is i2c driver
> > > > > > > > > > > > > > which exports axes of accelerometer. Additionaly
> > > > > > > > > > > > > > lis3lv02d can export also /dev/freefall if
> > > > > > > > > > > > > > registerer of i2c device provides irq number --
> > > > > > > > > > > > > > which is not case of this patch.
> > > > > > > > > > > > > >
> > > > > > > > > > > > > > So both drivers are doing different things and
> > > > > > > > > > > > > > both are useful.
> > > > > > > > > > > > > >
> > > > > > > > > > > > > > IIRC both dell-smo8800 and lis3lv02d represent
> > > > > > > > > > > > > > one HW device (that ST microelectronics
> > > > > > > > > > > > > > accelerometer) but due to complicated HW
> > > > > > > > > > > > > > abstraction and layers on Dell laptops it is
> > > > > > > > > > > > > > handled by two drivers, one ACPI and one i2c.
> > > > > > > > > > > > > >
> > > > > > > > > > > > > > Yes, in ideal world irq number should be passed
> > > > > > > > > > > > > > to lis3lv02d driver and that would export whole
> > > > > > > > > > > > > > device (with /dev/freefall too), but due to HW
> > > > > > > > > > > > > > abstraction it is too much complicated...
> > > > > > > > > > > > >
> > > > > > > > > > > > > Why? AFAICT, all that is required to pass that IRQ
> > > > > > > > > > > > > number all the way down to lis3lv02d is to set the
> > > > > > > > > > > > > irq field of the struct i2c_board_info you are
> > > > > > > > > > > > > passing to i2c_new_device(). And you can extract
> > > > > > > > > > > > > that IRQ number e.g. in
> > > > > > > > > > > > > check_acpi_smo88xx_device(). However, you would
> > > > > > > > > > > > > then need to make sure dell-smo8800 does not
> > > > > > > > > > > > > attempt to request the same IRQ on whitelisted
> > > > > > > > > > > > > machines. This got me thinking about a way to
> > > > > > > > > > > > > somehow incorporate your changes into dell-smo8800
> > > > > > > > > > > > > using Wolfram's bus_notifier suggestion, but I do
> > > > > > > > > > > > > not have a working solution for now. What is
> > > > > > > > > > > > > tempting about this approach is that you would not
> > > > > > > > > > > > > have to scan the ACPI namespace in search of
> > > > > > > > > > > > > SMO88xx devices, because smo8800_add() is
> > > > > > > > > > > > > automatically called for them. However, I fear that
> > > > > > > > > > > > > the resulting solution may be more complicated than
> > > > > > > > > > > > > the one you submitted.
> > > > > > > > > > > >
> > > > > > > > > > > > Then we need to deal with lot of problems. Order of
> > > > > > > > > > > > loading .ko modules is undefined. Binding devices to
> > > > > > > > > > > > drivers registered by .ko module is also in "random"
> > > > > > > > > > > > order. At any time any of those .ko module can be
> > > > > > > > > > > > unloaded or at least device unbind (via sysfs) from
> > > > > > > > > > > > driver... And there can be some pathological
> > > > > > > > > > > > situation (thanks to adding ACPI layer as Andy
> > > > > > > > > > > > pointed) that there will be more SMO88xx devices in
> > > > > > > > > > > > ACPI. Plus you can compile kernel with and without
> > > > > > > > > > > > those modules and also you can blacklist loading
> > > > > > > > > > > > them (so compile time check is not enough). And
> > > > > > > > > > > > still some correct message notifier must be used.
> > > > > > > > > > > >
> > > > > > > > > > > > I think such solution is much much more complicated,
> > > > > > > > > > > > there are lot of combinations of kernel configuration
> > > > > > > > > > > > and available dell devices...
> > > > > > > > > > >
> > > > > > > > > > > I tried a few more things, but ultimately failed to
> > > > > > > > > > > find a nice way to implement this.
> > > > > > > > > > >
> > > > > > > > > > > Another issue popped up, though. Linus' master branch
> > > > > > > > > > > contains a recent commit by Benjamin Tissoires (CC'ed),
> > > > > > > > > > > 4d5538f5882a ("i2c: use an IRQ to report Host Notify
> > > > > > > > > > > events, not alert") which breaks your patch. The
> > > > > > > > > > > reason for that is that lis3lv02d relies on the i2c
> > > > > > > > > > > client's IRQ being 0 to detect that it should not
> > > > > > > > > > > create /dev/freefall.
> > > > > > > > > > >
> > > > > > > > > > > Benjamin's patch causes the Host Notify IRQ to be
> > > > > > > > > > >
> > > > > > > > > > > assigned to the i2c client your patch creates, thus
> > > > > > > > > > > causing lis3lv02d to create /dev/freefall, which in
> > > > > > > > > > > turn conflicts with dell-smo8800 which is trying to
> > > > > > > > > > > create /dev/freefall itself.
> > > > > > > > > >
> > > > > > > > > > So 4d5538f5882a is breaking lis3lv02d driver...
> > > > > > > > >
> > > > > > > > > Apologies for that.
> > > > > > > > >
> > > > > > > > > I could easily fix this by adding a kernel API to know
> > > > > > > > > whether the provided irq is from Host Notify or if it was
> > > > > > > > > coming from an actual declaration. However, I have no idea
> > > > > > > > > how many other drivers would require this (hopefully only
> > > > > > > > > this one).
> > > > > > > > >
> > > > > > > > > One other solution would be to reserve the Host Notify IRQ
> > > > > > > > > and let the actual drivers that need it to set it, but
> > > > > > > > > this was not the best solution according to Dmitri. On my
> > > > > > > > > side, I am not entirely against this given that it's a
> > > > > > > > > chip feature, so the driver should be able to know that
> > > > > > > > > it's available.
> > > > > > > > >
> > > > > > > > > Dmitri, Wolfram, Jean, any preferences?
> > > > > > > >
> > > > > > > > I read this:
> > > > > > > >
> > > > > > > > "IIRC both dell-smo8800 and lis3lv02d represent one HW device
> > > > > > > > (that ST microelectronics accelerometer) but due to
> > > > > > > > complicated HW abstraction and layers on Dell laptops it is
> > > > > > > > handled by two drivers, one ACPI and one i2c."
> > > > > > > >
> > > > > > > > and that is the core of the issue. You have 2 drivers
> > > > > > > > fighting over the same device. Fix this and it will all
> > > > > > > > work.
> > > > > > >
> > > > > > > With my current implementation (which I sent in this patch),
> > > > > > > they are not fighting.
> > > > > > >
> > > > > > > dell-smo8800 exports /dev/freefall (and nothing more) and
> > > > > > > lis3lv02d only accelerometer device as lis3lv02d driver does
> > > > > > > not get IRQ number in platform data.
> > > > > > >
> > > > > > > > As far as I can see hp_accel instantiates lis3lv02d and
> > > > > > > > accesses it via ACPI methods, can the same be done for Dell?
> > > > > > >
> > > > > > > No, Dell does not have any ACPI methods. And as I wrote in ACPI
> > > > > > > or DMI is even not i2c address of device, so it needs to be
> > > > > > > specified in code itself.
> > > > > > >
> > > > > > > Really there is no other way... :-(
> > > > > >
> > > > > > Sure there is:
> > > > > >
> > > > > > 1. dell-smo8800 instantiates I2C device as "dell-smo8800-accel".
> > > > > > 2. dell-smo8800 provides read/write functions for lis3lv02d that
> > > > > > simply forward requests to dell-smo8800-accel i2c client.
> > > > > > 3. dell-smo8800 instantiates lis3lv02d instance like hp_accel
> > > > > > does.
> > > > >
> > > > > Sorry, but I do not understand how you mean it... Why to provides
> > > > > new read/write i2c functions which are already implemented by
> > > > > i2c-i801 bus and lis3lv02d i2c driver?
> > > >
> > > > Because that would allow you to avoid clashes with i2c creating
> > > > interrupt mapping for client residing on host-notify-capable
> > > > controller.
> > > >
> > > > > > Alternatively, can lis3lv02d be tasked to create /dev/freefall?
> > > > >
> > > > > If i2c_board_info contains IRQ then lis3lv02d create /dev/freefall
> > > > > device.
> > > > >
> > > > > But... what is problem with current implementation? Accelerometer
> > > > > HW provides two functions:
> > > > >
> > > > > 1) 3 axes reports
> > > > > 2) Disk freefall detection
> > > > >
> > > > > And 1) is handled by i2c driver lis3lv02d and 2) is by
> > > > > dell-smo8800. Both functions are independent here.
> > > > >
> > > > > I think you just trying to complicate this situation even more to
> > > > > be more complicated as currently is.
> > > >
> > > > Because this apparently does not work for you, does it?
> > >
> > > It is working fine. I do not see any problem.
> > >
> > > > In general,
> > > > if you want the same hardware be handled by 2 different drivers you
> > > > are going to have bad time.
> > >
> > > Yes, but in this case half of device is ACPI based and other half i2c
> > > based. This is problem of ACPI and Dell design.
> > >
> > > > It seems to be that /dev/freefall in dell-smo8800 and lis3lv02d are
> > > > the same, right?
> > >
> > > Yes. I understand that clean solution is to have one driver which
> > > provides everything.
> > >
> > > But because half of data are ACPI and half i2c, you still needs to
> > > create two drivers (one ACPI and one i2c). You can put both drivers into
> > > one .ko module, but still these will be two drivers due to how ACPI and
> > > i2c linux abstractions are different.
> > >
> > > > So, instead of having 2 drivers split the
> > > > functionality, can you forego registering smo8800 ACPI driver on
> > > > your whitelisted boxes and instead instantiate full i2c client
> > > > device with properly assigned both address and IRQ and let lis3lv02d
> > > > handle it (providing both accelerometer data and /dev/freefall)?
> > >
> > > With Michał we already discussed about it, see emails. Basically you can
> > > enable/disable kernel modules at compile time or blacklist at runtime
> > > (or even chose what will be compiled into vmlinux and what as external
> > > .ko module).
> >
> > This can be solved with a bit of Kconfig/IS_ENABLED() code.
> >
> > > Some distributions blacklist i2c-i801.ko module... And
> >
> > Any particular reason for that?
> >
> > > there can be also problem with initialization of i2c-i801 driver (fix is
> > > in commit a7ae81952cda, but does not have to work at every time!). So
> > > that move on whitelisted machines can potentially cause disappearance of
> > > /dev/freefall and users will not have hdd protection which is currently
> > > working.
> >
> > Well, I gave you 2 possible solutions (roll your own i2c read/write,
> > forward them to i2c client) or have faith in your implementation and let
> > lis3lv02d handle it.
> >
> > The 3rd one is to possibly add a flag to disable host notify to IRQ
> > mapping for given client (if Wolfram/Jean OK with it).
> >
> > Oh, the 4th one: change the irq in lis3lv02d.h to be "int" and change
> > the check in lis3lv02d.c to be "lis->irq <= 0" and instantiate your
> > i2c_client with board_info->irq = -1.
> >
> > Pick whichever you prefer.
> >
> > By the way, what do you need accelerometer for on these devices? They
> > don't appear to be tablets that could use one...
>
> Ah, you are talking about problem that after 4d5538f5882a lis3lv02d will
> not work... I thought that discussion is about different mechanism how
> to implement bus registration notification to smo8800 driver (or
> different solution to not have registration in i801).
>
Just because I am not sure I got everything right, could you confirm
that:
- in the current upstream tree, the dell-smo8800 driver is now broken
after 4d5538f5882a (i2c: use an IRQ to report Host Notify events, not
alert)
- this series adds an extra lis3lv02d on some machines and you have
problem fighting for the irq (but this is not upstream yet). The extra
lis3lv02d node is added from dell-smo8800
If the first point is not correct (by default, dell-smo8800 will not be
loaded at the same time than lis3lv02d), then it's a design issue with
the interactions between those 2 drivers.
If the first point is correct because ACPI declares both devices, then
there is an urgent fix to propose to not enable Host Notify by default
on Host Notifier capable adapters. (even though the design between the
2 drivers is wrong, it's considered as a regression).
Cheers,
Benjamin
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox