lkml.org 
[lkml]   [2018]   [Feb]   [7]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH v4 1/2] drivers: irqchip: pdc: Add PDC interrupt controller for QCOM SoCs
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

\
 
 \ /
  Last update: 2018-02-07 17:53    [W:0.079 / U:1.112 seconds]
©2003-2018 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site