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

[Xu Yilun]

> 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.

...

> +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.

> +	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.