core: dt_driver: swap TEE_result and retrieved device reference

[etienne-lms]

Changes dt_driver callback function to return a TEE_Result value and pass retrieved device reference by a output argument rather than the opposite.

This change updates dt_driver.c, dt_driver.h, dt_driver_test.c and all drivers implementing related dt_driver callback functions.

As a consequence, this change removes all type definition related to device specific callback handler function types which are useless as all these now comply with type dt_driver_probe_func defined in dt_driver.h.

This P-R is related to discussion thread #5954 (comment).

[etienne-lms]

Checkpatch warns on nodeoffset not being a identifiable name. IMO it's a false positive. This change series is not expected to rename those argument names.

WARNING: function definition argument 'int nodeoffset' should also have an identifier name
#944: FILE: core/include/kernel/dt_driver.h:172:
+TEE_Result dt_driver_device_from_parent(const void *fdt, int nodeoffset,

[etienne-lms]

This P-R is related to discussion thread #5954 (comment).
Maybe I should mention this in the commit message to give some background on why we make such a change.

[etienne-lms]

Bug found in dt_driver_test.c and fixed. CI make check should run ok now.

[etienne-lms]

I pushed 2 fixup commits. A Fix for a bug introduced in fixed_clock_probe() and a big fixup commit to restore per driver framework callback functions (clk, reset ctlr, gpio, pinctrl, i2c bus) to not have consumer callback using void ** device reference types but rather driver specific type (struct clk **, struct rstctrl ** etc...) as before this change series.
Sorry for the incoviences, i think i it would be easier for the reviews if I could squash straight those fixup commits.

(edited)

[jenswikl]

What do others think? Is it worth it to swap the TEE_Result and device reference?

[jenswikl]

This cast might be a problem, depending on the compiler. It may not be a problem in practice since all pointers use the same size, but a zealous compiler could complain about strict aliasing rules. I guess this is the reason why this somewhat backward way of returning the error value was chosen.

I'd prefer

                void *pinconf = NULL;

                res = dt_driver_device_from_node_idx_prop(prop_name, fdt,
                                                          nodeoffset, conf_id,
                                                          DT_DRIVER_PINCTRL,
                                                          &pinconf);
                 if (res) {
                        free(state);
                        return res;
                 }
                 state->confs[conf_id] = pinconf;

to avoid that ugly cast.

[jforissier]

What do others think? Is it worth it to swap the TEE_Result and device reference?

No strong opinion, but judging by the diffstat (+269 -334) it makes the code a bit more compact, and having a TEE_Result as a return value is indeed much more natural. So I'd say yes, it's worth it.

I'd prefer

               void *pinconf = NULL;

               res = dt_driver_device_from_node_idx_prop(prop_name, fdt,
                                                         nodeoffset, conf_id,
                                                         DT_DRIVER_PINCTRL,
                                                         &pinconf);

+1

[jenswikl]

Thanks @jforissier.
@etienne-lms, please address my comment and squash the commits as needed. I'll review the stuff once you're done.

[etienne-lms]

I pushed a fixup before I saw your last commit. I'll squash up all fixup commits and update the P-R.

[etienne-lms]

fixed and squashed.
Same checkpatch warning awaited:

WARNING: function definition argument 'int nodeoffset' should also have an identifier name
#872: FILE: core/include/kernel/dt_driver.h:150:
+TEE_Result dt_driver_device_from_node_idx_prop(const char *prop_name,

WARNING: function definition argument 'int nodeoffset' should also have an identifier name
#900: FILE: core/include/kernel/dt_driver.h:172:
+TEE_Result dt_driver_device_from_parent(const void *fdt, int nodeoffset,

[jenswikl]

Acked-by: Jens Wiklander <jens.wiklander@linaro.org>

[etienne-lms]

review tag applied