Questions about NVMEM

Questions about NVMEM

[Masahiro Yamada]

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?


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.



(C) looks reasonable because nvmem_config is pretty small.
(sizeof(struct nvmem_config) = 104 byte on 64bit systems)

Several subsystems receive configuration data from stack,
for example,

   "struct clk_init_data" in clk drivers,
   "struct uart_8250_port" in 8250 serial drivers.

sizeof(struct uart_8250_port) = 528 byte,
but it is still working in stack.





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





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





-- 
Best Regards
Masahiro Yamada

Re: Questions about NVMEM

[Srinivas Kandagatla]

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!


> Several subsystems receive configuration data from stack,
> for example,
> 
>     "struct clk_init_data" in clk drivers,
>     "struct uart_8250_port" in 8250 serial drivers.
> 
> sizeof(struct uart_8250_port) = 528 byte,
> but it is still working in stack.
> 
> 
> 
> 
> 
> (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.

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

thanks,
srini
> 
> 
> 
> 
> 

Re: Questions about NVMEM

[Masahiro Yamada]

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

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

Re: Questions about NVMEM

[Masahiro Yamada]

Hi Srinivas,


2017-09-11 20:13 GMT+09:00 Srinivas Kandagatla <srinivas.kandagatla@linaro.org>:
> 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.


I will post patches soon with some more fixes/cleanups.

Please check if you like it or not.



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


I was not saying this is mandatory.
I was just curious.

Then Greg answered - Looks like this is the style
at least some subsystems are going towards.

Oh well, this Makefile is unreadable (at least to me).



> 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!!

Changing module names might have certain degree of impacts,
but I do not have a strong opinion about this.

If you care "nvmem-" or "nvmem_",
please tell us your preference.
Socionext is trying to upstream a new driver.






-- 
Best Regards
Masahiro Yamada

Re: Questions about NVMEM

[Srinivas Kandagatla]

On 11/09/17 13:40, Masahiro Yamada wrote:
> If you care "nvmem-" or "nvmem_",
> please tell us your preference.
Would prefer "nvmem-"

thanks,
srini

> Socionext is trying to upstream a new driver.

Re: Questions about NVMEM

[Greg Kroah-Hartman]

On Mon, Sep 11, 2017 at 07:33:20PM +0900, Masahiro Yamada wrote:
> >> (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.

No, the way the current code is, is just fine, please leave it alone, it
is the style that other subsystems are moving to as well.

thanks,

greg k-h

Re: Questions about NVMEM

[Masahiro Yamada]

Hi Greg


2017-09-11 20:24 GMT+09:00 Greg Kroah-Hartman <gregkh@linuxfoundation.org>:
> On Mon, Sep 11, 2017 at 07:33:20PM +0900, Masahiro Yamada wrote:
>> >> (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.
>
> No, the way the current code is, is just fine, please leave it alone, it
> is the style that other subsystems are moving to as well.
>

I did not know that.

OK, if this is the preferred style, let's keep it.



-- 
Best Regards
Masahiro Yamada