Re: [PATCH] ata: ahci_platform: Add ACPI support for AMD Seattle SATA controller

[Hans de Goede <hdegoede@redhat.com>]

Hi,

On 09/11/2014 10:17 PM, Olof Johansson wrote:
> On Tue, Sep 9, 2014 at 1:37 PM,  <suravee.suthikulpanit@amd.com> wrote:
>> From: Suravee Suthikulpanit <Suravee.Suthikulpanit@amd.com>
>>
>> This patch adds ACPI support for non-PCI SATA contoller in ahci_platform driver.
>> It adds ACPI matching table in ahci_platform to support AMD Seattle SATA controller
>> with following ASL structure in DSDT:
>>
>>     Device (SATA0)
>>     {
>>       Name(_HID, "AMDI0600")    // Seattle AHSATA
>>       Name (_CCA, 1)            // Cache-coherent controller
>>       Name (_CRS, ResourceTemplate ()
>>       {
>>         Memory32Fixed (ReadWrite, 0xE0300000, 0x00010000)
>>         Interrupt (ResourceConsumer, Level, ActiveHigh, Exclusive,,,) { 387 }
>>       })
>>     }
>>
>> Since ATA driver should not require PCI support for ATA_ACPI,
>> this patch also removes dependency in the driver/ata/Kconfig.
>>
>> Signed-off-by: Suravee Suthikulpanit <Suravee.Suthikulpanit@amd.com>
>> ---
>>  drivers/ata/Kconfig         |  2 +-
>>  drivers/ata/ahci_platform.c | 13 +++++++++++++
>>  2 files changed, 14 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/ata/Kconfig b/drivers/ata/Kconfig
>> index e1b9278..f2e6c9e 100644
>> --- a/drivers/ata/Kconfig
>> +++ b/drivers/ata/Kconfig
>> @@ -48,7 +48,7 @@ config ATA_VERBOSE_ERROR
>>
>>  config ATA_ACPI
>>         bool "ATA ACPI Support"
>> -       depends on ACPI && PCI
>> +       depends on ACPI
>>         default y
>>         help
>>           This option adds support for ATA-related ACPI objects.
>> diff --git a/drivers/ata/ahci_platform.c b/drivers/ata/ahci_platform.c
>> index f61ddb9..3499bab 100644
>> --- a/drivers/ata/ahci_platform.c
>> +++ b/drivers/ata/ahci_platform.c
>> @@ -20,6 +20,9 @@
>>  #include <linux/platform_device.h>
>>  #include <linux/libata.h>
>>  #include <linux/ahci_platform.h>
>> +#ifdef CONFIG_ATA_ACPI
>> +#include <linux/acpi.h>
>> +#endif
>>  #include "ahci.h"
>>
>>  static const struct ata_port_info ahci_port_info = {
>> @@ -87,6 +90,13 @@ static const struct of_device_id ahci_of_match[] = {
>>  };
>>  MODULE_DEVICE_TABLE(of, ahci_of_match);
>>
>> +#ifdef CONFIG_ATA_ACPI
>> +static const struct acpi_device_id ahci_acpi_match[] = {
>> +       { "AMDI0600", 0 }, /* AMD Seattle AHCI */
>> +       { },
>> +};
>> +#endif
>> +
>>  static struct platform_driver ahci_driver = {
>>         .probe = ahci_probe,
>>         .remove = ata_platform_remove_one,
>> @@ -94,6 +104,9 @@ static struct platform_driver ahci_driver = {
>>                 .name = "ahci",
>>                 .owner = THIS_MODULE,
>>                 .of_match_table = ahci_of_match,
>> +#ifdef CONFIG_ATA_ACPI
>> +               .acpi_match_table = ACPI_PTR(ahci_acpi_match),
>> +#endif
> 
> The whole point of having ACPI_PTR() is to avoid having ifdefs like
> these. The structure member is always there, so there's no need to
> have it conditionally compiled out.
> 
> Come to think of it, the match table should be under CONFIG_ACPI
> instead -- why do a separate config option just for this?

To clarify this for people like me who were under the impression
that ACPI_PTR() does some kind of cast, this is the definition
of ACPI_PTR:

#ifdef  CONFIG_ACPI
#define ACPI_PTR(_ptr)  (_ptr)
#else
#define ACPI_PTR(_ptr)  (NULL)
#endif

Regards,

Hans


> 
> 
> -Olof
>