Re: [PATCH 07/10] spi: spi-dw-mchp: Add Sparx5 support

From: Mark Brown <broonie@kernel.org>
To: Lars Povlsen <lars.povlsen@microchip.com>
Cc: devicetree@vger.kernel.org,
	Alexandre Belloni <alexandre.belloni@bootlin.com>,
	linux-kernel@vger.kernel.org, linux-spi@vger.kernel.org,
	SoC Team <soc@kernel.org>,
	Microchip Linux Driver Support <UNGLinuxDriver@microchip.com>,
	linux-arm-kernel@lists.infradead.org
Subject: Re: [PATCH 07/10] spi: spi-dw-mchp: Add Sparx5 support
Date: Thu, 14 May 2020 11:25:16 +0100	[thread overview]
Message-ID: <20200514102516.GD5127@sirena.org.uk> (raw)
In-Reply-To: <20200513140031.25633-8-lars.povlsen@microchip.com>

[-- Attachment #1.1: Type: text/plain, Size: 1425 bytes --]

On Wed, May 13, 2020 at 04:00:28PM +0200, Lars Povlsen wrote:

> +static void dw_spi_mchp_set_cs_owner(struct dw_spi_mchp *dwsmchp,
> +				     const struct dw_spi_mchp_props *props,
> +				     u8 cs, u8 owner)
>  {
> +	u8 dummy = (owner == MSCC_IF_SI_OWNER_SIBM ?
> +		    MSCC_IF_SI_OWNER_SIMC : MSCC_IF_SI_OWNER_SIBM);

Please write normal conditional statements to improve legibility.

> +static void dw_spi_mchp_set_cs(struct spi_device *spi, bool nEnable)
> +{
> +	bool enable = !nEnable;	/* This keeps changing in the API... */

No, it doesn't.  The API has not changed for more than a decade.

> +	} else if (props->ss_force_ena_off) {
> +		if (enable) {
> +			/* Ensure CS toggles, so start off all disabled */
> +			regmap_write(dwsmchp->syscon, props->ss_force_val_off,
> +				     ~0);

What's all this force_ena_off stuff about?  The controller should not be
making decisions about management of the chip select, this will break
users.

> +	if (pdev->dev.of_node) {
> +		int i;
> +
> +		for (i = 0; i < dws->num_cs; i++) {
> +			int cs_gpio = of_get_named_gpio(pdev->dev.of_node,
> +					"cs-gpios", i);
> +
> +			if (cs_gpio == -EPROBE_DEFER) {
> +				ret = cs_gpio;
> +				goto out;
> +			}
> +
> +			if (gpio_is_valid(cs_gpio)) {
> +				ret = devm_gpio_request(&pdev->dev, cs_gpio,
> +						dev_name(&pdev->dev));
> +				if (ret)
> +					goto out;

Set use_gpio_descriptors and let the core manage the GPIO.

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel