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

[Vadym Kochan]

On Tue, Sep 22, 2020 at 10:48:27AM +0100, Srinivas Kandagatla wrote:
> 
> 
> 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

Indeed, it sounds very good and fixes the early dependency that
parser should be registered first before nvmem provider.

I will try!

> 
> > 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 */
> >