Messages in this thread | | | From | Masahiro Yamada <> | Date | Mon, 11 Sep 2017 19:33:20 +0900 | Subject | Re: 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
| |