[PATCH 1/2] acpi:apd: Add APM X-Gene ACPI I2C device support

[Loc Ho]

Hi,

On Wed, Dec 9, 2015 at 10:14 PM, Ken Xue <ken.xue at amd.com> wrote:
> On Wed, 2015-12-09 at 14:03 -0800, Loc Ho wrote:
>> Hi Ken,
>>
>> On Mon, Dec 7, 2015 at 6:49 PM, Ken Xue <ken.xue at amd.com> wrote:
>> >
>> > On Mon, 2015-12-07 at 17:16 -0700, Loc Ho wrote:
>> > > Add APM X-Gene ACPI I2C device support by hooks into existent
>> > > ACPI apd driver. To fully enable support, require another
>> > > patch to add the X-Gene ACPI node into the DW I2C driver.
>> > >
>> > > Signed-off-by: Loc Ho <lho at apm.com>
>> > > ---
>> > >  drivers/acpi/acpi_apd.c |    8 +++++++-
>> > >  1 files changed, 7 insertions(+), 1 deletions(-)
>> > >
>> > > diff --git a/drivers/acpi/acpi_apd.c b/drivers/acpi/acpi_apd.c
>> > > index a450e7a..6a9cb8d 100644
>> > > --- a/drivers/acpi/acpi_apd.c
>> > > +++ b/drivers/acpi/acpi_apd.c
>> > > @@ -51,7 +51,7 @@ struct apd_private_data {
>> > >       const struct apd_device_desc *dev_desc;
>> > >  };
>> > >
>> > > -#ifdef CONFIG_X86_AMD_PLATFORM_DEVICE
>> > > +#if defined(CONFIG_X86_AMD_PLATFORM_DEVICE) || defined(CONFIG_ARM64)
>> > >  #define APD_ADDR(desc)       ((unsigned long)&desc)
>> > >
>> > >  static int acpi_apd_setup(struct apd_private_data *pdata)
>> > > @@ -76,6 +76,11 @@ static struct apd_device_desc cz_i2c_desc = {
>> > >       .fixed_clk_rate = 133000000,
>> > >  };
>> > >
>> > > +static struct apd_device_desc xgene_i2c_desc = {
>> > > +     .setup = acpi_apd_setup,
>> > > +     .fixed_clk_rate = 100000000,
>> > > +};
>> > > +
>> > >  static struct apd_device_desc cz_uart_desc = {
>> > >       .setup = acpi_apd_setup,
>> > >       .fixed_clk_rate = 48000000,
>> > > @@ -135,6 +140,7 @@ static const struct acpi_device_id acpi_apd_device_ids[] = {
>> > >       { "AMD0010", APD_ADDR(cz_i2c_desc) },
>> > >       { "AMD0020", APD_ADDR(cz_uart_desc) },
>> > >       { "AMD0030", },
>> > > +     { "APMC0D0F", APD_ADDR(xgene_i2c_desc) },
>> > It is better to split AMD devices and ARM devices with macros:
>> > CONFIG_X86_AMD_PLATFORM_DEVICE and CONFIG_ARM64.
>>
>> This gets pretty ugly to me with define as it would look like this:
>>
>> #if defined(CONFIG_X86_AMD_PLATFORM_DEVICE) || defined(CONFIG_ARM64)
>> #define APD_ADDR(desc) ((unsigned long)&desc)
>>
>> static int acpi_apd_setup(struct apd_private_data *pdata)
>> {
>>    .....
>> }
>>
>> #ifdef CONFIG_X86_AMD_PLATFORM_DEVICE
>> static struct apd_device_desc cz_i2c_desc = {
>>     ....
>> };
>>
>> static struct apd_device_desc xgene_i2c_desc = {
>>     ...
>> };
>> #endif
>>
>> #ifdef CONFIG_ARM64
>> static struct apd_device_desc cz_uart_desc = {
>>     ....
>> };
>> #endif
>>
>> #else
>>
>> #define APD_ADDR(desc) (0UL)
>>
>> #endif /* CONFIG_X86_AMD_PLATFORM_DEVICE */
>>
>> And...
>>
>> #ifdef CONFIG_X86_AMD_PLATFORM_DEVICE
>> { "AMD0010", APD_ADDR(cz_i2c_desc) },
>> { "AMD0020", APD_ADDR(cz_uart_desc) },
>> { "AMD0030", },
>> #endif
>> #ifdef CONFIG_ARM64
>> { "APMC0D0F", APD_ADDR(xgene_i2c_desc) },
>> #endif
>>
>> Sure you want this?
> Yes. Even though it may look like too much macros for just several
> devices now. But I think AMD and other ARM socs may also try to leverage
> APD for more and more ACPI devices.
> It is a good direction that 1)improve efficiency of matching ACPI
> handler 2) split devices and potential hook routines into different
> classes clearly
>
> It also will be more convenient to move ARM devices out of APD if there
> is a new design for ARM ACPI device.
>

Okay... I will generate v2 when ready. One more question, does AMD
ARM64 SoC need it later?

-Loc