| Message ID | 1413744219-6859-1-git-send-email-marex@denx.de |
|---|---|
| State | Superseded |
| Delegated to: | Jagannadha Sutradharudu Teki |
| Headers | show |
On Sun 2014-10-19 20:43:33, Marek Vasut wrote: > Zap the offset-based register access and use the struct-based one > as this is the preferred method. > > No functional change, but there are some line-over-80 problems in > the driver, which will be addressed later. For the whole series, Acked-by: Pavel Machek <pavel@denx.de>
On 20 October 2014 00:13, Marek Vasut <marex@denx.de> wrote: > Zap the offset-based register access and use the struct-based one > as this is the preferred method. > > No functional change, but there are some line-over-80 problems in > the driver, which will be addressed later. > > Signed-off-by: Marek Vasut <marex@denx.de> > Cc: Chin Liang See <clsee@altera.com> > Cc: Dinh Nguyen <dinguyen@altera.com> > Cc: Albert Aribaud <albert.u.boot@aribaud.net> > Cc: Tom Rini <trini@ti.com> > Cc: Wolfgang Denk <wd@denx.de> > Cc: Pavel Machek <pavel@denx.de> > Cc: Jagannadha Sutradharudu Teki <jagannadh.teki@gmail.com> > --- > drivers/spi/altera_spi.c | 49 ++++++++++++++++++++++++------------------------ > 1 file changed, 25 insertions(+), 24 deletions(-) > > diff --git a/drivers/spi/altera_spi.c b/drivers/spi/altera_spi.c > index 5accbb5..13191f3 100644 > --- a/drivers/spi/altera_spi.c > +++ b/drivers/spi/altera_spi.c > @@ -12,11 +12,14 @@ > #include <malloc.h> > #include <spi.h> > > -#define ALTERA_SPI_RXDATA 0 > -#define ALTERA_SPI_TXDATA 4 > -#define ALTERA_SPI_STATUS 8 > -#define ALTERA_SPI_CONTROL 12 > -#define ALTERA_SPI_SLAVE_SEL 20 > +struct altera_spi_regs { > + u32 rxdata; > + u32 txdata; > + u32 status; > + u32 control; > + u32 _reserved; > + u32 slave_sel; > +}; Can you place this structure definition below of all macro defines, i don't think the next level patches does that, does they? > > #define ALTERA_SPI_STATUS_ROE_MSK (0x8) > #define ALTERA_SPI_STATUS_TOE_MSK (0x10) > @@ -39,8 +42,8 @@ > static ulong altera_spi_base_list[] = CONFIG_SYS_ALTERA_SPI_LIST; > > struct altera_spi_slave { > - struct spi_slave slave; > - ulong base; > + struct spi_slave slave; > + struct altera_spi_regs *regs; > }; > #define to_altera_spi_slave(s) container_of(s, struct altera_spi_slave, slave) > > @@ -54,16 +57,16 @@ __attribute__((weak)) > void spi_cs_activate(struct spi_slave *slave) > { > struct altera_spi_slave *altspi = to_altera_spi_slave(slave); > - writel(1 << slave->cs, altspi->base + ALTERA_SPI_SLAVE_SEL); > - writel(ALTERA_SPI_CONTROL_SSO_MSK, altspi->base + ALTERA_SPI_CONTROL); > + writel(1 << slave->cs, &altspi->regs->slave_sel); > + writel(ALTERA_SPI_CONTROL_SSO_MSK, &altspi->regs->control); > } > > __attribute__((weak)) > void spi_cs_deactivate(struct spi_slave *slave) > { > struct altera_spi_slave *altspi = to_altera_spi_slave(slave); > - writel(0, altspi->base + ALTERA_SPI_CONTROL); > - writel(0, altspi->base + ALTERA_SPI_SLAVE_SEL); > + writel(0, &altspi->regs->control); > + writel(0, &altspi->regs->slave_sel); > } > > void spi_init(void) > @@ -87,9 +90,9 @@ struct spi_slave *spi_setup_slave(unsigned int bus, unsigned int cs, > if (!altspi) > return NULL; > > - altspi->base = altera_spi_base_list[bus]; > - debug("%s: bus:%i cs:%i base:%lx\n", __func__, > - bus, cs, altspi->base); > + altspi->regs = (struct altera_spi_regs *)altera_spi_base_list[bus]; > + debug("%s: bus:%i cs:%i base:%p\n", __func__, > + bus, cs, altspi->regs); > > return &altspi->slave; > } > @@ -105,8 +108,8 @@ int spi_claim_bus(struct spi_slave *slave) > struct altera_spi_slave *altspi = to_altera_spi_slave(slave); > > debug("%s: bus:%i cs:%i\n", __func__, slave->bus, slave->cs); > - writel(0, altspi->base + ALTERA_SPI_CONTROL); > - writel(0, altspi->base + ALTERA_SPI_SLAVE_SEL); > + writel(0, &altspi->regs->control); > + writel(0, &altspi->regs->slave_sel); > return 0; > } > > @@ -115,7 +118,7 @@ void spi_release_bus(struct spi_slave *slave) > struct altera_spi_slave *altspi = to_altera_spi_slave(slave); > > debug("%s: bus:%i cs:%i\n", __func__, slave->bus, slave->cs); > - writel(0, altspi->base + ALTERA_SPI_SLAVE_SEL); > + writel(0, &altspi->regs->slave_sel); > } > > #ifndef CONFIG_ALTERA_SPI_IDLE_VAL > @@ -142,20 +145,18 @@ int spi_xfer(struct spi_slave *slave, unsigned int bitlen, const void *dout, > } > > /* empty read buffer */ > - if (readl(altspi->base + ALTERA_SPI_STATUS) & > - ALTERA_SPI_STATUS_RRDY_MSK) > - readl(altspi->base + ALTERA_SPI_RXDATA); > + if (readl(&altspi->regs->status) & ALTERA_SPI_STATUS_RRDY_MSK) > + readl(&altspi->regs->rxdata); > if (flags & SPI_XFER_BEGIN) > spi_cs_activate(slave); > > while (bytes--) { > uchar d = txp ? *txp++ : CONFIG_ALTERA_SPI_IDLE_VAL; > debug("%s: tx:%x ", __func__, d); > - writel(d, altspi->base + ALTERA_SPI_TXDATA); > - while (!(readl(altspi->base + ALTERA_SPI_STATUS) & > - ALTERA_SPI_STATUS_RRDY_MSK)) > + writel(d, &altspi->regs->txdata); > + while (!(readl(&altspi->regs->status) & ALTERA_SPI_STATUS_RRDY_MSK)) > ; > - d = readl(altspi->base + ALTERA_SPI_RXDATA); > + d = readl(&altspi->regs->rxdata); > if (rxp) > *rxp++ = d; > debug("rx:%x\n", d); > -- > 2.1.1 > thanks!
On Monday, October 20, 2014 at 04:53:15 PM, Jagan Teki wrote: [...] > > -#define ALTERA_SPI_RXDATA 0 > > -#define ALTERA_SPI_TXDATA 4 > > -#define ALTERA_SPI_STATUS 8 > > -#define ALTERA_SPI_CONTROL 12 > > -#define ALTERA_SPI_SLAVE_SEL 20 > > +struct altera_spi_regs { > > + u32 rxdata; > > + u32 txdata; > > + u32 status; > > + u32 control; > > + u32 _reserved; > > + u32 slave_sel; > > +}; > > Can you place this structure definition below of all macro defines, i > don't think the > next level patches does that, does they? Does it make sense to you, to first define the bits in registers and then the register layout ? It does not make sense to me, so I would prefer to keep it like it is. [...] Best regards, Marek Vasut
On 20 October 2014 20:40, Marek Vasut <marex@denx.de> wrote: > On Monday, October 20, 2014 at 04:53:15 PM, Jagan Teki wrote: > > [...] > >> > -#define ALTERA_SPI_RXDATA 0 >> > -#define ALTERA_SPI_TXDATA 4 >> > -#define ALTERA_SPI_STATUS 8 >> > -#define ALTERA_SPI_CONTROL 12 >> > -#define ALTERA_SPI_SLAVE_SEL 20 >> > +struct altera_spi_regs { >> > + u32 rxdata; >> > + u32 txdata; >> > + u32 status; >> > + u32 control; >> > + u32 _reserved; >> > + u32 slave_sel; >> > +}; >> >> Can you place this structure definition below of all macro defines, i >> don't think the >> next level patches does that, does they? > > Does it make sense to you, to first define the bits in registers and then > the register layout ? It does not make sense to me, so I would prefer to > keep it like it is. You're correct the way you replaced, usually the driver code will go like this -- >includes --> macros definitions --> global or structure definitions --> driver function calls. We follow this [1] to make the driver more readable. [1] http://patchwork.ozlabs.org/patch/265683/ thanks!
On Monday, October 20, 2014 at 07:19:33 PM, Jagan Teki wrote: > On 20 October 2014 20:40, Marek Vasut <marex@denx.de> wrote: > > On Monday, October 20, 2014 at 04:53:15 PM, Jagan Teki wrote: > > > > [...] > > > >> > -#define ALTERA_SPI_RXDATA 0 > >> > -#define ALTERA_SPI_TXDATA 4 > >> > -#define ALTERA_SPI_STATUS 8 > >> > -#define ALTERA_SPI_CONTROL 12 > >> > -#define ALTERA_SPI_SLAVE_SEL 20 > >> > +struct altera_spi_regs { > >> > + u32 rxdata; > >> > + u32 txdata; > >> > + u32 status; > >> > + u32 control; > >> > + u32 _reserved; > >> > + u32 slave_sel; > >> > +}; > >> > >> Can you place this structure definition below of all macro defines, i > >> don't think the > >> next level patches does that, does they? > > > > Does it make sense to you, to first define the bits in registers and then > > the register layout ? It does not make sense to me, so I would prefer to > > keep it like it is. > > You're correct the way you replaced, usually the driver code will go like > this > > -- >includes > > --> macros definitions > > --> global or structure definitions > > --> driver function calls. > > We follow this [1] to make the driver more readable. > > [1] http://patchwork.ozlabs.org/patch/265683/ I'm not sure what I am supposed to follow in the link above. Is it fine to assume that this patch does not need any change then ? Best regards, Marek Vasut
diff --git a/drivers/spi/altera_spi.c b/drivers/spi/altera_spi.c index 5accbb5..13191f3 100644 --- a/drivers/spi/altera_spi.c +++ b/drivers/spi/altera_spi.c @@ -12,11 +12,14 @@ #include <malloc.h> #include <spi.h> -#define ALTERA_SPI_RXDATA 0 -#define ALTERA_SPI_TXDATA 4 -#define ALTERA_SPI_STATUS 8 -#define ALTERA_SPI_CONTROL 12 -#define ALTERA_SPI_SLAVE_SEL 20 +struct altera_spi_regs { + u32 rxdata; + u32 txdata; + u32 status; + u32 control; + u32 _reserved; + u32 slave_sel; +}; #define ALTERA_SPI_STATUS_ROE_MSK (0x8) #define ALTERA_SPI_STATUS_TOE_MSK (0x10) @@ -39,8 +42,8 @@ static ulong altera_spi_base_list[] = CONFIG_SYS_ALTERA_SPI_LIST; struct altera_spi_slave { - struct spi_slave slave; - ulong base; + struct spi_slave slave; + struct altera_spi_regs *regs; }; #define to_altera_spi_slave(s) container_of(s, struct altera_spi_slave, slave) @@ -54,16 +57,16 @@ __attribute__((weak)) void spi_cs_activate(struct spi_slave *slave) { struct altera_spi_slave *altspi = to_altera_spi_slave(slave); - writel(1 << slave->cs, altspi->base + ALTERA_SPI_SLAVE_SEL); - writel(ALTERA_SPI_CONTROL_SSO_MSK, altspi->base + ALTERA_SPI_CONTROL); + writel(1 << slave->cs, &altspi->regs->slave_sel); + writel(ALTERA_SPI_CONTROL_SSO_MSK, &altspi->regs->control); } __attribute__((weak)) void spi_cs_deactivate(struct spi_slave *slave) { struct altera_spi_slave *altspi = to_altera_spi_slave(slave); - writel(0, altspi->base + ALTERA_SPI_CONTROL); - writel(0, altspi->base + ALTERA_SPI_SLAVE_SEL); + writel(0, &altspi->regs->control); + writel(0, &altspi->regs->slave_sel); } void spi_init(void) @@ -87,9 +90,9 @@ struct spi_slave *spi_setup_slave(unsigned int bus, unsigned int cs, if (!altspi) return NULL; - altspi->base = altera_spi_base_list[bus]; - debug("%s: bus:%i cs:%i base:%lx\n", __func__, - bus, cs, altspi->base); + altspi->regs = (struct altera_spi_regs *)altera_spi_base_list[bus]; + debug("%s: bus:%i cs:%i base:%p\n", __func__, + bus, cs, altspi->regs); return &altspi->slave; } @@ -105,8 +108,8 @@ int spi_claim_bus(struct spi_slave *slave) struct altera_spi_slave *altspi = to_altera_spi_slave(slave); debug("%s: bus:%i cs:%i\n", __func__, slave->bus, slave->cs); - writel(0, altspi->base + ALTERA_SPI_CONTROL); - writel(0, altspi->base + ALTERA_SPI_SLAVE_SEL); + writel(0, &altspi->regs->control); + writel(0, &altspi->regs->slave_sel); return 0; } @@ -115,7 +118,7 @@ void spi_release_bus(struct spi_slave *slave) struct altera_spi_slave *altspi = to_altera_spi_slave(slave); debug("%s: bus:%i cs:%i\n", __func__, slave->bus, slave->cs); - writel(0, altspi->base + ALTERA_SPI_SLAVE_SEL); + writel(0, &altspi->regs->slave_sel); } #ifndef CONFIG_ALTERA_SPI_IDLE_VAL @@ -142,20 +145,18 @@ int spi_xfer(struct spi_slave *slave, unsigned int bitlen, const void *dout, } /* empty read buffer */ - if (readl(altspi->base + ALTERA_SPI_STATUS) & - ALTERA_SPI_STATUS_RRDY_MSK) - readl(altspi->base + ALTERA_SPI_RXDATA); + if (readl(&altspi->regs->status) & ALTERA_SPI_STATUS_RRDY_MSK) + readl(&altspi->regs->rxdata); if (flags & SPI_XFER_BEGIN) spi_cs_activate(slave); while (bytes--) { uchar d = txp ? *txp++ : CONFIG_ALTERA_SPI_IDLE_VAL; debug("%s: tx:%x ", __func__, d); - writel(d, altspi->base + ALTERA_SPI_TXDATA); - while (!(readl(altspi->base + ALTERA_SPI_STATUS) & - ALTERA_SPI_STATUS_RRDY_MSK)) + writel(d, &altspi->regs->txdata); + while (!(readl(&altspi->regs->status) & ALTERA_SPI_STATUS_RRDY_MSK)) ; - d = readl(altspi->base + ALTERA_SPI_RXDATA); + d = readl(&altspi->regs->rxdata); if (rxp) *rxp++ = d; debug("rx:%x\n", d);
Zap the offset-based register access and use the struct-based one as this is the preferred method. No functional change, but there are some line-over-80 problems in the driver, which will be addressed later. Signed-off-by: Marek Vasut <marex@denx.de> Cc: Chin Liang See <clsee@altera.com> Cc: Dinh Nguyen <dinguyen@altera.com> Cc: Albert Aribaud <albert.u.boot@aribaud.net> Cc: Tom Rini <trini@ti.com> Cc: Wolfgang Denk <wd@denx.de> Cc: Pavel Machek <pavel@denx.de> Cc: Jagannadha Sutradharudu Teki <jagannadh.teki@gmail.com> --- drivers/spi/altera_spi.c | 49 ++++++++++++++++++++++++------------------------ 1 file changed, 25 insertions(+), 24 deletions(-)