Re: [PATCH 06/10] ratp: implement generic command support

[Sascha Hauer <s.hauer@pengutronix.de>]

On Fri, Feb 02, 2018 at 12:14:38PM +0100, Aleksander Morgado wrote:
> The RATP implementation now allows executing generic commands with a
> binary interface: binary requests are received and binary responses
> are returned.
> 
> Each command can define its own RATP request contents (e.g. to specify
> command-specific options) as well as its own RATP response contents
> (if any data is to be returned).
> 
> Each command is associated with a numeric unique command ID, and for
> easy reference these IDs are maintained in the common ratp_bb header.
> Modules may override generic implemented commands or include their own
> new ones (as long as the numeric IDs introduced are unique).
> 
> Signed-off-by: Aleksander Morgado <aleksander@aleksander.es>
> ---
> @@ -11,4 +29,33 @@ void barebox_ratp_command_run(void);
>  int  barebox_ratp_fs_call(struct ratp_bb_pkt *tx, struct ratp_bb_pkt **rx);
>  int  barebox_ratp_fs_mount(const char *path);
>  
> +/*
> + * RATP commands definition
> + */
> +
> +struct ratp_command {
> +	struct list_head  list;
> +	uint16_t          id;
> +	int		(*cmd)(const struct ratp_bb *req,
> +			       int req_len,
> +			       struct ratp_bb **rsp,
> +			       int *rsp_len);
> +}
> +#ifdef __x86_64__
> +/* This is required because the linker will put symbols on a 64 bit alignment */
> +__attribute__((aligned(64)))
> +#endif
> +;
> +
> +#define BAREBOX_RATP_CMD_START(_name)							\
> +extern const struct ratp_command __barebox_cmd_##_name;					\

You shouldn't use the same name as the existing barebox commands,
otherwise there might be name clashes. Add some _ratp_ to it.

> +const struct ratp_command __barebox_cmd_##_name						\
> +	__attribute__ ((unused,section (".barebox_ratp_cmd_" __stringify(_name)))) = {	\
> +	.id		= BB_RATP_TYPE_##_name,

I am not sure if I like the magic construction of the id field. Being
able to grep for BB_RATP_TYPE_PING and finding the user has advantages.
How about manually setting the field in the body of the command instead
of constructing it?

Sascha

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox