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.
--
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
