In reading this, remember that anything I don't complain about looked
fine to me. Overall this looks like pretty good code (though I haven't
compiled it and of course have no hardware to test it on). Thanks for
your continuing efforts on this SPI stuff.
> [-- Attachment #2:
> 0002-Add-Support-for-the-Mesa-Hostmot2-Buffered-SPI-funct.patch --]
> [-- Type: application/octet-stream, Encoding: base64, Size: 43K --]
It would be convenient if you could teach your mailer to attach these as
text/plain or text/x-patch. As it is, it requires an extra step to
actually view the attachments, at least in my mailer.
Looking at the mesa_7i65.comp, I am concerned about the order of
operations. In fact, I don't see how your design can achieve the
desired order.
A standard servo thread sequence is
read position feedback (A)
calculate new position
calculate new pid outputs
write new amplifer commands (B)
but the hal function mesa_7i65 does both reading and writing in one go.
So if you place it as (A) it will not write the new amplifier command
until the servo cycle after it was calculated; if you place it as
(B) then the feedback position seen in the next servo cycle will
actually be the one from late in this servo cycle.
The implementation I'd imagined involved mesa_7i65 and other bspi
drivers defining *NO* hal functions. Instead, they would register
function pointers with the hostmot2 driver to call at the appropriate
times (when processing a read or preparing a write). This would
require a modification to comp, which currently always rejects a
component with no functions.
> +int hm2_bspi_write_chan(char* name, int chan, u32 val)
> +{
> ...
> + r = hm2->llio->write(hm2->llio, hm2->bspi.instance[i].addr[chan],
> &buff, sizeof(u32));
> + if (r < 0) {
> + HM2_ERR("BSPI: hm2->llio->write failure %s\n", name);
> + }
> +
> + return 0;
> +}
It seems like this should be 'return r;' at the end, or maybe 'return
-1' in the 'if', so that the caller can determine that a failure has
occurred.
> +EXTRA_SETUP(){
> ...
> + if (r < 0) {
> + HM2_ERR_NO_LL("There have been %i errors during channel setup, "
> + "quitting\n", -r);
> + return r;
> + }
The return value from EXTRA_SETUP should be a negative errno value like
-EINVAL. This value will ultimately be used to look up the error
message to display from insmod. As written, 1 error will be reported as
'Operation not permitted' and 2 errors as 'No such file or directory',
and so on. (there are a lot of mistakes of this kind in emc today, but
I'd rather not make any new ones)
> @@ -79,12 +77,9 @@ int hm2_register_tram_write_region(hostmot2_t *hm2, u16
> addr, u16 size, u32 **bu
>
> return 0;
> }
> -
> -
> int hm2_allocate_tram_regions(hostmot2_t *hm2) {
Avoid unnecessary whitespace changes.
> + HM2_DBG("Parameter = %s\n", token);
> +
Not sure whether this should be included or whether it was just a
debugging thing that could just be dropped.
> Subject: [PATCH 3/3] Documentation for the Buffered SPI Hostmot2 Driver.
Hooray, documentation!
> +The exported functions are:
> +
> +int hm2_bspi_setup_chan(char *name, int chan, int cs, int bits, float mhz,
> +int delay, int cpol, int cpha, int clear, int echo)
It's great to have documentation to go with the code. I would
personally split this part into a "section 3" (programming APIs) manpage
but this is an excellent start.
> The expectation is that a sub-driver for each Mesa SPI daughter card
> +will be made available by Mesa or the EMC2 team so that the following
> +information is only for people integrating their own hardware.
Splitting the API docs into their own manual pages with just a
cross-reference at the bottom avoids this language. IMO there is no
duty for anyone on the emc team to write software for any particular
I/O card. (I'm not saying that I don't want these drivers to be written
or shipped with emc, just that nobody owes it to anybody)
The docs might benefit from a note about the load order of the modules,
which if I understand the code is something like
loadrt hostmot2
loadrt hm2_pci
loadrt mesa_7i65 # or other SPI driver
Perhaps this could go near here (since just specifying a num_bspis won't
*do* anything but take away GPIOs)?
> +\fBnum_bspis\fR [optional, default: -1]
> +Only enable the first N Buffered SPI drivers. If N is -1 then all the drivers
> +are enabled. Each BSPI driver can address 16 devices.
Jeff
------------------------------------------------------------------------------
WhatsUp Gold - Download Free Network Management Software
The most intuitive, comprehensive, and cost-effective network
management toolset available today. Delivers lowest initial
acquisition cost and overall TCO of any competing solution.
http://p.sf.net/sfu/whatsupgold-sd
_______________________________________________
Emc-developers mailing list
Emc-developers@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/emc-developers
