Jeff Epler wrote:
> Modified file emc2/src/hal/drivers/mesa-hostmot2/encoder.c
>
> Full file:
> <http://cvs.linuxcnc.org/cvs/emc2/src/hal/drivers/mesa-hostmot2/encoder.c?rev=1.3>
>
> Difference:
> <http://cvs.linuxcnc.org/cvs/emc2/src/hal/drivers/mesa-hostmot2/encoder.c.diff?r1=1.2;r2=1.3>
>
> Branch: TRUNK
>
> Log:
> revision 1.3
> date: 2008/08/23 15:37:40; author: jepler; state: Exp; lines: +125 -14
> expand the hm2 encoder interface, adding a new pin and some new parameters:
> pin: index-enable
> parameters: index-invert index-mask index-mask-invert filter counter-mode
A bunch of this commit makes no sense to me... Comments inline.
> --- encoder.c 2008/05/26 05:39:24 1.2
> +++ encoder.c 2008/08/23 15:37:40 1.3
> @@ -84,9 +84,10 @@
> if (hm2->encoder.instance == NULL) {
> ERR("out of memory!\n");
> r = -ENOMEM;
> - goto fail0;
> + goto fail;
???
There's memory allocation down below that needs to be undone in some
return paths. You squashed the two failure failure return path
together, dropping the kfree(). This leaks memory on some kinds of failure.
> }
>
> + hm2->encoder.stride = md->register_stride;
The register stride is global to the hm2 instance, there's no need for
the encoder to make a private copy.
> @@ -99,14 +100,21 @@
> r = hm2_register_tram_read_region(hm2, hm2->encoder.counter_addr,
> (hm2->encoder.num_instances * sizeof(u32)), &hm2->encoder.counter_reg);
> if (r < 0) {
> ERR("error registering tram read region for Encoder Counter register
> (%d)\n", r);
> - goto fail0;
> + goto fail;
> + }
> +
> +
> + r = hm2_register_tram_read_region(hm2, hm2->encoder.latch_control_addr,
> (hm2->encoder.num_instances * sizeof(u32)), &hm2->encoder.control_reg);
> + if (r < 0) {
> + ERR("error registering tram read region for Encoder Control register
> (%d)\n", r);
> + goto fail;
> }
I'm not sure reading the latch register belongs in the TRAM. Stuff in
the TRAM gets read in each call to the HAL read() function, whether
latch is enabled or not. It might be better to put it as a conditional
llio read in a new function hm2_encoder_read(), called from the bottom
of hm2_read() like hm2_raw_read() is.
I dont know how common encoder latching is, so I may be wrong on this one.
Hm, the commit email cuts off before the end of the diff. The other
thing that looked weird to me was you took out the kfree in
hm2_encoder_cleanup(). Was that intended, and if so why?
--
Sebastian Kuzminsky
Computer Science for life, that's my direction
Instead of b-balls, my homies throw exceptions -- MC Plus+
-------------------------------------------------------------------------
This SF.Net email is sponsored by the Moblin Your Move Developer's challenge
Build the coolest Linux based applications with Moblin SDK & win great prizes
Grand prize is a trip for two to an Open Source event anywhere in the world
http://moblin-contest.org/redirect.php?banner_id=100&url=/
_______________________________________________
Emc-developers mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/emc-developers
