* Re: [PATCH] i2c: i801: Register optional lis3lv02d i2c device on Dell machines
From: Wolfram Sang @ 2017-01-04 17:46 UTC (permalink / raw)
To: Benjamin Tissoires
Cc: Pali Rohár, Dmitry Torokhov, Michał Kępień,
Jean Delvare, 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: <20170104173928.GW5767@mail.corp.redhat.com>
> 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?
> *
> * 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?
^ permalink raw reply
* Re: [PATCH] i2c: i801: Register optional lis3lv02d i2c device on Dell machines
From: Benjamin Tissoires @ 2017-01-04 17:39 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: <20170104160643.GH3499@pali>
On Jan 04 2017 or thereabouts, Pali Rohár wrote:
> On Wednesday 04 January 2017 14:02:52 Benjamin Tissoires wrote:
> > > > > 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.
> >
> > On the principle, I agree. But let's be realistic: this device will be
> > created but no events will be generated from it.
>
> And I think this is a problem. This provides false information to kernel
> and also to userspace applications that there is working free fall
> sensor (even there is not).
>
> > It's a capability for the chip to provide freefall,
>
> That it is not truth. Not every chip has it. Platform data are there to
> provides this information for driver, if HW has freefall capability or
> not.
>
> > so I don't see why it's such an issue to
> > have one created which says nothing. (talking about non Dell platforms
> > here too)
> >
> > > >
> > > > > 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.
> >
> > I really don't follow you here. On a bus with no Host Notify, the IRQ
> > will be 0 and it will work in the same way.
>
> That it OK.
>
> > On a bus with Host Notify,
> > the IRQ should be set to something negative in the i2c_board_info
>
> Not only in i2c_board_info but also in DTS. Which means every one DTS
> needs to be updates -- also those outside of linux kernel. Which is for
> me wrong.
>
> > to be
> > sure to not have an Host Notify irq assigned. In both case the
> > lis3lv02d_i2c driver will just do:
> > if (client->irq > 0)
> > lis3_dev.irq = client->irq;
> >
> > So there is no bus adapter specifics in the driver.
> >
> > >
> > > > This is basically incorrect if we use one property for two different
> > > > things (host notify and hdd free fall).
> >
> > Host Notify is a way for a I2C device to report an interrupt without
> > having a dedicated line assigned (and so an IRQ).
>
> Yes. But lis3lv02d i2c devices which I have uses dedicated IRQ. They
> does not work in way like you describe.
>
> > So the chip could use
> > Host Notify to report hdd free fall if it supports the feature.
>
> Old existing HW could not. As those were not designed for such purpose.
>
> Yes, new HW can be designed in this way, but linux kernel should not
> drop (or modify) correct support for old HW devices just because there
> is new HW design.
I never said we should drop support of old hardware. I was just
explaining what is Host Notify and saying that it is not incompatible
with lis3lv02d.
>
> > > >
> > > > 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.
> >
> > I won't say the design is wrong because Host Notify really is just an
> > IRQ.
>
> Yes it is IRQ, but different. And lis3lv02d does not issue host notify
> IRQ, so it should not be assigned to lis3lv02d.
Sigh.
>
> > Where the issue lies now is that lis3lv02d makes the assumption
> > that an irq attached to an i2c_client means that it will report
> > freefall.
>
> Yes.
>
> > It was correct before, but now it's not true anymore.
>
> Agree.
>
> > So maybe this series should also add a way to provide the source of
> > the IRQ (Host Notify or ACPI or DT). Then, lis3lv02d could ignore or
> > not the incoming IRQ.
>
> Yes, this is an option how to fix this problem.
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.
*
* 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)
--
2.9.3
---
Dmitry, Wolfram, Jean, would this be acceptable for you?
>
> I would rather introduce new IRQ property in both DTS and lis3lv02d
> platform data (e.g. irq-freefall) which will unambiguously identify
> freefall IRQ.
Sigh. You asked above to not break existing DT bindings, so I don't
understand why you suddenly want to change every bindings by introducing
a change which breaks backward compatibility.
>
> But I do not think it should be part of my Dell patch. It is
> independent fix for lis3lv02d driver for bug which was introduced by
> commit 4d5538f5882a.
It's not part of the dell patch, but it can be part of the series you
are about to send. Please remember that the current state is that there
is no breakage (only a spurious /dev/freefall which you are not happy
about). The only breakage is with your patch, so it doesn't hurt to have
both sent together so you can control all the requirements at once.
So please incorporate my patch in your series if Wolfram, Jean and Dmitry
agree, and also add the fix in lis3lv02d_i2c to make use of the IRQ source.
Cheers,
Benjamin
>
> >
> > Cheers,
> > Benjamin
> >
> > > >
> > > > > 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
>
> --
> Pali Rohár
> pali.rohar@gmail.com
^ permalink raw reply related
* Re: [PATCH] i2c: i801: Register optional lis3lv02d i2c device on Dell machines
From: Pali Rohár @ 2017-01-04 16:06 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: <20170104130252.GV5767@mail.corp.redhat.com>
On Wednesday 04 January 2017 14:02:52 Benjamin Tissoires wrote:
> On Jan 04 2017 or thereabouts, Pali Rohár wrote:
> > 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.
>
> On the principle, I agree. But let's be realistic: this device will be
> created but no events will be generated from it.
And I think this is a problem. This provides false information to kernel
and also to userspace applications that there is working free fall
sensor (even there is not).
> It's a capability for the chip to provide freefall,
That it is not truth. Not every chip has it. Platform data are there to
provides this information for driver, if HW has freefall capability or
not.
> so I don't see why it's such an issue to
> have one created which says nothing. (talking about non Dell platforms
> here too)
>
> > >
> > > > 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.
>
> I really don't follow you here. On a bus with no Host Notify, the IRQ
> will be 0 and it will work in the same way.
That it OK.
> On a bus with Host Notify,
> the IRQ should be set to something negative in the i2c_board_info
Not only in i2c_board_info but also in DTS. Which means every one DTS
needs to be updates -- also those outside of linux kernel. Which is for
me wrong.
> to be
> sure to not have an Host Notify irq assigned. In both case the
> lis3lv02d_i2c driver will just do:
> if (client->irq > 0)
> lis3_dev.irq = client->irq;
>
> So there is no bus adapter specifics in the driver.
>
> >
> > > This is basically incorrect if we use one property for two different
> > > things (host notify and hdd free fall).
>
> Host Notify is a way for a I2C device to report an interrupt without
> having a dedicated line assigned (and so an IRQ).
Yes. But lis3lv02d i2c devices which I have uses dedicated IRQ. They
does not work in way like you describe.
> So the chip could use
> Host Notify to report hdd free fall if it supports the feature.
Old existing HW could not. As those were not designed for such purpose.
Yes, new HW can be designed in this way, but linux kernel should not
drop (or modify) correct support for old HW devices just because there
is new HW design.
> > >
> > > 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.
>
> I won't say the design is wrong because Host Notify really is just an
> IRQ.
Yes it is IRQ, but different. And lis3lv02d does not issue host notify
IRQ, so it should not be assigned to lis3lv02d.
> Where the issue lies now is that lis3lv02d makes the assumption
> that an irq attached to an i2c_client means that it will report
> freefall.
Yes.
> It was correct before, but now it's not true anymore.
Agree.
> So maybe this series should also add a way to provide the source of
> the IRQ (Host Notify or ACPI or DT). Then, lis3lv02d could ignore or
> not the incoming IRQ.
Yes, this is an option how to fix this problem.
I would rather introduce new IRQ property in both DTS and lis3lv02d
platform data (e.g. irq-freefall) which will unambiguously identify
freefall IRQ.
But I do not think it should be part of my Dell patch. It is
independent fix for lis3lv02d driver for bug which was introduced by
commit 4d5538f5882a.
>
> Cheers,
> Benjamin
>
> > >
> > > > 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
--
Pali Rohár
pali.rohar@gmail.com
^ permalink raw reply
* Re: [PATCH v3] i2c: designware: add reset interface
From: Andy Shevchenko @ 2017-01-04 15:35 UTC (permalink / raw)
To: Arnd Bergmann, linux-arm-kernel
Cc: Zhangfei Gao, Wolfram Sang, mika.westerberg, jarkko.nikula,
p.zabel, linux-i2c
In-Reply-To: <2921529.fKUMSWjgsl@wuerfel>
On Wed, 2017-01-04 at 15:55 +0100, Arnd Bergmann wrote:
> On Friday, December 23, 2016 9:40:51 PM CET Zhangfei Gao wrote:
> > @@ -176,6 +177,14 @@ static int dw_i2c_plat_probe(struct
> > platform_device *pdev)
> > dev->irq = irq;
> > platform_set_drvdata(pdev, dev);
> >
> > + dev->rst = devm_reset_control_get_optional_exclusive(&pdev-
> > >dev, NULL);
> > + if (IS_ERR(dev->rst)) {
> > + if (PTR_ERR(dev->rst) == -EPROBE_DEFER)
> > + return -EPROBE_DEFER;
> > + } else {
> > + reset_control_deassert(dev->rst);
> > + }
> > +
>
> Sorry for the late reply, I only now stumbled over this. I think it's
> generally wrong to ignore any error aside from -EPROBE_DEFER. It's
> better to single-out the error conditions you want to ignore (e.g.
> no reset specified) and ignore those but return an error for
> all other problems.
Which means that reset framework whenever work _optional is used should
return error iff (mind two f:s) there is a problem with existing
control.
>
> > @@ -270,10 +280,18 @@ static int dw_i2c_plat_probe(struct
> > platform_device *pdev)
> > }
> >
> > r = i2c_dw_probe(dev);
> > - if (r && !dev->pm_runtime_disabled)
> > - pm_runtime_disable(&pdev->dev);
> > + if (r)
> > + goto exit_probe;
> >
> > return r;
> > +
> > +exit_probe:
> > + if (!dev->pm_runtime_disabled)
> > + pm_runtime_disable(&pdev->dev);
> > +exit_reset:
> > + if (!IS_ERR_OR_NULL(dev->rst))
> > + reset_control_assert(dev->rst);
> > + return r;
> >
>
> try to avoid the IS_ERR_OR_NULL() check, it usually indicates either
> a bad interface, or that the interface is used wrong.
Please, fix reset framework first than.
For my understanding:
It should return NULL for optional reset control.
It should not fail on NULL argument.
> In this case, I think we can't get here with a NULL dev->rst
> pointer, so it's better to only check IS_ERR, or to explicitly
> set the pointer to NULL in case there is no reset line.
>
> Arnd
--
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy
^ permalink raw reply
* Re: [PATCH v3] i2c: designware: add reset interface
From: Arnd Bergmann @ 2017-01-04 14:55 UTC (permalink / raw)
To: linux-arm-kernel
Cc: Zhangfei Gao, Wolfram Sang, andriy.shevchenko, mika.westerberg,
jarkko.nikula, p.zabel, linux-i2c
In-Reply-To: <1482500451-30137-1-git-send-email-zhangfei.gao@linaro.org>
On Friday, December 23, 2016 9:40:51 PM CET Zhangfei Gao wrote:
> @@ -176,6 +177,14 @@ static int dw_i2c_plat_probe(struct platform_device *pdev)
> dev->irq = irq;
> platform_set_drvdata(pdev, dev);
>
> + dev->rst = devm_reset_control_get_optional_exclusive(&pdev->dev, NULL);
> + if (IS_ERR(dev->rst)) {
> + if (PTR_ERR(dev->rst) == -EPROBE_DEFER)
> + return -EPROBE_DEFER;
> + } else {
> + reset_control_deassert(dev->rst);
> + }
> +
Sorry for the late reply, I only now stumbled over this. I think it's
generally wrong to ignore any error aside from -EPROBE_DEFER. It's
better to single-out the error conditions you want to ignore (e.g.
no reset specified) and ignore those but return an error for
all other problems.
> @@ -270,10 +280,18 @@ static int dw_i2c_plat_probe(struct platform_device *pdev)
> }
>
> r = i2c_dw_probe(dev);
> - if (r && !dev->pm_runtime_disabled)
> - pm_runtime_disable(&pdev->dev);
> + if (r)
> + goto exit_probe;
>
> return r;
> +
> +exit_probe:
> + if (!dev->pm_runtime_disabled)
> + pm_runtime_disable(&pdev->dev);
> +exit_reset:
> + if (!IS_ERR_OR_NULL(dev->rst))
> + reset_control_assert(dev->rst);
> + return r;
>
try to avoid the IS_ERR_OR_NULL() check, it usually indicates either
a bad interface, or that the interface is used wrong.
In this case, I think we can't get here with a NULL dev->rst
pointer, so it's better to only check IS_ERR, or to explicitly
set the pointer to NULL in case there is no reset line.
Arnd
^ permalink raw reply
* Re: [PATCH 2/5] dt: bindings: i2c-mux-pca954x: Add documentation for interrupt controller
From: Rob Herring @ 2017-01-04 14:45 UTC (permalink / raw)
To: Phil Reid; +Cc: peda, wsa, mark.rutland, linux-i2c, devicetree
In-Reply-To: <1483522197-38819-3-git-send-email-preid@electromag.com.au>
On Wed, Jan 04, 2017 at 05:29:54PM +0800, Phil Reid wrote:
> Various muxes can aggregate multiple irq lines and provide a control
> register to determine the active line. Add bindings for interrupt
> controller support.
>
> Signed-off-by: Phil Reid <preid@electromag.com.au>
> ---
> Documentation/devicetree/bindings/i2c/i2c-mux-pca954x.txt | 14 +++++++++++++-
> 1 file changed, 13 insertions(+), 1 deletion(-)
>
> diff --git a/Documentation/devicetree/bindings/i2c/i2c-mux-pca954x.txt b/Documentation/devicetree/bindings/i2c/i2c-mux-pca954x.txt
> index cf53d5f..9f7c275 100644
> --- a/Documentation/devicetree/bindings/i2c/i2c-mux-pca954x.txt
> +++ b/Documentation/devicetree/bindings/i2c/i2c-mux-pca954x.txt
> @@ -19,7 +19,14 @@ Optional Properties:
> - i2c-mux-idle-disconnect: Boolean; if defined, forces mux to disconnect all
> children in idle state. This is necessary for example, if there are several
> multiplexers on the bus and the devices behind them use same I2C addresses.
> -
> + - interrupt-parent: Phandle for the interrupt controller that services
> + interrupts for this device.
> + - interrupts: Interrupt mapping for IRQ.
> + - interrupt-controller: Marks the device node as a interrupt controller.
> + - #interrupt-cells : Should be two.
> + - first cell is the pin number
> + - second cell is used to specify flags.
> + See also Documentation/devicetree/bindings/interrupt-controller/interrupts.txt
>
> Example:
>
> @@ -29,6 +36,11 @@ Example:
> #size-cells = <0>;
> reg = <0x74>;
>
> + interrupt-parent = <&ipic>;
> + interrupts = <17 IRQ_TYPE_LEVEL_LOW>;
> + interrupt-controller;
> + #interrupt-cells=<2>;
Needs spaces around the '='. With that,
Acked-by: Rob Herring <robh@kernel.org>
> +
> i2c@2 {
> #address-cells = <1>;
> #size-cells = <0>;
> --
> 1.8.3.1
>
^ permalink raw reply
* Re: [PATCH 2/5] dt: bindings: i2c-mux-pca954x: Add documentation for interrupt controller
From: Peter Rosin @ 2017-01-04 14:11 UTC (permalink / raw)
To: Phil Reid, wsa, robh+dt, mark.rutland, linux-i2c, devicetree
In-Reply-To: <1483522197-38819-3-git-send-email-preid@electromag.com.au>
On 2017-01-04 10:29, Phil Reid wrote:
> Various muxes can aggregate multiple irq lines and provide a control
> register to determine the active line. Add bindings for interrupt
> controller support.
>
I'm no irq expert, but looks good (superficially).
With the below nitpick,
Acked-by: Peter Rosin <peda@axentia.se>
Cheers,
peda
> Signed-off-by: Phil Reid <preid@electromag.com.au>
> ---
> Documentation/devicetree/bindings/i2c/i2c-mux-pca954x.txt | 14 +++++++++++++-
> 1 file changed, 13 insertions(+), 1 deletion(-)
>
> diff --git a/Documentation/devicetree/bindings/i2c/i2c-mux-pca954x.txt b/Documentation/devicetree/bindings/i2c/i2c-mux-pca954x.txt
> index cf53d5f..9f7c275 100644
> --- a/Documentation/devicetree/bindings/i2c/i2c-mux-pca954x.txt
> +++ b/Documentation/devicetree/bindings/i2c/i2c-mux-pca954x.txt
> @@ -19,7 +19,14 @@ Optional Properties:
> - i2c-mux-idle-disconnect: Boolean; if defined, forces mux to disconnect all
> children in idle state. This is necessary for example, if there are several
> multiplexers on the bus and the devices behind them use same I2C addresses.
> -
> + - interrupt-parent: Phandle for the interrupt controller that services
> + interrupts for this device.
> + - interrupts: Interrupt mapping for IRQ.
> + - interrupt-controller: Marks the device node as a interrupt controller.
as an interrupt
> + - #interrupt-cells : Should be two.
> + - first cell is the pin number
> + - second cell is used to specify flags.
> + See also Documentation/devicetree/bindings/interrupt-controller/interrupts.txt
>
> Example:
>
> @@ -29,6 +36,11 @@ Example:
> #size-cells = <0>;
> reg = <0x74>;
>
> + interrupt-parent = <&ipic>;
> + interrupts = <17 IRQ_TYPE_LEVEL_LOW>;
> + interrupt-controller;
> + #interrupt-cells=<2>;
> +
> i2c@2 {
> #address-cells = <1>;
> #size-cells = <0>;
>
^ permalink raw reply
* Re: [PATCH 3/5] i2c: mux: pca954x: Add interrupt controller support
From: Peter Rosin @ 2017-01-04 14:26 UTC (permalink / raw)
To: Phil Reid, wsa-z923LK4zBo2bacvFa/9K2g,
robh+dt-DgEjT+Ai2ygdnm+yROfE0A, mark.rutland-5wv7dgnIgG8,
linux-i2c-u79uwXL29TY76Z2rM5mHXA,
devicetree-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <89efbe82-4530-043e-3469-ece0748d0150-koto5C5qi+TLoDKTGw+V6w@public.gmane.org>
On 2017-01-04 15:13, Peter Rosin wrote:
> On 2017-01-04 10:29, Phil Reid wrote:
>> Various muxes can aggregate multiple interrupts from each i2c bus.
>> All of the muxes with interrupt support combine the active low irq lines
>> using an internal 'and' function and generate a combined active low
>> output. The muxes do provide the ability to read a control register to
>> determine which irq is active. By making the mux an irq controller isr
>> can potentially be reduced by reading the status register and then only
>> calling the registered isr on that bus segment.
>>
>> As there is no irq masking on the mux irq are disabled until irq_unmask is
>> called at least once.
>
> Irqs are not my strong point, I would prefer if someone else also had
> a look. And there are some comments below...
>
> Cheers,
> peda
>
>> Signed-off-by: Phil Reid <preid-qgqNFa1JUf/o2iN0hyhwsIdd74u8MsAO@public.gmane.org>
>> ---
>> drivers/i2c/muxes/i2c-mux-pca954x.c | 126 ++++++++++++++++++++++++++++++++++++
>> 1 file changed, 126 insertions(+)
>>
>> diff --git a/drivers/i2c/muxes/i2c-mux-pca954x.c b/drivers/i2c/muxes/i2c-mux-pca954x.c
>> index 981d145..f2dba7c 100644
>> --- a/drivers/i2c/muxes/i2c-mux-pca954x.c
>> +++ b/drivers/i2c/muxes/i2c-mux-pca954x.c
>> @@ -40,14 +40,19 @@
>> #include <linux/i2c.h>
>> #include <linux/i2c-mux.h>
>> #include <linux/i2c/pca954x.h>
>> +#include <linux/interrupt.h>
>> +#include <linux/irq.h>
>> #include <linux/module.h>
>> #include <linux/of.h>
>> #include <linux/of_device.h>
>> +#include <linux/of_irq.h>
>> #include <linux/pm.h>
>> #include <linux/slab.h>
>>
>> #define PCA954X_MAX_NCHANS 8
>>
>> +#define PCA954X_IRQ_OFFSET 4
>> +
>> enum pca_type {
>> pca_9540,
>> pca_9542,
>> @@ -62,6 +67,7 @@ enum pca_type {
>> struct chip_desc {
>> u8 nchans;
>> u8 enable; /* used for muxes only */
>> + u8 has_irq;
>> enum muxtype {
>> pca954x_ismux = 0,
>> pca954x_isswi
>> @@ -74,6 +80,9 @@ struct pca954x {
>> u8 last_chan; /* last register value */
>> u8 deselect;
>> struct i2c_client *client;
>> +
>> + struct irq_domain *irq;
>> + unsigned int irq_mask;
>> };
>>
>> /* Provide specs for the PCA954x types we know about */
>> @@ -86,19 +95,23 @@ struct pca954x {
>> [pca_9542] = {
>> .nchans = 2,
>> .enable = 0x4,
>> + .has_irq = 1,
>> .muxtype = pca954x_ismux,
>> },
>> [pca_9543] = {
>> .nchans = 2,
>> + .has_irq = 1,
>> .muxtype = pca954x_isswi,
>> },
>> [pca_9544] = {
>> .nchans = 4,
>> .enable = 0x4,
>> + .has_irq = 1,
>> .muxtype = pca954x_ismux,
>> },
>> [pca_9545] = {
>> .nchans = 4,
>> + .has_irq = 1,
>> .muxtype = pca954x_isswi,
>> },
>> [pca_9547] = {
>> @@ -203,6 +216,104 @@ static int pca954x_deselect_mux(struct i2c_mux_core *muxc, u32 chan)
>> return pca954x_reg_write(muxc->parent, client, data->last_chan);
>> }
>>
>> +static irqreturn_t pca954x_irq_handler(int irq, void *dev_id)
>> +{
>> + struct pca954x *data = dev_id;
>> + unsigned int child_irq;
>> + int ret, i, handled;
>> +
>> + ret = i2c_smbus_read_byte(data->client);
>> + if (ret < 0)
>> + return IRQ_NONE;
>> +
>> + for (i = 0; i < data->chip->nchans; i++) {
>> + if (ret & BIT(PCA954X_IRQ_OFFSET+i)) {
>
> Spaces around the +
>
>> + child_irq = irq_linear_revmap(data->irq, i);
>> + handle_nested_irq(child_irq);
>> + handled++;
>> + }
>> + }
>> + return handled ? IRQ_HANDLED : IRQ_NONE;
>> +}
>> +
>> +static void pca954x_irq_mask(struct irq_data *idata)
>> +{
>> + struct pca954x *data = irq_data_get_irq_chip_data(idata);
>> + unsigned int pos = idata->hwirq;
>> +
>> + data->irq_mask &= ~BIT(pos);
>> + if (!data->irq_mask)
>> + disable_irq(data->client->irq);
>> +}
>> +
>> +static void pca954x_irq_unmask(struct irq_data *idata)
>> +{
>> + struct pca954x *data = irq_data_get_irq_chip_data(idata);
>> + unsigned int pos = idata->hwirq;
>> +
>> + if (!data->irq_mask)
>> + enable_irq(data->client->irq);
>> + data->irq_mask |= BIT(pos);
>> +}
>> +
>> +static int pca954x_irq_set_type(struct irq_data *idata, unsigned int type)
>> +{
>> + if ((type & IRQ_TYPE_SENSE_MASK) != IRQ_TYPE_LEVEL_LOW)
>> + return -EINVAL;
>> + return 0;
>> +}
>> +
>> +static struct irq_chip pca954x_irq_chip = {
>> + .name = "i2c-mux-pca954x",
>> + .irq_mask = pca954x_irq_mask,
>> + .irq_unmask = pca954x_irq_unmask,
>> + .irq_set_type = pca954x_irq_set_type,
>> +};
>> +
>> +static int pca953x_irq_setup(struct i2c_mux_core *muxc)
Ooop,
pca954x_irq_setup
>> +{
>> + struct pca954x *data = i2c_mux_priv(muxc);
>> + struct i2c_client *client = data->client;
>> + int c, err, irq;
>> +
>> + if (!data->chip->has_irq || client->irq <= 0)
>> + return 0;
>> +
>> + data->irq = irq_domain_add_linear(client->dev.of_node,
>> + data->chip->nchans,
>> + &irq_domain_simple_ops, data);
>> + if (!data->irq) {
>> + err = -ENODEV;
>> + goto err_irq_domain;
>
> This label is not needed, just return -ENODEV
>
>> + }
>> +
>> + for (c = 0; c < data->chip->nchans; c++) {
>> + irq = irq_create_mapping(data->irq, c);
>> + irq_set_chip_data(irq, data);
>> + irq_set_chip_and_handler(irq, &pca954x_irq_chip,
>> + handle_simple_irq);
>> + }
>> +
>> + err = devm_request_threaded_irq(&client->dev, data->client->irq, NULL,
>> + pca954x_irq_handler,
>> + IRQF_ONESHOT | IRQF_SHARED,
>> + "pca953x", data);
>
> "pca954x"
>
>> + if (err)
>> + goto err_req_irq;
>> +
>> + disable_irq(data->client->irq);
>> +
>> + return 0;
>> +err_req_irq:
>> + for (c = 0; c < data->chip->nchans; c++) {
>> + irq = irq_find_mapping(data->irq, c);
>> + irq_dispose_mapping(irq);
>> + }
>> + irq_domain_remove(data->irq);
>> +err_irq_domain:
>> + return err;
>> +}
>> +
>> /*
>> * I2C init/probing/exit functions
>> */
>> @@ -258,6 +369,10 @@ static int pca954x_probe(struct i2c_client *client,
>> idle_disconnect_dt = of_node &&
>> of_property_read_bool(of_node, "i2c-mux-idle-disconnect");
>>
>> + ret = pca953x_irq_setup(muxc);
>> + if (ret)
>> + goto irq_setup_failed;
>> +
>> /* Now create an adapter for each channel */
>> for (num = 0; num < data->chip->nchans; num++) {
>> bool idle_disconnect_pd = false;
>> @@ -294,6 +409,7 @@ static int pca954x_probe(struct i2c_client *client,
>>
>> return 0;
>>
>> +irq_setup_failed:
>> virt_reg_failed:
>
> Rename the virt_reg_failed label to name what is reversed instead of what
> happened to fail. E.g. fail_del_adapters, or something.
>
>> i2c_mux_del_adapters(muxc);
>> return ret;
>> @@ -302,6 +418,16 @@ static int pca954x_probe(struct i2c_client *client,
>> static int pca954x_remove(struct i2c_client *client)
>> {
>> struct i2c_mux_core *muxc = i2c_get_clientdata(client);
>> + struct pca954x *data = i2c_mux_priv(muxc);
>> + int c, irq;
>> +
>> + if (data->irq) {
>> + for (c = 0; c < data->chip->nchans; c++) {
>> + irq = irq_find_mapping(data->irq, c);
>> + irq_dispose_mapping(irq);
>> + }
>> + irq_domain_remove(data->irq);
>> + }
>>
>> i2c_mux_del_adapters(muxc);
>> return 0;
>>
>
--
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 5/5] i2c: mux: pca954x: Add irq_mask_en to delay enabling irqs
From: Peter Rosin @ 2017-01-04 14:21 UTC (permalink / raw)
To: Phil Reid, wsa-z923LK4zBo2bacvFa/9K2g,
robh+dt-DgEjT+Ai2ygdnm+yROfE0A, mark.rutland-5wv7dgnIgG8,
linux-i2c-u79uwXL29TY76Z2rM5mHXA,
devicetree-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <1483522197-38819-6-git-send-email-preid-qgqNFa1JUf/o2iN0hyhwsIdd74u8MsAO@public.gmane.org>
On 2017-01-04 10:29, Phil Reid wrote:
> Unfortunately some hardware device will assert their irq line immediately
> on power on and provide no mechanism to mask the irq. As the i2c muxes
> provide no method to mask irq line this provides a work around by keeping
> the parent irq masked until enough device drivers have loaded to service
> all pending interrupts.
>
> For example the the ltc1760 assert its SMBALERT irq immediately on power
> on. With two ltc1760 attached to bus 0 & 1 on a pca954x mux when the first
> device is registered irq are enabled and fire continuously as the second
> device driver has not yet loaded. Setting this parameter to 0x3 while
> delay the irq being enabled until both devices are ready.
I think this also needs a comment in the code, including a description
of the limitations. If the interrupt is shared between two devices on
the same bus, you would have the exact same problem and this workaround
would be no good...
Overall, this series fixes the issues I had with the patch from half
a year ago or so. Thanks!
With nitpicks fixed,
Acked-by: Peter Rosin <peda-koto5C5qi+TLoDKTGw+V6w@public.gmane.org>
Cheers,
peda
> Signed-off-by: Phil Reid <preid-qgqNFa1JUf/o2iN0hyhwsIdd74u8MsAO@public.gmane.org>
> ---
> drivers/i2c/muxes/i2c-mux-pca954x.c | 9 ++++++++-
> 1 file changed, 8 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/i2c/muxes/i2c-mux-pca954x.c b/drivers/i2c/muxes/i2c-mux-pca954x.c
> index f2dba7c..16269e7 100644
> --- a/drivers/i2c/muxes/i2c-mux-pca954x.c
> +++ b/drivers/i2c/muxes/i2c-mux-pca954x.c
> @@ -83,6 +83,7 @@ struct pca954x {
>
> struct irq_domain *irq;
> unsigned int irq_mask;
> + unsigned int irq_mask_en;
Spell it out: irq_mask_enable
> };
>
> /* Provide specs for the PCA954x types we know about */
> @@ -251,9 +252,12 @@ static void pca954x_irq_unmask(struct irq_data *idata)
> struct pca954x *data = irq_data_get_irq_chip_data(idata);
> unsigned int pos = idata->hwirq;
>
> - if (!data->irq_mask)
> + if (!data->irq_mask_en && !data->irq_mask)
> enable_irq(data->client->irq);
> data->irq_mask |= BIT(pos);
> + if (data->irq_mask_en &&
> + (data->irq_mask & data->irq_mask) == data->irq_mask_en)
> + enable_irq(data->client->irq);
> }
>
> static int pca954x_irq_set_type(struct irq_data *idata, unsigned int type)
> @@ -369,6 +373,9 @@ static int pca954x_probe(struct i2c_client *client,
> idle_disconnect_dt = of_node &&
> of_property_read_bool(of_node, "i2c-mux-idle-disconnect");
>
> + of_property_read_u32(of_node, "i2c-mux-irq-mask-en",
"nxp,irq-mask-enable"
> + &data->irq_mask_en);
> +
> ret = pca953x_irq_setup(muxc);
> if (ret)
> goto irq_setup_failed;
>
--
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 4/5] dt: bindings: i2c-mux-pca954x: Add documentation for i2c-mux-irq-mask-en
From: Peter Rosin @ 2017-01-04 14:14 UTC (permalink / raw)
To: Phil Reid, wsa-z923LK4zBo2bacvFa/9K2g,
robh+dt-DgEjT+Ai2ygdnm+yROfE0A, mark.rutland-5wv7dgnIgG8,
linux-i2c-u79uwXL29TY76Z2rM5mHXA,
devicetree-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <1483522197-38819-5-git-send-email-preid-qgqNFa1JUf/o2iN0hyhwsIdd74u8MsAO@public.gmane.org>
On 2017-01-04 10:29, Phil Reid wrote:
> Unfortunately some hardware device will assert their irq line immediately
> on power on and provide no mechanism to mask the irq. As the i2c muxes
> provide no method to mask irq line this provides a work around by keeping
> the parent irq masked until enough device drivers have loaded to service
> all pending interrupts.
>
> For example the the ltc1760 assert its SMBALERT irq immediately on power
> on. With two ltc1760 attached to bus 0 & 1 on a pca954x mux when the first
> device is registered irq are enabled and fire continuously as the second
> device driver has not yet loaded. Setting this parameter to 0x3 while
> delay the irq being enabled until both devices are ready.
>
> Signed-off-by: Phil Reid <preid-qgqNFa1JUf/o2iN0hyhwsIdd74u8MsAO@public.gmane.org>
> ---
> Documentation/devicetree/bindings/i2c/i2c-mux-pca954x.txt | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/i2c/i2c-mux-pca954x.txt b/Documentation/devicetree/bindings/i2c/i2c-mux-pca954x.txt
> index 9f7c275..d3e63dc 100644
> --- a/Documentation/devicetree/bindings/i2c/i2c-mux-pca954x.txt
> +++ b/Documentation/devicetree/bindings/i2c/i2c-mux-pca954x.txt
> @@ -19,6 +19,8 @@ Optional Properties:
> - i2c-mux-idle-disconnect: Boolean; if defined, forces mux to disconnect all
> children in idle state. This is necessary for example, if there are several
> multiplexers on the bus and the devices behind them use same I2C addresses.
> + - i2c-mux-irq-mask-en: BitMask; Defines a mask for which irq lines need to be
> + unmasked before the parent irq line in enabled.
The prefix should be "nxp," instead of "i2c-mux-", and I don't think you should
abbreviate enable. So, nxp,irq-mask-enable.
With that fixed,
Acked-by: Peter Rosin <peda-koto5C5qi+TLoDKTGw+V6w@public.gmane.org>
Cheers,
peda
> - interrupt-parent: Phandle for the interrupt controller that services
> interrupts for this device.
> - interrupts: Interrupt mapping for IRQ.
> @@ -36,6 +38,7 @@ Example:
> #size-cells = <0>;
> reg = <0x74>;
>
> + i2c-mux-irq-mask-en = <0x3>;
> interrupt-parent = <&ipic>;
> interrupts = <17 IRQ_TYPE_LEVEL_LOW>;
> interrupt-controller;
>
--
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 3/5] i2c: mux: pca954x: Add interrupt controller support
From: Peter Rosin @ 2017-01-04 14:13 UTC (permalink / raw)
To: Phil Reid, wsa-z923LK4zBo2bacvFa/9K2g,
robh+dt-DgEjT+Ai2ygdnm+yROfE0A, mark.rutland-5wv7dgnIgG8,
linux-i2c-u79uwXL29TY76Z2rM5mHXA,
devicetree-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <1483522197-38819-4-git-send-email-preid-qgqNFa1JUf/o2iN0hyhwsIdd74u8MsAO@public.gmane.org>
On 2017-01-04 10:29, Phil Reid wrote:
> Various muxes can aggregate multiple interrupts from each i2c bus.
> All of the muxes with interrupt support combine the active low irq lines
> using an internal 'and' function and generate a combined active low
> output. The muxes do provide the ability to read a control register to
> determine which irq is active. By making the mux an irq controller isr
> can potentially be reduced by reading the status register and then only
> calling the registered isr on that bus segment.
>
> As there is no irq masking on the mux irq are disabled until irq_unmask is
> called at least once.
Irqs are not my strong point, I would prefer if someone else also had
a look. And there are some comments below...
Cheers,
peda
> Signed-off-by: Phil Reid <preid-qgqNFa1JUf/o2iN0hyhwsIdd74u8MsAO@public.gmane.org>
> ---
> drivers/i2c/muxes/i2c-mux-pca954x.c | 126 ++++++++++++++++++++++++++++++++++++
> 1 file changed, 126 insertions(+)
>
> diff --git a/drivers/i2c/muxes/i2c-mux-pca954x.c b/drivers/i2c/muxes/i2c-mux-pca954x.c
> index 981d145..f2dba7c 100644
> --- a/drivers/i2c/muxes/i2c-mux-pca954x.c
> +++ b/drivers/i2c/muxes/i2c-mux-pca954x.c
> @@ -40,14 +40,19 @@
> #include <linux/i2c.h>
> #include <linux/i2c-mux.h>
> #include <linux/i2c/pca954x.h>
> +#include <linux/interrupt.h>
> +#include <linux/irq.h>
> #include <linux/module.h>
> #include <linux/of.h>
> #include <linux/of_device.h>
> +#include <linux/of_irq.h>
> #include <linux/pm.h>
> #include <linux/slab.h>
>
> #define PCA954X_MAX_NCHANS 8
>
> +#define PCA954X_IRQ_OFFSET 4
> +
> enum pca_type {
> pca_9540,
> pca_9542,
> @@ -62,6 +67,7 @@ enum pca_type {
> struct chip_desc {
> u8 nchans;
> u8 enable; /* used for muxes only */
> + u8 has_irq;
> enum muxtype {
> pca954x_ismux = 0,
> pca954x_isswi
> @@ -74,6 +80,9 @@ struct pca954x {
> u8 last_chan; /* last register value */
> u8 deselect;
> struct i2c_client *client;
> +
> + struct irq_domain *irq;
> + unsigned int irq_mask;
> };
>
> /* Provide specs for the PCA954x types we know about */
> @@ -86,19 +95,23 @@ struct pca954x {
> [pca_9542] = {
> .nchans = 2,
> .enable = 0x4,
> + .has_irq = 1,
> .muxtype = pca954x_ismux,
> },
> [pca_9543] = {
> .nchans = 2,
> + .has_irq = 1,
> .muxtype = pca954x_isswi,
> },
> [pca_9544] = {
> .nchans = 4,
> .enable = 0x4,
> + .has_irq = 1,
> .muxtype = pca954x_ismux,
> },
> [pca_9545] = {
> .nchans = 4,
> + .has_irq = 1,
> .muxtype = pca954x_isswi,
> },
> [pca_9547] = {
> @@ -203,6 +216,104 @@ static int pca954x_deselect_mux(struct i2c_mux_core *muxc, u32 chan)
> return pca954x_reg_write(muxc->parent, client, data->last_chan);
> }
>
> +static irqreturn_t pca954x_irq_handler(int irq, void *dev_id)
> +{
> + struct pca954x *data = dev_id;
> + unsigned int child_irq;
> + int ret, i, handled;
> +
> + ret = i2c_smbus_read_byte(data->client);
> + if (ret < 0)
> + return IRQ_NONE;
> +
> + for (i = 0; i < data->chip->nchans; i++) {
> + if (ret & BIT(PCA954X_IRQ_OFFSET+i)) {
Spaces around the +
> + child_irq = irq_linear_revmap(data->irq, i);
> + handle_nested_irq(child_irq);
> + handled++;
> + }
> + }
> + return handled ? IRQ_HANDLED : IRQ_NONE;
> +}
> +
> +static void pca954x_irq_mask(struct irq_data *idata)
> +{
> + struct pca954x *data = irq_data_get_irq_chip_data(idata);
> + unsigned int pos = idata->hwirq;
> +
> + data->irq_mask &= ~BIT(pos);
> + if (!data->irq_mask)
> + disable_irq(data->client->irq);
> +}
> +
> +static void pca954x_irq_unmask(struct irq_data *idata)
> +{
> + struct pca954x *data = irq_data_get_irq_chip_data(idata);
> + unsigned int pos = idata->hwirq;
> +
> + if (!data->irq_mask)
> + enable_irq(data->client->irq);
> + data->irq_mask |= BIT(pos);
> +}
> +
> +static int pca954x_irq_set_type(struct irq_data *idata, unsigned int type)
> +{
> + if ((type & IRQ_TYPE_SENSE_MASK) != IRQ_TYPE_LEVEL_LOW)
> + return -EINVAL;
> + return 0;
> +}
> +
> +static struct irq_chip pca954x_irq_chip = {
> + .name = "i2c-mux-pca954x",
> + .irq_mask = pca954x_irq_mask,
> + .irq_unmask = pca954x_irq_unmask,
> + .irq_set_type = pca954x_irq_set_type,
> +};
> +
> +static int pca953x_irq_setup(struct i2c_mux_core *muxc)
> +{
> + struct pca954x *data = i2c_mux_priv(muxc);
> + struct i2c_client *client = data->client;
> + int c, err, irq;
> +
> + if (!data->chip->has_irq || client->irq <= 0)
> + return 0;
> +
> + data->irq = irq_domain_add_linear(client->dev.of_node,
> + data->chip->nchans,
> + &irq_domain_simple_ops, data);
> + if (!data->irq) {
> + err = -ENODEV;
> + goto err_irq_domain;
This label is not needed, just return -ENODEV
> + }
> +
> + for (c = 0; c < data->chip->nchans; c++) {
> + irq = irq_create_mapping(data->irq, c);
> + irq_set_chip_data(irq, data);
> + irq_set_chip_and_handler(irq, &pca954x_irq_chip,
> + handle_simple_irq);
> + }
> +
> + err = devm_request_threaded_irq(&client->dev, data->client->irq, NULL,
> + pca954x_irq_handler,
> + IRQF_ONESHOT | IRQF_SHARED,
> + "pca953x", data);
"pca954x"
> + if (err)
> + goto err_req_irq;
> +
> + disable_irq(data->client->irq);
> +
> + return 0;
> +err_req_irq:
> + for (c = 0; c < data->chip->nchans; c++) {
> + irq = irq_find_mapping(data->irq, c);
> + irq_dispose_mapping(irq);
> + }
> + irq_domain_remove(data->irq);
> +err_irq_domain:
> + return err;
> +}
> +
> /*
> * I2C init/probing/exit functions
> */
> @@ -258,6 +369,10 @@ static int pca954x_probe(struct i2c_client *client,
> idle_disconnect_dt = of_node &&
> of_property_read_bool(of_node, "i2c-mux-idle-disconnect");
>
> + ret = pca953x_irq_setup(muxc);
> + if (ret)
> + goto irq_setup_failed;
> +
> /* Now create an adapter for each channel */
> for (num = 0; num < data->chip->nchans; num++) {
> bool idle_disconnect_pd = false;
> @@ -294,6 +409,7 @@ static int pca954x_probe(struct i2c_client *client,
>
> return 0;
>
> +irq_setup_failed:
> virt_reg_failed:
Rename the virt_reg_failed label to name what is reversed instead of what
happened to fail. E.g. fail_del_adapters, or something.
> i2c_mux_del_adapters(muxc);
> return ret;
> @@ -302,6 +418,16 @@ static int pca954x_probe(struct i2c_client *client,
> static int pca954x_remove(struct i2c_client *client)
> {
> struct i2c_mux_core *muxc = i2c_get_clientdata(client);
> + struct pca954x *data = i2c_mux_priv(muxc);
> + int c, irq;
> +
> + if (data->irq) {
> + for (c = 0; c < data->chip->nchans; c++) {
> + irq = irq_find_mapping(data->irq, c);
> + irq_dispose_mapping(irq);
> + }
> + irq_domain_remove(data->irq);
> + }
>
> i2c_mux_del_adapters(muxc);
> return 0;
>
--
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 1/5] i2c: mux: pca954x: Add missing pca9542 definition to chip_desc
From: Peter Rosin @ 2017-01-04 14:08 UTC (permalink / raw)
To: Phil Reid, wsa-z923LK4zBo2bacvFa/9K2g,
robh+dt-DgEjT+Ai2ygdnm+yROfE0A, mark.rutland-5wv7dgnIgG8,
linux-i2c-u79uwXL29TY76Z2rM5mHXA,
devicetree-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <1483522197-38819-2-git-send-email-preid-qgqNFa1JUf/o2iN0hyhwsIdd74u8MsAO@public.gmane.org>
On 2017-01-04 10:29, Phil Reid wrote:
> The spec for the pca954x was missing. This chip is the same as the pca9540
> except that it has interrupt lines. While the i2c_device_id table mapped
> the pca9542 to the pca9540 definition the compatible table did not. In
> preparation for irq support add the pca9542 definition.
A new ACPI table was added behind your back, which needs the same
treatment as the i2c table.
With that fixed,
Acked-by: Peter Rosin <peda-koto5C5qi+TLoDKTGw+V6w@public.gmane.org>
Cheers,
peda
> Signed-off-by: Phil Reid <preid-qgqNFa1JUf/o2iN0hyhwsIdd74u8MsAO@public.gmane.org>
> ---
> drivers/i2c/muxes/i2c-mux-pca954x.c | 7 ++++++-
> 1 file changed, 6 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/i2c/muxes/i2c-mux-pca954x.c b/drivers/i2c/muxes/i2c-mux-pca954x.c
> index 8bc3d36..981d145 100644
> --- a/drivers/i2c/muxes/i2c-mux-pca954x.c
> +++ b/drivers/i2c/muxes/i2c-mux-pca954x.c
> @@ -83,6 +83,11 @@ struct pca954x {
> .enable = 0x4,
> .muxtype = pca954x_ismux,
> },
> + [pca_9542] = {
> + .nchans = 2,
> + .enable = 0x4,
> + .muxtype = pca954x_ismux,
> + },
> [pca_9543] = {
> .nchans = 2,
> .muxtype = pca954x_isswi,
> @@ -109,7 +114,7 @@ struct pca954x {
>
> static const struct i2c_device_id pca954x_id[] = {
> { "pca9540", pca_9540 },
> - { "pca9542", pca_9540 },
> + { "pca9542", pca_9542 },
> { "pca9543", pca_9543 },
> { "pca9544", pca_9544 },
> { "pca9545", pca_9545 },
>
--
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
* [PATCH 5/5] i2c: mux: pca954x: Add irq_mask_en to delay enabling irqs
From: Phil Reid @ 2017-01-04 9:29 UTC (permalink / raw)
To: peda, wsa, robh+dt, mark.rutland, preid, linux-i2c, devicetree
In-Reply-To: <1483522197-38819-1-git-send-email-preid@electromag.com.au>
Unfortunately some hardware device will assert their irq line immediately
on power on and provide no mechanism to mask the irq. As the i2c muxes
provide no method to mask irq line this provides a work around by keeping
the parent irq masked until enough device drivers have loaded to service
all pending interrupts.
For example the the ltc1760 assert its SMBALERT irq immediately on power
on. With two ltc1760 attached to bus 0 & 1 on a pca954x mux when the first
device is registered irq are enabled and fire continuously as the second
device driver has not yet loaded. Setting this parameter to 0x3 while
delay the irq being enabled until both devices are ready.
Signed-off-by: Phil Reid <preid@electromag.com.au>
---
drivers/i2c/muxes/i2c-mux-pca954x.c | 9 ++++++++-
1 file changed, 8 insertions(+), 1 deletion(-)
diff --git a/drivers/i2c/muxes/i2c-mux-pca954x.c b/drivers/i2c/muxes/i2c-mux-pca954x.c
index f2dba7c..16269e7 100644
--- a/drivers/i2c/muxes/i2c-mux-pca954x.c
+++ b/drivers/i2c/muxes/i2c-mux-pca954x.c
@@ -83,6 +83,7 @@ struct pca954x {
struct irq_domain *irq;
unsigned int irq_mask;
+ unsigned int irq_mask_en;
};
/* Provide specs for the PCA954x types we know about */
@@ -251,9 +252,12 @@ static void pca954x_irq_unmask(struct irq_data *idata)
struct pca954x *data = irq_data_get_irq_chip_data(idata);
unsigned int pos = idata->hwirq;
- if (!data->irq_mask)
+ if (!data->irq_mask_en && !data->irq_mask)
enable_irq(data->client->irq);
data->irq_mask |= BIT(pos);
+ if (data->irq_mask_en &&
+ (data->irq_mask & data->irq_mask) == data->irq_mask_en)
+ enable_irq(data->client->irq);
}
static int pca954x_irq_set_type(struct irq_data *idata, unsigned int type)
@@ -369,6 +373,9 @@ static int pca954x_probe(struct i2c_client *client,
idle_disconnect_dt = of_node &&
of_property_read_bool(of_node, "i2c-mux-idle-disconnect");
+ of_property_read_u32(of_node, "i2c-mux-irq-mask-en",
+ &data->irq_mask_en);
+
ret = pca953x_irq_setup(muxc);
if (ret)
goto irq_setup_failed;
--
1.8.3.1
^ permalink raw reply related
* [PATCH 0/5] i2c: mux: pca954x: Add interrupt controller support
From: Phil Reid @ 2017-01-04 9:29 UTC (permalink / raw)
To: peda, wsa, robh+dt, mark.rutland, preid, linux-i2c, devicetree
Various muxes can aggregate multiple interrupts from each i2c bus.
All of the muxes with interrupt support combine the active low irq lines
using an internal 'and' function and generate a combined active low
output. The muxes do provide the ability to read a control register to
determine which irq is active. By making the mux an irq controller isr
latenct can potentially be reduced by reading the status register and
then only calling the registered isr on that bus segment.
In addition an additional enable mask is added to work around devices
that assert irq immediately before being setup buy disabling the irq
from the mux until all devices are registered.
Phil Reid (5):
i2c: mux: pca954x: Add missing pca9542 definition to chip_desc
dt: bindings: i2c-mux-pca954x: Add documentation for interrupt
controller
i2c: mux: pca954x: Add interrupt controller support
dt: bindings: i2c-mux-pca954x: Add documentation for
i2c-mux-irq-mask-en
i2c: mux: pca954x: Add irq_mask_en to delay enabling irqs
.../devicetree/bindings/i2c/i2c-mux-pca954x.txt | 17 ++-
drivers/i2c/muxes/i2c-mux-pca954x.c | 140 ++++++++++++++++++++-
2 files changed, 155 insertions(+), 2 deletions(-)
--
1.8.3.1
^ permalink raw reply
* [PATCH 4/5] dt: bindings: i2c-mux-pca954x: Add documentation for i2c-mux-irq-mask-en
From: Phil Reid @ 2017-01-04 9:29 UTC (permalink / raw)
To: peda, wsa, robh+dt, mark.rutland, preid, linux-i2c, devicetree
In-Reply-To: <1483522197-38819-1-git-send-email-preid@electromag.com.au>
Unfortunately some hardware device will assert their irq line immediately
on power on and provide no mechanism to mask the irq. As the i2c muxes
provide no method to mask irq line this provides a work around by keeping
the parent irq masked until enough device drivers have loaded to service
all pending interrupts.
For example the the ltc1760 assert its SMBALERT irq immediately on power
on. With two ltc1760 attached to bus 0 & 1 on a pca954x mux when the first
device is registered irq are enabled and fire continuously as the second
device driver has not yet loaded. Setting this parameter to 0x3 while
delay the irq being enabled until both devices are ready.
Signed-off-by: Phil Reid <preid@electromag.com.au>
---
Documentation/devicetree/bindings/i2c/i2c-mux-pca954x.txt | 3 +++
1 file changed, 3 insertions(+)
diff --git a/Documentation/devicetree/bindings/i2c/i2c-mux-pca954x.txt b/Documentation/devicetree/bindings/i2c/i2c-mux-pca954x.txt
index 9f7c275..d3e63dc 100644
--- a/Documentation/devicetree/bindings/i2c/i2c-mux-pca954x.txt
+++ b/Documentation/devicetree/bindings/i2c/i2c-mux-pca954x.txt
@@ -19,6 +19,8 @@ Optional Properties:
- i2c-mux-idle-disconnect: Boolean; if defined, forces mux to disconnect all
children in idle state. This is necessary for example, if there are several
multiplexers on the bus and the devices behind them use same I2C addresses.
+ - i2c-mux-irq-mask-en: BitMask; Defines a mask for which irq lines need to be
+ unmasked before the parent irq line in enabled.
- interrupt-parent: Phandle for the interrupt controller that services
interrupts for this device.
- interrupts: Interrupt mapping for IRQ.
@@ -36,6 +38,7 @@ Example:
#size-cells = <0>;
reg = <0x74>;
+ i2c-mux-irq-mask-en = <0x3>;
interrupt-parent = <&ipic>;
interrupts = <17 IRQ_TYPE_LEVEL_LOW>;
interrupt-controller;
--
1.8.3.1
^ permalink raw reply related
* [PATCH 3/5] i2c: mux: pca954x: Add interrupt controller support
From: Phil Reid @ 2017-01-04 9:29 UTC (permalink / raw)
To: peda, wsa, robh+dt, mark.rutland, preid, linux-i2c, devicetree
In-Reply-To: <1483522197-38819-1-git-send-email-preid@electromag.com.au>
Various muxes can aggregate multiple interrupts from each i2c bus.
All of the muxes with interrupt support combine the active low irq lines
using an internal 'and' function and generate a combined active low
output. The muxes do provide the ability to read a control register to
determine which irq is active. By making the mux an irq controller isr
can potentially be reduced by reading the status register and then only
calling the registered isr on that bus segment.
As there is no irq masking on the mux irq are disabled until irq_unmask is
called at least once.
Signed-off-by: Phil Reid <preid@electromag.com.au>
---
drivers/i2c/muxes/i2c-mux-pca954x.c | 126 ++++++++++++++++++++++++++++++++++++
1 file changed, 126 insertions(+)
diff --git a/drivers/i2c/muxes/i2c-mux-pca954x.c b/drivers/i2c/muxes/i2c-mux-pca954x.c
index 981d145..f2dba7c 100644
--- a/drivers/i2c/muxes/i2c-mux-pca954x.c
+++ b/drivers/i2c/muxes/i2c-mux-pca954x.c
@@ -40,14 +40,19 @@
#include <linux/i2c.h>
#include <linux/i2c-mux.h>
#include <linux/i2c/pca954x.h>
+#include <linux/interrupt.h>
+#include <linux/irq.h>
#include <linux/module.h>
#include <linux/of.h>
#include <linux/of_device.h>
+#include <linux/of_irq.h>
#include <linux/pm.h>
#include <linux/slab.h>
#define PCA954X_MAX_NCHANS 8
+#define PCA954X_IRQ_OFFSET 4
+
enum pca_type {
pca_9540,
pca_9542,
@@ -62,6 +67,7 @@ enum pca_type {
struct chip_desc {
u8 nchans;
u8 enable; /* used for muxes only */
+ u8 has_irq;
enum muxtype {
pca954x_ismux = 0,
pca954x_isswi
@@ -74,6 +80,9 @@ struct pca954x {
u8 last_chan; /* last register value */
u8 deselect;
struct i2c_client *client;
+
+ struct irq_domain *irq;
+ unsigned int irq_mask;
};
/* Provide specs for the PCA954x types we know about */
@@ -86,19 +95,23 @@ struct pca954x {
[pca_9542] = {
.nchans = 2,
.enable = 0x4,
+ .has_irq = 1,
.muxtype = pca954x_ismux,
},
[pca_9543] = {
.nchans = 2,
+ .has_irq = 1,
.muxtype = pca954x_isswi,
},
[pca_9544] = {
.nchans = 4,
.enable = 0x4,
+ .has_irq = 1,
.muxtype = pca954x_ismux,
},
[pca_9545] = {
.nchans = 4,
+ .has_irq = 1,
.muxtype = pca954x_isswi,
},
[pca_9547] = {
@@ -203,6 +216,104 @@ static int pca954x_deselect_mux(struct i2c_mux_core *muxc, u32 chan)
return pca954x_reg_write(muxc->parent, client, data->last_chan);
}
+static irqreturn_t pca954x_irq_handler(int irq, void *dev_id)
+{
+ struct pca954x *data = dev_id;
+ unsigned int child_irq;
+ int ret, i, handled;
+
+ ret = i2c_smbus_read_byte(data->client);
+ if (ret < 0)
+ return IRQ_NONE;
+
+ for (i = 0; i < data->chip->nchans; i++) {
+ if (ret & BIT(PCA954X_IRQ_OFFSET+i)) {
+ child_irq = irq_linear_revmap(data->irq, i);
+ handle_nested_irq(child_irq);
+ handled++;
+ }
+ }
+ return handled ? IRQ_HANDLED : IRQ_NONE;
+}
+
+static void pca954x_irq_mask(struct irq_data *idata)
+{
+ struct pca954x *data = irq_data_get_irq_chip_data(idata);
+ unsigned int pos = idata->hwirq;
+
+ data->irq_mask &= ~BIT(pos);
+ if (!data->irq_mask)
+ disable_irq(data->client->irq);
+}
+
+static void pca954x_irq_unmask(struct irq_data *idata)
+{
+ struct pca954x *data = irq_data_get_irq_chip_data(idata);
+ unsigned int pos = idata->hwirq;
+
+ if (!data->irq_mask)
+ enable_irq(data->client->irq);
+ data->irq_mask |= BIT(pos);
+}
+
+static int pca954x_irq_set_type(struct irq_data *idata, unsigned int type)
+{
+ if ((type & IRQ_TYPE_SENSE_MASK) != IRQ_TYPE_LEVEL_LOW)
+ return -EINVAL;
+ return 0;
+}
+
+static struct irq_chip pca954x_irq_chip = {
+ .name = "i2c-mux-pca954x",
+ .irq_mask = pca954x_irq_mask,
+ .irq_unmask = pca954x_irq_unmask,
+ .irq_set_type = pca954x_irq_set_type,
+};
+
+static int pca953x_irq_setup(struct i2c_mux_core *muxc)
+{
+ struct pca954x *data = i2c_mux_priv(muxc);
+ struct i2c_client *client = data->client;
+ int c, err, irq;
+
+ if (!data->chip->has_irq || client->irq <= 0)
+ return 0;
+
+ data->irq = irq_domain_add_linear(client->dev.of_node,
+ data->chip->nchans,
+ &irq_domain_simple_ops, data);
+ if (!data->irq) {
+ err = -ENODEV;
+ goto err_irq_domain;
+ }
+
+ for (c = 0; c < data->chip->nchans; c++) {
+ irq = irq_create_mapping(data->irq, c);
+ irq_set_chip_data(irq, data);
+ irq_set_chip_and_handler(irq, &pca954x_irq_chip,
+ handle_simple_irq);
+ }
+
+ err = devm_request_threaded_irq(&client->dev, data->client->irq, NULL,
+ pca954x_irq_handler,
+ IRQF_ONESHOT | IRQF_SHARED,
+ "pca953x", data);
+ if (err)
+ goto err_req_irq;
+
+ disable_irq(data->client->irq);
+
+ return 0;
+err_req_irq:
+ for (c = 0; c < data->chip->nchans; c++) {
+ irq = irq_find_mapping(data->irq, c);
+ irq_dispose_mapping(irq);
+ }
+ irq_domain_remove(data->irq);
+err_irq_domain:
+ return err;
+}
+
/*
* I2C init/probing/exit functions
*/
@@ -258,6 +369,10 @@ static int pca954x_probe(struct i2c_client *client,
idle_disconnect_dt = of_node &&
of_property_read_bool(of_node, "i2c-mux-idle-disconnect");
+ ret = pca953x_irq_setup(muxc);
+ if (ret)
+ goto irq_setup_failed;
+
/* Now create an adapter for each channel */
for (num = 0; num < data->chip->nchans; num++) {
bool idle_disconnect_pd = false;
@@ -294,6 +409,7 @@ static int pca954x_probe(struct i2c_client *client,
return 0;
+irq_setup_failed:
virt_reg_failed:
i2c_mux_del_adapters(muxc);
return ret;
@@ -302,6 +418,16 @@ static int pca954x_probe(struct i2c_client *client,
static int pca954x_remove(struct i2c_client *client)
{
struct i2c_mux_core *muxc = i2c_get_clientdata(client);
+ struct pca954x *data = i2c_mux_priv(muxc);
+ int c, irq;
+
+ if (data->irq) {
+ for (c = 0; c < data->chip->nchans; c++) {
+ irq = irq_find_mapping(data->irq, c);
+ irq_dispose_mapping(irq);
+ }
+ irq_domain_remove(data->irq);
+ }
i2c_mux_del_adapters(muxc);
return 0;
--
1.8.3.1
^ permalink raw reply related
* [PATCH 1/5] i2c: mux: pca954x: Add missing pca9542 definition to chip_desc
From: Phil Reid @ 2017-01-04 9:29 UTC (permalink / raw)
To: peda, wsa, robh+dt, mark.rutland, preid, linux-i2c, devicetree
In-Reply-To: <1483522197-38819-1-git-send-email-preid@electromag.com.au>
The spec for the pca954x was missing. This chip is the same as the pca9540
except that it has interrupt lines. While the i2c_device_id table mapped
the pca9542 to the pca9540 definition the compatible table did not. In
preparation for irq support add the pca9542 definition.
Signed-off-by: Phil Reid <preid@electromag.com.au>
---
drivers/i2c/muxes/i2c-mux-pca954x.c | 7 ++++++-
1 file changed, 6 insertions(+), 1 deletion(-)
diff --git a/drivers/i2c/muxes/i2c-mux-pca954x.c b/drivers/i2c/muxes/i2c-mux-pca954x.c
index 8bc3d36..981d145 100644
--- a/drivers/i2c/muxes/i2c-mux-pca954x.c
+++ b/drivers/i2c/muxes/i2c-mux-pca954x.c
@@ -83,6 +83,11 @@ struct pca954x {
.enable = 0x4,
.muxtype = pca954x_ismux,
},
+ [pca_9542] = {
+ .nchans = 2,
+ .enable = 0x4,
+ .muxtype = pca954x_ismux,
+ },
[pca_9543] = {
.nchans = 2,
.muxtype = pca954x_isswi,
@@ -109,7 +114,7 @@ struct pca954x {
static const struct i2c_device_id pca954x_id[] = {
{ "pca9540", pca_9540 },
- { "pca9542", pca_9540 },
+ { "pca9542", pca_9542 },
{ "pca9543", pca_9543 },
{ "pca9544", pca_9544 },
{ "pca9545", pca_9545 },
--
1.8.3.1
^ permalink raw reply related
* [PATCH 2/5] dt: bindings: i2c-mux-pca954x: Add documentation for interrupt controller
From: Phil Reid @ 2017-01-04 9:29 UTC (permalink / raw)
To: peda, wsa, robh+dt, mark.rutland, preid, linux-i2c, devicetree
In-Reply-To: <1483522197-38819-1-git-send-email-preid@electromag.com.au>
Various muxes can aggregate multiple irq lines and provide a control
register to determine the active line. Add bindings for interrupt
controller support.
Signed-off-by: Phil Reid <preid@electromag.com.au>
---
Documentation/devicetree/bindings/i2c/i2c-mux-pca954x.txt | 14 +++++++++++++-
1 file changed, 13 insertions(+), 1 deletion(-)
diff --git a/Documentation/devicetree/bindings/i2c/i2c-mux-pca954x.txt b/Documentation/devicetree/bindings/i2c/i2c-mux-pca954x.txt
index cf53d5f..9f7c275 100644
--- a/Documentation/devicetree/bindings/i2c/i2c-mux-pca954x.txt
+++ b/Documentation/devicetree/bindings/i2c/i2c-mux-pca954x.txt
@@ -19,7 +19,14 @@ Optional Properties:
- i2c-mux-idle-disconnect: Boolean; if defined, forces mux to disconnect all
children in idle state. This is necessary for example, if there are several
multiplexers on the bus and the devices behind them use same I2C addresses.
-
+ - interrupt-parent: Phandle for the interrupt controller that services
+ interrupts for this device.
+ - interrupts: Interrupt mapping for IRQ.
+ - interrupt-controller: Marks the device node as a interrupt controller.
+ - #interrupt-cells : Should be two.
+ - first cell is the pin number
+ - second cell is used to specify flags.
+ See also Documentation/devicetree/bindings/interrupt-controller/interrupts.txt
Example:
@@ -29,6 +36,11 @@ Example:
#size-cells = <0>;
reg = <0x74>;
+ interrupt-parent = <&ipic>;
+ interrupts = <17 IRQ_TYPE_LEVEL_LOW>;
+ interrupt-controller;
+ #interrupt-cells=<2>;
+
i2c@2 {
#address-cells = <1>;
#size-cells = <0>;
--
1.8.3.1
^ permalink raw reply related
* Re: [PATCH] i2c: i801: Register optional lis3lv02d i2c device on Dell machines
From: Benjamin Tissoires @ 2017-01-04 13:02 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: <20170104120031.GG3499@pali>
On Jan 04 2017 or thereabouts, Pali Rohár wrote:
> 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.
On the principle, I agree. But let's be realistic: this device will be
created but no events will be generated from it. It's a capability for
the chip to provide freefall, so I don't see why it's such an issue to
have one created which says nothing. (talking about non Dell platforms
here too)
> >
> > > 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.
I really don't follow you here. On a bus with no Host Notify, the IRQ
will be 0 and it will work in the same way. On a bus with Host Notify,
the IRQ should be set to something negative in the i2c_board_info to be
sure to not have an Host Notify irq assigned. In both case the
lis3lv02d_i2c driver will just do:
if (client->irq > 0)
lis3_dev.irq = client->irq;
So there is no bus adapter specifics in the driver.
>
> > This is basically incorrect if we use one property for two different
> > things (host notify and hdd free fall).
Host Notify is a way for a I2C device to report an interrupt without
having a dedicated line assigned (and so an IRQ). So the chip could use
Host Notify to report hdd free fall if it supports the feature.
> >
> > 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.
I won't say the design is wrong because Host Notify really is just an
IRQ. Where the issue lies now is that lis3lv02d makes the assumption
that an irq attached to an i2c_client means that it will report
freefall. It was correct before, but now it's not true anymore.
So maybe this series should also add a way to provide the source of
the IRQ (Host Notify or ACPI or DT). Then, lis3lv02d could ignore or
not the incoming IRQ.
Cheers,
Benjamin
> >
> > > 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 12:33 UTC (permalink / raw)
To: Pali Rohár
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: <20170104113500.GF3499@pali>
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
^ permalink raw reply
* [PATCH v7 12/12] mux: adg792a: add mux controller driver for ADG792A/G
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>
Analog Devices ADG792A/G is a triple 4:1 mux.
Signed-off-by: Peter Rosin <peda@axentia.se>
---
drivers/mux/Kconfig | 12 ++++
drivers/mux/Makefile | 1 +
drivers/mux/mux-adg792a.c | 169 ++++++++++++++++++++++++++++++++++++++++++++++
3 files changed, 182 insertions(+)
create mode 100644 drivers/mux/mux-adg792a.c
diff --git a/drivers/mux/Kconfig b/drivers/mux/Kconfig
index de5a35ffe7af..39b06e19aa96 100644
--- a/drivers/mux/Kconfig
+++ b/drivers/mux/Kconfig
@@ -16,6 +16,18 @@ menuconfig MULTIPLEXER
if MULTIPLEXER
+config MUX_ADG792A
+ tristate "Analog Devices ADG792A/ADG792G Multiplexers"
+ depends on I2C
+ help
+ ADG792A and ADG792G Wide Bandwidth Triple 4:1 Multiplexers
+
+ The driver supports both operating the three multiplexers in
+ parallel and operating them independently.
+
+ To compile the driver as a module, choose M here: the module will
+ be called mux-adg792a.
+
config MUX_GPIO
tristate "GPIO-controlled Multiplexer"
depends on OF && GPIOLIB
diff --git a/drivers/mux/Makefile b/drivers/mux/Makefile
index facc43da3648..fddbb073fc77 100644
--- a/drivers/mux/Makefile
+++ b/drivers/mux/Makefile
@@ -3,4 +3,5 @@
#
obj-$(CONFIG_MULTIPLEXER) += mux-core.o
+obj-$(CONFIG_MUX_ADG792A) += mux-adg792a.o
obj-$(CONFIG_MUX_GPIO) += mux-gpio.o
diff --git a/drivers/mux/mux-adg792a.c b/drivers/mux/mux-adg792a.c
new file mode 100644
index 000000000000..c6d5c4571e7c
--- /dev/null
+++ b/drivers/mux/mux-adg792a.c
@@ -0,0 +1,169 @@
+/*
+ * Multiplexer driver for Analog Devices ADG792A/G Triple 4:1 mux
+ *
+ * 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/i2c.h>
+#include <linux/module.h>
+#include <linux/mux.h>
+
+#define ADG792A_LDSW BIT(0)
+#define ADG792A_RESET BIT(1)
+#define ADG792A_DISABLE(mux) (0x50 | (mux))
+#define ADG792A_DISABLE_ALL (0x5f)
+#define ADG792A_MUX(mux, state) (0xc0 | (((mux) + 1) << 2) | (state))
+#define ADG792A_MUX_ALL(state) (0xc0 | (state))
+
+#define ADG792A_DISABLE_STATE (4)
+
+static int adg792a_set(struct mux_control *mux, int state)
+{
+ struct i2c_client *i2c = to_i2c_client(mux->chip->dev.parent);
+ u8 cmd;
+
+ if (mux->chip->controllers == 1) {
+ /* parallel mux controller operation */
+ if (state == ADG792A_DISABLE_STATE)
+ cmd = ADG792A_DISABLE_ALL;
+ else
+ cmd = ADG792A_MUX_ALL(state);
+ } else {
+ unsigned int controller = mux_control_get_index(mux);
+
+ if (state == ADG792A_DISABLE_STATE)
+ cmd = ADG792A_DISABLE(controller);
+ else
+ cmd = ADG792A_MUX(controller, state);
+ }
+
+ return i2c_smbus_write_byte_data(i2c, cmd, ADG792A_LDSW);
+}
+
+static const struct mux_control_ops adg792a_ops = {
+ .set = adg792a_set,
+};
+
+static int adg792a_probe(struct i2c_client *i2c,
+ const struct i2c_device_id *id)
+{
+ struct device *dev = &i2c->dev;
+ struct mux_chip *mux_chip;
+ bool parallel;
+ int count;
+ int ret;
+ int i;
+
+ parallel = of_property_read_bool(i2c->dev.of_node, "adi,parallel");
+
+ mux_chip = devm_mux_chip_alloc(dev, parallel ? 1 : 3, 0);
+ if (!mux_chip)
+ return -ENOMEM;
+
+ mux_chip->ops = &adg792a_ops;
+
+ ret = i2c_smbus_write_byte_data(i2c, ADG792A_DISABLE_ALL,
+ ADG792A_RESET | ADG792A_LDSW);
+ if (ret < 0)
+ return ret;
+
+ for (i = 0; i < mux_chip->controllers; ++i) {
+ struct mux_control *mux = &mux_chip->mux[i];
+
+ mux->states = 4;
+ }
+
+ count = of_property_count_u32_elems(dev->of_node, "adi,idle-state");
+ for (i = 0; i < count; i += 2) {
+ u32 index;
+ u32 idle_state;
+
+ ret = of_property_read_u32_index(dev->of_node,
+ "adi,idle-state", i,
+ &index);
+ if (ret < 0)
+ return ret;
+ if (index >= mux_chip->controllers) {
+ dev_err(dev, "invalid mux %u\n", index);
+ return -EINVAL;
+ }
+
+ ret = of_property_read_u32_index(dev->of_node,
+ "adi,idle-state", i + 1,
+ &idle_state);
+ if (ret < 0)
+ return ret;
+ if (idle_state >= ADG792A_DISABLE_STATE) {
+ dev_err(dev, "invalid idle-state %u for mux %u\n",
+ idle_state, index);
+ return -EINVAL;
+ }
+ mux_chip->mux[index].idle_state = idle_state;
+ }
+
+ count = of_property_count_u32_elems(dev->of_node,
+ "adi,idle-high-impedance");
+ for (i = 0; i < count; ++i) {
+ u32 index;
+
+ ret = of_property_read_u32_index(dev->of_node,
+ "adi,idle-high-impedance", i,
+ &index);
+ if (ret < 0)
+ return ret;
+ if (index >= mux_chip->controllers) {
+ dev_err(dev, "invalid mux %u\n", index);
+ return -EINVAL;
+ }
+
+ mux_chip->mux[index].idle_state = ADG792A_DISABLE_STATE;
+ }
+
+ ret = devm_mux_chip_register(dev, mux_chip);
+ if (ret < 0) {
+ dev_err(dev, "failed to register mux-chip\n");
+ return ret;
+ }
+
+ if (parallel)
+ dev_info(dev, "triple pole quadruple throw mux registered\n");
+ else
+ dev_info(dev, "3x single pole quadruple throw muxes registered\n");
+
+ return 0;
+}
+
+static const struct i2c_device_id adg792a_id[] = {
+ { .name = "adg792a", },
+ { .name = "adg792g", },
+ { }
+};
+MODULE_DEVICE_TABLE(i2c, adg792a_id);
+
+static const struct of_device_id adg792a_of_match[] = {
+ { .compatible = "adi,adg792a", },
+ { .compatible = "adi,adg792g", },
+ { }
+};
+MODULE_DEVICE_TABLE(of, adg792a_of_match);
+
+static struct i2c_driver adg792a_driver = {
+ .driver = {
+ .name = "adg792a",
+ .of_match_table = of_match_ptr(adg792a_of_match),
+ },
+ .probe = adg792a_probe,
+ .id_table = adg792a_id,
+};
+module_i2c_driver(adg792a_driver);
+
+MODULE_DESCRIPTION("Analog Devices ADG792A/G Triple 4:1 mux driver");
+MODULE_AUTHOR("Peter Rosin <peda@axentia.se");
+MODULE_LICENSE("GPL v2");
--
2.1.4
^ permalink raw reply related
* [PATCH v7 11/12] dt-bindings: mux-adg792a: document devicetree bindings for ADG792A/G 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>
Analog Devices ADG792A/G is a triple 4:1 mux.
Signed-off-by: Peter Rosin <peda@axentia.se>
---
.../devicetree/bindings/mux/mux-adg792a.txt | 79 ++++++++++++++++++++++
1 file changed, 79 insertions(+)
create mode 100644 Documentation/devicetree/bindings/mux/mux-adg792a.txt
diff --git a/Documentation/devicetree/bindings/mux/mux-adg792a.txt b/Documentation/devicetree/bindings/mux/mux-adg792a.txt
new file mode 100644
index 000000000000..0b26dd11f070
--- /dev/null
+++ b/Documentation/devicetree/bindings/mux/mux-adg792a.txt
@@ -0,0 +1,79 @@
+Bindings for Analog Devices ADG792A/G Triple 4:1 Multiplexers
+
+Required properties:
+- compatible : "adi,adg792a" or "adi,adg792g"
+- #mux-control-cells : <0> if parallel, or <1> if not.
+* Standard mux-controller bindings as decribed in mux-controller.txt
+
+Optional properties for ADG792G:
+- gpio-controller : if present, #gpio-cells below is required.
+- #gpio-cells : should be <2>
+ - First cell is the GPO line number, i.e. 0 or 1
+ - Second cell is used to specify active high (0)
+ or active low (1)
+
+Optional properties:
+- adi,parallel : if present, the three muxes are bound together with a single
+ mux controller, controlling all three muxes in parallel.
+- adi,idle-state : if present, array of 2-tuples with mux controller number
+ and state that mux controllers will have when idle. States 0 through 3
+ correspond to signals A through D in the datasheet.
+- adi,idle-high-impedance : if present, array of mux controller numbers that
+ should be in the disconnected high-impedance state when idle.
+
+Mux controller states 0 through 3 correspond to signals A through D in the
+datasheet. If a mux controller is mentioned in neither adi,idle-state nor
+adi,idle-high-impedance it is left in its previously selected state when idle.
+
+Example:
+
+ /*
+ * Three independent mux controllers (of which one is used).
+ * Mux 0 is disconnected when idle, mux 1 idles with signal C
+ * and mux 2 idles with signal A.
+ */
+ &i2c0 {
+ mux: adg792a@50 {
+ compatible = "adi,adg792a";
+ reg = <0x50>;
+ #mux-control-cells = <1>;
+
+ adi,idle-high-impedance = <0>;
+ adi,idle-state = <1 2>, <2 0>;
+ };
+ };
+
+ adc-mux {
+ compatible = "io-channel-mux";
+ io-channels = <&adc 0>;
+ io-channel-names = "parent";
+
+ mux-controls = <&mux 1>;
+
+ channels = "sync-1", "", "out";
+ };
+
+
+ /*
+ * Three parallel muxes with one mux controller, useful e.g. if
+ * the adc is differential, thus needing two signals to be muxed
+ * simultaneously for correct operation.
+ */
+ &i2c0 {
+ pmux: adg792a@50 {
+ compatible = "adi,adg792a";
+ reg = <0x50>;
+ #mux-control-cells = <0>;
+ adi,parallel;
+ };
+ };
+
+ diff-adc-mux {
+ compatible = "io-channel-mux";
+ io-channels = <&adc 0>;
+ io-channel-names = "parent";
+
+ mux-controls = <&pmux>;
+
+ channels = "sync-1", "", "out";
+ };
--
2.1.4
^ permalink raw reply related
* [PATCH v7 10/12] i2c: i2c-mux-simple: new driver
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>
This is a generic simple i2c mux that uses the generic multiplexer
subsystem to do the muxing.
The user can select if the mux is to be mux-locked and parent-locked
as described in Documentation/i2c/i2c-topology.
Acked-by: Jonathan Cameron <jic23-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
Signed-off-by: Peter Rosin <peda-koto5C5qi+TLoDKTGw+V6w@public.gmane.org>
---
drivers/i2c/muxes/Kconfig | 13 +++
drivers/i2c/muxes/Makefile | 1 +
drivers/i2c/muxes/i2c-mux-simple.c | 182 +++++++++++++++++++++++++++++++++++++
3 files changed, 196 insertions(+)
create mode 100644 drivers/i2c/muxes/i2c-mux-simple.c
diff --git a/drivers/i2c/muxes/Kconfig b/drivers/i2c/muxes/Kconfig
index 10b3d17ae3ea..565921e09a96 100644
--- a/drivers/i2c/muxes/Kconfig
+++ b/drivers/i2c/muxes/Kconfig
@@ -73,6 +73,19 @@ config I2C_MUX_REG
This driver can also be built as a module. If so, the module
will be called i2c-mux-reg.
+config I2C_MUX_SIMPLE
+ tristate "Simple I2C multiplexer"
+ select MULTIPLEXER
+ depends on OF
+ help
+ If you say yes to this option, support will be included for a
+ simple generic I2C multiplexer. This driver provides access to
+ I2C busses connected through a MUX, which is controlled
+ by a generic MUX controller.
+
+ This driver can also be built as a module. If so, the module
+ will be called i2c-mux-simple.
+
config I2C_DEMUX_PINCTRL
tristate "pinctrl-based I2C demultiplexer"
depends on PINCTRL && OF
diff --git a/drivers/i2c/muxes/Makefile b/drivers/i2c/muxes/Makefile
index 9948fa45037f..6821d95c92a3 100644
--- a/drivers/i2c/muxes/Makefile
+++ b/drivers/i2c/muxes/Makefile
@@ -11,5 +11,6 @@ obj-$(CONFIG_I2C_MUX_PCA9541) += i2c-mux-pca9541.o
obj-$(CONFIG_I2C_MUX_PCA954x) += i2c-mux-pca954x.o
obj-$(CONFIG_I2C_MUX_PINCTRL) += i2c-mux-pinctrl.o
obj-$(CONFIG_I2C_MUX_REG) += i2c-mux-reg.o
+obj-$(CONFIG_I2C_MUX_SIMPLE) += i2c-mux-simple.o
ccflags-$(CONFIG_I2C_DEBUG_BUS) := -DDEBUG
diff --git a/drivers/i2c/muxes/i2c-mux-simple.c b/drivers/i2c/muxes/i2c-mux-simple.c
new file mode 100644
index 000000000000..0d1421767c4c
--- /dev/null
+++ b/drivers/i2c/muxes/i2c-mux-simple.c
@@ -0,0 +1,182 @@
+/*
+ * Generic simple I2C multiplexer
+ *
+ * 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/i2c.h>
+#include <linux/i2c-mux.h>
+#include <linux/module.h>
+#include <linux/mux.h>
+#include <linux/of_device.h>
+#include <linux/platform_device.h>
+
+#define SIMPLE_PARENT_LOCKED (0)
+#define SIMPLE_MUX_LOCKED (1)
+
+struct mux {
+ struct mux_control *control;
+
+ bool do_not_deselect;
+};
+
+static int i2c_mux_select(struct i2c_mux_core *muxc, u32 chan)
+{
+ struct mux *mux = i2c_mux_priv(muxc);
+ int ret;
+
+ ret = mux_control_select(mux->control, chan);
+ mux->do_not_deselect = ret < 0;
+
+ return ret;
+}
+
+static int i2c_mux_deselect(struct i2c_mux_core *muxc, u32 chan)
+{
+ struct mux *mux = i2c_mux_priv(muxc);
+
+ if (mux->do_not_deselect)
+ return 0;
+
+ return mux_control_deselect(mux->control);
+}
+
+static struct i2c_adapter *mux_parent_adapter(struct device *dev)
+{
+ struct device_node *np = dev->of_node;
+ struct device_node *parent_np;
+ struct i2c_adapter *parent;
+
+ parent_np = of_parse_phandle(np, "i2c-parent", 0);
+ if (!parent_np) {
+ dev_err(dev, "Cannot parse i2c-parent\n");
+ return ERR_PTR(-ENODEV);
+ }
+ parent = of_find_i2c_adapter_by_node(parent_np);
+ of_node_put(parent_np);
+ if (!parent)
+ return ERR_PTR(-EPROBE_DEFER);
+
+ return parent;
+}
+
+static const struct of_device_id i2c_mux_of_match[] = {
+ { .compatible = "i2c-mux-simple,parent-locked",
+ .data = (void *)SIMPLE_PARENT_LOCKED, },
+ { .compatible = "i2c-mux-simple,mux-locked",
+ .data = (void *)SIMPLE_MUX_LOCKED, },
+ {},
+};
+MODULE_DEVICE_TABLE(of, i2c_mux_of_match);
+
+static int i2c_mux_probe(struct platform_device *pdev)
+{
+ struct device *dev = &pdev->dev;
+ struct device_node *np = dev->of_node;
+ struct device_node *child;
+ const struct of_device_id *match;
+ struct i2c_mux_core *muxc;
+ struct mux *mux;
+ struct i2c_adapter *parent;
+ int children;
+ int ret;
+
+ if (!np)
+ return -ENODEV;
+
+ mux = devm_kzalloc(dev, sizeof(*mux), GFP_KERNEL);
+ if (!mux)
+ return -ENOMEM;
+
+ 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);
+ }
+
+ parent = mux_parent_adapter(dev);
+ if (IS_ERR(parent)) {
+ if (PTR_ERR(parent) != -EPROBE_DEFER)
+ dev_err(dev, "failed to get i2c-parent adapter\n");
+ return PTR_ERR(parent);
+ }
+
+ children = of_get_child_count(np);
+
+ muxc = i2c_mux_alloc(parent, dev, children, 0, 0,
+ i2c_mux_select, i2c_mux_deselect);
+ if (!muxc) {
+ ret = -ENOMEM;
+ goto err_parent;
+ }
+ muxc->priv = mux;
+
+ platform_set_drvdata(pdev, muxc);
+
+ match = of_match_device(of_match_ptr(i2c_mux_of_match), dev);
+ if (match)
+ muxc->mux_locked = !!of_device_get_match_data(dev);
+
+ for_each_child_of_node(np, child) {
+ u32 chan;
+
+ ret = of_property_read_u32(child, "reg", &chan);
+ if (ret < 0) {
+ dev_err(dev, "no reg property for node '%s'\n",
+ child->name);
+ goto err_children;
+ }
+
+ if (chan >= mux->control->states) {
+ dev_err(dev, "invalid reg %u\n", chan);
+ ret = -EINVAL;
+ goto err_children;
+ }
+
+ ret = i2c_mux_add_adapter(muxc, 0, chan, 0);
+ if (ret)
+ goto err_children;
+ }
+
+ dev_info(dev, "%d-port mux on %s adapter\n", children, parent->name);
+
+ return 0;
+
+err_children:
+ i2c_mux_del_adapters(muxc);
+err_parent:
+ i2c_put_adapter(parent);
+
+ return ret;
+}
+
+static int i2c_mux_remove(struct platform_device *pdev)
+{
+ struct i2c_mux_core *muxc = platform_get_drvdata(pdev);
+
+ i2c_mux_del_adapters(muxc);
+ i2c_put_adapter(muxc->parent);
+
+ return 0;
+}
+
+static struct platform_driver i2c_mux_driver = {
+ .probe = i2c_mux_probe,
+ .remove = i2c_mux_remove,
+ .driver = {
+ .name = "i2c-mux-simple",
+ .of_match_table = i2c_mux_of_match,
+ },
+};
+module_platform_driver(i2c_mux_driver);
+
+MODULE_DESCRIPTION("Simple I2C multiplexer driver");
+MODULE_AUTHOR("Peter Rosin <peda-koto5C5qi+TLoDKTGw+V6w@public.gmane.org>");
+MODULE_LICENSE("GPL v2");
--
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 09/12] dt-bindings: i2c: i2c-mux-simple: document i2c-mux-simple 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>
---
.../devicetree/bindings/i2c/i2c-mux-simple.txt | 81 ++++++++++++++++++++++
1 file changed, 81 insertions(+)
create mode 100644 Documentation/devicetree/bindings/i2c/i2c-mux-simple.txt
diff --git a/Documentation/devicetree/bindings/i2c/i2c-mux-simple.txt b/Documentation/devicetree/bindings/i2c/i2c-mux-simple.txt
new file mode 100644
index 000000000000..253d5027843b
--- /dev/null
+++ b/Documentation/devicetree/bindings/i2c/i2c-mux-simple.txt
@@ -0,0 +1,81 @@
+Simple I2C Bus Mux
+
+This binding describes an I2C bus multiplexer that uses a mux controller
+from the mux subsystem to route the I2C signals.
+
+ .-----. .-----.
+ | dev | | dev |
+ .------------. '-----' '-----'
+ | SoC | | |
+ | | .--------+--------'
+ | .------. | .------+ child bus A, on MUX value set to 0
+ | | I2C |-|--| Mux |
+ | '------' | '--+---+ child bus B, on MUX value set to 1
+ | .------. | | '----------+--------+--------.
+ | | MUX- | | | | | |
+ | | Ctrl |-|-----+ .-----. .-----. .-----.
+ | '------' | | dev | | dev | | dev |
+ '------------' '-----' '-----' '-----'
+
+Required properties:
+- compatible: i2c-mux-simple,mux-locked or i2c-mux-simple,parent-locked
+- i2c-parent: The phandle of the I2C bus that this multiplexer's master-side
+ port is connected to.
+- mux-controls: The phandle of the mux controller to use for operating the
+ mux.
+* Standard I2C mux properties. See i2c-mux.txt in this directory.
+* I2C child bus nodes. See i2c-mux.txt in this directory. The sub-bus number
+ is also the mux-controller state described in ../mux/mux-controller.txt
+
+For each i2c child node, an I2C child bus will be created. They will
+be numbered based on their order in the device tree.
+
+Whenever an access is made to a device on a child bus, the value set
+in the relevant node's reg property will be set as the state in the
+mux controller.
+
+Example:
+ mux: mux-controller {
+ compatible = "mux-gpio";
+ #mux-control-cells = <0>;
+
+ mux-gpios = <&pioA 0 GPIO_ACTIVE_HIGH>,
+ <&pioA 1 GPIO_ACTIVE_HIGH>;
+ };
+
+ i2c-mux {
+ compatible = "i2c-mux-simple,mux-locked";
+ i2c-parent = <&i2c1>;
+
+ mux-controls = <&mux>;
+
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ i2c@1 {
+ reg = <1>;
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ ssd1307: oled@3c {
+ compatible = "solomon,ssd1307fb-i2c";
+ reg = <0x3c>;
+ pwms = <&pwm 4 3000>;
+ reset-gpios = <&gpio2 7 1>;
+ reset-active-low;
+ };
+ };
+
+ i2c@3 {
+ reg = <3>;
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ pca9555: pca9555@20 {
+ compatible = "nxp,pca9555";
+ gpio-controller;
+ #gpio-cells = <2>;
+ reg = <0x20>;
+ };
+ };
+ };
--
2.1.4
^ permalink raw reply related
* [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
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