Re: [PATCH 4/5] ACPI / property: Allow holes in reference properties

[Rafael J. Wysocki]

On Fri, Sep 23, 2016 at 4:57 PM, Mika Westerberg
<mika.westerberg@xxxxxxxxxxxxxxx> wrote:
> DT allows holes or empty phandles for references. This is used for example
> in SPI subsystem where some chip selects are native and others are regular
> GPIOs. In ACPI _DSD we currently do not support this but instead the
> preceding reference "consumes" all following integer arguments.
>
> For example we would like to support something like the below ASL fragment
> for SPI:
>
>   Package () {
>       "cs-gpios",
>       Package () {
>           ^GPIO, 19, 0, 0, // GPIO CS0
>           0,               // Native CS
>           ^GPIO, 20, 0, 0, // GPIO CS1
>       }
>   }
>
> The zero in the middle means "no entry" or NULL reference. To support this
> we add a new function acpi_node_get_property_reference_fixed_args() that
> takes number of expected arguments as parameter. Function returns -ENOENT
> if the corresponding index resolves to the "no entry" reference.
>
> Signed-off-by: Mika Westerberg <mika.westerberg@xxxxxxxxxxxxxxx>
> ---
>  drivers/acpi/property.c | 116 +++++++++++++++++++++++++++++++++++-------------
>  include/linux/acpi.h    |   3 ++
>  2 files changed, 89 insertions(+), 30 deletions(-)
>
> diff --git a/drivers/acpi/property.c b/drivers/acpi/property.c
> index f2fd3fee588a..b255c4442a05 100644
> --- a/drivers/acpi/property.c
> +++ b/drivers/acpi/property.c
> @@ -472,6 +472,7 @@ static int acpi_data_get_property_array(struct acpi_device_data *data,
>   * @data: ACPI device data object containing the property
>   * @propname: Name of the property
>   * @index: Index of the reference to return
> + * @num_args: Maximum number of arguments after each reference
>   * @args: Location to store the returned reference with optional arguments
>   *
>   * Find property with @name, verifify that it is a package containing at least
> @@ -485,8 +486,8 @@ static int acpi_data_get_property_array(struct acpi_device_data *data,
>   * Return: %0 on success, negative error code on failure.
>   */
>  static int acpi_data_get_property_reference(struct acpi_device_data *data,
> -                                           const char *propname, size_t index,
> -                                           struct acpi_reference_args *args)
> +       const char *propname, size_t index, size_t num_args,
> +       struct acpi_reference_args *args)
>  {
>         const union acpi_object *element, *end;
>         const union acpi_object *obj;
> @@ -532,41 +533,52 @@ static int acpi_data_get_property_reference(struct acpi_device_data *data,
>         while (element < end) {
>                 u32 nargs, i;
>
> -               if (element->type != ACPI_TYPE_LOCAL_REFERENCE)
> -                       return -EPROTO;
> -
> -               ret = acpi_bus_get_device(element->reference.handle, &device);
> -               if (ret)
> -                       return -ENODEV;
> -
> -               element++;
> -               nargs = 0;
> -
> -               /* assume following integer elements are all args */
> -               for (i = 0; element + i < end; i++) {
> -                       int type = element[i].type;
> +               if (element->type == ACPI_TYPE_LOCAL_REFERENCE) {
> +                       ret = acpi_bus_get_device(element->reference.handle,
> +                                                 &device);
> +                       if (ret)
> +                               return -ENODEV;
> +
> +                       nargs = 0;
> +                       element++;
> +
> +                       /* assume following integer elements are all args */
> +                       for (i = 0; element + i < end && i < num_args; i++) {
> +                               int type = element[i].type;
> +
> +                               if (type == ACPI_TYPE_INTEGER)
> +                                       nargs++;
> +                               else if (type == ACPI_TYPE_LOCAL_REFERENCE)
> +                                       break;
> +                               else
> +                                       return -EPROTO;
> +                       }
>
> -                       if (type == ACPI_TYPE_INTEGER)
> -                               nargs++;
> -                       else if (type == ACPI_TYPE_LOCAL_REFERENCE)
> -                               break;
> -                       else
> +                       if (nargs > MAX_ACPI_REFERENCE_ARGS)
>                                 return -EPROTO;
> -               }
>
> -               if (idx++ == index) {
> -                       args->adev = device;
> -                       args->nargs = nargs;
> -                       for (i = 0; i < nargs; i++)
> -                               args->args[i] = element[i].integer.value;
> +                       if (idx == index) {
> +                               args->adev = device;
> +                               args->nargs = nargs;
> +                               for (i = 0; i < nargs; i++)
> +                                       args->args[i] = element[i].integer.value;
>
> -                       return 0;
> +                               return 0;
> +                       }
> +
> +                       element += nargs;
> +               } else if (element->type == ACPI_TYPE_INTEGER) {
> +                       if (idx == index)
> +                               return -ENOENT;
> +                       element++;
> +               } else {
> +                       return -EPROTO;
>                 }
>
> -               element += nargs;
> +               idx++;
>         }
>
> -       return -EPROTO;
> +       return -ENODATA;
>  }
>
>  /**
> @@ -582,10 +594,54 @@ int acpi_node_get_property_reference(struct fwnode_handle *fwnode,
>  {
>         struct acpi_device_data *data = acpi_device_data_of_node(fwnode);
>
> -       return data ? acpi_data_get_property_reference(data, name, index, args) : -EINVAL;
> +       if (!data)
> +               return -EINVAL;
> +
> +       return acpi_data_get_property_reference(data, name, index,
> +               MAX_ACPI_REFERENCE_ARGS, args);
>  }
>  EXPORT_SYMBOL_GPL(acpi_node_get_property_reference);

gpiolib-acpi is the only user of this AFAICS, so why don't we change
the definition of acpi_node_get_property_reference() to take one extra
argument and update the caller to pass MAX_ACPI_REFERENCE_ARGS?

We'd avoid having to define the thing below then.

>
> +/**
> + * acpi_node_get_property_reference_fixed_args - get a reference from property
> + * @fwnode: Firmware node to get the property from.
> + * @propname: Name of the property.
> + * @index: Index of the reference to return.
> + * @num_args: Expected number of arguments following each reference.
> + * @args: Location to store the returned reference with optional arguments.
> + *
> + * Using this function it is possible to leave "holes" in the property
> + * value set like in an example below:
> + *
> + * Package () {
> + *     "cs-gpios",
> + *     Package () {
> + *        ^GPIO, 19, 0, 0,
> + *        ^GPIO, 20, 0, 0,
> + *        0,
> + *        ^GPIO, 21, 0, 0,
> + *     }
> + * }
> + *
> + * Calling this function with index %2 return %-ENOENT and with index %3
> + * returns the last entry. If the property does not contain any more values
> + * %-ENODATA is returned. The NULL entry must be single integer and
> + * preferably contain value %0.
> + */
> +int acpi_node_get_property_reference_fixed_args(struct fwnode_handle *fwnode,
> +       const char *name, size_t index, size_t num_args,
> +       struct acpi_reference_args *args)
> +{
> +       struct acpi_device_data *data = acpi_device_data_of_node(fwnode);
> +
> +       if (!data)
> +               return -EINVAL;
> +
> +       return acpi_data_get_property_reference(data, name, index, num_args,
> +                                               args);
> +}
> +EXPORT_SYMBOL_GPL(acpi_node_get_property_reference_fixed_args);
> +
>  static int acpi_data_prop_read_single(struct acpi_device_data *data,
>                                       const char *propname,
>                                       enum dev_prop_type proptype, void *val)
> diff --git a/include/linux/acpi.h b/include/linux/acpi.h
> index c5eaf2f80a4c..c5d7c503843f 100644
> --- a/include/linux/acpi.h
> +++ b/include/linux/acpi.h
> @@ -930,6 +930,9 @@ int acpi_dev_get_property(struct acpi_device *adev, const char *name,
>  int acpi_node_get_property_reference(struct fwnode_handle *fwnode,
>                                      const char *name, size_t index,
>                                      struct acpi_reference_args *args);
> +int acpi_node_get_property_reference_fixed_args(struct fwnode_handle *fwnode,
> +                               const char *name, size_t index, size_t num_args,
> +                               struct acpi_reference_args *args);
>
>  int acpi_node_prop_get(struct fwnode_handle *fwnode, const char *propname,
>                        void **valptr);
> --

Thanks,
Rafael