Re: [PATCH v5 03/11] nvmem: Add a simple NVMEM framework for nvmem providers

[Stephen Boyd <sboyd-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org>]

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