Re: [PATCH 1/3] nvmem: core: introduce cells parser

[Srinivas Kandagatla <srinivas.kandagatla@xxxxxxxxxx>]

On 15/09/2020 13:41, Vadym Kochan wrote:
> Currently NVMEM core does not allow to register cells for an 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 then
> 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.

Overall this approach looks okay to me!

Rather than a binding for onie cell parser, I think we should add a 
generic bindings for parser something like:


nvmem-cell-parser = "onie-tlv-cells";


I also think the parser matching should be done based on this string 
which can remove
1. new DT node for onie parser.
2. works for both DT and non-DT setups.

nvmem provider register will automatically parse this once it finds a 
matching parser or it will EPROBE_DEFER!

Let me know if you think it works for you!


--srini

> Signed-off-by: Vadym Kochan <vadym.kochan@xxxxxxxxxxx>
> ---
>   drivers/nvmem/core.c           | 89 ++++++++++++++++++++++++++++++++++
>   include/linux/nvmem-provider.h | 27 +++++++++++
>   2 files changed, 116 insertions(+)
>
> diff --git a/drivers/nvmem/core.c b/drivers/nvmem/core.c
> index 6cd3edb2eaf6..82a96032bc3f 100644
> --- a/drivers/nvmem/core.c
> +++ b/drivers/nvmem/core.c
> @@ -55,6 +55,14 @@ struct nvmem_cell {
>   	struct list_head	node;
>   };
>   
> +struct nvmem_parser {
> +	struct device_node	*nvmem_of;
> +	struct kref		refcnt;
> +	struct list_head	head;
> +	nvmem_parse_t		cells_parse;
> +	void *priv;
> +};
> +
>   static DEFINE_MUTEX(nvmem_mutex);
>   static DEFINE_IDA(nvmem_ida);
>   
> @@ -64,6 +72,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,
> @@ -571,6 +582,30 @@ static int nvmem_add_cells_from_of(struct nvmem_device *nvmem)
>   	return 0;
>   }
>   
> +static struct nvmem_parser *
> +__nvmem_parser_find_of(const struct nvmem_device *nvmem)
> +{
> +	struct nvmem_parser *parser;
> +
> +	list_for_each_entry(parser, &nvmem_parser_list, head) {
> +		if (dev_of_node(nvmem->base_dev) == parser->nvmem_of)
> +			return parser;
> +	}
> +
> +	return NULL;
> +}
> +
> +static void nvmem_cells_parse(struct nvmem_device *nvmem)
> +{
> +	struct nvmem_parser *parser;
> +
> +	mutex_lock(&nvmem_parser_mutex);
> +	parser = __nvmem_parser_find_of(nvmem);
> +	if (parser && parser->cells_parse)
> +		parser->cells_parse(parser->priv, nvmem);
> +	mutex_unlock(&nvmem_parser_mutex);
> +}
> +
>   /**
>    * nvmem_register() - Register a nvmem device for given nvmem_config.
>    * Also creates a binary entry in /sys/bus/nvmem/devices/dev-name/nvmem
> @@ -674,6 +709,8 @@ struct nvmem_device *nvmem_register(const struct nvmem_config *config)
>   			goto err_teardown_compat;
>   	}
>   
> +	nvmem_cells_parse(nvmem);
> +
>   	rval = nvmem_add_cells_from_table(nvmem);
>   	if (rval)
>   		goto err_remove_cells;
> @@ -1630,6 +1667,58 @@ 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 device_node *nvmem_of;
> +	struct nvmem_parser *parser;
> +	int err;
> +
> +	if (!config->cells_parse)
> +		return ERR_PTR(-EINVAL);
> +
> +	if (!config->dev)
> +		return ERR_PTR(-EINVAL);
> +
> +	nvmem_of = of_parse_phandle(dev_of_node(config->dev), "nvmem", 0);
> +	if (!nvmem_of)
> +		return ERR_PTR(-EINVAL);
> +
> +	parser = kzalloc(sizeof(*parser), GFP_KERNEL);
> +	if (!parser) {
> +		err = -ENOMEM;
> +		goto err_alloc;
> +	}
> +
> +	parser->cells_parse = config->cells_parse;
> +	/* parser->cells_remove = config->cells_remove; */
> +	parser->nvmem_of = nvmem_of;
> +	parser->priv = config->priv;
> +	kref_init(&parser->refcnt);
> +
> +	mutex_lock(&nvmem_parser_mutex);
> +	list_add(&parser->head, &nvmem_parser_list);
> +	mutex_unlock(&nvmem_parser_mutex);
> +
> +	return parser;
> +
> +err_alloc:
> +	of_node_put(nvmem_of);
> +
> +	return ERR_PTR(err);
> +}
> +EXPORT_SYMBOL(nvmem_parser_register);
> +
> +void nvmem_parser_unregister(struct nvmem_parser *parser)
> +{
> +	mutex_lock(&nvmem_parser_mutex);
> +	of_node_put(parser->nvmem_of);
> +	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 06409a6c40bc..854d0cf5234f 100644
> --- a/include/linux/nvmem-provider.h
> +++ b/include/linux/nvmem-provider.h
> @@ -15,10 +15,13 @@
>   
>   struct nvmem_device;
>   struct nvmem_cell_info;
> +struct nvmem_cell_table;
> +
>   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 void (*nvmem_parse_t)(void *priv, struct nvmem_device *nvmem);
>   
>   enum nvmem_type {
>   	NVMEM_TYPE_UNKNOWN = 0,
> @@ -100,6 +103,12 @@ struct nvmem_cell_table {
>   	struct list_head	node;
>   };
>   
> +struct nvmem_parser_config {
> +	nvmem_parse_t	cells_parse;
> +	void		*priv;
> +	struct device	*dev;
> +};
> +
>   #if IS_ENABLED(CONFIG_NVMEM)
>   
>   struct nvmem_device *nvmem_register(const struct nvmem_config *cfg);
> @@ -110,9 +119,19 @@ struct nvmem_device *devm_nvmem_register(struct device *dev,
>   
>   int devm_nvmem_unregister(struct device *dev, struct nvmem_device *nvmem);
>   
> +int nvmem_cell_parser_register(const char *nvmem_name,
> +			       const struct nvmem_config *cfg);
> +void nvmem_cell_parser_unregister(const char *nvmem_name,
> +				  const struct nvmem_config *cfg);
> +
>   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)
> @@ -137,5 +156,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 */