[U-Boot] [PATCH] spi: mxc_spi: support driver model
Peng Fan
peng.fan at nxp.com
Tue Aug 8 12:23:00 UTC 2017
> -----Original Message-----
> From: Lothar Waßmann [mailto:LW at KARO-electronics.de]
> Sent: Tuesday, August 08, 2017 6:20 PM
> To: Peng Fan <peng.fan at nxp.com>
> Cc: jagan at openedev.com; u-boot at lists.denx.de
> Subject: Re: [U-Boot] [PATCH] spi: mxc_spi: support driver model
>
> Hi,
>
> On Tue, 8 Aug 2017 18:00:01 +0800 Peng Fan wrote:
> > Add driver model support for mxc spi driver.
> > Most functions are restructured to be reused by DM and non-DM.
> > Tested on mx6slevk/mx6qsabresd board.
> >
> > Signed-off-by: Peng Fan <peng.fan at nxp.com>
> > Cc: Jagan Teki <jagan at openedev.com>
> > cc: Stefano Babic <sbabic at denx.de>
> > ---
> > drivers/spi/mxc_spi.c | 183
> > +++++++++++++++++++++++++++++++++++++++++---------
> > 1 file changed, 150 insertions(+), 33 deletions(-)
> >
> > diff --git a/drivers/spi/mxc_spi.c b/drivers/spi/mxc_spi.c index
> > e1562c3..44fed94 100644
> > --- a/drivers/spi/mxc_spi.c
> > +++ b/drivers/spi/mxc_spi.c
> > @@ -5,6 +5,7 @@
> > */
> >
> > #include <common.h>
> > +#include <dm.h>
> > #include <malloc.h>
> > #include <spi.h>
> > #include <linux/errno.h>
> > @@ -14,6 +15,8 @@
> > #include <asm/arch/clock.h>
> > #include <asm/mach-imx/spi.h>
> >
> > +DECLARE_GLOBAL_DATA_PTR;
> > +
> > #ifdef CONFIG_MX27
> > /* i.MX27 has a completely wrong register layout and register definitions in
> the
> > * datasheet, the correct one is in the Freescale's Linux driver */
> > @@ -22,10 +25,6 @@ "See linux mxc_spi driver from Freescale for
> > details."
> > #endif
> >
> > -static unsigned long spi_bases[] = {
> > - MXC_SPI_BASE_ADDRESSES
> > -};
> > -
> > __weak int board_spi_cs_gpio(unsigned bus, unsigned cs) {
> > return -1;
> > @@ -51,6 +50,7 @@ struct mxc_spi_slave {
> > int ss_pol;
> > unsigned int max_hz;
> > unsigned int mode;
> > + struct gpio_desc ss;
> > };
> >
> > static inline struct mxc_spi_slave *to_mxc_spi_slave(struct spi_slave
> > *slave) @@ -58,19 +58,24 @@ static inline struct mxc_spi_slave
> *to_mxc_spi_slave(struct spi_slave *slave)
> > return container_of(slave, struct mxc_spi_slave, slave); }
> >
> > -void spi_cs_activate(struct spi_slave *slave)
> > +static void mxc_spi_cs_activate(struct mxc_spi_slave *mxcs)
> > {
> > - struct mxc_spi_slave *mxcs = to_mxc_spi_slave(slave);
> > - if (mxcs->gpio > 0)
> > - gpio_set_value(mxcs->gpio, mxcs->ss_pol);
> > + if (CONFIG_IS_ENABLED(DM_SPI)) {
> >
> Shouldn't this be DM_GPIO rather than DM_SPI?
When DM_SPI is enabled, the mxcs->ss will be initialized in the driver probe.
So keep DM_SPI here.
>
> > + dm_gpio_set_value(&mxcs->ss, mxcs->ss_pol);
> > + } else {
> > + if (mxcs->gpio > 0)
> > + gpio_set_value(mxcs->gpio, mxcs->ss_pol);
> > + }
> > }
> >
> > -void spi_cs_deactivate(struct spi_slave *slave)
> > +static void mxc_spi_cs_deactivate(struct mxc_spi_slave *mxcs)
> > {
> > - struct mxc_spi_slave *mxcs = to_mxc_spi_slave(slave);
> > - if (mxcs->gpio > 0)
> > - gpio_set_value(mxcs->gpio,
> > - !(mxcs->ss_pol));
> > + if (CONFIG_IS_ENABLED(DM_SPI)) {
> dto.
Same as above.
>
> > + dm_gpio_set_value(&mxcs->ss, !(mxcs->ss_pol));
> > + } else {
> > + if (mxcs->gpio > 0)
> > + gpio_set_value(mxcs->gpio, !(mxcs->ss_pol));
> > + }
> > }
> >
> > u32 get_cspi_div(u32 div)
> > @@ -211,10 +216,9 @@ static s32 spi_cfg_mxc(struct mxc_spi_slave
> > *mxcs, unsigned int cs) } #endif
> >
> > -int spi_xchg_single(struct spi_slave *slave, unsigned int bitlen,
> > +int spi_xchg_single(struct mxc_spi_slave *mxcs, unsigned int bitlen,
> > const u8 *dout, u8 *din, unsigned long flags) {
> > - struct mxc_spi_slave *mxcs = to_mxc_spi_slave(slave);
> > int nbytes = DIV_ROUND_UP(bitlen, 8);
> > u32 data, cnt, i;
> > struct cspi_regs *regs = (struct cspi_regs *)mxcs->base; @@ -327,8
> > +331,9 @@ int spi_xchg_single(struct spi_slave *slave, unsigned int
> > bitlen,
> >
> > }
> >
> > -int spi_xfer(struct spi_slave *slave, unsigned int bitlen, const void *dout,
> > - void *din, unsigned long flags)
> > +static int mxc_spi_xfer_internal(struct mxc_spi_slave *mxcs,
> > + unsigned int bitlen, const void *dout,
> > + void *din, unsigned long flags)
> > {
> > int n_bytes = DIV_ROUND_UP(bitlen, 8);
> > int n_bits;
> > @@ -337,11 +342,11 @@ int spi_xfer(struct spi_slave *slave, unsigned int
> bitlen, const void *dout,
> > u8 *p_outbuf = (u8 *)dout;
> > u8 *p_inbuf = (u8 *)din;
> >
> > - if (!slave)
> > - return -1;
> > + if (!mxcs)
> > + return -EINVAL;
> >
> > if (flags & SPI_XFER_BEGIN)
> > - spi_cs_activate(slave);
> > + mxc_spi_cs_activate(mxcs);
> >
> > while (n_bytes > 0) {
> > if (n_bytes < MAX_SPI_BYTES)
> > @@ -351,7 +356,7 @@ int spi_xfer(struct spi_slave *slave, unsigned int
> > bitlen, const void *dout,
> >
> > n_bits = blk_size * 8;
> >
> > - ret = spi_xchg_single(slave, n_bits, p_outbuf, p_inbuf, 0);
> > + ret = spi_xchg_single(mxcs, n_bits, p_outbuf, p_inbuf, 0);
> >
> > if (ret)
> > return ret;
> > @@ -363,12 +368,39 @@ int spi_xfer(struct spi_slave *slave, unsigned int
> bitlen, const void *dout,
> > }
> >
> > if (flags & SPI_XFER_END) {
> > - spi_cs_deactivate(slave);
> > + mxc_spi_cs_deactivate(mxcs);
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +static int mxc_spi_claim_bus_internal(struct mxc_spi_slave *mxcs, int
> > +cs) {
> > + struct cspi_regs *regs = (struct cspi_regs *)mxcs->base;
> > + int ret;
> > +
> > + reg_write(®s->rxdata, 1);
> > + udelay(1);
> > + ret = spi_cfg_mxc(mxcs, cs);
> > + if (ret) {
> > + printf("mxc_spi: cannot setup SPI controller\n");
> > + return ret;
> > }
> > + reg_write(®s->period, MXC_CSPIPERIOD_32KHZ);
> > + reg_write(®s->intr, 0);
> >
> > return 0;
> > }
> >
> > +#ifndef CONFIG_DM_SPI
> > +int spi_xfer(struct spi_slave *slave, unsigned int bitlen, const void *dout,
> > + void *din, unsigned long flags)
> > +{
> > + struct mxc_spi_slave *mxcs = to_mxc_spi_slave(slave);
> > +
> > + return mxc_spi_xfer_internal(mxcs, bitlen, dout, din, flags); }
> > +
> > void spi_init(void)
> > {
> > }
> > @@ -390,6 +422,7 @@ static int setup_cs_gpio(struct mxc_spi_slave *mxcs,
> > if (mxcs->gpio == -1)
> > return 0;
> >
> > + gpio_request(mxcs->gpio, "spi-cs");
> > ret = gpio_direction_output(mxcs->gpio, !(mxcs->ss_pol));
> > if (ret) {
> > printf("mxc_spi: cannot setup gpio %d\n", mxcs->gpio); @@ -
> 399,6
> > +432,10 @@ static int setup_cs_gpio(struct mxc_spi_slave *mxcs,
> > return 0;
> > }
> >
> > +static unsigned long spi_bases[] = {
> > + MXC_SPI_BASE_ADDRESSES
> > +};
> > +
> > struct spi_slave *spi_setup_slave(unsigned int bus, unsigned int cs,
> > unsigned int max_hz, unsigned int mode) { @@ -443,24
> +480,104 @@
> > void spi_free_slave(struct spi_slave *slave)
> >
> > int spi_claim_bus(struct spi_slave *slave) {
> > - int ret;
> > struct mxc_spi_slave *mxcs = to_mxc_spi_slave(slave);
> > - struct cspi_regs *regs = (struct cspi_regs *)mxcs->base;
> >
> > - reg_write(®s->rxdata, 1);
> > - udelay(1);
> > - ret = spi_cfg_mxc(mxcs, slave->cs);
> > + return mxc_spi_claim_bus_internal(mxcs, slave->cs); }
> > +
> > +void spi_release_bus(struct spi_slave *slave) {
> > + /* TODO: Shut the controller down */ } #else
> > +
> > +static int mxc_spi_probe(struct udevice *bus) {
> > + struct mxc_spi_slave *plat = bus->platdata;
> > + struct mxc_spi_slave *mxcs = dev_get_platdata(bus);
> > + int node = dev_of_offset(bus);
> > + const void *blob = gd->fdt_blob;
> > + int ret;
> > +
> > + if (gpio_request_by_name(bus, "cs-gpios", 0, &plat->ss,
> > + GPIOD_IS_OUT)) {
> > + dev_err(bus, "No cs-gpios property\n");
> > + return -EINVAL;
> > + }
> > +
> > + plat->base = dev_get_addr(bus);
> > + if (plat->base == FDT_ADDR_T_NONE)
> > + return -EINVAL;
> >
> IMO -ENODEV is more appropriate here.
Fix in V2.
>
> > + ret = dm_gpio_set_value(&plat->ss, !(mxcs->ss_pol));
> >
> No conditional on CONFIG_IS_ENABLED() here?
No need here. If DM_GPIO is not enabled, it will trigger a build error, so
User could enable DM_GPIO. We are moving a direction to use DM driver,
So let's not use non-dm gpio api here.
>
> > if (ret) {
> > - printf("mxc_spi: cannot setup SPI controller\n");
> > - return ret;
> > + dev_err(bus, "Setting cs error\n");
> > + return -EBUSY;
> >
> dm_gpio_set_value() returned an appropriate error code which should be
> passed to the caller rather than inventing a new error code.
Fix in V2.
Thanks,
Peng.
>
>
>
> Lothar Waßmann
More information about the U-Boot
mailing list