From mboxrd@z Thu Jan  1 00:00:00 1970
From: Marek Vasut <marex@denx.de>
Date: Wed, 4 Jul 2012 16:35:07 +0200
Subject: [U-Boot] [PATCH 2/7] dfu:usb: DFU USB function (f_dfu) support
	for g_dnl composite gadget
In-Reply-To: <20120704103956.6a390cc6@lmajewski.digital.local>
References: <1341308291-14663-1-git-send-email-l.majewski@samsung.com>
	<201207032321.56085.marex@denx.de>
	<20120704103956.6a390cc6@lmajewski.digital.local>
Message-ID: <201207041635.07670.marex@denx.de>
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

Dear Lukasz Majewski,

[...]

> > > +static void handle_getstatus(struct usb_request *req)
> > > +{
> > > +	struct dfu_status *dstat = (struct dfu_status *)req->buf;
> > > +	struct f_dfu *f_dfu = req->context;
> > > +
> > > +	switch (f_dfu->dfu_state) {
> > 
> > > +	case DFU_STATE_dfuDNLOAD_SYNC:
> > What's this crazy cammel case in here ? :-)
> 
> I know, that camel case descriptions are not welcome :-),
> but those are written in this way for purpose.
> 
> dfuDNLOAD_SYNC is the exact state name mentioned at DFU specification.
> Other states - like dfuDNBUSY are exactly the same.
> 
> From mine experience it is more readable. Please consider for
> example the Fig. A1 - Interface state transition diagram from page 28
> at DFU_1.1.pdf spec (link below).
> 
> http://www.usb.org/developers/devclass_docs/DFU_1.1.pdf

OK, good.

> > > +	case DFU_STATE_dfuDNBUSY:
> > > +		f_dfu->dfu_state = DFU_STATE_dfuDNLOAD_IDLE;
> > > +		break;
> > > +	case DFU_STATE_dfuMANIFEST_SYNC:
> > > +		break;
> > > +	default:
> > > +		break;
> > > +	}
> > > +
> > > +	/* send status response */
> > > +	dstat->bStatus = f_dfu->dfu_status;
> > > +	/* FIXME: set dstat->bwPollTimeout */
> > 
> > FIXME ... so fix it ;-)
> 
> This is not u-boot related - will be removed
> 
> > > +	dstat->bState = f_dfu->dfu_state;
> > > +	dstat->iString = 0;
> > > +}
> > > +
> > > +static void handle_getstate(struct usb_request *req)
> > > +{
> > > +	struct f_dfu *f_dfu = req->context;
> > > +
> > > +	((u8 *)req->buf)[0] = f_dfu->dfu_state & 0xff;
> > 
> > Now ... this is ubercrazy ... can't this be made without this
> > typecasting voodoo?
> 
> The problem is that req->buf is a void pointer. And the goal is to
> store dfu state at 8 bits.

Sure, but why not make the buffer u8 ?

> > > +	req->actual = sizeof(u8);
> > > +}
> > > +

[...]

> > > +static int
> > > +dfu_prepare_strings(struct f_dfu *f_dfu, int n)
> > > +{
> > > +	struct dfu_entity *de = NULL;
> > > +	int i = 0;
> > > +
> > > +	f_dfu->strings = calloc(((n + 1) * sizeof(struct
> > > usb_string)), 1);
> > 
> > calloc(n, 1) ... that's the same as malloc(), isn't it ?
> 
> I now wonder how mine mind produced this :-)
> 
> Correct version:
> 
> f_dfu->strings = calloc(sizeof(struct usb_string), n + 1);
> 
> I prefer calloc, since it delivers zeroed memory.

ok, that's better :)

[...]