[TOPIC-GPIO] boards: nrf52840_pca10090: Convert to use the new GPIO API

[anangl]

Update the code configuring pins that control the routing of certain
lines on the board, to take advantage of the introduced possibility
of configuring a pin as output with specified initial state.

Additionally, use gpio_pin_get_raw instead of gpio_pin_get for
checking the reset pin state, to save a few bytes in flash.

Signed-off-by: Andrzej Głąbek [email protected]

[lemrey]

So this new API will configure the pin as output and also set its state to low/high?

[pabigot]

Yes, that's what those flags do. Unfortunately there aren't OUTPUT_INIT_ACTIVE and OUTPUT_INIT_INACTIVE.

Since the pins are documented as being active low, and the new API supports operating on logical level, I think it would be more clear to have this be:

unsigned int flags = GPIO_ACTIVE_LOW;

if (cfg[i].pin) {
   flags |= GPIO_OUTPUT_LOW;
} else {
   flags |= GPIO_OUTPUT_HIGH;
}
err = gpio_pin_configure(port, cfg[i].pin, flags);

then...

[anangl]

Since the pins are documented as being active low

Well, in fact these pins are not documented as being active low. The DK documentation says only about the default destination and the optional destination, activated when a given pin is set as output and driven low or high, respectively. These pins are described as having active-low logic only in the comment before definitions of pins_on_p0 and pins_on_p1 tables (and I left that comment as it was, treating it as to be considered in the context of these tables only; should I change it?). They were also mentioned as being active low in the comment that I reworded, and I intentionally got rid of this "are active low" statement.

[pabigot]

OK then.

[pabigot]

This comment is still in the code, which is part of why I asked for the change:

 * The switches have active-low logic, so when writing to the port we will
 * need to invert the value to match the IS_ENABLED() logic.

However that's clear enough, so I'm ok with using the negative logic physical value.

[pabigot]

Yes, that's what those flags do. Unfortunately there aren't OUTPUT_INIT_ACTIVE and OUTPUT_INIT_INACTIVE.

Since the pins are documented as being active low, and the new API supports operating on logical level, I think it would be more clear to have this be:

unsigned int flags = GPIO_ACTIVE_LOW;

if (cfg[i].pin) {
   flags |= GPIO_OUTPUT_LOW;
} else {
   flags |= GPIO_OUTPUT_HIGH;
}
err = gpio_pin_configure(port, cfg[i].pin, flags);

then...

[pabigot]

... here do:

/* Wait until pin becomes active */
while (gpio_pin_get(port, pin) == 0) {
}

Other boards might include pins that are active-high, and using the logic level rather than physical level would make the code more consistent.

[pabigot]

I now see the original comment about using _raw to save a few bytes of flash. I prefer clarity of using normal logic, but if I'm alone in that I won't insist.

[anangl]

Other boards might include pins that are active-high, and using the logic level rather than physical level would make the code more consistent.

This reset pin here is in fact active high. And the above loop waits until it becomes inactive. As the accompanying comment says, this "lets the other side ensure that they are ready".
I'd prefer to left it as it is.

[pabigot]

Clearly I don't understand what this is doing well enough to assess it. We'll see if github allows me to remove my approval. edit: nope.

[lemrey]

Thank you for cleaning this up.

[mnkp]

@anangl Thanks! I overlooked this part when doing board conversion.