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

[Johan Hovold <johan@kernel.org>]

On Fri, Jul 22, 2022 at 10:49:19AM -0500, Bjorn Helgaas wrote:
> From: Bjorn Helgaas <bhelgaas@google.com>
> 
> 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@google.com>
> ---
>  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.

On the other hand, it looks like the patch doesn't touch the revision I
currently care about, so your call.

> -/* Qcom IP rev.: 2.1.0	Synopsys IP rev.: 4.01a */
> -static const struct qcom_pcie_ops ops_2_1_0 = {
> -	.get_resources = qcom_pcie_get_resources_2_1_0,
> -	.init = qcom_pcie_init_2_1_0,
> -	.post_init = qcom_pcie_post_init_2_1_0,
> -	.deinit = qcom_pcie_deinit_2_1_0,
> -	.ltssm_enable = qcom_pcie_2_1_0_ltssm_enable,
> -};

Similar problem with git blame here, but at least this seems a bit more
warranted as these structs are small enough that you may actually search
manually among them.

> +static const struct qcom_pcie_cfg sm8150_cfg = {
> +	/* sm8150 has qcom IP rev 1.5.0. However 1.5.0 ops are same as
> +	 * 1.9.0, so reuse the same.
> +	 */
> +	.ops = &ops_1_9_0,
> +};

> +static const struct qcom_pcie_cfg sc8180x_cfg = {
> +	.ops = &ops_1_9_0,
> +	.has_tbu_clk = true,
> +};
> +
>  static const struct qcom_pcie_cfg ipq8064_cfg = {
>  	.ops = &ops_2_1_0,
>  };
> @@ -1626,42 +1662,6 @@ static const struct qcom_pcie_cfg sdm845_cfg = {
>  	.has_tbu_clk = true,
>  };
>  
> -static const struct qcom_pcie_cfg sm8150_cfg = {
> -	/* sm8150 has qcom IP rev 1.5.0. However 1.5.0 ops are same as
> -	 * 1.9.0, so reuse the same.
> -	 */
> -	.ops = &ops_1_9_0,
> -};

> -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?

Also note that this conflicts with my sc8280xp-support and IP-revision
series:

	https://lore.kernel.org/all/20220714071348.6792-1-johan+linaro@kernel.org/

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?

Or do I need to rebase on top first?

Johan