Hi Jagan, On 25 November 2016 15:29 Jagan Teki wrote: > On Fri, Nov 25, 2016 at 8:08 PM, Phil Edworthy > <phil.edwor...@renesas.com> wrote: > > Or'ing together bit positions is clearly wrong. > > > > Signed-off-by: Phil Edworthy <phil.edwor...@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 _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
