Re: [PATCH RESEND v4] spi/pl022: add devicetree support
From: Linus Walleij <hidden>
Date: 2012-08-20 15:02:50
Also in:
linux-spi, lkml
(Currently make sure to include Mark Brown (see To: line) on all SPI patches, as he's collecting them.) On Mon, Aug 20, 2012 at 1:39 PM, Alexandre Pereira da Silva [off-list ref] wrote:
Add the chipselect array and cur_cs properties to pl022 main structure
OK...
quoted hunk ↗ jump to hunk
diff --git a/Documentation/devicetree/bindings/spi/spi_pl022.txt b/Documentation/devicetree/bindings/spi/spi_pl022.txt
This looks fine to me.
+#include <linux/gpio.h> +#include <linux/of_gpio.h>
Hm, it looks like two changes are embedded in this patch.
quoted hunk ↗ jump to hunk
/* * This macro is used to define some register default values.@@ -389,6 +391,8 @@ struct pl022 { char *dummypage; bool dma_running; #endif + int cur_cs; + int chipselect[0]; };
You forgot to add kerneldoc for these two. Please add! int chipselect[0] really? isn't int *chipselect what you really want to store in there?
+ if (gpio_is_valid(pl022->cur_cs)) + gpio_set_value(pl022->cur_cs, command); + else + pl022->cur_chip->cs_control(command); +}
So it seems like this should be two patches: - One adding pl022->cur_cs and the ability for the driver to control the chip select directly from msg->spi->chip_select Make sure it is possible to populate pl022->chipselect also from platform data since many people are using that still. - Another patch adding device tree and that stuff. So please split it in two.
- pl022->cur_chip->cs_control(SSP_CHIP_DESELECT); + pl022_cs_control(pl022, SSP_CHIP_DESELECT);
All these are OK and go in the first patch.
+ pl022->cur_cs = pl022->chipselect[msg->spi->chip_select];
Goes into the first patch...
quoted hunk ↗ jump to hunk
@@ -1761,12 +1772,14 @@ static const struct pl022_config_chip pl022_default_chip_info = { static int pl022_setup(struct spi_device *spi) { struct pl022_config_chip const *chip_info; + struct pl022_config_chip chip_info_dt;
Goes into second patch.
struct chip_data *chip;
struct ssp_clock_params clk_freq = { .cpsdvsr = 0, .scr = 0};
int status = 0;
struct pl022 *pl022 = spi_master_get_devdata(spi->master);
unsigned int bits = spi->bits_per_word;
u32 tmp;
+ struct device_node *np = spi->dev.of_node;Does this compile if CONFIG_OF is not selected? I have seen other drivers have #ifdef CONFIG_OF around these things. Make sure you test-compile without DT.
quoted hunk ↗ jump to hunk
@@ -1789,10 +1802,36 @@ static int pl022_setup(struct spi_device *spi) chip_info = spi->controller_data; if (chip_info == NULL) { - chip_info = &pl022_default_chip_info; - /* spi_board_info.controller_data not is supplied */ - dev_dbg(&spi->dev, - "using default controller_data settings\n"); + if (np) { + chip_info_dt = pl022_default_chip_info; + + of_property_read_u32(np, "pl022,hierarchy", + &chip_info_dt.hierarchy); + of_property_read_u32(np, "pl022,interface", + &chip_info_dt.iface); + chip_info_dt.slave_tx_disable = + of_property_read_bool(np, + "pl022,slave-tx-disable"); + of_property_read_u32(np, "pl022,com-mode", + &chip_info_dt.com_mode); + of_property_read_u32(np, "pl022,rx-level-trig", + &chip_info_dt.rx_lev_trig); + of_property_read_u32(np, "pl022,tx-level-trig", + &chip_info_dt.tx_lev_trig); + of_property_read_u32(np, "pl022,ctrl-len", + &chip_info_dt.ctrl_len); + of_property_read_u32(np, "pl022,wait-state", + &chip_info_dt.wait_state); + of_property_read_u32(np, "pl022,duplex", + &chip_info_dt.duplex); + + chip_info = &chip_info_dt; + } else { + chip_info = &pl022_default_chip_info; + /* spi_board_info.controller_data not is supplied */ + dev_dbg(&spi->dev, + "using default controller_data settings\n"); + }
Goes into second patch.
quoted hunk ↗ jump to hunk
@@ -1835,8 +1874,9 @@ static int pl022_setup(struct spi_device *spi) chip->xfer_type = chip_info->com_mode; if (!chip_info->cs_control) { chip->cs_control = null_cs_control; - dev_warn(&spi->dev, - "chip select function is NULL for this chip\n"); + if (!gpio_is_valid(pl022->chipselect[spi->chip_select])) + dev_warn(&spi->dev, + "chip select function is NULL for this chip\n"); } else chip->cs_control = chip_info->cs_control;
Goes into first patch.
quoted hunk ↗ jump to hunk
@@ -1988,7 +2028,8 @@ pl022_probe(struct amba_device *adev, const struct amba_id *id) struct pl022_ssp_controller *platform_info = adev->dev.platform_data; struct spi_master *master; struct pl022 *pl022 = NULL; /*Data for this driver */ - int status = 0; + struct device_node *np = adev->dev.of_node;
Does this compile without CONFIG_OF? (Second patch)
quoted hunk ↗ jump to hunk
+ int status = 0, i, num_cs; dev_info(&adev->dev, "ARM PL022 driver, device ID: 0x%08x\n", adev->periphid);@@ -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); + +
Shouldn't it be the other way around: platform data overrides DT data. Attempt DT and if platform_info->num_chipselect != 0 let it override. (Second patch.)
/* Allocate master with space for data */ - master = spi_alloc_master(dev, sizeof(struct pl022)); + master = spi_alloc_master(dev, + sizeof(struct pl022) + sizeof(int) * num_cs);
First patch.
quoted hunk ↗ jump to hunk
if (master == NULL) { dev_err(&adev->dev, "probe - cannot alloc SPI master\n"); status = -ENOMEM;@@ -2017,13 +2064,41 @@ 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;
OK first patch.
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;Does this compile without CONFIG_OF? Second patch.
+ 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;Why? Instead, add a int *chipselects; field to the struct pl022_ssp_controller platform data in include/linux/amba/pl022.h and copy it from there if num_chipselects in the same data != 0. (First patch.) Yours, Linus Walleij