Re: [PATCH 1/5] mfd: Add support for IO functions of AAEON devices

[Krzysztof Kozlowski <krzk@xxxxxxxxxx>]

On Tue, 25 May 2021 at 01:42, <aaeon.asus@xxxxxxxxx> wrote:
>
> From: Kunyang_Fan <kunyang_fan@xxxxxxxx>
>
> This adds the supports for multiple IO functions of the
> AAEON x86 devices and makes use of the WMI interface to
> control the these IO devices including:
>
> - GPIO
> - LED
> - Watchdog
> - HWMON
>
> It also adds the mfd child device drivers to support
> the above IO functions.
>
> Signed-off-by: Kunyang_Fan <kunyang_fan@xxxxxxxx>
> ---
>  MAINTAINERS             | 12 +++++++
>  drivers/mfd/Kconfig     | 12 +++++++
>  drivers/mfd/Makefile    |  1 +
>  drivers/mfd/mfd-aaeon.c | 77 +++++++++++++++++++++++++++++++++++++++++

Please CC all required maintainers - you skipped several people. You
will get their list from scripts/get_maintainer.pl.

>  4 files changed, 102 insertions(+)
>  create mode 100644 drivers/mfd/mfd-aaeon.c
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 1c19c1e2c970..49783dd44367 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -257,6 +257,18 @@ W: http://www.adaptec.com/
>  F:     Documentation/scsi/aacraid.rst
>  F:     drivers/scsi/aacraid/
>
> +AAEON DEVICE DRIVER WITH WMI INTERFACE
> +M:     Edward Lin<edward1_lin@xxxxxxxx>

Missing space.

> +M:     Kunyang Fan <kunyang_fan@xxxxxxxx>
> +M:     Frank Hsieh <frank2_hsieh@xxxxxxxx>
> +M:     Jacob Wu <jacob_wu@xxxxxxxx>

Why do you need four maintainers? Did they contribute to the code? Are
they all going to provide reviews?

> +S:     Supported
> +F:     drivers/gpio/gpio-aaeon.c
> +F:     drivers/hwmon/hwmon-aaeon.c
> +F:     drivers/leds/leds-aaeon.c
> +F:     drivers/mfd/mfd-aaeon.c
> +F:     drivers/watchdog/wdt_aaeon.c
> +
>  ABI/API
>  L:     linux-api@xxxxxxxxxxxxxxx
>  F:     include/linux/syscalls.h
> diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
> index a37d7d171382..f172564eed0d 100644
> --- a/drivers/mfd/Kconfig
> +++ b/drivers/mfd/Kconfig
> @@ -2053,6 +2053,18 @@ config MFD_WCD934X
>           This driver provides common support WCD934x audio codec and its
>           associated Pin Controller, Soundwire Controller and Audio codec.
>
> +

No double blank lines.

> +config MFD_AAEON
> +       tristate "AAEON WMI MFD devices"
> +       depends on ASUS_WMI
> +       help
> +         Say yes here to support mltiple IO devices on Single Board Computers

Please run spellcheck on entire submission.

> +         produced by AAEON.
> +
> +         This driver leverages the ASUS WMI interface to access device
> +         resources.
> +
> +
>  menu "Multimedia Capabilities Port drivers"
>         depends on ARCH_SA1100
>
> diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
> index 9367a92f795a..36fff3d0da7e 100644
> --- a/drivers/mfd/Makefile
> +++ b/drivers/mfd/Makefile
> @@ -264,3 +264,4 @@ obj-$(CONFIG_MFD_ROHM_BD718XX)      += rohm-bd718x7.o
>  obj-$(CONFIG_MFD_STMFX)        += stmfx.o
>
>  obj-$(CONFIG_SGI_MFD_IOC3)     += ioc3.o
> +obj-$(CONFIG_MFD_AAEON)                += mfd-aaeon.o
> diff --git a/drivers/mfd/mfd-aaeon.c b/drivers/mfd/mfd-aaeon.c
> new file mode 100644
> index 000000000000..9d2efde53cad
> --- /dev/null
> +++ b/drivers/mfd/mfd-aaeon.c
> @@ -0,0 +1,77 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * UP Board main platform driver and FPGA configuration support
> + *
> + * Copyright (c) 2021, AAEON Ltd.
> + *
> + * Author: Kunyang_Fan <knuyang_fan@xxxxxxxxxxxx>
> + *
> + * 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.

No need for GPL text since you use SPDX.

> + */
> +
> +#include <linux/acpi.h>
> +#include <linux/gpio.h>
> +#include <linux/kernel.h>
> +#include <linux/mfd/core.h>
> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +#include <linux/leds.h>
> +#include <linux/wmi.h>
> +
> +#define AAEON_WMI_MGMT_GUID      "97845ED0-4E6D-11DE-8A39-0800200C9A66"
> +
> +struct aaeon_wmi_priv {
> +       const struct mfd_cell *cells;
> +       size_t ncells;
> +};
> +
> +static const struct mfd_cell aaeon_mfd_cells[] = {
> +       { .name = "gpio-aaeon" },
> +       { .name = "hwmon-aaeon"},
> +       { .name = "leds-aaeon"},
> +       { .name = "wdt-aaeon"},
> +};
> +
> +static const struct aaeon_wmi_priv aaeon_wmi_priv_data = {
> +       .cells = aaeon_mfd_cells,
> +       .ncells = ARRAY_SIZE(aaeon_mfd_cells),
> +};
> +
> +static int aaeon_wmi_probe(struct wmi_device *wdev, const void *context)
> +{
> +       struct aaeon_wmi_priv *priv;
> +
> +       if (!wmi_has_guid(AAEON_WMI_MGMT_GUID)) {
> +               dev_info(&wdev->dev, "AAEON Management GUID not found\n");
> +               return -ENODEV;
> +       }
> +
> +

No double blank lines.

> +       priv = (struct aaeon_wmi_priv *)context;
> +       dev_set_drvdata(&wdev->dev, priv);

No, why do you need to store device ID table as private data?

Best regards,
Krzysztof