[U-Boot] [PATCH v3 1/3] spi: pl022: Simplify platdata code

[U-Boot] [PATCH v3 1/3] spi: pl022: Simplify platdata code

[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 <quentin.schulz@bootlin.com>
Signed-off-by: Jagan Teki <jagan@amarulasolutions.com>
---
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(-)

diff --git a/drivers/spi/pl022_spi.c b/drivers/spi/pl022_spi.c
index 86b71d2e21..05f4f6f481 100644
--- a/drivers/spi/pl022_spi.c
+++ b/drivers/spi/pl022_spi.c
@@ -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,
diff --git a/include/dm/platform_data/pl022_spi.h b/include/dm/platform_data/pl022_spi.h
index 77fe6da3cb..57d12ac912 100644
--- a/include/dm/platform_data/pl022_spi.h
+++ b/include/dm/platform_data/pl022_spi.h
@@ -10,19 +10,10 @@
 #ifndef __PL022_SPI_H__
 #define __PL022_SPI_H__
 
-#if !CONFIG_IS_ENABLED(OF_PLATDATA)
-#include <clk.h>
-#endif
-#include <fdtdec.h>
-
 struct pl022_spi_pdata {
 	fdt_addr_t addr;
 	fdt_size_t size;
-#if !CONFIG_IS_ENABLED(OF_PLATDATA)
-	struct clk clk;
-#else
 	unsigned int freq;
-#endif
 };
 
 #endif
-- 
2.18.0.321.gffc6fa0e3

[U-Boot] [PATCH v3 2/3] spi: pl022: Drop unnecessary include files

[Jagan Teki]

This patch can drop unnecessary include files in
pl022_spi driver.

Cc: Quentin Schulz <quentin.schulz@bootlin.com>
Signed-off-by: Jagan Teki <jagan@amarulasolutions.com>
---
Changes for v3:
- split patch from previous

 drivers/spi/pl022_spi.c | 5 -----
 1 file changed, 5 deletions(-)

diff --git a/drivers/spi/pl022_spi.c b/drivers/spi/pl022_spi.c
index 05f4f6f481..f2e5367225 100644
--- a/drivers/spi/pl022_spi.c
+++ b/drivers/spi/pl022_spi.c
@@ -9,16 +9,11 @@
  * Driver for ARM PL022 SPI Controller.
  */
 
-#include <asm/io.h>
 #include <clk.h>
 #include <common.h>
 #include <dm.h>
 #include <dm/platform_data/pl022_spi.h>
-#include <fdtdec.h>
-#include <linux/bitops.h>
-#include <linux/bug.h>
 #include <linux/io.h>
-#include <linux/kernel.h>
 #include <spi.h>
 
 #define SSP_CR0		0x000
-- 
2.18.0.321.gffc6fa0e3

[U-Boot] [PATCH v3 3/3] dm: platform_data: spi: s/pl022_spi.h/spi_pl022.h

[Jagan Teki]

Rename platform_data include file as spi_pl022.h from pl022_spi.h,
this is generic notation used for spi platdata include files.

Cc: Quentin Schulz <quentin.schulz@bootlin.com>
Signed-off-by: Jagan Teki <jagan@amarulasolutions.com>
---
Changes for v3:
- split patch from previous

 drivers/spi/pl022_spi.c                               | 2 +-
 include/dm/platform_data/{pl022_spi.h => spi_pl022.h} | 6 +++---
 2 files changed, 4 insertions(+), 4 deletions(-)
 rename include/dm/platform_data/{pl022_spi.h => spi_pl022.h} (81%)

diff --git a/drivers/spi/pl022_spi.c b/drivers/spi/pl022_spi.c
index f2e5367225..32bb8c8d21 100644
--- a/drivers/spi/pl022_spi.c
+++ b/drivers/spi/pl022_spi.c
@@ -12,7 +12,7 @@
 #include <clk.h>
 #include <common.h>
 #include <dm.h>
-#include <dm/platform_data/pl022_spi.h>
+#include <dm/platform_data/spi_pl022.h>
 #include <linux/io.h>
 #include <spi.h>
 
diff --git a/include/dm/platform_data/pl022_spi.h b/include/dm/platform_data/spi_pl022.h
similarity index 81%
rename from include/dm/platform_data/pl022_spi.h
rename to include/dm/platform_data/spi_pl022.h
index 57d12ac912..36e645c836 100644
--- a/include/dm/platform_data/pl022_spi.h
+++ b/include/dm/platform_data/spi_pl022.h
@@ -7,8 +7,8 @@
  * in ofdata_to_platdata.
  */
 
-#ifndef __PL022_SPI_H__
-#define __PL022_SPI_H__
+#ifndef __spi_pl022_h
+#define __spi_pl022_h
 
 struct pl022_spi_pdata {
 	fdt_addr_t addr;
@@ -16,4 +16,4 @@ struct pl022_spi_pdata {
 	unsigned int freq;
 };
 
-#endif
+#endif /* __spi_pl022_h */
-- 
2.18.0.321.gffc6fa0e3

[U-Boot] [PATCH v3 1/3] spi: pl022: Simplify platdata code

[Quentin Schulz]

Hi Jagan,

On Thu, Nov 22, 2018 at 12:03:47PM +0530, Jagan Teki wrote:
> 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 <quentin.schulz@bootlin.com>
> Signed-off-by: Jagan Teki <jagan@amarulasolutions.com>
> ---
> 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(-)
> 
> diff --git a/drivers/spi/pl022_spi.c b/drivers/spi/pl022_spi.c
> index 86b71d2e21..05f4f6f481 100644
> --- a/drivers/spi/pl022_spi.c
> +++ b/drivers/spi/pl022_spi.c
> @@ -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,
> diff --git a/include/dm/platform_data/pl022_spi.h b/include/dm/platform_data/pl022_spi.h
> index 77fe6da3cb..57d12ac912 100644
> --- a/include/dm/platform_data/pl022_spi.h
> +++ b/include/dm/platform_data/pl022_spi.h
> @@ -10,19 +10,10 @@
>  #ifndef __PL022_SPI_H__
>  #define __PL022_SPI_H__
>  
> -#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.

[1] https://lists.denx.de/pipermail/u-boot/2018-November/346470.html
[2] https://lists.denx.de/pipermail/u-boot/2018-November/347371.html

Quentin
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20181122/e3a251e6/attachment.sig>

[U-Boot] [PATCH v3 1/3] spi: pl022: Simplify platdata code

[Jagan Teki]

On Thu, Nov 22, 2018 at 1:21 PM Quentin Schulz
<quentin.schulz@bootlin.com> wrote:
>
> Hi Jagan,
>
> On Thu, Nov 22, 2018 at 12:03:47PM +0530, Jagan Teki wrote:
> > 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 <quentin.schulz@bootlin.com>
> > Signed-off-by: Jagan Teki <jagan@amarulasolutions.com>
> > ---
> > 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(-)
> >
> > diff --git a/drivers/spi/pl022_spi.c b/drivers/spi/pl022_spi.c
> > index 86b71d2e21..05f4f6f481 100644
> > --- a/drivers/spi/pl022_spi.c
> > +++ b/drivers/spi/pl022_spi.c
> > @@ -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,
> > diff --git a/include/dm/platform_data/pl022_spi.h b/include/dm/platform_data/pl022_spi.h
> > index 77fe6da3cb..57d12ac912 100644
> > --- a/include/dm/platform_data/pl022_spi.h
> > +++ b/include/dm/platform_data/pl022_spi.h
> > @@ -10,19 +10,10 @@
> >  #ifndef __PL022_SPI_H__
> >  #define __PL022_SPI_H__
> >
> > -#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?

[U-Boot] [PATCH v3 1/3] spi: pl022: Simplify platdata code

[Quentin Schulz]

Hi Jagan,

On Thu, Nov 22, 2018 at 01:31:25PM +0530, Jagan Teki wrote:
> On Thu, Nov 22, 2018 at 1:21 PM Quentin Schulz
> <quentin.schulz@bootlin.com> wrote:
> >
> > Hi Jagan,
> >
> > On Thu, Nov 22, 2018 at 12:03:47PM +0530, Jagan Teki wrote:
> > > 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 <quentin.schulz@bootlin.com>
> > > Signed-off-by: Jagan Teki <jagan@amarulasolutions.com>
> > > ---
> > > 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(-)
> > >
> > > diff --git a/drivers/spi/pl022_spi.c b/drivers/spi/pl022_spi.c
> > > index 86b71d2e21..05f4f6f481 100644
> > > --- a/drivers/spi/pl022_spi.c
> > > +++ b/drivers/spi/pl022_spi.c
> > > @@ -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,
> > > diff --git a/include/dm/platform_data/pl022_spi.h b/include/dm/platform_data/pl022_spi.h
> > > index 77fe6da3cb..57d12ac912 100644
> > > --- a/include/dm/platform_data/pl022_spi.h
> > > +++ b/include/dm/platform_data/pl022_spi.h
> > > @@ -10,19 +10,10 @@
> > >  #ifndef __PL022_SPI_H__
> > >  #define __PL022_SPI_H__
> > >
> > > -#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?

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.

[2] https://lists.denx.de/pipermail/u-boot/2018-November/347371.html
[3] https://elixir.bootlin.com/u-boot/latest/source/include/fdtdec.h#L25

Quentin
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20181122/ebb94c99/attachment.sig>

[U-Boot] [PATCH v3 1/3] spi: pl022: Simplify platdata code

[Jagan Teki]

On Thu, Nov 22, 2018 at 1:48 PM Quentin Schulz
<quentin.schulz@bootlin.com> wrote:
>
> Hi Jagan,
>
> On Thu, Nov 22, 2018 at 01:31:25PM +0530, Jagan Teki wrote:
> > On Thu, Nov 22, 2018 at 1:21 PM Quentin Schulz
> > <quentin.schulz@bootlin.com> wrote:
> > >
> > > Hi Jagan,
> > >
> > > On Thu, Nov 22, 2018 at 12:03:47PM +0530, Jagan Teki wrote:
> > > > 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 <quentin.schulz@bootlin.com>
> > > > Signed-off-by: Jagan Teki <jagan@amarulasolutions.com>
> > > > ---
> > > > 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(-)
> > > >
> > > > diff --git a/drivers/spi/pl022_spi.c b/drivers/spi/pl022_spi.c
> > > > index 86b71d2e21..05f4f6f481 100644
> > > > --- a/drivers/spi/pl022_spi.c
> > > > +++ b/drivers/spi/pl022_spi.c
> > > > @@ -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,
> > > > diff --git a/include/dm/platform_data/pl022_spi.h b/include/dm/platform_data/pl022_spi.h
> > > > index 77fe6da3cb..57d12ac912 100644
> > > > --- a/include/dm/platform_data/pl022_spi.h
> > > > +++ b/include/dm/platform_data/pl022_spi.h
> > > > @@ -10,19 +10,10 @@
> > > >  #ifndef __PL022_SPI_H__
> > > >  #define __PL022_SPI_H__
> > > >
> > > > -#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?
>
> 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.

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

[U-Boot] [PATCH v3 1/3] spi: pl022: Simplify platdata code

[Quentin Schulz]

Hi Jagan,

On Sat, Nov 24, 2018 at 05:41:03PM +0530, Jagan Teki wrote:
> On Thu, Nov 22, 2018 at 1:48 PM Quentin Schulz
> <quentin.schulz@bootlin.com> wrote:
> >
> > Hi Jagan,
> >
> > On Thu, Nov 22, 2018 at 01:31:25PM +0530, Jagan Teki wrote:
> > > On Thu, Nov 22, 2018 at 1:21 PM Quentin Schulz
> > > <quentin.schulz@bootlin.com> wrote:
> > > >
> > > > Hi Jagan,
> > > >
> > > > On Thu, Nov 22, 2018 at 12:03:47PM +0530, Jagan Teki wrote:
> > > > > 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 <quentin.schulz@bootlin.com>
> > > > > Signed-off-by: Jagan Teki <jagan@amarulasolutions.com>
> > > > > ---
> > > > > 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(-)
> > > > >
> > > > > diff --git a/drivers/spi/pl022_spi.c b/drivers/spi/pl022_spi.c
> > > > > index 86b71d2e21..05f4f6f481 100644
> > > > > --- a/drivers/spi/pl022_spi.c
> > > > > +++ b/drivers/spi/pl022_spi.c
> > > > > @@ -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,
> > > > > diff --git a/include/dm/platform_data/pl022_spi.h b/include/dm/platform_data/pl022_spi.h
> > > > > index 77fe6da3cb..57d12ac912 100644
> > > > > --- a/include/dm/platform_data/pl022_spi.h
> > > > > +++ b/include/dm/platform_data/pl022_spi.h
> > > > > @@ -10,19 +10,10 @@
> > > > >  #ifndef __PL022_SPI_H__
> > > > >  #define __PL022_SPI_H__
> > > > >
> > > > > -#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?
> >
> > 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.
> 
> 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.

Quentin

[1] https://elixir.bootlin.com/u-boot/latest/ident/fdt_addr_t
[2] https://elixir.bootlin.com/u-boot/latest/ident/fdt_size_t
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20181125/e69b2a66/attachment.sig>

[U-Boot] [PATCH v3 1/3] spi: pl022: Simplify platdata code

[Jagan Teki]

On Sun, Nov 25, 2018 at 6:02 PM Quentin Schulz
<quentin.schulz@bootlin.com> wrote:
>
> Hi Jagan,
>
> On Sat, Nov 24, 2018 at 05:41:03PM +0530, Jagan Teki wrote:
> > On Thu, Nov 22, 2018 at 1:48 PM Quentin Schulz
> > <quentin.schulz@bootlin.com> wrote:
> > >
> > > Hi Jagan,
> > >
> > > On Thu, Nov 22, 2018 at 01:31:25PM +0530, Jagan Teki wrote:
> > > > On Thu, Nov 22, 2018 at 1:21 PM Quentin Schulz
> > > > <quentin.schulz@bootlin.com> wrote:
> > > > >
> > > > > Hi Jagan,
> > > > >
> > > > > On Thu, Nov 22, 2018 at 12:03:47PM +0530, Jagan Teki wrote:
> > > > > > 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 <quentin.schulz@bootlin.com>
> > > > > > Signed-off-by: Jagan Teki <jagan@amarulasolutions.com>
> > > > > > ---
> > > > > > 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(-)
> > > > > >
> > > > > > diff --git a/drivers/spi/pl022_spi.c b/drivers/spi/pl022_spi.c
> > > > > > index 86b71d2e21..05f4f6f481 100644
> > > > > > --- a/drivers/spi/pl022_spi.c
> > > > > > +++ b/drivers/spi/pl022_spi.c
> > > > > > @@ -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,
> > > > > > diff --git a/include/dm/platform_data/pl022_spi.h b/include/dm/platform_data/pl022_spi.h
> > > > > > index 77fe6da3cb..57d12ac912 100644
> > > > > > --- a/include/dm/platform_data/pl022_spi.h
> > > > > > +++ b/include/dm/platform_data/pl022_spi.h
> > > > > > @@ -10,19 +10,10 @@
> > > > > >  #ifndef __PL022_SPI_H__
> > > > > >  #define __PL022_SPI_H__
> > > > > >
> > > > > > -#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?
> > >
> > > 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.
> >
> > 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.

with adding structure pointer of reg space. anyway we can look it
later. are you fine with v4?