spi: pl022: Simplify platdata code

pl022 spi driver support both OF_CONTROL and PLATDATA, this patch is trying to simplify the code that differentiating platdata vs of_control.
- Move OF_CONTROL code at one place
- Handle clock setup code directly in pl022_spi_ofdata_to_platdata

Cc Quentin Schulz <[email protected]>

Signed-off-by Jagan Teki <[email protected]>

---
Changes for v3:
- none
Changes for v2:
- Update commit message
- Use struct clk for clkdev

 drivers/spi/pl022_spi.c              | 48 ++++++++++++----------------
 include/dm/platform_data/pl022_spi.h |  9 ------
 2 files changed, 20 insertions(+), 37 deletions(-)

drivers/spi/pl022_spi.c +20 -28

index 86b71d2e21..05f4f6f481 100644
@@ -72,11 +72,7 @@
 
 struct pl022_spi_slave {
 	void *base;
-#if !CONFIG_IS_ENABLED(OF_PLATDATA)
-	struct clk clk;
-#else
 	unsigned int freq;
-#endif
 };
 
 /*

@@ -96,30 +92,13 @@ static int pl022_is_supported(struct pl022_spi_slave *ps)
 	return 0;
 }
 
-#if !CONFIG_IS_ENABLED(OF_PLATDATA)
-static int pl022_spi_ofdata_to_platdata(struct udevice *bus)
-{
-	struct pl022_spi_pdata *plat = bus->platdata;
-	const void *fdt = gd->fdt_blob;
-	int node = dev_of_offset(bus);
-
-	plat->addr = fdtdec_get_addr_size(fdt, node, "reg", &plat->size);
-
-	return clk_get_by_index(bus, 0, &plat->clk);
-}
-#endif
-
 static int pl022_spi_probe(struct udevice *bus)
 {
 	struct pl022_spi_pdata *plat = dev_get_platdata(bus);
 	struct pl022_spi_slave *ps = dev_get_priv(bus);
 
 	ps->base = ioremap(plat->addr, plat->size);
-#if !CONFIG_IS_ENABLED(OF_PLATDATA)
-	ps->clk = plat->clk;
-#else
 	ps->freq = plat->freq;
-#endif
 
 	/* Check the PL022 version */
 	if (!pl022_is_supported(ps))

@@ -240,11 +219,7 @@ static int pl022_spi_set_speed(struct udevice *bus, uint speed)
 	u16 scr = SSP_SCR_MIN, cr0 = 0, cpsr = SSP_CPSR_MIN, best_scr = scr,
 	    best_cpsr = cpsr;
 	u32 min, max, best_freq = 0, tmp;
-#if !CONFIG_IS_ENABLED(OF_PLATDATA)
-	u32 rate = clk_get_rate(&ps->clk);
-#else
 	u32 rate = ps->freq;
-#endif
 	bool found = false;
 
 	max = spi_rate(rate, SSP_CPSR_MIN, SSP_SCR_MIN);

@@ -316,6 +291,25 @@ static const struct dm_spi_ops pl022_spi_ops = {
 };
 
 #if !CONFIG_IS_ENABLED(OF_PLATDATA)
+static int pl022_spi_ofdata_to_platdata(struct udevice *bus)
+{
+	struct pl022_spi_pdata *plat = bus->platdata;
+	const void *fdt = gd->fdt_blob;
+	int node = dev_of_offset(bus);
+	struct clk clkdev;
+	int ret;
+
+	plat->addr = fdtdec_get_addr_size(fdt, node, "reg", &plat->size);
+
+	ret = clk_get_by_index(bus, 0, &clkdev);
+	if (ret)
+		return ret;
+
+	plat->freq = clk_get_rate(&clkdev);
+
+	return 0;
+}
+
 static const struct udevice_id pl022_spi_ids[] = {
 	{ .compatible = "arm,pl022-spi" },
 	{ }

@@ -327,11 +321,9 @@ U_BOOT_DRIVER(pl022_spi) = {
 	.id     = UCLASS_SPI,
 #if !CONFIG_IS_ENABLED(OF_PLATDATA)
 	.of_match = pl022_spi_ids,
-#endif
-	.ops    = &pl022_spi_ops,
-#if !CONFIG_IS_ENABLED(OF_PLATDATA)
 	.ofdata_to_platdata = pl022_spi_ofdata_to_platdata,
 #endif
+	.ops    = &pl022_spi_ops,
 	.platdata_auto_alloc_size = sizeof(struct pl022_spi_pdata),
 	.priv_auto_alloc_size = sizeof(struct pl022_spi_slave),
 	.probe  = pl022_spi_probe,

include/dm/platform_data/pl022_spi.h +0 -0

index 77fe6da3cb..57d12ac912 100644

Hi Jagan,

Quoted 125 lines · Jagan Teki

pl022 spi driver support both OF_CONTROL and PLATDATA, this patch is trying to simplify the code that differentiating platdata vs of_control.
- Move OF_CONTROL code at one place
- Handle clock setup code directly in pl022_spi_ofdata_to_platdata

Cc Quentin Schulz <[email protected]>

Signed-off-by Jagan Teki <[email protected]>

---
Changes for v3:
- none
Changes for v2:
- Update commit message
- Use struct clk for clkdev

 drivers/spi/pl022_spi.c              | 48 ++++++++++++----------------
 include/dm/platform_data/pl022_spi.h |  9 ------
 2 files changed, 20 insertions(+), 37 deletions(-)

drivers/spi/pl022_spi.c +20 -28

index 86b71d2e21..05f4f6f481 100644
@@ -72,11 +72,7 @@
 
 struct pl022_spi_slave {
      void *base;
-#if !CONFIG_IS_ENABLED(OF_PLATDATA)
-     struct clk clk;
-#else
      unsigned int freq;
-#endif
 };
 
 /*

@@ -96,30 +92,13 @@ static int pl022_is_supported(struct pl022_spi_slave *ps)
      return 0;
 }
 
-#if !CONFIG_IS_ENABLED(OF_PLATDATA)
-static int pl022_spi_ofdata_to_platdata(struct udevice *bus)
-{
-     struct pl022_spi_pdata *plat = bus->platdata;
-     const void *fdt = gd->fdt_blob;
-     int node = dev_of_offset(bus);
-
-     plat->addr = fdtdec_get_addr_size(fdt, node, "reg", &plat->size);
-
-     return clk_get_by_index(bus, 0, &plat->clk);
-}
-#endif
-
 static int pl022_spi_probe(struct udevice *bus)
 {
      struct pl022_spi_pdata *plat = dev_get_platdata(bus);
      struct pl022_spi_slave *ps = dev_get_priv(bus);
 
      ps->base = ioremap(plat->addr, plat->size);
-#if !CONFIG_IS_ENABLED(OF_PLATDATA)
-     ps->clk = plat->clk;
-#else
      ps->freq = plat->freq;
-#endif
 
      /* Check the PL022 version */
      if (!pl022_is_supported(ps))

@@ -240,11 +219,7 @@ static int pl022_spi_set_speed(struct udevice *bus, uint speed)
      u16 scr = SSP_SCR_MIN, cr0 = 0, cpsr = SSP_CPSR_MIN, best_scr = scr,
          best_cpsr = cpsr;
      u32 min, max, best_freq = 0, tmp;
-#if !CONFIG_IS_ENABLED(OF_PLATDATA)
-     u32 rate = clk_get_rate(&ps->clk);
-#else
      u32 rate = ps->freq;
-#endif
      bool found = false;
 
      max = spi_rate(rate, SSP_CPSR_MIN, SSP_SCR_MIN);

@@ -316,6 +291,25 @@ static const struct dm_spi_ops pl022_spi_ops = {
 };
 
 #if !CONFIG_IS_ENABLED(OF_PLATDATA)
+static int pl022_spi_ofdata_to_platdata(struct udevice *bus)
+{
+     struct pl022_spi_pdata *plat = bus->platdata;
+     const void *fdt = gd->fdt_blob;
+     int node = dev_of_offset(bus);
+     struct clk clkdev;
+     int ret;
+
+     plat->addr = fdtdec_get_addr_size(fdt, node, "reg", &plat->size);
+
+     ret = clk_get_by_index(bus, 0, &clkdev);
+     if (ret)
+             return ret;
+
+     plat->freq = clk_get_rate(&clkdev);
+
+     return 0;
+}
+
 static const struct udevice_id pl022_spi_ids[] = {
      { .compatible = "arm,pl022-spi" },
      { }

@@ -327,11 +321,9 @@ U_BOOT_DRIVER(pl022_spi) = {
      .id     = UCLASS_SPI,
 #if !CONFIG_IS_ENABLED(OF_PLATDATA)
      .of_match = pl022_spi_ids,
-#endif
-     .ops    = &pl022_spi_ops,
-#if !CONFIG_IS_ENABLED(OF_PLATDATA)
      .ofdata_to_platdata = pl022_spi_ofdata_to_platdata,
 #endif
+     .ops    = &pl022_spi_ops,
      .platdata_auto_alloc_size = sizeof(struct pl022_spi_pdata),
      .priv_auto_alloc_size = sizeof(struct pl022_spi_slave),
      .probe  = pl022_spi_probe,

include/dm/platform_data/pl022_spi.h +0 -0

index 77fe6da3cb..57d12ac912 100644
@@ -10,19 +10,10 @@
 #ifndef __PL022_SPI_H__
 #define __PL022_SPI_H__
 

include/dm/platform_data/pl022_spi.h line 17

Quoted 5 lines

include/dm/platform_data/pl022_spi.h +0 -5

-#if !CONFIG_IS_ENABLED(OF_PLATDATA)
-#include <clk.h>
-#endif
-#include <fdtdec.h>
-

As stated in your first version of the patch[1][2], I need fdtdec.h to be in this header file so that I can successfuly compile.

Quoted 129 lines · Quentin Schulz

Hi Jagan,

Quoted 125 lines · Jagan Teki

pl022 spi driver support both OF_CONTROL and PLATDATA, this patch is trying to simplify the code that differentiating platdata vs of_control.
- Move OF_CONTROL code at one place
- Handle clock setup code directly in pl022_spi_ofdata_to_platdata

Cc Quentin Schulz <[email protected]>

Signed-off-by Jagan Teki <[email protected]>

---
Changes for v3:
- none
Changes for v2:
- Update commit message
- Use struct clk for clkdev

 drivers/spi/pl022_spi.c              | 48 ++++++++++++----------------
 include/dm/platform_data/pl022_spi.h |  9 ------
 2 files changed, 20 insertions(+), 37 deletions(-)

drivers/spi/pl022_spi.c +20 -28

index 86b71d2e21..05f4f6f481 100644
@@ -72,11 +72,7 @@
 
 struct pl022_spi_slave {
      void *base;
-#if !CONFIG_IS_ENABLED(OF_PLATDATA)
-     struct clk clk;
-#else
      unsigned int freq;
-#endif
 };
 
 /*

@@ -96,30 +92,13 @@ static int pl022_is_supported(struct pl022_spi_slave *ps)
      return 0;
 }
 
-#if !CONFIG_IS_ENABLED(OF_PLATDATA)
-static int pl022_spi_ofdata_to_platdata(struct udevice *bus)
-{
-     struct pl022_spi_pdata *plat = bus->platdata;
-     const void *fdt = gd->fdt_blob;
-     int node = dev_of_offset(bus);
-
-     plat->addr = fdtdec_get_addr_size(fdt, node, "reg", &plat->size);
-
-     return clk_get_by_index(bus, 0, &plat->clk);
-}
-#endif
-
 static int pl022_spi_probe(struct udevice *bus)
 {
      struct pl022_spi_pdata *plat = dev_get_platdata(bus);
      struct pl022_spi_slave *ps = dev_get_priv(bus);
 
      ps->base = ioremap(plat->addr, plat->size);
-#if !CONFIG_IS_ENABLED(OF_PLATDATA)
-     ps->clk = plat->clk;
-#else
      ps->freq = plat->freq;
-#endif
 
      /* Check the PL022 version */
      if (!pl022_is_supported(ps))

@@ -240,11 +219,7 @@ static int pl022_spi_set_speed(struct udevice *bus, uint speed)
      u16 scr = SSP_SCR_MIN, cr0 = 0, cpsr = SSP_CPSR_MIN, best_scr = scr,
          best_cpsr = cpsr;
      u32 min, max, best_freq = 0, tmp;
-#if !CONFIG_IS_ENABLED(OF_PLATDATA)
-     u32 rate = clk_get_rate(&ps->clk);
-#else
      u32 rate = ps->freq;
-#endif
      bool found = false;
 
      max = spi_rate(rate, SSP_CPSR_MIN, SSP_SCR_MIN);

@@ -316,6 +291,25 @@ static const struct dm_spi_ops pl022_spi_ops = {
 };
 
 #if !CONFIG_IS_ENABLED(OF_PLATDATA)
+static int pl022_spi_ofdata_to_platdata(struct udevice *bus)
+{
+     struct pl022_spi_pdata *plat = bus->platdata;
+     const void *fdt = gd->fdt_blob;
+     int node = dev_of_offset(bus);
+     struct clk clkdev;
+     int ret;
+
+     plat->addr = fdtdec_get_addr_size(fdt, node, "reg", &plat->size);
+
+     ret = clk_get_by_index(bus, 0, &clkdev);
+     if (ret)
+             return ret;
+
+     plat->freq = clk_get_rate(&clkdev);
+
+     return 0;
+}
+
 static const struct udevice_id pl022_spi_ids[] = {
      { .compatible = "arm,pl022-spi" },
      { }

@@ -327,11 +321,9 @@ U_BOOT_DRIVER(pl022_spi) = {
      .id     = UCLASS_SPI,
 #if !CONFIG_IS_ENABLED(OF_PLATDATA)
      .of_match = pl022_spi_ids,
-#endif
-     .ops    = &pl022_spi_ops,
-#if !CONFIG_IS_ENABLED(OF_PLATDATA)
      .ofdata_to_platdata = pl022_spi_ofdata_to_platdata,
 #endif
+     .ops    = &pl022_spi_ops,
      .platdata_auto_alloc_size = sizeof(struct pl022_spi_pdata),
      .priv_auto_alloc_size = sizeof(struct pl022_spi_slave),
      .probe  = pl022_spi_probe,

include/dm/platform_data/pl022_spi.h +0 -0

index 77fe6da3cb..57d12ac912 100644
@@ -10,19 +10,10 @@
 #ifndef __PL022_SPI_H__
 #define __PL022_SPI_H__
 

Quoted 8 lines

include/dm/platform_data/pl022_spi.h line 17

Quoted 5 lines

include/dm/platform_data/pl022_spi.h +0 -5

-#if !CONFIG_IS_ENABLED(OF_PLATDATA)
-#include <clk.h>
-#endif
-#include <fdtdec.h>
-

As stated in your first version of the patch[1][2], I need fdtdec.h to be in this header file so that I can successfuly compile.

which board config I need to enable?

Hi Jagan,

Quoted 140 lines · Jagan Teki

Quoted 129 lines · Quentin Schulz

Hi Jagan,

Quoted 125 lines · Jagan Teki

pl022 spi driver support both OF_CONTROL and PLATDATA, this patch is trying to simplify the code that differentiating platdata vs of_control.
- Move OF_CONTROL code at one place
- Handle clock setup code directly in pl022_spi_ofdata_to_platdata

Cc Quentin Schulz <[email protected]>

Signed-off-by Jagan Teki <[email protected]>

---
Changes for v3:
- none
Changes for v2:
- Update commit message
- Use struct clk for clkdev

 drivers/spi/pl022_spi.c              | 48 ++++++++++++----------------
 include/dm/platform_data/pl022_spi.h |  9 ------
 2 files changed, 20 insertions(+), 37 deletions(-)

drivers/spi/pl022_spi.c +20 -28

index 86b71d2e21..05f4f6f481 100644
@@ -72,11 +72,7 @@
 
 struct pl022_spi_slave {
      void *base;
-#if !CONFIG_IS_ENABLED(OF_PLATDATA)
-     struct clk clk;
-#else
      unsigned int freq;
-#endif
 };
 
 /*

@@ -96,30 +92,13 @@ static int pl022_is_supported(struct pl022_spi_slave *ps)
      return 0;
 }
 
-#if !CONFIG_IS_ENABLED(OF_PLATDATA)
-static int pl022_spi_ofdata_to_platdata(struct udevice *bus)
-{
-     struct pl022_spi_pdata *plat = bus->platdata;
-     const void *fdt = gd->fdt_blob;
-     int node = dev_of_offset(bus);
-
-     plat->addr = fdtdec_get_addr_size(fdt, node, "reg", &plat->size);
-
-     return clk_get_by_index(bus, 0, &plat->clk);
-}
-#endif
-
 static int pl022_spi_probe(struct udevice *bus)
 {
      struct pl022_spi_pdata *plat = dev_get_platdata(bus);
      struct pl022_spi_slave *ps = dev_get_priv(bus);
 
      ps->base = ioremap(plat->addr, plat->size);
-#if !CONFIG_IS_ENABLED(OF_PLATDATA)
-     ps->clk = plat->clk;
-#else
      ps->freq = plat->freq;
-#endif
 
      /* Check the PL022 version */
      if (!pl022_is_supported(ps))

@@ -240,11 +219,7 @@ static int pl022_spi_set_speed(struct udevice *bus, uint speed)
      u16 scr = SSP_SCR_MIN, cr0 = 0, cpsr = SSP_CPSR_MIN, best_scr = scr,
          best_cpsr = cpsr;
      u32 min, max, best_freq = 0, tmp;
-#if !CONFIG_IS_ENABLED(OF_PLATDATA)
-     u32 rate = clk_get_rate(&ps->clk);
-#else
      u32 rate = ps->freq;
-#endif
      bool found = false;
 
      max = spi_rate(rate, SSP_CPSR_MIN, SSP_SCR_MIN);

@@ -316,6 +291,25 @@ static const struct dm_spi_ops pl022_spi_ops = {
 };
 
 #if !CONFIG_IS_ENABLED(OF_PLATDATA)
+static int pl022_spi_ofdata_to_platdata(struct udevice *bus)
+{
+     struct pl022_spi_pdata *plat = bus->platdata;
+     const void *fdt = gd->fdt_blob;
+     int node = dev_of_offset(bus);
+     struct clk clkdev;
+     int ret;
+
+     plat->addr = fdtdec_get_addr_size(fdt, node, "reg", &plat->size);
+
+     ret = clk_get_by_index(bus, 0, &clkdev);
+     if (ret)
+             return ret;
+
+     plat->freq = clk_get_rate(&clkdev);
+
+     return 0;
+}
+
 static const struct udevice_id pl022_spi_ids[] = {
      { .compatible = "arm,pl022-spi" },
      { }

@@ -327,11 +321,9 @@ U_BOOT_DRIVER(pl022_spi) = {
      .id     = UCLASS_SPI,
 #if !CONFIG_IS_ENABLED(OF_PLATDATA)
      .of_match = pl022_spi_ids,
-#endif
-     .ops    = &pl022_spi_ops,
-#if !CONFIG_IS_ENABLED(OF_PLATDATA)
      .ofdata_to_platdata = pl022_spi_ofdata_to_platdata,
 #endif
+     .ops    = &pl022_spi_ops,
      .platdata_auto_alloc_size = sizeof(struct pl022_spi_pdata),
      .priv_auto_alloc_size = sizeof(struct pl022_spi_slave),
      .probe  = pl022_spi_probe,

include/dm/platform_data/pl022_spi.h +0 -0

index 77fe6da3cb..57d12ac912 100644
@@ -10,19 +10,10 @@
 #ifndef __PL022_SPI_H__
 #define __PL022_SPI_H__
 

Quoted 8 lines

include/dm/platform_data/pl022_spi.h line 17

Quoted 5 lines

include/dm/platform_data/pl022_spi.h +0 -5

-#if !CONFIG_IS_ENABLED(OF_PLATDATA)
-#include <clk.h>
-#endif
-#include <fdtdec.h>
-

As stated in your first version of the patch[1][2], I need fdtdec.h to be in this header file so that I can successfuly compile.

Quoted 1 line

which board config I need to enable?

Not mainline.

The point I'm trying to make[2], is that ANY board defining platdata in a board file will need the include/dm/platform_data/pl022_spi.h header, this is the reason it's there, to be reused outside of the driver.

With your patch, each and every board file that will need to define a U_BOOT_DEVICE entry with .platdata being of type struct pl022_spi_pdata will need to include the fdtdec header because in this structure, we have both fdt_addr_t and fdt_size_t that are used which are only defined in include/fdtdec.h[3].

Your patch is wrong, because:
1) It breaks the compilation on my side. While I could hear the argument of "it's not mainline we don't care", there is 2)

2) It's absolutely horrendous design to rely on each header file or C file including this header to include also fdtdec.h. With this mindset, let's not include any header file except in the final C file. You include the header file where you use parts of it. Here we use fdt_addr_t and fdt_size_t in include/dm/platform_data/pl022_spi.h which are both defined in include/fdtdec.h.

Quoted 163 lines · Quentin Schulz

Hi Jagan,

Quoted 140 lines · Jagan Teki

Quoted 129 lines · Quentin Schulz

Hi Jagan,

Quoted 125 lines · Jagan Teki

pl022 spi driver support both OF_CONTROL and PLATDATA, this patch is trying to simplify the code that differentiating platdata vs of_control.
- Move OF_CONTROL code at one place
- Handle clock setup code directly in pl022_spi_ofdata_to_platdata

Cc Quentin Schulz <[email protected]>

Signed-off-by Jagan Teki <[email protected]>

---
Changes for v3:
- none
Changes for v2:
- Update commit message
- Use struct clk for clkdev

 drivers/spi/pl022_spi.c              | 48 ++++++++++++----------------
 include/dm/platform_data/pl022_spi.h |  9 ------
 2 files changed, 20 insertions(+), 37 deletions(-)

drivers/spi/pl022_spi.c +20 -28

index 86b71d2e21..05f4f6f481 100644
@@ -72,11 +72,7 @@
 
 struct pl022_spi_slave {
      void *base;
-#if !CONFIG_IS_ENABLED(OF_PLATDATA)
-     struct clk clk;
-#else
      unsigned int freq;
-#endif
 };
 
 /*

@@ -96,30 +92,13 @@ static int pl022_is_supported(struct pl022_spi_slave *ps)
      return 0;
 }
 
-#if !CONFIG_IS_ENABLED(OF_PLATDATA)
-static int pl022_spi_ofdata_to_platdata(struct udevice *bus)
-{
-     struct pl022_spi_pdata *plat = bus->platdata;
-     const void *fdt = gd->fdt_blob;
-     int node = dev_of_offset(bus);
-
-     plat->addr = fdtdec_get_addr_size(fdt, node, "reg", &plat->size);
-
-     return clk_get_by_index(bus, 0, &plat->clk);
-}
-#endif
-
 static int pl022_spi_probe(struct udevice *bus)
 {
      struct pl022_spi_pdata *plat = dev_get_platdata(bus);
      struct pl022_spi_slave *ps = dev_get_priv(bus);
 
      ps->base = ioremap(plat->addr, plat->size);
-#if !CONFIG_IS_ENABLED(OF_PLATDATA)
-     ps->clk = plat->clk;
-#else
      ps->freq = plat->freq;
-#endif
 
      /* Check the PL022 version */
      if (!pl022_is_supported(ps))

@@ -240,11 +219,7 @@ static int pl022_spi_set_speed(struct udevice *bus, uint speed)
      u16 scr = SSP_SCR_MIN, cr0 = 0, cpsr = SSP_CPSR_MIN, best_scr = scr,
          best_cpsr = cpsr;
      u32 min, max, best_freq = 0, tmp;
-#if !CONFIG_IS_ENABLED(OF_PLATDATA)
-     u32 rate = clk_get_rate(&ps->clk);
-#else
      u32 rate = ps->freq;
-#endif
      bool found = false;
 
      max = spi_rate(rate, SSP_CPSR_MIN, SSP_SCR_MIN);

@@ -316,6 +291,25 @@ static const struct dm_spi_ops pl022_spi_ops = {
 };
 
 #if !CONFIG_IS_ENABLED(OF_PLATDATA)
+static int pl022_spi_ofdata_to_platdata(struct udevice *bus)
+{
+     struct pl022_spi_pdata *plat = bus->platdata;
+     const void *fdt = gd->fdt_blob;
+     int node = dev_of_offset(bus);
+     struct clk clkdev;
+     int ret;
+
+     plat->addr = fdtdec_get_addr_size(fdt, node, "reg", &plat->size);
+
+     ret = clk_get_by_index(bus, 0, &clkdev);
+     if (ret)
+             return ret;
+
+     plat->freq = clk_get_rate(&clkdev);
+
+     return 0;
+}
+
 static const struct udevice_id pl022_spi_ids[] = {
      { .compatible = "arm,pl022-spi" },
      { }

@@ -327,11 +321,9 @@ U_BOOT_DRIVER(pl022_spi) = {
      .id     = UCLASS_SPI,
 #if !CONFIG_IS_ENABLED(OF_PLATDATA)
      .of_match = pl022_spi_ids,
-#endif
-     .ops    = &pl022_spi_ops,
-#if !CONFIG_IS_ENABLED(OF_PLATDATA)
      .ofdata_to_platdata = pl022_spi_ofdata_to_platdata,
 #endif
+     .ops    = &pl022_spi_ops,
      .platdata_auto_alloc_size = sizeof(struct pl022_spi_pdata),
      .priv_auto_alloc_size = sizeof(struct pl022_spi_slave),
      .probe  = pl022_spi_probe,

include/dm/platform_data/pl022_spi.h +0 -0

index 77fe6da3cb..57d12ac912 100644
@@ -10,19 +10,10 @@
 #ifndef __PL022_SPI_H__
 #define __PL022_SPI_H__
 

Quoted 8 lines

include/dm/platform_data/pl022_spi.h line 17

Quoted 5 lines

include/dm/platform_data/pl022_spi.h +0 -5

-#if !CONFIG_IS_ENABLED(OF_PLATDATA)
-#include <clk.h>
-#endif
-#include <fdtdec.h>
-

As stated in your first version of the patch[1][2], I need fdtdec.h to be in this header file so that I can successfuly compile.

Quoted 1 line

which board config I need to enable?

Not mainline.

The point I'm trying to make[2], is that ANY board defining platdata in a board file will need the include/dm/platform_data/pl022_spi.h header, this is the reason it's there, to be reused outside of the driver.

With your patch, each and every board file that will need to define a U_BOOT_DEVICE entry with .platdata being of type struct pl022_spi_pdata will need to include the fdtdec header because in this structure, we have both fdt_addr_t and fdt_size_t that are used which are only defined in include/fdtdec.h[3].

Your patch is wrong, because:
1) It breaks the compilation on my side. While I could hear the argument of "it's not mainline we don't care", there is 2)

Quoted 6 lines

2) It's absolutely horrendous design to rely on each header file or C file including this header to include also fdtdec.h. With this mindset, let's not include any header file except in the final C file. You include the header file where you use parts of it. Here we use fdt_addr_t and fdt_size_t in include/dm/platform_data/pl022_spi.h which are both defined in include/fdtdec.h.

Got your point, didn't notice that the driver is using devfdt_get_addr. I think we can switch to recent devfdt function to skip the fdtdec.h. like using devfdt_get_addr and devm_ioremap

Hi Jagan,

Quoted 172 lines · Jagan Teki

Quoted 163 lines · Quentin Schulz

Hi Jagan,

Quoted 140 lines · Jagan Teki

Quoted 129 lines · Quentin Schulz

Hi Jagan,

Quoted 125 lines · Jagan Teki

pl022 spi driver support both OF_CONTROL and PLATDATA, this patch is trying to simplify the code that differentiating platdata vs of_control.
- Move OF_CONTROL code at one place
- Handle clock setup code directly in pl022_spi_ofdata_to_platdata

Cc Quentin Schulz <[email protected]>

Signed-off-by Jagan Teki <[email protected]>

---
Changes for v3:
- none
Changes for v2:
- Update commit message
- Use struct clk for clkdev

 drivers/spi/pl022_spi.c              | 48 ++++++++++++----------------
 include/dm/platform_data/pl022_spi.h |  9 ------
 2 files changed, 20 insertions(+), 37 deletions(-)

drivers/spi/pl022_spi.c +20 -28

index 86b71d2e21..05f4f6f481 100644
@@ -72,11 +72,7 @@
 
 struct pl022_spi_slave {
      void *base;
-#if !CONFIG_IS_ENABLED(OF_PLATDATA)
-     struct clk clk;
-#else
      unsigned int freq;
-#endif
 };
 
 /*

@@ -96,30 +92,13 @@ static int pl022_is_supported(struct pl022_spi_slave *ps)
      return 0;
 }
 
-#if !CONFIG_IS_ENABLED(OF_PLATDATA)
-static int pl022_spi_ofdata_to_platdata(struct udevice *bus)
-{
-     struct pl022_spi_pdata *plat = bus->platdata;
-     const void *fdt = gd->fdt_blob;
-     int node = dev_of_offset(bus);
-
-     plat->addr = fdtdec_get_addr_size(fdt, node, "reg", &plat->size);
-
-     return clk_get_by_index(bus, 0, &plat->clk);
-}
-#endif
-
 static int pl022_spi_probe(struct udevice *bus)
 {
      struct pl022_spi_pdata *plat = dev_get_platdata(bus);
      struct pl022_spi_slave *ps = dev_get_priv(bus);
 
      ps->base = ioremap(plat->addr, plat->size);
-#if !CONFIG_IS_ENABLED(OF_PLATDATA)
-     ps->clk = plat->clk;
-#else
      ps->freq = plat->freq;
-#endif
 
      /* Check the PL022 version */
      if (!pl022_is_supported(ps))

@@ -240,11 +219,7 @@ static int pl022_spi_set_speed(struct udevice *bus, uint speed)
      u16 scr = SSP_SCR_MIN, cr0 = 0, cpsr = SSP_CPSR_MIN, best_scr = scr,
          best_cpsr = cpsr;
      u32 min, max, best_freq = 0, tmp;
-#if !CONFIG_IS_ENABLED(OF_PLATDATA)
-     u32 rate = clk_get_rate(&ps->clk);
-#else
      u32 rate = ps->freq;
-#endif
      bool found = false;
 
      max = spi_rate(rate, SSP_CPSR_MIN, SSP_SCR_MIN);

@@ -316,6 +291,25 @@ static const struct dm_spi_ops pl022_spi_ops = {
 };
 
 #if !CONFIG_IS_ENABLED(OF_PLATDATA)
+static int pl022_spi_ofdata_to_platdata(struct udevice *bus)
+{
+     struct pl022_spi_pdata *plat = bus->platdata;
+     const void *fdt = gd->fdt_blob;
+     int node = dev_of_offset(bus);
+     struct clk clkdev;
+     int ret;
+
+     plat->addr = fdtdec_get_addr_size(fdt, node, "reg", &plat->size);
+
+     ret = clk_get_by_index(bus, 0, &clkdev);
+     if (ret)
+             return ret;
+
+     plat->freq = clk_get_rate(&clkdev);
+
+     return 0;
+}
+
 static const struct udevice_id pl022_spi_ids[] = {
      { .compatible = "arm,pl022-spi" },
      { }

@@ -327,11 +321,9 @@ U_BOOT_DRIVER(pl022_spi) = {
      .id     = UCLASS_SPI,
 #if !CONFIG_IS_ENABLED(OF_PLATDATA)
      .of_match = pl022_spi_ids,
-#endif
-     .ops    = &pl022_spi_ops,
-#if !CONFIG_IS_ENABLED(OF_PLATDATA)
      .ofdata_to_platdata = pl022_spi_ofdata_to_platdata,
 #endif
+     .ops    = &pl022_spi_ops,
      .platdata_auto_alloc_size = sizeof(struct pl022_spi_pdata),
      .priv_auto_alloc_size = sizeof(struct pl022_spi_slave),
      .probe  = pl022_spi_probe,

include/dm/platform_data/pl022_spi.h +0 -0

index 77fe6da3cb..57d12ac912 100644
@@ -10,19 +10,10 @@
 #ifndef __PL022_SPI_H__
 #define __PL022_SPI_H__
 

Quoted 8 lines

include/dm/platform_data/pl022_spi.h line 17

Quoted 5 lines

include/dm/platform_data/pl022_spi.h +0 -5

-#if !CONFIG_IS_ENABLED(OF_PLATDATA)
-#include <clk.h>
-#endif
-#include <fdtdec.h>
-

As stated in your first version of the patch[1][2], I need fdtdec.h to be in this header file so that I can successfuly compile.

Quoted 1 line

which board config I need to enable?

Not mainline.

The point I'm trying to make[2], is that ANY board defining platdata in a board file will need the include/dm/platform_data/pl022_spi.h header, this is the reason it's there, to be reused outside of the driver.

With your patch, each and every board file that will need to define a U_BOOT_DEVICE entry with .platdata being of type struct pl022_spi_pdata will need to include the fdtdec header because in this structure, we have both fdt_addr_t and fdt_size_t that are used which are only defined in include/fdtdec.h[3].

Your patch is wrong, because:
1) It breaks the compilation on my side. While I could hear the argument of "it's not mainline we don't care", there is 2)

Quoted 6 lines

2) It's absolutely horrendous design to rely on each header file or C file including this header to include also fdtdec.h. With this mindset, let's not include any header file except in the final C file. You include the header file where you use parts of it. Here we use fdt_addr_t and fdt_size_t in include/dm/platform_data/pl022_spi.h which are both defined in include/fdtdec.h.

Quoted 3 lines

Got your point, didn't notice that the driver is using devfdt_get_addr. I think we can switch to recent devfdt function to skip the fdtdec.h. like using devfdt_get_addr and devm_ioremap

You will NOT be able to get rid of fdtdec.h.

In the include/dm/platform_data/pl022_spi.h header file you have the pl022_spi_pdata structure which have two variables, fdt_addr_t addr and fdt_size_t size. fdt_addr_t and fdt_size_t are only defined in fdtdec.h header file[1][2].

So the only way to get rid of fdtdec.h is to get rid of those two variables in the pl022_spi_pdata structure.