Changeset View
Standalone View
sys/dev/spibus/spigen.c
Context not available. | |||||
#define SPIGEN_MMAP_BUSY (1 << 1) | #define SPIGEN_MMAP_BUSY (1 << 1) | ||||
struct spigen_softc { | struct spigen_softc { | ||||
device_t sc_dev; | device_t sc_dev; | ||||
struct cdev *sc_cdev; | struct cdev *sc_cdev; | ||||
struct cdev *sc_adev; /* alias device */ | |||||
struct mtx sc_mtx; | struct mtx sc_mtx; | ||||
uint32_t sc_command_length_max; /* cannot change while mmapped */ | uint32_t sc_command_length_max; /* cannot change while mmapped */ | ||||
uint32_t sc_data_length_max; /* cannot change while mmapped */ | uint32_t sc_data_length_max; /* cannot change while mmapped */ | ||||
vm_object_t sc_mmap_buffer; /* command, then data */ | vm_object_t sc_mmap_buffer; /* command, then data */ | ||||
vm_offset_t sc_mmap_kvaddr; | vm_offset_t sc_mmap_kvaddr; | ||||
Context not available. | |||||
static int | static int | ||||
spigen_attach(device_t dev) | spigen_attach(device_t dev) | ||||
{ | { | ||||
struct spigen_softc *sc; | struct spigen_softc *sc; | ||||
const int unit = device_get_unit(dev); | const int unit = device_get_unit(dev); | ||||
int cs, res; | |||||
struct make_dev_args mda; | |||||
kevans: I'd think it's safe to remove the `#ifdef FDT`s from both of these bits and uniformly across… | |||||
spibus_get_cs(dev, &cs); | |||||
cs &= ~SPIBUS_CS_HIGH; /* trim 'cs high' bit */ | |||||
sc = device_get_softc(dev); | sc = device_get_softc(dev); | ||||
sc->sc_dev = dev; | sc->sc_dev = dev; | ||||
sc->sc_cdev = make_dev(&spigen_cdevsw, unit, | |||||
UID_ROOT, GID_OPERATOR, 0660, "spigen%d", unit); | make_dev_args_init(&mda); | ||||
mda.mda_flags = 0; /* see make_dev in kern_conf.c */ | |||||
mda.mda_devsw = &spigen_cdevsw; | |||||
mda.mda_cr = NULL; /* see make_dev in kern_conf.c */ | |||||
Done Inline Actionsian 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: ian suggested (in IRC channel) to make an alias device using the original naming scheme, as… | |||||
Done Inline Actionsinitial 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: 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: initial test code works (using make_dev_s and make_dev_alias_p).
I noticed that the alias… | |||||
Done Inline ActionsThere 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: There is general agreement that the 'spigenN' naming scheme is inadequate, but it's present in… | |||||
Done Inline Actionswill 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) bobf_mrp3.com: will place #ifdef SPIGEN_LEGACY_CDEVNAME around the next section that creates the alias, and… | |||||
mda.mda_uid = UID_ROOT; | |||||
mda.mda_gid = GID_OPERATOR; | |||||
mda.mda_mode = 0660; | |||||
mda.mda_unit = unit; | |||||
// res = make_dev_s(&mda, &(sc->sc_cdev), "spigen%d", unit); | |||||
res = make_dev_s(&mda, &(sc->sc_cdev), "spigen%d.%d", | |||||
device_get_unit(device_get_parent(dev)), cs); | |||||
if (res) { | |||||
return res; | |||||
} | |||||
// res = make_dev_alias_p(0, &(sc->sc_adev), sc->sc_cdev, "spigen%d.%d", | |||||
// device_get_unit(device_get_parent(dev)), cs); | |||||
res = make_dev_alias_p(0, &(sc->sc_adev), sc->sc_cdev, "spigen%d", unit); | |||||
if (res) { | |||||
if (sc->sc_cdev) { /* TODO: should I just rely on 'detach' to do this? */ | |||||
destroy_dev(sc->sc_cdev); | |||||
sc->sc_cdev = NULL; | |||||
} | |||||
return res; | |||||
} | |||||
sc->sc_cdev->si_drv1 = dev; | sc->sc_cdev->si_drv1 = dev; | ||||
ianUnsubmitted Done Inline ActionsThis 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. ian: This needs to be done by setting mda_si_drv1 = dev; before calling make_dev_s(). Much of the… | |||||
sc->sc_command_length_max = PAGE_SIZE; | sc->sc_command_length_max = PAGE_SIZE; | ||||
sc->sc_data_length_max = PAGE_SIZE; | sc->sc_data_length_max = PAGE_SIZE; | ||||
mtx_init(&sc->sc_mtx, device_get_nameunit(dev), NULL, MTX_DEF); | mtx_init(&sc->sc_mtx, device_get_nameunit(dev), NULL, MTX_DEF); | ||||
ianUnsubmitted Done Inline Actionsalso 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. ian: also move these 3 lines to before setting up the cdev, on the same theory... don't make… | |||||
spigen_sysctl_init(sc); | spigen_sysctl_init(sc); | ||||
Context not available. | |||||
} | } | ||||
mtx_unlock(&sc->sc_mtx); | mtx_unlock(&sc->sc_mtx); | ||||
mtx_destroy(&sc->sc_mtx); | mtx_destroy(&sc->sc_mtx); | ||||
if (sc->sc_cdev) | if (sc->sc_adev) | ||||
destroy_dev(sc->sc_cdev); | destroy_dev(sc->sc_adev); | ||||
if (sc->sc_cdev) | |||||
destroy_dev(sc->sc_cdev); | |||||
return (0); | return (0); | ||||
} | } | ||||
static devclass_t spigen_devclass; | static devclass_t spigen_devclass; | ||||
Context not available. |
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?