Re: [PATCH 1/3][v2] gpiolib: add gpiod_get_array and gpiod_put_array functions

[Rojhalat Ibrahim <imr@rtschenk.de>]

On Monday 09 February 2015 14:09:26 Mika Westerberg wrote:
> On Mon, Feb 09, 2015 at 02:12:39PM +0900, Alexandre Courbot wrote:
> > > +#ifdef CONFIG_ACPI
> > > +
> > > +static int acpi_find_gpio_count(struct acpi_resource *ares, void *data)
> > > +{
> > > +       unsigned int *count = data;
> > > +
> > > +       if (ares->type == ACPI_RESOURCE_TYPE_GPIO)
> > > +               *count = ares->data.gpio.pin_table_length;
> > > +
> > > +       return 1;
> > > +}
> > > +
> > > +static unsigned int acpi_gpio_count(struct device *dev, const char *con_id)
> > > +{
> > > +       struct acpi_device *adev = ACPI_COMPANION(dev);
> > > +       const union acpi_object *obj;
> > > +       const struct acpi_gpio_mapping *gm;
> > > +       unsigned int count = 0;
> > > +       int ret;
> > > +       char propname[32];
> > > +       unsigned int i;
> > > +
> > > +       /* Try first from _DSD */
> > > +       for (i = 0; i < ARRAY_SIZE(gpio_suffixes); i++) {
> > > +               if (con_id && strcmp(con_id, "gpios"))
> > > +                       snprintf(propname, sizeof(propname), "%s-%s",
> > > +                                con_id, gpio_suffixes[i]);
> > > +               else
> > > +                       snprintf(propname, sizeof(propname), "%s",
> > > +                                gpio_suffixes[i]);
> > > +
> > > +               ret = acpi_dev_get_property(adev, propname, ACPI_TYPE_ANY,
> > > +                                           &obj);
> > > +               if (ret == 0) {
> > > +                       if (obj->type == ACPI_TYPE_LOCAL_REFERENCE)
> > > +                               count = 1;
> > > +                       else if (obj->type == ACPI_TYPE_PACKAGE)
> > > +                               count = obj->package.count;
> > > +               } else if (adev->driver_gpios) {
> > > +                       for (gm = adev->driver_gpios; gm->name; gm++)
> > > +                               if (strcmp(propname, gm->name) == 0) {
> > > +                                       count = gm->size;
> > > +                                       break;
> > > +                               }
> > > +               }
> > > +               if (count)
> > > +                       break;
> > > +       }
> > > +
> > > +       /* Then from plain _CRS GPIOs */
> > > +       if (!count) {
> > > +               struct list_head resource_list;
> > > +
> > > +               INIT_LIST_HEAD(&resource_list);
> > > +               acpi_dev_get_resources(adev, &resource_list,
> > > +                                      acpi_find_gpio_count, &count);
> > > +               acpi_dev_free_resource_list(&resource_list);
> > > +       }
> > > +       return count;
> > > +}
> > 
> > I'd really like to have an acked-by from some of the ACPI people
> > (Mika?) for this part.
> 
> If I understand correctly the new interface allows requesting all GPIOs
> for a given device?
> 

That's the general idea.

> In that case the above code will not work. In ACPI a device may have
> several GpioIo/GpioInt resources and each can hold several pins. If I
> read the above code right, it re-assigns 'count' for each GPIO instead
> of adding to it.
> 

Ok. That can be fixed easily.

> The _DSD path also seems to return number of elements in a package:
> 
> 	Package () {"reset-gpios", Package() {^BTH, 1, 1, 0}},
> 
> Here for example it returns 4 which is not what is expected. I think in
> this case the correct value is 1.
> 
> Also there may be several GPIOs for a single property like:
> 
> 	Package () {"reset-gpios", Package() {^BTH, 1, 1, 0, ^BTH, 1, 2, 0}},
> 
> I'm not sure if that is handled correctly here.

Obviously not. So instead of taking the number of elements in the package
we could count the number of device references (ACPI_TYPE_LOCAL_REFERENCE)
within the package. That should give us the number of GPIOs in the package,
right?