Re: [PATCH RFC v2 1/3] drivers: irqchip: pdc: Add PDC interrupt controller for QCOM SoCs

[Rasmus Villemoes]

On 2018-02-02 15:58, Marc Zyngier wrote:
> Hi Lina,
> 
> On 02/02/18 14:21, Lina Iyer wrote:
>> From : Archana Sathyakumar <asathyak@xxxxxxxxxxxxxx>
>>
>> +
>> +static int qcom_pdc_translate(struct irq_domain *d,
>> +	struct irq_fwspec *fwspec, unsigned long *hwirq, unsigned int *type)
>> +{
>> +	if (is_of_node(fwspec->fwnode)) {
>> +		if (fwspec->param_count < 3)
>> +			return -EINVAL;
> 
> Why 3? Reading the DT binding, this is indeed set to 3 without any
> reason. I'd suggest this becomes 2, encoding the pin number and the
> trigger information, as the leading 0 is quite useless. Yes, I know
> other examples in the kernel are using this 0, and that was a
> consequence of retrofitting the omitted interrupt controllers (back in
> the days of the stupid gic_arch_extn...). Don't do that.
> 

Hi Marc

I'm about to send out a new revision of the ls-extirq patchset, and
thanks to you pointing me to these patches, I've read the comments on
the various revisions of this series and tried to take those into
account. But the above confused me, because in response to my first RFC
(https://patchwork.kernel.org/patch/10102643/) we have

On 2017-12-08 17:09, Marc Zyngier wrote:
> On 08/12/17 15:11, Alexander Stein wrote:
>> Hi Rasmus,
>>
>>> +
>>> +Required properties:
>>> +- compatible: should be "fsl,ls1021a-extirq"
>>> +- interrupt-controller: Identifies the node as an interrupt controller
>>> +- #interrupt-cells: Use the same format as specified by GIC in
arm,gic.txt.
>>
>> Do you really need 3 interrupt-cells here? As you've written below
you don't
>> support PPI anyway the 1st flag might be dropped then. So support
just 2 cells:
>> * IRQ number (IRQ0 - IRQ5)
>> * IRQ flags
>
> The convention for irqchip stacked on top of a GIC is to keep the
> interrupt specifier the same. It makes the maintenance if the DT much
> easier, and doesn't hurt at all.

Personally, I'd actually prefer the simpler interrupt specifiers without
a redundant 0. Maybe I'm just missing some difference between this case
and the ls-extirq one?

Thanks,
Rasmus