[PATCH 1/7] regmap: Move initialization of regcache related fields to regcache_init

[PATCH 1/7] regmap: Move initialization of regcache related fields to regcache_init

[Lars-Peter Clausen]

Move the initialization regcache related fields of the regmap struct to
regcache_init. This allows us to keep regmap and regcache code better
separated.

Signed-off-by: Lars-Peter Clausen <lars@metafoo.de>
---
 drivers/base/regmap/internal.h |    2 +-
 drivers/base/regmap/regcache.c |    9 ++++++++-
 drivers/base/regmap/regmap.c   |    8 +-------
 3 files changed, 10 insertions(+), 9 deletions(-)

diff --git a/drivers/base/regmap/internal.h b/drivers/base/regmap/internal.h
index 6483e0b..954f7b7 100644
--- a/drivers/base/regmap/internal.h
+++ b/drivers/base/regmap/internal.h
@@ -106,7 +106,7 @@ static inline void regmap_debugfs_exit(struct regmap *map) { }
 #endif
 
 /* regcache core declarations */
-int regcache_init(struct regmap *map);
+int regcache_init(struct regmap *map, const struct regmap_config *config);
 void regcache_exit(struct regmap *map);
 int regcache_read(struct regmap *map,
 		       unsigned int reg, unsigned int *value);
diff --git a/drivers/base/regmap/regcache.c b/drivers/base/regmap/regcache.c
index 27fae58..0ad6cfb 100644
--- a/drivers/base/regmap/regcache.c
+++ b/drivers/base/regmap/regcache.c
@@ -87,7 +87,7 @@ err_free:
 	return ret;
 }
 
-int regcache_init(struct regmap *map)
+int regcache_init(struct regmap *map, const struct regmap_config *config)
 {
 	int ret;
 	int i;
@@ -108,6 +108,13 @@ int regcache_init(struct regmap *map)
 		return -EINVAL;
 	}
 
+	map->reg_defaults = config->reg_defaults;
+	map->num_reg_defaults = config->num_reg_defaults;
+	map->num_reg_defaults_raw = config->num_reg_defaults_raw;
+	map->reg_defaults_raw = config->reg_defaults_raw;
+	map->cache_size_raw = (config->val_bits / 8) * config->num_reg_defaults_raw;
+	map->cache_word_size = config->val_bits / 8;
+
 	map->cache = NULL;
 	map->cache_ops = cache_types[i];
 
diff --git a/drivers/base/regmap/regmap.c b/drivers/base/regmap/regmap.c
index b84ebf9..3cf4785 100644
--- a/drivers/base/regmap/regmap.c
+++ b/drivers/base/regmap/regmap.c
@@ -159,12 +159,6 @@ struct regmap *regmap_init(struct device *dev,
 	map->volatile_reg = config->volatile_reg;
 	map->precious_reg = config->precious_reg;
 	map->cache_type = config->cache_type;
-	map->reg_defaults = config->reg_defaults;
-	map->num_reg_defaults = config->num_reg_defaults;
-	map->num_reg_defaults_raw = config->num_reg_defaults_raw;
-	map->reg_defaults_raw = config->reg_defaults_raw;
-	map->cache_size_raw = (config->val_bits / 8) * config->num_reg_defaults_raw;
-	map->cache_word_size = config->val_bits / 8;
 
 	if (config->read_flag_mask || config->write_flag_mask) {
 		map->read_flag_mask = config->read_flag_mask;
@@ -227,7 +221,7 @@ struct regmap *regmap_init(struct device *dev,
 		goto err_map;
 	}
 
-	ret = regcache_init(map);
+	ret = regcache_init(map, config);
 	if (ret < 0)
 		goto err_free_workbuf;
 
-- 
1.7.7.1

[PATCH 2/7] regmap: Make reg_config reg_defaults const

[Lars-Peter Clausen]

The reg_defaults field usually points to a static per driver array, which should
not be modified. Make requirement this explicit by making reg_defaults const.
To allow this the regcache_init code needs some minor changes. Previoulsy the
reg_config was not available in regcache_init and regmap->reg_defaults was used
to pass the default register set to regcache_init. Now that the reg_config is
available we can work on it directly.

Signed-off-by: Lars-Peter Clausen <lars@metafoo.de>
---
 drivers/base/regmap/regcache.c |    5 ++---
 include/linux/regmap.h         |    2 +-
 2 files changed, 3 insertions(+), 4 deletions(-)

diff --git a/drivers/base/regmap/regcache.c b/drivers/base/regmap/regcache.c
index 0ad6cfb..d687df6 100644
--- a/drivers/base/regmap/regcache.c
+++ b/drivers/base/regmap/regcache.c
@@ -108,7 +108,6 @@ int regcache_init(struct regmap *map, const struct regmap_config *config)
 		return -EINVAL;
 	}
 
-	map->reg_defaults = config->reg_defaults;
 	map->num_reg_defaults = config->num_reg_defaults;
 	map->num_reg_defaults_raw = config->num_reg_defaults_raw;
 	map->reg_defaults_raw = config->reg_defaults_raw;
@@ -127,10 +126,10 @@ int regcache_init(struct regmap *map, const struct regmap_config *config)
 	 * won't vanish from under us.  We'll need to make
 	 * a copy of it.
 	 */
-	if (map->reg_defaults) {
+	if (config->reg_defaults) {
 		if (!map->num_reg_defaults)
 			return -EINVAL;
-		tmp_buf = kmemdup(map->reg_defaults, map->num_reg_defaults *
+		tmp_buf = kmemdup(config->reg_defaults, map->num_reg_defaults *
 				  sizeof(struct reg_default), GFP_KERNEL);
 		if (!tmp_buf)
 			return -ENOMEM;
diff --git a/include/linux/regmap.h b/include/linux/regmap.h
index 1e4ec2b..458f15f 100644
--- a/include/linux/regmap.h
+++ b/include/linux/regmap.h
@@ -83,7 +83,7 @@ struct regmap_config {
 	bool (*precious_reg)(struct device *dev, unsigned int reg);
 
 	unsigned int max_register;
-	struct reg_default *reg_defaults;
+	const struct reg_default *reg_defaults;
 	unsigned int num_reg_defaults;
 	enum regcache_type cache_type;
 	const void *reg_defaults_raw;
-- 
1.7.7.1

Re: [PATCH 2/7] regmap: Make reg_config reg_defaults const

[Mark Brown]

On Wed, Nov 16, 2011 at 04:28:17PM +0100, Lars-Peter Clausen wrote:
> The reg_defaults field usually points to a static per driver array, which should
> not be modified. Make requirement this explicit by making reg_defaults const.

You should've got reams of sparse warnings from that - the indexed cache
is writing to the defaults table (which is very naughty and i keep
thinking about deleting it as a result).

Re: [PATCH 2/7] regmap: Make reg_config reg_defaults const

[Lars-Peter Clausen]

On 11/16/2011 05:13 PM, Mark Brown wrote:
> On Wed, Nov 16, 2011 at 04:28:17PM +0100, Lars-Peter Clausen wrote:
>> The reg_defaults field usually points to a static per driver array, which should
>> not be modified. Make requirement this explicit by making reg_defaults const.
> 
> You should've got reams of sparse warnings from that - the indexed cache
> is writing to the defaults table (which is very naughty and i keep
> thinking about deleting it as a result).

This patch makes reg_defaults const in the reg_config struct not in the
regmap struct.

Since we duplicate the reg_config reg_defaults into the regmap reg_defaults
the index cache works on a runtime allocated array. (Though I wouldn't
object to see it gone. For the average uscase it uses more memory and is
slower than the rbnode cache).

- Lars

Re: [PATCH 2/7] regmap: Make reg_config reg_defaults const

[Mark Brown]

On Wed, Nov 16, 2011 at 05:23:33PM +0100, Lars-Peter Clausen wrote:
> On 11/16/2011 05:13 PM, Mark Brown wrote:

> > You should've got reams of sparse warnings from that - the indexed cache
> > is writing to the defaults table (which is very naughty and i keep
> > thinking about deleting it as a result).

> This patch makes reg_defaults const in the reg_config struct not in the
> regmap struct.

> Since we duplicate the reg_config reg_defaults into the regmap reg_defaults
> the index cache works on a runtime allocated array. (Though I wouldn't
> object to see it gone. For the average uscase it uses more memory and is
> slower than the rbnode cache).

Did you actually check the sparse output?

Re: [PATCH 2/7] regmap: Make reg_config reg_defaults const

[Lars-Peter Clausen]

On 11/16/2011 05:24 PM, Mark Brown wrote:
> On Wed, Nov 16, 2011 at 05:23:33PM +0100, Lars-Peter Clausen wrote:
>> On 11/16/2011 05:13 PM, Mark Brown wrote:
> 
>>> You should've got reams of sparse warnings from that - the indexed cache
>>> is writing to the defaults table (which is very naughty and i keep
>>> thinking about deleting it as a result).
> 
>> This patch makes reg_defaults const in the reg_config struct not in the
>> regmap struct.
> 
>> Since we duplicate the reg_config reg_defaults into the regmap reg_defaults
>> the index cache works on a runtime allocated array. (Though I wouldn't
>> object to see it gone. For the average uscase it uses more memory and is
>> slower than the rbnode cache).
> 
> Did you actually check the sparse output?

No, because the only place we ever look at config->reg_defaults is in
regache_init. And now I just run sparse and as expected regmap doesn't
trigger any warnings or errors.

Re: [PATCH 2/7] regmap: Make reg_config reg_defaults const

[Mark Brown]

On Wed, Nov 16, 2011 at 05:36:20PM +0100, Lars-Peter Clausen wrote:
> On 11/16/2011 05:24 PM, Mark Brown wrote:

> > Did you actually check the sparse output?

> No, because the only place we ever look at config->reg_defaults is in
> regache_init. And now I just run sparse and as expected regmap doesn't
> trigger any warnings or errors.

  CHECK   drivers/base/regmap/regmap.c
drivers/base/regmap/regmap.c:162:27: warning: incorrect type in assignment (different modifiers)
drivers/base/regmap/regmap.c:162:27:    expected struct reg_default *reg_defaults
drivers/base/regmap/regmap.c:162:27:    got struct reg_default const *const reg_defaults

Re: [PATCH 2/7] regmap: Make reg_config reg_defaults const

[Lars-Peter Clausen]

On 11/16/2011 05:39 PM, Mark Brown wrote:
> On Wed, Nov 16, 2011 at 05:36:20PM +0100, Lars-Peter Clausen wrote:
>> On 11/16/2011 05:24 PM, Mark Brown wrote:
> 
>>> Did you actually check the sparse output?
> 
>> No, because the only place we ever look at config->reg_defaults is in
>> regache_init. And now I just run sparse and as expected regmap doesn't
>> trigger any warnings or errors.
> 
>   CHECK   drivers/base/regmap/regmap.c
> drivers/base/regmap/regmap.c:162:27: warning: incorrect type in assignment (different modifiers)
> drivers/base/regmap/regmap.c:162:27:    expected struct reg_default *reg_defaults
> drivers/base/regmap/regmap.c:162:27:    got struct reg_default const *const reg_defaults

Hu? which patches did you apply? If I have regmap/for-next plus patch 1 and
patch 2 of this series there isn't even a reference to reg_defaults in regmap.c

Re: [PATCH 2/7] regmap: Make reg_config reg_defaults const

[Mark Brown]

On Wed, Nov 16, 2011 at 05:50:45PM +0100, Lars-Peter Clausen wrote:

> Hu? which patches did you apply? If I have regmap/for-next plus patch 1 and
> patch 2 of this series there isn't even a reference to reg_defaults in regmap.c

None of them, I just changed the code to make the defaults const - I'm
just reviewing by eyeball as I'm well aware of the fact that this isn't
const.

Re: [PATCH 2/7] regmap: Make reg_config reg_defaults const

[Lars-Peter Clausen]

On 11/16/2011 05:51 PM, Mark Brown wrote:
> On Wed, Nov 16, 2011 at 05:50:45PM +0100, Lars-Peter Clausen wrote:
> 
>> Hu? which patches did you apply? If I have regmap/for-next plus patch 1 and
>> patch 2 of this series there isn't even a reference to reg_defaults in regmap.c
> 
> None of them, I just changed the code to make the defaults const - I'm
> just reviewing by eyeball as I'm well aware of the fact that this isn't
> const.

So you only applied part of the patch and are complaining that it you get an
error?

Re: [PATCH 2/7] regmap: Make reg_config reg_defaults const

[Mark Brown]

On Wed, Nov 16, 2011 at 06:01:40PM +0100, Lars-Peter Clausen wrote:
> On 11/16/2011 05:51 PM, Mark Brown wrote:

> > None of them, I just changed the code to make the defaults const - I'm
> > just reviewing by eyeball as I'm well aware of the fact that this isn't
> > const.

> So you only applied part of the patch and are complaining that it you get an
> error?

I'm saying that the way we're using the reg_defaults is unfortunately
excessively cute right now so just by looking at the code I'm able to
see that there's an issue having done the same change before but not
applied it due to the cuteness.  I only did a code mod to generate the
warnings since you weren't seeing any issue.

Re: [PATCH 2/7] regmap: Make reg_config reg_defaults const

[Lars-Peter Clausen]

On 11/16/2011 06:09 PM, Mark Brown wrote:
> On Wed, Nov 16, 2011 at 06:01:40PM +0100, Lars-Peter Clausen wrote:
>> On 11/16/2011 05:51 PM, Mark Brown wrote:
> 
>>> None of them, I just changed the code to make the defaults const - I'm
>>> just reviewing by eyeball as I'm well aware of the fact that this isn't
>>> const.
> 
>> So you only applied part of the patch and are complaining that it you get an
>> error?
> 
> I'm saying that the way we're using the reg_defaults is unfortunately
> excessively cute right now so just by looking at the code I'm able to
> see that there's an issue having done the same change before but not
> applied it due to the cuteness.  I only did a code mod to generate the
> warnings since you weren't seeing any issue.

Sorry, I can't follow you anymore. Do you still see a problem with the patch
as it is, and if yes why?

We never ever modify reg_defaults of the reg_config we only ever modify the
reg_defaults of the regmap struct which is always runtime allocated. In the
case the config contains reg_defaults it is kmemdup'd into the reg_defaults
of the regmap struct.

Re: [PATCH 2/7] regmap: Make reg_config reg_defaults const

[Mark Brown]

On Wed, Nov 16, 2011 at 06:20:28PM +0100, Lars-Peter Clausen wrote:

> Sorry, I can't follow you anymore. Do you still see a problem with the patch
> as it is, and if yes why?

No, it's probably fine.

Re: [PATCH 2/7] regmap: Make reg_config reg_defaults const

[Mark Brown]

On Wed, Nov 16, 2011 at 04:28:17PM +0100, Lars-Peter Clausen wrote:
> The reg_defaults field usually points to a static per driver array, which should
> not be modified. Make requirement this explicit by making reg_defaults const.

Applied, thanks.

[PATCH 3/7] regmap: Properly round cache_word_size

[Lars-Peter Clausen]

regcache currently only properly works with val sizes of 8 or 16. For all other
val sizes it will round them down to the next multiple of 8, which will cause
the cache to be too small. This patch fixes it by rounding the cache_word_size
up to the nearest storage size.

Signed-off-by: Lars-Peter Clausen <lars@metafoo.de>
---
 drivers/base/regmap/regcache.c |   11 +++++++++--
 1 files changed, 9 insertions(+), 2 deletions(-)

diff --git a/drivers/base/regmap/regcache.c b/drivers/base/regmap/regcache.c
index d687df6..6f0dbc0 100644
--- a/drivers/base/regmap/regcache.c
+++ b/drivers/base/regmap/regcache.c
@@ -111,8 +111,15 @@ int regcache_init(struct regmap *map, const struct regmap_config *config)
 	map->num_reg_defaults = config->num_reg_defaults;
 	map->num_reg_defaults_raw = config->num_reg_defaults_raw;
 	map->reg_defaults_raw = config->reg_defaults_raw;
-	map->cache_size_raw = (config->val_bits / 8) * config->num_reg_defaults_raw;
-	map->cache_word_size = config->val_bits / 8;
+
+	if (config->val_bits <= 8)
+		map->cache_word_size = 1;
+	else if (config->val_bits <= 16)
+		map->cache_word_size = 2;
+	else
+		return -EINVAL;
+
+	map->cache_size_raw = map->cache_word_size * config->num_reg_defaults_raw;
 
 	map->cache = NULL;
 	map->cache_ops = cache_types[i];
-- 
1.7.7.1

Re: [PATCH 3/7] regmap: Properly round cache_word_size

[Mark Brown]

On Wed, Nov 16, 2011 at 04:28:18PM +0100, Lars-Peter Clausen wrote:

> +	if (config->val_bits <= 8)
> +		map->cache_word_size = 1;
> +	else if (config->val_bits <= 16)
> +		map->cache_word_size = 2;
> +	else
> +		return -EINVAL;

Just do val_bits + 7 if you're going to do this.

Re: [PATCH 3/7] regmap: Properly round cache_word_size

[Lars-Peter Clausen]

On 11/16/2011 05:14 PM, Mark Brown wrote:
> On Wed, Nov 16, 2011 at 04:28:18PM +0100, Lars-Peter Clausen wrote:
> 
>> +	if (config->val_bits <= 8)
>> +		map->cache_word_size = 1;
>> +	else if (config->val_bits <= 16)
>> +		map->cache_word_size = 2;
>> +	else
>> +		return -EINVAL;
> 
> Just do val_bits + 7 if you're going to do this.

This won't work once we add support for a value bit-width of more than 16
bit. But I can use DIV_ROUND_UP for this patch and we change it once we add
support for 32 bit cache word size.

[PATCH 4/7] regmap: Try cached read before checking if a hardware read is possible

[Lars-Peter Clausen]

For some register format types we do not provide a parse_val so we can not do a
hardware read. But a cached read is still possible, so try to read from the
cache first, before checking whether a hardware read is possible. Otherwise the
cache becomes pretty useless for these register types.

Signed-off-by: Lars-Peter Clausen <lars@metafoo.de>
---
 drivers/base/regmap/regmap.c |    6 +++---
 1 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/base/regmap/regmap.c b/drivers/base/regmap/regmap.c
index 3cf4785..b96cf72 100644
--- a/drivers/base/regmap/regmap.c
+++ b/drivers/base/regmap/regmap.c
@@ -434,15 +434,15 @@ static int _regmap_read(struct regmap *map, unsigned int reg,
 {
 	int ret;
 
-	if (!map->format.parse_val)
-		return -EINVAL;
-
 	if (!map->cache_bypass) {
 		ret = regcache_read(map, reg, val);
 		if (ret == 0)
 			return 0;
 	}
 
+	if (!map->format.parse_val)
+		return -EINVAL;
+
 	if (map->cache_only)
 		return -EBUSY;
 
-- 
1.7.7.1

Re: [PATCH 4/7] regmap: Try cached read before checking if a hardware read is possible

[Mark Brown]

On Wed, Nov 16, 2011 at 04:28:19PM +0100, Lars-Peter Clausen wrote:
> For some register format types we do not provide a parse_val so we can not do a
> hardware read. But a cached read is still possible, so try to read from the
> cache first, before checking whether a hardware read is possible. Otherwise the
> cache becomes pretty useless for these register types.

Applied, thanks.

[PATCH 5/7] regmap: Check if a register is writable instead of readable in regcache_read

[Lars-Peter Clausen]

One of the reasons for using a cache is to have a software shadow of a register
which is writable but not readable. This allows us to do a read-modify-write
operation on such a register.

Currently regcache checks whether a register is readable when performing a
cached read and returns an error if not. Change this check to test whether the
register is writable. This makes more sense, since reading from the cache when
the register is not readable allows the operation described above, but if the
register is not writable there shouldn't  be a value for it in the cache anyway.

Signed-off-by: Lars-Peter Clausen <lars@metafoo.de>
---
 drivers/base/regmap/regcache.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/drivers/base/regmap/regcache.c b/drivers/base/regmap/regcache.c
index 6f0dbc0..a004a7a 100644
--- a/drivers/base/regmap/regcache.c
+++ b/drivers/base/regmap/regcache.c
@@ -206,7 +206,7 @@ int regcache_read(struct regmap *map,
 
 	BUG_ON(!map->cache_ops);
 
-	if (!regmap_readable(map, reg))
+	if (!regmap_writeable(map, reg))
 		return -EIO;
 
 	if (!regmap_volatile(map, reg))
-- 
1.7.7.1

Re: [PATCH 5/7] regmap: Check if a register is writable instead of readable in regcache_read

[Mark Brown]

On Wed, Nov 16, 2011 at 04:28:20PM +0100, Lars-Peter Clausen wrote:

> Currently regcache checks whether a register is readable when performing a
> cached read and returns an error if not. Change this check to test whether the
> register is writable. This makes more sense, since reading from the cache when
> the register is not readable allows the operation described above, but if the
> register is not writable there shouldn't  be a value for it in the cache anyway.

This logic doesn't entirely follow - one can have registers which are
volatile but could be read once at startup.  Plus...

> @@ -206,7 +206,7 @@ int regcache_read(struct regmap *map,
>  
>  	BUG_ON(!map->cache_ops);
>  
> -	if (!regmap_readable(map, reg))
> +	if (!regmap_writeable(map, reg))
>  		return -EIO;

...the code winds up just looking like an obvious bug.

Re: [PATCH 5/7] regmap: Check if a register is writable instead of readable in regcache_read

[Lars-Peter Clausen]

On 11/16/2011 05:16 PM, Mark Brown wrote:
> On Wed, Nov 16, 2011 at 04:28:20PM +0100, Lars-Peter Clausen wrote:
> 
>> Currently regcache checks whether a register is readable when performing a
>> cached read and returns an error if not. Change this check to test whether the
>> register is writable. This makes more sense, since reading from the cache when
>> the register is not readable allows the operation described above, but if the
>> register is not writable there shouldn't  be a value for it in the cache anyway.
> 
> This logic doesn't entirely follow - one can have registers which are
> volatile but could be read once at startup.  Plus...

Hm? The use case here is chips which do not support readback. So we never
want to fallback to a hardware read but still want to be able to do a cached
read.

> 
>> @@ -206,7 +206,7 @@ int regcache_read(struct regmap *map,
>>  
>>  	BUG_ON(!map->cache_ops);
>>  
>> -	if (!regmap_readable(map, reg))
>> +	if (!regmap_writeable(map, reg))
>>  		return -EIO;
> 
> ...the code winds up just looking like an obvious bug.

Why? If a register is not writable we won't have anything in the cache for
it. So reading from the cache for a register which is not writable doesn't
make any sense.

Re: [PATCH 5/7] regmap: Check if a register is writable instead of readable in regcache_read

[Mark Brown]

On Wed, Nov 16, 2011 at 05:34:33PM +0100, Lars-Peter Clausen wrote:
> On 11/16/2011 05:16 PM, Mark Brown wrote:

> > This logic doesn't entirely follow - one can have registers which are
> > volatile but could be read once at startup.  Plus...

> Hm? The use case here is chips which do not support readback. So we never
> want to fallback to a hardware read but still want to be able to do a cached
> read.

This code will be run on every chip, including chips with read/write
access.  Caches are useful for all chips.

> >> @@ -206,7 +206,7 @@ int regcache_read(struct regmap *map,

> >>  	BUG_ON(!map->cache_ops);

> >> -	if (!regmap_readable(map, reg))
> >> +	if (!regmap_writeable(map, reg))
> >>  		return -EIO;

> > ...the code winds up just looking like an obvious bug.

> Why? If a register is not writable we won't have anything in the cache for
> it. So reading from the cache for a register which is not writable doesn't
> make any sense.

If you're looking at the read function and it's checking to see if the
register is writeable the first thought would be that this is a
cut'n'paste error.  The above code is at best *way* too cute.

Re: [PATCH 5/7] regmap: Check if a register is writable instead of readable in regcache_read

[Lars-Peter Clausen]

On 11/16/2011 05:38 PM, Mark Brown wrote:
> On Wed, Nov 16, 2011 at 05:34:33PM +0100, Lars-Peter Clausen wrote:
>> On 11/16/2011 05:16 PM, Mark Brown wrote:
> 
>>> This logic doesn't entirely follow - one can have registers which are
>>> volatile but could be read once at startup.  Plus...
> 
>> Hm? The use case here is chips which do not support readback. So we never
>> want to fallback to a hardware read but still want to be able to do a cached
>> read.
> 
> This code will be run on every chip, including chips with read/write
> access.  Caches are useful for all chips.

Of course. And it still works for chips with read/write support with this
patch, but it doesn't work for chips without read support without this patch.

> 
>>>> @@ -206,7 +206,7 @@ int regcache_read(struct regmap *map,
> 
>>>>  	BUG_ON(!map->cache_ops);
> 
>>>> -	if (!regmap_readable(map, reg))
>>>> +	if (!regmap_writeable(map, reg))
>>>>  		return -EIO;
> 
>>> ...the code winds up just looking like an obvious bug.
> 
>> Why? If a register is not writable we won't have anything in the cache for
>> it. So reading from the cache for a register which is not writable doesn't
>> make any sense.
> 
> If you're looking at the read function and it's checking to see if the
> register is writeable the first thought would be that this is a
> cut'n'paste error.  The above code is at best *way* too cute.

We can of course add a comment explaining why it is regmap_writable instead
of regmap_readable.

Re: [PATCH 5/7] regmap: Check if a register is writable instead of readable in regcache_read

[Mark Brown]

On Wed, Nov 16, 2011 at 05:52:49PM +0100, Lars-Peter Clausen wrote:
> On 11/16/2011 05:38 PM, Mark Brown wrote:

> >> Hm? The use case here is chips which do not support readback. So we never
> >> want to fallback to a hardware read but still want to be able to do a cached
> >> read.

> > This code will be run on every chip, including chips with read/write
> > access.  Caches are useful for all chips.

> Of course. And it still works for chips with read/write support with this
> patch, but it doesn't work for chips without read support without this patch.

No, it'll fail if we ever cache volatile registers at startup (which
is a perfectly sensible thing to do for things like chip revisions -
they're not something we can hard code the default for but they're not
going to change at runtime).

> > If you're looking at the read function and it's checking to see if the
> > register is writeable the first thought would be that this is a
> > cut'n'paste error.  The above code is at best *way* too cute.

> We can of course add a comment explaining why it is regmap_writable instead
> of regmap_readable.

No, really - just do something legible and robust.  For example, teach
regmap_readable() about the cache.

Re: [PATCH 5/7] regmap: Check if a register is writable instead of readable in regcache_read

[Lars-Peter Clausen]

On 11/16/2011 05:56 PM, Mark Brown wrote:
> On Wed, Nov 16, 2011 at 05:52:49PM +0100, Lars-Peter Clausen wrote:
>> On 11/16/2011 05:38 PM, Mark Brown wrote:
> 
>>>> Hm? The use case here is chips which do not support readback. So we never
>>>> want to fallback to a hardware read but still want to be able to do a cached
>>>> read.
> 
>>> This code will be run on every chip, including chips with read/write
>>> access.  Caches are useful for all chips.
> 
>> Of course. And it still works for chips with read/write support with this
>> patch, but it doesn't work for chips without read support without this patch.
> 
> No, it'll fail if we ever cache volatile registers at startup (which
> is a perfectly sensible thing to do for things like chip revisions -
> they're not something we can hard code the default for but they're not
> going to change at runtime).
> 

Ah ok, now I get it, you are talking about that this will hypothetical break
a future patch ;)

>>> If you're looking at the read function and it's checking to see if the
>>> register is writeable the first thought would be that this is a
>>> cut'n'paste error.  The above code is at best *way* too cute.
> 
>> We can of course add a comment explaining why it is regmap_writable instead
>> of regmap_readable.
> 
> No, really - just do something legible and robust.  For example, teach
> regmap_readable() about the cache.

Doesn't make much sense. We call regmap_readable from regcache_read, which
is only called if we use a cache. So if we let regmap_readable return true
in case we use a cache it will always be true in regcache_read and we can
drop the check entirely.

I'll update the patch to just drop the check.

Re: [PATCH 5/7] regmap: Check if a register is writable instead of readable in regcache_read

[Mark Brown]

On Wed, Nov 16, 2011 at 06:09:06PM +0100, Lars-Peter Clausen wrote:
> On 11/16/2011 05:56 PM, Mark Brown wrote:

> > No, really - just do something legible and robust.  For example, teach
> > regmap_readable() about the cache.

> Doesn't make much sense. We call regmap_readable from regcache_read, which
> is only called if we use a cache. So if we let regmap_readable return true
> in case we use a cache it will always be true in regcache_read and we can
> drop the check entirely.

We should at least check that we actually have a cached value there -
the cache is sparse after all.

Re: [PATCH 5/7] regmap: Check if a register is writable instead of readable in regcache_read

[Lars-Peter Clausen]

On 11/16/2011 06:12 PM, Mark Brown wrote:
> On Wed, Nov 16, 2011 at 06:09:06PM +0100, Lars-Peter Clausen wrote:
>> On 11/16/2011 05:56 PM, Mark Brown wrote:
> 
>>> No, really - just do something legible and robust.  For example, teach
>>> regmap_readable() about the cache.
> 
>> Doesn't make much sense. We call regmap_readable from regcache_read, which
>> is only called if we use a cache. So if we let regmap_readable return true
>> in case we use a cache it will always be true in regcache_read and we can
>> drop the check entirely.
> 
> We should at least check that we actually have a cached value there -
> the cache is sparse after all.

That's what the cache already does today, you recently change the rbtree
implementation to return -ENOENT if there is no cached value.

Re: [PATCH 5/7] regmap: Check if a register is writable instead of readable in regcache_read

[Mark Brown]

On Wed, Nov 16, 2011 at 06:15:31PM +0100, Lars-Peter Clausen wrote:
> On 11/16/2011 06:12 PM, Mark Brown wrote:

> > We should at least check that we actually have a cached value there -
> > the cache is sparse after all.

> That's what the cache already does today, you recently change the rbtree
> implementation to return -ENOENT if there is no cached value.

Oh, sorry -  this is in the cache specific code isn't it?  In that case
yes the check is just totally redundant and can be removed on those
grounds alone.

[PATCH 6/7] regmap: Add support for 10/14 register formating

[Lars-Peter Clausen]

This patch adds support for 10 bits register, 14 bits value type register
formating. This is for example used by the Analog Devices AD5380.

Signed-off-by: Lars-Peter Clausen <lars@metafoo.de>
---
 drivers/base/regmap/regmap.c |   20 ++++++++++++++++++++
 1 files changed, 20 insertions(+), 0 deletions(-)

diff --git a/drivers/base/regmap/regmap.c b/drivers/base/regmap/regmap.c
index b96cf72..e533368 100644
--- a/drivers/base/regmap/regmap.c
+++ b/drivers/base/regmap/regmap.c
@@ -90,6 +90,16 @@ static void regmap_format_7_9_write(struct regmap *map,
 	*out = cpu_to_be16((reg << 9) | val);
 }
 
+static void regmap_format_10_14_write(struct regmap *map,
+				    unsigned int reg, unsigned int val)
+{
+	u8 *out = map->work_buf;
+
+	out[2] = val;
+	out[1] = (val >> 8) | (reg << 6);
+	out[0] = reg >> 2;
+}
+
 static void regmap_format_8(void *buf, unsigned int val)
 {
 	u8 *b = buf;
@@ -188,6 +198,16 @@ struct regmap *regmap_init(struct device *dev,
 		}
 		break;
 
+	case 10:
+		switch (config->val_bits) {
+		case 14:
+			map->format.format_write = regmap_format_10_14_write;
+			break;
+		default:
+			goto err_map;
+		}
+		break;
+
 	case 8:
 		map->format.format_reg = regmap_format_8;
 		break;
-- 
1.7.7.1

Re: [PATCH 6/7] regmap: Add support for 10/14 register formating

[Mark Brown]

On Wed, Nov 16, 2011 at 04:28:21PM +0100, Lars-Peter Clausen wrote:

> +
> +	out[2] = val;
> +	out[1] = (val >> 8) | (reg << 6);
> +	out[0] = reg >> 2;
> +}

Applied but it took me a couple of goes to veryify that this was correct
as you're filling things in the opposite order to that which things are
transmitted in.

[PATCH 7/7] staging:iio:dac: Add AD5380 driver

[Lars-Peter Clausen]

This patch adds support for the Analog Devices D5380, AD5381,
AD5382, AD5383, AD5384, AD5390, AD5391, AD5392 multi-channel
Digital to Analog Converters.

Signed-off-by: Lars-Peter Clausen <lars@metafoo.de>

---
There should be no compile time dependencies to the regmap patches earlier in
this series, so this patch can be merged independently of it.
---
 drivers/staging/iio/dac/Kconfig  |   11 +
 drivers/staging/iio/dac/Makefile |    1 +
 drivers/staging/iio/dac/ad5380.c |  669 ++++++++++++++++++++++++++++++++++++++
 3 files changed, 681 insertions(+), 0 deletions(-)
 create mode 100644 drivers/staging/iio/dac/ad5380.c

diff --git a/drivers/staging/iio/dac/Kconfig b/drivers/staging/iio/dac/Kconfig
index a71defb..aaa6d46 100644
--- a/drivers/staging/iio/dac/Kconfig
+++ b/drivers/staging/iio/dac/Kconfig
@@ -24,6 +24,17 @@ config AD5360
 	  To compile this driver as module choose M here: the module will be called
 	  ad5360.
 
+config AD5380
+	tristate "Analog Devices AD5380/81/82/83/84/90/91/92 DAC driver"
+	depends on (SPI_MASTER || I2C)
+	help
+	  Say yes here to build support for Analog Devices AD5380, AD5381,
+	  AD5382, AD5383, AD5384, AD5390, AD5391, AD5392 multi-channel
+	  Digital to Analog Converters (DAC).
+
+	  To compile this driver as module choose M here: the module will be called
+	  ad5380.
+
 config AD5421
 	tristate "Analog Devices AD5421 DAC driver"
 	depends on SPI
diff --git a/drivers/staging/iio/dac/Makefile b/drivers/staging/iio/dac/Makefile
index e75b0c8..840e94e 100644
--- a/drivers/staging/iio/dac/Makefile
+++ b/drivers/staging/iio/dac/Makefile
@@ -3,6 +3,7 @@
 #
 
 obj-$(CONFIG_AD5360) += ad5360.o
+obj-$(CONFIG_AD5380) += ad5380.o
 obj-$(CONFIG_AD5421) += ad5421.o
 obj-$(CONFIG_AD5624R_SPI) += ad5624r_spi.o
 obj-$(CONFIG_AD5064) += ad5064.o
diff --git a/drivers/staging/iio/dac/ad5380.c b/drivers/staging/iio/dac/ad5380.c
new file mode 100644
index 0000000..0d5536a
--- /dev/null
+++ b/drivers/staging/iio/dac/ad5380.c
@@ -0,0 +1,669 @@
+/*
+ * Analog devices AD5380, AD5381, AD5382, AD5383, AD5370, AD5371, AD5373
+ * multi-channel Digital to Analog Converters driver
+ *
+ * Copyright 2011 Analog Devices Inc.
+ *
+ * Licensed under the GPL-2.
+ */
+
+#include <linux/device.h>
+#include <linux/err.h>
+#include <linux/i2c.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/spi/spi.h>
+#include <linux/slab.h>
+#include <linux/sysfs.h>
+#include <linux/regmap.h>
+#include <linux/regulator/consumer.h>
+
+#include "../iio.h"
+#include "../sysfs.h"
+#include "dac.h"
+
+
+#define AD5380_REG_DATA(x)	(((x) << 2) | 3)
+#define AD5380_REG_OFFSET(x)	(((x) << 2) | 2)
+#define AD5380_REG_GAIN(x)	(((x) << 2) | 1)
+#define AD5380_REG_SF_PWR_DOWN	(8 << 2)
+#define AD5380_REG_SF_PWR_UP	(9 << 2)
+#define AD5380_REG_SF_CTRL	(12 << 2)
+
+#define AD5380_CTRL_PWR_DOWN_MODE_OFFSET	13
+#define AD5380_CTRL_INT_VREF_2V5		BIT(12)
+#define AD5380_CTRL_INT_VREF_EN			BIT(10)
+
+/**
+ * struct ad5380_chip_info - chip specific information
+ * @channel_template:	channel specification template
+ * @num_channels:	number of channels
+ * @int_vref:		internal vref in uV
+*/
+
+struct ad5380_chip_info {
+	struct iio_chan_spec	channel_template;
+	unsigned int		num_channels;
+	unsigned int		int_vref;
+};
+
+/**
+ * struct ad5380_state - driver instance specific data
+ * @regmap:		regmap instance used by the device
+ * @chip_info:		chip model specific constants, available modes etc
+ * @vref_reg:		vref supply regulator
+ * @vref:		actual reference voltage used in uA
+ * @pwr_down:		whether the chip is currently in power down mode
+ */
+
+struct ad5380_state {
+	struct regmap			*regmap;
+	const struct ad5380_chip_info	*chip_info;
+	struct regulator		*vref_reg;
+	int				vref;
+	bool				pwr_down;
+};
+
+enum ad5380_type {
+	ID_AD5380_3,
+	ID_AD5380_5,
+	ID_AD5381_3,
+	ID_AD5381_5,
+	ID_AD5382_3,
+	ID_AD5382_5,
+	ID_AD5383_3,
+	ID_AD5383_5,
+	ID_AD5384_3,
+	ID_AD5384_5,
+	ID_AD5390_3,
+	ID_AD5390_5,
+	ID_AD5391_3,
+	ID_AD5391_5,
+	ID_AD5392_3,
+	ID_AD5392_5,
+};
+
+#define AD5380_CHANNEL(_bits) {					\
+	.type = IIO_VOLTAGE,					\
+	.indexed = 1,						\
+	.output = 1,						\
+	.info_mask = IIO_CHAN_INFO_SCALE_SHARED_BIT |		\
+		IIO_CHAN_INFO_CALIBSCALE_SEPARATE_BIT |		\
+		IIO_CHAN_INFO_CALIBBIAS_SEPARATE_BIT,		\
+	.scan_type = IIO_ST('u', (_bits), 16, 14 - (_bits))	\
+}
+
+static const struct ad5380_chip_info ad5380_chip_info_tbl[] = {
+	[ID_AD5380_3] = {
+		.channel_template = AD5380_CHANNEL(14),
+		.num_channels = 40,
+		.int_vref = 1250000,
+	},
+	[ID_AD5380_5] = {
+		.channel_template = AD5380_CHANNEL(14),
+		.num_channels = 40,
+		.int_vref = 2500000,
+	},
+	[ID_AD5381_3] = {
+		.channel_template = AD5380_CHANNEL(12),
+		.num_channels = 16,
+		.int_vref = 1250000,
+	},
+	[ID_AD5381_5] = {
+		.channel_template = AD5380_CHANNEL(12),
+		.num_channels = 16,
+		.int_vref = 2500000,
+	},
+	[ID_AD5382_3] = {
+		.channel_template = AD5380_CHANNEL(14),
+		.num_channels = 32,
+		.int_vref = 1250000,
+	},
+	[ID_AD5382_5] = {
+		.channel_template = AD5380_CHANNEL(14),
+		.num_channels = 32,
+		.int_vref = 2500000,
+	},
+	[ID_AD5383_3] = {
+		.channel_template = AD5380_CHANNEL(12),
+		.num_channels = 32,
+		.int_vref = 1250000,
+	},
+	[ID_AD5383_5] = {
+		.channel_template = AD5380_CHANNEL(12),
+		.num_channels = 32,
+		.int_vref = 2500000,
+	},
+	[ID_AD5390_3] = {
+		.channel_template = AD5380_CHANNEL(14),
+		.num_channels = 16,
+		.int_vref = 1250000,
+	},
+	[ID_AD5390_5] = {
+		.channel_template = AD5380_CHANNEL(14),
+		.num_channels = 16,
+		.int_vref = 2500000,
+	},
+	[ID_AD5391_3] = {
+		.channel_template = AD5380_CHANNEL(12),
+		.num_channels = 16,
+		.int_vref = 1250000,
+	},
+	[ID_AD5392_5] = {
+		.channel_template = AD5380_CHANNEL(12),
+		.num_channels = 16,
+		.int_vref = 2500000,
+	},
+	[ID_AD5392_3] = {
+		.channel_template = AD5380_CHANNEL(14),
+		.num_channels = 8,
+		.int_vref = 1250000,
+	},
+	[ID_AD5392_5] = {
+		.channel_template = AD5380_CHANNEL(14),
+		.num_channels = 8,
+		.int_vref = 2500000,
+	},
+};
+
+static ssize_t ad5380_read_dac_powerdown(struct device *dev,
+	struct device_attribute *attr, char *buf)
+{
+	struct iio_dev *indio_dev = dev_get_drvdata(dev);
+	struct ad5380_state *st = iio_priv(indio_dev);
+
+	return sprintf(buf, "%d\n", st->pwr_down);
+}
+
+static ssize_t ad5380_write_dac_powerdown(struct device *dev,
+	struct device_attribute *attr, const char *buf, size_t len)
+{
+	struct iio_dev *indio_dev = dev_get_drvdata(dev);
+	struct ad5380_state *st = iio_priv(indio_dev);
+	bool pwr_down;
+	int ret;
+
+	ret = strtobool(buf, &pwr_down);
+	if (ret)
+		return ret;
+
+	mutex_lock(&indio_dev->mlock);
+
+	if (pwr_down)
+		ret = regmap_write(st->regmap, AD5380_REG_SF_PWR_DOWN, 0);
+	else
+		ret = regmap_write(st->regmap, AD5380_REG_SF_PWR_UP, 0);
+
+	st->pwr_down = pwr_down;
+
+	mutex_unlock(&indio_dev->mlock);
+
+	return ret ? ret : len;
+}
+
+static IIO_DEVICE_ATTR(out_voltage_powerdown,
+			S_IRUGO | S_IWUSR,
+			ad5380_read_dac_powerdown,
+			ad5380_write_dac_powerdown, 0);
+
+static const char ad5380_powerdown_modes[][15] = {
+	[0]	= "100kohm_to_gnd",
+	[1]	= "three_state",
+};
+
+static ssize_t ad5380_read_powerdown_mode(struct device *dev,
+	struct device_attribute *attr, char *buf)
+{
+	struct iio_dev *indio_dev = dev_get_drvdata(dev);
+	struct ad5380_state *st = iio_priv(indio_dev);
+	unsigned int mode;
+	int ret;
+
+	ret = regmap_read(st->regmap, AD5380_REG_SF_CTRL, &mode);
+	if (ret)
+		return ret;
+
+	mode = (mode >> AD5380_CTRL_PWR_DOWN_MODE_OFFSET) & 1;
+
+	return sprintf(buf, "%s\n",
+		ad5380_powerdown_modes[mode]);
+}
+
+static ssize_t ad5380_write_powerdown_mode(struct device *dev,
+	struct device_attribute *attr, const char *buf, size_t len)
+{
+	struct iio_dev *indio_dev = dev_get_drvdata(dev);
+	struct ad5380_state *st = iio_priv(indio_dev);
+	unsigned int i;
+	int ret;
+
+	for (i = 0; i < ARRAY_SIZE(ad5380_powerdown_modes); ++i) {
+		if (sysfs_streq(buf, ad5380_powerdown_modes[i]))
+			break;
+	}
+
+	if (i == ARRAY_SIZE(ad5380_powerdown_modes))
+		return -EINVAL;
+
+	ret = regmap_update_bits(st->regmap, AD5380_REG_SF_CTRL,
+		1 << AD5380_CTRL_PWR_DOWN_MODE_OFFSET,
+		i << AD5380_CTRL_PWR_DOWN_MODE_OFFSET);
+
+	return ret ? ret : len;
+}
+
+static IIO_DEVICE_ATTR(out_voltage_powerdown_mode,
+			S_IRUGO | S_IWUSR,
+			ad5380_read_powerdown_mode,
+			ad5380_write_powerdown_mode, 0);
+
+static IIO_CONST_ATTR(out_voltage_powerdown_mode_available,
+			"100kohm_to_gnd three_state");
+
+static struct attribute *ad5380_attributes[] = {
+	&iio_dev_attr_out_voltage_powerdown.dev_attr.attr,
+	&iio_dev_attr_out_voltage_powerdown_mode.dev_attr.attr,
+	&iio_const_attr_out_voltage_powerdown_mode_available.dev_attr.attr,
+	NULL,
+};
+
+static const struct attribute_group ad5380_attribute_group = {
+	.attrs = ad5380_attributes,
+};
+
+static unsigned int ad5380_info_to_reg(struct iio_chan_spec const *chan,
+	long info)
+{
+	switch (info) {
+	case 0:
+		return AD5380_REG_DATA(chan->address);
+	case IIO_CHAN_INFO_CALIBBIAS:
+		return AD5380_REG_OFFSET(chan->address);
+	case IIO_CHAN_INFO_CALIBSCALE:
+		return AD5380_REG_GAIN(chan->address);
+	default:
+		BUG();
+		break;
+	}
+
+	return 0;
+}
+
+static int ad5380_write_raw(struct iio_dev *indio_dev,
+	struct iio_chan_spec const *chan, int val, int val2, long info)
+{
+	const unsigned int max_val = (1 << chan->scan_type.realbits);
+	struct ad5380_state *st = iio_priv(indio_dev);
+
+	switch (info) {
+	case IIO_CHAN_INFO_CALIBBIAS:
+		val += (1 << chan->scan_type.realbits) / 2;
+	case 0:
+	case IIO_CHAN_INFO_CALIBSCALE:
+		if (val >= max_val || val < 0)
+			return -EINVAL;
+
+		return regmap_write(st->regmap, ad5380_info_to_reg(chan, info),
+			val << chan->scan_type.shift);
+	default:
+		break;
+	}
+
+	return -EINVAL;
+}
+
+static int ad5380_read_raw(struct iio_dev *indio_dev,
+	struct iio_chan_spec const *chan, int *val, int *val2, long info)
+{
+	struct ad5380_state *st = iio_priv(indio_dev);
+	unsigned long scale_uv;
+	int ret;
+
+	switch (info) {
+	case 0:
+	case IIO_CHAN_INFO_CALIBBIAS:
+	case IIO_CHAN_INFO_CALIBSCALE:
+		ret = regmap_read(st->regmap, ad5380_info_to_reg(chan, info),
+					val);
+		if (ret)
+			return ret;
+
+		if (info == IIO_CHAN_INFO_CALIBSCALE)
+			val -= (1 << chan->scan_type.realbits) / 2;
+
+		*val >>= chan->scan_type.shift;
+
+		return IIO_VAL_INT;
+	case IIO_CHAN_INFO_SCALE:
+		scale_uv = ((2 * st->vref) >> chan->scan_type.realbits) * 100;
+		*val =  scale_uv / 100000;
+		*val2 = (scale_uv % 100000) * 10;
+		return IIO_VAL_INT_PLUS_MICRO;
+	default:
+		return -EINVAL;
+	}
+
+	return IIO_VAL_INT;
+}
+
+static const struct iio_info ad5380_info = {
+	.read_raw = ad5380_read_raw,
+	.write_raw = ad5380_write_raw,
+	.attrs = &ad5380_attribute_group,
+	.driver_module = THIS_MODULE,
+};
+
+static int __devinit ad5380_alloc_channels(struct iio_dev *indio_dev)
+{
+	struct ad5380_state *st = iio_priv(indio_dev);
+	struct iio_chan_spec *channels;
+	unsigned int i;
+
+	channels = kcalloc(sizeof(struct iio_chan_spec),
+			st->chip_info->num_channels, GFP_KERNEL);
+
+	if (!channels)
+		return -ENOMEM;
+
+	for (i = 0; i < st->chip_info->num_channels; ++i) {
+		channels[i] = st->chip_info->channel_template;
+		channels[i].channel = i;
+		channels[i].address = i;
+	}
+
+	indio_dev->channels = channels;
+
+	return 0;
+}
+
+static int __devinit ad5380_probe(struct device *dev, struct regmap *regmap,
+	enum ad5380_type type, const char *name)
+{
+	struct iio_dev *indio_dev;
+	struct ad5380_state *st;
+	unsigned int ctrl = 0;
+	int ret;
+
+	indio_dev = iio_allocate_device(sizeof(*st));
+	if (indio_dev == NULL) {
+		dev_err(dev, "Failed to allocate iio device\n");
+		ret = -ENOMEM;
+		goto error_regmap_exit;
+	}
+
+	st = iio_priv(indio_dev);
+	dev_set_drvdata(dev, indio_dev);
+
+	st->chip_info = &ad5380_chip_info_tbl[type];
+	st->regmap = regmap;
+
+	indio_dev->dev.parent = dev;
+	indio_dev->name = name;
+	indio_dev->info = &ad5380_info;
+	indio_dev->modes = INDIO_DIRECT_MODE;
+	indio_dev->num_channels = st->chip_info->num_channels;
+
+	ret = ad5380_alloc_channels(indio_dev);
+	if (ret) {
+		dev_err(dev, "Failed to allocate channel spec: %d\n", ret);
+		goto error_free;
+	}
+
+	if (st->chip_info->int_vref == 2500000)
+		ctrl |= AD5380_CTRL_INT_VREF_2V5;
+
+	st->vref_reg = regulator_get(dev, "vref");
+	if (!IS_ERR(st->vref_reg)) {
+		ret = regulator_enable(st->vref_reg);
+		if (ret) {
+			dev_err(dev, "Failed to enable vref regulators: %d\n", ret);
+			goto error_free_reg;
+		}
+
+		st->vref = regulator_get_voltage(st->vref_reg);
+	} else {
+		st->vref = st->chip_info->int_vref;
+		ctrl |= AD5380_CTRL_INT_VREF_EN;
+	}
+
+	ret = regmap_write(st->regmap, AD5380_REG_SF_CTRL, ctrl);
+	if (ret) {
+		dev_err(dev, "Failed: %d\n", ret);
+		goto error_disable_reg;
+	}
+
+	ret = iio_device_register(indio_dev);
+	if (ret) {
+		dev_err(dev, "Failed to register iio device: %d\n", ret);
+		goto error_disable_reg;
+	}
+
+	return 0;
+
+error_disable_reg:
+	if (!IS_ERR(st->vref_reg))
+		regulator_disable(st->vref_reg);
+error_free_reg:
+	if (!IS_ERR(st->vref_reg))
+		regulator_put(st->vref_reg);
+
+	kfree(indio_dev->channels);
+error_free:
+	iio_free_device(indio_dev);
+error_regmap_exit:
+	regmap_exit(regmap);
+
+	return ret;
+}
+
+static int __devexit ad5380_remove(struct device *dev)
+{
+	struct iio_dev *indio_dev = dev_get_drvdata(dev);
+	struct ad5380_state *st = iio_priv(indio_dev);
+
+	iio_device_unregister(indio_dev);
+
+	kfree(indio_dev->channels);
+
+	if (!IS_ERR(st->vref_reg)) {
+		regulator_disable(st->vref_reg);
+		regulator_put(st->vref_reg);
+	}
+
+	iio_free_device(indio_dev);
+	regmap_exit(st->regmap);
+
+	return 0;
+}
+
+static bool ad5380_reg_false(struct device *dev, unsigned int reg)
+{
+	return false;
+}
+
+static const struct regmap_config ad5380_regmap_config = {
+	.reg_bits = 10,
+	.val_bits = 14,
+
+	.max_register = AD5380_REG_DATA(40),
+	.cache_type = REGCACHE_RBTREE,
+
+	.volatile_reg = ad5380_reg_false,
+	.readable_reg = ad5380_reg_false,
+};
+
+#if IS_ENABLED(CONFIG_SPI_MASTER)
+
+static int __devinit ad5380_spi_probe(struct spi_device *spi)
+{
+	const struct spi_device_id *id = spi_get_device_id(spi);
+	struct regmap *regmap;
+
+	regmap = regmap_init_spi(spi, &ad5380_regmap_config);
+
+	if (IS_ERR(regmap))
+		return PTR_ERR(regmap);
+
+	return ad5380_probe(&spi->dev, regmap,
+		id->driver_data, id->name);
+}
+
+static int __devexit ad5380_spi_remove(struct spi_device *spi)
+{
+	return ad5380_remove(&spi->dev);
+}
+
+static const struct spi_device_id ad5380_spi_ids[] = {
+	{ "ad5380-3", ID_AD5380_3 },
+	{ "ad5380-5", ID_AD5380_5 },
+	{ "ad5381-3", ID_AD5381_3 },
+	{ "ad5381-5", ID_AD5381_5 },
+	{ "ad5382-3", ID_AD5382_3 },
+	{ "ad5382-5", ID_AD5382_5 },
+	{ "ad5383-3", ID_AD5383_3 },
+	{ "ad5383-5", ID_AD5383_5 },
+	{ "ad5390-3", ID_AD5380_3 },
+	{ "ad5390-5", ID_AD5380_5 },
+	{ "ad5391-3", ID_AD5381_3 },
+	{ "ad5391-5", ID_AD5381_5 },
+	{ "ad5392-3", ID_AD5382_3 },
+	{ "ad5392-5", ID_AD5382_5 },
+	{ }
+};
+MODULE_DEVICE_TABLE(spi, ad5380_ids);
+
+static struct spi_driver ad5380_spi_driver = {
+	.driver = {
+		   .name = "ad5380",
+		   .owner = THIS_MODULE,
+	},
+	.probe = ad5380_spi_probe,
+	.remove = __devexit_p(ad5380_spi_remove),
+	.id_table = ad5380_spi_ids,
+};
+
+static inline int ad5380_spi_register_driver(void)
+{
+	return spi_register_driver(&ad5380_spi_driver);
+}
+
+static inline void ad5380_spi_unregister_driver(void)
+{
+	spi_unregister_driver(&ad5380_spi_driver);
+}
+
+#else
+
+static inline int ad5380_spi_register_driver(void)
+{
+	return 0;
+}
+
+static inline void ad5380_spi_unregister_driver(void)
+{
+}
+
+#endif
+
+#if IS_ENABLED(CONFIG_I2C)
+
+static int __devinit ad5380_i2c_probe(struct i2c_client *i2c,
+	const struct i2c_device_id *id)
+{
+	struct regmap *regmap;
+
+	regmap = regmap_init_i2c(i2c, &ad5380_regmap_config);
+
+	if (IS_ERR(regmap))
+		return PTR_ERR(regmap);
+
+	return ad5380_probe(&i2c->dev, regmap,
+		id->driver_data, id->name);
+}
+
+static int __devexit ad5380_i2c_remove(struct i2c_client *i2c)
+{
+	return ad5380_remove(&i2c->dev);
+}
+
+static const struct i2c_device_id ad5380_i2c_ids[] = {
+	{ "ad5380-3", ID_AD5380_3 },
+	{ "ad5380-5", ID_AD5380_5 },
+	{ "ad5381-3", ID_AD5381_3 },
+	{ "ad5381-5", ID_AD5381_5 },
+	{ "ad5382-3", ID_AD5382_3 },
+	{ "ad5382-5", ID_AD5382_5 },
+	{ "ad5383-3", ID_AD5383_3 },
+	{ "ad5383-5", ID_AD5383_5 },
+	{ "ad5390-3", ID_AD5380_3 },
+	{ "ad5390-5", ID_AD5380_5 },
+	{ "ad5391-3", ID_AD5381_3 },
+	{ "ad5391-5", ID_AD5381_5 },
+	{ "ad5392-3", ID_AD5382_3 },
+	{ "ad5392-5", ID_AD5382_5 },
+	{ }
+};
+MODULE_DEVICE_TABLE(i2c, ad5380_ids);
+
+static struct i2c_driver ad5380_i2c_driver = {
+	.driver = {
+		   .name = "ad5380",
+		   .owner = THIS_MODULE,
+	},
+	.probe = ad5380_i2c_probe,
+	.remove = __devexit_p(ad5380_i2c_remove),
+	.id_table = ad5380_i2c_ids,
+};
+
+static inline int ad5380_i2c_register_driver(void)
+{
+	return i2c_add_driver(&ad5380_i2c_driver);
+}
+
+static inline void ad5380_i2c_unregister_driver(void)
+{
+	i2c_del_driver(&ad5380_i2c_driver);
+}
+
+#else
+
+static inline int ad5380_i2c_register_driver(void)
+{
+	return 0;
+}
+
+static inline void ad5380_i2c_unregister_driver(void)
+{
+}
+
+#endif
+
+static __init int ad5380_spi_init(void)
+{
+	int ret;
+
+	ret = ad5380_spi_register_driver();
+	if (ret)
+		return ret;
+
+	ret = ad5380_i2c_register_driver();
+	if (ret) {
+		ad5380_spi_unregister_driver();
+		return ret;
+	}
+
+	return 0;
+}
+module_init(ad5380_spi_init);
+
+static __exit void ad5380_spi_exit(void)
+{
+	ad5380_i2c_unregister_driver();
+	ad5380_spi_unregister_driver();
+
+}
+module_exit(ad5380_spi_exit);
+
+MODULE_AUTHOR("Lars-Peter Clausen <lars@metafoo.de>");
+MODULE_DESCRIPTION("Analog Devices AD5380/81/82/83/90/91/92 DAC");
+MODULE_LICENSE("GPL v2");
-- 
1.7.7.1

Re: [PATCH 7/7] staging:iio:dac: Add AD5380 driver

[Lars-Peter Clausen]

On 11/16/2011 04:28 PM, Lars-Peter Clausen wrote:
> This patch adds support for the Analog Devices D5380, AD5381,
> AD5382, AD5383, AD5384, AD5390, AD5391, AD5392 multi-channel
> Digital to Analog Converters.
> 
> Signed-off-by: Lars-Peter Clausen <lars@metafoo.de>
> 
> ---
> There should be no compile time dependencies to the regmap patches earlier in
> this series, so this patch can be merged independently of it.
> ---
>  drivers/staging/iio/dac/Kconfig  |   11 +
>  drivers/staging/iio/dac/Makefile |    1 +
>  drivers/staging/iio/dac/ad5380.c |  669 ++++++++++++++++++++++++++++++++++++++
>  3 files changed, 681 insertions(+), 0 deletions(-)
>  create mode 100644 drivers/staging/iio/dac/ad5380.c
> 
> diff --git a/drivers/staging/iio/dac/Kconfig b/drivers/staging/iio/dac/Kconfig
> index a71defb..aaa6d46 100644
> --- a/drivers/staging/iio/dac/Kconfig
> +++ b/drivers/staging/iio/dac/Kconfig
> @@ -24,6 +24,17 @@ config AD5360
>  	  To compile this driver as module choose M here: the module will be called
>  	  ad5360.
>  
> +config AD5380
> +	tristate "Analog Devices AD5380/81/82/83/84/90/91/92 DAC driver"
> +	depends on (SPI_MASTER || I2C)

Somehow the 'select REGMAP_I2C if I2C' and 'select REGMAP_SPI if SPI_MASTER'
didn't made it into this patch. I'll fix it for v2.

> +	help
> +	  Say yes here to build support for Analog Devices AD5380, AD5381,
> +	  AD5382, AD5383, AD5384, AD5390, AD5391, AD5392 multi-channel
> +	  Digital to Analog Converters (DAC).
> +
> +	  To compile this driver as module choose M here: the module will be called
> +	  ad5380.
> +
>  config AD5421
>  	tristate "Analog Devices AD5421 DAC driver"
>  	depends on SPI
> diff --git a/drivers/staging/iio/dac/Makefile b/drivers/staging/iio/dac/Makefile
> index e75b0c8..840e94e 100644
> --- a/drivers/staging/iio/dac/Makefile
> +++ b/drivers/staging/iio/dac/Makefile
> @@ -3,6 +3,7 @@
>  #
>  
>  obj-$(CONFIG_AD5360) += ad5360.o
> +obj-$(CONFIG_AD5380) += ad5380.o
>  obj-$(CONFIG_AD5421) += ad5421.o
>  obj-$(CONFIG_AD5624R_SPI) += ad5624r_spi.o
>  obj-$(CONFIG_AD5064) += ad5064.o
> [...]

Re: [PATCH 7/7] staging:iio:dac: Add AD5380 driver

[Jonathan Cameron]

On 11/16/2011 03:28 PM, Lars-Peter Clausen wrote:
> This patch adds support for the Analog Devices D5380, AD5381,
> AD5382, AD5383, AD5384, AD5390, AD5391, AD5392 multi-channel
> Digital to Analog Converters.
> 
> Signed-off-by: Lars-Peter Clausen <lars@metafoo.de>
> 
> ---
> There should be no compile time dependencies to the regmap patches earlier in
> this series, so this patch can be merged independently of it.
Should probably have been a separate series!  Doesn't matter for review
though and I guess you justified some of the other patches with it.
> ---
>  drivers/staging/iio/dac/Kconfig  |   11 +
>  drivers/staging/iio/dac/Makefile |    1 +
>  drivers/staging/iio/dac/ad5380.c |  669 ++++++++++++++++++++++++++++++++++++++
>  3 files changed, 681 insertions(+), 0 deletions(-)
>  create mode 100644 drivers/staging/iio/dac/ad5380.c
> 
> diff --git a/drivers/staging/iio/dac/Kconfig b/drivers/staging/iio/dac/Kconfig
> index a71defb..aaa6d46 100644
> --- a/drivers/staging/iio/dac/Kconfig
> +++ b/drivers/staging/iio/dac/Kconfig
> @@ -24,6 +24,17 @@ config AD5360
>  	  To compile this driver as module choose M here: the module will be called
>  	  ad5360.
>  
> +config AD5380
> +	tristate "Analog Devices AD5380/81/82/83/84/90/91/92 DAC driver"
> +	depends on (SPI_MASTER || I2C)
> +	help
> +	  Say yes here to build support for Analog Devices AD5380, AD5381,
> +	  AD5382, AD5383, AD5384, AD5390, AD5391, AD5392 multi-channel
> +	  Digital to Analog Converters (DAC).
> +
> +	  To compile this driver as module choose M here: the module will be called
> +	  ad5380.
> +
>  config AD5421
>  	tristate "Analog Devices AD5421 DAC driver"
>  	depends on SPI
> diff --git a/drivers/staging/iio/dac/Makefile b/drivers/staging/iio/dac/Makefile
> index e75b0c8..840e94e 100644
> --- a/drivers/staging/iio/dac/Makefile
> +++ b/drivers/staging/iio/dac/Makefile
> @@ -3,6 +3,7 @@
>  #
>  
>  obj-$(CONFIG_AD5360) += ad5360.o
> +obj-$(CONFIG_AD5380) += ad5380.o
>  obj-$(CONFIG_AD5421) += ad5421.o
>  obj-$(CONFIG_AD5624R_SPI) += ad5624r_spi.o
>  obj-$(CONFIG_AD5064) += ad5064.o
> diff --git a/drivers/staging/iio/dac/ad5380.c b/drivers/staging/iio/dac/ad5380.c
> new file mode 100644
> index 0000000..0d5536a
> --- /dev/null
> +++ b/drivers/staging/iio/dac/ad5380.c
> @@ -0,0 +1,669 @@
> +/*
> + * Analog devices AD5380, AD5381, AD5382, AD5383, AD5370, AD5371, AD5373
> + * multi-channel Digital to Analog Converters driver
> + *
> + * Copyright 2011 Analog Devices Inc.
> + *
> + * Licensed under the GPL-2.
> + */
> +
> +#include <linux/device.h>
> +#include <linux/err.h>
> +#include <linux/i2c.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/spi/spi.h>
> +#include <linux/slab.h>
> +#include <linux/sysfs.h>
> +#include <linux/regmap.h>
> +#include <linux/regulator/consumer.h>
> +
> +#include "../iio.h"
> +#include "../sysfs.h"
> +#include "dac.h"
> +
> +
> +#define AD5380_REG_DATA(x)	(((x) << 2) | 3)
> +#define AD5380_REG_OFFSET(x)	(((x) << 2) | 2)
> +#define AD5380_REG_GAIN(x)	(((x) << 2) | 1)
> +#define AD5380_REG_SF_PWR_DOWN	(8 << 2)
> +#define AD5380_REG_SF_PWR_UP	(9 << 2)
> +#define AD5380_REG_SF_CTRL	(12 << 2)
> +
Could make this a bit as well... (then as follows where it is used)
> +#define AD5380_CTRL_PWR_DOWN_MODE_OFFSET	13
> +#define AD5380_CTRL_INT_VREF_2V5		BIT(12)
> +#define AD5380_CTRL_INT_VREF_EN			BIT(10)
> +
> +/**
> + * struct ad5380_chip_info - chip specific information
> + * @channel_template:	channel specification template
> + * @num_channels:	number of channels
> + * @int_vref:		internal vref in uV
> +*/
> +
> +struct ad5380_chip_info {
> +	struct iio_chan_spec	channel_template;
> +	unsigned int		num_channels;
> +	unsigned int		int_vref;
> +};
> +
> +/**
> + * struct ad5380_state - driver instance specific data
> + * @regmap:		regmap instance used by the device
> + * @chip_info:		chip model specific constants, available modes etc
> + * @vref_reg:		vref supply regulator
> + * @vref:		actual reference voltage used in uA
> + * @pwr_down:		whether the chip is currently in power down mode
> + */
> +
> +struct ad5380_state {
> +	struct regmap			*regmap;
> +	const struct ad5380_chip_info	*chip_info;
> +	struct regulator		*vref_reg;
> +	int				vref;
> +	bool				pwr_down;
> +};
> +
> +enum ad5380_type {
> +	ID_AD5380_3,
> +	ID_AD5380_5,
> +	ID_AD5381_3,
> +	ID_AD5381_5,
> +	ID_AD5382_3,
> +	ID_AD5382_5,
> +	ID_AD5383_3,
> +	ID_AD5383_5,
> +	ID_AD5384_3,
> +	ID_AD5384_5,
> +	ID_AD5390_3,
> +	ID_AD5390_5,
> +	ID_AD5391_3,
> +	ID_AD5391_5,
> +	ID_AD5392_3,
> +	ID_AD5392_5,
> +};
> +
> +#define AD5380_CHANNEL(_bits) {					\
> +	.type = IIO_VOLTAGE,					\
> +	.indexed = 1,						\
> +	.output = 1,						\
> +	.info_mask = IIO_CHAN_INFO_SCALE_SHARED_BIT |		\
> +		IIO_CHAN_INFO_CALIBSCALE_SEPARATE_BIT |		\
> +		IIO_CHAN_INFO_CALIBBIAS_SEPARATE_BIT,		\
> +	.scan_type = IIO_ST('u', (_bits), 16, 14 - (_bits))	\
> +}
> +
> +static const struct ad5380_chip_info ad5380_chip_info_tbl[] = {
> +	[ID_AD5380_3] = {
> +		.channel_template = AD5380_CHANNEL(14),
> +		.num_channels = 40,
> +		.int_vref = 1250000,
> +	},
> +	[ID_AD5380_5] = {
> +		.channel_template = AD5380_CHANNEL(14),
> +		.num_channels = 40,
> +		.int_vref = 2500000,
> +	},
> +	[ID_AD5381_3] = {
> +		.channel_template = AD5380_CHANNEL(12),
> +		.num_channels = 16,
> +		.int_vref = 1250000,
> +	},
> +	[ID_AD5381_5] = {
> +		.channel_template = AD5380_CHANNEL(12),
> +		.num_channels = 16,
> +		.int_vref = 2500000,
> +	},
> +	[ID_AD5382_3] = {
> +		.channel_template = AD5380_CHANNEL(14),
> +		.num_channels = 32,
> +		.int_vref = 1250000,
> +	},
> +	[ID_AD5382_5] = {
> +		.channel_template = AD5380_CHANNEL(14),
> +		.num_channels = 32,
> +		.int_vref = 2500000,
> +	},
> +	[ID_AD5383_3] = {
> +		.channel_template = AD5380_CHANNEL(12),
> +		.num_channels = 32,
> +		.int_vref = 1250000,
> +	},
> +	[ID_AD5383_5] = {
> +		.channel_template = AD5380_CHANNEL(12),
> +		.num_channels = 32,
> +		.int_vref = 2500000,
> +	},
> +	[ID_AD5390_3] = {
> +		.channel_template = AD5380_CHANNEL(14),
> +		.num_channels = 16,
> +		.int_vref = 1250000,
> +	},
> +	[ID_AD5390_5] = {
> +		.channel_template = AD5380_CHANNEL(14),
> +		.num_channels = 16,
> +		.int_vref = 2500000,
> +	},
> +	[ID_AD5391_3] = {
> +		.channel_template = AD5380_CHANNEL(12),
> +		.num_channels = 16,
> +		.int_vref = 1250000,
> +	},
> +	[ID_AD5392_5] = {
> +		.channel_template = AD5380_CHANNEL(12),
> +		.num_channels = 16,
> +		.int_vref = 2500000,
> +	},
> +	[ID_AD5392_3] = {
> +		.channel_template = AD5380_CHANNEL(14),
> +		.num_channels = 8,
> +		.int_vref = 1250000,
> +	},
> +	[ID_AD5392_5] = {
> +		.channel_template = AD5380_CHANNEL(14),
> +		.num_channels = 8,
> +		.int_vref = 2500000,
> +	},
> +};
> +
> +static ssize_t ad5380_read_dac_powerdown(struct device *dev,
> +	struct device_attribute *attr, char *buf)
> +{
> +	struct iio_dev *indio_dev = dev_get_drvdata(dev);
> +	struct ad5380_state *st = iio_priv(indio_dev);
> +
> +	return sprintf(buf, "%d\n", st->pwr_down);
> +}
> +
> +static ssize_t ad5380_write_dac_powerdown(struct device *dev,
> +	struct device_attribute *attr, const char *buf, size_t len)
> +{
> +	struct iio_dev *indio_dev = dev_get_drvdata(dev);
> +	struct ad5380_state *st = iio_priv(indio_dev);
> +	bool pwr_down;
> +	int ret;
> +
> +	ret = strtobool(buf, &pwr_down);
> +	if (ret)
> +		return ret;
> +
> +	mutex_lock(&indio_dev->mlock);
> +
> +	if (pwr_down)
> +		ret = regmap_write(st->regmap, AD5380_REG_SF_PWR_DOWN, 0);
> +	else
> +		ret = regmap_write(st->regmap, AD5380_REG_SF_PWR_UP, 0);
> +
> +	st->pwr_down = pwr_down;
> +
> +	mutex_unlock(&indio_dev->mlock);
> +
> +	return ret ? ret : len;
> +}
> +
> +static IIO_DEVICE_ATTR(out_voltage_powerdown,
> +			S_IRUGO | S_IWUSR,
> +			ad5380_read_dac_powerdown,
> +			ad5380_write_dac_powerdown, 0);
> +
> +static const char ad5380_powerdown_modes[][15] = {
> +	[0]	= "100kohm_to_gnd",
> +	[1]	= "three_state",
> +};
> +
> +static ssize_t ad5380_read_powerdown_mode(struct device *dev,
> +	struct device_attribute *attr, char *buf)
> +{
> +	struct iio_dev *indio_dev = dev_get_drvdata(dev);
> +	struct ad5380_state *st = iio_priv(indio_dev);
> +	unsigned int mode;
> +	int ret;
> +
> +	ret = regmap_read(st->regmap, AD5380_REG_SF_CTRL, &mode);
> +	if (ret)
> +		return ret;
> +
This is the clunkiest change if you make it a bit, but not too bad.
mode = !!(mode & AD5380_CTRL_PWR_DOWN_MODE_BIT);
> +	mode = (mode >> AD5380_CTRL_PWR_DOWN_MODE_OFFSET) & 1;
> +
> +	return sprintf(buf, "%s\n",
> +		ad5380_powerdown_modes[mode]);
> +}
> +
> +static ssize_t ad5380_write_powerdown_mode(struct device *dev,
> +	struct device_attribute *attr, const char *buf, size_t len)
> +{
> +	struct iio_dev *indio_dev = dev_get_drvdata(dev);
> +	struct ad5380_state *st = iio_priv(indio_dev);
> +	unsigned int i;
> +	int ret;
> +
Excess brackets for that for loop I think
> +	for (i = 0; i < ARRAY_SIZE(ad5380_powerdown_modes); ++i) {
> +		if (sysfs_streq(buf, ad5380_powerdown_modes[i]))
> +			break;
> +	}
> +
> +	if (i == ARRAY_SIZE(ad5380_powerdown_modes))
> +		return -EINVAL;
> +
> +	ret = regmap_update_bits(st->regmap, AD5380_REG_SF_CTRL,
> +		1 << AD5380_CTRL_PWR_DOWN_MODE_OFFSET,
> +		i << AD5380_CTRL_PWR_DOWN_MODE_OFFSET);
Obviously both of these will be cleaner with it as a bit.
> +
> +	return ret ? ret : len;
> +}
> +
Not a comment on this driver, but we need to have a think about
whether these will ever want to be controlled via in kernel
interfaces and if so how the heck we are going to do it!
> +static IIO_DEVICE_ATTR(out_voltage_powerdown_mode,
> +			S_IRUGO | S_IWUSR,
> +			ad5380_read_powerdown_mode,
> +			ad5380_write_powerdown_mode, 0);
> +
> +static IIO_CONST_ATTR(out_voltage_powerdown_mode_available,
> +			"100kohm_to_gnd three_state");
> +
> +static struct attribute *ad5380_attributes[] = {
> +	&iio_dev_attr_out_voltage_powerdown.dev_attr.attr,
> +	&iio_dev_attr_out_voltage_powerdown_mode.dev_attr.attr,
> +	&iio_const_attr_out_voltage_powerdown_mode_available.dev_attr.attr,
> +	NULL,
> +};
> +
> +static const struct attribute_group ad5380_attribute_group = {
> +	.attrs = ad5380_attributes,
> +};
> +
> +static unsigned int ad5380_info_to_reg(struct iio_chan_spec const *chan,
> +	long info)
> +{
> +	switch (info) {
> +	case 0:
> +		return AD5380_REG_DATA(chan->address);
> +	case IIO_CHAN_INFO_CALIBBIAS:
> +		return AD5380_REG_OFFSET(chan->address);
> +	case IIO_CHAN_INFO_CALIBSCALE:
> +		return AD5380_REG_GAIN(chan->address);
> +	default:
> +		BUG();
> +		break;
> +	}
> +
> +	return 0;
> +}
> +
> +static int ad5380_write_raw(struct iio_dev *indio_dev,
> +	struct iio_chan_spec const *chan, int val, int val2, long info)
> +{
> +	const unsigned int max_val = (1 << chan->scan_type.realbits);
> +	struct ad5380_state *st = iio_priv(indio_dev);
> +
> +	switch (info) {
> +	case IIO_CHAN_INFO_CALIBBIAS:
> +		val += (1 << chan->scan_type.realbits) / 2;
> +	case 0:
> +	case IIO_CHAN_INFO_CALIBSCALE:
> +		if (val >= max_val || val < 0)
> +			return -EINVAL;
> +
This is a somewhat uggly structure, I'd write this stuff out longhand for
readability or move this stuff that is shared out of the switch and make
default return -EINVAL;  Bit longer probably, but easier to read.
> +		return regmap_write(st->regmap, ad5380_info_to_reg(chan, info),
> +			val << chan->scan_type.shift);
> +	default:
> +		break;
> +	}
> +
> +	return -EINVAL;
> +}
> +
> +static int ad5380_read_raw(struct iio_dev *indio_dev,
> +	struct iio_chan_spec const *chan, int *val, int *val2, long info)
> +{
> +	struct ad5380_state *st = iio_priv(indio_dev);
> +	unsigned long scale_uv;
> +	int ret;
> +
> +	switch (info) {
> +	case 0:
> +	case IIO_CHAN_INFO_CALIBBIAS:
> +	case IIO_CHAN_INFO_CALIBSCALE:
> +		ret = regmap_read(st->regmap, ad5380_info_to_reg(chan, info),
> +					val);
> +		if (ret)
> +			return ret;
> +
Again, I'd unwrap this.  Marginally longer that way but simpler flow.
> +		if (info == IIO_CHAN_INFO_CALIBSCALE)
> +			val -= (1 << chan->scan_type.realbits) / 2;
> +
> +		*val >>= chan->scan_type.shift;
> +
> +		return IIO_VAL_INT;
> +	case IIO_CHAN_INFO_SCALE:
> +		scale_uv = ((2 * st->vref) >> chan->scan_type.realbits) * 100;
> +		*val =  scale_uv / 100000;
> +		*val2 = (scale_uv % 100000) * 10;
> +		return IIO_VAL_INT_PLUS_MICRO;
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	return IIO_VAL_INT;
> +}
> +
> +static const struct iio_info ad5380_info = {
> +	.read_raw = ad5380_read_raw,
> +	.write_raw = ad5380_write_raw,
> +	.attrs = &ad5380_attribute_group,
> +	.driver_module = THIS_MODULE,
> +};
> +
> +static int __devinit ad5380_alloc_channels(struct iio_dev *indio_dev)
> +{
> +	struct ad5380_state *st = iio_priv(indio_dev);
> +	struct iio_chan_spec *channels;
> +	unsigned int i;
> +
> +	channels = kcalloc(sizeof(struct iio_chan_spec),
> +			st->chip_info->num_channels, GFP_KERNEL);
> +
> +	if (!channels)
> +		return -ENOMEM;
> +
> +	for (i = 0; i < st->chip_info->num_channels; ++i) {
> +		channels[i] = st->chip_info->channel_template;
> +		channels[i].channel = i;
> +		channels[i].address = i;
> +	}
> +
> +	indio_dev->channels = channels;
> +
> +	return 0;
> +}
> +
> +static int __devinit ad5380_probe(struct device *dev, struct regmap *regmap,
> +	enum ad5380_type type, const char *name)
> +{
> +	struct iio_dev *indio_dev;
> +	struct ad5380_state *st;
> +	unsigned int ctrl = 0;
> +	int ret;
> +
> +	indio_dev = iio_allocate_device(sizeof(*st));
> +	if (indio_dev == NULL) {
> +		dev_err(dev, "Failed to allocate iio device\n");
> +		ret = -ENOMEM;
> +		goto error_regmap_exit;
> +	}
> +
> +	st = iio_priv(indio_dev);
> +	dev_set_drvdata(dev, indio_dev);
> +
> +	st->chip_info = &ad5380_chip_info_tbl[type];
> +	st->regmap = regmap;
> +
> +	indio_dev->dev.parent = dev;
> +	indio_dev->name = name;
> +	indio_dev->info = &ad5380_info;
> +	indio_dev->modes = INDIO_DIRECT_MODE;
> +	indio_dev->num_channels = st->chip_info->num_channels;
> +
> +	ret = ad5380_alloc_channels(indio_dev);
> +	if (ret) {
> +		dev_err(dev, "Failed to allocate channel spec: %d\n", ret);
> +		goto error_free;
> +	}
> +
> +	if (st->chip_info->int_vref == 2500000)
> +		ctrl |= AD5380_CTRL_INT_VREF_2V5;
> +
> +	st->vref_reg = regulator_get(dev, "vref");
> +	if (!IS_ERR(st->vref_reg)) {
> +		ret = regulator_enable(st->vref_reg);
> +		if (ret) {
Overly long line?
> +			dev_err(dev, "Failed to enable vref regulators: %d\n", ret);
> +			goto error_free_reg;
> +		}
> +
> +		st->vref = regulator_get_voltage(st->vref_reg);
> +	} else {
> +		st->vref = st->chip_info->int_vref;
> +		ctrl |= AD5380_CTRL_INT_VREF_EN;
> +	}
> +
> +	ret = regmap_write(st->regmap, AD5380_REG_SF_CTRL, ctrl);
> +	if (ret) {
This isn't exactly an informative message!  Perhaps drop it or
make it more useful.
> +		dev_err(dev, "Failed: %d\n", ret);
> +		goto error_disable_reg;
> +	}
> +
> +	ret = iio_device_register(indio_dev);
> +	if (ret) {
> +		dev_err(dev, "Failed to register iio device: %d\n", ret);
> +		goto error_disable_reg;
> +	}
> +
> +	return 0;
> +
> +error_disable_reg:
> +	if (!IS_ERR(st->vref_reg))
> +		regulator_disable(st->vref_reg);
> +error_free_reg:
> +	if (!IS_ERR(st->vref_reg))
> +		regulator_put(st->vref_reg);
> +
> +	kfree(indio_dev->channels);
> +error_free:
> +	iio_free_device(indio_dev);
> +error_regmap_exit:
> +	regmap_exit(regmap);
> +
> +	return ret;
> +}
> +
> +static int __devexit ad5380_remove(struct device *dev)
> +{
> +	struct iio_dev *indio_dev = dev_get_drvdata(dev);
> +	struct ad5380_state *st = iio_priv(indio_dev);
> +
> +	iio_device_unregister(indio_dev);
> +
> +	kfree(indio_dev->channels);
> +
> +	if (!IS_ERR(st->vref_reg)) {
> +		regulator_disable(st->vref_reg);
> +		regulator_put(st->vref_reg);
> +	}
> +
> +	iio_free_device(indio_dev);
> +	regmap_exit(st->regmap);
> +
> +	return 0;
> +}
> +
> +static bool ad5380_reg_false(struct device *dev, unsigned int reg)
> +{
> +	return false;
> +}
> +
> +static const struct regmap_config ad5380_regmap_config = {
> +	.reg_bits = 10,
> +	.val_bits = 14,
> +
> +	.max_register = AD5380_REG_DATA(40),
> +	.cache_type = REGCACHE_RBTREE,
> +
> +	.volatile_reg = ad5380_reg_false,
> +	.readable_reg = ad5380_reg_false,
> +};
> +
> +#if IS_ENABLED(CONFIG_SPI_MASTER)
> +
> +static int __devinit ad5380_spi_probe(struct spi_device *spi)
> +{
> +	const struct spi_device_id *id = spi_get_device_id(spi);
> +	struct regmap *regmap;
> +
> +	regmap = regmap_init_spi(spi, &ad5380_regmap_config);
> +
> +	if (IS_ERR(regmap))
> +		return PTR_ERR(regmap);
> +
> +	return ad5380_probe(&spi->dev, regmap,
> +		id->driver_data, id->name);
> +}
> +
> +static int __devexit ad5380_spi_remove(struct spi_device *spi)
> +{
> +	return ad5380_remove(&spi->dev);
> +}
> +
> +static const struct spi_device_id ad5380_spi_ids[] = {
> +	{ "ad5380-3", ID_AD5380_3 },
> +	{ "ad5380-5", ID_AD5380_5 },
> +	{ "ad5381-3", ID_AD5381_3 },
> +	{ "ad5381-5", ID_AD5381_5 },
> +	{ "ad5382-3", ID_AD5382_3 },
> +	{ "ad5382-5", ID_AD5382_5 },
> +	{ "ad5383-3", ID_AD5383_3 },
> +	{ "ad5383-5", ID_AD5383_5 },
> +	{ "ad5390-3", ID_AD5380_3 },
> +	{ "ad5390-5", ID_AD5380_5 },
> +	{ "ad5391-3", ID_AD5381_3 },
> +	{ "ad5391-5", ID_AD5381_5 },
> +	{ "ad5392-3", ID_AD5382_3 },
> +	{ "ad5392-5", ID_AD5382_5 },
as below, I'm guessing the id missmatch isn't intentional as
you defined the other ids.
> +	{ }
> +};
> +MODULE_DEVICE_TABLE(spi, ad5380_ids);
> +
> +static struct spi_driver ad5380_spi_driver = {
> +	.driver = {
> +		   .name = "ad5380",
> +		   .owner = THIS_MODULE,
> +	},
> +	.probe = ad5380_spi_probe,
> +	.remove = __devexit_p(ad5380_spi_remove),
> +	.id_table = ad5380_spi_ids,
> +};
> +
> +static inline int ad5380_spi_register_driver(void)
> +{
> +	return spi_register_driver(&ad5380_spi_driver);
> +}
> +
> +static inline void ad5380_spi_unregister_driver(void)
> +{
> +	spi_unregister_driver(&ad5380_spi_driver);
> +}
> +
> +#else
> +
> +static inline int ad5380_spi_register_driver(void)
> +{
> +	return 0;
> +}
> +
> +static inline void ad5380_spi_unregister_driver(void)
> +{
> +}
> +
> +#endif
> +
> +#if IS_ENABLED(CONFIG_I2C)
> +
> +static int __devinit ad5380_i2c_probe(struct i2c_client *i2c,
> +	const struct i2c_device_id *id)
> +{
> +	struct regmap *regmap;
> +
> +	regmap = regmap_init_i2c(i2c, &ad5380_regmap_config);
> +
> +	if (IS_ERR(regmap))
> +		return PTR_ERR(regmap);
> +
Looks like the following is sub 80 chars on one line?
> +	return ad5380_probe(&i2c->dev, regmap,
> +		id->driver_data, id->name);
> +}
> +
> +static int __devexit ad5380_i2c_remove(struct i2c_client *i2c)
> +{
> +	return ad5380_remove(&i2c->dev);
> +}
> +
> +static const struct i2c_device_id ad5380_i2c_ids[] = {
> +	{ "ad5380-3", ID_AD5380_3 },
> +	{ "ad5380-5", ID_AD5380_5 },
> +	{ "ad5381-3", ID_AD5381_3 },
> +	{ "ad5381-5", ID_AD5381_5 },
> +	{ "ad5382-3", ID_AD5382_3 },
> +	{ "ad5382-5", ID_AD5382_5 },
> +	{ "ad5383-3", ID_AD5383_3 },
> +	{ "ad5383-5", ID_AD5383_5 },
> +	{ "ad5390-3", ID_AD5380_3 },
> +	{ "ad5390-5", ID_AD5380_5 },
> +	{ "ad5391-3", ID_AD5381_3 },
> +	{ "ad5391-5", ID_AD5381_5 },
> +	{ "ad5392-3", ID_AD5382_3 },
> +	{ "ad5392-5", ID_AD5382_5 },
I'm guessing you defined the ID_AD5392 etc for a reason?
> +	{ }
> +};
> +MODULE_DEVICE_TABLE(i2c, ad5380_ids);
> +
> +static struct i2c_driver ad5380_i2c_driver = {
> +	.driver = {
> +		   .name = "ad5380",
> +		   .owner = THIS_MODULE,
> +	},
> +	.probe = ad5380_i2c_probe,
> +	.remove = __devexit_p(ad5380_i2c_remove),
> +	.id_table = ad5380_i2c_ids,
> +};
> +
> +static inline int ad5380_i2c_register_driver(void)
> +{
> +	return i2c_add_driver(&ad5380_i2c_driver);
> +}
> +
> +static inline void ad5380_i2c_unregister_driver(void)
> +{
> +	i2c_del_driver(&ad5380_i2c_driver);
> +}
> +
> +#else
> +
> +static inline int ad5380_i2c_register_driver(void)
> +{
> +	return 0;
> +}
> +
> +static inline void ad5380_i2c_unregister_driver(void)
> +{
> +}
> +
> +#endif
> +
> +static __init int ad5380_spi_init(void)
> +{
> +	int ret;
> +
> +	ret = ad5380_spi_register_driver();
> +	if (ret)
> +		return ret;
> +
> +	ret = ad5380_i2c_register_driver();
> +	if (ret) {
> +		ad5380_spi_unregister_driver();
> +		return ret;
> +	}
> +
> +	return 0;
> +}
> +module_init(ad5380_spi_init);
> +
> +static __exit void ad5380_spi_exit(void)
> +{
> +	ad5380_i2c_unregister_driver();
> +	ad5380_spi_unregister_driver();
> +
> +}
> +module_exit(ad5380_spi_exit);
> +
> +MODULE_AUTHOR("Lars-Peter Clausen <lars@metafoo.de>");
> +MODULE_DESCRIPTION("Analog Devices AD5380/81/82/83/90/91/92 DAC");
> +MODULE_LICENSE("GPL v2");

Re: [PATCH 7/7] staging:iio:dac: Add AD5380 driver

[Lars-Peter Clausen]

On 11/17/2011 09:24 PM, Jonathan Cameron wrote:
> On 11/16/2011 03:28 PM, Lars-Peter Clausen wrote:
>> This patch adds support for the Analog Devices D5380, AD5381,
>> AD5382, AD5383, AD5384, AD5390, AD5391, AD5392 multi-channel
>> Digital to Analog Converters.
>>
>> Signed-off-by: Lars-Peter Clausen <lars@metafoo.de>
>>
>> ---
>> There should be no compile time dependencies to the regmap patches earlier in
>> this series, so this patch can be merged independently of it.
> Should probably have been a separate series!  Doesn't matter for review
> though and I guess you justified some of the other patches with it.
>> ---
>>  drivers/staging/iio/dac/Kconfig  |   11 +
>>  drivers/staging/iio/dac/Makefile |    1 +
>>  drivers/staging/iio/dac/ad5380.c |  669 ++++++++++++++++++++++++++++++++++++++
>>  3 files changed, 681 insertions(+), 0 deletions(-)
>>  create mode 100644 drivers/staging/iio/dac/ad5380.c
>>[...]
>> +
>> +static ssize_t ad5380_write_powerdown_mode(struct device *dev,
>> +	struct device_attribute *attr, const char *buf, size_t len)
>> +{
>> +	struct iio_dev *indio_dev = dev_get_drvdata(dev);
>> +	struct ad5380_state *st = iio_priv(indio_dev);
>> +	unsigned int i;
>> +	int ret;
>> +
> Excess brackets for that for loop I think

I prefer to have them since the loop contains a multiple lines.

>> +	for (i = 0; i < ARRAY_SIZE(ad5380_powerdown_modes); ++i) {
>> +		if (sysfs_streq(buf, ad5380_powerdown_modes[i]))
>> +			break;
>> +	}
>> +
>> +	if (i == ARRAY_SIZE(ad5380_powerdown_modes))
>> +		return -EINVAL;
>> +
>> +	ret = regmap_update_bits(st->regmap, AD5380_REG_SF_CTRL,
>> +		1 << AD5380_CTRL_PWR_DOWN_MODE_OFFSET,
>> +		i << AD5380_CTRL_PWR_DOWN_MODE_OFFSET);
> Obviously both of these will be cleaner with it as a bit.

When writing this using BIT it would be i ? AD5380_CTRL_PWR_DOWN_MODE_BIT :
0... And also this is about semantics. The other bits are bits in the sense
of true/false. While this is an enumeration which just happens to have only
one bit. But I can change it, if you prefer using BIT here.

>> +
>> +	return ret ? ret : len;
>> +}
>> +
> Not a comment on this driver, but we need to have a think about
> whether these will ever want to be controlled via in kernel
> interfaces and if so how the heck we are going to do it!

Yes, definitely. But I think it is best to discuss this in a separate thread.

>> +static IIO_DEVICE_ATTR(out_voltage_powerdown_mode,
>> +			S_IRUGO | S_IWUSR,
>> +			ad5380_read_powerdown_mode,
>> +			ad5380_write_powerdown_mode, 0);
>> +
>> +static IIO_CONST_ATTR(out_voltage_powerdown_mode_available,
>> +			"100kohm_to_gnd three_state");
>> +
>> +static struct attribute *ad5380_attributes[] = {
>> +	&iio_dev_attr_out_voltage_powerdown.dev_attr.attr,
>> +	&iio_dev_attr_out_voltage_powerdown_mode.dev_attr.attr,
>> +	&iio_const_attr_out_voltage_powerdown_mode_available.dev_attr.attr,
>> +	NULL,
>> +};
> [...]
>> +static const struct i2c_device_id ad5380_i2c_ids[] = {
>> +	{ "ad5380-3", ID_AD5380_3 },
>> +	{ "ad5380-5", ID_AD5380_5 },
>> +	{ "ad5381-3", ID_AD5381_3 },
>> +	{ "ad5381-5", ID_AD5381_5 },
>> +	{ "ad5382-3", ID_AD5382_3 },
>> +	{ "ad5382-5", ID_AD5382_5 },
>> +	{ "ad5383-3", ID_AD5383_3 },
>> +	{ "ad5383-5", ID_AD5383_5 },
>> +	{ "ad5390-3", ID_AD5380_3 },
>> +	{ "ad5390-5", ID_AD5380_5 },
>> +	{ "ad5391-3", ID_AD5381_3 },
>> +	{ "ad5391-5", ID_AD5381_5 },
>> +	{ "ad5392-3", ID_AD5382_3 },
>> +	{ "ad5392-5", ID_AD5382_5 },
> I'm guessing you defined the ID_AD5392 etc for a reason?

Ooops, yes. Good catch.

Thanks for the review.

Re: [PATCH 7/7] staging:iio:dac: Add AD5380 driver

[J.I. Cameron]

On Nov 18 2011, Lars-Peter Clausen wrote:

>On 11/17/2011 09:24 PM, Jonathan Cameron wrote:
>> On 11/16/2011 03:28 PM, Lars-Peter Clausen wrote:
>>> This patch adds support for the Analog Devices D5380, AD5381,
>>> AD5382, AD5383, AD5384, AD5390, AD5391, AD5392 multi-channel
>>> Digital to Analog Converters.
>>>
>>> Signed-off-by: Lars-Peter Clausen <lars@metafoo.de>
>>>
>>> --- There should be no compile time dependencies to the regmap patches 
>>> earlier in this series, so this patch can be merged independently of 
>>> it.
>> Should probably have been a separate series!  Doesn't matter for review
>> though and I guess you justified some of the other patches with it.
>>> ---
>>>  drivers/staging/iio/dac/Kconfig  |   11 +
>>>  drivers/staging/iio/dac/Makefile |    1 +
>>>  drivers/staging/iio/dac/ad5380.c | 669 
>>> ++++++++++++++++++++++++++++++++++++++
>>>  3 files changed, 681 insertions(+), 0 deletions(-)
>>>  create mode 100644 drivers/staging/iio/dac/ad5380.c
>>>[...]
>>> +
>>> +static ssize_t ad5380_write_powerdown_mode(struct device *dev,
>>> +	struct device_attribute *attr, const char *buf, size_t len)
>>> +{
>>> +	struct iio_dev *indio_dev = dev_get_drvdata(dev);
>>> +	struct ad5380_state *st = iio_priv(indio_dev);
>>> +	unsigned int i;
>>> +	int ret;
>>> +
>> Excess brackets for that for loop I think
>
>I prefer to have them since the loop contains a multiple lines.
Hmm.. might be out of coding standard, but only just so fine...
>
>>> +	for (i = 0; i < ARRAY_SIZE(ad5380_powerdown_modes); ++i) {
>>> +		if (sysfs_streq(buf, ad5380_powerdown_modes[i]))
>>> +			break;
>>> +	}
>>> +
>>> +	if (i == ARRAY_SIZE(ad5380_powerdown_modes))
>>> +		return -EINVAL;
>>> +
>>> +	ret = regmap_update_bits(st->regmap, AD5380_REG_SF_CTRL,
>>> +		1 << AD5380_CTRL_PWR_DOWN_MODE_OFFSET,
>>> +		i << AD5380_CTRL_PWR_DOWN_MODE_OFFSET);
>> Obviously both of these will be cleaner with it as a bit.
>
>When writing this using BIT it would be i ? AD5380_CTRL_PWR_DOWN_MODE_BIT :
>0... And also this is about semantics. The other bits are bits in the sense
>of true/false. While this is an enumeration which just happens to have only
>one bit. But I can change it, if you prefer using BIT here.
Gah, I read them as both 1 <<. oops. you are of course correct that my
suggestion makes no sense so don't worry about that!
>
>>> +
>>> +	return ret ? ret : len;
>>> +}
>>> +
>> Not a comment on this driver, but we need to have a think about
>> whether these will ever want to be controlled via in kernel
>> interfaces and if so how the heck we are going to do it!
>
> Yes, definitely. But I think it is best to discuss this in a separate 
> thread.
Agreed. Feel free to start it ;)
>
>>> +static IIO_DEVICE_ATTR(out_voltage_powerdown_mode,
>>> +			S_IRUGO | S_IWUSR,
>>> +			ad5380_read_powerdown_mode,
>>> +			ad5380_write_powerdown_mode, 0);
>>> +
>>> +static IIO_CONST_ATTR(out_voltage_powerdown_mode_available,
>>> +			"100kohm_to_gnd three_state");
>>> +
>>> +static struct attribute *ad5380_attributes[] = {
>>> +	&iio_dev_attr_out_voltage_powerdown.dev_attr.attr,
>>> +	&iio_dev_attr_out_voltage_powerdown_mode.dev_attr.attr,
>>> +	&iio_const_attr_out_voltage_powerdown_mode_available.dev_attr.attr,
>>> +	NULL,
>>> +};
>> [...]
>>> +static const struct i2c_device_id ad5380_i2c_ids[] = {
>>> +	{ "ad5380-3", ID_AD5380_3 },
>>> +	{ "ad5380-5", ID_AD5380_5 },
>>> +	{ "ad5381-3", ID_AD5381_3 },
>>> +	{ "ad5381-5", ID_AD5381_5 },
>>> +	{ "ad5382-3", ID_AD5382_3 },
>>> +	{ "ad5382-5", ID_AD5382_5 },
>>> +	{ "ad5383-3", ID_AD5383_3 },
>>> +	{ "ad5383-5", ID_AD5383_5 },
>>> +	{ "ad5390-3", ID_AD5380_3 },
>>> +	{ "ad5390-5", ID_AD5380_5 },
>>> +	{ "ad5391-3", ID_AD5381_3 },
>>> +	{ "ad5391-5", ID_AD5381_5 },
>>> +	{ "ad5392-3", ID_AD5382_3 },
>>> +	{ "ad5392-5", ID_AD5382_5 },
>> I'm guessing you defined the ID_AD5392 etc for a reason?
>
>Ooops, yes. Good catch.
>
>Thanks for the review.
You are welcome.

Re: [PATCH 1/7] regmap: Move initialization of regcache related fields to regcache_init

[Mark Brown]

On Wed, Nov 16, 2011 at 04:28:16PM +0100, Lars-Peter Clausen wrote:
> Move the initialization regcache related fields of the regmap struct to
> regcache_init. This allows us to keep regmap and regcache code better
> separated.

Applied, thanks.