| Message ID | 1467683219-29326-2-git-send-email-qiang.zhao@nxp.com (mailing list archive) |
|---|---|
| State | Not Applicable |
| Headers | show |
Hi Zhao Qiang, Same comment as previous patch regarding the subject line. On Tue, Jul 05, 2016 at 09:46:59AM +0800, Zhao Qiang wrote: > there are init_qe_ic_sysfs and qeic_of_init, refactor > them. Same comment from previous patch about commit log. > > Signed-off-by: Zhao Qiang <qiang.zhao@nxp.com> > --- > drivers/irqchip/qe_ic.c | 83 +++++++++++++++++++++++++--------------------- > include/soc/fsl/qe/qe_ic.h | 7 ---- > 2 files changed, 45 insertions(+), 45 deletions(-) > > diff --git a/drivers/irqchip/qe_ic.c b/drivers/irqchip/qe_ic.c > index f7f9a81..46652c0 100644 > --- a/drivers/irqchip/qe_ic.c > +++ b/drivers/irqchip/qe_ic.c > @@ -317,27 +317,35 @@ 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(unsigned int flags) > { > + struct device_node *node; > struct qe_ic *qe_ic; > struct resource res; > - u32 temp = 0, ret, high_active = 0; > + u32 temp = 0, high_active = 0; > + int ret = 0; > + > + node = of_find_compatible_node(NULL, NULL, "fsl,qe-ic"); > + if (!node) > + return -ENODEV; > > 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)); > @@ -348,9 +356,9 @@ 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; > + pr_err("Failed to map QE_IC low IRQ\n"); > + ret = -ENOMEM; > + goto err_domain_remove; > } > > /* default priority scheme is grouped. If spread mode is */ > @@ -377,13 +385,23 @@ void __init qe_ic_init(struct device_node *node, unsigned int flags, > 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); > } > + return ret; of_node_put(node)? Explicitly return success? > + > +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) > @@ -490,37 +508,26 @@ static struct device device_qe_ic = { > .bus = &qe_ic_subsys, > }; > > -static int __init init_qe_ic_sysfs(void) > +static int __init init_qe_ic(void) > { > - int rc; > + int ret; > > - printk(KERN_DEBUG "Registering qe_ic with sysfs...\n"); > + ret = qe_ic_init(0); Sorry, build machine is down atm. How was qe_ic_init() called previously? Is that removed? > + if (ret) > + return ret; > > - rc = subsys_system_register(&qe_ic_subsys, NULL); > - if (rc) { > - printk(KERN_ERR "Failed registering qe_ic sys class\n"); > + ret = subsys_system_register(&qe_ic_subsys, NULL); > + if (ret) { > + pr_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"); > + ret = device_register(&device_qe_ic); > + if (ret) { > + pr_err("Failed registering qe_ic sys device\n"); > return -ENODEV; > } > - return 0; > -} > > -static int __init qeic_of_init(void) > -{ > - struct device_node *np; > - > - np = of_find_compatible_node(NULL, NULL, "fsl,qe-ic"); > - if (np) { > - qe_ic_init(np, 0, qe_ic_cascade_low_mpic, > - qe_ic_cascade_high_mpic); > - of_node_put(np); > - } > return 0; > } > > -subsys_initcall(qeic_of_init); > -subsys_initcall(init_qe_ic_sysfs); > +subsys_initcall(init_qe_ic); > 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) thx, Jason. > -- > 2.1.0.27.g96db324 >
On 07/05/2016 11:51 AM, Jason Cooper <jason@lakedaemon.net> wrote: > -----Original Message----- > From: Jason Cooper [mailto:jason@lakedaemon.net] > Sent: Tuesday, July 05, 2016 11:51 AM > To: Qiang Zhao <qiang.zhao@nxp.com> > Cc: oss@buserror.net; tglx@linutronix.de; marc.zyngier@arm.com; linuxppc- > dev@lists.ozlabs.org; linux-kernel@vger.kernel.org; Xiaobo Xie > <xiaobo.xie@nxp.com> > Subject: Re: [PATCH 2/2] qe/ic: refactor qe_ic to simplify > > Hi Zhao Qiang, > > Same comment as previous patch regarding the subject line. > > On Tue, Jul 05, 2016 at 09:46:59AM +0800, Zhao Qiang wrote: > > there are init_qe_ic_sysfs and qeic_of_init, refactor them. > > Same comment from previous patch about commit log. > > > > > Signed-off-by: Zhao Qiang <qiang.zhao@nxp.com> > > --- > > drivers/irqchip/qe_ic.c | 83 +++++++++++++++++++++++++------------------ > --- > > include/soc/fsl/qe/qe_ic.h | 7 ---- > > 2 files changed, 45 insertions(+), 45 deletions(-) > > > > diff --git a/drivers/irqchip/qe_ic.c b/drivers/irqchip/qe_ic.c index > > f7f9a81..46652c0 100644 > > --- a/drivers/irqchip/qe_ic.c > > +++ b/drivers/irqchip/qe_ic.c > > @@ -317,27 +317,35 @@ 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(unsigned int flags) > > { > > + struct device_node *node; > > struct qe_ic *qe_ic; > > struct resource res; > > - u32 temp = 0, ret, high_active = 0; > > + u32 temp = 0, high_active = 0; > > + int ret = 0; > > + > > + node = of_find_compatible_node(NULL, NULL, "fsl,qe-ic"); > > + if (!node) > > + return -ENODEV; > > > > 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)); @@ -348,9 > > +356,9 @@ 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; > > + pr_err("Failed to map QE_IC low IRQ\n"); > > + ret = -ENOMEM; > > + goto err_domain_remove; > > } > > > > /* default priority scheme is grouped. If spread mode is */ > > @@ -377,13 +385,23 @@ void __init qe_ic_init(struct device_node *node, > unsigned int flags, > > 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); > > } > > + return ret; > > of_node_put(node)? Explicitly return success? Yes, thank you very much! > > > + > > +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) @@ > > -490,37 +508,26 @@ static struct device device_qe_ic = { > > .bus = &qe_ic_subsys, > > }; > > > > -static int __init init_qe_ic_sysfs(void) > > +static int __init init_qe_ic(void) > > { > > - int rc; > > + int ret; > > > > - printk(KERN_DEBUG "Registering qe_ic with sysfs...\n"); > > + ret = qe_ic_init(0); > > Sorry, build machine is down atm. How was qe_ic_init() called previously? Is > that removed? Sorry, I don't understand, could you please explain? > > > + if (ret) > > + return ret; > > > > - rc = subsys_system_register(&qe_ic_subsys, NULL); > > - if (rc) { > > - printk(KERN_ERR "Failed registering qe_ic sys class\n"); > > + ret = subsys_system_register(&qe_ic_subsys, NULL); > > + if (ret) { > > + pr_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"); > > + ret = device_register(&device_qe_ic); > > + if (ret) { > > + pr_err("Failed registering qe_ic sys device\n"); > > return -ENODEV; > > } > > - return 0; > > -} > > > > -static int __init qeic_of_init(void) > > -{ > > - struct device_node *np; > > - > > - np = of_find_compatible_node(NULL, NULL, "fsl,qe-ic"); > > - if (np) { > > - qe_ic_init(np, 0, qe_ic_cascade_low_mpic, > > - qe_ic_cascade_high_mpic); > > - of_node_put(np); > > - } > > return 0; > > } > > > > -subsys_initcall(qeic_of_init); > > -subsys_initcall(init_qe_ic_sysfs); > > +subsys_initcall(init_qe_ic); > > 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) -Zhao Qiang BR
Morning! On Tue, Jul 05, 2016 at 07:38:49AM +0000, Qiang Zhao wrote: > On 07/05/2016 11:51 AM, Jason Cooper <jason@lakedaemon.net> wrote: > > On Tue, Jul 05, 2016 at 09:46:59AM +0800, Zhao Qiang wrote: ... > > > > > > Signed-off-by: Zhao Qiang <qiang.zhao@nxp.com> > > > --- > > > drivers/irqchip/qe_ic.c | 83 +++++++++++++++++++++++++------------------ > > --- > > > include/soc/fsl/qe/qe_ic.h | 7 ---- > > > 2 files changed, 45 insertions(+), 45 deletions(-) > > > > > > diff --git a/drivers/irqchip/qe_ic.c b/drivers/irqchip/qe_ic.c index > > > f7f9a81..46652c0 100644 > > > --- a/drivers/irqchip/qe_ic.c > > > +++ b/drivers/irqchip/qe_ic.c ... > > > - printk(KERN_DEBUG "Registering qe_ic with sysfs...\n"); > > > + ret = qe_ic_init(0); > > > > Sorry, build machine is down atm. How was qe_ic_init() called previously? Is > > that removed? > > Sorry, I don't understand, could you please explain? -ENOSLEEP when I wrote that. :) *My* build machine, with my copies of the kernel tree was down, so I had no easy way to dig into the source. And .... > > > -static int __init qeic_of_init(void) > > > -{ > > > - struct device_node *np; > > > - > > > - np = of_find_compatible_node(NULL, NULL, "fsl,qe-ic"); > > > - if (np) { > > > - qe_ic_init(np, 0, qe_ic_cascade_low_mpic, > > > - qe_ic_cascade_high_mpic); > > > - of_node_put(np); > > > - } > > > return 0; > > > } > > > > > > -subsys_initcall(qeic_of_init); this block is what I missed last night. :-/ Sorry for the noise. thx, Jason.
diff --git a/drivers/irqchip/qe_ic.c b/drivers/irqchip/qe_ic.c index f7f9a81..46652c0 100644 --- a/drivers/irqchip/qe_ic.c +++ b/drivers/irqchip/qe_ic.c @@ -317,27 +317,35 @@ 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(unsigned int flags) { + struct device_node *node; struct qe_ic *qe_ic; struct resource res; - u32 temp = 0, ret, high_active = 0; + u32 temp = 0, high_active = 0; + int ret = 0; + + node = of_find_compatible_node(NULL, NULL, "fsl,qe-ic"); + if (!node) + return -ENODEV; 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)); @@ -348,9 +356,9 @@ 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; + pr_err("Failed to map QE_IC low IRQ\n"); + ret = -ENOMEM; + goto err_domain_remove; } /* default priority scheme is grouped. If spread mode is */ @@ -377,13 +385,23 @@ void __init qe_ic_init(struct device_node *node, unsigned int flags, 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); } + return ret; + +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) @@ -490,37 +508,26 @@ static struct device device_qe_ic = { .bus = &qe_ic_subsys, }; -static int __init init_qe_ic_sysfs(void) +static int __init init_qe_ic(void) { - int rc; + int ret; - printk(KERN_DEBUG "Registering qe_ic with sysfs...\n"); + ret = qe_ic_init(0); + if (ret) + return ret; - rc = subsys_system_register(&qe_ic_subsys, NULL); - if (rc) { - printk(KERN_ERR "Failed registering qe_ic sys class\n"); + ret = subsys_system_register(&qe_ic_subsys, NULL); + if (ret) { + pr_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"); + ret = device_register(&device_qe_ic); + if (ret) { + pr_err("Failed registering qe_ic sys device\n"); return -ENODEV; } - return 0; -} -static int __init qeic_of_init(void) -{ - struct device_node *np; - - np = of_find_compatible_node(NULL, NULL, "fsl,qe-ic"); - if (np) { - qe_ic_init(np, 0, qe_ic_cascade_low_mpic, - qe_ic_cascade_high_mpic); - of_node_put(np); - } return 0; } -subsys_initcall(qeic_of_init); -subsys_initcall(init_qe_ic_sysfs); +subsys_initcall(init_qe_ic); 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)
there are init_qe_ic_sysfs and qeic_of_init, refactor them. Signed-off-by: Zhao Qiang <qiang.zhao@nxp.com> --- drivers/irqchip/qe_ic.c | 83 +++++++++++++++++++++++++--------------------- include/soc/fsl/qe/qe_ic.h | 7 ---- 2 files changed, 45 insertions(+), 45 deletions(-)