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

[Vadym Kochan]

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 <vadym.kochan@xxxxxxxxxxx>
> > ---
> > 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: at24@56 {
		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