Subject: Re: [PATCH v10 3/3] fpga: Add support for Lattice iCE40 FPGAs
To: Joel Holdsworth <joel@airwebreathe.org.uk>,
        <atull@opensource.altera.com>, <moritz.fischer@ettus.com>,
        <robh@kernel.org>, <devicetree@vger.kernel.org>,
        <linux-kernel@vger.kernel.org>, <linux-spi@vger.kernel.org>,
        <marex@denx.de>, <linux-fpga@vger.kernel.org>
References: <1482039234-21335-1-git-send-email-joel@airwebreathe.org.uk>
 <1482039234-21335-3-git-send-email-joel@airwebreathe.org.uk>
From: Vladimir Zapolskiy <vladimir_zapolskiy@mentor.com>
Message-ID: <2e32e0b2-6671-8ba3-39ae-23cd0388c654@mentor.com>
Date: Sun, 18 Dec 2016 21:18:02 +0200
User-Agent: Mozilla/5.0 (X11; Linux i686; rv:45.0) Gecko/20100101
 Icedove/45.2.0
MIME-Version: 1.0
In-Reply-To: <1482039234-21335-3-git-send-email-joel@airwebreathe.org.uk>
Content-Type: text/plain; charset="windows-1252"
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 2655
Lines: 103

Hi Joel,

On 12/18/2016 07:33 AM, Joel Holdsworth wrote:
> This patch adds support to the FPGA manager for configuring the SRAM of
> iCE40LM, iCE40LP, iCE40HX, iCE40 Ultra, iCE40 UltraLite and iCE40
> UltraPlus devices, through slave SPI.
> 
> Signed-off-by: Joel Holdsworth <joel@airwebreathe.org.uk>
> Reviewed-by: Marek Vasut <marex@denx.de>
> Reviewed-by: Moritz Fischer <moritz.fischer@ettus.com>
> Acked-by: Alan Tull <atull@opensource.altera.com>
> ---

[snip]

> +
> +static int ice40_fpga_probe(struct spi_device *spi)
> +{
> +	struct device *dev = &spi->dev;
> +	struct device_node *np = spi->dev.of_node;

struct device_node *np = dev->of_node?

And actually I don't see why local variable 'np' is needed in the function,
see the next comment.

> +	struct ice40_fpga_priv *priv;
> +	int ret;
> +
> +	if (!np) {
> +		dev_err(dev, "No Device Tree entry\n");
> +		return -EINVAL;
> +	}

I would suggest to remove this check completely, I don't see a good
reason why it is needed.

> +
> +	priv = devm_kzalloc(&spi->dev, sizeof(*priv), GFP_KERNEL);
> +	if (!priv)
> +		return -ENOMEM;
> +
> +	priv->dev = spi;
> +
> +	/* Check board setup data. */
> +	if (spi->max_speed_hz > ICE40_SPI_MAX_SPEED) {
> +		dev_err(dev, "SPI speed is too high, maximum speed is "
> +			__stringify(ICE40_SPI_MAX_SPEED) "\n");
> +		return -EINVAL;
> +	}
> +
> +	if (spi->max_speed_hz < ICE40_SPI_MIN_SPEED) {
> +		dev_err(dev, "SPI speed is too low, minimum speed is "
> +			__stringify(ICE40_SPI_MIN_SPEED) "\n");
> +		return -EINVAL;
> +	}
> +
> +	if (spi->mode & SPI_CPHA) {
> +		dev_err(dev, "Bad SPI mode, CPHA not supported\n");
> +		return -EINVAL;
> +	}
> +
> +	/* Set up the GPIOs */
> +	priv->cdone = devm_gpiod_get(dev, "cdone", GPIOD_IN);
> +	if (IS_ERR(priv->cdone)) {
> +		dev_err(dev, "Failed to get CDONE GPIO: %ld\n",
> +			PTR_ERR(priv->cdone));
> +		return -EINVAL;

You should do 'return PTR_ERR(priv->cdone)' here, this will allow
to handle at lease -EPROBE_DEFER correctly.

> +	}
> +
> +	priv->reset = devm_gpiod_get(dev, "reset", GPIOD_OUT_HIGH);
> +	if (IS_ERR(priv->reset)) {
> +		dev_err(dev, "Failed to get CRESET_B GPIO: %ld\n",
> +			PTR_ERR(priv->reset));
> +		return -EINVAL;

Same as above.

> +	}
> +
> +	/* Register with the FPGA manager */
> +	ret = fpga_mgr_register(dev, "Lattice iCE40 FPGA Manager",
> +				&ice40_fpga_ops, priv);
> +	if (ret) {
> +		dev_err(dev, "Unable to register FPGA manager");
> +		return ret;

I would suggest to do just 'return fpga_mgr_register(dev, ....);' here,
if the driver is not registered, the core will let the user know about it.

> +	}
> +
> +	return 0;
> +}
> +

--
With best wishes,
Vladimir