Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1456457 > unrolled thread

[PATCH 1/2] regmap: Add a function to check if a regmap register is cached

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCH 1/2] regmap: Add a function to check if a regmap register is cached Cristian Birsan <cristian.birsan@microchip.com> - 2016-08-04 17:00 +0200
    Re: [PATCH 1/2] regmap: Add a function to check if a regmap register  is cached Lars-Peter Clausen <lars@metafoo.de> - 2016-08-04 17:40 +0200
    Re: [PATCH 1/2] regmap: Add a function to check if a regmap register  is cached Mark Brown <broonie@kernel.org> - 2016-08-04 17:50 +0200
      [PATCH v2 1/2] regmap: Add a function to check if a regmap register is cached Cristian Birsan <cristian.birsan@microchip.com> - 2016-08-08 18:00 +0200
        Applied "regmap: Add a function to check if a regmap register is cached" to the regmap tree Mark Brown <broonie@kernel.org> - 2016-08-09 14:50 +0200
      [PATCH v2 2/2] regmap: debugfs: Add support for dumping write only device registers Cristian Birsan <cristian.birsan@microchip.com> - 2016-08-08 18:00 +0200
        Applied "regmap: debugfs: Add support for dumping write only device registers" to the regmap tree Mark Brown <broonie@kernel.org> - 2016-08-09 14:50 +0200
          Re: Applied "regmap: debugfs: Add support for dumping write only  device registers" to the regmap tree kbuild test robot <lkp@intel.com> - 2016-08-09 15:20 +0200
      [PATCH v2 0/2] Display regmap values in debugfs for write only registers Cristian Birsan <cristian.birsan@microchip.com> - 2016-08-08 18:00 +0200

#1456457 — [PATCH 1/2] regmap: Add a function to check if a regmap register is cached

Add a function to check if a regmap register is cached. This will be used
in debugfs to dump the cached values of write only registers.

Signed-off-by: Cristian Birsan <cristian.birsan@microchip.com>
---
 drivers/base/regmap/internal.h |  1 +
 drivers/base/regmap/regmap.c   | 14 ++++++++++++++
 2 files changed, 15 insertions(+)

diff --git a/drivers/base/regmap/internal.h b/drivers/base/regmap/internal.h
index 3df9770..cae04f4 100644
--- a/drivers/base/regmap/internal.h
+++ b/drivers/base/regmap/internal.h
@@ -171,6 +171,7 @@ struct regcache_ops {
 	int (*drop)(struct regmap *map, unsigned int min, unsigned int max);
 };
 
+bool regmap_cached(struct regmap *map, unsigned int reg);
 bool regmap_writeable(struct regmap *map, unsigned int reg);
 bool regmap_readable(struct regmap *map, unsigned int reg);
 bool regmap_volatile(struct regmap *map, unsigned int reg);
diff --git a/drivers/base/regmap/regmap.c b/drivers/base/regmap/regmap.c
index 4ac63c0..e07f3a9 100644
--- a/drivers/base/regmap/regmap.c
+++ b/drivers/base/regmap/regmap.c
@@ -92,6 +92,20 @@ bool regmap_writeable(struct regmap *map, unsigned int reg)
 	return true;
 }
 
+bool regmap_cached(struct regmap *map, unsigned int reg)
+{
+	if (map->cache == REGCACHE_NONE)
+		return false;
+
+	if (!map->cache_ops)
+		return false;
+
+	if (map->max_register && reg > map->max_register)
+		return false;
+
+	return true;
+}
+
 bool regmap_readable(struct regmap *map, unsigned int reg)
 {
 	if (!map->reg_read)
-- 
1.9.1

[toc] | [next] | [standalone]

#1456492 — Re: [PATCH 1/2] regmap: Add a function to check if a regmap register is cached

On 08/04/2016 04:55 PM, Cristian Birsan wrote:
> Add a function to check if a regmap register is cached. This will be used
> in debugfs to dump the cached values of write only registers.
> 
> Signed-off-by: Cristian Birsan <cristian.birsan@microchip.com>
> ---
>  drivers/base/regmap/internal.h |  1 +
>  drivers/base/regmap/regmap.c   | 14 ++++++++++++++
>  2 files changed, 15 insertions(+)
> 
> diff --git a/drivers/base/regmap/internal.h b/drivers/base/regmap/internal.h
> index 3df9770..cae04f4 100644
> --- a/drivers/base/regmap/internal.h
> +++ b/drivers/base/regmap/internal.h
> @@ -171,6 +171,7 @@ struct regcache_ops {
>  	int (*drop)(struct regmap *map, unsigned int min, unsigned int max);
>  };
>  
> +bool regmap_cached(struct regmap *map, unsigned int reg);
>  bool regmap_writeable(struct regmap *map, unsigned int reg);
>  bool regmap_readable(struct regmap *map, unsigned int reg);
>  bool regmap_volatile(struct regmap *map, unsigned int reg);
> diff --git a/drivers/base/regmap/regmap.c b/drivers/base/regmap/regmap.c
> index 4ac63c0..e07f3a9 100644
> --- a/drivers/base/regmap/regmap.c
> +++ b/drivers/base/regmap/regmap.c
> @@ -92,6 +92,20 @@ bool regmap_writeable(struct regmap *map, unsigned int reg)
>  	return true;
>  }
>  
> +bool regmap_cached(struct regmap *map, unsigned int reg)
> +{
> +	if (map->cache == REGCACHE_NONE)
> +		return false;
> +
> +	if (!map->cache_ops)
> +		return false;
> +
> +	if (map->max_register && reg > map->max_register)
> +		return false;	

There is a problem with this approach. It does not check if the register is
cached it only checks if the register is cacheable.

This works very poorly for devices with sparse register maps. Sparse
register maps do not assign a register to each register number. There are
often even large gaps in the register map and some devices use up their full
16-bit register space.

Now this change combined with the next patch will cause the register file to
contain an entry for every possible register number, even if the register
number is not assigned. Since unassigned registers are not cached
regcache_read() will return an error and the registers file will print XX
for the register value. This means for sparse register maps the registers
file will be full of thousands of such entries.

[toc] | [prev] | [next] | [standalone]

#1456507 — Re: [PATCH 1/2] regmap: Add a function to check if a regmap register is cached

[Multipart message — attachments visible in raw view] — view raw

On Thu, Aug 04, 2016 at 05:55:57PM +0300, Cristian Birsan wrote:

> +bool regmap_cached(struct regmap *map, unsigned int reg)
> +{
> +	if (map->cache == REGCACHE_NONE)
> +		return false;
> +
> +	if (!map->cache_ops)
> +		return false;
> +
> +	if (map->max_register && reg > map->max_register)
> +		return false;
> +
> +	return true;
> +}

As Lars said this isn't actually checking if the register is cached.  To
do this you need to modify the output code to check if there's a value
cached and try a read if there is one instead.

[toc] | [prev] | [next] | [standalone]

#1457870 — [PATCH v2 1/2] regmap: Add a function to check if a regmap register is cached

Add a function to check if a regmap register is cached. This will be used
in debugfs to dump the cached values of write only registers.

Signed-off-by: Cristian Birsan <cristian.birsan@microchip.com>
---
 drivers/base/regmap/internal.h |  1 +
 drivers/base/regmap/regmap.c   | 23 +++++++++++++++++++++++
 2 files changed, 24 insertions(+)

diff --git a/drivers/base/regmap/internal.h b/drivers/base/regmap/internal.h
index a038033..f4be4c1 100644
--- a/drivers/base/regmap/internal.h
+++ b/drivers/base/regmap/internal.h
@@ -173,6 +173,7 @@ struct regcache_ops {
 	int (*drop)(struct regmap *map, unsigned int min, unsigned int max);
 };
 
+bool regmap_cached(struct regmap *map, unsigned int reg);
 bool regmap_writeable(struct regmap *map, unsigned int reg);
 bool regmap_readable(struct regmap *map, unsigned int reg);
 bool regmap_volatile(struct regmap *map, unsigned int reg);
diff --git a/drivers/base/regmap/regmap.c b/drivers/base/regmap/regmap.c
index 51fa7d6..1f011f9 100644
--- a/drivers/base/regmap/regmap.c
+++ b/drivers/base/regmap/regmap.c
@@ -93,6 +93,29 @@ bool regmap_writeable(struct regmap *map, unsigned int reg)
 	return true;
 }
 
+bool regmap_cached(struct regmap *map, unsigned int reg)
+{
+	int ret;
+	unsigned int val;
+
+	if (map->cache == REGCACHE_NONE)
+		return false;
+
+	if (!map->cache_ops)
+		return false;
+
+	if (map->max_register && reg > map->max_register)
+		return false;
+
+	map->lock(map->lock_arg);
+	ret = regcache_read(map, reg, &val);
+	map->unlock(map->lock_arg);
+	if (ret)
+		return false;
+
+	return true;
+}
+
 bool regmap_readable(struct regmap *map, unsigned int reg)
 {
 	if (!map->reg_read)
-- 
1.9.1

[toc] | [prev] | [next] | [standalone]

#1458692 — Applied "regmap: Add a function to check if a regmap register is cached" to the regmap tree

The patch

   regmap: Add a function to check if a regmap register is cached

has been applied to the regmap tree at

   git://git.kernel.org/pub/scm/linux/kernel/git/broonie/regmap.git 

All being well this means that it will be integrated into the linux-next
tree (usually sometime in the next 24 hours) and sent to Linus during
the next merge window (or sooner if it is a bug fix), however if
problems are discovered then the patch may be dropped or reverted.  

You may get further e-mails resulting from automated or manual testing
and review of the tree, please engage with people reporting problems and
send followup patches addressing any issues that are reported if needed.

If any updates are required or you are submitting further changes they
should be sent as incremental updates against current git, existing
patches will not be replaced.

Please add any relevant lists and maintainers to the CCs when replying
to this mail.

Thanks,
Mark

From 1ea975cf1ef57b1e44c0aec4820f60bb3b60904b Mon Sep 17 00:00:00 2001
From: Cristian Birsan <cristian.birsan@microchip.com>
Date: Mon, 8 Aug 2016 18:44:21 +0300
Subject: [PATCH] regmap: Add a function to check if a regmap register is
 cached

Add a function to check if a regmap register is cached. This will be used
in debugfs to dump the cached values of write only registers.

Signed-off-by: Cristian Birsan <cristian.birsan@microchip.com>
Signed-off-by: Mark Brown <broonie@kernel.org>
---
 drivers/base/regmap/internal.h |  1 +
 drivers/base/regmap/regmap.c   | 23 +++++++++++++++++++++++
 2 files changed, 24 insertions(+)

diff --git a/drivers/base/regmap/internal.h b/drivers/base/regmap/internal.h
index a0380338946a..f4be4c19bb17 100644
--- a/drivers/base/regmap/internal.h
+++ b/drivers/base/regmap/internal.h
@@ -173,6 +173,7 @@ struct regcache_ops {
 	int (*drop)(struct regmap *map, unsigned int min, unsigned int max);
 };
 
+bool regmap_cached(struct regmap *map, unsigned int reg);
 bool regmap_writeable(struct regmap *map, unsigned int reg);
 bool regmap_readable(struct regmap *map, unsigned int reg);
 bool regmap_volatile(struct regmap *map, unsigned int reg);
diff --git a/drivers/base/regmap/regmap.c b/drivers/base/regmap/regmap.c
index 51fa7d66a393..1f011f9d6dcb 100644
--- a/drivers/base/regmap/regmap.c
+++ b/drivers/base/regmap/regmap.c
@@ -93,6 +93,29 @@ bool regmap_writeable(struct regmap *map, unsigned int reg)
 	return true;
 }
 
+bool regmap_cached(struct regmap *map, unsigned int reg)
+{
+	int ret;
+	unsigned int val;
+
+	if (map->cache == REGCACHE_NONE)
+		return false;
+
+	if (!map->cache_ops)
+		return false;
+
+	if (map->max_register && reg > map->max_register)
+		return false;
+
+	map->lock(map->lock_arg);
+	ret = regcache_read(map, reg, &val);
+	map->unlock(map->lock_arg);
+	if (ret)
+		return false;
+
+	return true;
+}
+
 bool regmap_readable(struct regmap *map, unsigned int reg)
 {
 	if (!map->reg_read)
-- 
2.8.1

[toc] | [prev] | [next] | [standalone]

#1457873 — [PATCH v2 2/2] regmap: debugfs: Add support for dumping write only device registers

Add support for dumping write only device registers in debugfs. This is
useful for audio codecs that have write only registers (like WM8731).
The logic that decides if a value can be printed is moved to
regmap_printable() function to allow for easier future updates.

Signed-off-by: Cristian Birsan <cristian.birsan@microchip.com>
---
 drivers/base/regmap/regmap-debugfs.c | 16 +++++++++++++---
 1 file changed, 13 insertions(+), 3 deletions(-)

diff --git a/drivers/base/regmap/regmap-debugfs.c b/drivers/base/regmap/regmap-debugfs.c
index 1ee3d40..36ce351 100644
--- a/drivers/base/regmap/regmap-debugfs.c
+++ b/drivers/base/regmap/regmap-debugfs.c
@@ -77,6 +77,17 @@ static void regmap_debugfs_free_dump_cache(struct regmap *map)
 	}
 }
 
+static bool regmap_printable(struct regmap *map, unsigned int reg)
+{
+	if (regmap_precious(map, reg))
+		return false;
+
+	if (!regmap_readable(map, reg) && !regmap_cached(map, reg))
+		return false;
+
+	return true;
+}
+
 /*
  * Work out where the start offset maps into register numbers, bearing
  * in mind that we suppress hidden registers.
@@ -105,8 +116,7 @@ static unsigned int regmap_debugfs_get_dump_start(struct regmap *map,
 	if (list_empty(&map->debugfs_off_cache)) {
 		for (; i <= map->max_register; i += map->reg_stride) {
 			/* Skip unprinted registers, closing off cache entry */
-			if (!regmap_readable(map, i) ||
-			    regmap_precious(map, i)) {
+			if (!regmap_printable(map, i)) {
 				if (c) {
 					c->max = p - 1;
 					c->max_reg = i - map->reg_stride;
@@ -204,7 +214,7 @@ static ssize_t regmap_read_debugfs(struct regmap *map, unsigned int from,
 	start_reg = regmap_debugfs_get_dump_start(map, from, *ppos, &p);
 
 	for (i = start_reg; i <= to; i += map->reg_stride) {
-		if (!regmap_readable(map, i))
+		if (!regmap_readable(map, i) && !regmap_cached(map, i))
 			continue;
 
 		if (regmap_precious(map, i))
-- 
1.9.1

[toc] | [prev] | [next] | [standalone]

#1458689 — Applied "regmap: debugfs: Add support for dumping write only device registers" to the regmap tree

The patch

   regmap: debugfs: Add support for dumping write only device registers

has been applied to the regmap tree at

   git://git.kernel.org/pub/scm/linux/kernel/git/broonie/regmap.git 

All being well this means that it will be integrated into the linux-next
tree (usually sometime in the next 24 hours) and sent to Linus during
the next merge window (or sooner if it is a bug fix), however if
problems are discovered then the patch may be dropped or reverted.  

You may get further e-mails resulting from automated or manual testing
and review of the tree, please engage with people reporting problems and
send followup patches addressing any issues that are reported if needed.

If any updates are required or you are submitting further changes they
should be sent as incremental updates against current git, existing
patches will not be replaced.

Please add any relevant lists and maintainers to the CCs when replying
to this mail.

Thanks,
Mark

From 359a2f17604e2c1c8938be2bba32a8cef9cf6303 Mon Sep 17 00:00:00 2001
From: Cristian Birsan <cristian.birsan@microchip.com>
Date: Mon, 8 Aug 2016 18:44:22 +0300
Subject: [PATCH] regmap: debugfs: Add support for dumping write only device
 registers

Add support for dumping write only device registers in debugfs. This is
useful for audio codecs that have write only registers (like WM8731).
The logic that decides if a value can be printed is moved to
regmap_printable() function to allow for easier future updates.

Signed-off-by: Cristian Birsan <cristian.birsan@microchip.com>
Signed-off-by: Mark Brown <broonie@kernel.org>
---
 drivers/base/regmap/regmap-debugfs.c | 16 +++++++++++++---
 1 file changed, 13 insertions(+), 3 deletions(-)

diff --git a/drivers/base/regmap/regmap-debugfs.c b/drivers/base/regmap/regmap-debugfs.c
index 1ee3d40861c7..36ce3511c733 100644
--- a/drivers/base/regmap/regmap-debugfs.c
+++ b/drivers/base/regmap/regmap-debugfs.c
@@ -77,6 +77,17 @@ static void regmap_debugfs_free_dump_cache(struct regmap *map)
 	}
 }
 
+static bool regmap_printable(struct regmap *map, unsigned int reg)
+{
+	if (regmap_precious(map, reg))
+		return false;
+
+	if (!regmap_readable(map, reg) && !regmap_cached(map, reg))
+		return false;
+
+	return true;
+}
+
 /*
  * Work out where the start offset maps into register numbers, bearing
  * in mind that we suppress hidden registers.
@@ -105,8 +116,7 @@ static unsigned int regmap_debugfs_get_dump_start(struct regmap *map,
 	if (list_empty(&map->debugfs_off_cache)) {
 		for (; i <= map->max_register; i += map->reg_stride) {
 			/* Skip unprinted registers, closing off cache entry */
-			if (!regmap_readable(map, i) ||
-			    regmap_precious(map, i)) {
+			if (!regmap_printable(map, i)) {
 				if (c) {
 					c->max = p - 1;
 					c->max_reg = i - map->reg_stride;
@@ -204,7 +214,7 @@ static ssize_t regmap_read_debugfs(struct regmap *map, unsigned int from,
 	start_reg = regmap_debugfs_get_dump_start(map, from, *ppos, &p);
 
 	for (i = start_reg; i <= to; i += map->reg_stride) {
-		if (!regmap_readable(map, i))
+		if (!regmap_readable(map, i) && !regmap_cached(map, i))
 			continue;
 
 		if (regmap_precious(map, i))
-- 
2.8.1

[toc] | [prev] | [next] | [standalone]

#1458733 — Re: Applied "regmap: debugfs: Add support for dumping write only device registers" to the regmap tree

[Multipart message — attachments visible in raw view] — view raw

Hi Mark,

[auto build test ERROR on regmap/for-next]
[also build test ERROR on v4.8-rc1 next-20160809]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Mark-Brown/Applied-regmap-debugfs-Add-support-for-dumping-write-only-device-registers-to-the-regmap-tree/20160809-205351
base:   https://git.kernel.org/pub/scm/linux/kernel/git/broonie/regmap.git for-next
config: x86_64-randconfig-x013-201632 (attached as .config)
compiler: gcc-6 (Debian 6.1.1-9) 6.1.1 20160705
reproduce:
        # save the attached .config to linux build tree
        make ARCH=x86_64 

All errors (new ones prefixed by >>):

   drivers/base/regmap/regmap-debugfs.c: In function 'regmap_printable':
>> drivers/base/regmap/regmap-debugfs.c:85:37: error: implicit declaration of function 'regmap_cached' [-Werror=implicit-function-declaration]
     if (!regmap_readable(map, reg) && !regmap_cached(map, reg))
                                        ^~~~~~~~~~~~~
   cc1: some warnings being treated as errors

vim +/regmap_cached +85 drivers/base/regmap/regmap-debugfs.c

    79	
    80	static bool regmap_printable(struct regmap *map, unsigned int reg)
    81	{
    82		if (regmap_precious(map, reg))
    83			return false;
    84	
  > 85		if (!regmap_readable(map, reg) && !regmap_cached(map, reg))
    86			return false;
    87	
    88		return true;

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[toc] | [prev] | [next] | [standalone]

#1457874 — [PATCH v2 0/2] Display regmap values in debugfs for write only registers

Hi,

Thank you for the review and for the very detailed comments. I updated the 
patch series to fix the problems identified. To make sure a register is 
really cached a regcache_read() is performed on cacheable registers.

The patch was tested using a wm8731 device.

Changes since v1 (https://lkml.org/lkml/2016/8/4/327)
	+ Fix regmap_cached() to return if a register is really cached
	  instead of cacheable. 
	+ Remove unnecesary regcache_read() in regmap_read_debugfs()  

Cristian Birsan (2):
  regmap: Add a function to check if a regmap register is cached
  regmap: debugfs: Add support for dumping write only device registers

 drivers/base/regmap/internal.h       |  1 +
 drivers/base/regmap/regmap-debugfs.c | 16 +++++++++++++---
 drivers/base/regmap/regmap.c         | 23 +++++++++++++++++++++++
 3 files changed, 37 insertions(+), 3 deletions(-)

-- 
1.9.1

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web