From: Lee Jones <lee.jones@linaro.org>
To: Frank Rowand <frowand.list@gmail.com>
Cc: andy.shevchenko@gmail.com, michael@walle.cc, robh+dt@kernel.org,
broonie@kernel.org, devicetree@vger.kernel.org,
linus.walleij@linaro.org, linux@roeck-us.net,
andriy.shevchenko@linux.intel.com, robin.murphy@arm.com,
gregkh@linuxfoundation.org, linux-arm-kernel@lists.infradead.org,
linux-kernel@vger.kernel.org
Subject: Re: [PATCH v2 1/3] mfd: core: Make a best effort attempt to match devices with the correct of_nodes
Date: Wed, 24 Jun 2020 07:45:38 +0100 [thread overview]
Message-ID: <20200624064538.GD954398@dell> (raw)
In-Reply-To: <2a25af37-a9b8-e4f3-6092-06c1c907dc9f@gmail.com>
On Tue, 23 Jun 2020, Frank Rowand wrote:
> On 2020-06-11 14:10, Lee Jones wrote:
> > Currently, when a child platform device (sometimes referred to as a
> > sub-device) is registered via the Multi-Functional Device (MFD) API,
> > the framework attempts to match the newly registered platform device
> > with its associated Device Tree (OF) node. Until now, the device has
> > been allocated the first node found with an identical OF compatible
> > string. Unfortunately, if there are, say for example '3' devices
> > which are to be handled by the same driver and therefore have the same
> > compatible string, each of them will be allocated a pointer to the
> > *first* node.
> >
> > An example Device Tree entry might look like this:
> >
> > mfd_of_test {
> > compatible = "mfd,of-test-parent";
> > #address-cells = <0x02>;
> > #size-cells = <0x02>;
> >
> > child@aaaaaaaaaaaaaaaa {
> > compatible = "mfd,of-test-child";
> > reg = <0xaaaaaaaa 0xaaaaaaaa 0 0x11>,
> > <0xbbbbbbbb 0xbbbbbbbb 0 0x22>;
> > };
> >
> > child@cccccccc {
> > compatible = "mfd,of-test-child";
> > reg = <0x00000000 0xcccccccc 0 0x33>;
> > };
> >
> > child@dddddddd00000000 {
> > compatible = "mfd,of-test-child";
> > reg = <0xdddddddd 0x00000000 0 0x44>;
> > };
> > };
> >
> > When used with example sub-device registration like this:
> >
> > static const struct mfd_cell mfd_of_test_cell[] = {
> > OF_MFD_CELL("mfd-of-test-child", NULL, NULL, 0, 0, "mfd,of-test-child"),
> > OF_MFD_CELL("mfd-of-test-child", NULL, NULL, 0, 1, "mfd,of-test-child"),
> > OF_MFD_CELL("mfd-of-test-child", NULL, NULL, 0, 2, "mfd,of-test-child")
> > };
> >
> > ... the current implementation will result in all devices being allocated
> > the first OF node found containing a matching compatible string:
> >
> > [0.712511] mfd-of-test-child mfd-of-test-child.0: Probing platform device: 0
> > [0.712710] mfd-of-test-child mfd-of-test-child.0: Using OF node: child@aaaaaaaaaaaaaaaa
> > [0.713033] mfd-of-test-child mfd-of-test-child.1: Probing platform device: 1
> > [0.713381] mfd-of-test-child mfd-of-test-child.1: Using OF node: child@aaaaaaaaaaaaaaaa
> > [0.713691] mfd-of-test-child mfd-of-test-child.2: Probing platform device: 2
> > [0.713889] mfd-of-test-child mfd-of-test-child.2: Using OF node: child@aaaaaaaaaaaaaaaa
> >
> > After this patch each device will be allocated a unique OF node:
> >
> > [0.712511] mfd-of-test-child mfd-of-test-child.0: Probing platform device: 0
> > [0.712710] mfd-of-test-child mfd-of-test-child.0: Using OF node: child@aaaaaaaaaaaaaaaa
> > [0.713033] mfd-of-test-child mfd-of-test-child.1: Probing platform device: 1
> > [0.713381] mfd-of-test-child mfd-of-test-child.1: Using OF node: child@cccccccc
> > [0.713691] mfd-of-test-child mfd-of-test-child.2: Probing platform device: 2
> > [0.713889] mfd-of-test-child mfd-of-test-child.2: Using OF node: child@dddddddd00000000
> >
> > Which is fine if all OF nodes are identical. However if we wish to
> > apply an attribute to particular device, we really need to ensure the
> > correct OF node will be associated with the device containing the
> > correct address. We accomplish this by matching the device's address
> > expressed in DT with one provided during sub-device registration.
> > Like this:
> >
> > static const struct mfd_cell mfd_of_test_cell[] = {
> > OF_MFD_CELL_REG("mfd-of-test-child", NULL, NULL, 0, 1, "mfd,of-test-child", 0xdddddddd00000000),
> > OF_MFD_CELL_REG("mfd-of-test-child", NULL, NULL, 0, 2, "mfd,of-test-child", 0xaaaaaaaaaaaaaaaa),
> > OF_MFD_CELL_REG("mfd-of-test-child", NULL, NULL, 0, 3, "mfd,of-test-child", 0x00000000cccccccc)
> > };
> >
> > This will ensure a specific device (designated here using the
> > platform_ids; 1, 2 and 3) is matched with a particular OF node:
> >
> > [0.712511] mfd-of-test-child mfd-of-test-child.0: Probing platform device: 0
> > [0.712710] mfd-of-test-child mfd-of-test-child.0: Using OF node: child@dddddddd00000000
> > [0.713033] mfd-of-test-child mfd-of-test-child.1: Probing platform device: 1
> > [0.713381] mfd-of-test-child mfd-of-test-child.1: Using OF node: child@aaaaaaaaaaaaaaaa
> > [0.713691] mfd-of-test-child mfd-of-test-child.2: Probing platform device: 2
> > [0.713889] mfd-of-test-child mfd-of-test-child.2: Using OF node: child@cccccccc
> >
> > This implementation is still not infallible, hence the mention of
> > "best effort" in the commit subject. Since we have not *insisted* on
> > the existence of 'reg' properties (in some scenarios they just do not
> > make sense) and no device currently uses the new 'of_reg' attribute,
> > we have to make an on-the-fly judgement call whether to associate the
> > OF node anyway. Which we do in cases where parent drivers haven't
> > specified a particular OF node to match to. So there is a *slight*
> > possibility of the following result (note: the implementation here is
> > convoluted, but it shows you one means by which this process can
> > still break):
> >
> > /*
> > * First entry will match to the first OF node with matching compatible
> > * Second will fail, since the first took its OF node and is no longer available
> > * Third will succeed
> > */
> > static const struct mfd_cell mfd_of_test_cell[] = {
> > OF_MFD_CELL("mfd-of-test-child", NULL, NULL, 0, 1, "mfd,of-test-child"),
> > OF_MFD_CELL_REG("mfd-of-test-child", NULL, NULL, 0, 2, "mfd,of-test-child", 0xaaaaaaaaaaaaaaaa),
> > OF_MFD_CELL_REG("mfd-of-test-child", NULL, NULL, 0, 3, "mfd,of-test-child", 0x00000000cccccccc)
> > };
> >
> > The result:
> >
> > [0.753869] mfd-of-test-parent mfd_of_test: Registering 3 devices
> > [0.756597] mfd-of-test-child: Failed to locate of_node [id: 2]
> > [0.759999] mfd-of-test-child mfd-of-test-child.1: Probing platform device: 1
> > [0.760314] mfd-of-test-child mfd-of-test-child.1: Using OF node: child@aaaaaaaaaaaaaaaa
> > [0.760908] mfd-of-test-child mfd-of-test-child.2: Probing platform device: 2
> > [0.761183] mfd-of-test-child mfd-of-test-child.2: No OF node associated with this device
> > [0.761621] mfd-of-test-child mfd-of-test-child.3: Probing platform device: 3
> > [0.761899] mfd-of-test-child mfd-of-test-child.3: Using OF node: child@cccccccc
> >
> > We could code around this with some pre-parsing semantics, but the
> > added complexity required to cover each and every corner-case is not
> > justified. Merely patching the current failing (via this patch) is
> > already working with some pretty small corner-cases. Other issues
> > should be patched in the parent drivers which can be achieved simply
> > by implementing OF_MFD_CELL_REG().
> >
> > Signed-off-by: Lee Jones <lee.jones@linaro.org>
> > ---
> >
> > Changelog:
> >
> > v1 => v2:
> > * Simply return -EAGAIN if node is already in use
> > * Allow for valid of_reg=0 by introducing use_of_reg boolean flag
> > * Split helpers out into separate patch
> >
> > drivers/mfd/mfd-core.c | 99 +++++++++++++++++++++++++++++++++++-----
> > include/linux/mfd/core.h | 10 ++++
> > 2 files changed, 97 insertions(+), 12 deletions(-)
[...]
> > diff --git a/include/linux/mfd/core.h b/include/linux/mfd/core.h
> > index d01d1299e49dc..a148b907bb7f1 100644
> > --- a/include/linux/mfd/core.h
> > +++ b/include/linux/mfd/core.h
> > @@ -78,6 +78,16 @@ struct mfd_cell {
> > */
> > const char *of_compatible;
> >
> > + /*
> > + * Address as defined in Device Tree. Used to compement 'of_compatible'
> > + * (above) when matching OF nodes with devices that have identical
> > + * compatible strings
> > + */
>
> Instead of the above comment, suggest something like instead (I have not properly
> line wrapped, to make it easier to see the difference):
>
> > + /*
> > + * Address as defined in Device Tree mfd child node "reg" property. Used in combination with 'of_compatible'
> > + * (above) when matching OF nodes with devices that have identical
> > + * compatible strings
> > + */
I'll split the difference and make it more clear, thanks.
--
Lee Jones [李琼斯]
Senior Technical Lead - Developer Services
Linaro.org │ Open source software for Arm SoCs
Follow Linaro: Facebook | Twitter | Blog
next prev parent reply other threads:[~2020-06-24 6:45 UTC|newest]
Thread overview: 39+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-06-11 19:10 [PATCH v2 1/3] mfd: core: Make a best effort attempt to match devices with the correct of_nodes Lee Jones
2020-06-11 19:10 ` [PATCH v2 2/3] mfd: core: Fix formatting of MFD helpers Lee Jones
2020-06-12 12:27 ` Frank Rowand
2020-06-11 19:10 ` [PATCH v2 3/3] mfd: core: Add OF_MFD_CELL_REG() helper Lee Jones
2020-06-12 12:28 ` Frank Rowand
2020-06-12 12:26 ` [PATCH v2 1/3] mfd: core: Make a best effort attempt to match devices with the correct of_nodes Frank Rowand
2020-06-15 1:19 ` Frank Rowand
2020-06-15 9:26 ` Lee Jones
2020-06-18 17:34 ` Frank Rowand
2020-06-22 8:50 ` Lee Jones
2020-06-22 14:32 ` Frank Rowand
2020-06-22 14:35 ` Frank Rowand
2020-06-22 15:10 ` Lee Jones
2020-06-22 18:01 ` Frank Rowand
2020-06-22 18:04 ` Frank Rowand
2020-06-22 19:11 ` Lee Jones
2020-06-22 22:23 ` Frank Rowand
2020-06-23 1:17 ` Frank Rowand
2020-06-23 1:37 ` Frank Rowand
2020-06-23 6:47 ` Lee Jones
2020-06-23 17:55 ` Frank Rowand
2020-06-23 19:59 ` Lee Jones
2020-06-23 22:33 ` Frank Rowand
2020-06-24 7:46 ` Lee Jones
2020-06-24 15:51 ` Frank Rowand
2020-06-24 16:14 ` Lee Jones
2020-06-24 16:25 ` Frank Rowand
2020-06-22 8:09 ` Lee Jones
2020-06-22 16:09 ` Frank Rowand
2020-06-22 16:53 ` Lee Jones
2020-06-23 22:21 ` Frank Rowand
2020-06-24 6:45 ` Lee Jones [this message]
2020-06-23 23:03 ` Frank Rowand
2020-06-24 6:41 ` Lee Jones
2020-06-24 7:47 ` Michael Walle
2020-06-24 8:23 ` Lee Jones
2020-06-24 9:19 ` Michael Walle
2020-06-24 11:24 ` Lee Jones
2020-06-11 19:12 Lee Jones
Reply instructions:
You may reply publicly 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=20200624064538.GD954398@dell \
--to=lee.jones@linaro.org \
--cc=andriy.shevchenko@linux.intel.com \
--cc=andy.shevchenko@gmail.com \
--cc=broonie@kernel.org \
--cc=devicetree@vger.kernel.org \
--cc=frowand.list@gmail.com \
--cc=gregkh@linuxfoundation.org \
--cc=linus.walleij@linaro.org \
--cc=linux-arm-kernel@lists.infradead.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux@roeck-us.net \
--cc=michael@walle.cc \
--cc=robh+dt@kernel.org \
--cc=robin.murphy@arm.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
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).