On Wednesday, July 28, 2010 13:56:42 Stefano Babic wrote: > The patch adds a cable driver that uses GPIOs > to control the JTAG interface. > It requires that the GPIOs sysfs is enabled > on the system. The GPIOs required are passed to the > driver at the connect time.
i'm guessing the "1/2" in the subject is an accident and there is no 2/2 ?
> --- a/configure.ac
> +++ b/configure.ac
> +CHECK_DRIVER([$cabledrivers], [enabled_cable_drivers], [gpio],
[ENABLE_CABLE_GPIO])
hmm, what tree exactly is this patch against ? doesnt look like the official
urjtag git tree nor does it seem to be up-to-date with the urjtag/ rewrite.
> --- a/src/tap/cable.c
> +++ b/src/tap/cable.c
> @@ -59,6 +59,7 @@ extern cable_driver_t igloo_cable_driver;
> extern cable_driver_t keithkoep_cable_driver;
> extern cable_driver_t lattice_cable_driver;
> extern cable_driver_t mpcbdm_cable_driver;
> +extern cable_driver_t gpio_cable_driver;
> extern cable_driver_t triton_cable_driver;
> extern cable_driver_t jim_cable_driver;
> extern cable_driver_t wiggler_cable_driver;
> @@ -116,6 +117,9 @@ cable_driver_t *cable_drivers[] = {
> #ifdef ENABLE_CABLE_MPCBDM
> &mpcbdm_cable_driver,
> #endif
> +#ifdef ENABLE_CABLE_GPIO
> + &gpio_cable_driver,
> +#endif
> #ifdef ENABLE_CABLE_TRITON
> &triton_cable_driver,
> #endif
this too indicates you're using a pretty old tree and thus missing a lot of
the cool unification work
> --- /dev/null
> +++ b/src/tap/cable/gpio.c
>
> +#include <sys/types.h>
> +#include <sys/stat.h>
> +#include <sys/mman.h>
unless i missed something, none of these headers are used. since they arent
portable, please punt them.
> +/* pin mapping */
> +enum {
> + GPIO_TDI = 0,
> + GPIO_TCK,
> + GPIO_TMS,
> + GPIO_TDO
> +};
your coding style seems to be off. you need to use spaces and indents should
be 4 spaces deep.
> +static int gpio_export(int gpio, int export) {
> +
decuddle that brace
> + int fd, ret;
> + char *fname;
> + char gpio_number[10];
> +
> + if (export)
> + fname = GPIO_EXPORT_PATH;
> + else
> + fname = GPIO_UNEXPORT_PATH;
> +
> + fd = open(fname, O_WRONLY);
> + if (!fd) {
> + printf("I cannot open %s to (un)export GPIO %d: %d\n", fname,
> gpio,
> errno); + return -1;
> + }
> +
> + snprintf(gpio_number, sizeof(gpio_number) - 2, "%d\n", gpio);
> +
> + ret = write(fd,gpio_number, strlen(gpio_number));
> + close(fd);
i would imagine using stdio with fopen here would make the code nicer
you also have to update the driver to use the new urj logging service as no
cable code is allowed to write to stdout/stderr anymore
> +static int gpio_direction(int gpio_number, int out)
> +{
> + int fd, ret;
> + char fname[50];
> + char buf[8];
> +
> + memset(fname, 0, sizeof(fname));
> + memset(buf, 0, sizeof(buf));
> + snprintf(fname, sizeof(fname),
> + "%sgpio%d/direction", GPIO_PATH, gpio_number);
> +
> + fd = open(fname, O_WRONLY);
> + if (!fd) {
> + printf("I cannot open %s to set GPIO\n", fname);
> + return -1;
> + }
> +
> + if (out)
> + strcpy(buf, "out");
> + else
> + strcpy(buf, "in");
> +
> + ret = write(fd,buf, strlen(buf));
> + close(fd);
> +
> + if (ret != strlen(buf)) {
> + printf("Error setting direction gpio %d %s %d\n",
> + gpio_number, out ? "out" : "in", ret);
> + return -1;
> + }
considering you sprintf & strcpy straight to those buffers, the memsets are
largely useless. again, here you would probably benefit from using fopen()
and friends instead of open() and such directly.
> +static int gpio_set_value(int fd, int value) {
> + int ret;
> + char buf[8];
> +
> + buf[0] = value + '0';
> +
> + ret = write(fd, buf, 1);
> +
> + if (ret != 1) {
> + printf("Error %d setting value gpio\n", ret);
> + return -1;
> + }
use fopen() and such
> +int gpio_get_value(int gpio) {
> + int ret;
> + char buf[8];
> + int fd;
> + char fname[50];
> +
> + snprintf(fname, sizeof(fname),
> + "%sgpio%d/value", GPIO_PATH, gpio);
> +
> + fd = open(fname, O_RDONLY);
> + if (!fd) {
> + printf("I cannot open %s to read GPIO %d\n", fname, gpio);
> + return -1;
> + }
> +
> + ret = read(fd, buf, 8);
> + close(fd);
> +
> + if (ret <= 0) {
> + printf("Error getting value gpio %d\n", errno);
> + return -1;
> + }
> +
> + if (buf[0] != '0' && buf[0] != '1') {
> + printf("Erroneous value for gpio %d: 0x%x\n", gpio, buf[0]);
> + return -1;
> + }
ditto
> +static int
> +gpio_open( cable_t *cable )
functions should be:
void foo (int i);
> +{
> + gpio_params_t *p = cable->params;
> + char fname[50];
> + int i, ret;
> +
> + /* Export all gpios */
> + for (i = 0; i < sizeof(gpios) / sizeof(gpios[0]); i++) {
use ARRAY_SIZE()
> + ret = gpio_export(gpios[i], 1);
> +
> + if (ret) {
> + printf ("I cannot export gpio (index) %d\n", i);
> + return -1;
> + }
> + gpio_direction(gpios[i], (i == GPIO_TDO) ? 0 : 1);
> + if (i != GPIO_TDO) {
> + memset(fname, 0, sizeof(fname));
> + snprintf(fname, sizeof(fname),
> + "%sgpio%d/value", GPIO_PATH, gpios[i]);
replace the memset with a single NUL terminator at the end of the array
> + printf( _("Initializing QONG GPIO JTAG Chain\n") );
dont think QONG is appropriate anymore
> + for (i = GPIO_TDI; i <= GPIO_TDO; i++) {
> + if (gpios[i] < 0) {
> + printf( _("Error: you must configure the gpios you
> need!\n") );
> + gpio_help(params[0]);
> + return 1;
> + }
> + }
> +
> + cable_params = malloc( sizeof(gpio_params_t));
> + if (!cable_params) {
> + printf( _("%s(%d) Out of memory\n"), __FILE__, __LINE__ );
> + free( cable );
> + return 4;
> + }
> + memset(cable_params, 0, sizeof(gpio_params_t));
use calloc if you want zero-ed memory
a lot of my comments apply to the rest of this patch, bu not much point in
quoting & copying & pasting those comments some more.
-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
