| Message ID | 20190209131500.29954-4-jagan@amarulasolutions.com |
|---|---|
| State | Changes Requested |
| Delegated to: | Jagannadha Sutradharudu Teki |
| Headers | show |
| Series | spi: Add Allwinner A31 SPI driver | expand |
On 09/02/2019 13:14, Jagan Teki wrote: > Update the existing register writes using setbits_le32 and > clrbits_le32 in required places. > > Signed-off-by: Jagan Teki <jagan@amarulasolutions.com> > --- > drivers/spi/sun4i_spi.c | 21 ++++++++------------- > 1 file changed, 8 insertions(+), 13 deletions(-) > > diff --git a/drivers/spi/sun4i_spi.c b/drivers/spi/sun4i_spi.c > index 87b396a96e..5446cebe7c 100644 > --- a/drivers/spi/sun4i_spi.c > +++ b/drivers/spi/sun4i_spi.c > @@ -283,20 +283,18 @@ static int sun4i_spi_claim_bus(struct udevice *dev) > { > struct sun4i_spi_priv *priv = dev_get_priv(dev->parent); > > - writel(SUN4I_CTL_ENABLE | SUN4I_CTL_MASTER | SUN4I_CTL_TP | > - SUN4I_CTL_CS_MANUAL | SUN4I_CTL_CS_ACTIVE_LOW, > - &priv->regs->ctl); > + setbits_le32(&priv->regs->ctl, SUN4I_CTL_ENABLE | > + SUN4I_CTL_MASTER | SUN4I_CTL_TP | > + SUN4I_CTL_CS_MANUAL | SUN4I_CTL_CS_ACTIVE_LOW); > + Careful, this changes semantics. The original call explicitly cleared all the remaining bits, which is important for the setup (some bits are default 1). And besides I am not sure what this change would improve anyway, as it isn't shorter. If at all, I'd use clrsetbits_le32(), to mask off the reserved bits 31:21. Rest is fine. Cheers, Andre. > return 0; > } > > static int sun4i_spi_release_bus(struct udevice *dev) > { > struct sun4i_spi_priv *priv = dev_get_priv(dev->parent); > - u32 reg; > > - reg = readl(&priv->regs->ctl); > - reg &= ~SUN4I_CTL_ENABLE; > - writel(reg, &priv->regs->ctl); > + clrbits_le32(&priv->regs->ctl, SUN4I_CTL_ENABLE); > > return 0; > } > @@ -309,7 +307,7 @@ static int sun4i_spi_xfer(struct udevice *dev, unsigned int bitlen, > struct dm_spi_slave_platdata *slave_plat = dev_get_parent_platdata(dev); > > u32 len = bitlen / 8; > - u32 reg, rx_fifocnt; > + u32 rx_fifocnt; > u8 nbytes; > int ret; > > @@ -324,10 +322,8 @@ static int sun4i_spi_xfer(struct udevice *dev, unsigned int bitlen, > if (flags & SPI_XFER_BEGIN) > sun4i_spi_set_cs(bus, slave_plat->cs, true); > > - reg = readl(&priv->regs->ctl); > - > /* Reset FIFOs */ > - writel(reg | SUN4I_CTL_RF_RST | SUN4I_CTL_TF_RST, &priv->regs->ctl); > + setbits_le32(&priv->regs->ctl, SUN4I_CTL_RF_RST | SUN4I_CTL_TF_RST); > > while (len) { > /* Setup the transfer now... */ > @@ -341,8 +337,7 @@ static int sun4i_spi_xfer(struct udevice *dev, unsigned int bitlen, > sun4i_spi_fill_fifo(priv, nbytes); > > /* Start the transfer */ > - reg = readl(&priv->regs->ctl); > - writel(reg | SUN4I_CTL_XCH, &priv->regs->ctl); > + setbits_le32(&priv->regs->ctl, SUN4I_CTL_XCH); > > /* Wait till RX FIFO to be empty */ > ret = readl_poll_timeout(&priv->regs->ctl, rx_fifocnt, >
On Wed, Feb 13, 2019 at 6:46 AM André Przywara <andre.przywara@arm.com> wrote: > > On 09/02/2019 13:14, Jagan Teki wrote: > > Update the existing register writes using setbits_le32 and > > clrbits_le32 in required places. > > > > Signed-off-by: Jagan Teki <jagan@amarulasolutions.com> > > --- > > drivers/spi/sun4i_spi.c | 21 ++++++++------------- > > 1 file changed, 8 insertions(+), 13 deletions(-) > > > > diff --git a/drivers/spi/sun4i_spi.c b/drivers/spi/sun4i_spi.c > > index 87b396a96e..5446cebe7c 100644 > > --- a/drivers/spi/sun4i_spi.c > > +++ b/drivers/spi/sun4i_spi.c > > @@ -283,20 +283,18 @@ static int sun4i_spi_claim_bus(struct udevice *dev) > > { > > struct sun4i_spi_priv *priv = dev_get_priv(dev->parent); > > > > - writel(SUN4I_CTL_ENABLE | SUN4I_CTL_MASTER | SUN4I_CTL_TP | > > - SUN4I_CTL_CS_MANUAL | SUN4I_CTL_CS_ACTIVE_LOW, > > - &priv->regs->ctl); > > + setbits_le32(&priv->regs->ctl, SUN4I_CTL_ENABLE | > > + SUN4I_CTL_MASTER | SUN4I_CTL_TP | > > + SUN4I_CTL_CS_MANUAL | SUN4I_CTL_CS_ACTIVE_LOW); > > + > > Careful, this changes semantics. The original call explicitly cleared > all the remaining bits, which is important for the setup (some bits are > default 1). Usually claiming would need to setup require bits, on that case setbits is safer rather than complete writel > And besides I am not sure what this change would improve anyway, as it > isn't shorter. If at all, I'd use clrsetbits_le32(), to mask off the > reserved bits 31:21. It's not about the shorter and just enable require bits which is usually do any claim call in SPI.
diff --git a/drivers/spi/sun4i_spi.c b/drivers/spi/sun4i_spi.c index 87b396a96e..5446cebe7c 100644 --- a/drivers/spi/sun4i_spi.c +++ b/drivers/spi/sun4i_spi.c @@ -283,20 +283,18 @@ static int sun4i_spi_claim_bus(struct udevice *dev) { struct sun4i_spi_priv *priv = dev_get_priv(dev->parent); - writel(SUN4I_CTL_ENABLE | SUN4I_CTL_MASTER | SUN4I_CTL_TP | - SUN4I_CTL_CS_MANUAL | SUN4I_CTL_CS_ACTIVE_LOW, - &priv->regs->ctl); + setbits_le32(&priv->regs->ctl, SUN4I_CTL_ENABLE | + SUN4I_CTL_MASTER | SUN4I_CTL_TP | + SUN4I_CTL_CS_MANUAL | SUN4I_CTL_CS_ACTIVE_LOW); + return 0; } static int sun4i_spi_release_bus(struct udevice *dev) { struct sun4i_spi_priv *priv = dev_get_priv(dev->parent); - u32 reg; - reg = readl(&priv->regs->ctl); - reg &= ~SUN4I_CTL_ENABLE; - writel(reg, &priv->regs->ctl); + clrbits_le32(&priv->regs->ctl, SUN4I_CTL_ENABLE); return 0; } @@ -309,7 +307,7 @@ static int sun4i_spi_xfer(struct udevice *dev, unsigned int bitlen, struct dm_spi_slave_platdata *slave_plat = dev_get_parent_platdata(dev); u32 len = bitlen / 8; - u32 reg, rx_fifocnt; + u32 rx_fifocnt; u8 nbytes; int ret; @@ -324,10 +322,8 @@ static int sun4i_spi_xfer(struct udevice *dev, unsigned int bitlen, if (flags & SPI_XFER_BEGIN) sun4i_spi_set_cs(bus, slave_plat->cs, true); - reg = readl(&priv->regs->ctl); - /* Reset FIFOs */ - writel(reg | SUN4I_CTL_RF_RST | SUN4I_CTL_TF_RST, &priv->regs->ctl); + setbits_le32(&priv->regs->ctl, SUN4I_CTL_RF_RST | SUN4I_CTL_TF_RST); while (len) { /* Setup the transfer now... */ @@ -341,8 +337,7 @@ static int sun4i_spi_xfer(struct udevice *dev, unsigned int bitlen, sun4i_spi_fill_fifo(priv, nbytes); /* Start the transfer */ - reg = readl(&priv->regs->ctl); - writel(reg | SUN4I_CTL_XCH, &priv->regs->ctl); + setbits_le32(&priv->regs->ctl, SUN4I_CTL_XCH); /* Wait till RX FIFO to be empty */ ret = readl_poll_timeout(&priv->regs->ctl, rx_fifocnt,
Update the existing register writes using setbits_le32 and clrbits_le32 in required places. Signed-off-by: Jagan Teki <jagan@amarulasolutions.com> --- drivers/spi/sun4i_spi.c | 21 ++++++++------------- 1 file changed, 8 insertions(+), 13 deletions(-)