Hi, On Wed, Dec 9, 2015 at 10:14 PM, Ken Xue <ken.xue@xxxxxxx> 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@xxxxxxx> 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@xxxxxxx> >> > > --- >> > > 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 -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html