| Message ID | 1480084688-24677-2-git-send-email-phil.edworthy@renesas.com |
|---|---|
| State | Superseded |
| Delegated to: | Jagannadha Sutradharudu Teki |
| Headers | show |
On 11/25/2016 03:38 PM, Phil Edworthy wrote: > Or'ing together bit positions is clearly wrong. > > Signed-off-by: Phil Edworthy <phil.edworthy@renesas.com> > --- > drivers/spi/cadence_qspi_apb.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/drivers/spi/cadence_qspi_apb.c b/drivers/spi/cadence_qspi_apb.c > index e285d3c..2403e71 100644 > --- a/drivers/spi/cadence_qspi_apb.c > +++ b/drivers/spi/cadence_qspi_apb.c > @@ -311,8 +311,8 @@ void cadence_qspi_apb_set_clk_mode(void *reg_base, > > cadence_qspi_apb_controller_disable(reg_base); > reg = readl(reg_base + CQSPI_REG_CONFIG); > - reg &= ~(1 << > - (CQSPI_REG_CONFIG_CLK_POL_LSB | CQSPI_REG_CONFIG_CLK_PHA_LSB)); > + reg &= ~(1 << CQSPI_REG_CONFIG_CLK_POL_LSB); > + reg &= ~(1 << CQSPI_REG_CONFIG_CLK_PHA_LSB); Oh, nice. Acked-by: Marek Vasut <marek.vasut@gmail.com> > reg |= ((clk_pol & 0x1) << CQSPI_REG_CONFIG_CLK_POL_LSB); > reg |= ((clk_pha & 0x1) << CQSPI_REG_CONFIG_CLK_PHA_LSB); >
On Fri, Nov 25, 2016 at 8:08 PM, Phil Edworthy <phil.edworthy@renesas.com> wrote: > Or'ing together bit positions is clearly wrong. > > Signed-off-by: Phil Edworthy <phil.edworthy@renesas.com> > --- > drivers/spi/cadence_qspi_apb.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/drivers/spi/cadence_qspi_apb.c b/drivers/spi/cadence_qspi_apb.c > index e285d3c..2403e71 100644 > --- a/drivers/spi/cadence_qspi_apb.c > +++ b/drivers/spi/cadence_qspi_apb.c > @@ -311,8 +311,8 @@ void cadence_qspi_apb_set_clk_mode(void *reg_base, > > cadence_qspi_apb_controller_disable(reg_base); > reg = readl(reg_base + CQSPI_REG_CONFIG); > - reg &= ~(1 << > - (CQSPI_REG_CONFIG_CLK_POL_LSB | CQSPI_REG_CONFIG_CLK_PHA_LSB)); > + reg &= ~(1 << CQSPI_REG_CONFIG_CLK_POL_LSB); > + reg &= ~(1 << CQSPI_REG_CONFIG_CLK_PHA_LSB); OK, but see below > > reg |= ((clk_pol & 0x1) << CQSPI_REG_CONFIG_CLK_POL_LSB); > reg |= ((clk_pha & 0x1) << CQSPI_REG_CONFIG_CLK_PHA_LSB); This existing code look not easy to understand for me with doing & and << operations with numeric 0x1. what about this Say for example POL and PHA on cadence has with bit 1 and 2 CQSPI_REG_CONFIG_CLK_POL_LSB BIT(1) CQSPI_REG_CONFIG_CLK_PHA_LSB BIT(2) reg &= ~(CQSPI_REG_CONFIG_CLK_PHA_LSB | CQSPI_REG_CONFIG_CLK_POL_LSB); if (mode & SPI_CPHA) reg |= CQSPI_REG_CONFIG_CLK_PHA_LSB; if (mode & SPI_CPOL) reg |= CQSPI_REG_CONFIG_CLK_POL_LSB; thanks!
Hi Jagan, On 25 November 2016 15:29 Jagan Teki wrote: > On Fri, Nov 25, 2016 at 8:08 PM, Phil Edworthy > <phil.edworthy@renesas.com> wrote: > > Or'ing together bit positions is clearly wrong. > > > > Signed-off-by: Phil Edworthy <phil.edworthy@renesas.com> > > --- > > drivers/spi/cadence_qspi_apb.c | 4 ++-- > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/spi/cadence_qspi_apb.c b/drivers/spi/cadence_qspi_apb.c > > index e285d3c..2403e71 100644 > > --- a/drivers/spi/cadence_qspi_apb.c > > +++ b/drivers/spi/cadence_qspi_apb.c > > @@ -311,8 +311,8 @@ void cadence_qspi_apb_set_clk_mode(void *reg_base, > > > > cadence_qspi_apb_controller_disable(reg_base); > > reg = readl(reg_base + CQSPI_REG_CONFIG); > > - reg &= ~(1 << > > - (CQSPI_REG_CONFIG_CLK_POL_LSB | > CQSPI_REG_CONFIG_CLK_PHA_LSB)); > > + reg &= ~(1 << CQSPI_REG_CONFIG_CLK_POL_LSB); > > + reg &= ~(1 << CQSPI_REG_CONFIG_CLK_PHA_LSB); > > OK, but see below > > > > > reg |= ((clk_pol & 0x1) << CQSPI_REG_CONFIG_CLK_POL_LSB); > > reg |= ((clk_pha & 0x1) << CQSPI_REG_CONFIG_CLK_PHA_LSB); > > This existing code look not easy to understand for me with doing & and > << operations with numeric 0x1. what about this > > Say for example POL and PHA on cadence has with bit 1 and 2 > CQSPI_REG_CONFIG_CLK_POL_LSB BIT(1) > CQSPI_REG_CONFIG_CLK_PHA_LSB BIT(2) > > reg &= ~(CQSPI_REG_CONFIG_CLK_PHA_LSB | > CQSPI_REG_CONFIG_CLK_POL_LSB); > > if (mode & SPI_CPHA) > reg |= CQSPI_REG_CONFIG_CLK_PHA_LSB; > if (mode & SPI_CPOL) > reg |= CQSPI_REG_CONFIG_CLK_POL_LSB; I completely agree. This is addressed in patch 4 along with the other code that defines shifts. > thanks! > -- > Jagan Teki > Free Software Engineer | www.openedev.com > U-Boot, Linux | Upstream Maintainer > Hyderabad, India. Thanks Phil
diff --git a/drivers/spi/cadence_qspi_apb.c b/drivers/spi/cadence_qspi_apb.c index e285d3c..2403e71 100644 --- a/drivers/spi/cadence_qspi_apb.c +++ b/drivers/spi/cadence_qspi_apb.c @@ -311,8 +311,8 @@ void cadence_qspi_apb_set_clk_mode(void *reg_base, cadence_qspi_apb_controller_disable(reg_base); reg = readl(reg_base + CQSPI_REG_CONFIG); - reg &= ~(1 << - (CQSPI_REG_CONFIG_CLK_POL_LSB | CQSPI_REG_CONFIG_CLK_PHA_LSB)); + reg &= ~(1 << CQSPI_REG_CONFIG_CLK_POL_LSB); + reg &= ~(1 << CQSPI_REG_CONFIG_CLK_PHA_LSB); reg |= ((clk_pol & 0x1) << CQSPI_REG_CONFIG_CLK_POL_LSB); reg |= ((clk_pha & 0x1) << CQSPI_REG_CONFIG_CLK_PHA_LSB);
Or'ing together bit positions is clearly wrong. Signed-off-by: Phil Edworthy <phil.edworthy@renesas.com> --- drivers/spi/cadence_qspi_apb.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)