Re: çå: [PATCH v9 2/4] arm64: dts: hisi: add kirin pcie node

[Bjorn Helgaas]

On Sat, Jun 17, 2017 at 08:33:08AM +0000, songxiaowei wrote:
> Hi Bjorn,
> 
> There are serval comments I can not understand, please help me to give more details.

Sure.  BTW, for some reason your response is all double-spaced, which
makes it a little hard to read.  Also, it includes HTML and an image,
which means the mailing list probably rejected it:
http://vger.kernel.org/majordomo-info.html#taboo

> diff --git a/Documentation/devicetree/bindings/pci/kirin-pcie.txt b/Documentation/devicetree/bindings/pci/kirin-pcie.txt
> 
> index 68ffa0fbcd73..20357d840af1 100644
> 
> --- a/Documentation/devicetree/bindings/pci/kirin-pcie.txt
> 
> +++ b/Documentation/devicetree/bindings/pci/kirin-pcie.txt
> 
> @@ -24,8 +24,8 @@ Example based on kirin960:
> 
>         pcie@f4000000 {
> 
>                 compatible = "hisilicon,kirin-pcie";
> 
> -                 reg = <0x0 0xf4000000 0x0 0x1000>, <0x0 0xff3fe000 0x0 0x1000>,
> 
> -                       <0x0 0xf3f20000 0x0 0x40000>, <0x0 0xF4000000 0 0x2000>;
> 
> +                reg = <0x0 0xf4000000 0x0  0x1000>, <0x0 0xff3fe000 0x0 0x1000>,
> 
> +                      <0x0 0xf3f20000 0x0 0x40000>, <0x0 0xf4000000 0x0 0x2000>;
> 
> [songxiaowei] The difference is add one more space between "0x0" and "0x1000" in the first element.
> 
>              But, I can't get your mind.

It changes from this:

        reg = <0x0 0xf4000000 0x0 0x1000>, <0x0 0xff3fe000 0x0 0x1000>,
              <0x0 0xf3f20000 0x0 0x40000>, <0x0 0xF4000000 0 0x2000>;

to this:

        reg = <0x0 0xf4000000 0x0  0x1000>, <0x0 0xff3fe000 0x0 0x1000>,
              <0x0 0xf3f20000 0x0 0x40000>, <0x0 0xf4000000 0x0 0x2000>;

The extra space makes the elements align vertically.  Columns of
numbers are conventionally right-aligned, which makes it easier to
compare their sizes.

This hunk also changed 0xF4000000 to 0xf4000000 in the second line, so
hex constants use lower-case consistently.

> --- a/arch/arm64/boot/dts/hisilicon/hi3660.dtsi
> 
> +++ b/arch/arm64/boot/dts/hisilicon/hi3660.dtsi
> 
> @@ -159,12 +159,12 @@
> 
>                  pcie@f4000000 {
> 
>                           compatible = "hisilicon,kirin960-pcie";
> 
> -                           reg = <0x0 0xf4000000 0x0 0x1000>,
> 
> -                                 <0x0 0xff3fe000 0x0 0x1000>,
> 
> +                          reg = <0x0 0xf4000000 0x0  0x1000>,
> 
> +                                <0x0 0xff3fe000 0x0  0x1000>,
> 
> [songxiaowei] Can your tell why using two spaces? Reg is listed as <addr_hi addr_lo size_hi size_lo>.

Again, just to make the sizes right-aligned so it's easier to compare
the sizes.

You can ignore this one if you want.  There are lots of examples in
the tree that don't align these.  I just think it looks sloppy when
things are almost but not quite aligned.

> -                                 <0x0 0xF5000000 0x0 0x2000>;
> +                                <0x0 0xf5000000 0x0  0x2000>;

Another case of making hex constants consistently lower-case.

> diff --git a/drivers/pci/dwc/pcie-kirin.c b/drivers/pci/dwc/pcie-kirin.c index f63e6548efae..41520dd1d5e5 100644
> 
> --- a/drivers/pci/dwc/pcie-kirin.c
> 
> +++ b/drivers/pci/dwc/pcie-kirin.c
> 
> @@ -35,8 +35,8 @@
> 
> #define REF_CLK_FREQ                    100000000
> 
>  /* PCIe ELBI registers */
> 
> -#define SOC_PCIECTRL_CTRL0_ADDR   0x000
> 
> -#define SOC_PCIECTRL_CTRL1_ADDR   0x004
> 
> +#define SOC_PCIECTRL_CTRL0_ADDR            0x000
> 
> +#define SOC_PCIECTRL_CTRL1_ADDR            0x004
> 
> #define SOC_PCIEPHY_CTRL2_ADDR             0x008
> 
> #define SOC_PCIEPHY_CTRL3_ADDR             0x00c
> ... 
> [songxiaowei] The space of the name of these macro definition and
> 
> the value are really different from 1 to 3 Tab, but it seem likes as bellow opened by vim.
> 
> [cid:image001.png@01D2E786.14883370]

The image shows this:

  +#define REF_CLK_FREQ                   100000000
  +
  +/* PCIe ELBI registers */
  +#define SOC_PCIECTRL_CTRL0_ADDR        0x000
  +#define SOC_PCIECTRL_CTRL1_ADDR        0x004
  +#define SOC_PCIEPHY_CTRL2_ADDR         0x008
  +#define SOC_PCIEPHY_CTRL3_ADDR         0x00c

So things are nicely aligned in the *patch*.  But we don't care about
alignment in the patch.  What we want is alignment in the *file*,
which ends up looking like this with your original patch:

  #define REF_CLK_FREQ                    100000000

  /* PCIe ELBI registers */
  #define SOC_PCIECTRL_CTRL0_ADDR 0x000
  #define SOC_PCIECTRL_CTRL1_ADDR 0x004
  #define SOC_PCIEPHY_CTRL2_ADDR          0x008
  #define SOC_PCIEPHY_CTRL3_ADDR          0x00c

My incremental diff probably makes the patch look ugly, but the
resulting file looks nicer.

>  struct kirin_pcie {
> 
> -        struct dw_pcie                   *pci;
> 
> -        void __iomem           *apb_base;
> ...
> +       struct dw_pcie          *pci;
> 
> +       void __iomem  *apb_base;
> ...
> [songxiaowei] it seems the variables list in the same coloumn with vim.

Yes, these variables were aligned in your original patch; it's just
that they used two or three tabs, when one or two was sufficient.  So
there's extra separation.  Not a big deal.

Bjorn