[U-Boot] [PATCH v2 1/8] spi: cadence_qspi: Fix clearing of pol/pha bits

[Phil Edworthy]

Hi Jagan,

On 25 November 2016 15:29 Jagan Teki wrote:
> On Fri, Nov 25, 2016 at 8:08 PM, Phil Edworthy
> <phil.edworthy at renesas.com> wrote:
> > Or'ing together bit positions is clearly wrong.
> >
> > Signed-off-by: Phil Edworthy <phil.edworthy at 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