Hi Minwoo,
Thank you for sending the patch over!
You might need to improve the subject a little - it should be brief but
still informative.
> __pci_set_mater() has debug log in there so that it would be better to
> take this function. So take __pci_set_master() function rather than
> open coding it. This patch didn't move __pci_set_master() to above to
> avoid churns.
[...]
It would be __pci_set_master() int he sentence above. Also, perhaps
"use" would be better than "take". Generally, this commit message might
need a little improvement to be more clear why are you do doing this.
[...]
> +static void __pci_set_master(struct pci_dev *dev, bool enable);
> static void do_pci_disable_device(struct pci_dev *dev)
> {
> - u16 pci_command;
> -
> - pci_read_config_word(dev, PCI_COMMAND, &pci_command);
> - if (pci_command & PCI_COMMAND_MASTER) {
> - pci_command &= ~PCI_COMMAND_MASTER;
> - pci_write_config_word(dev, PCI_COMMAND, pci_command);
> - }
> + __pci_set_master(dev, false);
>
> pcibios_disable_device(dev);
> }
You could use pci_clear_master(), which we export and that internally
calls __pci_set_master(), so there would be no need to add any forward
declarations or to move anything around in the file.
Having said that, there is a difference between do_pci_disable_device()
and how __pci_set_master() works - the latter sets the is_busmaster flag
accordingly on the given device whereas the former does not. This might
be of some significance - not sure if we should or should not set this,
since the do_pci_disable_device() does not do that (perhaps it's on
purpose or due to some hisoric reasons).
Krzysztof
