Subject: Re: [PATCH v5 12/42] powerpc/pci: Cleanup on pci_controller_ops

From: Gavin Shan <[email-masked]>
Date: Wed, 10 Jun 2015 16:20:08 +1000

On Wed, Jun 10, 2015 at 02:43:57PM +1000, Alexey Kardashevskiy wrote:
>On 06/04/2015 04:41 PM, Gavin Shan wrote:
>>Each PHB maintains one instance of "struct pci_controller_ops",
>>which includes various callbacks called by PCI subsystem. In the
>>definition of this struct, some callbacks have explicit names for
>>its arguments, but the left don't have.
>>
>>The patch removes all explicit names of the arguments to the
>>callbacks in "struct pci_controller_ops" to keep the code look
>>consistent.
>
>imho it is a bad idea. Self-documeted code gets less self-documented - how do
>I know what "unsigned long" parameters are for without grepping?
>

Ok. I'll change the function definations to always have explicit
argument names.

>>
>>Cc: Daniel Axtens <[email-masked]>
>>Signed-off-by: Gavin Shan <[email-masked]>
>>---
>>v5:
>>   * Newly introduced
>>---
>>  arch/powerpc/include/asm/pci-bridge.h | 8 ++++----
>>  1 file changed, 4 insertions(+), 4 deletions(-)
>>
>>diff --git a/arch/powerpc/include/asm/pci-bridge.h b/arch/powerpc/include/asm/pci-bridge.h
>>index 744884b..1252cd5 100644
>>--- a/arch/powerpc/include/asm/pci-bridge.h
>>+++ b/arch/powerpc/include/asm/pci-bridge.h
>>@@ -18,8 +18,8 @@ struct device_node;
>>   * PCI controller operations
>>   */
>>  struct pci_controller_ops {
>>-	void		(*dma_dev_setup)(struct pci_dev *dev);
>>-	void		(*dma_bus_setup)(struct pci_bus *bus);
>>+	void		(*dma_dev_setup)(struct pci_dev *);
>>+	void		(*dma_bus_setup)(struct pci_bus *);
>>
>>  	int		(*probe_mode)(struct pci_bus *);
>>
>>@@ -28,8 +28,8 @@ struct pci_controller_ops {
>>  	bool		(*enable_device_hook)(struct pci_dev *);
>>
>>  	/* Called during PCI resource reassignment */
>>-	resource_size_t (*window_alignment)(struct pci_bus *, unsigned long type);
>>-	void		(*reset_secondary_bus)(struct pci_dev *dev);
>>+	resource_size_t (*window_alignment)(struct pci_bus *, unsigned long);
>>+	void		(*reset_secondary_bus)(struct pci_dev *);
>>  };
>>
>>  /*
>>

Thanks,
Gavin