Re: [PATCH 2/2] PCI: qcom: Sort variants by Qcom IP rev

[Johan Hovold]

On Wed, Jul 27, 2022 at 05:02:20PM -0500, Bjorn Helgaas wrote:
> On Tue, Jul 26, 2022 at 02:45:34PM +0200, Johan Hovold wrote:
> > On Fri, Jul 22, 2022 at 10:49:19AM -0500, Bjorn Helgaas wrote:
> > > From: Bjorn Helgaas <bhelgaas@xxxxxxxxxx>
> > > 
> > > Previously the variant resource structs, ops, etc., were in no obvious
> > > order (mostly but not consistently in *Synopsys* IP rev order, which is not
> > > reflected in the naming).
> > > 
> > > Reorder them in order of the struct and function names.  No functional
> > > change intended.
> > > 
> > > Signed-off-by: Bjorn Helgaas <bhelgaas@xxxxxxxxxx>
> > > ---
> > >  drivers/pci/controller/dwc/pcie-qcom.c | 732 ++++++++++++-------------
> > >  1 file changed, 366 insertions(+), 366 deletions(-)
> > > 
> > > diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
> > > index c27e3494179f..d0237d821323 100644
> > > --- a/drivers/pci/controller/dwc/pcie-qcom.c
> > > +++ b/drivers/pci/controller/dwc/pcie-qcom.c
> > 
> > Moving code around like this makes code forensics harder as it messes up
> > git blame. At least the callbacks appears to be grouped by IP version
> > currently, so not sure how much you gain from moving the callbacks
> > around.
> 
> The existing hodge-podge is sloppy and makes code reading harder for
> everybody.  If we want them grouped by IP version, they should be
> *named* by IP version.

But they are named by IP version. It's just that people didn't add these
groups of callbacks in IP version order. (And the fact that different IP
version share some of these callbacks between revisions doesn't help.)

But sure, adding support for a new version would be easier if there's an
obvious sorting of the callbacks (as long as people notice the order).

> > > -static const struct qcom_pcie_cfg sc8180x_cfg = {
> > > -	.ops = &ops_1_9_0,
> > > -	.has_tbu_clk = true,
> > > -};
> > > -
> > >  static const struct qcom_pcie_cfg ipq6018_cfg = {
> > >  	.ops = &ops_2_9_0,
> > >  };
> > 
> > But this bit I disagree with. Why sort the SoCs configurations by IP
> > revision, when what you typically need is to look them up by name?
> 
> Makes sense.
> 
> > Also note that this conflicts with my sc8280xp-support and IP-revision
> > series:
> > 
> > 	https://lore.kernel.org/all/20220714071348.6792-1-johan+linaro@xxxxxxxxxx/
> > 
> > The result of applying that series is that these structs are renamed
> > after the IP revision (and sorted alphabetically) so the end-result is
> > similar.
> > 
> > Could you consider dropping this patch, or at least the struct
> > qcom_pcie_cfg bits, and applying the above series for 5.20?
> 
> I dropped it for now.  We can see how it shakes out after your series,
> but not sure I'll get to it for this cycle.

Ok. Thanks.

Johan