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

[Marcin Niestroj <m.niestroj@grinn-global.com>]

On 17.06.2016 18:09, Grygorii Strashko wrote:
> On 06/17/2016 06:31 PM, Marcin Niestroj wrote:
>>
>>
>> On 17.06.2016 16:42, Grygorii Strashko wrote:
>>> On 06/17/2016 05:05 PM, Marcin Niestroj wrote:
>>>>
>>>> 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(-)
>>>
>>> [...]
>>>
>>>>>> +{
>>>>>> +    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?
>>>>
>>>
>>> It's better to get it from DT and in case of DT boot it will - the real
>>> IRQ trigger type may depends on board.
>>>
>>
>> So what I understand, I need to remove IRQF_TRIGGER_RISING here.
>> Addidional flags will be passed by 'irq_flags' in
>> "interrupts = <irq_num irq_flags>" in DT, right?
>
> Indeed. And usually it is not "Addidional flags" - its mandatory cell for
>  the most of IRQ controllers.
>
> The problem with hard-coded IRQ trigger type values in code is that on
> different boards IRQ can be wired on different way - to the GIC/INTC,
> to SoC GPIO, to GPIO expanders and .. And they can support different sets
> of allowed IRQ triggering types. More over, on some boards IRQ signal can be
> inverted, for example :P
>
> So, It's more generic to not hard-code it if your driver is expected to be
> used only with DT.
> Also as per TRM http://www.ti.com/lit/ds/symlink/tps65217.pdf
> nINT - interrupt output (active low, open drain). Pin is pulled low if an interrupt bit is set. The
>        output goes high after the bit causing the interrupt in register INT has been read.
>        The interrupt sources can be masked in register INT, so no interrupt is generated when the
>        corresponding interrupt bit is set
>
>

Thanks very much for making this clear to me.

I have just send new patch version with fixes from your comments.

-- 
Regards,
Marcin Niestroj