[v2,2/4] PCI/quirks: Rename piix4_[io|mem]_quirk to add acpi

From: Deng-Cheng Zhu <dengcheng.zhu@imgtec.com>

Rename piix4_[io|mem]_quirk to piix4_acpi_[io|mem]_quirk because these 2
functions only deal with the function 3 (power management) of PIIX4.

Signed-off-by: Deng-Cheng Zhu <dengcheng.zhu@imgtec.com>
Reviewed-by: James Hogan <james.hogan@imgtec.com>
---
 drivers/pci/quirks.c |   22 ++++++++++++----------
 1 files changed, 12 insertions(+), 10 deletions(-)

On Fri, Oct 4, 2013 at 5:30 PM, Deng-Cheng Zhu <dengcheng.zhu@imgtec.com> wrote:
> From: Deng-Cheng Zhu <dengcheng.zhu@imgtec.com>
>
> Rename piix4_[io|mem]_quirk to piix4_acpi_[io|mem]_quirk because these 2
> functions only deal with the function 3 (power management) of PIIX4.
>
> Signed-off-by: Deng-Cheng Zhu <dengcheng.zhu@imgtec.com>
> Reviewed-by: James Hogan <james.hogan@imgtec.com>

I don't really see the point in patches 2-4.  We end up with 50 more
lines of code.  Most of this is new #defines.  They do add names where
before we only had numbers, which *sounds* attractive, but it looks
like each #define is used only once, and it's in chipset-dependent
code where a careful reader has to compare every line with the spec
anyway.  Adding a #define means you have to look at the definition,
the use, and the spec.  Without the #define, you only have to look at
the code and the spec.  I don't think it's worth it.

Bjorn

> ---
>  drivers/pci/quirks.c |   22 ++++++++++++----------
>  1 files changed, 12 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
> index 3e7e489..a2b66c3 100644
> --- a/drivers/pci/quirks.c
> +++ b/drivers/pci/quirks.c
> @@ -380,7 +380,8 @@ static void quirk_ali7101_acpi(struct pci_dev *dev)
>  }
>  DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_AL,     PCI_DEVICE_ID_AL_M7101,         quirk_ali7101_acpi);
>
> -static void piix4_io_quirk(struct pci_dev *dev, const char *name, unsigned int port, unsigned int enable)
> +static void piix4_acpi_io_quirk(struct pci_dev *dev, const char *name,
> +                               unsigned int port, unsigned int enable)
>  {
>         u32 devres;
>         u32 mask, size, base;
> @@ -406,7 +407,8 @@ static void piix4_io_quirk(struct pci_dev *dev, const char *name, unsigned int p
>         dev_info(&dev->dev, "%s PIO at %04x-%04x\n", name, base, base + size - 1);
>  }
>
> -static void piix4_mem_quirk(struct pci_dev *dev, const char *name, unsigned int port, unsigned int enable)
> +static void piix4_acpi_mem_quirk(struct pci_dev *dev, const char *name,
> +                                unsigned int port, unsigned int enable)
>  {
>         u32 devres;
>         u32 mask, size, base;
> @@ -447,23 +449,23 @@ static void quirk_piix4_acpi(struct pci_dev *dev)
>         /* Device resource A has enables for some of the other ones */
>         pci_read_config_dword(dev, 0x5c, &res_a);
>
> -       piix4_io_quirk(dev, "PIIX4 devres B", 0x60, 3 << 21);
> -       piix4_io_quirk(dev, "PIIX4 devres C", 0x64, 3 << 21);
> +       piix4_acpi_io_quirk(dev, "PIIX4 devres B", 0x60, 3 << 21);
> +       piix4_acpi_io_quirk(dev, "PIIX4 devres C", 0x64, 3 << 21);
>
>         /* Device resource D is just bitfields for static resources */
>
>         /* Device 12 enabled? */
>         if (res_a & (1 << 29)) {
> -               piix4_io_quirk(dev, "PIIX4 devres E", 0x68, 1 << 20);
> -               piix4_mem_quirk(dev, "PIIX4 devres F", 0x6c, 1 << 7);
> +               piix4_acpi_io_quirk(dev, "PIIX4 devres E", 0x68, 1 << 20);
> +               piix4_acpi_mem_quirk(dev, "PIIX4 devres F", 0x6c, 1 << 7);
>         }
>         /* Device 13 enabled? */
>         if (res_a & (1 << 30)) {
> -               piix4_io_quirk(dev, "PIIX4 devres G", 0x70, 1 << 20);
> -               piix4_mem_quirk(dev, "PIIX4 devres H", 0x74, 1 << 7);
> +               piix4_acpi_io_quirk(dev, "PIIX4 devres G", 0x70, 1 << 20);
> +               piix4_acpi_mem_quirk(dev, "PIIX4 devres H", 0x74, 1 << 7);
>         }
> -       piix4_io_quirk(dev, "PIIX4 devres I", 0x78, 1 << 20);
> -       piix4_io_quirk(dev, "PIIX4 devres J", 0x7c, 1 << 20);
> +       piix4_acpi_io_quirk(dev, "PIIX4 devres I", 0x78, 1 << 20);
> +       piix4_acpi_io_quirk(dev, "PIIX4 devres J", 0x7c, 1 << 20);
>  }
>  DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL,  PCI_DEVICE_ID_INTEL_82371AB_3,  quirk_piix4_acpi);
>  DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL,  PCI_DEVICE_ID_INTEL_82443MX_3,  quirk_piix4_acpi);
> --
> 1.7.1
>
>
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On 10/08/2013 04:14 PM, Bjorn Helgaas wrote:
> On Fri, Oct 4, 2013 at 5:30 PM, Deng-Cheng Zhu <dengcheng.zhu@imgtec.com> wrote:
>> From: Deng-Cheng Zhu <dengcheng.zhu@imgtec.com>
>>
>> Rename piix4_[io|mem]_quirk to piix4_acpi_[io|mem]_quirk because these 2
>> functions only deal with the function 3 (power management) of PIIX4.
>>
>> Signed-off-by: Deng-Cheng Zhu <dengcheng.zhu@imgtec.com>
>> Reviewed-by: James Hogan <james.hogan@imgtec.com>
> I don't really see the point in patches 2-4.  We end up with 50 more
> lines of code.  Most of this is new #defines.  They do add names where
> before we only had numbers, which *sounds* attractive, but it looks
> like each #define is used only once, and it's in chipset-dependent
> code where a careful reader has to compare every line with the spec
> anyway.  Adding a #define means you have to look at the definition,
> the use, and the spec.  Without the #define, you only have to look at
> the code and the spec.  I don't think it's worth it.

Sometimes (most of the time) reader doesn't want to dig into spec since the 
code is already there and the device is working. Reader may only want to 
know what the code is doing. The macros help (otherwise one can only know 
that pci_read_config_dword is reading a config register and then the output 
will be used to OR or AND). Variable names do give some hints, but macros 
give additional info. Yes, many of them are used only once, but they help a 
bit when used. Some examples are in the same file drivers/pci/quirks.c, 
such as ICH*. And you can easily find other examples by grepping, e.g. 
drivers/thermal/armada_thermal.c.

The above is about patch #4. About #2, it is an effort to make the code 
have more sense, as explained in the commit message. It IS an improvement, 
isn't it?

About #3, do you really think reusing the same logic by putting it in a 
function is worthless?


Deng-Cheng


--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html