alter spigen device name to indicate cs as sub-unit

[bobf_mrp3.com]

Summary

Rather than assigning spigen device names in order of creation, use a device name that corresponds to the owning spibus and cs register.

Example: spigen0.1 would be a child of spibus0, and use cs = 1

This recommended patch uses the existing 'spigenN' naming convention when FDT is not enabled. Otherwise, it grabs the spibus unit number, and 'cs' value from the ivars, and then creates an appropriate name from those two values (in lieu of using 'unit') when attaching the device.

The intent is for systems like Raspberry Pi to have a consistent way of using an SPI interface with a specific cs value from a user application. Otherwise, there is no consistent way of knowing which cs pin will be assigned to a particular spigen device. The alternative is to specify everything in "the right order" in an overlay file, which is less than ideal. Additionally, this duplicates (to some extent) the way Linux handles a similar situation with their 'spidev' device, so it would be somewhat familiar to those who also use Linux.

Test Plan

verify basic operation and correct naming.

[kevans]

sys/dev/spibus/spigen.c

192 ↗

(On Diff #42153)

I'd think it's safe to remove the #ifdef FDTs from both of these bits and uniformly across FDT and non-FDT platforms create spigen cdevs with the new naming convention, rather than mixing it up.

It seems like it would be safe- we shouldn't have hinted children on a single spibus with the same cs, right?

[bobf_mrp3.com]

sys/dev/spibus/spigen.c

202 ↗

(On Diff #42153)

ian suggested (in IRC channel) to make an alias device using the original naming scheme, as well as this naming scheme, and to use make_dev_s() and make_dev_alias_p() when doing it.

also 'cs' needs to be 'cs & (~SPIBUS_CS_HIGH)' [forgot to put that in]

[bobf_mrp3.com]

when this is all done, spigen will probably need a man page, to document the behavior, as well as documenting the sysctl API. I could make it a part of this change to add one...

[bobf_mrp3.com]

sys/dev/spibus/spigen.c

202 ↗

(On Diff #42153)

initial test code works (using make_dev_s and make_dev_alias_p).

I noticed that the alias (symlink) device names are created as owned by root:wheel with 0755 permissions. I didn't really expect this, but I expect that as it's permissions on the symlink itself, and not on the device file name that it points to, that it doesn't matter?

So the net result looks like this:
lrwxr-xr-x 1 root wheel 9 May 10 01:45 /dev/spigen0 -> spigen0.0
crw-rw---- 1 root operator 0x2c May 10 01:45 /dev/spigen0.0
crw-rw---- 1 root operator 0x2e May 10 01:45 /dev/spigen0.1
crw-rw---- 1 root operator 0x30 May 10 01:45 /dev/spigen0.2
lrwxr-xr-x 1 root wheel 9 May 10 01:45 /dev/spigen1 -> spigen0.1
lrwxr-xr-x 1 root wheel 9 May 10 01:45 /dev/spigen2 -> spigen0.2

On a related note, this might cause confusion for those who think that 'spigen0' relates to spigen0.0 through spigen0.2 . So should I use a different name instead?

The sysctl vars are using spigen.0 spigen.1 and spigen.2 . So the name that would need to change is the one prefixing the "0.0" through "0.2" versions.

So for now I'll propose 'spig0.0' through 'spig0.2' to disambiguate them. These will be the devices that user mode is expected to make use of.

If it's preferred to make THESE the symlink aliases, we can do that, too.

[bobf_mrp3.com]

this always creates the 'spigen#' device (based on unit number) as before, but then adds an alias 'spigenM.N' where 'M' is the spibus unit number, and 'N' is the cs pin ID, for each spigen device created.

it does not alter the sysctl vars, which are still based on 'dev.spibus.#' where '#' is the unit number.

It would be a simple matter to make 'spigen#' the alias, except for the inconsistency with the sysctl var naming.

[bobf_mrp3.com]

jhibbits suggested in IRC: it'd be better to have /dev/spi/X.Y, and have /dev/spigenN be a symlink, like USB is

[bobf_mrp3.com]

ian pointed out that uftdi has a '%location' sysctl that contains useful information, including the 'ugen' device alias.
"dev.uftdi.0.%location: bus=7 hubaddr=3 port=2 devaddr=4 interface=0 ugen=ugen7.4"

this ought to be implemented in spigen as well

[bobf_mrp3.com]

update '%location' in spibus sysctl handler to reflect the spibus unit (and for spigen, spigenN.M name) in addition to the cs .

also spigenU is the alias, and spigenN.M is the device. the sysctl vars for dev.spigen.U remain as they were, other than changes to '%location'.

[• ian]

sys/dev/spibus/spibus.c

128 ↗

(On Diff #42387)

Not necessarily better. Since spibus is the creator/owner of child device info, just using devi directly is fine.

139 ↗

(On Diff #42387)

I know it was me who said having the spigen= in the location (kinda like usb ugen does) would be nice, but this is indeed kinda hacky. Let's leave it out for now and just use bus=%d cs=%d in all cases, since that really does provide all the info you need to find the right spigen device anyway.

sys/dev/spibus/spigen.c

202 ↗

(On Diff #42153)

There is general agreement that the 'spigenN' naming scheme is inadequate, but it's present in stable-11 which lessens our freedom to just change it.

My current thinking is that we should use the spigen<BUS>.<CS> scheme always, and we should define a new kernel option, SPIGEN_LEGACY_CDEVNAME that causes us to create spigen# aliases sequentially numbered from 0-n like the old scheme used to. Then in stable-11 we find the few existing kernel configs that reference spigen and add that option, to avoid breaking users relying on it, when we MFC these changes. In 12-current we have more freedom to make changes, so we don't add that option to kernel configs, but users can enable it if needed.

[• ian]

sys/dev/spibus/spigen.c

226 ↗

(On Diff #42387)

This needs to be done by setting mda_si_drv1 = dev; before calling make_dev_s(). Much of the reason for make_dev_s() and the mda stuct is to avoid the race of making a cdev appear in /dev before the cdev struct is completely filled out and ready to use in the driver.

[bobf_mrp3.com]

sys/dev/spibus/spibus.c

139 ↗

(On Diff #42387)

I will take the '!strcmp(devclass "spigen")' compare out and just use what's in the 'else' clause. simpler too.

sys/dev/spibus/spigen.c

202 ↗

(On Diff #42153)

will place #ifdef SPIGEN_LEGACY_CDEVNAME around the next section that creates the alias, and also in the unload part (and variable declaration in the struct)

[• ian]

sys/dev/spibus/spigen.c

229 ↗

(On Diff #42387)

also move these 3 lines to before setting up the cdev, on the same theory... don't make /dev/spigenX.Y visiable in devfs until everything is set up and ready to start servicing requests in the driver.

[bobf_mrp3.com]

applied Ian's comments, added definition for 'options SPIGEN_LEGACY_CDEVNAME' in opt_spi.h . builds, have not tested the bootup (yet). will do that next.

[bobf_mrp3.com]

need to verify this is correct by booting it up