Re: [PATCH v6 3/3] fpga-mgr: Add Efinix SPI programming driver

[Ian Dannapel]

Hi,
thanks for the quick review.

On Tue, Apr 7, 2026 at 8:33 AM Xu Yilun <yilun.xu@xxxxxxxxxxxxxxx> wrote:
>
> > diff --git a/drivers/fpga/Makefile b/drivers/fpga/Makefile
> > index aeb89bb13517..21eb0ef1fc2e 100644
> > --- a/drivers/fpga/Makefile
> > +++ b/drivers/fpga/Makefile
> > @@ -24,6 +24,7 @@ obj-$(CONFIG_FPGA_MGR_VERSAL_FPGA)  += versal-fpga.o
> >  obj-$(CONFIG_FPGA_MGR_MICROCHIP_SPI) += microchip-spi.o
> >  obj-$(CONFIG_FPGA_MGR_LATTICE_SYSCONFIG)     += lattice-sysconfig.o
> >  obj-$(CONFIG_FPGA_MGR_LATTICE_SYSCONFIG_SPI) += lattice-sysconfig-spi.o
> > +obj-$(CONFIG_FPGA_MGR_EFINIX_SPI)    += efinix-spi.o
> >  obj-$(CONFIG_ALTERA_PR_IP_CORE)              += altera-pr-ip-core.o
> >  obj-$(CONFIG_ALTERA_PR_IP_CORE_PLAT) += altera-pr-ip-core-plat.o
>
> This is the tail of "FPGA Manager Drivers", move it here.
All right
>
> ...
>
> > +static int efinix_spi_write_init(struct fpga_manager *mgr,
> > +                              struct fpga_image_info *info,
> > +                              const char *buf, size_t count)
> > +{
> > +     struct device *dev = &mgr->dev;
>
> Why do you make this change? This is just one-time usage, and in some
> other functions you don't make the same change. Please delete it.
Will revert it
>
> > +     struct efinix_spi_conf *conf = mgr->priv;
> > +     struct spi_transfer assert_cs = {
> > +             .cs_change = 1,
> > +     };
> > +     struct spi_message message;
> > +     int ret;
> > +
> > +     if (info->flags & FPGA_MGR_PARTIAL_RECONFIG) {
> > +             dev_err(dev, "Partial reconfiguration not supported\n");
> > +             return -EOPNOTSUPP;
> > +     }
> > +
> > +     /*
> > +      * Efinix passive SPI configuration requires chip select to stay
> > +      * asserted from reset until the bitstream is fully clocked in.
> > +      * Lock the SPI bus so no other device can toggle CS between the
> > +      * reset pulse and the write/complete transfers.
> > +      */
> > +     spi_bus_lock(conf->spi->controller);
> > +     spi_message_init_with_transfers(&message, &assert_cs, 1);
> > +     ret = spi_sync_locked(conf->spi, &message);
> > +     if (ret) {
> > +             spi_bus_unlock(conf->spi->controller);
> > +             return ret;
> > +     }
> > +
> > +     /* Reset with CS asserted */
> > +     efinix_spi_reset(conf);
> > +
> > +     return 0;
> > +}
> > +
> > +static int efinix_spi_write(struct fpga_manager *mgr, const char *buf,
> > +                         size_t count)
> > +{
> > +     struct device *dev = &mgr->dev;
>
> ditto.
>
> > +     struct spi_transfer write_xfer = {
> > +             .tx_buf = buf,
> > +             .len = count,
> > +             .cs_change = 1, /* Keep CS asserted */
>
> Move this comment to its first appearance.
>
> ...
>
> > +static const struct of_device_id efinix_spi_of_match[] = {
> > +     { .compatible = "efinix,trion-config", },
> > +     {}
> > +};
> > +MODULE_DEVICE_TABLE(of, efinix_spi_of_match);
> > +
> > +static const struct spi_device_id efinix_ids[] = {
> > +     { "trion-config", 0 },
> > +     { "titanium-config", 0 },
> > +     { "topaz-config", 0 },
>
> Since you've trimmed of_match_table, any reason to keep 3
> spi_device_ids? IIUC you could keep them in sync.
I don't see any reason to have other IDs, will drop them.

I would also rename the file from efinix-spi.c to efinix-config.c to
match the dt bindings