| Message ID | 20181105091926.4971-1-jagan@amarulasolutions.com |
|---|---|
| State | Changes Requested |
| Delegated to: | Jagannadha Sutradharudu Teki |
| Headers | show |
| Series | [U-Boot,1/2] spi: pl022: Get rid of platdata ifdef's | expand |
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. 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@bootlin.com> > Signed-off-by: Jagan Teki <jagan@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. > @@ -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. Thanks, Quentin
On Mon, Nov 5, 2018 at 3:12 PM Quentin Schulz <quentin.schulz@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. > 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@bootlin.com> > > Signed-off-by: Jagan Teki <jagan@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. > > > @@ -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.
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@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@bootlin.com> > > > Signed-off-by: Jagan Teki <jagan@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
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); @@ -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> - 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
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 Cc: Quentin Schulz <quentin.schulz@bootlin.com> Signed-off-by: Jagan Teki <jagan@amarulasolutions.com> --- drivers/spi/pl022_spi.c | 48 ++++++++++++---------------- include/dm/platform_data/pl022_spi.h | 9 ------ 2 files changed, 20 insertions(+), 37 deletions(-)