[PATCH] spi: add a bits_per_word to struct spi_board_info

[PATCH] spi: add a bits_per_word to struct spi_board_info

[Feng Tang]

Currently a spi device's bits_per_word can only be inited
inside protocol driver's probe func, this patch will enable
platform code to specify this info when registering the
spi_board_info

Signed-off-by: Feng Tang <feng.tang-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
Cc: David Brownell <dbrownell-Rn4VEauK+AKRv+LV9MX5uipxlwaOVQ5f@public.gmane.org>
Cc: Grant Likely <grant.likely-s3s/WqlpOiPyB63q8FvJNQ@public.gmane.org>
---
 drivers/spi/spi.c       |    1 +
 include/linux/spi/spi.h |    3 +++
 2 files changed, 4 insertions(+), 0 deletions(-)

diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c
index a9e5c79..a8690e7 100644
--- a/drivers/spi/spi.c
+++ b/drivers/spi/spi.c
@@ -350,6 +350,7 @@ struct spi_device *spi_new_device(struct spi_master *master,
 	proxy->chip_select = chip->chip_select;
 	proxy->max_speed_hz = chip->max_speed_hz;
 	proxy->mode = chip->mode;
+	proxy->bits_per_word = chip->bits_per_word;
 	proxy->irq = chip->irq;
 	strlcpy(proxy->modalias, chip->modalias, sizeof(proxy->modalias));
 	proxy->dev.platform_data = (void *) chip->platform_data;
diff --git a/include/linux/spi/spi.h b/include/linux/spi/spi.h
index 92e52a1..f36d0d5 100644
--- a/include/linux/spi/spi.h
+++ b/include/linux/spi/spi.h
@@ -698,6 +698,8 @@ static inline ssize_t spi_w8r16(struct spi_device *spi, u8 cmd)
  * @mode: Initializes spi_device.mode; based on the chip datasheet, board
  *	wiring (some devices support both 3WIRE and standard modes), and
  *	possibly presence of an inverter in the chipselect path.
+ * @bits_per_word: Initializes spi_device.bits_per_word, usually it will
+ * be from 8 to 32 bits
  *
  * When adding new SPI devices to the device tree, these structures serve
  * as a partial device template.  They hold information which can't always
@@ -742,6 +744,7 @@ struct spi_board_info {
 	 * where the default of SPI_CS_HIGH = 0 is wrong.
 	 */
 	u8		mode;
+	u8		bits_per_word;
 
 	/* ... may need additional spi_device chip config data here.
 	 * avoid stuff protocol drivers can set; but include stuff
-- 
1.7.0.4


------------------------------------------------------------------------------
This SF.net Dev2Dev email is sponsored by:

Show off your parallel programming skills.
Enter the Intel(R) Threading Challenge 2010.
http://p.sf.net/sfu/intel-thread-sfd

Re: [PATCH] spi: add a bits_per_word to struct spi_board_info

[Grant Likely]

On Wed, Sep 01, 2010 at 02:24:49PM +0800, Feng Tang wrote:
> Currently a spi device's bits_per_word can only be inited
> inside protocol driver's probe func, this patch will enable
> platform code to specify this info when registering the
> spi_board_info

What is the use case that you would want to do this.  Knowing what the bits_per_word should be is the responsibility of the spi device driver.  What is the use-case where the driver wouldn't know what bits_per_word should be?

g.

> 
> Signed-off-by: Feng Tang <feng.tang-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> Cc: David Brownell <dbrownell-Rn4VEauK+AKRv+LV9MX5uipxlwaOVQ5f@public.gmane.org>
> Cc: Grant Likely <grant.likely-s3s/WqlpOiPyB63q8FvJNQ@public.gmane.org>
> ---
>  drivers/spi/spi.c       |    1 +
>  include/linux/spi/spi.h |    3 +++
>  2 files changed, 4 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c
> index a9e5c79..a8690e7 100644
> --- a/drivers/spi/spi.c
> +++ b/drivers/spi/spi.c
> @@ -350,6 +350,7 @@ struct spi_device *spi_new_device(struct spi_master *master,
>  	proxy->chip_select = chip->chip_select;
>  	proxy->max_speed_hz = chip->max_speed_hz;
>  	proxy->mode = chip->mode;
> +	proxy->bits_per_word = chip->bits_per_word;
>  	proxy->irq = chip->irq;
>  	strlcpy(proxy->modalias, chip->modalias, sizeof(proxy->modalias));
>  	proxy->dev.platform_data = (void *) chip->platform_data;
> diff --git a/include/linux/spi/spi.h b/include/linux/spi/spi.h
> index 92e52a1..f36d0d5 100644
> --- a/include/linux/spi/spi.h
> +++ b/include/linux/spi/spi.h
> @@ -698,6 +698,8 @@ static inline ssize_t spi_w8r16(struct spi_device *spi, u8 cmd)
>   * @mode: Initializes spi_device.mode; based on the chip datasheet, board
>   *	wiring (some devices support both 3WIRE and standard modes), and
>   *	possibly presence of an inverter in the chipselect path.
> + * @bits_per_word: Initializes spi_device.bits_per_word, usually it will
> + * be from 8 to 32 bits
>   *
>   * When adding new SPI devices to the device tree, these structures serve
>   * as a partial device template.  They hold information which can't always
> @@ -742,6 +744,7 @@ struct spi_board_info {
>  	 * where the default of SPI_CS_HIGH = 0 is wrong.
>  	 */
>  	u8		mode;
> +	u8		bits_per_word;
>  
>  	/* ... may need additional spi_device chip config data here.
>  	 * avoid stuff protocol drivers can set; but include stuff
> -- 
> 1.7.0.4
> 

------------------------------------------------------------------------------
This SF.net Dev2Dev email is sponsored by:

Show off your parallel programming skills.
Enter the Intel(R) Threading Challenge 2010.
http://p.sf.net/sfu/intel-thread-sfd

Re: [PATCH] spi: add a bits_per_word to struct spi_board_info

[Feng Tang]

On Wed, 1 Sep 2010 23:46:18 +0800
David Brownell <david-b-yBeKhBN/0LDR7s880joybQ@public.gmane.org> wrote:

> Re $SUBJECT ... consider it a quirk of how
> SPI devices get setup that this field isn't
> one of the ones the platform code can set up.
> 
> I've thought about fixing this before,
> and decided against it:
> 
> 
> > What is the use case that you would want to do
>  > this. Knowing what the bits_per_word should
> > be is the responsibility of the spi device
> > driver.  What is the > use-case where the
> > driver wouldn't know what bits_per_word
> > should be?
> 
> What's the case where platform code could
> know "better" than the driver?

Grant also asked me the same question. Yes, driver itself
know the bits_per_word info best in most case, but there is
some device (Option GTM501L spi modem) which supports multiple
bits_per_mode option, and here platform code is the good place
to set it.

One of my intension of this patch is to save some extra spi_setup.
For many spi protocol drivers in kernel, spi_setup() will be
called twice for them, first time is inside spi_new_device(it calls
spi_add_device), the second time will be inside driver's probe
func, after setting bits_per_word info. So if we can set it before
registering board info, then the second spi_setup can be removed for
many drivers.

Thanks,
Feng



------------------------------------------------------------------------------
This SF.net Dev2Dev email is sponsored by:

Show off your parallel programming skills.
Enter the Intel(R) Threading Challenge 2010.
http://p.sf.net/sfu/intel-thread-sfd

Re: [PATCH] spi: add a bits_per_word to struct spi_board_info

[David Brownell]

--- On Wed, 9/1/10, Feng Tang <feng.tang-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org> wrote:

> > 
> > What's the case where platform code could
> > know "better" than the driver?
> 
> Grant also asked me the same question.


I saw him ask a different question (which
I quoted) ...


 Yes, driver itself
> know the bits_per_word info best in most case, but there
> is
> some device (Option GTM501L spi modem) which supports
> multiple
> bits_per_mode option, and here platform code
is the good place to set it.


Not unless it knows which mode the driver uses...
a kind of layering violation.

> 
> So if we can set it before
> registering board info, then the second spi_setup can be
> removed for many drivers.

that doesn't seem to me like it'd be a startup
cost that's easily observed; hard for me to
worry about it, but I can see what you mean.


------------------------------------------------------------------------------
This SF.net Dev2Dev email is sponsored by:

Show off your parallel programming skills.
Enter the Intel(R) Threading Challenge 2010.
http://p.sf.net/sfu/intel-thread-sfd

Re: [PATCH] spi: add a bits_per_word to struct spi_board_info

[Feng Tang]

Hi David,


> > Grant also asked me the same question.
> 
> 
> I saw him ask a different question (which
> I quoted) ...

My mistake, I tried to answer Grant's and your questions in one email to
keep the discussion in one thread.

> 
> 
>  Yes, driver itself
> > know the bits_per_word info best in most case, but there
> > is
> > some device (Option GTM501L spi modem) which supports
> > multiple
> > bits_per_mode option, and here platform code
> is the good place to set it.
> 
> 
> Not unless it knows which mode the driver uses...
> a kind of layering violation.

spi_board_info has a member of "mode", so for my platform, I prefer to
set all these infos in platform code, while protocol driver can
check if the preset infos like speed_hz/mode/bits_per_word are supported.

I thought platform code have to know the detail of the spi device
when setting spi_board_info for it.

Also the following code quoted from spi_setup() indicates it supports
setting mode for spi_board_info
------------------------------------------------------------------------
	bad_bits = spi->mode & ~spi->master->mode_bits;
	if (bad_bits) {
		dev_dbg(&spi->dev, "setup: unsupported mode bits %x\n",
			bad_bits);
		return -EINVAL;
	}

Thanks,
Feng




------------------------------------------------------------------------------
This SF.net Dev2Dev email is sponsored by:

Show off your parallel programming skills.
Enter the Intel(R) Threading Challenge 2010.
http://p.sf.net/sfu/intel-thread-sfd

Re: [PATCH] spi: add a bits_per_word to struct spi_board_info

[Grant Likely]

On Thu, Sep 02, 2010 at 02:13:48PM +0800, Feng Tang wrote:
> Hi David,
> 
> 
> > > Grant also asked me the same question.
> > 
> > 
> > I saw him ask a different question (which
> > I quoted) ...
> 
> My mistake, I tried to answer Grant's and your questions in one email to
> keep the discussion in one thread.
> 
> > 
> > 
> >  Yes, driver itself
> > > know the bits_per_word info best in most case, but there
> > > is
> > > some device (Option GTM501L spi modem) which supports
> > > multiple
> > > bits_per_mode option, and here platform code
> > is the good place to set it.
> > 
> > 
> > Not unless it knows which mode the driver uses...
> > a kind of layering violation.
> 
> spi_board_info has a member of "mode", so for my platform, I prefer to
> set all these infos in platform code, while protocol driver can
> check if the preset infos like speed_hz/mode/bits_per_word are supported.

The real problem is that spi_drivers are normally allowed to change
bits_per_word as needed.  If the spi_driver gets unbound and
rebound, then the original platform-set value for bits_per_word is
lost and the behaviour changes.  If you really want to give the driver
hints about the best setting for bits_per_word or similar, then you
should probably use a driver-specific pdata structure.

g.


------------------------------------------------------------------------------
This SF.net Dev2Dev email is sponsored by:

Show off your parallel programming skills.
Enter the Intel(R) Threading Challenge 2010.
http://p.sf.net/sfu/intel-thread-sfd

Re: [PATCH] spi: add a bits_per_word to struct spi_board_info

[Feng Tang]

Hi Grant,

On Thu, 9 Sep 2010 04:21:59 +0800
Grant Likely <grant.likely-s3s/WqlpOiPyB63q8FvJNQ@public.gmane.org> wrote:

> > > 
> > >  Yes, driver itself
> > > > know the bits_per_word info best in most case, but there
> > > > is
> > > > some device (Option GTM501L spi modem) which supports
> > > > multiple
> > > > bits_per_mode option, and here platform code
> > > is the good place to set it.
> > > 
> > > 
> > > Not unless it knows which mode the driver uses...
> > > a kind of layering violation.
> > 
> > spi_board_info has a member of "mode", so for my platform, I prefer
> > to set all these infos in platform code, while protocol driver can
> > check if the preset infos like speed_hz/mode/bits_per_word are
> > supported.
> 
> The real problem is that spi_drivers are normally allowed to change
> bits_per_word as needed. 

For drivers who don't accept bits_per_word comes from platform code, it
could just ignores it and set the value it wants.

> If the spi_driver gets unbound and
> rebound, then the original platform-set value for bits_per_word is
> lost and the behaviour changes.  

Um, if you are talking about driver load/unload, then the bits_per_word
info won't get lost as it is set to spi_device from spi_board_info when
spi_new_device get called.

> If you really want to give the driver
> hints about the best setting for bits_per_word or similar, then you
> should probably use a driver-specific pdata structure.

Good point, really a solution to the GTM501L modem driver. But still,
adding the bits_per_word to spi_board_info will be more generic when
more devices which support multiple bits models show up.

Thanks,
Feng


------------------------------------------------------------------------------
This SF.net Dev2Dev email is sponsored by:

Show off your parallel programming skills.
Enter the Intel(R) Threading Challenge 2010.
http://p.sf.net/sfu/intel-thread-sfd

Re: [PATCH] spi: add a bits_per_word to struct spi_board_info

[Grant Likely]

On Thu, Sep 09, 2010 at 11:14:45AM +0800, Feng Tang wrote:
> Hi Grant,
> 
> On Thu, 9 Sep 2010 04:21:59 +0800
> Grant Likely <grant.likely-s3s/WqlpOiPyB63q8FvJNQ@public.gmane.org> wrote:
> 
> > > > 
> > > >  Yes, driver itself
> > > > > know the bits_per_word info best in most case, but there
> > > > > is
> > > > > some device (Option GTM501L spi modem) which supports
> > > > > multiple
> > > > > bits_per_mode option, and here platform code
> > > > is the good place to set it.
> > > > 
> > > > 
> > > > Not unless it knows which mode the driver uses...
> > > > a kind of layering violation.
> > > 
> > > spi_board_info has a member of "mode", so for my platform, I prefer
> > > to set all these infos in platform code, while protocol driver can
> > > check if the preset infos like speed_hz/mode/bits_per_word are
> > > supported.
> > 
> > The real problem is that spi_drivers are normally allowed to change
> > bits_per_word as needed. 
> 
> For drivers who don't accept bits_per_word comes from platform code, it
> could just ignores it and set the value it wants.
> 
> > If the spi_driver gets unbound and
> > rebound, then the original platform-set value for bits_per_word is
> > lost and the behaviour changes.  
> 
> Um, if you are talking about driver load/unload, then the bits_per_word
> info won't get lost as it is set to spi_device from spi_board_info when
> spi_new_device get called.

No, I'm talking about a specific device's spi_driver being unloaded
and reloaded.  spi_new_device would only be called if the spi bus
driver was reloaded, triggering a call to spi_register_master().

> > If you really want to give the driver
> > hints about the best setting for bits_per_word or similar, then you
> > should probably use a driver-specific pdata structure.
> 
> Good point, really a solution to the GTM501L modem driver. But still,
> adding the bits_per_word to spi_board_info will be more generic when
> more devices which support multiple bits models show up.

As described above, anything that a driver can legally modify cannot
be "preset" by the platform code because the initial preset values
will get lost.  A different mechanism must be used so that initial
conditions remain unchanged between successive binding of a driver.

g.

------------------------------------------------------------------------------
This SF.net Dev2Dev email is sponsored by:

Show off your parallel programming skills.
Enter the Intel(R) Threading Challenge 2010.
http://p.sf.net/sfu/intel-thread-sfd

Re: [PATCH] spi: add a bits_per_word to struct spi_board_info

[David Brownell]

You know, in retrospect, I shouldn't have put
most of those SPI device setup params into the
board setup data.

There's one which MUST be there:  polarity of
the chip select line.  The rest seem like they
could (and arguably should) all be handled by
driver-specific params.  (Possible exception:
clock rate, which sometimes matters even when
chips are not selected).

At any rate, adding MORE driver-specific params
(like bits-per-word) to board setup data seems
like the wrong direction to go...



------------------------------------------------------------------------------
This SF.net Dev2Dev email is sponsored by:

Show off your parallel programming skills.
Enter the Intel(R) Threading Challenge 2010.
http://p.sf.net/sfu/intel-thread-sfd

Re: [PATCH] spi: add a bits_per_word to struct spi_board_info

[Grant Likely]

On Wed, Sep 8, 2010 at 9:48 PM, David Brownell <david-b-yBeKhBN/0LDR7s880joybQ@public.gmane.org> wrote:
> You know, in retrospect, I shouldn't have put
> most of those SPI device setup params into the
> board setup data.

Out of curiosity, what was the reasoning behind registering the spi
board info separately instead of passing it explicitly via the spi bus
driver's platform_data?  (I ask only because I'm unfamiliar with the
history).

> There's one which MUST be there:  polarity of
> the chip select line.  The rest seem like they
> could (and arguably should) all be handled by
> driver-specific params.  (Possible exception:
> clock rate, which sometimes matters even when
> chips are not selected).

Of course, CS polarity and clock rate are more representative of the
interconnection than of the device.  Regardless though, it should be
done in such a way that a properly behaved spi_driver cannot change
the initial .probe() conditions.

> At any rate, adding MORE driver-specific params
> (like bits-per-word) to board setup data seems
> like the wrong direction to go...

indeed!

g.

------------------------------------------------------------------------------
This SF.net Dev2Dev email is sponsored by:

Show off your parallel programming skills.
Enter the Intel(R) Threading Challenge 2010.
http://p.sf.net/sfu/intel-thread-sfd

Re: [PATCH] spi: add a bits_per_word to struct spi_board_info

[Feng Tang]

On Thu, 9 Sep 2010 11:48:22 +0800
David Brownell <david-b-yBeKhBN/0LDR7s880joybQ@public.gmane.org> wrote:

> You know, in retrospect, I shouldn't have put
> most of those SPI device setup params into the
> board setup data.
> 
> There's one which MUST be there:  polarity of
> the chip select line.  The rest seem like they
> could (and arguably should) all be handled by
> driver-specific params.  (Possible exception:
> clock rate, which sometimes matters even when
> chips are not selected).
> 
> At any rate, adding MORE driver-specific params
> (like bits-per-word) to board setup data seems
> like the wrong direction to go..

Agree. Thank you two for helping me get an overall picture of
configuring these parameters.

-Feng

------------------------------------------------------------------------------
This SF.net Dev2Dev email is sponsored by:

Show off your parallel programming skills.
Enter the Intel(R) Threading Challenge 2010.
http://p.sf.net/sfu/intel-thread-sfd