Re: [spi-devel-general] [PATCH] spi: Allow using spi_bitbang_setup() with custom txrx_bufs()
On Thu, Nov 26, 2009 at 4:21 PM, Grant Likely wrote:
> On Wed, Nov 25, 2009 at 11:47 PM, Magnus Damm wrote:
>> On Thu, Nov 26, 2009 at 7:15 AM, Grant Likely
>> wrote:
>>> On Wed, Nov 25, 2009 at 1:26 AM, Magnus Damm wrote:
/* per-word shift register access, in hardware or bitbanging */
- cs->txrx_word = bitbang->txrx_word[spi->mode &
(SPI_CPOL|SPI_CPHA)];
- if (!cs->txrx_word)
- return -EINVAL;
+ if (bitbang->txrx_bufs == spi_bitbang_bufs) {
+ cs->txrx_word = bitbang->txrx_word[spi->mode & mode_mask];
+ if (!cs->txrx_word)
+ return -EINVAL;
+ }
>>>
>>> Hmmm... this smells like an ugly hack to me. It seems to me that if
>>> some bitbang backend drivers don't want this code, then it should be
>>> encoded into a callback so it can be overridden. Thoughts.
>>
>> Yeah, it's far from clean. I want to make use of spi_bitbang_setup()
>> in my MSIOF driver, but I want to avoid dummy txtx_word[] callbacks
>> that will be unused since i'm using a driver specific
>> bitbang->txrx_bufs function.
>>
>> I guess the attached patch is slightly cleaner? I like the idea of
>> letting bitbang drivers use shared code for
>> spi_bitbang_setup()/spi_bitbang_cleanup() with their private
>> setup_transfer() function which in turn calls
>> spi_bitbang_setup_transfer(). My impression is that there's quite a
>> bit of duplicated setup()/cleanup() code.
>
> This is certainly less ugly. But with the points brought up in the
> other thread, I want to have a close look at spi-bitbang before I
> start applying stuff. It seems nasty. Give me a few days.
Sure, I plan on posting a V2 of the MSIOF driver. I plan to keep the
dummy txrx_word() function for now - this to disconnect the cleanup
from the driver integration, hope that sounds like a good plan.
Cheers,
/ magnus
--
Let Crystal Reports handle the reporting - Free Crystal Reports 2008 30-Day
trial. Simplify your report design, integration and deployment - and focus on
what you do best, core application coding. Discover what's new with
Crystal Reports now. http://p.sf.net/sfu/bobj-july
___
spi-devel-general mailing list
spi-devel-general@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/spi-devel-general
[spi-devel-general] [SPAM] Plus de confort et Plus d'economies pour vous rechauffer
-- Let Crystal Reports handle the reporting - Free Crystal Reports 2008 30-Day trial. Simplify your report design, integration and deployment - and focus on what you do best, core application coding. Discover what's new with Crystal Reports now. http://p.sf.net/sfu/bobj-july___ spi-devel-general mailing list spi-devel-general@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/spi-devel-general
Re: [spi-devel-general] [PATCH] spi: Allow using spi_bitbang_setup() with custom txrx_bufs()
On Wed, Nov 25, 2009 at 11:47 PM, Magnus Damm wrote:
> Hi Grant,
>
> On Thu, Nov 26, 2009 at 7:15 AM, Grant Likely
> wrote:
>> On Wed, Nov 25, 2009 at 1:26 AM, Magnus Damm wrote:
>>> /* per-word shift register access, in hardware or bitbanging */
>>> - cs->txrx_word = bitbang->txrx_word[spi->mode & (SPI_CPOL|SPI_CPHA)];
>>> - if (!cs->txrx_word)
>>> - return -EINVAL;
>>> + if (bitbang->txrx_bufs == spi_bitbang_bufs) {
>>> + cs->txrx_word = bitbang->txrx_word[spi->mode & mode_mask];
>>> + if (!cs->txrx_word)
>>> + return -EINVAL;
>>> + }
>>
>> Hmmm... this smells like an ugly hack to me. It seems to me that if
>> some bitbang backend drivers don't want this code, then it should be
>> encoded into a callback so it can be overridden. Thoughts.
>
> Yeah, it's far from clean. I want to make use of spi_bitbang_setup()
> in my MSIOF driver, but I want to avoid dummy txtx_word[] callbacks
> that will be unused since i'm using a driver specific
> bitbang->txrx_bufs function.
>
> I guess the attached patch is slightly cleaner? I like the idea of
> letting bitbang drivers use shared code for
> spi_bitbang_setup()/spi_bitbang_cleanup() with their private
> setup_transfer() function which in turn calls
> spi_bitbang_setup_transfer(). My impression is that there's quite a
> bit of duplicated setup()/cleanup() code.
This is certainly less ugly. But with the points brought up in the
other thread, I want to have a close look at spi-bitbang before I
start applying stuff. It seems nasty. Give me a few days.
g.
--
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.
--
Let Crystal Reports handle the reporting - Free Crystal Reports 2008 30-Day
trial. Simplify your report design, integration and deployment - and focus on
what you do best, core application coding. Discover what's new with
Crystal Reports now. http://p.sf.net/sfu/bobj-july
___
spi-devel-general mailing list
spi-devel-general@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/spi-devel-general
Re: [spi-devel-general] [PATCH] spi: SuperH MSIOF SPI Master driver
On Thu, 26 Nov 2009 15:43:16 +0900 Paul Mundt wrote:
> > > +static void sh_msiof_modify_ctr_wait(struct sh_msiof_spi_priv *p,
> > > + unsigned long clr, unsigned long set)
> > > +{
> > > + unsigned long mask = clr | set;
> > > + unsigned long data;
> > > +
> > > + data = sh_msiof_read(p, CTR);
> > > + data &= ~clr;
> > > + data |= set;
> > > + sh_msiof_write(p, CTR, data);
> > > +
> > > + while ((sh_msiof_read(p, CTR) & mask) != set)
> > > + ;
> >
> > hm, confidence. No timeout needed here?
> >
> This definitely needs a timeout, nothing involving SPI inspires
> confidence. A cpu_relax() to prevent the compiler from optimizing the
> loop out would help, too.
We generally don't bother with the relax in an IO polling loop
like this. It involves an IO read/write which the compiler cannot fiddle
with and I believe that CPUs will generally take the opportunity to have
a little snooze while the slow IO operation is happening.
Can't hurt though ;)
--
Let Crystal Reports handle the reporting - Free Crystal Reports 2008 30-Day
trial. Simplify your report design, integration and deployment - and focus on
what you do best, core application coding. Discover what's new with
Crystal Reports now. http://p.sf.net/sfu/bobj-july
___
spi-devel-general mailing list
spi-devel-general@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/spi-devel-general
Re: [spi-devel-general] [PATCH] spi: SuperH MSIOF SPI Master driver
On Wed, Nov 25, 2009 at 12:11:52PM -0800, Andrew Morton wrote:
> On Tue, 24 Nov 2009 21:55:31 +0900
> Magnus Damm wrote:
> > +struct sh_msiof_spi_priv {
> > + struct spi_bitbang bitbang; /* must be first for spi_bitbang.c */
>
> Well if that's the case then spi_bitbang.c needs smacking. What causes
> this requirement?
>
spi_bitbang causes this requirement by being lazy with regards to
private data stashing. Both the drivers and the bitbang code use
spi_master_get_devdata() for getting their private data, which wraps
in to a dev_get_drvdata().
This needs to be reworked so that struct spi_master has its own private
data pointer which helper code like spi_bitbang can use, while freeing up
the struct device private data pointer so it can be freely used by
drivers as normal.
This is the same sort of private data through casting whimsy that the
framebuffer drivers used to employ, only fortunately that behaviour never
made it out of 2.3.x kernels -- until now!
> > + void __iomem *mapbase;
> > + struct clk *clk;
> > + struct platform_device *pdev;
> > + struct sh_msiof_spi_info *info;
> > + struct completion done;
> > + unsigned long flags;
> > + int tx_fifo_size;
> > + int rx_fifo_size;
> > +};
> > +
> >
> > ...
> >
> > +static void sh_msiof_modify_ctr_wait(struct sh_msiof_spi_priv *p,
> > +unsigned long clr, unsigned long set)
> > +{
> > + unsigned long mask = clr | set;
> > + unsigned long data;
> > +
> > + data = sh_msiof_read(p, CTR);
> > + data &= ~clr;
> > + data |= set;
> > + sh_msiof_write(p, CTR, data);
> > +
> > + while ((sh_msiof_read(p, CTR) & mask) != set)
> > + ;
>
> hm, confidence. No timeout needed here?
>
This definitely needs a timeout, nothing involving SPI inspires
confidence. A cpu_relax() to prevent the compiler from optimizing the
loop out would help, too.
--
Let Crystal Reports handle the reporting - Free Crystal Reports 2008 30-Day
trial. Simplify your report design, integration and deployment - and focus on
what you do best, core application coding. Discover what's new with
Crystal Reports now. http://p.sf.net/sfu/bobj-july
___
spi-devel-general mailing list
spi-devel-general@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/spi-devel-general
Re: [spi-devel-general] [PATCH] spi: Allow using spi_bitbang_setup() with custom txrx_bufs()
Hi Grant,
On Thu, Nov 26, 2009 at 7:15 AM, Grant Likely wrote:
> On Wed, Nov 25, 2009 at 1:26 AM, Magnus Damm wrote:
>> This patch modifies the shared spi bitbanging code
>> to allow using spi_bitbang_setup() even though the
>> txrx_word[] callbacks are unset. Useful for drivers
>> that want to make use of spi_bitbang_setup() but
>> have their own txrx_bufs() callback.
>> @@ -196,9 +205,11 @@ int spi_bitbang_setup(struct spi_device
>> }
>>
>> /* per-word shift register access, in hardware or bitbanging */
>> - cs->txrx_word = bitbang->txrx_word[spi->mode & (SPI_CPOL|SPI_CPHA)];
>> - if (!cs->txrx_word)
>> - return -EINVAL;
>> + if (bitbang->txrx_bufs == spi_bitbang_bufs) {
>> + cs->txrx_word = bitbang->txrx_word[spi->mode & mode_mask];
>> + if (!cs->txrx_word)
>> + return -EINVAL;
>> + }
>
> Hmmm... this smells like an ugly hack to me. It seems to me that if
> some bitbang backend drivers don't want this code, then it should be
> encoded into a callback so it can be overridden. Thoughts.
Yeah, it's far from clean. I want to make use of spi_bitbang_setup()
in my MSIOF driver, but I want to avoid dummy txtx_word[] callbacks
that will be unused since i'm using a driver specific
bitbang->txrx_bufs function.
I guess the attached patch is slightly cleaner? I like the idea of
letting bitbang drivers use shared code for
spi_bitbang_setup()/spi_bitbang_cleanup() with their private
setup_transfer() function which in turn calls
spi_bitbang_setup_transfer(). My impression is that there's quite a
bit of duplicated setup()/cleanup() code.
/ magnus
linux-2.6.33-pre-spi-word-mode-setup-20091126.patch
Description: Binary data
--
Let Crystal Reports handle the reporting - Free Crystal Reports 2008 30-Day
trial. Simplify your report design, integration and deployment - and focus on
what you do best, core application coding. Discover what's new with
Crystal Reports now. http://p.sf.net/sfu/bobj-july___
spi-devel-general mailing list
spi-devel-general@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/spi-devel-general
Re: [spi-devel-general] [PATCH v2]: NUC900: Add spi driver support for nuc900
Hi Andrew
How about this spi driver, Could it be applied?
2009/11/20 Wan ZongShun :
> Dear sirs,
>
> Thanks a lot for your help,and I fix the spi driver and re-submit it.
>
>
> Signed-off-by: Wan ZongShun
>
> ---
> arch/arm/mach-w90x900/include/mach/nuc900_spi.h | 35 ++
> drivers/spi/Kconfig | 7 +
> drivers/spi/Makefile | 1 +
> drivers/spi/spi_nuc900.c | 509
> +++
> 4 files changed, 552 insertions(+), 0 deletions(-)
> create mode 100644 arch/arm/mach-w90x900/include/mach/nuc900_spi.h
> create mode 100644 drivers/spi/spi_nuc900.c
>
> diff --git a/arch/arm/mach-w90x900/include/mach/nuc900_spi.h
> b/arch/arm/mach-w90x900/include/mach/nuc900_spi.h
> new file mode 100644
> index 000..24ea23b
> --- /dev/null
> +++ b/arch/arm/mach-w90x900/include/mach/nuc900_spi.h
> @@ -0,0 +1,35 @@
> +/*
> + * arch/arm/mach-w90x900/include/mach/nuc900_spi.h
> + *
> + * Copyright (c) 2009 Nuvoton technology corporation.
> + *
> + * Wan ZongShun
> + *
> + * 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;version 2 of the License.
> + *
> + */
> +
> +#ifndef __ASM_ARCH_SPI_H
> +#define __ASM_ARCH_SPI_H
> +
> +extern void mfp_set_groupg(struct device *dev);
> +
> +struct w90p910_spi_info {
> + unsigned int num_cs;
> + unsigned int lsb;
> + unsigned int txneg;
> + unsigned int rxneg;
> + unsigned int divider;
> + unsigned int sleep;
> + unsigned int txnum;
> + unsigned int txbitlen;
> + int bus_num;
> +};
> +
> +struct w90p910_spi_chip {
> + unsigned char bits_per_word;
> +};
> +
> +#endif /* __ASM_ARCH_SPI_H */
> diff --git a/drivers/spi/Kconfig b/drivers/spi/Kconfig
> index 4b6f7cb..2e1b20c 100644
> --- a/drivers/spi/Kconfig
> +++ b/drivers/spi/Kconfig
> @@ -244,6 +244,13 @@ config SPI_XILINX
> See the "OPB Serial Peripheral Interface (SPI) (v1.00e)"
> Product Specification document (DS464) for hardware details.
>
> +config SPI_NUC900
> + tristate "Nuvoton NUC900 series SPI"
> + depends on ARCH_W90X900 && EXPERIMENTAL
> + select SPI_BITBANG
> + help
> + SPI driver for Nuvoton NUC900 series ARM SoCs
> +
> #
> # Add new SPI master controllers in alphabetical order above this line
> #
> diff --git a/drivers/spi/Makefile b/drivers/spi/Makefile
> index 21a1182..694a4cb 100644
> --- a/drivers/spi/Makefile
> +++ b/drivers/spi/Makefile
> @@ -33,6 +33,7 @@ obj-$(CONFIG_SPI_TXX9) += spi_txx9.o
> obj-$(CONFIG_SPI_XILINX) += xilinx_spi.o
> obj-$(CONFIG_SPI_SH_SCI) += spi_sh_sci.o
> obj-$(CONFIG_SPI_STMP3XXX) += spi_stmp.o
> +obj-$(CONFIG_SPI_NUC900) += spi_nuc900.o
> # ... add above this line ...
>
> # SPI protocol drivers (device/link on bus)
> diff --git a/drivers/spi/spi_nuc900.c b/drivers/spi/spi_nuc900.c
> new file mode 100644
> index 000..6836fd1
> --- /dev/null
> +++ b/drivers/spi/spi_nuc900.c
> @@ -0,0 +1,509 @@
> +/* linux/drivers/spi/spi_nuc900.c
> + *
> + * Copyright (c) 2009 Nuvoton technology.
> + * Wan ZongShun
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + *
> +*/
> +
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +
> +#include
> +#include
> +
> +#include
> +
> +/* usi registers offset */
> +#define USI_CNT 0x00
> +#define USI_DIV 0x04
> +#define USI_SSR 0x08
> +#define USI_RX0 0x10
> +#define USI_TX0 0x10
> +
> +/* usi register bit */
> +#define ENINT (0x01 << 17)
> +#define ENFLG (0x01 << 16)
> +#define TXNUM (0x03 << 8)
> +#define TXNEG (0x01 << 2)
> +#define RXNEG (0x01 << 1)
> +#define LSB (0x01 << 10)
> +#define SELECTLEV (0x01 << 2)
> +#define SELECTPOL (0x01 << 31)
> +#define SELECTSLAVE 0x01
> +#define GOBUSY 0x01
> +
> +struct w90p910_spi {
> + struct spi_bitbang bitbang;
> + struct completion done;
> + void __iomem *regs;
> + int irq;
> + int len;
> + int count;
> + const unsigned char *tx;
> + unsigned char *rx;
> + struct clk *clk;
> + struct resource *ioarea;
> + struct spi_master *master;
> + struct spi_device *curdev;
> + struct device *dev;
> + struct w90p910_spi_info *pdata;
> + spinlock_t lock;
> +};
Re: [spi-devel-general] [PATCH] spi: SuperH MSIOF SPI Master driver
Hi Andrew,
On Thu, Nov 26, 2009 at 5:11 AM, Andrew Morton
wrote:
> On Tue, 24 Nov 2009 21:55:31 +0900
> Magnus Damm wrote:
>> This patch adds SPI Master support for the SuperH MSIOF
>> --- 0001/drivers/spi/spi.c
>> +++ work/drivers/spi/spi.c 2009-11-24 20:39:48.0 +0900
>> @@ -17,7 +17,6 @@
>> * along with this program; if not, write to the Free Software
>> * Foundation, Inc., 675 Mass Ave, Cambridge, MA 02139, USA.
>> */
>> -
>> #include
>> #include
>> #include
>
> whoops?
Yeah, i totally missed that one.
>> --- /dev/null
>> +++ work/drivers/spi/spi_sh_msiof.c 2009-11-24 20:39:49.0 +0900
>> @@ -0,0 +1,675 @@
>> +/*
>> + * SuperH MSIOF SPI Master Interface
>> + *
>> + * Copyright (c) 2009 Magnus Damm
>> + *
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License version 2 as
>> + * published by the Free Software Foundation.
>> + *
>> + */
>> +
>> +#include
>
> But not consistently.
Never. =)
>> +#include
>> +#include
>> +#include
>> +#include
>> +#include
>> +#include
>> +#include
>> +#include
>> +#include
>> +#include
>> +
>> +#include
>> +#include
>> +#include
>> +
>> +#include
>> +#include
>> +
>> +struct sh_msiof_spi_priv {
>> + struct spi_bitbang bitbang; /* must be first for spi_bitbang.c */
>
> Well if that's the case then spi_bitbang.c needs smacking. What causes
> this requirement?
The spi_bitbang.c functions spi_bitbang_setup() and
spi_bitbang_transfer() use spi_master_get_devdata() to get the bitbang
data structure. Most drivers want to be able to get a pointer to
private data using spi_master_get_devdata(), so the
bitbang-structure-as-first-member trick is used so the driver code and
the bitbang functions can coexist.
A couple of in-tree drivers do the same thing and store the bitbang
structure as the first member. For instance, have a look at
omap_uwire.c and spi_gpio.c.
Maybe adding a spi_alloc_bitbang_master() function to spi_bitbang.c
that does the bitbang structure allocation behind the scenes would be
a good thing. The bitbang drivers can then use that function instead
of spi_alloc_master(). The bitbang code itself can retrieve the
bitbang structure by doing offset calculations from the master
pointer. That would free up the private pointer.
There may be better ways to do this though...
>> +static void sh_msiof_modify_ctr_wait(struct sh_msiof_spi_priv *p,
>> + unsigned long clr, unsigned long set)
>> +{
>> + unsigned long mask = clr | set;
>> + unsigned long data;
>> +
>> + data = sh_msiof_read(p, CTR);
>> + data &= ~clr;
>> + data |= set;
>> + sh_msiof_write(p, CTR, data);
>> +
>> + while ((sh_msiof_read(p, CTR) & mask) != set)
>> + ;
>
> hm, confidence. No timeout needed here?
Good plan, I'll add some timeout handling code.
>> +static struct {
>> + unsigned short div;
>> + unsigned short scr;
>> +} sh_msiof_spi_clk_table[] = {
>> + { 1, 0x0007 },
>> + { 2, 0x },
>> + { 4, 0x0001 },
>> + { 8, 0x0002 },
>> + { 16, 0x0003 },
>> + { 32, 0x0004 },
>> + { 64, 0x1f00 },
>> + { 128, 0x1f01 },
>> + { 256, 0x1f02 },
>> + { 512, 0x1f03 },
>> + { 1024, 0x1f04 },
>> +};
>
> Could be const (to save some .data) btu I think the compiler does that
> itself nowadays.
I'll fix that up.
>> +static void sh_msiof_spi_set_clk_regs(struct sh_msiof_spi_priv *p,
>> + unsigned long parent_rate,
>> + unsigned long spi_hz)
>> +{
>> + unsigned long div = 1024;
>> + int k;
>> +
>> + if (!spi_hz || !parent_rate)
>> + WARN_ON(1);
>> + else
>> + div = parent_rate / spi_hz;
>
> This could be more neatly coded as
>
> if (!WARN_ON(!spi_hz || !parent_rate))
> div = parent_rate / spi_hz;
>
> also, if this warning ever triggers, you won't know whether it was due
> to !spi_hx or to !parent_rate, which might make you sad.
>
> Can spi_hz and parent_rate ever be zero here anyway? If not, zap it.
> If so, why? Programming bug?
I wanted to fail gracefully if the spi layer or the clock framework
handed us zeroes. They are most likely not supposed to do so, but it
probably doesn't hurt to keep the check. This to catch bugs that may
be around - the spi setup code flow is pretty complex and the clock
framework implementation may vary from processor to processor.
>> + /* TODO: make more fine grained */
>> +
>> + for (k = 0; k < ARRAY_SIZE(sh_msiof_spi_clk_table); k++) {
>> + if (sh_msiof_spi_clk_table[k].div >= div)
>> + break;
>> + }
>> +
>> + k = min_t(int, k, ARRAY_SIZE(sh_msiof_spi_clk_table) - 1);
>
> actually it's pretty obvious from all the above that k should have been
> size_t.
Cool, will fix.
>> +static u32 sh_msiof_spi_txrx_word(struct spi_device *spi, unsigned nsecs,
>>
Re: [spi-devel-general] [PATCH] spi: Allow using spi_bitbang_setup() with custom txrx_bufs()
On Wed, Nov 25, 2009 at 1:26 AM, Magnus Damm wrote:
> From: Magnus Damm
>
> This patch modifies the shared spi bitbanging code
> to allow using spi_bitbang_setup() even though the
> txrx_word[] callbacks are unset. Useful for drivers
> that want to make use of spi_bitbang_setup() but
> have their own txrx_bufs() callback.
>
> While at it, drop the MSIOF driver workaround.
>
> Signed-off-by: Magnus Damm
> ---
>
> Depends on the MSIOF driver.
>
> drivers/spi/spi_bitbang.c | 25 ++---
> drivers/spi/spi_sh_msiof.c | 11 ---
> 2 files changed, 14 insertions(+), 22 deletions(-)
>
> --- 0001/drivers/spi/spi_bitbang.c
> +++ work/drivers/spi/spi_bitbang.c 2009-11-25 15:30:45.0 +0900
> @@ -176,6 +176,14 @@ int spi_bitbang_setup_transfer(struct sp
> }
> EXPORT_SYMBOL_GPL(spi_bitbang_setup_transfer);
>
> +static int spi_bitbang_bufs(struct spi_device *spi, struct spi_transfer *t)
> +{
> + struct spi_bitbang_cs *cs = spi->controller_state;
> + unsigned nsecs = cs->nsecs;
> +
> + return cs->txrx_bufs(spi, cs->txrx_word, nsecs, t);
> +}
> +
> /**
> * spi_bitbang_setup - default setup for per-word I/O loops
> */
> @@ -183,6 +191,7 @@ int spi_bitbang_setup(struct spi_device
> {
> struct spi_bitbang_cs *cs = spi->controller_state;
> struct spi_bitbang *bitbang;
> + int mode_mask = SPI_CPOL | SPI_CPHA;
> int retval;
> unsigned long flags;
>
> @@ -196,9 +205,11 @@ int spi_bitbang_setup(struct spi_device
> }
>
> /* per-word shift register access, in hardware or bitbanging */
> - cs->txrx_word = bitbang->txrx_word[spi->mode & (SPI_CPOL|SPI_CPHA)];
> - if (!cs->txrx_word)
> - return -EINVAL;
> + if (bitbang->txrx_bufs == spi_bitbang_bufs) {
> + cs->txrx_word = bitbang->txrx_word[spi->mode & mode_mask];
> + if (!cs->txrx_word)
> + return -EINVAL;
> + }
Hmmm... this smells like an ugly hack to me. It seems to me that if
some bitbang backend drivers don't want this code, then it should be
encoded into a callback so it can be overridden. Thoughts.
g.
--
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.
--
Let Crystal Reports handle the reporting - Free Crystal Reports 2008 30-Day
trial. Simplify your report design, integration and deployment - and focus on
what you do best, core application coding. Discover what's new with
Crystal Reports now. http://p.sf.net/sfu/bobj-july
___
spi-devel-general mailing list
spi-devel-general@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/spi-devel-general
Re: [spi-devel-general] spi_mpc8xxx.c: chip select polarity problem
On Wed, Nov 25, 2009 at 1:41 PM, Torsten Fleischer
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
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.0 +0100
> +++ linux-2.6.31.6/drivers/spi/spi_mpc8xxx.c 2009-11-19 08:15:33.0
> +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.
--
Let Crystal Reports handle the reporting - Free Crystal Reports 2008 30-Day
trial. Simplify your report design, integration and deployment - and focus on
what you do best, core application coding. Discover what's new with
Crystal Reports now. http://p.sf.net/sfu/bobj-july
___
spi-devel-general mailing
Re: [spi-devel-general] [PATCH] spi: SuperH MSIOF SPI Master driver
On Tue, 24 Nov 2009 21:55:31 +0900
Magnus Damm wrote:
> From: Magnus Damm
>
> This patch adds SPI Master support for the SuperH MSIOF
> hardware block. Full duplex, spi mode 0-3, active high cs,
> 3-wire and lsb first should all be supported, but the driver
> has so far only been tested with "mmc_spi".
>
> The MSIOF hardware comes with 32-bit FIFOs for receive and
> transmit, and this driver simply breaks the SPI messages
> into FIFO-sized chunks. The MSIOF hardware manages the pins
> for clock, receive and transmit (sck/miso/mosi), but the chip
> select pin is managed by software and must be configured as
> a regular GPIO pin by the board code.
>
> Performance wise there is still room for improvement, but
> on a Ecovec board with the built-in sh7724 MSIOF0 this driver
> gets Mini-sd read speeds of about half a megabyte per second.
>
> Future work include better clock setup and merging of 8-bit
> transfers into 32-bit words to reduce interrupt load and
> improve throughput.
>
>
> ...
>
> --- 0001/drivers/spi/spi.c
> +++ work/drivers/spi/spi.c2009-11-24 20:39:48.0 +0900
> @@ -17,7 +17,6 @@
> * along with this program; if not, write to the Free Software
> * Foundation, Inc., 675 Mass Ave, Cambridge, MA 02139, USA.
> */
> -
> #include
> #include
> #include
whoops?
> --- /dev/null
> +++ work/drivers/spi/spi_sh_msiof.c 2009-11-24 20:39:49.0 +0900
> @@ -0,0 +1,675 @@
> +/*
> + * SuperH MSIOF SPI Master Interface
> + *
> + * Copyright (c) 2009 Magnus Damm
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + *
> + */
> +
> +#include
But not consistently.
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +
> +#include
> +#include
> +#include
> +
> +#include
> +#include
> +
> +struct sh_msiof_spi_priv {
> + struct spi_bitbang bitbang; /* must be first for spi_bitbang.c */
Well if that's the case then spi_bitbang.c needs smacking. What causes
this requirement?
> + void __iomem *mapbase;
> + struct clk *clk;
> + struct platform_device *pdev;
> + struct sh_msiof_spi_info *info;
> + struct completion done;
> + unsigned long flags;
> + int tx_fifo_size;
> + int rx_fifo_size;
> +};
> +
>
> ...
>
> +static void sh_msiof_modify_ctr_wait(struct sh_msiof_spi_priv *p,
> + unsigned long clr, unsigned long set)
> +{
> + unsigned long mask = clr | set;
> + unsigned long data;
> +
> + data = sh_msiof_read(p, CTR);
> + data &= ~clr;
> + data |= set;
> + sh_msiof_write(p, CTR, data);
> +
> + while ((sh_msiof_read(p, CTR) & mask) != set)
> + ;
hm, confidence. No timeout needed here?
> +}
> +
> +static irqreturn_t sh_msiof_spi_irq(int irq, void *data)
> +{
> + struct sh_msiof_spi_priv *p = data;
> +
> + /* just disable the interrupt and wake up */
> + sh_msiof_write(p, IER, 0);
> +
> + complete(&p->done);
> +
> + return IRQ_HANDLED;
> +}
> +
> +static struct {
> + unsigned short div;
> + unsigned short scr;
> +} sh_msiof_spi_clk_table[] = {
> + { 1, 0x0007 },
> + { 2, 0x },
> + { 4, 0x0001 },
> + { 8, 0x0002 },
> + { 16, 0x0003 },
> + { 32, 0x0004 },
> + { 64, 0x1f00 },
> + { 128, 0x1f01 },
> + { 256, 0x1f02 },
> + { 512, 0x1f03 },
> + { 1024, 0x1f04 },
> +};
Could be const (to save some .data) btu I think the compiler does that
itself nowadays.
> +static void sh_msiof_spi_set_clk_regs(struct sh_msiof_spi_priv *p,
> + unsigned long parent_rate,
> + unsigned long spi_hz)
> +{
> + unsigned long div = 1024;
> + int k;
> +
> + if (!spi_hz || !parent_rate)
> + WARN_ON(1);
> + else
> + div = parent_rate / spi_hz;
This could be more neatly coded as
if (!WARN_ON(!spi_hz || !parent_rate))
div = parent_rate / spi_hz;
also, if this warning ever triggers, you won't know whether it was due
to !spi_hx or to !parent_rate, which might make you sad.
Can spi_hz and parent_rate ever be zero here anyway? If not, zap it.
If so, why? Programming bug?
> + /* TODO: make more fine grained */
> +
> + for (k = 0; k < ARRAY_SIZE(sh_msiof_spi_clk_table); k++) {
> + if (sh_msiof_spi_clk_table[k].div >= div)
> + break;
> + }
> +
> + k = min_t(int, k, ARRAY_SIZE(sh_msiof_spi_clk_table) - 1);
actually it's pretty obvious from all the above that k should have been
size_t.
> + sh_msiof_write(p, TSCR, sh_msiof_spi_clk_table[k].scr);
> + sh_msiof_write(p, RSCR, sh_msiof_spi_clk_table[k].scr);
> +}
> +
> +static void sh_msiof_spi_set_pin_regs(struct sh_msiof_spi_priv *p,
> +
Re: [spi-devel-general] linux-next: spi tree build warning
Hi Grant, On Wed, 25 Nov 2009 07:30:19 -0700 Grant Likely wrote: > > Trivial fix committed to next-spi branch: > > diff --git a/drivers/spi/xilinx_spi.c b/drivers/spi/xilinx_spi.c > index b927812..9f38637 100644 > --- a/drivers/spi/xilinx_spi.c > +++ b/drivers/spi/xilinx_spi.c > @@ -408,8 +408,8 @@ struct spi_master *xilinx_spi_init(struct device *dev, > struc > goto free_irq; > } > > - dev_info(dev, "at 0x%08X mapped to 0x%08X, irq=%d\n", > - (u32)mem->start, (u32)xspi->regs, xspi->irq); > + dev_info(dev, "at 0x%08llX mapped to 0x%p, irq=%d\n", > + (unsigned long long)mem->start, xspi->regs, xspi->irq); > return master; Thanks. -- Cheers, Stephen Rothwells...@canb.auug.org.au http://www.canb.auug.org.au/~sfr/ pgpvCCvGd2oEv.pgp Description: PGP signature -- Let Crystal Reports handle the reporting - Free Crystal Reports 2008 30-Day trial. Simplify your report design, integration and deployment - and focus on what you do best, core application coding. Discover what's new with Crystal Reports now. http://p.sf.net/sfu/bobj-july___ spi-devel-general mailing list spi-devel-general@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/spi-devel-general
Re: [spi-devel-general] linux-next: spi tree build warning
On Wed, Nov 25, 2009 at 3:29 AM, Stephen Rothwell wrote:
> Hi Grant,
>
> Today's linux-next build (x86_64 allmodconfig) produced this warning:
>
> drivers/spi/xilinx_spi.c: In function 'xilinx_spi_init':
> drivers/spi/xilinx_spi.c:411: warning: cast from pointer to integer of
> different size
>
> Maybe introduced by commit 8387f616c653f878b33d859310a8ed5c568505ee
> ("xilinx_spi: Split into of driver and generic part").
Trivial fix committed to next-spi branch:
diff --git a/drivers/spi/xilinx_spi.c b/drivers/spi/xilinx_spi.c
index b927812..9f38637 100644
--- a/drivers/spi/xilinx_spi.c
+++ b/drivers/spi/xilinx_spi.c
@@ -408,8 +408,8 @@ struct spi_master *xilinx_spi_init(struct device *dev, struc
goto free_irq;
}
- dev_info(dev, "at 0x%08X mapped to 0x%08X, irq=%d\n",
- (u32)mem->start, (u32)xspi->regs, xspi->irq);
+ dev_info(dev, "at 0x%08llX mapped to 0x%p, irq=%d\n",
+ (unsigned long long)mem->start, xspi->regs, xspi->irq);
return master;
free_irq:
--
Let Crystal Reports handle the reporting - Free Crystal Reports 2008 30-Day
trial. Simplify your report design, integration and deployment - and focus on
what you do best, core application coding. Discover what's new with
Crystal Reports now. http://p.sf.net/sfu/bobj-july
___
spi-devel-general mailing list
spi-devel-general@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/spi-devel-general
[spi-devel-general] [SPAM] 200 Mutuelles comparees - Devis immediat
Afficher correctement cet email mutuelle-conseil.com votre assurance santé, à partir de 5,42€* / mois [Dentaire] - [Optique] - [Maladie] - [Maternité] - [Hospitalisation] C'est Rapide, pratique et 100% efficace >>>Venez comparer vos offres santé en 30 secondes seulement Soins courant Dentaire Optique Hospitalisation
[spi-devel-general] [PATCH] spi: Allow using spi_bitbang_setup() with custom txrx_bufs()
From: Magnus Damm
This patch modifies the shared spi bitbanging code
to allow using spi_bitbang_setup() even though the
txrx_word[] callbacks are unset. Useful for drivers
that want to make use of spi_bitbang_setup() but
have their own txrx_bufs() callback.
While at it, drop the MSIOF driver workaround.
Signed-off-by: Magnus Damm
---
Depends on the MSIOF driver.
drivers/spi/spi_bitbang.c | 25 ++---
drivers/spi/spi_sh_msiof.c | 11 ---
2 files changed, 14 insertions(+), 22 deletions(-)
--- 0001/drivers/spi/spi_bitbang.c
+++ work/drivers/spi/spi_bitbang.c 2009-11-25 15:30:45.0 +0900
@@ -176,6 +176,14 @@ int spi_bitbang_setup_transfer(struct sp
}
EXPORT_SYMBOL_GPL(spi_bitbang_setup_transfer);
+static int spi_bitbang_bufs(struct spi_device *spi, struct spi_transfer *t)
+{
+ struct spi_bitbang_cs *cs = spi->controller_state;
+ unsignednsecs = cs->nsecs;
+
+ return cs->txrx_bufs(spi, cs->txrx_word, nsecs, t);
+}
+
/**
* spi_bitbang_setup - default setup for per-word I/O loops
*/
@@ -183,6 +191,7 @@ int spi_bitbang_setup(struct spi_device
{
struct spi_bitbang_cs *cs = spi->controller_state;
struct spi_bitbang *bitbang;
+ int mode_mask = SPI_CPOL | SPI_CPHA;
int retval;
unsigned long flags;
@@ -196,9 +205,11 @@ int spi_bitbang_setup(struct spi_device
}
/* per-word shift register access, in hardware or bitbanging */
- cs->txrx_word = bitbang->txrx_word[spi->mode & (SPI_CPOL|SPI_CPHA)];
- if (!cs->txrx_word)
- return -EINVAL;
+ if (bitbang->txrx_bufs == spi_bitbang_bufs) {
+ cs->txrx_word = bitbang->txrx_word[spi->mode & mode_mask];
+ if (!cs->txrx_word)
+ return -EINVAL;
+ }
retval = bitbang->setup_transfer(spi, NULL);
if (retval < 0)
@@ -232,14 +243,6 @@ void spi_bitbang_cleanup(struct spi_devi
}
EXPORT_SYMBOL_GPL(spi_bitbang_cleanup);
-static int spi_bitbang_bufs(struct spi_device *spi, struct spi_transfer *t)
-{
- struct spi_bitbang_cs *cs = spi->controller_state;
- unsignednsecs = cs->nsecs;
-
- return cs->txrx_bufs(spi, cs->txrx_word, nsecs, t);
-}
-
/*--*/
/*
--- 0002/drivers/spi/spi_sh_msiof.c
+++ work/drivers/spi/spi_sh_msiof.c 2009-11-25 15:25:32.0 +0900
@@ -510,13 +510,6 @@ static int sh_msiof_spi_txrx(struct spi_
return bytes_done;
}
-static u32 sh_msiof_spi_txrx_word(struct spi_device *spi, unsigned nsecs,
- u32 word, u8 bits)
-{
- BUG_ON(1); /* unused but needed by bitbang code */
- return 0;
-}
-
static int sh_msiof_spi_probe(struct platform_device *pdev)
{
struct resource *r;
@@ -594,10 +587,6 @@ static int sh_msiof_spi_probe(struct pla
p->bitbang.chipselect = sh_msiof_spi_chipselect;
p->bitbang.setup_transfer = sh_msiof_spi_setup_transfer;
p->bitbang.txrx_bufs = sh_msiof_spi_txrx;
- p->bitbang.txrx_word[SPI_MODE_0] = sh_msiof_spi_txrx_word;
- p->bitbang.txrx_word[SPI_MODE_1] = sh_msiof_spi_txrx_word;
- p->bitbang.txrx_word[SPI_MODE_2] = sh_msiof_spi_txrx_word;
- p->bitbang.txrx_word[SPI_MODE_3] = sh_msiof_spi_txrx_word;
ret = spi_bitbang_start(&p->bitbang);
if (ret == 0)
--
Let Crystal Reports handle the reporting - Free Crystal Reports 2008 30-Day
trial. Simplify your report design, integration and deployment - and focus on
what you do best, core application coding. Discover what's new with
Crystal Reports now. http://p.sf.net/sfu/bobj-july
___
spi-devel-general mailing list
spi-devel-general@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/spi-devel-general
