Message-ID: <5582BDAE.5040008@linaro.org>
Date:	Thu, 18 Jun 2015 13:46:38 +0100
From:	Srinivas Kandagatla <srinivas.kandagatla@...aro.org>
To:	Stephen Boyd <sboyd@...eaurora.org>,
	linux-arm-kernel@...ts.infradead.org
CC:	Maxime Ripard <maxime.ripard@...e-electrons.com>,
	Rob Herring <robh+dt@...nel.org>,
	Kumar Gala <galak@...eaurora.org>,
	Mark Brown <broonie@...nel.org>, s.hauer@...gutronix.de,
	Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
	linux-api@...r.kernel.org, linux-kernel@...r.kernel.org,
	devicetree@...r.kernel.org, linux-arm-msm@...r.kernel.org,
	arnd@...db.de, pantelis.antoniou@...sulko.com, mporter@...sulko.com
Subject: Re: [PATCH v5 03/11] nvmem: Add a simple NVMEM framework for nvmem
 providers

Many thanks for review.

On 16/06/15 23:43, Stephen Boyd wrote:
> On 05/21/2015 09:43 AM, Srinivas Kandagatla wrote:
>>
>> diff --git a/drivers/nvmem/core.c b/drivers/nvmem/core.c
>> new file mode 100644
>> index 0000000..6c2f0b1
>> --- /dev/null
>> +++ b/drivers/nvmem/core.c
>> @@ -0,0 +1,398 @@
>> +/*
>> + * nvmem framework core.
>> + *
>> + * Copyright (C) 2015 Srinivas Kandagatla <srinivas.kandagatla@...aro.org>
>> + * Copyright (C) 2013 Maxime Ripard <maxime.ripard@...e-electrons.com>
>> + *
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License version 2 and
>> + * only version 2 as published by the Free Software Foundation.
>> + *
>> + * This program is distributed in the hope that it will be useful,
>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>> + * GNU General Public License for more details.
>> + */
>> +
>> +#include <linux/device.h>
>> +#include <linux/nvmem-provider.h>
>> +#include <linux/export.h>
>> +#include <linux/fs.h>
>> +#include <linux/idr.h>
>> +#include <linux/init.h>
>> +#include <linux/regmap.h>
>> +#include <linux/module.h>
>> +#include <linux/of.h>
>> +#include <linux/slab.h>
>> +#include <linux/uaccess.h>
>
> Is this include used?
>
Yep, not required.
>> +
>> +static int of_nvmem_match(struct device *dev, const void *nvmem_np)
>> +{
>> +	return dev->of_node == nvmem_np;
>> +}
>> +
>> +static struct nvmem_device *of_nvmem_find(struct device_node *nvmem_np)
>
> const?
Sure,
>
>> +{
>> +	struct device *d;
>> +
>> +	if (!nvmem_np)
>> +		return NULL;
>> +
>> +	d = class_find_device(&nvmem_class, NULL, nvmem_np, of_nvmem_match);
>> +
>> +	return d ? to_nvmem_device(d) : NULL;
>> +}
>> +
>> +static struct nvmem_cell *nvmem_find_cell(const char *cell_id)
>> +{
>> +	struct nvmem_cell *p;
>> +
>> +	list_for_each_entry(p, &nvmem_cells, node) {
>
> Unnecessary braces.
Yep, Will remove it.
>
>> +		if (p && !strcmp(p->name, cell_id))
>> +			return p;
>> +	}
>> +
>> +	return NULL;
>> +}
>> +
>> +static void nvmem_cell_drop(struct nvmem_cell *cell)
>> +{
>> +	mutex_lock(&nvmem_cells_mutex);
>> +	list_del(&cell->node);
>> +	mutex_unlock(&nvmem_cells_mutex);
>> +	kfree(cell);
>> +}
>> +
>> +static void nvmem_device_remove_all_cells(struct nvmem_device *nvmem)
>> +{
>> +	struct nvmem_cell *cell = NULL;
>
> Unnecessary initialization
Yes.
>
>> +	struct list_head *p, *n;
>> +
>> +	list_for_each_safe(p, n, &nvmem_cells) {
>> +		cell = list_entry(p, struct nvmem_cell, node);
>> +		if (cell->nvmem == nvmem)
>> +			nvmem_cell_drop(cell);
>> +	}
> [..]
>> +
>> +static int nvmem_add_cells(struct nvmem_device *nvmem,
>> +			   struct nvmem_config *cfg)
>> +{
>> +	struct nvmem_cell **cells;
>> +	struct nvmem_cell_info *info = cfg->cells;
>> +	int i, rval;
>> +
>> +	cells = kzalloc(sizeof(*cells) * cfg->ncells, GFP_KERNEL);
>
> kcalloc
This is temporary array, I did this on intention, to make it easy to 
kfree cells which are found invalid at runtime.
>
>> +	if (!cells)
>> +		return -ENOMEM;
>> +
>> +	for (i = 0; i < cfg->ncells; i++) {
>> +		cells[i] = kzalloc(sizeof(struct nvmem_cell), GFP_KERNEL);
>
> sizeof(**cells) ?
Yep.

>
>> +
>> +/**
>> + * nvmem_register(): Register a nvmem device for given nvmem.
>> + * Also creates an binary entry in /sys/class/nvmem/dev-name/nvmem
>> + *
>> + * @nvmem: nvmem device that needs to be created
>
> You mean @config?
>
Yes, I will fix it.

>> + *
>> + * The return value will be an ERR_PTR() on error or a valid pointer
>> +	nvmem->dev.of_node = config->dev->of_node;
>> +	dev_set_name(&nvmem->dev, "%s%d",
>> +		     config->name ? : "nvmem", config->id);
>
> It may be better to always name it nvmem%d so that we don't allow the
> possibility of conflicts.
We can do that, but I wanted to make the sysfs and dev entries more 
readable than just nvmem0, nvmem1...
>
>> +
>> +	nvmem->read_only = of_property_read_bool(nvmem->dev.of_node,
>> +						 "read-only");
>
> What if we're not using DT? How would we specify read_only?
>
Thanks for spotting, you are correct, I need to add read_only flag to 
nvmem_config too.


>> +
>> +	device_initialize(&nvmem->dev);
>> +
>> +	dev_dbg(&nvmem->dev, "Registering nvmem device %s\n",
>> +		dev_name(&nvmem->dev));
>> +
>> +	rval = device_add(&nvmem->dev);
>> +	if (rval) {
>> +		ida_simple_remove(&nvmem_ida, nvmem->id);
>> +		kfree(nvmem);
>> +		return ERR_PTR(rval);
>> +	}
>> +
>> +	/* update sysfs attributes */
>> +	if (nvmem->read_only)
>> +		sysfs_update_group(&nvmem->dev.kobj, &nvmem_bin_ro_group);
>
> It would be nice if this could be done before the device was registered.
> Perhaps have two device_types, one for read-only and one for read/write?

The attributes are actually coming from the class object, so we have no 
choice to update the attributes before the device is registered.

May it would be more safe to have default as read-only and modify it to 
read/write based on read-only flag?


>
>> +
>> + */
>> +int nvmem_unregister(struct nvmem_device *nvmem)
>> +{
>> +	mutex_lock(&nvmem_mutex);
>> +	if (nvmem->users) {
>> +		mutex_unlock(&nvmem_mutex);
>> +		return -EBUSY;
>
> Hmm... that doesn't seem nice. Typically when something is unregistered
> we have to pull the rug out from underneath the users and start
> returning errors to them. The provider needs to be free to unregister
> because it's been forcibly removed. So really this function should
> return void.
>
The consumer api is get/put style, so consumers who already have 
references to the provider, removing provider forcefully might lead to 
dangling pointer. Having said that I can give a try and see how it looks.

>> +	}
>> +	mutex_unlock(&nvmem_mutex);
>> +
>> +	nvmem_device_remove_all_cells(nvmem);
>> +	device_del(&nvmem->dev);
>> +
>> +	return 0;
>> +}
>> +EXPORT_SYMBOL_GPL(nvmem_unregister);
>> +
>> +static int nvmem_init(void)
>
> __init? And __exit on nvmem_exit?
yep, will do that.
>
>> +{
>> +	return class_register(&nvmem_class);
>
> I thought class was on the way out? Aren't we supposed to use bus types
> for new stuff?
Do you remember any conversation on the list about this? I could not 
find it on web.

on the other hand, nvmem is not really a bus, making it a bus type 
sounds incorrect to me.


--srini

>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists