Hello Ivan,
On 10.10.22 19:27, 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>
I found a small issue with the probe function, see below. While at it,
I noted two nitpicks you could address.
> +static int sysconfig_spi_bitstream_burst_init(struct sysconfig_priv *priv)
> +{
> + const u8 lsc_bitstream_burst[] = SYSCONFIG_LSC_BITSTREAM_BURST;
> + struct spi_device *spi = to_spi_device(priv->dev);
> + struct spi_transfer xfer = { 0 };
Nitpick: You want to zero all members. Using {} makes your
intention clearer even if they are functionally equivalent.
> +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);
> + }
Nitpick: I believe you could rewrite that using read_poll_timeout().
> +int sysconfig_probe(struct sysconfig_priv *priv)
[snip]
> + program = devm_gpiod_get_optional(dev, "program", GPIOD_OUT_LOW);
> + if (IS_ERR(program)) {
> + ret = PTR_ERR(program);
> + dev_err(dev, "Failed to get PROGRAM GPIO: %d\n", ret);
> + return ret;
This would print an error message for -EPROBE_DEFER, which just confuses users.
Please use dev_err_probe instead here and elsewhere in the probe function
to avoid this.
Cheers,
Ahmad
--
Pengutronix e.K. | |
Steuerwalder Str. 21 | http://www.pengutronix.de/ |
31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
