zephyrproject-rtos / zephyr

Primary Git Repository for the Zephyr Project. Zephyr is a new generation, scalable, optimized, secure RTOS for multiple hardware architectures.
https://docs.zephyrproject.org
Apache License 2.0
10.98k stars 6.69k forks source link

devicetree: add DT_GPIO_FOREACH() #24536

Open sslupsky opened 4 years ago

sslupsky commented 4 years ago

I was experimenting with automating initialization of gpio and wondered if there could be a devicetree macro defined for this purpose similar to the DT_INST_FOREACH() macro?

Here is an example of what I was thinking (this example assumes the compatible is "gpio"):

#define DT_GPIO_FOREACH(inst_expr) \
    UTIL_LISTIFY(DT_NUM_INST(gpio), DT_CALL_WITH_ARG, inst_expr)

#define MY_GPIO_INIT(inst) \
    dev = device_get_binding(DT_INST(inst,gpio)); \
    gpio_pin_configure(dev, 10, GPIO_OUTPUT_INACTIVE | GPIO_ACTIVE_HIGH);

and then using the following to instantiate the code:

    DT_GPIO_FOREACH(MY_GPIO_INIT);

I still need to figure out the correct syntax for the gpio_pin_configure() so that I get the correct port, pin and flags. But I think this illustrates the idea.

Does anyone have some feedback? Would this be useful to anyone else? These devicetree macros are a very steep learning curve so any pointers would be appreciated or if this is impossible to do please let me know.

galak commented 4 years ago

I'd like to get @pabigot and @mnkp comments first as the GPIO subsystem maintainers.

pabigot commented 4 years ago

My initial response is I don't think that'd be useful. MY_GPIO_INIT in your example doesn't do any error checking; I would want to have that in my code, and to have it diagnose which initialization failed. It's also not particularly common to have a single phandle-array property with multiple GPIOs, either.

I have tried for a long time to get some sort of common aggregate representation of devicetree GPIO information, e.g.:

struct gpio_dt {
    union {
       const char* name;
       struct device *device;
    };
    gpio_pin_t pin;
    gpio_dt_flags_t flags;
};

that could be initialized with something like:

struct gpio_dt isw = DT_INST_GPIO(0, isw_gpios);

with no success, though the previous devicetree API allowed generating an initializer list. Two in-tree platforms don't support 2-cell GPIO specifiers so that's probably hopeless, and might rule out this approach too.

Even if there were a consistent specifier pattern, if there are more than one GPIO specifiers to be initialized it's probably more space-efficient to iterate over a data structure than unwrap the initialization loop in code.

petejohanson commented 4 years ago

I'm currently experimenting with implementing a kscan driver for GPIO matrixes, and playing with using a DT that looks like:

kscan {
        compatible = "gpio-kscan";
        label = "Keyscan Matrix";
        row-gpios = <&gpioa 10 GPIO_ACTIVE_HIGH>,
                   <&gpioa 9 GPIO_ACTIVE_HIGH>,
                   <&gpioa 8 GPIO_ACTIVE_HIGH>,
                   <&gpiob 15 GPIO_ACTIVE_HIGH>,
                   <&gpioc 13 GPIO_ACTIVE_HIGH>,
                   <&gpioc 14 GPIO_ACTIVE_HIGH>,
                   <&gpioc 15 GPIO_ACTIVE_HIGH>,
                   <&gpioa 2 GPIO_ACTIVE_HIGH>;
        col-gpios = <&gpiob 11 GPIO_ACTIVE_HIGH>,
                <&gpiob 10 GPIO_ACTIVE_HIGH>,
                <&gpiob 2 GPIO_ACTIVE_HIGH>,
                <&gpiob 1 GPIO_ACTIVE_HIGH>,
                <&gpioa 7 GPIO_ACTIVE_HIGH>,
                <&gpiob 0 GPIO_ACTIVE_HIGH>;
    };

Because the number of columns + rows varies by board/shield, I need the ability to loop over all the rows/columns during initialization to get the various devices, store the relevant pins, etc into an array for later use for the scanning callback.

Having a foreach for this would be exactly what I would love to use, and would avoid hand writing a directive only to invoke it manually a bunch of times behind #if guards w/ DT_PROP_HAS_IDX checks.

henrikbrixandersen commented 4 years ago

Having a foreach for this would be exactly what I would love to use, and would avoid hand writing a directive only to invoke it manually a bunch of times behind #if guards w/ DT_PROP_HAS_IDX checks.

You can retrieve the length of the phandle-array using DT_PROP_LEN()/DT_INST_PROP_LEN() and loop through all entries.

petejohanson commented 4 years ago

@henrikbrixandersen The problem is, I can't invoke the macro DT_GPIO_FLAGS_BY_IDX, etc. at runtime with the index into gpios property. I can't do:

for (int idx = 0; idx < DT_PROP_LEN(DT_PATH(foo),row_gpios); idx++) {
    gpio_flag_t flag = DT_GPIO_FLAGS_BY_IDX(DT_PATH(foo),row_gpios,idx);
}

Which is why I'm needing this to be expanded at compile time, like the DT_INST_FOREACH does.

Or am I missing some other trick?

petejohanson commented 4 years ago

Ok, so I just pushed a branch that demonstrates what would be possible. You can see the branch or the commit that adds a new macro DT_FOREACH_PHA that would invoke the passed function/macro for each entry in the phandles/phandle-array.

Example from the kscan GPIO matrix driver I'm working on after the change:

#define INIT_GPIO_ITEM_INT(node_id,p,f) \
→       { .label = DT_LABEL(node_id), .pin = p, .flags = f },

#define INIT_GPIO_ITEM(...) INIT_GPIO_ITEM_INT(__VA_ARGS__)

static const struct kscan_gpio_config {
→       struct kscan_gpio_matrix_item_config rows[MATRIX_ROWS];
→       struct kscan_gpio_matrix_item_config cols[MATRIX_ROWS];
} kscan_gpio_config = {
→       .rows = {
→       →       DT_FOREACH_PHA(MATRIX_NODE_ID, row_gpios, INIT_GPIO_ITEM)
→       },
→       .cols = {
→       →       DT_FOREACH_PHA(MATRIX_NODE_ID, col_gpios, INIT_GPIO_ITEM)
→       }
};

Doing it this way with "exploding" the arguments into the function/macro requires the var_arg tricks, you could instead just pass in the node, prop, and idx as parameters, and require the macro invoked to do further macro calls to loop up values, but this felt easier, if more complex. It also has the added benefit of being truly generic, not specific to the GPIO usage at all.

Thoughts?