Messages in this thread Patch in this message | | | Date | Mon, 25 Mar 2019 09:24:06 +0000 | From | Marc Zyngier <> | Subject | Re: [patch v14 3/4] irqchip/qeic: merge qeic_of_init into qe_ic_init |
| |
On Mon, 25 Mar 2019 03:53:41 +0000, Qiang Zhao <qiang.zhao@nxp.com> wrote: > > qeic_of_init just get device_node of qeic from dtb and call qe_ic_init, > pass the device_node to qe_ic_init. > So merge qeic_of_init into qe_ic_init to get the qeic node in > qe_ic_init. > > Signed-off-by: Zhao Qiang <qiang.zhao@nxp.com> > --- > drivers/irqchip/irq-qeic.c | 110 ++++++++++++-------------------------------- > include/soc/fsl/qe/qe_ic.h | 7 --- > 2 files changed, 30 insertions(+), 87 deletions(-) > > diff --git a/drivers/irqchip/irq-qeic.c b/drivers/irqchip/irq-qeic.c > index 673afb4..a6ccbfb 100644 > --- a/drivers/irqchip/irq-qeic.c > +++ b/drivers/irqchip/irq-qeic.c > @@ -407,27 +407,30 @@ unsigned int qe_ic_get_high_irq(struct qe_ic *qe_ic) > return irq_linear_revmap(qe_ic->irqhost, irq); > } > > -void __init qe_ic_init(struct device_node *node, unsigned int flags, > - void (*low_handler)(struct irq_desc *desc), > - void (*high_handler)(struct irq_desc *desc)) > +static int __init qe_ic_init(struct device_node *node, > + struct device_node *parent) > { > struct qe_ic *qe_ic; > struct resource res; > - u32 temp = 0, ret, high_active = 0; > + int ret = 0; > > ret = of_address_to_resource(node, 0, &res); > - if (ret) > - return; > + if (ret) { > + ret = -ENODEV; > + goto err_put_node; > + } > > qe_ic = kzalloc(sizeof(*qe_ic), GFP_KERNEL); > - if (qe_ic == NULL) > - return; > + if (qe_ic == NULL) { > + ret = -ENOMEM; > + goto err_put_node; > + } > > qe_ic->irqhost = irq_domain_add_linear(node, NR_QE_IC_INTS, > &qe_ic_host_ops, qe_ic); > if (qe_ic->irqhost == NULL) { > - kfree(qe_ic); > - return; > + ret = -ENOMEM; > + goto err_free_qe_ic; > } > > qe_ic->regs = ioremap(res.start, resource_size(&res)); > @@ -438,42 +441,30 @@ void __init qe_ic_init(struct device_node *node, unsigned int flags, > qe_ic->virq_low = irq_of_parse_and_map(node, 1); > > if (qe_ic->virq_low == NO_IRQ) { > - printk(KERN_ERR "Failed to map QE_IC low IRQ\n"); > - kfree(qe_ic); > - return; > - } > - > - /* default priority scheme is grouped. If spread mode is */ > - /* required, configure cicr accordingly. */ > - if (flags & QE_IC_SPREADMODE_GRP_W) > - temp |= CICR_GWCC; > - if (flags & QE_IC_SPREADMODE_GRP_X) > - temp |= CICR_GXCC; > - if (flags & QE_IC_SPREADMODE_GRP_Y) > - temp |= CICR_GYCC; > - if (flags & QE_IC_SPREADMODE_GRP_Z) > - temp |= CICR_GZCC; > - if (flags & QE_IC_SPREADMODE_GRP_RISCA) > - temp |= CICR_GRTA; > - if (flags & QE_IC_SPREADMODE_GRP_RISCB) > - temp |= CICR_GRTB; > - > - /* choose destination signal for highest priority interrupt */ > - if (flags & QE_IC_HIGH_SIGNAL) { > - temp |= (SIGNAL_HIGH << CICR_HPIT_SHIFT); > - high_active = 1; > + pr_err("Failed to map QE_IC low IRQ\n"); > + ret = -ENOMEM; > + goto err_domain_remove;
None of this change is documented in the commit log. Why? How can you afford to get rid of it without breaking anything?
> } > > - qe_ic_write(qe_ic->regs, QEIC_CICR, temp); > - > irq_set_handler_data(qe_ic->virq_low, qe_ic); > - irq_set_chained_handler(qe_ic->virq_low, low_handler); > + irq_set_chained_handler(qe_ic->virq_low, qe_ic_cascade_low_mpic); > > if (qe_ic->virq_high != NO_IRQ && > qe_ic->virq_high != qe_ic->virq_low) { > irq_set_handler_data(qe_ic->virq_high, qe_ic); > - irq_set_chained_handler(qe_ic->virq_high, high_handler); > + irq_set_chained_handler(qe_ic->virq_high, > + qe_ic_cascade_high_mpic); > } > + of_node_put(node); > + return 0; > + > +err_domain_remove: > + irq_domain_remove(qe_ic->irqhost); > +err_free_qe_ic: > + kfree(qe_ic); > +err_put_node: > + of_node_put(node); > + return ret; > } > > void qe_ic_set_highest_priority(unsigned int virq, int high) > @@ -570,45 +561,4 @@ int qe_ic_set_high_priority(unsigned int virq, unsigned int priority, int high) > return 0; > } > > -static struct bus_type qe_ic_subsys = { > - .name = "qe_ic", > - .dev_name = "qe_ic", > -}; > - > -static struct device device_qe_ic = { > - .id = 0, > - .bus = &qe_ic_subsys, > -}; > - > -static int __init init_qe_ic_sysfs(void) > -{ > - int rc; > - > - printk(KERN_DEBUG "Registering qe_ic with sysfs...\n"); > - > - rc = subsys_system_register(&qe_ic_subsys, NULL); > - if (rc) { > - printk(KERN_ERR "Failed registering qe_ic sys class\n"); > - return -ENODEV; > - } > - rc = device_register(&device_qe_ic); > - if (rc) { > - printk(KERN_ERR "Failed registering qe_ic sys device\n"); > - return -ENODEV; > - } > - return 0; > -}
You're removing this sysfs stuff. Granted, it is completely pointless. Are you sure no userspace ever relied on this thing? In any case, this needs documenting.
> - > -static int __init qeic_of_init(struct device_node *node, > - struct device_node *parent) > -{ > - if (!node) > - return -ENODEV; > - qe_ic_init(node, 0, qe_ic_cascade_low_mpic, > - qe_ic_cascade_high_mpic); > - of_node_put(node); > - return 0; > -} > - > -IRQCHIP_DECLARE(qeic, "fsl,qe-ic", qeic_of_init); > -subsys_initcall(init_qe_ic_sysfs); > +IRQCHIP_DECLARE(qeic, "fsl,qe-ic", qe_ic_init); > diff --git a/include/soc/fsl/qe/qe_ic.h b/include/soc/fsl/qe/qe_ic.h > index 1e155ca..6113699 100644 > --- a/include/soc/fsl/qe/qe_ic.h > +++ b/include/soc/fsl/qe/qe_ic.h > @@ -58,16 +58,9 @@ enum qe_ic_grp_id { > }; > > #ifdef CONFIG_QUICC_ENGINE > -void qe_ic_init(struct device_node *node, unsigned int flags, > - void (*low_handler)(struct irq_desc *desc), > - void (*high_handler)(struct irq_desc *desc)); > unsigned int qe_ic_get_low_irq(struct qe_ic *qe_ic); > unsigned int qe_ic_get_high_irq(struct qe_ic *qe_ic); > #else > -static inline void qe_ic_init(struct device_node *node, unsigned int flags, > - void (*low_handler)(struct irq_desc *desc), > - void (*high_handler)(struct irq_desc *desc)) > -{} > static inline unsigned int qe_ic_get_low_irq(struct qe_ic *qe_ic) > { return 0; } > static inline unsigned int qe_ic_get_high_irq(struct qe_ic *qe_ic) > -- > 1.7.1 >
Thanks,
M.
-- Jazz is not dead, it just smell funny.
| |