[U-Boot,v2,5/9] dfu: tftp: update: Add dfu_write_from_mem_addr() function

This function allows writing via DFU data stored from fixed buffer address
(like e.g. loadaddr env variable).

Such predefined buffers are used in the update_tftp() code. In fact this
function is a wrapper on the dfu_write() and dfu_flush().

Signed-off-by: Lukasz Majewski <l.majewski@majess.pl>
---
Changes for v2:
- Use min() macro instead of comparison
- Change definition of left variable to be unsigned long - this allowed
  safe usage of min() macro
---
 drivers/dfu/dfu.c | 48 ++++++++++++++++++++++++++++++++++++++++++++++++
 include/dfu.h     |  1 +
 2 files changed, 49 insertions(+)

Hi Lukasz,

On 25 July 2015 at 02:11, Lukasz Majewski <l.majewski@majess.pl> wrote:
> This function allows writing via DFU data stored from fixed buffer address
> (like e.g. loadaddr env variable).
>
> Such predefined buffers are used in the update_tftp() code. In fact this
> function is a wrapper on the dfu_write() and dfu_flush().
>
> Signed-off-by: Lukasz Majewski <l.majewski@majess.pl>
> ---
> Changes for v2:
> - Use min() macro instead of comparison
> - Change definition of left variable to be unsigned long - this allowed
>   safe usage of min() macro
> ---
>  drivers/dfu/dfu.c | 48 ++++++++++++++++++++++++++++++++++++++++++++++++
>  include/dfu.h     |  1 +
>  2 files changed, 49 insertions(+)

Reviewed-by: Simon Glass <sjg@chromium.org>

But see nits below.

>
> diff --git a/drivers/dfu/dfu.c b/drivers/dfu/dfu.c
> index 2267dbf..c83ee41 100644
> --- a/drivers/dfu/dfu.c
> +++ b/drivers/dfu/dfu.c
> @@ -568,3 +568,51 @@ int dfu_get_alt(char *name)
>
>         return -ENODEV;
>  }
> +
> +/**
> + * dfu_write_from_mem_addr - this function adds support for writing data
> + *                           starting from fixed memory address (like $loadaddr)
> + *                           to dfu managed medium (e.g. NAND, MMC)

I think it's better to start with a direct one-line description, like:

+ * dfu_write_from_mem_addr - wite data to dfu-managed medium
+ *
+ * More detail goes here ...

> + *
> + * @param dfu - dfu entity to which we want to store data
> + * @param buf - fixed memory addres from where data starts
> + * @param size - number of bytes to write
> + *
> + * @return - 0 on success, other value on failure
> + */
> +int dfu_write_from_mem_addr(struct dfu_entity *dfu, void *buf, int size)

const void *buf? My understanding is that this buffer is not changed.

> +{
> +       unsigned long dfu_buf_size, write, left = size;
> +       int i, ret = 0;
> +       void *dp = buf;
> +
> +       /*
> +        * Here we must call dfu_get_buf(dfu) first to be sure that dfu_buf_size
> +        * has been properly initialized - e.g. if "dfu_bufsiz" has been taken
> +        * into account.
> +        */
> +       dfu_get_buf(dfu);
> +       dfu_buf_size = dfu_get_buf_size();
> +       debug("%s: dfu buf size: %lu\n", __func__, dfu_buf_size);
> +
> +       for (i = 0; left > 0; i++) {
> +               write = min(dfu_buf_size, left);
> +
> +               debug("%s: dp: 0x%p left: %lu write: %lu\n", __func__,
> +                     dp, left, write);
> +               ret = dfu_write(dfu, dp, write, i);
> +               if (ret) {
> +                       error("DFU write failed\n");
> +                       return ret;
> +               }
> +
> +               dp += write;
> +               left -= write;
> +       }
> +
> +       ret = dfu_flush(dfu, NULL, 0, i);
> +       if (ret)
> +               error("DFU flush failed!");
> +
> +       return ret;
> +}
> diff --git a/include/dfu.h b/include/dfu.h
> index 7f78cc2..e624c41 100644
> --- a/include/dfu.h
> +++ b/include/dfu.h
> @@ -162,6 +162,7 @@ bool dfu_usb_get_reset(void);
>  int dfu_read(struct dfu_entity *de, void *buf, int size, int blk_seq_num);
>  int dfu_write(struct dfu_entity *de, void *buf, int size, int blk_seq_num);
>  int dfu_flush(struct dfu_entity *de, void *buf, int size, int blk_seq_num);

Please can you put your function comment here in the header?

> +int dfu_write_from_mem_addr(struct dfu_entity *dfu, void *buf, int size);
>  /* Device specific */
>  #ifdef CONFIG_DFU_MMC
>  extern int dfu_fill_entity_mmc(struct dfu_entity *dfu, char *devstr, char *s);
> --
> 2.1.4
>

Regards,
Simon

Hi Lukasz,

On Sat, Jul 25, 2015 at 3:11 AM, Lukasz Majewski <l.majewski@majess.pl> wrote:
> This function allows writing via DFU data stored from fixed buffer address
> (like e.g. loadaddr env variable).
>
> Such predefined buffers are used in the update_tftp() code. In fact this
> function is a wrapper on the dfu_write() and dfu_flush().
>
> Signed-off-by: Lukasz Majewski <l.majewski@majess.pl>
> ---
> Changes for v2:
> - Use min() macro instead of comparison
> - Change definition of left variable to be unsigned long - this allowed
>   safe usage of min() macro
> ---

Address Simon's nits and then,
Acked-by: Joe Hershberger <joe.hershberger@ni.com>

Hi Simon,

> Hi Lukasz,
> 
> On 25 July 2015 at 02:11, Lukasz Majewski <l.majewski@majess.pl>
> wrote:
> > This function allows writing via DFU data stored from fixed buffer
> > address (like e.g. loadaddr env variable).
> >
> > Such predefined buffers are used in the update_tftp() code. In fact
> > this function is a wrapper on the dfu_write() and dfu_flush().
> >
> > Signed-off-by: Lukasz Majewski <l.majewski@majess.pl>
> > ---
> > Changes for v2:
> > - Use min() macro instead of comparison
> > - Change definition of left variable to be unsigned long - this
> > allowed safe usage of min() macro
> > ---
> >  drivers/dfu/dfu.c | 48
> > ++++++++++++++++++++++++++++++++++++++++++++++++ include/dfu.h
> > |  1 + 2 files changed, 49 insertions(+)
> 
> Reviewed-by: Simon Glass <sjg@chromium.org>
> 
> But see nits below.
> 
> >
> > diff --git a/drivers/dfu/dfu.c b/drivers/dfu/dfu.c
> > index 2267dbf..c83ee41 100644
> > --- a/drivers/dfu/dfu.c
> > +++ b/drivers/dfu/dfu.c
> > @@ -568,3 +568,51 @@ int dfu_get_alt(char *name)
> >
> >         return -ENODEV;
> >  }
> > +
> > +/**
> > + * dfu_write_from_mem_addr - this function adds support for
> > writing data
> > + *                           starting from fixed memory address
> > (like $loadaddr)
> > + *                           to dfu managed medium (e.g. NAND, MMC)
> 
> I think it's better to start with a direct one-line description, like:
> 
> + * dfu_write_from_mem_addr - wite data to dfu-managed medium
> + *
> + * More detail goes here ...

Ok

> 
> > + *
> > + * @param dfu - dfu entity to which we want to store data
> > + * @param buf - fixed memory addres from where data starts
> > + * @param size - number of bytes to write
> > + *
> > + * @return - 0 on success, other value on failure
> > + */
> > +int dfu_write_from_mem_addr(struct dfu_entity *dfu, void *buf, int
> > size)
> 
> const void *buf? My understanding is that this buffer is not changed.
In principle I agree with you.

For practical reasons, I would opt for leaving the buf definition as
void *, since other functions (like globally visible dfu_write) uses
this buffer as void *.

Adding const there would require removing this qualifier in the line
[1] since then dp pointer is passed to dfu_write(), which requires void
* pointer.
> 
> > +{
> > +       unsigned long dfu_buf_size, write, left = size;
> > +       int i, ret = 0;
> > +       void *dp = buf;
	    ^^^^^^^^^^^^^^^^ [1]

> > +
> > +       /*
> > +        * Here we must call dfu_get_buf(dfu) first to be sure that
> > dfu_buf_size
> > +        * has been properly initialized - e.g. if "dfu_bufsiz" has
> > been taken
> > +        * into account.
> > +        */
> > +       dfu_get_buf(dfu);
> > +       dfu_buf_size = dfu_get_buf_size();
> > +       debug("%s: dfu buf size: %lu\n", __func__, dfu_buf_size);
> > +
> > +       for (i = 0; left > 0; i++) {
> > +               write = min(dfu_buf_size, left);
> > +
> > +               debug("%s: dp: 0x%p left: %lu write: %lu\n",
> > __func__,
> > +                     dp, left, write);
> > +               ret = dfu_write(dfu, dp, write, i);
> > +               if (ret) {
> > +                       error("DFU write failed\n");
> > +                       return ret;
> > +               }
> > +
> > +               dp += write;
> > +               left -= write;
> > +       }
> > +
> > +       ret = dfu_flush(dfu, NULL, 0, i);
> > +       if (ret)
> > +               error("DFU flush failed!");
> > +
> > +       return ret;
> > +}
> > diff --git a/include/dfu.h b/include/dfu.h
> > index 7f78cc2..e624c41 100644
> > --- a/include/dfu.h
> > +++ b/include/dfu.h
> > @@ -162,6 +162,7 @@ bool dfu_usb_get_reset(void);
> >  int dfu_read(struct dfu_entity *de, void *buf, int size, int
> > blk_seq_num); int dfu_write(struct dfu_entity *de, void *buf, int
> > size, int blk_seq_num); int dfu_flush(struct dfu_entity *de, void
> > *buf, int size, int blk_seq_num);
> 
> Please can you put your function comment here in the header?

ok

> 
> > +int dfu_write_from_mem_addr(struct dfu_entity *dfu, void *buf, int
> > size); /* Device specific */
> >  #ifdef CONFIG_DFU_MMC
> >  extern int dfu_fill_entity_mmc(struct dfu_entity *dfu, char
> > *devstr, char *s); --
> > 2.1.4
> >
> 
> Regards,
> Simon

Best regards,

Lukasz Majewski