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

Hi Srinivas,

-----Original Message-----
From: Srinivas Kandagatla <redacted>
Sent: 2021年9月8日 16:49
To: Joakim Zhang <redacted>; Rob Herring
[off-list ref]
Cc: shawnguo@kernel.org; kernel@pengutronix.de; dl-linux-imx
[off-list ref]; 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:

quoted

Hi Srinivas,

[...]

quoted

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&data=04%7C01%7Cqiangqing.zhang%40nxp.com%7Cb8b85

eab6bc34

quoted

quoted

917b86e08d972a57dee%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%
7C6376

quoted

quoted

66877370588296%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLC
JQIjoiV2

quoted

quoted

luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=diFgK2ufOUK
eXwd

quoted

quoted

0Ez8pCFjCUH8rXz5jfW7io8KDKmw%3D&reserved=0

el.org%2Fpub%2Fscm%2Flinux%2Fkernel%2Fgit%2Fsrini%2Fnvmem.git%2Flog%

quoted

quoted

2F%3Fh%3Dtopic%2Fpost-processing&data=04%7C01%7Cqiangqing.zhan

quoted

quoted

g%40nxp.com%7Cadfa3ba63c634937876308d971e7e71f%7C686ea1d3bc2b4c6

quoted

quoted

fa92cd99c5c301635%7C0%7C0%7C637666063097239185%7CUnknown%7CT

quoted

quoted

WFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJ

quoted

quoted

XVCI6Mn0%3D%7C1000&sdata=W9yAnGm9rYzlSZuAAGiN4VHUtKYUTt9S

quoted

quoted

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,

quoted

quoted

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&data=04%7C01%7Cqiangqing.zhang%40nxp.com%
7Cb8b85eab6bc34917b86e08d972a57dee%7C686ea1d3bc2b4c6fa92cd99c5c3
01635%7C0%7C0%7C637666877370598253%7CUnknown%7CTWFpbGZsb3d8e
yJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D
%7C1000&sdata=APDzSbLob%2FRiZyyhU7VAhoUEAmSG95NsilQDQ53Hbf
A%3D&reserved=0

quoted

	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&data=04%7C01%7Cqiangqing.zhang%40nxp.com
%7Cb8b85eab6bc34917b86e08d972a57dee%7C686ea1d3bc2b4c6fa92cd99c5c
301635%7C0%7C0%7C637666877370598253%7CUnknown%7CTWFpbGZsb3d8
eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D
%7C1000&sdata=5E49DVzkpBVdkA4a%2B9tMXN%2B6k%2BG%2B3rQuVJ
qTUgdbmKU%3D&reserved=0

quoted

	if (!nvmem->cell_post_process) {} should be if (nvmem->cell_post_process)

{}, if we have this callback, we need do the post-processing.

quoted

I have pushed these changes now to the branch.

quoted

quoted

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.

quoted

quoted

2. In imx devicetree for mac-address nvmem cell make sure you add

cell-type = <NVMEM_CELL_TYPE_MAC_ADDRESS>;

quoted

quoted

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.

quoted

quoted

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

quoted

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?

Best Regards,
Joakim Zhang