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_ioremapYou 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
