spi_mpc8xxx.c: chip select polarity problem

spi_mpc8xxx.c: chip select polarity problem

[Torsten Fleischer]

Hi all,

I have 3 devices connected to the SPI bus of the MPC8313. For the Chip Select 
(CS) signals 3 GPIOs of the controller are used. But the driver uses the 
inverse polarity of the CS either during the initialization or at the transfer 
- depending on the setup of the flattened device tree.

Here is what I discovered:
The driver uses a polarity flag for each CS signal (the alow_flags array). 
These flags are set according to the 'gpios' property of the SPI node of the 
flattened device tree.
Is it correct that alow_flags[x] = 1 means CSx is active low?

During the initialization the driver sets the CS to the value of 
alow_flags[x]. I.e. CSx is High if alow_flags[x] = 1 and otherwise Low.

The flags are used in the function mpc8xxx_spi_cs_control() to take care about 
the polarity when setting the appropriate GPIO pin. But the function 
mpc8xxx_spi_chipselect() that calls the mpc8xxx_spi_cs_control() takes also 
care about the polarity of the CS (bool pol = spi->mode & SPI_CS_HIGH).

Lets assume alow_flags[x] = 1 and the property 'spi-cs-high' is not set for 
the SPI device. During initialization the driver sets the chip select signal 
'x' to High (see of_mpc8xxx_spi_get_chipselects()). This is OK if the chip 
select is active low, because this disables the device on start-up. But during 
the transfer the chip select signal is High and after the transfer is 
completed the signal is set to Low. This is not the intended behavior for an 
active low chip select.

I also tried to set alow_flags[x] = 0 for active low. In this case the 
transfer works, but the initial value for the CS is wrong (Low instead of 
High).

The problem seems to be that the polarity is taken into account twice (as 
described above).
So what would be the better solution: removing the usage of the alow_flags in 
mpc8xxx_spi_cs_control() or the variable 'pol' in mpc8xxx_spi_chipselect()?

Best Regards
Torsten Fleischer

Re: spi_mpc8xxx.c: chip select polarity problem

[Anton Vorontsov]

On Mon, Nov 16, 2009 at 05:42:46PM +0100, Torsten Fleischer wrote:
> Hi all,
> 
> I have 3 devices connected to the SPI bus of the MPC8313. For the Chip Select 
> (CS) signals 3 GPIOs of the controller are used. But the driver uses the 
> inverse polarity of the CS either during the initialization or at the transfer 
> - depending on the setup of the flattened device tree.
> 
> Here is what I discovered:
> The driver uses a polarity flag for each CS signal (the alow_flags array). 
> These flags are set according to the 'gpios' property of the SPI node of the 
> flattened device tree.
> Is it correct that alow_flags[x] = 1 means CSx is active low?

Either way should work. Though, now I tend to I think that
'spi-cs-high' is actually already encoded in the compatible
name. E.g. for device X we always know that the device assumes
CS to be active low, so it has no spi-cs-high.

How chip selects are wired to a device is another matter.

> During the initialization the driver sets the CS to the value of 
> alow_flags[x]. I.e. CSx is High if alow_flags[x] = 1 and otherwise Low.
> 
> The flags are used in the function mpc8xxx_spi_cs_control() to take care about 
> the polarity when setting the appropriate GPIO pin. But the function 
> mpc8xxx_spi_chipselect() that calls the mpc8xxx_spi_cs_control() takes also 
> care about the polarity of the CS (bool pol = spi->mode & SPI_CS_HIGH).
> 
> Lets assume alow_flags[x] = 1 and the property 'spi-cs-high' is not set for 
> the SPI device. During initialization the driver sets the chip select signal 
> 'x' to High (see of_mpc8xxx_spi_get_chipselects()). This is OK if the chip 
> select is active low, because this disables the device on start-up. But during 
> the transfer the chip select signal is High and after the transfer is 
> completed the signal is set to Low. This is not the intended behavior for an 
> active low chip select.
> 
> I also tried to set alow_flags[x] = 0 for active low. In this case the 
> transfer works, but the initial value for the CS is wrong (Low instead of 
> High).

So it might be better to fix up initial value in the platform code?

> The problem seems to be that the polarity is taken into account twice (as 
> described above).

Yep. So, today I'd suggest to not use spe-cs-high, even though
I was OK with it before.

> So what would be the better solution: removing the usage of the alow_flags in 
> mpc8xxx_spi_cs_control() or the variable 'pol' in mpc8xxx_spi_chipselect()?

Neither. 'pol' is still needed. Don't mix device wiring and the
chip select type. Driver may play active-low/high games with
a device, some drivers pass or clear SPI_CS_HIGH flags by themselves
(e.g. mmc_spi.c), so device-tree don't have to have spi-cs-high
flag specified.

But the wire from a GPIO controller to a SPI device can be inverted,
so you'll have to account that too!

Thanks,

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

Re: spi_mpc8xxx.c: chip select polarity problem

[Anton Vorontsov]

On Mon, Nov 16, 2009 at 08:10:37PM +0300, Anton Vorontsov wrote:
[...]
> > I also tried to set alow_flags[x] = 0 for active low. In this case the 
> > transfer works, but the initial value for the CS is wrong (Low instead of 
> > High).
> 
> So it might be better to fix up initial value in the platform code?

Oh, we actually cannot, because the driver calls
gpio_direction_output().

And since we don't know the mode prior to SPI device's driver
probe() finished, we'll have to set up an initial state in the
first SPI transfer. I.e. something like this:

diff --git a/drivers/spi/spi_mpc8xxx.c b/drivers/spi/spi_mpc8xxx.c
index 0fd0ec4..3b95382 100644
--- a/drivers/spi/spi_mpc8xxx.c
+++ b/drivers/spi/spi_mpc8xxx.c
@@ -114,6 +114,7 @@ struct spi_mpc8xxx_cs {
 	u32 rx_shift;		/* RX data reg shift when in qe mode */
 	u32 tx_shift;		/* TX data reg shift when in qe mode */
 	u32 hw_mode;		/* Holds HW mode register settings */
+	int initialized;
 };
 
 static inline void mpc8xxx_spi_write_reg(__be32 __iomem *reg, u32 val)
@@ -507,11 +508,17 @@ static int mpc8xxx_spi_transfer(struct spi_device *spi,
 				struct spi_message *m)
 {
 	struct mpc8xxx_spi *mpc8xxx_spi = spi_master_get_devdata(spi->master);
+	struct spi_mpc8xxx_cs *cs = spi->controller_state;
 	unsigned long flags;
 
 	m->actual_length = 0;
 	m->status = -EINPROGRESS;
 
+	if (cs && !cs->initialized) {
+		mpc8xxx_spi_chipselect(spi, BITBANG_CS_INACTIVE);
+		cs->initialized = 1;
+	}
+
 	spin_lock_irqsave(&mpc8xxx_spi->lock, flags);
 	list_add_tail(&m->queue, &mpc8xxx_spi->queue);
 	queue_work(mpc8xxx_spi->workqueue, &mpc8xxx_spi->work);

Re: spi_mpc8xxx.c: chip select polarity problem

[Torsten Fleischer]

On Mon, Nov 16, 2009 at 19:00PM, Anton Vorontsov wrote:
[...]
> > So it might be better to fix up initial value in the platform code?
> 
> Oh, we actually cannot, because the driver calls
> gpio_direction_output().
> 
> And since we don't know the mode prior to SPI device's driver
> probe() finished, we'll have to set up an initial state in the
> first SPI transfer. I.e. something like this:

In most cases the device drivers perform SPI transfers already in their 
probe() function. How can it be ensured that the CS of all other devices are 
inactive even if they are not initialized at that time?

Regards
Torsten Fleischer

Re: spi_mpc8xxx.c: chip select polarity problem

[Anton Vorontsov]

On Tue, Nov 17, 2009 at 09:09:28PM +0100, Torsten Fleischer wrote:
> On Mon, Nov 16, 2009 at 19:00PM, Anton Vorontsov wrote:
> [...]
> > > So it might be better to fix up initial value in the platform code?
> > 
> > Oh, we actually cannot, because the driver calls
> > gpio_direction_output().
> > 
> > And since we don't know the mode prior to SPI device's driver
> > probe() finished, we'll have to set up an initial state in the
> > first SPI transfer. I.e. something like this:
> 
> In most cases the device drivers perform SPI transfers already in their 
> probe() function. How can it be ensured that the CS of all other devices are 
> inactive even if they are not initialized at that time?

Good question. Oh, well... then we have to use spi-cs-high,
no matter that it is a duplication of the 'compatible' property.
SPI bus drivers don't know all the devices and their CS level,
and so spi-cs-high is the only way to tell that information. :-(

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

Re: spi_mpc8xxx.c: chip select polarity problem

[Anton Vorontsov]

On Tue, Nov 17, 2009 at 11:22:11PM +0300, Anton Vorontsov wrote:
> On Tue, Nov 17, 2009 at 09:09:28PM +0100, Torsten Fleischer wrote:
> > On Mon, Nov 16, 2009 at 19:00PM, Anton Vorontsov wrote:
> > [...]
> > > > So it might be better to fix up initial value in the platform code?
> > > 
> > > Oh, we actually cannot, because the driver calls
> > > gpio_direction_output().
> > > 
> > > And since we don't know the mode prior to SPI device's driver
> > > probe() finished, we'll have to set up an initial state in the
> > > first SPI transfer. I.e. something like this:
> > 
> > In most cases the device drivers perform SPI transfers already in their 
> > probe() function. How can it be ensured that the CS of all other devices are 
> > inactive even if they are not initialized at that time?
> 
> Good question. Oh, well... then we have to use spi-cs-high,
> no matter that it is a duplication of the 'compatible' property.
> SPI bus drivers don't know all the devices and their CS level,
> and so spi-cs-high is the only way to tell that information. :-(

Oh. On the other hand, we can postpone the gpio_direction_output()
call, and still require that the platform code (or firmware)
should be responsible for setting a sane default values on the
chip selects.

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

Re: spi_mpc8xxx.c: chip select polarity problem

[Torsten Fleischer]

On Wen, Nov 18, 2009 00:28:23 Anton Vorontsov wrote:
[...]
> > > > > So it might be better to fix up initial value in the platform code?
> > > >
> > > > Oh, we actually cannot, because the driver calls
> > > > gpio_direction_output().
> > > >
> > > > And since we don't know the mode prior to SPI device's driver
> > > > probe() finished, we'll have to set up an initial state in the
> > > > first SPI transfer. I.e. something like this:
> > >
> > > In most cases the device drivers perform SPI transfers already in their
> > > probe() function. How can it be ensured that the CS of all other
> > > devices are inactive even if they are not initialized at that time?
> >
> > Good question. Oh, well... then we have to use spi-cs-high,
> > no matter that it is a duplication of the 'compatible' property.
> > SPI bus drivers don't know all the devices and their CS level,
> > and so spi-cs-high is the only way to tell that information. :-(
> 
> Oh. On the other hand, we can postpone the gpio_direction_output()
> call, and still require that the platform code (or firmware)
> should be responsible for setting a sane default values on the
> chip selects.
> 

How about that?


diff -u -r -N linux-2.6.31.6_orig//drivers/spi/spi_mpc8xxx.c linux-2.6.31.6/drivers/spi/spi_mpc8xxx.c
--- linux-2.6.31.6_orig//drivers/spi/spi_mpc8xxx.c	2009-11-10 01:32:31.000000000 +0100
+++ linux-2.6.31.6/drivers/spi/spi_mpc8xxx.c	2009-11-18 10:47:06.000000000 +0100
@@ -114,6 +114,7 @@
 	u32 rx_shift;		/* RX data reg shift when in qe mode */
 	u32 tx_shift;		/* TX data reg shift when in qe mode */
 	u32 hw_mode;		/* Holds HW mode register settings */
+	int initialized;
 };
 
 static inline void mpc8xxx_spi_write_reg(__be32 __iomem *reg, u32 val)
@@ -437,6 +438,7 @@
 		cs = kzalloc(sizeof *cs, GFP_KERNEL);
 		if (!cs)
 			return -ENOMEM;
+		cs->initialized = 0;
 		spi->controller_state = cs;
 	}
 	mpc8xxx_spi = spi_master_get_devdata(spi->master);
@@ -503,15 +505,25 @@
 
 	return ret;
 }
+
+static void mpc8xxx_spi_cs_init(struct spi_device *spi);
+
+
 static int mpc8xxx_spi_transfer(struct spi_device *spi,
 				struct spi_message *m)
 {
 	struct mpc8xxx_spi *mpc8xxx_spi = spi_master_get_devdata(spi->master);
+	struct spi_mpc8xxx_cs *cs = spi->controller_state;
 	unsigned long flags;
 
 	m->actual_length = 0;
 	m->status = -EINPROGRESS;
 
+	if (cs && !cs->initialized) {
+		mpc8xxx_spi_cs_init(spi);
+		cs->initialized = 1;
+	}
+
 	spin_lock_irqsave(&mpc8xxx_spi->lock, flags);
 	list_add_tail(&m->queue, &mpc8xxx_spi->queue);
 	queue_work(mpc8xxx_spi->workqueue, &mpc8xxx_spi->work);
@@ -671,6 +683,17 @@
 	gpio_set_value(gpio, on ^ alow);
 }
 
+static void mpc8xxx_spi_cs_init(struct spi_device *spi)
+{
+	struct device *dev = spi->dev.parent;
+	struct mpc8xxx_spi_probe_info *pinfo = to_of_pinfo(dev->platform_data);
+	u16 cs = spi->chip_select;
+	int gpio = pinfo->gpios[cs];
+	bool on = (pinfo->alow_flags[cs] ^ !(spi->mode & SPI_CS_HIGH));
+
+	gpio_direction_output(gpio, on);
+}
+
 static int of_mpc8xxx_spi_get_chipselects(struct device *dev)
 {
 	struct device_node *np = dev_archdata_get_node(&dev->archdata);
@@ -720,14 +743,6 @@
 
 		pinfo->gpios[i] = gpio;
 		pinfo->alow_flags[i] = flags & OF_GPIO_ACTIVE_LOW;
-
-		ret = gpio_direction_output(pinfo->gpios[i],
-					    pinfo->alow_flags[i]);
-		if (ret) {
-			dev_err(dev, "can't set output direction for gpio "
-				"#%d: %d\n", i, ret);
-			goto err_loop;
-		}
 	}
 
 	pdata->max_chipselect = ngpios;

Re: spi_mpc8xxx.c: chip select polarity problem

[Anton Vorontsov]

On Wed, Nov 18, 2009 at 05:20:06PM +0100, Torsten Fleischer wrote:
[...]
> > Oh. On the other hand, we can postpone the gpio_direction_output()
> > call, and still require that the platform code (or firmware)
> > should be responsible for setting a sane default values on the
> > chip selects.
> > 
> 
> How about that?

Looks great, thanks!

Few minor issues below.

> diff -u -r -N linux-2.6.31.6_orig//drivers/spi/spi_mpc8xxx.c linux-2.6.31.6/drivers/spi/spi_mpc8xxx.c
> --- linux-2.6.31.6_orig//drivers/spi/spi_mpc8xxx.c	2009-11-10 01:32:31.000000000 +0100
> +++ linux-2.6.31.6/drivers/spi/spi_mpc8xxx.c	2009-11-18 10:47:06.000000000 +0100
> @@ -114,6 +114,7 @@
>  	u32 rx_shift;		/* RX data reg shift when in qe mode */
>  	u32 tx_shift;		/* TX data reg shift when in qe mode */
>  	u32 hw_mode;		/* Holds HW mode register settings */
> +	int initialized;
>  };
>  
>  static inline void mpc8xxx_spi_write_reg(__be32 __iomem *reg, u32 val)
> @@ -437,6 +438,7 @@
>  		cs = kzalloc(sizeof *cs, GFP_KERNEL);
>  		if (!cs)
>  			return -ENOMEM;
> +		cs->initialized = 0;

No need for this, we allocated cs with kzalloc, which zeroes
the memory anyway.

>  		spi->controller_state = cs;
>  	}
>  	mpc8xxx_spi = spi_master_get_devdata(spi->master);
> @@ -503,15 +505,25 @@
>  
>  	return ret;
>  }
> +
> +static void mpc8xxx_spi_cs_init(struct spi_device *spi);
> +
> +

Whenever possible, please avoid forward declarations.

>  static int mpc8xxx_spi_transfer(struct spi_device *spi,
>  				struct spi_message *m)
>  {
>  	struct mpc8xxx_spi *mpc8xxx_spi = spi_master_get_devdata(spi->master);
> +	struct spi_mpc8xxx_cs *cs = spi->controller_state;
>  	unsigned long flags;
>  
>  	m->actual_length = 0;
>  	m->status = -EINPROGRESS;
>  
> +	if (cs && !cs->initialized) {
> +		mpc8xxx_spi_cs_init(spi);
> +		cs->initialized = 1;
> +	}
> +
>  	spin_lock_irqsave(&mpc8xxx_spi->lock, flags);
>  	list_add_tail(&m->queue, &mpc8xxx_spi->queue);
>  	queue_work(mpc8xxx_spi->workqueue, &mpc8xxx_spi->work);
> @@ -671,6 +683,17 @@
>  	gpio_set_value(gpio, on ^ alow);
>  }
>  
> +static void mpc8xxx_spi_cs_init(struct spi_device *spi)
> +{
> +	struct device *dev = spi->dev.parent;
> +	struct mpc8xxx_spi_probe_info *pinfo = to_of_pinfo(dev->platform_data);
> +	u16 cs = spi->chip_select;
> +	int gpio = pinfo->gpios[cs];
> +	bool on = (pinfo->alow_flags[cs] ^ !(spi->mode & SPI_CS_HIGH));

The outermost parenthesis aren't needed.

> +
> +	gpio_direction_output(gpio, on);

gpio_direction_output() may fail, so spi_cs_init too.

I'd write it as 'return gpio_direction_outpit(..)', and in
mpc8xxx_spi_transfer something like this:

	if (cs && !cs->initialized) {
		int ret;

		ret = mpc8xxx_spi_cs_init(spi);
		if (ret) {
			dev_dbg(&spi->dev, "cs_init failed: %d\n", ret);
			return ret;
		}
		cs->initialized = 1;
	}


Thanks!

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

Re: spi_mpc8xxx.c: chip select polarity problem

[Grant Likely]

On Wed, Nov 18, 2009 at 4:29 PM, Anton Vorontsov
<avorontsov@ru.mvista.com> wrote:
> On Wed, Nov 18, 2009 at 05:20:06PM +0100, Torsten Fleischer wrote:
> [...]
>> > Oh. On the other hand, we can postpone the gpio_direction_output()
>> > call, and still require that the platform code (or firmware)
>> > should be responsible for setting a sane default values on the
>> > chip selects.
>> >
>>
>> How about that?
>
> Looks great, thanks!
>
> Few minor issues below.

Hey Torsten,  do you have an updated version of this change to address
the comments?  I'm collecting the last few things for some linux-next
exposure now.

g.

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

Re: spi_mpc8xxx.c: chip select polarity problem

[Torsten Fleischer]

On Sat, Nov 21, 2009 at 09:45:50 Grant Likely wrote:
[...]
> Hey Torsten,  do you have an updated version of this change to address
> the comments?  I'm collecting the last few things for some linux-next
> exposure now.

Hey Grant,

here is the updated version of the patch containing the recommended
changes.

Best Regards
Torsten


Signed-off-by: Torsten Fleischer <to-fleischer@t-online.de>
---

diff -u -r -N linux-2.6.31.6_orig//drivers/spi/spi_mpc8xxx.c linux-2.6.31.6/drivers/spi/spi_mpc8xxx.c
--- linux-2.6.31.6_orig//drivers/spi/spi_mpc8xxx.c	2009-11-10 01:32:31.000000000 +0100
+++ linux-2.6.31.6/drivers/spi/spi_mpc8xxx.c	2009-11-19 08:15:33.000000000 +0100
@@ -114,6 +114,7 @@
 	u32 rx_shift;		/* RX data reg shift when in qe mode */
 	u32 tx_shift;		/* TX data reg shift when in qe mode */
 	u32 hw_mode;		/* Holds HW mode register settings */
+	int initialized;
 };
 
 static inline void mpc8xxx_spi_write_reg(__be32 __iomem *reg, u32 val)
@@ -503,15 +504,52 @@
 
 	return ret;
 }
+
+
+struct mpc8xxx_spi_probe_info {
+	struct fsl_spi_platform_data pdata;
+	int *gpios;
+	bool *alow_flags;
+};
+
+static struct mpc8xxx_spi_probe_info *
+to_of_pinfo(struct fsl_spi_platform_data *pdata)
+{
+	return container_of(pdata, struct mpc8xxx_spi_probe_info, pdata);
+}
+
+static int mpc8xxx_spi_cs_init(struct spi_device *spi)
+{
+	struct device *dev = spi->dev.parent;
+	struct mpc8xxx_spi_probe_info *pinfo = to_of_pinfo(dev->platform_data);
+	u16 cs = spi->chip_select;
+	int gpio = pinfo->gpios[cs];
+	bool on = pinfo->alow_flags[cs] ^ !(spi->mode & SPI_CS_HIGH);
+
+	return gpio_direction_output(gpio, on);
+}
+
 static int mpc8xxx_spi_transfer(struct spi_device *spi,
 				struct spi_message *m)
 {
 	struct mpc8xxx_spi *mpc8xxx_spi = spi_master_get_devdata(spi->master);
+	struct spi_mpc8xxx_cs *cs = spi->controller_state;
 	unsigned long flags;
 
 	m->actual_length = 0;
 	m->status = -EINPROGRESS;
 
+	if (cs && !cs->initialized) {
+		int ret;
+
+		ret = mpc8xxx_spi_cs_init(spi);
+		if (ret) {
+			dev_dbg(&spi->dev, "cs_init failed: %d\n", ret);
+			return ret;
+		}
+		cs->initialized = 1;
+	}
+
 	spin_lock_irqsave(&mpc8xxx_spi->lock, flags);
 	list_add_tail(&m->queue, &mpc8xxx_spi->queue);
 	queue_work(mpc8xxx_spi->workqueue, &mpc8xxx_spi->work);
@@ -648,18 +686,6 @@
 	return 0;
 }
 
-struct mpc8xxx_spi_probe_info {
-	struct fsl_spi_platform_data pdata;
-	int *gpios;
-	bool *alow_flags;
-};
-
-static struct mpc8xxx_spi_probe_info *
-to_of_pinfo(struct fsl_spi_platform_data *pdata)
-{
-	return container_of(pdata, struct mpc8xxx_spi_probe_info, pdata);
-}
-
 static void mpc8xxx_spi_cs_control(struct spi_device *spi, bool on)
 {
 	struct device *dev = spi->dev.parent;
@@ -720,14 +746,6 @@
 
 		pinfo->gpios[i] = gpio;
 		pinfo->alow_flags[i] = flags & OF_GPIO_ACTIVE_LOW;
-
-		ret = gpio_direction_output(pinfo->gpios[i],
-					    pinfo->alow_flags[i]);
-		if (ret) {
-			dev_err(dev, "can't set output direction for gpio "
-				"#%d: %d\n", i, ret);
-			goto err_loop;
-		}
 	}
 
 	pdata->max_chipselect = ngpios;

Re: spi_mpc8xxx.c: chip select polarity problem

[Grant Likely]

Thanks.  However, there needs to be a proper description of what this
patch does to go in the commit header.  Can you please write one?

Thanks,
g.

On Sat, Nov 21, 2009 at 9:08 AM, Torsten Fleischer
<to-fleischer@t-online.de> wrote:
> On Sat, Nov 21, 2009 at 09:45:50 Grant Likely wrote:
> [...]
>> Hey Torsten, =A0do you have an updated version of this change to address
>> the comments? =A0I'm collecting the last few things for some linux-next
>> exposure now.
>
> Hey Grant,
>
> here is the updated version of the patch containing the recommended
> changes.
>
> Best Regards
> Torsten
>
>
> Signed-off-by: Torsten Fleischer <to-fleischer@t-online.de>
> ---
>
> diff -u -r -N linux-2.6.31.6_orig//drivers/spi/spi_mpc8xxx.c linux-2.6.31=
.6/drivers/spi/spi_mpc8xxx.c
> --- linux-2.6.31.6_orig//drivers/spi/spi_mpc8xxx.c =A0 =A0 =A02009-11-10 =
01:32:31.000000000 +0100
> +++ linux-2.6.31.6/drivers/spi/spi_mpc8xxx.c =A0 =A02009-11-19 08:15:33.0=
00000000 +0100
> @@ -114,6 +114,7 @@
> =A0 =A0 =A0 =A0u32 rx_shift; =A0 =A0 =A0 =A0 =A0 /* RX data reg shift whe=
n in qe mode */
> =A0 =A0 =A0 =A0u32 tx_shift; =A0 =A0 =A0 =A0 =A0 /* TX data reg shift whe=
n in qe mode */
> =A0 =A0 =A0 =A0u32 hw_mode; =A0 =A0 =A0 =A0 =A0 =A0/* Holds HW mode regis=
ter settings */
> + =A0 =A0 =A0 int initialized;
> =A0};
>
> =A0static inline void mpc8xxx_spi_write_reg(__be32 __iomem *reg, u32 val)
> @@ -503,15 +504,52 @@
>
> =A0 =A0 =A0 =A0return ret;
> =A0}
> +
> +
> +struct mpc8xxx_spi_probe_info {
> + =A0 =A0 =A0 struct fsl_spi_platform_data pdata;
> + =A0 =A0 =A0 int *gpios;
> + =A0 =A0 =A0 bool *alow_flags;
> +};
> +
> +static struct mpc8xxx_spi_probe_info *
> +to_of_pinfo(struct fsl_spi_platform_data *pdata)
> +{
> + =A0 =A0 =A0 return container_of(pdata, struct mpc8xxx_spi_probe_info, p=
data);
> +}
> +
> +static int mpc8xxx_spi_cs_init(struct spi_device *spi)
> +{
> + =A0 =A0 =A0 struct device *dev =3D spi->dev.parent;
> + =A0 =A0 =A0 struct mpc8xxx_spi_probe_info *pinfo =3D to_of_pinfo(dev->p=
latform_data);
> + =A0 =A0 =A0 u16 cs =3D spi->chip_select;
> + =A0 =A0 =A0 int gpio =3D pinfo->gpios[cs];
> + =A0 =A0 =A0 bool on =3D pinfo->alow_flags[cs] ^ !(spi->mode & SPI_CS_HI=
GH);
> +
> + =A0 =A0 =A0 return gpio_direction_output(gpio, on);
> +}
> +
> =A0static int mpc8xxx_spi_transfer(struct spi_device *spi,
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0struct spi=
_message *m)
> =A0{
> =A0 =A0 =A0 =A0struct mpc8xxx_spi *mpc8xxx_spi =3D spi_master_get_devdata=
(spi->master);
> + =A0 =A0 =A0 struct spi_mpc8xxx_cs *cs =3D spi->controller_state;
> =A0 =A0 =A0 =A0unsigned long flags;
>
> =A0 =A0 =A0 =A0m->actual_length =3D 0;
> =A0 =A0 =A0 =A0m->status =3D -EINPROGRESS;
>
> + =A0 =A0 =A0 if (cs && !cs->initialized) {
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 int ret;
> +
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 ret =3D mpc8xxx_spi_cs_init(spi);
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 if (ret) {
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 dev_dbg(&spi->dev, "cs_init=
 failed: %d\n", ret);
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 return ret;
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 }
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 cs->initialized =3D 1;
> + =A0 =A0 =A0 }
> +
> =A0 =A0 =A0 =A0spin_lock_irqsave(&mpc8xxx_spi->lock, flags);
> =A0 =A0 =A0 =A0list_add_tail(&m->queue, &mpc8xxx_spi->queue);
> =A0 =A0 =A0 =A0queue_work(mpc8xxx_spi->workqueue, &mpc8xxx_spi->work);
> @@ -648,18 +686,6 @@
> =A0 =A0 =A0 =A0return 0;
> =A0}
>
> -struct mpc8xxx_spi_probe_info {
> - =A0 =A0 =A0 struct fsl_spi_platform_data pdata;
> - =A0 =A0 =A0 int *gpios;
> - =A0 =A0 =A0 bool *alow_flags;
> -};
> -
> -static struct mpc8xxx_spi_probe_info *
> -to_of_pinfo(struct fsl_spi_platform_data *pdata)
> -{
> - =A0 =A0 =A0 return container_of(pdata, struct mpc8xxx_spi_probe_info, p=
data);
> -}
> -
> =A0static void mpc8xxx_spi_cs_control(struct spi_device *spi, bool on)
> =A0{
> =A0 =A0 =A0 =A0struct device *dev =3D spi->dev.parent;
> @@ -720,14 +746,6 @@
>
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0pinfo->gpios[i] =3D gpio;
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0pinfo->alow_flags[i] =3D flags & OF_GPIO_A=
CTIVE_LOW;
> -
> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 ret =3D gpio_direction_output(pinfo->gpios[=
i],
> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0=
 =A0 =A0 =A0 pinfo->alow_flags[i]);
> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 if (ret) {
> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 dev_err(dev, "can't set out=
put direction for gpio "
> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 "#%d: %d\n"=
, i, ret);
> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 goto err_loop;
> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 }
> =A0 =A0 =A0 =A0}
>
> =A0 =A0 =A0 =A0pdata->max_chipselect =3D ngpios;
>



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

Re: spi_mpc8xxx.c: chip select polarity problem

[Torsten Fleischer]

On Wen, Nov 25, 2009 at 01:33:57 Grant Likely wrote:
> Thanks.  However, there needs to be a proper description of what this
> patch does to go in the commit header.  Can you please write one?
> 
> Thanks,
> g.
> 
[...]

The initialization of the chip selects is removed from the probe() function of 
the spi_mpc8xxx driver, because the driver doesn't know the polarity of the 
chip selects of the SPI devices at the time of its initialization.

For this reason the initialization of the several chip selects is postponed 
to the point of time when the very first SPI transfer to the associated device
occurs.


Signed-off-by: Torsten Fleischer <to-fleischer@t-online.de>
---

diff -u -r -N linux-2.6.31.6_orig//drivers/spi/spi_mpc8xxx.c linux-2.6.31.6/drivers/spi/spi_mpc8xxx.c
--- linux-2.6.31.6_orig//drivers/spi/spi_mpc8xxx.c	2009-11-10 01:32:31.000000000 +0100
+++ linux-2.6.31.6/drivers/spi/spi_mpc8xxx.c	2009-11-19 08:15:33.000000000 +0100
@@ -114,6 +114,7 @@
 	u32 rx_shift;		/* RX data reg shift when in qe mode */
 	u32 tx_shift;		/* TX data reg shift when in qe mode */
 	u32 hw_mode;		/* Holds HW mode register settings */
+	int initialized;
 };
 
 static inline void mpc8xxx_spi_write_reg(__be32 __iomem *reg, u32 val)
@@ -503,15 +504,52 @@
 
 	return ret;
 }
+
+
+struct mpc8xxx_spi_probe_info {
+	struct fsl_spi_platform_data pdata;
+	int *gpios;
+	bool *alow_flags;
+};
+
+static struct mpc8xxx_spi_probe_info *
+to_of_pinfo(struct fsl_spi_platform_data *pdata)
+{
+	return container_of(pdata, struct mpc8xxx_spi_probe_info, pdata);
+}
+
+static int mpc8xxx_spi_cs_init(struct spi_device *spi)
+{
+	struct device *dev = spi->dev.parent;
+	struct mpc8xxx_spi_probe_info *pinfo = to_of_pinfo(dev->platform_data);
+	u16 cs = spi->chip_select;
+	int gpio = pinfo->gpios[cs];
+	bool on = pinfo->alow_flags[cs] ^ !(spi->mode & SPI_CS_HIGH);
+
+	return gpio_direction_output(gpio, on);
+}
+
 static int mpc8xxx_spi_transfer(struct spi_device *spi,
 				struct spi_message *m)
 {
 	struct mpc8xxx_spi *mpc8xxx_spi = spi_master_get_devdata(spi->master);
+	struct spi_mpc8xxx_cs *cs = spi->controller_state;
 	unsigned long flags;
 
 	m->actual_length = 0;
 	m->status = -EINPROGRESS;
 
+	if (cs && !cs->initialized) {
+		int ret;
+
+		ret = mpc8xxx_spi_cs_init(spi);
+		if (ret) {
+			dev_dbg(&spi->dev, "cs_init failed: %d\n", ret);
+			return ret;
+		}
+		cs->initialized = 1;
+	}
+
 	spin_lock_irqsave(&mpc8xxx_spi->lock, flags);
 	list_add_tail(&m->queue, &mpc8xxx_spi->queue);
 	queue_work(mpc8xxx_spi->workqueue, &mpc8xxx_spi->work);
@@ -648,18 +686,6 @@
 	return 0;
 }
 
-struct mpc8xxx_spi_probe_info {
-	struct fsl_spi_platform_data pdata;
-	int *gpios;
-	bool *alow_flags;
-};
-
-static struct mpc8xxx_spi_probe_info *
-to_of_pinfo(struct fsl_spi_platform_data *pdata)
-{
-	return container_of(pdata, struct mpc8xxx_spi_probe_info, pdata);
-}
-
 static void mpc8xxx_spi_cs_control(struct spi_device *spi, bool on)
 {
 	struct device *dev = spi->dev.parent;
@@ -720,14 +746,6 @@
 
 		pinfo->gpios[i] = gpio;
 		pinfo->alow_flags[i] = flags & OF_GPIO_ACTIVE_LOW;
-
-		ret = gpio_direction_output(pinfo->gpios[i],
-					    pinfo->alow_flags[i]);
-		if (ret) {
-			dev_err(dev, "can't set output direction for gpio "
-				"#%d: %d\n", i, ret);
-			goto err_loop;
-		}
 	}
 
 	pdata->max_chipselect = ngpios;

Re: spi_mpc8xxx.c: chip select polarity problem

[Grant Likely]

On Wed, Nov 25, 2009 at 1:41 PM, Torsten Fleischer
<to-fleischer@t-online.de> wrote:
> On Wen, Nov 25, 2009 at 01:33:57 Grant Likely wrote:
>> Thanks.  However, there needs to be a proper description of what this
>> patch does to go in the commit header.  Can you please write one?
>>
>> Thanks,
>> g.
>>
> [...]
>
> The initialization of the chip selects is removed from the probe() function of
> the spi_mpc8xxx driver, because the driver doesn't know the polarity of the
> chip selects of the SPI devices at the time of its initialization.
>
> For this reason the initialization of the several chip selects is postponed
> to the point of time when the very first SPI transfer to the associated device
> occurs.
>
>
> Signed-off-by: Torsten Fleischer <to-fleischer@t-online.de>

Ah.  I understand what you're doing now.   Hmmm.  This approach
concerns me because it relies on firmware or platform code to get CS
gpios set up properly before the driver is probed.  Firmware doesn't
always get it right, and I prefer to avoid platform specific setups as
much as possible.  Why can't the CS polarity be encoded into the
device tree so the driver *does* have the polarity data at probe time?

g.

> ---
>
> diff -u -r -N linux-2.6.31.6_orig//drivers/spi/spi_mpc8xxx.c linux-2.6.31.6/drivers/spi/spi_mpc8xxx.c
> --- linux-2.6.31.6_orig//drivers/spi/spi_mpc8xxx.c      2009-11-10 01:32:31.000000000 +0100
> +++ linux-2.6.31.6/drivers/spi/spi_mpc8xxx.c    2009-11-19 08:15:33.000000000 +0100
> @@ -114,6 +114,7 @@
>        u32 rx_shift;           /* RX data reg shift when in qe mode */
>        u32 tx_shift;           /* TX data reg shift when in qe mode */
>        u32 hw_mode;            /* Holds HW mode register settings */
> +       int initialized;
>  };
>
>  static inline void mpc8xxx_spi_write_reg(__be32 __iomem *reg, u32 val)
> @@ -503,15 +504,52 @@
>
>        return ret;
>  }
> +
> +
> +struct mpc8xxx_spi_probe_info {
> +       struct fsl_spi_platform_data pdata;
> +       int *gpios;
> +       bool *alow_flags;
> +};
> +
> +static struct mpc8xxx_spi_probe_info *
> +to_of_pinfo(struct fsl_spi_platform_data *pdata)
> +{
> +       return container_of(pdata, struct mpc8xxx_spi_probe_info, pdata);
> +}
> +
> +static int mpc8xxx_spi_cs_init(struct spi_device *spi)
> +{
> +       struct device *dev = spi->dev.parent;
> +       struct mpc8xxx_spi_probe_info *pinfo = to_of_pinfo(dev->platform_data);
> +       u16 cs = spi->chip_select;
> +       int gpio = pinfo->gpios[cs];
> +       bool on = pinfo->alow_flags[cs] ^ !(spi->mode & SPI_CS_HIGH);
> +
> +       return gpio_direction_output(gpio, on);
> +}
> +
>  static int mpc8xxx_spi_transfer(struct spi_device *spi,
>                                struct spi_message *m)
>  {
>        struct mpc8xxx_spi *mpc8xxx_spi = spi_master_get_devdata(spi->master);
> +       struct spi_mpc8xxx_cs *cs = spi->controller_state;
>        unsigned long flags;
>
>        m->actual_length = 0;
>        m->status = -EINPROGRESS;
>
> +       if (cs && !cs->initialized) {
> +               int ret;
> +
> +               ret = mpc8xxx_spi_cs_init(spi);
> +               if (ret) {
> +                       dev_dbg(&spi->dev, "cs_init failed: %d\n", ret);
> +                       return ret;
> +               }
> +               cs->initialized = 1;
> +       }
> +
>        spin_lock_irqsave(&mpc8xxx_spi->lock, flags);
>        list_add_tail(&m->queue, &mpc8xxx_spi->queue);
>        queue_work(mpc8xxx_spi->workqueue, &mpc8xxx_spi->work);
> @@ -648,18 +686,6 @@
>        return 0;
>  }
>
> -struct mpc8xxx_spi_probe_info {
> -       struct fsl_spi_platform_data pdata;
> -       int *gpios;
> -       bool *alow_flags;
> -};
> -
> -static struct mpc8xxx_spi_probe_info *
> -to_of_pinfo(struct fsl_spi_platform_data *pdata)
> -{
> -       return container_of(pdata, struct mpc8xxx_spi_probe_info, pdata);
> -}
> -
>  static void mpc8xxx_spi_cs_control(struct spi_device *spi, bool on)
>  {
>        struct device *dev = spi->dev.parent;
> @@ -720,14 +746,6 @@
>
>                pinfo->gpios[i] = gpio;
>                pinfo->alow_flags[i] = flags & OF_GPIO_ACTIVE_LOW;
> -
> -               ret = gpio_direction_output(pinfo->gpios[i],
> -                                           pinfo->alow_flags[i]);
> -               if (ret) {
> -                       dev_err(dev, "can't set output direction for gpio "
> -                               "#%d: %d\n", i, ret);
> -                       goto err_loop;
> -               }
>        }
>
>        pdata->max_chipselect = ngpios;
>



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

Re: spi_mpc8xxx.c: chip select polarity problem

[Grant Likely]

On Wed, Nov 25, 2009 at 1:41 PM, Torsten Fleischer
<to-fleischer@t-online.de> wrote:
> On Wen, Nov 25, 2009 at 01:33:57 Grant Likely wrote:
>> Thanks. =A0However, there needs to be a proper description of what this
>> patch does to go in the commit header. =A0Can you please write one?
>>
>> Thanks,
>> g.
>>
> [...]
>
> The initialization of the chip selects is removed from the probe() functi=
on of
> the spi_mpc8xxx driver, because the driver doesn't know the polarity of t=
he
> chip selects of the SPI devices at the time of its initialization.
>
> For this reason the initialization of the several chip selects is postpon=
ed
> to the point of time when the very first SPI transfer to the associated d=
evice
> occurs.
>
>
> Signed-off-by: Torsten Fleischer <to-fleischer@t-online.de>

Ah.  I understand what you're doing now.   Hmmm.  This approach
concerns me because it relies on firmware or platform code to get CS
gpios set up properly before the driver is probed.  Firmware doesn't
always get it right, and I prefer to avoid platform specific setups as
much as possible.  Why can't the CS polarity be encoded into the
device tree so the driver *does* have the polarity data at probe time?

g.

> ---
>
> diff -u -r -N linux-2.6.31.6_orig//drivers/spi/spi_mpc8xxx.c linux-2.6.31=
.6/drivers/spi/spi_mpc8xxx.c
> --- linux-2.6.31.6_orig//drivers/spi/spi_mpc8xxx.c =A0 =A0 =A02009-11-10 =
01:32:31.000000000 +0100
> +++ linux-2.6.31.6/drivers/spi/spi_mpc8xxx.c =A0 =A02009-11-19 08:15:33.0=
00000000 +0100
> @@ -114,6 +114,7 @@
> =A0 =A0 =A0 =A0u32 rx_shift; =A0 =A0 =A0 =A0 =A0 /* RX data reg shift whe=
n in qe mode */
> =A0 =A0 =A0 =A0u32 tx_shift; =A0 =A0 =A0 =A0 =A0 /* TX data reg shift whe=
n in qe mode */
> =A0 =A0 =A0 =A0u32 hw_mode; =A0 =A0 =A0 =A0 =A0 =A0/* Holds HW mode regis=
ter settings */
> + =A0 =A0 =A0 int initialized;
> =A0};
>
> =A0static inline void mpc8xxx_spi_write_reg(__be32 __iomem *reg, u32 val)
> @@ -503,15 +504,52 @@
>
> =A0 =A0 =A0 =A0return ret;
> =A0}
> +
> +
> +struct mpc8xxx_spi_probe_info {
> + =A0 =A0 =A0 struct fsl_spi_platform_data pdata;
> + =A0 =A0 =A0 int *gpios;
> + =A0 =A0 =A0 bool *alow_flags;
> +};
> +
> +static struct mpc8xxx_spi_probe_info *
> +to_of_pinfo(struct fsl_spi_platform_data *pdata)
> +{
> + =A0 =A0 =A0 return container_of(pdata, struct mpc8xxx_spi_probe_info, p=
data);
> +}
> +
> +static int mpc8xxx_spi_cs_init(struct spi_device *spi)
> +{
> + =A0 =A0 =A0 struct device *dev =3D spi->dev.parent;
> + =A0 =A0 =A0 struct mpc8xxx_spi_probe_info *pinfo =3D to_of_pinfo(dev->p=
latform_data);
> + =A0 =A0 =A0 u16 cs =3D spi->chip_select;
> + =A0 =A0 =A0 int gpio =3D pinfo->gpios[cs];
> + =A0 =A0 =A0 bool on =3D pinfo->alow_flags[cs] ^ !(spi->mode & SPI_CS_HI=
GH);
> +
> + =A0 =A0 =A0 return gpio_direction_output(gpio, on);
> +}
> +
> =A0static int mpc8xxx_spi_transfer(struct spi_device *spi,
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0struct spi=
_message *m)
> =A0{
> =A0 =A0 =A0 =A0struct mpc8xxx_spi *mpc8xxx_spi =3D spi_master_get_devdata=
(spi->master);
> + =A0 =A0 =A0 struct spi_mpc8xxx_cs *cs =3D spi->controller_state;
> =A0 =A0 =A0 =A0unsigned long flags;
>
> =A0 =A0 =A0 =A0m->actual_length =3D 0;
> =A0 =A0 =A0 =A0m->status =3D -EINPROGRESS;
>
> + =A0 =A0 =A0 if (cs && !cs->initialized) {
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 int ret;
> +
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 ret =3D mpc8xxx_spi_cs_init(spi);
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 if (ret) {
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 dev_dbg(&spi->dev, "cs_init=
 failed: %d\n", ret);
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 return ret;
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 }
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 cs->initialized =3D 1;
> + =A0 =A0 =A0 }
> +
> =A0 =A0 =A0 =A0spin_lock_irqsave(&mpc8xxx_spi->lock, flags);
> =A0 =A0 =A0 =A0list_add_tail(&m->queue, &mpc8xxx_spi->queue);
> =A0 =A0 =A0 =A0queue_work(mpc8xxx_spi->workqueue, &mpc8xxx_spi->work);
> @@ -648,18 +686,6 @@
> =A0 =A0 =A0 =A0return 0;
> =A0}
>
> -struct mpc8xxx_spi_probe_info {
> - =A0 =A0 =A0 struct fsl_spi_platform_data pdata;
> - =A0 =A0 =A0 int *gpios;
> - =A0 =A0 =A0 bool *alow_flags;
> -};
> -
> -static struct mpc8xxx_spi_probe_info *
> -to_of_pinfo(struct fsl_spi_platform_data *pdata)
> -{
> - =A0 =A0 =A0 return container_of(pdata, struct mpc8xxx_spi_probe_info, p=
data);
> -}
> -
> =A0static void mpc8xxx_spi_cs_control(struct spi_device *spi, bool on)
> =A0{
> =A0 =A0 =A0 =A0struct device *dev =3D spi->dev.parent;
> @@ -720,14 +746,6 @@
>
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0pinfo->gpios[i] =3D gpio;
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0pinfo->alow_flags[i] =3D flags & OF_GPIO_A=
CTIVE_LOW;
> -
> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 ret =3D gpio_direction_output(pinfo->gpios[=
i],
> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0=
 =A0 =A0 =A0 pinfo->alow_flags[i]);
> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 if (ret) {
> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 dev_err(dev, "can't set out=
put direction for gpio "
> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 "#%d: %d\n"=
, i, ret);
> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 goto err_loop;
> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 }
> =A0 =A0 =A0 =A0}
>
> =A0 =A0 =A0 =A0pdata->max_chipselect =3D ngpios;
>



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

Re: spi_mpc8xxx.c: chip select polarity problem

[Anton Vorontsov]

On Wed, Nov 25, 2009 at 03:11:57PM -0700, Grant Likely wrote:
> On Wed, Nov 25, 2009 at 1:41 PM, Torsten Fleischer
> <to-fleischer@t-online.de> wrote:
> > On Wen, Nov 25, 2009 at 01:33:57 Grant Likely wrote:
> >> Thanks.  However, there needs to be a proper description of what this
> >> patch does to go in the commit header.  Can you please write one?
> >>
> >> Thanks,
> >> g.
> >>
> > [...]
> >
> > The initialization of the chip selects is removed from the probe() function of
> > the spi_mpc8xxx driver, because the driver doesn't know the polarity of the
> > chip selects of the SPI devices at the time of its initialization.
> >
> > For this reason the initialization of the several chip selects is postponed
> > to the point of time when the very first SPI transfer to the associated device
> > occurs.
> >
> >
> > Signed-off-by: Torsten Fleischer <to-fleischer@t-online.de>
> 
> Ah.  I understand what you're doing now.   Hmmm.  This approach
> concerns me because it relies on firmware or platform code to get CS
> gpios set up properly before the driver is probed.

Yes, that was said at the very beginning of this thread.

>  Firmware doesn't
> always get it right, and I prefer to avoid platform specific setups as
> much as possible.  Why can't the CS polarity be encoded into the
> device tree so the driver *does* have the polarity data at probe time?

We have the spi-cs-high property, but it duplicates compatible
property. 'compatible' is enough to tell whether some device has
cs-high or cs-low (device's driver knows that already).

The problem is that SPI bus master doesn't know all the devices,
so it can't extract that information from the compatible string.
To workaround that we can use 'spi-cs-high', but that's ugly
workaround.

SPI modes (0,1,2,3) is another matter, some devices can work in
several modes, so 'spi-cpol' and 'spi-cpha' are actually useful.

-- 
Anton Vorontsov
email: cbouatmailru@gmail.com
irc://irc.freenode.net/bd2
_______________________________________________
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

Re: spi_mpc8xxx.c: chip select polarity problem

[Anton Vorontsov]

On Wed, Nov 25, 2009 at 03:11:57PM -0700, Grant Likely wrote:
> On Wed, Nov 25, 2009 at 1:41 PM, Torsten Fleischer
> <to-fleischer@t-online.de> wrote:
> > On Wen, Nov 25, 2009 at 01:33:57 Grant Likely wrote:
> >> Thanks.  However, there needs to be a proper description of what this
> >> patch does to go in the commit header.  Can you please write one?
> >>
> >> Thanks,
> >> g.
> >>
> > [...]
> >
> > The initialization of the chip selects is removed from the probe() function of
> > the spi_mpc8xxx driver, because the driver doesn't know the polarity of the
> > chip selects of the SPI devices at the time of its initialization.
> >
> > For this reason the initialization of the several chip selects is postponed
> > to the point of time when the very first SPI transfer to the associated device
> > occurs.
> >
> >
> > Signed-off-by: Torsten Fleischer <to-fleischer@t-online.de>
> 
> Ah.  I understand what you're doing now.   Hmmm.  This approach
> concerns me because it relies on firmware or platform code to get CS
> gpios set up properly before the driver is probed.

Yes, that was said at the very beginning of this thread.

>  Firmware doesn't
> always get it right, and I prefer to avoid platform specific setups as
> much as possible.  Why can't the CS polarity be encoded into the
> device tree so the driver *does* have the polarity data at probe time?

We have the spi-cs-high property, but it duplicates compatible
property. 'compatible' is enough to tell whether some device has
cs-high or cs-low (device's driver knows that already).

The problem is that SPI bus master doesn't know all the devices,
so it can't extract that information from the compatible string.
To workaround that we can use 'spi-cs-high', but that's ugly
workaround.

SPI modes (0,1,2,3) is another matter, some devices can work in
several modes, so 'spi-cpol' and 'spi-cpha' are actually useful.

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

Re: spi_mpc8xxx.c: chip select polarity problem

[Torsten Fleischer]

On Thu, Nov 26, 2009 at 13:12:04 Anton Vorontsov wrote:
[...]
> > Ah.  I understand what you're doing now.   Hmmm.  This approach
> > concerns me because it relies on firmware or platform code to get CS
> > gpios set up properly before the driver is probed.
> 
> Yes, that was said at the very beginning of this thread.
> 
> >  Firmware doesn't
> > always get it right, and I prefer to avoid platform specific setups as
> > much as possible.  Why can't the CS polarity be encoded into the
> > device tree so the driver *does* have the polarity data at probe time?
> 
> We have the spi-cs-high property, but it duplicates compatible
> property. 'compatible' is enough to tell whether some device has
> cs-high or cs-low (device's driver knows that already).
> 
> The problem is that SPI bus master doesn't know all the devices,
> so it can't extract that information from the compatible string.
> To workaround that we can use 'spi-cs-high', but that's ugly
> workaround.
> 
> SPI modes (0,1,2,3) is another matter, some devices can work in
> several modes, so 'spi-cpol' and 'spi-cpha' are actually useful.
> 
To get a sane initial state the needed GPIOs can be set to be inputs during 
the driver's initialization.
This requires pull-up/pull-down resistors connected to the chip select 
lines. I think we can assume that they exist, because the GPIOs are all inputs 
after the controller's hardware reset and the resistors are needed to have a 
well-defined voltage level on the chip select lines. Normally the level is set 
so that the devices are disabled.

Therefore, it doesn't matter if the firmware sets the GPIOs wrong.

Re: spi_mpc8xxx.c: chip select polarity problem

[Grant Likely]

On Thu, Nov 26, 2009 at 10:27 AM, Torsten Fleischer
<to-fleischer@t-online.de> wrote:
> On Thu, Nov 26, 2009 at 13:12:04 Anton Vorontsov wrote:
> [...]
>> > Ah.  I understand what you're doing now.   Hmmm.  This approach
>> > concerns me because it relies on firmware or platform code to get CS
>> > gpios set up properly before the driver is probed.
>>
>> Yes, that was said at the very beginning of this thread.
>>
>> >  Firmware doesn't
>> > always get it right, and I prefer to avoid platform specific setups as
>> > much as possible.  Why can't the CS polarity be encoded into the
>> > device tree so the driver *does* have the polarity data at probe time?
>>
>> We have the spi-cs-high property, but it duplicates compatible
>> property. 'compatible' is enough to tell whether some device has
>> cs-high or cs-low (device's driver knows that already).
>>
>> The problem is that SPI bus master doesn't know all the devices,
>> so it can't extract that information from the compatible string.
>> To workaround that we can use 'spi-cs-high', but that's ugly
>> workaround.
>>
>> SPI modes (0,1,2,3) is another matter, some devices can work in
>> several modes, so 'spi-cpol' and 'spi-cpha' are actually useful.
>>
> To get a sane initial state the needed GPIOs can be set to be inputs during
> the driver's initialization.
> This requires pull-up/pull-down resistors connected to the chip select
> lines. I think we can assume that they exist, because the GPIOs are all inputs
> after the controller's hardware reset and the resistors are needed to have a
> well-defined voltage level on the chip select lines. Normally the level is set
> so that the devices are disabled.
>
> Therefore, it doesn't matter if the firmware sets the GPIOs wrong.

No, that's just shifting responsibility from firmware to hardware.
There is just as much broken hardware out there as broken firmware.
The assumption cannot be made that the initial state of the pin is the
inactive state of the CS line.  Plus, some GPIO pins are output only
and the inital state cannot be read.

g.

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

Re: spi_mpc8xxx.c: chip select polarity problem

[Grant Likely]

On Thu, Nov 26, 2009 at 10:27 AM, Torsten Fleischer
<to-fleischer@t-online.de> wrote:
> On Thu, Nov 26, 2009 at 13:12:04 Anton Vorontsov wrote:
> [...]
>> > Ah. =A0I understand what you're doing now. =A0 Hmmm. =A0This approach
>> > concerns me because it relies on firmware or platform code to get CS
>> > gpios set up properly before the driver is probed.
>>
>> Yes, that was said at the very beginning of this thread.
>>
>> > =A0Firmware doesn't
>> > always get it right, and I prefer to avoid platform specific setups as
>> > much as possible. =A0Why can't the CS polarity be encoded into the
>> > device tree so the driver *does* have the polarity data at probe time?
>>
>> We have the spi-cs-high property, but it duplicates compatible
>> property. 'compatible' is enough to tell whether some device has
>> cs-high or cs-low (device's driver knows that already).
>>
>> The problem is that SPI bus master doesn't know all the devices,
>> so it can't extract that information from the compatible string.
>> To workaround that we can use 'spi-cs-high', but that's ugly
>> workaround.
>>
>> SPI modes (0,1,2,3) is another matter, some devices can work in
>> several modes, so 'spi-cpol' and 'spi-cpha' are actually useful.
>>
> To get a sane initial state the needed GPIOs can be set to be inputs duri=
ng
> the driver's initialization.
> This requires pull-up/pull-down resistors connected to the chip select
> lines. I think we can assume that they exist, because the GPIOs are all i=
nputs
> after the controller's hardware reset and the resistors are needed to hav=
e a
> well-defined voltage level on the chip select lines. Normally the level i=
s set
> so that the devices are disabled.
>
> Therefore, it doesn't matter if the firmware sets the GPIOs wrong.

No, that's just shifting responsibility from firmware to hardware.
There is just as much broken hardware out there as broken firmware.
The assumption cannot be made that the initial state of the pin is the
inactive state of the CS line.  Plus, some GPIO pins are output only
and the inital state cannot be read.

g.

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

Re: spi_mpc8xxx.c: chip select polarity problem

[Grant Likely]

On Thu, Nov 26, 2009 at 5:12 AM, Anton Vorontsov
<avorontsov@ru.mvista.com> wrote:
> On Wed, Nov 25, 2009 at 03:11:57PM -0700, Grant Likely wrote:
>> On Wed, Nov 25, 2009 at 1:41 PM, Torsten Fleischer
>> <to-fleischer@t-online.de> wrote:
>> > On Wen, Nov 25, 2009 at 01:33:57 Grant Likely wrote:
>> >> Thanks.  However, there needs to be a proper description of what this
>> >> patch does to go in the commit header.  Can you please write one?
>> >>
>> >> Thanks,
>> >> g.
>> >>
>> > [...]
>> >
>> > The initialization of the chip selects is removed from the probe() function of
>> > the spi_mpc8xxx driver, because the driver doesn't know the polarity of the
>> > chip selects of the SPI devices at the time of its initialization.
>> >
>> > For this reason the initialization of the several chip selects is postponed
>> > to the point of time when the very first SPI transfer to the associated device
>> > occurs.
>> >
>> >
>> > Signed-off-by: Torsten Fleischer <to-fleischer@t-online.de>
>>
>> Ah.  I understand what you're doing now.   Hmmm.  This approach
>> concerns me because it relies on firmware or platform code to get CS
>> gpios set up properly before the driver is probed.
>
> Yes, that was said at the very beginning of this thread.

I also came in part way through as I wasn't an SPI maintainer when
this thread started.  :-)

>>  Firmware doesn't
>> always get it right, and I prefer to avoid platform specific setups as
>> much as possible.  Why can't the CS polarity be encoded into the
>> device tree so the driver *does* have the polarity data at probe time?
>
> We have the spi-cs-high property, but it duplicates compatible
> property. 'compatible' is enough to tell whether some device has
> cs-high or cs-low (device's driver knows that already).

But the device's driver isn't controlling the CS line, the SPI bus
driver is.  Besides, there is no guarantee that all drivers will
actualy be loaded before something starts using SPI.  The bus driver
*must* know what the active state of each CS line is before activating
any devices.

The spi bus binding is deficient in this case.  A property (can be
optional) need to be added to the spi bus node to explicitly state the
CS polarities.  It's not entirely sane to look for a "spi-cs-high'
property in the spi device nodes because the SPI bus may not be fully
populated (ie. if a device happens to be sitting on the bus, but isn't
in the device tree yet).  Before any SPI transactions go out, it is
the responsibility of the bus driver to ensure all CS lines are in the
correct state.

> The problem is that SPI bus master doesn't know all the devices,
> so it can't extract that information from the compatible string.
> To workaround that we can use 'spi-cs-high', but that's ugly
> workaround.

It doesn't need to know about the devices, but is must know how all of
its CS lines behave.  So it isn't an really an ugly workaround, but I
do think the binding is insufficient for the SPI bus driver (see
below)

> SPI modes (0,1,2,3) is another matter, some devices can work in
> several modes, so 'spi-cpol' and 'spi-cpha' are actually useful.

yes.  spi-cpol and spi-cpha are actually properties of the device, and
belong in the spi device node, not the spi bus node.

The spi-cs-high property is defined in
Documentation/powerpc/dts-bindings/spi-bus.txt, but it definitely was
a mistake for the reasons I described above.  It does work in some
cases, but a property at the bus node would be better.  The driver can
still fallback to looking for spi-cs-high properties in child nodes.

Currently the spi-cs-high property is parsed in the
of_register_spi_devices() function, but the CS polarity needs to be
known before registering devices.  It needs to be factored out into
another utility function callable by spi bus drivers so that it can
get polarity data at probe time.

Cheers,
g.

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

Re: spi_mpc8xxx.c: chip select polarity problem

[Grant Likely]

On Thu, Nov 26, 2009 at 5:12 AM, Anton Vorontsov
<avorontsov@ru.mvista.com> wrote:
> On Wed, Nov 25, 2009 at 03:11:57PM -0700, Grant Likely wrote:
>> On Wed, Nov 25, 2009 at 1:41 PM, Torsten Fleischer
>> <to-fleischer@t-online.de> wrote:
>> > On Wen, Nov 25, 2009 at 01:33:57 Grant Likely wrote:
>> >> Thanks. =A0However, there needs to be a proper description of what th=
is
>> >> patch does to go in the commit header. =A0Can you please write one?
>> >>
>> >> Thanks,
>> >> g.
>> >>
>> > [...]
>> >
>> > The initialization of the chip selects is removed from the probe() fun=
ction of
>> > the spi_mpc8xxx driver, because the driver doesn't know the polarity o=
f the
>> > chip selects of the SPI devices at the time of its initialization.
>> >
>> > For this reason the initialization of the several chip selects is post=
poned
>> > to the point of time when the very first SPI transfer to the associate=
d device
>> > occurs.
>> >
>> >
>> > Signed-off-by: Torsten Fleischer <to-fleischer@t-online.de>
>>
>> Ah. =A0I understand what you're doing now. =A0 Hmmm. =A0This approach
>> concerns me because it relies on firmware or platform code to get CS
>> gpios set up properly before the driver is probed.
>
> Yes, that was said at the very beginning of this thread.

I also came in part way through as I wasn't an SPI maintainer when
this thread started.  :-)

>> =A0Firmware doesn't
>> always get it right, and I prefer to avoid platform specific setups as
>> much as possible. =A0Why can't the CS polarity be encoded into the
>> device tree so the driver *does* have the polarity data at probe time?
>
> We have the spi-cs-high property, but it duplicates compatible
> property. 'compatible' is enough to tell whether some device has
> cs-high or cs-low (device's driver knows that already).

But the device's driver isn't controlling the CS line, the SPI bus
driver is.  Besides, there is no guarantee that all drivers will
actualy be loaded before something starts using SPI.  The bus driver
*must* know what the active state of each CS line is before activating
any devices.

The spi bus binding is deficient in this case.  A property (can be
optional) need to be added to the spi bus node to explicitly state the
CS polarities.  It's not entirely sane to look for a "spi-cs-high'
property in the spi device nodes because the SPI bus may not be fully
populated (ie. if a device happens to be sitting on the bus, but isn't
in the device tree yet).  Before any SPI transactions go out, it is
the responsibility of the bus driver to ensure all CS lines are in the
correct state.

> The problem is that SPI bus master doesn't know all the devices,
> so it can't extract that information from the compatible string.
> To workaround that we can use 'spi-cs-high', but that's ugly
> workaround.

It doesn't need to know about the devices, but is must know how all of
its CS lines behave.  So it isn't an really an ugly workaround, but I
do think the binding is insufficient for the SPI bus driver (see
below)

> SPI modes (0,1,2,3) is another matter, some devices can work in
> several modes, so 'spi-cpol' and 'spi-cpha' are actually useful.

yes.  spi-cpol and spi-cpha are actually properties of the device, and
belong in the spi device node, not the spi bus node.

The spi-cs-high property is defined in
Documentation/powerpc/dts-bindings/spi-bus.txt, but it definitely was
a mistake for the reasons I described above.  It does work in some
cases, but a property at the bus node would be better.  The driver can
still fallback to looking for spi-cs-high properties in child nodes.

Currently the spi-cs-high property is parsed in the
of_register_spi_devices() function, but the CS polarity needs to be
known before registering devices.  It needs to be factored out into
another utility function callable by spi bus drivers so that it can
get polarity data at probe time.

Cheers,
g.

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

Re: spi_mpc8xxx.c: chip select polarity problem

[Anton Vorontsov]

On Thu, Nov 26, 2009 at 11:16:34AM -0700, Grant Likely wrote:
[...]
> The spi-cs-high property is defined in
> Documentation/powerpc/dts-bindings/spi-bus.txt, but it definitely was
> a mistake

Yup.

> Currently the spi-cs-high property is parsed in the
> of_register_spi_devices() function, but the CS polarity needs to be
> known before registering devices.  It needs to be factored out into
> another utility function callable by spi bus drivers so that it can
> get polarity data at probe time.

Untill we have this, Torsten's patch is a real improvement, and
works for non-broken hw/fw.

So I think it should be applied.

Thanks,

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

Re: spi_mpc8xxx.c: chip select polarity problem

[Grant Likely]

On Thu, Nov 26, 2009 at 11:41 AM, Anton Vorontsov
<avorontsov@ru.mvista.com> wrote:
> On Thu, Nov 26, 2009 at 11:16:34AM -0700, Grant Likely wrote:
> [...]
>> The spi-cs-high property is defined in
>> Documentation/powerpc/dts-bindings/spi-bus.txt, but it definitely was
>> a mistake
>
> Yup.
>
>> Currently the spi-cs-high property is parsed in the
>> of_register_spi_devices() function, but the CS polarity needs to be
>> known before registering devices.  It needs to be factored out into
>> another utility function callable by spi bus drivers so that it can
>> get polarity data at probe time.
>
> Untill we have this, Torsten's patch is a real improvement, and
> works for non-broken hw/fw.
>
> So I think it should be applied.

I disagree since it only band-aids the problem and uglifies the driver
in the process.  In the immediate term the driver needs to be changed
to read the spi-cs-high property out of the child nodes before
registering the devices.  I'm not going to apply this patch.

g.

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

Re: spi_mpc8xxx.c: chip select polarity problem

[Grant Likely]

On Thu, Nov 26, 2009 at 11:41 AM, Anton Vorontsov
<avorontsov@ru.mvista.com> wrote:
> On Thu, Nov 26, 2009 at 11:16:34AM -0700, Grant Likely wrote:
> [...]
>> The spi-cs-high property is defined in
>> Documentation/powerpc/dts-bindings/spi-bus.txt, but it definitely was
>> a mistake
>
> Yup.
>
>> Currently the spi-cs-high property is parsed in the
>> of_register_spi_devices() function, but the CS polarity needs to be
>> known before registering devices. =A0It needs to be factored out into
>> another utility function callable by spi bus drivers so that it can
>> get polarity data at probe time.
>
> Untill we have this, Torsten's patch is a real improvement, and
> works for non-broken hw/fw.
>
> So I think it should be applied.

I disagree since it only band-aids the problem and uglifies the driver
in the process.  In the immediate term the driver needs to be changed
to read the spi-cs-high property out of the child nodes before
registering the devices.  I'm not going to apply this patch.

g.

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

Re: spi_mpc8xxx.c: chip select polarity problem

[Anton Vorontsov]

On Thu, Nov 26, 2009 at 11:50:05AM -0700, Grant Likely wrote:
> On Thu, Nov 26, 2009 at 11:41 AM, Anton Vorontsov
> <avorontsov@ru.mvista.com> wrote:
> > On Thu, Nov 26, 2009 at 11:16:34AM -0700, Grant Likely wrote:
> > [...]
> >> The spi-cs-high property is defined in
> >> Documentation/powerpc/dts-bindings/spi-bus.txt, but it definitely was
> >> a mistake
> >
> > Yup.
> >
> >> Currently the spi-cs-high property is parsed in the
> >> of_register_spi_devices() function, but the CS polarity needs to be
> >> known before registering devices.  It needs to be factored out into
> >> another utility function callable by spi bus drivers so that it can
> >> get polarity data at probe time.
> >
> > Untill we have this, Torsten's patch is a real improvement, and
> > works for non-broken hw/fw.
> >
> > So I think it should be applied.
> 
> I disagree since it only band-aids the problem and uglifies the driver
> in the process.  In the immediate term the driver needs to be changed
> to read the spi-cs-high property out of the child nodes before
> registering the devices.

Hm. I thought we agreed that spi-cs-high is not good? Why do you
encourage using it then? We'll have to uglify the driver with
legacy device-tree handling code.

-- 
Anton Vorontsov
email: cbouatmailru@gmail.com
irc://irc.freenode.net/bd2
_______________________________________________
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

Re: spi_mpc8xxx.c: chip select polarity problem

[Anton Vorontsov]

On Thu, Nov 26, 2009 at 11:50:05AM -0700, Grant Likely wrote:
> On Thu, Nov 26, 2009 at 11:41 AM, Anton Vorontsov
> <avorontsov@ru.mvista.com> wrote:
> > On Thu, Nov 26, 2009 at 11:16:34AM -0700, Grant Likely wrote:
> > [...]
> >> The spi-cs-high property is defined in
> >> Documentation/powerpc/dts-bindings/spi-bus.txt, but it definitely was
> >> a mistake
> >
> > Yup.
> >
> >> Currently the spi-cs-high property is parsed in the
> >> of_register_spi_devices() function, but the CS polarity needs to be
> >> known before registering devices.  It needs to be factored out into
> >> another utility function callable by spi bus drivers so that it can
> >> get polarity data at probe time.
> >
> > Untill we have this, Torsten's patch is a real improvement, and
> > works for non-broken hw/fw.
> >
> > So I think it should be applied.
> 
> I disagree since it only band-aids the problem and uglifies the driver
> in the process.  In the immediate term the driver needs to be changed
> to read the spi-cs-high property out of the child nodes before
> registering the devices.

Hm. I thought we agreed that spi-cs-high is not good? Why do you
encourage using it then? We'll have to uglify the driver with
legacy device-tree handling code.

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

Re: spi_mpc8xxx.c: chip select polarity problem

[Grant Likely]

On Thu, Nov 26, 2009 at 12:01 PM, Anton Vorontsov
<avorontsov@ru.mvista.com> wrote:
> On Thu, Nov 26, 2009 at 11:50:05AM -0700, Grant Likely wrote:
>> On Thu, Nov 26, 2009 at 11:41 AM, Anton Vorontsov
>> <avorontsov@ru.mvista.com> wrote:
>> > On Thu, Nov 26, 2009 at 11:16:34AM -0700, Grant Likely wrote:
>> > [...]
>> >> The spi-cs-high property is defined in
>> >> Documentation/powerpc/dts-bindings/spi-bus.txt, but it definitely was
>> >> a mistake
>> >
>> > Yup.
>> >
>> >> Currently the spi-cs-high property is parsed in the
>> >> of_register_spi_devices() function, but the CS polarity needs to be
>> >> known before registering devices.  It needs to be factored out into
>> >> another utility function callable by spi bus drivers so that it can
>> >> get polarity data at probe time.
>> >
>> > Untill we have this, Torsten's patch is a real improvement, and
>> > works for non-broken hw/fw.
>> >
>> > So I think it should be applied.
>>
>> I disagree since it only band-aids the problem and uglifies the driver
>> in the process.  In the immediate term the driver needs to be changed
>> to read the spi-cs-high property out of the child nodes before
>> registering the devices.
>
> Hm. I thought we agreed that spi-cs-high is not good? Why do you
> encourage using it then? We'll have to uglify the driver with
> legacy device-tree handling code.

spi-cs-high is definitely not a complete solution, but it isn't
actively evil either.  Plus it is documented and (presumably) in
active use. so support for it should not be dropped.

Regardless, there needs to be a library function for parsing all the
SPI child nodes and returning the active state for each GPIO chip
select.  All the code for parsing the old spi-cs-high properties can
be contained in the same place as a new yet-to-be-defined bus node cs
polarity property.  The rework to the driver itself is not ugly.

g.

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

Re: spi_mpc8xxx.c: chip select polarity problem

[Grant Likely]

On Thu, Nov 26, 2009 at 12:01 PM, Anton Vorontsov
<avorontsov@ru.mvista.com> wrote:
> On Thu, Nov 26, 2009 at 11:50:05AM -0700, Grant Likely wrote:
>> On Thu, Nov 26, 2009 at 11:41 AM, Anton Vorontsov
>> <avorontsov@ru.mvista.com> wrote:
>> > On Thu, Nov 26, 2009 at 11:16:34AM -0700, Grant Likely wrote:
>> > [...]
>> >> The spi-cs-high property is defined in
>> >> Documentation/powerpc/dts-bindings/spi-bus.txt, but it definitely was
>> >> a mistake
>> >
>> > Yup.
>> >
>> >> Currently the spi-cs-high property is parsed in the
>> >> of_register_spi_devices() function, but the CS polarity needs to be
>> >> known before registering devices. =A0It needs to be factored out into
>> >> another utility function callable by spi bus drivers so that it can
>> >> get polarity data at probe time.
>> >
>> > Untill we have this, Torsten's patch is a real improvement, and
>> > works for non-broken hw/fw.
>> >
>> > So I think it should be applied.
>>
>> I disagree since it only band-aids the problem and uglifies the driver
>> in the process. =A0In the immediate term the driver needs to be changed
>> to read the spi-cs-high property out of the child nodes before
>> registering the devices.
>
> Hm. I thought we agreed that spi-cs-high is not good? Why do you
> encourage using it then? We'll have to uglify the driver with
> legacy device-tree handling code.

spi-cs-high is definitely not a complete solution, but it isn't
actively evil either.  Plus it is documented and (presumably) in
active use. so support for it should not be dropped.

Regardless, there needs to be a library function for parsing all the
SPI child nodes and returning the active state for each GPIO chip
select.  All the code for parsing the old spi-cs-high properties can
be contained in the same place as a new yet-to-be-defined bus node cs
polarity property.  The rework to the driver itself is not ugly.

g.

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

Re: spi_mpc8xxx.c: chip select polarity problem

[Torsten Fleischer]

On Thu, Nov 26, 2009 at 20:17:35,  Grant Likely wrote:
[...]
> spi-cs-high is definitely not a complete solution, but it isn't
> actively evil either.  Plus it is documented and (presumably) in
> active use. so support for it should not be dropped.
> 
> Regardless, there needs to be a library function for parsing all the
> SPI child nodes and returning the active state for each GPIO chip
> select.  All the code for parsing the old spi-cs-high properties can
> be contained in the same place as a new yet-to-be-defined bus node cs
> polarity property.  The rework to the driver itself is not ugly.
> 

The following patch adds a function to get the active state of the chip select
of a SPI device by looking for the 'spi-cs-high' property in the associated device
tree node.
This function is used by the spi_mpc8xxx driver to set a proper initial value
to the associated GPIOs.


Signed-off-by: Torsten Fleischer <to-fleischer@t-online.de>
---

diff -ruN linux-2.6.32_orig//drivers/of/of_spi.c linux-2.6.32/drivers/of/of_spi.c
--- linux-2.6.32_orig//drivers/of/of_spi.c	2009-12-03 04:51:21.000000000 +0100
+++ linux-2.6.32/drivers/of/of_spi.c	2009-12-09 12:37:01.000000000 +0100
@@ -10,6 +10,49 @@
 #include <linux/device.h>
 #include <linux/spi/spi.h>
 #include <linux/of_spi.h>
+#include <linux/errno.h>
+
+/**
+ * of_get_spi_cs_active_state - Gets the chip select's active state of a SPI
+ * child devices.
+ * @np:        parent node of the SPI device nodes
+ * @index:     index/address of the SPI device (refers to the 'reg' property)
+ * @cs_active: pointer to return the chip select's active state
+ *             (true = active high; false = active low)
+ *
+ * Returns 0 on success; negative errno on failure
+ */
+int of_get_spi_cs_active_state(struct device_node *np, int index, bool *cs_active)
+{
+	struct device_node *child;
+	const int *prop;
+	int len;
+	bool active = 0;
+
+	/* search for the matching SPI device */
+	for_each_child_of_node(np, child) {
+		prop = of_get_property(child, "reg", &len);
+		if (!prop || len < sizeof(*prop)) {
+			/* property 'reg' not available (not an error) */
+			continue;
+		}
+
+		if ( *prop == index ) {
+			/* matching device found */
+			if (of_find_property(child, "spi-cs-high", NULL))
+				active = 1;
+
+			if (cs_active)
+				*cs_active = active;
+
+			return 0;
+		}
+	}
+
+	return -ENODEV;
+}
+EXPORT_SYMBOL(of_get_spi_cs_active_state);
+
 
 /**
  * of_register_spi_devices - Register child devices onto the SPI bus
diff -ruN linux-2.6.32_orig//drivers/spi/spi_mpc8xxx.c linux-2.6.32/drivers/spi/spi_mpc8xxx.c
--- linux-2.6.32_orig//drivers/spi/spi_mpc8xxx.c	2009-12-03 04:51:21.000000000 +0100
+++ linux-2.6.32/drivers/spi/spi_mpc8xxx.c	2009-12-09 12:50:36.000000000 +0100
@@ -705,6 +705,7 @@
 	for (; i < ngpios; i++) {
 		int gpio;
 		enum of_gpio_flags flags;
+		bool astate;
 
 		gpio = of_get_gpio_flags(np, i, &flags);
 		if (!gpio_is_valid(gpio)) {
@@ -721,8 +722,15 @@
 		pinfo->gpios[i] = gpio;
 		pinfo->alow_flags[i] = flags & OF_GPIO_ACTIVE_LOW;
 
+		ret = of_get_spi_cs_active_state(np, i, &astate);
+		if (ret) {
+			dev_err(dev, "can't get cs active state of device "
+				"#%d: %d\n", i, ret);
+			goto err_loop;
+		}
+
 		ret = gpio_direction_output(pinfo->gpios[i],
-					    pinfo->alow_flags[i]);
+					    pinfo->alow_flags[i] ^ !astate);
 		if (ret) {
 			dev_err(dev, "can't set output direction for gpio "
 				"#%d: %d\n", i, ret);
diff -ruN linux-2.6.32_orig//include/linux/of_spi.h linux-2.6.32/include/linux/of_spi.h
--- linux-2.6.32_orig//include/linux/of_spi.h	2009-12-03 04:51:21.000000000 +0100
+++ linux-2.6.32/include/linux/of_spi.h	2009-12-09 12:36:48.000000000 +0100
@@ -15,4 +15,7 @@
 extern void of_register_spi_devices(struct spi_master *master,
 				    struct device_node *np);
 
+extern int of_get_spi_cs_active_state(struct device_node *np,
+				      int index, bool *cs_active);
+
 #endif /* __LINUX_OF_SPI */
 

Re: spi_mpc8xxx.c: chip select polarity problem

[Grant Likely]

On Wed, Dec 9, 2009 at 8:49 AM, Torsten Fleischer
<to-fleischer@t-online.de> wrote:
> On Thu, Nov 26, 2009 at 20:17:35, =A0Grant Likely wrote:
> [...]
>> spi-cs-high is definitely not a complete solution, but it isn't
>> actively evil either. =A0Plus it is documented and (presumably) in
>> active use. so support for it should not be dropped.
>>
>> Regardless, there needs to be a library function for parsing all the
>> SPI child nodes and returning the active state for each GPIO chip
>> select. =A0All the code for parsing the old spi-cs-high properties can
>> be contained in the same place as a new yet-to-be-defined bus node cs
>> polarity property. =A0The rework to the driver itself is not ugly.
>>
>
> The following patch adds a function to get the active state of the chip s=
elect
> of a SPI device by looking for the 'spi-cs-high' property in the associat=
ed device
> tree node.
> This function is used by the spi_mpc8xxx driver to set a proper initial v=
alue
> to the associated GPIOs.
>
>
> Signed-off-by: Torsten Fleischer <to-fleischer@t-online.de>

I like this better.  See comments below.

> ---
>
> diff -ruN linux-2.6.32_orig//drivers/of/of_spi.c linux-2.6.32/drivers/of/=
of_spi.c
> --- linux-2.6.32_orig//drivers/of/of_spi.c =A0 =A0 =A02009-12-03 04:51:21=
.000000000 +0100
> +++ linux-2.6.32/drivers/of/of_spi.c =A0 =A02009-12-09 12:37:01.000000000=
 +0100
> @@ -10,6 +10,49 @@
> =A0#include <linux/device.h>
> =A0#include <linux/spi/spi.h>
> =A0#include <linux/of_spi.h>
> +#include <linux/errno.h>
> +
> +/**
> + * of_get_spi_cs_active_state - Gets the chip select's active state of a=
 SPI
> + * child devices.
> + * @np: =A0 =A0 =A0 =A0parent node of the SPI device nodes
> + * @index: =A0 =A0 index/address of the SPI device (refers to the 'reg' =
property)
> + * @cs_active: pointer to return the chip select's active state
> + * =A0 =A0 =A0 =A0 =A0 =A0 (true =3D active high; false =3D active low)
> + *
> + * Returns 0 on success; negative errno on failure
> + */
> +int of_get_spi_cs_active_state(struct device_node *np, int index, bool *=
cs_active)
> +{
> + =A0 =A0 =A0 struct device_node *child;
> + =A0 =A0 =A0 const int *prop;
> + =A0 =A0 =A0 int len;
> + =A0 =A0 =A0 bool active =3D 0;
> +
> + =A0 =A0 =A0 /* search for the matching SPI device */
> + =A0 =A0 =A0 for_each_child_of_node(np, child) {
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 prop =3D of_get_property(child, "reg", &len=
);
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 if (!prop || len < sizeof(*prop)) {
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 /* property 'reg' not avail=
able (not an error) */
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 continue;
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 }
> +
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 if ( *prop =3D=3D index ) {
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 /* matching device found */
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 if (of_find_property(child,=
 "spi-cs-high", NULL))
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 active =3D =
1;
> +
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 if (cs_active)
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 *cs_active =
=3D active;

A little odd.  If cs_active is NULL, then this routine does nothing,
and the caller is entirely defective.  Either test at the top of the
function or not at all.  Then *cs_active can be assigned directly.
But even then, this function will probably need to be reworked (see
comments below).

> +
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 return 0;
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 }
> + =A0 =A0 =A0 }
> +
> + =A0 =A0 =A0 return -ENODEV;
> +}
> +EXPORT_SYMBOL(of_get_spi_cs_active_state);
> +
>
> =A0/**
> =A0* of_register_spi_devices - Register child devices onto the SPI bus
> diff -ruN linux-2.6.32_orig//drivers/spi/spi_mpc8xxx.c linux-2.6.32/drive=
rs/spi/spi_mpc8xxx.c
> --- linux-2.6.32_orig//drivers/spi/spi_mpc8xxx.c =A0 =A0 =A0 =A02009-12-0=
3 04:51:21.000000000 +0100
> +++ linux-2.6.32/drivers/spi/spi_mpc8xxx.c =A0 =A0 =A02009-12-09 12:50:36=
.000000000 +0100
> @@ -705,6 +705,7 @@
> =A0 =A0 =A0 =A0for (; i < ngpios; i++) {
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0int gpio;
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0enum of_gpio_flags flags;
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 bool astate;
>
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0gpio =3D of_get_gpio_flags(np, i, &flags);
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0if (!gpio_is_valid(gpio)) {
> @@ -721,8 +722,15 @@
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0pinfo->gpios[i] =3D gpio;
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0pinfo->alow_flags[i] =3D flags & OF_GPIO_A=
CTIVE_LOW;
>
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 ret =3D of_get_spi_cs_active_state(np, i, &=
astate);
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 if (ret) {
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 dev_err(dev, "can't get cs =
active state of device "
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 "#%d: %d\n"=
, i, ret);
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 goto err_loop;
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 }

This is a bit heavy handed in that it expects the device tree to be
fully populated with all SPI devices which isn't always a given.  For
example a board that has some unpopulated SPI devices could have some
gaps in the GPIO CS layout.  If a node can't be found, then just
ignore it silently and move on to the next.  I'd do something like
this:

+               astate =3D of_get_spi_cs_active_state(np, i);
=A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0ret =3D gpio_direction_output(pinfo->gpios[i=
],
- =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =
=A0 =A0 =A0 pinfo->alow_flags[i]);
+ =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =
=A0 =A0 =A0 pinfo->alow_flags[i] ^ !astate);

BTW, why the xor?  The usage is non-obvious enough that I'd like to
see a comment describing the use case.

g.

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

Re: spi_mpc8xxx.c: chip select polarity problem

[Torsten Fleischer]

On Wed, Dec 9, 2009 at 18:46:51 Grant Likely wrote:
[...]
> > +               ret = of_get_spi_cs_active_state(np, i, &astate);
> > +               if (ret) {
> > +                       dev_err(dev, "can't get cs active state of device
> > " +                               "#%d: %d\n", i, ret);
> > +                       goto err_loop;
> > +               }
> 
> This is a bit heavy handed in that it expects the device tree to be
> fully populated with all SPI devices which isn't always a given.  For
> example a board that has some unpopulated SPI devices could have some
> gaps in the GPIO CS layout.  If a node can't be found, then just
> ignore it silently and move on to the next.  I'd do something like
> this:
> 
> +               astate = of_get_spi_cs_active_state(np, i);

What should be returned if the node can't be found, 'true' or 'false? 

Maybe its better to do the following:

+               ret = of_get_spi_cs_active_state(np, i, &astate);
+               if (ret) {
+                       /* Device node not found */
+                       continue;
+               }

>                ret = gpio_direction_output(pinfo->gpios[i],
> -                                           pinfo->alow_flags[i]);
> +                                           pinfo->alow_flags[i] ^ !astate);
> 
> BTW, why the xor?  The usage is non-obvious enough that I'd like to
> see a comment describing the use case.
 
If I understand it right, the alow_flags describe the wiring. If set to 0 the 
wiring is non-inverted, if set to 1 its inverted respectively. To take this 
into account the active state has to be xor'd with the appropriate alow_flag.
 

Re: spi_mpc8xxx.c: chip select polarity problem

[Torsten Fleischer]

On Wed, Dec 9, 2009 at 18:46:51 Grant Likely wrote:
> <to-fleischer@t-online.de> wrote:
> [...]
> > The following patch adds a function to get the active state of the chip
> > select of a SPI device by looking for the 'spi-cs-high' property in the
> > associated device tree node.
> > This function is used by the spi_mpc8xxx driver to set a proper initial
> > value to the associated GPIOs.
> >
> >
> > Signed-off-by: Torsten Fleischer <to-fleischer@t-online.de>
> 
> I like this better.  See comments below.
> 
[...]

Hey Grant,

below there is a new version of the patch containing the modifications
according to your comments.

Thanks for the hints,
Torsten


Signed-off-by: Torsten Fleischer <to-fleischer@t-online.de>
---

diff -ruN linux-2.6.32_orig//drivers/of/of_spi.c linux-2.6.32/drivers/of/of_spi.c
--- linux-2.6.32_orig//drivers/of/of_spi.c	2009-12-03 04:51:21.000000000 +0100
+++ linux-2.6.32/drivers/of/of_spi.c	2009-12-11 17:57:22.016787665 +0100
@@ -12,6 +12,43 @@
 #include <linux/of_spi.h>
 
 /**
+ * of_get_spi_cs_active_state - Gets the chip select's active state of a SPI
+ * child devices.
+ * @np:        parent node of the SPI device nodes
+ * @index:     index/address of the SPI device (refers to the 'reg' property)
+ *
+ * Returns 0 for 'active low' or if the child's node can't be found.
+ * Otherwise it returns 1 ('active high').
+ */
+bool of_get_spi_cs_active_state(struct device_node *np, int index)
+{
+	struct device_node *child;
+	const int *prop;
+	int len;
+	bool active = 0;
+
+	/* search for the matching SPI device */
+	for_each_child_of_node(np, child) {
+		prop = of_get_property(child, "reg", &len);
+		if (!prop || len < sizeof(*prop)) {
+			/* property 'reg' not available (not an error) */
+			continue;
+		}
+
+		if ( *prop == index ) {
+			/* matching device found */
+			if (of_find_property(child, "spi-cs-high", NULL))
+				active = 1;
+			break;
+		}
+	}
+
+	return active;
+}
+EXPORT_SYMBOL(of_get_spi_cs_active_state);
+
+
+/**
  * of_register_spi_devices - Register child devices onto the SPI bus
  * @master:	Pointer to spi_master device
  * @np:		parent node of SPI device nodes
diff -ruN linux-2.6.32_orig//drivers/spi/spi_mpc8xxx.c linux-2.6.32/drivers/spi/spi_mpc8xxx.c
--- linux-2.6.32_orig//drivers/spi/spi_mpc8xxx.c	2009-12-03 04:51:21.000000000 +0100
+++ linux-2.6.32/drivers/spi/spi_mpc8xxx.c	2009-12-11 17:15:47.957379333 +0100
@@ -705,6 +705,7 @@
 	for (; i < ngpios; i++) {
 		int gpio;
 		enum of_gpio_flags flags;
+		bool astate;
 
 		gpio = of_get_gpio_flags(np, i, &flags);
 		if (!gpio_is_valid(gpio)) {
@@ -721,8 +722,18 @@
 		pinfo->gpios[i] = gpio;
 		pinfo->alow_flags[i] = flags & OF_GPIO_ACTIVE_LOW;
 
+		/* get the active state of the SPI device */
+		astate = of_get_spi_cs_active_state(np, i);
+
+		/*
+		 *  alow_flags describe the wiring of the chip select
+		 *  (0 = non-inverted, 1 = inverted); this must be taken into
+		 *  account when setting the GPIO's initial value
+		 *  (see also mpc8xxx_spi_cs_control()); note that astate will
+		 *  be 0 for active low and 1 for active high respectively
+		 */
 		ret = gpio_direction_output(pinfo->gpios[i],
-					    pinfo->alow_flags[i]);
+					    pinfo->alow_flags[i] ^ !astate);
 		if (ret) {
 			dev_err(dev, "can't set output direction for gpio "
 				"#%d: %d\n", i, ret);
diff -ruN linux-2.6.32_orig//include/linux/of_spi.h linux-2.6.32/include/linux/of_spi.h
--- linux-2.6.32_orig//include/linux/of_spi.h	2009-12-03 04:51:21.000000000 +0100
+++ linux-2.6.32/include/linux/of_spi.h	2009-12-11 17:10:21.681380685 +0100
@@ -15,4 +15,7 @@
 extern void of_register_spi_devices(struct spi_master *master,
 				    struct device_node *np);
 
+extern bool of_get_spi_cs_active_state(struct device_node *np,
+					   int index);
+
 #endif /* __LINUX_OF_SPI */