* [PATCH v3] gpio: pl061: Fix the issue failed to register the ACPI interrtupion
From: Wei Xu @ 2019-08-19 13:27 UTC (permalink / raw)
To: xuwei5, linux-gpio, linux-kernel, linux-arm-kernel, linus.walleij,
rjw, lenb, mika.westerberg, andy.shevchenko
Cc: linuxarm, shameerali.kolothum.thodi, jonathan.cameron, john.garry,
salil.mehta, shiju.jose, jinying, zhangyi.ac, liguozhu,
tangkunshan, huangdaode
Invoke acpi_gpiochip_request_interrupts after the acpi data has been
attached to the pl061 acpi node to register interruption.
Otherwise it will be failed to register interruption for the ACPI case.
Because in the gpiochip_add_data_with_key, acpi_gpiochip_add is invoked
after gpiochip_add_irqchip but at that time the acpi data has not been
attached yet.
Tested with below steps:
qemu-system-aarch64 \
-machine virt,gic-version=3 -cpu cortex-a57 \
-m 1G,maxmem=4G,slots=4 \
-kernel Image -initrd rootfs.cpio.gz \
-net none -nographic \
-bios QEMU_EFI.fd \
-append "console=ttyAMA0 acpi=force earlycon=pl011,0x9000000"
The pl061 interruption is missed and the following output is not in the
/proc/interrupts on the v5.3-rc4 compared with the v5.2.0-rc7.
43: 0 ARMH0061:00 3 Edge ACPI:Event
Fixes: 04ce935c6b2a ("gpio: pl061: Pass irqchip when adding gpiochip")
Signed-off-by: Wei Xu <xuwei5@hisilicon.com>
---
v2 -> v3:
* addressed the comments of Andy to show only affected output of
/proc/interrupts and drop the whole log of v5.2.0-rc7
v1- > v2:
* rebased on https://git.kernel.org/pub/scm/linux/kernel/git/linusw/linux-gpio.git/log/?h=devel
* attached the log based on QEMU v3.0.0 and Linux kernel v5.2.0-rc7
---
drivers/gpio/gpio-pl061.c | 7 +++++++
1 file changed, 7 insertions(+)
diff --git a/drivers/gpio/gpio-pl061.c b/drivers/gpio/gpio-pl061.c
index 722ce5c..e1a434e 100644
--- a/drivers/gpio/gpio-pl061.c
+++ b/drivers/gpio/gpio-pl061.c
@@ -8,6 +8,7 @@
*
* Data sheet: ARM DDI 0190B, September 2000
*/
+#include <linux/acpi.h>
#include <linux/spinlock.h>
#include <linux/errno.h>
#include <linux/init.h>
@@ -24,6 +25,9 @@
#include <linux/pinctrl/consumer.h>
#include <linux/pm.h>
+#include "gpiolib.h"
+#include "gpiolib-acpi.h"
+
#define GPIODIR 0x400
#define GPIOIS 0x404
#define GPIOIBE 0x408
@@ -345,6 +349,9 @@ static int pl061_probe(struct amba_device *adev, const struct amba_id *id)
if (ret)
return ret;
+ if (has_acpi_companion(dev))
+ acpi_gpiochip_request_interrupts(&pl061->gc);
+
amba_set_drvdata(adev, pl061);
dev_info(dev, "PL061 GPIO chip registered\n");
--
2.8.1
^ permalink raw reply related
* Re: [PATCH v3] arm64: dts: ls1088a: fix gpio node
From: Shawn Guo @ 2019-08-19 13:20 UTC (permalink / raw)
To: Hui Song
Cc: Li Yang, Rob Herring, Mark Rutland, Linus Walleij,
Bartosz Golaszewski, linux-arm-kernel, devicetree, linux-kernel,
linux-gpio
In-Reply-To: <20190815103016.23125-1-hui.song_1@nxp.com>
On Thu, Aug 15, 2019 at 06:30:16PM +0800, Hui Song wrote:
> From: Song Hui <hui.song_1@nxp.com>
>
> Update the nodes to include little-endian
So the commit log needs an update as well?
Shawn
> property to be consistent with the hardware
> and add ls1088a gpio specify compatible.
>
> Signed-off-by: Song Hui <hui.song_1@nxp.com>
> ---
> Changes in v3:
> - delete the attribute of little-endian.
> Changes in v2:
> - update the subject.
>
> arch/arm64/boot/dts/freescale/fsl-ls1088a.dtsi | 8 ++++----
> 1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/arch/arm64/boot/dts/freescale/fsl-ls1088a.dtsi b/arch/arm64/boot/dts/freescale/fsl-ls1088a.dtsi
> index dfbead4..ff669c8 100644
> --- a/arch/arm64/boot/dts/freescale/fsl-ls1088a.dtsi
> +++ b/arch/arm64/boot/dts/freescale/fsl-ls1088a.dtsi
> @@ -269,7 +269,7 @@
> };
>
> gpio0: gpio@2300000 {
> - compatible = "fsl,qoriq-gpio";
> + compatible = "fsl,ls1088a-gpio", "fsl,qoriq-gpio";
> reg = <0x0 0x2300000 0x0 0x10000>;
> interrupts = <0 36 IRQ_TYPE_LEVEL_HIGH>;
> little-endian;
> @@ -280,7 +280,7 @@
> };
>
> gpio1: gpio@2310000 {
> - compatible = "fsl,qoriq-gpio";
> + compatible = "fsl,ls1088a-gpio", "fsl,qoriq-gpio";
> reg = <0x0 0x2310000 0x0 0x10000>;
> interrupts = <0 36 IRQ_TYPE_LEVEL_HIGH>;
> little-endian;
> @@ -291,7 +291,7 @@
> };
>
> gpio2: gpio@2320000 {
> - compatible = "fsl,qoriq-gpio";
> + compatible = "fsl,ls1088a-gpio", "fsl,qoriq-gpio";
> reg = <0x0 0x2320000 0x0 0x10000>;
> interrupts = <0 37 IRQ_TYPE_LEVEL_HIGH>;
> little-endian;
> @@ -302,7 +302,7 @@
> };
>
> gpio3: gpio@2330000 {
> - compatible = "fsl,qoriq-gpio";
> + compatible = "fsl,ls1088a-gpio", "fsl,qoriq-gpio";
> reg = <0x0 0x2330000 0x0 0x10000>;
> interrupts = <0 37 IRQ_TYPE_LEVEL_HIGH>;
> little-endian;
> --
> 2.9.5
>
^ permalink raw reply
* Re: [PATCH v2] arm64: dts: ls1088a: fix gpio node
From: Shawn Guo @ 2019-08-19 13:18 UTC (permalink / raw)
To: Hui Song
Cc: Mark Rutland, devicetree, linux-gpio, Linus Walleij, linux-kernel,
Li Yang, Bartosz Golaszewski, Rob Herring, linux-arm-kernel
In-Reply-To: <20190819114358.GX5999@X250>
On Mon, Aug 19, 2019 at 01:43:59PM +0200, Shawn Guo wrote:
> On Tue, Aug 13, 2019 at 10:04:57AM +0800, Hui Song wrote:
> > From: Song Hui <hui.song_1@nxp.com>
> >
> > Update the nodes to include little-endian
> > property to be consistent with the hardware
> > and add ls1088a gpio specify compatible.
> >
> > Signed-off-by: Song Hui <hui.song_1@nxp.com>
>
> Applied, thanks.
Just noticed there is a new version, so dropped this one.
Shawn
^ permalink raw reply
* Re: [PATCH v2] gpio: pl061: Fix the issue failed to register the ACPI interrtupion
From: Wei Xu @ 2019-08-19 12:59 UTC (permalink / raw)
To: Andy Shevchenko
Cc: open list:GPIO SUBSYSTEM, Linux Kernel Mailing List,
linux-arm Mailing List, Linus Walleij, Rafael J. Wysocki,
Len Brown, Mika Westerberg, Linuxarm, shameerali.kolothum.thodi,
Jonathan Cameron, John Garry, salil.mehta, shiju.jose, jinying,
zhangyi.ac, liguozhu, tangkunshan, huangdaode
In-Reply-To: <CAHp75VfjE4V7yY1b3JYd_Mk9-8RTok2WCN=-MMrUBw5NN90o2A@mail.gmail.com>
Hi Andy,
Thanks!
On 2019/8/16 21:40, Andy Shevchenko wrote:
> On Fri, Aug 16, 2019 at 12:07 PM Wei Xu <xuwei5@hisilicon.com> wrote:
>> Invoke acpi_gpiochip_request_interrupts after the acpi data has been
>> attached to the pl061 acpi node to register interruption.
>>
>> Otherwise it will be failed to register interruption for the ACPI case.
>> Because in the gpiochip_add_data_with_key, acpi_gpiochip_add is invoked
>> after gpiochip_add_irqchip but at that time the acpi data has not been
>> attached yet.
>> 2. cat /proc/interrupts in the guest console:
>>
>> estuary:/$ cat /proc/interrupts
>> CPU0
>> 2: 3228 GICv3 27 Level arch_timer
>> 4: 15 GICv3 33 Level uart-pl011
>> 42: 0 GICv3 23 Level arm-pmu
>> IPI0: 0 Rescheduling interrupts
>> IPI1: 0 Function call interrupts
>> IPI2: 0 CPU stop interrupts
>> IPI3: 0 CPU stop (for crash dump) interrupts
>> IPI4: 0 Timer broadcast interrupts
>> IPI5: 0 IRQ work interrupts
>> IPI6: 0 CPU wake-up interrupts
>> Err: 0
>>
>> But on QEMU v3.0.0 and Linux kernel v5.2.0-rc7, pl061 interruption is
>> there as below:
>>
>> estuary:/$ cat /proc/interrupts
>> CPU0
>> 2: 2648 GICv3 27 Level arch_timer
>> 4: 12 GICv3 33 Level uart-pl011
>> 42: 0 GICv3 23 Level arm-pmu
>> 43: 0 ARMH0061:00 3 Edge ACPI:Event
>> IPI0: 0 Rescheduling interrupts
>> IPI1: 0 Function call interrupts
>> IPI2: 0 CPU stop interrupts
>> IPI3: 0 CPU stop (for crash dump) interrupts
>> IPI4: 0 Timer broadcast interrupts
>> IPI5: 0 IRQ work interrupts
>> IPI6: 0 CPU wake-up interrupts
>> Err: 0
> In above show only affected line.
OK. Will update it in v3.
>> And the whole dmesg log on Linux kernel v5.2.0-rc7 is as below:
> NO!
> Please, remove this huge noise!
Sorry for the noise!
I will drop it in v3.
Best Regards,
Wei
^ permalink raw reply
* Re: [PATCH v2] Skip deferred request irqs for devices known to fail
From: Andy Shevchenko @ 2019-08-19 11:52 UTC (permalink / raw)
To: Ian W MORRISON
Cc: benjamin.tissoires, hdegoede, mika.westerberg, linus.walleij,
bgolaszewski, linux-gpio, linux-acpi, linux-kernel, stable
In-Reply-To: <20190819112637.29943-1-ianwmorrison@gmail.com>
On Mon, Aug 19, 2019 at 09:26:37PM +1000, Ian W MORRISON wrote:
> Patch ca876c7483b6 "gpiolib-acpi: make sure we trigger edge events at
> least once on boot" causes the MINIX family of mini PCs to fail to boot
> resulting in a "black screen".
>
> This patch excludes MINIX devices from executing this trigger in order
> to successfully boot.
Thanks for an update.
> Cc: stable@vger.kernel.org
> Signed-off-by: Ian W MORRISON <ianwmorrison@gmail.com>
> Reviewed-by: Hans de Goede <hdegoede@redhat.com>
> Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Hmm... Did I really give the tag?
Too many stuff is going on, anyway, please consider more comments below.
First of all, the subject should start from "gpiolib: acpi: " prefix.
Then, Fixes tag seems to be missed.
> +/*
> + * Run deferred acpi_gpiochip_request_irqs()
> + * but exclude devices known to fail
Missed period.
> +*/
Missed leading space (the column of stars).
> static int acpi_gpio_handle_deferred_request_irqs(void)
> {
> struct acpi_gpio_chip *acpi_gpio, *tmp;
> + const struct dmi_system_id *dmi_id;
>
> - mutex_lock(&acpi_gpio_deferred_req_irqs_lock);
> - list_for_each_entry_safe(acpi_gpio, tmp,
> + dmi_id = dmi_first_match(skip_deferred_request_irqs_table);
> + if (dmi_id)
> + return 0;
The idea of positive check is exactly for...
> + else {
...getting rid of this redundant 'else' followed by unneeded level of indentation.
> + mutex_lock(&acpi_gpio_deferred_req_irqs_lock);
> + list_for_each_entry_safe(acpi_gpio, tmp,
> &acpi_gpio_deferred_req_irqs_list,
> deferred_req_irqs_list_entry)
> - acpi_gpiochip_request_irqs(acpi_gpio);
> + acpi_gpiochip_request_irqs(acpi_gpio);
>
> - acpi_gpio_deferred_req_irqs_done = true;
> - mutex_unlock(&acpi_gpio_deferred_req_irqs_lock);
> + acpi_gpio_deferred_req_irqs_done = true;
> + mutex_unlock(&acpi_gpio_deferred_req_irqs_lock);
> + }
>
> return 0;
> }
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply
* Re: [PATCH v2] arm64: dts: ls1088a: fix gpio node
From: Shawn Guo @ 2019-08-19 11:43 UTC (permalink / raw)
To: Hui Song
Cc: Li Yang, Rob Herring, Mark Rutland, Linus Walleij,
Bartosz Golaszewski, linux-arm-kernel, devicetree, linux-kernel,
linux-gpio
In-Reply-To: <20190813020457.45370-1-hui.song_1@nxp.com>
On Tue, Aug 13, 2019 at 10:04:57AM +0800, Hui Song wrote:
> From: Song Hui <hui.song_1@nxp.com>
>
> Update the nodes to include little-endian
> property to be consistent with the hardware
> and add ls1088a gpio specify compatible.
>
> Signed-off-by: Song Hui <hui.song_1@nxp.com>
Applied, thanks.
^ permalink raw reply
* Re: [PATCH] Skip deferred request irqs for devices known to fail
From: Ian W MORRISON @ 2019-08-19 11:35 UTC (permalink / raw)
To: Hans de Goede
Cc: benjamin.tissoires, mika.westerberg, Andy Shevchenko,
Linus Walleij, bgolaszewski, linux-gpio, linux-acpi, linux-kernel,
stable
In-Reply-To: <c144cbd0-1773-14cd-62c9-6f41eab5894a@redhat.com>
Hi Hans and everyone,
On Mon, 19 Aug 2019 at 04:59, Hans de Goede <hdegoede@redhat.com> wrote:
>
> Hi Ian, et. al.,
>
> As such I guess we may need to go with the blacklist patch you suggested
> which sucks, but having these devices not boot sucks even harder.
>
> I guess this problem did not magically fix it self in the mean time
> (with newer kernels) ?
>
Unfortunately it didn't 'self-fix' with later kernels.
> Can you resubmit your patch with Andy's review remarks addressed?
>
> In case you've lost Andy's reply I will reproduce the review remarks
> below.
>
> Regards,
>
> Hans
>
Resubmitted as requested.
Many thanks and best regards,
Ian
^ permalink raw reply
* Re: [PATCH v2] Skip deferred request irqs for devices known to fail
From: Hans de Goede @ 2019-08-19 11:31 UTC (permalink / raw)
To: Ian W MORRISON, benjamin.tissoires, mika.westerberg,
andriy.shevchenko, linus.walleij, bgolaszewski
Cc: linux-gpio, linux-acpi, linux-kernel, stable
In-Reply-To: <20190819112637.29943-1-ianwmorrison@gmail.com>
Hi All,
On 19-08-19 13:26, Ian W MORRISON wrote:
> Patch ca876c7483b6 "gpiolib-acpi: make sure we trigger edge events at
> least once on boot" causes the MINIX family of mini PCs to fail to boot
> resulting in a "black screen".
>
> This patch excludes MINIX devices from executing this trigger in order
> to successfully boot.
>
> Cc: stable@vger.kernel.org
> Signed-off-by: Ian W MORRISON <ianwmorrison@gmail.com>
> Reviewed-by: Hans de Goede <hdegoede@redhat.com>
> Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Erm, you cannot just add someone's Reviewed-by tag because they have
looked at the patch, you are only supposed to do that when the reviewer
actually puts that in his reply, then you can copy the line and
add it to your commit msg. Please drop these.
Also I might be able to get my hands on a Minix Neo Z83-4 myself
in a couple of days and then I can try to reproduce this, so lets
wait a bit for that and see how that goes.
Regards,
Hans
> ---
> drivers/gpio/gpiolib-acpi.c | 33 +++++++++++++++++++++++++++------
> 1 file changed, 27 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/gpio/gpiolib-acpi.c b/drivers/gpio/gpiolib-acpi.c
> index fdee8afa5339..f6c3dcdc91c9 100644
> --- a/drivers/gpio/gpiolib-acpi.c
> +++ b/drivers/gpio/gpiolib-acpi.c
> @@ -13,6 +13,7 @@
> #include <linux/gpio/machine.h>
> #include <linux/export.h>
> #include <linux/acpi.h>
> +#include <linux/dmi.h>
> #include <linux/interrupt.h>
> #include <linux/mutex.h>
> #include <linux/pinctrl/pinctrl.h>
> @@ -20,6 +21,17 @@
> #include "gpiolib.h"
> #include "gpiolib-acpi.h"
>
> +static const struct dmi_system_id skip_deferred_request_irqs_table[] = {
> + {
> + .ident = "MINIX Z83-4",
> + .matches = {
> + DMI_EXACT_MATCH(DMI_SYS_VENDOR, "MINIX"),
> + DMI_MATCH(DMI_PRODUCT_NAME, "Z83-4"),
> + },
> + },
> + {}
> +};
> +
> /**
> * struct acpi_gpio_event - ACPI GPIO event handler data
> *
> @@ -1273,19 +1285,28 @@ bool acpi_can_fallback_to_crs(struct acpi_device *adev, const char *con_id)
> return con_id == NULL;
> }
>
> -/* Run deferred acpi_gpiochip_request_irqs() */
> +/*
> + * Run deferred acpi_gpiochip_request_irqs()
> + * but exclude devices known to fail
> +*/
> static int acpi_gpio_handle_deferred_request_irqs(void)
> {
> struct acpi_gpio_chip *acpi_gpio, *tmp;
> + const struct dmi_system_id *dmi_id;
>
> - mutex_lock(&acpi_gpio_deferred_req_irqs_lock);
> - list_for_each_entry_safe(acpi_gpio, tmp,
> + dmi_id = dmi_first_match(skip_deferred_request_irqs_table);
> + if (dmi_id)
> + return 0;
> + else {
> + mutex_lock(&acpi_gpio_deferred_req_irqs_lock);
> + list_for_each_entry_safe(acpi_gpio, tmp,
> &acpi_gpio_deferred_req_irqs_list,
> deferred_req_irqs_list_entry)
> - acpi_gpiochip_request_irqs(acpi_gpio);
> + acpi_gpiochip_request_irqs(acpi_gpio);
>
> - acpi_gpio_deferred_req_irqs_done = true;
> - mutex_unlock(&acpi_gpio_deferred_req_irqs_lock);
> + acpi_gpio_deferred_req_irqs_done = true;
> + mutex_unlock(&acpi_gpio_deferred_req_irqs_lock);
> + }
>
> return 0;
> }
>
^ permalink raw reply
* [PATCH v2] Skip deferred request irqs for devices known to fail
From: Ian W MORRISON @ 2019-08-19 11:26 UTC (permalink / raw)
To: benjamin.tissoires, hdegoede, mika.westerberg, andriy.shevchenko,
linus.walleij, bgolaszewski
Cc: linux-gpio, linux-acpi, linux-kernel, stable, Ian W MORRISON
Patch ca876c7483b6 "gpiolib-acpi: make sure we trigger edge events at
least once on boot" causes the MINIX family of mini PCs to fail to boot
resulting in a "black screen".
This patch excludes MINIX devices from executing this trigger in order
to successfully boot.
Cc: stable@vger.kernel.org
Signed-off-by: Ian W MORRISON <ianwmorrison@gmail.com>
Reviewed-by: Hans de Goede <hdegoede@redhat.com>
Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
drivers/gpio/gpiolib-acpi.c | 33 +++++++++++++++++++++++++++------
1 file changed, 27 insertions(+), 6 deletions(-)
diff --git a/drivers/gpio/gpiolib-acpi.c b/drivers/gpio/gpiolib-acpi.c
index fdee8afa5339..f6c3dcdc91c9 100644
--- a/drivers/gpio/gpiolib-acpi.c
+++ b/drivers/gpio/gpiolib-acpi.c
@@ -13,6 +13,7 @@
#include <linux/gpio/machine.h>
#include <linux/export.h>
#include <linux/acpi.h>
+#include <linux/dmi.h>
#include <linux/interrupt.h>
#include <linux/mutex.h>
#include <linux/pinctrl/pinctrl.h>
@@ -20,6 +21,17 @@
#include "gpiolib.h"
#include "gpiolib-acpi.h"
+static const struct dmi_system_id skip_deferred_request_irqs_table[] = {
+ {
+ .ident = "MINIX Z83-4",
+ .matches = {
+ DMI_EXACT_MATCH(DMI_SYS_VENDOR, "MINIX"),
+ DMI_MATCH(DMI_PRODUCT_NAME, "Z83-4"),
+ },
+ },
+ {}
+};
+
/**
* struct acpi_gpio_event - ACPI GPIO event handler data
*
@@ -1273,19 +1285,28 @@ bool acpi_can_fallback_to_crs(struct acpi_device *adev, const char *con_id)
return con_id == NULL;
}
-/* Run deferred acpi_gpiochip_request_irqs() */
+/*
+ * Run deferred acpi_gpiochip_request_irqs()
+ * but exclude devices known to fail
+*/
static int acpi_gpio_handle_deferred_request_irqs(void)
{
struct acpi_gpio_chip *acpi_gpio, *tmp;
+ const struct dmi_system_id *dmi_id;
- mutex_lock(&acpi_gpio_deferred_req_irqs_lock);
- list_for_each_entry_safe(acpi_gpio, tmp,
+ dmi_id = dmi_first_match(skip_deferred_request_irqs_table);
+ if (dmi_id)
+ return 0;
+ else {
+ mutex_lock(&acpi_gpio_deferred_req_irqs_lock);
+ list_for_each_entry_safe(acpi_gpio, tmp,
&acpi_gpio_deferred_req_irqs_list,
deferred_req_irqs_list_entry)
- acpi_gpiochip_request_irqs(acpi_gpio);
+ acpi_gpiochip_request_irqs(acpi_gpio);
- acpi_gpio_deferred_req_irqs_done = true;
- mutex_unlock(&acpi_gpio_deferred_req_irqs_lock);
+ acpi_gpio_deferred_req_irqs_done = true;
+ mutex_unlock(&acpi_gpio_deferred_req_irqs_lock);
+ }
return 0;
}
--
2.17.1
^ permalink raw reply related
* Re: [PATCH V4 2/2] gpio: inverter: document the inverter bindings
From: Harish Jenny K N @ 2019-08-19 9:36 UTC (permalink / raw)
To: Linus Walleij, Rob Herring
Cc: Bartosz Golaszewski, Mark Rutland,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
open list:GPIO SUBSYSTEM, Balasubramani Vivekanandan
In-Reply-To: <CACRpkdbmyc9LsJ2xiX=zAQR9FZ9dmwu-nPrNbt1Tgud9+rBGpw@mail.gmail.com>
Hi Rob,
On 10/08/19 2:21 PM, Linus Walleij wrote:
> On Fri, Aug 9, 2019 at 4:08 PM Rob Herring <robh+dt@kernel.org> wrote:
>> On Mon, Aug 5, 2019 at 5:15 AM Linus Walleij <linus.walleij@linaro.org> wrote:
>>> There is some level of ambition here which is inherently a bit fuzzy
>>> around the edges. ("How long is the coast of Britain?" comes to mind.)
>>>
>>> Surely the intention of device tree is not to recreate the schematic
>>> in all detail. What we want is a model of the hardware that will
>>> suffice for the operating system usecases.
>>>
>>> But sometimes the DTS files will become confusing: why is this
>>> component using GPIO_ACTIVE_LOW when another system
>>> doesn't have that flag? If there is an explicit inverter, the
>>> DTS gets more readable for a human.
>>>
>>> But arguable that is case for adding inverters as syntactic
>>> sugar in the DTS compiler instead...
>> If you really want something more explicit, then add a new GPIO
>> 'inverted' flag. Then a device can always have the same HIGH/LOW flag.
>> That also solves the abstract it for userspace problem.
> I think there are some intricate ontologies at work here.
>
> Consider this example: a GPIO is controlling a chip select
> regulator, say Acme Foo. The chip select
> has a pin named CSN. We know from convention that the
> "N" at the end of that pin name means "negative" i.e. active
> low, and that is how the electronics engineers think about
> that chip select line: it activates the IC when
> the line goes low.
>
> The regulator subsystem and I think all subsystems in the
> Linux kernel say the consumer pin should be named and
> tagged after the datsheet of the regulator.
>
> So it has for example:
>
> foo {
> compatible = "acme,foo";
> cs-gpios = <&gpio0 6 GPIO_ACTIVE_LOW>;
> };
>
> (It would be inappropriate to name it "csn-gpios" since
> we have an established flag for active low. But it is another
> of these syntactic choices where people likely do mistakes.)
>
> I think it would be appropriate for the DT binding to say
> that this flag must always be GPIO_ACTIVE_LOW since
> the bindings are seen from the component point of view,
> and thus this is always active low.
>
> It would even be reasonable for a yaml schema to enfore
> this, if it could. It is defined as active low after all.
>
> Now if someone adds an inverter on that line between
> gpio0 and Acme Foo it looks like this:
>
> foo {
> compatible = "acme,foo";
> cs-gpios = <&gpio0 6 GPIO_ACTIVE_HIGH>;
> };
>
> And now we get cognitive dissonance or whatever I should
> call it: someone reading this DTS sheet and the data
> sheet for the component Acme Foo to troubleshoot
> this will be confused: this component has CS active
> low and still it is specified as active high? Unless they
> also look at the schematic or the board and find the
> inverter things are pretty muddy and they will likely curse
> and solve the situation with the usual trial-and-error,
> inserting some random cursewords as a comment.
>
> With an intermediate inverter node, the cs-gpios
> can go back to GPIO_ACTIVE_LOW and follow
> the bindings:
>
> inv0: inverter {
> compatible = "gpio-inverter";
> gpio-controller;
> #gpio-cells = <1>;
> inverted-gpios = <&gpio0 6 GPIO_ACTIVE_HIGH>;
> };
>
> foo {
> compatible = "acme,foo";
> cs-gpios = <&inv0 0 GPIO_ACTIVE_LOW>;
> };
>
> And now Acme Foo bindings can keep enforcing cs-gpios
> to always be tagged GPIO_ACTIVE_LOW.
Can you please review/let us know your opinion on this ? I think the idea here is to also isolate the changes to a separate consumer driver and avoid getting inversions inside gpiolib.
Thanks.
Regards,
Harish Jenny K N
^ permalink raw reply
* Re: [PATCH] pinctrl: intel: remap the pin number to gpio offset for irq enabled pin
From: Andy Shevchenko @ 2019-08-19 9:42 UTC (permalink / raw)
To: Mika Westerberg
Cc: Chris Chiu, linus.walleij, linux-gpio, linux-kernel, linux
In-Reply-To: <20190819071413.GI19908@lahna.fi.intel.com>
On Mon, Aug 19, 2019 at 10:14:13AM +0300, Mika Westerberg wrote:
> On Fri, Aug 16, 2019 at 05:38:38PM +0800, Chris Chiu wrote:
> > On Asus X571GT, GPIO 297 is configured as an interrupt and serves
> > for the touchpad. The touchpad will report input events much less
> > than expected after S3 suspend/resume, which results in extremely
> > slow cursor movement. However, the number of interrupts observed
> > from /proc/interrupts increases much more than expected even no
> > touching touchpad.
> >
> > This is due to the value of PADCFG0 of PIN 225 for the interrupt
> > has been changed from 0x80800102 to 0x80100102. The GPIROUTIOXAPIC
> > is toggled on which results in the spurious interrupts. The PADCFG0
> > of PIN 225 is expected to be saved during suspend, but the 297 is
> > saved instead because the gpiochip_line_is_irq() expect the GPIO
> > offset but what's really passed to it is PIN number. In this case,
> > the /sys/kernel/debug/pinctrl/INT3450:00/gpio-ranges shows
> >
> > 288: INT3450:00 GPIOS [436 - 459] PINS [216 - 239]
> >
> > So gpiochip_line_is_irq() returns true for GPIO offset 297, the
> > suspend routine spuriously saves the content for PIN 297 which
> > we expect to save for PIN 225.
>
> Nice work nailing the issue!
>
> > This commit maps the PIN number to GPIO offset first in the
> > intel_pinctrl_should_save() to make sure the values for the
> > specific PINs can be correctly saved and then restored.
> >
> > Signed-off-by: Chris Chiu <chiu@endlessm.com>
>
> Acked-by: Mika Westerberg <mika.westerberg@linux.intel.com>
>
> I think this should also have:
>
> Fixes: c538b9436751 ("pinctrl: intel: Only restore pins that are used by the driver")
Pushed to my review and testing queue, thanks!
P.S. I have added Fixes tag.
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply
* [PATCH] gpio: Use callback presence to determine need of valid_mask
From: Linus Walleij @ 2019-08-19 9:30 UTC (permalink / raw)
To: linux-gpio
Cc: Bartosz Golaszewski, Linus Walleij, Benjamin Gaignard,
Amelie Delaunay, Stephen Boyd, Bjorn Andersson
After we switched the two drivers that have .need_valid_mask
set to use the callback for setting up the .valid_mask,
we can just use the presence of the .init_valid_mask()
callback (or the OF reserved ranges, nota bene) to determine
whether to allocate the mask or not and we can drop the
.need_valid_mask field altogether.
Cc: Benjamin Gaignard <benjamin.gaignard@st.com>
Cc: Amelie Delaunay <amelie.delaunay@st.com>
Cc: Stephen Boyd <swboyd@chromium.org>
Cc: Bjorn Andersson <bjorn.andersson@linaro.org>
Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
drivers/gpio/gpiolib.c | 4 +---
drivers/pinctrl/pinctrl-stmfx.c | 1 -
drivers/pinctrl/qcom/pinctrl-msm.c | 4 ++--
include/linux/gpio/driver.h | 9 ---------
4 files changed, 3 insertions(+), 15 deletions(-)
diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index 22b87c6e8cd5..01aa5440454c 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -363,9 +363,7 @@ static unsigned long *gpiochip_allocate_mask(struct gpio_chip *chip)
static int gpiochip_alloc_valid_mask(struct gpio_chip *gc)
{
- if (of_gpio_need_valid_mask(gc))
- gc->need_valid_mask = true;
- if (!gc->need_valid_mask)
+ if (!(of_gpio_need_valid_mask(gc) || gc->init_valid_mask))
return 0;
gc->valid_mask = gpiochip_allocate_mask(gc);
diff --git a/drivers/pinctrl/pinctrl-stmfx.c b/drivers/pinctrl/pinctrl-stmfx.c
index 13b6d6f72bcc..dd5aa9a2dfe5 100644
--- a/drivers/pinctrl/pinctrl-stmfx.c
+++ b/drivers/pinctrl/pinctrl-stmfx.c
@@ -662,7 +662,6 @@ static int stmfx_pinctrl_probe(struct platform_device *pdev)
pctl->gpio_chip.ngpio = pctl->pctl_desc.npins;
pctl->gpio_chip.can_sleep = true;
pctl->gpio_chip.of_node = np;
- pctl->gpio_chip.need_valid_mask = true;
pctl->gpio_chip.init_valid_mask = stmfx_pinctrl_gpio_init_valid_mask;
ret = devm_gpiochip_add_data(pctl->dev, &pctl->gpio_chip, pctl);
diff --git a/drivers/pinctrl/qcom/pinctrl-msm.c b/drivers/pinctrl/qcom/pinctrl-msm.c
index a5d8f75da4a7..b8a1c43222f8 100644
--- a/drivers/pinctrl/qcom/pinctrl-msm.c
+++ b/drivers/pinctrl/qcom/pinctrl-msm.c
@@ -654,7 +654,6 @@ static const struct gpio_chip msm_gpio_template = {
.request = gpiochip_generic_request,
.free = gpiochip_generic_free,
.dbg_show = msm_gpio_dbg_show,
- .init_valid_mask = msm_gpio_init_valid_mask,
};
/* For dual-edge interrupts in software, since some hardware has no
@@ -1016,7 +1015,8 @@ static int msm_gpio_init(struct msm_pinctrl *pctrl)
chip->parent = pctrl->dev;
chip->owner = THIS_MODULE;
chip->of_node = pctrl->dev->of_node;
- chip->need_valid_mask = msm_gpio_needs_valid_mask(pctrl);
+ if (msm_gpio_needs_valid_mask(pctrl))
+ chip->init_valid_mask = msm_gpio_init_valid_mask;
pctrl->irq_chip.name = "msmgpio";
pctrl->irq_chip.irq_enable = msm_gpio_irq_enable;
diff --git a/include/linux/gpio/driver.h b/include/linux/gpio/driver.h
index dc03323897ef..340121c7d2fb 100644
--- a/include/linux/gpio/driver.h
+++ b/include/linux/gpio/driver.h
@@ -403,15 +403,6 @@ struct gpio_chip {
struct gpio_irq_chip irq;
#endif /* CONFIG_GPIOLIB_IRQCHIP */
- /**
- * @need_valid_mask:
- *
- * If set core allocates @valid_mask with all its values initialized
- * with init_valid_mask() or set to one if init_valid_mask() is not
- * defined
- */
- bool need_valid_mask;
-
/**
* @valid_mask:
*
--
2.21.0
^ permalink raw reply related
* [PATCH] pinctrl: stmfx: Use the callback to populate valid_mask
From: Linus Walleij @ 2019-08-19 9:11 UTC (permalink / raw)
To: linux-gpio
Cc: Bartosz Golaszewski, Linus Walleij, Benjamin Gaignard,
Amelie Delaunay
This makes use of the existing callback to populate the
valid mask instead of iteratively setting it up during
probe.
Cc: Benjamin Gaignard <benjamin.gaignard@st.com>
Cc: Amelie Delaunay <amelie.delaunay@st.com>
Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
Testing this requires the patch:
"gpio: Pass mask and size with the init_valid_mask()"
that I just sent out.
---
drivers/pinctrl/pinctrl-stmfx.c | 17 ++++++++++++++---
1 file changed, 14 insertions(+), 3 deletions(-)
diff --git a/drivers/pinctrl/pinctrl-stmfx.c b/drivers/pinctrl/pinctrl-stmfx.c
index d3332da35637..13b6d6f72bcc 100644
--- a/drivers/pinctrl/pinctrl-stmfx.c
+++ b/drivers/pinctrl/pinctrl-stmfx.c
@@ -585,12 +585,24 @@ static int stmfx_pinctrl_gpio_function_enable(struct stmfx_pinctrl *pctl)
return stmfx_function_enable(pctl->stmfx, func);
}
+static int stmfx_pinctrl_gpio_init_valid_mask(struct gpio_chip *gc,
+ unsigned long *valid_mask,
+ unsigned int ngpios)
+{
+ struct stmfx_pinctrl *pctl = gpiochip_get_data(gc);
+ u32 n;
+
+ for_each_clear_bit(n, &pctl->gpio_valid_mask, ngpios)
+ clear_bit(n, valid_mask);
+
+ return 0;
+}
+
static int stmfx_pinctrl_probe(struct platform_device *pdev)
{
struct stmfx *stmfx = dev_get_drvdata(pdev->dev.parent);
struct device_node *np = pdev->dev.of_node;
struct stmfx_pinctrl *pctl;
- u32 n;
int irq, ret;
pctl = devm_kzalloc(stmfx->dev, sizeof(*pctl), GFP_KERNEL);
@@ -651,6 +663,7 @@ static int stmfx_pinctrl_probe(struct platform_device *pdev)
pctl->gpio_chip.can_sleep = true;
pctl->gpio_chip.of_node = np;
pctl->gpio_chip.need_valid_mask = true;
+ pctl->gpio_chip.init_valid_mask = stmfx_pinctrl_gpio_init_valid_mask;
ret = devm_gpiochip_add_data(pctl->dev, &pctl->gpio_chip, pctl);
if (ret) {
@@ -668,8 +681,6 @@ static int stmfx_pinctrl_probe(struct platform_device *pdev)
pctl->irq_chip.irq_set_type = stmfx_pinctrl_irq_set_type;
pctl->irq_chip.irq_bus_lock = stmfx_pinctrl_irq_bus_lock;
pctl->irq_chip.irq_bus_sync_unlock = stmfx_pinctrl_irq_bus_sync_unlock;
- for_each_clear_bit(n, &pctl->gpio_valid_mask, pctl->gpio_chip.ngpio)
- clear_bit(n, pctl->gpio_chip.valid_mask);
ret = gpiochip_irqchip_add_nested(&pctl->gpio_chip, &pctl->irq_chip,
0, handle_bad_irq, IRQ_TYPE_NONE);
--
2.21.0
^ permalink raw reply related
* Re: [PATCH] gpio: ftgpio: Move hardware initialization
From: Phil Reid @ 2019-08-19 9:02 UTC (permalink / raw)
To: Linus Walleij, linux-gpio; +Cc: Bartosz Golaszewski
In-Reply-To: <20190819082704.14237-1-linus.walleij@linaro.org>
On 19/08/2019 16:27, Linus Walleij wrote:
> It is probably wise to initialize the hardware before registering
> the irq chip.
>
> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
> ---
> drivers/gpio/gpio-ftgpio010.c | 8 ++++----
> 1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpio/gpio-ftgpio010.c b/drivers/gpio/gpio-ftgpio010.c
> index 250e71f3e688..3118d282514a 100644
> --- a/drivers/gpio/gpio-ftgpio010.c
> +++ b/drivers/gpio/gpio-ftgpio010.c
> @@ -296,10 +296,6 @@ static int ftgpio_gpio_probe(struct platform_device *pdev)
> girq->handler = handle_bad_irq;
> girq->parents[0] = irq;
>
> - ret = devm_gpiochip_add_data(dev, &g->gc, g);
> - if (ret)
> - goto dis_clk;
> -
> /* Disable, unmask and clear all interrupts */
> writel(0x0, g->base + GPIO_INT_EN);
> writel(0x0, g->base + GPIO_INT_MASK);
> @@ -308,6 +304,10 @@ static int ftgpio_gpio_probe(struct platform_device *pdev)
> /* Clear any use of debounce */
> writel(0x0, g->base + GPIO_DEBOUNCE_EN);
>
> + ret = devm_gpiochip_add_data(dev, &g->gc, g);
> + if (ret)
> + goto dis_clk;
> +
> platform_set_drvdata(pdev, g);
Should it also be after platform_set_drvdata?
But it doesn't look like it's used anywhere other than in _remove
so I guess it does't matter.
> dev_info(dev, "FTGPIO010 @%p registered\n", g->base);
>
>
--
Regards
Phil Reid
^ permalink raw reply
* [PATCH] gpio: Pass mask and size with the init_valid_mask()
From: Linus Walleij @ 2019-08-19 8:49 UTC (permalink / raw)
To: linux-gpio
Cc: Bartosz Golaszewski, Linus Walleij, Stephen Boyd, Bjorn Andersson
It is more helpful for drivers to have the affected fields
directly available when we use the callback to set up the
valid mask. Change this and switch over the only user
(MSM) to use the passed parameters. If we do this we can
also move the mask out of publicly visible struct fields.
Cc: Stephen Boyd <swboyd@chromium.org>
Cc: Bjorn Andersson <bjorn.andersson@linaro.org>
Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
drivers/gpio/gpiolib.c | 8 +++++---
drivers/pinctrl/qcom/pinctrl-msm.c | 19 ++++++++++---------
include/linux/gpio/driver.h | 4 +++-
3 files changed, 18 insertions(+), 13 deletions(-)
diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index 5277b8f1ff7c..22b87c6e8cd5 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -375,10 +375,12 @@ static int gpiochip_alloc_valid_mask(struct gpio_chip *gc)
return 0;
}
-static int gpiochip_init_valid_mask(struct gpio_chip *gpiochip)
+static int gpiochip_init_valid_mask(struct gpio_chip *gc)
{
- if (gpiochip->init_valid_mask)
- return gpiochip->init_valid_mask(gpiochip);
+ if (gc->init_valid_mask)
+ return gc->init_valid_mask(gc,
+ gc->valid_mask,
+ gc->ngpio);
return 0;
}
diff --git a/drivers/pinctrl/qcom/pinctrl-msm.c b/drivers/pinctrl/qcom/pinctrl-msm.c
index 7f35c196bb3e..a5d8f75da4a7 100644
--- a/drivers/pinctrl/qcom/pinctrl-msm.c
+++ b/drivers/pinctrl/qcom/pinctrl-msm.c
@@ -593,24 +593,25 @@ static void msm_gpio_dbg_show(struct seq_file *s, struct gpio_chip *chip)
#define msm_gpio_dbg_show NULL
#endif
-static int msm_gpio_init_valid_mask(struct gpio_chip *chip)
+static int msm_gpio_init_valid_mask(struct gpio_chip *gc,
+ unsigned long *valid_mask,
+ unsigned int ngpios)
{
- struct msm_pinctrl *pctrl = gpiochip_get_data(chip);
+ struct msm_pinctrl *pctrl = gpiochip_get_data(gc);
int ret;
unsigned int len, i;
- unsigned int max_gpios = pctrl->soc->ngpios;
const int *reserved = pctrl->soc->reserved_gpios;
u16 *tmp;
/* Driver provided reserved list overrides DT and ACPI */
if (reserved) {
- bitmap_fill(chip->valid_mask, max_gpios);
+ bitmap_fill(valid_mask, ngpios);
for (i = 0; reserved[i] >= 0; i++) {
- if (i >= max_gpios || reserved[i] >= max_gpios) {
+ if (i >= ngpios || reserved[i] >= ngpios) {
dev_err(pctrl->dev, "invalid list of reserved GPIOs\n");
return -EINVAL;
}
- clear_bit(reserved[i], chip->valid_mask);
+ clear_bit(reserved[i], valid_mask);
}
return 0;
@@ -622,7 +623,7 @@ static int msm_gpio_init_valid_mask(struct gpio_chip *chip)
if (ret < 0)
return 0;
- if (ret > max_gpios)
+ if (ret > ngpios)
return -EINVAL;
tmp = kmalloc_array(len, sizeof(*tmp), GFP_KERNEL);
@@ -635,9 +636,9 @@ static int msm_gpio_init_valid_mask(struct gpio_chip *chip)
goto out;
}
- bitmap_zero(chip->valid_mask, max_gpios);
+ bitmap_zero(valid_mask, ngpios);
for (i = 0; i < len; i++)
- set_bit(tmp[i], chip->valid_mask);
+ set_bit(tmp[i], valid_mask);
out:
kfree(tmp);
diff --git a/include/linux/gpio/driver.h b/include/linux/gpio/driver.h
index 72d48a2bab65..dc03323897ef 100644
--- a/include/linux/gpio/driver.h
+++ b/include/linux/gpio/driver.h
@@ -363,7 +363,9 @@ struct gpio_chip {
void (*dbg_show)(struct seq_file *s,
struct gpio_chip *chip);
- int (*init_valid_mask)(struct gpio_chip *chip);
+ int (*init_valid_mask)(struct gpio_chip *chip,
+ unsigned long *valid_mask,
+ unsigned int ngpios);
int base;
u16 ngpio;
--
2.21.0
^ permalink raw reply related
* [PATCH] gpio: ftgpio: Move hardware initialization
From: Linus Walleij @ 2019-08-19 8:27 UTC (permalink / raw)
To: linux-gpio; +Cc: Bartosz Golaszewski, Linus Walleij
It is probably wise to initialize the hardware before registering
the irq chip.
Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
drivers/gpio/gpio-ftgpio010.c | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/drivers/gpio/gpio-ftgpio010.c b/drivers/gpio/gpio-ftgpio010.c
index 250e71f3e688..3118d282514a 100644
--- a/drivers/gpio/gpio-ftgpio010.c
+++ b/drivers/gpio/gpio-ftgpio010.c
@@ -296,10 +296,6 @@ static int ftgpio_gpio_probe(struct platform_device *pdev)
girq->handler = handle_bad_irq;
girq->parents[0] = irq;
- ret = devm_gpiochip_add_data(dev, &g->gc, g);
- if (ret)
- goto dis_clk;
-
/* Disable, unmask and clear all interrupts */
writel(0x0, g->base + GPIO_INT_EN);
writel(0x0, g->base + GPIO_INT_MASK);
@@ -308,6 +304,10 @@ static int ftgpio_gpio_probe(struct platform_device *pdev)
/* Clear any use of debounce */
writel(0x0, g->base + GPIO_DEBOUNCE_EN);
+ ret = devm_gpiochip_add_data(dev, &g->gc, g);
+ if (ret)
+ goto dis_clk;
+
platform_set_drvdata(pdev, g);
dev_info(dev, "FTGPIO010 @%p registered\n", g->base);
--
2.21.0
^ permalink raw reply related
* Re: [PATCH 2/2] irq/irq_sim: use irq domain
From: Marc Zyngier @ 2019-08-19 8:18 UTC (permalink / raw)
To: Bartosz Golaszewski
Cc: Linus Walleij, Jonathan Cameron, Hartmut Knaack,
Lars-Peter Clausen, Peter Meerwald-Stadler, Thomas Gleixner,
linux-gpio, linux-kernel, linux-iio, Bartosz Golaszewski
In-Reply-To: <20190812125256.9690-3-brgl@bgdev.pl>
On Mon, 12 Aug 2019 13:52:56 +0100,
Bartosz Golaszewski <brgl@bgdev.pl> wrote:
>
> From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
>
> We currently have a dedicated function to map the interrupt simulator
> offsets to global interrupt numbers. This is something that irq_domain
> should handle.
>
> Create a linear irq_domain when initializing the interrupt simulator
> and modify the irq_sim_fire() function to only take as parameter the
> global interrupt number.
>
> Convert both users in the same patch to using the new interface.
>
> Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> ---
> drivers/gpio/gpio-mockup.c | 11 ++-
> drivers/iio/dummy/iio_dummy_evgen.c | 22 ++++--
> include/linux/irq_sim.h | 5 +-
> kernel/irq/Kconfig | 1 +
> kernel/irq/irq_sim.c | 110 +++++++++++++++++-----------
> 5 files changed, 94 insertions(+), 55 deletions(-)
>
> diff --git a/drivers/gpio/gpio-mockup.c b/drivers/gpio/gpio-mockup.c
> index 9b28ffec5826..4cf594f0e7cd 100644
> --- a/drivers/gpio/gpio-mockup.c
> +++ b/drivers/gpio/gpio-mockup.c
> @@ -186,7 +186,7 @@ static int gpio_mockup_to_irq(struct gpio_chip *gc, unsigned int offset)
> {
> struct gpio_mockup_chip *chip = gpiochip_get_data(gc);
>
> - return irq_sim_irqnum(chip->irqsim, offset);
> + return irq_create_mapping(irq_sim_get_domain(chip->irqsim), offset);
> }
>
> static void gpio_mockup_free(struct gpio_chip *gc, unsigned int offset)
> @@ -228,6 +228,7 @@ static ssize_t gpio_mockup_debugfs_write(struct file *file,
> struct gpio_mockup_dbgfs_private *priv;
> int rv, val, curr, irq, irq_type;
> struct gpio_mockup_chip *chip;
> + struct irq_domain *domain;
> struct seq_file *sfile;
> struct gpio_desc *desc;
> struct gpio_chip *gc;
> @@ -248,6 +249,7 @@ static ssize_t gpio_mockup_debugfs_write(struct file *file,
> gc = &chip->gc;
> desc = &gc->gpiodev->descs[priv->offset];
> sim = chip->irqsim;
> + domain = irq_sim_get_domain(sim);
>
> mutex_lock(&chip->lock);
>
> @@ -257,12 +259,15 @@ static ssize_t gpio_mockup_debugfs_write(struct file *file,
> if (curr == val)
> goto out;
>
> - irq = irq_sim_irqnum(sim, priv->offset);
> + irq = irq_find_mapping(domain, priv->offset);
> + if (!irq)
> + return -ENOENT;
> +
> irq_type = irq_get_trigger_type(irq);
>
> if ((val == 1 && (irq_type & IRQ_TYPE_EDGE_RISING)) ||
> (val == 0 && (irq_type & IRQ_TYPE_EDGE_FALLING)))
> - irq_sim_fire(sim, priv->offset);
> + irq_sim_fire(irq);
> }
>
> /* Change the value unless we're actively driving the line. */
> diff --git a/drivers/iio/dummy/iio_dummy_evgen.c b/drivers/iio/dummy/iio_dummy_evgen.c
> index efbcd4a5609e..cc827f60a535 100644
> --- a/drivers/iio/dummy/iio_dummy_evgen.c
> +++ b/drivers/iio/dummy/iio_dummy_evgen.c
> @@ -31,14 +31,13 @@
> * @lock: protect the evgen state
> * @inuse: mask of which irqs are connected
> * @irq_sim: interrupt simulator
> - * @base: base of irq range
> */
> struct iio_dummy_eventgen {
> struct iio_dummy_regs regs[IIO_EVENTGEN_NO];
> struct mutex lock;
> bool inuse[IIO_EVENTGEN_NO];
> struct irq_sim *irq_sim;
> - int base;
> + struct irq_domain *domain;
> };
>
> /* We can only ever have one instance of this 'device' */
> @@ -56,7 +55,7 @@ static int iio_dummy_evgen_create(void)
> return PTR_ERR(iio_evgen->irq_sim);
> }
>
> - iio_evgen->base = irq_sim_irqnum(iio_evgen->irq_sim, 0);
> + iio_evgen->domain = irq_sim_get_domain(iio_evgen->irq_sim);
> mutex_init(&iio_evgen->lock);
>
> return 0;
> @@ -78,7 +77,7 @@ int iio_dummy_evgen_get_irq(void)
> mutex_lock(&iio_evgen->lock);
> for (i = 0; i < IIO_EVENTGEN_NO; i++) {
> if (!iio_evgen->inuse[i]) {
> - ret = irq_sim_irqnum(iio_evgen->irq_sim, i);
> + ret = irq_create_mapping(iio_evgen->domain, i);
> iio_evgen->inuse[i] = true;
> break;
> }
> @@ -99,15 +98,21 @@ EXPORT_SYMBOL_GPL(iio_dummy_evgen_get_irq);
> */
> void iio_dummy_evgen_release_irq(int irq)
> {
> + struct irq_data *irqd;
> +
> + irqd = irq_get_irq_data(irq);
> +
> mutex_lock(&iio_evgen->lock);
> - iio_evgen->inuse[irq - iio_evgen->base] = false;
> + iio_evgen->inuse[irqd_to_hwirq(irqd)] = false;
> mutex_unlock(&iio_evgen->lock);
> }
> EXPORT_SYMBOL_GPL(iio_dummy_evgen_release_irq);
>
> struct iio_dummy_regs *iio_dummy_evgen_get_regs(int irq)
> {
> - return &iio_evgen->regs[irq - iio_evgen->base];
> + struct irq_data *irqd = irq_get_irq_data(irq);
> +
> + return &iio_evgen->regs[irqd_to_hwirq(irqd)];
> }
> EXPORT_SYMBOL_GPL(iio_dummy_evgen_get_regs);
>
> @@ -129,7 +134,7 @@ static ssize_t iio_evgen_poke(struct device *dev,
> {
> struct iio_dev_attr *this_attr = to_iio_dev_attr(attr);
> unsigned long event;
> - int ret;
> + int ret, irq;
>
> ret = kstrtoul(buf, 10, &event);
> if (ret)
> @@ -138,7 +143,8 @@ static ssize_t iio_evgen_poke(struct device *dev,
> iio_evgen->regs[this_attr->address].reg_id = this_attr->address;
> iio_evgen->regs[this_attr->address].reg_data = event;
>
> - irq_sim_fire(iio_evgen->irq_sim, this_attr->address);
> + irq = irq_create_mapping(iio_evgen->domain, this_attr->address);
> + irq_sim_fire(irq);
That's not the way the API is intended to be used. irq_create_mapping() is
something that should only happen once, or at least as rarely as possible.
At interrupt handling time, you should call irq_find_mapping() instead.
You need to rethink the way this driver is using interrupts.
>
> return len;
> }
> diff --git a/include/linux/irq_sim.h b/include/linux/irq_sim.h
> index 4bbf036145e2..4056d0e7f0b4 100644
> --- a/include/linux/irq_sim.h
> +++ b/include/linux/irq_sim.h
> @@ -7,6 +7,7 @@
> #define _LINUX_IRQ_SIM_H
>
> #include <linux/irq_work.h>
> +#include <linux/irqdomain.h>
> #include <linux/device.h>
>
> /*
> @@ -19,7 +20,7 @@ struct irq_sim;
> struct irq_sim *irq_sim_new(unsigned int num_irqs);
> struct irq_sim *devm_irq_sim_new(struct device *dev, unsigned int num_irqs);
> void irq_sim_free(struct irq_sim *sim);
> -void irq_sim_fire(struct irq_sim *sim, unsigned int offset);
> -int irq_sim_irqnum(struct irq_sim *sim, unsigned int offset);
> +void irq_sim_fire(int virq);
> +struct irq_domain *irq_sim_get_domain(struct irq_sim *sim);
>
> #endif /* _LINUX_IRQ_SIM_H */
> diff --git a/kernel/irq/Kconfig b/kernel/irq/Kconfig
> index f92d9a687372..d0890f7729d4 100644
> --- a/kernel/irq/Kconfig
> +++ b/kernel/irq/Kconfig
> @@ -68,6 +68,7 @@ config IRQ_DOMAIN
> config IRQ_SIM
> bool
> select IRQ_WORK
> + select IRQ_DOMAIN
>
> # Support for hierarchical irq domains
> config IRQ_DOMAIN_HIERARCHY
> diff --git a/kernel/irq/irq_sim.c b/kernel/irq/irq_sim.c
> index 79f0a6494b6c..a1c91aefb6cd 100644
> --- a/kernel/irq/irq_sim.c
> +++ b/kernel/irq/irq_sim.c
> @@ -15,13 +15,14 @@ struct irq_sim_work_ctx {
> struct irq_sim_irq_ctx {
> int irqnum;
> bool enabled;
> + struct irq_sim_work_ctx *work_ctx;
> };
>
> struct irq_sim {
> struct irq_sim_work_ctx work_ctx;
> int irq_base;
> unsigned int irq_count;
> - struct irq_sim_irq_ctx *irqs;
> + struct irq_domain *domain;
> };
>
> struct irq_sim_devres {
> @@ -74,11 +75,46 @@ static void irq_sim_handle_irq(struct irq_work *work)
> offset = find_next_bit(work_ctx->pending,
> sim->irq_count, offset);
> clear_bit(offset, work_ctx->pending);
> - irqnum = irq_sim_irqnum(sim, offset);
> + irqnum = irq_find_mapping(sim->domain, offset);
> handle_simple_irq(irq_to_desc(irqnum));
> }
> }
>
> +static int irq_sim_domain_map(struct irq_domain *domain,
> + unsigned int virq, irq_hw_number_t hw)
> +{
> + struct irq_sim *sim = domain->host_data;
> + struct irq_sim_irq_ctx *ctx;
> +
> + ctx = kzalloc(sizeof(*ctx), GFP_KERNEL);
> + if (!ctx)
> + return -ENOMEM;
> +
> + irq_set_chip(virq, &irq_sim_irqchip);
> + irq_set_chip_data(virq, ctx);
> + irq_set_handler(virq, handle_simple_irq);
> + irq_modify_status(virq, IRQ_NOREQUEST | IRQ_NOAUTOEN, IRQ_NOPROBE);
> + ctx->work_ctx = &sim->work_ctx;
> +
> + return 0;
> +}
> +
> +static void irq_sim_domain_unmap(struct irq_domain *domain, unsigned int virq)
> +{
> + struct irq_sim_irq_ctx *ctx;
> + struct irq_data *irqd;
> +
> + irqd = irq_domain_get_irq_data(domain, virq);
> + ctx = irq_data_get_irq_chip_data(irqd);
> +
> + kfree(ctx);
I'd expect something like:
irq_set_handler(virq, NULL);
irq_domain_reset_irq_data(irqd);
in addition to the above.
> +}
> +
> +static const struct irq_domain_ops irq_sim_domain_ops = {
> + .map = irq_sim_domain_map,
> + .unmap = irq_sim_domain_unmap,
> +};
> +
> /**
> * irq_sim_new - Create a new interrupt simulator: allocate a range of
> * dummy interrupts.
> @@ -91,42 +127,21 @@ static void irq_sim_handle_irq(struct irq_work *work)
> struct irq_sim *irq_sim_new(unsigned int num_irqs)
> {
> struct irq_sim *sim;
> - int i;
>
> sim = kmalloc(sizeof(*sim), GFP_KERNEL);
> if (!sim)
> return ERR_PTR(-ENOMEM);
>
> - sim->irqs = kmalloc_array(num_irqs, sizeof(*sim->irqs), GFP_KERNEL);
> - if (!sim->irqs) {
> - kfree(sim);
> - return ERR_PTR(-ENOMEM);
> - }
> -
> - sim->irq_base = irq_alloc_descs(-1, 0, num_irqs, 0);
> - if (sim->irq_base < 0) {
> - kfree(sim->irqs);
> - kfree(sim);
> - return ERR_PTR(sim->irq_base);
> - }
> -
> sim->work_ctx.pending = bitmap_zalloc(num_irqs, GFP_KERNEL);
> if (!sim->work_ctx.pending) {
> - kfree(sim->irqs);
> kfree(sim);
> - irq_free_descs(sim->irq_base, num_irqs);
> return ERR_PTR(-ENOMEM);
> }
>
> - for (i = 0; i < num_irqs; i++) {
> - sim->irqs[i].irqnum = sim->irq_base + i;
> - sim->irqs[i].enabled = false;
> - irq_set_chip(sim->irq_base + i, &irq_sim_irqchip);
> - irq_set_chip_data(sim->irq_base + i, &sim->irqs[i]);
> - irq_set_handler(sim->irq_base + i, &handle_simple_irq);
> - irq_modify_status(sim->irq_base + i,
> - IRQ_NOREQUEST | IRQ_NOAUTOEN, IRQ_NOPROBE);
> - }
> + sim->domain = irq_domain_create_linear(NULL, num_irqs,
> + &irq_sim_domain_ops, sim);
> + if (!sim->domain)
> + return ERR_PTR(-ENOMEM);
Aren't you now leaking memory from the sim structure?
>
> init_irq_work(&sim->work_ctx.work, irq_sim_handle_irq);
> sim->irq_count = num_irqs;
> @@ -143,10 +158,17 @@ EXPORT_SYMBOL_GPL(irq_sim_new);
> */
> void irq_sim_free(struct irq_sim *sim)
> {
> + int i, irq;
> +
> + for (i = 0; i < sim->irq_count; i++) {
> + irq = irq_find_mapping(sim->domain, i);
> + if (irq)
> + irq_dispose_mapping(irq);
> + }
> +
> + irq_domain_remove(sim->domain);
> irq_work_sync(&sim->work_ctx.work);
> bitmap_free(sim->work_ctx.pending);
> - irq_free_descs(sim->irq_base, sim->irq_count);
> - kfree(sim->irqs);
> kfree(sim);
> }
> EXPORT_SYMBOL_GPL(irq_sim_free);
> @@ -189,27 +211,31 @@ EXPORT_SYMBOL_GPL(devm_irq_sim_new);
> /**
> * irq_sim_fire - Enqueue an interrupt.
> *
> - * @sim: The interrupt simulator object.
> - * @offset: Offset of the simulated interrupt which should be fired.
> + * @virq: Virtual interrupt number to fire. It must be associated with
> + * an existing interrupt simulator.
> */
> -void irq_sim_fire(struct irq_sim *sim, unsigned int offset)
> +void irq_sim_fire(int virq)
> {
> - if (sim->irqs[offset].enabled) {
> - set_bit(offset, sim->work_ctx.pending);
> - irq_work_queue(&sim->work_ctx.work);
> + struct irq_sim_irq_ctx *ctx;
> + struct irq_data *irqd;
> +
> + irqd = irq_get_irq_data(virq);
> + ctx = irq_data_get_irq_chip_data(irqd);
> +
> + if (ctx->enabled) {
> + set_bit(irqd_to_hwirq(irqd), ctx->work_ctx->pending);
> + irq_work_queue(&ctx->work_ctx->work);
> }
> }
> EXPORT_SYMBOL_GPL(irq_sim_fire);
>
> /**
> - * irq_sim_irqnum - Get the allocated number of a dummy interrupt.
> + * irq_sim_get_domain - Retrieve the interrupt domain for this simulator.
> *
> - * @sim: The interrupt simulator object.
> - * @offset: Offset of the simulated interrupt for which to retrieve
> - * the number.
> + * @sim: The interrupt simulator the domain of which we retrieve.
> */
> -int irq_sim_irqnum(struct irq_sim *sim, unsigned int offset)
> +struct irq_domain *irq_sim_get_domain(struct irq_sim *sim)
> {
> - return sim->irqs[offset].irqnum;
> + return sim->domain;
> }
> -EXPORT_SYMBOL_GPL(irq_sim_irqnum);
> +EXPORT_SYMBOL(irq_sim_get_domain);
This seems to be reinventing irq_find_matching_fwnode(). Please consider
allocating a fwnode as part of the irq_sim structure, and use this fwnode
to lookup the domain instead.
M.
--
Jazz is not dead, it just smells funny.
^ permalink raw reply
* [PATCH] pinctrl: rk805: Make structures constant
From: Nishka Dasgupta @ 2019-08-19 7:57 UTC (permalink / raw)
To: linus.walleij, linux-gpio; +Cc: Nishka Dasgupta
Static structures rk805_pinctrl_desc and rk805_gpio_chip, of types
gpio_chip and pinctrl_desc respectively, are not used except to be
copied into the fields of a different variable. Hence make
rk805_pinctrl_desc and rk805_gpio_chip both constant to protect them
from unintended modification.
Issue found with Coccinelle.
Signed-off-by: Nishka Dasgupta <nishkadg.linux@gmail.com>
---
drivers/pinctrl/pinctrl-rk805.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/pinctrl/pinctrl-rk805.c b/drivers/pinctrl/pinctrl-rk805.c
index a8459cafd4ea..26adbe9d6d42 100644
--- a/drivers/pinctrl/pinctrl-rk805.c
+++ b/drivers/pinctrl/pinctrl-rk805.c
@@ -197,7 +197,7 @@ static int rk805_gpio_get_direction(struct gpio_chip *chip, unsigned int offset)
return !(val & pci->pin_cfg[offset].dir_msk);
}
-static struct gpio_chip rk805_gpio_chip = {
+static const struct gpio_chip rk805_gpio_chip = {
.label = "rk805-gpio",
.request = gpiochip_generic_request,
.free = gpiochip_generic_free,
@@ -404,7 +404,7 @@ static const struct pinconf_ops rk805_pinconf_ops = {
.pin_config_set = rk805_pinconf_set,
};
-static struct pinctrl_desc rk805_pinctrl_desc = {
+static const struct pinctrl_desc rk805_pinctrl_desc = {
.name = "rk805-pinctrl",
.pctlops = &rk805_pinctrl_ops,
.pmxops = &rk805_pinmux_ops,
--
2.19.1
^ permalink raw reply related
* Re: [PATCH] arm64: dts: ls1028a: fix gpio nodes
From: Shawn Guo @ 2019-08-19 7:40 UTC (permalink / raw)
To: Hui Song
Cc: Li Yang, Rob Herring, Mark Rutland, Linus Walleij,
Bartosz Golaszewski, linux-arm-kernel, devicetree, linux-kernel,
linux-gpio
In-Reply-To: <20190805065700.7601-1-hui.song_1@nxp.com>
On Mon, Aug 05, 2019 at 02:57:00PM +0800, Hui Song wrote:
> From: Song Hui <hui.song_1@nxp.com>
>
> Update the nodes to include little-endian
> property to be consistent with the hardware.
>
> Signed-off-by: Song Hui <hui.song_1@nxp.com>
Applied, thanks.
^ permalink raw reply
* Re: [PATCH] pinctrl: intel: remap the pin number to gpio offset for irq enabled pin
From: Mika Westerberg @ 2019-08-19 7:14 UTC (permalink / raw)
To: Chris Chiu
Cc: andriy.shevchenko, linus.walleij, linux-gpio, linux-kernel, linux
In-Reply-To: <20190816093838.81461-1-chiu@endlessm.com>
On Fri, Aug 16, 2019 at 05:38:38PM +0800, Chris Chiu wrote:
> On Asus X571GT, GPIO 297 is configured as an interrupt and serves
> for the touchpad. The touchpad will report input events much less
> than expected after S3 suspend/resume, which results in extremely
> slow cursor movement. However, the number of interrupts observed
> from /proc/interrupts increases much more than expected even no
> touching touchpad.
>
> This is due to the value of PADCFG0 of PIN 225 for the interrupt
> has been changed from 0x80800102 to 0x80100102. The GPIROUTIOXAPIC
> is toggled on which results in the spurious interrupts. The PADCFG0
> of PIN 225 is expected to be saved during suspend, but the 297 is
> saved instead because the gpiochip_line_is_irq() expect the GPIO
> offset but what's really passed to it is PIN number. In this case,
> the /sys/kernel/debug/pinctrl/INT3450:00/gpio-ranges shows
>
> 288: INT3450:00 GPIOS [436 - 459] PINS [216 - 239]
>
> So gpiochip_line_is_irq() returns true for GPIO offset 297, the
> suspend routine spuriously saves the content for PIN 297 which
> we expect to save for PIN 225.
Nice work nailing the issue!
> This commit maps the PIN number to GPIO offset first in the
> intel_pinctrl_should_save() to make sure the values for the
> specific PINs can be correctly saved and then restored.
>
> Signed-off-by: Chris Chiu <chiu@endlessm.com>
Acked-by: Mika Westerberg <mika.westerberg@linux.intel.com>
I think this should also have:
Fixes: c538b9436751 ("pinctrl: intel: Only restore pins that are used by the driver")
^ permalink raw reply
* Re: [PATCH] gpio: mockup: don't depend twice on GPIOLIB
From: Uwe Kleine-König @ 2019-08-19 5:49 UTC (permalink / raw)
To: Linus Walleij; +Cc: Bartosz Golaszewski, open list:GPIO SUBSYSTEM
In-Reply-To: <CACRpkdYjeNFP0KrF+RpFOvWWzmY5iKcRK9EOPqMX3t_6vwhbeA@mail.gmail.com>
[-- Attachment #1.1: Type: text/plain, Size: 1254 bytes --]
Hello Linus,
On 8/15/19 10:15 AM, Linus Walleij wrote:
> On Wed, Aug 14, 2019 at 10:12 PM Uwe Kleine-König <uwe@kleine-koenig.org> wrote:
>> On 7/25/19 3:10 PM, Uwe Kleine-König wrote:
>>> config GPIO_MOCKUP is defined in a big if GPIOLIB ... endif block so it
>>> doesn't need to depend explicitly on GPIOLIB.
>>>
>>> Signed-off-by: Uwe Kleine-König <uwe@kleine-koenig.org>
>>> ---
>>> drivers/gpio/Kconfig | 1 -
>>> 1 file changed, 1 deletion(-)
>>>
>>> diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
>>> index bb13c266c329..856fcd260ba2 100644
>>> --- a/drivers/gpio/Kconfig
>>> +++ b/drivers/gpio/Kconfig
>>> @@ -1465,7 +1465,6 @@ endmenu
>>>
>>> config GPIO_MOCKUP
>>> tristate "GPIO Testing Driver"
>>> - depends on GPIOLIB
>>> select IRQ_SIM
>>> help
>>> This enables GPIO Testing driver, which provides a way to test GPIO
>>>
>>
>> I didn't get feedback for this patch. Did it fall through the cracks?
>
> Weird, the patch is not in my inbox :(
>
> I downloaded from lore.kernel.org and applied, thanks!
Thanks for that. Though I'm not entirely happy with that as this
procedure messed up my name in my S-o-b. Would you mind to fix that?
Best regards
Uwe
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply
* Re: [PATCH v9 02/22] pinctrl: tegra: Flush pinctrl writes during resume
From: Linus Walleij @ 2019-08-18 22:20 UTC (permalink / raw)
To: Sowjanya Komatineni
Cc: thierry.reding@gmail.com, Jon Hunter, Thomas Gleixner,
Jason Cooper, Marc Zyngier, Stefan Agner, Mark Rutland,
Peter De Schrijver, Prashant Gaikwad, Stephen Boyd, linux-clk,
open list:GPIO SUBSYSTEM, jckuo, Joseph Lo, talho, linux-tegra,
linux-kernel@vger.kernel.org, Mikko Perttunen, spatra,
Rob Herring, Dmitry Osipenko,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
Rafael J. Wysocki, viresh kumar, Linux PM list
In-Reply-To: <1565984527-5272-3-git-send-email-skomatineni@nvidia.com>
On Fri, Aug 16, 2019 at 9:42 PM Sowjanya Komatineni
<skomatineni@nvidia.com> wrote:
> This patch adds pinctrl register read to flush all the prior pinctrl
> writes and then adds barrier for pinctrl register read to complete
> during resume to make sure all pinctrl changes are effective.
>
> Acked-by: Thierry Reding <treding@nvidia.com>
> Reviewed-by: Dmitry Osipenko <digetx@gmail.com>
> Signed-off-by: Sowjanya Komatineni <skomatineni@nvidia.com>
Patch applied.
Yours,
Linus Walleij
^ permalink raw reply
* Re: [PATCH v9 01/22] pinctrl: tegra: Fix write barrier placement in pmx_writel
From: Linus Walleij @ 2019-08-18 22:20 UTC (permalink / raw)
To: Sowjanya Komatineni
Cc: thierry.reding@gmail.com, Jon Hunter, Thomas Gleixner,
Jason Cooper, Marc Zyngier, Stefan Agner, Mark Rutland,
Peter De Schrijver, Prashant Gaikwad, Stephen Boyd, linux-clk,
open list:GPIO SUBSYSTEM, jckuo, Joseph Lo, talho, linux-tegra,
linux-kernel@vger.kernel.org, Mikko Perttunen, spatra,
Rob Herring, Dmitry Osipenko,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
Rafael J. Wysocki, viresh kumar, Linux PM list
In-Reply-To: <1565984527-5272-2-git-send-email-skomatineni@nvidia.com>
On Fri, Aug 16, 2019 at 9:42 PM Sowjanya Komatineni
<skomatineni@nvidia.com> wrote:
>
> pmx_writel uses writel which inserts write barrier before the
> register write.
>
> This patch has fix to replace writel with writel_relaxed followed
> by a readback and memory barrier to ensure write operation is
> completed for successful pinctrl change.
>
> Acked-by: Thierry Reding <treding@nvidia.com>
> Reviewed-by: Dmitry Osipenko <digetx@gmail.com>
> Signed-off-by: Sowjanya Komatineni <skomatineni@nvidia.com>
Took out the previous patches and applied this instead.
Yours,
Linus Walleij
^ permalink raw reply
* Re: [PATCH 2/2] irq/irq_sim: use irq domain
From: Jonathan Cameron @ 2019-08-18 19:45 UTC (permalink / raw)
To: Bartosz Golaszewski
Cc: Linus Walleij, Hartmut Knaack, Lars-Peter Clausen,
Peter Meerwald-Stadler, Thomas Gleixner, Marc Zyngier, linux-gpio,
linux-kernel, linux-iio, Bartosz Golaszewski
In-Reply-To: <20190812125256.9690-3-brgl@bgdev.pl>
On Mon, 12 Aug 2019 14:52:56 +0200
Bartosz Golaszewski <brgl@bgdev.pl> wrote:
> From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
>
> We currently have a dedicated function to map the interrupt simulator
> offsets to global interrupt numbers. This is something that irq_domain
> should handle.
>
> Create a linear irq_domain when initializing the interrupt simulator
> and modify the irq_sim_fire() function to only take as parameter the
> global interrupt number.
>
> Convert both users in the same patch to using the new interface.
>
> Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
Hmm. Again, in principle looks fine, but what advantages do we get from
using an irq domain? Pure code wise, it's longer and only a little bit
simpler.
Jonathan
> ---
> drivers/gpio/gpio-mockup.c | 11 ++-
> drivers/iio/dummy/iio_dummy_evgen.c | 22 ++++--
> include/linux/irq_sim.h | 5 +-
> kernel/irq/Kconfig | 1 +
> kernel/irq/irq_sim.c | 110 +++++++++++++++++-----------
> 5 files changed, 94 insertions(+), 55 deletions(-)
>
> diff --git a/drivers/gpio/gpio-mockup.c b/drivers/gpio/gpio-mockup.c
> index 9b28ffec5826..4cf594f0e7cd 100644
> --- a/drivers/gpio/gpio-mockup.c
> +++ b/drivers/gpio/gpio-mockup.c
> @@ -186,7 +186,7 @@ static int gpio_mockup_to_irq(struct gpio_chip *gc, unsigned int offset)
> {
> struct gpio_mockup_chip *chip = gpiochip_get_data(gc);
>
> - return irq_sim_irqnum(chip->irqsim, offset);
> + return irq_create_mapping(irq_sim_get_domain(chip->irqsim), offset);
> }
>
> static void gpio_mockup_free(struct gpio_chip *gc, unsigned int offset)
> @@ -228,6 +228,7 @@ static ssize_t gpio_mockup_debugfs_write(struct file *file,
> struct gpio_mockup_dbgfs_private *priv;
> int rv, val, curr, irq, irq_type;
> struct gpio_mockup_chip *chip;
> + struct irq_domain *domain;
> struct seq_file *sfile;
> struct gpio_desc *desc;
> struct gpio_chip *gc;
> @@ -248,6 +249,7 @@ static ssize_t gpio_mockup_debugfs_write(struct file *file,
> gc = &chip->gc;
> desc = &gc->gpiodev->descs[priv->offset];
> sim = chip->irqsim;
> + domain = irq_sim_get_domain(sim);
>
> mutex_lock(&chip->lock);
>
> @@ -257,12 +259,15 @@ static ssize_t gpio_mockup_debugfs_write(struct file *file,
> if (curr == val)
> goto out;
>
> - irq = irq_sim_irqnum(sim, priv->offset);
> + irq = irq_find_mapping(domain, priv->offset);
> + if (!irq)
> + return -ENOENT;
> +
> irq_type = irq_get_trigger_type(irq);
>
> if ((val == 1 && (irq_type & IRQ_TYPE_EDGE_RISING)) ||
> (val == 0 && (irq_type & IRQ_TYPE_EDGE_FALLING)))
> - irq_sim_fire(sim, priv->offset);
> + irq_sim_fire(irq);
> }
>
> /* Change the value unless we're actively driving the line. */
> diff --git a/drivers/iio/dummy/iio_dummy_evgen.c b/drivers/iio/dummy/iio_dummy_evgen.c
> index efbcd4a5609e..cc827f60a535 100644
> --- a/drivers/iio/dummy/iio_dummy_evgen.c
> +++ b/drivers/iio/dummy/iio_dummy_evgen.c
> @@ -31,14 +31,13 @@
> * @lock: protect the evgen state
> * @inuse: mask of which irqs are connected
> * @irq_sim: interrupt simulator
> - * @base: base of irq range
> */
> struct iio_dummy_eventgen {
> struct iio_dummy_regs regs[IIO_EVENTGEN_NO];
> struct mutex lock;
> bool inuse[IIO_EVENTGEN_NO];
> struct irq_sim *irq_sim;
> - int base;
> + struct irq_domain *domain;
> };
>
> /* We can only ever have one instance of this 'device' */
> @@ -56,7 +55,7 @@ static int iio_dummy_evgen_create(void)
> return PTR_ERR(iio_evgen->irq_sim);
> }
>
> - iio_evgen->base = irq_sim_irqnum(iio_evgen->irq_sim, 0);
> + iio_evgen->domain = irq_sim_get_domain(iio_evgen->irq_sim);
> mutex_init(&iio_evgen->lock);
>
> return 0;
> @@ -78,7 +77,7 @@ int iio_dummy_evgen_get_irq(void)
> mutex_lock(&iio_evgen->lock);
> for (i = 0; i < IIO_EVENTGEN_NO; i++) {
> if (!iio_evgen->inuse[i]) {
> - ret = irq_sim_irqnum(iio_evgen->irq_sim, i);
> + ret = irq_create_mapping(iio_evgen->domain, i);
> iio_evgen->inuse[i] = true;
> break;
> }
> @@ -99,15 +98,21 @@ EXPORT_SYMBOL_GPL(iio_dummy_evgen_get_irq);
> */
> void iio_dummy_evgen_release_irq(int irq)
> {
> + struct irq_data *irqd;
> +
> + irqd = irq_get_irq_data(irq);
> +
> mutex_lock(&iio_evgen->lock);
> - iio_evgen->inuse[irq - iio_evgen->base] = false;
> + iio_evgen->inuse[irqd_to_hwirq(irqd)] = false;
> mutex_unlock(&iio_evgen->lock);
> }
> EXPORT_SYMBOL_GPL(iio_dummy_evgen_release_irq);
>
> struct iio_dummy_regs *iio_dummy_evgen_get_regs(int irq)
> {
> - return &iio_evgen->regs[irq - iio_evgen->base];
> + struct irq_data *irqd = irq_get_irq_data(irq);
> +
> + return &iio_evgen->regs[irqd_to_hwirq(irqd)];
> }
> EXPORT_SYMBOL_GPL(iio_dummy_evgen_get_regs);
>
> @@ -129,7 +134,7 @@ static ssize_t iio_evgen_poke(struct device *dev,
> {
> struct iio_dev_attr *this_attr = to_iio_dev_attr(attr);
> unsigned long event;
> - int ret;
> + int ret, irq;
>
> ret = kstrtoul(buf, 10, &event);
> if (ret)
> @@ -138,7 +143,8 @@ static ssize_t iio_evgen_poke(struct device *dev,
> iio_evgen->regs[this_attr->address].reg_id = this_attr->address;
> iio_evgen->regs[this_attr->address].reg_data = event;
>
> - irq_sim_fire(iio_evgen->irq_sim, this_attr->address);
> + irq = irq_create_mapping(iio_evgen->domain, this_attr->address);
> + irq_sim_fire(irq);
>
> return len;
> }
> diff --git a/include/linux/irq_sim.h b/include/linux/irq_sim.h
> index 4bbf036145e2..4056d0e7f0b4 100644
> --- a/include/linux/irq_sim.h
> +++ b/include/linux/irq_sim.h
> @@ -7,6 +7,7 @@
> #define _LINUX_IRQ_SIM_H
>
> #include <linux/irq_work.h>
> +#include <linux/irqdomain.h>
> #include <linux/device.h>
>
> /*
> @@ -19,7 +20,7 @@ struct irq_sim;
> struct irq_sim *irq_sim_new(unsigned int num_irqs);
> struct irq_sim *devm_irq_sim_new(struct device *dev, unsigned int num_irqs);
> void irq_sim_free(struct irq_sim *sim);
> -void irq_sim_fire(struct irq_sim *sim, unsigned int offset);
> -int irq_sim_irqnum(struct irq_sim *sim, unsigned int offset);
> +void irq_sim_fire(int virq);
> +struct irq_domain *irq_sim_get_domain(struct irq_sim *sim);
>
> #endif /* _LINUX_IRQ_SIM_H */
> diff --git a/kernel/irq/Kconfig b/kernel/irq/Kconfig
> index f92d9a687372..d0890f7729d4 100644
> --- a/kernel/irq/Kconfig
> +++ b/kernel/irq/Kconfig
> @@ -68,6 +68,7 @@ config IRQ_DOMAIN
> config IRQ_SIM
> bool
> select IRQ_WORK
> + select IRQ_DOMAIN
>
> # Support for hierarchical irq domains
> config IRQ_DOMAIN_HIERARCHY
> diff --git a/kernel/irq/irq_sim.c b/kernel/irq/irq_sim.c
> index 79f0a6494b6c..a1c91aefb6cd 100644
> --- a/kernel/irq/irq_sim.c
> +++ b/kernel/irq/irq_sim.c
> @@ -15,13 +15,14 @@ struct irq_sim_work_ctx {
> struct irq_sim_irq_ctx {
> int irqnum;
> bool enabled;
> + struct irq_sim_work_ctx *work_ctx;
> };
>
> struct irq_sim {
> struct irq_sim_work_ctx work_ctx;
> int irq_base;
> unsigned int irq_count;
> - struct irq_sim_irq_ctx *irqs;
> + struct irq_domain *domain;
> };
>
> struct irq_sim_devres {
> @@ -74,11 +75,46 @@ static void irq_sim_handle_irq(struct irq_work *work)
> offset = find_next_bit(work_ctx->pending,
> sim->irq_count, offset);
> clear_bit(offset, work_ctx->pending);
> - irqnum = irq_sim_irqnum(sim, offset);
> + irqnum = irq_find_mapping(sim->domain, offset);
> handle_simple_irq(irq_to_desc(irqnum));
> }
> }
>
> +static int irq_sim_domain_map(struct irq_domain *domain,
> + unsigned int virq, irq_hw_number_t hw)
> +{
> + struct irq_sim *sim = domain->host_data;
> + struct irq_sim_irq_ctx *ctx;
> +
> + ctx = kzalloc(sizeof(*ctx), GFP_KERNEL);
> + if (!ctx)
> + return -ENOMEM;
> +
> + irq_set_chip(virq, &irq_sim_irqchip);
> + irq_set_chip_data(virq, ctx);
> + irq_set_handler(virq, handle_simple_irq);
> + irq_modify_status(virq, IRQ_NOREQUEST | IRQ_NOAUTOEN, IRQ_NOPROBE);
> + ctx->work_ctx = &sim->work_ctx;
> +
> + return 0;
> +}
> +
> +static void irq_sim_domain_unmap(struct irq_domain *domain, unsigned int virq)
> +{
> + struct irq_sim_irq_ctx *ctx;
> + struct irq_data *irqd;
> +
> + irqd = irq_domain_get_irq_data(domain, virq);
> + ctx = irq_data_get_irq_chip_data(irqd);
> +
> + kfree(ctx);
> +}
> +
> +static const struct irq_domain_ops irq_sim_domain_ops = {
> + .map = irq_sim_domain_map,
> + .unmap = irq_sim_domain_unmap,
> +};
> +
> /**
> * irq_sim_new - Create a new interrupt simulator: allocate a range of
> * dummy interrupts.
> @@ -91,42 +127,21 @@ static void irq_sim_handle_irq(struct irq_work *work)
> struct irq_sim *irq_sim_new(unsigned int num_irqs)
> {
> struct irq_sim *sim;
> - int i;
>
> sim = kmalloc(sizeof(*sim), GFP_KERNEL);
> if (!sim)
> return ERR_PTR(-ENOMEM);
>
> - sim->irqs = kmalloc_array(num_irqs, sizeof(*sim->irqs), GFP_KERNEL);
> - if (!sim->irqs) {
> - kfree(sim);
> - return ERR_PTR(-ENOMEM);
> - }
> -
> - sim->irq_base = irq_alloc_descs(-1, 0, num_irqs, 0);
> - if (sim->irq_base < 0) {
> - kfree(sim->irqs);
> - kfree(sim);
> - return ERR_PTR(sim->irq_base);
> - }
> -
> sim->work_ctx.pending = bitmap_zalloc(num_irqs, GFP_KERNEL);
> if (!sim->work_ctx.pending) {
> - kfree(sim->irqs);
> kfree(sim);
> - irq_free_descs(sim->irq_base, num_irqs);
> return ERR_PTR(-ENOMEM);
> }
>
> - for (i = 0; i < num_irqs; i++) {
> - sim->irqs[i].irqnum = sim->irq_base + i;
> - sim->irqs[i].enabled = false;
> - irq_set_chip(sim->irq_base + i, &irq_sim_irqchip);
> - irq_set_chip_data(sim->irq_base + i, &sim->irqs[i]);
> - irq_set_handler(sim->irq_base + i, &handle_simple_irq);
> - irq_modify_status(sim->irq_base + i,
> - IRQ_NOREQUEST | IRQ_NOAUTOEN, IRQ_NOPROBE);
> - }
> + sim->domain = irq_domain_create_linear(NULL, num_irqs,
> + &irq_sim_domain_ops, sim);
> + if (!sim->domain)
> + return ERR_PTR(-ENOMEM);
>
> init_irq_work(&sim->work_ctx.work, irq_sim_handle_irq);
> sim->irq_count = num_irqs;
> @@ -143,10 +158,17 @@ EXPORT_SYMBOL_GPL(irq_sim_new);
> */
> void irq_sim_free(struct irq_sim *sim)
> {
> + int i, irq;
> +
> + for (i = 0; i < sim->irq_count; i++) {
> + irq = irq_find_mapping(sim->domain, i);
> + if (irq)
> + irq_dispose_mapping(irq);
> + }
> +
> + irq_domain_remove(sim->domain);
> irq_work_sync(&sim->work_ctx.work);
> bitmap_free(sim->work_ctx.pending);
> - irq_free_descs(sim->irq_base, sim->irq_count);
> - kfree(sim->irqs);
> kfree(sim);
> }
> EXPORT_SYMBOL_GPL(irq_sim_free);
> @@ -189,27 +211,31 @@ EXPORT_SYMBOL_GPL(devm_irq_sim_new);
> /**
> * irq_sim_fire - Enqueue an interrupt.
> *
> - * @sim: The interrupt simulator object.
> - * @offset: Offset of the simulated interrupt which should be fired.
> + * @virq: Virtual interrupt number to fire. It must be associated with
> + * an existing interrupt simulator.
> */
> -void irq_sim_fire(struct irq_sim *sim, unsigned int offset)
> +void irq_sim_fire(int virq)
> {
> - if (sim->irqs[offset].enabled) {
> - set_bit(offset, sim->work_ctx.pending);
> - irq_work_queue(&sim->work_ctx.work);
> + struct irq_sim_irq_ctx *ctx;
> + struct irq_data *irqd;
> +
> + irqd = irq_get_irq_data(virq);
> + ctx = irq_data_get_irq_chip_data(irqd);
> +
> + if (ctx->enabled) {
> + set_bit(irqd_to_hwirq(irqd), ctx->work_ctx->pending);
> + irq_work_queue(&ctx->work_ctx->work);
> }
> }
> EXPORT_SYMBOL_GPL(irq_sim_fire);
>
> /**
> - * irq_sim_irqnum - Get the allocated number of a dummy interrupt.
> + * irq_sim_get_domain - Retrieve the interrupt domain for this simulator.
> *
> - * @sim: The interrupt simulator object.
> - * @offset: Offset of the simulated interrupt for which to retrieve
> - * the number.
> + * @sim: The interrupt simulator the domain of which we retrieve.
> */
> -int irq_sim_irqnum(struct irq_sim *sim, unsigned int offset)
> +struct irq_domain *irq_sim_get_domain(struct irq_sim *sim)
> {
> - return sim->irqs[offset].irqnum;
> + return sim->domain;
> }
> -EXPORT_SYMBOL_GPL(irq_sim_irqnum);
> +EXPORT_SYMBOL(irq_sim_get_domain);
^ permalink raw reply
* Re: [PATCH 1/2] irq/irq_sim: make the irq_sim structure opaque
From: Jonathan Cameron @ 2019-08-18 19:41 UTC (permalink / raw)
To: Bartosz Golaszewski
Cc: Linus Walleij, Hartmut Knaack, Lars-Peter Clausen,
Peter Meerwald-Stadler, Thomas Gleixner, Marc Zyngier, linux-gpio,
linux-kernel, linux-iio, Bartosz Golaszewski
In-Reply-To: <20190818203836.6cceb4d3@archlinux>
On Sun, 18 Aug 2019 20:38:36 +0100
Jonathan Cameron <jic23@kernel.org> wrote:
> On Mon, 12 Aug 2019 14:52:55 +0200
> Bartosz Golaszewski <brgl@bgdev.pl> wrote:
>
> > From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> >
> > There's no good reason to export the interrupt simulator structure to
> > users and have them provide memory for it. Let's make all the related
> > data structures opaque and convert both users. This way we have a lot
> > less APIs exposed in the header.
> >
> > Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
>
> I agree in principle, but there is the disadvantage that all drivers
> that use it now need to bother with another allocation. I guess
> it is still worthwhile though.
>
> One comment inline.
Ignore that one as it's in code you get rid of in patch 2 ;)
Acked-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
>
> Jonathan
>
> > ---
> > drivers/gpio/gpio-mockup.c | 12 ++---
> > drivers/iio/dummy/iio_dummy_evgen.c | 18 +++----
> > include/linux/irq_sim.h | 24 ++-------
> > kernel/irq/irq_sim.c | 83 ++++++++++++++++++-----------
> > 4 files changed, 70 insertions(+), 67 deletions(-)
> >
> > diff --git a/drivers/gpio/gpio-mockup.c b/drivers/gpio/gpio-mockup.c
> > index f1a9c0544e3f..9b28ffec5826 100644
> > --- a/drivers/gpio/gpio-mockup.c
> > +++ b/drivers/gpio/gpio-mockup.c
> > @@ -53,7 +53,7 @@ struct gpio_mockup_line_status {
> > struct gpio_mockup_chip {
> > struct gpio_chip gc;
> > struct gpio_mockup_line_status *lines;
> > - struct irq_sim irqsim;
> > + struct irq_sim *irqsim;
> > struct dentry *dbg_dir;
> > struct mutex lock;
> > };
> > @@ -186,7 +186,7 @@ static int gpio_mockup_to_irq(struct gpio_chip *gc, unsigned int offset)
> > {
> > struct gpio_mockup_chip *chip = gpiochip_get_data(gc);
> >
> > - return irq_sim_irqnum(&chip->irqsim, offset);
> > + return irq_sim_irqnum(chip->irqsim, offset);
> > }
> >
> > static void gpio_mockup_free(struct gpio_chip *gc, unsigned int offset)
> > @@ -247,7 +247,7 @@ static ssize_t gpio_mockup_debugfs_write(struct file *file,
> > chip = priv->chip;
> > gc = &chip->gc;
> > desc = &gc->gpiodev->descs[priv->offset];
> > - sim = &chip->irqsim;
> > + sim = chip->irqsim;
> >
> > mutex_lock(&chip->lock);
> >
> > @@ -431,9 +431,9 @@ static int gpio_mockup_probe(struct platform_device *pdev)
> > return rv;
> > }
> >
> > - rv = devm_irq_sim_init(dev, &chip->irqsim, gc->ngpio);
> > - if (rv < 0)
> > - return rv;
> > + chip->irqsim = devm_irq_sim_new(dev, gc->ngpio);
> > + if (IS_ERR(chip->irqsim))
> > + return PTR_ERR(chip->irqsim);
> >
> > rv = devm_gpiochip_add_data(dev, &chip->gc, chip);
> > if (rv)
> > diff --git a/drivers/iio/dummy/iio_dummy_evgen.c b/drivers/iio/dummy/iio_dummy_evgen.c
> > index a6edf30567aa..efbcd4a5609e 100644
> > --- a/drivers/iio/dummy/iio_dummy_evgen.c
> > +++ b/drivers/iio/dummy/iio_dummy_evgen.c
> > @@ -37,7 +37,7 @@ struct iio_dummy_eventgen {
> > struct iio_dummy_regs regs[IIO_EVENTGEN_NO];
> > struct mutex lock;
> > bool inuse[IIO_EVENTGEN_NO];
> > - struct irq_sim irq_sim;
> > + struct irq_sim *irq_sim;
> > int base;
> > };
> >
> > @@ -46,19 +46,17 @@ static struct iio_dummy_eventgen *iio_evgen;
> >
> > static int iio_dummy_evgen_create(void)
> > {
> > - int ret;
> > -
> > iio_evgen = kzalloc(sizeof(*iio_evgen), GFP_KERNEL);
> > if (!iio_evgen)
> > return -ENOMEM;
> >
> > - ret = irq_sim_init(&iio_evgen->irq_sim, IIO_EVENTGEN_NO);
> > - if (ret < 0) {
> > + iio_evgen->irq_sim = irq_sim_new(IIO_EVENTGEN_NO);
> > + if (IS_ERR(iio_evgen->irq_sim)) {
> > kfree(iio_evgen);
> > - return ret;
> > + return PTR_ERR(iio_evgen->irq_sim);
> > }
> >
> > - iio_evgen->base = irq_sim_irqnum(&iio_evgen->irq_sim, 0);
> > + iio_evgen->base = irq_sim_irqnum(iio_evgen->irq_sim, 0);
> > mutex_init(&iio_evgen->lock);
> >
> > return 0;
> > @@ -80,7 +78,7 @@ int iio_dummy_evgen_get_irq(void)
> > mutex_lock(&iio_evgen->lock);
> > for (i = 0; i < IIO_EVENTGEN_NO; i++) {
> > if (!iio_evgen->inuse[i]) {
> > - ret = irq_sim_irqnum(&iio_evgen->irq_sim, i);
> > + ret = irq_sim_irqnum(iio_evgen->irq_sim, i);
> > iio_evgen->inuse[i] = true;
> > break;
> > }
> > @@ -115,7 +113,7 @@ EXPORT_SYMBOL_GPL(iio_dummy_evgen_get_regs);
> >
> > static void iio_dummy_evgen_free(void)
> > {
> > - irq_sim_fini(&iio_evgen->irq_sim);
> > + irq_sim_free(iio_evgen->irq_sim);
> > kfree(iio_evgen);
> > }
> >
> > @@ -140,7 +138,7 @@ static ssize_t iio_evgen_poke(struct device *dev,
> > iio_evgen->regs[this_attr->address].reg_id = this_attr->address;
> > iio_evgen->regs[this_attr->address].reg_data = event;
> >
> > - irq_sim_fire(&iio_evgen->irq_sim, this_attr->address);
> > + irq_sim_fire(iio_evgen->irq_sim, this_attr->address);
> >
> > return len;
> > }
> > diff --git a/include/linux/irq_sim.h b/include/linux/irq_sim.h
> > index 4500d453a63e..4bbf036145e2 100644
> > --- a/include/linux/irq_sim.h
> > +++ b/include/linux/irq_sim.h
> > @@ -14,27 +14,11 @@
> > * requested like normal irqs and enqueued from process context.
> > */
> >
> > -struct irq_sim_work_ctx {
> > - struct irq_work work;
> > - unsigned long *pending;
> > -};
> > +struct irq_sim;
> >
> > -struct irq_sim_irq_ctx {
> > - int irqnum;
> > - bool enabled;
> > -};
> > -
> > -struct irq_sim {
> > - struct irq_sim_work_ctx work_ctx;
> > - int irq_base;
> > - unsigned int irq_count;
> > - struct irq_sim_irq_ctx *irqs;
> > -};
> > -
> > -int irq_sim_init(struct irq_sim *sim, unsigned int num_irqs);
> > -int devm_irq_sim_init(struct device *dev, struct irq_sim *sim,
> > - unsigned int num_irqs);
> > -void irq_sim_fini(struct irq_sim *sim);
> > +struct irq_sim *irq_sim_new(unsigned int num_irqs);
> > +struct irq_sim *devm_irq_sim_new(struct device *dev, unsigned int num_irqs);
> > +void irq_sim_free(struct irq_sim *sim);
> > void irq_sim_fire(struct irq_sim *sim, unsigned int offset);
> > int irq_sim_irqnum(struct irq_sim *sim, unsigned int offset);
> >
> > diff --git a/kernel/irq/irq_sim.c b/kernel/irq/irq_sim.c
> > index b992f88c5613..79f0a6494b6c 100644
> > --- a/kernel/irq/irq_sim.c
> > +++ b/kernel/irq/irq_sim.c
> > @@ -7,6 +7,23 @@
> > #include <linux/irq_sim.h>
> > #include <linux/irq.h>
> >
> > +struct irq_sim_work_ctx {
> > + struct irq_work work;
> > + unsigned long *pending;
> > +};
> > +
> > +struct irq_sim_irq_ctx {
> > + int irqnum;
> > + bool enabled;
> > +};
> > +
> > +struct irq_sim {
> > + struct irq_sim_work_ctx work_ctx;
> > + int irq_base;
> > + unsigned int irq_count;
> > + struct irq_sim_irq_ctx *irqs;
> > +};
> > +
> > struct irq_sim_devres {
> > struct irq_sim *sim;
> > };
> > @@ -63,34 +80,42 @@ static void irq_sim_handle_irq(struct irq_work *work)
> > }
> >
> > /**
> > - * irq_sim_init - Initialize the interrupt simulator: allocate a range of
> > - * dummy interrupts.
> > + * irq_sim_new - Create a new interrupt simulator: allocate a range of
> > + * dummy interrupts.
> > *
> > - * @sim: The interrupt simulator object to initialize.
> > * @num_irqs: Number of interrupts to allocate
> > *
> > - * On success: return the base of the allocated interrupt range.
> > - * On failure: a negative errno.
> > + * On success: return the new irq_sim object.
> > + * On failure: a negative errno wrapped with ERR_PTR().
> > */
> > -int irq_sim_init(struct irq_sim *sim, unsigned int num_irqs)
> > +struct irq_sim *irq_sim_new(unsigned int num_irqs)
> > {
> > + struct irq_sim *sim;
> > int i;
> >
> > + sim = kmalloc(sizeof(*sim), GFP_KERNEL);
> > + if (!sim)
> > + return ERR_PTR(-ENOMEM);
> > +
> > sim->irqs = kmalloc_array(num_irqs, sizeof(*sim->irqs), GFP_KERNEL);
> > - if (!sim->irqs)
> > - return -ENOMEM;
> > + if (!sim->irqs) {
> > + kfree(sim);
> > + return ERR_PTR(-ENOMEM);
> > + }
> >
> > sim->irq_base = irq_alloc_descs(-1, 0, num_irqs, 0);
> > if (sim->irq_base < 0) {
> > kfree(sim->irqs);
> > - return sim->irq_base;
> > + kfree(sim);
> > + return ERR_PTR(sim->irq_base);
> > }
> >
> > sim->work_ctx.pending = bitmap_zalloc(num_irqs, GFP_KERNEL);
> > if (!sim->work_ctx.pending) {
> > kfree(sim->irqs);
> > + kfree(sim);
>
> This rather looks like a function that could benefit from some
> unified error paths. That was true already, but adding another step
> makes it an even better idea. Goto fun :)
>
> > irq_free_descs(sim->irq_base, num_irqs);
> > - return -ENOMEM;
> > + return ERR_PTR(-ENOMEM);
> > }
> >
> > for (i = 0; i < num_irqs; i++) {
> > @@ -106,64 +131,60 @@ int irq_sim_init(struct irq_sim *sim, unsigned int num_irqs)
> > init_irq_work(&sim->work_ctx.work, irq_sim_handle_irq);
> > sim->irq_count = num_irqs;
> >
> > - return sim->irq_base;
> > + return sim;
> > }
> > -EXPORT_SYMBOL_GPL(irq_sim_init);
> > +EXPORT_SYMBOL_GPL(irq_sim_new);
> >
> > /**
> > - * irq_sim_fini - Deinitialize the interrupt simulator: free the interrupt
> > + * irq_sim_free - Deinitialize the interrupt simulator: free the interrupt
> > * descriptors and allocated memory.
> > *
> > * @sim: The interrupt simulator to tear down.
> > */
> > -void irq_sim_fini(struct irq_sim *sim)
> > +void irq_sim_free(struct irq_sim *sim)
> > {
> > irq_work_sync(&sim->work_ctx.work);
> > bitmap_free(sim->work_ctx.pending);
> > irq_free_descs(sim->irq_base, sim->irq_count);
> > kfree(sim->irqs);
> > + kfree(sim);
> > }
> > -EXPORT_SYMBOL_GPL(irq_sim_fini);
> > +EXPORT_SYMBOL_GPL(irq_sim_free);
> >
> > static void devm_irq_sim_release(struct device *dev, void *res)
> > {
> > struct irq_sim_devres *this = res;
> >
> > - irq_sim_fini(this->sim);
> > + irq_sim_free(this->sim);
> > }
> >
> > /**
> > - * irq_sim_init - Initialize the interrupt simulator for a managed device.
> > + * devm_irq_sim_new - Create a new interrupt simulator for a managed device.
> > *
> > * @dev: Device to initialize the simulator object for.
> > - * @sim: The interrupt simulator object to initialize.
> > * @num_irqs: Number of interrupts to allocate
> > *
> > - * On success: return the base of the allocated interrupt range.
> > - * On failure: a negative errno.
> > + * On success: return a new irq_sim object.
> > + * On failure: a negative errno wrapped with ERR_PTR().
> > */
> > -int devm_irq_sim_init(struct device *dev, struct irq_sim *sim,
> > - unsigned int num_irqs)
> > +struct irq_sim *devm_irq_sim_new(struct device *dev, unsigned int num_irqs)
> > {
> > struct irq_sim_devres *dr;
> > - int rv;
> >
> > dr = devres_alloc(devm_irq_sim_release, sizeof(*dr), GFP_KERNEL);
> > if (!dr)
> > - return -ENOMEM;
> > + return ERR_PTR(-ENOMEM);
> >
> > - rv = irq_sim_init(sim, num_irqs);
> > - if (rv < 0) {
> > + dr->sim = irq_sim_new(num_irqs);
> > + if (IS_ERR(dr->sim)) {
> > devres_free(dr);
> > - return rv;
> > + return dr->sim;
> > }
> >
> > - dr->sim = sim;
> > devres_add(dev, dr);
> > -
> > - return rv;
> > + return dr->sim;
> > }
> > -EXPORT_SYMBOL_GPL(devm_irq_sim_init);
> > +EXPORT_SYMBOL_GPL(devm_irq_sim_new);
> >
> > /**
> > * irq_sim_fire - Enqueue an interrupt.
>
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox