Re: [RFC] Potential issue with GPIO/IRQ flags

Re: [RFC] Potential issue with GPIO/IRQ flags

[Rob Herring]

On Wed, Sep 16, 2015 at 4:07 PM, Andrew F. Davis <afd-l0cyMroinI0@public.gmane.org> wrote:
> Hello all,
>
> I've noticed that in a few DT bindings GPIO_ACTIVE_* defines are
> incorrectly used as interrupt flags. GPIO_ACTIVE_*'s are defined
> in:
>
> include/dt-bindings/gpio/gpio.h
>
> and are used to describe GPIO pins. IRQ types are defined in:
>
> include/dt-bindings/interrupt-controller/irq.h
>
> and are flags for IRQ pins.

It is perfectly valid for the meaning of the field to be defined by
the interrupt controller, and gpio interrupts could do something
different. We've tried to standardize this though.

> These seem to have been mixed up in a few places, take for example:
> arch/arm/boot/dts/tegra124-jetson-tk1.dts. On line 1393 we see the
> correct usage, but just before on line 1384 we see the issue.
> GPIO_ACTIVE_HIGH is defined as 0, the same as IRQ_TYPE_NONE. If
> this IRQ was not hard-coded with the correct edge in the driver
> this would not work. What the author probably wanted was
> IRQ_TYPE_LEVEL_HIGH.
>
> Now lets look at commit c21e678b256b, in this the IRQ flags did not
> matter as the correct flag was hard-coded (IRQF_TRIGGER_LOW), this
> patch moves this to the DT, but changed the flag to GPIO_ACTIVE_LOW
> instead of the desired IRQ_TYPE_LEVEL_LOW. GPIO_ACTIVE_LOW is defined
> as 1, or IRQ_TYPE_EDGE_RISING in IRQ flags, which is not the
> equivalent to IRQF_TRIGGER_LOW the author was probably looking for.
>
> A quick grep (git grep "interrupt.*GPIO_ACTIVE_") shows several more
> instances of this. I found this by using one of these files as an
> example and giving myself a lot of problems, so I would like to fix
> this before it spreads anymore.
>
> I have a couple of ideas of how to go at this, first would be to
> just replace the incorrect flags with what was intended, but for
> some of these I don't know what was intended and do not have the
> board to test.
>
> My other solution would be to just change all instances of the GPIO
> flags to their value corresponding IRQ flags:
>
> - interrupts = <11 GPIO_ACTIVE_LOW>;
> + interrupts = <11 IRQ_TYPE_EDGE_RISING>;
>
> this would not make any functional change as the defines would
> still evaluate to the same value, but would make it obvious where
> a problem may be and that they should probably be checked and
> corrected, maybe we could even put a comment after:
>
> - interrupts = <11 GPIO_ACTIVE_LOW>;
> + interrupts = <11 IRQ_TYPE_EDGE_RISING>; // FIXME: Check IRQ type
>
> Well, what do you think?

This seems fine. It is no less wrong.

Rob

Re: [RFC] Potential issue with GPIO/IRQ flags

[Rob Herring]

On Wed, Sep 16, 2015 at 4:07 PM, Andrew F. Davis <afd@ti.com> wrote:
> Hello all,
>
> I've noticed that in a few DT bindings GPIO_ACTIVE_* defines are
> incorrectly used as interrupt flags. GPIO_ACTIVE_*'s are defined
> in:
>
> include/dt-bindings/gpio/gpio.h
>
> and are used to describe GPIO pins. IRQ types are defined in:
>
> include/dt-bindings/interrupt-controller/irq.h
>
> and are flags for IRQ pins.

It is perfectly valid for the meaning of the field to be defined by
the interrupt controller, and gpio interrupts could do something
different. We've tried to standardize this though.

> These seem to have been mixed up in a few places, take for example:
> arch/arm/boot/dts/tegra124-jetson-tk1.dts. On line 1393 we see the
> correct usage, but just before on line 1384 we see the issue.
> GPIO_ACTIVE_HIGH is defined as 0, the same as IRQ_TYPE_NONE. If
> this IRQ was not hard-coded with the correct edge in the driver
> this would not work. What the author probably wanted was
> IRQ_TYPE_LEVEL_HIGH.
>
> Now lets look at commit c21e678b256b, in this the IRQ flags did not
> matter as the correct flag was hard-coded (IRQF_TRIGGER_LOW), this
> patch moves this to the DT, but changed the flag to GPIO_ACTIVE_LOW
> instead of the desired IRQ_TYPE_LEVEL_LOW. GPIO_ACTIVE_LOW is defined
> as 1, or IRQ_TYPE_EDGE_RISING in IRQ flags, which is not the
> equivalent to IRQF_TRIGGER_LOW the author was probably looking for.
>
> A quick grep (git grep "interrupt.*GPIO_ACTIVE_") shows several more
> instances of this. I found this by using one of these files as an
> example and giving myself a lot of problems, so I would like to fix
> this before it spreads anymore.
>
> I have a couple of ideas of how to go at this, first would be to
> just replace the incorrect flags with what was intended, but for
> some of these I don't know what was intended and do not have the
> board to test.
>
> My other solution would be to just change all instances of the GPIO
> flags to their value corresponding IRQ flags:
>
> - interrupts = <11 GPIO_ACTIVE_LOW>;
> + interrupts = <11 IRQ_TYPE_EDGE_RISING>;
>
> this would not make any functional change as the defines would
> still evaluate to the same value, but would make it obvious where
> a problem may be and that they should probably be checked and
> corrected, maybe we could even put a comment after:
>
> - interrupts = <11 GPIO_ACTIVE_LOW>;
> + interrupts = <11 IRQ_TYPE_EDGE_RISING>; // FIXME: Check IRQ type
>
> Well, what do you think?

This seems fine. It is no less wrong.

Rob

Re: [RFC] Potential issue with GPIO/IRQ flags

[Andrew F. Davis]

On 09/16/2015 08:26 PM, Rob Herring wrote:
> On Wed, Sep 16, 2015 at 4:07 PM, Andrew F. Davis <afd-l0cyMroinI0@public.gmane.org> wrote:
>> Hello all,
>>
>> I've noticed that in a few DT bindings GPIO_ACTIVE_* defines are
>> incorrectly used as interrupt flags. GPIO_ACTIVE_*'s are defined
>> in:
>>
>> include/dt-bindings/gpio/gpio.h
>>
>> and are used to describe GPIO pins. IRQ types are defined in:
>>
>> include/dt-bindings/interrupt-controller/irq.h
>>
>> and are flags for IRQ pins.
>
> It is perfectly valid for the meaning of the field to be defined by
> the interrupt controller, and gpio interrupts could do something
> different. We've tried to standardize this though.
>

Sure, but in this case these are not what the interrupt controller
is expecting.

>> These seem to have been mixed up in a few places, take for example:
>> arch/arm/boot/dts/tegra124-jetson-tk1.dts. On line 1393 we see the
>> correct usage, but just before on line 1384 we see the issue.
>> GPIO_ACTIVE_HIGH is defined as 0, the same as IRQ_TYPE_NONE. If
>> this IRQ was not hard-coded with the correct edge in the driver
>> this would not work. What the author probably wanted was
>> IRQ_TYPE_LEVEL_HIGH.
>>
>> Now lets look at commit c21e678b256b, in this the IRQ flags did not
>> matter as the correct flag was hard-coded (IRQF_TRIGGER_LOW), this
>> patch moves this to the DT, but changed the flag to GPIO_ACTIVE_LOW
>> instead of the desired IRQ_TYPE_LEVEL_LOW. GPIO_ACTIVE_LOW is defined
>> as 1, or IRQ_TYPE_EDGE_RISING in IRQ flags, which is not the
>> equivalent to IRQF_TRIGGER_LOW the author was probably looking for.
>>
>> A quick grep (git grep "interrupt.*GPIO_ACTIVE_") shows several more
>> instances of this. I found this by using one of these files as an
>> example and giving myself a lot of problems, so I would like to fix
>> this before it spreads anymore.
>>
>> I have a couple of ideas of how to go at this, first would be to
>> just replace the incorrect flags with what was intended, but for
>> some of these I don't know what was intended and do not have the
>> board to test.
>>
>> My other solution would be to just change all instances of the GPIO
>> flags to their value corresponding IRQ flags:
>>
>> - interrupts = <11 GPIO_ACTIVE_LOW>;
>> + interrupts = <11 IRQ_TYPE_EDGE_RISING>;
>>
>> this would not make any functional change as the defines would
>> still evaluate to the same value, but would make it obvious where
>> a problem may be and that they should probably be checked and
>> corrected, maybe we could even put a comment after:
>>
>> - interrupts = <11 GPIO_ACTIVE_LOW>;
>> + interrupts = <11 IRQ_TYPE_EDGE_RISING>; // FIXME: Check IRQ type
>>
>> Well, what do you think?
>
> This seems fine. It is no less wrong.
>

I'm not sure what you mean here.

Regards,
Andrew

> Rob
>
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC] Potential issue with GPIO/IRQ flags

[Andrew F. Davis]

On 09/16/2015 08:26 PM, Rob Herring wrote:
> On Wed, Sep 16, 2015 at 4:07 PM, Andrew F. Davis <afd@ti.com> wrote:
>> Hello all,
>>
>> I've noticed that in a few DT bindings GPIO_ACTIVE_* defines are
>> incorrectly used as interrupt flags. GPIO_ACTIVE_*'s are defined
>> in:
>>
>> include/dt-bindings/gpio/gpio.h
>>
>> and are used to describe GPIO pins. IRQ types are defined in:
>>
>> include/dt-bindings/interrupt-controller/irq.h
>>
>> and are flags for IRQ pins.
>
> It is perfectly valid for the meaning of the field to be defined by
> the interrupt controller, and gpio interrupts could do something
> different. We've tried to standardize this though.
>

Sure, but in this case these are not what the interrupt controller
is expecting.

>> These seem to have been mixed up in a few places, take for example:
>> arch/arm/boot/dts/tegra124-jetson-tk1.dts. On line 1393 we see the
>> correct usage, but just before on line 1384 we see the issue.
>> GPIO_ACTIVE_HIGH is defined as 0, the same as IRQ_TYPE_NONE. If
>> this IRQ was not hard-coded with the correct edge in the driver
>> this would not work. What the author probably wanted was
>> IRQ_TYPE_LEVEL_HIGH.
>>
>> Now lets look at commit c21e678b256b, in this the IRQ flags did not
>> matter as the correct flag was hard-coded (IRQF_TRIGGER_LOW), this
>> patch moves this to the DT, but changed the flag to GPIO_ACTIVE_LOW
>> instead of the desired IRQ_TYPE_LEVEL_LOW. GPIO_ACTIVE_LOW is defined
>> as 1, or IRQ_TYPE_EDGE_RISING in IRQ flags, which is not the
>> equivalent to IRQF_TRIGGER_LOW the author was probably looking for.
>>
>> A quick grep (git grep "interrupt.*GPIO_ACTIVE_") shows several more
>> instances of this. I found this by using one of these files as an
>> example and giving myself a lot of problems, so I would like to fix
>> this before it spreads anymore.
>>
>> I have a couple of ideas of how to go at this, first would be to
>> just replace the incorrect flags with what was intended, but for
>> some of these I don't know what was intended and do not have the
>> board to test.
>>
>> My other solution would be to just change all instances of the GPIO
>> flags to their value corresponding IRQ flags:
>>
>> - interrupts = <11 GPIO_ACTIVE_LOW>;
>> + interrupts = <11 IRQ_TYPE_EDGE_RISING>;
>>
>> this would not make any functional change as the defines would
>> still evaluate to the same value, but would make it obvious where
>> a problem may be and that they should probably be checked and
>> corrected, maybe we could even put a comment after:
>>
>> - interrupts = <11 GPIO_ACTIVE_LOW>;
>> + interrupts = <11 IRQ_TYPE_EDGE_RISING>; // FIXME: Check IRQ type
>>
>> Well, what do you think?
>
> This seems fine. It is no less wrong.
>

I'm not sure what you mean here.

Regards,
Andrew

> Rob
>

Re: [RFC] Potential issue with GPIO/IRQ flags

[Rob Herring]

On Thu, Sep 17, 2015 at 10:53 AM, Andrew F. Davis <afd-l0cyMroinI0@public.gmane.org> wrote:
> On 09/16/2015 08:26 PM, Rob Herring wrote:
>>
>> On Wed, Sep 16, 2015 at 4:07 PM, Andrew F. Davis <afd-l0cyMroinI0@public.gmane.org> wrote:
>>>
>>> Hello all,
>>>
>>> I've noticed that in a few DT bindings GPIO_ACTIVE_* defines are
>>> incorrectly used as interrupt flags. GPIO_ACTIVE_*'s are defined
>>> in:
>>>
>>> include/dt-bindings/gpio/gpio.h
>>>
>>> and are used to describe GPIO pins. IRQ types are defined in:
>>>
>>> include/dt-bindings/interrupt-controller/irq.h
>>>
>>> and are flags for IRQ pins.
>>
>>
>> It is perfectly valid for the meaning of the field to be defined by
>> the interrupt controller, and gpio interrupts could do something
>> different. We've tried to standardize this though.
>>
>
> Sure, but in this case these are not what the interrupt controller
> is expecting.

Understood. I was talking generally, not this specific case.

>>> These seem to have been mixed up in a few places, take for example:
>>> arch/arm/boot/dts/tegra124-jetson-tk1.dts. On line 1393 we see the
>>> correct usage, but just before on line 1384 we see the issue.
>>> GPIO_ACTIVE_HIGH is defined as 0, the same as IRQ_TYPE_NONE. If
>>> this IRQ was not hard-coded with the correct edge in the driver
>>> this would not work. What the author probably wanted was
>>> IRQ_TYPE_LEVEL_HIGH.
>>>
>>> Now lets look at commit c21e678b256b, in this the IRQ flags did not
>>> matter as the correct flag was hard-coded (IRQF_TRIGGER_LOW), this
>>> patch moves this to the DT, but changed the flag to GPIO_ACTIVE_LOW
>>> instead of the desired IRQ_TYPE_LEVEL_LOW. GPIO_ACTIVE_LOW is defined
>>> as 1, or IRQ_TYPE_EDGE_RISING in IRQ flags, which is not the
>>> equivalent to IRQF_TRIGGER_LOW the author was probably looking for.
>>>
>>> A quick grep (git grep "interrupt.*GPIO_ACTIVE_") shows several more
>>> instances of this. I found this by using one of these files as an
>>> example and giving myself a lot of problems, so I would like to fix
>>> this before it spreads anymore.
>>>
>>> I have a couple of ideas of how to go at this, first would be to
>>> just replace the incorrect flags with what was intended, but for
>>> some of these I don't know what was intended and do not have the
>>> board to test.
>>>
>>> My other solution would be to just change all instances of the GPIO
>>> flags to their value corresponding IRQ flags:
>>>
>>> - interrupts = <11 GPIO_ACTIVE_LOW>;
>>> + interrupts = <11 IRQ_TYPE_EDGE_RISING>;
>>>
>>> this would not make any functional change as the defines would
>>> still evaluate to the same value, but would make it obvious where
>>> a problem may be and that they should probably be checked and
>>> corrected, maybe we could even put a comment after:
>>>
>>> - interrupts = <11 GPIO_ACTIVE_LOW>;
>>> + interrupts = <11 IRQ_TYPE_EDGE_RISING>; // FIXME: Check IRQ type
>>>
>>> Well, what do you think?
>>
>>
>> This seems fine. It is no less wrong.
>>
>
> I'm not sure what you mean here.

In this example, the correct value is probably IRQ_TYPE_LEVEL_LOW or
IRQ_TYPE_EDGE_FALLING if the original text was correct in its
intentions (but broken in implementation). Since the change you
propose doesn't change the actual dtb at all, if it was wrong before
it will still be wrong.

Rob

Re: [RFC] Potential issue with GPIO/IRQ flags

[Rob Herring]

On Thu, Sep 17, 2015 at 10:53 AM, Andrew F. Davis <afd@ti.com> wrote:
> On 09/16/2015 08:26 PM, Rob Herring wrote:
>>
>> On Wed, Sep 16, 2015 at 4:07 PM, Andrew F. Davis <afd@ti.com> wrote:
>>>
>>> Hello all,
>>>
>>> I've noticed that in a few DT bindings GPIO_ACTIVE_* defines are
>>> incorrectly used as interrupt flags. GPIO_ACTIVE_*'s are defined
>>> in:
>>>
>>> include/dt-bindings/gpio/gpio.h
>>>
>>> and are used to describe GPIO pins. IRQ types are defined in:
>>>
>>> include/dt-bindings/interrupt-controller/irq.h
>>>
>>> and are flags for IRQ pins.
>>
>>
>> It is perfectly valid for the meaning of the field to be defined by
>> the interrupt controller, and gpio interrupts could do something
>> different. We've tried to standardize this though.
>>
>
> Sure, but in this case these are not what the interrupt controller
> is expecting.

Understood. I was talking generally, not this specific case.

>>> These seem to have been mixed up in a few places, take for example:
>>> arch/arm/boot/dts/tegra124-jetson-tk1.dts. On line 1393 we see the
>>> correct usage, but just before on line 1384 we see the issue.
>>> GPIO_ACTIVE_HIGH is defined as 0, the same as IRQ_TYPE_NONE. If
>>> this IRQ was not hard-coded with the correct edge in the driver
>>> this would not work. What the author probably wanted was
>>> IRQ_TYPE_LEVEL_HIGH.
>>>
>>> Now lets look at commit c21e678b256b, in this the IRQ flags did not
>>> matter as the correct flag was hard-coded (IRQF_TRIGGER_LOW), this
>>> patch moves this to the DT, but changed the flag to GPIO_ACTIVE_LOW
>>> instead of the desired IRQ_TYPE_LEVEL_LOW. GPIO_ACTIVE_LOW is defined
>>> as 1, or IRQ_TYPE_EDGE_RISING in IRQ flags, which is not the
>>> equivalent to IRQF_TRIGGER_LOW the author was probably looking for.
>>>
>>> A quick grep (git grep "interrupt.*GPIO_ACTIVE_") shows several more
>>> instances of this. I found this by using one of these files as an
>>> example and giving myself a lot of problems, so I would like to fix
>>> this before it spreads anymore.
>>>
>>> I have a couple of ideas of how to go at this, first would be to
>>> just replace the incorrect flags with what was intended, but for
>>> some of these I don't know what was intended and do not have the
>>> board to test.
>>>
>>> My other solution would be to just change all instances of the GPIO
>>> flags to their value corresponding IRQ flags:
>>>
>>> - interrupts = <11 GPIO_ACTIVE_LOW>;
>>> + interrupts = <11 IRQ_TYPE_EDGE_RISING>;
>>>
>>> this would not make any functional change as the defines would
>>> still evaluate to the same value, but would make it obvious where
>>> a problem may be and that they should probably be checked and
>>> corrected, maybe we could even put a comment after:
>>>
>>> - interrupts = <11 GPIO_ACTIVE_LOW>;
>>> + interrupts = <11 IRQ_TYPE_EDGE_RISING>; // FIXME: Check IRQ type
>>>
>>> Well, what do you think?
>>
>>
>> This seems fine. It is no less wrong.
>>
>
> I'm not sure what you mean here.

In this example, the correct value is probably IRQ_TYPE_LEVEL_LOW or
IRQ_TYPE_EDGE_FALLING if the original text was correct in its
intentions (but broken in implementation). Since the change you
propose doesn't change the actual dtb at all, if it was wrong before
it will still be wrong.

Rob

Re: [RFC] Potential issue with GPIO/IRQ flags

[Andrew F. Davis]

On 09/17/2015 12:20 PM, Rob Herring wrote:
> On Thu, Sep 17, 2015 at 10:53 AM, Andrew F. Davis <afd@ti.com> wrote:
>> On 09/16/2015 08:26 PM, Rob Herring wrote:
>>>
>>> On Wed, Sep 16, 2015 at 4:07 PM, Andrew F. Davis <afd@ti.com> wrote:
>>>>
>>>> Hello all,
>>>>
>>>> I've noticed that in a few DT bindings GPIO_ACTIVE_* defines are
>>>> incorrectly used as interrupt flags. GPIO_ACTIVE_*'s are defined
>>>> in:
>>>>
>>>> include/dt-bindings/gpio/gpio.h
>>>>
>>>> and are used to describe GPIO pins. IRQ types are defined in:
>>>>
>>>> include/dt-bindings/interrupt-controller/irq.h
>>>>
>>>> and are flags for IRQ pins.
>>>
>>>
>>> It is perfectly valid for the meaning of the field to be defined by
>>> the interrupt controller, and gpio interrupts could do something
>>> different. We've tried to standardize this though.
>>>
>>
>> Sure, but in this case these are not what the interrupt controller
>> is expecting.
>
> Understood. I was talking generally, not this specific case.
>
>>>> These seem to have been mixed up in a few places, take for example:
>>>> arch/arm/boot/dts/tegra124-jetson-tk1.dts. On line 1393 we see the
>>>> correct usage, but just before on line 1384 we see the issue.
>>>> GPIO_ACTIVE_HIGH is defined as 0, the same as IRQ_TYPE_NONE. If
>>>> this IRQ was not hard-coded with the correct edge in the driver
>>>> this would not work. What the author probably wanted was
>>>> IRQ_TYPE_LEVEL_HIGH.
>>>>
>>>> Now lets look at commit c21e678b256b, in this the IRQ flags did not
>>>> matter as the correct flag was hard-coded (IRQF_TRIGGER_LOW), this
>>>> patch moves this to the DT, but changed the flag to GPIO_ACTIVE_LOW
>>>> instead of the desired IRQ_TYPE_LEVEL_LOW. GPIO_ACTIVE_LOW is defined
>>>> as 1, or IRQ_TYPE_EDGE_RISING in IRQ flags, which is not the
>>>> equivalent to IRQF_TRIGGER_LOW the author was probably looking for.
>>>>
>>>> A quick grep (git grep "interrupt.*GPIO_ACTIVE_") shows several more
>>>> instances of this. I found this by using one of these files as an
>>>> example and giving myself a lot of problems, so I would like to fix
>>>> this before it spreads anymore.
>>>>
>>>> I have a couple of ideas of how to go at this, first would be to
>>>> just replace the incorrect flags with what was intended, but for
>>>> some of these I don't know what was intended and do not have the
>>>> board to test.
>>>>
>>>> My other solution would be to just change all instances of the GPIO
>>>> flags to their value corresponding IRQ flags:
>>>>
>>>> - interrupts = <11 GPIO_ACTIVE_LOW>;
>>>> + interrupts = <11 IRQ_TYPE_EDGE_RISING>;
>>>>
>>>> this would not make any functional change as the defines would
>>>> still evaluate to the same value, but would make it obvious where
>>>> a problem may be and that they should probably be checked and
>>>> corrected, maybe we could even put a comment after:
>>>>
>>>> - interrupts = <11 GPIO_ACTIVE_LOW>;
>>>> + interrupts = <11 IRQ_TYPE_EDGE_RISING>; // FIXME: Check IRQ type
>>>>
>>>> Well, what do you think?
>>>
>>>
>>> This seems fine. It is no less wrong.
>>>
>>
>> I'm not sure what you mean here.
>
> In this example, the correct value is probably IRQ_TYPE_LEVEL_LOW or
> IRQ_TYPE_EDGE_FALLING if the original text was correct in its
> intentions (but broken in implementation). Since the change you
> propose doesn't change the actual dtb at all, if it was wrong before
> it will still be wrong.
>

I see, that's kinda what I want, maybe for this example the intentions
are obvious but my concern is with a couple others that I don't know
what the trigger was meant to be and don't have a board to test the
changes with, so I would never be sure if I causing any regressions
with the fixes. Most of the affected boards are Tegra based (that's
why I cc'd linux-tegra), I was hoping they would be interested in
testing and finding the right values.

Andrew

> Rob
>

-- 
Andrew F. Davis

Re: [RFC] Potential issue with GPIO/IRQ flags

[Stephen Warren]

On 09/17/2015 11:21 AM, Andrew F. Davis wrote:
> 
> 
> On 09/17/2015 12:20 PM, Rob Herring wrote:
>> On Thu, Sep 17, 2015 at 10:53 AM, Andrew F. Davis <afd-l0cyMroinI0@public.gmane.org> wrote:
>>> On 09/16/2015 08:26 PM, Rob Herring wrote:
>>>>
>>>> On Wed, Sep 16, 2015 at 4:07 PM, Andrew F. Davis <afd-l0cyMroinI0@public.gmane.org> wrote:
>>>>>
>>>>> Hello all,
>>>>>
>>>>> I've noticed that in a few DT bindings GPIO_ACTIVE_* defines are
>>>>> incorrectly used as interrupt flags. GPIO_ACTIVE_*'s are defined
>>>>> in:
>>>>>
>>>>> include/dt-bindings/gpio/gpio.h
>>>>>
>>>>> and are used to describe GPIO pins. IRQ types are defined in:
>>>>>
>>>>> include/dt-bindings/interrupt-controller/irq.h
>>>>>
>>>>> and are flags for IRQ pins.
>>>>
>>>>
>>>> It is perfectly valid for the meaning of the field to be defined by
>>>> the interrupt controller, and gpio interrupts could do something
>>>> different. We've tried to standardize this though.
>>>>
>>>
>>> Sure, but in this case these are not what the interrupt controller
>>> is expecting.
>>
>> Understood. I was talking generally, not this specific case.
>>
>>>>> These seem to have been mixed up in a few places, take for example:
>>>>> arch/arm/boot/dts/tegra124-jetson-tk1.dts. On line 1393 we see the
>>>>> correct usage, but just before on line 1384 we see the issue.
>>>>> GPIO_ACTIVE_HIGH is defined as 0, the same as IRQ_TYPE_NONE. If
>>>>> this IRQ was not hard-coded with the correct edge in the driver
>>>>> this would not work. What the author probably wanted was
>>>>> IRQ_TYPE_LEVEL_HIGH.
>>>>>
>>>>> Now lets look at commit c21e678b256b, in this the IRQ flags did not
>>>>> matter as the correct flag was hard-coded (IRQF_TRIGGER_LOW), this
>>>>> patch moves this to the DT, but changed the flag to GPIO_ACTIVE_LOW
>>>>> instead of the desired IRQ_TYPE_LEVEL_LOW. GPIO_ACTIVE_LOW is defined
>>>>> as 1, or IRQ_TYPE_EDGE_RISING in IRQ flags, which is not the
>>>>> equivalent to IRQF_TRIGGER_LOW the author was probably looking for.
>>>>>
>>>>> A quick grep (git grep "interrupt.*GPIO_ACTIVE_") shows several more
>>>>> instances of this. I found this by using one of these files as an
>>>>> example and giving myself a lot of problems, so I would like to fix
>>>>> this before it spreads anymore.
>>>>>
>>>>> I have a couple of ideas of how to go at this, first would be to
>>>>> just replace the incorrect flags with what was intended, but for
>>>>> some of these I don't know what was intended and do not have the
>>>>> board to test.
>>>>>
>>>>> My other solution would be to just change all instances of the GPIO
>>>>> flags to their value corresponding IRQ flags:
>>>>>
>>>>> - interrupts = <11 GPIO_ACTIVE_LOW>;
>>>>> + interrupts = <11 IRQ_TYPE_EDGE_RISING>;
>>>>>
>>>>> this would not make any functional change as the defines would
>>>>> still evaluate to the same value, but would make it obvious where
>>>>> a problem may be and that they should probably be checked and
>>>>> corrected, maybe we could even put a comment after:
>>>>>
>>>>> - interrupts = <11 GPIO_ACTIVE_LOW>;
>>>>> + interrupts = <11 IRQ_TYPE_EDGE_RISING>; // FIXME: Check IRQ type
>>>>>
>>>>> Well, what do you think?
>>>>
>>>>
>>>> This seems fine. It is no less wrong.
>>>>
>>>
>>> I'm not sure what you mean here.
>>
>> In this example, the correct value is probably IRQ_TYPE_LEVEL_LOW or
>> IRQ_TYPE_EDGE_FALLING if the original text was correct in its
>> intentions (but broken in implementation). Since the change you
>> propose doesn't change the actual dtb at all, if it was wrong before
>> it will still be wrong.
>>
> 
> I see, that's kinda what I want, maybe for this example the intentions
> are obvious but my concern is with a couple others that I don't know
> what the trigger was meant to be and don't have a board to test the
> changes with, so I would never be sure if I causing any regressions
> with the fixes. Most of the affected boards are Tegra based (that's
> why I cc'd linux-tegra), I was hoping they would be interested in
> testing and finding the right values.

Presumably/hopefully if you send specific patches, the various
maintainers/owners of those DT files will validate/ack then; you don't
need to be able to test all of the changes yourself.

Re: [RFC] Potential issue with GPIO/IRQ flags

[Stephen Warren]

On 09/17/2015 11:21 AM, Andrew F. Davis wrote:
> 
> 
> On 09/17/2015 12:20 PM, Rob Herring wrote:
>> On Thu, Sep 17, 2015 at 10:53 AM, Andrew F. Davis <afd@ti.com> wrote:
>>> On 09/16/2015 08:26 PM, Rob Herring wrote:
>>>>
>>>> On Wed, Sep 16, 2015 at 4:07 PM, Andrew F. Davis <afd@ti.com> wrote:
>>>>>
>>>>> Hello all,
>>>>>
>>>>> I've noticed that in a few DT bindings GPIO_ACTIVE_* defines are
>>>>> incorrectly used as interrupt flags. GPIO_ACTIVE_*'s are defined
>>>>> in:
>>>>>
>>>>> include/dt-bindings/gpio/gpio.h
>>>>>
>>>>> and are used to describe GPIO pins. IRQ types are defined in:
>>>>>
>>>>> include/dt-bindings/interrupt-controller/irq.h
>>>>>
>>>>> and are flags for IRQ pins.
>>>>
>>>>
>>>> It is perfectly valid for the meaning of the field to be defined by
>>>> the interrupt controller, and gpio interrupts could do something
>>>> different. We've tried to standardize this though.
>>>>
>>>
>>> Sure, but in this case these are not what the interrupt controller
>>> is expecting.
>>
>> Understood. I was talking generally, not this specific case.
>>
>>>>> These seem to have been mixed up in a few places, take for example:
>>>>> arch/arm/boot/dts/tegra124-jetson-tk1.dts. On line 1393 we see the
>>>>> correct usage, but just before on line 1384 we see the issue.
>>>>> GPIO_ACTIVE_HIGH is defined as 0, the same as IRQ_TYPE_NONE. If
>>>>> this IRQ was not hard-coded with the correct edge in the driver
>>>>> this would not work. What the author probably wanted was
>>>>> IRQ_TYPE_LEVEL_HIGH.
>>>>>
>>>>> Now lets look at commit c21e678b256b, in this the IRQ flags did not
>>>>> matter as the correct flag was hard-coded (IRQF_TRIGGER_LOW), this
>>>>> patch moves this to the DT, but changed the flag to GPIO_ACTIVE_LOW
>>>>> instead of the desired IRQ_TYPE_LEVEL_LOW. GPIO_ACTIVE_LOW is defined
>>>>> as 1, or IRQ_TYPE_EDGE_RISING in IRQ flags, which is not the
>>>>> equivalent to IRQF_TRIGGER_LOW the author was probably looking for.
>>>>>
>>>>> A quick grep (git grep "interrupt.*GPIO_ACTIVE_") shows several more
>>>>> instances of this. I found this by using one of these files as an
>>>>> example and giving myself a lot of problems, so I would like to fix
>>>>> this before it spreads anymore.
>>>>>
>>>>> I have a couple of ideas of how to go at this, first would be to
>>>>> just replace the incorrect flags with what was intended, but for
>>>>> some of these I don't know what was intended and do not have the
>>>>> board to test.
>>>>>
>>>>> My other solution would be to just change all instances of the GPIO
>>>>> flags to their value corresponding IRQ flags:
>>>>>
>>>>> - interrupts = <11 GPIO_ACTIVE_LOW>;
>>>>> + interrupts = <11 IRQ_TYPE_EDGE_RISING>;
>>>>>
>>>>> this would not make any functional change as the defines would
>>>>> still evaluate to the same value, but would make it obvious where
>>>>> a problem may be and that they should probably be checked and
>>>>> corrected, maybe we could even put a comment after:
>>>>>
>>>>> - interrupts = <11 GPIO_ACTIVE_LOW>;
>>>>> + interrupts = <11 IRQ_TYPE_EDGE_RISING>; // FIXME: Check IRQ type
>>>>>
>>>>> Well, what do you think?
>>>>
>>>>
>>>> This seems fine. It is no less wrong.
>>>>
>>>
>>> I'm not sure what you mean here.
>>
>> In this example, the correct value is probably IRQ_TYPE_LEVEL_LOW or
>> IRQ_TYPE_EDGE_FALLING if the original text was correct in its
>> intentions (but broken in implementation). Since the change you
>> propose doesn't change the actual dtb at all, if it was wrong before
>> it will still be wrong.
>>
> 
> I see, that's kinda what I want, maybe for this example the intentions
> are obvious but my concern is with a couple others that I don't know
> what the trigger was meant to be and don't have a board to test the
> changes with, so I would never be sure if I causing any regressions
> with the fixes. Most of the affected boards are Tegra based (that's
> why I cc'd linux-tegra), I was hoping they would be interested in
> testing and finding the right values.

Presumably/hopefully if you send specific patches, the various
maintainers/owners of those DT files will validate/ack then; you don't
need to be able to test all of the changes yourself.

Re: [RFC] Potential issue with GPIO/IRQ flags

[Andrew F. Davis]

On 09/17/2015 05:35 PM, Stephen Warren wrote:
> On 09/17/2015 11:21 AM, Andrew F. Davis wrote:
>>
>>
>> On 09/17/2015 12:20 PM, Rob Herring wrote:
>>> On Thu, Sep 17, 2015 at 10:53 AM, Andrew F. Davis <afd-l0cyMroinI0@public.gmane.org> wrote:
>>>> On 09/16/2015 08:26 PM, Rob Herring wrote:
>>>>>
>>>>> On Wed, Sep 16, 2015 at 4:07 PM, Andrew F. Davis <afd-l0cyMroinI0@public.gmane.org> wrote:
>>>>>>
>>>>>> Hello all,
>>>>>>
>>>>>> I've noticed that in a few DT bindings GPIO_ACTIVE_* defines are
>>>>>> incorrectly used as interrupt flags. GPIO_ACTIVE_*'s are defined
>>>>>> in:
>>>>>>
>>>>>> include/dt-bindings/gpio/gpio.h
>>>>>>
>>>>>> and are used to describe GPIO pins. IRQ types are defined in:
>>>>>>
>>>>>> include/dt-bindings/interrupt-controller/irq.h
>>>>>>
>>>>>> and are flags for IRQ pins.
>>>>>
>>>>>
>>>>> It is perfectly valid for the meaning of the field to be defined by
>>>>> the interrupt controller, and gpio interrupts could do something
>>>>> different. We've tried to standardize this though.
>>>>>
>>>>
>>>> Sure, but in this case these are not what the interrupt controller
>>>> is expecting.
>>>
>>> Understood. I was talking generally, not this specific case.
>>>
>>>>>> These seem to have been mixed up in a few places, take for example:
>>>>>> arch/arm/boot/dts/tegra124-jetson-tk1.dts. On line 1393 we see the
>>>>>> correct usage, but just before on line 1384 we see the issue.
>>>>>> GPIO_ACTIVE_HIGH is defined as 0, the same as IRQ_TYPE_NONE. If
>>>>>> this IRQ was not hard-coded with the correct edge in the driver
>>>>>> this would not work. What the author probably wanted was
>>>>>> IRQ_TYPE_LEVEL_HIGH.
>>>>>>
>>>>>> Now lets look at commit c21e678b256b, in this the IRQ flags did not
>>>>>> matter as the correct flag was hard-coded (IRQF_TRIGGER_LOW), this
>>>>>> patch moves this to the DT, but changed the flag to GPIO_ACTIVE_LOW
>>>>>> instead of the desired IRQ_TYPE_LEVEL_LOW. GPIO_ACTIVE_LOW is defined
>>>>>> as 1, or IRQ_TYPE_EDGE_RISING in IRQ flags, which is not the
>>>>>> equivalent to IRQF_TRIGGER_LOW the author was probably looking for.
>>>>>>
>>>>>> A quick grep (git grep "interrupt.*GPIO_ACTIVE_") shows several more
>>>>>> instances of this. I found this by using one of these files as an
>>>>>> example and giving myself a lot of problems, so I would like to fix
>>>>>> this before it spreads anymore.
>>>>>>
>>>>>> I have a couple of ideas of how to go at this, first would be to
>>>>>> just replace the incorrect flags with what was intended, but for
>>>>>> some of these I don't know what was intended and do not have the
>>>>>> board to test.
>>>>>>
>>>>>> My other solution would be to just change all instances of the GPIO
>>>>>> flags to their value corresponding IRQ flags:
>>>>>>
>>>>>> - interrupts = <11 GPIO_ACTIVE_LOW>;
>>>>>> + interrupts = <11 IRQ_TYPE_EDGE_RISING>;
>>>>>>
>>>>>> this would not make any functional change as the defines would
>>>>>> still evaluate to the same value, but would make it obvious where
>>>>>> a problem may be and that they should probably be checked and
>>>>>> corrected, maybe we could even put a comment after:
>>>>>>
>>>>>> - interrupts = <11 GPIO_ACTIVE_LOW>;
>>>>>> + interrupts = <11 IRQ_TYPE_EDGE_RISING>; // FIXME: Check IRQ type
>>>>>>
>>>>>> Well, what do you think?
>>>>>
>>>>>
>>>>> This seems fine. It is no less wrong.
>>>>>
>>>>
>>>> I'm not sure what you mean here.
>>>
>>> In this example, the correct value is probably IRQ_TYPE_LEVEL_LOW or
>>> IRQ_TYPE_EDGE_FALLING if the original text was correct in its
>>> intentions (but broken in implementation). Since the change you
>>> propose doesn't change the actual dtb at all, if it was wrong before
>>> it will still be wrong.
>>>
>>
>> I see, that's kinda what I want, maybe for this example the intentions
>> are obvious but my concern is with a couple others that I don't know
>> what the trigger was meant to be and don't have a board to test the
>> changes with, so I would never be sure if I causing any regressions
>> with the fixes. Most of the affected boards are Tegra based (that's
>> why I cc'd linux-tegra), I was hoping they would be interested in
>> testing and finding the right values.
>
> Presumably/hopefully if you send specific patches, the various
> maintainers/owners of those DT files will validate/ack then; you don't
> need to be able to test all of the changes yourself.
>

Well that was what I was going to do, but I found in some cases I didn't
know what the right value should be. Submitting patches would be easier,
but instead I had to write that block of text to try to recruit some help
from the original authors and people with the boards to find these correct
values.

--
Andrew

Re: [RFC] Potential issue with GPIO/IRQ flags

[Andrew F. Davis]

On 09/17/2015 05:35 PM, Stephen Warren wrote:
> On 09/17/2015 11:21 AM, Andrew F. Davis wrote:
>>
>>
>> On 09/17/2015 12:20 PM, Rob Herring wrote:
>>> On Thu, Sep 17, 2015 at 10:53 AM, Andrew F. Davis <afd@ti.com> wrote:
>>>> On 09/16/2015 08:26 PM, Rob Herring wrote:
>>>>>
>>>>> On Wed, Sep 16, 2015 at 4:07 PM, Andrew F. Davis <afd@ti.com> wrote:
>>>>>>
>>>>>> Hello all,
>>>>>>
>>>>>> I've noticed that in a few DT bindings GPIO_ACTIVE_* defines are
>>>>>> incorrectly used as interrupt flags. GPIO_ACTIVE_*'s are defined
>>>>>> in:
>>>>>>
>>>>>> include/dt-bindings/gpio/gpio.h
>>>>>>
>>>>>> and are used to describe GPIO pins. IRQ types are defined in:
>>>>>>
>>>>>> include/dt-bindings/interrupt-controller/irq.h
>>>>>>
>>>>>> and are flags for IRQ pins.
>>>>>
>>>>>
>>>>> It is perfectly valid for the meaning of the field to be defined by
>>>>> the interrupt controller, and gpio interrupts could do something
>>>>> different. We've tried to standardize this though.
>>>>>
>>>>
>>>> Sure, but in this case these are not what the interrupt controller
>>>> is expecting.
>>>
>>> Understood. I was talking generally, not this specific case.
>>>
>>>>>> These seem to have been mixed up in a few places, take for example:
>>>>>> arch/arm/boot/dts/tegra124-jetson-tk1.dts. On line 1393 we see the
>>>>>> correct usage, but just before on line 1384 we see the issue.
>>>>>> GPIO_ACTIVE_HIGH is defined as 0, the same as IRQ_TYPE_NONE. If
>>>>>> this IRQ was not hard-coded with the correct edge in the driver
>>>>>> this would not work. What the author probably wanted was
>>>>>> IRQ_TYPE_LEVEL_HIGH.
>>>>>>
>>>>>> Now lets look at commit c21e678b256b, in this the IRQ flags did not
>>>>>> matter as the correct flag was hard-coded (IRQF_TRIGGER_LOW), this
>>>>>> patch moves this to the DT, but changed the flag to GPIO_ACTIVE_LOW
>>>>>> instead of the desired IRQ_TYPE_LEVEL_LOW. GPIO_ACTIVE_LOW is defined
>>>>>> as 1, or IRQ_TYPE_EDGE_RISING in IRQ flags, which is not the
>>>>>> equivalent to IRQF_TRIGGER_LOW the author was probably looking for.
>>>>>>
>>>>>> A quick grep (git grep "interrupt.*GPIO_ACTIVE_") shows several more
>>>>>> instances of this. I found this by using one of these files as an
>>>>>> example and giving myself a lot of problems, so I would like to fix
>>>>>> this before it spreads anymore.
>>>>>>
>>>>>> I have a couple of ideas of how to go at this, first would be to
>>>>>> just replace the incorrect flags with what was intended, but for
>>>>>> some of these I don't know what was intended and do not have the
>>>>>> board to test.
>>>>>>
>>>>>> My other solution would be to just change all instances of the GPIO
>>>>>> flags to their value corresponding IRQ flags:
>>>>>>
>>>>>> - interrupts = <11 GPIO_ACTIVE_LOW>;
>>>>>> + interrupts = <11 IRQ_TYPE_EDGE_RISING>;
>>>>>>
>>>>>> this would not make any functional change as the defines would
>>>>>> still evaluate to the same value, but would make it obvious where
>>>>>> a problem may be and that they should probably be checked and
>>>>>> corrected, maybe we could even put a comment after:
>>>>>>
>>>>>> - interrupts = <11 GPIO_ACTIVE_LOW>;
>>>>>> + interrupts = <11 IRQ_TYPE_EDGE_RISING>; // FIXME: Check IRQ type
>>>>>>
>>>>>> Well, what do you think?
>>>>>
>>>>>
>>>>> This seems fine. It is no less wrong.
>>>>>
>>>>
>>>> I'm not sure what you mean here.
>>>
>>> In this example, the correct value is probably IRQ_TYPE_LEVEL_LOW or
>>> IRQ_TYPE_EDGE_FALLING if the original text was correct in its
>>> intentions (but broken in implementation). Since the change you
>>> propose doesn't change the actual dtb at all, if it was wrong before
>>> it will still be wrong.
>>>
>>
>> I see, that's kinda what I want, maybe for this example the intentions
>> are obvious but my concern is with a couple others that I don't know
>> what the trigger was meant to be and don't have a board to test the
>> changes with, so I would never be sure if I causing any regressions
>> with the fixes. Most of the affected boards are Tegra based (that's
>> why I cc'd linux-tegra), I was hoping they would be interested in
>> testing and finding the right values.
>
> Presumably/hopefully if you send specific patches, the various
> maintainers/owners of those DT files will validate/ack then; you don't
> need to be able to test all of the changes yourself.
>

Well that was what I was going to do, but I found in some cases I didn't
know what the right value should be. Submitting patches would be easier,
but instead I had to write that block of text to try to recruit some help
from the original authors and people with the boards to find these correct
values.

--
Andrew

[RFC] Potential issue with GPIO/IRQ flags

[Andrew F. Davis]

Hello all,

I've noticed that in a few DT bindings GPIO_ACTIVE_* defines are
incorrectly used as interrupt flags. GPIO_ACTIVE_*'s are defined
in:

include/dt-bindings/gpio/gpio.h

and are used to describe GPIO pins. IRQ types are defined in:

include/dt-bindings/interrupt-controller/irq.h

and are flags for IRQ pins.

These seem to have been mixed up in a few places, take for example:
arch/arm/boot/dts/tegra124-jetson-tk1.dts. On line 1393 we see the
correct usage, but just before on line 1384 we see the issue.
GPIO_ACTIVE_HIGH is defined as 0, the same as IRQ_TYPE_NONE. If
this IRQ was not hard-coded with the correct edge in the driver
this would not work. What the author probably wanted was
IRQ_TYPE_LEVEL_HIGH.

Now lets look at commit c21e678b256b, in this the IRQ flags did not
matter as the correct flag was hard-coded (IRQF_TRIGGER_LOW), this
patch moves this to the DT, but changed the flag to GPIO_ACTIVE_LOW
instead of the desired IRQ_TYPE_LEVEL_LOW. GPIO_ACTIVE_LOW is defined
as 1, or IRQ_TYPE_EDGE_RISING in IRQ flags, which is not the
equivalent to IRQF_TRIGGER_LOW the author was probably looking for.

A quick grep (git grep "interrupt.*GPIO_ACTIVE_") shows several more
instances of this. I found this by using one of these files as an
example and giving myself a lot of problems, so I would like to fix
this before it spreads anymore.

I have a couple of ideas of how to go at this, first would be to
just replace the incorrect flags with what was intended, but for
some of these I don't know what was intended and do not have the
board to test.

My other solution would be to just change all instances of the GPIO
flags to their value corresponding IRQ flags:

- interrupts = <11 GPIO_ACTIVE_LOW>;
+ interrupts = <11 IRQ_TYPE_EDGE_RISING>;

this would not make any functional change as the defines would
still evaluate to the same value, but would make it obvious where
a problem may be and that they should probably be checked and
corrected, maybe we could even put a comment after:

- interrupts = <11 GPIO_ACTIVE_LOW>;
+ interrupts = <11 IRQ_TYPE_EDGE_RISING>; // FIXME: Check IRQ type

Well, what do you think?

Thanks,
-- 
Andrew F. Davis

[RFC] Potential issue with GPIO/IRQ flags

[Andrew F. Davis]

Hello all,

I've noticed that in a few DT bindings GPIO_ACTIVE_* defines are
incorrectly used as interrupt flags. GPIO_ACTIVE_*'s are defined
in:

include/dt-bindings/gpio/gpio.h

and are used to describe GPIO pins. IRQ types are defined in:

include/dt-bindings/interrupt-controller/irq.h

and are flags for IRQ pins.

These seem to have been mixed up in a few places, take for example:
arch/arm/boot/dts/tegra124-jetson-tk1.dts. On line 1393 we see the
correct usage, but just before on line 1384 we see the issue.
GPIO_ACTIVE_HIGH is defined as 0, the same as IRQ_TYPE_NONE. If
this IRQ was not hard-coded with the correct edge in the driver
this would not work. What the author probably wanted was
IRQ_TYPE_LEVEL_HIGH.

Now lets look at commit c21e678b256b, in this the IRQ flags did not
matter as the correct flag was hard-coded (IRQF_TRIGGER_LOW), this
patch moves this to the DT, but changed the flag to GPIO_ACTIVE_LOW
instead of the desired IRQ_TYPE_LEVEL_LOW. GPIO_ACTIVE_LOW is defined
as 1, or IRQ_TYPE_EDGE_RISING in IRQ flags, which is not the
equivalent to IRQF_TRIGGER_LOW the author was probably looking for.

A quick grep (git grep "interrupt.*GPIO_ACTIVE_") shows several more
instances of this. I found this by using one of these files as an
example and giving myself a lot of problems, so I would like to fix
this before it spreads anymore.

I have a couple of ideas of how to go at this, first would be to
just replace the incorrect flags with what was intended, but for
some of these I don't know what was intended and do not have the
board to test.

My other solution would be to just change all instances of the GPIO
flags to their value corresponding IRQ flags:

- interrupts = <11 GPIO_ACTIVE_LOW>;
+ interrupts = <11 IRQ_TYPE_EDGE_RISING>;

this would not make any functional change as the defines would
still evaluate to the same value, but would make it obvious where
a problem may be and that they should probably be checked and
corrected, maybe we could even put a comment after:

- interrupts = <11 GPIO_ACTIVE_LOW>;
+ interrupts = <11 IRQ_TYPE_EDGE_RISING>; // FIXME: Check IRQ type

Well, what do you think?

Thanks,
-- 
Andrew F. Davis