lkml.org 
[lkml]   [2016]   [Sep]   [1]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [Patch v3 02/11] irqchip: axi-intc: Clean up irqdomain argument and read/write
From
Date
Hi,

Thank you for the reviews.

Some comments below

On 08/31/2016 05:52 PM, Marc Zyngier wrote:
> On 31/08/16 17:35, Zubair Lutfullah Kakakhel wrote:
>> The drivers read/write function handling is a bit quirky.
>> And the irqmask is passed directly to the handler.
>>
>> Add a new irqchip struct to pass to the handler and
>> cleanup read/write handling.
>>
>> Signed-off-by: Zubair Lutfullah Kakakhel <Zubair.Kakakhel@imgtec.com>
>>
>> ---
>> V2 -> V3
>> New patch. Cleans up driver structure
>> ---
...
>> static int __init xilinx_intc_of_init(struct device_node *intc,
>> struct device_node *parent)
>> {
>> - u32 nr_irq, intr_mask;
>> + u32 nr_irq;
>> int ret;
>> + struct xintc_irq_chip *irqc;
>> +
>> + irqc = kzalloc(sizeof(*irqc), GFP_KERNEL);
>
> Now that you dynamically allocate things, how are you handling failures?
>
>> + if (!irqc)
>> + return -ENOMEM;
>> +
>> + xintc_irqc = irqc;
>>
>> - intc_baseaddr = of_iomap(intc, 0);
>> - BUG_ON(!intc_baseaddr);
>> + irqc->base = of_iomap(intc, 0);
>> + BUG_ON(!irqc->base);
>>
>> ret = of_property_read_u32(intc, "xlnx,num-intr-inputs", &nr_irq);
>> if (ret < 0) {
>> @@ -150,43 +176,40 @@ static int __init xilinx_intc_of_init(struct device_node *intc,
>> return ret;
>
> All the return paths should now take care of releasing the allocated
> resources.
>

Thanks for pointing it out. I'll fix it in the next series.

>> }
>>
>> - ret = of_property_read_u32(intc, "xlnx,kind-of-intr", &intr_mask);
>> + ret = of_property_read_u32(intc, "xlnx,kind-of-intr", &irqc->intr_mask);
>> if (ret < 0) {
>> pr_err("%s: unable to read xlnx,kind-of-intr\n", __func__);
>> return ret;
>> }
>>
>> - if (intr_mask >> nr_irq)
>> + if (irqc->intr_mask >> nr_irq)
>> pr_warn("%s: mismatch in kind-of-intr param\n", __func__);
>>
>> pr_info("%s: num_irq=%d, edge=0x%x\n",
>> - intc->full_name, nr_irq, intr_mask);
>> + intc->full_name, nr_irq, irqc->intr_mask);
>>
>> - write_fn = intc_write32;
>> - read_fn = intc_read32;
>> + irqc->read = intc_read32;
>> + irqc->write = intc_write32;
>>
>> /*
>> * Disable all external interrupts until they are
>> * explicity requested.
>> */
>> - write_fn(0, intc_baseaddr + IER);
>> + xintc_write(irqc, IER, 0);
>>
>> /* Acknowledge any pending interrupts just in case. */
>> - write_fn(0xffffffff, intc_baseaddr + IAR);
>> + xintc_write(irqc, IAR, 0xffffffff);
>>
>> /* Turn on the Master Enable. */
>> - write_fn(MER_HIE | MER_ME, intc_baseaddr + MER);
>> - if (!(read_fn(intc_baseaddr + MER) & (MER_HIE | MER_ME))) {
>> - write_fn = intc_write32_be;
>> - read_fn = intc_read32_be;
>> - write_fn(MER_HIE | MER_ME, intc_baseaddr + MER);
>> + xintc_write(irqc, MER, MER_HIE | MER_ME);
>> + if (!(xintc_read(irqc, MER) & (MER_HIE | MER_ME))) {
>> + irqc->read = intc_read32_be;
>> + irqc->write = intc_write32_be;
>> + xintc_write(irqc, MER, MER_HIE | MER_ME);
>> }
>>
>> - /* Yeah, okay, casting the intr_mask to a void* is butt-ugly, but I'm
>> - * lazy and Michal can clean it up to something nicer when he tests
>> - * and commits this patch. ~~gcl */
>> root_domain = irq_domain_add_linear(intc, nr_irq, &xintc_irq_domain_ops,
>> - (void *)intr_mask);
>> + irqc);
>>
>> irq_set_default_host(root_domain);
>>
>>
>
> You haven't addressed the comment on get_irq() which could be static (at
> least from solely looking at this file).

Apologies. In a rush to get a series out before heading home, it slipped my mind.
It is needed as it is used in arch/microblaze/kernel/irq.c

But I'll add a patch to rename it correctly. get_irq is far too generic outside arch code.

Thanks,
ZubairLK

>
> Thanks,
>
> M.
>

\
 
 \ /
  Last update: 2016-09-17 09:58    [W:0.130 / U:0.120 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site