| Message ID | 20181009112351.17256-1-marek.vasut+renesas@gmail.com |
|---|---|
| State | Deferred |
| Delegated to: | Marek Vasut |
| Headers | show |
| Series | [U-Boot,1/2] mmc: tmio: Pass full address to tmio_sd_addr_is_dmaable() | expand |
Hi Marek, On Tue, Oct 9, 2018 at 8:26 PM Marek Vasut <marek.vasut@gmail.com> wrote: > > Pass the entire source data pointer to tmio_sd_addr_is_dmaable() This statement sounds like the current code is passing the pointer address only partially. Is it right? > so we don't have to apply casts throughout the code. I do not understand this either since I see a cast in your code too. In the previous code, the caller casts src->address when it passes it to tmio_sd_addr_is_dmaable(). In the new code, 'src' is casted in tmio_sd_addr_is_dmaable(). To me, you just moved the location of casting. What is the difference (i.e. benefit)? If you want to change this code, I am fine. But, I'd like to know the reason. At least, I am so confused with your commit description. > Signed-off-by: Marek Vasut <marek.vasut+renesas@gmail.com> > Cc: Masahiro Yamada <yamada.masahiro@socionext.com> > --- > drivers/mmc/tmio-common.c | 6 ++++-- > 1 file changed, 4 insertions(+), 2 deletions(-) > > diff --git a/drivers/mmc/tmio-common.c b/drivers/mmc/tmio-common.c > index b311b80be8..6b21941991 100644 > --- a/drivers/mmc/tmio-common.c > +++ b/drivers/mmc/tmio-common.c > @@ -372,8 +372,10 @@ static int tmio_sd_dma_xfer(struct udevice *dev, struct mmc_data *data) > } > > /* check if the address is DMA'able */ > -static bool tmio_sd_addr_is_dmaable(unsigned long addr) > +static bool tmio_sd_addr_is_dmaable(const char *src) > { > + uintptr_t addr = (uintptr_t)src; > + > if (!IS_ALIGNED(addr, TMIO_SD_DMA_MINALIGN)) > return false; > > @@ -486,7 +488,7 @@ int tmio_sd_send_cmd(struct udevice *dev, struct mmc_cmd *cmd, > if (data) { > /* use DMA if the HW supports it and the buffer is aligned */ > if (priv->caps & TMIO_SD_CAP_DMA_INTERNAL && > - tmio_sd_addr_is_dmaable((long)data->src)) > + tmio_sd_addr_is_dmaable(data->src)) > ret = tmio_sd_dma_xfer(dev, data); > else > ret = tmio_sd_pio_xfer(dev, data); > -- > 2.18.0 > > _______________________________________________ > U-Boot mailing list > U-Boot@lists.denx.de > https://lists.denx.de/listinfo/u-boot
On 10/09/2018 02:24 PM, Masahiro Yamada wrote: > Hi Marek, Hi, > On Tue, Oct 9, 2018 at 8:26 PM Marek Vasut <marek.vasut@gmail.com> wrote: >> >> Pass the entire source data pointer to tmio_sd_addr_is_dmaable() > > > This statement sounds like > the current code is passing the pointer address only partially. > Is it right? With this change it is. >> so we don't have to apply casts throughout the code. > > I do not understand this either > since I see a cast in your code too. There is a cast, but it's isolated to this function. > In the previous code, the caller casts src->address > when it passes it to tmio_sd_addr_is_dmaable(). > > In the new code, 'src' is casted > in tmio_sd_addr_is_dmaable(). > > To me, you just moved the location of casting. > What is the difference (i.e. benefit)? I moved the cast from the code into the function, which I think is cleaner. > If you want to change this code, I am fine. > But, I'd like to know the reason. > > At least, I am so confused with your commit description. > > > > > > >> Signed-off-by: Marek Vasut <marek.vasut+renesas@gmail.com> >> Cc: Masahiro Yamada <yamada.masahiro@socionext.com> >> --- >> drivers/mmc/tmio-common.c | 6 ++++-- >> 1 file changed, 4 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/mmc/tmio-common.c b/drivers/mmc/tmio-common.c >> index b311b80be8..6b21941991 100644 >> --- a/drivers/mmc/tmio-common.c >> +++ b/drivers/mmc/tmio-common.c >> @@ -372,8 +372,10 @@ static int tmio_sd_dma_xfer(struct udevice *dev, struct mmc_data *data) >> } >> >> /* check if the address is DMA'able */ >> -static bool tmio_sd_addr_is_dmaable(unsigned long addr) >> +static bool tmio_sd_addr_is_dmaable(const char *src) >> { >> + uintptr_t addr = (uintptr_t)src; >> + >> if (!IS_ALIGNED(addr, TMIO_SD_DMA_MINALIGN)) >> return false; >> >> @@ -486,7 +488,7 @@ int tmio_sd_send_cmd(struct udevice *dev, struct mmc_cmd *cmd, >> if (data) { >> /* use DMA if the HW supports it and the buffer is aligned */ >> if (priv->caps & TMIO_SD_CAP_DMA_INTERNAL && >> - tmio_sd_addr_is_dmaable((long)data->src)) >> + tmio_sd_addr_is_dmaable(data->src)) >> ret = tmio_sd_dma_xfer(dev, data); >> else >> ret = tmio_sd_pio_xfer(dev, data); >> -- >> 2.18.0 >> >> _______________________________________________ >> U-Boot mailing list >> U-Boot@lists.denx.de >> https://lists.denx.de/listinfo/u-boot > > >
On Tue, Oct 9, 2018 at 11:55 PM Marek Vasut <marek.vasut@gmail.com> wrote: > > On 10/09/2018 02:24 PM, Masahiro Yamada wrote: > > Hi Marek, > > Hi, > > > On Tue, Oct 9, 2018 at 8:26 PM Marek Vasut <marek.vasut@gmail.com> wrote: > >> > >> Pass the entire source data pointer to tmio_sd_addr_is_dmaable() > > > > > > This statement sounds like > > the current code is passing the pointer address only partially. > > Is it right? > > With this change it is. Is anything wrong with my code? How about your patch title "mmc: tmio: Pass full address to tmio_sd_addr_is_dmaable()" ? Does it mean my code is not passing full address? > >> so we don't have to apply casts throughout the code. > > > > I do not understand this either > > since I see a cast in your code too. > > There is a cast, but it's isolated to this function. > > > In the previous code, the caller casts src->address > > when it passes it to tmio_sd_addr_is_dmaable(). > > > > In the new code, 'src' is casted > > in tmio_sd_addr_is_dmaable(). > > > > To me, you just moved the location of casting. > > What is the difference (i.e. benefit)? > > I moved the cast from the code into the function, which I think is cleaner. I do not think so. If you like this patch, just go for it. But, I believe you need to update the patch title and description since this is just a matter of personal preference.
On 10/09/2018 05:35 PM, Masahiro Yamada wrote: > On Tue, Oct 9, 2018 at 11:55 PM Marek Vasut <marek.vasut@gmail.com> wrote: >> >> On 10/09/2018 02:24 PM, Masahiro Yamada wrote: >>> Hi Marek, >> >> Hi, >> >>> On Tue, Oct 9, 2018 at 8:26 PM Marek Vasut <marek.vasut@gmail.com> wrote: >>>> >>>> Pass the entire source data pointer to tmio_sd_addr_is_dmaable() >>> >>> >>> This statement sounds like >>> the current code is passing the pointer address only partially. >>> Is it right? >> >> With this change it is. > > > Is anything wrong with my code? Don't think so. > How about your patch title > "mmc: tmio: Pass full address to tmio_sd_addr_is_dmaable()" ? > > Does it mean my code is not passing full address? Could use a rephrasing, yeah >>>> so we don't have to apply casts throughout the code. >>> >>> I do not understand this either >>> since I see a cast in your code too. >> >> There is a cast, but it's isolated to this function. >> >>> In the previous code, the caller casts src->address >>> when it passes it to tmio_sd_addr_is_dmaable(). >>> >>> In the new code, 'src' is casted >>> in tmio_sd_addr_is_dmaable(). >>> >>> To me, you just moved the location of casting. >>> What is the difference (i.e. benefit)? >> >> I moved the cast from the code into the function, which I think is cleaner. > > I do not think so. So would you prefer to see stuff like function foo(long bar) {...} foo((cast)baz); ... foo((cast)quux); In the code :) > If you like this patch, just go for it. > > But, I believe you need to update the patch title and description > since this is just a matter of personal preference. > >
On Wed, Oct 10, 2018 at 1:17 AM Marek Vasut <marek.vasut@gmail.com> wrote: > > On 10/09/2018 05:35 PM, Masahiro Yamada wrote: > > On Tue, Oct 9, 2018 at 11:55 PM Marek Vasut <marek.vasut@gmail.com> wrote: > >> > >> On 10/09/2018 02:24 PM, Masahiro Yamada wrote: > >>> Hi Marek, > >> > >> Hi, > >> > >>> On Tue, Oct 9, 2018 at 8:26 PM Marek Vasut <marek.vasut@gmail.com> wrote: > >>>> > >>>> Pass the entire source data pointer to tmio_sd_addr_is_dmaable() > >>> > >>> > >>> This statement sounds like > >>> the current code is passing the pointer address only partially. > >>> Is it right? > >> > >> With this change it is. > > > > > > Is anything wrong with my code? > > Don't think so. > > > How about your patch title > > "mmc: tmio: Pass full address to tmio_sd_addr_is_dmaable()" ? > > > > Does it mean my code is not passing full address? > > Could use a rephrasing, yeah > > >>>> so we don't have to apply casts throughout the code. > >>> > >>> I do not understand this either > >>> since I see a cast in your code too. > >> > >> There is a cast, but it's isolated to this function. > >> > >>> In the previous code, the caller casts src->address > >>> when it passes it to tmio_sd_addr_is_dmaable(). > >>> > >>> In the new code, 'src' is casted > >>> in tmio_sd_addr_is_dmaable(). > >>> > >>> To me, you just moved the location of casting. > >>> What is the difference (i.e. benefit)? > >> > >> I moved the cast from the code into the function, which I think is cleaner. > > > > I do not think so. > > So would you prefer to see stuff like > > function foo(long bar) > {...} > > foo((cast)baz); > > ... > > foo((cast)quux); > > In the code :) It is a hypothetical situation. If there were multiple function calls, I would agree with you.
diff --git a/drivers/mmc/tmio-common.c b/drivers/mmc/tmio-common.c index b311b80be8..6b21941991 100644 --- a/drivers/mmc/tmio-common.c +++ b/drivers/mmc/tmio-common.c @@ -372,8 +372,10 @@ static int tmio_sd_dma_xfer(struct udevice *dev, struct mmc_data *data) } /* check if the address is DMA'able */ -static bool tmio_sd_addr_is_dmaable(unsigned long addr) +static bool tmio_sd_addr_is_dmaable(const char *src) { + uintptr_t addr = (uintptr_t)src; + if (!IS_ALIGNED(addr, TMIO_SD_DMA_MINALIGN)) return false; @@ -486,7 +488,7 @@ int tmio_sd_send_cmd(struct udevice *dev, struct mmc_cmd *cmd, if (data) { /* use DMA if the HW supports it and the buffer is aligned */ if (priv->caps & TMIO_SD_CAP_DMA_INTERNAL && - tmio_sd_addr_is_dmaable((long)data->src)) + tmio_sd_addr_is_dmaable(data->src)) ret = tmio_sd_dma_xfer(dev, data); else ret = tmio_sd_pio_xfer(dev, data);
Pass the entire source data pointer to tmio_sd_addr_is_dmaable() so we don't have to apply casts throughout the code. Signed-off-by: Marek Vasut <marek.vasut+renesas@gmail.com> Cc: Masahiro Yamada <yamada.masahiro@socionext.com> --- drivers/mmc/tmio-common.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-)