Re: [PATCH v3 2/2] kernel: Add reset button platform device for APU2/APU3

[Andy Shevchenko <andy.shevchenko@xxxxxxxxx>]

On Wed, Nov 14, 2018 at 9:27 AM Florian Eckert <fe@xxxxxxxxxx> wrote:
>
> This will add a x86 platform device "gpio-keys-polled" which uses the
> new gpio-apu drive for APU2 and APU3 boards from PC Engines.

Subject more likely should start from x86/platform.

On the other hand, why it's under arch/x86 and not part of PDx86
(drivers/platform/x86)?

>
> Signed-off-by: Florian Eckert <fe@xxxxxxxxxx>
> ---
>  arch/x86/Kconfig               | 14 ++++++++
>  arch/x86/platform/Makefile     |  1 +
>  arch/x86/platform/amd/Makefile |  1 +
>  arch/x86/platform/amd/apu.c    | 72 ++++++++++++++++++++++++++++++++++++++++++
>  4 files changed, 88 insertions(+)
>  create mode 100644 arch/x86/platform/amd/Makefile
>  create mode 100644 arch/x86/platform/amd/apu.c
>
> diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
> index 9d734f3c8234..97c53286fdb6 100644
> --- a/arch/x86/Kconfig
> +++ b/arch/x86/Kconfig
> @@ -2820,6 +2820,20 @@ config TS5500
>
>  endif # X86_32
>
> +if X86_64
> +config APU
> +       bool "PCEngines APU System Support"
> +       help
> +         This option enables system support for the PCEngines APU platform.
> +         At present this just sets up the reset button control on
> +         APU2/APU3 boards. However, other system specific setup should
> +         get added here.
> +
> +         Note: You must still enable the drivers for GPIO and LED support
> +         (GPIO_APU & LEDS_APU) to actually use the LEDs and the GPIOs
> +
> +endif # X86_64
> +
>  config AMD_NB
>         def_bool y
>         depends on CPU_SUP_AMD && PCI
> diff --git a/arch/x86/platform/Makefile b/arch/x86/platform/Makefile
> index d0e835470d01..a95d18810c29 100644
> --- a/arch/x86/platform/Makefile
> +++ b/arch/x86/platform/Makefile
> @@ -1,6 +1,7 @@
>  # SPDX-License-Identifier: GPL-2.0
>  # Platform specific code goes here
>  obj-y  += atom/
> +obj-y  += amd/
>  obj-y  += ce4100/
>  obj-y  += efi/
>  obj-y  += geode/
> diff --git a/arch/x86/platform/amd/Makefile b/arch/x86/platform/amd/Makefile
> new file mode 100644
> index 000000000000..bf04c5799d7f
> --- /dev/null
> +++ b/arch/x86/platform/amd/Makefile
> @@ -0,0 +1 @@
> +obj-$(CONFIG_APU)              +=apu.o
> diff --git a/arch/x86/platform/amd/apu.c b/arch/x86/platform/amd/apu.c
> new file mode 100644
> index 000000000000..a4b695881177
> --- /dev/null
> +++ b/arch/x86/platform/amd/apu.c
> @@ -0,0 +1,72 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * System Specific setup for PC-Engines APU2/APU3 devices
> + *
> + * Copyright (C) 2018 Florian Eckert <fe@xxxxxxxxxx>
> + */
> +
> +#include <linux/platform_device.h>

> +#include <linux/gpio.h>

Mustn't be present in new code. Either consumer or driver (in very
rare cases both, not here)

> +#include <linux/input.h>
> +#include <linux/gpio_keys.h>
> +#include <linux/dmi.h>
> +
> +static struct gpio_keys_button apu_gpio_buttons[] = {
> +       {
> +               .code                   = KEY_RESTART,
> +               .gpio                   = 20,
> +               .active_low             = 1,
> +               .desc                   = "Reset button",
> +               .type                   = EV_KEY,
> +               .debounce_interval      = 60,
> +       }
> +};
> +
> +static struct gpio_keys_platform_data apu_buttons_data = {
> +       .buttons                = apu_gpio_buttons,
> +       .nbuttons               = ARRAY_SIZE(apu_gpio_buttons),
> +       .poll_interval          = 20,
> +};
> +
> +static struct platform_device apu_buttons_dev = {
> +       .name                   = "gpio-keys-polled",
> +       .id                     = 1,
> +       .dev = {
> +               .platform_data          = &apu_buttons_data,
> +       }
> +};
> +
> +static struct platform_device *apu_devs[] __initdata = {
> +               &apu_buttons_dev,
> +};
> +
> +static void __init register_apu(void)
> +{
> +       /* Setup push button control through gpio-apu driver */
> +       platform_add_devices(apu_devs, ARRAY_SIZE(apu_devs));
> +}
> +
> +static int __init apu_init(void)
> +{

> +       if (!dmi_match(DMI_SYS_VENDOR, "PC Engines")) {
> +               pr_err("No PC Engines board detected\n");
> +               return -ENODEV;
> +       }
> +
> +       if (!(dmi_match(DMI_PRODUCT_NAME, "APU2") ||
> +             dmi_match(DMI_PRODUCT_NAME, "apu2") ||
> +             dmi_match(DMI_PRODUCT_NAME, "PC Engines apu2") ||
> +             dmi_match(DMI_PRODUCT_NAME, "APU3") ||
> +             dmi_match(DMI_PRODUCT_NAME, "apu3") ||
> +             dmi_match(DMI_PRODUCT_NAME, "PC Engines apu3"))) {
> +               pr_err("Unknown PC Engines board: %s\n",
> +                       dmi_get_system_info(DMI_PRODUCT_NAME));
> +               return -ENODEV;
> +       }

Do we really need this here?
If so, perhaps a common code should be created in a way like
x86_apple_machine global variable?

> +
> +       register_apu();
> +
> +       return 0;
> +}
> +
> +device_initcall(apu_init);
> --
> 2.11.0
>


-- 
With Best Regards,
Andy Shevchenko