| Message ID | 1483363905-2806-7-git-send-email-hanjun.guo@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi, Can we merge patch 4 & 6 into one patch so that we keep refactoring part as one piece ? I do not see a reason to keep them separate or have patch 5 in between. You can refactor what needs to be refactored, add necessary functions to iort.c and then support ACPI for irq-gic-v3-its-platform-msi.c Thanks, Tomasz On 02.01.2017 14:31, Hanjun Guo wrote: > Introduce its_pmsi_init_one() to refactor the code to isolate > ACPI&DT common code to prepare for ACPI later. > > Signed-off-by: Hanjun Guo <hanjun.guo@linaro.org> > Tested-by: Sinan Kaya <okaya@codeaurora.org> > Tested-by: Majun <majun258@huawei.com> > Tested-by: Xinwei Kong <kong.kongxinwei@hisilicon.com> > Cc: Marc Zyngier <marc.zyngier@arm.com> > Cc: Tomasz Nowicki <tn@semihalf.com> > Cc: Thomas Gleixner <tglx@linutronix.de> > --- > drivers/irqchip/irq-gic-v3-its-platform-msi.c | 45 ++++++++++++++++----------- > 1 file changed, 27 insertions(+), 18 deletions(-) > > diff --git a/drivers/irqchip/irq-gic-v3-its-platform-msi.c b/drivers/irqchip/irq-gic-v3-its-platform-msi.c > index 16587a9..ff72704 100644 > --- a/drivers/irqchip/irq-gic-v3-its-platform-msi.c > +++ b/drivers/irqchip/irq-gic-v3-its-platform-msi.c > @@ -84,34 +84,43 @@ static int its_pmsi_prepare(struct irq_domain *domain, struct device *dev, > {}, > }; > > -static int __init its_pmsi_init(void) > +static int __init its_pmsi_init_one(struct fwnode_handle *fwnode, > + const char *name) > { > - struct device_node *np; > struct irq_domain *parent; > > + parent = irq_find_matching_fwnode(fwnode, DOMAIN_BUS_NEXUS); > + if (!parent || !msi_get_domain_info(parent)) { > + pr_err("%s: unable to locate ITS domain\n", name); > + return -ENXIO; > + } > + > + if (!platform_msi_create_irq_domain(fwnode, &its_pmsi_domain_info, > + parent)) { > + pr_err("%s: unable to create platform domain\n", name); > + return -ENXIO; > + } > + > + pr_info("Platform MSI: %s domain created\n", name); > + return 0; > +} > + > +static void __init its_pmsi_of_init(void) > +{ > + struct device_node *np; > + > for (np = of_find_matching_node(NULL, its_device_id); np; > np = of_find_matching_node(np, its_device_id)) { > if (!of_property_read_bool(np, "msi-controller")) > continue; > > - parent = irq_find_matching_host(np, DOMAIN_BUS_NEXUS); > - if (!parent || !msi_get_domain_info(parent)) { > - pr_err("%s: unable to locate ITS domain\n", > - np->full_name); > - continue; > - } > - > - if (!platform_msi_create_irq_domain(of_node_to_fwnode(np), > - &its_pmsi_domain_info, > - parent)) { > - pr_err("%s: unable to create platform domain\n", > - np->full_name); > - continue; > - } > - > - pr_info("Platform MSI: %s domain created\n", np->full_name); > + its_pmsi_init_one(of_node_to_fwnode(np), np->full_name); > } > +} > > +static int __init its_pmsi_init(void) > +{ > + its_pmsi_of_init(); > return 0; > } > early_initcall(its_pmsi_init); >
Hi Tomasz, On 2017/1/3 15:41, Tomasz Nowicki wrote: > Hi, > > Can we merge patch 4 & 6 into one patch so that we keep refactoring part > as one piece ? I do not see a reason to keep them separate or have patch > 5 in between. You can refactor what needs to be refactored, add > necessary functions to iort.c and then support ACPI for > irq-gic-v3-its-platform-msi.c There are two functions here, - retrieve the dev id from IORT which was DT based only; - init the platform msi domain from MADT; For each of them split it into two steps, - refactor the code for ACPI later and it's easy for review because wen can easily to figure out it has functional change or not - add ACPI functionality Does it make sense? Thanks Hanjun
On 04.01.2017 08:02, Hanjun Guo wrote: > Hi Tomasz, > > On 2017/1/3 15:41, Tomasz Nowicki wrote: >> Hi, >> >> Can we merge patch 4 & 6 into one patch so that we keep refactoring part >> as one piece ? I do not see a reason to keep them separate or have patch >> 5 in between. You can refactor what needs to be refactored, add >> necessary functions to iort.c and then support ACPI for >> irq-gic-v3-its-platform-msi.c > > There are two functions here, > - retrieve the dev id from IORT which was DT based only; > > - init the platform msi domain from MADT; > > For each of them split it into two steps, > - refactor the code for ACPI later and it's easy for review > because wen can easily to figure out it has functional > change or not > > - add ACPI functionality > > Does it make sense? It is up to Marc, but personally I prefer: 1. Refactor dev id retrieving and init function in one patch and highlight no functional changes in changelog 2. Crate necessary infrastructure in iort.c 3. Then add ACPI support to irq-gic-v3-its-platform-msi.c Thanks, Tomasz
On 2017/1/4 15:29, Tomasz Nowicki wrote: > On 04.01.2017 08:02, Hanjun Guo wrote: >> Hi Tomasz, >> >> On 2017/1/3 15:41, Tomasz Nowicki wrote: >>> Hi, >>> >>> Can we merge patch 4 & 6 into one patch so that we keep refactoring part >>> as one piece ? I do not see a reason to keep them separate or have patch >>> 5 in between. You can refactor what needs to be refactored, add >>> necessary functions to iort.c and then support ACPI for >>> irq-gic-v3-its-platform-msi.c >> >> There are two functions here, >> - retrieve the dev id from IORT which was DT based only; >> >> - init the platform msi domain from MADT; >> >> For each of them split it into two steps, >> - refactor the code for ACPI later and it's easy for review >> because wen can easily to figure out it has functional >> change or not >> >> - add ACPI functionality >> >> Does it make sense? > > It is up to Marc, but personally I prefer: > 1. Refactor dev id retrieving and init function in one patch and > highlight no functional changes in changelog > 2. Crate necessary infrastructure in iort.c > 3. Then add ACPI support to irq-gic-v3-its-platform-msi.c I have no strong preferences, and it's easy to do so as just need to squash/reorder the patches. Marc, Lorenzo, could you give some suggestions here? Thanks Hanjun
On 04/01/17 08:25, Hanjun Guo wrote: > On 2017/1/4 15:29, Tomasz Nowicki wrote: >> On 04.01.2017 08:02, Hanjun Guo wrote: >>> Hi Tomasz, >>> >>> On 2017/1/3 15:41, Tomasz Nowicki wrote: >>>> Hi, >>>> >>>> Can we merge patch 4 & 6 into one patch so that we keep refactoring part >>>> as one piece ? I do not see a reason to keep them separate or have patch >>>> 5 in between. You can refactor what needs to be refactored, add >>>> necessary functions to iort.c and then support ACPI for >>>> irq-gic-v3-its-platform-msi.c >>> >>> There are two functions here, >>> - retrieve the dev id from IORT which was DT based only; >>> >>> - init the platform msi domain from MADT; >>> >>> For each of them split it into two steps, >>> - refactor the code for ACPI later and it's easy for review >>> because wen can easily to figure out it has functional >>> change or not >>> >>> - add ACPI functionality >>> >>> Does it make sense? >> >> It is up to Marc, but personally I prefer: >> 1. Refactor dev id retrieving and init function in one patch and >> highlight no functional changes in changelog >> 2. Crate necessary infrastructure in iort.c >> 3. Then add ACPI support to irq-gic-v3-its-platform-msi.c > > I have no strong preferences, and it's easy to do so as just > need to squash/reorder the patches. > > Marc, Lorenzo, could you give some suggestions here? I think it'd make the reviewing easier to have patches that are semantically grouped together (all the ACPI IORT together, for example). It would help understanding where you're aiming at instead of jumping from irqchip to ACPI and back every other patch... Thanks, M.
On 2017/1/4 17:02, Marc Zyngier wrote: > On 04/01/17 08:25, Hanjun Guo wrote: >> On 2017/1/4 15:29, Tomasz Nowicki wrote: >>> On 04.01.2017 08:02, Hanjun Guo wrote: >>>> Hi Tomasz, >>>> >>>> On 2017/1/3 15:41, Tomasz Nowicki wrote: >>>>> Hi, >>>>> >>>>> Can we merge patch 4 & 6 into one patch so that we keep refactoring part >>>>> as one piece ? I do not see a reason to keep them separate or have patch >>>>> 5 in between. You can refactor what needs to be refactored, add >>>>> necessary functions to iort.c and then support ACPI for >>>>> irq-gic-v3-its-platform-msi.c >>>> >>>> There are two functions here, >>>> - retrieve the dev id from IORT which was DT based only; >>>> >>>> - init the platform msi domain from MADT; >>>> >>>> For each of them split it into two steps, >>>> - refactor the code for ACPI later and it's easy for review >>>> because wen can easily to figure out it has functional >>>> change or not >>>> >>>> - add ACPI functionality >>>> >>>> Does it make sense? >>> >>> It is up to Marc, but personally I prefer: >>> 1. Refactor dev id retrieving and init function in one patch and >>> highlight no functional changes in changelog >>> 2. Crate necessary infrastructure in iort.c >>> 3. Then add ACPI support to irq-gic-v3-its-platform-msi.c >> >> I have no strong preferences, and it's easy to do so as just >> need to squash/reorder the patches. >> >> Marc, Lorenzo, could you give some suggestions here? > > I think it'd make the reviewing easier to have patches that are > semantically grouped together (all the ACPI IORT together, for example). > > It would help understanding where you're aiming at instead of jumping > from irqchip to ACPI and back every other patch... OK, I will reorder the patches and address the comments, then post a new version. Thanks Hanjun
diff --git a/drivers/irqchip/irq-gic-v3-its-platform-msi.c b/drivers/irqchip/irq-gic-v3-its-platform-msi.c index 16587a9..ff72704 100644 --- a/drivers/irqchip/irq-gic-v3-its-platform-msi.c +++ b/drivers/irqchip/irq-gic-v3-its-platform-msi.c @@ -84,34 +84,43 @@ static int its_pmsi_prepare(struct irq_domain *domain, struct device *dev, {}, }; -static int __init its_pmsi_init(void) +static int __init its_pmsi_init_one(struct fwnode_handle *fwnode, + const char *name) { - struct device_node *np; struct irq_domain *parent; + parent = irq_find_matching_fwnode(fwnode, DOMAIN_BUS_NEXUS); + if (!parent || !msi_get_domain_info(parent)) { + pr_err("%s: unable to locate ITS domain\n", name); + return -ENXIO; + } + + if (!platform_msi_create_irq_domain(fwnode, &its_pmsi_domain_info, + parent)) { + pr_err("%s: unable to create platform domain\n", name); + return -ENXIO; + } + + pr_info("Platform MSI: %s domain created\n", name); + return 0; +} + +static void __init its_pmsi_of_init(void) +{ + struct device_node *np; + for (np = of_find_matching_node(NULL, its_device_id); np; np = of_find_matching_node(np, its_device_id)) { if (!of_property_read_bool(np, "msi-controller")) continue; - parent = irq_find_matching_host(np, DOMAIN_BUS_NEXUS); - if (!parent || !msi_get_domain_info(parent)) { - pr_err("%s: unable to locate ITS domain\n", - np->full_name); - continue; - } - - if (!platform_msi_create_irq_domain(of_node_to_fwnode(np), - &its_pmsi_domain_info, - parent)) { - pr_err("%s: unable to create platform domain\n", - np->full_name); - continue; - } - - pr_info("Platform MSI: %s domain created\n", np->full_name); + its_pmsi_init_one(of_node_to_fwnode(np), np->full_name); } +} +static int __init its_pmsi_init(void) +{ + its_pmsi_of_init(); return 0; } early_initcall(its_pmsi_init);