On 06/18/2015 05:46 AM, Srinivas Kandagatla wrote:

Many thanks for review.

On 16/06/15 23:43, Stephen Boyd wrote:

quoted

On 05/21/2015 09:43 AM, Srinivas Kandagatla wrote:

quoted

quoted

+
+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.

Ok, but how does that change using kcalloc over kzalloc? I must have
missed something.

quoted

quoted

+ *
+ * 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...

Well sysfs is not really for humans. It's for machines. The nvmem
devices could have a name property so that a more human readable string
is present.

quoted

quoted

+
+    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?

Can you assign the attributes to the device_type in the nvmem::struct
device? I don't see why these attributes need to be part of the class.

quoted

quoted

+{
+    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.

I found this post on the cpu class that Sudeep tried to introduce[1].
And there's this post from Kay that alludes to a unification of busses
and classes[2]. And some other post where Kay says class is dead [3].

[1] https://lkml.org/lkml/2014/8/21/191
[2] https://lwn.net/Articles/471821/
[3] https://lkml.org/lkml/2010/11/11/17

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project