[flashrom] [PATCH] linux_spi.c: set SPI mode, word justification and bits per word on init.
Carl-Daniel Hailfinger
c-d.hailfinger.devel.2006 at gmx.net
Sat Mar 3 13:18:45 CET 2012
Am 02.03.2012 23:51 schrieb Stefan Tauner:
> On Fri, 02 Mar 2012 22:43:44 +0100
> Carl-Daniel Hailfinger <c-d.hailfinger.devel.2006 at gmx.net> wrote:
>
>> Am 02.03.2012 00:43 schrieb Stefan Tauner:
>>> Previously we relied on a correctly set up state.
>>>
>>> ---
>>> untested.
>>>
>>> Signed-off-by: Stefan Tauner <stefan.tauner at student.tuwien.ac.at>
>>> ---
>>> linux_spi.c | 23 +++++++++++++++++++++++
>>> 1 files changed, 23 insertions(+), 0 deletions(-)
>>>
>>> diff --git a/linux_spi.c b/linux_spi.c
>>> index d994389..d29c59a 100644
>>> --- a/linux_spi.c
>>> +++ b/linux_spi.c
>>> @@ -57,6 +57,8 @@ int linux_spi_init(void)
>>> {
>>> char *p, *endp, *dev;
>>> uint32_t speed = 0;
>>> + /* FIXME: make the following configurable by CLI options. */
>>> + uint8_t mode = SPI_MODE_0, lsb = 0, bits = 0; /* mode 0, msb first, 8 bits */
>> Can you move that comment above the variable definitions?
> done, reads now:
> /* SPI mode 0, msb first, 8 bits (i.e. value=0) */
>
>> Where should we note that SPI_MODE_0 also implies CS# active low?
> it does not. the test program seems to be outdated, the actual code
> masks the CPOL/CPHA bits.
Let me rephrase that: Setting the SPI mode handles CPOL/CPHA/CS/LSB/...
all in one. The bits which are not set have their default value (0).
http://tomoyo.sourceforge.jp/cgi-bin/lxr/source/drivers/spi/spidev.c#L72
#define SPI_MODE_MASK (SPI_CPHA | SPI_CPOL | SPI_CS_HIGH | SPI_LSB_FIRST
| SPI_3WIRE | SPI_LOOP | SPI_NO_CS | SPI_READY)
>>>
>>> dev = extract_programmer_param("dev");
>>> if (!dev || !strlen(dev)) {
>>> @@ -92,6 +94,27 @@ int linux_spi_init(void)
>>> msg_pdbg("Using %d kHz clock\n", speed);
>>> }
>>>
>>> + if (ioctl(fd, SPI_IOC_WR_MODE, &mode) == -1) {
>>> + msg_perr("%s: failed to set SPI mode to %u: %s\n",
>>> + __func__, mode, strerror(errno));
>>> + close(fd);
>>> + return 1;
>>> + }
>>> +
>>> + if (ioctl(fd, SPI_IOC_WR_LSB_FIRST, &lsb) == -1) {
>>> + msg_perr("%s: failed to set SPI justification to %u: %s\n",
>>> + __func__, lsb, strerror(errno));
>> This message would benefit from an explanation what SPI justification
>> is. Suggestion:
>> msg_perr("%s: failed to set SPI bit order to %s first: %s\n", __func_,
>> lsb ? "LSB" : "MSB", strerror(errno));
> right, was too lazy to think about a better term/solution at the
> time; fixed.
>
>>> + close(fd);
>>> + return 1;
>>> + }
>>> +
>>> + if (ioctl(fd, SPI_IOC_WR_BITS_PER_WORD, &bits) == -1) {
>>> + msg_perr("%s: failed to set the number of bits in an SPI word to %u: %s\n",
>>> + __func__, bits, strerror(errno));
>> bits is 0. The error message would suggest that we tried to set the
>> number of bits to 0. Does 0 also mean 8 bits, or would we have to set 8
>> bits with bits=8?
> bits = 0 is the only defined value in the documentation and is actually
> the only one implemented in the code and means 8 bits per word.
The documentation is explicit that bits is the number of bits, unless
bits=0, in which case it is treated as 8 bits.
I also have checked the code in the Linux kernel and it agrees with me.
Note that retval in ioctl handling is _not_ the value entered by the
user, but an error/success code for accessing user data.
http://tomoyo.sourceforge.jp/cgi-bin/lxr/source/drivers/spi/spidev.c#L408
http://tomoyo.sourceforge.jp/cgi-bin/lxr/source/drivers/spi/spi.c#L727
> i have changed the message to this:
> msg_perr("%s: failed to set the number of bits per SPI word to %s: %s\n",
> __func__, bits == 0 ? "8" : "<undef>", strerror(errno));
>
>>> + close(fd);
>>> + return 1;
>>> + }
>>> +
>>> if (register_shutdown(linux_spi_shutdown, NULL))
>>> return 1;
>>>
>> As an alternative, we could avoid the whole close(fd) dance by calling
>> register_shutdown() first, and then letting it do the work for us
>> automatically after we return 1.
> how do we do it in other programmers? we should probably define and
> document a single suggested way so that we dont have to discuss this
> every time. :)
A single suggested way (or maybe two ways, depending on the hardware
interface design we're using) is a good idea as long as it's not a hard
unbreakable rule afterwards.
The following two ways seem to make sense for linux_spi IMHO:
ret = 1; goto out_shutdown;
or
setting everything after register_shutdown.
If we want to restore old settings on shutdown, it gets a bit hackier.
One possible way would be:
int restore_settings;
init (){
open fd else return 1;
restore_settings=0;
register_shutdown() else return 1;
get all settings and store them in file-level variables or a file-level
struct else return 1;
restore_settings=1;
set all settings else return 1;
return 0;
}
shutdown() {
if (restore_settings){
restore settings;
}
close(fd);
}
> in this particular case i think it makes sense. in general relying on
> the shutdown function only may be a bit hard to grasp/implement for
> complicated init functions that allocate/manipulate lots of stuff (e.g.
> serprog).
Agreed.
Side note: How should we handle a failed register_shutdown()? Call the
shutdown function manually and return 1? Or is there any other good way
to clean up?
Regards,
Carl-Daniel
--
http://www.hailfinger.org/
More information about the flashrom
mailing list