linux-acpi.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Olof Johansson <olof@lixom.net>
To: John Garry <john.garry@huawei.com>
Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net>,
	Len Brown <lenb@kernel.org>,
	jeremy.linton@arm.com, Arnd Bergmann <arnd@arndb.de>,
	Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
	linux-acpi@vger.kernel.org, Hanjun Guo <guohanjun@huawei.com>,
	Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Subject: Re: [PATCH RFC 2/2] soc: Add a basic ACPI generic driver
Date: Tue, 28 Jan 2020 09:51:04 -0800	[thread overview]
Message-ID: <CAOesGMiCVSvL8H+haLoz=xyiX1CxBSRL_pbCgx-DLhN+5xRn9g@mail.gmail.com> (raw)
In-Reply-To: <1580210059-199540-3-git-send-email-john.garry@huawei.com>

Hi,

On Tue, Jan 28, 2020 at 3:18 AM John Garry <john.garry@huawei.com> wrote:
>
> Add a generic driver for platforms which populate their ACPI PPTT
> processor package ID Type Structure according to suggestion in the ACPI
> spec - see ACPI 6.2, section 5.2.29.3 ID structure Type 2.
>
> The soc_id is from member LEVEL_2_ID.
>
> For this, we need to use a whitelist of platforms which are known to
> populate the structure as suggested.
>
> For now, only the vendor and soc_id fields are exposed.
>
> Signed-off-by: John Garry <john.garry@huawei.com>
> ---
>  drivers/soc/Makefile       |   1 +
>  drivers/soc/acpi_generic.c | 102 +++++++++++++++++++++++++++++++++++++
>  2 files changed, 103 insertions(+)
>  create mode 100644 drivers/soc/acpi_generic.c
>
> diff --git a/drivers/soc/Makefile b/drivers/soc/Makefile
> index 8b49d782a1ab..2a59a30a22cd 100644
> --- a/drivers/soc/Makefile
> +++ b/drivers/soc/Makefile
> @@ -3,6 +3,7 @@
>  # Makefile for the Linux Kernel SOC specific device drivers.
>  #
>
> +obj-$(CONFIG_ACPI_PPTT)                += acpi_generic.o
>  obj-$(CONFIG_ARCH_ACTIONS)     += actions/
>  obj-$(CONFIG_SOC_ASPEED)       += aspeed/
>  obj-$(CONFIG_ARCH_AT91)                += atmel/

Based on everything I've seen so far, this should go under drivers/acpi instead.

> diff --git a/drivers/soc/acpi_generic.c b/drivers/soc/acpi_generic.c
> new file mode 100644
> index 000000000000..34a1f5f8e063
> --- /dev/null
> +++ b/drivers/soc/acpi_generic.c
> @@ -0,0 +1,102 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (c) John Garry, john.garry@huawei.com
> + */
> +
> +#define pr_fmt(fmt) "SOC ACPI GENERIC: " fmt
> +
> +#include <linux/acpi.h>
> +#include <linux/sys_soc.h>
> +
> +/*
> + * Known platforms that fill in PPTT package ID structures according to
> + * ACPI spec examples, that being:
> + * - Custom driver attribute is in ID Type Structure VENDOR_ID member
> + * - SoC id is in ID Type Structure LEVEL_2_ID member
> + *    See ACPI SPEC 6.2 Table 5-154 for PPTT ID Type Structure
> + */
> +static struct acpi_platform_list plat_list[] = {
> +       {"HISI  ", "HIP08   ", 0, ACPI_SIG_PPTT, all_versions},
> +       { } /* End */
> +};

As others have said, this will become a mess over time, and will
require changes for every new platform. Which, unfortunately, is
exactly what ACPI is supposed to provide relief from by making
standardized platforms... standardized.

> +
> +struct acpi_generic_soc_struct {
> +       struct soc_device_attribute dev_attr;
> +       u32 vendor;
> +};
> +
> +static ssize_t vendor_show(struct device *dev,
> +                          struct device_attribute *attr,
> +                          char *buf)
> +{
> +       struct acpi_generic_soc_struct *soc = dev_get_drvdata(dev);
> +       u8 vendor_id[5] = {};
> +
> +       *(u32 *)vendor_id = soc->vendor;
> +
> +       return sprintf(buf, "%s\n", vendor_id);
> +}
> +
> +static DEVICE_ATTR_RO(vendor);
> +
> +static __init int soc_acpi_generic_init(void)
> +{
> +       int index;
> +
> +       index = acpi_match_platform_list(plat_list);
> +       if (index < 0)
> +               return -ENOENT;
> +
> +       index = 0;
> +       while (true) {
> +               struct acpi_pptt_package_info info;
> +
> +               if (!acpi_pptt_get_package_info(index, &info)) {
> +                       struct soc_device_attribute *soc_dev_attr;
> +                       struct acpi_generic_soc_struct *soc;
> +                       struct soc_device *soc_dev;
> +                       u8 soc_id[9] = {};
> +
> +                       *(u64 *)soc_id = info.LEVEL_2_ID;
> +
> +                       soc = kzalloc(sizeof(*soc), GFP_KERNEL);
> +                       if (!soc)
> +                               return -ENOMEM;
> +
> +                       soc_dev_attr = &soc->dev_attr;
> +                       soc_dev_attr->soc_id = kasprintf(GFP_KERNEL, "%s",
> +                                                        soc_id);
> +                       if (!soc_dev_attr->soc_id) {
> +                               kfree(soc);
> +                               return -ENOMEM;
> +                       }
> +                       soc->vendor = info.vendor_id;
> +
> +                       soc_dev = soc_device_register(soc_dev_attr);
> +                       if (IS_ERR(soc_dev)) {
> +                               int ret = PTR_ERR(soc_dev);
> +
> +                               pr_info("could not register soc (%d) index=%d\n",
> +                                       ret, index);
> +                               kfree(soc_dev_attr->soc_id);
> +                               kfree(soc);
> +                               return ret;
> +                       }
> +                       dev_set_drvdata(soc_device_to_device(soc_dev), soc);
> +                       device_create_file(soc_device_to_device(soc_dev),
> +                                          &dev_attr_vendor);

Hmm, this doesn't look like much of a driver to me. This looks like
the export of an attribute to userspace, and should probably be done
by ACPI core instead of creating an empty driver for it.

This would also solve the whitelist issue -- always export this
property if it's set. If it's wrong, then the platform vendor needs to
fix it up. That's the approach that is used for other aspects of the
standardized platforms, right? We don't want to litter the kernel with
white/blacklists -- that's not a net improvement.


-Olof

  parent reply	other threads:[~2020-01-28 17:51 UTC|newest]

Thread overview: 30+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-01-28 11:14 [PATCH RFC 0/2] Add basic generic ACPI soc driver John Garry
2020-01-28 11:14 ` [PATCH RFC 1/2] ACPI/PPTT: Add acpi_pptt_get_package_info() API John Garry
2020-01-28 12:34   ` Sudeep Holla
2020-01-28 14:04     ` John Garry
2020-01-28 14:54       ` Sudeep Holla
2020-01-29 11:03         ` John Garry
2020-01-30 11:23     ` Sudeep Holla
2020-01-30 16:12       ` John Garry
2020-01-30 17:41         ` Sudeep Holla
2020-01-31 10:58           ` John Garry
2020-01-28 11:14 ` [PATCH RFC 2/2] soc: Add a basic ACPI generic driver John Garry
2020-01-28 11:56   ` Greg KH
2020-01-28 13:33     ` John Garry
2020-01-28 12:50   ` Arnd Bergmann
2020-01-28 14:46     ` John Garry
2020-01-28 15:20   ` Sudeep Holla
2020-01-28 15:59     ` John Garry
2020-01-28 16:17       ` Sudeep Holla
2020-01-28 17:51   ` Olof Johansson [this message]
2020-01-28 18:22     ` John Garry
2020-01-28 19:11       ` Rafael J. Wysocki
2020-01-28 19:28         ` John Garry
2020-01-28 22:30           ` Rafael J. Wysocki
2020-01-29 10:27             ` John Garry
2020-01-28 20:06       ` Olof Johansson
2020-01-29  9:58         ` John Garry
2020-01-28 16:56 ` [PATCH RFC 0/2] Add basic generic ACPI soc driver Jeremy Linton
2020-01-28 17:28   ` John Garry
2020-01-28 19:04     ` Jeremy Linton
2020-01-28 20:07       ` John Garry

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to='CAOesGMiCVSvL8H+haLoz=xyiX1CxBSRL_pbCgx-DLhN+5xRn9g@mail.gmail.com' \
    --to=olof@lixom.net \
    --cc=arnd@arndb.de \
    --cc=gregkh@linuxfoundation.org \
    --cc=guohanjun@huawei.com \
    --cc=jeremy.linton@arm.com \
    --cc=john.garry@huawei.com \
    --cc=lenb@kernel.org \
    --cc=linux-acpi@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=rjw@rjwysocki.net \
    --subject='Re: [PATCH RFC 2/2] soc: Add a basic ACPI generic driver' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).