On 04/09/2018 02:04 PM, Simon Horman wrote:

>> We now have rcar_pcie_hw_init_{h1|gen2|gen3}() differing only in the PCIe
>> PHY init code and all ending with a call to rcar_pcie_hw_init(), thus it
>> makes  sense to move that call into the driver's probe() method and then
>> rename those functions to rcar_pcie_phy_init_{h1|gen2|gen3}() -- doing
>> this saves 48 bytes of object code (AArch64 gcc 4.8.5)...
> 
> I'm not sure the churn is worth it, but if you do then that is find by me.

   s/find/fine/? :-)
   I think it's worth it -- makes the code follow more closely the manuals
where the only gen1/2/3 specific init is PHY related.

>> Signed-off-by: Sergei Shtylyov <sergei.shtyl...@cogentembedded.com>
>>
>> ---
>>  drivers/pci/host/pcie-rcar.c |   42 
>> ++++++++++++++++++++++--------------------
>>  1 file changed, 22 insertions(+), 20 deletions(-)
>>
>> Index: pci/drivers/pci/host/pcie-rcar.c
>> ===================================================================
>> --- pci.orig/drivers/pci/host/pcie-rcar.c
>> +++ pci/drivers/pci/host/pcie-rcar.c
[...]
>> @@ -1082,17 +1078,18 @@ static int rcar_pcie_parse_map_dma_range
>>  }
>>  
>>  static const struct of_device_id rcar_pcie_of_match[] = {
>> -    { .compatible = "renesas,pcie-r8a7779", .data = rcar_pcie_hw_init_h1 },
>> +    { .compatible = "renesas,pcie-r8a7779",
>> +      .data = rcar_pcie_phy_init_h1 },
>>      { .compatible = "renesas,pcie-r8a7790",
>> -      .data = rcar_pcie_hw_init_gen2 },
>> +      .data = rcar_pcie_phy_init_gen2 },
>>      { .compatible = "renesas,pcie-r8a7791",
>> -      .data = rcar_pcie_hw_init_gen2 },
>> +      .data = rcar_pcie_phy_init_gen2 },
>>      { .compatible = "renesas,pcie-rcar-gen2",
>> -      .data = rcar_pcie_hw_init_gen2 },
>> +      .data = rcar_pcie_phy_init_gen2 },
>>      { .compatible = "renesas,pcie-r8a7795",
>> -      .data = rcar_pcie_hw_init_gen3 },
>> +      .data = rcar_pcie_phy_init_gen3 },
>>      { .compatible = "renesas,pcie-rcar-gen3",
>> -      .data = rcar_pcie_hw_init_gen3 },
>> +      .data = rcar_pcie_phy_init_gen3 },
>>      {},
> 
> I would avoid the line wrapping here, but its up to you.

   I didn't want to break the 80-colums limit; and then again, wanted to keep 
the initializers alike... 

>> @@ -1140,7 +1137,7 @@ static int rcar_pcie_probe(struct platfo
>>      struct rcar_pcie *pcie;
>>      unsigned int data;
>>      int err;
>> -    int (*hw_init_fn)(struct rcar_pcie *);
>> +    int (*phy_init_fn)(struct rcar_pcie *);
> 
> Looking at this I wonder if we also need a phy_cleanup() code or
> similar.

   Makes sense -- iff we start supporting PM?..

[...]

MBR, Sergei

Reply via email to