Re: [PATCH v2 1/2] powerpc/PCI: Add pcibios_device_change_notifier for powerpc

[Jesse Larrew <jlarrew@linux.vnet.ibm.com>]

On 05/22/2012 09:33 PM, Hiroo Matsumoto wrote:

> +static int pcibios_device_change_notifier(struct notifier_block *nb,
> +					  unsigned long action, void *data)
> +{
> +	struct pci_dev *dev = to_pci_dev(data);
> +



In the general case, we don't know what *data contains until we evaluate
'action'. This conversion should probably be moved inside the case:
statement.

> +	switch (action) {
> +	case BUS_NOTIFY_ADD_DEVICE:
> +		/* Setup OF node pointer in the device */
> +		dev->dev.of_node = pci_device_to_OF_node(dev);
> +
> +		/* Fixup NUMA node as it may not be setup yet by the generic
> +		 * code and is needed by the DMA init
> +		 */
> +		set_dev_node(&dev->dev, pcibus_to_node(dev->bus));
> +
> +		/* Hook up default DMA ops */
> +		set_dma_ops(&dev->dev, pci_dma_ops);
> +		set_dma_offset(&dev->dev, PCI_DRAM_OFFSET);
> +
> +		/* Additional platform DMA/iommu setup */
> +		if (ppc_md.pci_dma_dev_setup)
> +			ppc_md.pci_dma_dev_setup(dev);
> +
> +		/* Read default IRQs and fixup if necessary */
> +		pci_read_irq_line(dev);
> +		if (ppc_md.pci_irq_fixup)
> +			ppc_md.pci_irq_fixup(dev);
> +
> +		break;
> +	}
> +
> +	return 0;
> +}
> +
> +static struct notifier_block device_nb = {
> +	.notifier_call = pcibios_device_change_notifier,
> +};



This is just a nit pick, but I think the naming of
pcibios_device_change_notifier() is a bit misleading. It doesn't
actually notify anything, but instead it *handles* notifications.
Perhaps a better name would be pcibios_device_change_handler() or
pcibios_device_change_callback()?

Sincerely,

Jesse Larrew
Software Engineer, Linux on Power Kernel Team
IBM Linux Technology Center
Phone: (512) 973-2052 (T/L: 363-2052)
jlarrew@linux.vnet.ibm.com