Re: [PATCH v2 5/5] gpio / ACPI: Add support for ACPI GPIO operation regions

[Mika Westerberg <mika.westerberg@linux.intel.com>]

On Thu, Mar 13, 2014 at 03:32:01PM +0100, Linus Walleij wrote:
> On Mon, Mar 10, 2014 at 1:54 PM, Mika Westerberg
> <mika.westerberg@linux.intel.com> wrote:
> 
> Here:
> 
> > +static acpi_status
> > +acpi_gpio_adr_space_handler(u32 function, acpi_physical_address address,
> > +                           u32 bits, u64 *value, void *handler_context,
> > +                           void *region_context)
> > +{
> > +       struct acpi_gpio_chip *achip = region_context;
> > +       struct gpio_chip *chip = achip->chip;
> > +       struct acpi_resource_gpio *agpio;
> > +       struct acpi_resource *ares;
> > +       acpi_status status;
> > +       bool pull;
> 
> Should be named pull_up, right?

Yes.

> 
> > +       int i;
> > +
> > +       status = acpi_buffer_to_resource(achip->conn_info.connection,
> > +                                        achip->conn_info.length, &ares);
> > +       if (ACPI_FAILURE(status))
> > +               return status;
> > +
> > +       if (WARN_ON(ares->type != ACPI_RESOURCE_TYPE_GPIO)) {
> > +               ACPI_FREE(ares);
> > +               return AE_BAD_PARAMETER;
> > +       }
> > +
> > +       agpio = &ares->data.gpio;
> > +       pull = agpio->pin_config == ACPI_PIN_CONFIG_PULLUP;
> 
> So here ACPI tells us that the pin is pulled up.
> 
> And I am suspicious since this is strictly speaking pin control business
> and not GPIO, and I won't let pin control stuff sneak into the GPIO
> subsystem under the radar just because I'm not paying close enough
> attention.

This has more to do in finding out what will be the initial value of the
GPIO when we set it to output (given that it is output).

> I have been told that these things are not dynamic (i.e. we only get
> informed that a pin is pulled up/down, we cannot actively change the
> config) is this correct?

As far as I can tell, yes.

> If any sort of advanced pin control business is going to come into
> ACPI a sibling driver in drivers/pinctrl/pinctrl-acpi.c needs to be
> created that can select CONFIG_PINCONF properly reflect this
> stuff in debugfs etc. (Maybe also give proper names on the pins
> since I hear this is coming to ACPI!)

No advanced pin control business, just GPIO stuff :)

> > +       if (WARN_ON(agpio->io_restriction == ACPI_IO_RESTRICT_INPUT &&
> > +           function == ACPI_WRITE)) {
> > +               ACPI_FREE(ares);
> > +               return AE_BAD_PARAMETER;
> > +       }
> > +
> > +       for (i = 0; i < agpio->pin_table_length; i++) {
> > +               unsigned pin = agpio->pin_table[i];
> > +               struct acpi_gpio_connection *conn;
> > +               struct gpio_desc *desc;
> > +               bool found;
> > +
> > +               desc = gpiochip_get_desc(chip, pin);
> > +               if (IS_ERR(desc)) {
> > +                       status = AE_ERROR;
> > +                       goto out;
> > +               }
> > +
> > +               mutex_lock(&achip->conn_lock);
> > +
> > +               found = false;
> > +               list_for_each_entry(conn, &achip->conns, node) {
> > +                       if (conn->desc == desc) {
> > +                               found = true;
> > +                               break;
> > +                       }
> > +               }
> > +               if (!found) {
> > +                       int ret;
> > +
> > +                       ret = gpiochip_request_own_desc(desc, "ACPI:OpRegion");
> > +                       if (ret) {
> > +                               status = AE_ERROR;
> > +                               mutex_unlock(&achip->conn_lock);
> > +                               goto out;
> > +                       }
> > +
> > +                       switch (agpio->io_restriction) {
> > +                       case ACPI_IO_RESTRICT_INPUT:
> > +                               gpiod_direction_input(desc);
> > +                               break;
> > +                       case ACPI_IO_RESTRICT_OUTPUT:
> > +                               gpiod_direction_output(desc, pull);
> 
> Can you explain why the fact that the line is pulled down affects the
> default output value like this? I don't get it.

That's the thing - ACPI doesn't tell us what is the initial value of the
pin. There is no such field in GpioIo() resource.

So I'm assuming here that if it says that the pin is pulled up, the default
value will be high.

> A comment in the code would be needed I think.
> 
> This looks more like a typical piece of code for open collector/drain
> (which is already handled by gpiolib) not pull up/down.
>
> 
> > +                               break;
> > +                       default:
> > +                               /*
> > +                                * Assume that the BIOS has configured the
> > +                                * direction and pull accordingly.
> > +                                */
> > +                               break;
> > +                       }
> > +
> > +                       conn = kzalloc(sizeof(*conn), GFP_KERNEL);
> > +                       if (!conn) {
> > +                               status = AE_NO_MEMORY;
> > +                               mutex_unlock(&achip->conn_lock);
> > +                               goto out;
> > +                       }
> > +
> > +                       conn->desc = desc;
> > +
> > +                       list_add_tail(&conn->node, &achip->conns);
> > +               }
> > +
> > +               mutex_unlock(&achip->conn_lock);
> > +
> > +
> > +               if (function == ACPI_WRITE)
> > +                       gpiod_set_raw_value(desc, !!((1 << i) & *value));
> 
> What is this? How can the expression !!((1 << i) possibly evaluate to
> anything else than "true"? I don't get it. Just (desc, *value) seem more
> apropriate.

We are dealing with multiple pins here. So for example if
agpio->pin_table_length == 2 and *value == 0x2 we get:

i == 0: !!((1 << 0) & 0x2) --> false
i == 1: !!((1 << 1) & 0x2) --> true

Maybe

	gpiod_set_raw_value(desc, (*value >> i) & 1);

is cleaner?