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

[Lukasz Majewski]

Hi Marek,

> 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.

From my perspective this looks like a proper behaviour.

We send response that we have received such request and display
information that it is not supported.

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



-- 
Best regards,

Lukasz Majewski

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