lkml.org 
[lkml]   [2017]   [Sep]   [11]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
Date
SubjectRe: Questions about NVMEM
Hi Srinivas,



2017-09-11 18:38 GMT+09:00 Srinivas Kandagatla <srinivas.kandagatla@linaro.org>:
>
>
> On 11/09/17 05:44, Masahiro Yamada wrote:
>>
>> Hi Srinivas,
>>
>>
>> I have 3 questions about the nvmem sybsystem.
>>
>> Please correct me if something is missing from my thought.
>>
>>
>>
>>
>> (Q1) How to allocate struct nvmem_config?
>>
>> I see 3 ways in allocating struct nvmem_config.
>> What is a good / bad practice?
>>
>>
>> (A) Allocate statically in .data section
>>
>> bcm-ocotp.c
>> imx-ocotp.c
>> lpc18xx_eeprom.c
>> lpc18xx_otp.c
>> mxs-octop.c
>> qfprom.c
>> rockchip-efuse.c
>> sunxi_sid
>> vf610-ocotp.c
>> meson-efuse.c
>>
>> (B) devm_kzalloc()
>>
>> imx-iim.c
>> mtk-efuse.c
>> drivers/misc/eeprom/at24.c
>>
>> (C) Stack
>>
>> drivers/thunderbolt/switch.c
>>
>>
>>
>> I think (A) is safe only when we know the system has
>> just one instance of the device.
>> (A) should not be used if two or more instances exist.
>> Is this correct?
>
>
> That is correct.
>
>>
>>
>> I think (B) is wasting memory because nvmem_register()
>> copies all members of nvmem_config to nvmem_device.
>> nvmem_config is never dereferenced after nvmem_register() finished.
>> I do not see much sense to keep it until the driver is detached.
>>
> I agree.
>
>>
>>
>> (C) looks reasonable because nvmem_config is pretty small.
>> (sizeof(struct nvmem_config) = 104 byte on 64bit systems)
>>
> Yep, thats much better indeed!

OK.
I think (B) should be fixed as soon as possible
because new drivers often copy existing drivers.



>>
>>
>>
>> (Q2) Is nvmem_config::read_only necessary?
>>
>> If .reg_write() callback is set, it is probably writable.
>> If .reg_write() is missing, it must be read-only.
>>
>> I have no idea when nvmem_config::read_only is useful...
>
>
> You can mark particular instance of provider as read-only which could be
> specific to board.
>
> reg_write callbacks can be implemented by provider driver, but read-only
> flag would give the flexibility at board level.


Hmm, I did not get it.
Please help me to be clearer.


For each instance, the driver passes a different
nvmem_config to nvmem_register().

The driver should be able to do
config.reg_write = NULL;
to specify this instance is read-only.


Do we really need to specify both?
config.reg_write = NULL;
config.read_only = true;


I know nvmem_register() understands DT property "read-only".
This DT property is definitely useful for the
board-level and/or instance-granule flexibility.


But, I do not find a good example where
nvmem_config.read_only provides additional value.


For example, drivers/misc/eeprom/eeprom_93xx46.c
conditionally sets the read_only flag, like this:

edev->nvmem_config.read_only = pd->flags & EE_READONLY;



If nvmem had not supported .read_only flag,
the driver would probably have done like this:

if (!(pd->flags & EE_READONLY))
edev->nvmem_config.reg_write = eeprom_93xx46_write;


This should be fine.



>>
>>
>>
>> (Q3) The style of drivers/nvmem/Makefile
>>
>> This Makefile looks ugly to me.
>> All nvmem drivers are just single file modules.
>> Why are they renamed when modules are created?
>>
>> For the name-space reason for modules,
>> prefix "nvmem-" makes sense to me.
>>
>> It is true that adding "nvmem-" prefix is redundant while
>> they are located in drivers/nvmem/ directory,
>> but renaming in the Makefile is even more annoying to me.
>> Having said that, we may not want to churn this.
>
> This is mainly done for consistent module naming.
> I prefer to have nvmem- prefix for nvmem modules.
>

I 100% agree that all nvmem modules should have "nvmem-" prefix
consistently.

My question was, why .c files do not have the same file name as
the module name?

The more straight-forward way would be:
drivers/nvmem/nvmem_core.c
drivers/nvmem/nvmem-bcm-ocotp.c
drivers/nvmem/nvmem-imx-iim.c
etc.


--
Best Regards
Masahiro Yamada

\
 
 \ /
  Last update: 2017-09-11 12:35    [W:0.076 / U:0.340 seconds]
©2003-2017 Jasper Spaans. hosted at Digital OceanAdvertise on this site