| Message ID | 20241217114226.1223724-2-u.kleine-koenig@baylibre.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | e490ceff433fbc111404ce040bd9e8f41f12af16 |
| Headers | show |
| Series | spi: spidev: Align ordering of spidev_spi_ids[] and spidev_dt_ids[] | expand |
On Tue, 17 Dec 2024 12:42:25 +0100, Uwe Kleine-König wrote: > There is a 1:1 correspondance between the list of spi device-ids and the > devicetree compatibles. The latter is ordered alphabetically by vendor > and device. To simplify keeping the two lists in sync, mention the > vendor in a comment for the spi device-ids and order alphabetically, > too. > > > [...] Applied to https://git.kernel.org/pub/scm/linux/kernel/git/broonie/spi.git for-next Thanks! [1/1] spi: spidev: Align ordering of spidev_spi_ids[] and spidev_dt_ids[] commit: e490ceff433fbc111404ce040bd9e8f41f12af16 All being well this means that it will be integrated into the linux-next tree (usually sometime in the next 24 hours) and sent to Linus during the next merge window (or sooner if it is a bug fix), however if problems are discovered then the patch may be dropped or reverted. You may get further e-mails resulting from automated or manual testing and review of the tree, please engage with people reporting problems and send followup patches addressing any issues that are reported if needed. If any updates are required or you are submitting further changes they should be sent as incremental updates against current git, existing patches will not be replaced. Please add any relevant lists and maintainers to the CCs when replying to this mail. Thanks, Mark
Hi Uwe, On Tue, Dec 17, 2024 at 12:42 PM Uwe Kleine-König <u.kleine-koenig@baylibre.com> wrote: > There is a 1:1 correspondance between the list of spi device-ids and the > devicetree compatibles. The latter is ordered alphabetically by vendor > and device. To simplify keeping the two lists in sync, mention the > vendor in a comment for the spi device-ids and order alphabetically, > too. > > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@baylibre.com> Thanks for your patch! > --- a/drivers/spi/spidev.c > +++ b/drivers/spi/spidev.c > @@ -698,20 +698,24 @@ static const struct class spidev_class = { > .name = "spidev", > }; > > +/* > + * The spi device ids are expected to match the device names of the > + * spidev_dt_ids array below. Both arrays are kept in the same ordering. > + */ > static const struct spi_device_id spidev_spi_ids[] = { > - { .name = "bh2228fv" }, > - { .name = "dh2228fv" }, > - { .name = "jg10309-01" }, > - { .name = "ltc2488" }, > - { .name = "sx1301" }, > - { .name = "bk4" }, > - { .name = "bk4-spi" }, > - { .name = "dhcom-board" }, > - { .name = "m53cpld" }, > - { .name = "spi-petra" }, > - { .name = "spi-authenta" }, > - { .name = "em3581" }, > - { .name = "si3210" }, > + { .name = /* cisco */ "spi-petra" }, Pity we can't use { .name = strchr("cisco,spi-petra", ',') + 1 }, else we could do some macros on top to keep the tables in sync... Gr{oetje,eeting}s, Geert
On Wed, Dec 18, 2024 at 11:32:16AM +0100, Geert Uytterhoeven wrote: > Hi Uwe, > > On Tue, Dec 17, 2024 at 12:42 PM Uwe Kleine-König > <u.kleine-koenig@baylibre.com> wrote: > > There is a 1:1 correspondance between the list of spi device-ids and the > > devicetree compatibles. The latter is ordered alphabetically by vendor > > and device. To simplify keeping the two lists in sync, mention the > > vendor in a comment for the spi device-ids and order alphabetically, > > too. > > > > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@baylibre.com> > > Thanks for your patch! > > > --- a/drivers/spi/spidev.c > > +++ b/drivers/spi/spidev.c > > @@ -698,20 +698,24 @@ static const struct class spidev_class = { > > .name = "spidev", > > }; > > > > +/* > > + * The spi device ids are expected to match the device names of the > > + * spidev_dt_ids array below. Both arrays are kept in the same ordering. > > + */ > > static const struct spi_device_id spidev_spi_ids[] = { > > - { .name = "bh2228fv" }, > > - { .name = "dh2228fv" }, > > - { .name = "jg10309-01" }, > > - { .name = "ltc2488" }, > > - { .name = "sx1301" }, > > - { .name = "bk4" }, > > - { .name = "bk4-spi" }, > > - { .name = "dhcom-board" }, > > - { .name = "m53cpld" }, > > - { .name = "spi-petra" }, > > - { .name = "spi-authenta" }, > > - { .name = "em3581" }, > > - { .name = "si3210" }, > > + { .name = /* cisco */ "spi-petra" }, > > Pity we can't use > > { .name = strchr("cisco,spi-petra", ',') + 1 }, My suggestion is nice enough IMHO. > else we could do some macros on top to keep the tables in sync... I thought about that already before sending this patch. The best one I came up with is: static const struct spi_device_id spidev_spi_ids[] = { #define DEVICE(vendor, devname) { .name = devname }, #include "spidevices.c" #undef DEVICE {} }; MODULE_DEVICE_TABLE(spi, spidev_spi_ids); static const struct of_device_id spidev_dt_ids[] = { #define DEVICE(vendor, devname) { .compatible = vendor "," devname, .data = &spidev_of_check }, #include "spidevices.c" #undef DEVICE {} }; MODULE_DEVICE_TABLE(of, spidev_dt_ids); where spidevices.c looks as follows: DEVICE("cisco", "spi-petra") DEVICE("dh", "dhcom-board") DEVICE("elgin", "jg10309-01") DEVICE("lineartechnology", "ltc2488") DEVICE("lwn", "bk4") DEVICE("menlo", "m53cpld") DEVICE("micron", "spi-authenta") DEVICE("rohm", "bh2228fv") DEVICE("rohm", "dh2228fv") DEVICE("semtech", "sx1301") DEVICE("silabs", "em3581") DEVICE("silabs", "si3210") I didn't like that enough to propose it. Also I didn't test, but I think this could work. Best regards Uwe
Hi Uwe, On Wed, Dec 18, 2024 at 6:17 PM Uwe Kleine-König <u.kleine-koenig@baylibre.com> wrote: > On Wed, Dec 18, 2024 at 11:32:16AM +0100, Geert Uytterhoeven wrote: > > On Tue, Dec 17, 2024 at 12:42 PM Uwe Kleine-König > > <u.kleine-koenig@baylibre.com> wrote: > > > There is a 1:1 correspondance between the list of spi device-ids and the > > > devicetree compatibles. The latter is ordered alphabetically by vendor > > > and device. To simplify keeping the two lists in sync, mention the > > > vendor in a comment for the spi device-ids and order alphabetically, > > > too. > > > > > > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@baylibre.com> > > > > Thanks for your patch! > > > > > --- a/drivers/spi/spidev.c > > > +++ b/drivers/spi/spidev.c > > > @@ -698,20 +698,24 @@ static const struct class spidev_class = { > > > .name = "spidev", > > > }; > > > > > > +/* > > > + * The spi device ids are expected to match the device names of the > > > + * spidev_dt_ids array below. Both arrays are kept in the same ordering. > > > + */ > > > static const struct spi_device_id spidev_spi_ids[] = { > > > - { .name = "bh2228fv" }, > > > - { .name = "dh2228fv" }, > > > - { .name = "jg10309-01" }, > > > - { .name = "ltc2488" }, > > > - { .name = "sx1301" }, > > > - { .name = "bk4" }, > > > - { .name = "bk4-spi" }, > > > - { .name = "dhcom-board" }, > > > - { .name = "m53cpld" }, > > > - { .name = "spi-petra" }, > > > - { .name = "spi-authenta" }, > > > - { .name = "em3581" }, > > > - { .name = "si3210" }, > > > + { .name = /* cisco */ "spi-petra" }, > > > > Pity we can't use > > > > { .name = strchr("cisco,spi-petra", ',') + 1 }, > > My suggestion is nice enough IMHO. Sure. > > else we could do some macros on top to keep the tables in sync... > > I thought about that already before sending this patch. The best one I > came up with is: > > static const struct spi_device_id spidev_spi_ids[] = { > #define DEVICE(vendor, devname) { .name = devname }, > #include "spidevices.c" > #undef DEVICE > {} > }; > MODULE_DEVICE_TABLE(spi, spidev_spi_ids); > > static const struct of_device_id spidev_dt_ids[] = { > #define DEVICE(vendor, devname) { .compatible = vendor "," devname, .data = &spidev_of_check }, > #include "spidevices.c" > #undef DEVICE > {} > }; > MODULE_DEVICE_TABLE(of, spidev_dt_ids); > > where spidevices.c looks as follows: > > DEVICE("cisco", "spi-petra") > DEVICE("dh", "dhcom-board") > DEVICE("elgin", "jg10309-01") > DEVICE("lineartechnology", "ltc2488") > DEVICE("lwn", "bk4") > DEVICE("menlo", "m53cpld") > DEVICE("micron", "spi-authenta") > DEVICE("rohm", "bh2228fv") > DEVICE("rohm", "dh2228fv") > DEVICE("semtech", "sx1301") > DEVICE("silabs", "em3581") > DEVICE("silabs", "si3210") > > I didn't like that enough to propose it. Indeed, splitting vendor and device names hurts grepability. Further improvements could be: - Generate spidev_spi_ids[] from spidev_dt_ids[] at runtime during module_init() (consumes cycles :-(, - Teach the subsystem matching code to strip the vendor prefix, to get rid of spidev_spi_ids[]. Gr{oetje,eeting}s, Geert
On Wed, Dec 18, 2024 at 8:02 PM Geert Uytterhoeven <geert@linux-m68k.org> wrote: > Further improvements could be: > - Generate spidev_spi_ids[] from spidev_dt_ids[] at runtime > during module_init() (consumes cycles :-(, > - Teach the subsystem matching code to strip the vendor prefix, > to get rid of spidev_spi_ids[]. Oops, and modutils, as the tables are used by userspace :-( Gr{oetje,eeting}s, Geert
Hello Geert, On Wed, Dec 18, 2024 at 08:10:57PM +0100, Geert Uytterhoeven wrote: > On Wed, Dec 18, 2024 at 8:02 PM Geert Uytterhoeven <geert@linux-m68k.org> wrote: > > Further improvements could be: > > - Generate spidev_spi_ids[] from spidev_dt_ids[] at runtime > > during module_init() (consumes cycles :-(, > > - Teach the subsystem matching code to strip the vendor prefix, > > to get rid of spidev_spi_ids[]. > > Oops, and modutils, as the tables are used by userspace :-( Then how about a build-time check comparing spi and of module info? Best regards Uwe
Hi Uwe, On Thu, Dec 19, 2024 at 9:02 AM Uwe Kleine-König <u.kleine-koenig@baylibre.com> wrote: > On Wed, Dec 18, 2024 at 08:10:57PM +0100, Geert Uytterhoeven wrote: > > On Wed, Dec 18, 2024 at 8:02 PM Geert Uytterhoeven <geert@linux-m68k.org> wrote: > > > Further improvements could be: > > > - Generate spidev_spi_ids[] from spidev_dt_ids[] at runtime > > > during module_init() (consumes cycles :-(, > > > - Teach the subsystem matching code to strip the vendor prefix, > > > to get rid of spidev_spi_ids[]. > > > > Oops, and modutils, as the tables are used by userspace :-( > > Then how about a build-time check comparing spi and of module info? That's a good and non-disruptive idea, also for i2c! Gr{oetje,eeting}s, Geert
diff --git a/drivers/spi/spidev.c b/drivers/spi/spidev.c index 2b87b9ae56c0..58ae4304fdab 100644 --- a/drivers/spi/spidev.c +++ b/drivers/spi/spidev.c @@ -698,20 +698,24 @@ static const struct class spidev_class = { .name = "spidev", }; +/* + * The spi device ids are expected to match the device names of the + * spidev_dt_ids array below. Both arrays are kept in the same ordering. + */ static const struct spi_device_id spidev_spi_ids[] = { - { .name = "bh2228fv" }, - { .name = "dh2228fv" }, - { .name = "jg10309-01" }, - { .name = "ltc2488" }, - { .name = "sx1301" }, - { .name = "bk4" }, - { .name = "bk4-spi" }, - { .name = "dhcom-board" }, - { .name = "m53cpld" }, - { .name = "spi-petra" }, - { .name = "spi-authenta" }, - { .name = "em3581" }, - { .name = "si3210" }, + { .name = /* cisco */ "spi-petra" }, + { .name = /* dh */ "dhcom-board" }, + { .name = /* elgin */ "jg10309-01" }, + { .name = /* lineartechnology */ "ltc2488" }, + { .name = /* lwn */ "bk4" }, + { .name = /* lwn */ "bk4-spi" }, + { .name = /* menlo */ "m53cpld" }, + { .name = /* micron */ "spi-authenta" }, + { .name = /* rohm */ "bh2228fv" }, + { .name = /* rohm */ "dh2228fv" }, + { .name = /* semtech */ "sx1301" }, + { .name = /* silabs */ "em3581" }, + { .name = /* silabs */ "si3210" }, {}, }; MODULE_DEVICE_TABLE(spi, spidev_spi_ids);
There is a 1:1 correspondance between the list of spi device-ids and the devicetree compatibles. The latter is ordered alphabetically by vendor and device. To simplify keeping the two lists in sync, mention the vendor in a comment for the spi device-ids and order alphabetically, too. Signed-off-by: Uwe Kleine-König <u.kleine-koenig@baylibre.com> --- drivers/spi/spidev.c | 30 +++++++++++++++++------------- 1 file changed, 17 insertions(+), 13 deletions(-) base-commit: fdb298fa865b0136f7be842e6c2e6310dede421a