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

[Sascha Hauer]

On Tue, Feb 06, 2018 at 05:49:10PM +0100, Aleksander Morgado wrote:
> >> +
> >> +#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.
> >
> 
> Ah, sure.
> 
> >> +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?
> >
> 
> You mean doing this instead?
> 
> BAREBOX_RATP_CMD_START(PING)
> .cmd = ratp_cmd_ping,
> .id = BB_RATP_TYPE_PING
> BAREBOX_RATP_CMD_END

Yes, that's what I meant.

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

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

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

[Aleksander Morgado]

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

Ah, sure.

>> +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?
>

You mean doing this instead?

BAREBOX_RATP_CMD_START(PING)
.cmd = ratp_cmd_ping,
.id = BB_RATP_TYPE_PING
BAREBOX_RATP_CMD_END


-- 
Aleksander
https://aleksander.es

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

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

[Sascha Hauer]

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

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