Re: [PATCH v4] gpio: Add support for Intel SoC PMIC (Crystal Cove)
On Tue, Jun 03, 2014 at 10:10:13AM +0200, Linus Walleij wrote: > On Fri, May 30, 2014 at 10:25 AM, Mika Westerberg > wrote: > > > I'm thinking that could we solve this so that we call > > acpi_gpiochip_request_interrupts() at the end of gpiochip_irqchip_add() > > and convert both pinctrl-baytrail and gpio-lynxpoint to use > > gpiochip_irqchip_add()? > > Yes that seems like a great way to solve it actually. > > Is someone able to do this refactoring? I have both Haswell and Baytrail hardware here so I can take a look if I have time. > I don't know if you have a case of an ACPI-based GPIO controller > that is *not* supplying interrupts? Because in that case this > would even be required for the thing to work, right? Both Haswell and Baytrail support interrupts but only the later provides ACPI events as far as I can tell. -- 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/
Re: [PATCH v4] gpio: Add support for Intel SoC PMIC (Crystal Cove)
On Fri, May 30, 2014 at 10:25 AM, Mika Westerberg wrote: > I'm thinking that could we solve this so that we call > acpi_gpiochip_request_interrupts() at the end of gpiochip_irqchip_add() > and convert both pinctrl-baytrail and gpio-lynxpoint to use > gpiochip_irqchip_add()? Yes that seems like a great way to solve it actually. Is someone able to do this refactoring? I don't know if you have a case of an ACPI-based GPIO controller that is *not* supplying interrupts? Because in that case this would even be required for the thing to work, right? Yours, Linus Walleij -- 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/
Re: [PATCH v4] gpio: Add support for Intel SoC PMIC (Crystal Cove)
On Fri, May 30, 2014 at 4:12 AM, Zhu, Lejun wrote:
> retval = gpiochip_add(>chip);
> if (retval) {
> dev_warn(>dev, "add gpio chip error: %d\n", retval);
> return ret;
> }
>
> gpiochip_irqchip_add(>chip, _irqchip, 0,
> handle_simple_irq, IRQ_TYPE_NONE);
>
> retval = request_threaded_irq(irq, NULL, crystalcove_gpio_irq_handler,
> IRQF_ONESHOT, KBUILD_MODNAME, cg);
You should request the interrupt before you add the irqchip
I think. But it shouldn't really matter, mainly to avoid tearing
down the irqchip if getting the irq should fail.
> But this code will trigger a crash in gpiolib-acpi. Currently at the end
> of gpiochip_add(), it calls:
>
> gpiochip_add() -> acpi_gpiochip_add() -> acpi_gpiochip_request_interrupts()
>
> acpi_gpiochip_request_interrupts() needs ->to_irq to work. Without having
> called gpiochip_irqchip_add() already, this will be NULL:
>
> if (!chip->to_irq)
> return;<-- It will return here.
>
> INIT_LIST_HEAD(_gpio->events);
>
> In the tear down path, acpi_gpiochip_free_interrupts() will find to_irq is
> no longer NULL, then it will walk an uninitialized list.
>
> So, should this be fixed in gpiolib-acpi?
Maybe, maybe in the drivers. I think Mika has a proposed solution...
Yours,
Linus Walleij
--
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/
Re: [PATCH v4] gpio: Add support for Intel SoC PMIC (Crystal Cove)
On Fri, May 30, 2014 at 4:12 AM, Zhu, Lejun lejun@linux.intel.com wrote:
retval = gpiochip_add(cg-chip);
if (retval) {
dev_warn(pdev-dev, add gpio chip error: %d\n, retval);
return ret;
}
gpiochip_irqchip_add(cg-chip, crystalcove_irqchip, 0,
handle_simple_irq, IRQ_TYPE_NONE);
retval = request_threaded_irq(irq, NULL, crystalcove_gpio_irq_handler,
IRQF_ONESHOT, KBUILD_MODNAME, cg);
You should request the interrupt before you add the irqchip
I think. But it shouldn't really matter, mainly to avoid tearing
down the irqchip if getting the irq should fail.
But this code will trigger a crash in gpiolib-acpi. Currently at the end
of gpiochip_add(), it calls:
gpiochip_add() - acpi_gpiochip_add() - acpi_gpiochip_request_interrupts()
acpi_gpiochip_request_interrupts() needs -to_irq to work. Without having
called gpiochip_irqchip_add() already, this will be NULL:
if (!chip-to_irq)
return;-- It will return here.
INIT_LIST_HEAD(acpi_gpio-events);
In the tear down path, acpi_gpiochip_free_interrupts() will find to_irq is
no longer NULL, then it will walk an uninitialized list.
So, should this be fixed in gpiolib-acpi?
Maybe, maybe in the drivers. I think Mika has a proposed solution...
Yours,
Linus Walleij
--
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/
Re: [PATCH v4] gpio: Add support for Intel SoC PMIC (Crystal Cove)
On Fri, May 30, 2014 at 10:25 AM, Mika Westerberg mika.westerb...@linux.intel.com wrote: I'm thinking that could we solve this so that we call acpi_gpiochip_request_interrupts() at the end of gpiochip_irqchip_add() and convert both pinctrl-baytrail and gpio-lynxpoint to use gpiochip_irqchip_add()? Yes that seems like a great way to solve it actually. Is someone able to do this refactoring? I don't know if you have a case of an ACPI-based GPIO controller that is *not* supplying interrupts? Because in that case this would even be required for the thing to work, right? Yours, Linus Walleij -- 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/
Re: [PATCH v4] gpio: Add support for Intel SoC PMIC (Crystal Cove)
On Tue, Jun 03, 2014 at 10:10:13AM +0200, Linus Walleij wrote: On Fri, May 30, 2014 at 10:25 AM, Mika Westerberg mika.westerb...@linux.intel.com wrote: I'm thinking that could we solve this so that we call acpi_gpiochip_request_interrupts() at the end of gpiochip_irqchip_add() and convert both pinctrl-baytrail and gpio-lynxpoint to use gpiochip_irqchip_add()? Yes that seems like a great way to solve it actually. Is someone able to do this refactoring? I have both Haswell and Baytrail hardware here so I can take a look if I have time. I don't know if you have a case of an ACPI-based GPIO controller that is *not* supplying interrupts? Because in that case this would even be required for the thing to work, right? Both Haswell and Baytrail support interrupts but only the later provides ACPI events as far as I can tell. -- 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/
Re: [PATCH v4] gpio: Add support for Intel SoC PMIC (Crystal Cove)
On Thu, May 29, 2014 at 05:22:05PM +0200, Linus Walleij wrote: > On Thu, May 29, 2014 at 6:03 PM, Grygorii Strashko > wrote: > > > Also, I'd like to note that GPIO IRQs can be accessible not only > > when GPIO chips is added, but also when IRQ domain is registered > > (at least it's valid for DT cases). In these cases gpiod_to_irq() > > might be not used at all. > > Yes. We concluded some time back that gpio_chip:s and > irq_chip:s are orthogonal abstractions: you should be able > to use one of them without paying any respect to the other. > > We only added the ability to flag GPIO lines as used for > IRQs so they would not be set to output by mistake... > (Straightening up the semantics.) > > The only real semantic dependence that really makes sense > is .to_irq() which leads to this semantic registration ordering. acpi_gpiochip_request_interrupts() depends on ->to_irq() to be set before acpi_gpiochip_add() is called. Since the ordering changes this won't work anymore. I'm thinking that could we solve this so that we call acpi_gpiochip_request_interrupts() at the end of gpiochip_irqchip_add() and convert both pinctrl-baytrail and gpio-lynxpoint to use gpiochip_irqchip_add()? -- 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/
Re: [PATCH v4] gpio: Add support for Intel SoC PMIC (Crystal Cove)
On Thu, May 29, 2014 at 05:22:05PM +0200, Linus Walleij wrote: On Thu, May 29, 2014 at 6:03 PM, Grygorii Strashko grygorii.stras...@ti.com wrote: Also, I'd like to note that GPIO IRQs can be accessible not only when GPIO chips is added, but also when IRQ domain is registered (at least it's valid for DT cases). In these cases gpiod_to_irq() might be not used at all. Yes. We concluded some time back that gpio_chip:s and irq_chip:s are orthogonal abstractions: you should be able to use one of them without paying any respect to the other. We only added the ability to flag GPIO lines as used for IRQs so they would not be set to output by mistake... (Straightening up the semantics.) The only real semantic dependence that really makes sense is .to_irq() which leads to this semantic registration ordering. acpi_gpiochip_request_interrupts() depends on -to_irq() to be set before acpi_gpiochip_add() is called. Since the ordering changes this won't work anymore. I'm thinking that could we solve this so that we call acpi_gpiochip_request_interrupts() at the end of gpiochip_irqchip_add() and convert both pinctrl-baytrail and gpio-lynxpoint to use gpiochip_irqchip_add()? -- 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/
Re: [PATCH v4] gpio: Add support for Intel SoC PMIC (Crystal Cove)
On 5/29/2014 9:37 PM, Linus Walleij wrote:
> On Tue, May 27, 2014 at 2:04 PM, Grygorii Strashko
> wrote:
>> On 05/27/2014 11:46 AM, Mika Westerberg wrote:
(...)
>
> My idea is that you should call gpiochip_add() *first* and then
> add the IRQs to the chip. In succession.
>
> Rationale: with dynamic GPIO numbers, gpio_to_irq()
> cannot reasonably be working before the gpiochip is added,
> so it should be added first, then the irqchip. Since irq_to_gpio()
> is *NOT* to be used (rather obliterated), this is the sequence
> we mandate.
>
> This is how the new irqchip helpers work by the way. As I
> introduce this to more and more drivers it will look more and
> more like this. And attack patches tagged RFT switching the
> semantics of drivers are appreciated.
>
> Yours,
> Linus Walleij
>
Thanks. I'll use this sequence during probe().
(...)
cg->regmap = pmic->regmap;
retval = gpiochip_add(>chip);
if (retval) {
dev_warn(>dev, "add gpio chip error: %d\n", retval);
return ret;
}
gpiochip_irqchip_add(>chip, _irqchip, 0,
handle_simple_irq, IRQ_TYPE_NONE);
retval = request_threaded_irq(irq, NULL, crystalcove_gpio_irq_handler,
IRQF_ONESHOT, KBUILD_MODNAME, cg);
if (retval) {
dev_warn(>dev, "request irq failed: %d\n", retval);
WARN_ON(gpiochip_remove(>chip));
return retval;
}
return 0;
}
Is the code above OK?
But this code will trigger a crash in gpiolib-acpi. Currently at the end
of gpiochip_add(), it calls:
gpiochip_add() -> acpi_gpiochip_add() -> acpi_gpiochip_request_interrupts()
acpi_gpiochip_request_interrupts() needs ->to_irq to work. Without having
called gpiochip_irqchip_add() already, this will be NULL:
if (!chip->to_irq)
return;<-- It will return here.
INIT_LIST_HEAD(_gpio->events);
In the tear down path, acpi_gpiochip_free_interrupts() will find to_irq is
no longer NULL, then it will walk an uninitialized list.
So, should this be fixed in gpiolib-acpi?
Best Regards
Lejun
--
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/
Re: [PATCH v4] gpio: Add support for Intel SoC PMIC (Crystal Cove)
On Thu, May 29, 2014 at 6:03 PM, Grygorii Strashko wrote: > Also, I'd like to note that GPIO IRQs can be accessible not only > when GPIO chips is added, but also when IRQ domain is registered > (at least it's valid for DT cases). In these cases gpiod_to_irq() > might be not used at all. Yes. We concluded some time back that gpio_chip:s and irq_chip:s are orthogonal abstractions: you should be able to use one of them without paying any respect to the other. We only added the ability to flag GPIO lines as used for IRQs so they would not be set to output by mistake... (Straightening up the semantics.) The only real semantic dependence that really makes sense is .to_irq() which leads to this semantic registration ordering. Yours, Linus Walleij -- 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/
Re: [PATCH v4] gpio: Add support for Intel SoC PMIC (Crystal Cove)
Hi All, On 05/29/2014 06:00 PM, Mika Westerberg wrote: > On Thu, May 29, 2014 at 03:37:37PM +0200, Linus Walleij wrote: >> My idea is that you should call gpiochip_add() *first* and then >> add the IRQs to the chip. In succession. >> >> Rationale: with dynamic GPIO numbers, gpio_to_irq() >> cannot reasonably be working before the gpiochip is added, >> so it should be added first, then the irqchip. Since irq_to_gpio() >> is *NOT* to be used (rather obliterated), this is the sequence >> we mandate. > > Thanks for the explanation. Makes sense. > Thanks a lot Linus for your comments here :) Also, I'd like to note that GPIO IRQs can be accessible not only when GPIO chips is added, but also when IRQ domain is registered (at least it's valid for DT cases). In these cases gpiod_to_irq() might be not used at all. Regards, -grygorii -- 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/
Re: [PATCH v4] gpio: Add support for Intel SoC PMIC (Crystal Cove)
On Thu, May 29, 2014 at 03:37:37PM +0200, Linus Walleij wrote: > My idea is that you should call gpiochip_add() *first* and then > add the IRQs to the chip. In succession. > > Rationale: with dynamic GPIO numbers, gpio_to_irq() > cannot reasonably be working before the gpiochip is added, > so it should be added first, then the irqchip. Since irq_to_gpio() > is *NOT* to be used (rather obliterated), this is the sequence > we mandate. Thanks for the explanation. Makes sense. I was wrong again, oh well it happens ;-) -- 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/
Re: [PATCH v4] gpio: Add support for Intel SoC PMIC (Crystal Cove)
On Tue, May 27, 2014 at 2:04 PM, Grygorii Strashko wrote: > On 05/27/2014 11:46 AM, Mika Westerberg wrote: > Regarding remove()/suspend() routines, It's like an axiom for me: > - always disable irq > - always stop all works/threads created by driver > - do everything else > (It's proved by dozens hours of debugging). > > Anyway, above is just my opinion :) We cannot really let such things be up to someone's opinion... we need to figure out what is the right way to do it and do it that way, else we have ambigous semantics in gpiolib and/or irqchip, which i *hate*. > So, It's up to you, because it's your code :) No, let's figure out what is right... And, FWIW your sequence looks perfectly reasonable. > Also FYI, I did fast analysis of GPIO drivers - funny statistic below: > - 16 drivers setup IRQs BEFORE calling gpiochip_add(); > - 22 drivers setup IRQs AFTER calling gpiochip_add(); My idea is that you should call gpiochip_add() *first* and then add the IRQs to the chip. In succession. Rationale: with dynamic GPIO numbers, gpio_to_irq() cannot reasonably be working before the gpiochip is added, so it should be added first, then the irqchip. Since irq_to_gpio() is *NOT* to be used (rather obliterated), this is the sequence we mandate. This is how the new irqchip helpers work by the way. As I introduce this to more and more drivers it will look more and more like this. And attack patches tagged RFT switching the semantics of drivers are appreciated. Yours, Linus Walleij -- 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/
Re: [PATCH v4] gpio: Add support for Intel SoC PMIC (Crystal Cove)
On Tue, May 27, 2014 at 2:04 PM, Grygorii Strashko grygorii.stras...@ti.com wrote: On 05/27/2014 11:46 AM, Mika Westerberg wrote: Regarding remove()/suspend() routines, It's like an axiom for me: - always disable irq - always stop all works/threads created by driver - do everything else (It's proved by dozens hours of debugging). Anyway, above is just my opinion :) We cannot really let such things be up to someone's opinion... we need to figure out what is the right way to do it and do it that way, else we have ambigous semantics in gpiolib and/or irqchip, which i *hate*. So, It's up to you, because it's your code :) No, let's figure out what is right... And, FWIW your sequence looks perfectly reasonable. Also FYI, I did fast analysis of GPIO drivers - funny statistic below: - 16 drivers setup IRQs BEFORE calling gpiochip_add(); - 22 drivers setup IRQs AFTER calling gpiochip_add(); My idea is that you should call gpiochip_add() *first* and then add the IRQs to the chip. In succession. Rationale: with dynamic GPIO numbers, gpio_to_irq() cannot reasonably be working before the gpiochip is added, so it should be added first, then the irqchip. Since irq_to_gpio() is *NOT* to be used (rather obliterated), this is the sequence we mandate. This is how the new irqchip helpers work by the way. As I introduce this to more and more drivers it will look more and more like this. And attack patches tagged RFT switching the semantics of drivers are appreciated. Yours, Linus Walleij -- 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/
Re: [PATCH v4] gpio: Add support for Intel SoC PMIC (Crystal Cove)
On Thu, May 29, 2014 at 03:37:37PM +0200, Linus Walleij wrote: My idea is that you should call gpiochip_add() *first* and then add the IRQs to the chip. In succession. Rationale: with dynamic GPIO numbers, gpio_to_irq() cannot reasonably be working before the gpiochip is added, so it should be added first, then the irqchip. Since irq_to_gpio() is *NOT* to be used (rather obliterated), this is the sequence we mandate. Thanks for the explanation. Makes sense. I was wrong again, oh well it happens ;-) -- 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/
Re: [PATCH v4] gpio: Add support for Intel SoC PMIC (Crystal Cove)
Hi All, On 05/29/2014 06:00 PM, Mika Westerberg wrote: On Thu, May 29, 2014 at 03:37:37PM +0200, Linus Walleij wrote: My idea is that you should call gpiochip_add() *first* and then add the IRQs to the chip. In succession. Rationale: with dynamic GPIO numbers, gpio_to_irq() cannot reasonably be working before the gpiochip is added, so it should be added first, then the irqchip. Since irq_to_gpio() is *NOT* to be used (rather obliterated), this is the sequence we mandate. Thanks for the explanation. Makes sense. Thanks a lot Linus for your comments here :) Also, I'd like to note that GPIO IRQs can be accessible not only when GPIO chips is added, but also when IRQ domain is registered (at least it's valid for DT cases). In these cases gpiod_to_irq() might be not used at all. Regards, -grygorii -- 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/
Re: [PATCH v4] gpio: Add support for Intel SoC PMIC (Crystal Cove)
On Thu, May 29, 2014 at 6:03 PM, Grygorii Strashko grygorii.stras...@ti.com wrote: Also, I'd like to note that GPIO IRQs can be accessible not only when GPIO chips is added, but also when IRQ domain is registered (at least it's valid for DT cases). In these cases gpiod_to_irq() might be not used at all. Yes. We concluded some time back that gpio_chip:s and irq_chip:s are orthogonal abstractions: you should be able to use one of them without paying any respect to the other. We only added the ability to flag GPIO lines as used for IRQs so they would not be set to output by mistake... (Straightening up the semantics.) The only real semantic dependence that really makes sense is .to_irq() which leads to this semantic registration ordering. Yours, Linus Walleij -- 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/
Re: [PATCH v4] gpio: Add support for Intel SoC PMIC (Crystal Cove)
On 5/29/2014 9:37 PM, Linus Walleij wrote:
On Tue, May 27, 2014 at 2:04 PM, Grygorii Strashko
grygorii.stras...@ti.com wrote:
On 05/27/2014 11:46 AM, Mika Westerberg wrote:
(...)
My idea is that you should call gpiochip_add() *first* and then
add the IRQs to the chip. In succession.
Rationale: with dynamic GPIO numbers, gpio_to_irq()
cannot reasonably be working before the gpiochip is added,
so it should be added first, then the irqchip. Since irq_to_gpio()
is *NOT* to be used (rather obliterated), this is the sequence
we mandate.
This is how the new irqchip helpers work by the way. As I
introduce this to more and more drivers it will look more and
more like this. And attack patches tagged RFT switching the
semantics of drivers are appreciated.
Yours,
Linus Walleij
Thanks. I'll use this sequence during probe().
(...)
cg-regmap = pmic-regmap;
retval = gpiochip_add(cg-chip);
if (retval) {
dev_warn(pdev-dev, add gpio chip error: %d\n, retval);
return ret;
}
gpiochip_irqchip_add(cg-chip, crystalcove_irqchip, 0,
handle_simple_irq, IRQ_TYPE_NONE);
retval = request_threaded_irq(irq, NULL, crystalcove_gpio_irq_handler,
IRQF_ONESHOT, KBUILD_MODNAME, cg);
if (retval) {
dev_warn(pdev-dev, request irq failed: %d\n, retval);
WARN_ON(gpiochip_remove(cg-chip));
return retval;
}
return 0;
}
Is the code above OK?
But this code will trigger a crash in gpiolib-acpi. Currently at the end
of gpiochip_add(), it calls:
gpiochip_add() - acpi_gpiochip_add() - acpi_gpiochip_request_interrupts()
acpi_gpiochip_request_interrupts() needs -to_irq to work. Without having
called gpiochip_irqchip_add() already, this will be NULL:
if (!chip-to_irq)
return;-- It will return here.
INIT_LIST_HEAD(acpi_gpio-events);
In the tear down path, acpi_gpiochip_free_interrupts() will find to_irq is
no longer NULL, then it will walk an uninitialized list.
So, should this be fixed in gpiolib-acpi?
Best Regards
Lejun
--
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/
Re: [PATCH v4] gpio: Add support for Intel SoC PMIC (Crystal Cove)
On Tue, May 27, 2014 at 03:04:09PM +0300, Grygorii Strashko wrote:
> Hi Mika,
>
> On 05/27/2014 11:46 AM, Mika Westerberg wrote:
> > On Tue, May 27, 2014 at 12:24:56PM +0300, Grygorii Strashko wrote:
> +
> + if (retval) {
> + dev_warn(>dev, "request irq failed: %d\n", retval);
> + goto out;
> + }
> +
> + retval = gpiochip_add(>chip);
> + if (retval) {
> + dev_warn(>dev, "add gpio chip error: %d\n",
> retval);
> + goto out_free_irq;
> + }
> >>
> >> As to my mind, It'll be better to setup IRQ as last probing step and
> >> free it as the first step of driver removing.
> >
> > When gpiochip_add() is called the chip is exported to outside world. At
> > that point anyone can start requesting GPIOs and setup GPIO based
> > interrupts. How does that work if you setup the IRQ after you call
> > gpiochip_add()?
> >
>
> It's difficult for me to imagine case when GPIO will be accessed
> until GPIO driver's probe is finished.
Once you call gpiochip_add() your driver gets registered to the GPIO
subsystem and advertised outside. It doesn't matter whether your probe
function is finished or not.
> Regarding remove()/suspend() routines, It's like an axiom for me:
> - always disable irq
> - always stop all works/threads created by driver
> - do everything else
> (It's proved by dozens hours of debugging).
That's true for remove and suspend, yes but I'm not talking about them.
> Anyway, above is just my opinion :)
> So, It's up to you, because it's your code :)
No it's not, it's Lejun's driver :)
--
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/
Re: [PATCH v4] gpio: Add support for Intel SoC PMIC (Crystal Cove)
Hi Mika,
On 05/27/2014 11:46 AM, Mika Westerberg wrote:
> On Tue, May 27, 2014 at 12:24:56PM +0300, Grygorii Strashko wrote:
+
+ if (retval) {
+ dev_warn(>dev, "request irq failed: %d\n", retval);
+ goto out;
+ }
+
+ retval = gpiochip_add(>chip);
+ if (retval) {
+ dev_warn(>dev, "add gpio chip error: %d\n", retval);
+ goto out_free_irq;
+ }
>>
>> As to my mind, It'll be better to setup IRQ as last probing step and
>> free it as the first step of driver removing.
>
> When gpiochip_add() is called the chip is exported to outside world. At
> that point anyone can start requesting GPIOs and setup GPIO based
> interrupts. How does that work if you setup the IRQ after you call
> gpiochip_add()?
>
It's difficult for me to imagine case when GPIO will be accessed
until GPIO driver's probe is finished.
Regarding remove()/suspend() routines, It's like an axiom for me:
- always disable irq
- always stop all works/threads created by driver
- do everything else
(It's proved by dozens hours of debugging).
Anyway, above is just my opinion :)
So, It's up to you, because it's your code :)
Also FYI, I did fast analysis of GPIO drivers - funny statistic below:
- 16 drivers setup IRQs BEFORE calling gpiochip_add();
- 22 drivers setup IRQs AFTER calling gpiochip_add();
Best regards,
-grygorii
--
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/
Re: [PATCH v4] gpio: Add support for Intel SoC PMIC (Crystal Cove)
On 5/27/2014 5:24 PM, Grygorii Strashko wrote:
> Hi Lejun,
>
> On 05/27/2014 08:38 AM, Alexandre Courbot wrote:
>> On Fri, May 23, 2014 at 11:00 AM, Zhu, Lejun
>> wrote:
>>> +static int crystalcove_gpio_probe(struct platform_device *pdev)
>>> +{
>>> + int irq = platform_get_irq(pdev, 0);
>
> Pls note, that platform_get_irq() may return error code.
Thank you. I'll fix it.
>
> devm_gpiochip_add? ;)
>
Huh? Can't find the API...
>>> +
>>> + if (retval) {
>>> + dev_warn(>dev, "request irq failed: %d\n", retval);
>>> + goto out;
>>> + }
>>> +
>>> + retval = gpiochip_add(>chip);
>>> + if (retval) {
>>> + dev_warn(>dev, "add gpio chip error: %d\n", retval);
>>> + goto out_free_irq;
>>> + }
>
> As to my mind, It'll be better to setup IRQ as last probing step and
> free it as the first step of driver removing.
Mika suggested this order. Please see his mail for the reason. Is there
anything bad might happen if I setup IRQ first then do gpiochip_add?
Best Regards
Lejun
--
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/
Re: [PATCH v4] gpio: Add support for Intel SoC PMIC (Crystal Cove)
On Tue, May 27, 2014 at 12:24:56PM +0300, Grygorii Strashko wrote:
> >> +
> >> + if (retval) {
> >> + dev_warn(>dev, "request irq failed: %d\n", retval);
> >> + goto out;
> >> + }
> >> +
> >> + retval = gpiochip_add(>chip);
> >> + if (retval) {
> >> + dev_warn(>dev, "add gpio chip error: %d\n", retval);
> >> + goto out_free_irq;
> >> + }
>
> As to my mind, It'll be better to setup IRQ as last probing step and
> free it as the first step of driver removing.
When gpiochip_add() is called the chip is exported to outside world. At
that point anyone can start requesting GPIOs and setup GPIO based
interrupts. How does that work if you setup the IRQ after you call
gpiochip_add()?
--
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/
Re: [PATCH v4] gpio: Add support for Intel SoC PMIC (Crystal Cove)
Hi Lejun,
On 05/27/2014 08:38 AM, Alexandre Courbot wrote:
> On Fri, May 23, 2014 at 11:00 AM, Zhu, Lejun
> wrote:
>> Devices based on Intel SoC products such as Baytrail have a Power
>> Management IC. In the PMIC there are subsystems for voltage regulation,
>> A/D conversion, GPIO and PWMs. The PMIC in Baytrail-T platform is called
>> Crystal Cove.
>>
>> This patch adds support for the GPIO function in Crystal Cove.
>
> A few minor comments below in case you make another version, but
> overall looks pretty good to me.
>
> Reviewed-by: Alexandre Courbot
>
>>
>> v2:
>> - Use IRQ chip helper to provide irqdomain.
>> - Implement .remove and can now build as a module.
>> - Various fix for unreadable or ugly code pieces.
>> v3:
>> - More fix in irq_handler and probe.
>> v4:
>> - Minor fix of one return statement.
>>
>> Signed-off-by: Yang, Bin
>> Signed-off-by: Zhu, Lejun
>> Reviewed-by: Mika Westerberg
>> ---
[...]
>> +}
>> +
>> +static int crystalcove_gpio_probe(struct platform_device *pdev)
>> +{
>> + int irq = platform_get_irq(pdev, 0);
Pls note, that platform_get_irq() may return error code.
>> + struct crystalcove_gpio *cg;
>> + int retval;
>> + struct device *dev = pdev->dev.parent;
>> +
>> + cg = devm_kzalloc(>dev, sizeof(*cg), GFP_KERNEL);
>> + if (!cg)
>> + return -ENOMEM;
>> +
>> + mutex_init(>buslock);
>> + cg->chip.label = KBUILD_MODNAME;
>> + cg->chip.direction_input = crystalcove_gpio_direction_input;
>> + cg->chip.direction_output = crystalcove_gpio_direction_output;
>> + cg->chip.get = crystalcove_gpio_get;
>> + cg->chip.set = crystalcove_gpio_set;
>> + cg->chip.base = -1;
>> + cg->chip.ngpio = NUM_GPIO;
>> + cg->chip.can_sleep = true;
>> + cg->chip.dev = dev;
>> + cg->chip.dbg_show = crystalcove_gpio_dbg_show;
>> +
>> + gpiochip_irqchip_add(>chip, _irqchip, 0,
>> +handle_simple_irq, IRQ_TYPE_NONE);
>> +
>> + retval = request_threaded_irq(irq, NULL,
>> crystalcove_gpio_irq_handler,
>> + IRQF_ONESHOT, KBUILD_MODNAME, cg);
>
> Can't you use devm_request_threaded_irq() here?
devm_gpiochip_add? ;)
>
>> +
>> + if (retval) {
>> + dev_warn(>dev, "request irq failed: %d\n", retval);
>> + goto out;
>> + }
>> +
>> + retval = gpiochip_add(>chip);
>> + if (retval) {
>> + dev_warn(>dev, "add gpio chip error: %d\n", retval);
>> + goto out_free_irq;
>> + }
As to my mind, It'll be better to setup IRQ as last probing step and
free it as the first step of driver removing.
>> +
>> + platform_set_drvdata(pdev, cg);
>> +
>> + return 0;
>> +
>> +out_free_irq:
>> + free_irq(irq, cg);
>> +out:
>> + return retval;
>> +}
Best regards,
-grygorii
--
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/
Re: [PATCH v4] gpio: Add support for Intel SoC PMIC (Crystal Cove)
On 5/27/2014 1:38 PM, Alexandre Courbot wrote:
> On Fri, May 23, 2014 at 11:00 AM, Zhu, Lejun
> wrote:
>> +static void crystalcove_update_irq_type(int gpio, int type)
>> +{
>> + u8 ctli = GPIO_TO_CTL(gpio, I);
>> +
>> + type &= IRQ_TYPE_EDGE_BOTH;
>> + intel_soc_pmic_clearb(ctli, CTLI_INTCNT_BE);
>> +
>> + if (type == IRQ_TYPE_EDGE_BOTH)
>> + intel_soc_pmic_setb(ctli, CTLI_INTCNT_BE);
>> + else if (type == IRQ_TYPE_EDGE_RISING)
>> + intel_soc_pmic_setb(ctli, CTLI_INTCNT_PE);
>> + else if (type & IRQ_TYPE_EDGE_FALLING)
>> + intel_soc_pmic_setb(ctli, CTLI_INTCNT_NE);
>
> Maybe a switch would be nicer here to choose the right value? And a
> single call to intel_soc_pmic_setb() after the value is picked.
Thank you. I will fix this in the next version.
Best Regards
Lejun
--
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/
Re: [PATCH v4] gpio: Add support for Intel SoC PMIC (Crystal Cove)
On Tue, May 27, 2014 at 2:38 PM, Alexandre Courbot wrote: >> + gpiochip_irqchip_add(>chip, _irqchip, 0, >> +handle_simple_irq, IRQ_TYPE_NONE); >> + >> + retval = request_threaded_irq(irq, NULL, >> crystalcove_gpio_irq_handler, >> + IRQF_ONESHOT, KBUILD_MODNAME, cg); > > Can't you use devm_request_threaded_irq() here? Ah, I suppose doing so would keep the IRQ handler active longer than we want - please ignore this comment. -- 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/
Re: [PATCH v4] gpio: Add support for Intel SoC PMIC (Crystal Cove)
On Tue, May 27, 2014 at 2:38 PM, Alexandre Courbot gnu...@gmail.com wrote: + gpiochip_irqchip_add(cg-chip, crystalcove_irqchip, 0, +handle_simple_irq, IRQ_TYPE_NONE); + + retval = request_threaded_irq(irq, NULL, crystalcove_gpio_irq_handler, + IRQF_ONESHOT, KBUILD_MODNAME, cg); Can't you use devm_request_threaded_irq() here? Ah, I suppose doing so would keep the IRQ handler active longer than we want - please ignore this comment. -- 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/
Re: [PATCH v4] gpio: Add support for Intel SoC PMIC (Crystal Cove)
On 5/27/2014 1:38 PM, Alexandre Courbot wrote:
On Fri, May 23, 2014 at 11:00 AM, Zhu, Lejun lejun@linux.intel.com
wrote:
+static void crystalcove_update_irq_type(int gpio, int type)
+{
+ u8 ctli = GPIO_TO_CTL(gpio, I);
+
+ type = IRQ_TYPE_EDGE_BOTH;
+ intel_soc_pmic_clearb(ctli, CTLI_INTCNT_BE);
+
+ if (type == IRQ_TYPE_EDGE_BOTH)
+ intel_soc_pmic_setb(ctli, CTLI_INTCNT_BE);
+ else if (type == IRQ_TYPE_EDGE_RISING)
+ intel_soc_pmic_setb(ctli, CTLI_INTCNT_PE);
+ else if (type IRQ_TYPE_EDGE_FALLING)
+ intel_soc_pmic_setb(ctli, CTLI_INTCNT_NE);
Maybe a switch would be nicer here to choose the right value? And a
single call to intel_soc_pmic_setb() after the value is picked.
Thank you. I will fix this in the next version.
Best Regards
Lejun
--
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/
Re: [PATCH v4] gpio: Add support for Intel SoC PMIC (Crystal Cove)
Hi Lejun,
On 05/27/2014 08:38 AM, Alexandre Courbot wrote:
On Fri, May 23, 2014 at 11:00 AM, Zhu, Lejun lejun@linux.intel.com
wrote:
Devices based on Intel SoC products such as Baytrail have a Power
Management IC. In the PMIC there are subsystems for voltage regulation,
A/D conversion, GPIO and PWMs. The PMIC in Baytrail-T platform is called
Crystal Cove.
This patch adds support for the GPIO function in Crystal Cove.
A few minor comments below in case you make another version, but
overall looks pretty good to me.
Reviewed-by: Alexandre Courbot acour...@nvidia.com
v2:
- Use IRQ chip helper to provide irqdomain.
- Implement .remove and can now build as a module.
- Various fix for unreadable or ugly code pieces.
v3:
- More fix in irq_handler and probe.
v4:
- Minor fix of one return statement.
Signed-off-by: Yang, Bin bin.y...@intel.com
Signed-off-by: Zhu, Lejun lejun@linux.intel.com
Reviewed-by: Mika Westerberg mika.westerb...@linux.intel.com
---
[...]
+}
+
+static int crystalcove_gpio_probe(struct platform_device *pdev)
+{
+ int irq = platform_get_irq(pdev, 0);
Pls note, that platform_get_irq() may return error code.
+ struct crystalcove_gpio *cg;
+ int retval;
+ struct device *dev = pdev-dev.parent;
+
+ cg = devm_kzalloc(pdev-dev, sizeof(*cg), GFP_KERNEL);
+ if (!cg)
+ return -ENOMEM;
+
+ mutex_init(cg-buslock);
+ cg-chip.label = KBUILD_MODNAME;
+ cg-chip.direction_input = crystalcove_gpio_direction_input;
+ cg-chip.direction_output = crystalcove_gpio_direction_output;
+ cg-chip.get = crystalcove_gpio_get;
+ cg-chip.set = crystalcove_gpio_set;
+ cg-chip.base = -1;
+ cg-chip.ngpio = NUM_GPIO;
+ cg-chip.can_sleep = true;
+ cg-chip.dev = dev;
+ cg-chip.dbg_show = crystalcove_gpio_dbg_show;
+
+ gpiochip_irqchip_add(cg-chip, crystalcove_irqchip, 0,
+handle_simple_irq, IRQ_TYPE_NONE);
+
+ retval = request_threaded_irq(irq, NULL,
crystalcove_gpio_irq_handler,
+ IRQF_ONESHOT, KBUILD_MODNAME, cg);
Can't you use devm_request_threaded_irq() here?
devm_gpiochip_add? ;)
+
+ if (retval) {
+ dev_warn(pdev-dev, request irq failed: %d\n, retval);
+ goto out;
+ }
+
+ retval = gpiochip_add(cg-chip);
+ if (retval) {
+ dev_warn(pdev-dev, add gpio chip error: %d\n, retval);
+ goto out_free_irq;
+ }
As to my mind, It'll be better to setup IRQ as last probing step and
free it as the first step of driver removing.
+
+ platform_set_drvdata(pdev, cg);
+
+ return 0;
+
+out_free_irq:
+ free_irq(irq, cg);
+out:
+ return retval;
+}
Best regards,
-grygorii
--
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/
Re: [PATCH v4] gpio: Add support for Intel SoC PMIC (Crystal Cove)
On Tue, May 27, 2014 at 12:24:56PM +0300, Grygorii Strashko wrote:
+
+ if (retval) {
+ dev_warn(pdev-dev, request irq failed: %d\n, retval);
+ goto out;
+ }
+
+ retval = gpiochip_add(cg-chip);
+ if (retval) {
+ dev_warn(pdev-dev, add gpio chip error: %d\n, retval);
+ goto out_free_irq;
+ }
As to my mind, It'll be better to setup IRQ as last probing step and
free it as the first step of driver removing.
When gpiochip_add() is called the chip is exported to outside world. At
that point anyone can start requesting GPIOs and setup GPIO based
interrupts. How does that work if you setup the IRQ after you call
gpiochip_add()?
--
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/
Re: [PATCH v4] gpio: Add support for Intel SoC PMIC (Crystal Cove)
On 5/27/2014 5:24 PM, Grygorii Strashko wrote:
Hi Lejun,
On 05/27/2014 08:38 AM, Alexandre Courbot wrote:
On Fri, May 23, 2014 at 11:00 AM, Zhu, Lejun lejun@linux.intel.com
wrote:
+static int crystalcove_gpio_probe(struct platform_device *pdev)
+{
+ int irq = platform_get_irq(pdev, 0);
Pls note, that platform_get_irq() may return error code.
Thank you. I'll fix it.
devm_gpiochip_add? ;)
Huh? Can't find the API...
+
+ if (retval) {
+ dev_warn(pdev-dev, request irq failed: %d\n, retval);
+ goto out;
+ }
+
+ retval = gpiochip_add(cg-chip);
+ if (retval) {
+ dev_warn(pdev-dev, add gpio chip error: %d\n, retval);
+ goto out_free_irq;
+ }
As to my mind, It'll be better to setup IRQ as last probing step and
free it as the first step of driver removing.
Mika suggested this order. Please see his mail for the reason. Is there
anything bad might happen if I setup IRQ first then do gpiochip_add?
Best Regards
Lejun
--
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/
Re: [PATCH v4] gpio: Add support for Intel SoC PMIC (Crystal Cove)
Hi Mika,
On 05/27/2014 11:46 AM, Mika Westerberg wrote:
On Tue, May 27, 2014 at 12:24:56PM +0300, Grygorii Strashko wrote:
+
+ if (retval) {
+ dev_warn(pdev-dev, request irq failed: %d\n, retval);
+ goto out;
+ }
+
+ retval = gpiochip_add(cg-chip);
+ if (retval) {
+ dev_warn(pdev-dev, add gpio chip error: %d\n, retval);
+ goto out_free_irq;
+ }
As to my mind, It'll be better to setup IRQ as last probing step and
free it as the first step of driver removing.
When gpiochip_add() is called the chip is exported to outside world. At
that point anyone can start requesting GPIOs and setup GPIO based
interrupts. How does that work if you setup the IRQ after you call
gpiochip_add()?
It's difficult for me to imagine case when GPIO will be accessed
until GPIO driver's probe is finished.
Regarding remove()/suspend() routines, It's like an axiom for me:
- always disable irq
- always stop all works/threads created by driver
- do everything else
(It's proved by dozens hours of debugging).
Anyway, above is just my opinion :)
So, It's up to you, because it's your code :)
Also FYI, I did fast analysis of GPIO drivers - funny statistic below:
- 16 drivers setup IRQs BEFORE calling gpiochip_add();
- 22 drivers setup IRQs AFTER calling gpiochip_add();
Best regards,
-grygorii
--
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/
Re: [PATCH v4] gpio: Add support for Intel SoC PMIC (Crystal Cove)
On Tue, May 27, 2014 at 03:04:09PM +0300, Grygorii Strashko wrote:
Hi Mika,
On 05/27/2014 11:46 AM, Mika Westerberg wrote:
On Tue, May 27, 2014 at 12:24:56PM +0300, Grygorii Strashko wrote:
+
+ if (retval) {
+ dev_warn(pdev-dev, request irq failed: %d\n, retval);
+ goto out;
+ }
+
+ retval = gpiochip_add(cg-chip);
+ if (retval) {
+ dev_warn(pdev-dev, add gpio chip error: %d\n,
retval);
+ goto out_free_irq;
+ }
As to my mind, It'll be better to setup IRQ as last probing step and
free it as the first step of driver removing.
When gpiochip_add() is called the chip is exported to outside world. At
that point anyone can start requesting GPIOs and setup GPIO based
interrupts. How does that work if you setup the IRQ after you call
gpiochip_add()?
It's difficult for me to imagine case when GPIO will be accessed
until GPIO driver's probe is finished.
Once you call gpiochip_add() your driver gets registered to the GPIO
subsystem and advertised outside. It doesn't matter whether your probe
function is finished or not.
Regarding remove()/suspend() routines, It's like an axiom for me:
- always disable irq
- always stop all works/threads created by driver
- do everything else
(It's proved by dozens hours of debugging).
That's true for remove and suspend, yes but I'm not talking about them.
Anyway, above is just my opinion :)
So, It's up to you, because it's your code :)
No it's not, it's Lejun's driver :)
--
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/
Re: [PATCH v4] gpio: Add support for Intel SoC PMIC (Crystal Cove)
On Fri, May 23, 2014 at 11:00 AM, Zhu, Lejun wrote: > Devices based on Intel SoC products such as Baytrail have a Power > Management IC. In the PMIC there are subsystems for voltage regulation, > A/D conversion, GPIO and PWMs. The PMIC in Baytrail-T platform is called > Crystal Cove. > > This patch adds support for the GPIO function in Crystal Cove. A few minor comments below in case you make another version, but overall looks pretty good to me. Reviewed-by: Alexandre Courbot > > v2: > - Use IRQ chip helper to provide irqdomain. > - Implement .remove and can now build as a module. > - Various fix for unreadable or ugly code pieces. > v3: > - More fix in irq_handler and probe. > v4: > - Minor fix of one return statement. > > Signed-off-by: Yang, Bin > Signed-off-by: Zhu, Lejun > Reviewed-by: Mika Westerberg > --- > drivers/gpio/Kconfig| 13 ++ > drivers/gpio/Makefile | 1 + > drivers/gpio/gpio-crystalcove.c | 345 > > 3 files changed, 359 insertions(+) > create mode 100644 drivers/gpio/gpio-crystalcove.c > > diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig > index a86c49a..fed08d9d 100644 > --- a/drivers/gpio/Kconfig > +++ b/drivers/gpio/Kconfig > @@ -440,6 +440,19 @@ config GPIO_ARIZONA > help > Support for GPIOs on Wolfson Arizona class devices. > > +config GPIO_CRYSTAL_COVE > + tristate "GPIO support for Crystal Cove PMIC" > + depends on INTEL_SOC_PMIC > + select GPIOLIB_IRQCHIP > + help > + Support for GPIO pins on Crystal Cove PMIC. > + > + Say Yes if you have a Intel SoC based tablet with Crystal Cove PMIC > + inside. > + > + This driver can also be built as a module. If so, the module will be > + called gpio-crystalcove. > + > config GPIO_LP3943 > tristate "TI/National Semiconductor LP3943 GPIO expander" > depends on MFD_LP3943 > diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile > index 6309aff..e6cd935 100644 > --- a/drivers/gpio/Makefile > +++ b/drivers/gpio/Makefile > @@ -20,6 +20,7 @@ obj-$(CONFIG_GPIO_BCM_KONA) += gpio-bcm-kona.o > obj-$(CONFIG_GPIO_BT8XX) += gpio-bt8xx.o > obj-$(CONFIG_GPIO_CLPS711X)+= gpio-clps711x.o > obj-$(CONFIG_GPIO_CS5535) += gpio-cs5535.o > +obj-$(CONFIG_GPIO_CRYSTAL_COVE)+= gpio-crystalcove.o > obj-$(CONFIG_GPIO_DA9052) += gpio-da9052.o > obj-$(CONFIG_GPIO_DA9055) += gpio-da9055.o > obj-$(CONFIG_GPIO_DAVINCI) += gpio-davinci.o > diff --git a/drivers/gpio/gpio-crystalcove.c b/drivers/gpio/gpio-crystalcove.c > new file mode 100644 > index 000..76b6d57 > --- /dev/null > +++ b/drivers/gpio/gpio-crystalcove.c > @@ -0,0 +1,345 @@ > +/* > + * gpio-crystalcove.c - Intel Crystal Cove GPIO Driver > + * > + * Copyright (C) 2012, 2014 Intel Corporation. All rights reserved. > + * > + * This program is free software; you can redistribute it and/or > + * modify it under the terms of the GNU General Public License version > + * 2 as published by the Free Software Foundation. > + * > + * This program is distributed in the hope that it will be useful, > + * but WITHOUT ANY WARRANTY; without even the implied warranty of > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > + * GNU General Public License for more details. > + * > + * Author: Yang, Bin > + */ > + > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > + > +#define NUM_GPIO 16 > + > +#define UPDATE_TYPEBIT(0) > +#define UPDATE_MASKBIT(1) > + > +#define GPIO0IRQ 0x0b > +#define GPIO1IRQ 0x0c > +#define MGPIO0IRQS00x19 > +#define MGPIO1IRQS00x1a > +#define MGPIO0IRQSX0x1b > +#define MGPIO1IRQSX0x1c > +#define GPIO0P0CTLO0x2b > +#define GPIO0P0CTLI0x33 > +#define GPIO1P0CTLO0x3b > +#define GPIO1P0CTLI0x43 > + > +#define CTLI_INTCNT_NE (1 << 1) > +#define CTLI_INTCNT_PE (2 << 1) > +#define CTLI_INTCNT_BE (3 << 1) > + > +#define CTLO_DIR_OUT (1 << 5) > + > +#define CTLO_DRV_CMOS (0 << 4) > +#define CTLO_DRV_OD(1 << 4) > + > +#define CTLO_DRV_REN (1 << 3) > + > +#define CTLO_RVAL_2KDW (0) > +#define CTLO_RVAL_2KUP (1 << 1) > +#define CTLO_RVAL_50KDW(2 << 1) > +#define CTLO_RVAL_50KUP(3 << 1) > + > +#define CTLO_INPUT_DEF (CTLO_DRV_CMOS | CTLO_DRV_REN | CTLO_RVAL_2KUP) > +#define CTLO_OUTPUT_DEF(CTLO_DIR_OUT | CTLO_INPUT_DEF) > + > +#define GPIO_TO_CTL(gpio, dir) \ > + ((gpio < 8 ? GPIO0P0CTL ## dir : GPIO1P0CTL ## dir) + (gpio % 8)) > + > +/** > + * struct crystalcove_gpio - Crystal Cove GPIO controller > + * @buslock: for bus lock/sync and unlock. > + * @chip: the
Re: [PATCH v4] gpio: Add support for Intel SoC PMIC (Crystal Cove)
On Fri, May 23, 2014 at 11:00 AM, Zhu, Lejun lejun@linux.intel.com wrote: Devices based on Intel SoC products such as Baytrail have a Power Management IC. In the PMIC there are subsystems for voltage regulation, A/D conversion, GPIO and PWMs. The PMIC in Baytrail-T platform is called Crystal Cove. This patch adds support for the GPIO function in Crystal Cove. A few minor comments below in case you make another version, but overall looks pretty good to me. Reviewed-by: Alexandre Courbot acour...@nvidia.com v2: - Use IRQ chip helper to provide irqdomain. - Implement .remove and can now build as a module. - Various fix for unreadable or ugly code pieces. v3: - More fix in irq_handler and probe. v4: - Minor fix of one return statement. Signed-off-by: Yang, Bin bin.y...@intel.com Signed-off-by: Zhu, Lejun lejun@linux.intel.com Reviewed-by: Mika Westerberg mika.westerb...@linux.intel.com --- drivers/gpio/Kconfig| 13 ++ drivers/gpio/Makefile | 1 + drivers/gpio/gpio-crystalcove.c | 345 3 files changed, 359 insertions(+) create mode 100644 drivers/gpio/gpio-crystalcove.c diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig index a86c49a..fed08d9d 100644 --- a/drivers/gpio/Kconfig +++ b/drivers/gpio/Kconfig @@ -440,6 +440,19 @@ config GPIO_ARIZONA help Support for GPIOs on Wolfson Arizona class devices. +config GPIO_CRYSTAL_COVE + tristate GPIO support for Crystal Cove PMIC + depends on INTEL_SOC_PMIC + select GPIOLIB_IRQCHIP + help + Support for GPIO pins on Crystal Cove PMIC. + + Say Yes if you have a Intel SoC based tablet with Crystal Cove PMIC + inside. + + This driver can also be built as a module. If so, the module will be + called gpio-crystalcove. + config GPIO_LP3943 tristate TI/National Semiconductor LP3943 GPIO expander depends on MFD_LP3943 diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile index 6309aff..e6cd935 100644 --- a/drivers/gpio/Makefile +++ b/drivers/gpio/Makefile @@ -20,6 +20,7 @@ obj-$(CONFIG_GPIO_BCM_KONA) += gpio-bcm-kona.o obj-$(CONFIG_GPIO_BT8XX) += gpio-bt8xx.o obj-$(CONFIG_GPIO_CLPS711X)+= gpio-clps711x.o obj-$(CONFIG_GPIO_CS5535) += gpio-cs5535.o +obj-$(CONFIG_GPIO_CRYSTAL_COVE)+= gpio-crystalcove.o obj-$(CONFIG_GPIO_DA9052) += gpio-da9052.o obj-$(CONFIG_GPIO_DA9055) += gpio-da9055.o obj-$(CONFIG_GPIO_DAVINCI) += gpio-davinci.o diff --git a/drivers/gpio/gpio-crystalcove.c b/drivers/gpio/gpio-crystalcove.c new file mode 100644 index 000..76b6d57 --- /dev/null +++ b/drivers/gpio/gpio-crystalcove.c @@ -0,0 +1,345 @@ +/* + * gpio-crystalcove.c - Intel Crystal Cove GPIO Driver + * + * Copyright (C) 2012, 2014 Intel Corporation. All rights reserved. + * + * This program is free software; you can redistribute it and/or + * modify it under the terms of the GNU General Public License version + * 2 as published by the Free Software Foundation. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * Author: Yang, Bin bin.y...@intel.com + */ + +#include linux/module.h +#include linux/kernel.h +#include linux/slab.h +#include linux/io.h +#include linux/delay.h +#include linux/interrupt.h +#include linux/device.h +#include linux/platform_device.h +#include linux/seq_file.h +#include linux/sched.h +#include linux/mfd/intel_soc_pmic.h +#include linux/gpio.h +#include linux/bitops.h + +#define NUM_GPIO 16 + +#define UPDATE_TYPEBIT(0) +#define UPDATE_MASKBIT(1) + +#define GPIO0IRQ 0x0b +#define GPIO1IRQ 0x0c +#define MGPIO0IRQS00x19 +#define MGPIO1IRQS00x1a +#define MGPIO0IRQSX0x1b +#define MGPIO1IRQSX0x1c +#define GPIO0P0CTLO0x2b +#define GPIO0P0CTLI0x33 +#define GPIO1P0CTLO0x3b +#define GPIO1P0CTLI0x43 + +#define CTLI_INTCNT_NE (1 1) +#define CTLI_INTCNT_PE (2 1) +#define CTLI_INTCNT_BE (3 1) + +#define CTLO_DIR_OUT (1 5) + +#define CTLO_DRV_CMOS (0 4) +#define CTLO_DRV_OD(1 4) + +#define CTLO_DRV_REN (1 3) + +#define CTLO_RVAL_2KDW (0) +#define CTLO_RVAL_2KUP (1 1) +#define CTLO_RVAL_50KDW(2 1) +#define CTLO_RVAL_50KUP(3 1) + +#define CTLO_INPUT_DEF (CTLO_DRV_CMOS | CTLO_DRV_REN | CTLO_RVAL_2KUP) +#define CTLO_OUTPUT_DEF(CTLO_DIR_OUT | CTLO_INPUT_DEF) + +#define GPIO_TO_CTL(gpio, dir) \ + ((gpio 8 ? GPIO0P0CTL ## dir :
[PATCH v4] gpio: Add support for Intel SoC PMIC (Crystal Cove)
Devices based on Intel SoC products such as Baytrail have a Power
Management IC. In the PMIC there are subsystems for voltage regulation,
A/D conversion, GPIO and PWMs. The PMIC in Baytrail-T platform is called
Crystal Cove.
This patch adds support for the GPIO function in Crystal Cove.
v2:
- Use IRQ chip helper to provide irqdomain.
- Implement .remove and can now build as a module.
- Various fix for unreadable or ugly code pieces.
v3:
- More fix in irq_handler and probe.
v4:
- Minor fix of one return statement.
Signed-off-by: Yang, Bin
Signed-off-by: Zhu, Lejun
Reviewed-by: Mika Westerberg
---
drivers/gpio/Kconfig| 13 ++
drivers/gpio/Makefile | 1 +
drivers/gpio/gpio-crystalcove.c | 345
3 files changed, 359 insertions(+)
create mode 100644 drivers/gpio/gpio-crystalcove.c
diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
index a86c49a..fed08d9d 100644
--- a/drivers/gpio/Kconfig
+++ b/drivers/gpio/Kconfig
@@ -440,6 +440,19 @@ config GPIO_ARIZONA
help
Support for GPIOs on Wolfson Arizona class devices.
+config GPIO_CRYSTAL_COVE
+ tristate "GPIO support for Crystal Cove PMIC"
+ depends on INTEL_SOC_PMIC
+ select GPIOLIB_IRQCHIP
+ help
+ Support for GPIO pins on Crystal Cove PMIC.
+
+ Say Yes if you have a Intel SoC based tablet with Crystal Cove PMIC
+ inside.
+
+ This driver can also be built as a module. If so, the module will be
+ called gpio-crystalcove.
+
config GPIO_LP3943
tristate "TI/National Semiconductor LP3943 GPIO expander"
depends on MFD_LP3943
diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile
index 6309aff..e6cd935 100644
--- a/drivers/gpio/Makefile
+++ b/drivers/gpio/Makefile
@@ -20,6 +20,7 @@ obj-$(CONFIG_GPIO_BCM_KONA) += gpio-bcm-kona.o
obj-$(CONFIG_GPIO_BT8XX) += gpio-bt8xx.o
obj-$(CONFIG_GPIO_CLPS711X)+= gpio-clps711x.o
obj-$(CONFIG_GPIO_CS5535) += gpio-cs5535.o
+obj-$(CONFIG_GPIO_CRYSTAL_COVE)+= gpio-crystalcove.o
obj-$(CONFIG_GPIO_DA9052) += gpio-da9052.o
obj-$(CONFIG_GPIO_DA9055) += gpio-da9055.o
obj-$(CONFIG_GPIO_DAVINCI) += gpio-davinci.o
diff --git a/drivers/gpio/gpio-crystalcove.c b/drivers/gpio/gpio-crystalcove.c
new file mode 100644
index 000..76b6d57
--- /dev/null
+++ b/drivers/gpio/gpio-crystalcove.c
@@ -0,0 +1,345 @@
+/*
+ * gpio-crystalcove.c - Intel Crystal Cove GPIO Driver
+ *
+ * Copyright (C) 2012, 2014 Intel Corporation. All rights reserved.
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License version
+ * 2 as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ *
+ * Author: Yang, Bin
+ */
+
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+
+#define NUM_GPIO 16
+
+#define UPDATE_TYPEBIT(0)
+#define UPDATE_MASKBIT(1)
+
+#define GPIO0IRQ 0x0b
+#define GPIO1IRQ 0x0c
+#define MGPIO0IRQS00x19
+#define MGPIO1IRQS00x1a
+#define MGPIO0IRQSX0x1b
+#define MGPIO1IRQSX0x1c
+#define GPIO0P0CTLO0x2b
+#define GPIO0P0CTLI0x33
+#define GPIO1P0CTLO0x3b
+#define GPIO1P0CTLI0x43
+
+#define CTLI_INTCNT_NE (1 << 1)
+#define CTLI_INTCNT_PE (2 << 1)
+#define CTLI_INTCNT_BE (3 << 1)
+
+#define CTLO_DIR_OUT (1 << 5)
+
+#define CTLO_DRV_CMOS (0 << 4)
+#define CTLO_DRV_OD(1 << 4)
+
+#define CTLO_DRV_REN (1 << 3)
+
+#define CTLO_RVAL_2KDW (0)
+#define CTLO_RVAL_2KUP (1 << 1)
+#define CTLO_RVAL_50KDW(2 << 1)
+#define CTLO_RVAL_50KUP(3 << 1)
+
+#define CTLO_INPUT_DEF (CTLO_DRV_CMOS | CTLO_DRV_REN | CTLO_RVAL_2KUP)
+#define CTLO_OUTPUT_DEF(CTLO_DIR_OUT | CTLO_INPUT_DEF)
+
+#define GPIO_TO_CTL(gpio, dir) \
+ ((gpio < 8 ? GPIO0P0CTL ## dir : GPIO1P0CTL ## dir) + (gpio % 8))
+
+/**
+ * struct crystalcove_gpio - Crystal Cove GPIO controller
+ * @buslock: for bus lock/sync and unlock.
+ * @chip: the abstract gpio_chip structure.
+ * @update: pending IRQ setting update, to be written to the chip upon unlock.
+ * @trigger_type: the trigger type of the IRQ.
+ * @set_irq_mask: true if the IRQ mask needs to be set, false to clear.
+ */
+struct crystalcove_gpio {
+ struct mutexbuslock; /* irq_bus_lock */
+ struct gpio_chipchip;
+ int update;
+ int trigger_type;
+ bool
[PATCH v4] gpio: Add support for Intel SoC PMIC (Crystal Cove)
Devices based on Intel SoC products such as Baytrail have a Power Management IC. In the PMIC there are subsystems for voltage regulation, A/D conversion, GPIO and PWMs. The PMIC in Baytrail-T platform is called Crystal Cove. This patch adds support for the GPIO function in Crystal Cove. v2: - Use IRQ chip helper to provide irqdomain. - Implement .remove and can now build as a module. - Various fix for unreadable or ugly code pieces. v3: - More fix in irq_handler and probe. v4: - Minor fix of one return statement. Signed-off-by: Yang, Bin bin.y...@intel.com Signed-off-by: Zhu, Lejun lejun@linux.intel.com Reviewed-by: Mika Westerberg mika.westerb...@linux.intel.com --- drivers/gpio/Kconfig| 13 ++ drivers/gpio/Makefile | 1 + drivers/gpio/gpio-crystalcove.c | 345 3 files changed, 359 insertions(+) create mode 100644 drivers/gpio/gpio-crystalcove.c diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig index a86c49a..fed08d9d 100644 --- a/drivers/gpio/Kconfig +++ b/drivers/gpio/Kconfig @@ -440,6 +440,19 @@ config GPIO_ARIZONA help Support for GPIOs on Wolfson Arizona class devices. +config GPIO_CRYSTAL_COVE + tristate GPIO support for Crystal Cove PMIC + depends on INTEL_SOC_PMIC + select GPIOLIB_IRQCHIP + help + Support for GPIO pins on Crystal Cove PMIC. + + Say Yes if you have a Intel SoC based tablet with Crystal Cove PMIC + inside. + + This driver can also be built as a module. If so, the module will be + called gpio-crystalcove. + config GPIO_LP3943 tristate TI/National Semiconductor LP3943 GPIO expander depends on MFD_LP3943 diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile index 6309aff..e6cd935 100644 --- a/drivers/gpio/Makefile +++ b/drivers/gpio/Makefile @@ -20,6 +20,7 @@ obj-$(CONFIG_GPIO_BCM_KONA) += gpio-bcm-kona.o obj-$(CONFIG_GPIO_BT8XX) += gpio-bt8xx.o obj-$(CONFIG_GPIO_CLPS711X)+= gpio-clps711x.o obj-$(CONFIG_GPIO_CS5535) += gpio-cs5535.o +obj-$(CONFIG_GPIO_CRYSTAL_COVE)+= gpio-crystalcove.o obj-$(CONFIG_GPIO_DA9052) += gpio-da9052.o obj-$(CONFIG_GPIO_DA9055) += gpio-da9055.o obj-$(CONFIG_GPIO_DAVINCI) += gpio-davinci.o diff --git a/drivers/gpio/gpio-crystalcove.c b/drivers/gpio/gpio-crystalcove.c new file mode 100644 index 000..76b6d57 --- /dev/null +++ b/drivers/gpio/gpio-crystalcove.c @@ -0,0 +1,345 @@ +/* + * gpio-crystalcove.c - Intel Crystal Cove GPIO Driver + * + * Copyright (C) 2012, 2014 Intel Corporation. All rights reserved. + * + * This program is free software; you can redistribute it and/or + * modify it under the terms of the GNU General Public License version + * 2 as published by the Free Software Foundation. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * Author: Yang, Bin bin.y...@intel.com + */ + +#include linux/module.h +#include linux/kernel.h +#include linux/slab.h +#include linux/io.h +#include linux/delay.h +#include linux/interrupt.h +#include linux/device.h +#include linux/platform_device.h +#include linux/seq_file.h +#include linux/sched.h +#include linux/mfd/intel_soc_pmic.h +#include linux/gpio.h +#include linux/bitops.h + +#define NUM_GPIO 16 + +#define UPDATE_TYPEBIT(0) +#define UPDATE_MASKBIT(1) + +#define GPIO0IRQ 0x0b +#define GPIO1IRQ 0x0c +#define MGPIO0IRQS00x19 +#define MGPIO1IRQS00x1a +#define MGPIO0IRQSX0x1b +#define MGPIO1IRQSX0x1c +#define GPIO0P0CTLO0x2b +#define GPIO0P0CTLI0x33 +#define GPIO1P0CTLO0x3b +#define GPIO1P0CTLI0x43 + +#define CTLI_INTCNT_NE (1 1) +#define CTLI_INTCNT_PE (2 1) +#define CTLI_INTCNT_BE (3 1) + +#define CTLO_DIR_OUT (1 5) + +#define CTLO_DRV_CMOS (0 4) +#define CTLO_DRV_OD(1 4) + +#define CTLO_DRV_REN (1 3) + +#define CTLO_RVAL_2KDW (0) +#define CTLO_RVAL_2KUP (1 1) +#define CTLO_RVAL_50KDW(2 1) +#define CTLO_RVAL_50KUP(3 1) + +#define CTLO_INPUT_DEF (CTLO_DRV_CMOS | CTLO_DRV_REN | CTLO_RVAL_2KUP) +#define CTLO_OUTPUT_DEF(CTLO_DIR_OUT | CTLO_INPUT_DEF) + +#define GPIO_TO_CTL(gpio, dir) \ + ((gpio 8 ? GPIO0P0CTL ## dir : GPIO1P0CTL ## dir) + (gpio % 8)) + +/** + * struct crystalcove_gpio - Crystal Cove GPIO controller + * @buslock: for bus lock/sync and unlock. + * @chip: the abstract gpio_chip structure. + * @update: pending IRQ setting update, to be written to the chip upon unlock. + * @trigger_type: the trigger type of the IRQ. + * @set_irq_mask: true if the IRQ mask needs
