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

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

On 08/09/2021 09:57, Joakim Zhang wrote:
> 
> Hi Srinivas,
> 
>> -----Original Message-----
>> From: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
>> Sent: 2021年9月8日 16:49
>> To: Joakim Zhang <qiangqing.zhang@nxp.com>; Rob Herring
>> <robh@kernel.org>
>> Cc: shawnguo@kernel.org; kernel@pengutronix.de; dl-linux-imx
>> <linux-imx@nxp.com>; devicetree@vger.kernel.org;
>> linux-kernel@vger.kernel.org
>> Subject: Re: [PATCH V1 1/4] bindings: nvmem: introduce "reverse-data"
>> property
>>
>>
>>
>> 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%2F&amp;data=04%7C01%7Cqiangqing.zhang%40nxp.com%7Cb8b85
>> eab6bc34
>>>>
>> 917b86e08d972a57dee%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%
>> 7C6376
>>>>
>> 66877370588296%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLC
>> JQIjoiV2
>>>>
>> luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&amp;sdata=diFgK2ufOUK
>> eXwd
>>>> 0Ez8pCFjCUH8rXz5jfW7io8KDKmw%3D&amp;reserved=0
>>>>
>> el.org%2Fpub%2Fscm%2Flinux%2Fkernel%2Fgit%2Fsrini%2Fnvmem.git%2Flog%
>>>>
>> 2F%3Fh%3Dtopic%2Fpost-processing&amp;data=04%7C01%7Cqiangqing.zhan
>>>>
>> g%40nxp.com%7Cadfa3ba63c634937876308d971e7e71f%7C686ea1d3bc2b4c6
>>>>
>> fa92cd99c5c301635%7C0%7C0%7C637666063097239185%7CUnknown%7CT
>>>>
>> WFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJ
>>>>
>> XVCI6Mn0%3D%7C1000&amp;sdata=W9yAnGm9rYzlSZuAAGiN4VHUtKYUTt9S
>>>> oyGQ9QsY7fI%3D&amp;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://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgit.kern
>> el.org%2Fpub%2Fscm%2Flinux%2Fkernel%2Fgit%2Fsrini%2Fnvmem.git%2Fcom
>> mit%2F%3Fh%3Dtopic%2Fpost-processing%26id%3D624f2cc99b48bbfe05c11e
>> 58fb73f84abb1a646e&amp;data=04%7C01%7Cqiangqing.zhang%40nxp.com%
>> 7Cb8b85eab6bc34917b86e08d972a57dee%7C686ea1d3bc2b4c6fa92cd99c5c3
>> 01635%7C0%7C0%7C637666877370598253%7CUnknown%7CTWFpbGZsb3d8e
>> yJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D
>> %7C1000&amp;sdata=APDzSbLob%2FRiZyyhU7VAhoUEAmSG95NsilQDQ53Hbf
>> A%3D&amp;reserved=0
>>> 	of_get_property() can't get the cell value, so I change to
>>> of_property_read_s32()
>>> 2)
>> https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgit.kern
>> el.org%2Fpub%2Fscm%2Flinux%2Fkernel%2Fgit%2Fsrini%2Fnvmem.git%2Fcom
>> mit%2F%3Fh%3Dtopic%2Fpost-processing%26id%3Da424302c7b15da41e1e8d
>> e56b0c78021b9a96c1e&amp;data=04%7C01%7Cqiangqing.zhang%40nxp.com
>> %7Cb8b85eab6bc34917b86e08d972a57dee%7C686ea1d3bc2b4c6fa92cd99c5c
>> 301635%7C0%7C0%7C637666877370598253%7CUnknown%7CTWFpbGZsb3d8
>> eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D
>> %7C1000&amp;sdata=5E49DVzkpBVdkA4a%2B9tMXN%2B6k%2BG%2B3rQuVJ
>> qTUgdbmKU%3D&amp;reserved=0
>>> 	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.
> 
> As you define the type variable is "int", so had better use of_property_read_s32(), instead if of_property_read_u32(), right?

We should probably make that u32, as we are not expecting any negative 
range.

I tried to fix this in new patches.

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