Mike Frysinger wrote:
> On Thursday, July 29, 2010 09:19:20 Stefano Babic wrote:
>> +++ b/urjtag/include/urjtag/cable.h
>> @@ -59,6 +59,10 @@ typedef enum URJ_CABLE_PARAM_KEY
>> URJ_CABLE_PARAM_KEY_DESC, /* string generic_usbconn */
>> URJ_CABLE_PARAM_KEY_DRIVER, /* string generic_usbconn */
>> URJ_CABLE_PARAM_KEY_BITMAP, /* string wiggler */
>> + URJ_CABLE_PARAM_KEY_GPIOTDI, /* lu gpio used as TDI
>> + URJ_CABLE_PARAM_KEY_GPIOTDO, /* lu gpio used as
>> + URJ_CABLE_PARAM_KEY_GPIOTMS, /* lu gpio used
>> + URJ_CABLE_PARAM_KEY_GPIOTCK, /* lu gpio
>> }
>> urj_cable_param_key_t;
>
> not the fault of your patch, but this global parameter stuff is kind of
> obnoxious in general
>
> what if we changed the syntax from:
> cable gpio [tdi=...] [tdo=...] [tms=...] [tck=...]
> to something like the wiggler:
> cable gpio [tdi,tdo,tms,tck]
>
> i think that should simplify things a bit ?
Do you mean we have positional values ? Something like
cable gpio 17,12,34,23 ? It is seems to me very easy to swap some
values. And it should be always in mind that the first element is tdi,
the second one tdo, and so on.
I mean, if it is explicitely required to associate the pin number with
its name as now, there is less possibility (agree, not impossible...) to
do something wrong. And the position is not important, we can write
"cable gpio tdi=17 tdo=12 tms=34 tck=23" or "cable gpio tms=34 tck=23
tdo=12 tdi=17", and both work.
>
>> --- /dev/null
>> +++ b/urjtag/src/tap/cable/gpio.c
>>
>> +static int gpios[] = {
>> + [GPIO_TDI] = -1,
>> + [GPIO_TCK] = -1,
>> + [GPIO_TMS] = -1,
>> + [GPIO_TDO] = -1
>> +};
>
> cables can store local state in cable->params, so you'll want to drop this
> storage and allocate/initialize it on the fly
Agree, I fix it.
>
>> +static int gpio_export (int gpio, int export)
>> +{
>> +
>> + int ret;
>
> kill that newline after the brace
Ok
> use urj_warning() and you can drop the errno stuff. also, i would avoid
> using
> personal pronouns such as "I".
Understood.
> this string indirection is why i suggested fopen and friends ... you should
> be
> able to fwrite straight to the fp without needing the gpio_number/sprintf
> indirection.
>
> these last two comments seem to apply to a bunch of places in this new patch
ok, I understand now what you mean. I will fix it (globally)
>
> i would just fprintf the value as an %u and let the kernel worry about it
Understood (now)
Stefano
--
=====================================================================
DENX Software Engineering GmbH, MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-0 Fax: +49-8142-66989-80 Email: [email protected]
=====================================================================
------------------------------------------------------------------------------
The Palm PDK Hot Apps Program offers developers who use the
Plug-In Development Kit to bring their C/C++ apps to Palm for a share
of $1 Million in cash or HP Products. Visit us here for more details:
http://p.sf.net/sfu/dev2dev-palm
_______________________________________________
UrJTAG-development mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/urjtag-development
