Re: [PATCH v17 1/2] fpga: lattice-sysconfig-spi: add Lattice sysCONFIG FPGA manager

[Ivan Bornyakov]

On Fri, Oct 14, 2022 at 11:19:29PM +0800, Xu Yilun wrote:
> On 2022-10-11 at 22:38:20 +0300, Ivan Bornyakov wrote:
> > Add support to the FPGA manager for programming Lattice ECP5 FPGA over
> > slave SPI sysCONFIG interface.
> > 
> > sysCONFIG interface core functionality is separate from both ECP5 and
> > SPI specifics, so support for other FPGAs with different port types can
> > be added in the future.
> > 
> > Signed-off-by: Ivan Bornyakov <i.bornyakov@xxxxxxxxxxx>
> 
> [...]
> 
> > +
> > +static int sysconfig_read_busy(struct sysconfig_priv *priv)
> > +{
> > +	const u8 lsc_check_busy[] = SYSCONFIG_LSC_CHECK_BUSY;
> > +	u8 busy;
> > +	int ret;
> > +
> > +	ret = sysconfig_cmd_read(priv, lsc_check_busy, sizeof(lsc_check_busy),
> > +				 &busy, sizeof(busy));
> > +
> > +	return ret ? : busy;
> > +}
> > +
> > +static int sysconfig_poll_busy(struct sysconfig_priv *priv)
> > +{
> > +	unsigned long timeout;
> > +	int ret;
> > +
> > +	timeout = jiffies + msecs_to_jiffies(SYSCONFIG_POLL_BUSY_TIMEOUT_MS);
> > +
> > +	while (time_before(jiffies, timeout)) {
> > +		ret = sysconfig_read_busy(priv);
> > +		if (ret <= 0)
> > +			return ret;
> > +
> > +		usleep_range(SYSCONFIG_POLL_INTERVAL_US,
> > +			     SYSCONFIG_POLL_INTERVAL_US * 2);
> > +	}
> > +
> > +	return -ETIMEDOUT;
> 
> As mentioned by Ahmad, could read_poll_timeout() be used?
> 

Surely, it could. IMHO it's just easier to read this way...

> > +}
> > +
> > +static int sysconfig_read_status(struct sysconfig_priv *priv, u32 *status)
> > +{
> > +	const u8 lsc_read_status[] = SYSCONFIG_LSC_READ_STATUS;
> > +	__be32 device_status;
> > +	int ret;
> > +
> > +	ret = sysconfig_cmd_read(priv, lsc_read_status, sizeof(lsc_read_status),
> > +				 &device_status, sizeof(device_status));
> > +	if (ret)
> > +		return ret;
> > +
> > +	*status = be32_to_cpu(device_status);
> > +
> > +	return 0;
> > +}
> > +
> > +static int sysconfig_poll_status(struct sysconfig_priv *priv, u32 *status)
> > +{
> > +	int ret = sysconfig_poll_busy(priv);
> > +
> > +	if (ret)
> > +		return ret;
> > +
> > +	return sysconfig_read_status(priv, status);
> > +}
> > +
> > +static int sysconfig_poll_gpio(struct gpio_desc *gpio, bool is_active)
> > +{
> > +	unsigned long timeout;
> > +	int value;
> > +
> > +	timeout = jiffies + msecs_to_jiffies(SYSCONFIG_POLL_GPIO_TIMEOUT_MS);
> > +
> > +	while (time_before(jiffies, timeout)) {
> > +		value = gpiod_get_value(gpio);
> > +		if (value < 0)
> > +			return value;
> > +
> > +		if ((is_active && value) || (!is_active && !value))
> > +			return 0;
> > +
> > +		usleep_range(SYSCONFIG_POLL_INTERVAL_US,
> > +			     SYSCONFIG_POLL_INTERVAL_US * 2);
> > +	}
> > +
> > +	return -ETIMEDOUT;
> 
> Same.
> 
> [...]
> 
> > +int sysconfig_probe(struct sysconfig_priv *priv)
> > +{
> > +	struct gpio_desc *program, *init, *done;
> > +	struct device *dev = priv->dev;
> > +	struct fpga_manager *mgr;
> > +
> > +	if (!dev)
> > +		return -ENODEV;
> > +
> > +	if (!priv->bitstream_burst_write_init) {
> > +		dev_err(dev,
> > +			"Callback for preparation for bitstream burst write is not defined\n");
> > +		return -EOPNOTSUPP;
> 
> -EINVAL is better?
> 
> > +	}
> > +
> > +	if (!priv->bitstream_burst_write) {
> > +		dev_err(dev,
> > +			"Callback for bitstream burst write is not defined\n");
> > +		return -EOPNOTSUPP;
> > +	}
> > +
> > +	if (!priv->bitstream_burst_write_complete) {
> > +		dev_err(dev,
> > +			"Callback for finishing bitstream burst write is not defined\n");
> > +		return -EOPNOTSUPP;
> > +	}
> 
> command_transfer is optional?
> 
> And I think different err log for each missing callback is too trivial,
> maybe just say like "ops missing" if any mandatory callback is missing.
> 
> > +
> > +	program = devm_gpiod_get_optional(dev, "program", GPIOD_OUT_LOW);
> > +	if (IS_ERR(program))
> > +		return dev_err_probe(dev, PTR_ERR(program),
> > +				     "Failed to get PROGRAM GPIO\n");
> > +
> > +	init = devm_gpiod_get_optional(dev, "init", GPIOD_IN);
> > +	if (IS_ERR(init))
> > +		return dev_err_probe(dev, PTR_ERR(init),
> > +				     "Failed to get INIT GPIO\n");
> > +
> > +	done = devm_gpiod_get_optional(dev, "done", GPIOD_IN);
> > +	if (IS_ERR(done))
> > +		return dev_err_probe(dev, PTR_ERR(done),
> > +				     "Failed to get DONE GPIO\n");
> > +
> > +	priv->program = program;
> > +	priv->init = init;
> > +	priv->done = done;
> > +
> > +	mgr = devm_fpga_mgr_register(dev, "Lattice sysCONFIG FPGA Manager",
> > +				     &sysconfig_fpga_mgr_ops, priv);
> > +
> > +	return PTR_ERR_OR_ZERO(mgr);
> > +}
> > +EXPORT_SYMBOL(sysconfig_probe);