From mboxrd@z Thu Jan  1 00:00:00 1970
From: AKASHI Takahiro <takahiro.akashi@linaro.org>
Date: Mon, 22 Jun 2020 09:58:20 +0900
Subject: [PATCH v2 03/17] dfu: rename dfu_tftp_write() to
 dfu_write_by_name()
In-Reply-To: <CADg8p96fwH3FSzOkQCa71V6y8Dc84r+XKMOMdQ2V4B+=aSu85Q@mail.gmail.com>
References: <20200617025515.23585-1-takahiro.akashi@linaro.org>
 <20200617025515.23585-4-takahiro.akashi@linaro.org>
 <CADg8p96fwH3FSzOkQCa71V6y8Dc84r+XKMOMdQ2V4B+=aSu85Q@mail.gmail.com>
Message-ID: <20200622005820.GA7316@laputa>
List-Id: <u-boot.lists.denx.de>
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
To: u-boot@lists.denx.de

On Sun, Jun 21, 2020 at 12:05:29AM +0530, Sughosh Ganu wrote:
> On Wed, 17 Jun 2020 at 08:25, AKASHI Takahiro <takahiro.akashi@linaro.org>
> wrote:
> 
> > This function is essentially independent from tffp, and will also be
> > utilised in implementing UEFI capsule update in a later commit.
> > So just give it a more generic name.
> > In addition, a new configuration option, CONFIG_DFU_ALT, was introduced
> > so that the file will be compiled with different options, particularly
> > one added in a later commit.
> >
> > Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
> > ---
> >  drivers/dfu/Kconfig                   |  5 +++++
> >  drivers/dfu/Makefile                  |  2 +-
> >  drivers/dfu/{dfu_tftp.c => dfu_alt.c} | 17 ++++++++++++--
> >  include/dfu.h                         | 32 +++++++++++++--------------
> >  4 files changed, 37 insertions(+), 19 deletions(-)
> >  rename drivers/dfu/{dfu_tftp.c => dfu_alt.c} (67%)
> >
> 
> You are renaming dfu_tftp.c to dfu_alt.c. Can the two api's not be added to
> dfu.c directly. Is there a need for a separate dfu_alt.c. It finally
> depends on what Lukasz thinks.
> 
> <snip>
> 
> 
> > index ffae4bb54f80..5b1b13d7170d 100644
> > --- a/drivers/dfu/dfu_tftp.c
> > +++ b/drivers/dfu/dfu_alt.c
> > @@ -10,8 +10,21 @@
> >  #include <errno.h>
> >  #include <dfu.h>
> >
> > -int dfu_tftp_write(char *dfu_entity_name, unsigned int addr, unsigned int
> > len,
> > -                  char *interface, char *devstring)
> > +/**
> > + * dfu_write_by_name() - write data to DFU medium
> > + * @dfu_entity_name:    Name of DFU entity to write
> > + * @addr:               Address of data buffer to write
> > + * @len:                Number of bytes
> > + * @interface:          Destination DFU medium (e.g. "mmc")
> > + * @devstring:          Instance number of destination DFU medium (e.g.
> > "1")
> > + *
> > + * This function is storing data received on DFU supported medium which
> > + * is specified by @dfu_entity_name.
> > + *
> > + * Return:              0 - on success, error code - otherwise
> > + */
> > +int dfu_write_by_name(char *dfu_entity_name, unsigned int addr,
> > +                     unsigned int len, char *interface, char *devstring)
> >  {
> >
> 
> I know that this has been taken from the original dfu_tftp.c, but the addr
> parameter needs to be a void pointer. The unsigned int does not work when
> addr is greater than 4GB. This needs to be changed for both the api's that
> are being added.

I can agree, but wonder why this has not caused any issue so far.

I know that the caller, update_tftp(), defines and uses a "ulong"
variable for this parameter. A casting like "(void *)(uintptr_t)addr"
can be misleading.

-Takahiro Akashi


> -sughosh