Re: Questions about NVMEM

[Srinivas Kandagatla]

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