Blockers on IIO usage of regmap.
Subsystems (7)
Hi Mark,
Just a quick heads up that the big blocker for us making more use of regmap is lack of default control of cs_change for spi buses. That leads to a lot of hand crafted spi read / write routines and is rather tedious.
When I have the time I'll look into sorting that side out.
Also, we will need a couple of variants of bulk read for spi. Classic ones are
TX Add0...Add7 XXXXXXXXXXX XXXXXXXXXXX XXXXXXXX RX XXXXXXXXXXX Da0.....Da7 Db0.....Db7 etc
TX Add0...Add7 XXXXXXXXXXX XXXXXXXXXXX XXXXXXXXXX RX XXXXXXXXXXX XXXXXXXXXXX Da0.....Da7 Db0....Db7
TX Ada0...Ada7 Adb1...Adb7 Adc1...Adc7 etc RX XXXXXXXXXXX Da0.....Da7 Db0.....Db7 etc
Last one is a kind of 'simulated' bulk read where address is simply incremented (just for complexity that increment may well be by 2 rather than one...) That one is only sort of a 'bulk' read, but rather common.
So basically we need some bus specific 'mode' hook somewhere. Given we have separate init functions for the buses could we add a struct regmap_spi_config to the parameter list?
IIRC IIO has several drivers in each of the above categories so sharing the code via regmap would be great!
What do you think?
Jonathan
Quoted 8 lines · Jonathan Cameron
Hi Mark,
Just a quick heads up that the big blocker for us making more use of regmap is lack of default control of
cs_changefor spi buses. That leads to a lot of hand crafted spi read / write routines and is rather tedious.
Quoted 8 lines
When I have the time I'll look into sorting that side out.
Also, we will need a couple of variants of bulk read for spi. Classic ones are
TX Add0...Add7 XXXXXXXXXXX XXXXXXXXXXX XXXXXXXX RX XXXXXXXXXXX Da0.....Da7 Db0.....Db7 etc
oops, been a bit inconsistent here. Add0 should have been Ada0 for first address. (thanks Anish) To clarify
Ada0..Ada7 is the address of first register for bulk read (address incrementing assumed for later addresses) Da0...Da7 is the data from that address.
XXXX is don't care.
Quoted 6 lines
TX Add0...Add7 XXXXXXXXXXX XXXXXXXXXXX XXXXXXXXXX RX XXXXXXXXXXX XXXXXXXXXXX Da0.....Da7 Db0....Db7TX Ada0...Ada7 Adb1...Adb7 Adc1...Adc7 etc RX XXXXXXXXXXX Da0.....Da7 Db0.....Db7 etc
One more common varient is a 'magic' register read which corresponds to read all in a cyclic fashion. That one would be nasty for caching as it won't be obvious to regmap which registers were actually read.
Quoted 15 lines
Last one is a kind of 'simulated' bulk read where address is simply incremented (just for complexity that increment may well be by 2 rather than one...) That one is only sort of a 'bulk' read, but rather common.
So basically we need some bus specific 'mode' hook somewhere. Given we have separate init functions for the buses could we add a struct
regmap_spi_configto the parameter list?IIRC IIO has several drivers in each of the above categories so sharing the code via regmap would be great!
What do you think?
Jonathan
Quoted 3 lines · Jonathan Cameron
Just a quick heads up that the big blocker for us making more use of regmap is lack of default control of
cs_changefor spi buses. That leads to
Looking at what you've got here there's nothing interesting with /CS here, it's all about the data formatting. Certainly you've not mentioned anything odd with /CS in any of the code.
Quoted 2 lines
TX Add0...Add7 XXXXXXXXXXX XXXXXXXXXXX XXXXXXXX RX XXXXXXXXXXX Da0.....Da7 Db0.....Db7 etc
This is the standard one that most things do so is already supported.
Quoted 2 lines
TX Add0...Add7 XXXXXXXXXXX XXXXXXXXXXX XXXXXXXXXX RX XXXXXXXXXXX XXXXXXXXXXX Da0.....Da7 Db0....Db7
This is just inserting a delay so should be trivial to implement and doesn't even look terribly SPI specific, just set a flag and it should be fine. All we need here is someone to actually implement it. I guess the delay will be OK for any read?
Quoted 2 lines
TX Ada0...Ada7 Adb1...Adb7 Adc1...Adc7 etc RX XXXXXXXXXXX Da0.....Da7 Db0.....Db7 etc
This one is much more of a contortion to implement - it's pretty SPI specific to stream the register addresses and it's really not what the system is set up for. On the other hand if you can come up with a taseful way to implement it then I don't see a problem.
Quoted 3 lines
So basically we need some bus specific 'mode' hook somewhere. Given we have separate init functions for the buses could we add a struct
regmap_spi_configto the parameter list?
Or just embed stuff into the main config structure at least for the delay. Nothing there seems too outrageous, and the delay sounds like something someone might do on other buses (the delay will be to allow time for the ADCs or whatever to set up).
Quoted 9 lines · Mark Brown
Quoted 3 lines · Jonathan Cameron
Just a quick heads up that the big blocker for us making more use of regmap is lack of default control of
cs_changefor spi buses. That leads toLooking at what you've got here there's nothing interesting with /CS here, it's all about the data formatting. Certainly you've not mentioned anything odd with /CS in any of the code.
Sorry, email got somewhat mashed up in editing (pesky dentist appointment in the middle!).
you are quite correct. Two completely different issues. The spi one needs addressing in the spi core, so is a topic for another thread entirely (I have proposal but need to test that thoroughly before sending out). Here I was simply going to say there are issues with lack of control of that except on a individual transfer basis.
Anyhow, on to the actual email topic...
Quoted 6 lines
Quoted 3 lines
TX Add0...Add7 XXXXXXXXXXX XXXXXXXXXXX XXXXXXXX RX XXXXXXXXXXX Da0.....Da7 Db0.....Db7 etcThis is the standard one that most things do so is already supported.
Quoted 7 lines
Quoted 2 lines
TX Add0...Add7 XXXXXXXXXXX XXXXXXXXXXX XXXXXXXXXX RX XXXXXXXXXXX XXXXXXXXXXX Da0.....Da7 Db0....Db7This is just inserting a delay so should be trivial to implement and doesn't even look terribly SPI specific, just set a flag and it should be fine. All we need here is someone to actually implement it. I guess the delay will be OK for any read?
Yup. It tends to be required for all reads on a given device. Note it isn't typically required for writes on the devices I have but I can see some devices may need a 'gap' for that as well. There is a slight kicker which is 'magic' address values that trigger a read of all registers whilst other addresses don't work for burst reads.
Quoted 8 lines
Quoted 3 lines
TX Ada0...Ada7 Adb1...Adb7 Adc1...Adc7 etc RX XXXXXXXXXXX Da0.....Da7 Db0.....Db7 etcThis one is much more of a contortion to implement - it's pretty SPI specific to stream the register addresses and it's really not what the system is set up for. On the other hand if you can come up with a taseful way to implement it then I don't see a problem.
I'll have a play and get back to you. Another option might be a hook into regmap to push in register values from reads that regmap itself doesn't know how to do. That way the debugfs stuff is useful and things aren't 'too' clunky + caching etc will work. This sort of interleaved read write is vital for performance on some devices that otherwise fit perfectly into regmap so may be worth the effort!
It may seem odd, but it might also be handy to have a regmap_get_bus hook in regmap for these odd cases as it saves on carrying a bonus pointer to the underlying bus around the code (often also needed to get you access to stuff that isn't really part of the bus at all such as auxiliary irq numbers).
Quoted 9 lines
Quoted 4 lines
So basically we need some bus specific 'mode' hook somewhere. Given we have separate init functions for the buses could we add a struct
regmap_spi_configto the parameter list?Or just embed stuff into the main config structure at least for the delay. Nothing there seems too outrageous, and the delay sounds like something someone might do on other buses (the delay will be to allow time for the ADCs or whatever to set up).
Delay indeed is general enough for this. Might need to think a bit on the naming so it's clear it isn't a time value, but rather a case of 'don't care' data sitting before what you actually requested.
I'll bash out a patch once I've sorted the CS stuff (as my test part adis16350 needs that before I can use regmap for it at all).
Thanks
Jonathan
Quoted 36 lines · J.I. Cameron
Quoted 9 lines · Mark Brown
Quoted 3 lines · Jonathan Cameron
Just a quick heads up that the big blocker for us making more use of regmap is lack of default control of
cs_changefor spi buses. That leads toLooking at what you've got here there's nothing interesting with /CS here, it's all about the data formatting. Certainly you've not mentioned anything odd with /CS in any of the code.
Sorry, email got somewhat mashed up in editing (pesky dentist appointment in the middle!).
you are quite correct. Two completely different issues. The spi one needs addressing in the spi core, so is a topic for another thread entirely (I have proposal but need to test that thoroughly before sending out). Here I was simply going to say there are issues with lack of control of that except on a individual transfer basis.
Anyhow, on to the actual email topic...
Quoted 6 lines
Quoted 3 lines
TX Add0...Add7 XXXXXXXXXXX XXXXXXXXXXX XXXXXXXX RX XXXXXXXXXXX Da0.....Da7 Db0.....Db7 etcThis is the standard one that most things do so is already supported.
Quoted 7 lines
Quoted 2 lines
TX Add0...Add7 XXXXXXXXXXX XXXXXXXXXXX XXXXXXXXXX RX XXXXXXXXXXX XXXXXXXXXXX Da0.....Da7 Db0....Db7This is just inserting a delay so should be trivial to implement and doesn't even look terribly SPI specific, just set a flag and it should be fine. All we need here is someone to actually implement it. I guess the delay will be OK for any read?
Quoted 5 lines
Yup. It tends to be required for all reads on a given device. Note it isn't typically required for writes on the devices I have but I can see some devices may need a 'gap' for that as well. There is a slight kicker which is 'magic' address values that trigger a read of all registers whilst other addresses don't work for burst reads.
Hohum. Every time I look at these devices things turn out to be more complex. Given we have a whole heap of similar devices would it be valid to define our own local regmap-spi-adis.c file to handle the variant?
To explain what we actually have (with all the cs line changes as well)
Writes are all 8 bit
CS -_______________________-
TX Ada0...Ada7 Da0....Da7
RX XXXXXXXXXXXXXXXXXXXXXXXXX
Reads are 16 bit with either of the two 8 bit register addresses given the same value
CS -______________________-_____________________-
TX Ada0....Ada7 XXXXXXXX
RX XXXXXXXXXXXXXXXXXXXXX Da0...Da7 Db0....Db7
Can interpret Da0...Da7 and Db0....Db7 as single 16 bit register and consider this device to just have a weird write method and normal read. Might be easier. I'll define Ax<n> as 16 bit address for the burst read.
Bursts are mostly
CS -_________________________________________________________________________________________-
TX Magic_address (3e) 000000000 XXXXXXXXXXXXX XXXXXXXXXXXXXXXXXXXXXXXXXX ~~ XXXXXXXXXXXX RX XXXXXXXXXXXXXXXXXXXXXXXXXXXX Axa0....Axa15 Axb0....Axb15 Axc0...Axc15 ~~ Axz0...Axz15
There is a tedious set of variant (old) where burst mode isn't supported and the duplex method given below is needed. Note burst reading only applies to a particular set of registers and crucially can run at a higher spi rate than normal reading (sometimes - depends on sampling rate).
I would just ignore regmap for these devices, except they otherwise fit well (lots of static registers)
Perhaps the burst mode thing is better handled by just providing a hook to allow data to be pushed into regmap (from 'magic' sources), but the weird write read combination looks to me like something that makes sense to have in regmap (be it as another bus variant).
Just for reference, this setup applies to
iio/imu/adis16400 (my test part is a adis16350) iio/accel/adis16201 (no burst mode). iio/accel/adis16203 (no burst mode). iio/accel/adis16204 (no burst mode). iio/accel/adis16209 (no burst mode). iio/accel/adis16220 (no burst mode - weird device ;) iio/accel/adis16240 (no burst mode) iio/gyro/adis16260 (no burst mode)
It's rather hacky and VERY restricted, but the series that follows this illustrates what I mean. It's a fun one to actually apply though as you'll need my iio-blue.git tree, Mark's regmap
tree and the additional write mask patches. Oh and it includes a first pass at adding cs_change
global control to spi devices. Patches should be as replies to this and are definitely an RFC where we can pick and choose what we want...
Quoted 39 lines
Quoted 8 lines
Quoted 3 lines
TX Ada0...Ada7 Adb1...Adb7 Adc1...Adc7 etc RX XXXXXXXXXXX Da0.....Da7 Db0.....Db7 etcThis one is much more of a contortion to implement - it's pretty SPI specific to stream the register addresses and it's really not what the system is set up for. On the other hand if you can come up with a taseful way to implement it then I don't see a problem.
I'll have a play and get back to you. Another option might be a hook into regmap to push in register values from reads that regmap itself doesn't know how to do. That way the debugfs stuff is useful and things aren't 'too' clunky + caching etc will work. This sort of interleaved read write is vital for performance on some devices that otherwise fit perfectly into regmap so may be worth the effort!
It may seem odd, but it might also be handy to have a
regmap_get_bushook in regmap for these odd cases as it saves on carrying a bonus pointer to the underlying bus around the code (often also needed to get you access to stuff that isn't really part of the bus at all such as auxiliary irq numbers).Quoted 9 lines
Quoted 4 lines
So basically we need some bus specific 'mode' hook somewhere. Given we have separate init functions for the buses could we add a struct
regmap_spi_configto the parameter list?Or just embed stuff into the main config structure at least for the delay. Nothing there seems too outrageous, and the delay sounds like something someone might do on other buses (the delay will be to allow time for the ADCs or whatever to set up).
Delay indeed is general enough for this. Might need to think a bit on the naming so it's clear it isn't a time value, but rather a case of 'don't care' data sitting before what you actually requested.
I'll bash out a patch once I've sorted the CS stuff (as my test part adis16350 needs that before I can use regmap for it at all).
Thanks
Jonathan
PATCH 1/6 SPI: add ability to say we want a cs change after every transfer.
This allows a number of drivers to make use of utility functions such spi_write_then_read as well as making use of regmap possible.
--- drivers/spi/spi.c | 8 ++++++++ include/linux/spi/spi.h | 2 ++ 2 files changed, 10 insertions(+), 0 deletions(-)
drivers/spi/spi.c +8 -0
@@ -781,6 +781,14 @@ static int __spi_async(struct spi_device *spi, struct spi_message *message) } } +if (spi->cs_between_transfers) { +struct spi_transfer *xfer; +list_for_each_entry (xfer, &message->transfers, transfer_list) { +if (!list_is_last (&xfer->transfer_list, &message->transfers)) + xfer->cs_change =1 ; + } + } + message->spi = spi; message->status = -EINPROGRESS ;return master->transfer (spi, message);
include/linux/spi/spi.h +2 -0
@@ -90,6 +90,8 @@ struct spi_device {void *controller_data;char modalias[SPI_NAME_SIZE ]; + unsigned cs_between_transfers:1; +/* * likely need more hooks for more protocol options affecting how * the controller talks to each chip, like:
Signature · 1 line
1.7.3.4
Re: [PATCH 1/6] SPI: add ability to say we want a cs change after every transfer.
Quoted 4 lines · Jonathan Cameron
This allows a number of drivers to make use of utility functions such
spi_write_then_readas well as making use of regmap possible.Signed-off-by Jonathan Cameron <[email protected]>
We could add this in regmap if required.
Re: [PATCH 1/6] SPI: add ability to say we want a cs change after every transfer.
Quoted 7 lines · Mark Brown
Quoted 4 lines · Jonathan Cameron
This allows a number of drivers to make use of utility functions such
spi_write_then_readas well as making use of regmap possible.Signed-off-by Jonathan Cameron <[email protected]>We could add this in regmap if required.
Makes more sense in spi and has been in their 'may be needed comment' from the first. Also, lots of other users who don't fit into regmap (because they don't have any registers to speak of :)
Quoted 5 lines
--
To unsubscribe from this list: send the line "unsubscribe linux-iio" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/6] SPI: add ability to say we want a cs change after every transfer.
Quoted 7 lines · Jonathan Cameron
Quoted 1 line · Mark Brown
We could add this in regmap if required.
Makes more sense in spi and has been in their 'may be needed comment' from the first. Also, lots of other users who don't fit into regmap (because they don't have any registers to speak of :)
Right, but obviously anything doing SPI transfers can just set the flag in the transfers anyway. It looked like you were adding this to the core due to other intervening abstractions, probably this is just a problem with your changelog.
Re: [PATCH 1/6] SPI: add ability to say we want a cs change after every transfer.
Quoted 5 lines · Mark Brown
Quoted 3 lines · Jonathan Cameron
Quoted 1 line · Mark Brown
We could add this in regmap if required.
Quoted 8 lines
Quoted 3 lines
Makes more sense in spi and has been in their 'may be needed comment' from the first. Also, lots of other users who don't fit into regmap (because they don't have any registers to speak of :)
Right, but obviously anything doing SPI transfers can just set the flag in the transfers anyway. It looked like you were adding this to the core due to other intervening abstractions, probably this is just a problem with your changelog.
I'll make it clearer that it has uses there elsewhere as well not to mention merely stopping stupid bugs in drivers where this is always needed.
Quoted 5 lines
--
To unsubscribe from this list: send the line "unsubscribe linux-iio" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
cover Using regmap with ADIS devices.
In reply to Jonathan CameronAim - allow all the advantages of regmap to work with the 'unusual' spi interfaces of Analog Devices ADIS parts.
Missing - means of feeding results of burst reads back into regmap.
The spi change clearly needs to be circulated much more widely. I'll do that at a later date.
It 'might' be possible to make regmap understand the non symmetric nature of these registers (reads are 16 bit, writes 8 ;) but I couldn't really see how to do it cleanly - hence this proposal.
Anyhow, read and poke fun at it. It's not even vaguely cleaned up. I want to get suggestions on how to do it better before I waste time on doing that.
To sumarise main trick. Treat all registers as 16 bit and mess with the register addresses in the write function to get it to split them in two.
Note I've halved all register addresses in the driver as that will work better for caching in regmap at a later date.
Jonathan
Jonathan Cameron (6):
SPI: add ability to say we want a cs change after every transfer.
regmap: Add a magic bus type to handle quirks of analog devices ADIS
sensors.
staging:iio:imu: adis16400 partial conversion to regmap. regmap-spi-adi + staging:iio:imu:adis16400 halve register addresses regmap-spi-adi generalize regmap_spi_read. staging:iio:imu:adis16400 make use of regmap bulk read capabilities
drivers/base/regmap/Kconfig | 5 +- drivers/base/regmap/Makefile | 1 + drivers/base/regmap/regmap-spi-adi.c | 93 +++++++++ drivers/spi/spi.c | 8 + drivers/staging/iio/imu/Kconfig | 1 + drivers/staging/iio/imu/adis16400.h | 100 +++++----- drivers/staging/iio/imu/adis16400_core.c | 300 ++++++++++++++++++----------- drivers/staging/iio/imu/adis16400_ring.c | 69 ++------ include/linux/regmap.h | 2 + include/linux/spi/spi.h | 2 + 10 files changed, 360 insertions(+), 221 deletions(-) create mode 100644 drivers/base/regmap/regmap-spi-adi.c
Signature · 1 line
1.7.3.4
PATCH 2/6 regmap: Add a magic bus type to handle quirks of analog devices ADIS sensors.
In reply to Jonathan CameronThese devices look like 8 bit registers for writes and 16 bit registers for reads. As you might imagine this causes some 'issues' hence this regmap bus implementation claims they are always 16bit and does the mangling to make the writes work.
--- drivers/base/regmap/Kconfig | 5 ++- drivers/base/regmap/Makefile | 1 + drivers/base/regmap/regmap-spi-adi.c | 70 ++++++++++++++++++++++++++++++++++ include/linux/regmap.h | 2 + 4 files changed, 77 insertions(+), 1 deletions(-)
drivers/base/regmap/Kconfig +4 -1
@@ -3,7 +3,7 @@ # subsystems should select the appropriate symbols. config REGMAP - default y if (REGMAP_I2C || REGMAP_SPI) + default y if (REGMAP_I2C || REGMAP_SPI || REGMAP_SPI_ADI) bool config REGMAP_I2C
@@ -11,3 +11,6 @@ config REGMAP_I2C config REGMAP_SPI tristate + +config REGMAP_SPI_ADI + tristate
drivers/base/regmap/Makefile +1 -0
@@ -2,3 +2,4 @@ obj-$(CONFIG_REGMAP) += regmap.o obj-$(CONFIG_DEBUG_FS) += regmap-debugfs.o obj-$(CONFIG_REGMAP_I2C) += regmap-i2c.o obj-$(CONFIG_REGMAP_SPI) += regmap-spi.o +obj-$(CONFIG_REGMAP_SPI_ADI) += regmap-spi-adi.o
drivers/base/regmap/regmap-spi-adi.c new file +70 -0
70 lines not on this page · raw message
include/linux/regmap.h +2 -0
@@ -111,6 +111,8 @@ struct regmap *regmap_init_i2c(struct i2c_client *i2c, const struct regmap_config *config);struct regmap *regmap_init_spi (struct spi_device *dev,const struct regmap_config *config); +struct regmap *regmap_init_spi_adi (struct spi_device *dev, +const struct regmap_config *config);void regmap_exit (struct regmap *map);int regmap_write (struct regmap *map,unsigned int reg,unsigned int val);
Signature · 1 line
1.7.3.4
Re: [PATCH 2/6] regmap: Add a magic bus type to handle quirks of analog devices ADIS sensors.
Quoted 5 lines · Jonathan Cameron
These devices look like 8 bit registers for writes and 16 bit registers for reads. As you might imagine this causes some 'issues' hence this regmap bus implementation claims they are always 16bit and does the mangling to make the writes work.
---
You've not signed this off. To be honest I'm not terribly happy about pushing this into the regmap core code; if we start needing to do stuff like this we should expose the bus interface.
drivers/base/regmap/regmap-spi-adi.c line 23
Quoted 7 lines
drivers/base/regmap/regmap-spi-adi.c new file +7
-0+ +static int regmap_spi_write (struct device *dev,const void *data,size_t count) +{ +/* Now this only works for 8 bit addresss 16 bit register first byte of data +* is the lower address, second two the value */ +struct spi_device *spi =to_spi_device (dev); +int ret;
Indentation.
drivers/base/regmap/regmap-spi-adi.c line 52
Quoted 4 lines
drivers/base/regmap/regmap-spi-adi.c new file +4
-0+static struct regmap_bus regmap_spi_adi = { + .write = regmap_spi_write, + .read = regmap_spi_read, +};
You want to implement the gather write too if you can.
Re: [PATCH 2/6] regmap: Add a magic bus type to handle quirks of analog devices ADIS sensors.
Quoted 8 lines · Mark Brown
Quoted 5 lines · Jonathan Cameron
These devices look like 8 bit registers for writes and 16 bit registers for reads. As you might imagine this causes some 'issues' hence this regmap bus implementation claims they are always 16bit and does the mangling to make the writes work.
---You've not signed this off.
Indeed. Hence I'm not happy with it :) Just put it out there for comment. Weird spi usage is hardly unusual so these issues were bound to pop up at some point.
Quoted 3 lines
To be honest I'm not terribly happy about
pushing this into the regmap core code; if we start needing to do stuff like this we should expose the bus interface.
That's certainly an option, but I'd really like to use the regmap caching stuff in here. These things can have quite a few registers that other than their weird read / write quirks look much like any other register based device. (particularly ignoring the burst reads but they tend to apply to volatile registers only so caching is probably irrelevant).
At the moment, the only hooks AFAIKS to allow this are at the bus level. I'm not sure where else they could go. (I haven't actually looked much at the cache code yet though).
Actually I may have misunderstood, do you mean expose the bus interface within regmap or just not use regmap at all?
Quoted 11 lines
drivers/base/regmap/regmap-spi-adi.c line 23
Quoted 8 lines
drivers/base/regmap/regmap-spi-adi.c new file +7
-0+ +static int regmap_spi_write (struct device *dev,const void *data,size_t count) +{ +/* Now this only works for 8 bit addresss 16 bit register first byte of data +* is the lower address, second two the value */ +struct spi_device *spi =to_spi_device (dev); +int ret;Indentation.
Quoted 6 lines
drivers/base/regmap/regmap-spi-adi.c line 52
Quoted 4 lines
drivers/base/regmap/regmap-spi-adi.c new file +4
-0+static struct regmap_bus regmap_spi_adi = { + .write = regmap_spi_write, + .read = regmap_spi_read, +};You want to implement the gather write too if you can.
Doesn't really exist other than by linearising them into a series of calls to the write function. (assuming I understand what those functions are doing right!)
Quoted 5 lines
--
To unsubscribe from this list: send the line "unsubscribe linux-iio" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/6] regmap: Add a magic bus type to handle quirks of analog devices ADIS sensors.
Quoted 13 lines · Jonathan Cameron
Quoted 5 lines · Mark Brown
On Wed, Sep 07, 2011 at 05:19:43PM +0100, Jonathan Cameron wrote:
To be honest I'm not terribly happy about
pushing this into the regmap core code; if we start needing to do stuff like this we should expose the bus interface.That's certainly an option, but I'd really like to use the regmap caching stuff in here. These things can have quite a few registers that other than their weird read / write quirks look much like any other register based device. (particularly ignoring the burst reads but they tend to apply to volatile registers only so caching is probably irrelevant).
Quoted 3 lines
At the moment, the only hooks AFAIKS to allow this are at the bus level. I'm not sure where else they could go. (I haven't actually looked much at the cache code yet though).
Well, if there's not hooks they could be added if it's not too painful. However I'm not convinced that's sane.
Quoted 2 lines
Actually I may have misunderstood, do you mean expose the bus interface within regmap or just not use regmap at all?
Either. Or either expose marshalling or add configuration for the marshalling depending on what's being done, I'm not convinced you need to be working at SPI level at all.
Quoted 10 lines
Quoted 6 lines
drivers/base/regmap/regmap-spi-adi.c line 52
Quoted 4 lines
drivers/base/regmap/regmap-spi-adi.c new file +4
-0+static struct regmap_bus regmap_spi_adi = { + .write = regmap_spi_write, + .read = regmap_spi_read, +};You want to implement the gather write too if you can.
Doesn't really exist other than by linearising them into a series of calls to the write function. (assuming I understand what those functions are doing right!)
If you build up a series of SPI transfers you just need to control when /CS gets bounced appropriately.
Re: [PATCH 2/6] regmap: Add a magic bus type to handle quirks of analog devices ADIS sensors.
Quoted 21 lines · Mark Brown
Quoted 13 lines · Jonathan Cameron
Quoted 5 lines · Mark Brown
On Wed, Sep 07, 2011 at 05:19:43PM +0100, Jonathan Cameron wrote:
To be honest I'm not terribly happy about
pushing this into the regmap core code; if we start needing to do stuff like this we should expose the bus interface.That's certainly an option, but I'd really like to use the regmap caching stuff in here. These things can have quite a few registers that other than their weird read / write quirks look much like any other register based device. (particularly ignoring the burst reads but they tend to apply to volatile registers only so caching is probably irrelevant).
Quoted 3 lines
At the moment, the only hooks AFAIKS to allow this are at the bus level. I'm not sure where else they could go. (I haven't actually looked much at the cache code yet though).
Well, if there's not hooks they could be added if it's not too painful. However I'm not convinced that's sane.
Quoted 6 lines
Quoted 2 lines
Actually I may have misunderstood, do you mean expose the bus interface within regmap or just not use regmap at all?
Either. Or either expose marshalling or add configuration for the marshalling depending on what's being done, I'm not convinced you need to be working at SPI level at all.
I'm a little unclear what you mean by 'marshalling'. Could you expand on how that might work?
Quoted 8 lines
Quoted 7 lines
Quoted 7 lines
drivers/base/regmap/regmap-spi-adi.c line 52
Quoted 5 lines
drivers/base/regmap/regmap-spi-adi.c new file +4
-0+static struct regmap_bus regmap_spi_adi = { + .write = regmap_spi_write, + .read = regmap_spi_read, +};You want to implement the gather write too if you can.
Quoted 6 lines
Quoted 3 lines
Doesn't really exist other than by linearising them into a series of calls to the write function. (assuming I understand what those functions are doing right!)
If you build up a series of SPI transfers you just need to control when /CS gets bounced appropriately.
Sure, might be worth doing. To be honest I don't really care about writes as these are write rarely read often devices other than single register writes to clear interrupts (even that is often a read).
Quoted 5 lines
--
To unsubscribe from this list: send the line "unsubscribe linux-iio" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/6] regmap: Add a magic bus type to handle quirks of analog devices ADIS sensors.
Quoted 8 lines · Jonathan Cameron
Quoted 3 lines · Mark Brown
Either. Or either expose marshalling or add configuration for the marshalling depending on what's being done, I'm not convinced you need to be working at SPI level at all.
I'm a little unclear what you mean by 'marshalling'. Could you expand on how that might work?
Marshalling is the process of formatting data for transmit and parsing it once it's been read in.
Re: [PATCH 2/6] regmap: Add a magic bus type to handle quirks of analog devices ADIS sensors.
Quoted 7 lines · Mark Brown
Quoted 5 lines · Jonathan Cameron
Quoted 3 lines · Mark Brown
Either. Or either expose marshalling or add configuration for the marshalling depending on what's being done, I'm not convinced you need to be working at SPI level at all.
Quoted 5 lines
Quoted 2 lines
I'm a little unclear what you mean by 'marshalling'. Could you expand on how that might work?
Marshalling is the process of formatting data for transmit and parsing it once it's been read in.
Ah, got you now. Yes, something in there might work or as you say add some hooks to do it outside.
PATCH 3/6 staging:iio:imu: adis16400 partial conversion to regmap.
In reply to Jonathan CameronThis requires the regmap-spi-adi regmap bus to deal with complex read / write combinations.
--- drivers/staging/iio/imu/Kconfig | 1 + drivers/staging/iio/imu/adis16400.h | 2 + drivers/staging/iio/imu/adis16400_core.c | 300 ++++++++++++++++++----------- drivers/staging/iio/imu/adis16400_ring.c | 2 +- 4 files changed, 190 insertions(+), 115 deletions(-)
drivers/staging/iio/imu/Kconfig +1 -0
@@ -6,6 +6,7 @@ comment "Inertial measurement units" config ADIS16400 tristate "Analog Devices ADIS16400 and similar IMU SPI driver" depends on SPI + select REGMAP_SPI_ADI select IIO_SW_RING if IIO_RING_BUFFER select IIO_TRIGGER if IIO_RING_BUFFER help
drivers/staging/iio/imu/adis16400.h +2 -0
@@ -141,6 +141,7 @@ struct adis16400_chip_info {unsigned long default_scan_mask; }; +struct regmap ;/** * struct adis16400_state - device instance specific data * @us: actual spi_device
@@ -150,6 +151,7 @@ struct adis16400_chip_info {* @buf_lock: mutex to protect tx and rx **/ struct adis16400_state { +struct regmap *regmap;struct spi_device *us;struct iio_trigger *trig;struct mutex buf_lock;
drivers/staging/iio/imu/adis16400_core.c +186 -114
478 lines not on this page · raw message
drivers/staging/iio/imu/adis16400_ring.c +1 -1
@@ -47,7 +47,7 @@ static int adis16400_spi_read_burst(struct device *dev, u8 *rx)spi_setup (st->us); ret =spi_sync (st->us, &msg); -if (ret) +if (ret <0 )dev_err (&st->us->dev,"problem when burst reading" ); st->us->max_speed_hz = old_speed_hz;
Signature · 1 line
1.7.3.4
Re: [PATCH 3/6] staging:iio:imu: adis16400 partial conversion to regmap.
Quoted 2 lines · Jonathan Cameron
This requires the regmap-spi-adi regmap bus to deal with complex read / write combinations.
Forgot to mention that I cheated here in that all supported devices claim all registers exist though for some they
are undefined. The devices won't actually try reading from
them and it won't do any harm if the debug code does so. Just won't get meaningful values for non existent sensors output registers. Can clean this up, but at the cost of a fair bit of complexity as will need to work it out from the chan_spec array.
Quoted 555 lines
Signed-off-by Jonathan Cameron <[email protected]>--- drivers/staging/iio/imu/Kconfig | 1 + drivers/staging/iio/imu/adis16400.h | 2 + drivers/staging/iio/imu/adis16400_core.c | 300 ++++++++++++++++++----------- drivers/staging/iio/imu/adis16400_ring.c | 2 +- 4 files changed, 190 insertions(+), 115 deletions(-)drivers/staging/iio/imu/Kconfig +1
-0@@ -6,6 +6,7 @@ comment "Inertial measurement units" config ADIS16400 tristate "Analog Devices ADIS16400 and similar IMU SPI driver" depends on SPI + select REGMAP_SPI_ADI select IIO_SW_RING if IIO_RING_BUFFER select IIO_TRIGGER if IIO_RING_BUFFER helpdrivers/staging/iio/imu/adis16400.h +2
-0@@ -141,6 +141,7 @@ struct adis16400_chip_info {unsigned long default_scan_mask; }; +struct regmap ;/** * struct adis16400_state - device instance specific data * @us: actual spi_device @@ -150,6 +151,7 @@ struct adis16400_chip_info {* @buf_lock: mutex to protect tx and rx **/ struct adis16400_state { +struct regmap *regmap;struct spi_device *us;struct iio_trigger *trig;struct mutex buf_lock;drivers/staging/iio/imu/adis16400_core.c +186
-114@@ -25,6 +25,8 @@#include <linux/sysfs.h> #include <linux/list.h> #include <linux/module.h> +#include <linux/regmap.h> +#include <linux/err.h> #include "../iio.h" #include "../sysfs.h" @@ -42,28 +44,6 @@ enum adis16400_chip_variant { ADIS16400, }; -/** -* adis16400_spi_write_reg_8() - write single byte to a register -* @dev: device associated with child of actual device (iio_dev or iio_trig) -* @reg_address: the address of the register to be written -* @val: the value to write -*/ -static int adis16400_spi_write_reg_8 (struct iio_dev *indio_dev, -u8 reg_address, -u8 val) -{ -int ret; -struct adis16400_state *st =iio_priv (indio_dev); - -mutex_lock (&st->buf_lock); - st->tx[0 ] =ADIS16400_WRITE_REG (reg_address); - st->tx[1 ] = val; - - ret =spi_write (st->us, st->tx,2 ); -mutex_unlock (&st->buf_lock); - -return ret; -}/** * adis16400_spi_write_reg_16() - write 2 bytes to a pair of registers @@ -71,43 +51,14 @@ static int adis16400_spi_write_reg_8(struct iio_dev *indio_dev,* @reg_address: the address of the lower of the two registers. Second register * is assumed to have address one greater. * @val: value to be written -* -* At the moment the spi framework doesn't allow global setting of cs_change. -* This means that use cannot be made of spi_write. */ static int adis16400_spi_write_reg_16 (struct iio_dev *indio_dev, -u8 lower_reg_address, -u16 value) +u8 lower_reg_address, +u16 value) { -int ret; -struct spi_message msg;struct adis16400_state *st =iio_priv (indio_dev); -struct spi_transfer xfers[] = { - { - .tx_buf = st->tx, - .bits_per_word =8 , - .len =2 , - .cs_change =1 , - }, { - .tx_buf = st->tx +2 , - .bits_per_word =8 , - .len =2 , - }, - }; - -mutex_lock (&st->buf_lock); - st->tx[0 ] =ADIS16400_WRITE_REG (lower_reg_address); - st->tx[1 ] = value &0xFF ; - st->tx[2 ] =ADIS16400_WRITE_REG (lower_reg_address +1 ); - st->tx[3 ] = (value >>8 ) &0xFF ; -spi_message_init (&msg); -spi_message_add_tail (&xfers[0 ], &msg); -spi_message_add_tail (&xfers[1 ], &msg); - ret =spi_sync (st->us, &msg); -mutex_unlock (&st->buf_lock); - -return ret; +return regmap_write (st->regmap, lower_reg_address, value); }/** @@ -116,49 +67,22 @@ static int adis16400_spi_write_reg_16(struct iio_dev *indio_dev,* @reg_address: the address of the lower of the two registers. Second register * is assumed to have address one greater. * @val: somewhere to pass back the value read -* -* At the moment the spi framework doesn't allow global setting of cs_change. -* This means that use cannot be made of spi_read. **/ static int adis16400_spi_read_reg_16 (struct iio_dev *indio_dev, -u8 lower_reg_address, -u16 *val) +u8 lower_reg_address, +u16 *val) { -struct spi_message msg;struct adis16400_state *st =iio_priv (indio_dev);int ret; -struct spi_transfer xfers[] = { - { - .tx_buf = st->tx, - .bits_per_word =8 , - .len =2 , - .cs_change =1 , - }, { - .rx_buf = st->rx, - .bits_per_word =8 , - .len =2 , - }, - }; +unsigned int value; -mutex_lock (&st->buf_lock); - st->tx[0 ] =ADIS16400_READ_REG (lower_reg_address); - st->tx[1 ] =0 ; - -spi_message_init (&msg); -spi_message_add_tail (&xfers[0 ], &msg); -spi_message_add_tail (&xfers[1 ], &msg); - ret =spi_sync (st->us, &msg); -if (ret) { -dev_err (&st->us->dev, -"problem when reading 16 bit register 0x%02X" , - lower_reg_address); - goto error_ret; - } - *val = (st->rx[0 ] <<8 ) | st->rx[1 ]; + ret =regmap_read (st->regmap, lower_reg_address, &value); +if (ret <0 ) +return ret; -error_ret: -mutex_unlock (&st->buf_lock); -return ret; + *val = value; + +return 0 ; } static ssize_t adis16400_read_frequency(struct device *dev,@@ -172,7 +96,7 @@ static ssize_t adis16400_read_frequency(struct device *dev, ret =adis16400_spi_read_reg_16 (indio_dev,ADIS16400_SMPL_PRD , &t); -if (ret) +if (ret <0 )return ret; sps = (t &ADIS16400_SMPL_PRD_TIME_BASE ) ?53 :1638 ; sps /= (t &ADIS16400_SMPL_PRD_DIV_MASK ) +1 ;@@ -192,7 +116,7 @@ static ssize_t adis16400_write_frequency(struct device *dev,u8 t; ret =strict_strtol (buf,10 , &val); -if (ret) +if (ret <0 )return ret;mutex_lock (&indio_dev->mlock);@@ -206,22 +130,22 @@ static ssize_t adis16400_write_frequency(struct device *dev,else st->us->max_speed_hz =ADIS16400_SPI_FAST ; - ret =adis16400_spi_write_reg_8 (indio_dev, + ret =adis16400_spi_write_reg_16 (indio_dev,ADIS16400_SMPL_PRD , t);mutex_unlock (&indio_dev->mlock); -return ret ? ret : len; +return ret <0 ? ret : len; }static int adis16400_reset (struct iio_dev *indio_dev) {int ret; - ret =adis16400_spi_write_reg_8 (indio_dev, + ret =adis16400_spi_write_reg_16 (indio_dev,ADIS16400_GLOB_CMD ,ADIS16400_GLOB_CMD_SW_RESET ); -if (ret) +if (ret <0 )dev_err (&indio_dev->dev,"problem resetting device" );return ret;@@ -252,7 +176,7 @@ int adis16400_set_irq(struct iio_dev *indio_dev, bool enable)u16 msc; ret =adis16400_spi_read_reg_16 (indio_dev,ADIS16400_MSC_CTRL , &msc); -if (ret) +if (ret <0 ) goto error_ret; msc |=ADIS16400_MSC_CTRL_DATA_RDY_POL_HIGH ;@@ -262,8 +186,6 @@ int adis16400_set_irq(struct iio_dev *indio_dev, bool enable) msc &= ~ADIS16400_MSC_CTRL_DATA_RDY_EN ; ret =adis16400_spi_write_reg_16 (indio_dev,ADIS16400_MSC_CTRL , msc); -if (ret) - goto error_ret; error_ret:return ret;@@ -338,7 +260,7 @@ static int adis16400_self_test(struct iio_dev *indio_dev) ret =adis16400_spi_write_reg_16 (indio_dev,ADIS16400_MSC_CTRL ,ADIS16400_MSC_CTRL_MEM_TEST ); -if (ret) { +if (ret <0 ) {dev_err (&indio_dev->dev,"problem starting self test" ); goto err_ret; }@@ -362,24 +284,24 @@ static int adis16400_initial_setup(struct iio_dev *indio_dev)spi_setup (st->us); ret =adis16400_set_irq (indio_dev, false); -if (ret) { +if (ret <0 ) {dev_err (&indio_dev->dev,"disable irq failed" ); goto err_ret; } ret =adis16400_self_test (indio_dev); -if (ret) { +if (ret <0 ) {dev_err (&indio_dev->dev,"self test failure" ); goto err_ret; } ret =adis16400_check_status (indio_dev); -if (ret) { +if (ret <0 ) {adis16400_reset (indio_dev);dev_err (&indio_dev->dev,"device not playing ball -> reset" );msleep (ADIS16400_STARTUP_DELAY ); ret =adis16400_check_status (indio_dev); -if (ret) { +if (ret <0 ) {dev_err (&indio_dev->dev,"giving up" ); goto err_ret; }@@ -387,7 +309,7 @@ static int adis16400_initial_setup(struct iio_dev *indio_dev)if (st->variant->flags &ADIS16400_HAS_PROD_ID ) { ret =adis16400_spi_read_reg_16 (indio_dev,ADIS16400_PRODUCT_ID , &prod_id); -if (ret) +if (ret <0 ) goto err_ret;if ((prod_id &0xF000 ) != st->variant->product_id)@@ -492,7 +414,7 @@ static int adis16400_read_raw(struct iio_dev *indio_dev, ret =adis16400_spi_read_reg_16 (indio_dev, adis16400_addresses[chan->address][0 ], &val16); -if (ret) { +if (ret <0 ) {mutex_unlock (&indio_dev->mlock);return ret; }@@ -539,7 +461,7 @@ static int adis16400_read_raw(struct iio_dev *indio_dev, adis16400_addresses[chan->address][1 ], &val16);mutex_unlock (&indio_dev->mlock); -if (ret) +if (ret <0 )return ret; val16 = ((val16 &0xFFF ) <<4 ) >>4 ; *val = val16;@@ -1016,6 +938,146 @@ static const struct iio_info adis16400_info = { .attrs = &adis16400_attribute_group, }; +/* This may technically result in read attempts to undefined registers */ +static bool adis16400_reg_readable (struct device *dev, +unsigned int reg) +{ +switch (reg) { +case ADIS16400_FLASH_CNT : +case ADIS16400_SUPPLY_OUT : +case ADIS16400_XGYRO_OUT : +case ADIS16400_YGYRO_OUT : +case ADIS16400_ZGYRO_OUT : +case ADIS16400_XACCL_OUT : +case ADIS16400_YACCL_OUT : +case ADIS16400_ZACCL_OUT : +case ADIS16400_XMAGN_OUT : +case ADIS16400_YMAGN_OUT : +case ADIS16400_ZMAGN_OUT : +case ADIS16400_TEMP_OUT : +case ADIS16400_AUX_ADC : +case ADIS16400_XGYRO_OFF : +case ADIS16400_YGYRO_OFF : +case ADIS16400_ZGYRO_OFF : +case ADIS16400_XACCL_OFF : +case ADIS16400_YACCL_OFF : +case ADIS16400_ZACCL_OFF : +case ADIS16400_XMAGN_HIF : +case ADIS16400_YMAGN_HIF : +case ADIS16400_ZMAGN_HIF : +case ADIS16400_XMAGN_SIF : +case ADIS16400_YMAGN_SIF : +case ADIS16400_ZMAGN_SIF : +case ADIS16400_GPIO_CTRL : +case ADIS16400_MSC_CTRL : +case ADIS16400_SMPL_PRD : +case ADIS16400_SENS_AVG : +case ADIS16400_DIAG_STAT : +case ADIS16400_ALM_MAG1 : +case ADIS16400_ALM_MAG2 : +case ADIS16400_ALM_SMPL1 : +case ADIS16400_ALM_SMPL2 : +case ADIS16400_ALM_CTRL : +case ADIS16400_AUX_DAC : +case ADIS16400_PRODUCT_ID : +return true; +default : +return false; + }; +} + +static bool adis16400_reg_precious (struct device *dev, +unsigned int reg) +{ +switch (reg) { +case ADIS16400_SUPPLY_OUT : +case ADIS16400_XGYRO_OUT : +case ADIS16400_YGYRO_OUT : +case ADIS16400_ZGYRO_OUT : +case ADIS16400_XACCL_OUT : +case ADIS16400_YACCL_OUT : +case ADIS16400_ZACCL_OUT : +case ADIS16400_XMAGN_OUT : +case ADIS16400_YMAGN_OUT : +case ADIS16400_ZMAGN_OUT : +case ADIS16400_TEMP_OUT : +case ADIS16400_AUX_ADC : +case ADIS16400_DIAG_STAT : +return true; +default : +return 0 ; + } +} + +static bool adis16400_reg_volatile (struct device *dev, +unsigned int reg) +{ +switch (reg) { +case ADIS16400_FLASH_CNT : +case ADIS16400_SUPPLY_OUT : +case ADIS16400_XGYRO_OUT : +case ADIS16400_YGYRO_OUT : +case ADIS16400_ZGYRO_OUT : +case ADIS16400_XACCL_OUT : +case ADIS16400_YACCL_OUT : +case ADIS16400_ZACCL_OUT : +case ADIS16400_XMAGN_OUT : +case ADIS16400_YMAGN_OUT : +case ADIS16400_ZMAGN_OUT : +case ADIS16400_TEMP_OUT : +case ADIS16400_AUX_ADC : +case ADIS16400_DIAG_STAT : + +return true; +default : +return 0 ; + } +} +static bool adis16400_reg_writeable (struct device *dev, +unsigned int reg) +{ +switch (reg) { +case ADIS16400_XGYRO_OFF : +case ADIS16400_YGYRO_OFF : +case ADIS16400_ZGYRO_OFF : +case ADIS16400_XACCL_OFF : +case ADIS16400_YACCL_OFF : +case ADIS16400_ZACCL_OFF : +case ADIS16400_XMAGN_HIF : +case ADIS16400_YMAGN_HIF : +case ADIS16400_ZMAGN_HIF : +case ADIS16400_XMAGN_SIF : +case ADIS16400_YMAGN_SIF : +case ADIS16400_ZMAGN_SIF : +case ADIS16400_GPIO_CTRL : +case ADIS16400_MSC_CTRL : +case ADIS16400_SMPL_PRD : +case ADIS16400_SENS_AVG : +case ADIS16400_SLP_CNT : +case ADIS16400_GLOB_CMD : +case ADIS16400_ALM_MAG1 : +case ADIS16400_ALM_MAG2 : +case ADIS16400_ALM_SMPL1 : +case ADIS16400_ALM_SMPL2 : +case ADIS16400_ALM_CTRL : +case ADIS16400_AUX_DAC : +return true; +default : +return false; + }; +} +static struct regmap_config adis16400_regmap_config = { + .reg_bits =8 , + .val_bits =16 , + .writeable_reg = &adis16400_reg_writeable, + .readable_reg = &adis16400_reg_readable, + .precious_reg = &adis16400_reg_precious, + .volatile_reg = &adis16400_reg_volatile, + .max_register =0x56 , + .write_flag_mask =0x80 , + .read_flag_mask =0 , +}; +static int __devinit adis16400_probe (struct spi_device *spi) {int ret;@@ -1026,8 +1088,14 @@ static int __devinit adis16400_probe(struct spi_device *spi) goto error_ret; } st =iio_priv (indio_dev); + st->regmap =regmap_init_spi_adi (spi, &adis16400_regmap_config); +if (IS_ERR (st->regmap)) { + ret =PTR_ERR (st->regmap); + goto error_free_dev; + }/* this is only used for removal purposes */ spi_set_drvdata (spi, indio_dev); + spi->cs_between_transfers =1 ; st->us = spi;mutex_init (&st->buf_lock);@@ -1042,29 +1110,29 @@ static int __devinit adis16400_probe(struct spi_device *spi) indio_dev->modes =INDIO_DIRECT_MODE ; ret =adis16400_configure_ring (indio_dev); -if (ret) - goto error_free_dev; +if (ret <0 ) + goto error_free_regmap; ret =iio_ring_buffer_register (indio_dev, st->variant->channels, st->variant->num_channels); -if (ret) { +if (ret <0 ) {dev_err (&spi->dev,"failed to initialize the ring\n" ); goto error_unreg_ring_funcs; }if (spi->irq) { ret =adis16400_probe_trigger (indio_dev); -if (ret) +if (ret <0 ) goto error_uninitialize_ring; }/* Get the device into a sane initial state */ ret =adis16400_initial_setup (indio_dev); -if (ret) +if (ret <0 ) goto error_remove_trigger; ret =iio_device_register (indio_dev); -if (ret) +if (ret <0 ) goto error_remove_trigger;return 0 ;@@ -1076,6 +1144,8 @@ error_uninitialize_ring:iio_ring_buffer_unregister (indio_dev); error_unreg_ring_funcs:adis16400_unconfigure_ring (indio_dev); +error_free_regmap: +regmap_exit (st->regmap); error_free_dev:iio_free_device (indio_dev); error_ret:@@ -1087,14 +1157,16 @@ static int adis16400_remove(struct spi_device *spi) {int ret;struct iio_dev *indio_dev =spi_get_drvdata (spi); +struct adis16400_state *st =iio_priv (indio_dev); ret =adis16400_stop_device (indio_dev); -if (ret) +if (ret <0 ) goto err_ret;adis16400_remove_trigger (indio_dev);iio_ring_buffer_unregister (indio_dev);adis16400_unconfigure_ring (indio_dev); +regmap_exit (st->regmap);iio_device_unregister (indio_dev);return 0 ;drivers/staging/iio/imu/adis16400_ring.c +1
-1@@ -47,7 +47,7 @@ static int adis16400_spi_read_burst(struct device *dev, u8 *rx)spi_setup (st->us); ret =spi_sync (st->us, &msg); -if (ret) +if (ret <0 )dev_err (&st->us->dev,"problem when burst reading" ); st->us->max_speed_hz = old_speed_hz;
PATCH 4/6 regmap-spi-adi + staging:iio:imu:adis16400 halve register addresses
In reply to Jonathan CameronThe register addresses were even numbers only. That makes for very bad caching, so lets halve them all.
Nasty trick needed with the write_flag_mask being halved to make this work.
--- drivers/base/regmap/regmap-spi-adi.c | 8 ++- drivers/staging/iio/imu/adis16400.h | 88 +++++++++++++++--------------- drivers/staging/iio/imu/adis16400_core.c | 2 +- drivers/staging/iio/imu/adis16400_ring.c | 22 ++++---- 4 files changed, 61 insertions(+), 59 deletions(-)
drivers/base/regmap/regmap-spi-adi.c +5 -3
@@ -25,14 +25,13 @@ static int regmap_spi_write(struct device *dev, const void *data, size_t count)BUG_ON (count !=3 );/* Fiddling needed as the value is bigendian */ - rawdata[0 ] +=1 ; + rawdata[0 ] = (rawdata[0 ] <<1 ) +1 ; ret =spi_write (spi, data,2 );if (ret <0 )return ret; rawdata[1 ] = rawdata[0 ] -1 ; - ret =spi_write (spi, data +1 ,2 ); -return ret; +return spi_write (spi, data +1 ,2 ); } static int regmap_spi_read(struct device *dev,
@@ -40,6 +39,9 @@ static int regmap_spi_read(struct device *dev, void *val, size_t val_size) {struct spi_device *spi =to_spi_device (dev); +u8 *regraw = (u8 *)reg; + + regraw[0 ] <<=1 ;BUG_ON (reg_size !=1 || val_size !=2 );
drivers/staging/iio/imu/adis16400.h +48 -48
106 lines not on this page · raw message
drivers/staging/iio/imu/adis16400_core.c +1 -1
@@ -1074,7 +1074,7 @@ static struct regmap_config adis16400_regmap_config = { .precious_reg = &adis16400_reg_precious, .volatile_reg = &adis16400_reg_volatile, .max_register = 0x56, - .write_flag_mask = 0x80, + .write_flag_mask = 0x40, /* doubling of this occurs */ .read_flag_mask = 0, };
drivers/staging/iio/imu/adis16400_ring.c +11 -11
@@ -57,17 +57,17 @@ static int adis16400_spi_read_burst(struct device *dev, u8 *rx) }static const u16 read_all_tx_array[] = { -cpu_to_be16 (ADIS16400_READ_REG (ADIS16400_SUPPLY_OUT )), -cpu_to_be16 (ADIS16400_READ_REG (ADIS16400_XGYRO_OUT )), -cpu_to_be16 (ADIS16400_READ_REG (ADIS16400_YGYRO_OUT )), -cpu_to_be16 (ADIS16400_READ_REG (ADIS16400_ZGYRO_OUT )), -cpu_to_be16 (ADIS16400_READ_REG (ADIS16400_XACCL_OUT )), -cpu_to_be16 (ADIS16400_READ_REG (ADIS16400_YACCL_OUT )), -cpu_to_be16 (ADIS16400_READ_REG (ADIS16400_ZACCL_OUT )), -cpu_to_be16 (ADIS16400_READ_REG (ADIS16350_XTEMP_OUT )), -cpu_to_be16 (ADIS16400_READ_REG (ADIS16350_YTEMP_OUT )), -cpu_to_be16 (ADIS16400_READ_REG (ADIS16350_ZTEMP_OUT )), -cpu_to_be16 (ADIS16400_READ_REG (ADIS16400_AUX_ADC )), +cpu_to_be16 (ADIS16400_READ_REG (ADIS16400_SUPPLY_OUT <<1 )), +cpu_to_be16 (ADIS16400_READ_REG (ADIS16400_XGYRO_OUT <<1 )), +cpu_to_be16 (ADIS16400_READ_REG (ADIS16400_YGYRO_OUT <<1 )), +cpu_to_be16 (ADIS16400_READ_REG (ADIS16400_ZGYRO_OUT <<1 )), +cpu_to_be16 (ADIS16400_READ_REG (ADIS16400_XACCL_OUT <<1 )), +cpu_to_be16 (ADIS16400_READ_REG (ADIS16400_YACCL_OUT <<1 )), +cpu_to_be16 (ADIS16400_READ_REG (ADIS16400_ZACCL_OUT <<1 )), +cpu_to_be16 (ADIS16400_READ_REG (ADIS16350_XTEMP_OUT <<1 )), +cpu_to_be16 (ADIS16400_READ_REG (ADIS16350_YTEMP_OUT <<1 )), +cpu_to_be16 (ADIS16400_READ_REG (ADIS16350_ZTEMP_OUT <<1 )), +cpu_to_be16 (ADIS16400_READ_REG (ADIS16400_AUX_ADC <<1 )), };static int adis16350_spi_read_all (struct device *dev,u8 *rx)
Signature · 1 line
1.7.3.4
PATCH 5/6 regmap-spi-adi generalize regmap_spi_read.
In reply to Jonathan CameronComplexity here is that we have to break the transers after every word so that the chip select can go high.
Note this is NOT a burst read, but rather a interleaved register read - there may well be better ways of doing this.
The burst read supported by for example the adis16400 is too odd to fit well into regmap.
--- drivers/base/regmap/regmap-spi-adi.c | 31 ++++++++++++++++++++++++++----- 1 files changed, 26 insertions(+), 5 deletions(-)
drivers/base/regmap/regmap-spi-adi.c +26 -5
@@ -23,7 +23,7 @@ static int regmap_spi_write(struct device *dev, const void *data, size_t count)int ret;u8 *rawdata = (u8 *)data; -BUG_ON (count !=3 ); +// BUG_ON(count != 3); /* Fiddling needed as the value is bigendian */ rawdata[0 ] = (rawdata[0 ] <<1 ) +1 ; ret =spi_write (spi, data,2 );
@@ -39,13 +39,34 @@ static int regmap_spi_read(struct device *dev, void *val, size_t val_size) {struct spi_device *spi =to_spi_device (dev); -u8 *regraw = (u8 *)reg; +struct spi_transfer *xfers; +struct spi_message msg; +int i, ret; +u8 startreg = ((u8 *)reg)[0 ] <<1 ; - regraw[0 ] <<=1 ; +/* Could keep a single register read special version */ + xfers =kzalloc (sizeof (*xfers)*((val_size >>1 ) +1 ),GFP_KERNEL ); +if (xfers ==NULL ) +return -ENOMEM ; + +for (i =0 ; i < val_size >>1 ; i++) { + xfers[i].tx_buf = (u16 *)val + i; + ((u8 *)val)[i*2 ] = startreg +2 *i; + ((u8 *)val)[i*2 +1 ] =0 ; + xfers[i +1 ].rx_buf = (u16 *)val + i; + xfers[i].bits_per_word =8 ; + xfers[i].len =2 ; + } + xfers[i].bits_per_word =8 ; + xfers[i].len =2 ; +spi_message_init (&msg); +for (i =0 ; i < (val_size >>1 ) +1 ; i++) +spi_message_add_tail (&xfers[i], &msg); -BUG_ON (reg_size !=1 || val_size !=2 ); + ret =spi_sync (spi, &msg); +kfree (xfers); -return spi_write_then_read (spi, reg,2 , val, val_size); +return ret; }static struct regmap_bus regmap_spi_adi = {
Signature · 1 line
1.7.3.4
PATCH 6/6 staging:iio:imu:adis16400 make use of regmap bulk read capabilities
In reply to Jonathan CameronNote this only works because of the spi cs change default fun.
--- drivers/staging/iio/imu/adis16400.h | 4 +- drivers/staging/iio/imu/adis16400_ring.c | 67 +++++------------------------- 2 files changed, 13 insertions(+), 58 deletions(-)
drivers/staging/iio/imu/adis16400.h +2 -2
@@ -122,7 +122,7 @@/* SLP_CNT */ #define ADIS16400_SLP_CNT_POWER_OFF (1<<8) -#define ADIS16400_MAX_TX 24 +#define ADIS16400_MAX_TX 2#define ADIS16400_MAX_RX 24#define ADIS16400_SPI_SLOW (u32)(300 * 1000)
@@ -158,7 +158,7 @@ struct adis16400_state {struct adis16400_chip_info *variant;u8 tx[ADIS16400_MAX_TX ]____cacheline_aligned ; -u8 rx[ADIS16400_MAX_RX ]____cacheline_aligned ; +u16 rx[ADIS16400_MAX_RX >>1 ]____cacheline_aligned ; };int adis16400_set_irq (struct iio_dev *indio_dev,bool enable);
drivers/staging/iio/imu/adis16400_ring.c +11 -56
113 lines not on this page · raw message
Signature · 1 line
1.7.3.4
On Wed, Sep 07, 2011 at 05:10:24PM +0100, Jonathan Cameron wrote:
Please fix your mailer to word wrap at less than 80 colums, they're really quite hard to read as a result of this.
Quoted 6 lines
CS -_______________________-
TX Ada0...Ada7 Da0....Da7
RX XXXXXXXXXXXXXXXXXXXXXXXXXReads are 16 bit with either of the two 8 bit register addresses given the same value
Quoted 7 lines
CS -______________________-_____________________-
TX Ada0....Ada7 XXXXXXXX
RX XXXXXXXXXXXXXXXXXXXXX Da0...Da7 Db0....Db7Can interpret Da0...Da7 and Db0....Db7 as single 16 bit register and consider this device to just have a weird write method and normal read. Might be easier. I'll define Ax<n> as 16 bit address for the burst read.
This is starting to seem pretty far off the reservation.
Quoted 3 lines
Perhaps the burst mode thing is better handled by just providing a hook to allow data to be pushed into regmap (from 'magic' sources), but the weird write read combination looks to me like something that makes sense to have in regmap (be it as another bus variant).
Probably not as a bus, it sounds like a marshalling difference rather than a bus - the buses should really only understand byte streams. I don't have any bright ideas on how to deal with this, it's fairly far away from the problem space I'm worried about.
Quoted 15 lines · Mark Brown
On Wed, Sep 07, 2011 at 05:10:24PM +0100, Jonathan Cameron wrote:
Please fix your mailer to word wrap at less than 80 colums, they're really quite hard to read as a result of this.
Quoted 9 lines
CS -_______________________-
TX Ada0...Ada7 Da0....Da7
RX XXXXXXXXXXXXXXXXXXXXXXXXXReads are 16 bit with either of the two 8 bit register addresses given the same value
CS -______________________-_____________________-
TX Ada0....Ada7 XXXXXXXX
RX XXXXXXXXXXXXXXXXXXXXX Da0...Da7 Db0....Db7
Quoted 5 lines
Quoted 3 lines
Can interpret Da0...Da7 and Db0....Db7 as single 16 bit register and consider this device to just have a weird write method and normal read. Might be easier. I'll define Ax<n> as 16 bit address for the burst read.
This is starting to seem pretty far off the reservation.
Indeed.
Quoted 9 lines
Quoted 4 lines
Perhaps the burst mode thing is better handled by just providing a hook to allow data to be pushed into regmap (from 'magic' sources), but the weird write read combination looks to me like something that makes sense to have in regmap (be it as another bus variant).
Probably not as a bus, it sounds like a marshalling difference rather than a bus - the buses should really only understand byte streams. I don't have any bright ideas on how to deal with this, it's fairly far away from the problem space I'm worried about.
The right answer maybe that it isn't a good idea to do it at all, but it wasn't obvious until I tried! Having done it I'm not sure either way.
Quoted 5 lines
--
To unsubscribe from this list: send the line "unsubscribe linux-iio" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html