| Message ID | 1314701731-19099-1-git-send-email-chander.kashyap@linaro.org |
|---|---|
| State | Changes Requested |
| Delegated to: | Andy Fleming |
| Headers | show |
Hi Chander. this patch is correct. Acked-by: Jaehoon Chung <jh80.chung@samsung.com> Chander Kashyap wrote: > mmc data transfer width is set as following: > WIDE8[5]: > 0 = Depend on WIDE4 > 1 = 8-bit mode > WIDE4[1]: > 1 = 4-bit mode > 0 = 1-bit mode > > In case of 4-bit mode reset 8-bit mode and > in case of 1-bit mode reset 8-bit mode and 4-bit mode > > Signed-off-by: Chander Kashyap <chander.kashyap@linaro.org> > --- > drivers/mmc/s5p_mmc.c | 10 +++++++--- > 1 files changed, 7 insertions(+), 3 deletions(-) > > diff --git a/drivers/mmc/s5p_mmc.c b/drivers/mmc/s5p_mmc.c > index 7786ecf..6e6ad08 100644 > --- a/drivers/mmc/s5p_mmc.c > +++ b/drivers/mmc/s5p_mmc.c > @@ -368,12 +368,16 @@ static void mmc_set_ios(struct mmc *mmc) > * 1 = 4-bit mode > * 0 = 1-bit mode > */ > - if (mmc->bus_width == 8) > + if (mmc->bus_width == 8) { > ctrl |= (1 << 5); > - else if (mmc->bus_width == 4) > + ctrl &= ~(1 << 1); > + } else if (mmc->bus_width == 4) { > ctrl |= (1 << 1); > - else > + ctrl &= ~(1 << 5); > + } else { > ctrl &= ~(1 << 1); > + ctrl &= ~(1 << 5); > + } > > /* > * OUTEDGEINV[2]
On Tue, Aug 30, 2011 at 5:55 AM, Chander Kashyap <chander.kashyap@linaro.org> wrote: > mmc data transfer width is set as following: > WIDE8[5]: > 0 = Depend on WIDE4 > 1 = 8-bit mode > WIDE4[1]: > 1 = 4-bit mode > 0 = 1-bit mode > > In case of 4-bit mode reset 8-bit mode and > in case of 1-bit mode reset 8-bit mode and 4-bit mode > > Signed-off-by: Chander Kashyap <chander.kashyap@linaro.org> > --- > drivers/mmc/s5p_mmc.c | 10 +++++++--- > 1 files changed, 7 insertions(+), 3 deletions(-) > > diff --git a/drivers/mmc/s5p_mmc.c b/drivers/mmc/s5p_mmc.c > index 7786ecf..6e6ad08 100644 > --- a/drivers/mmc/s5p_mmc.c > +++ b/drivers/mmc/s5p_mmc.c > @@ -368,12 +368,16 @@ static void mmc_set_ios(struct mmc *mmc) > * 1 = 4-bit mode > * 0 = 1-bit mode > */ > - if (mmc->bus_width == 8) > + if (mmc->bus_width == 8) { > ctrl |= (1 << 5); > - else if (mmc->bus_width == 4) > + ctrl &= ~(1 << 1); I know these were like this before, but those numbers are awfully magical. You should really define constants for them. Also, this seems like a very confusing way to do this? Why not clear both bits at the start, and then set the one that is needed? Andy
Dear Andy Fleming, On 1 September 2011 10:23, Andy Fleming <afleming@gmail.com> wrote: > On Tue, Aug 30, 2011 at 5:55 AM, Chander Kashyap > <chander.kashyap@linaro.org> wrote: >> mmc data transfer width is set as following: >> WIDE8[5]: >> 0 = Depend on WIDE4 >> 1 = 8-bit mode >> WIDE4[1]: >> 1 = 4-bit mode >> 0 = 1-bit mode >> >> In case of 4-bit mode reset 8-bit mode and >> in case of 1-bit mode reset 8-bit mode and 4-bit mode >> >> Signed-off-by: Chander Kashyap <chander.kashyap@linaro.org> >> --- >> drivers/mmc/s5p_mmc.c | 10 +++++++--- >> 1 files changed, 7 insertions(+), 3 deletions(-) >> >> diff --git a/drivers/mmc/s5p_mmc.c b/drivers/mmc/s5p_mmc.c >> index 7786ecf..6e6ad08 100644 >> --- a/drivers/mmc/s5p_mmc.c >> +++ b/drivers/mmc/s5p_mmc.c >> @@ -368,12 +368,16 @@ static void mmc_set_ios(struct mmc *mmc) >> * 1 = 4-bit mode >> * 0 = 1-bit mode >> */ >> - if (mmc->bus_width == 8) >> + if (mmc->bus_width == 8) { >> ctrl |= (1 << 5); >> - else if (mmc->bus_width == 4) >> + ctrl &= ~(1 << 1); > > > I know these were like this before, but those numbers are awfully > magical. You should really define constants for them. We decided to use comments instead of defines. > > Also, this seems like a very confusing way to do this? Why not clear > both bits at the start, and then set the one that is needed? Agreed. Thanks Minkyu Kang
Dear Minkyu Kang, In message <CALrBrZ3H+k8bu-1OwF5C5BNHuphj_J8jXXV2xgu6jPO5gmJELg@mail.gmail.com> you wrote: > > > I know these were like this before, but those numbers are awfully > > magical. You should really define constants for them. > > We decided to use comments instead of defines. And now we ask you to fix this, because we don;t want such magic constants. So please go on and change this. Thanks. Wolfgang Denk
Dear Wolfgang Denk, On 1 September 2011 14:51, Wolfgang Denk <wd@denx.de> wrote: > Dear Minkyu Kang, > > In message <CALrBrZ3H+k8bu-1OwF5C5BNHuphj_J8jXXV2xgu6jPO5gmJELg@mail.gmail.com> you wrote: >> >> > I know these were like this before, but those numbers are awfully >> > magical. You should really define constants for them. >> >> We decided to use comments instead of defines. > > And now we ask you to fix this, because we don;t want such magic > constants. So please go on and change this. > OK. Thanks. Minkyu Kang
diff --git a/drivers/mmc/s5p_mmc.c b/drivers/mmc/s5p_mmc.c index 7786ecf..6e6ad08 100644 --- a/drivers/mmc/s5p_mmc.c +++ b/drivers/mmc/s5p_mmc.c @@ -368,12 +368,16 @@ static void mmc_set_ios(struct mmc *mmc) * 1 = 4-bit mode * 0 = 1-bit mode */ - if (mmc->bus_width == 8) + if (mmc->bus_width == 8) { ctrl |= (1 << 5); - else if (mmc->bus_width == 4) + ctrl &= ~(1 << 1); + } else if (mmc->bus_width == 4) { ctrl |= (1 << 1); - else + ctrl &= ~(1 << 5); + } else { ctrl &= ~(1 << 1); + ctrl &= ~(1 << 5); + } /* * OUTEDGEINV[2]
mmc data transfer width is set as following: WIDE8[5]: 0 = Depend on WIDE4 1 = 8-bit mode WIDE4[1]: 1 = 4-bit mode 0 = 1-bit mode In case of 4-bit mode reset 8-bit mode and in case of 1-bit mode reset 8-bit mode and 4-bit mode Signed-off-by: Chander Kashyap <chander.kashyap@linaro.org> --- drivers/mmc/s5p_mmc.c | 10 +++++++--- 1 files changed, 7 insertions(+), 3 deletions(-)