new utility usr.sbin/spi

[bobf_mrp3.com]

Summary

similar to i2c, allows command line operation and testing of spi interfaces, including querying status and setting mode/speed.

Test Plan

there are some parameter error conditions that do not generate informative messages as to the reason for failure (you simply get a 'usage' output). They could be made more 'friendly'. Additional testing could be made over time [and patches submitted] to cover all of these.

[rpokala]

I started but didn't finish reading through the manpage. My comments so far are in-line.

I suspect running igor (textproc/igor) against the page would also be helpful.

usr.sbin/spi/spi.8

27 ↗

(On Diff #41331)

I think this is usually done as:

The
.Nm
utility can be use to perform raw data transfers (read, write,

28 ↗

(On Diff #41331)

or simultaneous read/write) with devices on the SPI bus, via the
.Xr spigen 4
device.

30 ↗

(On Diff #41331)

A
.Xr spigen 4
device is associated with a specific
.Sq chip select
.Pq cs
pin on the

32 ↗

(On Diff #41331)

specified on the command line,
.Nm
assumes
.Sq spigen0 .

34 ↗

(On Diff #41331)

For more information on the spigen device, see
.Xr spigen 4 .

37 ↗

(On Diff #41331)

The longest option is "-f device", so this should be

.Bl -tag -width ".Fl f Ar device"

39 ↗

(On Diff #41331)

Hex numbers are not digits; perhaps "2-character", or "8-bit"?

[bobf_mrp3.com]

usr.sbin/spi/spi.8

27 ↗

(On Diff #41331)

thanks for all of this, I'm not that good at man pages, and although the markup 'works' it may not be the way you'd want it in the base distro. So I'll look over all of this and update it accordingly. Still learning how to use THIS system too, so if there is an easy way to 'patch the patch'...

[rpokala]

Oh, heh, I didn't even realize that this wasn't @ian, since he and I talked about this yesterday. :-)

WRT the markup: I spent several days recently writing two manpages from scratch myself, so it's (sadly) still in my brain. :-)

WRT "patching the patch", you can use the arc command-line interface to Phabricator. See

https://wiki.freebsd.org/action/show/Phabricator

and particularly

https://wiki.freebsd.org/action/show/Phabricator#Address_Revision_Feedback

Thanks!

[bobf_mrp3.com]

usr.sbin/spi/spi.c

185 ↗

(On Diff #41331)

optarg[1] == 0 (not '0') - oops

[bobf_mrp3.com]

usr.sbin/spi/spi.8

30 ↗

(On Diff #41331)

kept 'An' rather than 'A' [seems to read better].

[bobf_mrp3.com]

applied man page changes to spi.8 (thanks) . may tweek it again based on 'igor'.

added diff for usr.sbin/Makefile (adding 'spi' to the list of things to build)

made minor change to spi.c (a bug I found while testing it).

[bobf_mrp3.com]

I'm making additional (igor-based, other) changes to man page. will update once complete. Otherwise, it's essentially done.

[bobf_mrp3.com]

this one passes 'igor' on the man page, builds properly. no known issues with it. I think it's ready to ship.

[bcr]

You need to bump the .Dd of the man page to the day of the commit, since these are content changes. Thank you for working on this.

[• ian]

usr.sbin/spi/spi.c

84 ↗

(On Diff #41389)

I have no idea what this comment means. It wasn't until I saw how the array was used that I realized that this table is for reversing bits in a byte, the comment should just say that :) (and maybe be named something like reversebits)

204 ↗

(On Diff #41389)

strlcat() is better than strncat() for this sort of thing, easier to use.

But, make the logic something like:

if (optarg[0] == '/')
    strlcpy(dev_name, optarg, sizeof(dev_name);
else
    snprintf(dev_name, sizeof(dev_name), "/dev/%s", optarg);

So that it works with -f spigen0 and -f /dev/spigen0.

215 ↗

(On Diff #41389)

You can do better error checking with strtol() and strtoul(), something like:

opt.mode = strtol(optarg, &p, 0);
if (p == optarg || opt.mode < 0 || opt.mode > 3)
	/* report error */

252 ↗

(On Diff #41389)

the standard comment for this (recognized by code analysis tools to supress warnings) is

/* FALLTHROUGH */

422 ↗

(On Diff #41389)

variable names should be lowercase, and especially should not have hungarian prefixes.

641 ↗

(On Diff #41389)

if you're not going to send separate command and data, might as well put everything into the command array and leave data length zero.

[bobf_mrp3.com]

usr.sbin/spi/spi.c

422 ↗

(On Diff #41389)

I got rid of the more 'blatant' hungarian naming, but things like 'hdev' and 'popt' remain. I could change those too, I suppose, but at some point you have to say 'does it really matter?'

[bobf_mrp3.com]

made recommended changes by ian (some 'hungarians' might remain, if you want to extend the definition to include things like 'hdev' and 'popt').

added the 'command' feature we discussed in IRC, implemented as '-C'. In short, if you include bytes on the command line via '-C' they are sent before any data bytes, and the values read back during this part are discarded i.e. not part of the data output.

So, as in the help file, the following command:

spi -C "00 01" -d w

would send a 00 byte, a 01 byte, and read back nothing. And this command:

spi -C "12 34" -d r -c 2

would read nothing from stdin, send '12 34 00 00', and write the last 2 read-in bytes to stdout.

Additional clarification of a 'feature' (was 'bug') that allows you to skip the white space between hexadecimal characters. So "0000" is treated the same as "00 00". Since 2-character pairs are enforced, there's no danger of ambiguity if you leave off a digit.

Additional testing still needed to make sure I didn't break anything after making the '-C' change, which really is very very useful.

[bobf_mrp3.com]

usr.sbin/spi/spi.c

393 ↗

(On Diff #41521)

the 3 lines above this one need to be removed. it's a bug. I changed tactics while adding -C and forgot to remove them. Found during testing. the call to 'perform_read' should always be done regardless of the presence of command bytes (this is handled later on).

[bobf_mrp3.com]

fixed bug related to '-C' (removed the 3 lines of code I mentioned in an inline comment), and another bug in 'verbose' output (newlines being done wrong)

[bobf_mrp3.com]

usr.sbin/spi/spi.c

512 ↗

(On Diff #41631)

with the use of 'symlink' alias devices for https://reviews.freebsd.org/D15301 this function will need to be able to handle those symlinks. The implementation in this utility could follow the symlink and store the canonical device name and unit (from the name), or it could re-calculate it here on each call [which isn't very frequent]. Either one would be acceptable and compatible, so long as the canonical device naming is consistent with the sysctl variable naming; that is, the 'spigen0' device must have its sysctl vars in 'dev.spigen.0' (and so on).

[bobf_mrp3.com]

updated to be compatible with https://reviews.freebsd.org/D15301 changes. canonicalizes device file name so that a symlink can be used. Determines unit number from device file name using the canonicalized device name itself, or a matching 'spigen=' entry in '%location' (see D15301).

[bobf_mrp3.com]

usr.sbin/spi/spi.c

46 ↗

(On Diff #42675)

may need to make this 'spigen0.0' depending on how things go with DS15301 and whether or not 11.x will be relevant.

[bobf_mrp3.com]

for RPi it needs support added to the bcm2835_spi driver. See D15031

[bobf_mrp3.com]

made changes to accomodate latest changes to D15301 . Removed reliance on sysctl info (can be added later, as needed).

[• ian]

usr.sbin/spi/Makefile

1 ↗

(On Diff #44264)

listing the author is uncommon in other nearby makefiles (only bsnmpd does it). most have just the $FreeBSD:$ line followed by a blank line.

usr.sbin/spi/spi.8

1 ↗

(On Diff #44264)

This needs a copyright block at the top.

And a .Dd line.

usr.sbin/spi/spi.c

3 ↗

(On Diff #44264)

Recent consensus is that "all rights reserved" hasn't been necessary since 1989 and should be left off new files.

It's a good idea to exactly copy src/share/etc/bsd-style-copyright and edit in your info. That gets you the new SPDX id stuff and all.

28 ↗

(On Diff #44264)

This probably shouldn't be here

45 ↗

(On Diff #44264)

there should be a tab between #define and the symbol name (icky arbitrary style(9) rule)

422 ↗

(On Diff #41389)

popt for pointer-to-opt was in use long before hungarian came along. as long as we don't have camelcase and lpszStr (which I can only ever see as "lip-sized string") I think we're good. :)

[bobf_mrp3.com]

cleaned up a couple of nitty style-related things. Ian go ahead and edit whatever you see fit for style etc.. Thanks also, because I probably wouldn't know what to do anyway.

[bobf_mrp3.com]

additional things that showed up after the last diff