passing two interrupts two an I2C driver

passing two interrupts two an I2C driver

[Jacek Anaszewski]

Hi all,

I'd like to consult the implementation of DT binding for the I2C device
that exposes two interrupt pins (INT1 and INT2). Both pins can be
configured to generate either data ready interrupts or event interrupts.
I want to implement DT binding that will handle also the situation
when only one of the interrupt sources is routed from the device
to the CPU.

Below is my implementation using interrupt-map:

+++ b/Documentation/devicetree/bindings/iio/pressure/lps331ap.txt
@@ -0,0 +1,41 @@
+* STMicroelectronics LPS331AP barometer sensor
+
+Required properties:
+
+  - compatible : should be "st,lps331ap"
+  - reg : the I2C address of the barometer
+
+Optional properties:
+
+  - st,data-ready-interrupt-pin : redirect DRDY on pin INT1 (1) or pin 
INT2 (2) (u8)
+  - interrupt-parent : phandle to the interrupt map subnode
+  - interrupts : interrupt mapping for LPS331AP interrupt sources:
+                2 sources: 0 - data ready, 1 - threshold event
+  - irq-map : irq sub-node defining interrupt map
+             (all properties listed below are required):
+      - #interrupt-cells : should be 1
+      - #address-cells : should be 0
+      - #size-cells : should be 0
+      - interrupt-map : table of entries consisting of three child 
elements:
+         - unit_interrupt_specifier - 0 : data ready, 1 : threshold event
+         - interrupt parent phandle
+         - parent unit interrupt specifier consisiting of two elements:
+             - index of the interrupt within the controller
+             - flags : should be 0
+
+Example:
+
+lps331ap@5d {
+       compatible = "st,lps331ap";
+       reg = <0x5d>;
+       drdy-int-pin = /bits/ 8 <2>;
+       interrupt-parent = <&irq_map>;
+       interrupts = <0>, <1>;
+
+       irq_map: irq-map {
+               #interrupt-cells = <1>;
+               #address-cells = <0>;
+               #size-cells = <0>;
+               interrupt-map = <0 &gpf0 5 0>;
+       };
+};

And here is how the driver uses this information:

  - if interrupt-map is empty then the driver configures
    itself to work without interrupt support
  - if only one interrupt source is available then the driver
    configures the device to generate data ready interrupts on
    the corresponding INTx pin (in this case the driver must know which
    of the device pins is routed to the cpu -
    st,data-ready-interrupt-pin property conveys this information)
  - if both interrupt sources are available then the driver configures
    the device to generate data ready interrupts on the interrupt pin
    corresponding to the interrupt source with index 0 and event
    interrupts to the interrupt source with index 1.

This solution seems to be a little awkward so I'd like to ask
if there is any neater way to handle presented requirements.
The solution must facilitate passing information about two
interrupt sources two the I2C driver. I have been unable to find
similar solution in the kernel so far.

Thanks,
Jacek Anaszewski

Re: passing two interrupts two an I2C driver

[Stephen Warren]

On 08/16/2013 08:47 AM, Jacek Anaszewski wrote:
> Hi all,
> 
> I'd like to consult the implementation of DT binding for the I2C device
> that exposes two interrupt pins (INT1 and INT2). Both pins can be
> configured to generate either data ready interrupts or event interrupts.
> I want to implement DT binding that will handle also the situation
> when only one of the interrupt sources is routed from the device
> to the CPU.
> 
> Below is my implementation using interrupt-map:

> +  - interrupt-parent : phandle to the interrupt map subnode

When using interrupt-parent to point at an interrupt map, I believe you
usually just point at the current node; there's no need to a child node.

> +  - interrupts : interrupt mapping for LPS331AP interrupt sources:
> +                2 sources: 0 - data ready, 1 - threshold event

> +  - irq-map : irq sub-node defining interrupt map
> +             (all properties listed below are required):

So, this node isn't required.

> +      - #interrupt-cells : should be 1

> +      - #address-cells : should be 0
> +      - #size-cells : should be 0

There are no addressed entities in this node, so I don't see why those
two properties are needed.

> +      - interrupt-map : table of entries consisting of three child elements:
> +         - unit_interrupt_specifier - 0 : data ready, 1 : threshold event
> +         - interrupt parent phandle
> +         - parent unit interrupt specifier consisiting of two elements:
> +             - index of the interrupt within the controller
> +             - flags : should be 0

It's up to the binding for the node referenced by the phandle to define
how many cells need be present for "flags", and their meaning. This
binding shouldn't attempt to describe those. Equally, the concept of
interrupt-map should be defined elsewwere (e.g.
Documentation/devicetree/bindings/interrupt-controller/interrupts.txt);
it's a generic that shouldn't need duplication in each binding that uses
interrupts.

> +Example:
> +
> +lps331ap@5d {
> +       compatible = "st,lps331ap";
> +       reg = <0x5d>;
> +       drdy-int-pin = /bits/ 8 <2>;
> +       interrupt-parent = <&irq_map>;
> +       interrupts = <0>, <1>;
> +
> +       irq_map: irq-map {
> +               #interrupt-cells = <1>;
> +               #address-cells = <0>;
> +               #size-cells = <0>;
> +               interrupt-map = <0 &gpf0 5 0>;
> +       };
> +};
> 
> And here is how the driver uses this information:
> 
>  - if interrupt-map is empty then the driver configures
>    itself to work without interrupt support

The presence or lack of interrupt support should be driven by the
presence of the interrupts property. interrupt-map should only be used
(if present) to assist in the parsing of the interrupts property.

>  - if only one interrupt source is available then the driver
>    configures the device to generate data ready interrupts on
>    the corresponding INTx pin (in this case the driver must know which
>    of the device pins is routed to the cpu -
>    st,data-ready-interrupt-pin property conveys this information)
>  - if both interrupt sources are available then the driver configures
>    the device to generate data ready interrupts on the interrupt pin
>    corresponding to the interrupt source with index 0 and event
>    interrupts to the interrupt source with index 1.
>
> This solution seems to be a little awkward so I'd like to ask
> if there is any neater way to handle presented requirements.
> The solution must facilitate passing information about two
> interrupt sources two the I2C driver. I have been unable to find
> similar solution in the kernel so far.

Indeed. I think it would be better to work as follows:

interrupts: contains one or two interrupt specifiers. The first entry
always defines the data ready interrupt. The second entry, if present,
defines the threshold event interrupt. This at least allows the
following combinations to be very simple expressed:

* no interrrupts
* just data
* both data and threshold (assuming they're routed to the same parent)

(you could swap the order if it's likely to be more common to have just
a threshold interrupt without any data interrupt).

In order to allow the presence of a threshold interrupt but no data
interrupt, then I think you would need interrupt-map:

lps331ap: lps331ap@5d {
	compatible = "st,lps331ap";
	reg = <0x5d>;
	interrupt-parent = <&lps331ap>;
	interrupts = <0>, <1>;
	interrupt-map =	<0 0>, /* nowhere */
			<1 &gpf0 6 0>;
};

In either of those interrupt-map entries, you should be able to set the
phandle to 0 to indicate no interrupt, hence allowing the first entry to
have "no interrupt". Whether that works currently in the code, I have no
idea.

If only the interrupts property were (phandle, args*)*, we could just
set the phandle to 0 in the interrupts property directly. Oh well, roll
on DT 2.0;-)

Re: passing two interrupts two an I2C driver

[Mark Rutland]

On Fri, Aug 16, 2013 at 07:48:55PM +0100, Stephen Warren wrote:
> On 08/16/2013 08:47 AM, Jacek Anaszewski wrote:
> > Hi all,
> > 
> > I'd like to consult the implementation of DT binding for the I2C device
> > that exposes two interrupt pins (INT1 and INT2). Both pins can be
> > configured to generate either data ready interrupts or event interrupts.
> > I want to implement DT binding that will handle also the situation
> > when only one of the interrupt sources is routed from the device
> > to the CPU.
> > 
> > Below is my implementation using interrupt-map:
> 
> > +  - interrupt-parent : phandle to the interrupt map subnode
> 
> When using interrupt-parent to point at an interrupt map, I believe you
> usually just point at the current node; there's no need to a child node.
> 
> > +  - interrupts : interrupt mapping for LPS331AP interrupt sources:
> > +                2 sources: 0 - data ready, 1 - threshold event
> 
> > +  - irq-map : irq sub-node defining interrupt map
> > +             (all properties listed below are required):
> 
> So, this node isn't required.
> 
> > +      - #interrupt-cells : should be 1
> 
> > +      - #address-cells : should be 0
> > +      - #size-cells : should be 0
> 
> There are no addressed entities in this node, so I don't see why those
> two properties are needed.
> 
> > +      - interrupt-map : table of entries consisting of three child elements:
> > +         - unit_interrupt_specifier - 0 : data ready, 1 : threshold event
> > +         - interrupt parent phandle
> > +         - parent unit interrupt specifier consisiting of two elements:
> > +             - index of the interrupt within the controller
> > +             - flags : should be 0
> 
> It's up to the binding for the node referenced by the phandle to define
> how many cells need be present for "flags", and their meaning. This
> binding shouldn't attempt to describe those. Equally, the concept of
> interrupt-map should be defined elsewwere (e.g.
> Documentation/devicetree/bindings/interrupt-controller/interrupts.txt);
> it's a generic that shouldn't need duplication in each binding that uses
> interrupts.
> 
> > +Example:
> > +
> > +lps331ap@5d {
> > +       compatible = "st,lps331ap";
> > +       reg = <0x5d>;
> > +       drdy-int-pin = /bits/ 8 <2>;
> > +       interrupt-parent = <&irq_map>;
> > +       interrupts = <0>, <1>;
> > +
> > +       irq_map: irq-map {
> > +               #interrupt-cells = <1>;
> > +               #address-cells = <0>;
> > +               #size-cells = <0>;
> > +               interrupt-map = <0 &gpf0 5 0>;
> > +       };
> > +};
> > 
> > And here is how the driver uses this information:
> > 
> >  - if interrupt-map is empty then the driver configures
> >    itself to work without interrupt support
> 
> The presence or lack of interrupt support should be driven by the
> presence of the interrupts property. interrupt-map should only be used
> (if present) to assist in the parsing of the interrupts property.
> 
> >  - if only one interrupt source is available then the driver
> >    configures the device to generate data ready interrupts on
> >    the corresponding INTx pin (in this case the driver must know which
> >    of the device pins is routed to the cpu -
> >    st,data-ready-interrupt-pin property conveys this information)
> >  - if both interrupt sources are available then the driver configures
> >    the device to generate data ready interrupts on the interrupt pin
> >    corresponding to the interrupt source with index 0 and event
> >    interrupts to the interrupt source with index 1.
> >
> > This solution seems to be a little awkward so I'd like to ask
> > if there is any neater way to handle presented requirements.
> > The solution must facilitate passing information about two
> > interrupt sources two the I2C driver. I have been unable to find
> > similar solution in the kernel so far.
> 
> Indeed. I think it would be better to work as follows:
> 
> interrupts: contains one or two interrupt specifiers. The first entry
> always defines the data ready interrupt. The second entry, if present,
> defines the threshold event interrupt. This at least allows the
> following combinations to be very simple expressed:
> 
> * no interrrupts
> * just data
> * both data and threshold (assuming they're routed to the same parent)
> 
> (you could swap the order if it's likely to be more common to have just
> a threshold interrupt without any data interrupt).
> 
> In order to allow the presence of a threshold interrupt but no data
> interrupt, then I think you would need interrupt-map:
> 
> lps331ap: lps331ap@5d {
> 	compatible = "st,lps331ap";
> 	reg = <0x5d>;
> 	interrupt-parent = <&lps331ap>;
> 	interrupts = <0>, <1>;
> 	interrupt-map =	<0 0>, /* nowhere */
> 			<1 &gpf0 6 0>;
> };

The interrupt-names property exists for this purpose (describing
interrupts which may or may not be present). Describing a nonexistent
interrupt and mapping it nowhere feels like a hack to me when we can
describe exactly what's present.

Thanks,
Mark.

Re: passing two interrupts two an I2C driver

[Stephen Warren]

On 08/19/2013 02:42 AM, Mark Rutland wrote:
> On Fri, Aug 16, 2013 at 07:48:55PM +0100, Stephen Warren wrote:
>> On 08/16/2013 08:47 AM, Jacek Anaszewski wrote:
>>> Hi all,
>>>
>>> I'd like to consult the implementation of DT binding for the I2C device
>>> that exposes two interrupt pins (INT1 and INT2). Both pins can be
>>> configured to generate either data ready interrupts or event interrupts.
>>> I want to implement DT binding that will handle also the situation
>>> when only one of the interrupt sources is routed from the device
>>> to the CPU.
>>>
>>> Below is my implementation using interrupt-map:
>>
>>> +  - interrupt-parent : phandle to the interrupt map subnode
>>
>> When using interrupt-parent to point at an interrupt map, I believe you
>> usually just point at the current node; there's no need to a child node.
>>
>>> +  - interrupts : interrupt mapping for LPS331AP interrupt sources:
>>> +                2 sources: 0 - data ready, 1 - threshold event
>>
>>> +  - irq-map : irq sub-node defining interrupt map
>>> +             (all properties listed below are required):
>>
>> So, this node isn't required.
>>
>>> +      - #interrupt-cells : should be 1
>>
>>> +      - #address-cells : should be 0
>>> +      - #size-cells : should be 0
>>
>> There are no addressed entities in this node, so I don't see why those
>> two properties are needed.
>>
>>> +      - interrupt-map : table of entries consisting of three child elements:
>>> +         - unit_interrupt_specifier - 0 : data ready, 1 : threshold event
>>> +         - interrupt parent phandle
>>> +         - parent unit interrupt specifier consisiting of two elements:
>>> +             - index of the interrupt within the controller
>>> +             - flags : should be 0
>>
>> It's up to the binding for the node referenced by the phandle to define
>> how many cells need be present for "flags", and their meaning. This
>> binding shouldn't attempt to describe those. Equally, the concept of
>> interrupt-map should be defined elsewwere (e.g.
>> Documentation/devicetree/bindings/interrupt-controller/interrupts.txt);
>> it's a generic that shouldn't need duplication in each binding that uses
>> interrupts.
>>
>>> +Example:
>>> +
>>> +lps331ap@5d {
>>> +       compatible = "st,lps331ap";
>>> +       reg = <0x5d>;
>>> +       drdy-int-pin = /bits/ 8 <2>;
>>> +       interrupt-parent = <&irq_map>;
>>> +       interrupts = <0>, <1>;
>>> +
>>> +       irq_map: irq-map {
>>> +               #interrupt-cells = <1>;
>>> +               #address-cells = <0>;
>>> +               #size-cells = <0>;
>>> +               interrupt-map = <0 &gpf0 5 0>;
>>> +       };
>>> +};
>>>
>>> And here is how the driver uses this information:
>>>
>>>  - if interrupt-map is empty then the driver configures
>>>    itself to work without interrupt support
>>
>> The presence or lack of interrupt support should be driven by the
>> presence of the interrupts property. interrupt-map should only be used
>> (if present) to assist in the parsing of the interrupts property.
>>
>>>  - if only one interrupt source is available then the driver
>>>    configures the device to generate data ready interrupts on
>>>    the corresponding INTx pin (in this case the driver must know which
>>>    of the device pins is routed to the cpu -
>>>    st,data-ready-interrupt-pin property conveys this information)
>>>  - if both interrupt sources are available then the driver configures
>>>    the device to generate data ready interrupts on the interrupt pin
>>>    corresponding to the interrupt source with index 0 and event
>>>    interrupts to the interrupt source with index 1.
>>>
>>> This solution seems to be a little awkward so I'd like to ask
>>> if there is any neater way to handle presented requirements.
>>> The solution must facilitate passing information about two
>>> interrupt sources two the I2C driver. I have been unable to find
>>> similar solution in the kernel so far.
>>
>> Indeed. I think it would be better to work as follows:
>>
>> interrupts: contains one or two interrupt specifiers. The first entry
>> always defines the data ready interrupt. The second entry, if present,
>> defines the threshold event interrupt. This at least allows the
>> following combinations to be very simple expressed:
>>
>> * no interrrupts
>> * just data
>> * both data and threshold (assuming they're routed to the same parent)
>>
>> (you could swap the order if it's likely to be more common to have just
>> a threshold interrupt without any data interrupt).
>>
>> In order to allow the presence of a threshold interrupt but no data
>> interrupt, then I think you would need interrupt-map:
>>
>> lps331ap: lps331ap@5d {
>> 	compatible = "st,lps331ap";
>> 	reg = <0x5d>;
>> 	interrupt-parent = <&lps331ap>;
>> 	interrupts = <0>, <1>;
>> 	interrupt-map =	<0 0>, /* nowhere */
>> 			<1 &gpf0 6 0>;
>> };
> 
> The interrupt-names property exists for this purpose (describing
> interrupts which may or may not be present). Describing a nonexistent
> interrupt and mapping it nowhere feels like a hack to me when we can
> describe exactly what's present.

But the rules for interrupts basically precludes the useful use of
interrupt-names.

The interrupts property was introduced long before interrupt-names. As
such, the rule was always that entries in interrupts had to appear at a
specific index in the property, in other words, the property had to be
in a specific order, and there's no way of missing entries out.

The interrupt-names property was added much later and more as a
documentation for the order in *.dts than as the primary lookup key.
Even with an interrupt-names property present, the order of entries in
interrupts is still fixed.

So, using interrupt-names doesn't allow you to have optional entries in
interrupts, nor re-order the property. We really should not have added
interrupt-names, since it gives false impressions.

For newer bindings such as clocks/clock-names, clock-names is the
primary lookup key, so things can be optional.

We should document which properties are purely looked up by index, and
which properties have a useful *-names property associated with them as
the primary lookup key.

Re: passing two interrupts two an I2C driver

[Mark Rutland]

On Mon, Aug 19, 2013 at 05:09:34PM +0100, Stephen Warren wrote:
> On 08/19/2013 02:42 AM, Mark Rutland wrote:
> > On Fri, Aug 16, 2013 at 07:48:55PM +0100, Stephen Warren wrote:
> >> On 08/16/2013 08:47 AM, Jacek Anaszewski wrote:
> >>> Hi all,
> >>>
> >>> I'd like to consult the implementation of DT binding for the I2C device
> >>> that exposes two interrupt pins (INT1 and INT2). Both pins can be
> >>> configured to generate either data ready interrupts or event interrupts.
> >>> I want to implement DT binding that will handle also the situation
> >>> when only one of the interrupt sources is routed from the device
> >>> to the CPU.
> >>>
> >>> Below is my implementation using interrupt-map:
> >>
> >>> +  - interrupt-parent : phandle to the interrupt map subnode
> >>
> >> When using interrupt-parent to point at an interrupt map, I believe you
> >> usually just point at the current node; there's no need to a child node.
> >>
> >>> +  - interrupts : interrupt mapping for LPS331AP interrupt sources:
> >>> +                2 sources: 0 - data ready, 1 - threshold event
> >>
> >>> +  - irq-map : irq sub-node defining interrupt map
> >>> +             (all properties listed below are required):
> >>
> >> So, this node isn't required.
> >>
> >>> +      - #interrupt-cells : should be 1
> >>
> >>> +      - #address-cells : should be 0
> >>> +      - #size-cells : should be 0
> >>
> >> There are no addressed entities in this node, so I don't see why those
> >> two properties are needed.
> >>
> >>> +      - interrupt-map : table of entries consisting of three child elements:
> >>> +         - unit_interrupt_specifier - 0 : data ready, 1 : threshold event
> >>> +         - interrupt parent phandle
> >>> +         - parent unit interrupt specifier consisiting of two elements:
> >>> +             - index of the interrupt within the controller
> >>> +             - flags : should be 0
> >>
> >> It's up to the binding for the node referenced by the phandle to define
> >> how many cells need be present for "flags", and their meaning. This
> >> binding shouldn't attempt to describe those. Equally, the concept of
> >> interrupt-map should be defined elsewwere (e.g.
> >> Documentation/devicetree/bindings/interrupt-controller/interrupts.txt);
> >> it's a generic that shouldn't need duplication in each binding that uses
> >> interrupts.
> >>
> >>> +Example:
> >>> +
> >>> +lps331ap@5d {
> >>> +       compatible = "st,lps331ap";
> >>> +       reg = <0x5d>;
> >>> +       drdy-int-pin = /bits/ 8 <2>;
> >>> +       interrupt-parent = <&irq_map>;
> >>> +       interrupts = <0>, <1>;
> >>> +
> >>> +       irq_map: irq-map {
> >>> +               #interrupt-cells = <1>;
> >>> +               #address-cells = <0>;
> >>> +               #size-cells = <0>;
> >>> +               interrupt-map = <0 &gpf0 5 0>;
> >>> +       };
> >>> +};
> >>>
> >>> And here is how the driver uses this information:
> >>>
> >>>  - if interrupt-map is empty then the driver configures
> >>>    itself to work without interrupt support
> >>
> >> The presence or lack of interrupt support should be driven by the
> >> presence of the interrupts property. interrupt-map should only be used
> >> (if present) to assist in the parsing of the interrupts property.
> >>
> >>>  - if only one interrupt source is available then the driver
> >>>    configures the device to generate data ready interrupts on
> >>>    the corresponding INTx pin (in this case the driver must know which
> >>>    of the device pins is routed to the cpu -
> >>>    st,data-ready-interrupt-pin property conveys this information)
> >>>  - if both interrupt sources are available then the driver configures
> >>>    the device to generate data ready interrupts on the interrupt pin
> >>>    corresponding to the interrupt source with index 0 and event
> >>>    interrupts to the interrupt source with index 1.
> >>>
> >>> This solution seems to be a little awkward so I'd like to ask
> >>> if there is any neater way to handle presented requirements.
> >>> The solution must facilitate passing information about two
> >>> interrupt sources two the I2C driver. I have been unable to find
> >>> similar solution in the kernel so far.
> >>
> >> Indeed. I think it would be better to work as follows:
> >>
> >> interrupts: contains one or two interrupt specifiers. The first entry
> >> always defines the data ready interrupt. The second entry, if present,
> >> defines the threshold event interrupt. This at least allows the
> >> following combinations to be very simple expressed:
> >>
> >> * no interrrupts
> >> * just data
> >> * both data and threshold (assuming they're routed to the same parent)
> >>
> >> (you could swap the order if it's likely to be more common to have just
> >> a threshold interrupt without any data interrupt).
> >>
> >> In order to allow the presence of a threshold interrupt but no data
> >> interrupt, then I think you would need interrupt-map:
> >>
> >> lps331ap: lps331ap@5d {
> >> 	compatible = "st,lps331ap";
> >> 	reg = <0x5d>;
> >> 	interrupt-parent = <&lps331ap>;
> >> 	interrupts = <0>, <1>;
> >> 	interrupt-map =	<0 0>, /* nowhere */
> >> 			<1 &gpf0 6 0>;
> >> };
> > 
> > The interrupt-names property exists for this purpose (describing
> > interrupts which may or may not be present). Describing a nonexistent
> > interrupt and mapping it nowhere feels like a hack to me when we can
> > describe exactly what's present.
> 
> But the rules for interrupts basically precludes the useful use of
> interrupt-names.
> 
> The interrupts property was introduced long before interrupt-names. As
> such, the rule was always that entries in interrupts had to appear at a
> specific index in the property, in other words, the property had to be
> in a specific order, and there's no way of missing entries out.

Ok, so for bindings that existed before reg-names, and never mentioned
reg-names, indexed accesses to reg entries must always be used.

> 
> The interrupt-names property was added much later and more as a
> documentation for the order in *.dts than as the primary lookup key.
> Even with an interrupt-names property present, the order of entries in
> interrupts is still fixed.
> 
> So, using interrupt-names doesn't allow you to have optional entries in
> interrupts, nor re-order the property. We really should not have added
> interrupt-names, since it gives false impressions.

I'm not sure why that has to be true for new bindings. The new bindings
were never used before the existence of reg-names, so it doesn't seem so
bad for the new bindings to require the use of reg-names, or to define
some optional usage of reg-names.

> 
> For newer bindings such as clocks/clock-names, clock-names is the
> primary lookup key, so things can be optional.

I don't see why a new class of bindings is that different from new
instances of bindings in this regard.

> 
> We should document which properties are purely looked up by index, and
> which properties have a useful *-names property associated with them as
> the primary lookup key.
> 

Certainly, but I think this can be more fine-grained than "all clock
propeties may use clock-names semantically, all reg properties may not
use reg-names semantically".

Thanks,
Mark.

Re: passing two interrupts two an I2C driver

[Stephen Warren]

On 08/20/2013 02:44 AM, Mark Rutland wrote:
> On Mon, Aug 19, 2013 at 05:09:34PM +0100, Stephen Warren wrote:
>> On 08/19/2013 02:42 AM, Mark Rutland wrote:
>>> On Fri, Aug 16, 2013 at 07:48:55PM +0100, Stephen Warren wrote:
>>>> On 08/16/2013 08:47 AM, Jacek Anaszewski wrote:
>>>>> Hi all,
>>>>>
>>>>> I'd like to consult the implementation of DT binding for the I2C device
>>>>> that exposes two interrupt pins (INT1 and INT2). Both pins can be
>>>>> configured to generate either data ready interrupts or event interrupts.
>>>>> I want to implement DT binding that will handle also the situation
>>>>> when only one of the interrupt sources is routed from the device
>>>>> to the CPU.
>>>>>
>>>>> Below is my implementation using interrupt-map:
>>>>
>>>>> +  - interrupt-parent : phandle to the interrupt map subnode
>>>>
>>>> When using interrupt-parent to point at an interrupt map, I believe you
>>>> usually just point at the current node; there's no need to a child node.
>>>>
>>>>> +  - interrupts : interrupt mapping for LPS331AP interrupt sources:
>>>>> +                2 sources: 0 - data ready, 1 - threshold event
>>>>
>>>>> +  - irq-map : irq sub-node defining interrupt map
>>>>> +             (all properties listed below are required):
>>>>
>>>> So, this node isn't required.
>>>>
>>>>> +      - #interrupt-cells : should be 1
>>>>
>>>>> +      - #address-cells : should be 0
>>>>> +      - #size-cells : should be 0
>>>>
>>>> There are no addressed entities in this node, so I don't see why those
>>>> two properties are needed.
>>>>
>>>>> +      - interrupt-map : table of entries consisting of three child elements:
>>>>> +         - unit_interrupt_specifier - 0 : data ready, 1 : threshold event
>>>>> +         - interrupt parent phandle
>>>>> +         - parent unit interrupt specifier consisiting of two elements:
>>>>> +             - index of the interrupt within the controller
>>>>> +             - flags : should be 0
>>>>
>>>> It's up to the binding for the node referenced by the phandle to define
>>>> how many cells need be present for "flags", and their meaning. This
>>>> binding shouldn't attempt to describe those. Equally, the concept of
>>>> interrupt-map should be defined elsewwere (e.g.
>>>> Documentation/devicetree/bindings/interrupt-controller/interrupts.txt);
>>>> it's a generic that shouldn't need duplication in each binding that uses
>>>> interrupts.
>>>>
>>>>> +Example:
>>>>> +
>>>>> +lps331ap@5d {
>>>>> +       compatible = "st,lps331ap";
>>>>> +       reg = <0x5d>;
>>>>> +       drdy-int-pin = /bits/ 8 <2>;
>>>>> +       interrupt-parent = <&irq_map>;
>>>>> +       interrupts = <0>, <1>;
>>>>> +
>>>>> +       irq_map: irq-map {
>>>>> +               #interrupt-cells = <1>;
>>>>> +               #address-cells = <0>;
>>>>> +               #size-cells = <0>;
>>>>> +               interrupt-map = <0 &gpf0 5 0>;
>>>>> +       };
>>>>> +};
>>>>>
>>>>> And here is how the driver uses this information:
>>>>>
>>>>>  - if interrupt-map is empty then the driver configures
>>>>>    itself to work without interrupt support
>>>>
>>>> The presence or lack of interrupt support should be driven by the
>>>> presence of the interrupts property. interrupt-map should only be used
>>>> (if present) to assist in the parsing of the interrupts property.
>>>>
>>>>>  - if only one interrupt source is available then the driver
>>>>>    configures the device to generate data ready interrupts on
>>>>>    the corresponding INTx pin (in this case the driver must know which
>>>>>    of the device pins is routed to the cpu -
>>>>>    st,data-ready-interrupt-pin property conveys this information)
>>>>>  - if both interrupt sources are available then the driver configures
>>>>>    the device to generate data ready interrupts on the interrupt pin
>>>>>    corresponding to the interrupt source with index 0 and event
>>>>>    interrupts to the interrupt source with index 1.
>>>>>
>>>>> This solution seems to be a little awkward so I'd like to ask
>>>>> if there is any neater way to handle presented requirements.
>>>>> The solution must facilitate passing information about two
>>>>> interrupt sources two the I2C driver. I have been unable to find
>>>>> similar solution in the kernel so far.
>>>>
>>>> Indeed. I think it would be better to work as follows:
>>>>
>>>> interrupts: contains one or two interrupt specifiers. The first entry
>>>> always defines the data ready interrupt. The second entry, if present,
>>>> defines the threshold event interrupt. This at least allows the
>>>> following combinations to be very simple expressed:
>>>>
>>>> * no interrrupts
>>>> * just data
>>>> * both data and threshold (assuming they're routed to the same parent)
>>>>
>>>> (you could swap the order if it's likely to be more common to have just
>>>> a threshold interrupt without any data interrupt).
>>>>
>>>> In order to allow the presence of a threshold interrupt but no data
>>>> interrupt, then I think you would need interrupt-map:
>>>>
>>>> lps331ap: lps331ap@5d {
>>>> 	compatible = "st,lps331ap";
>>>> 	reg = <0x5d>;
>>>> 	interrupt-parent = <&lps331ap>;
>>>> 	interrupts = <0>, <1>;
>>>> 	interrupt-map =	<0 0>, /* nowhere */
>>>> 			<1 &gpf0 6 0>;
>>>> };
>>>
>>> The interrupt-names property exists for this purpose (describing
>>> interrupts which may or may not be present). Describing a nonexistent
>>> interrupt and mapping it nowhere feels like a hack to me when we can
>>> describe exactly what's present.
>>
>> But the rules for interrupts basically precludes the useful use of
>> interrupt-names.
>>
>> The interrupts property was introduced long before interrupt-names. As
>> such, the rule was always that entries in interrupts had to appear at a
>> specific index in the property, in other words, the property had to be
>> in a specific order, and there's no way of missing entries out.
> 
> Ok, so for bindings that existed before reg-names, and never mentioned
> reg-names, indexed accesses to reg entries must always be used.
> 
>> The interrupt-names property was added much later and more as a
>> documentation for the order in *.dts than as the primary lookup key.
>> Even with an interrupt-names property present, the order of entries in
>> interrupts is still fixed.
>>
>> So, using interrupt-names doesn't allow you to have optional entries in
>> interrupts, nor re-order the property. We really should not have added
>> interrupt-names, since it gives false impressions.
> 
> I'm not sure why that has to be true for new bindings. The new bindings
> were never used before the existence of reg-names, so it doesn't seem so
> bad for the new bindings to require the use of reg-names, or to define
> some optional usage of reg-names.

IIRC, the argument put forth was that OSs may assume a complete lack of
the reg-names property and only allow looking up reg values by index,
and hence making use of reg-names would prohibit those OSs from
supporting new bindings. Although that said, quite why reg-names was
then allowed as a property I don't know. IIRC, another argument was that
we didn't want to force people into the overhead of parsing via
reg-names when a simple index could be used instead, so reg had to
always have a defined order even if reg-names was allowed.

On the assumption that reg-names wasn't retro-fitted to any "old"
bindings that originally just documents reg and not reg-names, then I
suppose we could say:

If a binding defines reg-names, either reg-names or hard-coded indices
may be used to find entries in reg. Otherwise, only hard-coded indices
may be allowed.

But I'm worried that people will see reg-names and assume they can put
entries in arbitrary order...

Re: passing two interrupts two an I2C driver

[Mark Rutland]

On Tue, Aug 20, 2013 at 05:25:23PM +0100, Stephen Warren wrote:
> On 08/20/2013 02:44 AM, Mark Rutland wrote:
> > On Mon, Aug 19, 2013 at 05:09:34PM +0100, Stephen Warren wrote:
> >> On 08/19/2013 02:42 AM, Mark Rutland wrote:
> >>> On Fri, Aug 16, 2013 at 07:48:55PM +0100, Stephen Warren wrote:
> >>>> On 08/16/2013 08:47 AM, Jacek Anaszewski wrote:
> >>>>> Hi all,
> >>>>>
> >>>>> I'd like to consult the implementation of DT binding for the I2C device
> >>>>> that exposes two interrupt pins (INT1 and INT2). Both pins can be
> >>>>> configured to generate either data ready interrupts or event interrupts.
> >>>>> I want to implement DT binding that will handle also the situation
> >>>>> when only one of the interrupt sources is routed from the device
> >>>>> to the CPU.
> >>>>>
> >>>>> Below is my implementation using interrupt-map:
> >>>>
> >>>>> +  - interrupt-parent : phandle to the interrupt map subnode
> >>>>
> >>>> When using interrupt-parent to point at an interrupt map, I believe you
> >>>> usually just point at the current node; there's no need to a child node.
> >>>>
> >>>>> +  - interrupts : interrupt mapping for LPS331AP interrupt sources:
> >>>>> +                2 sources: 0 - data ready, 1 - threshold event
> >>>>
> >>>>> +  - irq-map : irq sub-node defining interrupt map
> >>>>> +             (all properties listed below are required):
> >>>>
> >>>> So, this node isn't required.
> >>>>
> >>>>> +      - #interrupt-cells : should be 1
> >>>>
> >>>>> +      - #address-cells : should be 0
> >>>>> +      - #size-cells : should be 0
> >>>>
> >>>> There are no addressed entities in this node, so I don't see why those
> >>>> two properties are needed.
> >>>>
> >>>>> +      - interrupt-map : table of entries consisting of three child elements:
> >>>>> +         - unit_interrupt_specifier - 0 : data ready, 1 : threshold event
> >>>>> +         - interrupt parent phandle
> >>>>> +         - parent unit interrupt specifier consisiting of two elements:
> >>>>> +             - index of the interrupt within the controller
> >>>>> +             - flags : should be 0
> >>>>
> >>>> It's up to the binding for the node referenced by the phandle to define
> >>>> how many cells need be present for "flags", and their meaning. This
> >>>> binding shouldn't attempt to describe those. Equally, the concept of
> >>>> interrupt-map should be defined elsewwere (e.g.
> >>>> Documentation/devicetree/bindings/interrupt-controller/interrupts.txt);
> >>>> it's a generic that shouldn't need duplication in each binding that uses
> >>>> interrupts.
> >>>>
> >>>>> +Example:
> >>>>> +
> >>>>> +lps331ap@5d {
> >>>>> +       compatible = "st,lps331ap";
> >>>>> +       reg = <0x5d>;
> >>>>> +       drdy-int-pin = /bits/ 8 <2>;
> >>>>> +       interrupt-parent = <&irq_map>;
> >>>>> +       interrupts = <0>, <1>;
> >>>>> +
> >>>>> +       irq_map: irq-map {
> >>>>> +               #interrupt-cells = <1>;
> >>>>> +               #address-cells = <0>;
> >>>>> +               #size-cells = <0>;
> >>>>> +               interrupt-map = <0 &gpf0 5 0>;
> >>>>> +       };
> >>>>> +};
> >>>>>
> >>>>> And here is how the driver uses this information:
> >>>>>
> >>>>>  - if interrupt-map is empty then the driver configures
> >>>>>    itself to work without interrupt support
> >>>>
> >>>> The presence or lack of interrupt support should be driven by the
> >>>> presence of the interrupts property. interrupt-map should only be used
> >>>> (if present) to assist in the parsing of the interrupts property.
> >>>>
> >>>>>  - if only one interrupt source is available then the driver
> >>>>>    configures the device to generate data ready interrupts on
> >>>>>    the corresponding INTx pin (in this case the driver must know which
> >>>>>    of the device pins is routed to the cpu -
> >>>>>    st,data-ready-interrupt-pin property conveys this information)
> >>>>>  - if both interrupt sources are available then the driver configures
> >>>>>    the device to generate data ready interrupts on the interrupt pin
> >>>>>    corresponding to the interrupt source with index 0 and event
> >>>>>    interrupts to the interrupt source with index 1.
> >>>>>
> >>>>> This solution seems to be a little awkward so I'd like to ask
> >>>>> if there is any neater way to handle presented requirements.
> >>>>> The solution must facilitate passing information about two
> >>>>> interrupt sources two the I2C driver. I have been unable to find
> >>>>> similar solution in the kernel so far.
> >>>>
> >>>> Indeed. I think it would be better to work as follows:
> >>>>
> >>>> interrupts: contains one or two interrupt specifiers. The first entry
> >>>> always defines the data ready interrupt. The second entry, if present,
> >>>> defines the threshold event interrupt. This at least allows the
> >>>> following combinations to be very simple expressed:
> >>>>
> >>>> * no interrrupts
> >>>> * just data
> >>>> * both data and threshold (assuming they're routed to the same parent)
> >>>>
> >>>> (you could swap the order if it's likely to be more common to have just
> >>>> a threshold interrupt without any data interrupt).
> >>>>
> >>>> In order to allow the presence of a threshold interrupt but no data
> >>>> interrupt, then I think you would need interrupt-map:
> >>>>
> >>>> lps331ap: lps331ap@5d {
> >>>> 	compatible = "st,lps331ap";
> >>>> 	reg = <0x5d>;
> >>>> 	interrupt-parent = <&lps331ap>;
> >>>> 	interrupts = <0>, <1>;
> >>>> 	interrupt-map =	<0 0>, /* nowhere */
> >>>> 			<1 &gpf0 6 0>;
> >>>> };
> >>>
> >>> The interrupt-names property exists for this purpose (describing
> >>> interrupts which may or may not be present). Describing a nonexistent
> >>> interrupt and mapping it nowhere feels like a hack to me when we can
> >>> describe exactly what's present.
> >>
> >> But the rules for interrupts basically precludes the useful use of
> >> interrupt-names.
> >>
> >> The interrupts property was introduced long before interrupt-names. As
> >> such, the rule was always that entries in interrupts had to appear at a
> >> specific index in the property, in other words, the property had to be
> >> in a specific order, and there's no way of missing entries out.
> > 
> > Ok, so for bindings that existed before reg-names, and never mentioned
> > reg-names, indexed accesses to reg entries must always be used.
> > 
> >> The interrupt-names property was added much later and more as a
> >> documentation for the order in *.dts than as the primary lookup key.
> >> Even with an interrupt-names property present, the order of entries in
> >> interrupts is still fixed.
> >>
> >> So, using interrupt-names doesn't allow you to have optional entries in
> >> interrupts, nor re-order the property. We really should not have added
> >> interrupt-names, since it gives false impressions.
> > 
> > I'm not sure why that has to be true for new bindings. The new bindings
> > were never used before the existence of reg-names, so it doesn't seem so
> > bad for the new bindings to require the use of reg-names, or to define
> > some optional usage of reg-names.
> 
> IIRC, the argument put forth was that OSs may assume a complete lack of
> the reg-names property and only allow looking up reg values by index,
> and hence making use of reg-names would prohibit those OSs from
> supporting new bindings. Although that said, quite why reg-names was
> then allowed as a property I don't know. IIRC, another argument was that
> we didn't want to force people into the overhead of parsing via
> reg-names when a simple index could be used instead, so reg had to
> always have a defined order even if reg-names was allowed.
> 
> On the assumption that reg-names wasn't retro-fitted to any "old"
> bindings that originally just documents reg and not reg-names, then I
> suppose we could say:
> 
> If a binding defines reg-names, either reg-names or hard-coded indices
> may be used to find entries in reg. Otherwise, only hard-coded indices
> may be allowed.
> 
> But I'm worried that people will see reg-names and assume they can put
> entries in arbitrary order...

We should fix the binding documents to make clear which bindings require
their reg properties at fixed indexes, and those which allow for
arbitrarily ordered named reg entries. Named reg entries are really
useful for blocks with optional components, and given we have drivers
using them, they're already mandatory for some bindings.

Thanks,
Mark.

Re: passing two interrupts two an I2C driver

[Rob Herring]

On 08/21/2013 03:54 AM, Mark Rutland wrote:
> On Tue, Aug 20, 2013 at 05:25:23PM +0100, Stephen Warren wrote:
>> On 08/20/2013 02:44 AM, Mark Rutland wrote:
>>> On Mon, Aug 19, 2013 at 05:09:34PM +0100, Stephen Warren wrote:
>>>> On 08/19/2013 02:42 AM, Mark Rutland wrote:
>>>>> On Fri, Aug 16, 2013 at 07:48:55PM +0100, Stephen Warren wrote:
>>>>>> On 08/16/2013 08:47 AM, Jacek Anaszewski wrote:
>>>>>>> Hi all,
>>>>>>>
>>>>>>> I'd like to consult the implementation of DT binding for the I2C device
>>>>>>> that exposes two interrupt pins (INT1 and INT2). Both pins can be
>>>>>>> configured to generate either data ready interrupts or event interrupts.
>>>>>>> I want to implement DT binding that will handle also the situation
>>>>>>> when only one of the interrupt sources is routed from the device
>>>>>>> to the CPU.
>>>>>>>
>>>>>>> Below is my implementation using interrupt-map:
>>>>>>
>>>>>>> +  - interrupt-parent : phandle to the interrupt map subnode
>>>>>>
>>>>>> When using interrupt-parent to point at an interrupt map, I believe you
>>>>>> usually just point at the current node; there's no need to a child node.
>>>>>>
>>>>>>> +  - interrupts : interrupt mapping for LPS331AP interrupt sources:
>>>>>>> +                2 sources: 0 - data ready, 1 - threshold event
>>>>>>
>>>>>>> +  - irq-map : irq sub-node defining interrupt map
>>>>>>> +             (all properties listed below are required):
>>>>>>
>>>>>> So, this node isn't required.
>>>>>>
>>>>>>> +      - #interrupt-cells : should be 1
>>>>>>
>>>>>>> +      - #address-cells : should be 0
>>>>>>> +      - #size-cells : should be 0
>>>>>>
>>>>>> There are no addressed entities in this node, so I don't see why those
>>>>>> two properties are needed.
>>>>>>
>>>>>>> +      - interrupt-map : table of entries consisting of three child elements:
>>>>>>> +         - unit_interrupt_specifier - 0 : data ready, 1 : threshold event
>>>>>>> +         - interrupt parent phandle
>>>>>>> +         - parent unit interrupt specifier consisiting of two elements:
>>>>>>> +             - index of the interrupt within the controller
>>>>>>> +             - flags : should be 0
>>>>>>
>>>>>> It's up to the binding for the node referenced by the phandle to define
>>>>>> how many cells need be present for "flags", and their meaning. This
>>>>>> binding shouldn't attempt to describe those. Equally, the concept of
>>>>>> interrupt-map should be defined elsewwere (e.g.
>>>>>> Documentation/devicetree/bindings/interrupt-controller/interrupts.txt);
>>>>>> it's a generic that shouldn't need duplication in each binding that uses
>>>>>> interrupts.
>>>>>>
>>>>>>> +Example:
>>>>>>> +
>>>>>>> +lps331ap@5d {
>>>>>>> +       compatible = "st,lps331ap";
>>>>>>> +       reg = <0x5d>;
>>>>>>> +       drdy-int-pin = /bits/ 8 <2>;
>>>>>>> +       interrupt-parent = <&irq_map>;
>>>>>>> +       interrupts = <0>, <1>;
>>>>>>> +
>>>>>>> +       irq_map: irq-map {
>>>>>>> +               #interrupt-cells = <1>;
>>>>>>> +               #address-cells = <0>;
>>>>>>> +               #size-cells = <0>;
>>>>>>> +               interrupt-map = <0 &gpf0 5 0>;
>>>>>>> +       };
>>>>>>> +};
>>>>>>>
>>>>>>> And here is how the driver uses this information:
>>>>>>>
>>>>>>>  - if interrupt-map is empty then the driver configures
>>>>>>>    itself to work without interrupt support
>>>>>>
>>>>>> The presence or lack of interrupt support should be driven by the
>>>>>> presence of the interrupts property. interrupt-map should only be used
>>>>>> (if present) to assist in the parsing of the interrupts property.
>>>>>>
>>>>>>>  - if only one interrupt source is available then the driver
>>>>>>>    configures the device to generate data ready interrupts on
>>>>>>>    the corresponding INTx pin (in this case the driver must know which
>>>>>>>    of the device pins is routed to the cpu -
>>>>>>>    st,data-ready-interrupt-pin property conveys this information)
>>>>>>>  - if both interrupt sources are available then the driver configures
>>>>>>>    the device to generate data ready interrupts on the interrupt pin
>>>>>>>    corresponding to the interrupt source with index 0 and event
>>>>>>>    interrupts to the interrupt source with index 1.
>>>>>>>
>>>>>>> This solution seems to be a little awkward so I'd like to ask
>>>>>>> if there is any neater way to handle presented requirements.
>>>>>>> The solution must facilitate passing information about two
>>>>>>> interrupt sources two the I2C driver. I have been unable to find
>>>>>>> similar solution in the kernel so far.
>>>>>>
>>>>>> Indeed. I think it would be better to work as follows:
>>>>>>
>>>>>> interrupts: contains one or two interrupt specifiers. The first entry
>>>>>> always defines the data ready interrupt. The second entry, if present,
>>>>>> defines the threshold event interrupt. This at least allows the
>>>>>> following combinations to be very simple expressed:
>>>>>>
>>>>>> * no interrrupts
>>>>>> * just data
>>>>>> * both data and threshold (assuming they're routed to the same parent)
>>>>>>
>>>>>> (you could swap the order if it's likely to be more common to have just
>>>>>> a threshold interrupt without any data interrupt).
>>>>>>
>>>>>> In order to allow the presence of a threshold interrupt but no data
>>>>>> interrupt, then I think you would need interrupt-map:
>>>>>>
>>>>>> lps331ap: lps331ap@5d {
>>>>>> 	compatible = "st,lps331ap";
>>>>>> 	reg = <0x5d>;
>>>>>> 	interrupt-parent = <&lps331ap>;
>>>>>> 	interrupts = <0>, <1>;
>>>>>> 	interrupt-map =	<0 0>, /* nowhere */
>>>>>> 			<1 &gpf0 6 0>;
>>>>>> };
>>>>>
>>>>> The interrupt-names property exists for this purpose (describing
>>>>> interrupts which may or may not be present). Describing a nonexistent
>>>>> interrupt and mapping it nowhere feels like a hack to me when we can
>>>>> describe exactly what's present.
>>>>
>>>> But the rules for interrupts basically precludes the useful use of
>>>> interrupt-names.
>>>>
>>>> The interrupts property was introduced long before interrupt-names. As
>>>> such, the rule was always that entries in interrupts had to appear at a
>>>> specific index in the property, in other words, the property had to be
>>>> in a specific order, and there's no way of missing entries out.
>>>
>>> Ok, so for bindings that existed before reg-names, and never mentioned
>>> reg-names, indexed accesses to reg entries must always be used.
>>>
>>>> The interrupt-names property was added much later and more as a
>>>> documentation for the order in *.dts than as the primary lookup key.
>>>> Even with an interrupt-names property present, the order of entries in
>>>> interrupts is still fixed.
>>>>
>>>> So, using interrupt-names doesn't allow you to have optional entries in
>>>> interrupts, nor re-order the property. We really should not have added
>>>> interrupt-names, since it gives false impressions.
>>>
>>> I'm not sure why that has to be true for new bindings. The new bindings
>>> were never used before the existence of reg-names, so it doesn't seem so
>>> bad for the new bindings to require the use of reg-names, or to define
>>> some optional usage of reg-names.
>>
>> IIRC, the argument put forth was that OSs may assume a complete lack of
>> the reg-names property and only allow looking up reg values by index,
>> and hence making use of reg-names would prohibit those OSs from
>> supporting new bindings. Although that said, quite why reg-names was
>> then allowed as a property I don't know. IIRC, another argument was that
>> we didn't want to force people into the overhead of parsing via
>> reg-names when a simple index could be used instead, so reg had to
>> always have a defined order even if reg-names was allowed.
>>
>> On the assumption that reg-names wasn't retro-fitted to any "old"
>> bindings that originally just documents reg and not reg-names, then I
>> suppose we could say:
>>
>> If a binding defines reg-names, either reg-names or hard-coded indices
>> may be used to find entries in reg. Otherwise, only hard-coded indices
>> may be allowed.
>>
>> But I'm worried that people will see reg-names and assume they can put
>> entries in arbitrary order...
> 
> We should fix the binding documents to make clear which bindings require
> their reg properties at fixed indexes, and those which allow for
> arbitrarily ordered named reg entries. Named reg entries are really
> useful for blocks with optional components, and given we have drivers
> using them, they're already mandatory for some bindings.

No, we should fix the bindings that have arbitrarily ordered reg
properties. There is no obvious reg examples of this that I see from a
quick scan of dts files.

There is this questionable use of "empty" for interrupt-names:

arch/arm/boot/dts/imx23.dtsi:
	interrupts = <0 14 20 0
			13 13 13 13>;
	interrupt-names = "empty", "ssp0", "ssp1", "empty",
			"gpmi0", "gpmi1", "gpmi2", "gpmi3";


Rob

Re: passing two interrupts two an I2C driver

[Stephen Warren]

On 08/22/2013 06:45 AM, Rob Herring wrote:
> On 08/21/2013 03:54 AM, Mark Rutland wrote:
...
>> We should fix the binding documents to make clear which bindings require
>> their reg properties at fixed indexes, and those which allow for
>> arbitrarily ordered named reg entries. Named reg entries are really
>> useful for blocks with optional components, and given we have drivers
>> using them, they're already mandatory for some bindings.
> 
> No, we should fix the bindings that have arbitrarily ordered reg
> properties. There is no obvious reg examples of this that I see from a
> quick scan of dts files.
> 
> There is this questionable use of "empty" for interrupt-names:
> 
> arch/arm/boot/dts/imx23.dtsi:
> 	interrupts = <0 14 20 0
> 			13 13 13 13>;
> 	interrupt-names = "empty", "ssp0", "ssp1", "empty",
> 			"gpmi0", "gpmi1", "gpmi2", "gpmi3";
> 

Well, there's been an assertion that bindings that define an ordered
interrupts property shouldn't have a interrupt-names property, and
bindings that define interrupt-names entries explicitly don't require a
specific order of entries in the interrupts property.

I assume from your statement, you don't agree, and think that interrupts
must always be in a specific order, even if interrupt-names exists?

If so, that'd make interrupt-names at least a bit useless and at worst
horribly misleading, since people will assume it defines the order and
that the order can be arbitrary.

Re: passing two interrupts two an I2C driver

[Jacek Anaszewski]

On 08/19/2013 10:42 AM, Mark Rutland wrote:
> On Fri, Aug 16, 2013 at 07:48:55PM +0100, Stephen Warren wrote:
>> On 08/16/2013 08:47 AM, Jacek Anaszewski wrote:
>>> Hi all,
>>>
>>> I'd like to consult the implementation of DT binding for the I2C device
>>> that exposes two interrupt pins (INT1 and INT2). Both pins can be
>>> configured to generate either data ready interrupts or event interrupts.
>>> I want to implement DT binding that will handle also the situation
>>> when only one of the interrupt sources is routed from the device
>>> to the CPU.
>>>
>>> Below is my implementation using interrupt-map:
>>
>>> +  - interrupt-parent : phandle to the interrupt map subnode
>>
>> When using interrupt-parent to point at an interrupt map, I believe you
>> usually just point at the current node; there's no need to a child node.
>>
>>> +  - interrupts : interrupt mapping for LPS331AP interrupt sources:
>>> +                2 sources: 0 - data ready, 1 - threshold event
>>
>>> +  - irq-map : irq sub-node defining interrupt map
>>> +             (all properties listed below are required):
>>
>> So, this node isn't required.
>>
>>> +      - #interrupt-cells : should be 1
>>
>>> +      - #address-cells : should be 0
>>> +      - #size-cells : should be 0
>>
>> There are no addressed entities in this node, so I don't see why those
>> two properties are needed.
>>
>>> +      - interrupt-map : table of entries consisting of three child elements:
>>> +         - unit_interrupt_specifier - 0 : data ready, 1 : threshold event
>>> +         - interrupt parent phandle
>>> +         - parent unit interrupt specifier consisiting of two elements:
>>> +             - index of the interrupt within the controller
>>> +             - flags : should be 0
>>
>> It's up to the binding for the node referenced by the phandle to define
>> how many cells need be present for "flags", and their meaning. This
>> binding shouldn't attempt to describe those. Equally, the concept of
>> interrupt-map should be defined elsewwere (e.g.
>> Documentation/devicetree/bindings/interrupt-controller/interrupts.txt);
>> it's a generic that shouldn't need duplication in each binding that uses
>> interrupts.
>>
>>> +Example:
>>> +
>>> +lps331ap@5d {
>>> +       compatible = "st,lps331ap";
>>> +       reg =<0x5d>;
>>> +       drdy-int-pin = /bits/ 8<2>;
>>> +       interrupt-parent =<&irq_map>;
>>> +       interrupts =<0>,<1>;
>>> +
>>> +       irq_map: irq-map {
>>> +               #interrupt-cells =<1>;
>>> +               #address-cells =<0>;
>>> +               #size-cells =<0>;
>>> +               interrupt-map =<0&gpf0 5 0>;
>>> +       };
>>> +};
>>>
>>> And here is how the driver uses this information:
>>>
>>>   - if interrupt-map is empty then the driver configures
>>>     itself to work without interrupt support
>>
>> The presence or lack of interrupt support should be driven by the
>> presence of the interrupts property. interrupt-map should only be used
>> (if present) to assist in the parsing of the interrupts property.
>>
>>>   - if only one interrupt source is available then the driver
>>>     configures the device to generate data ready interrupts on
>>>     the corresponding INTx pin (in this case the driver must know which
>>>     of the device pins is routed to the cpu -
>>>     st,data-ready-interrupt-pin property conveys this information)
>>>   - if both interrupt sources are available then the driver configures
>>>     the device to generate data ready interrupts on the interrupt pin
>>>     corresponding to the interrupt source with index 0 and event
>>>     interrupts to the interrupt source with index 1.
>>>
>>> This solution seems to be a little awkward so I'd like to ask
>>> if there is any neater way to handle presented requirements.
>>> The solution must facilitate passing information about two
>>> interrupt sources two the I2C driver. I have been unable to find
>>> similar solution in the kernel so far.
>>
>> Indeed. I think it would be better to work as follows:
>>
>> interrupts: contains one or two interrupt specifiers. The first entry
>> always defines the data ready interrupt. The second entry, if present,
>> defines the threshold event interrupt. This at least allows the
>> following combinations to be very simple expressed:
>>
>> * no interrrupts
>> * just data
>> * both data and threshold (assuming they're routed to the same parent)
>>
>> (you could swap the order if it's likely to be more common to have just
>> a threshold interrupt without any data interrupt).
>>
>> In order to allow the presence of a threshold interrupt but no data
>> interrupt, then I think you would need interrupt-map:
>>
>> lps331ap: lps331ap@5d {
>> 	compatible = "st,lps331ap";
>> 	reg =<0x5d>;
>> 	interrupt-parent =<&lps331ap>;
>> 	interrupts =<0>,<1>;
>> 	interrupt-map =	<0 0>, /* nowhere */
>> 			<1&gpf0 6 0>;
>> };
>
> The interrupt-names property exists for this purpose (describing
> interrupts which may or may not be present). Describing a nonexistent
> interrupt and mapping it nowhere feels like a hack to me when we can
> describe exactly what's present.
>
> Thanks,
> Mark.

 From what I've figured out in order to obtain the interrupt id
by name the function

platform_get_irq_by_name(struct platform_device *dev, const char *name)

has to be exploited. Unfortunately required platform_device structure
is not available in the struct i2c_client that is passed to the
probe function of an i2c driver. Is there some different way to parse
the interrupt-names property from the I2C driver?

Thanks,
Jacek

Re: passing two interrupts two an I2C driver

[Pawel Moll]

On Wed, 2013-08-21 at 12:53 +0100, Jacek Anaszewski wrote:
>  From what I've figured out in order to obtain the interrupt id
> by name the function
> 
> platform_get_irq_by_name(struct platform_device *dev, const char *name)
> 
> has to be exploited. Unfortunately required platform_device structure
> is not available in the struct i2c_client

Well, platform devices belong to platform bus, nothing to do with I2C.
Platform devices are associated with an array of named resources and the
code creating such devices from the DT nodes (of_device_alloc()) do the
necessary magic.

>  that is passed to the
> probe function of an i2c driver. Is there some different way to parse
> the interrupt-names property from the I2C driver?

Interesting. Very interesting indeed. It seems (at least at the first
sight) that I2C framework assumes that only one interrupt can be
generated by a I2C device:

struct i2c_client {                                                                         
[...]
        int irq;                        /* irq issued by device         */                  
[...]
};                                                                               

So let me ask such question... If Device Tree didn't exist, how would
you make drive such device? I guess it would require some custom code,
as struct i2c_board_info has single int irq as well... If this is the
case, you could parse the irq properties in a similar way as done in
of_irq_to_resource() in drivers/of/irq.c. Doesn't like the optimal
solution, though...

I've copied Wolfram and the I2C mailing list in hope of them shedding
some light at the matter.

Pawel

Re: passing two interrupts two an I2C driver

[Pawel Moll]

[resending, this time really copying Wolfram and linux-i2c...]

On Wed, 2013-08-21 at 12:53 +0100, Jacek Anaszewski wrote:
>  From what I've figured out in order to obtain the interrupt id
> by name the function
> 
> platform_get_irq_by_name(struct platform_device *dev, const char *name)
> 
> has to be exploited. Unfortunately required platform_device structure
> is not available in the struct i2c_client

Well, platform devices belong to platform bus, nothing to do with I2C.
Platform devices are associated with an array of named resources and the
code creating such devices from the DT nodes (of_device_alloc()) do the
necessary magic.

>  that is passed to the
> probe function of an i2c driver. Is there some different way to parse
> the interrupt-names property from the I2C driver?

Interesting. Very interesting indeed. It seems (at least at the first
sight) that I2C framework assumes that only one interrupt can be
generated by a I2C device:

struct i2c_client {                                                                         
[...]
        int irq;                        /* irq issued by device         */                  
[...]
};                                                                               

So let me ask such question... If Device Tree didn't exist, how would
you make drive such device? I guess it would require some custom code,
as struct i2c_board_info has single int irq as well... If this is the
case, you could parse the irq properties in a similar way as done in
of_irq_to_resource() in drivers/of/irq.c. Doesn't like the optimal
solution, though...

I've copied Wolfram and the I2C mailing list in hope of them shedding
some light at the matter.

Pawel

Re: passing two interrupts two an I2C driver

[Pawel Moll]

[resending, this time really copying Wolfram and linux-i2c...]

On Wed, 2013-08-21 at 12:53 +0100, Jacek Anaszewski wrote:
>  From what I've figured out in order to obtain the interrupt id
> by name the function
> 
> platform_get_irq_by_name(struct platform_device *dev, const char *name)
> 
> has to be exploited. Unfortunately required platform_device structure
> is not available in the struct i2c_client

Well, platform devices belong to platform bus, nothing to do with I2C.
Platform devices are associated with an array of named resources and the
code creating such devices from the DT nodes (of_device_alloc()) do the
necessary magic.

>  that is passed to the
> probe function of an i2c driver. Is there some different way to parse
> the interrupt-names property from the I2C driver?

Interesting. Very interesting indeed. It seems (at least at the first
sight) that I2C framework assumes that only one interrupt can be
generated by a I2C device:

struct i2c_client {                                                                         
[...]
        int irq;                        /* irq issued by device         */                  
[...]
};                                                                               

So let me ask such question... If Device Tree didn't exist, how would
you make drive such device? I guess it would require some custom code,
as struct i2c_board_info has single int irq as well... If this is the
case, you could parse the irq properties in a similar way as done in
of_irq_to_resource() in drivers/of/irq.c. Doesn't like the optimal
solution, though...

I've copied Wolfram and the I2C mailing list in hope of them shedding
some light at the matter.

Pawel

Re: passing two interrupts two an I2C driver

[Pawel Moll]

[resending, this time really copying Wolfram and linux-i2c...]

On Wed, 2013-08-21 at 12:53 +0100, Jacek Anaszewski wrote:
>  From what I've figured out in order to obtain the interrupt id
> by name the function
>=20
> platform_get_irq_by_name(struct platform_device *dev, const char *name)
>=20
> has to be exploited. Unfortunately required platform_device structure
> is not available in the struct i2c_client

Well, platform devices belong to platform bus, nothing to do with I2C.
Platform devices are associated with an array of named resources and the
code creating such devices from the DT nodes (of_device_alloc()) do the
necessary magic.

>  that is passed to the
> probe function of an i2c driver. Is there some different way to parse
> the interrupt-names property from the I2C driver?

Interesting. Very interesting indeed. It seems (at least at the first
sight) that I2C framework assumes that only one interrupt can be
generated by a I2C device:

struct i2c_client {                                                        =
                =20
[...]
        int irq;                        /* irq issued by device         */ =
                =20
[...]
};                                                                         =
     =20

So let me ask such question... If Device Tree didn't exist, how would
you make drive such device? I guess it would require some custom code,
as struct i2c_board_info has single int irq as well... If this is the
case, you could parse the irq properties in a similar way as done in
of_irq_to_resource() in drivers/of/irq.c. Doesn't like the optimal
solution, though...

I've copied Wolfram and the I2C mailing list in hope of them shedding
some light at the matter.

Pawel

Re: passing two interrupts two an I2C driver

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 309 bytes --]

On Wed, Aug 21, 2013 at 01:37:04PM +0100, Pawel Moll wrote:

> So let me ask such question... If Device Tree didn't exist, how would
> you make drive such device? I guess it would require some custom code,

It's always done using platform data, same for SPI - if we update one we
should probably update both.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: passing two interrupts two an I2C driver

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 309 bytes --]

On Wed, Aug 21, 2013 at 01:37:04PM +0100, Pawel Moll wrote:

> So let me ask such question... If Device Tree didn't exist, how would
> you make drive such device? I guess it would require some custom code,

It's always done using platform data, same for SPI - if we update one we
should probably update both.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: passing two interrupts two an I2C driver

[Pawel Moll]

On Wed, 2013-08-21 at 18:54 +0100, Mark Brown wrote:
> On Wed, Aug 21, 2013 at 01:37:04PM +0100, Pawel Moll wrote:
> 
> > So let me ask such question... If Device Tree didn't exist, how would
> > you make drive such device? I guess it would require some custom code,
> 
> It's always done using platform data, same for SPI - if we update one we
> should probably update both.

If the platform data used to carry the (custom) irq data, the DT-powered
driver could interrogate the DT on is own, couldn't it? Of course there
should be some helper available, maybe something of that sort? (warning,
untested)

8<---------------------
diff --git a/drivers/of/irq.c b/drivers/of/irq.c
index 1264923..d2a02dd 100644
--- a/drivers/of/irq.c
+++ b/drivers/of/irq.c
@@ -46,6 +46,15 @@ unsigned int irq_of_parse_and_map(struct device_node *dev, int index)
 }
 EXPORT_SYMBOL_GPL(irq_of_parse_and_map);
 
+unsigned int irq_of_parse_and_map_by_name(struct device_node *dev,
+		const char *name)
+{
+	int index = of_property_match_string(dev, "interrupt-names", name);
+
+	return index < 0 ? 0 : irq_of_parse_and_map(dev, index);
+}
+EXPORT_SYMBOL_GPL(irq_of_parse_and_map_by_name);
+
 /**
  * of_irq_find_parent - Given a device node, find its interrupt parent node
  * @child: pointer to device node
8<---------------------

Pawel

Re: passing two interrupts two an I2C driver

[Pawel Moll]

On Wed, 2013-08-21 at 18:54 +0100, Mark Brown wrote:
> On Wed, Aug 21, 2013 at 01:37:04PM +0100, Pawel Moll wrote:
> 
> > So let me ask such question... If Device Tree didn't exist, how would
> > you make drive such device? I guess it would require some custom code,
> 
> It's always done using platform data, same for SPI - if we update one we
> should probably update both.

If the platform data used to carry the (custom) irq data, the DT-powered
driver could interrogate the DT on is own, couldn't it? Of course there
should be some helper available, maybe something of that sort? (warning,
untested)

8<---------------------
diff --git a/drivers/of/irq.c b/drivers/of/irq.c
index 1264923..d2a02dd 100644
--- a/drivers/of/irq.c
+++ b/drivers/of/irq.c
@@ -46,6 +46,15 @@ unsigned int irq_of_parse_and_map(struct device_node *dev, int index)
 }
 EXPORT_SYMBOL_GPL(irq_of_parse_and_map);
 
+unsigned int irq_of_parse_and_map_by_name(struct device_node *dev,
+		const char *name)
+{
+	int index = of_property_match_string(dev, "interrupt-names", name);
+
+	return index < 0 ? 0 : irq_of_parse_and_map(dev, index);
+}
+EXPORT_SYMBOL_GPL(irq_of_parse_and_map_by_name);
+
 /**
  * of_irq_find_parent - Given a device node, find its interrupt parent node
  * @child: pointer to device node
8<---------------------

Pawel

Re: passing two interrupts two an I2C driver

[Pawel Moll]

On Wed, 2013-08-21 at 18:54 +0100, Mark Brown wrote:
> On Wed, Aug 21, 2013 at 01:37:04PM +0100, Pawel Moll wrote:
>=20
> > So let me ask such question... If Device Tree didn't exist, how would
> > you make drive such device? I guess it would require some custom code,
>=20
> It's always done using platform data, same for SPI - if we update one we
> should probably update both.

If the platform data used to carry the (custom) irq data, the DT-powered
driver could interrogate the DT on is own, couldn't it? Of course there
should be some helper available, maybe something of that sort? (warning,
untested)

8<---------------------
diff --git a/drivers/of/irq.c b/drivers/of/irq.c
index 1264923..d2a02dd 100644
--- a/drivers/of/irq.c
+++ b/drivers/of/irq.c
@@ -46,6 +46,15 @@ unsigned int irq_of_parse_and_map(struct device_node *de=
v, int index)
 }
 EXPORT_SYMBOL_GPL(irq_of_parse_and_map);
=20
+unsigned int irq_of_parse_and_map_by_name(struct device_node *dev,
+=09=09const char *name)
+{
+=09int index =3D of_property_match_string(dev, "interrupt-names", name);
+
+=09return index < 0 ? 0 : irq_of_parse_and_map(dev, index);
+}
+EXPORT_SYMBOL_GPL(irq_of_parse_and_map_by_name);
+
 /**
  * of_irq_find_parent - Given a device node, find its interrupt parent nod=
e
  * @child: pointer to device node
8<---------------------

Pawel

Re: passing two interrupts two an I2C driver

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 540 bytes --]

On Thu, Aug 22, 2013 at 10:23:28AM +0100, Pawel Moll wrote:

> If the platform data used to carry the (custom) irq data, the DT-powered
> driver could interrogate the DT on is own, couldn't it? Of course there
> should be some helper available, maybe something of that sort? (warning,
> untested)

Yes, that's probably the most straightforward thing - we'd need to
either have the bindings specify which interrupt must be first for
reading i2c->irq or just have the drivers always do a name based lookup
if there's more than one interrupt.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: passing two interrupts two an I2C driver

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 540 bytes --]

On Thu, Aug 22, 2013 at 10:23:28AM +0100, Pawel Moll wrote:

> If the platform data used to carry the (custom) irq data, the DT-powered
> driver could interrogate the DT on is own, couldn't it? Of course there
> should be some helper available, maybe something of that sort? (warning,
> untested)

Yes, that's probably the most straightforward thing - we'd need to
either have the bindings specify which interrupt must be first for
reading i2c->irq or just have the drivers always do a name based lookup
if there's more than one interrupt.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: passing two interrupts two an I2C driver

[Pawel Moll]

On Thu, 2013-08-22 at 12:26 +0100, Mark Brown wrote:
> On Thu, Aug 22, 2013 at 10:23:28AM +0100, Pawel Moll wrote:
> 
> > If the platform data used to carry the (custom) irq data, the DT-powered
> > driver could interrogate the DT on is own, couldn't it? Of course there
> > should be some helper available, maybe something of that sort? (warning,
> > untested)
> 
> Yes, that's probably the most straightforward thing - we'd need to
> either have the bindings specify which interrupt must be first for
> reading i2c->irq or just have the drivers always do a name based lookup
> if there's more than one interrupt.

... or make sure that of_i2c_register_devices() does *not* set i2c->irq
(or rather: set it to 0) when there is more than one interrupt in the
tree...

Paweł

Re: passing two interrupts two an I2C driver

[Pawel Moll]

On Thu, 2013-08-22 at 12:26 +0100, Mark Brown wrote:
> On Thu, Aug 22, 2013 at 10:23:28AM +0100, Pawel Moll wrote:
> 
> > If the platform data used to carry the (custom) irq data, the DT-powered
> > driver could interrogate the DT on is own, couldn't it? Of course there
> > should be some helper available, maybe something of that sort? (warning,
> > untested)
> 
> Yes, that's probably the most straightforward thing - we'd need to
> either have the bindings specify which interrupt must be first for
> reading i2c->irq or just have the drivers always do a name based lookup
> if there's more than one interrupt.

... or make sure that of_i2c_register_devices() does *not* set i2c->irq
(or rather: set it to 0) when there is more than one interrupt in the
tree...

Paweł

Re: passing two interrupts two an I2C driver

[Pawel Moll]

On Thu, 2013-08-22 at 12:26 +0100, Mark Brown wrote:
> On Thu, Aug 22, 2013 at 10:23:28AM +0100, Pawel Moll wrote:
>=20
> > If the platform data used to carry the (custom) irq data, the DT-powere=
d
> > driver could interrogate the DT on is own, couldn't it? Of course there
> > should be some helper available, maybe something of that sort? (warning=
,
> > untested)
>=20
> Yes, that's probably the most straightforward thing - we'd need to
> either have the bindings specify which interrupt must be first for
> reading i2c->irq or just have the drivers always do a name based lookup
> if there's more than one interrupt.

... or make sure that of_i2c_register_devices() does *not* set i2c->irq
(or rather: set it to 0) when there is more than one interrupt in the
tree...

Pawe=C5=82

Re: passing two interrupts two an I2C driver

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 643 bytes --]

On Thu, Aug 22, 2013 at 12:44:04PM +0100, Pawel Moll wrote:
> On Thu, 2013-08-22 at 12:26 +0100, Mark Brown wrote:

> > Yes, that's probably the most straightforward thing - we'd need to
> > either have the bindings specify which interrupt must be first for
> > reading i2c->irq or just have the drivers always do a name based lookup
> > if there's more than one interrupt.

> ... or make sure that of_i2c_register_devices() does *not* set i2c->irq
> (or rather: set it to 0) when there is more than one interrupt in the
> tree...

Yes, that'd be a really good idea if the drivers are always getting the
interrupts by name, provides a backup.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: passing two interrupts two an I2C driver

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 643 bytes --]

On Thu, Aug 22, 2013 at 12:44:04PM +0100, Pawel Moll wrote:
> On Thu, 2013-08-22 at 12:26 +0100, Mark Brown wrote:

> > Yes, that's probably the most straightforward thing - we'd need to
> > either have the bindings specify which interrupt must be first for
> > reading i2c->irq or just have the drivers always do a name based lookup
> > if there's more than one interrupt.

> ... or make sure that of_i2c_register_devices() does *not* set i2c->irq
> (or rather: set it to 0) when there is more than one interrupt in the
> tree...

Yes, that'd be a really good idea if the drivers are always getting the
interrupts by name, provides a backup.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]