[U-Boot] [PATCH 1/2] spi: pl022: Get rid of platdata ifdef's
Quentin Schulz
quentin.schulz at bootlin.com
Wed Nov 14 10:30:57 UTC 2018
Hi Jagan,
On Wed, Nov 14, 2018 at 03:11:26PM +0530, Jagan Teki wrote:
> On Mon, Nov 5, 2018 at 3:12 PM Quentin Schulz
> <quentin.schulz at bootlin.com> wrote:
> >
> > Hi Jagan,
> >
> > On Mon, Nov 05, 2018 at 02:49:25PM +0530, Jagan Teki wrote:
> > > pl022 spi driver support both OF_CONTROL and PLATDATA and
> > > it's using #ifdef check for differentiating platdata vs of_control.
> > > So this patch simplify the code and drop the ifdefs
> > >
> >
> > You drop a few ifdef, not all. I also find this commit log a bit cryptic.
>
> Dropped ifdef which differentiate platdata vs ofcontrol, ie what I
> mentioned in the commit.
>
IMHO, your commit log implies you dropped all the ifdefs to
differentiate between platdata and of_control. That is not true, there
are still some left.
You indeed remove a few but ultimately, it's a consequence of
simplifying the driver to store the frequency of the clock gotten from
DM instead of storing the clock itself. This is just fine for this
commit log.
> > You basically store the frequency of the clock gotten from DM instead of
> > storing the clock itself, which is indeed a welcome simplification of
> > the driver (and you re-order function pointers in the driver structure).
> >
> > > Cc: Quentin Schulz <quentin.schulz at bootlin.com>
> > > Signed-off-by: Jagan Teki <jagan at amarulasolutions.com>
> > > ---
> > > 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..dd27444a43 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);
> >
> > We don't need the u32 rate temp variable anymore so better replace it in
> > this function with ps->freq directly.
>
> rate is using many places, it's okay to have temp.
>
Three, but fine.
> >
> > > @@ -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;
> > > + struct udevice *clkdev;
> > > + const void *fdt = gd->fdt_blob;
> > > + int node = dev_of_offset(bus);
> > > + 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>
> >
> > Nope, I need this one else it does not compile.
>
> I don't think we need this include here. one thing need to fix with is
> clkdev variable, I did that and it's compiled.
The issue I'm having is with fdt_addr_t and fdt_size_t. Any board file
or driver that would include this header would in turn need to include
fdtdec.h. Couldn't we just leave the fdtdec.h header inclusion in this
file so that we don't have to add it in all the files that include the
include/dm/platform_data/pl022_spi.h header please?
Thanks,
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/20181114/7689b87a/attachment.sig>
More information about the U-Boot
mailing list