Messages in this thread | | | Date | Fri, 14 Oct 2022 23:19:29 +0800 | From | Xu Yilun <> | Subject | Re: [PATCH v17 1/2] fpga: lattice-sysconfig-spi: add Lattice sysCONFIG FPGA manager |
| |
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@metrotek.ru>
[...]
> + > +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?
> +} > + > +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);
| |