Re: [PATCHv3 3/3] gpio: sch: Enable IRQ support for Quark X1000
On Wed, Nov 05, 2014 at 11:33:01AM +0200, Chang, Rebecca Swee Fun wrote:
> Sorry for the late reply, was working on something else.
>
> > -Original Message-
> > From: Westerberg, Mika
> > Sent: 16 October, 2014 6:19 PM
> > To: Chang, Rebecca Swee Fun
> > Cc: Linus Walleij; GPIO Subsystem Mailing List; Linux Kernel Mailing List;
> > Denis
> > Turischev
> > Subject: Re: [PATCHv3 3/3] gpio: sch: Enable IRQ support for Quark X1000
> >
> > On Thu, Oct 16, 2014 at 11:51:15AM +0800, Chang Rebecca Swee Fun wrote:
> > > Intel Quark X1000 GPIO controller supports interrupt handling for both
> > > core power well and resume power well. This patch is to enable the IRQ
> > > support and provide IRQ handling for Intel Quark X1000 GPIO-SCH device
> > > driver.
> > >
> > > This piece of work is derived from Dan O'Donovan's initial work for
> > > Quark X1000 enabling.
> > >
> > > Signed-off-by: Chang Rebecca Swee Fun
> > >
> >
> > In addition to what Varka Bhadram pointed out, I have few comments. See
> > below.
> >
> > Overall, looks good.
> >
> > > ---
> > > drivers/gpio/gpio-sch.c | 257
> > > ---
> > > 1 file changed, 245 insertions(+), 12 deletions(-)
> > >
> (...)
>
> > > +static void sch_gpio_irq_disable(struct irq_data *d) {
> > > + struct sch_gpio *sch = container_of(d, struct sch_gpio, data);
> > > + u32 gpio_num;
> > > +
> > > + gpio_num = d->irq - sch->irq_base;
> > > + sch_gpio_register_clear(sch, gpio_num, GGPE); }
> > > +
> > > +static void sch_gpio_irq_ack(struct irq_data *d) {
> > > + struct sch_gpio *sch = container_of(d, struct sch_gpio, data);
> > > + u32 gpio_num;
> > > +
> > > + gpio_num = d->irq - sch->irq_base;
> > > + sch_gpio_reg_set(&(sch->chip), gpio_num, GTS, 1);
> >
> > Maybe use the new sch_gpio_register_set() here instead?
>
> According to Alexandre's review feedback, sch_gpio_register_set() and
> sch_gpio_register_clear() actually having similar block of codes with
> sch_gpio_reg_set(). He suggested to call sch_gpio_reg_set(gc, gpio,
> reg, 1) in place of sch_gpio_register_set() and sch_gpio_reg_set(gc,
> gpio, reg, 0) for sch_gpio_register_clear(). I'm now in the progress
> of reworking the patches in that direction.
Sounds good.
> >
> > > +}
> > > +
> > > +static int sch_gpio_irq_type(struct irq_data *d, unsigned type) {
> > > + struct sch_gpio *sch = container_of(d, struct sch_gpio, data);
> > > + unsigned long flags;
> > > + u32 gpio_num;
> > > +
> > > + if (d == NULL)
> > > + return -EINVAL;
> >
> > How can that happen?
>
> This is just to ensure the irq_data struct contains data. If you think
> this is no needed, I will remove it in next submission.
I think you can drop that check. Also if 'd' would be NULL you will
crash already in
struct sch_gpio *sch = container_of(d, struct sch_gpio, data);
> (...)
> > > +
> > > +static void sch_gpio_irqs_init(struct sch_gpio *sch, unsigned int
> > > +num) {
> > > + unsigned int i;
> > > +
> > > + for (i = 0; i < num; i++) {
> > > + irq_set_chip_data(i + sch->irq_base, sch);
> > > + irq_set_chip_and_handler_name(i + sch->irq_base,
> > > + _irq_chip,
> > > + handle_simple_irq,
> > > + "sch_gpio_irq_chip");
> >
> > Hmm, I wonder if this
> >
> > irq_set_chip_and_handler_name(i + sch->irq_base,
> > _irq_chip,
> > handle_simple_irq,
> > "sch_gpio_irq_chip");
> >
> > looks better. Up to you.
> >
> I think I will take what you've suggested. :)
>
> (...)
> > > static int sch_gpio_probe(struct platform_device *pdev) {
> > > struct sch_gpio *sch;
> > > - struct resource *res;
> > > + struct resource *res, *irq;
> > > + int err;
> > >
> > > sch = devm_kzalloc(>dev, sizeof(*sch), GFP_KERNEL);
> > > if (!sch)
> > > @@ -203,6 +376,17 @@ static int sch_gpio_probe(struct platform_device
> > *pdev)
> > >pdev->name))
> > > return -EBUSY;
> > >
> > > + irq = platform_get_resource(pdev, IORESOURCE_IRQ,
RE: [PATCHv3 3/3] gpio: sch: Enable IRQ support for Quark X1000
Sorry for the late reply, was working on something else.
> -Original Message-
> From: Westerberg, Mika
> Sent: 16 October, 2014 6:19 PM
> To: Chang, Rebecca Swee Fun
> Cc: Linus Walleij; GPIO Subsystem Mailing List; Linux Kernel Mailing List;
> Denis
> Turischev
> Subject: Re: [PATCHv3 3/3] gpio: sch: Enable IRQ support for Quark X1000
>
> On Thu, Oct 16, 2014 at 11:51:15AM +0800, Chang Rebecca Swee Fun wrote:
> > Intel Quark X1000 GPIO controller supports interrupt handling for both
> > core power well and resume power well. This patch is to enable the IRQ
> > support and provide IRQ handling for Intel Quark X1000 GPIO-SCH device
> > driver.
> >
> > This piece of work is derived from Dan O'Donovan's initial work for
> > Quark X1000 enabling.
> >
> > Signed-off-by: Chang Rebecca Swee Fun
> >
>
> In addition to what Varka Bhadram pointed out, I have few comments. See
> below.
>
> Overall, looks good.
>
> > ---
> > drivers/gpio/gpio-sch.c | 257
> > ---
> > 1 file changed, 245 insertions(+), 12 deletions(-)
> >
(...)
> > +static void sch_gpio_irq_disable(struct irq_data *d) {
> > + struct sch_gpio *sch = container_of(d, struct sch_gpio, data);
> > + u32 gpio_num;
> > +
> > + gpio_num = d->irq - sch->irq_base;
> > + sch_gpio_register_clear(sch, gpio_num, GGPE); }
> > +
> > +static void sch_gpio_irq_ack(struct irq_data *d) {
> > + struct sch_gpio *sch = container_of(d, struct sch_gpio, data);
> > + u32 gpio_num;
> > +
> > + gpio_num = d->irq - sch->irq_base;
> > + sch_gpio_reg_set(&(sch->chip), gpio_num, GTS, 1);
>
> Maybe use the new sch_gpio_register_set() here instead?
According to Alexandre's review feedback, sch_gpio_register_set() and
sch_gpio_register_clear() actually having similar block of codes with
sch_gpio_reg_set(). He suggested to call sch_gpio_reg_set(gc, gpio, reg, 1) in
place of sch_gpio_register_set() and sch_gpio_reg_set(gc, gpio, reg, 0) for
sch_gpio_register_clear(). I'm now in the progress of reworking the patches in
that direction.
>
> > +}
> > +
> > +static int sch_gpio_irq_type(struct irq_data *d, unsigned type) {
> > + struct sch_gpio *sch = container_of(d, struct sch_gpio, data);
> > + unsigned long flags;
> > + u32 gpio_num;
> > +
> > + if (d == NULL)
> > + return -EINVAL;
>
> How can that happen?
This is just to ensure the irq_data struct contains data. If you think this is
no needed, I will remove it in next submission.
(...)
> > +
> > +static void sch_gpio_irqs_init(struct sch_gpio *sch, unsigned int
> > +num) {
> > + unsigned int i;
> > +
> > + for (i = 0; i < num; i++) {
> > + irq_set_chip_data(i + sch->irq_base, sch);
> > + irq_set_chip_and_handler_name(i + sch->irq_base,
> > + _irq_chip,
> > + handle_simple_irq,
> > + "sch_gpio_irq_chip");
>
> Hmm, I wonder if this
>
> irq_set_chip_and_handler_name(i + sch->irq_base,
> _irq_chip,
> handle_simple_irq,
> "sch_gpio_irq_chip");
>
> looks better. Up to you.
>
I think I will take what you've suggested. :)
(...)
> > static int sch_gpio_probe(struct platform_device *pdev) {
> > struct sch_gpio *sch;
> > - struct resource *res;
> > + struct resource *res, *irq;
> > + int err;
> >
> > sch = devm_kzalloc(>dev, sizeof(*sch), GFP_KERNEL);
> > if (!sch)
> > @@ -203,6 +376,17 @@ static int sch_gpio_probe(struct platform_device
> *pdev)
> > pdev->name))
> > return -EBUSY;
> >
> > + irq = platform_get_resource(pdev, IORESOURCE_IRQ, 0);
> > + sch->irq_support = !!irq;
> > + if (sch->irq_support) {
> > + sch->irq_num = irq->start;
> > + if (sch->irq_num < 0) {
>
> Is this really possible? I mean can't you just detect the irq support by
> looking
> sch->irq_num? If it is > 0 then it has interrupt support. I think that irq 0
> is not
> valid.
Yes, I can do that, but the irq_num was being passed from MFD LPC_SCH. This
implementation is to avoid enabling irq support if LPC_SCH passed in a negative
value.
As for irq 0, I think it is valid. Based on my observation in "cat
/proc/interrupt" there is a 0 irq line available.
>
> > + de
RE: [PATCHv3 3/3] gpio: sch: Enable IRQ support for Quark X1000
Sorry for the late reply, was working on something else.
-Original Message-
From: Westerberg, Mika
Sent: 16 October, 2014 6:19 PM
To: Chang, Rebecca Swee Fun
Cc: Linus Walleij; GPIO Subsystem Mailing List; Linux Kernel Mailing List;
Denis
Turischev
Subject: Re: [PATCHv3 3/3] gpio: sch: Enable IRQ support for Quark X1000
On Thu, Oct 16, 2014 at 11:51:15AM +0800, Chang Rebecca Swee Fun wrote:
Intel Quark X1000 GPIO controller supports interrupt handling for both
core power well and resume power well. This patch is to enable the IRQ
support and provide IRQ handling for Intel Quark X1000 GPIO-SCH device
driver.
This piece of work is derived from Dan O'Donovan's initial work for
Quark X1000 enabling.
Signed-off-by: Chang Rebecca Swee Fun
rebecca.swee.fun.ch...@intel.com
In addition to what Varka Bhadram pointed out, I have few comments. See
below.
Overall, looks good.
---
drivers/gpio/gpio-sch.c | 257
---
1 file changed, 245 insertions(+), 12 deletions(-)
(...)
+static void sch_gpio_irq_disable(struct irq_data *d) {
+ struct sch_gpio *sch = container_of(d, struct sch_gpio, data);
+ u32 gpio_num;
+
+ gpio_num = d-irq - sch-irq_base;
+ sch_gpio_register_clear(sch, gpio_num, GGPE); }
+
+static void sch_gpio_irq_ack(struct irq_data *d) {
+ struct sch_gpio *sch = container_of(d, struct sch_gpio, data);
+ u32 gpio_num;
+
+ gpio_num = d-irq - sch-irq_base;
+ sch_gpio_reg_set((sch-chip), gpio_num, GTS, 1);
Maybe use the new sch_gpio_register_set() here instead?
According to Alexandre's review feedback, sch_gpio_register_set() and
sch_gpio_register_clear() actually having similar block of codes with
sch_gpio_reg_set(). He suggested to call sch_gpio_reg_set(gc, gpio, reg, 1) in
place of sch_gpio_register_set() and sch_gpio_reg_set(gc, gpio, reg, 0) for
sch_gpio_register_clear(). I'm now in the progress of reworking the patches in
that direction.
+}
+
+static int sch_gpio_irq_type(struct irq_data *d, unsigned type) {
+ struct sch_gpio *sch = container_of(d, struct sch_gpio, data);
+ unsigned long flags;
+ u32 gpio_num;
+
+ if (d == NULL)
+ return -EINVAL;
How can that happen?
This is just to ensure the irq_data struct contains data. If you think this is
no needed, I will remove it in next submission.
(...)
+
+static void sch_gpio_irqs_init(struct sch_gpio *sch, unsigned int
+num) {
+ unsigned int i;
+
+ for (i = 0; i num; i++) {
+ irq_set_chip_data(i + sch-irq_base, sch);
+ irq_set_chip_and_handler_name(i + sch-irq_base,
+ sch_irq_chip,
+ handle_simple_irq,
+ sch_gpio_irq_chip);
Hmm, I wonder if this
irq_set_chip_and_handler_name(i + sch-irq_base,
sch_irq_chip,
handle_simple_irq,
sch_gpio_irq_chip);
looks better. Up to you.
I think I will take what you've suggested. :)
(...)
static int sch_gpio_probe(struct platform_device *pdev) {
struct sch_gpio *sch;
- struct resource *res;
+ struct resource *res, *irq;
+ int err;
sch = devm_kzalloc(pdev-dev, sizeof(*sch), GFP_KERNEL);
if (!sch)
@@ -203,6 +376,17 @@ static int sch_gpio_probe(struct platform_device
*pdev)
pdev-name))
return -EBUSY;
+ irq = platform_get_resource(pdev, IORESOURCE_IRQ, 0);
+ sch-irq_support = !!irq;
+ if (sch-irq_support) {
+ sch-irq_num = irq-start;
+ if (sch-irq_num 0) {
Is this really possible? I mean can't you just detect the irq support by
looking
sch-irq_num? If it is 0 then it has interrupt support. I think that irq 0
is not
valid.
Yes, I can do that, but the irq_num was being passed from MFD LPC_SCH. This
implementation is to avoid enabling irq support if LPC_SCH passed in a negative
value.
As for irq 0, I think it is valid. Based on my observation in cat
/proc/interrupt there is a 0 irq line available.
+ dev_warn(pdev-dev,
+failed to obtain irq number for device\n);
+ sch-irq_support = false;
+ }
+ }
+
spin_lock_init(sch-lock);
sch-iobase = res-start;
sch-chip = sch_gpio_chip;
@@ -251,15 +435,64 @@ static int sch_gpio_probe(struct platform_device
*pdev)
return -ENODEV;
}
+ err = gpiochip_add(sch-chip);
+ if (err 0)
+ goto err_sch_gpio;
+
+ if (sch-irq_support) {
+ sch-irq_base = irq_alloc_descs(-1, 0, sch-chip.ngpio,
+ NUMA_NO_NODE);
+ if (sch-irq_base 0) {
+ dev_err(pdev-dev,
+ failed to allocate GPIO IRQ
Re: [PATCHv3 3/3] gpio: sch: Enable IRQ support for Quark X1000
On Wed, Nov 05, 2014 at 11:33:01AM +0200, Chang, Rebecca Swee Fun wrote:
Sorry for the late reply, was working on something else.
-Original Message-
From: Westerberg, Mika
Sent: 16 October, 2014 6:19 PM
To: Chang, Rebecca Swee Fun
Cc: Linus Walleij; GPIO Subsystem Mailing List; Linux Kernel Mailing List;
Denis
Turischev
Subject: Re: [PATCHv3 3/3] gpio: sch: Enable IRQ support for Quark X1000
On Thu, Oct 16, 2014 at 11:51:15AM +0800, Chang Rebecca Swee Fun wrote:
Intel Quark X1000 GPIO controller supports interrupt handling for both
core power well and resume power well. This patch is to enable the IRQ
support and provide IRQ handling for Intel Quark X1000 GPIO-SCH device
driver.
This piece of work is derived from Dan O'Donovan's initial work for
Quark X1000 enabling.
Signed-off-by: Chang Rebecca Swee Fun
rebecca.swee.fun.ch...@intel.com
In addition to what Varka Bhadram pointed out, I have few comments. See
below.
Overall, looks good.
---
drivers/gpio/gpio-sch.c | 257
---
1 file changed, 245 insertions(+), 12 deletions(-)
(...)
+static void sch_gpio_irq_disable(struct irq_data *d) {
+ struct sch_gpio *sch = container_of(d, struct sch_gpio, data);
+ u32 gpio_num;
+
+ gpio_num = d-irq - sch-irq_base;
+ sch_gpio_register_clear(sch, gpio_num, GGPE); }
+
+static void sch_gpio_irq_ack(struct irq_data *d) {
+ struct sch_gpio *sch = container_of(d, struct sch_gpio, data);
+ u32 gpio_num;
+
+ gpio_num = d-irq - sch-irq_base;
+ sch_gpio_reg_set((sch-chip), gpio_num, GTS, 1);
Maybe use the new sch_gpio_register_set() here instead?
According to Alexandre's review feedback, sch_gpio_register_set() and
sch_gpio_register_clear() actually having similar block of codes with
sch_gpio_reg_set(). He suggested to call sch_gpio_reg_set(gc, gpio,
reg, 1) in place of sch_gpio_register_set() and sch_gpio_reg_set(gc,
gpio, reg, 0) for sch_gpio_register_clear(). I'm now in the progress
of reworking the patches in that direction.
Sounds good.
+}
+
+static int sch_gpio_irq_type(struct irq_data *d, unsigned type) {
+ struct sch_gpio *sch = container_of(d, struct sch_gpio, data);
+ unsigned long flags;
+ u32 gpio_num;
+
+ if (d == NULL)
+ return -EINVAL;
How can that happen?
This is just to ensure the irq_data struct contains data. If you think
this is no needed, I will remove it in next submission.
I think you can drop that check. Also if 'd' would be NULL you will
crash already in
struct sch_gpio *sch = container_of(d, struct sch_gpio, data);
(...)
+
+static void sch_gpio_irqs_init(struct sch_gpio *sch, unsigned int
+num) {
+ unsigned int i;
+
+ for (i = 0; i num; i++) {
+ irq_set_chip_data(i + sch-irq_base, sch);
+ irq_set_chip_and_handler_name(i + sch-irq_base,
+ sch_irq_chip,
+ handle_simple_irq,
+ sch_gpio_irq_chip);
Hmm, I wonder if this
irq_set_chip_and_handler_name(i + sch-irq_base,
sch_irq_chip,
handle_simple_irq,
sch_gpio_irq_chip);
looks better. Up to you.
I think I will take what you've suggested. :)
(...)
static int sch_gpio_probe(struct platform_device *pdev) {
struct sch_gpio *sch;
- struct resource *res;
+ struct resource *res, *irq;
+ int err;
sch = devm_kzalloc(pdev-dev, sizeof(*sch), GFP_KERNEL);
if (!sch)
@@ -203,6 +376,17 @@ static int sch_gpio_probe(struct platform_device
*pdev)
pdev-name))
return -EBUSY;
+ irq = platform_get_resource(pdev, IORESOURCE_IRQ, 0);
+ sch-irq_support = !!irq;
+ if (sch-irq_support) {
+ sch-irq_num = irq-start;
+ if (sch-irq_num 0) {
Is this really possible? I mean can't you just detect the irq support by
looking
sch-irq_num? If it is 0 then it has interrupt support. I think that irq
0 is not
valid.
Yes, I can do that, but the irq_num was being passed from MFD LPC_SCH.
This implementation is to avoid enabling irq support if LPC_SCH passed
in a negative value. As for irq 0, I think it is valid. Based on my
observation in cat /proc/interrupt there is a 0 irq line available.
I see.
In that case, how about following?
irq = platform_get_irq(pdev, 0)
if (irq = 0) {
/* Add irq support here */
}
+ dev_warn(pdev-dev,
+ failed to obtain irq number for device\n);
+ sch-irq_support = false;
+ }
+ }
+
spin_lock_init(sch-lock);
sch-iobase = res-start;
sch-chip = sch_gpio_chip;
@@ -251,15 +435,64 @@ static int sch_gpio_probe(struct
RE: [PATCHv3 3/3] gpio: sch: Enable IRQ support for Quark X1000
> > +
> > +static int sch_gpio_irq_type(struct irq_data *d, unsigned type)
> > +{
> > + struct sch_gpio *sch = container_of(d, struct sch_gpio, data);
> > + unsigned long flags;
> > + u32 gpio_num;
> > +
> > + if (d == NULL)
> > + return -EINVAL;
> > +
> > + gpio_num = d->irq - sch->irq_base;
> > +
> > + spin_lock_irqsave(>lock, flags);
> > +
> > + switch (type) {
> > + case IRQ_TYPE_EDGE_RISING:
> > + sch_gpio_register_set(sch, gpio_num, GTPE);
> > + sch_gpio_register_clear(sch, gpio_num, GTNE);
>
> You will have the same problem as I pointed out in patch 1/3 that
> sch_gpio_register_set/clear() will try to acquire the already-locked
> sch->lock. No way this can ever work, or I am under a serious
> misapprehension.
>
> > + break;
> > +
> > + case IRQ_TYPE_EDGE_FALLING:
> > + sch_gpio_register_set(sch, gpio_num, GTNE);
> > + sch_gpio_register_clear(sch, gpio_num, GTPE);
> > + break;
> > +
> > + case IRQ_TYPE_EDGE_BOTH:
> > + sch_gpio_register_set(sch, gpio_num, GTPE);
> > + sch_gpio_register_set(sch, gpio_num, GTNE);
> > + break;
> > +
> > + case IRQ_TYPE_NONE:
> > + sch_gpio_register_clear(sch, gpio_num, GTPE);
> > + sch_gpio_register_clear(sch, gpio_num, GTNE);
> > + break;
> > +
> > + default:
> > + spin_unlock_irqrestore(>lock, flags);
> > + return -EINVAL;
> > + }
> > +
> > + spin_unlock_irqrestore(>lock, flags);
> > +
> > + return 0;
> > +}
> > +
> > +static struct irq_chip sch_irq_chip = {
> > + .irq_enable = sch_gpio_irq_enable,
> > + .irq_disable= sch_gpio_irq_disable,
> > + .irq_ack= sch_gpio_irq_ack,
> > + .irq_set_type = sch_gpio_irq_type,
> > +};
> > +
> > +static void sch_gpio_irqs_init(struct sch_gpio *sch, unsigned int num)
> > +{
> > + unsigned int i;
> > +
> > + for (i = 0; i < num; i++) {
> > + irq_set_chip_data(i + sch->irq_base, sch);
> > + irq_set_chip_and_handler_name(i + sch->irq_base,
> > + _irq_chip,
> > + handle_simple_irq,
> > + "sch_gpio_irq_chip");
> > + }
> > +}
> > +
> > +static void sch_gpio_irqs_deinit(struct sch_gpio *sch, unsigned int num)
> > +{
> > + unsigned int i;
> > +
> > + for (i = 0; i < num; i++) {
> > + irq_set_chip_data(i + sch->irq_base, 0);
> > + irq_set_chip_and_handler_name(i + sch->irq_base, 0, 0, 0);
> > + }
> > +}
> > +
> > +static void sch_gpio_irq_disable_all(struct sch_gpio *sch, unsigned int
> > num)
> > +{
> > + unsigned long flags;
> > + unsigned int gpio_num;
> > +
> > + spin_lock_irqsave(>lock, flags);
> > +
> > + for (gpio_num = 0; gpio_num < num; gpio_num++) {
> > + sch_gpio_register_clear(sch, gpio_num, GTPE);
> > + sch_gpio_register_clear(sch, gpio_num, GTNE);
> > + sch_gpio_register_clear(sch, gpio_num, GGPE);
> > + sch_gpio_register_clear(sch, gpio_num, GSMI);
>
> Same here.
Alright. I should have noticed this double locking issue earlier. Thanks. I
think the next version will be much cleaner. Thanks for spending time and
effort to review.
Rebecca
RE: [PATCHv3 3/3] gpio: sch: Enable IRQ support for Quark X1000
+
+static int sch_gpio_irq_type(struct irq_data *d, unsigned type)
+{
+ struct sch_gpio *sch = container_of(d, struct sch_gpio, data);
+ unsigned long flags;
+ u32 gpio_num;
+
+ if (d == NULL)
+ return -EINVAL;
+
+ gpio_num = d-irq - sch-irq_base;
+
+ spin_lock_irqsave(sch-lock, flags);
+
+ switch (type) {
+ case IRQ_TYPE_EDGE_RISING:
+ sch_gpio_register_set(sch, gpio_num, GTPE);
+ sch_gpio_register_clear(sch, gpio_num, GTNE);
You will have the same problem as I pointed out in patch 1/3 that
sch_gpio_register_set/clear() will try to acquire the already-locked
sch-lock. No way this can ever work, or I am under a serious
misapprehension.
+ break;
+
+ case IRQ_TYPE_EDGE_FALLING:
+ sch_gpio_register_set(sch, gpio_num, GTNE);
+ sch_gpio_register_clear(sch, gpio_num, GTPE);
+ break;
+
+ case IRQ_TYPE_EDGE_BOTH:
+ sch_gpio_register_set(sch, gpio_num, GTPE);
+ sch_gpio_register_set(sch, gpio_num, GTNE);
+ break;
+
+ case IRQ_TYPE_NONE:
+ sch_gpio_register_clear(sch, gpio_num, GTPE);
+ sch_gpio_register_clear(sch, gpio_num, GTNE);
+ break;
+
+ default:
+ spin_unlock_irqrestore(sch-lock, flags);
+ return -EINVAL;
+ }
+
+ spin_unlock_irqrestore(sch-lock, flags);
+
+ return 0;
+}
+
+static struct irq_chip sch_irq_chip = {
+ .irq_enable = sch_gpio_irq_enable,
+ .irq_disable= sch_gpio_irq_disable,
+ .irq_ack= sch_gpio_irq_ack,
+ .irq_set_type = sch_gpio_irq_type,
+};
+
+static void sch_gpio_irqs_init(struct sch_gpio *sch, unsigned int num)
+{
+ unsigned int i;
+
+ for (i = 0; i num; i++) {
+ irq_set_chip_data(i + sch-irq_base, sch);
+ irq_set_chip_and_handler_name(i + sch-irq_base,
+ sch_irq_chip,
+ handle_simple_irq,
+ sch_gpio_irq_chip);
+ }
+}
+
+static void sch_gpio_irqs_deinit(struct sch_gpio *sch, unsigned int num)
+{
+ unsigned int i;
+
+ for (i = 0; i num; i++) {
+ irq_set_chip_data(i + sch-irq_base, 0);
+ irq_set_chip_and_handler_name(i + sch-irq_base, 0, 0, 0);
+ }
+}
+
+static void sch_gpio_irq_disable_all(struct sch_gpio *sch, unsigned int
num)
+{
+ unsigned long flags;
+ unsigned int gpio_num;
+
+ spin_lock_irqsave(sch-lock, flags);
+
+ for (gpio_num = 0; gpio_num num; gpio_num++) {
+ sch_gpio_register_clear(sch, gpio_num, GTPE);
+ sch_gpio_register_clear(sch, gpio_num, GTNE);
+ sch_gpio_register_clear(sch, gpio_num, GGPE);
+ sch_gpio_register_clear(sch, gpio_num, GSMI);
Same here.
Alright. I should have noticed this double locking issue earlier. Thanks. I
think the next version will be much cleaner. Thanks for spending time and
effort to review.
Rebecca
Re: [PATCHv3 3/3] gpio: sch: Enable IRQ support for Quark X1000
On Thu, Oct 16, 2014 at 12:51 PM, Chang Rebecca Swee Fun
wrote:
> Intel Quark X1000 GPIO controller supports interrupt handling for
> both core power well and resume power well. This patch is to enable
> the IRQ support and provide IRQ handling for Intel Quark X1000
> GPIO-SCH device driver.
>
> This piece of work is derived from Dan O'Donovan's initial work for
> Quark X1000 enabling.
>
> Signed-off-by: Chang Rebecca Swee Fun
> ---
> drivers/gpio/gpio-sch.c | 257
> ---
> 1 file changed, 245 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/gpio/gpio-sch.c b/drivers/gpio/gpio-sch.c
> index 952990f..dd84b1f 100644
> --- a/drivers/gpio/gpio-sch.c
> +++ b/drivers/gpio/gpio-sch.c
> @@ -28,17 +28,30 @@
> #include
>
> #include
> +#include
> +#include
>
> #define GEN0x00
> #define GIO0x04
> #define GLV0x08
> +#define GTPE 0x0C
> +#define GTNE 0x10
> +#define GGPE 0x14
> +#define GSMI 0x18
> +#define GTS0x1C
> +#define CGNMIEN0x40
> +#define RGNMIEN0x44
>
> struct sch_gpio {
> struct gpio_chip chip;
> + struct irq_data data;
> spinlock_t lock;
> unsigned short iobase;
> unsigned short core_base;
> unsigned short resume_base;
> + int irq_base;
> + int irq_num;
> + bool irq_support;
> };
>
> #define to_sch_gpio(c) container_of(c, struct sch_gpio, chip)
> @@ -66,10 +79,11 @@ static unsigned sch_gpio_bit(struct sch_gpio *sch,
> unsigned gpio)
> static void sch_gpio_register_set(struct sch_gpio *sch, unsigned gpio,
> unsigned reg)
> {
> + unsigned long flags;
> unsigned short offset, bit;
> u8 enable;
>
> - spin_lock(>lock);
> + spin_lock_irqsave(>lock, flags);
>
> offset = sch_gpio_offset(sch, gpio, reg);
> bit = sch_gpio_bit(sch, gpio);
> @@ -78,16 +92,17 @@ static void sch_gpio_register_set(struct sch_gpio *sch,
> unsigned gpio,
> if (!(enable & BIT(bit)))
> outb(enable | BIT(bit), sch->iobase + offset);
>
> - spin_unlock(>lock);
> + spin_unlock_irqrestore(>lock, flags);
> }
>
> static void sch_gpio_register_clear(struct sch_gpio *sch, unsigned gpio,
> unsigned reg)
> {
> + unsigned long flags;
> unsigned short offset, bit;
> u8 disable;
>
> - spin_lock(>lock);
> + spin_lock_irqsave(>lock, flags);
>
> offset = sch_gpio_offset(sch, gpio, reg);
> bit = sch_gpio_bit(sch, gpio);
> @@ -96,7 +111,7 @@ static void sch_gpio_register_clear(struct sch_gpio *sch,
> unsigned gpio,
> if (disable & BIT(bit))
> outb(disable & ~BIT(bit), sch->iobase + offset);
>
> - spin_unlock(>lock);
> + spin_unlock_irqrestore(>lock, flags);
> }
>
> static int sch_gpio_reg_get(struct gpio_chip *gc, unsigned gpio, unsigned
> reg)
> @@ -134,10 +149,11 @@ static void sch_gpio_reg_set(struct gpio_chip *gc,
> unsigned gpio, unsigned reg,
> static int sch_gpio_direction_in(struct gpio_chip *gc, unsigned gpio_num)
> {
> struct sch_gpio *sch = to_sch_gpio(gc);
> + unsigned long flags;
>
> - spin_lock(>lock);
> + spin_lock_irqsave(>lock, flags);
> sch_gpio_register_set(sch, gpio_num, GIO);
> - spin_unlock(>lock);
> + spin_unlock_irqrestore(>lock, flags);
> return 0;
> }
>
> @@ -149,20 +165,22 @@ static int sch_gpio_get(struct gpio_chip *gc, unsigned
> gpio_num)
> static void sch_gpio_set(struct gpio_chip *gc, unsigned gpio_num, int val)
> {
> struct sch_gpio *sch = to_sch_gpio(gc);
> + unsigned long flags;
>
> - spin_lock(>lock);
> + spin_lock_irqsave(>lock, flags);
> sch_gpio_reg_set(gc, gpio_num, GLV, val);
> - spin_unlock(>lock);
> + spin_unlock_irqrestore(>lock, flags);
> }
>
> static int sch_gpio_direction_out(struct gpio_chip *gc, unsigned gpio_num,
> int val)
> {
> struct sch_gpio *sch = to_sch_gpio(gc);
> + unsigned long flags;
>
> - spin_lock(>lock);
> + spin_lock_irqsave(>lock, flags);
> sch_gpio_register_clear(sch, gpio_num, GIO);
> - spin_unlock(>lock);
> + spin_unlock_irqrestore(>lock, flags);
>
> /*
> * according to the datasheet, writing to the level register has no
> @@ -177,6 +195,13 @@ static int sch_gpio_direction_out(struct gpio_chip *gc,
> unsigned gpio_num,
> return 0;
> }
>
> +static int sch_gpio_to_irq(struct gpio_chip *gc, unsigned offset)
> +{
> + struct sch_gpio *sch = to_sch_gpio(gc);
> +
> + return sch->irq_base + offset;
> +}
> +
> static struct gpio_chip sch_gpio_chip = {
> .label = "sch_gpio",
> .owner = THIS_MODULE,
> @@ -184,12 +209,160 @@ static struct gpio_chip sch_gpio_chip = {
> .get= sch_gpio_get,
Re: [PATCHv3 3/3] gpio: sch: Enable IRQ support for Quark X1000
On Thu, Oct 16, 2014 at 12:51 PM, Chang Rebecca Swee Fun
rebecca.swee.fun.ch...@intel.com wrote:
Intel Quark X1000 GPIO controller supports interrupt handling for
both core power well and resume power well. This patch is to enable
the IRQ support and provide IRQ handling for Intel Quark X1000
GPIO-SCH device driver.
This piece of work is derived from Dan O'Donovan's initial work for
Quark X1000 enabling.
Signed-off-by: Chang Rebecca Swee Fun rebecca.swee.fun.ch...@intel.com
---
drivers/gpio/gpio-sch.c | 257
---
1 file changed, 245 insertions(+), 12 deletions(-)
diff --git a/drivers/gpio/gpio-sch.c b/drivers/gpio/gpio-sch.c
index 952990f..dd84b1f 100644
--- a/drivers/gpio/gpio-sch.c
+++ b/drivers/gpio/gpio-sch.c
@@ -28,17 +28,30 @@
#include linux/pci_ids.h
#include linux/gpio.h
+#include linux/interrupt.h
+#include linux/irq.h
#define GEN0x00
#define GIO0x04
#define GLV0x08
+#define GTPE 0x0C
+#define GTNE 0x10
+#define GGPE 0x14
+#define GSMI 0x18
+#define GTS0x1C
+#define CGNMIEN0x40
+#define RGNMIEN0x44
struct sch_gpio {
struct gpio_chip chip;
+ struct irq_data data;
spinlock_t lock;
unsigned short iobase;
unsigned short core_base;
unsigned short resume_base;
+ int irq_base;
+ int irq_num;
+ bool irq_support;
};
#define to_sch_gpio(c) container_of(c, struct sch_gpio, chip)
@@ -66,10 +79,11 @@ static unsigned sch_gpio_bit(struct sch_gpio *sch,
unsigned gpio)
static void sch_gpio_register_set(struct sch_gpio *sch, unsigned gpio,
unsigned reg)
{
+ unsigned long flags;
unsigned short offset, bit;
u8 enable;
- spin_lock(sch-lock);
+ spin_lock_irqsave(sch-lock, flags);
offset = sch_gpio_offset(sch, gpio, reg);
bit = sch_gpio_bit(sch, gpio);
@@ -78,16 +92,17 @@ static void sch_gpio_register_set(struct sch_gpio *sch,
unsigned gpio,
if (!(enable BIT(bit)))
outb(enable | BIT(bit), sch-iobase + offset);
- spin_unlock(sch-lock);
+ spin_unlock_irqrestore(sch-lock, flags);
}
static void sch_gpio_register_clear(struct sch_gpio *sch, unsigned gpio,
unsigned reg)
{
+ unsigned long flags;
unsigned short offset, bit;
u8 disable;
- spin_lock(sch-lock);
+ spin_lock_irqsave(sch-lock, flags);
offset = sch_gpio_offset(sch, gpio, reg);
bit = sch_gpio_bit(sch, gpio);
@@ -96,7 +111,7 @@ static void sch_gpio_register_clear(struct sch_gpio *sch,
unsigned gpio,
if (disable BIT(bit))
outb(disable ~BIT(bit), sch-iobase + offset);
- spin_unlock(sch-lock);
+ spin_unlock_irqrestore(sch-lock, flags);
}
static int sch_gpio_reg_get(struct gpio_chip *gc, unsigned gpio, unsigned
reg)
@@ -134,10 +149,11 @@ static void sch_gpio_reg_set(struct gpio_chip *gc,
unsigned gpio, unsigned reg,
static int sch_gpio_direction_in(struct gpio_chip *gc, unsigned gpio_num)
{
struct sch_gpio *sch = to_sch_gpio(gc);
+ unsigned long flags;
- spin_lock(sch-lock);
+ spin_lock_irqsave(sch-lock, flags);
sch_gpio_register_set(sch, gpio_num, GIO);
- spin_unlock(sch-lock);
+ spin_unlock_irqrestore(sch-lock, flags);
return 0;
}
@@ -149,20 +165,22 @@ static int sch_gpio_get(struct gpio_chip *gc, unsigned
gpio_num)
static void sch_gpio_set(struct gpio_chip *gc, unsigned gpio_num, int val)
{
struct sch_gpio *sch = to_sch_gpio(gc);
+ unsigned long flags;
- spin_lock(sch-lock);
+ spin_lock_irqsave(sch-lock, flags);
sch_gpio_reg_set(gc, gpio_num, GLV, val);
- spin_unlock(sch-lock);
+ spin_unlock_irqrestore(sch-lock, flags);
}
static int sch_gpio_direction_out(struct gpio_chip *gc, unsigned gpio_num,
int val)
{
struct sch_gpio *sch = to_sch_gpio(gc);
+ unsigned long flags;
- spin_lock(sch-lock);
+ spin_lock_irqsave(sch-lock, flags);
sch_gpio_register_clear(sch, gpio_num, GIO);
- spin_unlock(sch-lock);
+ spin_unlock_irqrestore(sch-lock, flags);
/*
* according to the datasheet, writing to the level register has no
@@ -177,6 +195,13 @@ static int sch_gpio_direction_out(struct gpio_chip *gc,
unsigned gpio_num,
return 0;
}
+static int sch_gpio_to_irq(struct gpio_chip *gc, unsigned offset)
+{
+ struct sch_gpio *sch = to_sch_gpio(gc);
+
+ return sch-irq_base + offset;
+}
+
static struct gpio_chip sch_gpio_chip = {
.label = sch_gpio,
.owner = THIS_MODULE,
@@ -184,12 +209,160 @@ static struct gpio_chip sch_gpio_chip = {
.get
Re: [PATCHv3 3/3] gpio: sch: Enable IRQ support for Quark X1000
On Thu, Oct 16, 2014 at 11:51:15AM +0800, Chang Rebecca Swee Fun wrote:
> Intel Quark X1000 GPIO controller supports interrupt handling for
> both core power well and resume power well. This patch is to enable
> the IRQ support and provide IRQ handling for Intel Quark X1000
> GPIO-SCH device driver.
>
> This piece of work is derived from Dan O'Donovan's initial work for
> Quark X1000 enabling.
>
> Signed-off-by: Chang Rebecca Swee Fun
In addition to what Varka Bhadram pointed out, I have few comments. See
below.
Overall, looks good.
> ---
> drivers/gpio/gpio-sch.c | 257
> ---
> 1 file changed, 245 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/gpio/gpio-sch.c b/drivers/gpio/gpio-sch.c
> index 952990f..dd84b1f 100644
> --- a/drivers/gpio/gpio-sch.c
> +++ b/drivers/gpio/gpio-sch.c
> @@ -28,17 +28,30 @@
> #include
>
> #include
> +#include
> +#include
>
> #define GEN 0x00
> #define GIO 0x04
> #define GLV 0x08
> +#define GTPE 0x0C
> +#define GTNE 0x10
> +#define GGPE 0x14
> +#define GSMI 0x18
> +#define GTS 0x1C
> +#define CGNMIEN 0x40
> +#define RGNMIEN 0x44
>
> struct sch_gpio {
> struct gpio_chip chip;
> + struct irq_data data;
> spinlock_t lock;
> unsigned short iobase;
> unsigned short core_base;
> unsigned short resume_base;
> + int irq_base;
> + int irq_num;
> + bool irq_support;
> };
>
> #define to_sch_gpio(c) container_of(c, struct sch_gpio, chip)
> @@ -66,10 +79,11 @@ static unsigned sch_gpio_bit(struct sch_gpio *sch,
> unsigned gpio)
> static void sch_gpio_register_set(struct sch_gpio *sch, unsigned gpio,
> unsigned reg)
> {
> + unsigned long flags;
> unsigned short offset, bit;
> u8 enable;
>
> - spin_lock(>lock);
> + spin_lock_irqsave(>lock, flags);
>
> offset = sch_gpio_offset(sch, gpio, reg);
> bit = sch_gpio_bit(sch, gpio);
> @@ -78,16 +92,17 @@ static void sch_gpio_register_set(struct sch_gpio *sch,
> unsigned gpio,
> if (!(enable & BIT(bit)))
> outb(enable | BIT(bit), sch->iobase + offset);
>
> - spin_unlock(>lock);
> + spin_unlock_irqrestore(>lock, flags);
> }
>
> static void sch_gpio_register_clear(struct sch_gpio *sch, unsigned gpio,
> unsigned reg)
> {
> + unsigned long flags;
> unsigned short offset, bit;
> u8 disable;
>
> - spin_lock(>lock);
> + spin_lock_irqsave(>lock, flags);
>
> offset = sch_gpio_offset(sch, gpio, reg);
> bit = sch_gpio_bit(sch, gpio);
> @@ -96,7 +111,7 @@ static void sch_gpio_register_clear(struct sch_gpio *sch,
> unsigned gpio,
> if (disable & BIT(bit))
> outb(disable & ~BIT(bit), sch->iobase + offset);
>
> - spin_unlock(>lock);
> + spin_unlock_irqrestore(>lock, flags);
> }
>
> static int sch_gpio_reg_get(struct gpio_chip *gc, unsigned gpio, unsigned
> reg)
> @@ -134,10 +149,11 @@ static void sch_gpio_reg_set(struct gpio_chip *gc,
> unsigned gpio, unsigned reg,
> static int sch_gpio_direction_in(struct gpio_chip *gc, unsigned gpio_num)
> {
> struct sch_gpio *sch = to_sch_gpio(gc);
> + unsigned long flags;
>
> - spin_lock(>lock);
> + spin_lock_irqsave(>lock, flags);
> sch_gpio_register_set(sch, gpio_num, GIO);
> - spin_unlock(>lock);
> + spin_unlock_irqrestore(>lock, flags);
> return 0;
> }
>
> @@ -149,20 +165,22 @@ static int sch_gpio_get(struct gpio_chip *gc, unsigned
> gpio_num)
> static void sch_gpio_set(struct gpio_chip *gc, unsigned gpio_num, int val)
> {
> struct sch_gpio *sch = to_sch_gpio(gc);
> + unsigned long flags;
>
> - spin_lock(>lock);
> + spin_lock_irqsave(>lock, flags);
> sch_gpio_reg_set(gc, gpio_num, GLV, val);
> - spin_unlock(>lock);
> + spin_unlock_irqrestore(>lock, flags);
> }
>
> static int sch_gpio_direction_out(struct gpio_chip *gc, unsigned gpio_num,
> int val)
> {
> struct sch_gpio *sch = to_sch_gpio(gc);
> + unsigned long flags;
>
> - spin_lock(>lock);
> + spin_lock_irqsave(>lock, flags);
> sch_gpio_register_clear(sch, gpio_num, GIO);
> - spin_unlock(>lock);
> + spin_unlock_irqrestore(>lock, flags);
>
> /*
>* according to the datasheet, writing to the level register has no
> @@ -177,6 +195,13 @@ static int sch_gpio_direction_out(struct gpio_chip *gc,
> unsigned gpio_num,
> return 0;
> }
>
> +static int sch_gpio_to_irq(struct gpio_chip *gc, unsigned offset)
> +{
> + struct sch_gpio *sch = to_sch_gpio(gc);
> +
> + return sch->irq_base + offset;
> +}
> +
> static struct gpio_chip sch_gpio_chip = {
> .label = "sch_gpio",
> .owner = THIS_MODULE,
> @@ -184,12 +209,160 @@ static struct gpio_chip sch_gpio_chip = {
> .get=
Re: [PATCHv3 3/3] gpio: sch: Enable IRQ support for Quark X1000
On Thu, Oct 16, 2014 at 11:51:15AM +0800, Chang Rebecca Swee Fun wrote:
Intel Quark X1000 GPIO controller supports interrupt handling for
both core power well and resume power well. This patch is to enable
the IRQ support and provide IRQ handling for Intel Quark X1000
GPIO-SCH device driver.
This piece of work is derived from Dan O'Donovan's initial work for
Quark X1000 enabling.
Signed-off-by: Chang Rebecca Swee Fun rebecca.swee.fun.ch...@intel.com
In addition to what Varka Bhadram pointed out, I have few comments. See
below.
Overall, looks good.
---
drivers/gpio/gpio-sch.c | 257
---
1 file changed, 245 insertions(+), 12 deletions(-)
diff --git a/drivers/gpio/gpio-sch.c b/drivers/gpio/gpio-sch.c
index 952990f..dd84b1f 100644
--- a/drivers/gpio/gpio-sch.c
+++ b/drivers/gpio/gpio-sch.c
@@ -28,17 +28,30 @@
#include linux/pci_ids.h
#include linux/gpio.h
+#include linux/interrupt.h
+#include linux/irq.h
#define GEN 0x00
#define GIO 0x04
#define GLV 0x08
+#define GTPE 0x0C
+#define GTNE 0x10
+#define GGPE 0x14
+#define GSMI 0x18
+#define GTS 0x1C
+#define CGNMIEN 0x40
+#define RGNMIEN 0x44
struct sch_gpio {
struct gpio_chip chip;
+ struct irq_data data;
spinlock_t lock;
unsigned short iobase;
unsigned short core_base;
unsigned short resume_base;
+ int irq_base;
+ int irq_num;
+ bool irq_support;
};
#define to_sch_gpio(c) container_of(c, struct sch_gpio, chip)
@@ -66,10 +79,11 @@ static unsigned sch_gpio_bit(struct sch_gpio *sch,
unsigned gpio)
static void sch_gpio_register_set(struct sch_gpio *sch, unsigned gpio,
unsigned reg)
{
+ unsigned long flags;
unsigned short offset, bit;
u8 enable;
- spin_lock(sch-lock);
+ spin_lock_irqsave(sch-lock, flags);
offset = sch_gpio_offset(sch, gpio, reg);
bit = sch_gpio_bit(sch, gpio);
@@ -78,16 +92,17 @@ static void sch_gpio_register_set(struct sch_gpio *sch,
unsigned gpio,
if (!(enable BIT(bit)))
outb(enable | BIT(bit), sch-iobase + offset);
- spin_unlock(sch-lock);
+ spin_unlock_irqrestore(sch-lock, flags);
}
static void sch_gpio_register_clear(struct sch_gpio *sch, unsigned gpio,
unsigned reg)
{
+ unsigned long flags;
unsigned short offset, bit;
u8 disable;
- spin_lock(sch-lock);
+ spin_lock_irqsave(sch-lock, flags);
offset = sch_gpio_offset(sch, gpio, reg);
bit = sch_gpio_bit(sch, gpio);
@@ -96,7 +111,7 @@ static void sch_gpio_register_clear(struct sch_gpio *sch,
unsigned gpio,
if (disable BIT(bit))
outb(disable ~BIT(bit), sch-iobase + offset);
- spin_unlock(sch-lock);
+ spin_unlock_irqrestore(sch-lock, flags);
}
static int sch_gpio_reg_get(struct gpio_chip *gc, unsigned gpio, unsigned
reg)
@@ -134,10 +149,11 @@ static void sch_gpio_reg_set(struct gpio_chip *gc,
unsigned gpio, unsigned reg,
static int sch_gpio_direction_in(struct gpio_chip *gc, unsigned gpio_num)
{
struct sch_gpio *sch = to_sch_gpio(gc);
+ unsigned long flags;
- spin_lock(sch-lock);
+ spin_lock_irqsave(sch-lock, flags);
sch_gpio_register_set(sch, gpio_num, GIO);
- spin_unlock(sch-lock);
+ spin_unlock_irqrestore(sch-lock, flags);
return 0;
}
@@ -149,20 +165,22 @@ static int sch_gpio_get(struct gpio_chip *gc, unsigned
gpio_num)
static void sch_gpio_set(struct gpio_chip *gc, unsigned gpio_num, int val)
{
struct sch_gpio *sch = to_sch_gpio(gc);
+ unsigned long flags;
- spin_lock(sch-lock);
+ spin_lock_irqsave(sch-lock, flags);
sch_gpio_reg_set(gc, gpio_num, GLV, val);
- spin_unlock(sch-lock);
+ spin_unlock_irqrestore(sch-lock, flags);
}
static int sch_gpio_direction_out(struct gpio_chip *gc, unsigned gpio_num,
int val)
{
struct sch_gpio *sch = to_sch_gpio(gc);
+ unsigned long flags;
- spin_lock(sch-lock);
+ spin_lock_irqsave(sch-lock, flags);
sch_gpio_register_clear(sch, gpio_num, GIO);
- spin_unlock(sch-lock);
+ spin_unlock_irqrestore(sch-lock, flags);
/*
* according to the datasheet, writing to the level register has no
@@ -177,6 +195,13 @@ static int sch_gpio_direction_out(struct gpio_chip *gc,
unsigned gpio_num,
return 0;
}
+static int sch_gpio_to_irq(struct gpio_chip *gc, unsigned offset)
+{
+ struct sch_gpio *sch = to_sch_gpio(gc);
+
+ return sch-irq_base + offset;
+}
+
static struct gpio_chip sch_gpio_chip = {
.label = sch_gpio,
.owner = THIS_MODULE,
@@ -184,12 +209,160 @@ static struct gpio_chip sch_gpio_chip = {
.get= sch_gpio_get,
Re: [PATCHv3 3/3] gpio: sch: Enable IRQ support for Quark X1000
On 10/16/2014 09:21 AM, Chang Rebecca Swee Fun wrote:
Intel Quark X1000 GPIO controller supports interrupt handling for
both core power well and resume power well. This patch is to enable
the IRQ support and provide IRQ handling for Intel Quark X1000
GPIO-SCH device driver.
(...)
static int sch_gpio_probe(struct platform_device *pdev)
{
struct sch_gpio *sch;
- struct resource *res;
+ struct resource *res, *irq;
+ int err;
sch = devm_kzalloc(>dev, sizeof(*sch), GFP_KERNEL);
if (!sch)
@@ -203,6 +376,17 @@ static int sch_gpio_probe(struct platform_device *pdev)
pdev->name))
return -EBUSY;
+ irq = platform_get_resource(pdev, IORESOURCE_IRQ, 0);
+ sch->irq_support = !!irq;
+ if (sch->irq_support) {
+ sch->irq_num = irq->start;
+ if (sch->irq_num < 0) {
+ dev_warn(>dev,
+"failed to obtain irq number for device\n");
+ sch->irq_support = false;
+ }
+ }
+
spin_lock_init(>lock);
sch->iobase = res->start;
sch->chip = sch_gpio_chip;
@@ -251,15 +435,64 @@ static int sch_gpio_probe(struct platform_device *pdev)
return -ENODEV;
}
+ err = gpiochip_add(>chip);
+ if (err < 0)
+ goto err_sch_gpio;
+
+ if (sch->irq_support) {
+ sch->irq_base = irq_alloc_descs(-1, 0, sch->chip.ngpio,
+ NUMA_NO_NODE);
+ if (sch->irq_base < 0) {
+ dev_err(>dev,
+ "failed to allocate GPIO IRQ descs\n");
+ goto err_sch_intr_chip;
+ }
+
+ /* disable interrupts */
+ sch_gpio_irq_disable_all(sch, sch->chip.ngpio);
+
+ err = request_irq(sch->irq_num, sch_gpio_irq_handler,
+ IRQF_SHARED, KBUILD_MODNAME, sch);
+ if (err) {
+ dev_err(>dev,
+ "%s failed to request IRQ\n", __func__);
+ goto err_sch_request_irq;
+ }
+
+ sch_gpio_irqs_init(sch, sch->chip.ngpio);
+ }
+
platform_set_drvdata(pdev, sch);
- return gpiochip_add(>chip);
+ return 0;
+
+err_sch_request_irq:
+ irq_free_descs(sch->irq_base, sch->chip.ngpio);
+
+err_sch_intr_chip:
+ if (gpiochip_remove(>chip))
gpiochip_remove() return 'void' [0].
+ dev_err(>dev,
+ "%s gpiochip_remove() failed\n", __func__);
+
+err_sch_gpio:
+ release_region(res->start, resource_size(res));
+
+ return err;
}
[0]:https://git.kernel.org/cgit/linux/kernel/git/linusw/linux-gpio.git/tree/drivers/gpio/gpiolib.c#n311
--
Regards,
Varka Bhadram.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCHv3 3/3] gpio: sch: Enable IRQ support for Quark X1000
Intel Quark X1000 GPIO controller supports interrupt handling for
both core power well and resume power well. This patch is to enable
the IRQ support and provide IRQ handling for Intel Quark X1000
GPIO-SCH device driver.
This piece of work is derived from Dan O'Donovan's initial work for
Quark X1000 enabling.
Signed-off-by: Chang Rebecca Swee Fun
---
drivers/gpio/gpio-sch.c | 257 ---
1 file changed, 245 insertions(+), 12 deletions(-)
diff --git a/drivers/gpio/gpio-sch.c b/drivers/gpio/gpio-sch.c
index 952990f..dd84b1f 100644
--- a/drivers/gpio/gpio-sch.c
+++ b/drivers/gpio/gpio-sch.c
@@ -28,17 +28,30 @@
#include
#include
+#include
+#include
#define GEN0x00
#define GIO0x04
#define GLV0x08
+#define GTPE 0x0C
+#define GTNE 0x10
+#define GGPE 0x14
+#define GSMI 0x18
+#define GTS0x1C
+#define CGNMIEN0x40
+#define RGNMIEN0x44
struct sch_gpio {
struct gpio_chip chip;
+ struct irq_data data;
spinlock_t lock;
unsigned short iobase;
unsigned short core_base;
unsigned short resume_base;
+ int irq_base;
+ int irq_num;
+ bool irq_support;
};
#define to_sch_gpio(c) container_of(c, struct sch_gpio, chip)
@@ -66,10 +79,11 @@ static unsigned sch_gpio_bit(struct sch_gpio *sch, unsigned
gpio)
static void sch_gpio_register_set(struct sch_gpio *sch, unsigned gpio,
unsigned reg)
{
+ unsigned long flags;
unsigned short offset, bit;
u8 enable;
- spin_lock(>lock);
+ spin_lock_irqsave(>lock, flags);
offset = sch_gpio_offset(sch, gpio, reg);
bit = sch_gpio_bit(sch, gpio);
@@ -78,16 +92,17 @@ static void sch_gpio_register_set(struct sch_gpio *sch,
unsigned gpio,
if (!(enable & BIT(bit)))
outb(enable | BIT(bit), sch->iobase + offset);
- spin_unlock(>lock);
+ spin_unlock_irqrestore(>lock, flags);
}
static void sch_gpio_register_clear(struct sch_gpio *sch, unsigned gpio,
unsigned reg)
{
+ unsigned long flags;
unsigned short offset, bit;
u8 disable;
- spin_lock(>lock);
+ spin_lock_irqsave(>lock, flags);
offset = sch_gpio_offset(sch, gpio, reg);
bit = sch_gpio_bit(sch, gpio);
@@ -96,7 +111,7 @@ static void sch_gpio_register_clear(struct sch_gpio *sch,
unsigned gpio,
if (disable & BIT(bit))
outb(disable & ~BIT(bit), sch->iobase + offset);
- spin_unlock(>lock);
+ spin_unlock_irqrestore(>lock, flags);
}
static int sch_gpio_reg_get(struct gpio_chip *gc, unsigned gpio, unsigned reg)
@@ -134,10 +149,11 @@ static void sch_gpio_reg_set(struct gpio_chip *gc,
unsigned gpio, unsigned reg,
static int sch_gpio_direction_in(struct gpio_chip *gc, unsigned gpio_num)
{
struct sch_gpio *sch = to_sch_gpio(gc);
+ unsigned long flags;
- spin_lock(>lock);
+ spin_lock_irqsave(>lock, flags);
sch_gpio_register_set(sch, gpio_num, GIO);
- spin_unlock(>lock);
+ spin_unlock_irqrestore(>lock, flags);
return 0;
}
@@ -149,20 +165,22 @@ static int sch_gpio_get(struct gpio_chip *gc, unsigned
gpio_num)
static void sch_gpio_set(struct gpio_chip *gc, unsigned gpio_num, int val)
{
struct sch_gpio *sch = to_sch_gpio(gc);
+ unsigned long flags;
- spin_lock(>lock);
+ spin_lock_irqsave(>lock, flags);
sch_gpio_reg_set(gc, gpio_num, GLV, val);
- spin_unlock(>lock);
+ spin_unlock_irqrestore(>lock, flags);
}
static int sch_gpio_direction_out(struct gpio_chip *gc, unsigned gpio_num,
int val)
{
struct sch_gpio *sch = to_sch_gpio(gc);
+ unsigned long flags;
- spin_lock(>lock);
+ spin_lock_irqsave(>lock, flags);
sch_gpio_register_clear(sch, gpio_num, GIO);
- spin_unlock(>lock);
+ spin_unlock_irqrestore(>lock, flags);
/*
* according to the datasheet, writing to the level register has no
@@ -177,6 +195,13 @@ static int sch_gpio_direction_out(struct gpio_chip *gc,
unsigned gpio_num,
return 0;
}
+static int sch_gpio_to_irq(struct gpio_chip *gc, unsigned offset)
+{
+ struct sch_gpio *sch = to_sch_gpio(gc);
+
+ return sch->irq_base + offset;
+}
+
static struct gpio_chip sch_gpio_chip = {
.label = "sch_gpio",
.owner = THIS_MODULE,
@@ -184,12 +209,160 @@ static struct gpio_chip sch_gpio_chip = {
.get= sch_gpio_get,
.direction_output = sch_gpio_direction_out,
.set= sch_gpio_set,
+ .to_irq = sch_gpio_to_irq,
};
+static void sch_gpio_irq_enable(struct irq_data *d)
+{
+ struct sch_gpio *sch = container_of(d, struct sch_gpio, data);
+ u32 gpio_num;
+
+ gpio_num =
[PATCHv3 3/3] gpio: sch: Enable IRQ support for Quark X1000
Intel Quark X1000 GPIO controller supports interrupt handling for
both core power well and resume power well. This patch is to enable
the IRQ support and provide IRQ handling for Intel Quark X1000
GPIO-SCH device driver.
This piece of work is derived from Dan O'Donovan's initial work for
Quark X1000 enabling.
Signed-off-by: Chang Rebecca Swee Fun rebecca.swee.fun.ch...@intel.com
---
drivers/gpio/gpio-sch.c | 257 ---
1 file changed, 245 insertions(+), 12 deletions(-)
diff --git a/drivers/gpio/gpio-sch.c b/drivers/gpio/gpio-sch.c
index 952990f..dd84b1f 100644
--- a/drivers/gpio/gpio-sch.c
+++ b/drivers/gpio/gpio-sch.c
@@ -28,17 +28,30 @@
#include linux/pci_ids.h
#include linux/gpio.h
+#include linux/interrupt.h
+#include linux/irq.h
#define GEN0x00
#define GIO0x04
#define GLV0x08
+#define GTPE 0x0C
+#define GTNE 0x10
+#define GGPE 0x14
+#define GSMI 0x18
+#define GTS0x1C
+#define CGNMIEN0x40
+#define RGNMIEN0x44
struct sch_gpio {
struct gpio_chip chip;
+ struct irq_data data;
spinlock_t lock;
unsigned short iobase;
unsigned short core_base;
unsigned short resume_base;
+ int irq_base;
+ int irq_num;
+ bool irq_support;
};
#define to_sch_gpio(c) container_of(c, struct sch_gpio, chip)
@@ -66,10 +79,11 @@ static unsigned sch_gpio_bit(struct sch_gpio *sch, unsigned
gpio)
static void sch_gpio_register_set(struct sch_gpio *sch, unsigned gpio,
unsigned reg)
{
+ unsigned long flags;
unsigned short offset, bit;
u8 enable;
- spin_lock(sch-lock);
+ spin_lock_irqsave(sch-lock, flags);
offset = sch_gpio_offset(sch, gpio, reg);
bit = sch_gpio_bit(sch, gpio);
@@ -78,16 +92,17 @@ static void sch_gpio_register_set(struct sch_gpio *sch,
unsigned gpio,
if (!(enable BIT(bit)))
outb(enable | BIT(bit), sch-iobase + offset);
- spin_unlock(sch-lock);
+ spin_unlock_irqrestore(sch-lock, flags);
}
static void sch_gpio_register_clear(struct sch_gpio *sch, unsigned gpio,
unsigned reg)
{
+ unsigned long flags;
unsigned short offset, bit;
u8 disable;
- spin_lock(sch-lock);
+ spin_lock_irqsave(sch-lock, flags);
offset = sch_gpio_offset(sch, gpio, reg);
bit = sch_gpio_bit(sch, gpio);
@@ -96,7 +111,7 @@ static void sch_gpio_register_clear(struct sch_gpio *sch,
unsigned gpio,
if (disable BIT(bit))
outb(disable ~BIT(bit), sch-iobase + offset);
- spin_unlock(sch-lock);
+ spin_unlock_irqrestore(sch-lock, flags);
}
static int sch_gpio_reg_get(struct gpio_chip *gc, unsigned gpio, unsigned reg)
@@ -134,10 +149,11 @@ static void sch_gpio_reg_set(struct gpio_chip *gc,
unsigned gpio, unsigned reg,
static int sch_gpio_direction_in(struct gpio_chip *gc, unsigned gpio_num)
{
struct sch_gpio *sch = to_sch_gpio(gc);
+ unsigned long flags;
- spin_lock(sch-lock);
+ spin_lock_irqsave(sch-lock, flags);
sch_gpio_register_set(sch, gpio_num, GIO);
- spin_unlock(sch-lock);
+ spin_unlock_irqrestore(sch-lock, flags);
return 0;
}
@@ -149,20 +165,22 @@ static int sch_gpio_get(struct gpio_chip *gc, unsigned
gpio_num)
static void sch_gpio_set(struct gpio_chip *gc, unsigned gpio_num, int val)
{
struct sch_gpio *sch = to_sch_gpio(gc);
+ unsigned long flags;
- spin_lock(sch-lock);
+ spin_lock_irqsave(sch-lock, flags);
sch_gpio_reg_set(gc, gpio_num, GLV, val);
- spin_unlock(sch-lock);
+ spin_unlock_irqrestore(sch-lock, flags);
}
static int sch_gpio_direction_out(struct gpio_chip *gc, unsigned gpio_num,
int val)
{
struct sch_gpio *sch = to_sch_gpio(gc);
+ unsigned long flags;
- spin_lock(sch-lock);
+ spin_lock_irqsave(sch-lock, flags);
sch_gpio_register_clear(sch, gpio_num, GIO);
- spin_unlock(sch-lock);
+ spin_unlock_irqrestore(sch-lock, flags);
/*
* according to the datasheet, writing to the level register has no
@@ -177,6 +195,13 @@ static int sch_gpio_direction_out(struct gpio_chip *gc,
unsigned gpio_num,
return 0;
}
+static int sch_gpio_to_irq(struct gpio_chip *gc, unsigned offset)
+{
+ struct sch_gpio *sch = to_sch_gpio(gc);
+
+ return sch-irq_base + offset;
+}
+
static struct gpio_chip sch_gpio_chip = {
.label = sch_gpio,
.owner = THIS_MODULE,
@@ -184,12 +209,160 @@ static struct gpio_chip sch_gpio_chip = {
.get= sch_gpio_get,
.direction_output = sch_gpio_direction_out,
.set= sch_gpio_set,
+ .to_irq = sch_gpio_to_irq,
};
+static void
Re: [PATCHv3 3/3] gpio: sch: Enable IRQ support for Quark X1000
On 10/16/2014 09:21 AM, Chang Rebecca Swee Fun wrote:
Intel Quark X1000 GPIO controller supports interrupt handling for
both core power well and resume power well. This patch is to enable
the IRQ support and provide IRQ handling for Intel Quark X1000
GPIO-SCH device driver.
(...)
static int sch_gpio_probe(struct platform_device *pdev)
{
struct sch_gpio *sch;
- struct resource *res;
+ struct resource *res, *irq;
+ int err;
sch = devm_kzalloc(pdev-dev, sizeof(*sch), GFP_KERNEL);
if (!sch)
@@ -203,6 +376,17 @@ static int sch_gpio_probe(struct platform_device *pdev)
pdev-name))
return -EBUSY;
+ irq = platform_get_resource(pdev, IORESOURCE_IRQ, 0);
+ sch-irq_support = !!irq;
+ if (sch-irq_support) {
+ sch-irq_num = irq-start;
+ if (sch-irq_num 0) {
+ dev_warn(pdev-dev,
+failed to obtain irq number for device\n);
+ sch-irq_support = false;
+ }
+ }
+
spin_lock_init(sch-lock);
sch-iobase = res-start;
sch-chip = sch_gpio_chip;
@@ -251,15 +435,64 @@ static int sch_gpio_probe(struct platform_device *pdev)
return -ENODEV;
}
+ err = gpiochip_add(sch-chip);
+ if (err 0)
+ goto err_sch_gpio;
+
+ if (sch-irq_support) {
+ sch-irq_base = irq_alloc_descs(-1, 0, sch-chip.ngpio,
+ NUMA_NO_NODE);
+ if (sch-irq_base 0) {
+ dev_err(pdev-dev,
+ failed to allocate GPIO IRQ descs\n);
+ goto err_sch_intr_chip;
+ }
+
+ /* disable interrupts */
+ sch_gpio_irq_disable_all(sch, sch-chip.ngpio);
+
+ err = request_irq(sch-irq_num, sch_gpio_irq_handler,
+ IRQF_SHARED, KBUILD_MODNAME, sch);
+ if (err) {
+ dev_err(pdev-dev,
+ %s failed to request IRQ\n, __func__);
+ goto err_sch_request_irq;
+ }
+
+ sch_gpio_irqs_init(sch, sch-chip.ngpio);
+ }
+
platform_set_drvdata(pdev, sch);
- return gpiochip_add(sch-chip);
+ return 0;
+
+err_sch_request_irq:
+ irq_free_descs(sch-irq_base, sch-chip.ngpio);
+
+err_sch_intr_chip:
+ if (gpiochip_remove(sch-chip))
gpiochip_remove() return 'void' [0].
+ dev_err(pdev-dev,
+ %s gpiochip_remove() failed\n, __func__);
+
+err_sch_gpio:
+ release_region(res-start, resource_size(res));
+
+ return err;
}
[0]:https://git.kernel.org/cgit/linux/kernel/git/linusw/linux-gpio.git/tree/drivers/gpio/gpiolib.c#n311
--
Regards,
Varka Bhadram.
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
