Re: Questions about NVMEM

From: Srinivas Kandagatla
Date: Mon Sep 11 2017 - 07:14:08 EST


Hi Masahiro,

On 11/09/17 11:33, Masahiro Yamada wrote:

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

I agree, are you planning to send the fixes for these?
or
Am happy to do that if not.





(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 agree, there is some level of redundancy here, we should be able to get rid of it, as you said by removing the read_only flag from config which can be derived from r/w callbacks.


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.
Yep this should work too.





(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?
Not sure if this is some thing mandatory! but its good to have kinda thing.

My take was irrespective of what name the files are, the module names should have a consistent prefix of "nvmem-"

On the other hand there is some inconsistency in some of the module prefix, some of them are being used with prefix "nvmem_" and others with "nvmem-".
we should fix this too.
Its one of my todo thing, which I probably should fix now!!


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.


thanks,
srini

-- Best Regards Masahiro Yamada