Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]
Groups > linux.kernel > #1550075 > unrolled thread
| Started by | Dmitry Torokhov <dmitry.torokhov@gmail.com> |
|---|---|
| First post | 2017-01-03 19:40 +0100 |
| Last post | 2017-01-03 21:30 +0100 |
| Articles | 20 on this page of 40 — 8 participants |
Back to article view | Back to linux.kernel
This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by
below is the oldest one visible, not the original post.
Re: [PATCH] i2c: i801: Register optional lis3lv02d i2c device on Dell machines Dmitry Torokhov <dmitry.torokhov@gmail.com> - 2017-01-03 19:40 +0100
Re: [PATCH] i2c: i801: Register optional lis3lv02d i2c device on Dell machines Pali Rohár <pali.rohar@gmail.com> - 2017-01-03 20:00 +0100
RE: [PATCH] i2c: i801: Register optional lis3lv02d i2c device on Dell machines <Mario.Limonciello@dell.com> - 2017-01-03 20:10 +0100
Re: [PATCH] i2c: i801: Register optional lis3lv02d i2c device on Dell machines Dmitry Torokhov <dmitry.torokhov@gmail.com> - 2017-01-03 20:50 +0100
Re: [PATCH] i2c: i801: Register optional lis3lv02d i2c device on Dell machines Pali Rohár <pali.rohar@gmail.com> - 2017-01-03 21:10 +0100
Re: [PATCH] i2c: i801: Register optional lis3lv02d i2c device on Dell machines Dmitry Torokhov <dmitry.torokhov@gmail.com> - 2017-01-03 21:40 +0100
Re: [PATCH] i2c: i801: Register optional lis3lv02d i2c device on Dell machines Pali Rohár <pali.rohar@gmail.com> - 2017-01-03 21:50 +0100
Re: [PATCH] i2c: i801: Register optional lis3lv02d i2c device on Dell machines Dmitry Torokhov <dmitry.torokhov@gmail.com> - 2017-01-03 22:10 +0100
Re: [PATCH] i2c: i801: Register optional lis3lv02d i2c device on Dell machines Pali Rohár <pali.rohar@gmail.com> - 2017-01-04 09:20 +0100
Re: [PATCH] i2c: i801: Register optional lis3lv02d i2c device on Dell machines Benjamin Tissoires <benjamin.tissoires@redhat.com> - 2017-01-04 10:10 +0100
Re: [PATCH] i2c: i801: Register optional lis3lv02d i2c device on Dell machines Pali Rohár <pali.rohar@gmail.com> - 2017-01-04 10:20 +0100
Re: [PATCH] i2c: i801: Register optional lis3lv02d i2c device on Dell machines Benjamin Tissoires <benjamin.tissoires@redhat.com> - 2017-01-04 11:20 +0100
Re: [PATCH] i2c: i801: Register optional lis3lv02d i2c device on Dell machines Pali Rohár <pali.rohar@gmail.com> - 2017-01-04 11:30 +0100
Re: [PATCH] i2c: i801: Register optional lis3lv02d i2c device on Dell machines Benjamin Tissoires <benjamin.tissoires@redhat.com> - 2017-01-04 11:40 +0100
Re: [PATCH] i2c: i801: Register optional lis3lv02d i2c device on Dell machines Pali Rohár <pali.rohar@gmail.com> - 2017-01-04 12:30 +0100
Re: [PATCH] i2c: i801: Register optional lis3lv02d i2c device on Dell machines Pali Rohár <pali.rohar@gmail.com> - 2017-01-04 13:10 +0100
Re: [PATCH] i2c: i801: Register optional lis3lv02d i2c device on Dell machines Benjamin Tissoires <benjamin.tissoires@redhat.com> - 2017-01-04 14:10 +0100
Re: [PATCH] i2c: i801: Register optional lis3lv02d i2c device on Dell machines Pali Rohár <pali.rohar@gmail.com> - 2017-01-04 17:10 +0100
Re: [PATCH] i2c: i801: Register optional lis3lv02d i2c device on Dell machines Benjamin Tissoires <benjamin.tissoires@redhat.com> - 2017-01-04 18:40 +0100
Re: [PATCH] i2c: i801: Register optional lis3lv02d i2c device on Dell machines Wolfram Sang <wsa@the-dreams.de> - 2017-01-04 18:50 +0100
Re: [PATCH] i2c: i801: Register optional lis3lv02d i2c device on Dell machines Dmitry Torokhov <dmitry.torokhov@gmail.com> - 2017-01-04 19:00 +0100
Re: [PATCH] i2c: i801: Register optional lis3lv02d i2c device on Dell machines Benjamin Tissoires <benjamin.tissoires@redhat.com> - 2017-01-04 23:00 +0100
Re: [PATCH] i2c: i801: Register optional lis3lv02d i2c device on Dell machines Dmitry Torokhov <dmitry.torokhov@gmail.com> - 2017-01-04 23:10 +0100
Re: [PATCH] i2c: i801: Register optional lis3lv02d i2c device on Dell machines Dmitry Torokhov <dmitry.torokhov@gmail.com> - 2017-01-04 23:10 +0100
Re: [PATCH] i2c: i801: Register optional lis3lv02d i2c device on Dell machines Wolfram Sang <wsa@the-dreams.de> - 2017-01-04 23:10 +0100
Re: [PATCH] i2c: i801: Register optional lis3lv02d i2c device on Dell machines Dmitry Torokhov <dmitry.torokhov@gmail.com> - 2017-01-04 23:10 +0100
Re: [PATCH] i2c: do not enable fall back to Host Notify by default kbuild test robot <lkp@intel.com> - 2017-01-05 03:30 +0100
Re: [PATCH] i2c: do not enable fall back to Host Notify by default kbuild test robot <lkp@intel.com> - 2017-01-05 03:30 +0100
Re: [PATCH] i2c: i801: Register optional lis3lv02d i2c device on Dell machines Pali Rohár <pali.rohar@gmail.com> - 2017-01-05 10:00 +0100
Re: [PATCH] i2c: i801: Register optional lis3lv02d i2c device on Dell machines Benjamin Tissoires <benjamin.tissoires@redhat.com> - 2017-01-05 10:30 +0100
Re: [PATCH] i2c: i801: Register optional lis3lv02d i2c device on Dell machines Jean Delvare <jdelvare@suse.de> - 2017-01-04 20:10 +0100
Re: [PATCH] i2c: i801: Register optional lis3lv02d i2c device on Dell machines Andy Shevchenko <andy.shevchenko@gmail.com> - 2017-01-04 11:00 +0100
Re: [PATCH] i2c: i801: Register optional lis3lv02d i2c device on Dell machines Benjamin Tissoires <benjamin.tissoires@redhat.com> - 2017-01-04 11:30 +0100
Re: [PATCH] i2c: i801: Register optional lis3lv02d i2c device on Dell machines Pali Rohár <pali.rohar@gmail.com> - 2017-01-04 12:40 +0100
Re: [PATCH] i2c: i801: Register optional lis3lv02d i2c device on Dell machines Benjamin Tissoires <benjamin.tissoires@redhat.com> - 2017-01-04 13:40 +0100
Re: [PATCH] i2c: i801: Register optional lis3lv02d i2c device on Dell machines Benjamin Tissoires <benjamin.tissoires@redhat.com> - 2017-01-03 22:40 +0100
Re: [PATCH] i2c: i801: Register optional lis3lv02d i2c device on Dell machines Dmitry Torokhov <dmitry.torokhov@gmail.com> - 2017-01-04 07:40 +0100
Re: [PATCH] i2c: i801: Register optional lis3lv02d i2c device on Dell machines Pali Rohár <pali.rohar@gmail.com> - 2017-01-04 10:30 +0100
Re: [PATCH] i2c: i801: Register optional lis3lv02d i2c device on Dell machines Benjamin Tissoires <benjamin.tissoires@redhat.com> - 2017-01-04 10:30 +0100
Re: [PATCH] i2c: i801: Register optional lis3lv02d i2c device on Dell machines Andy Shevchenko <andy.shevchenko@gmail.com> - 2017-01-03 21:30 +0100
Page 2 of 2 — ← Prev page 1 [2]
| From | Dmitry Torokhov <dmitry.torokhov@gmail.com> |
|---|---|
| Date | 2017-01-04 19:00 +0100 |
| Message-ID | <sVY0V-7Sy-11@gated-at.bofh.it> |
| In reply to | #1551031 |
On Wed, Jan 04, 2017 at 06:46:19PM +0100, Wolfram Sang wrote:
>
> > How about:
> > ---
> > From daa7571bbf337704332c0cfeec9b8fd5aeae596f Mon Sep 17 00:00:00 2001
> > From: Benjamin Tissoires <benjamin.tissoires@redhat.com>
> > Date: Wed, 4 Jan 2017 18:26:54 +0100
> > Subject: [PATCH] I2C: add the source of the IRQ in struct i2c_client
> >
> > With commit 4d5538f5882a ("i2c: use an IRQ to report Host Notify events,
> > not alert"), the IRQ provided in struct i2c_client might be assigned while
> > it has not been explicitly declared by either the platform information
> > or OF or ACPI.
> > Some drivers (lis3lv02d) rely on the fact that the IRQ gets assigned or
> > not to trigger a different behavior (exposing /dev/freefall in this case).
> >
> > Provide a way for others to know who set the IRQ and so they can behave
> > accordingly.
> >
> > Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
> > ---
> > drivers/i2c/i2c-core.c | 7 +++++++
> > include/linux/i2c.h | 11 +++++++++++
> > 2 files changed, 18 insertions(+)
> >
> > diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c
> > index cf9e396..226c75d 100644
> > --- a/drivers/i2c/i2c-core.c
> > +++ b/drivers/i2c/i2c-core.c
> > @@ -935,8 +935,12 @@ static int i2c_device_probe(struct device *dev)
> > irq = of_irq_get_byname(dev->of_node, "irq");
> > if (irq == -EINVAL || irq == -ENODATA)
> > irq = of_irq_get(dev->of_node, 0);
> > + if (irq > 0)
> > + client->irq_source = I2C_IRQ_SOURCE_OF;
> > } else if (ACPI_COMPANION(dev)) {
> > irq = acpi_dev_gpio_irq_get(ACPI_COMPANION(dev), 0);
> > + if (irq > 0)
> > + client->irq_source = I2C_IRQ_SOURCE_ACPI;
> > }
> > if (irq == -EPROBE_DEFER)
> > return irq;
> > @@ -947,6 +951,8 @@ static int i2c_device_probe(struct device *dev)
> > if (irq < 0) {
> > dev_dbg(dev, "Using Host Notify IRQ\n");
> > irq = i2c_smbus_host_notify_to_irq(client);
> > + if (irq > 0)
> > + client->irq_source = I2C_IRQ_SOURCE_HOST_NOTIFY;
> > }
> > if (irq < 0)
> > irq = 0;
> > @@ -1317,6 +1323,7 @@ i2c_new_device(struct i2c_adapter *adap, struct i2c_board_info const *info)
> > client->flags = info->flags;
> > client->addr = info->addr;
> > client->irq = info->irq;
> > + client->irq_source = I2C_IRQ_SOURCE_PLATFORM;
> >
> > strlcpy(client->name, info->type, sizeof(client->name));
> >
> > diff --git a/include/linux/i2c.h b/include/linux/i2c.h
> > index b2109c5..7d0368d 100644
> > --- a/include/linux/i2c.h
> > +++ b/include/linux/i2c.h
> > @@ -213,6 +213,13 @@ struct i2c_driver {
> > };
> > #define to_i2c_driver(d) container_of(d, struct i2c_driver, driver)
> >
> > +enum i2c_irq_source {
> > + I2C_IRQ_SOURCE_PLATFORM,
> > + I2C_IRQ_SOURCE_OF,
> > + I2C_IRQ_SOURCE_ACPI,
> > + I2C_IRQ_SOURCE_HOST_NOTIFY,
> > +};
> > +
> > /**
> > * struct i2c_client - represent an I2C slave device
> > * @flags: I2C_CLIENT_TEN indicates the device uses a ten bit chip address;
> > @@ -227,6 +234,9 @@ struct i2c_driver {
> > * userspace_devices list
> > * @slave_cb: Callback when I2C slave mode of an adapter is used. The adapter
> > * calls it to pass on slave events to the slave driver.
> > + * @irq_source: Enum which provides the source of the IRQ. Useful to know
> > + * if the IRQ was issued from Host Notify or if it was provided by an other
> > + * component.
>
> I'd think some documentation somewhere makes sense why we need to
> distinguish this in some cases?
I'd rather drivers be oblivious of the source of interrupt. If they need
to distinguish between them that means that our IRQ abstration failed.
>
> > *
> > * An i2c_client identifies a single device (i.e. chip) connected to an
> > * i2c bus. The behaviour exposed to Linux is defined by the driver
> > @@ -245,6 +255,7 @@ struct i2c_client {
> > #if IS_ENABLED(CONFIG_I2C_SLAVE)
> > i2c_slave_cb_t slave_cb; /* callback for slave mode */
> > #endif
> > + enum i2c_irq_source irq_source; /* which component assigned the irq */
> > };
> > #define to_i2c_client(d) container_of(d, struct i2c_client, dev)
> >
> > Dmitry, Wolfram, Jean, would this be acceptable for you?
>
> Adding something to i2c_driver is not exactly cheap, but from what I
> glimpsed from this thread, this is one of the cleanest solution to this
> problem?
>
As Benjamin said, it is really property of device [instance], not
driver. I.e. driver could handle both wired IRQ and HostNotify-based
scheme similarly, it is device (and board) that knows how stuff is
connected.
Maybe we could do something like this (untested):
From e362a0277fd1bd6112f258664d8831d9bc6b78da Mon Sep 17 00:00:00 2001
From: Dmitry Torokhov <dmitry.torokhov@gmail.com>
Date: Wed, 4 Jan 2017 09:33:43 -0800
Subject: [PATCH] i2c: do not enable fall back to Host Notify by default
Falling back unconditionally to HostNotify as primary client's interrupt
breaks some drivers which alter their functionality depending on whether
interrupt is present or not, so let's introduce a board flag telling I2C
core explicitly if we want wired interrupt or HostNotify-based one:
I2C_CLIENT_HOST_NOTIFY.
For DT-based systems we introduce "host-notofy" property that we convert
to I2C_CLIENT_HOST_NOTIFY board flag.
Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
---
Documentation/devicetree/bindings/i2c/i2c.txt | 8 ++++++++
drivers/i2c/i2c-core.c | 17 ++++++++---------
include/linux/i2c.h | 1 +
3 files changed, 17 insertions(+), 9 deletions(-)
diff --git a/Documentation/devicetree/bindings/i2c/i2c.txt b/Documentation/devicetree/bindings/i2c/i2c.txt
index 5fa691e6f638..cee9d5055fa2 100644
--- a/Documentation/devicetree/bindings/i2c/i2c.txt
+++ b/Documentation/devicetree/bindings/i2c/i2c.txt
@@ -62,6 +62,9 @@ wants to support one of the below features, it should adapt the bindings below.
"irq" and "wakeup" names are recognized by I2C core, other names are
left to individual drivers.
+- host-notify
+ device uses SMBus host notify protocol instead of interrupt line.
+
- multi-master
states that there is another master active on this bus. The OS can use
this information to adapt power management to keep the arbitration awake
@@ -81,6 +84,11 @@ Binding may contain optional "interrupts" property, describing interrupts
used by the device. I2C core will assign "irq" interrupt (or the very first
interrupt if not using interrupt names) as primary interrupt for the slave.
+Alternatively, devices supporting SMbus Host Notify, and connected to
+adapters that support this feature, may use "host-notify" property. I2C
+core will create a virtual interrupt for Host Notify and assign it as
+primary interrupt for the slave.
+
Also, if device is marked as a wakeup source, I2C core will set up "wakeup"
interrupt for the device. If "wakeup" interrupt name is not present in the
binding, then primary interrupt will be used as wakeup interrupt.
diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c
index cf9e396d7702..250969fa7670 100644
--- a/drivers/i2c/i2c-core.c
+++ b/drivers/i2c/i2c-core.c
@@ -931,7 +931,10 @@ static int i2c_device_probe(struct device *dev)
if (!client->irq) {
int irq = -ENOENT;
- if (dev->of_node) {
+ if (client->flags & I2C_CLIENT_HOST_HOTIFY) {
+ dev_dbg(dev, "Using Host Notify IRQ\n");
+ irq = i2c_smbus_host_notify_to_irq(client);
+ } else if (dev->of_node) {
irq = of_irq_get_byname(dev->of_node, "irq");
if (irq == -EINVAL || irq == -ENODATA)
irq = of_irq_get(dev->of_node, 0);
@@ -940,14 +943,7 @@ static int i2c_device_probe(struct device *dev)
}
if (irq == -EPROBE_DEFER)
return irq;
- /*
- * ACPI and OF did not find any useful IRQ, try to see
- * if Host Notify can be used.
- */
- if (irq < 0) {
- dev_dbg(dev, "Using Host Notify IRQ\n");
- irq = i2c_smbus_host_notify_to_irq(client);
- }
+
if (irq < 0)
irq = 0;
@@ -1716,6 +1712,9 @@ static struct i2c_client *of_i2c_register_device(struct i2c_adapter *adap,
info.of_node = of_node_get(node);
info.archdata = &dev_ad;
+ if (of_read_property_bool(node, "host-notify"))
+ info.flags |= I2C_CLIENT_HOST_NOTIFY;
+
if (of_get_property(node, "wakeup-source", NULL))
info.flags |= I2C_CLIENT_WAKE;
diff --git a/include/linux/i2c.h b/include/linux/i2c.h
index b2109c522dec..4b45ec46161f 100644
--- a/include/linux/i2c.h
+++ b/include/linux/i2c.h
@@ -665,6 +665,7 @@ i2c_unlock_adapter(struct i2c_adapter *adapter)
#define I2C_CLIENT_TEN 0x10 /* we have a ten bit chip address */
/* Must equal I2C_M_TEN below */
#define I2C_CLIENT_SLAVE 0x20 /* we are the slave */
+#define I2C_CLIENT_HOST_NOTIFY 0x40 /* We want to use I2C host notify */
#define I2C_CLIENT_WAKE 0x80 /* for board_info; true iff can wake */
#define I2C_CLIENT_SCCB 0x9000 /* Use Omnivision SCCB protocol */
/* Must match I2C_M_STOP|IGNORE_NAK */
--
2.11.0.390.gc69c2f50cf-goog
--
Dmitry
[toc] | [prev] | [next] | [standalone]
| From | Benjamin Tissoires <benjamin.tissoires@redhat.com> |
|---|---|
| Date | 2017-01-04 23:00 +0100 |
| Message-ID | <sW1Lb-1ZK-7@gated-at.bofh.it> |
| In reply to | #1551040 |
On Jan 04 2017 or thereabouts, Dmitry Torokhov wrote:
> On Wed, Jan 04, 2017 at 06:46:19PM +0100, Wolfram Sang wrote:
> >
> > > How about:
> > > ---
> > > From daa7571bbf337704332c0cfeec9b8fd5aeae596f Mon Sep 17 00:00:00 2001
> > > From: Benjamin Tissoires <benjamin.tissoires@redhat.com>
> > > Date: Wed, 4 Jan 2017 18:26:54 +0100
> > > Subject: [PATCH] I2C: add the source of the IRQ in struct i2c_client
> > >
> > > With commit 4d5538f5882a ("i2c: use an IRQ to report Host Notify events,
> > > not alert"), the IRQ provided in struct i2c_client might be assigned while
> > > it has not been explicitly declared by either the platform information
> > > or OF or ACPI.
> > > Some drivers (lis3lv02d) rely on the fact that the IRQ gets assigned or
> > > not to trigger a different behavior (exposing /dev/freefall in this case).
> > >
> > > Provide a way for others to know who set the IRQ and so they can behave
> > > accordingly.
> > >
> > > Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
> > > ---
> > > drivers/i2c/i2c-core.c | 7 +++++++
> > > include/linux/i2c.h | 11 +++++++++++
> > > 2 files changed, 18 insertions(+)
> > >
> > > diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c
> > > index cf9e396..226c75d 100644
> > > --- a/drivers/i2c/i2c-core.c
> > > +++ b/drivers/i2c/i2c-core.c
> > > @@ -935,8 +935,12 @@ static int i2c_device_probe(struct device *dev)
> > > irq = of_irq_get_byname(dev->of_node, "irq");
> > > if (irq == -EINVAL || irq == -ENODATA)
> > > irq = of_irq_get(dev->of_node, 0);
> > > + if (irq > 0)
> > > + client->irq_source = I2C_IRQ_SOURCE_OF;
> > > } else if (ACPI_COMPANION(dev)) {
> > > irq = acpi_dev_gpio_irq_get(ACPI_COMPANION(dev), 0);
> > > + if (irq > 0)
> > > + client->irq_source = I2C_IRQ_SOURCE_ACPI;
> > > }
> > > if (irq == -EPROBE_DEFER)
> > > return irq;
> > > @@ -947,6 +951,8 @@ static int i2c_device_probe(struct device *dev)
> > > if (irq < 0) {
> > > dev_dbg(dev, "Using Host Notify IRQ\n");
> > > irq = i2c_smbus_host_notify_to_irq(client);
> > > + if (irq > 0)
> > > + client->irq_source = I2C_IRQ_SOURCE_HOST_NOTIFY;
> > > }
> > > if (irq < 0)
> > > irq = 0;
> > > @@ -1317,6 +1323,7 @@ i2c_new_device(struct i2c_adapter *adap, struct i2c_board_info const *info)
> > > client->flags = info->flags;
> > > client->addr = info->addr;
> > > client->irq = info->irq;
> > > + client->irq_source = I2C_IRQ_SOURCE_PLATFORM;
> > >
> > > strlcpy(client->name, info->type, sizeof(client->name));
> > >
> > > diff --git a/include/linux/i2c.h b/include/linux/i2c.h
> > > index b2109c5..7d0368d 100644
> > > --- a/include/linux/i2c.h
> > > +++ b/include/linux/i2c.h
> > > @@ -213,6 +213,13 @@ struct i2c_driver {
> > > };
> > > #define to_i2c_driver(d) container_of(d, struct i2c_driver, driver)
> > >
> > > +enum i2c_irq_source {
> > > + I2C_IRQ_SOURCE_PLATFORM,
> > > + I2C_IRQ_SOURCE_OF,
> > > + I2C_IRQ_SOURCE_ACPI,
> > > + I2C_IRQ_SOURCE_HOST_NOTIFY,
> > > +};
> > > +
> > > /**
> > > * struct i2c_client - represent an I2C slave device
> > > * @flags: I2C_CLIENT_TEN indicates the device uses a ten bit chip address;
> > > @@ -227,6 +234,9 @@ struct i2c_driver {
> > > * userspace_devices list
> > > * @slave_cb: Callback when I2C slave mode of an adapter is used. The adapter
> > > * calls it to pass on slave events to the slave driver.
> > > + * @irq_source: Enum which provides the source of the IRQ. Useful to know
> > > + * if the IRQ was issued from Host Notify or if it was provided by an other
> > > + * component.
> >
> > I'd think some documentation somewhere makes sense why we need to
> > distinguish this in some cases?
>
> I'd rather drivers be oblivious of the source of interrupt. If they need
> to distinguish between them that means that our IRQ abstration failed.
>
> >
> > > *
> > > * An i2c_client identifies a single device (i.e. chip) connected to an
> > > * i2c bus. The behaviour exposed to Linux is defined by the driver
> > > @@ -245,6 +255,7 @@ struct i2c_client {
> > > #if IS_ENABLED(CONFIG_I2C_SLAVE)
> > > i2c_slave_cb_t slave_cb; /* callback for slave mode */
> > > #endif
> > > + enum i2c_irq_source irq_source; /* which component assigned the irq */
> > > };
> > > #define to_i2c_client(d) container_of(d, struct i2c_client, dev)
> > >
> > > Dmitry, Wolfram, Jean, would this be acceptable for you?
> >
> > Adding something to i2c_driver is not exactly cheap, but from what I
> > glimpsed from this thread, this is one of the cleanest solution to this
> > problem?
> >
>
> As Benjamin said, it is really property of device [instance], not
> driver. I.e. driver could handle both wired IRQ and HostNotify-based
> scheme similarly, it is device (and board) that knows how stuff is
> connected.
>
> Maybe we could do something like this (untested):
>
>
> From e362a0277fd1bd6112f258664d8831d9bc6b78da Mon Sep 17 00:00:00 2001
> From: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> Date: Wed, 4 Jan 2017 09:33:43 -0800
> Subject: [PATCH] i2c: do not enable fall back to Host Notify by default
>
> Falling back unconditionally to HostNotify as primary client's interrupt
> breaks some drivers which alter their functionality depending on whether
> interrupt is present or not, so let's introduce a board flag telling I2C
> core explicitly if we want wired interrupt or HostNotify-based one:
> I2C_CLIENT_HOST_NOTIFY.
>
> For DT-based systems we introduce "host-notofy" property that we convert
typo: s/host-notofy/host-notify/
> to I2C_CLIENT_HOST_NOTIFY board flag.
>
> Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> ---
> Documentation/devicetree/bindings/i2c/i2c.txt | 8 ++++++++
> drivers/i2c/i2c-core.c | 17 ++++++++---------
> include/linux/i2c.h | 1 +
> 3 files changed, 17 insertions(+), 9 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/i2c/i2c.txt b/Documentation/devicetree/bindings/i2c/i2c.txt
> index 5fa691e6f638..cee9d5055fa2 100644
> --- a/Documentation/devicetree/bindings/i2c/i2c.txt
> +++ b/Documentation/devicetree/bindings/i2c/i2c.txt
> @@ -62,6 +62,9 @@ wants to support one of the below features, it should adapt the bindings below.
> "irq" and "wakeup" names are recognized by I2C core, other names are
> left to individual drivers.
>
> +- host-notify
> + device uses SMBus host notify protocol instead of interrupt line.
> +
> - multi-master
> states that there is another master active on this bus. The OS can use
> this information to adapt power management to keep the arbitration awake
> @@ -81,6 +84,11 @@ Binding may contain optional "interrupts" property, describing interrupts
> used by the device. I2C core will assign "irq" interrupt (or the very first
> interrupt if not using interrupt names) as primary interrupt for the slave.
>
> +Alternatively, devices supporting SMbus Host Notify, and connected to
> +adapters that support this feature, may use "host-notify" property. I2C
> +core will create a virtual interrupt for Host Notify and assign it as
> +primary interrupt for the slave.
> +
> Also, if device is marked as a wakeup source, I2C core will set up "wakeup"
> interrupt for the device. If "wakeup" interrupt name is not present in the
> binding, then primary interrupt will be used as wakeup interrupt.
> diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c
> index cf9e396d7702..250969fa7670 100644
> --- a/drivers/i2c/i2c-core.c
> +++ b/drivers/i2c/i2c-core.c
> @@ -931,7 +931,10 @@ static int i2c_device_probe(struct device *dev)
> if (!client->irq) {
> int irq = -ENOENT;
>
> - if (dev->of_node) {
> + if (client->flags & I2C_CLIENT_HOST_HOTIFY) {
typo: s/I2C_CLIENT_HOST_HOTIFY/I2C_CLIENT_HOST_NOTIFY/
With these fixed, the code is:
Tested-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
I tested both with and without the I2C_CLIENT_HOST_HOTIFY flag on the
Thinkpad T450s, and everything is in order.
Thanks Dmitry for the patch!
Cheers,
Benjamin
> + dev_dbg(dev, "Using Host Notify IRQ\n");
> + irq = i2c_smbus_host_notify_to_irq(client);
> + } else if (dev->of_node) {
> irq = of_irq_get_byname(dev->of_node, "irq");
> if (irq == -EINVAL || irq == -ENODATA)
> irq = of_irq_get(dev->of_node, 0);
> @@ -940,14 +943,7 @@ static int i2c_device_probe(struct device *dev)
> }
> if (irq == -EPROBE_DEFER)
> return irq;
> - /*
> - * ACPI and OF did not find any useful IRQ, try to see
> - * if Host Notify can be used.
> - */
> - if (irq < 0) {
> - dev_dbg(dev, "Using Host Notify IRQ\n");
> - irq = i2c_smbus_host_notify_to_irq(client);
> - }
> +
> if (irq < 0)
> irq = 0;
>
> @@ -1716,6 +1712,9 @@ static struct i2c_client *of_i2c_register_device(struct i2c_adapter *adap,
> info.of_node = of_node_get(node);
> info.archdata = &dev_ad;
>
> + if (of_read_property_bool(node, "host-notify"))
> + info.flags |= I2C_CLIENT_HOST_NOTIFY;
> +
> if (of_get_property(node, "wakeup-source", NULL))
> info.flags |= I2C_CLIENT_WAKE;
>
> diff --git a/include/linux/i2c.h b/include/linux/i2c.h
> index b2109c522dec..4b45ec46161f 100644
> --- a/include/linux/i2c.h
> +++ b/include/linux/i2c.h
> @@ -665,6 +665,7 @@ i2c_unlock_adapter(struct i2c_adapter *adapter)
> #define I2C_CLIENT_TEN 0x10 /* we have a ten bit chip address */
> /* Must equal I2C_M_TEN below */
> #define I2C_CLIENT_SLAVE 0x20 /* we are the slave */
> +#define I2C_CLIENT_HOST_NOTIFY 0x40 /* We want to use I2C host notify */
> #define I2C_CLIENT_WAKE 0x80 /* for board_info; true iff can wake */
> #define I2C_CLIENT_SCCB 0x9000 /* Use Omnivision SCCB protocol */
> /* Must match I2C_M_STOP|IGNORE_NAK */
> --
> 2.11.0.390.gc69c2f50cf-goog
>
>
> --
> Dmitry
[toc] | [prev] | [next] | [standalone]
| From | Dmitry Torokhov <dmitry.torokhov@gmail.com> |
|---|---|
| Date | 2017-01-04 23:10 +0100 |
| Message-ID | <sW1US-2ik-45@gated-at.bofh.it> |
| In reply to | #1551430 |
On Wed, Jan 04, 2017 at 10:49:06PM +0100, Benjamin Tissoires wrote:
> On Jan 04 2017 or thereabouts, Dmitry Torokhov wrote:
> > On Wed, Jan 04, 2017 at 06:46:19PM +0100, Wolfram Sang wrote:
> > >
> > > > How about:
> > > > ---
> > > > From daa7571bbf337704332c0cfeec9b8fd5aeae596f Mon Sep 17 00:00:00 2001
> > > > From: Benjamin Tissoires <benjamin.tissoires@redhat.com>
> > > > Date: Wed, 4 Jan 2017 18:26:54 +0100
> > > > Subject: [PATCH] I2C: add the source of the IRQ in struct i2c_client
> > > >
> > > > With commit 4d5538f5882a ("i2c: use an IRQ to report Host Notify events,
> > > > not alert"), the IRQ provided in struct i2c_client might be assigned while
> > > > it has not been explicitly declared by either the platform information
> > > > or OF or ACPI.
> > > > Some drivers (lis3lv02d) rely on the fact that the IRQ gets assigned or
> > > > not to trigger a different behavior (exposing /dev/freefall in this case).
> > > >
> > > > Provide a way for others to know who set the IRQ and so they can behave
> > > > accordingly.
> > > >
> > > > Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
> > > > ---
> > > > drivers/i2c/i2c-core.c | 7 +++++++
> > > > include/linux/i2c.h | 11 +++++++++++
> > > > 2 files changed, 18 insertions(+)
> > > >
> > > > diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c
> > > > index cf9e396..226c75d 100644
> > > > --- a/drivers/i2c/i2c-core.c
> > > > +++ b/drivers/i2c/i2c-core.c
> > > > @@ -935,8 +935,12 @@ static int i2c_device_probe(struct device *dev)
> > > > irq = of_irq_get_byname(dev->of_node, "irq");
> > > > if (irq == -EINVAL || irq == -ENODATA)
> > > > irq = of_irq_get(dev->of_node, 0);
> > > > + if (irq > 0)
> > > > + client->irq_source = I2C_IRQ_SOURCE_OF;
> > > > } else if (ACPI_COMPANION(dev)) {
> > > > irq = acpi_dev_gpio_irq_get(ACPI_COMPANION(dev), 0);
> > > > + if (irq > 0)
> > > > + client->irq_source = I2C_IRQ_SOURCE_ACPI;
> > > > }
> > > > if (irq == -EPROBE_DEFER)
> > > > return irq;
> > > > @@ -947,6 +951,8 @@ static int i2c_device_probe(struct device *dev)
> > > > if (irq < 0) {
> > > > dev_dbg(dev, "Using Host Notify IRQ\n");
> > > > irq = i2c_smbus_host_notify_to_irq(client);
> > > > + if (irq > 0)
> > > > + client->irq_source = I2C_IRQ_SOURCE_HOST_NOTIFY;
> > > > }
> > > > if (irq < 0)
> > > > irq = 0;
> > > > @@ -1317,6 +1323,7 @@ i2c_new_device(struct i2c_adapter *adap, struct i2c_board_info const *info)
> > > > client->flags = info->flags;
> > > > client->addr = info->addr;
> > > > client->irq = info->irq;
> > > > + client->irq_source = I2C_IRQ_SOURCE_PLATFORM;
> > > >
> > > > strlcpy(client->name, info->type, sizeof(client->name));
> > > >
> > > > diff --git a/include/linux/i2c.h b/include/linux/i2c.h
> > > > index b2109c5..7d0368d 100644
> > > > --- a/include/linux/i2c.h
> > > > +++ b/include/linux/i2c.h
> > > > @@ -213,6 +213,13 @@ struct i2c_driver {
> > > > };
> > > > #define to_i2c_driver(d) container_of(d, struct i2c_driver, driver)
> > > >
> > > > +enum i2c_irq_source {
> > > > + I2C_IRQ_SOURCE_PLATFORM,
> > > > + I2C_IRQ_SOURCE_OF,
> > > > + I2C_IRQ_SOURCE_ACPI,
> > > > + I2C_IRQ_SOURCE_HOST_NOTIFY,
> > > > +};
> > > > +
> > > > /**
> > > > * struct i2c_client - represent an I2C slave device
> > > > * @flags: I2C_CLIENT_TEN indicates the device uses a ten bit chip address;
> > > > @@ -227,6 +234,9 @@ struct i2c_driver {
> > > > * userspace_devices list
> > > > * @slave_cb: Callback when I2C slave mode of an adapter is used. The adapter
> > > > * calls it to pass on slave events to the slave driver.
> > > > + * @irq_source: Enum which provides the source of the IRQ. Useful to know
> > > > + * if the IRQ was issued from Host Notify or if it was provided by an other
> > > > + * component.
> > >
> > > I'd think some documentation somewhere makes sense why we need to
> > > distinguish this in some cases?
> >
> > I'd rather drivers be oblivious of the source of interrupt. If they need
> > to distinguish between them that means that our IRQ abstration failed.
> >
> > >
> > > > *
> > > > * An i2c_client identifies a single device (i.e. chip) connected to an
> > > > * i2c bus. The behaviour exposed to Linux is defined by the driver
> > > > @@ -245,6 +255,7 @@ struct i2c_client {
> > > > #if IS_ENABLED(CONFIG_I2C_SLAVE)
> > > > i2c_slave_cb_t slave_cb; /* callback for slave mode */
> > > > #endif
> > > > + enum i2c_irq_source irq_source; /* which component assigned the irq */
> > > > };
> > > > #define to_i2c_client(d) container_of(d, struct i2c_client, dev)
> > > >
> > > > Dmitry, Wolfram, Jean, would this be acceptable for you?
> > >
> > > Adding something to i2c_driver is not exactly cheap, but from what I
> > > glimpsed from this thread, this is one of the cleanest solution to this
> > > problem?
> > >
> >
> > As Benjamin said, it is really property of device [instance], not
> > driver. I.e. driver could handle both wired IRQ and HostNotify-based
> > scheme similarly, it is device (and board) that knows how stuff is
> > connected.
> >
> > Maybe we could do something like this (untested):
> >
> >
> > From e362a0277fd1bd6112f258664d8831d9bc6b78da Mon Sep 17 00:00:00 2001
> > From: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> > Date: Wed, 4 Jan 2017 09:33:43 -0800
> > Subject: [PATCH] i2c: do not enable fall back to Host Notify by default
> >
> > Falling back unconditionally to HostNotify as primary client's interrupt
> > breaks some drivers which alter their functionality depending on whether
> > interrupt is present or not, so let's introduce a board flag telling I2C
> > core explicitly if we want wired interrupt or HostNotify-based one:
> > I2C_CLIENT_HOST_NOTIFY.
> >
> > For DT-based systems we introduce "host-notofy" property that we convert
>
> typo: s/host-notofy/host-notify/
>
> > to I2C_CLIENT_HOST_NOTIFY board flag.
> >
> > Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> > ---
> > Documentation/devicetree/bindings/i2c/i2c.txt | 8 ++++++++
> > drivers/i2c/i2c-core.c | 17 ++++++++---------
> > include/linux/i2c.h | 1 +
> > 3 files changed, 17 insertions(+), 9 deletions(-)
> >
> > diff --git a/Documentation/devicetree/bindings/i2c/i2c.txt b/Documentation/devicetree/bindings/i2c/i2c.txt
> > index 5fa691e6f638..cee9d5055fa2 100644
> > --- a/Documentation/devicetree/bindings/i2c/i2c.txt
> > +++ b/Documentation/devicetree/bindings/i2c/i2c.txt
> > @@ -62,6 +62,9 @@ wants to support one of the below features, it should adapt the bindings below.
> > "irq" and "wakeup" names are recognized by I2C core, other names are
> > left to individual drivers.
> >
> > +- host-notify
> > + device uses SMBus host notify protocol instead of interrupt line.
> > +
> > - multi-master
> > states that there is another master active on this bus. The OS can use
> > this information to adapt power management to keep the arbitration awake
> > @@ -81,6 +84,11 @@ Binding may contain optional "interrupts" property, describing interrupts
> > used by the device. I2C core will assign "irq" interrupt (or the very first
> > interrupt if not using interrupt names) as primary interrupt for the slave.
> >
> > +Alternatively, devices supporting SMbus Host Notify, and connected to
> > +adapters that support this feature, may use "host-notify" property. I2C
> > +core will create a virtual interrupt for Host Notify and assign it as
> > +primary interrupt for the slave.
> > +
> > Also, if device is marked as a wakeup source, I2C core will set up "wakeup"
> > interrupt for the device. If "wakeup" interrupt name is not present in the
> > binding, then primary interrupt will be used as wakeup interrupt.
> > diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c
> > index cf9e396d7702..250969fa7670 100644
> > --- a/drivers/i2c/i2c-core.c
> > +++ b/drivers/i2c/i2c-core.c
> > @@ -931,7 +931,10 @@ static int i2c_device_probe(struct device *dev)
> > if (!client->irq) {
> > int irq = -ENOENT;
> >
> > - if (dev->of_node) {
> > + if (client->flags & I2C_CLIENT_HOST_HOTIFY) {
>
> typo: s/I2C_CLIENT_HOST_HOTIFY/I2C_CLIENT_HOST_NOTIFY/
>
> With these fixed, the code is:
> Tested-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
>
> I tested both with and without the I2C_CLIENT_HOST_HOTIFY flag on the
> Thinkpad T450s, and everything is in order.
>
> Thanks Dmitry for the patch!
Thanks Benjamin. Let me submit the patch "officially" and CC Rob & DT
folks on binding change.
--
Dmitry
[toc] | [prev] | [next] | [standalone]
| From | Dmitry Torokhov <dmitry.torokhov@gmail.com> |
|---|---|
| Date | 2017-01-04 23:10 +0100 |
| Message-ID | <sW1US-2ik-19@gated-at.bofh.it> |
| In reply to | #1551040 |
On Wed, Jan 04, 2017 at 02:00:30PM -0800, Dmitry Torokhov wrote: > On Wed, Jan 04, 2017 at 10:55:47PM +0100, Wolfram Sang wrote: > > > > > From e362a0277fd1bd6112f258664d8831d9bc6b78da Mon Sep 17 00:00:00 2001 > > > From: Dmitry Torokhov <dmitry.torokhov@gmail.com> > > > Date: Wed, 4 Jan 2017 09:33:43 -0800 > > > Subject: [PATCH] i2c: do not enable fall back to Host Notify by default > > > > > > Falling back unconditionally to HostNotify as primary client's interrupt > > > breaks some drivers which alter their functionality depending on whether > > > interrupt is present or not, so let's introduce a board flag telling I2C > > > core explicitly if we want wired interrupt or HostNotify-based one: > > > I2C_CLIENT_HOST_NOTIFY. > > > > > > For DT-based systems we introduce "host-notofy" property that we convert > > > to I2C_CLIENT_HOST_NOTIFY board flag. > > > > > > Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com> > > > > Yay, this looks better to me. One nit: > > > > > +Alternatively, devices supporting SMbus Host Notify, and connected to > > > +adapters that support this feature, may use "host-notify" property. I2C > > > +core will create a virtual interrupt for Host Notify and assign it as > > > +primary interrupt for the slave. > > > > This paragraph sounds Linux-ish while binding docs should be OS > > agnostic. Maybe we can shorten the second sentence to "It will be > > assigned then as the primary interrupt for the slave."? > > Heh, I just sent out patch to DT folks. Provided that they are fine with > the new property I can either send V2 or you could edit when applying, > whatever is easier for you. That said, both variants are Linux-ish to me: I do not believe that "primary slave interrupt" for I2C devices is a generic concept. Is it? Do BSD and Windows use it? Also, it matches the paragraph above that also talks about I2C core. In any case, it is your decision, I'm just making random noise here. Thanks. -- Dmitry
[toc] | [prev] | [next] | [standalone]
| From | Wolfram Sang <wsa@the-dreams.de> |
|---|---|
| Date | 2017-01-04 23:10 +0100 |
| Message-ID | <sW1US-2ik-21@gated-at.bofh.it> |
| In reply to | #1551040 |
[Multipart message — attachments visible in raw view] — view raw
> From e362a0277fd1bd6112f258664d8831d9bc6b78da Mon Sep 17 00:00:00 2001 > From: Dmitry Torokhov <dmitry.torokhov@gmail.com> > Date: Wed, 4 Jan 2017 09:33:43 -0800 > Subject: [PATCH] i2c: do not enable fall back to Host Notify by default > > Falling back unconditionally to HostNotify as primary client's interrupt > breaks some drivers which alter their functionality depending on whether > interrupt is present or not, so let's introduce a board flag telling I2C > core explicitly if we want wired interrupt or HostNotify-based one: > I2C_CLIENT_HOST_NOTIFY. > > For DT-based systems we introduce "host-notofy" property that we convert > to I2C_CLIENT_HOST_NOTIFY board flag. > > Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com> Yay, this looks better to me. One nit: > +Alternatively, devices supporting SMbus Host Notify, and connected to > +adapters that support this feature, may use "host-notify" property. I2C > +core will create a virtual interrupt for Host Notify and assign it as > +primary interrupt for the slave. This paragraph sounds Linux-ish while binding docs should be OS agnostic. Maybe we can shorten the second sentence to "It will be assigned then as the primary interrupt for the slave."?
[toc] | [prev] | [next] | [standalone]
| From | Dmitry Torokhov <dmitry.torokhov@gmail.com> |
|---|---|
| Date | 2017-01-04 23:10 +0100 |
| Message-ID | <sW1US-2ik-23@gated-at.bofh.it> |
| In reply to | #1551441 |
On Wed, Jan 04, 2017 at 10:55:47PM +0100, Wolfram Sang wrote: > > > From e362a0277fd1bd6112f258664d8831d9bc6b78da Mon Sep 17 00:00:00 2001 > > From: Dmitry Torokhov <dmitry.torokhov@gmail.com> > > Date: Wed, 4 Jan 2017 09:33:43 -0800 > > Subject: [PATCH] i2c: do not enable fall back to Host Notify by default > > > > Falling back unconditionally to HostNotify as primary client's interrupt > > breaks some drivers which alter their functionality depending on whether > > interrupt is present or not, so let's introduce a board flag telling I2C > > core explicitly if we want wired interrupt or HostNotify-based one: > > I2C_CLIENT_HOST_NOTIFY. > > > > For DT-based systems we introduce "host-notofy" property that we convert > > to I2C_CLIENT_HOST_NOTIFY board flag. > > > > Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com> > > Yay, this looks better to me. One nit: > > > +Alternatively, devices supporting SMbus Host Notify, and connected to > > +adapters that support this feature, may use "host-notify" property. I2C > > +core will create a virtual interrupt for Host Notify and assign it as > > +primary interrupt for the slave. > > This paragraph sounds Linux-ish while binding docs should be OS > agnostic. Maybe we can shorten the second sentence to "It will be > assigned then as the primary interrupt for the slave."? Heh, I just sent out patch to DT folks. Provided that they are fine with the new property I can either send V2 or you could edit when applying, whatever is easier for you. Thanks. -- Dmitry
[toc] | [prev] | [next] | [standalone]
| From | kbuild test robot <lkp@intel.com> |
|---|---|
| Date | 2017-01-05 03:30 +0100 |
| Subject | Re: [PATCH] i2c: do not enable fall back to Host Notify by default |
| Message-ID | <sW5Yu-4QA-3@gated-at.bofh.it> |
| In reply to | #1551040 |
[Multipart message — attachments visible in raw view] — view raw
Hi Dmitry,
[auto build test ERROR on wsa/i2c/for-next]
[also build test ERROR on v4.10-rc2 next-20170104]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
url: https://github.com/0day-ci/linux/commits/Dmitry-Torokhov/i2c-do-not-enable-fall-back-to-Host-Notify-by-default/20170105-095405
base: https://git.kernel.org/pub/scm/linux/kernel/git/wsa/linux.git i2c/for-next
config: x86_64-randconfig-x017-201701 (attached as .config)
compiler: gcc-6 (Debian 6.2.0-3) 6.2.0 20160901
reproduce:
# save the attached .config to linux build tree
make ARCH=x86_64
All errors (new ones prefixed by >>):
drivers/i2c/i2c-core.c: In function 'i2c_device_probe':
drivers/i2c/i2c-core.c:934:23: error: 'I2C_CLIENT_HOST_HOTIFY' undeclared (first use in this function)
if (client->flags & I2C_CLIENT_HOST_HOTIFY) {
^~~~~~~~~~~~~~~~~~~~~~
drivers/i2c/i2c-core.c:934:23: note: each undeclared identifier is reported only once for each function it appears in
drivers/i2c/i2c-core.c: In function 'of_i2c_register_device':
>> drivers/i2c/i2c-core.c:1715:6: error: implicit declaration of function 'of_read_property_bool' [-Werror=implicit-function-declaration]
if (of_read_property_bool(node, "host-notify"))
^~~~~~~~~~~~~~~~~~~~~
cc1: some warnings being treated as errors
vim +/of_read_property_bool +1715 drivers/i2c/i2c-core.c
1709 }
1710
1711 info.addr = addr;
1712 info.of_node = of_node_get(node);
1713 info.archdata = &dev_ad;
1714
> 1715 if (of_read_property_bool(node, "host-notify"))
1716 info.flags |= I2C_CLIENT_HOST_NOTIFY;
1717
1718 if (of_get_property(node, "wakeup-source", NULL))
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
[toc] | [prev] | [next] | [standalone]
| From | kbuild test robot <lkp@intel.com> |
|---|---|
| Date | 2017-01-05 03:30 +0100 |
| Subject | Re: [PATCH] i2c: do not enable fall back to Host Notify by default |
| Message-ID | <sW5Yu-4QA-17@gated-at.bofh.it> |
| In reply to | #1551040 |
[Multipart message — attachments visible in raw view] — view raw
Hi Dmitry,
[auto build test ERROR on wsa/i2c/for-next]
[also build test ERROR on v4.10-rc2 next-20170104]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
url: https://github.com/0day-ci/linux/commits/Dmitry-Torokhov/i2c-do-not-enable-fall-back-to-Host-Notify-by-default/20170105-095405
base: https://git.kernel.org/pub/scm/linux/kernel/git/wsa/linux.git i2c/for-next
config: x86_64-randconfig-x011-201701 (attached as .config)
compiler: gcc-6 (Debian 6.2.0-3) 6.2.0 20160901
reproduce:
# save the attached .config to linux build tree
make ARCH=x86_64
All errors (new ones prefixed by >>):
drivers/i2c/i2c-core.c: In function 'i2c_device_probe':
>> drivers/i2c/i2c-core.c:934:23: error: 'I2C_CLIENT_HOST_HOTIFY' undeclared (first use in this function)
if (client->flags & I2C_CLIENT_HOST_HOTIFY) {
^~~~~~~~~~~~~~~~~~~~~~
drivers/i2c/i2c-core.c:934:23: note: each undeclared identifier is reported only once for each function it appears in
vim +/I2C_CLIENT_HOST_HOTIFY +934 drivers/i2c/i2c-core.c
928 if (!client)
929 return 0;
930
931 if (!client->irq) {
932 int irq = -ENOENT;
933
> 934 if (client->flags & I2C_CLIENT_HOST_HOTIFY) {
935 dev_dbg(dev, "Using Host Notify IRQ\n");
936 irq = i2c_smbus_host_notify_to_irq(client);
937 } else if (dev->of_node) {
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
[toc] | [prev] | [next] | [standalone]
| From | Pali Rohár <pali.rohar@gmail.com> |
|---|---|
| Date | 2017-01-05 10:00 +0100 |
| Message-ID | <sWc3U-pg-11@gated-at.bofh.it> |
| In reply to | #1551040 |
On Wednesday 04 January 2017 09:54:56 Dmitry Torokhov wrote:
> On Wed, Jan 04, 2017 at 06:46:19PM +0100, Wolfram Sang wrote:
> >
> > > How about:
> > > ---
> > > From daa7571bbf337704332c0cfeec9b8fd5aeae596f Mon Sep 17 00:00:00 2001
> > > From: Benjamin Tissoires <benjamin.tissoires@redhat.com>
> > > Date: Wed, 4 Jan 2017 18:26:54 +0100
> > > Subject: [PATCH] I2C: add the source of the IRQ in struct i2c_client
> > >
> > > With commit 4d5538f5882a ("i2c: use an IRQ to report Host Notify events,
> > > not alert"), the IRQ provided in struct i2c_client might be assigned while
> > > it has not been explicitly declared by either the platform information
> > > or OF or ACPI.
> > > Some drivers (lis3lv02d) rely on the fact that the IRQ gets assigned or
> > > not to trigger a different behavior (exposing /dev/freefall in this case).
> > >
> > > Provide a way for others to know who set the IRQ and so they can behave
> > > accordingly.
> > >
> > > Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
> > > ---
> > > drivers/i2c/i2c-core.c | 7 +++++++
> > > include/linux/i2c.h | 11 +++++++++++
> > > 2 files changed, 18 insertions(+)
> > >
> > > diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c
> > > index cf9e396..226c75d 100644
> > > --- a/drivers/i2c/i2c-core.c
> > > +++ b/drivers/i2c/i2c-core.c
> > > @@ -935,8 +935,12 @@ static int i2c_device_probe(struct device *dev)
> > > irq = of_irq_get_byname(dev->of_node, "irq");
> > > if (irq == -EINVAL || irq == -ENODATA)
> > > irq = of_irq_get(dev->of_node, 0);
> > > + if (irq > 0)
> > > + client->irq_source = I2C_IRQ_SOURCE_OF;
> > > } else if (ACPI_COMPANION(dev)) {
> > > irq = acpi_dev_gpio_irq_get(ACPI_COMPANION(dev), 0);
> > > + if (irq > 0)
> > > + client->irq_source = I2C_IRQ_SOURCE_ACPI;
> > > }
> > > if (irq == -EPROBE_DEFER)
> > > return irq;
> > > @@ -947,6 +951,8 @@ static int i2c_device_probe(struct device *dev)
> > > if (irq < 0) {
> > > dev_dbg(dev, "Using Host Notify IRQ\n");
> > > irq = i2c_smbus_host_notify_to_irq(client);
> > > + if (irq > 0)
> > > + client->irq_source = I2C_IRQ_SOURCE_HOST_NOTIFY;
> > > }
> > > if (irq < 0)
> > > irq = 0;
> > > @@ -1317,6 +1323,7 @@ i2c_new_device(struct i2c_adapter *adap, struct i2c_board_info const *info)
> > > client->flags = info->flags;
> > > client->addr = info->addr;
> > > client->irq = info->irq;
> > > + client->irq_source = I2C_IRQ_SOURCE_PLATFORM;
> > >
> > > strlcpy(client->name, info->type, sizeof(client->name));
> > >
> > > diff --git a/include/linux/i2c.h b/include/linux/i2c.h
> > > index b2109c5..7d0368d 100644
> > > --- a/include/linux/i2c.h
> > > +++ b/include/linux/i2c.h
> > > @@ -213,6 +213,13 @@ struct i2c_driver {
> > > };
> > > #define to_i2c_driver(d) container_of(d, struct i2c_driver, driver)
> > >
> > > +enum i2c_irq_source {
> > > + I2C_IRQ_SOURCE_PLATFORM,
> > > + I2C_IRQ_SOURCE_OF,
> > > + I2C_IRQ_SOURCE_ACPI,
> > > + I2C_IRQ_SOURCE_HOST_NOTIFY,
> > > +};
> > > +
> > > /**
> > > * struct i2c_client - represent an I2C slave device
> > > * @flags: I2C_CLIENT_TEN indicates the device uses a ten bit chip address;
> > > @@ -227,6 +234,9 @@ struct i2c_driver {
> > > * userspace_devices list
> > > * @slave_cb: Callback when I2C slave mode of an adapter is used. The adapter
> > > * calls it to pass on slave events to the slave driver.
> > > + * @irq_source: Enum which provides the source of the IRQ. Useful to know
> > > + * if the IRQ was issued from Host Notify or if it was provided by an other
> > > + * component.
> >
> > I'd think some documentation somewhere makes sense why we need to
> > distinguish this in some cases?
>
> I'd rather drivers be oblivious of the source of interrupt. If they need
> to distinguish between them that means that our IRQ abstration failed.
>
> >
> > > *
> > > * An i2c_client identifies a single device (i.e. chip) connected to an
> > > * i2c bus. The behaviour exposed to Linux is defined by the driver
> > > @@ -245,6 +255,7 @@ struct i2c_client {
> > > #if IS_ENABLED(CONFIG_I2C_SLAVE)
> > > i2c_slave_cb_t slave_cb; /* callback for slave mode */
> > > #endif
> > > + enum i2c_irq_source irq_source; /* which component assigned the irq */
> > > };
> > > #define to_i2c_client(d) container_of(d, struct i2c_client, dev)
> > >
> > > Dmitry, Wolfram, Jean, would this be acceptable for you?
> >
> > Adding something to i2c_driver is not exactly cheap, but from what I
> > glimpsed from this thread, this is one of the cleanest solution to this
> > problem?
> >
>
> As Benjamin said, it is really property of device [instance], not
> driver. I.e. driver could handle both wired IRQ and HostNotify-based
> scheme similarly, it is device (and board) that knows how stuff is
> connected.
>
> Maybe we could do something like this (untested):
>
>
> From e362a0277fd1bd6112f258664d8831d9bc6b78da Mon Sep 17 00:00:00 2001
> From: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> Date: Wed, 4 Jan 2017 09:33:43 -0800
> Subject: [PATCH] i2c: do not enable fall back to Host Notify by default
>
> Falling back unconditionally to HostNotify as primary client's interrupt
> breaks some drivers which alter their functionality depending on whether
> interrupt is present or not, so let's introduce a board flag telling I2C
> core explicitly if we want wired interrupt or HostNotify-based one:
> I2C_CLIENT_HOST_NOTIFY.
>
> For DT-based systems we introduce "host-notofy" property that we convert
> to I2C_CLIENT_HOST_NOTIFY board flag.
>
> Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> ---
> Documentation/devicetree/bindings/i2c/i2c.txt | 8 ++++++++
> drivers/i2c/i2c-core.c | 17 ++++++++---------
> include/linux/i2c.h | 1 +
> 3 files changed, 17 insertions(+), 9 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/i2c/i2c.txt b/Documentation/devicetree/bindings/i2c/i2c.txt
> index 5fa691e6f638..cee9d5055fa2 100644
> --- a/Documentation/devicetree/bindings/i2c/i2c.txt
> +++ b/Documentation/devicetree/bindings/i2c/i2c.txt
> @@ -62,6 +62,9 @@ wants to support one of the below features, it should adapt the bindings below.
> "irq" and "wakeup" names are recognized by I2C core, other names are
> left to individual drivers.
>
> +- host-notify
> + device uses SMBus host notify protocol instead of interrupt line.
> +
> - multi-master
> states that there is another master active on this bus. The OS can use
> this information to adapt power management to keep the arbitration awake
> @@ -81,6 +84,11 @@ Binding may contain optional "interrupts" property, describing interrupts
> used by the device. I2C core will assign "irq" interrupt (or the very first
> interrupt if not using interrupt names) as primary interrupt for the slave.
>
> +Alternatively, devices supporting SMbus Host Notify, and connected to
> +adapters that support this feature, may use "host-notify" property. I2C
> +core will create a virtual interrupt for Host Notify and assign it as
> +primary interrupt for the slave.
> +
> Also, if device is marked as a wakeup source, I2C core will set up "wakeup"
> interrupt for the device. If "wakeup" interrupt name is not present in the
> binding, then primary interrupt will be used as wakeup interrupt.
> diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c
> index cf9e396d7702..250969fa7670 100644
> --- a/drivers/i2c/i2c-core.c
> +++ b/drivers/i2c/i2c-core.c
> @@ -931,7 +931,10 @@ static int i2c_device_probe(struct device *dev)
> if (!client->irq) {
> int irq = -ENOENT;
>
> - if (dev->of_node) {
> + if (client->flags & I2C_CLIENT_HOST_HOTIFY) {
> + dev_dbg(dev, "Using Host Notify IRQ\n");
> + irq = i2c_smbus_host_notify_to_irq(client);
> + } else if (dev->of_node) {
> irq = of_irq_get_byname(dev->of_node, "irq");
> if (irq == -EINVAL || irq == -ENODATA)
> irq = of_irq_get(dev->of_node, 0);
> @@ -940,14 +943,7 @@ static int i2c_device_probe(struct device *dev)
> }
> if (irq == -EPROBE_DEFER)
> return irq;
> - /*
> - * ACPI and OF did not find any useful IRQ, try to see
> - * if Host Notify can be used.
> - */
> - if (irq < 0) {
> - dev_dbg(dev, "Using Host Notify IRQ\n");
> - irq = i2c_smbus_host_notify_to_irq(client);
> - }
> +
> if (irq < 0)
> irq = 0;
>
> @@ -1716,6 +1712,9 @@ static struct i2c_client *of_i2c_register_device(struct i2c_adapter *adap,
> info.of_node = of_node_get(node);
> info.archdata = &dev_ad;
>
> + if (of_read_property_bool(node, "host-notify"))
> + info.flags |= I2C_CLIENT_HOST_NOTIFY;
> +
> if (of_get_property(node, "wakeup-source", NULL))
> info.flags |= I2C_CLIENT_WAKE;
>
> diff --git a/include/linux/i2c.h b/include/linux/i2c.h
> index b2109c522dec..4b45ec46161f 100644
> --- a/include/linux/i2c.h
> +++ b/include/linux/i2c.h
> @@ -665,6 +665,7 @@ i2c_unlock_adapter(struct i2c_adapter *adapter)
> #define I2C_CLIENT_TEN 0x10 /* we have a ten bit chip address */
> /* Must equal I2C_M_TEN below */
> #define I2C_CLIENT_SLAVE 0x20 /* we are the slave */
> +#define I2C_CLIENT_HOST_NOTIFY 0x40 /* We want to use I2C host notify */
> #define I2C_CLIENT_WAKE 0x80 /* for board_info; true iff can wake */
> #define I2C_CLIENT_SCCB 0x9000 /* Use Omnivision SCCB protocol */
> /* Must match I2C_M_STOP|IGNORE_NAK */
> --
> 2.11.0.390.gc69c2f50cf-goog
>
>
Looks good, this seems to be elegant solution to our problem.
But then it is needed to patch those touchpad drivers to add that
I2C_CLIENT_HOST_NOTIFY flag, right?
--
Pali Rohár
pali.rohar@gmail.com
[toc] | [prev] | [next] | [standalone]
| From | Benjamin Tissoires <benjamin.tissoires@redhat.com> |
|---|---|
| Date | 2017-01-05 10:30 +0100 |
| Message-ID | <sWcwW-Vl-33@gated-at.bofh.it> |
| In reply to | #1551765 |
On Jan 05 2017 or thereabouts, Pali Rohár wrote:
> On Wednesday 04 January 2017 09:54:56 Dmitry Torokhov wrote:
> > On Wed, Jan 04, 2017 at 06:46:19PM +0100, Wolfram Sang wrote:
> > >
> > > > How about:
> > > > ---
> > > > From daa7571bbf337704332c0cfeec9b8fd5aeae596f Mon Sep 17 00:00:00 2001
> > > > From: Benjamin Tissoires <benjamin.tissoires@redhat.com>
> > > > Date: Wed, 4 Jan 2017 18:26:54 +0100
> > > > Subject: [PATCH] I2C: add the source of the IRQ in struct i2c_client
> > > >
> > > > With commit 4d5538f5882a ("i2c: use an IRQ to report Host Notify events,
> > > > not alert"), the IRQ provided in struct i2c_client might be assigned while
> > > > it has not been explicitly declared by either the platform information
> > > > or OF or ACPI.
> > > > Some drivers (lis3lv02d) rely on the fact that the IRQ gets assigned or
> > > > not to trigger a different behavior (exposing /dev/freefall in this case).
> > > >
> > > > Provide a way for others to know who set the IRQ and so they can behave
> > > > accordingly.
> > > >
> > > > Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
> > > > ---
> > > > drivers/i2c/i2c-core.c | 7 +++++++
> > > > include/linux/i2c.h | 11 +++++++++++
> > > > 2 files changed, 18 insertions(+)
> > > >
> > > > diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c
> > > > index cf9e396..226c75d 100644
> > > > --- a/drivers/i2c/i2c-core.c
> > > > +++ b/drivers/i2c/i2c-core.c
> > > > @@ -935,8 +935,12 @@ static int i2c_device_probe(struct device *dev)
> > > > irq = of_irq_get_byname(dev->of_node, "irq");
> > > > if (irq == -EINVAL || irq == -ENODATA)
> > > > irq = of_irq_get(dev->of_node, 0);
> > > > + if (irq > 0)
> > > > + client->irq_source = I2C_IRQ_SOURCE_OF;
> > > > } else if (ACPI_COMPANION(dev)) {
> > > > irq = acpi_dev_gpio_irq_get(ACPI_COMPANION(dev), 0);
> > > > + if (irq > 0)
> > > > + client->irq_source = I2C_IRQ_SOURCE_ACPI;
> > > > }
> > > > if (irq == -EPROBE_DEFER)
> > > > return irq;
> > > > @@ -947,6 +951,8 @@ static int i2c_device_probe(struct device *dev)
> > > > if (irq < 0) {
> > > > dev_dbg(dev, "Using Host Notify IRQ\n");
> > > > irq = i2c_smbus_host_notify_to_irq(client);
> > > > + if (irq > 0)
> > > > + client->irq_source = I2C_IRQ_SOURCE_HOST_NOTIFY;
> > > > }
> > > > if (irq < 0)
> > > > irq = 0;
> > > > @@ -1317,6 +1323,7 @@ i2c_new_device(struct i2c_adapter *adap, struct i2c_board_info const *info)
> > > > client->flags = info->flags;
> > > > client->addr = info->addr;
> > > > client->irq = info->irq;
> > > > + client->irq_source = I2C_IRQ_SOURCE_PLATFORM;
> > > >
> > > > strlcpy(client->name, info->type, sizeof(client->name));
> > > >
> > > > diff --git a/include/linux/i2c.h b/include/linux/i2c.h
> > > > index b2109c5..7d0368d 100644
> > > > --- a/include/linux/i2c.h
> > > > +++ b/include/linux/i2c.h
> > > > @@ -213,6 +213,13 @@ struct i2c_driver {
> > > > };
> > > > #define to_i2c_driver(d) container_of(d, struct i2c_driver, driver)
> > > >
> > > > +enum i2c_irq_source {
> > > > + I2C_IRQ_SOURCE_PLATFORM,
> > > > + I2C_IRQ_SOURCE_OF,
> > > > + I2C_IRQ_SOURCE_ACPI,
> > > > + I2C_IRQ_SOURCE_HOST_NOTIFY,
> > > > +};
> > > > +
> > > > /**
> > > > * struct i2c_client - represent an I2C slave device
> > > > * @flags: I2C_CLIENT_TEN indicates the device uses a ten bit chip address;
> > > > @@ -227,6 +234,9 @@ struct i2c_driver {
> > > > * userspace_devices list
> > > > * @slave_cb: Callback when I2C slave mode of an adapter is used. The adapter
> > > > * calls it to pass on slave events to the slave driver.
> > > > + * @irq_source: Enum which provides the source of the IRQ. Useful to know
> > > > + * if the IRQ was issued from Host Notify or if it was provided by an other
> > > > + * component.
> > >
> > > I'd think some documentation somewhere makes sense why we need to
> > > distinguish this in some cases?
> >
> > I'd rather drivers be oblivious of the source of interrupt. If they need
> > to distinguish between them that means that our IRQ abstration failed.
> >
> > >
> > > > *
> > > > * An i2c_client identifies a single device (i.e. chip) connected to an
> > > > * i2c bus. The behaviour exposed to Linux is defined by the driver
> > > > @@ -245,6 +255,7 @@ struct i2c_client {
> > > > #if IS_ENABLED(CONFIG_I2C_SLAVE)
> > > > i2c_slave_cb_t slave_cb; /* callback for slave mode */
> > > > #endif
> > > > + enum i2c_irq_source irq_source; /* which component assigned the irq */
> > > > };
> > > > #define to_i2c_client(d) container_of(d, struct i2c_client, dev)
> > > >
> > > > Dmitry, Wolfram, Jean, would this be acceptable for you?
> > >
> > > Adding something to i2c_driver is not exactly cheap, but from what I
> > > glimpsed from this thread, this is one of the cleanest solution to this
> > > problem?
> > >
> >
> > As Benjamin said, it is really property of device [instance], not
> > driver. I.e. driver could handle both wired IRQ and HostNotify-based
> > scheme similarly, it is device (and board) that knows how stuff is
> > connected.
> >
> > Maybe we could do something like this (untested):
> >
> >
> > From e362a0277fd1bd6112f258664d8831d9bc6b78da Mon Sep 17 00:00:00 2001
> > From: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> > Date: Wed, 4 Jan 2017 09:33:43 -0800
> > Subject: [PATCH] i2c: do not enable fall back to Host Notify by default
> >
> > Falling back unconditionally to HostNotify as primary client's interrupt
> > breaks some drivers which alter their functionality depending on whether
> > interrupt is present or not, so let's introduce a board flag telling I2C
> > core explicitly if we want wired interrupt or HostNotify-based one:
> > I2C_CLIENT_HOST_NOTIFY.
> >
> > For DT-based systems we introduce "host-notofy" property that we convert
> > to I2C_CLIENT_HOST_NOTIFY board flag.
> >
> > Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> > ---
> > Documentation/devicetree/bindings/i2c/i2c.txt | 8 ++++++++
> > drivers/i2c/i2c-core.c | 17 ++++++++---------
> > include/linux/i2c.h | 1 +
> > 3 files changed, 17 insertions(+), 9 deletions(-)
> >
> > diff --git a/Documentation/devicetree/bindings/i2c/i2c.txt b/Documentation/devicetree/bindings/i2c/i2c.txt
> > index 5fa691e6f638..cee9d5055fa2 100644
> > --- a/Documentation/devicetree/bindings/i2c/i2c.txt
> > +++ b/Documentation/devicetree/bindings/i2c/i2c.txt
> > @@ -62,6 +62,9 @@ wants to support one of the below features, it should adapt the bindings below.
> > "irq" and "wakeup" names are recognized by I2C core, other names are
> > left to individual drivers.
> >
> > +- host-notify
> > + device uses SMBus host notify protocol instead of interrupt line.
> > +
> > - multi-master
> > states that there is another master active on this bus. The OS can use
> > this information to adapt power management to keep the arbitration awake
> > @@ -81,6 +84,11 @@ Binding may contain optional "interrupts" property, describing interrupts
> > used by the device. I2C core will assign "irq" interrupt (or the very first
> > interrupt if not using interrupt names) as primary interrupt for the slave.
> >
> > +Alternatively, devices supporting SMbus Host Notify, and connected to
> > +adapters that support this feature, may use "host-notify" property. I2C
> > +core will create a virtual interrupt for Host Notify and assign it as
> > +primary interrupt for the slave.
> > +
> > Also, if device is marked as a wakeup source, I2C core will set up "wakeup"
> > interrupt for the device. If "wakeup" interrupt name is not present in the
> > binding, then primary interrupt will be used as wakeup interrupt.
> > diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c
> > index cf9e396d7702..250969fa7670 100644
> > --- a/drivers/i2c/i2c-core.c
> > +++ b/drivers/i2c/i2c-core.c
> > @@ -931,7 +931,10 @@ static int i2c_device_probe(struct device *dev)
> > if (!client->irq) {
> > int irq = -ENOENT;
> >
> > - if (dev->of_node) {
> > + if (client->flags & I2C_CLIENT_HOST_HOTIFY) {
> > + dev_dbg(dev, "Using Host Notify IRQ\n");
> > + irq = i2c_smbus_host_notify_to_irq(client);
> > + } else if (dev->of_node) {
> > irq = of_irq_get_byname(dev->of_node, "irq");
> > if (irq == -EINVAL || irq == -ENODATA)
> > irq = of_irq_get(dev->of_node, 0);
> > @@ -940,14 +943,7 @@ static int i2c_device_probe(struct device *dev)
> > }
> > if (irq == -EPROBE_DEFER)
> > return irq;
> > - /*
> > - * ACPI and OF did not find any useful IRQ, try to see
> > - * if Host Notify can be used.
> > - */
> > - if (irq < 0) {
> > - dev_dbg(dev, "Using Host Notify IRQ\n");
> > - irq = i2c_smbus_host_notify_to_irq(client);
> > - }
> > +
> > if (irq < 0)
> > irq = 0;
> >
> > @@ -1716,6 +1712,9 @@ static struct i2c_client *of_i2c_register_device(struct i2c_adapter *adap,
> > info.of_node = of_node_get(node);
> > info.archdata = &dev_ad;
> >
> > + if (of_read_property_bool(node, "host-notify"))
> > + info.flags |= I2C_CLIENT_HOST_NOTIFY;
> > +
> > if (of_get_property(node, "wakeup-source", NULL))
> > info.flags |= I2C_CLIENT_WAKE;
> >
> > diff --git a/include/linux/i2c.h b/include/linux/i2c.h
> > index b2109c522dec..4b45ec46161f 100644
> > --- a/include/linux/i2c.h
> > +++ b/include/linux/i2c.h
> > @@ -665,6 +665,7 @@ i2c_unlock_adapter(struct i2c_adapter *adapter)
> > #define I2C_CLIENT_TEN 0x10 /* we have a ten bit chip address */
> > /* Must equal I2C_M_TEN below */
> > #define I2C_CLIENT_SLAVE 0x20 /* we are the slave */
> > +#define I2C_CLIENT_HOST_NOTIFY 0x40 /* We want to use I2C host notify */
> > #define I2C_CLIENT_WAKE 0x80 /* for board_info; true iff can wake */
> > #define I2C_CLIENT_SCCB 0x9000 /* Use Omnivision SCCB protocol */
> > /* Must match I2C_M_STOP|IGNORE_NAK */
> > --
> > 2.11.0.390.gc69c2f50cf-goog
> >
> >
>
> Looks good, this seems to be elegant solution to our problem.
>
> But then it is needed to patch those touchpad drivers to add that
> I2C_CLIENT_HOST_NOTIFY flag, right?
>
Yes, but currently the 2 drivers that are using Host Notify upstream are
rmi_smbus and elan_i2c. Both don't have an automatic binding (yet), so
there is nothing to worry about for now.
Cheers,
Benjamin
[toc] | [prev] | [next] | [standalone]
| From | Jean Delvare <jdelvare@suse.de> |
|---|---|
| Date | 2017-01-04 20:10 +0100 |
| Message-ID | <sVZ6F-lZ-3@gated-at.bofh.it> |
| In reply to | #1550193 |
On Tue, 3 Jan 2017 12:59:37 -0800, Dmitry Torokhov wrote: > On Tue, Jan 03, 2017 at 09:39:13PM +0100, Pali Rohár wrote: > > Some distributions blacklist i2c-i801.ko module... And > > Any particular reason for that? At some point in time, the i2c-i801 driver caused problems on a few systems. They decided that blacklisting the driver for everybody was the easiest way to fix the problem. Of course this doesn't make any sense and should be reverted. The root cause of the problem should have been investigated back then. Maybe it's fixed by now and they will never now... -- Jean Delvare SUSE L3 Support
[toc] | [prev] | [next] | [standalone]
| From | Andy Shevchenko <andy.shevchenko@gmail.com> |
|---|---|
| Date | 2017-01-04 11:00 +0100 |
| Subject | Re: [PATCH] i2c: i801: Register optional lis3lv02d i2c device on Dell machines |
| Message-ID | <sVQwp-2U6-17@gated-at.bofh.it> |
| In reply to | #1550169 |
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
[toc] | [prev] | [next] | [standalone]
| From | Benjamin Tissoires <benjamin.tissoires@redhat.com> |
|---|---|
| Date | 2017-01-04 11:30 +0100 |
| Message-ID | <sVQZr-3pc-19@gated-at.bofh.it> |
| In reply to | #1550597 |
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
[toc] | [prev] | [next] | [standalone]
| From | Pali Rohár <pali.rohar@gmail.com> |
|---|---|
| Date | 2017-01-04 12:40 +0100 |
| Message-ID | <sVS5b-44p-29@gated-at.bofh.it> |
| In reply to | #1550623 |
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
[toc] | [prev] | [next] | [standalone]
| From | Benjamin Tissoires <benjamin.tissoires@redhat.com> |
|---|---|
| Date | 2017-01-04 13:40 +0100 |
| Message-ID | <sVT1f-4FT-21@gated-at.bofh.it> |
| In reply to | #1550683 |
On Jan 04 2017 or thereabouts, Pali Rohár wrote: > 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. Sorry but I can't find your notes about errors in your previous emails. This solution indeed is a little bit more complex than just adding the bits in i2c-i801, but I don't really see the reason for an *adapter* driver to instantiate specific *clients* depending on some DMI matching. It's a platform issue, so it should be solved at a platform level, not at the i2c adapter level. Plus the bus notifier solutions guarantees plain separation between the elements and prevents any conflicts, with or without compile guards. > > My current path (after fixing IRQ to -1) is smaller more intuitive and This will only fix the fact that you have 2 concurrent drivers on the same resource (freefall), not the fact that the global design of having 2 drivers which do not cooperate is wrong. > do not introduce new complicated parts like bus notifier and new "fake" > i2c driver... It's not a "fake i2c driver". It's a platform driver. If you don't like the idea of the platform driver, just add the bus notifier code in dell-smo8800, but this will pull a new dependency on I2C in this ACPI 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
[toc] | [prev] | [next] | [standalone]
| From | Benjamin Tissoires <benjamin.tissoires@redhat.com> |
|---|---|
| Date | 2017-01-03 22:40 +0100 |
| Message-ID | <sVEYi-3J6-35@gated-at.bofh.it> |
| In reply to | #1550130 |
On Jan 03 2017 or thereabouts, 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.
>
> Alternatively, can lis3lv02d be tasked to create /dev/freefall?
>
> 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).
> Benjamin, is there anything "special" in RMI SMbus ACPI descriptors we
> could use?
>
No, there is nothing special. Same situation for Elan with their latest
touchpads over PS/2. There is just a knowledge from the driver that
there is a device connected on a Host Notify capable bus on a specific
address. To give you an idea, on Windows, the Synaptics (and Elan)
driver even ships the equivalent of i2c-i801 to be sure to have one
driver for it...
So the knowledge is all in the driver.
Cheers,
Benjamin
[toc] | [prev] | [next] | [standalone]
| From | Dmitry Torokhov <dmitry.torokhov@gmail.com> |
|---|---|
| Date | 2017-01-04 07:40 +0100 |
| Message-ID | <sVNoR-RY-19@gated-at.bofh.it> |
| In reply to | #1550221 |
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? Thanks. -- Dmitry
[toc] | [prev] | [next] | [standalone]
| From | Pali Rohár <pali.rohar@gmail.com> |
|---|---|
| Date | 2017-01-04 10:30 +0100 |
| Message-ID | <sVQ3n-2IT-19@gated-at.bofh.it> |
| In reply to | #1550460 |
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
[toc] | [prev] | [next] | [standalone]
| From | Benjamin Tissoires <benjamin.tissoires@redhat.com> |
|---|---|
| Date | 2017-01-04 10:30 +0100 |
| Message-ID | <sVQ3n-2IT-21@gated-at.bofh.it> |
| In reply to | #1550460 |
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
[toc] | [prev] | [next] | [standalone]
| From | Andy Shevchenko <andy.shevchenko@gmail.com> |
|---|---|
| Date | 2017-01-03 21:30 +0100 |
| Subject | Re: [PATCH] i2c: i801: Register optional lis3lv02d i2c device on Dell machines |
| Message-ID | <sVDSx-34i-1@gated-at.bofh.it> |
| In reply to | #1550092 |
On Tue, Jan 3, 2017 at 8:50 PM, Pali Rohár <pali.rohar@gmail.com> wrote: > On Tuesday 03 January 2017 19:38:43 Dmitry Torokhov wrote: >> "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. And as I wrote there is still a way to provide it without hardcoding on model basis. > Really there is no other way... :-( -- With Best Regards, Andy Shevchenko
[toc] | [prev] | [standalone]
Page 2 of 2 — ← Prev page 1 [2]
Back to top | Article view | linux.kernel
csiph-web