From: Boris Brezillon <firstname.lastname@example.org> To: Alban <email@example.com> Cc: Srinivas Kandagatla <firstname.lastname@example.org>, Bartosz Golaszewski <email@example.com>, Jonathan Corbet <firstname.lastname@example.org>, Sekhar Nori <email@example.com>, Kevin Hilman <firstname.lastname@example.org>, Russell King <email@example.com>, Arnd Bergmann <firstname.lastname@example.org>, Greg Kroah-Hartman <email@example.com>, David Woodhouse <firstname.lastname@example.org>, Brian Norris <email@example.com>, Marek Vasut <firstname.lastname@example.org>, Richard Weinberger <email@example.com>, Grygorii Strashko <firstname.lastname@example.org>, "David S . Miller" <email@example.com>, Naren <firstname.lastname@example.org>, Mauro Carvalho Chehab <email@example.com>, Andrew Morton <firstname.lastname@example.org>, Lukas Wunner <email@example.com>, Dan Carpenter <firstname.lastname@example.org>, Florian Fainelli <email@example.com>, Ivan Khoronzhuk <firstname.lastname@example.org>, Sven Van Asbroeck <email@example.com>, Paolo Abeni <firstname.lastname@example.org>, Rob Herring <email@example.com>, David Lechner <firstname.lastname@example.org>, Andrew Lunn <email@example.com>, firstname.lastname@example.org, email@example.com, firstname.lastname@example.org, email@example.com, firstname.lastname@example.org, email@example.com, firstname.lastname@example.org, Bartosz Golaszewski <email@example.com> Subject: Re: [PATCH v2 06/29] mtd: Add support for reading MTD devices via the nvmem API Date: Tue, 21 Aug 2018 14:57:25 +0200 Message-ID: <20180821145725.3b385399@bbrezillon> (raw) In-Reply-To: <20180821142716.0adcdd56@eos> On Tue, 21 Aug 2018 14:27:16 +0200 Alban <firstname.lastname@example.org> wrote: > On Tue, 21 Aug 2018 07:44:04 +0200 > Boris Brezillon <email@example.com> wrote: > > > On Tue, 21 Aug 2018 00:53:27 +0200 > > Alban <firstname.lastname@example.org> wrote: > > > > > On Sun, 19 Aug 2018 18:46:09 +0200 > > > Boris Brezillon <email@example.com> wrote: > > > > > > > On Sun, 19 Aug 2018 13:31:06 +0200 > > > > Alban <firstname.lastname@example.org> wrote: > > > > > > > > > On Fri, 17 Aug 2018 18:27:20 +0200 > > > > > Boris Brezillon <email@example.com> wrote: > > > > > > > > > > > Hi Bartosz, > > > > > > > > > > > > On Fri, 10 Aug 2018 10:05:03 +0200 > > > > > > Bartosz Golaszewski <firstname.lastname@example.org> wrote: > > > > > > > > > > > > > From: Alban Bedel <email@example.com> > > > > > > > > > > > > > > Allow drivers that use the nvmem API to read data stored on > > > > > > > MTD devices. For this the mtd devices are registered as > > > > > > > read-only NVMEM providers. > > > > > > > > > > > > > > Signed-off-by: Alban Bedel <firstname.lastname@example.org> > > > > > > > [Bartosz: > > > > > > > - use the managed variant of nvmem_register(), > > > > > > > - set the nvmem name] > > > > > > > Signed-off-by: Bartosz Golaszewski > > > > > > > <email@example.com> > > > > > > > > > > > > What happened to the 2 other patches of Alban's series? I'd > > > > > > really like the DT case to be handled/agreed on in the same > > > > > > patchset, but IIRC, Alban and Srinivas disagreed on how this > > > > > > should be represented. I hope this time we'll come to an > > > > > > agreement, because the MTD <-> NVMEM glue has been floating > > > > > > around for quite some time... > > > > > > > > > > These other patches were to fix what I consider a fundamental > > > > > flaw in the generic NVMEM bindings, however we couldn't agree > > > > > on this point. Bartosz later contacted me to take over this > > > > > series and I suggested to just change the MTD NVMEM binding to > > > > > use a compatible string on the NVMEM cells as an alternative > > > > > solution to fix the clash with the old style MTD partition. > > > > > > > > > > However all this has no impact on the code needed to add NVMEM > > > > > support to MTD, so the above patch didn't change at all. > > > > > > > > It does have an impact on the supported binding though. > > > > nvmem->dev.of_node is automatically assigned to mtd->dev.of_node, > > > > which means people will be able to define their NVMEM cells > > > > directly under the MTD device and reference them from other nodes > > > > (even if it's not documented), and as you said, it conflict with > > > > the old MTD partition bindings. So we'd better agree on this > > > > binding before merging this patch. > > > > > > Unless the nvmem cell node has a compatible string, then it won't be > > > considered as a partition by the MTD code. That is were the clash > > > is, both bindings allow free named child nodes without a compatible > > > string. > > > > Except the current nvmem cells parsing code does not enforce that, and > > existing DTs rely on this behavior, so we're screwed. Or are you > > suggesting to add a new "bool check_cells_compat;" field to > > nvmem_config? > > There is no nvmem cell parsing at the moment. The DT lookup just > resolve the phandle to the cell node, take the parent node and search > for the nvmem provider that has this OF node. So extending it in case > the node has a *new* compatible string would not break users of the old > binding, none of them has a compatible string. But we want to enforce the compat check on MTD devices, otherwise old MTD partitions (those defined under the MTD node) will be considered as NVMEM cells by the NVMEM framework. Hence the bool check_cells_compat field. > > > > > > > > I see several options: > > > > > > > > 1/ provide a way to tell the NVMEM framework not to use > > > > parent->of_node even if it's != NULL. This way we really don't > > > > support defining NVMEM cells in the DT, and also don't support > > > > referencing the nvmem device using a phandle. > > > > > > I really don't get what the point of this would be. Make the whole > > > API useless? > > > > No, just allow Bartosz to get his changes merged without waiting for > > you and Srinivas to agree on how to handle the new binding. As I said > > earlier, this mtd <-> nvmem stuff has been around for quite some time, > > and instead of trying to find an approach that makes everyone happy, > > you decided to let the patchset die. > > As long as that wouldn't prevent using DT in the future I'm fine with > it. > > > > > > > > 2/ define a new binding where all nvmem-cells are placed in an > > > > "nvmem" subnode (just like we have this "partitions" subnode > > > > for partitions), and then add a config->of_node field so that the > > > > nvmem provider can explicitly specify the DT node representing > > > > the nvmem device. We'll also need to set this field to > > > > ERR_PTR(-ENOENT) in case this node does not exist so that the > > > > nvmem framework knows that it should not assign > > > > nvmem->dev.of_node to parent->of_node > > > > > > This is not good. First the NVMEM device is only a virtual concept > > > of the Linux kernel, it has no place in the DT. > > > > nvmem-cells is a virtual concept too, still, you define them in the > > DT. > > To be honest I also think that naming this concept "nvmem" in the DT was > a bad idea. Perhaps something like "driver-data" or "data-cell" would > have been better as that would make it clear what this is about, nvmem > is just the Linux implementation of this concept. I'm fine using a different name. > > > > Secondly the NVMEM > > > provider (here the MTD device) then has to manually parse its DT > > > node to find this subnode, pass it to the NVMEM framework to later > > > again resolve it back to the MTD device. > > > > We don't resolve it back to the MTD device, because the MTD device is > > just the parent of the nvmem device. > > > > > Not very complex but still a lot of > > > useless code, just registering the MTD device is a lot simpler and > > > much more inline with most other kernel API that register a > > > "service" available from a device. > > > > I'm not a big fan of this option either, but I thought I had to > > propose it. > > > > > > > > > 3/ only declare partitions as nvmem providers. This would solve > > > > the problem we have with partitions defined in the DT since > > > > defining sub-partitions in the DT is not (yet?) supported and > > > > partition nodes are supposed to be leaf nodes. Still, I'm not > > > > a big fan of this solution because it will prevent us from > > > > supporting sub-partitions if we ever want/need to. > > > > > > That sound like a poor workaround. > > > > Yes, that's a workaround. And the reason I propose it, is, again, > > because I don't want to block Bartosz. > > > > > Remember that this problem could > > > appear with any device that has a binding that use child nodes. > > > > I'm talking about partitions, and you're talking about mtd devices. > > Right now partitions don't have subnodes, and if we define that > > partition subnodes should describe nvmem-cells, then it becomes part > > of the official binding. So, no, the problem you mention does not > > (yet) exist. > > That would add another binding that allow free named child nodes > without compatible string although experience has repeatedly shown that > this was a bad idea. Yes, I agree. Just thought it was important to have this solution in the list, even if it's just to reject it. > > > > > > > > 4/ Add a ->of_xlate() hook that would be called if present by the > > > > framework instead of using the default parsing we have right > > > > now. > > > > > > That is a bit cleaner, but I don't think it would be worse the > > > complexity. > > > > But it's way more flexible than putting everything in the nvmem > > framework. BTW, did you notice that nvmem-cells parsing does not work > > with flashes bigger than 4GB, because the framework assumes > > #address-cells and #size-cells are always 1. That's probably something > > we'll have to fix for the MTD case. > > Yes, however that's just an implementation limitation which is trivial > to solve. Agree. I was just pointing it in case you hadn't noticed. > > > > Furthermore xlate functions are more about converting > > > from hardware parameters to internal kernel representation than to > > > hide extra DT parsing. > > > > Hm, how is that different? ->of_xlate() is just a way for drivers to > > have their own DT representation, which is exactly what we want here. > > There is a big difference. DT represent the hardware and the > relationship between the devices in an OS independent format. We don't > add extra stuff in there just to map back internal Linux API details. And I'm not talking about adding SW information in the DT, I'm talking about HW specific description. We have the same solution for pinctrl configs (it's HW/driver specific). > > > > > > > > 5/ Tell the nvmem framework the name of the subnode containing > > > > nvmem cell definitions (if NULL that means cells are directly > > > > defined under the nvmem provider node). We would set it to > > > > "nvmem-cells" (or whatever you like) for the MTD case. > > > > > > If so please match on compatible and not on the node name. > > > > If you like. > > > > > > > > 6/ Extend the current NVMEM cell lookup to check if the parent node > > > of the cell has a compatible string set to "nvmem-cells". If it > > > doesn't it mean we have the current binding and this node is the > > > NVMEM device. If it does the device node is just the next parent. > > > This is trivial to implement (literally 2 lines of code) and cover > > > all the cases currently known. > > > > Except Srinivas was not happy with this solution, and this stalled the > > discussion. I'm trying to find other options and you keep rejecting > > all of them to come back to this one. > > Well, I think this is the best solution :/ > > > > > > > 7/ Just add a compatible string to the nvmem cell. No code change is > > > needed, > > > > That's not true!!! > > What is not true in this statement? The current nvmem lookup don't care > about compatible strings, so the cell lookup would just works fine. The > MTD partition parser won't consider them as a partition because of the > compatible string. Problem solved! No because partitions defined the old way (as direct subnodes of the MTD node) will be considered as NVMEM cells by the NVMEM framework, and I don't want that. Plus, I don't want people to start defining their NVMEM cells and forget the compat string (which would work just fine because the NVMEM framework doesn't care). > > > What forces people to add this compatible in their > > DT? Nothing. I'll tell you what will happen: people will start > > defining their nvmem cells directly under the MTD node because that > > *works*, and even if the binding is not documented and we consider it > > invalid, we'll be stuck supporting it forever. > > Do note that undocumented bindings are not allowed. DTS that use > undocumented bindings (normally) just get rejected. Except that's just in theory. In practice, if people can do something wrong, they'll complain if you later fix the bug and break their setup. So no, if we go for the "nvmem cells have an 'nvmem-cell' compat", then I'd like the NVMEM framework to enforce that somehow.
next prev parent reply index Thread overview: 92+ messages / expand[flat|nested] mbox.gz Atom feed top 2018-08-10 8:04 [PATCH v2 00/29] at24: remove at24_platform_data Bartosz Golaszewski 2018-08-10 8:04 ` [PATCH v2 01/29] nvmem: add support for cell lookups Bartosz Golaszewski 2018-08-24 15:08 ` Boris Brezillon 2018-08-24 15:27 ` Andrew Lunn 2018-08-25 6:27 ` Boris Brezillon 2018-08-27 8:56 ` Bartosz Golaszewski 2018-08-27 9:00 ` Boris Brezillon 2018-08-27 13:37 ` Bartosz Golaszewski 2018-08-27 14:01 ` Boris Brezillon 2018-08-28 10:15 ` Srinivas Kandagatla 2018-08-28 11:56 ` Bartosz Golaszewski 2018-08-28 13:45 ` Srinivas Kandagatla 2018-08-28 14:41 ` Bartosz Golaszewski 2018-08-28 14:48 ` Srinivas Kandagatla 2018-08-28 14:53 ` Boris Brezillon 2018-08-28 15:09 ` Srinivas Kandagatla 2018-08-10 8:04 ` [PATCH v2 02/29] Documentation: nvmem: document lookup entries Bartosz Golaszewski 2018-08-31 20:30 ` Brian Norris 2018-09-01 13:11 ` Bartosz Golaszewski 2018-08-10 8:05 ` [PATCH v2 03/29] nvmem: add a notifier chain Bartosz Golaszewski 2018-08-10 8:33 ` Srinivas Kandagatla 2018-08-10 8:05 ` [PATCH v2 04/29] nvmem: provide nvmem_dev_name() Bartosz Golaszewski 2018-08-10 8:10 ` Srinivas Kandagatla 2018-08-10 8:05 ` [PATCH v2 05/29] nvmem: remove the name field from struct nvmem_device Bartosz Golaszewski 2018-08-10 8:33 ` Srinivas Kandagatla 2018-08-10 8:05 ` [PATCH v2 06/29] mtd: Add support for reading MTD devices via the nvmem API Bartosz Golaszewski 2018-08-17 16:27 ` Boris Brezillon 2018-08-19 11:31 ` Alban 2018-08-19 16:46 ` Boris Brezillon 2018-08-20 10:43 ` Srinivas Kandagatla 2018-08-20 18:20 ` Boris Brezillon 2018-08-20 18:50 ` Bartosz Golaszewski 2018-08-20 19:06 ` Boris Brezillon 2018-08-20 21:27 ` Alban 2018-08-21 5:07 ` Boris Brezillon 2018-08-21 9:50 ` Srinivas Kandagatla 2018-08-21 9:56 ` Boris Brezillon 2018-08-21 10:11 ` Srinivas Kandagatla 2018-08-21 10:43 ` Boris Brezillon 2018-08-21 11:39 ` Alban 2018-08-21 12:00 ` Srinivas Kandagatla 2018-08-21 13:01 ` Boris Brezillon 2018-08-23 10:29 ` Alban 2018-08-24 14:39 ` Boris Brezillon 2018-08-28 10:20 ` Srinivas Kandagatla 2018-08-20 22:53 ` Alban 2018-08-21 5:44 ` Boris Brezillon 2018-08-21 9:38 ` Srinivas Kandagatla 2018-08-21 11:31 ` Boris Brezillon 2018-08-21 13:34 ` Srinivas Kandagatla 2018-08-21 13:37 ` Srinivas Kandagatla 2018-08-21 13:57 ` Boris Brezillon 2018-08-21 12:27 ` Alban 2018-08-21 12:57 ` Boris Brezillon [this message] 2018-08-21 13:57 ` Alban 2018-08-21 14:26 ` Boris Brezillon 2018-08-21 14:33 ` Srinivas Kandagatla 2018-08-10 8:05 ` [PATCH v2 07/29] ARM: davinci: dm365-evm: use nvmem lookup for mac address Bartosz Golaszewski 2018-08-10 8:05 ` [PATCH v2 08/29] ARM: davinci: dm644-evm: " Bartosz Golaszewski 2018-08-10 8:05 ` [PATCH v2 09/29] ARM: davinci: dm646x-evm: " Bartosz Golaszewski 2018-08-10 8:05 ` [PATCH v2 10/29] ARM: davinci: da830-evm: " Bartosz Golaszewski 2018-08-10 8:05 ` [PATCH v2 11/29] ARM: davinci: mityomapl138: add nvmem cells lookup entries Bartosz Golaszewski 2018-08-10 8:05 ` [PATCH v2 12/29] ARM: davinci: da850-evm: use nvmem lookup for mac address Bartosz Golaszewski 2018-08-10 8:05 ` [PATCH v2 13/29] ARM: davinci: da850-evm: remove unnecessary include Bartosz Golaszewski 2018-08-10 8:05 ` [PATCH v2 14/29] net: simplify eth_platform_get_mac_address() Bartosz Golaszewski 2018-08-10 14:39 ` Andy Shevchenko 2018-08-10 16:17 ` Bartosz Golaszewski 2018-08-10 8:05 ` [PATCH v2 15/29] net: split eth_platform_get_mac_address() into subroutines Bartosz Golaszewski 2018-08-31 19:54 ` Brian Norris 2018-08-10 8:05 ` [PATCH v2 16/29] net: add support for nvmem to eth_platform_get_mac_address() Bartosz Golaszewski 2018-08-10 8:05 ` [PATCH v2 17/29] net: davinci_emac: use eth_platform_get_mac_address() Bartosz Golaszewski 2018-08-10 8:05 ` [PATCH v2 18/29] ARM: davinci: da850-evm: remove dead MTD code Bartosz Golaszewski 2018-08-10 8:05 ` [PATCH v2 19/29] ARM: davinci: mityomapl138: don't read the MAC address from machine code Bartosz Golaszewski 2018-08-10 8:05 ` [PATCH v2 20/29] ARM: davinci: dm365-evm: use device properties for at24 eeprom Bartosz Golaszewski 2018-08-10 8:05 ` [PATCH v2 21/29] ARM: davinci: da830-evm: " Bartosz Golaszewski 2018-08-10 8:05 ` [PATCH v2 22/29] ARM: davinci: dm644x-evm: " Bartosz Golaszewski 2018-08-10 8:05 ` [PATCH v2 23/29] ARM: davinci: dm646x-evm: " Bartosz Golaszewski 2018-08-10 8:05 ` [PATCH v2 24/29] ARM: davinci: sffsdr: fix the at24 eeprom device name Bartosz Golaszewski 2018-08-10 8:05 ` [PATCH v2 25/29] ARM: davinci: sffsdr: use device properties for at24 eeprom Bartosz Golaszewski 2018-08-10 8:05 ` [PATCH v2 26/29] ARM: davinci: remove dead code related to MAC address reading Bartosz Golaszewski 2018-08-10 8:05 ` [PATCH v2 27/29] ARM: davinci: mityomapl138: use nvmem notifiers Bartosz Golaszewski 2018-08-10 8:05 ` [PATCH v2 28/29] ARM: davinci: mityomapl138: use device properties for at24 eeprom Bartosz Golaszewski 2018-08-10 8:05 ` [PATCH v2 29/29] eeprom: at24: kill at24_platform_data Bartosz Golaszewski 2018-08-10 8:41 ` [PATCH v2 00/29] at24: remove at24_platform_data Srinivas Kandagatla 2018-08-31 19:46 ` Brian Norris 2018-10-03 20:15 ` Bartosz Golaszewski 2018-10-03 20:30 ` Lukas Wunner 2018-10-03 21:04 ` Florian Fainelli 2018-10-04 11:06 ` Bartosz Golaszewski 2018-10-04 13:58 ` Arnd Bergmann 2018-10-04 14:35 ` Sowmini Varadhan 2018-10-04 14:38 ` Arnd Bergmann
Reply instructions: You may reply publically to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=20180821145725.3b385399@bbrezillon \ --firstname.lastname@example.org \ --email@example.com \ --firstname.lastname@example.org \ --email@example.com \ --firstname.lastname@example.org \ --email@example.com \ --firstname.lastname@example.org \ --email@example.com \ --firstname.lastname@example.org \ --email@example.com \ --firstname.lastname@example.org \ --email@example.com \ --firstname.lastname@example.org \ --email@example.com \ --firstname.lastname@example.org \ --email@example.com \ --firstname.lastname@example.org \ --email@example.com \ --firstname.lastname@example.org \ --email@example.com \ --firstname.lastname@example.org \ --email@example.com \ --firstname.lastname@example.org \ --email@example.com \ --firstname.lastname@example.org \ --email@example.com \ --firstname.lastname@example.org \ --email@example.com \ --firstname.lastname@example.org \ --email@example.com \ --firstname.lastname@example.org \ --email@example.com \ --firstname.lastname@example.org \ --email@example.com \ --firstname.lastname@example.org \ --email@example.com \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: link
LKML Archive on lore.kernel.org Archives are clonable: git clone --mirror https://lore.kernel.org/lkml/0 lkml/git/0.git git clone --mirror https://lore.kernel.org/lkml/1 lkml/git/1.git git clone --mirror https://lore.kernel.org/lkml/2 lkml/git/2.git git clone --mirror https://lore.kernel.org/lkml/3 lkml/git/3.git git clone --mirror https://lore.kernel.org/lkml/4 lkml/git/4.git git clone --mirror https://lore.kernel.org/lkml/5 lkml/git/5.git git clone --mirror https://lore.kernel.org/lkml/6 lkml/git/6.git git clone --mirror https://lore.kernel.org/lkml/7 lkml/git/7.git # If you have public-inbox 1.1+ installed, you may # initialize and index your mirror using the following commands: public-inbox-init -V2 lkml lkml/ https://lore.kernel.org/lkml \ firstname.lastname@example.org email@example.com public-inbox-index lkml Newsgroup available over NNTP: nntp://nntp.lore.kernel.org/org.kernel.vger.linux-kernel AGPL code for this site: git clone https://public-inbox.org/ public-inbox