Re: passing two interrupts two an I2C driver

[Mark Rutland <mark.rutland@arm.com>]

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.