Mike Frysinger wrote:
> 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.
Hi Mike,
>
> i'm guessing the "1/2" in the subject is an accident and there is no 2/2 ?
Yes, generated by format-patch...
>
>> --- 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.
I see, I am not updated. I will rebase the patch on the current tree and
I will resubmit, sorry for the confusion.
> your coding style seems to be off. you need to use spaces and indents should
> be 4 spaces deep.
Got it. I will fix it.
>
> i would imagine using stdio with fopen here would make the code nicer
Mike, I have only a concern using fopen/fwrite when I set the gpios. See
a later comment.
> 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
Agree, I will fix it.
> considering you sprintf & strcpy straight to those buffers, the memsets are
> largely useless.
Agree, I can get rid of memset
> 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
Well, the point to use directly open/write is to not have buffered
functions and to be sure the gpio is set as soon as possible. Using
fopen/fwrite I have at least to call always fflush() after setting the
gpios. Should not be better to use open/write to drive the hardware ?
>> + /* Export all gpios */
>> + for (i = 0; i < sizeof(gpios) / sizeof(gpios[0]); i++) {
>
> use ARRAY_SIZE()
Ok, thanks.
>> + printf( _("Initializing QONG GPIO JTAG Chain\n") );
>
> dont think QONG is appropriate anymore
...forget to drop it, thanks.
> 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.
Thanks, I will apply your comments globally for the whole patch.
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
