Re: [PATCH V3] acpi:apd:add AMD ACPI2Platform device support for x86 system.

[Ken Xue]

On Tue, 2015-02-03 at 23:07 +0100, Rafael J. Wysocki wrote:
> On Tuesday, February 03, 2015 12:06:06 PM Andy Shevchenko wrote:
> > On Tue, Feb 3, 2015 at 3:54 AM, Ken Xue <Ken.Xue@xxxxxxx> wrote:
> > > This new feature is to interpret AMD specific ACPI device to
> > > platform device such as I2C, UART found on AMD CZ and later chipsets. It
> > > based on example intel LPSS. Now, it can support AMD I2C & UART.
> > >
> > 
> > Few minor things below.
> > Be free to fix in depend on what Mika and Rafael confirm.
> > 
> > Anyway,
> > Reviewed-by: Andy Shevchenko <andy.shevchenko@xxxxxxxxx>
> 
> Thanks Andy!
> 
> Ken, please address the Andy's comments given below, they all make sense to me.
> 
Thanks for all your comments.

> > > Signed-off-by: Ken Xue <Ken.Xue@xxxxxxx>
> > > ---
> > >  arch/x86/Kconfig        |  11 +++
> > >  drivers/acpi/Makefile   |   2 +-
> > >  drivers/acpi/acpi_apd.c | 201 ++++++++++++++++++++++++++++++++++++++++++++++++
> > >  drivers/acpi/internal.h |   2 +
> > >  drivers/acpi/scan.c     |   1 +
> > >  5 files changed, 216 insertions(+), 1 deletion(-)
> > >  create mode 100644 drivers/acpi/acpi_apd.c
> > >
> > > diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
> > > index 0dc9d01..ddf8d42 100644
> > > --- a/arch/x86/Kconfig
> > > +++ b/arch/x86/Kconfig
> > > @@ -496,6 +496,17 @@ config X86_INTEL_LPSS
> > >           things like clock tree (common clock framework) and pincontrol
> > >           which are needed by the LPSS peripheral drivers.
> > >
> > > +config X86_AMD_PLATFORM_DEVICE
> > > +       bool "AMD ACPI2Platform devices support"
> > > +       depends on ACPI
> > > +       select COMMON_CLK
> > > +       select PINCTRL
> > > +       ---help---
> > > +         Select to interpret AMD specific ACPI device to platform device
> > > +         such as I2C, UART found on AMD Carrizo and later chipsets. Selecting
> > > +         this option enables things like clock tree (common clock framework)
> > > +         and pinctrl.
> > 
> > Sounds 'like' is redundant here. It explicitly enables common clock
> > and pin control frameworks.
> > 
I just copy the last sentence from description of X86_INTEL_LPSS. but it
is OK, i will remove it from next release.

> > > +
> > >  config IOSF_MBI
> > >         tristate "Intel SoC IOSF Sideband support for SoC platforms"
> > >         depends on PCI
> > > diff --git a/drivers/acpi/Makefile b/drivers/acpi/Makefile
> > > index f74317c..0071141 100644
> > > --- a/drivers/acpi/Makefile
> > > +++ b/drivers/acpi/Makefile
> > > @@ -40,7 +40,7 @@ acpi-$(CONFIG_ARCH_MIGHT_HAVE_ACPI_PDC) += processor_pdc.o
> > >  acpi-y                         += ec.o
> > >  acpi-$(CONFIG_ACPI_DOCK)       += dock.o
> > >  acpi-y                         += pci_root.o pci_link.o pci_irq.o
> > > -acpi-y                         += acpi_lpss.o
> > > +acpi-y                         += acpi_lpss.o acpi_apd.o
> > >  acpi-y                         += acpi_platform.o
> > >  acpi-y                         += acpi_pnp.o
> > >  acpi-y                         += int340x_thermal.o
> > > diff --git a/drivers/acpi/acpi_apd.c b/drivers/acpi/acpi_apd.c
> > > new file mode 100644
> > > index 0000000..b27bc1c
> > > --- /dev/null
> > > +++ b/drivers/acpi/acpi_apd.c
> > > @@ -0,0 +1,201 @@
> > > +/*
> > > + * AMD ACPI support for ACPI2platform device.
> > > + *
> > > + * Copyright (c) 2014,2015 AMD Corporation.
> > > + * Authors: Ken Xue <Ken.Xue@xxxxxxx>
> > > + *     Wu, Jeff <Jeff.Wu@xxxxxxx>
> > > + *
> > > + * This program is free software; you can redistribute it and/or modify
> > > + * it under the terms of the GNU General Public License version 2 as
> > > + * published by the Free Software Foundation.
> > > + */
> > > +
> > > +#include <linux/clk-provider.h>
> > > +#include <linux/platform_device.h>
> > > +#include <linux/pm_domain.h>
> > > +#include <linux/clkdev.h>
> > > +#include <linux/acpi.h>
> > > +#include <linux/err.h>
> > > +#include <linux/clk.h>
> > > +#include <linux/pm.h>
> > > +
> > > +#include "internal.h"
> > > +
> > > +ACPI_MODULE_NAME("acpi_apd");
> > > +struct apd_private_data;
> > > +
> > > +/**
> > 
> > /*, or use description on per flag basis.
> > 
OK.

> > > + * device flags of acpi_apd_dev_desc.
> > > + * ACPI_APD_SYSFS : add device attributes in sysfs
> > > + * ACPI_APD_PM : attach power domain to device
> > > + * ACPI_APD_PM_ON : power on device when attach power domain
> > > + */
> > > +#define ACPI_APD_SYSFS BIT(0)
> > > +#define ACPI_APD_PM    BIT(1)
> > > +#define ACPI_APD_PM_ON BIT(2)
> > > +
> > > +/**
> > > + * struct apd_device_desc - a descriptor for apd device
> > > + * @flags: device flags like ACPI_APD_SYSFS ACPI_APD_PM ACPI_APD_PM_ON
> > 
> > %ACPI_APD_SYSFS, %ACPI_APD_PM, %ACPI_APD_APM_ON.
> > 
> > Could it be combination? State this explicitly.
> > 
OK.

> > > + * @fixed_clk_rate: fixed rate input clock source for acpi device;
> > > + *                     0 means no fixed rate input clock source
> > > + * @setup: a hook routine to set device resource during create platform device
> > > + *
> > > + * device description defined as acpi_device_id.driver_data
> > 
> > d -> D
> > 
OK.

> > > + */
> > > +struct apd_device_desc {
> > > +       unsigned int flags;
> > > +       unsigned int fixed_clk_rate;
> > > +       int (*setup)(struct apd_private_data *pdata);
> > > +};
> > > +
> > > +struct apd_private_data {
> > > +       struct clk *clk;
> > > +       struct acpi_device *adev;
> > > +       const struct apd_device_desc *dev_desc;
> > > +};
> > > +
> > > +#ifdef CONFIG_X86_AMD_PLATFORM_DEVICE
> > > +#define APD_ADDR(desc) ((unsigned long)&desc)
> > > +
> > > +static int acpi_apd_setup(struct apd_private_data *pdata)
> > > +{
> > > +       const struct apd_device_desc *dev_desc = pdata->dev_desc;
> > > +       struct clk *clk = ERR_PTR(-ENODEV);
> > > +
> > > +       if (dev_desc->fixed_clk_rate) {
> > > +               clk = clk_register_fixed_rate(&pdata->adev->dev,
> > > +                                       dev_name(&pdata->adev->dev),
> > > +                                       NULL, CLK_IS_ROOT,
> > > +                                       dev_desc->fixed_clk_rate);
> > > +               clk_register_clkdev(clk, NULL, dev_name(&pdata->adev->dev));
> > > +               pdata->clk = clk;
> > > +       }
> > 
> > What about
> > if (!âclk_rate)
> >  return 0;
> > â
OK.

> > return 0; ?
> > 
> > And question why we need int to be returned at all?
> >
It just provides a possibility to return error. In current use case,
there is no need to return. But i believe it is better to reserve this
design.

>  
> > > +
> > > +       return 0;
> > > +}
> > > +
> > > +static struct apd_device_desc cz_i2c_desc = {
> > > +       .setup = acpi_apd_setup,
> > > +       .fixed_clk_rate = 133000000,
> > > +};
> > > +
> > > +static struct apd_device_desc cz_uart_desc = {
> > > +       .setup = acpi_apd_setup,
> > > +       .fixed_clk_rate = 48000000,
> > > +};
> > > +
> > > +#else
> > > +
> > > +#define APD_ADDR(desc) (0UL)
> > > +
> > > +#endif /* CONFIG_X86_AMD_PLATFORM_DEVICE */
> > > +
> > > +static int acpi_apd_create_device(struct acpi_device *adev,
> > > +                                  const struct acpi_device_id *id)
> > > +{
> > > +       const struct apd_device_desc *dev_desc;
> > > +       struct apd_private_data *pdata;
> > > +       struct platform_device *pdev;
> > > +       int ret;
> > > +
> > > +       dev_desc = (struct apd_device_desc *)id->driver_data;
> > 
> > You may move this assignment to the definition block. And if it
> > doesn't fit one line you may cast to (void *), though I don't know if
> > Rafael likes such trick.
> > 
OK.

> > > +       if (!dev_desc) {
> > > +               pdev = acpi_create_platform_device(adev);
> > > +               return IS_ERR_OR_NULL(pdev) ? PTR_ERR(pdev) : 1;
> > 
> > I guess it worth to add a comment what means return code here.
> > 
OK.

> > > +       }
> > > +       pdata = kzalloc(sizeof(*pdata), GFP_KERNEL);
> > > +       if (!pdata)
> > > +               return -ENOMEM;
> > > +
> > > +       pdata->adev = adev;
> > > +       pdata->dev_desc = dev_desc;
> > > +
> > > +       if (dev_desc->setup) {
> > > +               ret = dev_desc->setup(pdata);
> > > +               if (ret)
> > > +                       goto err_out;
> > > +       }
> > > +
> > > +       adev->driver_data = pdata;
> > > +       pdev = acpi_create_platform_device(adev);
> > > +       if (!IS_ERR_OR_NULL(pdev))
> > > +               return 1;
> > > +
> > > +       ret = PTR_ERR(pdev);
> > > +       adev->driver_data = NULL;
> > > +
> > > + err_out:
> > > +       kfree(pdata);
> > > +       return ret;
> > > +}
> > > +
> > > +static const struct acpi_device_id acpi_apd_device_ids[] = {
> > > +       /* Generic apd devices */
> > > +       { "AMD0010", APD_ADDR(cz_i2c_desc) },
> > > +       { "AMD0020", APD_ADDR(cz_uart_desc) },
> > > +       { }
> > > +};
> > > +
> > > +#ifdef CONFIG_X86_AMD_PLATFORM_DEVICE
> > > +static int acpi_apd_platform_notify(struct notifier_block *nb,
> > > +                                    unsigned long action, void *data)
> > > +{
> > > +       struct platform_device *pdev = to_platform_device(data);
> > > +       const struct acpi_device_id *id = NULL;
> > 
> > Redundant assignment.
> > 
OK.

> > > +       struct apd_private_data *pdata;
> > > +       struct acpi_device *adev;
> > > +       bool power_on;
> > > +       int ret;
> > > +
> > > +       id = acpi_match_device(acpi_apd_device_ids, &pdev->dev);
> > > +       if (!id || !id->driver_data)
> > > +               return 0;
> > > +
> > > +       if (acpi_bus_get_device(ACPI_HANDLE(&pdev->dev), &adev))
> > 
> > Better of course to do like
> > status = â
> > if (status) [don't remember how to check status with ACPI API]
> >  return 0;
> > 
I use same coding style as LPSS and other examples in current kernel.
I don't think it is really need to be changed.

> > > +               return 0;
> > > +
> > > +       pdata = acpi_driver_data(adev);
> > > +       if (!pdata || !pdata->dev_desc)
> > > +               return 0;
> > > +
> > > +       switch (action) {
> > > +       case BUS_NOTIFY_ADD_DEVICE:
> > > +               if (pdata->dev_desc->flags & ACPI_APD_PM) {
> > > +                       power_on = !!(pdata->dev_desc->flags & ACPI_APD_PM_ON);
> > > +                       ret = dev_pm_domain_attach(&pdev->dev, power_on);
> > > +
> > > +                       if (ret)
> > > +                               dev_dbg(&pdev->dev,
> > > +                               "failed to attached pm domain (%d)\n", ret);
> > > +               }
> > > +               break;
> > > +       case BUS_NOTIFY_DEL_DEVICE:
> > > +               if (pdata->dev_desc->flags & ACPI_APD_PM) {
> > > +                       power_on = !!(pdata->dev_desc->flags & ACPI_APD_PM_ON);
> > > +                       dev_pm_domain_detach(&pdev->dev, power_on);
> > > +               }
> > > +               break;
> > > +       }
> > > +
> > > +       return 0;
> > > +}
> > > +
> > > +static struct notifier_block acpi_apd_nb = {
> > > +       .notifier_call = acpi_apd_platform_notify,
> > > +};
> > > +#endif
> > > +
> > > +static struct acpi_scan_handler apd_handler = {
> > > +       .ids = acpi_apd_device_ids,
> > > +       .attach = acpi_apd_create_device,
> > > +};
> > > +
> > > +void __init acpi_apd_init(void)
> > > +{
> > > +#ifdef CONFIG_X86_AMD_PLATFORM_DEVICE
> > > +       bus_register_notifier(&platform_bus_type, &acpi_apd_nb);
> > > +#endif
> > > +
> > > +       acpi_scan_add_handler(&apd_handler);
> > > +}
> > > diff --git a/drivers/acpi/internal.h b/drivers/acpi/internal.h
> > > index 163e82f..c24ae9d 100644
> > > --- a/drivers/acpi/internal.h
> > > +++ b/drivers/acpi/internal.h
> > > @@ -68,6 +68,8 @@ static inline void acpi_debugfs_init(void) { return; }
> > >  #endif
> > >  void acpi_lpss_init(void);
> > >
> > > +void acpi_apd_init(void);
> > > +
> > >  acpi_status acpi_hotplug_schedule(struct acpi_device *adev, u32 src);
> > >  bool acpi_queue_hotplug_work(struct work_struct *work);
> > >  void acpi_device_hotplug(struct acpi_device *adev, u32 src);
> > > diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c
> > > index dc4d896..bbca783 100644
> > > --- a/drivers/acpi/scan.c
> > > +++ b/drivers/acpi/scan.c
> > > @@ -2544,6 +2544,7 @@ int __init acpi_scan_init(void)
> > >         acpi_pci_link_init();
> > >         acpi_processor_init();
> > >         acpi_lpss_init();
> > > +       acpi_apd_init();
> > >         acpi_cmos_rtc_init();
> > >         acpi_container_init();
> > >         acpi_memory_hotplug_init();
> > > --
> > > 1.9.1
> > >
> > >
> > >
> > 
> > 
> > 
> > 
> 


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/