On 22 August 2018 at 11:16, Diego Sueiro <diego.sueiro@...> wrote:

Folks,

I was thinking about doing a code cleanup in the dts.fixup and uart and gpio driver for the i.MX7 SoC, where we create redundant defines already covered by devicetree aliases.

Here is a breif example for the CONFIG_UART_IMX_UART_2_NAME:

boards/arm/colibri_imx7d_m4/colibri_imx7d_m4.dts:

<...>

aliases {

                <...>

                uart-2 = &uart2;

                <...>

        };

<...>

This alias will generate the following define in <build>/zephyr/include/generated/generated_dts_board.h:

#define UART_2_LABEL                        NXP_IMX_UART_30890000_LABEL

This is an excellent idea.  The only downside (upside?) is having to standardize on all of the subsystem driver labels.  And it still doesn't solve the device specific properties unless we were to extend this idea to automatically generate generic labels for all device specific properties by adding a #define to point the generic label + property to the device compatible + address value.  Then you'd have to build up the platform data structures to contain this stuff.

And the associated change will be:

diff --git a/arch/arm/soc/nxp_imx/mcimx7_m4/dts.fixup b/arch/arm/soc/nxp_imx/mcimx7_m4/dts.fixup

index f8765dc..8e46695 100644

--- a/arch/arm/soc/nxp_imx/mcimx7_m4/dts.fixup

+++ b/arch/arm/soc/nxp_imx/mcimx7_m4/dts.fixup

@@ -65,7 +65,6 @@

 #define CONFIG_UART_IMX_UART_1_IRQ_PRI      NXP_IMX_UART_30860000_IRQ_0_PRIORITY

 #define CONFIG_UART_IMX_UART_1_MODEM_MODE   NXP_IMX_UART_30860000_MODEM_MODE

 

-#define CONFIG_UART_IMX_UART_2_NAME         NXP_IMX_UART_30890000_LABEL

 #define CONFIG_UART_IMX_UART_2_BASE_ADDRESS NXP_IMX_UART_30890000_BASE_ADDRESS

 #define CONFIG_UART_IMX_UART_2_BAUD_RATE    NXP_IMX_UART_30890000_CURRENT_SPEED

 #define CONFIG_UART_IMX_UART_2_IRQ_NUM      NXP_IMX_UART_30890000_IRQ_0

diff --git a/drivers/serial/uart_imx.c b/drivers/serial/uart_imx.c

index 6a8f701..a3ef5ea 100644

--- a/drivers/serial/uart_imx.c

+++ b/drivers/serial/uart_imx.c

@@ -337,7 +337,7 @@ static const struct imx_uart_config imx_uart_2_config = {

 

 static struct imx_uart_data imx_uart_2_data;

 

-DEVICE_AND_API_INIT(uart_2, CONFIG_UART_IMX_UART_2_NAME, &uart_imx_init,

+DEVICE_AND_API_INIT(uart_2, UART_2_LABEL, &uart_imx_init,

                &imx_uart_2_data, &imx_uart_2_config,

                PRE_KERNEL_1, CONFIG_KERNEL_INIT_PRIORITY_DEVICE,

                &uart_imx_driver_api); 

I used this strategy for the imx i2c and pwm drivers.

I believe that this will simplify and let the code a little bit cleaner.

What do you think? Do you see any caveats about this? 

PS.: I found that we could potentially remove 1828 defines in dts.fixup files in master (grep -R --exclude-dir={build,samples,tests} --include="dts.fixup" "#define CONFIG_"  ./* | grep -v "PRIO_BITS" | wc -l)

Regards,

Andy