2021-06-08 19:21:40

by Vadym Kochan

[permalink] [raw]
Subject: [PATCH v2 1/3] nvmem: core: introduce cells parser

Current implementation does not allow to register cells for already
registered nvmem device and requires that this should be done before. But
there might a driver which needs to parse the nvmem device and after
register a cells table.

Introduce nvmem_parser API which allows to register cells parser which
is called during nvmem device registration. During this stage the parser
can read the nvmem device and register the cells table.

Signed-off-by: Vadym Kochan <[email protected]>
---
v2:
1) Added nvmem_parser_data so parser implementation
should fill it with cells table and lookups which
will be registered by core.c after cells_parse was
succeed.

2) Removed cells_remove callback from nvmem_parser_config which
is not needed because cells table & lookups are
registered/unregistered automatically by core.

3) Use new device property to read cells parser name during nvmem
registration. Removed of_node usage.

4) parser's module refcount is incremented/decremented on each parser
bind/unbind to nvmem device.

drivers/nvmem/core.c | 178 +++++++++++++++++++++++++++++++++
include/linux/nvmem-provider.h | 31 ++++++
2 files changed, 209 insertions(+)

diff --git a/drivers/nvmem/core.c b/drivers/nvmem/core.c
index bca671ff4e54..648373ced6d4 100644
--- a/drivers/nvmem/core.c
+++ b/drivers/nvmem/core.c
@@ -39,6 +39,7 @@ struct nvmem_device {
nvmem_reg_read_t reg_read;
nvmem_reg_write_t reg_write;
struct gpio_desc *wp_gpio;
+ struct nvmem_parser_data *parser_data;
void *priv;
};

@@ -57,6 +58,13 @@ struct nvmem_cell {
struct list_head node;
};

+struct nvmem_parser {
+ struct list_head head;
+ nvmem_parse_t cells_parse;
+ const char *name;
+ struct module *owner;
+};
+
static DEFINE_MUTEX(nvmem_mutex);
static DEFINE_IDA(nvmem_ida);

@@ -66,6 +74,9 @@ static LIST_HEAD(nvmem_cell_tables);
static DEFINE_MUTEX(nvmem_lookup_mutex);
static LIST_HEAD(nvmem_lookup_list);

+static DEFINE_MUTEX(nvmem_parser_mutex);
+static LIST_HEAD(nvmem_parser_list);
+
static BLOCKING_NOTIFIER_HEAD(nvmem_notifier);

static int __nvmem_reg_read(struct nvmem_device *nvmem, unsigned int offset,
@@ -418,6 +429,118 @@ static struct bus_type nvmem_bus_type = {
.name = "nvmem",
};

+static struct nvmem_parser *__nvmem_parser_get(const char *name)
+{
+ struct nvmem_parser *tmp, *parser = NULL;
+
+ list_for_each_entry(tmp, &nvmem_parser_list, head) {
+ if (strcmp(name, tmp->name) == 0) {
+ parser = tmp;
+ break;
+ }
+ }
+
+ if (!parser)
+ return ERR_PTR(-EPROBE_DEFER);
+
+ if (!try_module_get(parser->owner)) {
+ pr_err("could not increase module refcount for parser %s\n",
+ parser->name);
+ return ERR_PTR(-EINVAL);
+
+ }
+
+ return parser;
+}
+
+static void nvmem_parser_put(const struct nvmem_parser *parser)
+{
+ module_put(parser->owner);
+}
+
+static int nvmem_parser_bind(struct nvmem_device *nvmem, const char *name)
+{
+ struct nvmem_parser_data *data;
+ struct nvmem_parser *parser;
+ int err;
+
+ mutex_lock(&nvmem_parser_mutex);
+
+ parser = __nvmem_parser_get(name);
+ err = PTR_ERR_OR_ZERO(parser);
+ if (!err) {
+ data = kzalloc(sizeof(*data), GFP_KERNEL);
+ if (data) {
+ data->parser = parser;
+ nvmem->parser_data = data;
+ } else {
+ nvmem_parser_put(parser);
+ err = -ENOMEM;
+ }
+ }
+
+ mutex_unlock(&nvmem_parser_mutex);
+
+ return err;
+}
+
+static void nvmem_parser_unbind(const struct nvmem_device *nvmem)
+{
+ struct nvmem_parser_data *data = nvmem->parser_data;
+
+ if (data->table.cells) {
+ nvmem_del_cell_table(&data->table);
+ kfree(data->table.cells);
+ }
+ if (data->lookup) {
+ nvmem_del_cell_lookups(data->lookup, data->nlookups);
+ kfree(data->lookup);
+ }
+
+ nvmem_parser_put(data->parser);
+}
+
+static void nvmem_parser_data_register(struct nvmem_device *nvmem,
+ struct nvmem_parser_data *data)
+{
+ if (data->table.cells) {
+ if (!data->table.nvmem_name)
+ data->table.nvmem_name = nvmem_dev_name(nvmem);
+
+ nvmem_add_cell_table(&data->table);
+ }
+
+ if (data->lookup) {
+ struct nvmem_cell_lookup *lookup = &data->lookup[0];
+ int i = 0;
+
+ for (i = 0; i < data->nlookups; i++, lookup++) {
+ if (!lookup->nvmem_name)
+ lookup->nvmem_name = nvmem_dev_name(nvmem);
+
+ if (!lookup->dev_id)
+ lookup->dev_id = data->parser->name;
+ }
+
+ nvmem_add_cell_lookups(data->lookup, data->nlookups);
+ }
+}
+
+static void nvmem_cells_parse(struct nvmem_device *nvmem)
+{
+ struct nvmem_parser_data *data = nvmem->parser_data;
+ struct nvmem_parser *parser = data->parser;
+ int err;
+
+ mutex_lock(&nvmem_parser_mutex);
+
+ err = parser->cells_parse(nvmem, data);
+ if (!err)
+ nvmem_parser_data_register(nvmem, data);
+
+ mutex_unlock(&nvmem_parser_mutex);
+}
+
static void nvmem_cell_drop(struct nvmem_cell *cell)
{
blocking_notifier_call_chain(&nvmem_notifier, NVMEM_CELL_REMOVE, cell);
@@ -435,6 +558,9 @@ static void nvmem_device_remove_all_cells(const struct nvmem_device *nvmem)

list_for_each_entry_safe(cell, p, &nvmem->cells, node)
nvmem_cell_drop(cell);
+
+ if (nvmem->parser_data)
+ nvmem_parser_unbind(nvmem);
}

static void nvmem_cell_add(struct nvmem_cell *cell)
@@ -739,6 +865,7 @@ static int nvmem_add_cells_from_of(struct nvmem_device *nvmem)
struct nvmem_device *nvmem_register(const struct nvmem_config *config)
{
struct nvmem_device *nvmem;
+ const char *parser_name;
int rval;

if (!config->dev)
@@ -809,6 +936,16 @@ struct nvmem_device *nvmem_register(const struct nvmem_config *config)
nvmem->read_only = device_property_present(config->dev, "read-only") ||
config->read_only || !nvmem->reg_write;

+ if (!device_property_read_string(config->dev, "nvmem-cell-parser-name",
+ &parser_name)) {
+ rval = nvmem_parser_bind(nvmem, parser_name);
+ if (rval) {
+ ida_free(&nvmem_ida, nvmem->id);
+ kfree(nvmem);
+ return ERR_PTR(rval);
+ }
+ }
+
#ifdef CONFIG_NVMEM_SYSFS
nvmem->dev.groups = nvmem_dev_groups;
#endif
@@ -837,6 +974,9 @@ struct nvmem_device *nvmem_register(const struct nvmem_config *config)
goto err_teardown_compat;
}

+ if (nvmem->parser_data)
+ nvmem_cells_parse(nvmem);
+
rval = nvmem_add_cells_from_table(nvmem);
if (rval)
goto err_remove_cells;
@@ -857,6 +997,8 @@ struct nvmem_device *nvmem_register(const struct nvmem_config *config)
err_device_del:
device_del(&nvmem->dev);
err_put_device:
+ if (nvmem->parser_data)
+ nvmem_parser_unbind(nvmem);
put_device(&nvmem->dev);

return ERR_PTR(rval);
@@ -1891,6 +2033,42 @@ const char *nvmem_dev_name(struct nvmem_device *nvmem)
}
EXPORT_SYMBOL_GPL(nvmem_dev_name);

+struct nvmem_parser *
+nvmem_parser_register(const struct nvmem_parser_config *config)
+{
+ struct nvmem_parser *parser;
+
+ if (!config->cells_parse)
+ return ERR_PTR(-EINVAL);
+
+ if (!config->name)
+ return ERR_PTR(-EINVAL);
+
+ parser = kzalloc(sizeof(*parser), GFP_KERNEL);
+ if (!parser)
+ return ERR_PTR(-ENOMEM);
+
+ parser->cells_parse = config->cells_parse;
+ parser->owner = config->owner;
+ parser->name = config->name;
+
+ mutex_lock(&nvmem_parser_mutex);
+ list_add(&parser->head, &nvmem_parser_list);
+ mutex_unlock(&nvmem_parser_mutex);
+
+ return parser;
+}
+EXPORT_SYMBOL(nvmem_parser_register);
+
+void nvmem_parser_unregister(struct nvmem_parser *parser)
+{
+ mutex_lock(&nvmem_parser_mutex);
+ list_del(&parser->head);
+ kfree(parser);
+ mutex_unlock(&nvmem_parser_mutex);
+}
+EXPORT_SYMBOL(nvmem_parser_unregister);
+
static int __init nvmem_init(void)
{
return bus_register(&nvmem_bus_type);
diff --git a/include/linux/nvmem-provider.h b/include/linux/nvmem-provider.h
index e162b757b6d5..447241706554 100644
--- a/include/linux/nvmem-provider.h
+++ b/include/linux/nvmem-provider.h
@@ -15,10 +15,15 @@

struct nvmem_device;
struct nvmem_cell_info;
+struct nvmem_cell_table;
+struct nvmem_parser_data;
+
typedef int (*nvmem_reg_read_t)(void *priv, unsigned int offset,
void *val, size_t bytes);
typedef int (*nvmem_reg_write_t)(void *priv, unsigned int offset,
void *val, size_t bytes);
+typedef int (*nvmem_parse_t)(struct nvmem_device *nvmem,
+ struct nvmem_parser_data *data);

enum nvmem_type {
NVMEM_TYPE_UNKNOWN = 0,
@@ -117,6 +122,19 @@ struct nvmem_cell_table {
struct list_head node;
};

+struct nvmem_parser_config {
+ const char *name;
+ nvmem_parse_t cells_parse;
+ struct module *owner;
+};
+
+struct nvmem_parser_data {
+ struct nvmem_cell_table table;
+ struct nvmem_cell_lookup *lookup;
+ int nlookups;
+ struct nvmem_parser *parser;
+};
+
#if IS_ENABLED(CONFIG_NVMEM)

struct nvmem_device *nvmem_register(const struct nvmem_config *cfg);
@@ -130,6 +148,11 @@ int devm_nvmem_unregister(struct device *dev, struct nvmem_device *nvmem);
void nvmem_add_cell_table(struct nvmem_cell_table *table);
void nvmem_del_cell_table(struct nvmem_cell_table *table);

+struct nvmem_parser *
+nvmem_parser_register(const struct nvmem_parser_config *config);
+
+void nvmem_parser_unregister(struct nvmem_parser *parser);
+
#else

static inline struct nvmem_device *nvmem_register(const struct nvmem_config *c)
@@ -154,5 +177,13 @@ devm_nvmem_unregister(struct device *dev, struct nvmem_device *nvmem)
static inline void nvmem_add_cell_table(struct nvmem_cell_table *table) {}
static inline void nvmem_del_cell_table(struct nvmem_cell_table *table) {}

+static inline struct nvmem_parser *
+nvmem_parser_register(const struct nvmem_parser_config *config)
+{
+ return -EOPNOTSUPP;
+}
+
+static inline void nvmem_parser_unregister(struct nvmem_parser *parser) {}
+
#endif /* CONFIG_NVMEM */
#endif /* ifndef _LINUX_NVMEM_PROVIDER_H */
--
2.17.1


2021-06-08 22:51:22

by kernel test robot

[permalink] [raw]
Subject: Re: [PATCH v2 1/3] nvmem: core: introduce cells parser

Hi Vadym,

I love your patch! Yet something to improve:

[auto build test ERROR on robh/for-next]
[also build test ERROR on linux/master linus/master v5.13-rc5 next-20210608]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url: https://github.com/0day-ci/linux/commits/Vadym-Kochan/nvmem-add-ONIE-NVMEM-cells-parser/20210609-031008
base: https://git.kernel.org/pub/scm/linux/kernel/git/robh/linux.git for-next
config: powerpc-allnoconfig (attached as .config)
compiler: powerpc-linux-gcc (GCC) 9.3.0
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# https://github.com/0day-ci/linux/commit/e7cd03f23c4980914d9de330fd018cb733f84d8f
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review Vadym-Kochan/nvmem-add-ONIE-NVMEM-cells-parser/20210609-031008
git checkout e7cd03f23c4980914d9de330fd018cb733f84d8f
# save the attached .config to linux build tree
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross ARCH=powerpc

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <[email protected]>

All errors (new ones prefixed by >>):

In file included from include/linux/rtc.h:18,
from arch/powerpc/kernel/time.c:47:
include/linux/nvmem-provider.h: In function 'nvmem_parser_register':
>> include/linux/nvmem-provider.h:183:9: error: returning 'int' from a function with return type 'struct nvmem_parser *' makes pointer from integer without a cast [-Werror=int-conversion]
183 | return -EOPNOTSUPP;
| ^
cc1: all warnings being treated as errors


vim +183 include/linux/nvmem-provider.h

179
180 static inline struct nvmem_parser *
181 nvmem_parser_register(const struct nvmem_parser_config *config)
182 {
> 183 return -EOPNOTSUPP;
184 }
185

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/[email protected]


Attachments:
(No filename) (2.23 kB)
.config.gz (6.97 kB)
Download all attachments

2021-06-09 12:31:47

by kernel test robot

[permalink] [raw]
Subject: Re: [PATCH v2 1/3] nvmem: core: introduce cells parser

Hi Vadym,

I love your patch! Perhaps something to improve:

[auto build test WARNING on robh/for-next]
[also build test WARNING on linux/master linus/master v5.13-rc5 next-20210608]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url: https://github.com/0day-ci/linux/commits/Vadym-Kochan/nvmem-add-ONIE-NVMEM-cells-parser/20210609-031008
base: https://git.kernel.org/pub/scm/linux/kernel/git/robh/linux.git for-next
config: s390-randconfig-r012-20210608 (attached as .config)
compiler: clang version 13.0.0 (https://github.com/llvm/llvm-project d2012d965d60c3258b3a69d024491698f8aec386)
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# install s390 cross compiling tool for clang build
# apt-get install binutils-s390x-linux-gnu
# https://github.com/0day-ci/linux/commit/e7cd03f23c4980914d9de330fd018cb733f84d8f
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review Vadym-Kochan/nvmem-add-ONIE-NVMEM-cells-parser/20210609-031008
git checkout e7cd03f23c4980914d9de330fd018cb733f84d8f
# save the attached .config to linux build tree
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross ARCH=s390

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <[email protected]>

All warnings (new ones prefixed by >>):

In file included from lib/vsprintf.c:36:
In file included from include/linux/rtc.h:18:
>> include/linux/nvmem-provider.h:183:9: warning: incompatible integer to pointer conversion returning 'int' from a function with result type 'struct nvmem_parser *' [-Wint-conversion]
return -EOPNOTSUPP;
^~~~~~~~~~~
In file included from lib/vsprintf.c:40:
In file included from include/net/addrconf.h:50:
In file included from include/linux/ipv6.h:88:
In file included from include/linux/tcp.h:17:
In file included from include/linux/skbuff.h:31:
In file included from include/linux/dma-mapping.h:10:
In file included from include/linux/scatterlist.h:9:
In file included from arch/s390/include/asm/io.h:75:
include/asm-generic/io.h:464:31: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
val = __raw_readb(PCI_IOBASE + addr);
~~~~~~~~~~ ^
include/asm-generic/io.h:477:61: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
val = __le16_to_cpu((__le16 __force)__raw_readw(PCI_IOBASE + addr));
~~~~~~~~~~ ^
include/uapi/linux/byteorder/big_endian.h:36:59: note: expanded from macro '__le16_to_cpu'
#define __le16_to_cpu(x) __swab16((__force __u16)(__le16)(x))
^
include/uapi/linux/swab.h:102:54: note: expanded from macro '__swab16'
#define __swab16(x) (__u16)__builtin_bswap16((__u16)(x))
^
In file included from lib/vsprintf.c:40:
In file included from include/net/addrconf.h:50:
In file included from include/linux/ipv6.h:88:
In file included from include/linux/tcp.h:17:
In file included from include/linux/skbuff.h:31:
In file included from include/linux/dma-mapping.h:10:
In file included from include/linux/scatterlist.h:9:
In file included from arch/s390/include/asm/io.h:75:
include/asm-generic/io.h:490:61: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
val = __le32_to_cpu((__le32 __force)__raw_readl(PCI_IOBASE + addr));
~~~~~~~~~~ ^
include/uapi/linux/byteorder/big_endian.h:34:59: note: expanded from macro '__le32_to_cpu'
#define __le32_to_cpu(x) __swab32((__force __u32)(__le32)(x))
^
include/uapi/linux/swab.h:115:54: note: expanded from macro '__swab32'
#define __swab32(x) (__u32)__builtin_bswap32((__u32)(x))
^
In file included from lib/vsprintf.c:40:
In file included from include/net/addrconf.h:50:
In file included from include/linux/ipv6.h:88:
In file included from include/linux/tcp.h:17:
In file included from include/linux/skbuff.h:31:
In file included from include/linux/dma-mapping.h:10:
In file included from include/linux/scatterlist.h:9:
In file included from arch/s390/include/asm/io.h:75:
include/asm-generic/io.h:501:33: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
__raw_writeb(value, PCI_IOBASE + addr);
~~~~~~~~~~ ^
include/asm-generic/io.h:511:59: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
__raw_writew((u16 __force)cpu_to_le16(value), PCI_IOBASE + addr);
~~~~~~~~~~ ^
include/asm-generic/io.h:521:59: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
__raw_writel((u32 __force)cpu_to_le32(value), PCI_IOBASE + addr);
~~~~~~~~~~ ^
include/asm-generic/io.h:609:20: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
readsb(PCI_IOBASE + addr, buffer, count);
~~~~~~~~~~ ^
include/asm-generic/io.h:617:20: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
readsw(PCI_IOBASE + addr, buffer, count);
~~~~~~~~~~ ^
include/asm-generic/io.h:625:20: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
readsl(PCI_IOBASE + addr, buffer, count);
~~~~~~~~~~ ^
include/asm-generic/io.h:634:21: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
writesb(PCI_IOBASE + addr, buffer, count);
~~~~~~~~~~ ^
include/asm-generic/io.h:643:21: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
writesw(PCI_IOBASE + addr, buffer, count);
~~~~~~~~~~ ^
include/asm-generic/io.h:652:21: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
writesl(PCI_IOBASE + addr, buffer, count);
~~~~~~~~~~ ^
13 warnings generated.
--
In file included from lib/test_printf.c:13:
In file included from include/linux/rtc.h:18:
>> include/linux/nvmem-provider.h:183:9: warning: incompatible integer to pointer conversion returning 'int' from a function with result type 'struct nvmem_parser *' [-Wint-conversion]
return -EOPNOTSUPP;
^~~~~~~~~~~
lib/test_printf.c:157:52: warning: format specifies type 'unsigned char' but the argument has type 'int' [-Wformat]
test("0|1|1|128|255", "%hhu|%hhu|%hhu|%hhu|%hhu", 0, 1, 257, 128, -1);
~~~~ ^
%d
lib/test_printf.c:137:40: note: expanded from macro 'test'
__test(expect, strlen(expect), fmt, ##__VA_ARGS__)
~~~ ^~~~~~~~~~~
lib/test_printf.c:157:55: warning: format specifies type 'unsigned char' but the argument has type 'int' [-Wformat]
test("0|1|1|128|255", "%hhu|%hhu|%hhu|%hhu|%hhu", 0, 1, 257, 128, -1);
~~~~ ^
%d
lib/test_printf.c:137:40: note: expanded from macro 'test'
__test(expect, strlen(expect), fmt, ##__VA_ARGS__)
~~~ ^~~~~~~~~~~
lib/test_printf.c:157:58: warning: format specifies type 'unsigned char' but the argument has type 'int' [-Wformat]
test("0|1|1|128|255", "%hhu|%hhu|%hhu|%hhu|%hhu", 0, 1, 257, 128, -1);
~~~~ ^~~
%d
lib/test_printf.c:137:40: note: expanded from macro 'test'
__test(expect, strlen(expect), fmt, ##__VA_ARGS__)
~~~ ^~~~~~~~~~~
lib/test_printf.c:157:63: warning: format specifies type 'unsigned char' but the argument has type 'int' [-Wformat]
test("0|1|1|128|255", "%hhu|%hhu|%hhu|%hhu|%hhu", 0, 1, 257, 128, -1);
~~~~ ^~~
%d
lib/test_printf.c:137:40: note: expanded from macro 'test'
__test(expect, strlen(expect), fmt, ##__VA_ARGS__)
~~~ ^~~~~~~~~~~
lib/test_printf.c:157:68: warning: format specifies type 'unsigned char' but the argument has type 'int' [-Wformat]
test("0|1|1|128|255", "%hhu|%hhu|%hhu|%hhu|%hhu", 0, 1, 257, 128, -1);
~~~~ ^~
%d
lib/test_printf.c:137:40: note: expanded from macro 'test'
__test(expect, strlen(expect), fmt, ##__VA_ARGS__)
~~~ ^~~~~~~~~~~
lib/test_printf.c:158:52: warning: format specifies type 'char' but the argument has type 'int' [-Wformat]
test("0|1|1|-128|-1", "%hhd|%hhd|%hhd|%hhd|%hhd", 0, 1, 257, 128, -1);
~~~~ ^
%d
lib/test_printf.c:137:40: note: expanded from macro 'test'
__test(expect, strlen(expect), fmt, ##__VA_ARGS__)
~~~ ^~~~~~~~~~~
lib/test_printf.c:158:55: warning: format specifies type 'char' but the argument has type 'int' [-Wformat]
test("0|1|1|-128|-1", "%hhd|%hhd|%hhd|%hhd|%hhd", 0, 1, 257, 128, -1);
~~~~ ^
%d
lib/test_printf.c:137:40: note: expanded from macro 'test'
__test(expect, strlen(expect), fmt, ##__VA_ARGS__)
~~~ ^~~~~~~~~~~
lib/test_printf.c:158:58: warning: format specifies type 'char' but the argument has type 'int' [-Wformat]
test("0|1|1|-128|-1", "%hhd|%hhd|%hhd|%hhd|%hhd", 0, 1, 257, 128, -1);
~~~~ ^~~
%d
lib/test_printf.c:137:40: note: expanded from macro 'test'
__test(expect, strlen(expect), fmt, ##__VA_ARGS__)
~~~ ^~~~~~~~~~~
lib/test_printf.c:158:63: warning: format specifies type 'char' but the argument has type 'int' [-Wformat]
test("0|1|1|-128|-1", "%hhd|%hhd|%hhd|%hhd|%hhd", 0, 1, 257, 128, -1);
~~~~ ^~~
%d
lib/test_printf.c:137:40: note: expanded from macro 'test'
__test(expect, strlen(expect), fmt, ##__VA_ARGS__)
~~~ ^~~~~~~~~~~
lib/test_printf.c:158:68: warning: format specifies type 'char' but the argument has type 'int' [-Wformat]
test("0|1|1|-128|-1", "%hhd|%hhd|%hhd|%hhd|%hhd", 0, 1, 257, 128, -1);
~~~~ ^~
%d
lib/test_printf.c:137:40: note: expanded from macro 'test'
__test(expect, strlen(expect), fmt, ##__VA_ARGS__)
~~~ ^~~~~~~~~~~
lib/test_printf.c:159:41: warning: format specifies type 'unsigned short' but the argument has type 'int' [-Wformat]
test("2015122420151225", "%ho%ho%#ho", 1037, 5282, -11627);
~~~ ^~~~
%o
lib/test_printf.c:137:40: note: expanded from macro 'test'
__test(expect, strlen(expect), fmt, ##__VA_ARGS__)
~~~ ^~~~~~~~~~~
lib/test_printf.c:159:47: warning: format specifies type 'unsigned short' but the argument has type 'int' [-Wformat]
test("2015122420151225", "%ho%ho%#ho", 1037, 5282, -11627);
~~~ ^~~~
%o
lib/test_printf.c:137:40: note: expanded from macro 'test'
__test(expect, strlen(expect), fmt, ##__VA_ARGS__)
~~~ ^~~~~~~~~~~
lib/test_printf.c:159:53: warning: format specifies type 'unsigned short' but the argument has type 'int' [-Wformat]
test("2015122420151225", "%ho%ho%#ho", 1037, 5282, -11627);
~~~~ ^~~~~~
%#o
lib/test_printf.c:137:40: note: expanded from macro 'test'
__test(expect, strlen(expect), fmt, ##__VA_ARGS__)
~~~ ^~~~~~~~~~~
14 warnings generated.
--
In file included from block/partitions/msdos.c:31:
In file included from block/partitions/check.h:3:
In file included from include/linux/blkdev.h:25:
In file included from include/linux/scatterlist.h:9:
In file included from arch/s390/include/asm/io.h:75:
include/asm-generic/io.h:464:31: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
val = __raw_readb(PCI_IOBASE + addr);
~~~~~~~~~~ ^
include/asm-generic/io.h:477:61: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
val = __le16_to_cpu((__le16 __force)__raw_readw(PCI_IOBASE + addr));
~~~~~~~~~~ ^
include/uapi/linux/byteorder/big_endian.h:36:59: note: expanded from macro '__le16_to_cpu'
#define __le16_to_cpu(x) __swab16((__force __u16)(__le16)(x))
^
include/uapi/linux/swab.h:102:54: note: expanded from macro '__swab16'
#define __swab16(x) (__u16)__builtin_bswap16((__u16)(x))
^
In file included from block/partitions/msdos.c:31:
In file included from block/partitions/check.h:3:
In file included from include/linux/blkdev.h:25:
In file included from include/linux/scatterlist.h:9:
In file included from arch/s390/include/asm/io.h:75:
include/asm-generic/io.h:490:61: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
val = __le32_to_cpu((__le32 __force)__raw_readl(PCI_IOBASE + addr));
~~~~~~~~~~ ^
include/uapi/linux/byteorder/big_endian.h:34:59: note: expanded from macro '__le32_to_cpu'
#define __le32_to_cpu(x) __swab32((__force __u32)(__le32)(x))
^
include/uapi/linux/swab.h:115:54: note: expanded from macro '__swab32'
#define __swab32(x) (__u32)__builtin_bswap32((__u32)(x))
^
In file included from block/partitions/msdos.c:31:
In file included from block/partitions/check.h:3:
In file included from include/linux/blkdev.h:25:
In file included from include/linux/scatterlist.h:9:
In file included from arch/s390/include/asm/io.h:75:
include/asm-generic/io.h:501:33: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
__raw_writeb(value, PCI_IOBASE + addr);
~~~~~~~~~~ ^
include/asm-generic/io.h:511:59: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
__raw_writew((u16 __force)cpu_to_le16(value), PCI_IOBASE + addr);
~~~~~~~~~~ ^
include/asm-generic/io.h:521:59: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
__raw_writel((u32 __force)cpu_to_le32(value), PCI_IOBASE + addr);
~~~~~~~~~~ ^
include/asm-generic/io.h:609:20: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
readsb(PCI_IOBASE + addr, buffer, count);
~~~~~~~~~~ ^
include/asm-generic/io.h:617:20: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
readsw(PCI_IOBASE + addr, buffer, count);
~~~~~~~~~~ ^
include/asm-generic/io.h:625:20: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
readsl(PCI_IOBASE + addr, buffer, count);
~~~~~~~~~~ ^
include/asm-generic/io.h:634:21: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
writesb(PCI_IOBASE + addr, buffer, count);
~~~~~~~~~~ ^
include/asm-generic/io.h:643:21: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
writesw(PCI_IOBASE + addr, buffer, count);
~~~~~~~~~~ ^
include/asm-generic/io.h:652:21: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
writesl(PCI_IOBASE + addr, buffer, count);
~~~~~~~~~~ ^
In file included from block/partitions/msdos.c:32:
In file included from block/partitions/efi.h:20:
In file included from include/linux/efi.h:20:
In file included from include/linux/rtc.h:18:
>> include/linux/nvmem-provider.h:183:9: warning: incompatible integer to pointer conversion returning 'int' from a function with result type 'struct nvmem_parser *' [-Wint-conversion]
return -EOPNOTSUPP;
^~~~~~~~~~~
13 warnings generated.
--
In file included from kernel/time/ntp.c:10:
In file included from include/linux/clocksource.h:22:
In file included from arch/s390/include/asm/io.h:75:
include/asm-generic/io.h:464:31: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
val = __raw_readb(PCI_IOBASE + addr);
~~~~~~~~~~ ^
include/asm-generic/io.h:477:61: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
val = __le16_to_cpu((__le16 __force)__raw_readw(PCI_IOBASE + addr));
~~~~~~~~~~ ^
include/uapi/linux/byteorder/big_endian.h:36:59: note: expanded from macro '__le16_to_cpu'
#define __le16_to_cpu(x) __swab16((__force __u16)(__le16)(x))
^
include/uapi/linux/swab.h:102:54: note: expanded from macro '__swab16'
#define __swab16(x) (__u16)__builtin_bswap16((__u16)(x))
^
In file included from kernel/time/ntp.c:10:
In file included from include/linux/clocksource.h:22:
In file included from arch/s390/include/asm/io.h:75:
include/asm-generic/io.h:490:61: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
val = __le32_to_cpu((__le32 __force)__raw_readl(PCI_IOBASE + addr));
~~~~~~~~~~ ^
include/uapi/linux/byteorder/big_endian.h:34:59: note: expanded from macro '__le32_to_cpu'
#define __le32_to_cpu(x) __swab32((__force __u32)(__le32)(x))
^
include/uapi/linux/swab.h:115:54: note: expanded from macro '__swab32'
#define __swab32(x) (__u32)__builtin_bswap32((__u32)(x))
^
In file included from kernel/time/ntp.c:10:
In file included from include/linux/clocksource.h:22:
In file included from arch/s390/include/asm/io.h:75:
include/asm-generic/io.h:501:33: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
__raw_writeb(value, PCI_IOBASE + addr);
~~~~~~~~~~ ^
include/asm-generic/io.h:511:59: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
__raw_writew((u16 __force)cpu_to_le16(value), PCI_IOBASE + addr);
~~~~~~~~~~ ^
include/asm-generic/io.h:521:59: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
__raw_writel((u32 __force)cpu_to_le32(value), PCI_IOBASE + addr);
~~~~~~~~~~ ^
include/asm-generic/io.h:609:20: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
readsb(PCI_IOBASE + addr, buffer, count);
~~~~~~~~~~ ^
include/asm-generic/io.h:617:20: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
readsw(PCI_IOBASE + addr, buffer, count);
~~~~~~~~~~ ^
include/asm-generic/io.h:625:20: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
readsl(PCI_IOBASE + addr, buffer, count);
~~~~~~~~~~ ^
include/asm-generic/io.h:634:21: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
writesb(PCI_IOBASE + addr, buffer, count);
~~~~~~~~~~ ^
include/asm-generic/io.h:643:21: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
writesw(PCI_IOBASE + addr, buffer, count);
~~~~~~~~~~ ^
include/asm-generic/io.h:652:21: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
writesl(PCI_IOBASE + addr, buffer, count);
~~~~~~~~~~ ^
In file included from kernel/time/ntp.c:19:
In file included from include/linux/rtc.h:18:
>> include/linux/nvmem-provider.h:183:9: warning: incompatible integer to pointer conversion returning 'int' from a function with result type 'struct nvmem_parser *' [-Wint-conversion]
return -EOPNOTSUPP;
^~~~~~~~~~~
kernel/time/ntp.c:245:19: warning: unused function 'ntp_synced' [-Wunused-function]
static inline int ntp_synced(void)
^
14 warnings generated.
..


vim +183 include/linux/nvmem-provider.h

179
180 static inline struct nvmem_parser *
181 nvmem_parser_register(const struct nvmem_parser_config *config)
182 {
> 183 return -EOPNOTSUPP;
184 }
185

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/[email protected]


Attachments:
(No filename) (24.02 kB)
.config.gz (14.83 kB)
Download all attachments

2021-06-14 10:57:03

by Srinivas Kandagatla

[permalink] [raw]
Subject: Re: [PATCH v2 1/3] nvmem: core: introduce cells parser



On 08/06/2021 20:03, Vadym Kochan wrote:
> Current implementation does not allow to register cells for already
> registered nvmem device and requires that this should be done before. But
> there might a driver which needs to parse the nvmem device and after
> register a cells table.
>
> Introduce nvmem_parser API which allows to register cells parser which
> is called during nvmem device registration. During this stage the parser
> can read the nvmem device and register the cells table.
>
> Signed-off-by: Vadym Kochan <[email protected]>
> ---
> v2:
> 1) Added nvmem_parser_data so parser implementation
> should fill it with cells table and lookups which
> will be registered by core.c after cells_parse was
> succeed.
>
> 2) Removed cells_remove callback from nvmem_parser_config which
> is not needed because cells table & lookups are
> registered/unregistered automatically by core.
>
> 3) Use new device property to read cells parser name during nvmem
> registration. Removed of_node usage.
>
> 4) parser's module refcount is incremented/decremented on each parser
> bind/unbind to nvmem device.
>
> drivers/nvmem/core.c | 178 +++++++++++++++++++++++++++++++++
> include/linux/nvmem-provider.h | 31 ++++++
> 2 files changed, 209 insertions(+)
>
> diff --git a/drivers/nvmem/core.c b/drivers/nvmem/core.c
> index bca671ff4e54..648373ced6d4 100644
> --- a/drivers/nvmem/core.c
> +++ b/drivers/nvmem/core.c
> @@ -39,6 +39,7 @@ struct nvmem_device {
> nvmem_reg_read_t reg_read;
> nvmem_reg_write_t reg_write;
> struct gpio_desc *wp_gpio;
> + struct nvmem_parser_data *parser_data;

This should be renamed to nvmem_cell_info_parser or something on those
lines to avoid any misunderstanding on what exactly this parser is about.

May be can totally avoid this by using parser name only during register.

> void *priv;
> };
>
> @@ -57,6 +58,13 @@ struct nvmem_cell {
> struct list_head node;
> };
>
> +struct nvmem_parser {
> + struct list_head head;
> + nvmem_parse_t cells_parse;
> + const char *name;
> + struct module *owner;
> +};
> +
> static DEFINE_MUTEX(nvmem_mutex);
> static DEFINE_IDA(nvmem_ida);
>
> @@ -66,6 +74,9 @@ static LIST_HEAD(nvmem_cell_tables);
> static DEFINE_MUTEX(nvmem_lookup_mutex);
> static LIST_HEAD(nvmem_lookup_list);
>
> +static DEFINE_MUTEX(nvmem_parser_mutex);
> +static LIST_HEAD(nvmem_parser_list);
> +
> static BLOCKING_NOTIFIER_HEAD(nvmem_notifier);
>
> static int __nvmem_reg_read(struct nvmem_device *nvmem, unsigned int offset,
> @@ -418,6 +429,118 @@ static struct bus_type nvmem_bus_type = {
> .name = "nvmem",
> };
>
> +static struct nvmem_parser *__nvmem_parser_get(const char *name)
> +{
> + struct nvmem_parser *tmp, *parser = NULL;
> +
> + list_for_each_entry(tmp, &nvmem_parser_list, head) {
> + if (strcmp(name, tmp->name) == 0) {
> + parser = tmp;
> + break;
> + }
> + }
> +
> + if (!parser)
> + return ERR_PTR(-EPROBE_DEFER);
> +
> + if (!try_module_get(parser->owner)) {
> + pr_err("could not increase module refcount for parser %s\n",
> + parser->name);
> + return ERR_PTR(-EINVAL);
> +
> + }
> +
> + return parser;
> +}
> +
> +static void nvmem_parser_put(const struct nvmem_parser *parser)
> +{
> + module_put(parser->owner);
> +}
> +
> +static int nvmem_parser_bind(struct nvmem_device *nvmem, const char *name)
> +{
Do we really need parser bind/unbind mechanisms for what we are trying
to do here.

It's just simply parsing cell info during nvmem register, do we really
care if parser is there or not after that?

code can be probably made much simpler by just doing this in nvmem_register

parser_get()
parse_get_cell_info()
parser_put()

AFAIU, that is all we need.

> + struct nvmem_parser_data *data;
> + struct nvmem_parser *parser;
> + int err;
> +
> + mutex_lock(&nvmem_parser_mutex);
> +
> + parser = __nvmem_parser_get(name);
> + err = PTR_ERR_OR_ZERO(parser);
> + if (!err) { > + data = kzalloc(sizeof(*data), GFP_KERNEL);
> + if (data) {
> + data->parser = parser;
> + nvmem->parser_data = data;
> + } else {
> + nvmem_parser_put(parser);
> + err = -ENOMEM;
> + }
> + }
> +
> + mutex_unlock(&nvmem_parser_mutex);
> +
> + return err;
> +}
> +
> +static void nvmem_parser_unbind(const struct nvmem_device *nvmem)
> +{
> + struct nvmem_parser_data *data = nvmem->parser_data;
> +
> + if (data->table.cells) {
> + nvmem_del_cell_table(&data->table);
> + kfree(data->table.cells);
who has allocated memory for this, its confusing for this to be freed in
core.
> + }
> + if (data->lookup) { > + nvmem_del_cell_lookups(data->lookup, data->nlookups);
> + kfree(data->lookup);
> + }
> +
> + nvmem_parser_put(data->parser);
> +}
> +
> +static void nvmem_parser_data_register(struct nvmem_device *nvmem,
> + struct nvmem_parser_data *data)
> +{
> + if (data->table.cells) {
> + if (!data->table.nvmem_name)
> + data->table.nvmem_name = nvmem_dev_name(nvmem);
> +
> + nvmem_add_cell_table(&data->table);
> + }
> +
> + if (data->lookup) {
Why do we need lookups?
the cells are already associated with provider. lookups are normally
used for associating devices with nvmemcell more for old non-dt machine
code.

you can remove this.
> + struct nvmem_cell_lookup *lookup = &data->lookup[0];
> + int i = 0;
> +
> + for (i = 0; i < data->nlookups; i++, lookup++) {
> + if (!lookup->nvmem_name)
> + lookup->nvmem_name = nvmem_dev_name(nvmem);
> +
> + if (!lookup->dev_id)
> + lookup->dev_id = data->parser->name;
> + }
> +
> + nvmem_add_cell_lookups(data->lookup, data->nlookups);
> + }
> +}
> +

--srini

2021-06-16 13:29:36

by Vadym Kochan

[permalink] [raw]
Subject: Re: [PATCH v2 1/3] nvmem: core: introduce cells parser

Hi Srinivas,

On Mon, Jun 14, 2021 at 11:44:59AM +0100, Srinivas Kandagatla wrote:
>
>
> On 08/06/2021 20:03, Vadym Kochan wrote:
> > Current implementation does not allow to register cells for already
> > registered nvmem device and requires that this should be done before. But
> > there might a driver which needs to parse the nvmem device and after
> > register a cells table.
> >
> > Introduce nvmem_parser API which allows to register cells parser which
> > is called during nvmem device registration. During this stage the parser
> > can read the nvmem device and register the cells table.
> >
> > Signed-off-by: Vadym Kochan <[email protected]>
> > ---
> > v2:
> > 1) Added nvmem_parser_data so parser implementation
> > should fill it with cells table and lookups which
> > will be registered by core.c after cells_parse was
> > succeed.
> >
> > 2) Removed cells_remove callback from nvmem_parser_config which
> > is not needed because cells table & lookups are
> > registered/unregistered automatically by core.
> >
> > 3) Use new device property to read cells parser name during nvmem
> > registration. Removed of_node usage.
> >
> > 4) parser's module refcount is incremented/decremented on each parser
> > bind/unbind to nvmem device.
> >
> > drivers/nvmem/core.c | 178 +++++++++++++++++++++++++++++++++
> > include/linux/nvmem-provider.h | 31 ++++++
> > 2 files changed, 209 insertions(+)
> >
> > diff --git a/drivers/nvmem/core.c b/drivers/nvmem/core.c
> > index bca671ff4e54..648373ced6d4 100644
> > --- a/drivers/nvmem/core.c
> > +++ b/drivers/nvmem/core.c
> > @@ -39,6 +39,7 @@ struct nvmem_device {
> > nvmem_reg_read_t reg_read;
> > nvmem_reg_write_t reg_write;
> > struct gpio_desc *wp_gpio;
> > + struct nvmem_parser_data *parser_data;
>
> This should be renamed to nvmem_cell_info_parser or something on those lines
> to avoid any misunderstanding on what exactly this parser is about.
>
> May be can totally avoid this by using parser name only during register.
>

I added this to keep parsed cells particulary for this nvmem in case
same parser is used for several nvmem's and mostly because of using also
cell lookup info. I will try to also answer your below question why do I need
lookups ?

I use of_get_mac_address() func to fetch mac-address from nvmem cell.
Eventually this func calls of_get_mac_addr_nvmem() which (as I understand it
correctly) can find cells via DT by parsing "nvmem-cell-names" or via cell lookup
info of platform_device. I use the 2nd option with the following sample
solution:

## DT ##
eeprom_at24: [email protected] {
compatible = "atmel,24c32";
nvmem-cell-parser-name = "onie-tlv-cells";
reg = <0x56>;
};

onie_tlv_parser: onie-tlv-cells {
compatible = "nvmem-cell-parser";
status = "okay";

---> add ability here to map cell con_id to cell_name ?

};

some_dev_node {
compatible = "xxx";
base-mac-provider = <&onie_tlv_parser>;
status = "okay";
};
########

== CODE ==
base_mac_np = of_parse_phandle(np, "base-mac-provider", 0);
ret = of_get_mac_address(base_mac_np, base_mac);
==========


And it works with this implementation because onie-tlv-cells is
registered as platform_device which name is the same as parser's name.
So the really tricky part for me is to make this cells lookup work.

Of course would be great if you can point a way/idea to get rid the need of
lookups.

> > void *priv;
> > };
> > @@ -57,6 +58,13 @@ struct nvmem_cell {
> > struct list_head node;
> > };
> > +struct nvmem_parser {
> > + struct list_head head;
> > + nvmem_parse_t cells_parse;
> > + const char *name;
> > + struct module *owner;
> > +};
> > +
> > static DEFINE_MUTEX(nvmem_mutex);
> > static DEFINE_IDA(nvmem_ida);
> > @@ -66,6 +74,9 @@ static LIST_HEAD(nvmem_cell_tables);
> > static DEFINE_MUTEX(nvmem_lookup_mutex);
> > static LIST_HEAD(nvmem_lookup_list);
> > +static DEFINE_MUTEX(nvmem_parser_mutex);
> > +static LIST_HEAD(nvmem_parser_list);
> > +
> > static BLOCKING_NOTIFIER_HEAD(nvmem_notifier);
> > static int __nvmem_reg_read(struct nvmem_device *nvmem, unsigned int offset,
> > @@ -418,6 +429,118 @@ static struct bus_type nvmem_bus_type = {
> > .name = "nvmem",
> > };
> > +static struct nvmem_parser *__nvmem_parser_get(const char *name)
> > +{
> > + struct nvmem_parser *tmp, *parser = NULL;
> > +
> > + list_for_each_entry(tmp, &nvmem_parser_list, head) {
> > + if (strcmp(name, tmp->name) == 0) {
> > + parser = tmp;
> > + break;
> > + }
> > + }
> > +
> > + if (!parser)
> > + return ERR_PTR(-EPROBE_DEFER);
> > +
> > + if (!try_module_get(parser->owner)) {
> > + pr_err("could not increase module refcount for parser %s\n",
> > + parser->name);
> > + return ERR_PTR(-EINVAL);
> > +
> > + }
> > +
> > + return parser;
> > +}
> > +
> > +static void nvmem_parser_put(const struct nvmem_parser *parser)
> > +{
> > + module_put(parser->owner);
> > +}
> > +
> > +static int nvmem_parser_bind(struct nvmem_device *nvmem, const char *name)
> > +{
> Do we really need parser bind/unbind mechanisms for what we are trying to do
> here.
>
> It's just simply parsing cell info during nvmem register, do we really care
> if parser is there or not after that?
>
> code can be probably made much simpler by just doing this in nvmem_register
>
> parser_get()
> parse_get_cell_info()
> parser_put()
>
> AFAIU, that is all we need.
>

because of need of lookup info (just in case if it is needed) the unbind
stage should free the lookups during the nvmem cells removal, but cells table
can be freed even during nvmem_register after cells were added.

> > + struct nvmem_parser_data *data;
> > + struct nvmem_parser *parser;
> > + int err;
> > +
> > + mutex_lock(&nvmem_parser_mutex);
> > +
> > + parser = __nvmem_parser_get(name);
> > + err = PTR_ERR_OR_ZERO(parser);
> > + if (!err) { > + data = kzalloc(sizeof(*data), GFP_KERNEL);
> > + if (data) {
> > + data->parser = parser;
> > + nvmem->parser_data = data;
> > + } else {
> > + nvmem_parser_put(parser);
> > + err = -ENOMEM;
> > + }
> > + }
> > +
> > + mutex_unlock(&nvmem_parser_mutex);
> > +
> > + return err;
> > +}
> > +
> > +static void nvmem_parser_unbind(const struct nvmem_device *nvmem)
> > +{
> > + struct nvmem_parser_data *data = nvmem->parser_data;
> > +
> > + if (data->table.cells) {
> > + nvmem_del_cell_table(&data->table);
> > + kfree(data->table.cells);
> who has allocated memory for this, its confusing for this to be freed in
> core.

Well, looks like I need to call parser->cells_clear() so the parser driver will clear
things by itself.

> > + }
> > + if (data->lookup) { > + nvmem_del_cell_lookups(data->lookup, data->nlookups);
> > + kfree(data->lookup);
> > + }
> > +
> > + nvmem_parser_put(data->parser);
> > +}
> > +
> > +static void nvmem_parser_data_register(struct nvmem_device *nvmem,
> > + struct nvmem_parser_data *data)
> > +{
> > + if (data->table.cells) {
> > + if (!data->table.nvmem_name)
> > + data->table.nvmem_name = nvmem_dev_name(nvmem);
> > +
> > + nvmem_add_cell_table(&data->table);
> > + }
> > +
> > + if (data->lookup) {
> Why do we need lookups?
> the cells are already associated with provider. lookups are normally used
> for associating devices with nvmemcell more for old non-dt machine code.
>
> you can remove this.

I hope I replied this in 1st reply.

> > + struct nvmem_cell_lookup *lookup = &data->lookup[0];
> > + int i = 0;
> > +
> > + for (i = 0; i < data->nlookups; i++, lookup++) {
> > + if (!lookup->nvmem_name)
> > + lookup->nvmem_name = nvmem_dev_name(nvmem);
> > +
> > + if (!lookup->dev_id)
> > + lookup->dev_id = data->parser->name;
> > + }
> > +
> > + nvmem_add_cell_lookups(data->lookup, data->nlookups);
> > + }
> > +}
> > +
>
> --srini

2021-06-21 11:03:49

by Srinivas Kandagatla

[permalink] [raw]
Subject: Re: [PATCH v2 1/3] nvmem: core: introduce cells parser



On 16/06/2021 13:33, Vadym Kochan wrote:
>>> diff --git a/drivers/nvmem/core.c b/drivers/nvmem/core.c
>>> index bca671ff4e54..648373ced6d4 100644
>>> --- a/drivers/nvmem/core.c
>>> +++ b/drivers/nvmem/core.c
>>> @@ -39,6 +39,7 @@ struct nvmem_device {
>>> nvmem_reg_read_t reg_read;
>>> nvmem_reg_write_t reg_write;
>>> struct gpio_desc *wp_gpio;
>>> + struct nvmem_parser_data *parser_data;
>> This should be renamed to nvmem_cell_info_parser or something on those lines
>> to avoid any misunderstanding on what exactly this parser is about.
>>
>> May be can totally avoid this by using parser name only during register.
>>
> I added this to keep parsed cells particulary for this nvmem in case
> same parser is used for several nvmem's and mostly because of using also
> cell lookup info. I will try to also answer your below question why do I need
> lookups ?
>
> I use of_get_mac_address() func to fetch mac-address from nvmem cell.
> Eventually this func calls of_get_mac_addr_nvmem() which (as I understand it
> correctly) can find cells via DT by parsing "nvmem-cell-names" or via cell lookup
> info of platform_device. I use the 2nd option with the following sample
> solution:
>
> ## DT ##
> eeprom_at24: [email protected] {
> compatible = "atmel,24c32";
> nvmem-cell-parser-name = "onie-tlv-cells";
> reg = <0x56>;
> };
>
> onie_tlv_parser: onie-tlv-cells {
> compatible = "nvmem-cell-parser";
> status = "okay";
>
> ---> add ability here to map cell con_id to cell_name ?
>
> };
>
> some_dev_node {
> compatible = "xxx";
> base-mac-provider = <&onie_tlv_parser>;

Real nvmem provider is eeprom_at24, why do you use onie_tlv_parse as
your mac provider?
If you use eeprom_at24 then of_get_mac_address() should get mac-address
directly from cell info.


> status = "okay";
> };
> ########
>
> == CODE ==
> base_mac_np = of_parse_phandle(np, "base-mac-provider", 0);
> ret = of_get_mac_address(base_mac_np, base_mac);
> ==========
>
>
> And it works with this implementation because onie-tlv-cells is
> registered as platform_device which name is the same as parser's name.
> So the really tricky part for me is to make this cells lookup work.

cell lookups are more of intended for board files, adding them in this
case is really not correct. The whole purpose of this driver is to
parse the tlv cell infos into nvmem cell info.


--srini


>
> Of course would be great if you can point a way/idea to get rid the need of
> lookups.
>