Re: [PATCH v4 1/2] drivers: irqchip: pdc: Add PDC interrupt controller for QCOM SoCs

[Lina Iyer <ilina@codeaurora.org>]

On Wed, Feb 07 2018 at 16:43 +0000, Marc Zyngier wrote:
>On 07/02/18 15:49, Lina Iyer wrote:
>> From : Archana Sathyakumar <asathyak@codeaurora.org>
>>
>> The Power Domain Controller (PDC) on QTI SoCs like SDM845 houses an
>> interrupt controller along with other domain control functions to handle
>> interrupt related functions like handle falling edge or active low which
>> are not detected at the GIC and handle wakeup interrupts.
>>
>> The interrupt controller is on an always-on domain for the purpose of
>> waking up the processor. Only a subset of the processor's interrupts are
>> routed through the PDC to the GIC. The PDC powers on the processors'
>> domain, when in low power mode and replays pending interrupts so the GIC
>> may wake up the processor.
>>
>> Signed-off-by: Archana Sathyakumar <asathyak@codeaurora.org>
>> Signed-off-by: Lina Iyer <ilina@codeaurora.org>
>> ---

>> +	parent_hwirq = get_parent_hwirq(hwirq);
>
>Now that you return a well known value on error, how about testing it
>and erroring out instead of propagating it to the parent?
>
Hmm.. Okay.

>> +	region = kcalloc(n, sizeof(*region), GFP_KERNEL);
>> +	if (!region)
>> +		return -ENOMEM;
>> +
>> +	ret = of_property_read_u32_array(np, "qcom,pdc-ranges", region, n);
>> +	if (ret)
>> +		goto fail;
>> +
>> +	pdc_region_cnt = n / 3;
>> +	pdc_region = kcalloc(pdc_region_cnt, sizeof(*pdc_region), GFP_KERNEL);
>> +	if (!pdc_region) {
>> +		pdc_region_cnt = 0;
>> +		ret = -ENOMEM;
>> +		goto fail;
>> +	}
>> +
>> +	for (n = 0; n < pdc_region_cnt; n++, region += 3) {
>> +		pdc_region[n].pin_base    = region[0];
>> +		pdc_region[n].parent_base = region[1];
>> +		pdc_region[n].cnt         = region[2];
>
>Here's an alternative version that doesn't require any bounce buffer:
>
>	for (n = 0; n < pdc_region_cnt; n++) {
>		ret = of_property_read_u32_index(np, "qcom,pdc-ranges",
>				 n * 3 + 0, &pdc_region[n].pin_base);
>		if (ret)
>			goto fail;
>		ret = of_property_read_u32_index(np, "qcom,pdc-ranges",
>				 n * 3 + 1, &pdc_region[n].parent_base);
>		if (ret)
>			goto fail;
>		ret = of_property_read_u32_index(np, "qcom,pdc-ranges",
>				 n * 3 + 2, &pdc_region[n].cnt);
>		if (ret)
>			goto fail;
>	}
>
>And you can get rid of "region" altogether, because...
>
>> +	}
>> +
>> +fail:
>> +	kfree(region);
>
>... now that once you've incremented "region" in your for() loop, this
>kfree isn't going to do what you think it does.
>
Sure.

>> +	return ret;
>> +}
>> +
>> +static int qcom_pdc_init(struct device_node *node, struct device_node *parent)
>> +{
>> +	struct irq_domain *parent_domain, *pdc_domain;
>> +	int ret;
>> +
>> +	pdc_base = of_iomap(node, 0);
>> +	if (!pdc_base) {
>> +		pr_err("%pOF: unable to map PDC registers\n", node->full_name);
>
>The whole point of the %pOF specifier is that you don't pass the
>full_name field, but just the node itself. This is why I pointed you to
>commit ce4fecf1fe15 so that you could see how it is used...
>
Oops. I had seen the commit earlier, hence I didn't pay close attention
this time. Sorry, my bad.

Thanks,
Lina