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 ?
> --- /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
> +static int gpio_export (int gpio, int export)
> +{
> +
> + int ret;
kill that newline after the brace
> + if (!fp) {
> + urj_log (URJ_LOG_LEVEL_NORMAL,
> + _("I cannot open %s to (un)export GPIO %d: %d\n"), fname, gpio,
> errno); + return -1;
> + }
use urj_warning() and you can drop the errno stuff. also, i would avoid using
personal pronouns such as "I".
> + snprintf (gpio_number, sizeof(gpio_number) - 2, "%d\n", gpio);
> +
> + ret = fwrite (gpio_number, strlen(gpio_number), 1, fp);
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
> +static int gpio_set_value (FILE *fp, int value)
> +{
> + int ret;
> + char buf[8];
> +
> + buf[0] = value + '0';
> +
> + ret = fwrite (buf, 1, 1, fp);
i would just fprintf the value as an %u and let the kernel worry about it
> +static int gpio_get_value (int gpio)
> +{
> + int ret;
> + char buf[8];
> + char fname[50];
> + FILE *fp;
> +
> + snprintf (fname, sizeof(fname),
> + "%sgpio%d/value", GPIO_PATH, gpio);
> +
> + fp = fopen (fname, "r");
> + if (!fp) {
> + urj_log (URJ_LOG_LEVEL_NORMAL,
> + _("I cannot open %s to read GPIO %d\n"), fname, gpio);
> + return -1;
> + }
> +
> + ret = fread (buf, 1, 1, fp);
fscanf
> + if (buf[0] != '0' && buf[0] != '1') {
> + printf("Erroneous value for gpio %d: 0x%x\n", gpio, buf[0]);
missed a printf
> +static void
> +gpio_disconnect (urj_cable_t *cable)
> +{
> + gpio_close (cable);
> + urj_tap_chain_disconnect (cable->chain);
> +}
maybe call the generic disconnect first and then do gpio_close() ?
-mike
signature.asc
Description: This is a digitally signed message part.
------------------------------------------------------------------------------ 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
