[PATCH 0/8] nvmem: Various small fixes and improvements

[PATCH 0/8] nvmem: Various small fixes and improvements

[Alban Bedel]

Hi,

this series is mostly small bug fixes, but also add a new API
to make things simpler in drivers that need to request an optional cell.

Alban Bedel (8):
  nvmem: core: Set the provider read-only when no write callback is
    given
  nvmem: core: Fix of_nvmem_cell_get() for optional cells
  nvmem: Add nvmem_cell_get_optional and devm_nvmem_cell_get_optional
  nvmem: core: Fix cell lookup when no cell is found
  nvmem: core: Properly handle connection ID in of_nvmem_device_get()
  nvmem: core: Always reference the device returned by
    nvmem_device_get()
  nvmem: core: Fix device reference leak
  nvmem: core: Avoid useless iterations in nvmem_cell_get_from_lookup()

 drivers/nvmem/core.c           | 86 +++++++++++++++++++++++++++-------
 include/linux/nvmem-consumer.h | 16 +++++++
 2 files changed, 86 insertions(+), 16 deletions(-)

-- 
2.19.1

[PATCH 1/8] nvmem: core: Set the provider read-only when no write callback is given

[Alban Bedel]

If no write callback is given the device should be marked as read-only.
While at it also move from a bit or to a logical or as that is a logical
expression.

Signed-off-by: Alban Bedel <albeu@free.fr>
---
 drivers/nvmem/core.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/nvmem/core.c b/drivers/nvmem/core.c
index f7301bb4ef3b..cf2e1091fe89 100644
--- a/drivers/nvmem/core.c
+++ b/drivers/nvmem/core.c
@@ -646,8 +646,8 @@ struct nvmem_device *nvmem_register(const struct nvmem_config *config)
 			     config->name ? config->id : nvmem->id);
 	}
 
-	nvmem->read_only = device_property_present(config->dev, "read-only") |
-			   config->read_only;
+	nvmem->read_only = device_property_present(config->dev, "read-only") ||
+			   config->read_only || !nvmem->reg_write;
 
 	if (config->root_only)
 		nvmem->dev.groups = nvmem->read_only ?
-- 
2.19.1

[PATCH 2/8] nvmem: core: Fix of_nvmem_cell_get() for optional cells

[Alban Bedel]

of_nvmem_cell_get() should return -ENOENT when a cell isn't defined,
otherwise callers can't distinguish between a missing cell and other
errors.

Signed-off-by: Alban Bedel <albeu@free.fr>
---
 drivers/nvmem/core.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/nvmem/core.c b/drivers/nvmem/core.c
index cf2e1091fe89..f8c43da6f2ca 100644
--- a/drivers/nvmem/core.c
+++ b/drivers/nvmem/core.c
@@ -1031,7 +1031,7 @@ struct nvmem_cell *of_nvmem_cell_get(struct device_node *np, const char *id)
 
 	cell_np = of_parse_phandle(np, "nvmem-cells", index);
 	if (!cell_np)
-		return ERR_PTR(-EINVAL);
+		return ERR_PTR(-ENOENT);
 
 	nvmem_np = of_get_next_parent(cell_np);
 	if (!nvmem_np)
-- 
2.19.1

[PATCH 3/8] nvmem: Add nvmem_cell_get_optional and devm_nvmem_cell_get_optional

[Alban Bedel]

Add helper functions to make the driver code simpler when a cell is
optional. Using these functions just return NULL when the cell doesn't
exists or if nvmem is disabled.

Signed-off-by: Alban Bedel <albeu@free.fr>
---
 drivers/nvmem/core.c           | 48 ++++++++++++++++++++++++++++++++++
 include/linux/nvmem-consumer.h | 16 ++++++++++++
 2 files changed, 64 insertions(+)

diff --git a/drivers/nvmem/core.c b/drivers/nvmem/core.c
index f8c43da6f2ca..8e1b52559467 100644
--- a/drivers/nvmem/core.c
+++ b/drivers/nvmem/core.c
@@ -1083,6 +1083,30 @@ struct nvmem_cell *nvmem_cell_get(struct device *dev, const char *id)
 }
 EXPORT_SYMBOL_GPL(nvmem_cell_get);
 
+/**
+ * nvmem_cell_get_optional() - Get an optional nvmem cell of device from
+ * a given id.
+ *
+ * @dev: Device that requests the nvmem cell.
+ * @cell_id: nvmem cell name to get.
+ *
+ * Return: Will be NULL if no cell with the given name is defined,
+ * an ERR_PTR() on error or a valid pointer to a struct nvmem_cell.
+ * The nvmem_cell will be freed by the nvmem_cell_put().
+ */
+struct nvmem_cell *nvmem_cell_get_optional(struct device *dev,
+					   const char *cell_id)
+{
+	struct nvmem_cell *cell;
+
+	cell = nvmem_cell_get(dev, cell_id);
+	if (IS_ERR(cell) && PTR_ERR(cell) == -ENOENT)
+		return NULL;
+
+	return cell;
+}
+EXPORT_SYMBOL_GPL(nvmem_cell_get_optional);
+
 static void devm_nvmem_cell_release(struct device *dev, void *res)
 {
 	nvmem_cell_put(*(struct nvmem_cell **)res);
@@ -1118,6 +1142,30 @@ struct nvmem_cell *devm_nvmem_cell_get(struct device *dev, const char *id)
 }
 EXPORT_SYMBOL_GPL(devm_nvmem_cell_get);
 
+/**
+ * devm_nvmem_cell_get() - Get an optional nvmem cell of device from
+ * a given id.
+ *
+ * @dev: Device that requests the nvmem cell.
+ * @id: nvmem cell name id to get.
+ *
+ * Return: Will be NULL if the cell doesn't exists, an ERR_PTR() on
+ * error or a valid pointer to a struct nvmem_cell.  The nvmem_cell
+ * will be freed by the automatically once the device is freed.
+ */
+struct nvmem_cell *devm_nvmem_cell_get_optional(struct device *dev,
+						const char *cell_id)
+{
+	struct nvmem_cell *cell;
+
+	cell = devm_nvmem_cell_get(dev, cell_id);
+	if (IS_ERR(cell) && PTR_ERR(cell) == -ENOENT)
+		return NULL;
+
+	return cell;
+}
+EXPORT_SYMBOL_GPL(devm_nvmem_cell_get_optional);
+
 static int devm_nvmem_cell_match(struct device *dev, void *res, void *data)
 {
 	struct nvmem_cell **c = res;
diff --git a/include/linux/nvmem-consumer.h b/include/linux/nvmem-consumer.h
index 312bfa5efd80..8d7bf21a9adc 100644
--- a/include/linux/nvmem-consumer.h
+++ b/include/linux/nvmem-consumer.h
@@ -56,7 +56,11 @@ enum {
 
 /* Cell based interface */
 struct nvmem_cell *nvmem_cell_get(struct device *dev, const char *id);
+struct nvmem_cell *nvmem_cell_get_optional(struct device *dev,
+					   const char *id);
 struct nvmem_cell *devm_nvmem_cell_get(struct device *dev, const char *id);
+struct nvmem_cell *devm_nvmem_cell_get_optional(struct device *dev,
+						const char *id);
 void nvmem_cell_put(struct nvmem_cell *cell);
 void devm_nvmem_cell_put(struct device *dev, struct nvmem_cell *cell);
 void *nvmem_cell_read(struct nvmem_cell *cell, size_t *len);
@@ -96,12 +100,24 @@ static inline struct nvmem_cell *nvmem_cell_get(struct device *dev,
 	return ERR_PTR(-EOPNOTSUPP);
 }
 
+static inline struct nvmem_cell *nvmem_cell_get_optional(struct device *dev,
+							 const char *id)
+{
+	return NULL;
+}
+
 static inline struct nvmem_cell *devm_nvmem_cell_get(struct device *dev,
 						     const char *id)
 {
 	return ERR_PTR(-EOPNOTSUPP);
 }
 
+static inline struct nvmem_cell *
+devm_nvmem_cell_get_optional(struct device *dev, const char *id)
+{
+	return NULL;
+}
+
 static inline void devm_nvmem_cell_put(struct device *dev,
 				       struct nvmem_cell *cell)
 {
-- 
2.19.1

[PATCH 4/8] nvmem: core: Fix cell lookup when no cell is found

[Alban Bedel]

If the cell list is not empty and nvmem_find_cell_by_node/name() is
called for a cell that is not present in the list they will return an
invalid pointer instead of NULL. This happen because
list_for_each_entry() stop once it reach the list head again, but as
the list head is not contained in a struct nvmem_cell the iteration
variable then contains an invalid value.

This is easily solved by using a variable to iterate over the list and
one to return the cell found.

Signed-off-by: Alban Bedel <albeu@free.fr>
---
 drivers/nvmem/core.c | 16 ++++++++++------
 1 file changed, 10 insertions(+), 6 deletions(-)

diff --git a/drivers/nvmem/core.c b/drivers/nvmem/core.c
index 8e1b52559467..a7556b20cff4 100644
--- a/drivers/nvmem/core.c
+++ b/drivers/nvmem/core.c
@@ -525,12 +525,14 @@ static int nvmem_add_cells_from_table(struct nvmem_device *nvmem)
 static struct nvmem_cell *
 nvmem_find_cell_by_name(struct nvmem_device *nvmem, const char *cell_id)
 {
-	struct nvmem_cell *cell = NULL;
+	struct nvmem_cell *iter, *cell = NULL;
 
 	mutex_lock(&nvmem_mutex);
-	list_for_each_entry(cell, &nvmem->cells, node) {
-		if (strcmp(cell_id, cell->name) == 0)
+	list_for_each_entry(iter, &nvmem->cells, node) {
+		if (strcmp(cell_id, iter->name) == 0) {
+			cell = iter;
 			break;
+		}
 	}
 	mutex_unlock(&nvmem_mutex);
 
@@ -994,12 +996,14 @@ nvmem_cell_get_from_lookup(struct device *dev, const char *con_id)
 static struct nvmem_cell *
 nvmem_find_cell_by_node(struct nvmem_device *nvmem, struct device_node *np)
 {
-	struct nvmem_cell *cell = NULL;
+	struct nvmem_cell *iter, *cell = NULL;
 
 	mutex_lock(&nvmem_mutex);
-	list_for_each_entry(cell, &nvmem->cells, node) {
-		if (np == cell->np)
+	list_for_each_entry(iter, &nvmem->cells, node) {
+		if (np == iter->np) {
+			cell = iter;
 			break;
+		}
 	}
 	mutex_unlock(&nvmem_mutex);
 
-- 
2.19.1

[PATCH 5/8] nvmem: core: Properly handle connection ID in of_nvmem_device_get()

[Alban Bedel]

of_nvmem_device_get() would crash if NULL was passed as a connection
ID. Rework this to use the usual sementic of assuming the first
connection when no connection ID is given.

Furthermore of_nvmem_device_get() would return -EINVAL when it failed
to resolve the connection, making it impossible to properly implement
an optional connection. Return -ENOENT instead to let the caller know
that the connection doesn't exists.

Signed-off-by: Alban Bedel <albeu@free.fr>
---
 drivers/nvmem/core.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/drivers/nvmem/core.c b/drivers/nvmem/core.c
index a7556b20cff4..28e01a9876c6 100644
--- a/drivers/nvmem/core.c
+++ b/drivers/nvmem/core.c
@@ -839,13 +839,14 @@ struct nvmem_device *of_nvmem_device_get(struct device_node *np, const char *id)
 {
 
 	struct device_node *nvmem_np;
-	int index;
+	int index = 0;
 
-	index = of_property_match_string(np, "nvmem-names", id);
+	if (id)
+		index = of_property_match_string(np, "nvmem-names", id);
 
 	nvmem_np = of_parse_phandle(np, "nvmem", index);
 	if (!nvmem_np)
-		return ERR_PTR(-EINVAL);
+		return ERR_PTR(-ENOENT);
 
 	return __nvmem_device_get(nvmem_np, NULL);
 }
-- 
2.19.1

[PATCH 6/8] nvmem: core: Always reference the device returned by nvmem_device_get()

[Alban Bedel]

In nvmem_device_get(), when the device lookup fails with DT it
currently fallback on nvmem_find() which is wrong for two reasons.
First nvmem_find() return NULL when nothing is found instead of an
ERR_PTR. But nvmem_find() also just lookup the device, it doesn't
reference the module and increment the reference count like it is done
in the DT path.

To fix this we replace the call to nvmem_find() with a call to
__nvmem_device_get() which does all the referencing and return a
proper ERR_PTR in case of error.

Signed-off-by: Alban Bedel <albeu@free.fr>
---
 drivers/nvmem/core.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/nvmem/core.c b/drivers/nvmem/core.c
index 28e01a9876c6..2fa97b373601 100644
--- a/drivers/nvmem/core.c
+++ b/drivers/nvmem/core.c
@@ -874,7 +874,7 @@ struct nvmem_device *nvmem_device_get(struct device *dev, const char *dev_name)
 
 	}
 
-	return nvmem_find(dev_name);
+	return __nvmem_device_get(NULL, dev_name);
 }
 EXPORT_SYMBOL_GPL(nvmem_device_get);
 
-- 
2.19.1

[PATCH 7/8] nvmem: core: Fix device reference leak

[Alban Bedel]

__nvmem_device_get() make use of bus_find_device() to get the relevant
device and this function increase the reference count of the device
found, however this is not accounted for anywhere. Fix
__nvmem_device_get() and __nvmem_device_put() to properly release this
reference count.

Signed-off-by: Alban Bedel <albeu@free.fr>
---
 drivers/nvmem/core.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/nvmem/core.c b/drivers/nvmem/core.c
index 2fa97b373601..176fe72f4eb5 100644
--- a/drivers/nvmem/core.c
+++ b/drivers/nvmem/core.c
@@ -811,6 +811,7 @@ static struct nvmem_device *__nvmem_device_get(struct device_node *np,
 			"could not increase module refcount for cell %s\n",
 			nvmem_dev_name(nvmem));
 
+		put_device(&nvmem->dev);
 		return ERR_PTR(-EINVAL);
 	}
 
@@ -821,6 +822,7 @@ static struct nvmem_device *__nvmem_device_get(struct device_node *np,
 
 static void __nvmem_device_put(struct nvmem_device *nvmem)
 {
+	put_device(&nvmem->dev);
 	module_put(nvmem->owner);
 	kref_put(&nvmem->refcnt, nvmem_device_release);
 }
-- 
2.19.1

[PATCH 8/8] nvmem: core: Avoid useless iterations in nvmem_cell_get_from_lookup()

[Alban Bedel]

Once the correct cell has been found there is no need to continue
iterating, just stop there. While at it replace the goto used to leave
the loop with simple break statements.

Signed-off-by: Alban Bedel <albeu@free.fr>
---
 drivers/nvmem/core.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/nvmem/core.c b/drivers/nvmem/core.c
index 176fe72f4eb5..9334f074defb 100644
--- a/drivers/nvmem/core.c
+++ b/drivers/nvmem/core.c
@@ -977,7 +977,7 @@ nvmem_cell_get_from_lookup(struct device *dev, const char *con_id)
 			if (IS_ERR(nvmem)) {
 				/* Provider may not be registered yet. */
 				cell = ERR_CAST(nvmem);
-				goto out;
+				break;
 			}
 
 			cell = nvmem_find_cell_by_name(nvmem,
@@ -985,12 +985,11 @@ nvmem_cell_get_from_lookup(struct device *dev, const char *con_id)
 			if (!cell) {
 				__nvmem_device_put(nvmem);
 				cell = ERR_PTR(-ENOENT);
-				goto out;
 			}
+			break;
 		}
 	}
 
-out:
 	mutex_unlock(&nvmem_lookup_mutex);
 	return cell;
 }
-- 
2.19.1

Re: [PATCH 0/8] nvmem: Various small fixes and improvements

[Srinivas Kandagatla]

On 06/01/2019 19:28, Alban Bedel wrote:
> Hi,
> 
> this series is mostly small bug fixes, but also add a new API
> to make things simpler in drivers that need to request an optional cell.
> 
> Alban Bedel (8):
>    nvmem: core: Set the provider read-only when no write callback is
>      given
>    nvmem: core: Fix of_nvmem_cell_get() for optional cells
>    nvmem: Add nvmem_cell_get_optional and devm_nvmem_cell_get_optional
>    nvmem: core: Fix cell lookup when no cell is found
>    nvmem: core: Properly handle connection ID in of_nvmem_device_get()
>    nvmem: core: Always reference the device returned by
>      nvmem_device_get()
>    nvmem: core: Fix device reference leak
>    nvmem: core: Avoid useless iterations in nvmem_cell_get_from_lookup()
> 
>   drivers/nvmem/core.c           | 86 +++++++++++++++++++++++++++-------
>   include/linux/nvmem-consumer.h | 16 +++++++
>   2 files changed, 86 insertions(+), 16 deletions(-)
> 

Thanks Alban for the work!
Most of the patches look good to me except nvmem_cell_get_optional()!

Will send out comments on that patch separately!

-srini

Re: [PATCH 3/8] nvmem: Add nvmem_cell_get_optional and devm_nvmem_cell_get_optional

[Srinivas Kandagatla]

On 06/01/2019 19:28, Alban Bedel wrote:
> Add helper functions to make the driver code simpler when a cell is
> optional. Using these functions just return NULL when the cell doesn't
> exists or if nvmem is disabled.
> 
> Signed-off-by: Alban Bedel<albeu@free.fr>
> ---
>   drivers/nvmem/core.c           | 48 ++++++++++++++++++++++++++++++++++
>   include/linux/nvmem-consumer.h | 16 ++++++++++++
>   2 files changed, 64 insertions(+)
> 
> diff --git a/drivers/nvmem/core.c b/drivers/nvmem/core.c
> index f8c43da6f2ca..8e1b52559467 100644
> --- a/drivers/nvmem/core.c
> +++ b/drivers/nvmem/core.c
> @@ -1083,6 +1083,30 @@ struct nvmem_cell *nvmem_cell_get(struct device *dev, const char *id)
>   }
>   EXPORT_SYMBOL_GPL(nvmem_cell_get);
>   
> +/**
> + * nvmem_cell_get_optional() - Get an optional nvmem cell of device from
> + * a given id.
> + *
> + * @dev: Device that requests the nvmem cell.
> + * @cell_id: nvmem cell name to get.
> + *
> + * Return: Will be NULL if no cell with the given name is defined,
> + * an ERR_PTR() on error or a valid pointer to a struct nvmem_cell.
> + * The nvmem_cell will be freed by the nvmem_cell_put().
> + */
> +struct nvmem_cell *nvmem_cell_get_optional(struct device *dev,
> +					   const char *cell_id)
> +{
> +	struct nvmem_cell *cell;
> +
> +	cell = nvmem_cell_get(dev, cell_id);
> +	if (IS_ERR(cell) && PTR_ERR(cell) == -ENOENT)
> +		return NULL;

What is the real use-case here, it does not make sense to me to add this 
additional call just to return NULL when cell is not found!


--srini
> +
> +	return cell;
> +}

Re: [PATCH 3/8] nvmem: Add nvmem_cell_get_optional and devm_nvmem_cell_get_optional

[Alban]

[-- Attachment #1: Type: text/plain, Size: 2345 bytes --]

On Tue, 15 Jan 2019 12:40:53 +0000
Srinivas Kandagatla <srinivas.kandagatla@linaro.org> wrote:

> On 06/01/2019 19:28, Alban Bedel wrote:
> > Add helper functions to make the driver code simpler when a cell is
> > optional. Using these functions just return NULL when the cell doesn't
> > exists or if nvmem is disabled.
> > 
> > Signed-off-by: Alban Bedel<albeu@free.fr>
> > ---
> >   drivers/nvmem/core.c           | 48 ++++++++++++++++++++++++++++++++++
> >   include/linux/nvmem-consumer.h | 16 ++++++++++++
> >   2 files changed, 64 insertions(+)
> > 
> > diff --git a/drivers/nvmem/core.c b/drivers/nvmem/core.c
> > index f8c43da6f2ca..8e1b52559467 100644
> > --- a/drivers/nvmem/core.c
> > +++ b/drivers/nvmem/core.c
> > @@ -1083,6 +1083,30 @@ struct nvmem_cell *nvmem_cell_get(struct device *dev, const char *id)
> >   }
> >   EXPORT_SYMBOL_GPL(nvmem_cell_get);
> >   
> > +/**
> > + * nvmem_cell_get_optional() - Get an optional nvmem cell of device from
> > + * a given id.
> > + *
> > + * @dev: Device that requests the nvmem cell.
> > + * @cell_id: nvmem cell name to get.
> > + *
> > + * Return: Will be NULL if no cell with the given name is defined,
> > + * an ERR_PTR() on error or a valid pointer to a struct nvmem_cell.
> > + * The nvmem_cell will be freed by the nvmem_cell_put().
> > + */
> > +struct nvmem_cell *nvmem_cell_get_optional(struct device *dev,
> > +					   const char *cell_id)
> > +{
> > +	struct nvmem_cell *cell;
> > +
> > +	cell = nvmem_cell_get(dev, cell_id);
> > +	if (IS_ERR(cell) && PTR_ERR(cell) == -ENOENT)
> > +		return NULL;  
> 
> What is the real use-case here, it does not make sense to me to add this 
> additional call just to return NULL when cell is not found!

It also return NULL when nvmem is not compiled in. I quiet like such
convenience functions as they make the driver code much simpler and
the intent explicit. It replace:

	data->cell = devm_nvmem_cell_get(dev, "my-cell");
	if (IS_ERR(data->cell) {
		if (PTR_ERR(data->cell) == -ENOENT ||
		    PTR_ERR(data->cell) == -EOPNOTSUPP)
			data->cell = NULL;
		else
			return PTR_ERR(data->cell);
	}

with:

	data->cell = dev_nvmem_cell_get_optional(dev, "my-cell");
	if (IS_ERR(cell))
		return PTR_ERR(data->cell);

It's your call if you find that useful or not.

Alban

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 3/8] nvmem: Add nvmem_cell_get_optional and devm_nvmem_cell_get_optional

[Srinivas Kandagatla]

On 16/01/2019 18:26, Alban wrote:
>> What is the real use-case here, it does not make sense to me to add this
>> additional call just to return NULL when cell is not found!
> It also return NULL when nvmem is not compiled in. I quiet like such
> convenience functions as they make the driver code much simpler and
> the intent explicit. It replace:
> 
> 	data->cell = devm_nvmem_cell_get(dev, "my-cell");
> 	if (IS_ERR(data->cell) {
> 		if (PTR_ERR(data->cell) == -ENOENT ||
> 		    PTR_ERR(data->cell) == -EOPNOTSUPP)
> 			data->cell = NULL;
> 		else
> 			return PTR_ERR(data->cell);
> 	}
> 
> with:
> 
> 	data->cell = dev_nvmem_cell_get_optional(dev, "my-cell");
> 	if (IS_ERR(cell))
> 		return PTR_ERR(data->cell);
> 
> It's your call if you find that useful or not.
I don't think this should belong to nvmem core in anyway! Its more of 
consumer specific logic!

I have already applied all of the patches in this series except this one!

thanks,
srini