On Friday, August 19, 2011 00:05:53 steve tell wrote: > Two new routines are added to tap/register.c: > > uint64_t urj_tap_register_get_field_value (const urj_tap_register_t *tr, > int msb, int lsb); int urj_tap_register_set_field_value > (urj_tap_register_t *tr, uint64_t val, int msb, int lsb);
i'm not too keen on this name. how about:
urj_tap_register_set_value_bit_range
> --- a/urjtag/src/tap/discovery.c
> +++ b/urjtag/src/tap/discovery.c
>
> + if(maxlen < MIN_REGISTER_LENGTH)
> + maxlen = DEFAULT_MAX_REGISTER_LENGTH;
i'd prefer we do:
if (maxlen == 0)
maxlen = DEFAULT_MAX_REGISTER_LENGTH;
all other values will automatically get handled by the for() loop by kicking
out early and returning -1. i'd rather not allow invalid values to go
silently unnoticed.
also, please use "if (" and not "if(". this comes up a few times in the
patch.
> --- a/urjtag/src/tap/register.c
> +++ b/urjtag/src/tap/register.c
>
> + if(msb >= lsb) {
> + for (bit = lsb; bit <= msb; ++bit) {
> + tr->data[bit] = !!(val & 1);
> + val >>= 1;
> + }
> + } else {
> + for (bit = lsb; bit >= msb; --bit) {
> + tr->data[bit] = !!(val & 1);
> + val >>= 1;
> + }
> + }
unsigned int bit, step = msb >= lsb ? 1 : -1;
for (bit = lsb; bit <= msb * step; bit += step) {
tr->data[bit] = !!(val & 1);
val >>= 1;
}
also, i see you've let some tabs creep in here ... we only use spaces
> +uint64_t
> +urj_tap_register_get_field_value (const urj_tap_register_t *tr, int msb,
> +{
> + if (!tr)
> + return 0;
this seems to have a diff set of sanity checks than the set func. the set
func always assumes tr is valid, and instead checks lsb/msb against tr. the
get func always assumes the lsb/msb are sane wrt tr.
they should have the exact same checks i think.
> + if(msb >= lsb) {
> + for (bit = lsb; bit <= msb; ++bit) {
> + if (tr->data[bit] & 1)
> + l |= b;
> + b <<= 1;
> + }
> + } else {
> + for (bit = lsb; bit >= msb; --bit) {
> + if (tr->data[bit] & 1)
> + l |= b;
> + b <<= 1;
> + }
> + }
use the step trick i suggested for the set func
-mike
signature.asc
Description: This is a digitally signed message part.
------------------------------------------------------------------------------ EMC VNX: the world's simplest storage, starting under $10K The only unified storage solution that offers unified management Up to 160% more powerful than alternatives and 25% more efficient. Guaranteed. http://p.sf.net/sfu/emc-vnx-dev2dev
_______________________________________________ UrJTAG-development mailing list [email protected] https://lists.sourceforge.net/lists/listinfo/urjtag-development
