[RFC/DRAFT] SPI OF bindings, MMC-over-SPI, chip-selects and so on

[RFC/DRAFT] SPI OF bindings, MMC-over-SPI, chip-selects and so on

[Anton Vorontsov]

Hi all,

This is just a bait for further discussion of OF/SPI, chip-selects,
e.t.c.

I've converted the spi_mpc83xx to the OF driver (using Grant's
SPI_MASTER_OF work + some additions), and implemented MMC-over-SPI
bindings. This stuff extensively using GPIOs, and I think this will
work for the "bridged SPI" too, since the SPI bridge could be
represented as GPIO controller (inside the SPI controller node).

Thoughts?

-- 
Anton Vorontsov
email: cbouatmailru@gmail.com
irc://irc.freenode.net/bd2

[PATCH 1/4] [SPI] spi_mpc83xx: convert to the OF platform driver

[Anton Vorontsov]

This patch converts the driver to speak OF directly. FSL SPI controllers
do not use internal chip-select machines, so boards must use GPIOs for
these purposes.

Signed-off-by: Anton Vorontsov <avorontsov@ru.mvista.com>
---
 Documentation/powerpc/booting-without-of.txt |    1 +
 drivers/spi/Kconfig                          |    3 +-
 drivers/spi/spi_mpc83xx.c                    |  279 +++++++++++++++++---------
 3 files changed, 184 insertions(+), 99 deletions(-)

diff --git a/Documentation/powerpc/booting-without-of.txt b/Documentation/powerpc/booting-without-of.txt
index 8545f82..011bf5e 100644
--- a/Documentation/powerpc/booting-without-of.txt
+++ b/Documentation/powerpc/booting-without-of.txt
@@ -1607,6 +1607,7 @@ platforms are moved over to use the flattened-device-tree model.
      controller you have.
    - interrupt-parent : the phandle for the interrupt controller that
      services interrupts for this device.
+   - gpios : (optional) may specify GPIOs used for SPI chip-selects
 
    Example:
 	spi@4c0 {
diff --git a/drivers/spi/Kconfig b/drivers/spi/Kconfig
index 3f0dcae..4c9afeb 100644
--- a/drivers/spi/Kconfig
+++ b/drivers/spi/Kconfig
@@ -129,7 +129,8 @@ config SPI_MPC52xx_PSC
 
 config SPI_MPC83xx
 	tristate "Freescale MPC8xxx SPI controller"
-	depends on SPI_MASTER && FSL_SOC && EXPERIMENTAL
+	depends on SPI_MASTER && OF_GPIO && FSL_SOC && EXPERIMENTAL
+	select SPI_MASTER_OF
 	help
 	  This enables using the Freescale MPC8xxx SPI controllers in master
 	  mode.
diff --git a/drivers/spi/spi_mpc83xx.c b/drivers/spi/spi_mpc83xx.c
index 6832da6..7ae4096 100644
--- a/drivers/spi/spi_mpc83xx.c
+++ b/drivers/spi/spi_mpc83xx.c
@@ -21,11 +21,14 @@
 #include <linux/device.h>
 #include <linux/spi/spi.h>
 #include <linux/spi/spi_bitbang.h>
-#include <linux/platform_device.h>
-#include <linux/fsl_devices.h>
+#include <linux/spi/spi_of.h>
+#include <linux/of_platform.h>
+#include <linux/gpio.h>
+#include <linux/of_gpio.h>
 
 #include <asm/irq.h>
 #include <asm/io.h>
+#include <sysdev/fsl_soc.h>
 
 /* SPI Controller registers */
 struct mpc83xx_spi_reg {
@@ -66,6 +69,11 @@ struct mpc83xx_spi_reg {
 #define	SPIM_NE		0x00000200	/* Not empty */
 #define	SPIM_NF		0x00000100	/* Not full */
 
+enum spi83xx_mode {
+	CPU_MODE = 0,
+	CPU_QE_MODE,
+};
+
 /* SPI Controller driver's private data. */
 struct mpc83xx_spi {
 	struct mpc83xx_spi_reg __iomem *base;
@@ -80,6 +88,7 @@ struct mpc83xx_spi {
 
 	unsigned int count;
 	int irq;
+	unsigned int *gpios;
 
 	unsigned nsecs;		/* (clock cycle time)/2 */
 
@@ -87,10 +96,7 @@ struct mpc83xx_spi {
 	u32 rx_shift;		/* RX data reg shift when in qe mode */
 	u32 tx_shift;		/* TX data reg shift when in qe mode */
 
-	bool qe_mode;
-
-	void (*activate_cs) (u8 cs, u8 polarity);
-	void (*deactivate_cs) (u8 cs, u8 polarity);
+	enum spi83xx_mode mode;
 
 	u8 busy;
 
@@ -151,16 +157,13 @@ MPC83XX_SPI_TX_BUF(u32)
 
 static void mpc83xx_spi_chipselect(struct spi_device *spi, int value)
 {
-	struct mpc83xx_spi *mpc83xx_spi;
 	u8 pol = spi->mode & SPI_CS_HIGH ? 1 : 0;
+	struct mpc83xx_spi *mpc83xx_spi = spi_master_get_devdata(spi->master);
 	struct spi_mpc83xx_cs	*cs = spi->controller_state;
+	unsigned int cs_gpio = mpc83xx_spi->gpios[spi->chip_select];
 
-	mpc83xx_spi = spi_master_get_devdata(spi->master);
-
-	if (value == BITBANG_CS_INACTIVE) {
-		if (mpc83xx_spi->deactivate_cs)
-			mpc83xx_spi->deactivate_cs(spi->chip_select, pol);
-	}
+	if (value == BITBANG_CS_INACTIVE && gpio_is_valid(cs_gpio))
+		gpio_set_value(cs_gpio, !pol);
 
 	if (value == BITBANG_CS_ACTIVE) {
 		u32 regval = mpc83xx_spi_read_reg(&mpc83xx_spi->base->mode);
@@ -184,8 +187,8 @@ static void mpc83xx_spi_chipselect(struct spi_device *spi, int value)
 			mpc83xx_spi_write_reg(tmp_ptr, regval);
 			local_irq_restore(flags);
 		}
-		if (mpc83xx_spi->activate_cs)
-			mpc83xx_spi->activate_cs(spi->chip_select, pol);
+		if (gpio_is_valid(cs_gpio))
+			gpio_set_value(cs_gpio, pol);
 	}
 }
 
@@ -225,14 +228,14 @@ int mpc83xx_spi_setup_transfer(struct spi_device *spi, struct spi_transfer *t)
 	if (bits_per_word <= 8) {
 		cs->get_rx = mpc83xx_spi_rx_buf_u8;
 		cs->get_tx = mpc83xx_spi_tx_buf_u8;
-		if (mpc83xx_spi->qe_mode) {
+		if (mpc83xx_spi->mode == CPU_QE_MODE) {
 			cs->rx_shift = 16;
 			cs->tx_shift = 24;
 		}
 	} else if (bits_per_word <= 16) {
 		cs->get_rx = mpc83xx_spi_rx_buf_u16;
 		cs->get_tx = mpc83xx_spi_tx_buf_u16;
-		if (mpc83xx_spi->qe_mode) {
+		if (mpc83xx_spi->mode == CPU_QE_MODE) {
 			cs->rx_shift = 16;
 			cs->tx_shift = 16;
 		}
@@ -242,7 +245,7 @@ int mpc83xx_spi_setup_transfer(struct spi_device *spi, struct spi_transfer *t)
 	} else
 		return -EINVAL;
 
-	if (mpc83xx_spi->qe_mode && spi->mode & SPI_LSB_FIRST) {
+	if (mpc83xx_spi->mode == CPU_QE_MODE && spi->mode & SPI_LSB_FIRST) {
 		cs->tx_shift = 0;
 		if (bits_per_word <= 8)
 			cs->rx_shift = 8;
@@ -536,92 +539,152 @@ static void mpc83xx_spi_cleanup(struct spi_device *spi)
 	kfree(spi->controller_state);
 }
 
-static int __init mpc83xx_spi_probe(struct platform_device *dev)
+static unsigned int of_num_children(struct device_node *parent)
+{
+	unsigned int count = 0;
+	struct device_node *child = NULL;
+
+	for_each_child_of_node(parent, child)
+		count++;
+
+	return count;
+}
+
+static void __devinit mpc83xx_spi_hwinit(struct mpc83xx_spi *mpc83xx_spi)
 {
+	u32 regval;
+
+	/* SPI controller initializations */
+	mpc83xx_spi_write_reg(&mpc83xx_spi->base->mode, 0);
+	mpc83xx_spi_write_reg(&mpc83xx_spi->base->mask, 0);
+	mpc83xx_spi_write_reg(&mpc83xx_spi->base->command, 0);
+	mpc83xx_spi_write_reg(&mpc83xx_spi->base->event, 0xffffffff);
+
+	/* Enable SPI interface */
+	regval = SPMODE_INIT_VAL | SPMODE_ENABLE;
+	if (mpc83xx_spi->mode == CPU_QE_MODE)
+		regval |= SPMODE_OP;
+
+	mpc83xx_spi_write_reg(&mpc83xx_spi->base->mode, regval);
+}
+
+static int __devinit mpc83xx_spi_probe(struct of_device *ofdev,
+				       const struct of_device_id *match)
+{
+	struct device *dev = &ofdev->dev;
+	struct device_node *np = ofdev->node;
 	struct spi_master *master;
 	struct mpc83xx_spi *mpc83xx_spi;
-	struct fsl_spi_platform_data *pdata;
-	struct resource *r;
-	u32 regval;
 	int ret = 0;
+	int size;
+	const char *mode;
+	const u32 *bus_num;
+	int i = 0;
+
+	master = spi_alloc_master(dev, sizeof(struct mpc83xx_spi));
+	if (!master)
+		return -ENOMEM;
+	dev_set_drvdata(dev, master);
+	mpc83xx_spi = spi_master_get_devdata(master);
 
-	/* Get resources(memory, IRQ) associated with the device */
-	master = spi_alloc_master(&dev->dev, sizeof(struct mpc83xx_spi));
+	master->setup = mpc83xx_spi_setup;
+	master->transfer = mpc83xx_spi_transfer;
+	master->cleanup = mpc83xx_spi_cleanup;
+	mpc83xx_spi->get_rx = mpc83xx_spi_rx_buf_u8;
+	mpc83xx_spi->get_tx = mpc83xx_spi_tx_buf_u8;
 
-	if (master == NULL) {
+	bus_num = of_get_property(np, "reg", &size);
+	if (!bus_num || size < sizeof(*bus_num)) {
+		dev_err(dev, "unable to get reg property from OF\n");
+		ret = -EINVAL;
+		goto err_bus_num;
+	}
+	master->bus_num = *bus_num;
+
+	master->num_chipselect = of_num_children(np);
+	mpc83xx_spi->gpios = kmalloc(
+		sizeof(mpc83xx_spi->gpios[0]) * master->num_chipselect,
+		GFP_KERNEL);
+	if (!mpc83xx_spi->gpios) {
 		ret = -ENOMEM;
-		goto err;
+		goto err_alloc_gpios;
 	}
 
-	platform_set_drvdata(dev, master);
-	pdata = dev->dev.platform_data;
+	if (master->num_chipselect == 1) {
+		/* no retval check, CS isn't mandatory for single device */
+		mpc83xx_spi->gpios[0] = of_get_gpio(np, 0);
+		i++;
+	}
 
-	if (pdata == NULL) {
-		ret = -ENODEV;
-		goto free_master;
+	for (; i < master->num_chipselect; i++) {
+		mpc83xx_spi->gpios[i] = of_get_gpio(np, i);
+		if (!gpio_is_valid(mpc83xx_spi->gpios[i])) {
+			dev_err(dev, "chipselect %d without GPIO\n", i);
+			goto err_of_get_gpio;
+		}
 	}
 
-	r = platform_get_resource(dev, IORESOURCE_MEM, 0);
-	if (r == NULL) {
-		ret = -ENODEV;
-		goto free_master;
+	for (i = 0; i < master->num_chipselect; i++) {
+		int gpio = mpc83xx_spi->gpios[i];
+
+		if (!gpio_is_valid(gpio))
+			continue;
+
+		ret = gpio_request(gpio, dev->bus_id);
+		if (ret)
+			goto err_gpio_request;
+
+		ret = gpio_direction_output(gpio, 0);
+		if (ret)
+			goto err_gpio_dir;
 	}
-	master->setup = mpc83xx_spi_setup;
-	master->transfer = mpc83xx_spi_transfer;
-	master->cleanup = mpc83xx_spi_cleanup;
 
-	mpc83xx_spi = spi_master_get_devdata(master);
-	mpc83xx_spi->activate_cs = pdata->activate_cs;
-	mpc83xx_spi->deactivate_cs = pdata->deactivate_cs;
-	mpc83xx_spi->qe_mode = pdata->qe_mode;
-	mpc83xx_spi->get_rx = mpc83xx_spi_rx_buf_u8;
-	mpc83xx_spi->get_tx = mpc83xx_spi_tx_buf_u8;
-	mpc83xx_spi->spibrg = pdata->sysclk;
+#ifdef CONFIG_QUICC_ENGINE
+	mpc83xx_spi->spibrg = get_brgfreq();
+#endif
+	if (!mpc83xx_spi->spibrg || mpc83xx_spi->spibrg == -1) {
+		mpc83xx_spi->spibrg = fsl_get_sys_freq();
+		if (!mpc83xx_spi->spibrg || mpc83xx_spi->spibrg == -1) {
+			dev_err(dev, "unable to get frequency from OF\n");
+			goto err_sysclk;
+		}
+	}
+
+	mode = of_get_property(np, "mode", NULL);
+	if (mode && !strcmp(mode, "cpu-qe"))
+		mpc83xx_spi->mode = CPU_QE_MODE;
 
 	mpc83xx_spi->rx_shift = 0;
 	mpc83xx_spi->tx_shift = 0;
-	if (mpc83xx_spi->qe_mode) {
+	if (mpc83xx_spi->mode == CPU_QE_MODE) {
 		mpc83xx_spi->rx_shift = 16;
 		mpc83xx_spi->tx_shift = 24;
 	}
 
 	init_completion(&mpc83xx_spi->done);
 
-	mpc83xx_spi->base = ioremap(r->start, r->end - r->start + 1);
-	if (mpc83xx_spi->base == NULL) {
+	mpc83xx_spi->base = of_iomap(np, 0);
+	if (!mpc83xx_spi->base) {
+		dev_err(dev, "unable to get memory from OF\n");
 		ret = -ENOMEM;
-		goto put_master;
+		goto err_iomap;
 	}
 
-	mpc83xx_spi->irq = platform_get_irq(dev, 0);
-
-	if (mpc83xx_spi->irq < 0) {
+	mpc83xx_spi->irq = irq_of_parse_and_map(np, 0);
+	if (mpc83xx_spi->irq == NO_IRQ) {
+		dev_err(dev, "unable to get IRQ from OF\n");
 		ret = -ENXIO;
-		goto unmap_io;
+		goto err_irq_parse;
 	}
 
 	/* Register for SPI Interrupt */
 	ret = request_irq(mpc83xx_spi->irq, mpc83xx_spi_irq,
 			  0, "mpc83xx_spi", mpc83xx_spi);
+	if (ret != 0) {
+		dev_err(dev, "unable to request IRQ\n");
+		goto err_irq_request;
+	}
 
-	if (ret != 0)
-		goto unmap_io;
-
-	master->bus_num = pdata->bus_num;
-	master->num_chipselect = pdata->max_chipselect;
-
-	/* SPI controller initializations */
-	mpc83xx_spi_write_reg(&mpc83xx_spi->base->mode, 0);
-	mpc83xx_spi_write_reg(&mpc83xx_spi->base->mask, 0);
-	mpc83xx_spi_write_reg(&mpc83xx_spi->base->command, 0);
-	mpc83xx_spi_write_reg(&mpc83xx_spi->base->event, 0xffffffff);
-
-	/* Enable SPI interface */
-	regval = pdata->initial_spmode | SPMODE_INIT_VAL | SPMODE_ENABLE;
-	if (pdata->qe_mode)
-		regval |= SPMODE_OP;
-
-	mpc83xx_spi_write_reg(&mpc83xx_spi->base->mode, regval);
 	spin_lock_init(&mpc83xx_spi->lock);
 	init_completion(&mpc83xx_spi->done);
 	INIT_WORK(&mpc83xx_spi->work, mpc83xx_spi_work);
@@ -629,42 +692,53 @@ static int __init mpc83xx_spi_probe(struct platform_device *dev)
 
 	mpc83xx_spi->workqueue = create_singlethread_workqueue(
 		master->dev.parent->bus_id);
-	if (mpc83xx_spi->workqueue == NULL) {
+	if (!mpc83xx_spi->workqueue) {
 		ret = -EBUSY;
-		goto free_irq;
+		goto err_wqueue;
 	}
 
+	mpc83xx_spi_hwinit(mpc83xx_spi);
+
 	ret = spi_register_master(master);
 	if (ret < 0)
-		goto unreg_master;
+		goto err_spi_reg;
+
+	spi_of_register_devices(master, np);
 
 	printk(KERN_INFO
 	       "%s: MPC83xx SPI Controller driver at 0x%p (irq = %d)\n",
-	       dev->dev.bus_id, mpc83xx_spi->base, mpc83xx_spi->irq);
+	       dev->bus_id, mpc83xx_spi->base, mpc83xx_spi->irq);
 
 	return ret;
 
-unreg_master:
+err_spi_reg:
 	destroy_workqueue(mpc83xx_spi->workqueue);
-free_irq:
+err_wqueue:
 	free_irq(mpc83xx_spi->irq, mpc83xx_spi);
-unmap_io:
+err_irq_request:
+	irq_dispose_mapping(mpc83xx_spi->irq);
+err_irq_parse:
 	iounmap(mpc83xx_spi->base);
-put_master:
+err_iomap:
+err_sysclk:
+err_gpio_dir:
+	i++;
+err_gpio_request:
+	while (--i >= 0)
+		gpio_free(mpc83xx_spi->gpios[i]);
+err_of_get_gpio:
+	kfree(mpc83xx_spi->gpios);
+err_alloc_gpios:
+err_bus_num:
 	spi_master_put(master);
-free_master:
-	kfree(master);
-err:
 	return ret;
 }
 
-static int __exit mpc83xx_spi_remove(struct platform_device *dev)
+static int __devexit mpc83xx_spi_remove(struct of_device *ofdev)
 {
-	struct mpc83xx_spi *mpc83xx_spi;
-	struct spi_master *master;
-
-	master = platform_get_drvdata(dev);
-	mpc83xx_spi = spi_master_get_devdata(master);
+	struct spi_master *master = dev_get_drvdata(&ofdev->dev);
+	struct mpc83xx_spi *mpc83xx_spi = spi_master_get_devdata(master);
+	int i = master->num_chipselect;
 
 	flush_workqueue(mpc83xx_spi->workqueue);
 	destroy_workqueue(mpc83xx_spi->workqueue);
@@ -673,29 +747,38 @@ static int __exit mpc83xx_spi_remove(struct platform_device *dev)
 	free_irq(mpc83xx_spi->irq, mpc83xx_spi);
 	iounmap(mpc83xx_spi->base);
 
+	while (--i >= 0)
+		gpio_free(mpc83xx_spi->gpios[i]);
+
 	return 0;
 }
 
-MODULE_ALIAS("platform:mpc83xx_spi");
-static struct platform_driver mpc83xx_spi_driver = {
-	.remove = __exit_p(mpc83xx_spi_remove),
-	.driver = {
-		.name = "mpc83xx_spi",
-		.owner = THIS_MODULE,
+static const struct of_device_id mpc83xx_spi_match[] = {
+	{ .compatible = "fsl,spi", },
+	{},
+};
+MODULE_DEVICE_TABLE(of, mpc83xx_spi_match);
+
+static struct of_platform_driver mpc83xx_spi_driver = {
+	.match_table	= mpc83xx_spi_match,
+	.probe		= mpc83xx_spi_probe,
+	.remove		= __devexit_p(mpc83xx_spi_remove),
+	.driver		= {
+		.name	= "mpc83xx_spi",
+		.owner	= THIS_MODULE,
 	},
 };
 
 static int __init mpc83xx_spi_init(void)
 {
-	return platform_driver_probe(&mpc83xx_spi_driver, mpc83xx_spi_probe);
+	return of_register_platform_driver(&mpc83xx_spi_driver);
 }
+module_init(mpc83xx_spi_init);
 
 static void __exit mpc83xx_spi_exit(void)
 {
-	platform_driver_unregister(&mpc83xx_spi_driver);
+	of_unregister_platform_driver(&mpc83xx_spi_driver);
 }
-
-module_init(mpc83xx_spi_init);
 module_exit(mpc83xx_spi_exit);
 
 MODULE_AUTHOR("Kumar Gala");
-- 
1.5.5.1

Re: [PATCH 1/4] [SPI] spi_mpc83xx: convert to the OF platform driver

[Grant Likely]

On Wed, May 21, 2008 at 9:41 AM, Anton Vorontsov
<avorontsov@ru.mvista.com> wrote:
> This patch converts the driver to speak OF directly. FSL SPI controllers
> do not use internal chip-select machines, so boards must use GPIOs for
> these purposes.
>

Mostly this looks good to me, but I didn't look
> Signed-off-by: Anton Vorontsov <avorontsov@ru.mvista.com>
> ---
>  Documentation/powerpc/booting-without-of.txt |    1 +
>  drivers/spi/Kconfig                          |    3 +-
>  drivers/spi/spi_mpc83xx.c                    |  279 +++++++++++++++++---------
>  3 files changed, 184 insertions(+), 99 deletions(-)
>
> @@ -536,92 +539,152 @@ static void mpc83xx_spi_cleanup(struct spi_device *spi)
>        kfree(spi->controller_state);
>  }
>
> -static int __init mpc83xx_spi_probe(struct platform_device *dev)
> +static unsigned int of_num_children(struct device_node *parent)
> +{
> +       unsigned int count = 0;
> +       struct device_node *child = NULL;
> +
> +       for_each_child_of_node(parent, child)
> +               count++;
> +
> +       return count;
> +}

This smells like it should be in common of code; but I don't think
this routine is needed at all (see below)

[...]
> -       if (master == NULL) {
> +       bus_num = of_get_property(np, "reg", &size);
> +       if (!bus_num || size < sizeof(*bus_num)) {
> +               dev_err(dev, "unable to get reg property from OF\n");
> +               ret = -EINVAL;
> +               goto err_bus_num;
> +       }
> +       master->bus_num = *bus_num;

Does the driver really need to define its own bus num?  I know your
using it for binding with a board info structure, but I think there
are better ways to do it (I'll elaborate in my reply to your patch
that adds support for boardinfo structures).  It's better to let the
SPI infrastructure assign an SPI bus number.  and use other methods to
ensure that extra data is provided to the driver.  Besides, there is
no guarantee that 'reg' will be unique.

> +
> +       master->num_chipselect = of_num_children(np);

This assumes that there are no gaps in the assigned CS numbers of
child nodes, or that the child nodes are an exhaustive list of
attached devices, neither of which may be true.  num_chipselect should
be calculated from the number of GPIOs specified instead.

Cheers,
g.

-- 
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.

Re: [PATCH 1/4] [SPI] spi_mpc83xx: convert to the OF platform driver

[Anton Vorontsov]

On Wed, May 21, 2008 at 10:50:02AM -0600, Grant Likely wrote:
[..]
> > +
> > +       master->num_chipselect = of_num_children(np);
> 
> This assumes that there are no gaps in the assigned CS numbers of
> child nodes, or that the child nodes are an exhaustive list of
> attached devices, neither of which may be true.  num_chipselect should
> be calculated from the number of GPIOs specified instead.

[I'm not arguing just a thought.]

- every SPI device must have its own chip-select, otherwise SPI device
  node should not be a part of a SPI controller node;
- or there is just once device on the SPI bus with chip-select always
  asserted, no gpios = <> is specified (this case is implemented);
- or the SPI is bridged, gpios = <> should list behind-the-bridge devices'
  chip-selects, and driver should understand that there is a "special"
  (bridge) device somewhere on the bus (not implemented).

-- 
Anton Vorontsov
email: cbouatmailru@gmail.com
irc://irc.freenode.net/bd2

Re: [PATCH 1/4] [SPI] spi_mpc83xx: convert to the OF platform driver

[Grant Likely]

On Wed, May 21, 2008 at 11:05 AM, Anton Vorontsov
<avorontsov@ru.mvista.com> wrote:
> On Wed, May 21, 2008 at 10:50:02AM -0600, Grant Likely wrote:
> [..]
>> > +
>> > +       master->num_chipselect = of_num_children(np);
>>
>> This assumes that there are no gaps in the assigned CS numbers of
>> child nodes, or that the child nodes are an exhaustive list of
>> attached devices, neither of which may be true.  num_chipselect should
>> be calculated from the number of GPIOs specified instead.
>
> [I'm not arguing just a thought.]

:-)

Here's some quick feedback off the top of my head...

> - every SPI device must have its own chip-select, otherwise SPI device
>  node should not be a part of a SPI controller node;

How about this example:  Board has SPI controller with 4 CS lines and
4 devices.  Board vendor has 3 versions of board with different
devices populated (Ver1 has all 4; Ver2 has 1 and 3; Ver3 has 1, 3 and
4).  Board firmware drops nodes from tree for non-present devices
before booting kernel (not arguing if this is the best way for
firmware to behave; but it is valid and legal).  Therefore there may
be gaps in the CS assignments.

Or how about this one: the SPI devices are off board and are plugged
in after boot.  They aren't available to be described in the device
tree, but are added at a later time in response to some hot plug
event.  The SPI bus needs to be already set up with the correct number
of CS lines.

I don't think you can assume that the device tree data will be exhaustive.

> - or there is just once device on the SPI bus with chip-select always
>  asserted, no gpios = <> is specified (this case is implemented);
> - or the SPI is bridged, gpios = <> should list behind-the-bridge devices'
>  chip-selects, and driver should understand that there is a "special"
>  (bridge) device somewhere on the bus (not implemented).

In this case, I think the gpios would be a property of the spi-bridge,
not the spi-master.  The gpios of the spi-master would have to specify
how to address the bridge; not the downstream devices.  The bridge;
when addressed correctly; would then proceed with addressing the
downstream.  In other words; the 'reg' of spi-devices attached to a
bridge would be an address that the bridge understands, not an address
that the master understands.

Cheers,
g.

-- 
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.

[PATCH 2/4] [OF] spi_of: add support for dedicated SPI constructors

[Anton Vorontsov]

Dedicated (usually the ones that need to fill platform data) constructors
will create board info, so SPI core will probe them as normal SPI devices.

Signed-off-by: Anton Vorontsov <avorontsov@ru.mvista.com>
---
 drivers/spi/spi_of.c       |   67 ++++++++++++++++++++++++++++++++++++++++++++
 include/linux/spi/spi_of.h |    5 +++
 2 files changed, 72 insertions(+), 0 deletions(-)

diff --git a/drivers/spi/spi_of.c b/drivers/spi/spi_of.c
index b5ae434..2e1a11f 100644
--- a/drivers/spi/spi_of.c
+++ b/drivers/spi/spi_of.c
@@ -11,6 +11,66 @@
 #include <linux/spi/spi.h>
 #include <linux/spi/spi_of.h>
 
+/*
+ * Caller have no idea who is master, i.e. this function does not
+ * accept pointer to the master, instead we use board infos.
+ */
+int of_spi_device_probe_common(struct device_node *np,
+			       struct spi_board_info *spi_binfo,
+			       const char *modalias)
+{
+	struct device_node *parent;
+	const u32 *bus_num;
+	const u32 *chip_select;
+	const u32 *max_speed;
+	int size;
+
+	parent = of_get_parent(np);
+	if (!parent) {
+		pr_err("%s: no parent\n", np->full_name);
+		return -EINVAL;
+	}
+
+	bus_num = of_get_property(parent, "reg", &size);
+	if (!bus_num || size < sizeof(*bus_num)) {
+		pr_err("%s: no reg specified for parent\n", np->full_name);
+		of_node_put(parent);
+		return -EINVAL;
+	}
+	spi_binfo->bus_num = *bus_num;
+	of_node_put(parent);
+
+	chip_select = of_get_property(np, "reg", &size);
+	if (!chip_select || size < sizeof(*chip_select)) {
+		pr_err("%s: no reg (chip-select) specified\n", np->full_name);
+		return -EINVAL;
+	}
+	spi_binfo->chip_select = *chip_select;
+
+	max_speed = of_get_property(np, "max-speed", &size);
+	if (!max_speed || size < sizeof(*max_speed))
+		spi_binfo->max_speed_hz = 100000;
+	else
+		spi_binfo->max_speed_hz = *max_speed;
+
+	strcpy(spi_binfo->modalias, modalias);
+
+	/*
+	 * spi_of_register_devices() should not probe this device, it will
+	 * be managed by the dedicated driver.
+	 */
+	np->data = spi_binfo;
+
+	return 0;
+}
+EXPORT_SYMBOL(of_spi_device_probe_common);
+
+void of_spi_device_remove_common(struct device_node *np)
+{
+	/* For some reason dedicated driver changed its mind. */
+	np->data = NULL;
+}
+
 /**
  * spi_of_register_devices - Register child devices onto the SPI bus
  * @master:	Pointer to spi_master device
@@ -29,6 +89,13 @@ void spi_of_register_devices(struct spi_master *master, struct device_node *np)
 	int len;
 
 	for_each_child_of_node(np, nc) {
+		if (nc->data) {
+			dev_dbg(&master->dev, "%s: device seem to be not "
+				"a simple one, somebody took care of it "
+				"already\n", np->full_name);
+			continue;
+		}
+
 		/* Alloc an spi_device */
 		spi = spi_alloc_device(master);
 		if (!spi) {
diff --git a/include/linux/spi/spi_of.h b/include/linux/spi/spi_of.h
index c943f98..1ea2d82 100644
--- a/include/linux/spi/spi_of.h
+++ b/include/linux/spi/spi_of.h
@@ -12,6 +12,11 @@
 #include <linux/of.h>
 #include <linux/spi/spi.h>
 
+extern int of_spi_device_probe_common(struct device_node *np,
+				      struct spi_board_info *spi_binfo,
+				      const char *modalias);
+extern void of_spi_device_remove_common(struct device_node *np);
+
 extern void spi_of_register_devices(struct spi_master *master,
 				    struct device_node *np);
 
-- 
1.5.5.1

Re: [PATCH 2/4] [OF] spi_of: add support for dedicated SPI constructors

[Guennadi Liakhovetski]

On Wed, 21 May 2008, Anton Vorontsov wrote:

> Dedicated (usually the ones that need to fill platform data) constructors
> will create board info, so SPI core will probe them as normal SPI devices.
> 
> Signed-off-by: Anton Vorontsov <avorontsov@ru.mvista.com>
> ---
>  drivers/spi/spi_of.c       |   67 ++++++++++++++++++++++++++++++++++++++++++++
>  include/linux/spi/spi_of.h |    5 +++
>  2 files changed, 72 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/spi/spi_of.c b/drivers/spi/spi_of.c
> index b5ae434..2e1a11f 100644
> --- a/drivers/spi/spi_of.c
> +++ b/drivers/spi/spi_of.c
> @@ -11,6 +11,66 @@
>  #include <linux/spi/spi.h>
>  #include <linux/spi/spi_of.h>
>  
> +/*
> + * Caller have no idea who is master, i.e. this function does not
> + * accept pointer to the master, instead we use board infos.
> + */
> +int of_spi_device_probe_common(struct device_node *np,
> +			       struct spi_board_info *spi_binfo,
> +			       const char *modalias)
> +{

Hm, I might well misunderstand something here, but it looks to me like you 
are again trying to use both OF _and_ platform (spi_board_info) bindings 
for your SPI setup? And this is exactly what we are trying to avoid in 
Grant's series of patches...

Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer

Re: [PATCH 2/4] [OF] spi_of: add support for dedicated SPI constructors

[Anton Vorontsov]

On Wed, May 21, 2008 at 05:56:33PM +0200, Guennadi Liakhovetski wrote:
> On Wed, 21 May 2008, Anton Vorontsov wrote:
> 
> > Dedicated (usually the ones that need to fill platform data) constructors
> > will create board info, so SPI core will probe them as normal SPI devices.
> > 
> > Signed-off-by: Anton Vorontsov <avorontsov@ru.mvista.com>
> > ---
> >  drivers/spi/spi_of.c       |   67 ++++++++++++++++++++++++++++++++++++++++++++
> >  include/linux/spi/spi_of.h |    5 +++
> >  2 files changed, 72 insertions(+), 0 deletions(-)
> > 
> > diff --git a/drivers/spi/spi_of.c b/drivers/spi/spi_of.c
> > index b5ae434..2e1a11f 100644
> > --- a/drivers/spi/spi_of.c
> > +++ b/drivers/spi/spi_of.c
> > @@ -11,6 +11,66 @@
> >  #include <linux/spi/spi.h>
> >  #include <linux/spi/spi_of.h>
> >  
> > +/*
> > + * Caller have no idea who is master, i.e. this function does not
> > + * accept pointer to the master, instead we use board infos.
> > + */
> > +int of_spi_device_probe_common(struct device_node *np,
> > +			       struct spi_board_info *spi_binfo,
> > +			       const char *modalias)
> > +{
> 
> Hm, I might well misunderstand something here, but it looks to me like you 
> are again trying to use both OF _and_ platform (spi_board_info) bindings 
> for your SPI setup?

Yes, you didn't misunderstand. ;-)

> And this is exactly what we are trying to avoid in 
> Grant's series of patches...

I didn't find other way... The show stopper is "master" argument,
drivers don't know about masters (and should not, since if they should,
then this implies that masters should be registered prior to devices,
and that complicates everything).

What is the problem with board infos, btw? I missed that part. Board
infos are good because:

1. This is how things work in real life: SPI isn't probe-able bus, so
   platform code (or board code, or OF helpers) should explicitly probe
   and create devices.
2. With platform infos we're probing in a Linux usual way (e.g.
   master's .bus_id + chip_select matching).

-- 
Anton Vorontsov
email: cbouatmailru@gmail.com
irc://irc.freenode.net/bd2

Re: [PATCH 2/4] [OF] spi_of: add support for dedicated SPI constructors

[Guennadi Liakhovetski]

On Wed, 21 May 2008, Anton Vorontsov wrote:

> On Wed, May 21, 2008 at 05:56:33PM +0200, Guennadi Liakhovetski wrote:
> > 
> > Hm, I might well misunderstand something here, but it looks to me like you 
> > are again trying to use both OF _and_ platform (spi_board_info) bindings 
> > for your SPI setup?
> 
> Yes, you didn't misunderstand. ;-)
> 
> > And this is exactly what we are trying to avoid in 
> > Grant's series of patches...
> 
> I didn't find other way... The show stopper is "master" argument,
> drivers don't know about masters (and should not, since if they should,
> then this implies that masters should be registered prior to devices,
> and that complicates everything).
> 
> What is the problem with board infos, btw? I missed that part. Board

In short: board infos are not bad as such. I find it bad if you have to 
use both OF and platform bindings to describe _one_ piece of hardware. For 
more details you might want to re-read the thread, so, we don't have to 
discuss the same issues again.

Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer

Re: [PATCH 2/4] [OF] spi_of: add support for dedicated SPI constructors

[Anton Vorontsov]

On Wed, May 21, 2008 at 06:24:58PM +0200, Guennadi Liakhovetski wrote:
> On Wed, 21 May 2008, Anton Vorontsov wrote:
> 
> > On Wed, May 21, 2008 at 05:56:33PM +0200, Guennadi Liakhovetski wrote:
> > > 
> > > Hm, I might well misunderstand something here, but it looks to me like you 
> > > are again trying to use both OF _and_ platform (spi_board_info) bindings 
> > > for your SPI setup?
> > 
> > Yes, you didn't misunderstand. ;-)
> > 
> > > And this is exactly what we are trying to avoid in 
> > > Grant's series of patches...
> > 
> > I didn't find other way... The show stopper is "master" argument,
> > drivers don't know about masters (and should not, since if they should,
> > then this implies that masters should be registered prior to devices,
> > and that complicates everything).
> > 
> > What is the problem with board infos, btw? I missed that part. Board
> 
> In short: board infos are not bad as such. I find it bad if you have to 
> use both OF and platform bindings to describe _one_ piece of hardware.

This particular discussion isn't about describing hardware (since
we're describing it via device tree), but about implementation
details, such as:

1. Passing platform_data to the drivers;
2. Creating "SPI Linux devices" from the OF description.

I see there ways:

1. Grant Likely's approach (works great for simple drivers which don't
   need SPI platform_data).
2. Old board infos approach, there we can do whatever we want.
3. Implementing OF bindings for the every SPI driver that needs
   platform_data.

I could do "3", let's see what it will look like...

-- 
Anton Vorontsov
email: cbouatmailru@gmail.com
irc://irc.freenode.net/bd2

Re: [PATCH 2/4] [OF] spi_of: add support for dedicated SPI constructors

[Grant Likely]

On Wed, May 21, 2008 at 10:48 AM, Anton Vorontsov
<avorontsov@ru.mvista.com> wrote:
> On Wed, May 21, 2008 at 06:24:58PM +0200, Guennadi Liakhovetski wrote:
>> On Wed, 21 May 2008, Anton Vorontsov wrote:
>>
>> > On Wed, May 21, 2008 at 05:56:33PM +0200, Guennadi Liakhovetski wrote:
>> > >
>> > > Hm, I might well misunderstand something here, but it looks to me like you
>> > > are again trying to use both OF _and_ platform (spi_board_info) bindings
>> > > for your SPI setup?
>> >
>> > Yes, you didn't misunderstand. ;-)
>> >
>> > > And this is exactly what we are trying to avoid in
>> > > Grant's series of patches...
>> >
>> > I didn't find other way... The show stopper is "master" argument,
>> > drivers don't know about masters (and should not, since if they should,
>> > then this implies that masters should be registered prior to devices,
>> > and that complicates everything).
>> >
>> > What is the problem with board infos, btw? I missed that part. Board
>>
>> In short: board infos are not bad as such. I find it bad if you have to
>> use both OF and platform bindings to describe _one_ piece of hardware.

Sonething to note:  'platform bindings' is the wrong terminology.  I'd
like to be careful here because the term 'platform' is already
overloaded and leads to confusion.  Specifically, the 'platform bus'
doesn't come into play at all here and 'platform code' simply refers
to the board specific code in arch/powerpc/platforms.  Really, the
discussion is between using 'board info' to pass data vs. using the OF
api.

>
> This particular discussion isn't about describing hardware (since
> we're describing it via device tree), but about implementation
> details, such as:
>
> 1. Passing platform_data to the drivers;
> 2. Creating "SPI Linux devices" from the OF description.
>
> I see there ways:
>
> 1. Grant Likely's approach (works great for simple drivers which don't
>   need SPI platform_data).
> 2. Old board infos approach, there we can do whatever we want.
> 3. Implementing OF bindings for the every SPI driver that needs
>   platform_data.

or perhaps: 4. allow platform code to pass supplementary board info to
specific spi devices when appropriate (so the '1.' path is always
used, but it can also parse a board info structure if one is provided.
 This is different from '2.' which short circuits the spi_of path
entirely).

I concede that sometimes platform code just has to pass data to the
driver that cannot be described in the device tree.  callback pointers
being the most significant example and we do need a sane way to do so.

That being said; in most cases I'd much rather see code added to the
driver itself to extract data from the device tree.  This has the
advantage that the data transformation code stays with the driver
where it belongs and it keeps code common as much as possible
(discourage duplication of code in the platform directories).

Cheers,
g.

-- 
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.

Re: [PATCH 2/4] [OF] spi_of: add support for dedicated SPI constructors

[Guennadi Liakhovetski]

On Wed, 21 May 2008, Grant Likely wrote:

> I concede that sometimes platform code just has to pass data to the
> driver that cannot be described in the device tree.  callback pointers
> being the most significant example and we do need a sane way to do so.

Sorry, I just cannot understand. So far my understanding was, that OF 
bindings were there to _completely_ describe hardware, that cannot be 
autoconfigured or probed. So a _generic_ driver gets all information it 
needs from the device tree and needs no additional information to handle 
the hardware.

Now if a driver needs additional information from "platform code" what's 
the purpose of partial data in the device tree at all? Just use board 
information as usual.

What good is an OF-binding if it doesn't provide complete information?

Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer

Re: [PATCH 2/4] [OF] spi_of: add support for dedicated SPI constructors

[Grant Likely]

On Wed, May 21, 2008 at 11:51 AM, Guennadi Liakhovetski
<g.liakhovetski@gmx.de> wrote:
> On Wed, 21 May 2008, Grant Likely wrote:
>
>> I concede that sometimes platform code just has to pass data to the
>> driver that cannot be described in the device tree.  callback pointers
>> being the most significant example and we do need a sane way to do so.
>
> Sorry, I just cannot understand. So far my understanding was, that OF
> bindings were there to _completely_ describe hardware, that cannot be
> autoconfigured or probed.

Correct, they describe the hardware... to a level sufficient to
identify it usefully and uniquely to an operating system.

> So a _generic_ driver gets all information it
> needs from the device tree and needs no additional information to handle
> the hardware.

Also correct.

>
> Now if a driver needs additional information from "platform code" what's
> the purpose of partial data in the device tree at all? Just use board
> information as usual.

(Note; it must be said that I think the situation is *rare* and should
be avoided)

If some behaviour is entirely board specific and doesn't have any
possibility of being duplicated on other boards, then it makes sense
for the code implementing that behaviour to live with the platform
code.  For example; consider the example of the ads7846 device driver.
 It needs a pointer to a function that will tell it whether or not the
pen is down.  This is entirely a board specific kind of thing; it
might happen via a GPIO or a key press or a phase of the moon.  The
device doesn't care; but the driver needs a way to test the state.

The driver needs some method to obtain the pointer to the function.
It cannot be encoded in the device tree because it is a function
pointer, not raw 'data'.  The device tree may have some data that
tells the platform code how it is wired up; but the pointer itself
cannot be encoded into the device tree.  I see two obvious ways to do
this.  (note: platform code cannot simply export the function because
it might be a multiplatform kernel with multiple platforms defining
the same function).

1) Pass it in a data structure passed with the device structure
2) Make the device driver call out to a lookup function that returns
the function pointer.

Option 1) is basically some form of pdata scheme.  Option 2 requires
the driver to use some API to go looking for the relevant function.
I'm not convinced yet on which method is the best; but I cannot say
that platform code must never pass data to drivers that is not
directly encoded into the device tree.

Besides, we must always account for the situation where the provided
device tree has a bug or the hardware has a quirk and platform code
needs to do something to fix it up.  Or if you prefer: 99% of the time
everything is handled with common code reading the device tree
directly; but in the 1% of the time where that does not work (due to
bugs or quirks) we have options to insert workaround code.

>
> What good is an OF-binding if it doesn't provide complete information?

It still increases the amount of common code even if there are rare
corner cases that require board specific code.

Cheers,
g.

-- 
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.

Re: [PATCH 2/4] [OF] spi_of: add support for dedicated SPI constructors

[Guennadi Liakhovetski]

On Wed, 21 May 2008, Grant Likely wrote:

> If some behaviour is entirely board specific and doesn't have any
> possibility of being duplicated on other boards, then it makes sense
> for the code implementing that behaviour to live with the platform
> code.

Exactly - "entirely board specific" and "doesn't have any possibility of 
being duplicated on other boards."

> > What good is an OF-binding if it doesn't provide complete information?
> 
> It still increases the amount of common code even if there are rare
> corner cases that require board specific code.

Right again - _rare_ corner cases. Whereas we are talking about _all_ SPI 
busses, maybe apart from those, where the controller itself switches CSs 
in a well-defined way, and the driver doesn't need any additional 
information to handle this.

Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer

Re: [PATCH 2/4] [OF] spi_of: add support for dedicated SPI constructors

[Grant Likely]

On Wed, May 21, 2008 at 1:20 PM, Guennadi Liakhovetski
<g.liakhovetski@gmx.de> wrote:
> On Wed, 21 May 2008, Grant Likely wrote:
>
>> If some behaviour is entirely board specific and doesn't have any
>> possibility of being duplicated on other boards, then it makes sense
>> for the code implementing that behaviour to live with the platform
>> code.
>
> Exactly - "entirely board specific" and "doesn't have any possibility of
> being duplicated on other boards."
>
>> > What good is an OF-binding if it doesn't provide complete information?
>>
>> It still increases the amount of common code even if there are rare
>> corner cases that require board specific code.
>
> Right again - _rare_ corner cases. Whereas we are talking about _all_ SPI
> busses, maybe apart from those, where the controller itself switches CSs
> in a well-defined way, and the driver doesn't need any additional
> information to handle this.

Ah, I see where we are crossing our wires.  I was talking about the
case of registering spi devices.  I agree that the spi bus should not
need any additional information.

Cheers,
g.

-- 
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.

Re: [PATCH 2/4] [OF] spi_of: add support for dedicated SPI constructors

[Guennadi Liakhovetski]

On Wed, 21 May 2008, Grant Likely wrote:

> On Wed, May 21, 2008 at 1:20 PM, Guennadi Liakhovetski
> <g.liakhovetski@gmx.de> wrote:
> >
> > Right again - _rare_ corner cases. Whereas we are talking about _all_ SPI
> > busses, maybe apart from those, where the controller itself switches CSs
> > in a well-defined way, and the driver doesn't need any additional
> > information to handle this.
> 
> Ah, I see where we are crossing our wires.  I was talking about the
> case of registering spi devices.  I agree that the spi bus should not
> need any additional information.

No, sorry for not making it clear. I wrote "busses" because on those 
controllers, that control CS themselves _devices_ don't need any 
additional info. But I meant, that describing SPI _devices_ should be done 
in only one way or another - either using fdt, or platform data, not both.

Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer

Re: [PATCH 2/4] [OF] spi_of: add support for dedicated SPI constructors

[Grant Likely]

On Wed, May 21, 2008 at 2:00 PM, Guennadi Liakhovetski
<g.liakhovetski@gmx.de> wrote:
> On Wed, 21 May 2008, Grant Likely wrote:
>
>> On Wed, May 21, 2008 at 1:20 PM, Guennadi Liakhovetski
>> <g.liakhovetski@gmx.de> wrote:
>> >
>> > Right again - _rare_ corner cases. Whereas we are talking about _all_ SPI
>> > busses, maybe apart from those, where the controller itself switches CSs
>> > in a well-defined way, and the driver doesn't need any additional
>> > information to handle this.
>>
>> Ah, I see where we are crossing our wires.  I was talking about the
>> case of registering spi devices.  I agree that the spi bus should not
>> need any additional information.
>
> No, sorry for not making it clear. I wrote "busses" because on those
> controllers, that control CS themselves _devices_ don't need any
> additional info. But I meant, that describing SPI _devices_ should be done
> in only one way or another - either using fdt, or platform data, not both.

Yes, I agree with that too... with the one caveat that platform code
should have some method to supplement the data in the device tree
*only when it is absolutely necessary to do so*.  (note: I say
'platform code' here, not 'platform data'.  They are two different
things)  I make no claims on what that method should be at this time.

Cheers,
g.

-- 
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.

Re: [PATCH 2/4] [OF] spi_of: add support for dedicated SPI constructors

[Grant Likely]

On Wed, May 21, 2008 at 9:41 AM, Anton Vorontsov
<avorontsov@ru.mvista.com> wrote:
> Dedicated (usually the ones that need to fill platform data) constructors
> will create board info, so SPI core will probe them as normal SPI devices.
>
> Signed-off-by: Anton Vorontsov <avorontsov@ru.mvista.com>
> ---
>  drivers/spi/spi_of.c       |   67 ++++++++++++++++++++++++++++++++++++++++++++
>  include/linux/spi/spi_of.h |    5 +++
>  2 files changed, 72 insertions(+), 0 deletions(-)
>
> diff --git a/drivers/spi/spi_of.c b/drivers/spi/spi_of.c
> index b5ae434..2e1a11f 100644
> --- a/drivers/spi/spi_of.c
> +++ b/drivers/spi/spi_of.c
> @@ -11,6 +11,66 @@
>  #include <linux/spi/spi.h>
>  #include <linux/spi/spi_of.h>
>
> +/*
> + * Caller have no idea who is master, i.e. this function does not
> + * accept pointer to the master, instead we use board infos.
> + */
> +int of_spi_device_probe_common(struct device_node *np,
> +                              struct spi_board_info *spi_binfo,
> +                              const char *modalias)
> +{
> +       struct device_node *parent;
> +       const u32 *bus_num;
> +       const u32 *chip_select;
> +       const u32 *max_speed;
> +       int size;
> +
> +       parent = of_get_parent(np);
> +       if (!parent) {
> +               pr_err("%s: no parent\n", np->full_name);
> +               return -EINVAL;
> +       }
> +
> +       bus_num = of_get_property(parent, "reg", &size);
> +       if (!bus_num || size < sizeof(*bus_num)) {
> +               pr_err("%s: no reg specified for parent\n", np->full_name);
> +               of_node_put(parent);
> +               return -EINVAL;
> +       }
> +       spi_binfo->bus_num = *bus_num;
> +       of_node_put(parent);
> +
> +       chip_select = of_get_property(np, "reg", &size);
> +       if (!chip_select || size < sizeof(*chip_select)) {
> +               pr_err("%s: no reg (chip-select) specified\n", np->full_name);
> +               return -EINVAL;
> +       }
> +       spi_binfo->chip_select = *chip_select;
> +
> +       max_speed = of_get_property(np, "max-speed", &size);
> +       if (!max_speed || size < sizeof(*max_speed))
> +               spi_binfo->max_speed_hz = 100000;
> +       else
> +               spi_binfo->max_speed_hz = *max_speed;
> +
> +       strcpy(spi_binfo->modalias, modalias);
> +
> +       /*
> +        * spi_of_register_devices() should not probe this device, it will
> +        * be managed by the dedicated driver.
> +        */
> +       np->data = spi_binfo;
> +
> +       return 0;
> +}
> +EXPORT_SYMBOL(of_spi_device_probe_common);

This is mostly a duplication of stuff in spi_of_register_devices() and
it short circuits that path.  While I'm not hugely fond of boardinfo
in general, I'd much rather see it passed via spi_of_register_devices
instead of using a different path.  I know we've discussed this before
and I resisted settling on a solution (like adding a specific
platform_data pointer instead of using device_node->data) mostly
because I'm being cautious.  I still don't know if it would be better
to use a device_node value or to have some form of callback into the
boards platform code.  But, either method would be better than having
multiple paths for registering SPI devices which are described in the
device tree.

Cheers,
g.

-- 
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.

[PATCH 3/4] [OF] MMC-over-SPI OF constructor

[Anton Vorontsov]

Signed-off-by: Anton Vorontsov <avorontsov@ru.mvista.com>
---
 Documentation/powerpc/booting-without-of.txt |   24 +++++
 drivers/of/Kconfig                           |    6 ++
 drivers/of/Makefile                          |    1 +
 drivers/of/spi_mmc.c                         |  122 ++++++++++++++++++++++++++
 4 files changed, 153 insertions(+), 0 deletions(-)
 create mode 100644 drivers/of/spi_mmc.c

diff --git a/Documentation/powerpc/booting-without-of.txt b/Documentation/powerpc/booting-without-of.txt
index 011bf5e..474f50a 100644
--- a/Documentation/powerpc/booting-without-of.txt
+++ b/Documentation/powerpc/booting-without-of.txt
@@ -3113,7 +3113,31 @@ platforms are moved over to use the flattened-device-tree model.
 			};
 		};
 
+    t) MMC-over-SPI
 
+      Required properties:
+      - #address-cells : should be 0;
+      - #size-cells : should be 0;
+      - compatible : "linux,mmc-spi".
+      - linux,modalias  - (optional) permissible value is "mmc_spi", only used
+        if dedicated driver failed to probe.
+      - reg : should specify SPI address (chip-select number).
+      - max-speed : (optional) maximum frequency for this device (Hz).
+      - mmc,ocr-mask : (optional) Linux-specific MMC OCR mask (slot voltage).
+      - gpios : (optional) may specify GPIOs in this order: Write-Protect GPIO,
+        Card-Detect GPIO.
+
+      Example:
+
+	mmc-slot@0 {
+		compatible = "linux,mmc-spi";
+		linux,modalias = "mmc_spi";
+		reg = <0>;
+		max-speed = <50000000>;
+		mmc,ocr-mask = <0x00200000>;
+		gpios = <&sdcsr_pio 6 0   /*  WP */
+			 &sdcsr_pio 7 1>; /* nCD */
+	};
 
 VII - Marvell Discovery mv64[345]6x System Controller chips
 ===========================================================
diff --git a/drivers/of/Kconfig b/drivers/of/Kconfig
index 3a7a11a..80aaf8b 100644
--- a/drivers/of/Kconfig
+++ b/drivers/of/Kconfig
@@ -13,3 +13,9 @@ config OF_I2C
 	depends on PPC_OF && I2C
 	help
 	  OpenFirmware I2C accessors
+
+config OF_MMC_SPI
+	def_bool y if MMC_SPI
+	depends on OF_GPIO
+	help
+	  OpenFirmware MMC-over-SPI constructor
diff --git a/drivers/of/Makefile b/drivers/of/Makefile
index 548772e..f6ee8b3 100644
--- a/drivers/of/Makefile
+++ b/drivers/of/Makefile
@@ -2,3 +2,4 @@ obj-y = base.o
 obj-$(CONFIG_OF_DEVICE) += device.o platform.o
 obj-$(CONFIG_OF_GPIO)   += gpio.o
 obj-$(CONFIG_OF_I2C)	+= of_i2c.o
+obj-$(CONFIG_OF_MMC_SPI)	+= spi_mmc.o
diff --git a/drivers/of/spi_mmc.c b/drivers/of/spi_mmc.c
new file mode 100644
index 0000000..90fff50
--- /dev/null
+++ b/drivers/of/spi_mmc.c
@@ -0,0 +1,122 @@
+/*
+ * OpenFirmware MMC-over-SPI constructor
+ *
+ * Copyright (c) MontaVista Software, Inc. 2008.
+ *
+ * Author: Anton Vorontsov <avorontsov@ru.mvista.com>
+ *
+ * This program is free software; you can redistribute  it and/or modify it
+ * under  the terms of  the GNU General  Public License as published by the
+ * Free Software Foundation;  either version 2 of the  License, or (at your
+ * option) any later version.
+ */
+
+#include <linux/init.h>
+#include <linux/kernel.h>
+#include <linux/of_platform.h>
+#include <linux/spi/spi.h>
+#include <linux/spi/spi_of.h>
+#include <linux/spi/mmc_spi.h>
+#include <linux/mmc/host.h>
+#include <linux/gpio.h>
+#include <linux/of_gpio.h>
+
+struct of_mmc_spi {
+	int wp_gpio;
+	int cd_gpio;
+	struct spi_board_info spi_binfo;
+	struct mmc_spi_platform_data mmc_pdata;
+};
+
+static int mmc_get_ro(struct device *dev)
+{
+	/* luckily spi core copies pdata pointer, not the data */
+	struct of_mmc_spi *oms = container_of(dev->platform_data,
+					      struct of_mmc_spi, mmc_pdata);
+	return gpio_get_value(oms->wp_gpio);
+}
+
+static int __devinit of_mmc_spi_probe(struct of_device *ofdev,
+				      const struct of_device_id *match)
+{
+	int ret = -EINVAL;
+	struct of_mmc_spi *oms = kzalloc(sizeof(*oms), GFP_KERNEL);
+	struct device_node *np = ofdev->node;
+	struct device *dev = &ofdev->dev;
+	const u32 *ocr_mask;
+	int size;
+
+	if (!oms)
+		return -ENOMEM;
+
+	ocr_mask = of_get_property(np, "mmc,ocr-mask", &size);
+	if (ocr_mask && size >= sizeof(ocr_mask))
+		oms->mmc_pdata.ocr_mask = *ocr_mask;
+
+	oms->wp_gpio = of_get_gpio(np, 0);
+	if (gpio_is_valid(oms->wp_gpio)) {
+		ret = gpio_request(oms->wp_gpio, dev->bus_id);
+		if (ret < 0)
+			goto err_wp_gpio;
+		oms->mmc_pdata.get_ro = &mmc_get_ro;
+	}
+
+	oms->cd_gpio = of_get_gpio(np, 1);
+	if (gpio_is_valid(oms->cd_gpio)) {
+		ret = gpio_request(oms->cd_gpio, dev->bus_id);
+		if (ret < 0)
+			goto err_cd_gpio;
+	}
+
+	oms->spi_binfo.platform_data = &oms->mmc_pdata;
+
+	ret = of_spi_device_probe_common(np, &oms->spi_binfo, "mmc_spi");
+	if (ret)
+		goto err_common;
+
+	ret = spi_register_board_info(&oms->spi_binfo, 1);
+	if (ret)
+		goto err_binfo;
+
+	dev_info(dev, "slot with%s write-protect and with%s card-detect "
+		 "recognition\n", gpio_is_valid(oms->wp_gpio) ? "" : "out",
+		 gpio_is_valid(oms->cd_gpio) ? "" : "out");
+
+	return 0;
+err_binfo:
+	of_spi_device_remove_common(np);
+err_common:
+	if (gpio_is_valid(oms->cd_gpio))
+		gpio_free(oms->cd_gpio);
+err_cd_gpio:
+	if (gpio_is_valid(oms->wp_gpio))
+		gpio_free(oms->wp_gpio);
+err_wp_gpio:
+	kfree(oms);
+	return ret;
+}
+
+static const struct of_device_id of_mmc_spi_match[] = {
+	{ .compatible = "linux,mmc-spi", },
+	{},
+};
+MODULE_DEVICE_TABLE(of, of_mmc_spi_match);
+
+static struct of_platform_driver of_mmc_spi_driver = {
+	.match_table	= of_mmc_spi_match,
+	.probe		= of_mmc_spi_probe,
+	.driver		= {
+		.name	= "of_mmc_spi",
+		.owner	= THIS_MODULE,
+	},
+};
+
+static int __init of_mmc_spi_init(void)
+{
+	return of_register_platform_driver(&of_mmc_spi_driver);
+}
+arch_initcall(of_mmc_spi_init);
+
+/* SPI board infos aren't hot-plugable, thus no module_exit */
+
+MODULE_LICENSE("GPL");
-- 
1.5.5.1

[PATCH 4/4] [POWERPC] 86xx: mpc8610_hpcd: support for MMC-over-SPI and PIXIS' GPIOs

[Anton Vorontsov]

This patch implements support for PIXIS' GPIOs and adds appropriate
nodes to support MMC-over-SPI.

Signed-off-by: Anton Vorontsov <avorontsov@ru.mvista.com>
---
 arch/powerpc/boot/dts/mpc8610_hpcd.dts     |   32 ++++++++
 arch/powerpc/platforms/86xx/Kconfig        |    2 +
 arch/powerpc/platforms/86xx/mpc8610_hpcd.c |  119 ++++++++++++++++++++++++++++
 3 files changed, 153 insertions(+), 0 deletions(-)

diff --git a/arch/powerpc/boot/dts/mpc8610_hpcd.dts b/arch/powerpc/boot/dts/mpc8610_hpcd.dts
index 44e9287..e3bf5e6 100644
--- a/arch/powerpc/boot/dts/mpc8610_hpcd.dts
+++ b/arch/powerpc/boot/dts/mpc8610_hpcd.dts
@@ -100,8 +100,18 @@
 		};
 
 		board-control@3,0 {
+			#address-cells = <1>;
+			#size-cells = <1>;
 			compatible = "fsl,fpga-pixis";
+			ranges = <0 3 0 0x20>;
 			reg = <3 0 0x20>;
+
+			sdcsr_pio: gpio-controller@a {
+				#gpio-cells = <2>;
+				compatible = "fsl,fpga-pixis-pio";
+				reg = <0xa 1>;
+				gpio-controller;
+			};
 		};
 	};
 
@@ -193,6 +203,28 @@
 			reg = <0xe4000 0x100>;
 		};
 
+		spi@7000 {
+			#address-cells = <1>;
+			#size-cells = <0>;
+			compatible = "fsl,mpc8610-spi", "fsl,spi", "simple-bus";
+			cell-index = <0>;
+			reg = <0x7000 0x40>;
+			interrupts = <59 2>;
+			interrupt-parent = <&mpic>;
+			mode = "cpu";
+			/* gpios = <&sdcsr_pio 0 1>; */
+
+			mmc-slot@0 {
+				compatible = "linux,mmc-spi";
+				linux,modalias = "mmc_spi";
+				reg = <0>;
+				max-speed = <50000000>;
+				mmc,ocr-mask = <0x00200000>;
+				gpios = <&sdcsr_pio 6 0   /*  WP */
+					 &sdcsr_pio 7 1>; /* nCD */
+			};
+		};
+
 		i2s@16000 {
 			compatible = "fsl,mpc8610-ssi";
 			cell-index = <0>;
diff --git a/arch/powerpc/platforms/86xx/Kconfig b/arch/powerpc/platforms/86xx/Kconfig
index 39c02af..a7eb89c 100644
--- a/arch/powerpc/platforms/86xx/Kconfig
+++ b/arch/powerpc/platforms/86xx/Kconfig
@@ -22,6 +22,8 @@ config MPC8610_HPCD
 	bool "Freescale MPC8610 HPCD"
 	select DEFAULT_UIMAGE
 	select FSL_ULI1575
+	select GENERIC_GPIO
+	select HAVE_GPIO_LIB
 	help
 	  This option enables support for the MPC8610 HPCD board.
 
diff --git a/arch/powerpc/platforms/86xx/mpc8610_hpcd.c b/arch/powerpc/platforms/86xx/mpc8610_hpcd.c
index fe163d4..9c1a56b 100644
--- a/arch/powerpc/platforms/86xx/mpc8610_hpcd.c
+++ b/arch/powerpc/platforms/86xx/mpc8610_hpcd.c
@@ -23,6 +23,9 @@
 #include <linux/delay.h>
 #include <linux/seq_file.h>
 #include <linux/of.h>
+#include <linux/io.h>
+#include <linux/gpio.h>
+#include <linux/of_gpio.h>
 
 #include <asm/system.h>
 #include <asm/time.h>
@@ -42,6 +45,122 @@
 
 static unsigned char *pixis_bdcfg0, *pixis_arch;
 
+struct px_gpio_chip {
+	struct of_mm_gpio_chip mm_gc;
+	spinlock_t lock;
+
+	/* mask for active-low pins */
+	u8 active_low;
+};
+
+static inline struct px_gpio_chip *
+to_px_gpio_chip(struct of_mm_gpio_chip *mm_gc)
+{
+	return container_of(mm_gc, struct px_gpio_chip, mm_gc);
+}
+
+static int px_gpio_get(struct gpio_chip *gc, unsigned int gpio)
+{
+	struct of_mm_gpio_chip *mm_gc = to_of_mm_gpio_chip(gc);
+	struct px_gpio_chip *px_gc = to_px_gpio_chip(mm_gc);
+	u8 __iomem *regs = mm_gc->regs;
+	u32 pin_mask = 1 << gpio;
+
+	return (in_8(regs) ^ px_gc->active_low) & pin_mask;
+}
+
+static void px_gpio_set(struct gpio_chip *gc, unsigned int gpio, int val)
+{
+	struct of_mm_gpio_chip *mm_gc = to_of_mm_gpio_chip(gc);
+	struct px_gpio_chip *px_gc = to_px_gpio_chip(mm_gc);
+	u8 __iomem *regs = mm_gc->regs;
+	unsigned long flags;
+	u32 pin_mask = 1 << gpio;
+
+	spin_lock_irqsave(&px_gc->lock, flags);
+
+	if (((!!val << gpio) ^ px_gc->active_low) & pin_mask)
+		setbits8(regs, pin_mask);
+	else
+		clrbits8(regs, pin_mask);
+
+	spin_unlock_irqrestore(&px_gc->lock, flags);
+}
+
+static int px_gpio_dir_in(struct gpio_chip *gc, unsigned int gpio)
+{
+	return 0;
+}
+
+static int px_gpio_dir_out(struct gpio_chip *gc, unsigned int gpio, int val)
+{
+	px_gpio_set(gc, gpio, val);
+	return 0;
+}
+
+#define PX_GPIO_FLAG_ACTIVE_LOW	(1 << 0)
+
+int px_gpio_xlate(struct of_gpio_chip *of_gc, struct device_node *np,
+		  const void *gpio_spec)
+{
+	struct of_mm_gpio_chip *mm_gc = to_of_mm_gpio_chip(&of_gc->gc);
+	struct px_gpio_chip *px_gc = to_px_gpio_chip(mm_gc);
+	const u32 *gpio = gpio_spec;
+
+	if (*gpio > of_gc->gc.ngpio)
+		return -EINVAL;
+
+	if (gpio[1] & PX_GPIO_FLAG_ACTIVE_LOW)
+		px_gc->active_low |= 1 << *gpio;
+
+	return *gpio;
+}
+
+static int __init pixis_gpio_init(void)
+{
+	struct device_node *np;
+
+	for_each_compatible_node(np, NULL, "fsl,fpga-pixis-pio") {
+		int ret;
+		struct px_gpio_chip *px_gc;
+		struct of_mm_gpio_chip *mm_gc;
+		struct of_gpio_chip *of_gc;
+		struct gpio_chip *gc;
+
+		px_gc = kzalloc(sizeof(*px_gc), GFP_KERNEL);
+		if (!px_gc) {
+			ret = -ENOMEM;
+			goto err;
+		}
+
+		spin_lock_init(&px_gc->lock);
+
+		mm_gc = &px_gc->mm_gc;
+		of_gc = &mm_gc->of_gc;
+		gc = &of_gc->gc;
+
+		of_gc->gpio_cells = 2;
+		of_gc->xlate = px_gpio_xlate;
+		gc->ngpio = 8;
+		gc->direction_input = px_gpio_dir_in;
+		gc->direction_output = px_gpio_dir_out;
+		gc->get = px_gpio_get;
+		gc->set = px_gpio_set;
+
+		ret = of_mm_gpiochip_add(np, mm_gc);
+		if (ret)
+			goto err;
+		continue;
+err:
+		pr_err("%s: registration failed with status %d\n",
+		       np->full_name, ret);
+		kfree(px_gc);
+		/* try others anyway */
+	}
+	return 0;
+}
+machine_arch_initcall(mpc86xx_hpcd, pixis_gpio_init);
+
 static struct of_device_id __initdata mpc8610_ids[] = {
 	{ .compatible = "fsl,mpc8610-immr", },
 	{ .compatible = "simple-bus", },
-- 
1.5.5.1

Re: [RFC/DRAFT] SPI OF bindings, MMC-over-SPI, chip-selects and so on

[Guennadi Liakhovetski]

On Wed, 21 May 2008, Anton Vorontsov wrote:

> This is just a bait for further discussion of OF/SPI, chip-selects,
> e.t.c.
> 
> I've converted the spi_mpc83xx to the OF driver (using Grant's
> SPI_MASTER_OF work + some additions), and implemented MMC-over-SPI
> bindings. This stuff extensively using GPIOs, and I think this will
> work for the "bridged SPI" too, since the SPI bridge could be
> represented as GPIO controller (inside the SPI controller node).

Yes, but do you really _want_ to represent them as GPIOs? Firstly they are 
not really "G" P, secondly, they are not even "I", rather only "O". And do 
you really want to register your SPI CS lines to the whole system for 
everyone's (ab)use?...

Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer

Re: [RFC/DRAFT] SPI OF bindings, MMC-over-SPI, chip-selects and so on

[Anton Vorontsov]

On Wed, May 21, 2008 at 05:54:20PM +0200, Guennadi Liakhovetski wrote:
> On Wed, 21 May 2008, Anton Vorontsov wrote:
> 
> > This is just a bait for further discussion of OF/SPI, chip-selects,
> > e.t.c.
> > 
> > I've converted the spi_mpc83xx to the OF driver (using Grant's
> > SPI_MASTER_OF work + some additions), and implemented MMC-over-SPI
> > bindings. This stuff extensively using GPIOs, and I think this will
> > work for the "bridged SPI" too, since the SPI bridge could be
> > represented as GPIO controller (inside the SPI controller node).
> 
> Yes, but do you really _want_ to represent them as GPIOs? Firstly they are 
> not really "G" P, secondly, they are not even "I", rather only "O".

"General Purpose Input/Output" implies that lines could be only I or
only O, or both. (plus "dedicated function", though, here David Brownell
will probably disagree ;-) GPIOs are really general.

> And do 
> you really want to register your SPI CS lines to the whole system for 
> everyone's (ab)use?...

This is represented via device tree, so the only abusers could be
hardware designers. :-)

-- 
Anton Vorontsov
email: cbouatmailru@gmail.com
irc://irc.freenode.net/bd2

Re: [RFC/DRAFT] SPI OF bindings, MMC-over-SPI, chip-selects and so on

[Grant Likely]

On Wed, May 21, 2008 at 9:41 AM, Anton Vorontsov
<avorontsov@ru.mvista.com> wrote:
> Hi all,
>
> This is just a bait for further discussion of OF/SPI, chip-selects,
> e.t.c.
>
> I've converted the spi_mpc83xx to the OF driver (using Grant's
> SPI_MASTER_OF work + some additions), and implemented MMC-over-SPI
> bindings. This stuff extensively using GPIOs, and I think this will
> work for the "bridged SPI" too, since the SPI bridge could be
> represented as GPIO controller (inside the SPI controller node).

The GPIOs case doesn't bother me too much.  If the controller supports
GPIO chip selects (and pretty much all of them could I think) then the
SPI master driver just needs to specify that it supports N chip
selects and have a gpios property that lists all the relevant GPIOs
(just like you've done).  So I think this bit is looking good.

More complex cases would still probably need some form of spi-bridge
node (and most likely requiring changes to the SPI infrastructure to
make bridging easy; but that is just driver details).

The board info handling I think requires more thought... (details in
another email)

Cheers,
g.

-- 
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.

Re: [RFC/DRAFT] SPI OF bindings, MMC-over-SPI, chip-selects and so on

[Grant Likely]

On Wed, May 21, 2008 at 9:41 AM, Anton Vorontsov
<avorontsov@ru.mvista.com> wrote:
> Hi all,
>
> This is just a bait for further discussion of OF/SPI, chip-selects,
> e.t.c.

BTW; spi-devel-general@lists.sourceforge.net should probably be CC'd
on the next posting of this series.

Cheers,
g.

-- 
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.