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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <a755902d-f2ef-126a-c7aa-d75b264fa076@pengutronix.de>
Date:   Tue, 11 Oct 2022 09:29:55 +0200
From:   Ahmad Fatoum <a.fatoum@...gutronix.de>
To:     Ivan Bornyakov <i.bornyakov@...rotek.ru>, mdf@...nel.org,
        hao.wu@...el.com, yilun.xu@...el.com, trix@...hat.com,
        dg@...ix.com, j.zink@...gutronix.de, robh+dt@...nel.org,
        krzysztof.kozlowski+dt@...aro.org
Cc:     devicetree@...r.kernel.org, system@...rotek.ru,
        linux-fpga@...r.kernel.org, linux-kernel@...r.kernel.org,
        kernel@...gutronix.de
Subject: Re: [PATCH v16 1/2] fpga: lattice-sysconfig-spi: add Lattice
 sysCONFIG FPGA manager

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@...rotek.ru>

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 |