[PATCH v6 01/17] dfu: rename dfu_tftp_write() to dfu_write_by_name()

[AKASHI Takahiro]

Tom, Lukasz,

On Thu, Sep 10, 2020 at 10:41:39PM -0400, Tom Rini wrote:
> On Fri, Sep 11, 2020 at 09:46:14AM +0900, AKASHI Takahiro wrote:
> > On Wed, Sep 09, 2020 at 10:26:34PM -0400, Tom Rini wrote:
> > > On Thu, Sep 10, 2020 at 10:54:18AM +0900, AKASHI Takahiro wrote:
> > > > On Wed, Sep 09, 2020 at 02:30:53PM +0200, Heinrich Schuchardt wrote:
> > > > > On 07.09.20 07:34, AKASHI Takahiro wrote:
> > > > > > This function is essentially independent from tftp, 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 at linaro.org>
> > > > > > ---
> > > > > >  common/update.c                       |  5 +++--
> > > > > >  drivers/dfu/Kconfig                   |  5 +++++
> > > > > >  drivers/dfu/Makefile                  |  2 +-
> > > > > >  drivers/dfu/{dfu_tftp.c => dfu_alt.c} | 17 ++++++++++++--
> > > > > >  include/dfu.h                         | 32 +++++++++++++--------------
> > > > > >  5 files changed, 40 insertions(+), 21 deletions(-)
> > > > > >  rename drivers/dfu/{dfu_tftp.c => dfu_alt.c} (67%)
> > > > > >
> > > > > > diff --git a/common/update.c b/common/update.c
> > > > > > index 36b6b7523d50..39946776d74f 100644
> > > > > > --- a/common/update.c
> > > > > > +++ b/common/update.c
> > > > > > @@ -324,8 +324,9 @@ got_update_file:
> > > > > >  			}
> > > > > >  		} else if (fit_image_check_type(fit, noffset,
> > > > > >  						IH_TYPE_FIRMWARE)) {
> > > > > > -			ret = dfu_tftp_write(fit_image_name, update_addr,
> > > > > > -					     update_size, interface, devstring);
> > > > > > +			ret = dfu_write_by_name(fit_image_name, update_addr,
> > > > > > +						update_size, interface,
> > > > > > +						devstring);
> > > > > >  			if (ret)
> > > > > >  				return ret;
> > > > > >  		}
> > > > > > diff --git a/drivers/dfu/Kconfig b/drivers/dfu/Kconfig
> > > > > > index 0eec00ba734d..78f901ff348a 100644
> > > > > > --- a/drivers/dfu/Kconfig
> > > > > > +++ b/drivers/dfu/Kconfig
> > > > > > @@ -14,8 +14,13 @@ config DFU_OVER_TFTP
> > > > > >  	depends on NET
> > > > > >
> > > > > >  if DFU
> > > > > > +config DFU_ALT
> > > > > > +	bool
> > > > > 
> > > > > You cannot define CONFIG_DFU_ALT as bool. This symbol already exists as
> > > > > string in include/configs/odroid.h, include/configs/s5p_goni.h, et.al.
> > > > > It is used to initialize environment variable dfu_alt_info. Your patch
> > > > > leads to an error on Travis:
> > > > > 
> > > > >        arm:  +   odroid
> > > > 
> > > > Okay, will fix it.
> > > > DFU_WRITE_ALT? Or, as Heinrich suggested, move them
> > > > to dfu.c if Lukasz agrees.
> > > > 
> > > > # I haven't seen any comments from him yet though.
> > > 
> > > I think what you're introducing as "CONFIG_DFU_ALT" and renaming
> > > "drivers/dfu/dfu_tftp.c" to "drivers/dfu/dfu_alt.c" is what's confusing.
> > > That I think needs a different name than "alt" to convey that it's a
> > > generic "from DRAM to flash/etc" hook.
> > 
> > Whatever the name is, I need reviews on all of my DFU patches from Lukasz
> > in the first place.
> 
> The lower impact on the rest of the DFU subsytem and other automated
> testing that can be done, the easier it can be for someone else to
> review the DFU parts if Lukasz isn't able to find time to do it.

It sounds nice, but who will take that role?

Please name that person(s) if you're still busy on other stuffs.
        -> Lukasz

-Takahiro Akashi

> Thanks.
> 
> -- 
> Tom