From mboxrd@z Thu Jan  1 00:00:00 1970
From: Lukasz Majewski <l.majewski@samsung.com>
Date: Thu, 10 Apr 2014 12:08:44 +0200
Subject: [U-Boot] [PATCH 2/2] dfu,
	nand: add medium specific polltimeout function
In-Reply-To: <22E0B499-3CF1-4809-8F4A-AAF720924411@antoniou-consulting.com>
References: <1397106486-1233-1-git-send-email-hs@denx.de>
	<1397106486-1233-2-git-send-email-hs@denx.de>
	<201404100954.30588.marex@denx.de>
	<22E0B499-3CF1-4809-8F4A-AAF720924411@antoniou-consulting.com>
Message-ID: <20140410120844.768cd853@amdc2363>
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

Hi Pantelis,

> Hi Marek,
> 
> On Apr 10, 2014, at 10:54 AM, Marek Vasut wrote:
> 
> > On Thursday, April 10, 2014 at 07:08:06 AM, Heiko Schocher wrote:
> >> add a possibility to add a medium specific polltimeout
> >> function. So it is possible to define different
> >> poll timeouts.
> >> 
> >> Used on nand medium, for setting the DFU_MANIFEST_POLL_TIMEOUT
> >> only on nand ubi partitions, which is currently the only
> >> usecase.
> >> 
> >> Signed-off-by: Heiko Schocher <hs@denx.de>
> >> Cc: Lukasz Majewski <l.majewski@samsung.com>
> >> Cc: Kyungmin Park <kyungmin.park@samsung.com>
> >> Cc: Marek Vasut <marex@denx.de>
> >> Cc: Pantelis Antoniou <panto@antoniou-consulting.com>
> > 
> > [...]
> > 
> >> @@ -174,6 +174,17 @@ static void dnload_request_flush(struct
> >> usb_ep *ep, struct usb_request *req) req->length,
> >> f_dfu->blk_seq_num); }
> >> 
> >> +static void dfu_set_poll_timeout_manifest(struct dfu_status
> >> *dstat,
> >> +					  struct f_dfu *f_dfu)
> >> +{
> >> +	struct dfu_entity *dfu =
> >> dfu_get_entity(f_dfu->altsetting); +
> >> +	if (dfu->poll_timeout)
> >> +		dfu_set_poll_timeout(dstat,
> >> dfu->poll_timeout(dfu));
> >> +	else
> >> +		dfu_set_poll_timeout(dstat,
> >> DFU_MANIFEST_POLL_TIMEOUT); +}
> > 
> > Don't you think it'd be better (yet more intrusive) to have all the
> > DFU users have default implementation of dfu->poll_timeout() ? Then
> > you'd be able to avoid this if and even get rid of this
> > dfu_set_poll_timeout_manifest() function.
> > 
> 
> Could work, but why not a simple accessor like this:
> 
> static inline unsigned int dfu_get_poll_timeout(struct dfu_entity
> *dfu) {
> 
> 	return dfu->poll_timeout ? dfu->poll_timeout(dfu); 
> 		DFU_MANIFEST_POLL_TIMEOUT);
> }
> 
> and  dfu_set_poll_timeout(dstat, dfu_get_poll_timeout(dfu));
> 
> You even get the benefit of have a method to read the timeout value
> if we ever needed sometime in the future.

Seems reasonable for me: +1

Some comment: 

Guys, please be consistent with CCing people. I didn't receive this
thread. Also this original reply from Pantelis was not CCed to Heiko.


> 
> > 
> > [...]
> > 
> > Best regards,
> > Marek Vasut
> 
> Regards
> 
> -- Pantelis



-- 
Best regards,

Lukasz Majewski

Samsung R&D Institute Poland (SRPOL) | Linux Platform Group