Re: [PATCH RFC 2/2] soc: Add a basic ACPI generic driver

[Arnd Bergmann]

On Tue, Jan 28, 2020 at 12:18 PM John Garry <john.garry@xxxxxxxxxx> 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@xxxxxxxxxx>

Would it be possible to make this the root device for all on-chip devices
to correctly reflect the hierarchy inside of the soc?

> +/*
> + * 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 */
> +};

That matches a single machine, right? It doesn't seem very generic
that way.

> +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);
> +}

I'd rather not see nonstandard attributes in a "generic" driver at
all. Maybe the
you can simply concatenate the vendor and  LEVEL_2_ID into a single string
here?

> +                       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);

On the other hand, it would make sense to fill out additional fields here.
You have already matched the name of the board from the
acpi_platform_list, so there are two strings available that could be put
into the "machine" field, and it would make sense to fill out "family" with
something that identifies it as coming from ACPI PPTT data.

             Arnd