Re: [PATCH V1 1/4] bindings: nvmem: introduce "reverse-data" property

[Srinivas Kandagatla <srinivas.kandagatla@linaro.org>]

On 08/09/2021 08:14, Joakim Zhang wrote:
> 
> Hi Srinivas,
> 
> [...]
>> I have pushed some nvmem core patches which are just compile tested to
>> https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgit.kern
>> el.org%2Fpub%2Fscm%2Flinux%2Fkernel%2Fgit%2Fsrini%2Fnvmem.git%2Flog%
>> 2F%3Fh%3Dtopic%2Fpost-processing&data=04%7C01%7Cqiangqing.zhan
>> g%40nxp.com%7Cadfa3ba63c634937876308d971e7e71f%7C686ea1d3bc2b4c6
>> fa92cd99c5c301635%7C0%7C0%7C637666063097239185%7CUnknown%7CT
>> WFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJ
>> XVCI6Mn0%3D%7C1000&sdata=W9yAnGm9rYzlSZuAAGiN4VHUtKYUTt9S
>> oyGQ9QsY7fI%3D&reserved=0
>>
>> This should provide the callback hook I was talking about.
> 
> Thanks a lot! Yes, this could be more common, vendors can parse their mac
> address for different encoding style, also can extend for other cases.

Yes, that is the idea,
> 
>> Can you take a look at them and let me know if it works for you.
> 
> There are some small issues need to be update:
> 1) https://git.kernel.org/pub/scm/linux/kernel/git/srini/nvmem.git/commit/?h=topic/post-processing&id=624f2cc99b48bbfe05c11e58fb73f84abb1a646e
> 	of_get_property() can't get the cell value, so I change to of_property_read_s32()
> 2) https://git.kernel.org/pub/scm/linux/kernel/git/srini/nvmem.git/commit/?h=topic/post-processing&id=a424302c7b15da41e1e8de56b0c78021b9a96c1e
> 	if (!nvmem->cell_post_process) {} should be if (nvmem->cell_post_process) {}, if we have this callback, we need do the post-processing.
> 
I have pushed these changes now to the branch.

>> I have also added some test changes to imx provider driver as well, which you
>> might have to take a closer look to get it working.
>>
>> You need to look at adding/changing two things:
>>
>> 1. setting reverse_mac_address flag in imx driver.
>> 	Does IMX always has mac-address reversed? if yes then we do not need
>> any new bindings for imx nvmem provider, if no we might need to add some
>> kind of flag to indicate this.
> 
> No, it's depend on how to program the effuse.
> To avoid introducing consumer property in devicetree, I prefer to move reverse_mac_address
> flag into ocotp_params struct, since each platforms has their own, it's easy to indicate this. I tried
> it, and works. >

As long as provider can figure out how the efuse is programmed then it 
is fine with me.


>> 2. In imx devicetree for mac-address nvmem cell make sure you add
>>
>> cell-type = <NVMEM_CELL_TYPE_MAC_ADDRESS>;
>>
>>
>>
>>
>>>
>>>> Option 2: nvmem core handles the post processing.
>>>>
>>>> Pros:
>>>> - provider driver does not need to implement callbacks
>>>>
>>>> Cons:
>>>> - We have to find a way to define vendor specific non-standard
>>>> encoding information in generic bindings which is going to be a
>>>> challenge and high chance of ending up in to much of clutter in generic
>> bindings.
>>>>
>>>> Finally, The way I look at this is that once we start adding
>>>> post-processing in nvmem core then we might endup with code that will
>>>> not be really used for most of the usecases and might endup with
>>>> cases that might not be possible to handle in the core.
>>>>
>>>>
>>>> Does Option 1 work for you?
>>>
>>> Yes, I also prefer to implement it in specific driver, as you mention
>>> above, these code are for very rarely use cases.
>>>
>>> If we chose Option 1, I want to implement it totally in specific
>>> driver(imx-ocotp.c), and I have a draft, could it be acdeptable?
>> Yes, this is the direction, however we need a proper callback to do this. And
>> offset information is still comes from Device tree.
>>
>>
>> Have a look at the patches pushed into topic/post-processing branch.
> 
> I have improved this patch set according above comments and tested it. Also rebase to
> the nvmem/for-next branch.
> 
> I plan to keep you as the nvmem part author and send out this patch set with dts changes. If it's fine for you?

Yes please, can you pick the new patches from the branch before you send 
the series out.

--srini
>    
> Best Regards,
> Joakim Zhang
>