Re: [PATCH] spi: spidev: Align ordering of spidev_spi_ids[] and spidev_dt_ids[]

[Geert Uytterhoeven <geert@xxxxxxxxxxxxxx>]

Hi Uwe,

On Wed, Dec 18, 2024 at 6:17 PM Uwe Kleine-König
<u.kleine-koenig@xxxxxxxxxxxx> 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@xxxxxxxxxxxx> 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@xxxxxxxxxxxx>
> >
> > 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

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@xxxxxxxxxxxxxx

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds