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

[Lina Iyer]

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@xxxxxxxxxxxxxx>
> >
> > 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@xxxxxxxxxxxxxx>
> > Signed-off-by: Lina Iyer <ilina@xxxxxxxxxxxxxx>
> > ---

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