Re: [PATCH v3 1/5] mfd: tps65217: Add support for IRQs

Marcin Niestroj
2016-06-17T14:05:22Z

On 16.06.2016 15:03, Grygorii Strashko wrote:
> On 06/16/2016 02:41 PM, Marcin Niestroj wrote:
>> Add support for handling IRQs: power button, AC and USB power state
>> changes. Mask and interrupt bits are shared within one register, which
>> prevents us to use regmap_irq implementation. New irq_domain is
>> created in
>> order to add interrupt handling for each tps65217's subsystem. IRQ
>> resources have been added for charger subsystem to be able to notify
>> about
>> AC and USB state changes.
>>
>> Signed-off-by: Marcin Niestroj <m.niestroj@grinn-global.com>
>> ---
>> drivers/mfd/Kconfig | 1 +
>> drivers/mfd/tps65217.c | 194
>> +++++++++++++++++++++++++++++++++++++++++--
>> include/linux/mfd/tps65217.h | 11 +++
>> 3 files changed, 198 insertions(+), 8 deletions(-)
>>
>> diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
>> index 1bcf601..f8c9580 100644
>> --- a/drivers/mfd/Kconfig
>> +++ b/drivers/mfd/Kconfig
>> @@ -1200,6 +1200,7 @@ config MFD_TPS65217
>> depends on I2C
>> select MFD_CORE
>> select REGMAP_I2C
>> + select IRQ_DOMAIN
>> help
>> If you say yes here you get support for the TPS65217 series of
>> Power Management / White LED chips.
>> diff --git a/drivers/mfd/tps65217.c b/drivers/mfd/tps65217.c
>> index 049a6fc..7763dbc 100644
>> --- a/drivers/mfd/tps65217.c
>> +++ b/drivers/mfd/tps65217.c
>> @@ -15,22 +15,99 @@
>> * GNU General Public License for more details.
>> */
>>
>> -#include <linux/kernel.h>
>> #include <linux/device.h>
>> -#include <linux/module.h>
>> -#include <linux/platform_device.h>
>> +#include <linux/err.h>
>> #include <linux/init.h>
>> +#include <linux/interrupt.h>
>> #include <linux/i2c.h>
>> -#include <linux/slab.h>
>> -#include <linux/regmap.h>
>> -#include <linux/err.h>
>> +#include <linux/irq.h>
>> +#include <linux/irqdomain.h>
>> +#include <linux/kernel.h>
>> +#include <linux/module.h>
>> #include <linux/of.h>
>> #include <linux/of_device.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/regmap.h>
>> +#include <linux/slab.h>
>>
>> #include <linux/mfd/core.h>
>> #include <linux/mfd/tps65217.h>
>>
>> -static const struct mfd_cell tps65217s[] = {
>> +static struct resource charger_resources[] = {
>> + DEFINE_RES_IRQ_NAMED(TPS65217_IRQ_AC, "AC"),
>> + DEFINE_RES_IRQ_NAMED(TPS65217_IRQ_USB, "USB"),
>> +};
>> +
>> +struct tps65217_irq {
>> + int mask;
>> + int interrupt;
>> +};
>> +
>> +static const struct tps65217_irq tps65217_irqs[] = {
>> + [TPS65217_IRQ_PB] = {
>> + .mask = TPS65217_INT_PBM,
>> + .interrupt = TPS65217_INT_PBI,
>> + },
>> + [TPS65217_IRQ_AC] = {
>> + .mask = TPS65217_INT_ACM,
>> + .interrupt = TPS65217_INT_ACI,
>> + },
>> + [TPS65217_IRQ_USB] = {
>> + .mask = TPS65217_INT_USBM,
>> + .interrupt = TPS65217_INT_USBI,
>> + },
>> +};
>> +
>> +static void tps65217_irq_lock(struct irq_data *data)
>> +{
>> + struct tps65217 *tps = irq_data_get_irq_chip_data(data);
>> +
>> + mutex_lock(&tps->irq_lock);
>> +}
>> +
>> +static void tps65217_irq_sync_unlock(struct irq_data *data)
>> +{
>> + struct tps65217 *tps = irq_data_get_irq_chip_data(data);
>> + int ret;
>> +
>> + ret = tps65217_reg_write(tps, TPS65217_REG_INT, tps->irq_mask,
>> + TPS65217_PROTECT_NONE);
>> + if (ret != 0)
>> + dev_err(tps->dev, "Failed to sync IRQ masks\n");
>> +
>> + mutex_unlock(&tps->irq_lock);
>> +}
>> +
>> +static const inline struct tps65217_irq *
>> +irq_to_tps65217_irq(struct tps65217 *tps, struct irq_data *data)
>> +{
>> + return &tps65217_irqs[data->hwirq];
>> +}
>> +
>> +static void tps65217_irq_enable(struct irq_data *data)
>> +{
>> + struct tps65217 *tps = irq_data_get_irq_chip_data(data);
>> + const struct tps65217_irq *irq_data = irq_to_tps65217_irq(tps,
>> data);
>> +
>> + tps->irq_mask &= ~irq_data->mask;
>> +}
>> +
>> +static void tps65217_irq_disable(struct irq_data *data)
>> +{
>> + struct tps65217 *tps = irq_data_get_irq_chip_data(data);
>> + const struct tps65217_irq *irq_data = irq_to_tps65217_irq(tps,
>> data);
>> +
>> + tps->irq_mask |= irq_data->mask;
>> +}
>> +
>> +static struct irq_chip tps65217_irq_chip = {
>> + .irq_bus_lock = tps65217_irq_lock,
>> + .irq_bus_sync_unlock = tps65217_irq_sync_unlock,
>> + .irq_enable = tps65217_irq_enable,
>> + .irq_disable = tps65217_irq_disable,
>> +};
>> +
>> +static struct mfd_cell tps65217s[] = {
>> {
>> .name = "tps65217-pmic",
>> .of_compatible = "ti,tps65217-pmic",
>> @@ -41,10 +118,89 @@ static const struct mfd_cell tps65217s[] = {
>> },
>> {
>> .name = "tps65217-charger",
>> + .num_resources = ARRAY_SIZE(charger_resources),
>> + .resources = charger_resources,
>> .of_compatible = "ti,tps65217-charger",
>> },
>> };
>>
>> +static irqreturn_t tps65217_irq_thread(int irq, void *data)
>> +{
>> + struct tps65217 *tps = data;
>> + unsigned int status;
>> + bool handled = false;
>> + int i;
>> + int ret;
>> +
>> + ret = tps65217_reg_read(tps, TPS65217_REG_INT, &status);
>> + if (ret < 0) {
>> + dev_err(tps->dev, "Failed to read IRQ status: %d\n",
>> + ret);
>> + return IRQ_NONE;
>> + }
>> +
>> + for (i = 0; i < ARRAY_SIZE(tps65217_irqs); i++) {
>> + if (status & tps65217_irqs[i].interrupt) {
>> + handle_nested_irq(irq_find_mapping(tps->irq_domain, i));
>> + handled = true;
>> + }
>> + }
>> +
>> + if (handled)
>> + return IRQ_HANDLED;
>> +
>> + return IRQ_NONE;
>> +}
>> +
>> +static int tps65217_irq_map(struct irq_domain *h, unsigned int virq,
>> + irq_hw_number_t hw)
>> +{
>> + struct tps65217 *tps = h->host_data;
>> +
>> + irq_set_chip_data(virq, tps);
>> + irq_set_chip_and_handler(virq, &tps65217_irq_chip, handle_edge_irq);
>> + irq_set_nested_thread(virq, 1);
>> + irq_set_noprobe(virq);
>
> irq_set_parent()?

Yes, it is missing. Thanks for spotting it.

Will add that.

>
>> +
>> + return 0;
>> +}
>> +
>> +static const struct irq_domain_ops tps65217_irq_domain_ops = {
>> + .map = tps65217_irq_map,
>> +};
>> +
>> +static int tps65217_irq_init(struct tps65217 *tps, int irq)
>> +{
>> + int ret;
>> +
>> + mutex_init(&tps->irq_lock);
>> +
>> + /* Mask all interrupt sources */
>> + tps->irq_mask = (TPS65217_INT_RESERVEDM | TPS65217_INT_PBM
>> + | TPS65217_INT_ACM | TPS65217_INT_USBM);
>> + tps65217_reg_write(tps, TPS65217_REG_INT, tps->irq_mask,
>> + TPS65217_PROTECT_NONE);
>> +
>> + tps->irq_domain = irq_domain_add_linear(tps->dev->of_node,
>> + TPS65217_NUM_IRQ, &tps65217_irq_domain_ops, tps);
>> + if (!tps->irq_domain) {
>> + dev_err(tps->dev, "Could not create IRQ domain\n");
>> + return -ENOMEM;
>> + }
>> +
>> + ret = devm_request_threaded_irq(tps->dev, irq, NULL,
>> + tps65217_irq_thread,
>> + IRQF_TRIGGER_RISING | IRQF_ONESHOT,
>
> Are there any reasons why IRQ trigger type specified here explicitly?

Not really. I have configured it that way, it worked and I forgot about
it when preparing patches. Could you give some hint here?

>
>> + "tps65217-irq", tps);
>> + if (ret) {
>> + dev_err(tps->dev, "Failed to request IRQ %d: %d\n",
>> + irq, ret);
>> + return ret;
>> + }
>> +
>> + return 0;
>> +}
>
>
>
>

--
Marcin Niestroj