| Message ID | 1446642770-4681-23-git-send-email-gwshan@linux.vnet.ibm.com |
|---|---|
| State | Not Applicable |
| Headers | show |
Gavin Shan <gwshan@linux.vnet.ibm.com> writes: > This introduces pnv_ioda_init_pe() to initialize the specified PE > instance (phb->ioda.pe_array[x]). It's used by pnv_ioda_alloc_pe() > and pnv_ioda_reserve_pe(). No logical changes introduced. > > Signed-off-by: Gavin Shan <gwshan@linux.vnet.ibm.com> > --- > arch/powerpc/platforms/powernv/pci-ioda.c | 14 ++++++++++---- > 1 file changed, 10 insertions(+), 4 deletions(-) > > diff --git a/arch/powerpc/platforms/powernv/pci-ioda.c b/arch/powerpc/platforms/powernv/pci-ioda.c > index ef93a01..488e0f8 100644 > --- a/arch/powerpc/platforms/powernv/pci-ioda.c > +++ b/arch/powerpc/platforms/powernv/pci-ioda.c > @@ -129,6 +129,14 @@ static inline bool pnv_pci_is_mem_pref_64(unsigned long flags) > (IORESOURCE_MEM_64 | IORESOURCE_PREFETCH)); > } > > +static struct pnv_ioda_pe *pnv_ioda_init_pe(struct pnv_phb *phb, int pe_no) > +{ > + phb->ioda.pe_array[pe_no].phb = phb; > + phb->ioda.pe_array[pe_no].pe_number = pe_no; > + > + return &phb->ioda.pe_array[pe_no]; You have the function returning the newly initalized PE here... > +} > + > static void pnv_ioda_reserve_pe(struct pnv_phb *phb, int pe_no) > { > if (!(pe_no >= 0 && pe_no < phb->ioda.total_pe_num)) { > @@ -141,8 +149,7 @@ static void pnv_ioda_reserve_pe(struct pnv_phb *phb, int pe_no) > pr_debug("%s: PE %d was reserved on PHB#%x\n", > __func__, pe_no, phb->hose->global_number); > > - phb->ioda.pe_array[pe_no].phb = phb; > - phb->ioda.pe_array[pe_no].pe_number = pe_no; > + pnv_ioda_init_pe(phb, pe_no); ... but then you ignore the result here and in the other function you've modified. It looks like you're using the result in the next patch though, so I wonder if you would be better to merge this patch with the next one. However, as I said before I'll defer to Alexey on decisions about how to split the patch series if he has a different opinion. Regards, Daniel
On Tue, Nov 17, 2015 at 11:30:49AM +1100, Daniel Axtens wrote: >Gavin Shan <gwshan@linux.vnet.ibm.com> writes: > >> This introduces pnv_ioda_init_pe() to initialize the specified PE >> instance (phb->ioda.pe_array[x]). It's used by pnv_ioda_alloc_pe() >> and pnv_ioda_reserve_pe(). No logical changes introduced. >> >> Signed-off-by: Gavin Shan <gwshan@linux.vnet.ibm.com> >> --- >> arch/powerpc/platforms/powernv/pci-ioda.c | 14 ++++++++++---- >> 1 file changed, 10 insertions(+), 4 deletions(-) >> >> diff --git a/arch/powerpc/platforms/powernv/pci-ioda.c b/arch/powerpc/platforms/powernv/pci-ioda.c >> index ef93a01..488e0f8 100644 >> --- a/arch/powerpc/platforms/powernv/pci-ioda.c >> +++ b/arch/powerpc/platforms/powernv/pci-ioda.c >> @@ -129,6 +129,14 @@ static inline bool pnv_pci_is_mem_pref_64(unsigned long flags) >> (IORESOURCE_MEM_64 | IORESOURCE_PREFETCH)); >> } >> >> +static struct pnv_ioda_pe *pnv_ioda_init_pe(struct pnv_phb *phb, int pe_no) >> +{ >> + phb->ioda.pe_array[pe_no].phb = phb; >> + phb->ioda.pe_array[pe_no].pe_number = pe_no; >> + >> + return &phb->ioda.pe_array[pe_no]; >You have the function returning the newly initalized PE here... > >> +} >> + >> static void pnv_ioda_reserve_pe(struct pnv_phb *phb, int pe_no) >> { >> if (!(pe_no >= 0 && pe_no < phb->ioda.total_pe_num)) { >> @@ -141,8 +149,7 @@ static void pnv_ioda_reserve_pe(struct pnv_phb *phb, int pe_no) >> pr_debug("%s: PE %d was reserved on PHB#%x\n", >> __func__, pe_no, phb->hose->global_number); >> >> - phb->ioda.pe_array[pe_no].phb = phb; >> - phb->ioda.pe_array[pe_no].pe_number = pe_no; >> + pnv_ioda_init_pe(phb, pe_no); >... but then you ignore the result here and in the other function you've >modified. > >It looks like you're using the result in the next patch though, so I >wonder if you would be better to merge this patch with the next >one. However, as I said before I'll defer to Alexey on decisions about >how to split the patch series if he has a different opinion. > I'd like to keep this separate when thinking about the rule I was told before: one patch does one thing if it can. Also, merging it to next one will make next one harder to be reiview. Thanks, Gavin -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 11/17/2015 12:58 PM, Gavin Shan wrote: > On Tue, Nov 17, 2015 at 11:30:49AM +1100, Daniel Axtens wrote: >> Gavin Shan <gwshan@linux.vnet.ibm.com> writes: >> >>> This introduces pnv_ioda_init_pe() to initialize the specified PE >>> instance (phb->ioda.pe_array[x]). It's used by pnv_ioda_alloc_pe() >>> and pnv_ioda_reserve_pe(). No logical changes introduced. >>> >>> Signed-off-by: Gavin Shan <gwshan@linux.vnet.ibm.com> >>> --- >>> arch/powerpc/platforms/powernv/pci-ioda.c | 14 ++++++++++---- >>> 1 file changed, 10 insertions(+), 4 deletions(-) >>> >>> diff --git a/arch/powerpc/platforms/powernv/pci-ioda.c b/arch/powerpc/platforms/powernv/pci-ioda.c >>> index ef93a01..488e0f8 100644 >>> --- a/arch/powerpc/platforms/powernv/pci-ioda.c >>> +++ b/arch/powerpc/platforms/powernv/pci-ioda.c >>> @@ -129,6 +129,14 @@ static inline bool pnv_pci_is_mem_pref_64(unsigned long flags) >>> (IORESOURCE_MEM_64 | IORESOURCE_PREFETCH)); >>> } >>> >>> +static struct pnv_ioda_pe *pnv_ioda_init_pe(struct pnv_phb *phb, int pe_no) >>> +{ >>> + phb->ioda.pe_array[pe_no].phb = phb; >>> + phb->ioda.pe_array[pe_no].pe_number = pe_no; >>> + >>> + return &phb->ioda.pe_array[pe_no]; >> You have the function returning the newly initalized PE here... >> >>> +} >>> + >>> static void pnv_ioda_reserve_pe(struct pnv_phb *phb, int pe_no) >>> { >>> if (!(pe_no >= 0 && pe_no < phb->ioda.total_pe_num)) { >>> @@ -141,8 +149,7 @@ static void pnv_ioda_reserve_pe(struct pnv_phb *phb, int pe_no) >>> pr_debug("%s: PE %d was reserved on PHB#%x\n", >>> __func__, pe_no, phb->hose->global_number); >>> >>> - phb->ioda.pe_array[pe_no].phb = phb; >>> - phb->ioda.pe_array[pe_no].pe_number = pe_no; >>> + pnv_ioda_init_pe(phb, pe_no); >> ... but then you ignore the result here and in the other function you've >> modified. >> >> It looks like you're using the result in the next patch though, so I >> wonder if you would be better to merge this patch with the next >> one. However, as I said before I'll defer to Alexey on decisions about >> how to split the patch series if he has a different opinion. >> > > I'd like to keep this separate when thinking about the rule I was told before: > one patch does one thing if it can. Also, merging it to next one will make > next one harder to be reiview. This patch merged into the next one will make the next one easier to review because you won't have to change there the code which you just added in this patch (which is always good).
On Tue, Nov 17, 2015 at 01:37:33PM +1100, Alexey Kardashevskiy wrote: >On 11/17/2015 12:58 PM, Gavin Shan wrote: >>On Tue, Nov 17, 2015 at 11:30:49AM +1100, Daniel Axtens wrote: >>>Gavin Shan <gwshan@linux.vnet.ibm.com> writes: >>> >>>>This introduces pnv_ioda_init_pe() to initialize the specified PE >>>>instance (phb->ioda.pe_array[x]). It's used by pnv_ioda_alloc_pe() >>>>and pnv_ioda_reserve_pe(). No logical changes introduced. >>>> >>>>Signed-off-by: Gavin Shan <gwshan@linux.vnet.ibm.com> >>>>--- >>>> arch/powerpc/platforms/powernv/pci-ioda.c | 14 ++++++++++---- >>>> 1 file changed, 10 insertions(+), 4 deletions(-) >>>> >>>>diff --git a/arch/powerpc/platforms/powernv/pci-ioda.c b/arch/powerpc/platforms/powernv/pci-ioda.c >>>>index ef93a01..488e0f8 100644 >>>>--- a/arch/powerpc/platforms/powernv/pci-ioda.c >>>>+++ b/arch/powerpc/platforms/powernv/pci-ioda.c >>>>@@ -129,6 +129,14 @@ static inline bool pnv_pci_is_mem_pref_64(unsigned long flags) >>>> (IORESOURCE_MEM_64 | IORESOURCE_PREFETCH)); >>>> } >>>> >>>>+static struct pnv_ioda_pe *pnv_ioda_init_pe(struct pnv_phb *phb, int pe_no) >>>>+{ >>>>+ phb->ioda.pe_array[pe_no].phb = phb; >>>>+ phb->ioda.pe_array[pe_no].pe_number = pe_no; >>>>+ >>>>+ return &phb->ioda.pe_array[pe_no]; >>>You have the function returning the newly initalized PE here... >>> >>>>+} >>>>+ >>>> static void pnv_ioda_reserve_pe(struct pnv_phb *phb, int pe_no) >>>> { >>>> if (!(pe_no >= 0 && pe_no < phb->ioda.total_pe_num)) { >>>>@@ -141,8 +149,7 @@ static void pnv_ioda_reserve_pe(struct pnv_phb *phb, int pe_no) >>>> pr_debug("%s: PE %d was reserved on PHB#%x\n", >>>> __func__, pe_no, phb->hose->global_number); >>>> >>>>- phb->ioda.pe_array[pe_no].phb = phb; >>>>- phb->ioda.pe_array[pe_no].pe_number = pe_no; >>>>+ pnv_ioda_init_pe(phb, pe_no); >>>... but then you ignore the result here and in the other function you've >>>modified. >>> >>>It looks like you're using the result in the next patch though, so I >>>wonder if you would be better to merge this patch with the next >>>one. However, as I said before I'll defer to Alexey on decisions about >>>how to split the patch series if he has a different opinion. >>> >> >>I'd like to keep this separate when thinking about the rule I was told before: >>one patch does one thing if it can. Also, merging it to next one will make >>next one harder to be reiview. > >This patch merged into the next one will make the next one easier to review >because you won't have to change there the code which you just added in this >patch (which is always good). > Ok & will do in next revision. Thanks, Gavin -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/arch/powerpc/platforms/powernv/pci-ioda.c b/arch/powerpc/platforms/powernv/pci-ioda.c index ef93a01..488e0f8 100644 --- a/arch/powerpc/platforms/powernv/pci-ioda.c +++ b/arch/powerpc/platforms/powernv/pci-ioda.c @@ -129,6 +129,14 @@ static inline bool pnv_pci_is_mem_pref_64(unsigned long flags) (IORESOURCE_MEM_64 | IORESOURCE_PREFETCH)); } +static struct pnv_ioda_pe *pnv_ioda_init_pe(struct pnv_phb *phb, int pe_no) +{ + phb->ioda.pe_array[pe_no].phb = phb; + phb->ioda.pe_array[pe_no].pe_number = pe_no; + + return &phb->ioda.pe_array[pe_no]; +} + static void pnv_ioda_reserve_pe(struct pnv_phb *phb, int pe_no) { if (!(pe_no >= 0 && pe_no < phb->ioda.total_pe_num)) { @@ -141,8 +149,7 @@ static void pnv_ioda_reserve_pe(struct pnv_phb *phb, int pe_no) pr_debug("%s: PE %d was reserved on PHB#%x\n", __func__, pe_no, phb->hose->global_number); - phb->ioda.pe_array[pe_no].phb = phb; - phb->ioda.pe_array[pe_no].pe_number = pe_no; + pnv_ioda_init_pe(phb, pe_no); } static int pnv_ioda_alloc_pe(struct pnv_phb *phb) @@ -156,8 +163,7 @@ static int pnv_ioda_alloc_pe(struct pnv_phb *phb) return IODA_INVALID_PE; } while(test_and_set_bit(pe, phb->ioda.pe_alloc)); - phb->ioda.pe_array[pe].phb = phb; - phb->ioda.pe_array[pe].pe_number = pe; + pnv_ioda_init_pe(phb, pe); return pe; }
This introduces pnv_ioda_init_pe() to initialize the specified PE instance (phb->ioda.pe_array[x]). It's used by pnv_ioda_alloc_pe() and pnv_ioda_reserve_pe(). No logical changes introduced. Signed-off-by: Gavin Shan <gwshan@linux.vnet.ibm.com> --- arch/powerpc/platforms/powernv/pci-ioda.c | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-)