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