Hi Srinivas, > -----Original Message----- > From: Srinivas Kandagatla <srinivas.kandagatla@xxxxxxxxxx> > Sent: 2021年9月8日 16:49 > To: Joakim Zhang <qiangqing.zhang@xxxxxxx>; Rob Herring > <robh@xxxxxxxxxx> > Cc: shawnguo@xxxxxxxxxx; kernel@xxxxxxxxxxxxxx; dl-linux-imx > <linux-imx@xxxxxxx>; devicetree@xxxxxxxxxxxxxxx; > linux-kernel@xxxxxxxxxxxxxxx > 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&data=04%7C01%7Cqiangqing.zhang%40nxp.com%7Cb8b85 > eab6bc34 > >> > 917b86e08d972a57dee%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0% > 7C6376 > >> > 66877370588296%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLC > JQIjoiV2 > >> > luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=diFgK2ufOUK > eXwd > >> 0Ez8pCFjCUH8rXz5jfW7io8KDKmw%3D&reserved=0 > >> > 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://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 > > 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 > > 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? Best Regards, Joakim Zhang
