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.sch...@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.sch...@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.sch...@bootlin.com> > > > > > Signed-off-by: Jagan Teki <ja...@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
signature.asc
Description: PGP signature
_______________________________________________ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot