On 06/18/2015 05:46 AM, Srinivas Kandagatla wrote:
> Many thanks for review.
>
> On 16/06/15 23:43, Stephen Boyd wrote:
>> On 05/21/2015 09:43 AM, Srinivas Kandagatla wrote:
>>>
>>>> +
>>>> +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.
>
>
>>> + *
>>> + * 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.
>
>>> +
>>> + 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.
>>
>>> +{
>>> + 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
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
