Message-ID: <DB8PR04MB679589EB203FAF1517FC4811E6D49@DB8PR04MB6795.eurprd04.prod.outlook.com>
Date:   Wed, 8 Sep 2021 08:57:33 +0000
From:   Joakim Zhang <qiangqing.zhang@....com>
To:     Srinivas Kandagatla <srinivas.kandagatla@...aro.org>,
        Rob Herring <robh@...nel.org>
CC:     "shawnguo@...nel.org" <shawnguo@...nel.org>,
        "kernel@...gutronix.de" <kernel@...gutronix.de>,
        dl-linux-imx <linux-imx@....com>,
        "devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: RE: [PATCH V1 1/4] bindings: nvmem: introduce "reverse-data" property


Hi Srinivas,

> -----Original Message-----
> From: Srinivas Kandagatla <srinivas.kandagatla@...aro.org>
> Sent: 2021年9月8日 16:49
> To: Joakim Zhang <qiangqing.zhang@....com>; Rob Herring
> <robh@...nel.org>
> Cc: shawnguo@...nel.org; kernel@...gutronix.de; dl-linux-imx
> <linux-imx@....com>; devicetree@...r.kernel.org;
> linux-kernel@...r.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?

Best Regards,
Joakim Zhang

Powered by blists - more mailing lists