linux-kernel - Re: [PATCH v3] spi/pl022: add devicetree support

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <20120701083923.GA29380@avionic-0098.mockup.avionic-design.de>
Date:	Sun, 1 Jul 2012 10:39:24 +0200
From:	Thierry Reding <thierry.reding@...onic-design.de>
To:	Alexandre Pereira da Silva <aletes.xgr@...il.com>
Cc:	Roland Stigge <stigge@...com.de>, linux-doc@...r.kernel.org,
	devicetree-discuss@...ts.ozlabs.org, linux-kernel@...r.kernel.org,
	Rob Herring <rob.herring@...xeda.com>,
	Rob Landley <rob@...dley.net>,
	spi-devel-general@...ts.sourceforge.net
Subject: Re: [PATCH v3] spi/pl022: add devicetree support

On Tue, Jun 26, 2012 at 11:05:36AM -0300, Alexandre Pereira da Silva wrote:
> Add the chipselect array and cur_cs properties to pl022 main structure
> 
> Add a wrapper function to decide if the cs should be controlled by the
> cs_control callback or the chipselect gpio
> 
> Populate chipselect property from cs-gpios
> 
> Populate master->dev.of_node, so the spi bus can find child spi
> devices and register them
> 
> At pl022 setup, fill chip_data structure from dt nodes, if not provided
> by platform.
> 
> Signed-off-by: Alexandre Pereira da Silva <aletes.xgr@...il.com>
> Tested-by: Roland Stigge <stigge@...com.de>
> ---
> Applies to v3.5-rc4
> 
> Changes since v2:
> * Use IS_ENABLED instead of #ifdef
> * Remove bogus const change
> 
> Changes since v1:
> * return EPROBE_DEFFER if gpios are not initialized yet
> 
> Thanks Thierry Reding for reviewing and improvements suggestions
> 
>  .../devicetree/bindings/spi/spi_pl022.txt          |   15 +++
>  drivers/spi/spi-pl022.c                            |  115 ++++++++++++++++----
>  2 files changed, 111 insertions(+), 19 deletions(-)
> 
[...]
> diff --git a/drivers/spi/spi-pl022.c b/drivers/spi/spi-pl022.c
[...]
> @@ -1998,8 +2039,14 @@ pl022_probe(struct amba_device *adev, const struct amba_id *id)
>  		goto err_no_pdata;
>  	}
>  
> +	num_cs = platform_info->num_chipselect;
> +#if IS_ENABLED(CONFIG_OF)
> +	of_property_read_u32(np, "pl022,num-chipselects", &num_cs);
> +#endif
> +

Actually this has even more advantages if you use it as a condition in
an if expression:

	if (IS_ENABLED(CONFIG_OF))
		of_property_read_u32(...);

The above will be compiled wether CONFIG_OF is enabled or not, but it
will not be included in the final output because of the compiler's DCE
(dead code elimination) seeing this as "if (0)". The result is that the
source code gets much better compile coverage and errors in the
conditional code can be caught earlier.

> @@ -2017,13 +2064,40 @@ pl022_probe(struct amba_device *adev, const struct amba_id *id)
>  	 * on this board
>  	 */
>  	master->bus_num = platform_info->bus_id;
> -	master->num_chipselect = platform_info->num_chipselect;
> +	master->num_chipselect = num_cs;
>  	master->cleanup = pl022_cleanup;
>  	master->setup = pl022_setup;
>  	master->prepare_transfer_hardware = pl022_prepare_transfer_hardware;
>  	master->transfer_one_message = pl022_transfer_one_message;
>  	master->unprepare_transfer_hardware = pl022_unprepare_transfer_hardware;
>  	master->rt = platform_info->rt;
> +	master->dev.of_node = dev->of_node;
> +
> +#if IS_ENABLED(CONFIG_OF)
> +	for (i = 0; i < num_cs; i++) {
> +		int cs_gpio = of_get_named_gpio(np, "cs-gpios", i);
> +
> +		if (cs_gpio == -EPROBE_DEFER) {
> +			status = -EPROBE_DEFER;
> +			goto err_no_gpio;
> +		}
> +
> +		pl022->chipselect[i] = cs_gpio;
> +
> +		if (gpio_is_valid(cs_gpio)) {
> +			if (gpio_request(cs_gpio, "ssp-pl022"))
> +				dev_err(&adev->dev,
> +					"could not request %d gpio\n", cs_gpio);
> +			else if (gpio_direction_output(cs_gpio, 1))
> +				dev_err(&adev->dev,
> +					"could set gpio %d as output\n",
> +					cs_gpio);
> +		}
> +	}
> +#else
> +	for (i = 0; i < num_cs; i++)
> +		pl022->chipselect[i] = -EINVAL;
> +#endif

Same here.

>  	/*
>  	 * Supports mode 0-3, loopback, and active low CS. Transfers are
> @@ -2130,6 +2204,9 @@ pl022_probe(struct amba_device *adev, const struct amba_id *id)
>   err_no_ioremap:
>  	amba_release_regions(adev);
>   err_no_ioregion:
> + #if IS_ENABLED(CONFIG_OF)
> + err_no_gpio:
> + #endif

And here.

Thierry

Content of type "application/pgp-signature" skipped