[U-Boot] [PATCH 06/10] usb:g_dnl:f_thor: USB download function to support TIZEN's THOR protocol

[Marek Vasut <marex@denx.de>]

Dear Lukasz Majewski,

> Hi Marek,
> 
> > Dear Lukasz Majewski,
> > 
> > [...]
> > 
> > > +static struct f_thor *thor_func;
> > > +static inline struct f_thor *func_to_thor(struct usb_function *f)
> > > +{
> > > +	return container_of(f, struct f_thor, usb_function);
> > > +}
> > > +
> > > +DEFINE_CACHE_ALIGN_BUFFER(char, thor_tx_data_buf, sizeof(struct
> > > rsp_box)); +DEFINE_CACHE_ALIGN_BUFFER(char, thor_rx_data_buf,
> > > sizeof(struct rqt_box));
> > 
> > This should either be uint8_t or unsigned char. A buffer shall not be
> > (signed) char.
> 
> Yes. I agree. This buffer shall be unsigned char. I will correct that.
> 
> > Also, I suspect you want to use DEFINE_CACHE_ALIGN_BUFFER here, no ?
> 
> I'm a bit confused.... I do use DEFINE_CACHE_ALIGN_BUFFER for those
> buffers.

OOPS!

> > > +/* ********************************************************** */
> > > +/*         THOR protocol - transmission handling	      */
> > > +/* ********************************************************** */
> > > +DEFINE_CACHE_ALIGN_BUFFER(char, f_name, F_NAME_BUF_SIZE);
> > 
> > Ditto
> 
> I believe that buffer for storing file name (f_name) shall be defined
> as char.

OK, good point.

[...]
> > > +
> > > +	rsp->rsp = rqt->rqt;
> > > +	rsp->rsp_data = rqt->rqt_data;
> > > +
> > > +	switch (rqt->rqt_data) {
> > > +	case RQT_CMD_REBOOT:
> > > +		debug("TARGET RESET\n");
> > > +		send_rsp(rsp);
> > > +		g_dnl_unregister();
> > > +		dfu_free_entities();
> > > +		run_command("reset", 0);
> > > +		break;
> > > +	case RQT_CMD_POWEROFF:
> > > +	case RQT_CMD_EFSCLEAR:
> > > +		send_rsp(rsp);
> > 
> > This case fallthrough is intentional here ?
> 
> Yes. Thor protocol requires to receive response from device even when
> HOST PC ordered it to power off.
> 
> Also, on the target only reboot command is supported.

But this will fall through into the default: branch here.

> > > +	default:
> > > +		printf("Command not supported -> cmd: %d\n",
> > > rqt->rqt_data);
> > > +		return -EINVAL;
> > > +	}
> > > +
> > > +	return true;
> > > +}

[...]