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.
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
