Re: [PATCH v2 1/3] mfd: core: Make a best effort attempt to match devices with the correct of_nodes
On 2020-06-24 11:14, Lee Jones wrote: > On Wed, 24 Jun 2020, Frank Rowand wrote: > >> On 2020-06-24 02:46, Lee Jones wrote: >>> On Tue, 23 Jun 2020, Frank Rowand wrote: >>> On 2020-06-23 14:59, Lee Jones wrote: >> >> < big snip > >> >> Thanks for the replies in the above portion. > > NP. > >> But yes or no to my solution #2 (with some slight changes to >> make it better (more gracious handling of the detected error) as >> discussed elsewhere in the email thread)? > > Please see "[0]" above! > > AFAICT your solution #2 involves bombing out *all* devices if there is > a duplicate compatible with no 'reg' property value. This is a) > over-kill and b) not an error, as I mentioned: As I mentioned above, I set you up to have this misunderstanding by a mistake in one of my earlier emails. So now that I have pointed out what I meant here by "more gracious handling of the detected error", what do you think of my amended solution #2? >>> >>> Explained above, but the LT;DR is that it's not correct. >> >> I don't agree with you, I think my solution is better. Even if I >> prefer my solution, I find your solution to be good enough. > > I still don't see how it could work, but please feel free to submit a > subsequent patch and we can discuss it on its own merits. > >> So I am dropping my specific objection to returning -EAGAIN from >> mfd_match_of_node_to_dev() when the node has previously been >> allocated to a device. > > Great. Thanks for taking an interest. > > Does this mean I can apply your Reviewed-by? > No, please do not. I don't want to give the patch that strong of an endorsement.
Re: [PATCH v2 1/3] mfd: core: Make a best effort attempt to match devices with the correct of_nodes
On Wed, 24 Jun 2020, Frank Rowand wrote: > On 2020-06-24 02:46, Lee Jones wrote: > > On Tue, 23 Jun 2020, Frank Rowand wrote: > > > >> On 2020-06-23 14:59, Lee Jones wrote: > > < big snip > > > Thanks for the replies in the above portion. NP. > But yes or no to my solution #2 (with some slight changes to > make it better (more gracious handling of the detected error) as > discussed elsewhere in the email thread)? > >>> > >>> Please see "[0]" above! > >>> > >>> AFAICT your solution #2 involves bombing out *all* devices if there is > >>> a duplicate compatible with no 'reg' property value. This is a) > >>> over-kill and b) not an error, as I mentioned: > >> > >> As I mentioned above, I set you up to have this misunderstanding by > >> a mistake in one of my earlier emails. So now that I have pointed > >> out what I meant here by "more gracious handling of the detected > >> error", what do you think of my amended solution #2? > > > > Explained above, but the LT;DR is that it's not correct. > > I don't agree with you, I think my solution is better. Even if I > prefer my solution, I find your solution to be good enough. I still don't see how it could work, but please feel free to submit a subsequent patch and we can discuss it on its own merits. > So I am dropping my specific objection to returning -EAGAIN from > mfd_match_of_node_to_dev() when the node has previously been > allocated to a device. Great. Thanks for taking an interest. Does this mean I can apply your Reviewed-by? -- Lee Jones [李琼斯] Senior Technical Lead - Developer Services Linaro.org │ Open source software for Arm SoCs Follow Linaro: Facebook | Twitter | Blog
Re: [PATCH v2 1/3] mfd: core: Make a best effort attempt to match devices with the correct of_nodes
On 2020-06-24 02:46, Lee Jones wrote: > On Tue, 23 Jun 2020, Frank Rowand wrote: > >> On 2020-06-23 14:59, Lee Jones wrote: < big snip > Thanks for the replies in the above portion. But yes or no to my solution #2 (with some slight changes to make it better (more gracious handling of the detected error) as discussed elsewhere in the email thread)? >>> >>> Please see "[0]" above! >>> >>> AFAICT your solution #2 involves bombing out *all* devices if there is >>> a duplicate compatible with no 'reg' property value. This is a) >>> over-kill and b) not an error, as I mentioned: >> >> As I mentioned above, I set you up to have this misunderstanding by >> a mistake in one of my earlier emails. So now that I have pointed >> out what I meant here by "more gracious handling of the detected >> error", what do you think of my amended solution #2? > > Explained above, but the LT;DR is that it's not correct. I don't agree with you, I think my solution is better. Even if I prefer my solution, I find your solution to be good enough. So I am dropping my specific objection to returning -EAGAIN from mfd_match_of_node_to_dev() when the node has previously been allocated to a device. > > It also suffers with false positives. >>> >> >> Sorry for the very long response, but it seemed we were operating >> under some different understandings and I hope I have clarified some >> things. > > Likewise. :) >
Re: [PATCH v2 1/3] mfd: core: Make a best effort attempt to match devices with the correct of_nodes
On Wed, 24 Jun 2020, Michael Walle wrote: > Am 2020-06-24 10:23, schrieb Lee Jones: > > On Wed, 24 Jun 2020, Michael Walle wrote: > > [..] > > > > Although Rob mentioned to maybe relax that, but I sill fail to see > > > the advantage to have an arbitrary reg property instead of a unique > > > node name. > > > > I don't have a strong opinion either way. > > > > We can *also* add node name matching if Rob deems it fit. > > Where do you see a use of the reg property? The vast proportion of devices do and will have 'reg' properties. > You already expressed > that you see exposing the internal offset as a hack: > > "Placing "internal offsets" into the 'reg' property is a hack." [1] > > So what are you putting into reg instead? Rob suggested "anything" > documented in the hardware manual. But isn't this just also something > we make up and especially for the MFD driver. Thus IMHO it doesn't > qualify as a unit-address, which - as far as I understand it - is > unique on the parent bus. To repeat my argument, its not a defined > thing like an I2C address. So our argument in the past (although this is going back the best part of 10 years) has always been that; if devices are different, there is almost always a documented (in the H/W manual/datasheet) way to differentiate/address them. Whether that's a physical address, an offset, a bank ID, an I2C/SPI address or whatever. As to not being able to use that address/ID due to the DT rules surrounding address space as per the example in your previous email, well that's a rule specific to DT and makes little sense in some real world cases (such as, dare I say it, the AB8500). You'll have to take the aforementioned point and the point about using node names instead of 'reg' properties up with Rob and the other interested DT folk. Again, I'm happy to extend that functionality if it becomes acceptable practice. > [1] https://lore.kernel.org/linux-devicetree/20200609185231.GO4106@dell/ > > -michael -- Lee Jones [李琼斯] Senior Technical Lead - Developer Services Linaro.org │ Open source software for Arm SoCs Follow Linaro: Facebook | Twitter | Blog
Re: [PATCH v2 1/3] mfd: core: Make a best effort attempt to match devices with the correct of_nodes
Am 2020-06-24 10:23, schrieb Lee Jones: On Wed, 24 Jun 2020, Michael Walle wrote: [..] Although Rob mentioned to maybe relax that, but I sill fail to see the advantage to have an arbitrary reg property instead of a unique node name. I don't have a strong opinion either way. We can *also* add node name matching if Rob deems it fit. Where do you see a use of the reg property? You already expressed that you see exposing the internal offset as a hack: "Placing "internal offsets" into the 'reg' property is a hack." [1] So what are you putting into reg instead? Rob suggested "anything" documented in the hardware manual. But isn't this just also something we make up and especially for the MFD driver. Thus IMHO it doesn't qualify as a unit-address, which - as far as I understand it - is unique on the parent bus. To repeat my argument, its not a defined thing like an I2C address. [1] https://lore.kernel.org/linux-devicetree/20200609185231.GO4106@dell/ -michael
Re: [PATCH v2 1/3] mfd: core: Make a best effort attempt to match devices with the correct of_nodes
On Wed, 24 Jun 2020, Michael Walle wrote:
> Hi,
>
> Am 2020-06-24 08:41, schrieb Lee Jones:
> > 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.
> > >
> > > As you mentioned elsewhere in this thread, this series "fixes" the
> > > problem related to the "stericsson,ab8500-pwm" compatible.
> > >
> > > I know, I said I would drop discussion of that compatible, but bear
> > > with me for a second. :-)
> > >
> > > The "problem" is that the devices for multiple mfd child nodes with
> > > the same compatible value of "stericsson,ab8500-pwm" will all have
> > > a pointer to the first child node. At the moment the same child
> > > of_node being used by more than one device does not cause any
> > > incorrect behavior.
> > >
> > > Just in case the driver for "stericsson,ab8500-pwm" is modified
> > > in a way that the child of_node needs to be distinct for each
> > > device, and that changes gets back ported, it would be useful
> > > to have Fixes: tags for this patch series.
> > >
> > > So, at your discretion (and I'll let you worry about the correct
> > > Fixes: tag format), this series fixes:
> > >
> > > bad76991d7847b7877ae797cc79745d82ffd9120 mfd: Register ab8500
> > > devices using the newly DT:ed MFD API
> >
> > This patch isn't actually broken.
> >
> > The issue is the DTB, which [0] addresses.
> >
> > [0]
> > https://lkml.kernel.org/lkml/20200622083432.1491715-1-lee.jo...@linaro.org/
>
> Now, I'm confused; because this patch doesn't use the reg property
> but a different node name.
The fix mentioned above is orthogonal to this set.
The *only* reason for the differing node names there is to circumvent
the following DTC warnings:
arch/arm/boot/dts/ste-ab8500.dtsi:210.16-214.7: ERROR (duplicate_node_names):
/soc/prcmu@80157000/ab8500/ab8500-pwm: Duplicate node name
arch/arm/boot/dts/ste-ab8500.dtsi:216.16-220.7: ERROR (duplicate_node_names):
/soc/prcmu@80157000/ab8500/ab8500-pwm: Duplicate node name
arch/arm/boot/dts/ste-ab8500.dtsi:216.16-220.7: ERROR (duplicate_node_names):
/soc/prcmu@80157000/ab8500/ab8500-pwm: Duplicate node name
> I'd actually prefer this for any MFD
> driver which has multiple nodes of the same compatible string. See
> my reasoning here [1]. But until now, no one has responded. Thus,
> I'd rather see a OF_MFD_CELL_NAME() which matches the node name
> instead of the OF_MFD_CELL_REG() macro.
>
> This would also circumvent the fact that the unit-address has one
> number space. Eg. it is not possible to have:
>
> mfd {
> compatible = "mfd,compatible";
>
> gpio@0 {
> reg = <0>;
> };
> gpio@1 {
> reg = <1>;
> };
> pwm@0 {
> reg = <0>;
> };
> };
>
> Although Rob mentioned to maybe relax that, but I sill fail to see
> the advantage to have an arbitrary reg property instead of a unique
> node name.
I don't have a strong opinion either way.
We can *also* add node name matching if Rob deems it fit.
--
Lee Jones [李琼斯]
Senior Technical Lead - Developer Services
Linaro.org │ Open source software for Arm SoCs
Follow Linaro: Facebook | Twitter | Blog
Re: [PATCH v2 1/3] mfd: core: Make a best effort attempt to match devices with the correct of_nodes
Hi,
Am 2020-06-24 08:41, schrieb Lee Jones:
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.
As you mentioned elsewhere in this thread, this series "fixes" the
problem related to the "stericsson,ab8500-pwm" compatible.
I know, I said I would drop discussion of that compatible, but bear
with me for a second. :-)
The "problem" is that the devices for multiple mfd child nodes with
the same compatible value of "stericsson,ab8500-pwm" will all have
a pointer to the first child node. At the moment the same child
of_node being used by more than one device does not cause any
incorrect behavior.
Just in case the driver for "stericsson,ab8500-pwm" is modified
in a way that the child of_node needs to be distinct for each
device, and that changes gets back ported, it would be useful
to have Fixes: tags for this patch series.
So, at your discretion (and I'll let you worry about the correct
Fixes: tag format), this series fixes:
bad76991d7847b7877ae797cc79745d82ffd9120 mfd: Register ab8500 devices
using the newly DT:ed MFD API
This patch isn't actually broken.
The issue is the DTB, which [0] addresses.
[0]
https://lkml.kernel.org/lkml/20200622083432.1491715-1-lee.jo...@linaro.org/
Now, I'm confused; because this patch doesn't use the reg property
but a different node name. I'd actually prefer this for any MFD
driver which has multiple nodes of the same compatible string. See
my reasoning here [1]. But until now, no one has responded. Thus,
I'd rather see a OF_MFD_CELL_NAME() which matches the node name
instead of the OF_MFD_CELL_REG() macro.
This would also circumvent the fact that the unit-address has one
number space. Eg. it is not possible to have:
mfd {
compatible = "mfd,compatible";
gpio@0 {
reg = <0>;
};
gpio@1 {
reg = <1>;
};
pwm@0 {
reg = <0>;
};
};
Although Rob mentioned to maybe relax that, but I sill fail to see
the advantage to have an arbitrary reg property instead of a unique
node name.
[1]
https://lore.kernel.org/linux-devicetree/0709f20bc61afb6656bc57312eb69...@walle.cc/
--
-michael
Re: [PATCH v2 1/3] mfd: core: Make a best effort attempt to match devices with the correct of_nodes
On Tue, 23 Jun 2020, Frank Rowand wrote:
> On 2020-06-23 14:59, Lee Jones wrote:
> > Suggestion #2
> >
> >> 2) Modify patch 1/3. The small part of the patch to modify is:
> >>
> >> +static int mfd_match_of_node_to_dev(struct platform_device *pdev,
> >> + struct device_node *np,
> >> + const struct mfd_cell *cell)
> >> +{
> >> + struct mfd_of_node_entry *of_entry;
> >> + const __be32 *reg;
> >> + u64 of_node_addr;
> >> +
> >> + /* Skip devices 'disabled' by Device Tree */
> >> + if (!of_device_is_available(np))
> >> + return -ENODEV;
> >> +
> >> + /* Skip if OF node has previously been allocated to a device */
> >> + list_for_each_entry(of_entry, _of_node_list, list)
> >>
> >> Change:
> >>
> >> + if (of_entry->np == np)
> >> + return -EAGAIN;
> >>
> >> To:
> >>
> >> + if (of_entry->np == np) {
> >> + if (!cell->use_of_reg)
> >> + return -EINVAL;
> >> + else
> >> + return -EAGAIN;
> >>
> >> There may be a better choice than EINVAL, but I am just showing the
> >> method.
> >>
> >> You may also want to refactor this section of the patch slightly
> >> differently to achieve the same result. It was just easiest to
> >> show the suggested change the way I did it.
> >>
> >> The test that returns EINVAL detects the issue that the FDT does
> >> not match the binding (there is more one child node with the
> >> "stericsson,ab8500-pwm" compatible.
> >
> > My reply to suggestion #2
> >
> > So here, instead of just failing a single device, we fail everything?
> > Sounds a lot like throwing the baby out with the bath water. How is
> > that an improvement?
>
> I could have sworn that I had replied with a solution to this issue. So I
> searched and searched and searched my emails in the thread. And checked my
> drafts email folder. Then finally realized I had made a stupid mistake.
>
> I did reply about this, but I had put my "-Frank" signature at the end
> of a comment much higher in the email. So of course I expect you stopped
> reading at that point and never saw my answer. My apologies!!!
>
> The email in question is:
>
>
> https://lore.kernel.org/linux-devicetree/eae9cc00-e67a-cb6a-37c2-f2235782e...@gmail.com/
>
> and what I wrote at this point in that email is:
>
> You can modify more extensively than my simple example, changing
> mfd_add_device() to more gracefully handle an EINVAL returned by
> mfd_match_of_node_to_dev().
>
> Thus a modification to my suggestion #2 to make it _not_ fail
> everything.
>
> I didn't really flesh out all that "more gracefully handle" means.
> Among other things, it could include a pr_warn() that provides
> a fairly specific possible cause of the problem (eg the corner
> case mentioned near the end of the patch 1/3 header that shows
> mixing OF_MFD_CELL() and OF_MFD_CELL_REG() for the same compatible
> value. It may be tricky coming up with good phrasing in a pr_warn()
> that describes the generic issue instead of the specific example.
The current solution already provides a warning if we fail to match a
device with its requested OF node. It's also semantically incorrect
to error out just because a node with the same compatible string is
already taken, since there maybe another one coming up (which will be
found on the next iteration post return of -EAGAIN).
Providing a more accurate warning describing *why* a node wasn't found
it also non-trivial, for the same reasons as it's hard to do during a
pre-validation routine.
> >>> [0]
> >>
> >> Is "[0]" the patch series, especially patch 1/3?
> >
> > No, this is my reply to your suggestion #2.
> >
> > The [0] is referenced further down.
> >
> > [...]
> >
> > * False positives can occur and will fail as a result
>
> ((What is an example of a false positive?)) Never mind, now that
> I see that the previous issue is a fatal flaw, this becomes
> academic.
> >>>
> >>> That's okay, I don't mind discussing.
> >>>
> >>> Ironically, the 'ab8500-pwm' is a good example of a false positive,
> >>> since it's fine for the DT nodes to be identical. So long as there
> >>> are nodes present for each instance, it doesn't matter which one is
> >>> allocated to which device .Forcing a 'reg' property onto them for no>
> >>> good reason it not a valid solution here.
> >>
>
> Start of my comment that I wrote with "too many shortcuts" (see below):
>
> >> I thought that one of the points of this patch series was to add a
> >> "reg" property to any mfd child that was described by the
> >> OF_MFD_CELL_REG() macro.
> >
> > The OF_MFD_CELL_REG() macro didn't exist until this
Re: [PATCH v2 1/3] mfd: core: Make a best effort attempt to match devices with the correct of_nodes
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@ {
> > compatible = "mfd,of-test-child";
> > reg = <0x 0x 0 0x11>,
> > <0x 0x 0 0x22>;
> > };
> >
> > child@ {
> > compatible = "mfd,of-test-child";
> > reg = <0x 0x 0 0x33>;
> > };
> >
> > child@ {
> > compatible = "mfd,of-test-child";
> > reg = <0x 0x 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@
> > [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@
> > [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@
> >
> > 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@
> > [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@
> > [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@
> >
> > 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", 0x),
> > OF_MFD_CELL_REG("mfd-of-test-child", NULL, NULL, 0, 2,
> > "mfd,of-test-child", 0x),
> > OF_MFD_CELL_REG("mfd-of-test-child", NULL, NULL, 0, 3,
> > "mfd,of-test-child", 0x)
> > };
> >
> > 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@
> > [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@
> > [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@
> >
> > 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
Re: [PATCH v2 1/3] mfd: core: Make a best effort attempt to match devices with the correct of_nodes
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. > > As you mentioned elsewhere in this thread, this series "fixes" the > problem related to the "stericsson,ab8500-pwm" compatible. > > I know, I said I would drop discussion of that compatible, but bear > with me for a second. :-) > > The "problem" is that the devices for multiple mfd child nodes with > the same compatible value of "stericsson,ab8500-pwm" will all have > a pointer to the first child node. At the moment the same child > of_node being used by more than one device does not cause any > incorrect behavior. > > Just in case the driver for "stericsson,ab8500-pwm" is modified > in a way that the child of_node needs to be distinct for each > device, and that changes gets back ported, it would be useful > to have Fixes: tags for this patch series. > > So, at your discretion (and I'll let you worry about the correct > Fixes: tag format), this series fixes: > > bad76991d7847b7877ae797cc79745d82ffd9120 mfd: Register ab8500 devices using > the newly DT:ed MFD API This patch isn't actually broken. The issue is the DTB, which [0] addresses. [0] https://lkml.kernel.org/lkml/20200622083432.1491715-1-lee.jo...@linaro.org/ > c94bb233a9fee3314dc5d9c7de9fa702e91283f2 mfd: Make MFD core code Device Tree > and IRQ domain aware It sounds reasonable to list this one, thanks. -- Lee Jones [李琼斯] Senior Technical Lead - Developer Services Linaro.org │ Open source software for Arm SoCs Follow Linaro: Facebook | Twitter | Blog
Re: [PATCH v2 1/3] mfd: core: Make a best effort attempt to match devices with the correct of_nodes
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.
As you mentioned elsewhere in this thread, this series "fixes" the
problem related to the "stericsson,ab8500-pwm" compatible.
I know, I said I would drop discussion of that compatible, but bear
with me for a second. :-)
The "problem" is that the devices for multiple mfd child nodes with
the same compatible value of "stericsson,ab8500-pwm" will all have
a pointer to the first child node. At the moment the same child
of_node being used by more than one device does not cause any
incorrect behavior.
Just in case the driver for "stericsson,ab8500-pwm" is modified
in a way that the child of_node needs to be distinct for each
device, and that changes gets back ported, it would be useful
to have Fixes: tags for this patch series.
So, at your discretion (and I'll let you worry about the correct
Fixes: tag format), this series fixes:
bad76991d7847b7877ae797cc79745d82ffd9120 mfd: Register ab8500 devices using the
newly DT:ed MFD API
c94bb233a9fee3314dc5d9c7de9fa702e91283f2 mfd: Make MFD core code Device Tree
and IRQ domain aware
-Frank
>
> An example Device Tree entry might look like this:
>
> mfd_of_test {
> compatible = "mfd,of-test-parent";
> #address-cells = <0x02>;
> #size-cells = <0x02>;
>
> child@ {
> compatible = "mfd,of-test-child";
> reg = <0x 0x 0 0x11>,
> <0x 0x 0 0x22>;
> };
>
> child@ {
> compatible = "mfd,of-test-child";
> reg = <0x 0x 0 0x33>;
> };
>
> child@ {
> compatible = "mfd,of-test-child";
> reg = <0x 0x 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@
> [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@
> [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@
>
> 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@
> [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@
> [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@
>
> 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", 0x),
> OF_MFD_CELL_REG("mfd-of-test-child", NULL, NULL, 0, 2,
> "mfd,of-test-child", 0x),
> OF_MFD_CELL_REG("mfd-of-test-child", NULL, NULL, 0, 3,
> "mfd,of-test-child", 0x)
> };
>
> 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]
Re: [PATCH v2 1/3] mfd: core: Make a best effort attempt to match devices with the correct of_nodes
On 2020-06-23 14:59, Lee Jones wrote:
> Suggestion #2
>
>> 2) Modify patch 1/3. The small part of the patch to modify is:
>>
>> +static int mfd_match_of_node_to_dev(struct platform_device *pdev,
>> +struct device_node *np,
>> +const struct mfd_cell *cell)
>> +{
>> +struct mfd_of_node_entry *of_entry;
>> +const __be32 *reg;
>> +u64 of_node_addr;
>> +
>> +/* Skip devices 'disabled' by Device Tree */
>> +if (!of_device_is_available(np))
>> +return -ENODEV;
>> +
>> +/* Skip if OF node has previously been allocated to a device */
>> +list_for_each_entry(of_entry, _of_node_list, list)
>>
>> Change:
>>
>> +if (of_entry->np == np)
>> +return -EAGAIN;
>>
>> To:
>>
>> +if (of_entry->np == np) {
>> +if (!cell->use_of_reg)
>> +return -EINVAL;
>> +else
>> +return -EAGAIN;
>>
>> There may be a better choice than EINVAL, but I am just showing the
>> method.
>>
>> You may also want to refactor this section of the patch slightly
>> differently to achieve the same result. It was just easiest to
>> show the suggested change the way I did it.
>>
>> The test that returns EINVAL detects the issue that the FDT does
>> not match the binding (there is more one child node with the
>> "stericsson,ab8500-pwm" compatible.
>
> My reply to suggestion #2
>
> So here, instead of just failing a single device, we fail everything?
> Sounds a lot like throwing the baby out with the bath water. How is
> that an improvement?
I could have sworn that I had replied with a solution to this issue. So I
searched and searched and searched my emails in the thread. And checked my
drafts email folder. Then finally realized I had made a stupid mistake.
I did reply about this, but I had put my "-Frank" signature at the end
of a comment much higher in the email. So of course I expect you stopped
reading at that point and never saw my answer. My apologies!!!
The email in question is:
https://lore.kernel.org/linux-devicetree/eae9cc00-e67a-cb6a-37c2-f2235782e...@gmail.com/
and what I wrote at this point in that email is:
You can modify more extensively than my simple example, changing
mfd_add_device() to more gracefully handle an EINVAL returned by
mfd_match_of_node_to_dev().
Thus a modification to my suggestion #2 to make it _not_ fail
everything.
I didn't really flesh out all that "more gracefully handle" means.
Among other things, it could include a pr_warn() that provides
a fairly specific possible cause of the problem (eg the corner
case mentioned near the end of the patch 1/3 header that shows
mixing OF_MFD_CELL() and OF_MFD_CELL_REG() for the same compatible
value. It may be tricky coming up with good phrasing in a pr_warn()
that describes the generic issue instead of the specific example.
Again, sorry.
>>>
>>> [0]
>>
>> Is "[0]" the patch series, especially patch 1/3?
>
> No, this is my reply to your suggestion #2.
>
> The [0] is referenced further down.
>
> [...]
>
> * False positives can occur and will fail as a result
((What is an example of a false positive?)) Never mind, now that
I see that the previous issue is a fatal flaw, this becomes
academic.
>>>
>>> That's okay, I don't mind discussing.
>>>
>>> Ironically, the 'ab8500-pwm' is a good example of a false positive,
>>> since it's fine for the DT nodes to be identical. So long as there
>>> are nodes present for each instance, it doesn't matter which one is
>>> allocated to which device .Forcing a 'reg' property onto them for no> good
>>> reason it not a valid solution here.
>>
Start of my comment that I wrote with "too many shortcuts" (see below):
>> I thought that one of the points of this patch series was to add a
>> "reg" property to any mfd child that was described by the
>> OF_MFD_CELL_REG() macro.
>
> The OF_MFD_CELL_REG() macro didn't exist until this patch-set.
Maybe the way I wrote that took too many shortcuts. Let me re-phrase.
I thought that one of the points of this patch set was to add the
of_reg and use_of_reg fields to struct mfd_cell. The expected use
of the of_reg and use_of_reg fields was to allow the presence of a
"reg" property in a devicetree mfd child node to be used to match
specific child nodes to specific elements of the mfd_add_devices()
cell array parameter, with the match occurring when the array elements
are processed (currently in mfd_match_of_node_to_dev(), which is
called by mfd_add_device()).
The key point being the matching specific devicetree mfd child nodes
to specific cell array members.
The
Re: [PATCH v2 1/3] mfd: core: Make a best effort attempt to match devices with the correct of_nodes
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@ {
> compatible = "mfd,of-test-child";
> reg = <0x 0x 0 0x11>,
> <0x 0x 0 0x22>;
> };
>
> child@ {
> compatible = "mfd,of-test-child";
> reg = <0x 0x 0 0x33>;
> };
>
> child@ {
> compatible = "mfd,of-test-child";
> reg = <0x 0x 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@
> [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@
> [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@
>
> 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@
> [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@
> [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@
>
> 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", 0x),
> OF_MFD_CELL_REG("mfd-of-test-child", NULL, NULL, 0, 2,
> "mfd,of-test-child", 0x),
> OF_MFD_CELL_REG("mfd-of-test-child", NULL, NULL, 0, 3,
> "mfd,of-test-child", 0x)
> };
>
> 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@
> [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@
> [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@
>
> 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
Re: [PATCH v2 1/3] mfd: core: Make a best effort attempt to match devices with the correct of_nodes
Suggestion #2
> 2) Modify patch 1/3. The small part of the patch to modify is:
>
> +static int mfd_match_of_node_to_dev(struct platform_device *pdev,
> +struct device_node *np,
> +const struct mfd_cell *cell)
> +{
> +struct mfd_of_node_entry *of_entry;
> +const __be32 *reg;
> +u64 of_node_addr;
> +
> +/* Skip devices 'disabled' by Device Tree */
> +if (!of_device_is_available(np))
> +return -ENODEV;
> +
> +/* Skip if OF node has previously been allocated to a device */
> +list_for_each_entry(of_entry, _of_node_list, list)
>
> Change:
>
> +if (of_entry->np == np)
> +return -EAGAIN;
>
> To:
>
> +if (of_entry->np == np) {
> +if (!cell->use_of_reg)
> +return -EINVAL;
> +else
> +return -EAGAIN;
>
> There may be a better choice than EINVAL, but I am just showing the
> method.
>
> You may also want to refactor this section of the patch slightly
> differently to achieve the same result. It was just easiest to
> show the suggested change the way I did it.
>
> The test that returns EINVAL detects the issue that the FDT does
> not match the binding (there is more one child node with the
> "stericsson,ab8500-pwm" compatible.
My reply to suggestion #2
> >>> So here, instead of just failing a single device, we fail everything?
> >>> Sounds a lot like throwing the baby out with the bath water. How is
> >>> that an improvement?
> >
> > [0]
>
> Is "[0]" the patch series, especially patch 1/3?
No, this is my reply to your suggestion #2.
The [0] is referenced further down.
[...]
> >>> * False positives can occur and will fail as a result
> >>
> >> ((What is an example of a false positive?)) Never mind, now that
> >> I see that the previous issue is a fatal flaw, this becomes
> >> academic.
> >
> > That's okay, I don't mind discussing.
> >
> > Ironically, the 'ab8500-pwm' is a good example of a false positive,
> > since it's fine for the DT nodes to be identical. So long as there
> > are nodes present for each instance, it doesn't matter which one is
> > allocated to which device .Forcing a 'reg' property onto them for no> good
> > reason it not a valid solution here.
>
> I thought that one of the points of this patch series was to add a
> "reg" property to any mfd child that was described by the
> OF_MFD_CELL_REG() macro.
The OF_MFD_CELL_REG() macro didn't exist until this patch-set.
There are currently no users.
> And that was meant to fix the problem where multiple indistinguishable
> children existed. The only instance I found of that (using the
> weak search on OF_MFD_CELL()) was of compatible "stericsson,ab8500-pwm"
> in drivers/mfd/ab8500-core.c. You agreed with my email that
> reported that.
No, I agreed with you that there is a current problem with
"stericsson,ab8500-pwm", as identified by Michael. I didn't actually
know about this issue until *after* drafting this patch-set. To be
clear the "stericsson,ab8500-pwm" scenario is not the reason for this
set's existence.
Also, please forget about the OF_MFD_* macros, they are totally
agnostic to this effort. The only relevance they have here is the
addition of 1 extra macro which *could* be used to provide the 'reg'
property where appropriate.
> So I thought that drivers/mfd/ab8500-core.c would be modified to
> replace the multiple instances of compatible "stericsson,ab8500-pwm"
> in OF_MFD_CELL() with OF_MFD_CELL_REG().
That is not my vision. There is no need for "stericsson,ab8500-pwm"
to have 'reg' properties as far as I see it.
> This is another problem with the patch series: there is no user
> of OF_MFD_CELL_REG(). Please add one to the series.
That's not a problem with this patch-set, it's a problem with your
understanding of this patch-set. :)
As far as I know, there aren't any current users who would benefit
from this work. Instead, it is designed to provide future submitters
with another tool to help them link their child devices to the correct
OF nodes. That's not to say that current users can't and won't
benefit from this. Just that they are not the target audience.
> >>> The above actually makes the solution worse, not better.
> >>>
> >>
> >> Patch 1/3 silently fails to deal with a broken devicetree.
> >> It results on one of the three ab8500-pwm child nodes in
> >> the hypothetical devicetree source tree not being added.
> >>
> >> That is not a good result either.
> >
> > No it doesn't. In the case of 'ab8500-pwm' the OF node is not set for
> > 2 of the devices and warnings are presented in the kernel log.
>
>
Re: [PATCH v2 1/3] mfd: core: Make a best effort attempt to match devices with the correct of_nodes
On 2020-06-23 01:47, Lee Jones wrote:
> On Mon, 22 Jun 2020, Frank Rowand wrote:
>
>> On 2020-06-22 14:11, Lee Jones wrote:
>>> On Mon, 22 Jun 2020, Frank Rowand wrote:
>>>
On 2020-06-22 10:10, Lee Jones wrote:
> On Mon, 22 Jun 2020, Frank Rowand wrote:
>
>> On 2020-06-22 03:50, Lee Jones wrote:
>>> On Thu, 18 Jun 2020, Frank Rowand wrote:
>>>
On 2020-06-15 04:26, Lee Jones wrote:
> On Sun, 14 Jun 2020, Frank Rowand wrote:
>
>> Hi Lee,
>>
>> I'm looking at 5.8-rc1.
>>
>> The only use of OF_MFD_CELL() where the same compatible is specified
>> for multiple elements of a struct mfd_cell array is for compatible
>> "stericsson,ab8500-pwm" in drivers/mfd/ab8500-core.c:
>>
>> OF_MFD_CELL("ab8500-pwm",
>> NULL, NULL, 0, 1, "stericsson,ab8500-pwm"),
>> OF_MFD_CELL("ab8500-pwm",
>> NULL, NULL, 0, 2, "stericsson,ab8500-pwm"),
>> OF_MFD_CELL("ab8500-pwm",
>> NULL, NULL, 0, 3, "stericsson,ab8500-pwm"),
OF_MFD_CELL("ab8500-pwm",
NULL, NULL, 0, 0, "stericsson,ab8500-pwm"),
OF_MFD_CELL_REG("ab8500-pwm-mc",
NULL, NULL, 0, 0, "stericsson,ab8500-pwm", 0),
OF_MFD_CELL_REG("ab8500-pwm-mc",
NULL, NULL, 0, 1, "stericsson,ab8500-pwm", 1),
OF_MFD_CELL_REG("ab8500-pwm-mc",
NULL, NULL, 0, 2, "stericsson,ab8500-pwm", 2),
>>
>> The only .dts or .dtsi files where I see compatible
>> "stericsson,ab8500-pwm"
>> are:
>>
>>arch/arm/boot/dts/ste-ab8500.dtsi
>>arch/arm/boot/dts/ste-ab8505.dtsi
>>
>> These two .dtsi files only have a single node with this compatible.
>> Chasing back to .dts and .dtsi files that include these two .dtsi
>> files, I see no case where there are multiple nodes with this
>> compatible.
>>
>> So it looks to me like there is no .dts in mainline that is providing
>> the three "stericsson,ab8500-pwm" nodes that
>> drivers/mfd/ab8500-core.c
>> is expecting. No case that there are multiple mfd child nodes where
>> mfd_add_device() would assign the first of n child nodes with the
>> same compatible to multiple devices.
>>
>> So it appears to me that drivers/mfd/ab8500-core.c is currently
>> broken.
>> Am I missing something here?
>>
>> If I am correct, then either drivers/mfd/ab8500-core.c or
>> ste-ab8500.dtsi and ste-ab8505.dtsi need to be fixed.
>
> Your analysis is correct.
OK, if I'm not overlooking anything, that is good news.
Existing .dts source files only have one "ab8500-pwm" child. They
already
work correcly.
Create a new compatible for the case of multiple children. In my
example
I will add "-mc" (multiple children) to the existing compatible. There
is likely a better name, but this lets me provide an example.
Modify drivers/mfd/ab8500-core.c to use the new compatible, and new
.dts
source files with multiple children use the new compatible:
OF_MFD_CELL("ab8500-pwm",
NULL, NULL, 0, 0, "stericsson,ab8500-pwm"),
OF_MFD_CELL_REG("ab8500-pwm-mc",
NULL, NULL, 0, 0, "stericsson,ab8500-pwm", 0),
OF_MFD_CELL_REG("ab8500-pwm-mc",
NULL, NULL, 0, 1, "stericsson,ab8500-pwm", 1),
OF_MFD_CELL_REG("ab8500-pwm-mc",
NULL, NULL, 0, 2, "stericsson,ab8500-pwm", 2),
The "OF_MFD_CELL" entry is the existing entry, which will handle
current
.dts source files. The new "OF_MFD_CELL_REG" entries will handle new
.dts source files.
>>>
>>> Sorry, but I'm not sure what the above exercise is supposed to solve.
>>>
>>> Could you explain it for me please?
>>
>> The OF_MFD_CELL() entry handles all of the existing .dts source files
>> that only have one ab8500-pwm child nodes. So existing .dtb blobs
>> continue to work.
>>
>> The OF_MFD_CELL_REG() entries will handle all of the new .dts source
>> files that will have up to 3 ab8500-pwm child nodes.
>>
>> Compatibility is maintained for existing .dtb files. A new kernel
>> version with the changes will support new .dtb files that contain
>> multiple ab8500-pwm child nodes.
>
> I can see *what* you're trying to do. I was
Re: [PATCH v2 1/3] mfd: core: Make a best effort attempt to match devices with the correct of_nodes
On Mon, 22 Jun 2020, Frank Rowand wrote:
> On 2020-06-22 14:11, Lee Jones wrote:
> > On Mon, 22 Jun 2020, Frank Rowand wrote:
> >
> >> On 2020-06-22 10:10, Lee Jones wrote:
> >>> On Mon, 22 Jun 2020, Frank Rowand wrote:
> >>>
> On 2020-06-22 03:50, Lee Jones wrote:
> > On Thu, 18 Jun 2020, Frank Rowand wrote:
> >
> >> On 2020-06-15 04:26, Lee Jones wrote:
> >>> On Sun, 14 Jun 2020, Frank Rowand wrote:
> >>>
> Hi Lee,
>
> I'm looking at 5.8-rc1.
>
> The only use of OF_MFD_CELL() where the same compatible is specified
> for multiple elements of a struct mfd_cell array is for compatible
> "stericsson,ab8500-pwm" in drivers/mfd/ab8500-core.c:
>
> OF_MFD_CELL("ab8500-pwm",
> NULL, NULL, 0, 1, "stericsson,ab8500-pwm"),
> OF_MFD_CELL("ab8500-pwm",
> NULL, NULL, 0, 2, "stericsson,ab8500-pwm"),
> OF_MFD_CELL("ab8500-pwm",
> NULL, NULL, 0, 3, "stericsson,ab8500-pwm"),
> >>
> >> OF_MFD_CELL("ab8500-pwm",
> >> NULL, NULL, 0, 0, "stericsson,ab8500-pwm"),
> >>
> >> OF_MFD_CELL_REG("ab8500-pwm-mc",
> >> NULL, NULL, 0, 0, "stericsson,ab8500-pwm", 0),
> >> OF_MFD_CELL_REG("ab8500-pwm-mc",
> >> NULL, NULL, 0, 1, "stericsson,ab8500-pwm", 1),
> >> OF_MFD_CELL_REG("ab8500-pwm-mc",
> >> NULL, NULL, 0, 2, "stericsson,ab8500-pwm", 2),
> >>
>
> The only .dts or .dtsi files where I see compatible
> "stericsson,ab8500-pwm"
> are:
>
> arch/arm/boot/dts/ste-ab8500.dtsi
> arch/arm/boot/dts/ste-ab8505.dtsi
>
> These two .dtsi files only have a single node with this compatible.
> Chasing back to .dts and .dtsi files that include these two .dtsi
> files, I see no case where there are multiple nodes with this
> compatible.
>
> So it looks to me like there is no .dts in mainline that is providing
> the three "stericsson,ab8500-pwm" nodes that
> drivers/mfd/ab8500-core.c
> is expecting. No case that there are multiple mfd child nodes where
> mfd_add_device() would assign the first of n child nodes with the
> same compatible to multiple devices.
>
> So it appears to me that drivers/mfd/ab8500-core.c is currently
> broken.
> Am I missing something here?
>
> If I am correct, then either drivers/mfd/ab8500-core.c or
> ste-ab8500.dtsi and ste-ab8505.dtsi need to be fixed.
> >>>
> >>> Your analysis is correct.
> >>
> >> OK, if I'm not overlooking anything, that is good news.
> >>
> >> Existing .dts source files only have one "ab8500-pwm" child. They
> >> already
> >> work correcly.
> >>
> >> Create a new compatible for the case of multiple children. In my
> >> example
> >> I will add "-mc" (multiple children) to the existing compatible. There
> >> is likely a better name, but this lets me provide an example.
> >>
> >> Modify drivers/mfd/ab8500-core.c to use the new compatible, and new
> >> .dts
> >> source files with multiple children use the new compatible:
> >>
> >> OF_MFD_CELL("ab8500-pwm",
> >> NULL, NULL, 0, 0, "stericsson,ab8500-pwm"),
> >>
> >> OF_MFD_CELL_REG("ab8500-pwm-mc",
> >> NULL, NULL, 0, 0, "stericsson,ab8500-pwm", 0),
> >> OF_MFD_CELL_REG("ab8500-pwm-mc",
> >> NULL, NULL, 0, 1, "stericsson,ab8500-pwm", 1),
> >> OF_MFD_CELL_REG("ab8500-pwm-mc",
> >> NULL, NULL, 0, 2, "stericsson,ab8500-pwm", 2),
> >>
> >> The "OF_MFD_CELL" entry is the existing entry, which will handle
> >> current
> >> .dts source files. The new "OF_MFD_CELL_REG" entries will handle new
> >> .dts source files.
> >
> > Sorry, but I'm not sure what the above exercise is supposed to solve.
> >
> > Could you explain it for me please?
>
> The OF_MFD_CELL() entry handles all of the existing .dts source files
> that only have one ab8500-pwm child nodes. So existing .dtb blobs
> continue to work.
>
> The OF_MFD_CELL_REG() entries will handle all of the new .dts source
> files that will have up to 3 ab8500-pwm child nodes.
>
> Compatibility is maintained for existing .dtb files. A new kernel
> version with the changes will support new .dtb files that contain
> multiple ab8500-pwm child nodes.
> >>>
> >>> I can see *what* you're trying to do. I was looking for an
> >>> explanation of *how* you
Re: [PATCH v2 1/3] mfd: core: Make a best effort attempt to match devices with the correct of_nodes
On 2020-06-22 20:17, Frank Rowand wrote:
> On 2020-06-22 17:23, Frank Rowand wrote:
>> On 2020-06-22 14:11, Lee Jones wrote:
>>> On Mon, 22 Jun 2020, Frank Rowand wrote:
>>>
On 2020-06-22 10:10, Lee Jones wrote:
> On Mon, 22 Jun 2020, Frank Rowand wrote:
>
>> On 2020-06-22 03:50, Lee Jones wrote:
>>> On Thu, 18 Jun 2020, Frank Rowand wrote:
>>>
On 2020-06-15 04:26, Lee Jones wrote:
> On Sun, 14 Jun 2020, Frank Rowand wrote:
>
>> Hi Lee,
>>
>> I'm looking at 5.8-rc1.
>>
>> The only use of OF_MFD_CELL() where the same compatible is specified
>> for multiple elements of a struct mfd_cell array is for compatible
>> "stericsson,ab8500-pwm" in drivers/mfd/ab8500-core.c:
>>
>> OF_MFD_CELL("ab8500-pwm",
>> NULL, NULL, 0, 1, "stericsson,ab8500-pwm"),
>> OF_MFD_CELL("ab8500-pwm",
>> NULL, NULL, 0, 2, "stericsson,ab8500-pwm"),
>> OF_MFD_CELL("ab8500-pwm",
>> NULL, NULL, 0, 3, "stericsson,ab8500-pwm"),
OF_MFD_CELL("ab8500-pwm",
NULL, NULL, 0, 0, "stericsson,ab8500-pwm"),
OF_MFD_CELL_REG("ab8500-pwm-mc",
NULL, NULL, 0, 0, "stericsson,ab8500-pwm", 0),
OF_MFD_CELL_REG("ab8500-pwm-mc",
NULL, NULL, 0, 1, "stericsson,ab8500-pwm", 1),
OF_MFD_CELL_REG("ab8500-pwm-mc",
NULL, NULL, 0, 2, "stericsson,ab8500-pwm", 2),
>>
>> The only .dts or .dtsi files where I see compatible
>> "stericsson,ab8500-pwm"
>> are:
>>
>>arch/arm/boot/dts/ste-ab8500.dtsi
>>arch/arm/boot/dts/ste-ab8505.dtsi
>>
>> These two .dtsi files only have a single node with this compatible.
>> Chasing back to .dts and .dtsi files that include these two .dtsi
>> files, I see no case where there are multiple nodes with this
>> compatible.
>>
>> So it looks to me like there is no .dts in mainline that is providing
>> the three "stericsson,ab8500-pwm" nodes that
>> drivers/mfd/ab8500-core.c
>> is expecting. No case that there are multiple mfd child nodes where
>> mfd_add_device() would assign the first of n child nodes with the
>> same compatible to multiple devices.
>>
>> So it appears to me that drivers/mfd/ab8500-core.c is currently
>> broken.
>> Am I missing something here?
>>
>> If I am correct, then either drivers/mfd/ab8500-core.c or
>> ste-ab8500.dtsi and ste-ab8505.dtsi need to be fixed.
>
> Your analysis is correct.
OK, if I'm not overlooking anything, that is good news.
Existing .dts source files only have one "ab8500-pwm" child. They
already
work correcly.
Create a new compatible for the case of multiple children. In my
example
I will add "-mc" (multiple children) to the existing compatible. There
is likely a better name, but this lets me provide an example.
Modify drivers/mfd/ab8500-core.c to use the new compatible, and new
.dts
source files with multiple children use the new compatible:
OF_MFD_CELL("ab8500-pwm",
NULL, NULL, 0, 0, "stericsson,ab8500-pwm"),
OF_MFD_CELL_REG("ab8500-pwm-mc",
NULL, NULL, 0, 0, "stericsson,ab8500-pwm", 0),
OF_MFD_CELL_REG("ab8500-pwm-mc",
NULL, NULL, 0, 1, "stericsson,ab8500-pwm", 1),
OF_MFD_CELL_REG("ab8500-pwm-mc",
NULL, NULL, 0, 2, "stericsson,ab8500-pwm", 2),
The "OF_MFD_CELL" entry is the existing entry, which will handle
current
.dts source files. The new "OF_MFD_CELL_REG" entries will handle new
.dts source files.
>>>
>>> Sorry, but I'm not sure what the above exercise is supposed to solve.
>>>
>>> Could you explain it for me please?
>>
>> The OF_MFD_CELL() entry handles all of the existing .dts source files
>> that only have one ab8500-pwm child nodes. So existing .dtb blobs
>> continue to work.
>>
>> The OF_MFD_CELL_REG() entries will handle all of the new .dts source
>> files that will have up to 3 ab8500-pwm child nodes.
>>
>> Compatibility is maintained for existing .dtb files. A new kernel
>> version with the changes will support new .dtb files that contain
>> multiple ab8500-pwm child nodes.
>
> I can see *what* you're trying to do. I was
Re: [PATCH v2 1/3] mfd: core: Make a best effort attempt to match devices with the correct of_nodes
On 2020-06-22 17:23, Frank Rowand wrote:
> On 2020-06-22 14:11, Lee Jones wrote:
>> On Mon, 22 Jun 2020, Frank Rowand wrote:
>>
>>> On 2020-06-22 10:10, Lee Jones wrote:
On Mon, 22 Jun 2020, Frank Rowand wrote:
> On 2020-06-22 03:50, Lee Jones wrote:
>> On Thu, 18 Jun 2020, Frank Rowand wrote:
>>
>>> On 2020-06-15 04:26, Lee Jones wrote:
On Sun, 14 Jun 2020, Frank Rowand wrote:
> Hi Lee,
>
> I'm looking at 5.8-rc1.
>
> The only use of OF_MFD_CELL() where the same compatible is specified
> for multiple elements of a struct mfd_cell array is for compatible
> "stericsson,ab8500-pwm" in drivers/mfd/ab8500-core.c:
>
> OF_MFD_CELL("ab8500-pwm",
> NULL, NULL, 0, 1, "stericsson,ab8500-pwm"),
> OF_MFD_CELL("ab8500-pwm",
> NULL, NULL, 0, 2, "stericsson,ab8500-pwm"),
> OF_MFD_CELL("ab8500-pwm",
> NULL, NULL, 0, 3, "stericsson,ab8500-pwm"),
>>>
>>> OF_MFD_CELL("ab8500-pwm",
>>> NULL, NULL, 0, 0, "stericsson,ab8500-pwm"),
>>>
>>> OF_MFD_CELL_REG("ab8500-pwm-mc",
>>> NULL, NULL, 0, 0, "stericsson,ab8500-pwm", 0),
>>> OF_MFD_CELL_REG("ab8500-pwm-mc",
>>> NULL, NULL, 0, 1, "stericsson,ab8500-pwm", 1),
>>> OF_MFD_CELL_REG("ab8500-pwm-mc",
>>> NULL, NULL, 0, 2, "stericsson,ab8500-pwm", 2),
>>>
>
> The only .dts or .dtsi files where I see compatible
> "stericsson,ab8500-pwm"
> are:
>
>arch/arm/boot/dts/ste-ab8500.dtsi
>arch/arm/boot/dts/ste-ab8505.dtsi
>
> These two .dtsi files only have a single node with this compatible.
> Chasing back to .dts and .dtsi files that include these two .dtsi
> files, I see no case where there are multiple nodes with this
> compatible.
>
> So it looks to me like there is no .dts in mainline that is providing
> the three "stericsson,ab8500-pwm" nodes that drivers/mfd/ab8500-core.c
> is expecting. No case that there are multiple mfd child nodes where
> mfd_add_device() would assign the first of n child nodes with the
> same compatible to multiple devices.
>
> So it appears to me that drivers/mfd/ab8500-core.c is currently
> broken.
> Am I missing something here?
>
> If I am correct, then either drivers/mfd/ab8500-core.c or
> ste-ab8500.dtsi and ste-ab8505.dtsi need to be fixed.
Your analysis is correct.
>>>
>>> OK, if I'm not overlooking anything, that is good news.
>>>
>>> Existing .dts source files only have one "ab8500-pwm" child. They
>>> already
>>> work correcly.
>>>
>>> Create a new compatible for the case of multiple children. In my
>>> example
>>> I will add "-mc" (multiple children) to the existing compatible. There
>>> is likely a better name, but this lets me provide an example.
>>>
>>> Modify drivers/mfd/ab8500-core.c to use the new compatible, and new .dts
>>> source files with multiple children use the new compatible:
>>>
>>> OF_MFD_CELL("ab8500-pwm",
>>> NULL, NULL, 0, 0, "stericsson,ab8500-pwm"),
>>>
>>> OF_MFD_CELL_REG("ab8500-pwm-mc",
>>> NULL, NULL, 0, 0, "stericsson,ab8500-pwm", 0),
>>> OF_MFD_CELL_REG("ab8500-pwm-mc",
>>> NULL, NULL, 0, 1, "stericsson,ab8500-pwm", 1),
>>> OF_MFD_CELL_REG("ab8500-pwm-mc",
>>> NULL, NULL, 0, 2, "stericsson,ab8500-pwm", 2),
>>>
>>> The "OF_MFD_CELL" entry is the existing entry, which will handle current
>>> .dts source files. The new "OF_MFD_CELL_REG" entries will handle new
>>> .dts source files.
>>
>> Sorry, but I'm not sure what the above exercise is supposed to solve.
>>
>> Could you explain it for me please?
>
> The OF_MFD_CELL() entry handles all of the existing .dts source files
> that only have one ab8500-pwm child nodes. So existing .dtb blobs
> continue to work.
>
> The OF_MFD_CELL_REG() entries will handle all of the new .dts source
> files that will have up to 3 ab8500-pwm child nodes.
>
> Compatibility is maintained for existing .dtb files. A new kernel
> version with the changes will support new .dtb files that contain
> multiple ab8500-pwm child nodes.
I can see *what* you're trying to do. I was looking for an
explanation of *how* you think that will work. FWIW, I don't think
what you're proposing will work as you envisage. I thought that
perhaps I was missing
Re: [PATCH v2 1/3] mfd: core: Make a best effort attempt to match devices with the correct of_nodes
On 2020-06-22 14:11, Lee Jones wrote:
> On Mon, 22 Jun 2020, Frank Rowand wrote:
>
>> On 2020-06-22 10:10, Lee Jones wrote:
>>> On Mon, 22 Jun 2020, Frank Rowand wrote:
>>>
On 2020-06-22 03:50, Lee Jones wrote:
> On Thu, 18 Jun 2020, Frank Rowand wrote:
>
>> On 2020-06-15 04:26, Lee Jones wrote:
>>> On Sun, 14 Jun 2020, Frank Rowand wrote:
>>>
Hi Lee,
I'm looking at 5.8-rc1.
The only use of OF_MFD_CELL() where the same compatible is specified
for multiple elements of a struct mfd_cell array is for compatible
"stericsson,ab8500-pwm" in drivers/mfd/ab8500-core.c:
OF_MFD_CELL("ab8500-pwm",
NULL, NULL, 0, 1, "stericsson,ab8500-pwm"),
OF_MFD_CELL("ab8500-pwm",
NULL, NULL, 0, 2, "stericsson,ab8500-pwm"),
OF_MFD_CELL("ab8500-pwm",
NULL, NULL, 0, 3, "stericsson,ab8500-pwm"),
>>
>> OF_MFD_CELL("ab8500-pwm",
>> NULL, NULL, 0, 0, "stericsson,ab8500-pwm"),
>>
>> OF_MFD_CELL_REG("ab8500-pwm-mc",
>> NULL, NULL, 0, 0, "stericsson,ab8500-pwm", 0),
>> OF_MFD_CELL_REG("ab8500-pwm-mc",
>> NULL, NULL, 0, 1, "stericsson,ab8500-pwm", 1),
>> OF_MFD_CELL_REG("ab8500-pwm-mc",
>> NULL, NULL, 0, 2, "stericsson,ab8500-pwm", 2),
>>
The only .dts or .dtsi files where I see compatible
"stericsson,ab8500-pwm"
are:
arch/arm/boot/dts/ste-ab8500.dtsi
arch/arm/boot/dts/ste-ab8505.dtsi
These two .dtsi files only have a single node with this compatible.
Chasing back to .dts and .dtsi files that include these two .dtsi
files, I see no case where there are multiple nodes with this
compatible.
So it looks to me like there is no .dts in mainline that is providing
the three "stericsson,ab8500-pwm" nodes that drivers/mfd/ab8500-core.c
is expecting. No case that there are multiple mfd child nodes where
mfd_add_device() would assign the first of n child nodes with the
same compatible to multiple devices.
So it appears to me that drivers/mfd/ab8500-core.c is currently broken.
Am I missing something here?
If I am correct, then either drivers/mfd/ab8500-core.c or
ste-ab8500.dtsi and ste-ab8505.dtsi need to be fixed.
>>>
>>> Your analysis is correct.
>>
>> OK, if I'm not overlooking anything, that is good news.
>>
>> Existing .dts source files only have one "ab8500-pwm" child. They
>> already
>> work correcly.
>>
>> Create a new compatible for the case of multiple children. In my example
>> I will add "-mc" (multiple children) to the existing compatible. There
>> is likely a better name, but this lets me provide an example.
>>
>> Modify drivers/mfd/ab8500-core.c to use the new compatible, and new .dts
>> source files with multiple children use the new compatible:
>>
>> OF_MFD_CELL("ab8500-pwm",
>> NULL, NULL, 0, 0, "stericsson,ab8500-pwm"),
>>
>> OF_MFD_CELL_REG("ab8500-pwm-mc",
>> NULL, NULL, 0, 0, "stericsson,ab8500-pwm", 0),
>> OF_MFD_CELL_REG("ab8500-pwm-mc",
>> NULL, NULL, 0, 1, "stericsson,ab8500-pwm", 1),
>> OF_MFD_CELL_REG("ab8500-pwm-mc",
>> NULL, NULL, 0, 2, "stericsson,ab8500-pwm", 2),
>>
>> The "OF_MFD_CELL" entry is the existing entry, which will handle current
>> .dts source files. The new "OF_MFD_CELL_REG" entries will handle new
>> .dts source files.
>
> Sorry, but I'm not sure what the above exercise is supposed to solve.
>
> Could you explain it for me please?
The OF_MFD_CELL() entry handles all of the existing .dts source files
that only have one ab8500-pwm child nodes. So existing .dtb blobs
continue to work.
The OF_MFD_CELL_REG() entries will handle all of the new .dts source
files that will have up to 3 ab8500-pwm child nodes.
Compatibility is maintained for existing .dtb files. A new kernel
version with the changes will support new .dtb files that contain
multiple ab8500-pwm child nodes.
>>>
>>> I can see *what* you're trying to do. I was looking for an
>>> explanation of *how* you think that will work. FWIW, I don't think
>>> what you're proposing will work as you envisage. I thought that
>>> perhaps I was missing something, which is why I requested further
>>> explanation.
>>>
>> And of course the patch that creates OF_MFD_CELL_REG() needs to precede
>> this change.
>>
Re: [PATCH v2 1/3] mfd: core: Make a best effort attempt to match devices with the correct of_nodes
On Mon, 22 Jun 2020, Frank Rowand wrote:
> On 2020-06-22 10:10, Lee Jones wrote:
> > On Mon, 22 Jun 2020, Frank Rowand wrote:
> >
> >> On 2020-06-22 03:50, Lee Jones wrote:
> >>> On Thu, 18 Jun 2020, Frank Rowand wrote:
> >>>
> On 2020-06-15 04:26, Lee Jones wrote:
> > On Sun, 14 Jun 2020, Frank Rowand wrote:
> >
> >> Hi Lee,
> >>
> >> I'm looking at 5.8-rc1.
> >>
> >> The only use of OF_MFD_CELL() where the same compatible is specified
> >> for multiple elements of a struct mfd_cell array is for compatible
> >> "stericsson,ab8500-pwm" in drivers/mfd/ab8500-core.c:
> >>
> >> OF_MFD_CELL("ab8500-pwm",
> >> NULL, NULL, 0, 1, "stericsson,ab8500-pwm"),
> >> OF_MFD_CELL("ab8500-pwm",
> >> NULL, NULL, 0, 2, "stericsson,ab8500-pwm"),
> >> OF_MFD_CELL("ab8500-pwm",
> >> NULL, NULL, 0, 3, "stericsson,ab8500-pwm"),
>
> OF_MFD_CELL("ab8500-pwm",
> NULL, NULL, 0, 0, "stericsson,ab8500-pwm"),
>
> OF_MFD_CELL_REG("ab8500-pwm-mc",
> NULL, NULL, 0, 0, "stericsson,ab8500-pwm", 0),
> OF_MFD_CELL_REG("ab8500-pwm-mc",
> NULL, NULL, 0, 1, "stericsson,ab8500-pwm", 1),
> OF_MFD_CELL_REG("ab8500-pwm-mc",
> NULL, NULL, 0, 2, "stericsson,ab8500-pwm", 2),
>
> >>
> >> The only .dts or .dtsi files where I see compatible
> >> "stericsson,ab8500-pwm"
> >> are:
> >>
> >>arch/arm/boot/dts/ste-ab8500.dtsi
> >>arch/arm/boot/dts/ste-ab8505.dtsi
> >>
> >> These two .dtsi files only have a single node with this compatible.
> >> Chasing back to .dts and .dtsi files that include these two .dtsi
> >> files, I see no case where there are multiple nodes with this
> >> compatible.
> >>
> >> So it looks to me like there is no .dts in mainline that is providing
> >> the three "stericsson,ab8500-pwm" nodes that drivers/mfd/ab8500-core.c
> >> is expecting. No case that there are multiple mfd child nodes where
> >> mfd_add_device() would assign the first of n child nodes with the
> >> same compatible to multiple devices.
> >>
> >> So it appears to me that drivers/mfd/ab8500-core.c is currently broken.
> >> Am I missing something here?
> >>
> >> If I am correct, then either drivers/mfd/ab8500-core.c or
> >> ste-ab8500.dtsi and ste-ab8505.dtsi need to be fixed.
> >
> > Your analysis is correct.
>
> OK, if I'm not overlooking anything, that is good news.
>
> Existing .dts source files only have one "ab8500-pwm" child. They
> already
> work correcly.
>
> Create a new compatible for the case of multiple children. In my example
> I will add "-mc" (multiple children) to the existing compatible. There
> is likely a better name, but this lets me provide an example.
>
> Modify drivers/mfd/ab8500-core.c to use the new compatible, and new .dts
> source files with multiple children use the new compatible:
>
> OF_MFD_CELL("ab8500-pwm",
> NULL, NULL, 0, 0, "stericsson,ab8500-pwm"),
>
> OF_MFD_CELL_REG("ab8500-pwm-mc",
> NULL, NULL, 0, 0, "stericsson,ab8500-pwm", 0),
> OF_MFD_CELL_REG("ab8500-pwm-mc",
> NULL, NULL, 0, 1, "stericsson,ab8500-pwm", 1),
> OF_MFD_CELL_REG("ab8500-pwm-mc",
> NULL, NULL, 0, 2, "stericsson,ab8500-pwm", 2),
>
> The "OF_MFD_CELL" entry is the existing entry, which will handle current
> .dts source files. The new "OF_MFD_CELL_REG" entries will handle new
> .dts source files.
> >>>
> >>> Sorry, but I'm not sure what the above exercise is supposed to solve.
> >>>
> >>> Could you explain it for me please?
> >>
> >> The OF_MFD_CELL() entry handles all of the existing .dts source files
> >> that only have one ab8500-pwm child nodes. So existing .dtb blobs
> >> continue to work.
> >>
> >> The OF_MFD_CELL_REG() entries will handle all of the new .dts source
> >> files that will have up to 3 ab8500-pwm child nodes.
> >>
> >> Compatibility is maintained for existing .dtb files. A new kernel
> >> version with the changes will support new .dtb files that contain
> >> multiple ab8500-pwm child nodes.
> >
> > I can see *what* you're trying to do. I was looking for an
> > explanation of *how* you think that will work. FWIW, I don't think
> > what you're proposing will work as you envisage. I thought that
> > perhaps I was missing something, which is why I requested further
> > explanation.
> >
> And of course the patch that creates OF_MFD_CELL_REG() needs to precede
> this change.
>
> I would remove the fallback code in
Re: [PATCH v2 1/3] mfd: core: Make a best effort attempt to match devices with the correct of_nodes
On 2020-06-22 13:01, Frank Rowand wrote:
> On 2020-06-22 10:10, Lee Jones wrote:
>> On Mon, 22 Jun 2020, Frank Rowand wrote:
>>
>>> On 2020-06-22 03:50, Lee Jones wrote:
On Thu, 18 Jun 2020, Frank Rowand wrote:
> On 2020-06-15 04:26, Lee Jones wrote:
>> On Sun, 14 Jun 2020, Frank Rowand wrote:
>>
>>> Hi Lee,
>>>
>>> I'm looking at 5.8-rc1.
>>>
>>> The only use of OF_MFD_CELL() where the same compatible is specified
>>> for multiple elements of a struct mfd_cell array is for compatible
>>> "stericsson,ab8500-pwm" in drivers/mfd/ab8500-core.c:
>>>
>>> OF_MFD_CELL("ab8500-pwm",
>>> NULL, NULL, 0, 1, "stericsson,ab8500-pwm"),
>>> OF_MFD_CELL("ab8500-pwm",
>>> NULL, NULL, 0, 2, "stericsson,ab8500-pwm"),
>>> OF_MFD_CELL("ab8500-pwm",
>>> NULL, NULL, 0, 3, "stericsson,ab8500-pwm"),
>
> OF_MFD_CELL("ab8500-pwm",
> NULL, NULL, 0, 0, "stericsson,ab8500-pwm"),
>
> OF_MFD_CELL_REG("ab8500-pwm-mc",
> NULL, NULL, 0, 0, "stericsson,ab8500-pwm", 0),
> OF_MFD_CELL_REG("ab8500-pwm-mc",
> NULL, NULL, 0, 1, "stericsson,ab8500-pwm", 1),
> OF_MFD_CELL_REG("ab8500-pwm-mc",
> NULL, NULL, 0, 2, "stericsson,ab8500-pwm", 2),
>
>>>
>>> The only .dts or .dtsi files where I see compatible
>>> "stericsson,ab8500-pwm"
>>> are:
>>>
>>>arch/arm/boot/dts/ste-ab8500.dtsi
>>>arch/arm/boot/dts/ste-ab8505.dtsi
>>>
>>> These two .dtsi files only have a single node with this compatible.
>>> Chasing back to .dts and .dtsi files that include these two .dtsi
>>> files, I see no case where there are multiple nodes with this
>>> compatible.
>>>
>>> So it looks to me like there is no .dts in mainline that is providing
>>> the three "stericsson,ab8500-pwm" nodes that drivers/mfd/ab8500-core.c
>>> is expecting. No case that there are multiple mfd child nodes where
>>> mfd_add_device() would assign the first of n child nodes with the
>>> same compatible to multiple devices.
>>>
>>> So it appears to me that drivers/mfd/ab8500-core.c is currently broken.
>>> Am I missing something here?
>>>
>>> If I am correct, then either drivers/mfd/ab8500-core.c or
>>> ste-ab8500.dtsi and ste-ab8505.dtsi need to be fixed.
>>
>> Your analysis is correct.
>
> OK, if I'm not overlooking anything, that is good news.
>
> Existing .dts source files only have one "ab8500-pwm" child. They already
> work correcly.
>
> Create a new compatible for the case of multiple children. In my example
> I will add "-mc" (multiple children) to the existing compatible. There
> is likely a better name, but this lets me provide an example.
>
> Modify drivers/mfd/ab8500-core.c to use the new compatible, and new .dts
> source files with multiple children use the new compatible:
>
> OF_MFD_CELL("ab8500-pwm",
> NULL, NULL, 0, 0, "stericsson,ab8500-pwm"),
>
> OF_MFD_CELL_REG("ab8500-pwm-mc",
> NULL, NULL, 0, 0, "stericsson,ab8500-pwm", 0),
> OF_MFD_CELL_REG("ab8500-pwm-mc",
> NULL, NULL, 0, 1, "stericsson,ab8500-pwm", 1),
> OF_MFD_CELL_REG("ab8500-pwm-mc",
> NULL, NULL, 0, 2, "stericsson,ab8500-pwm", 2),
>
> The "OF_MFD_CELL" entry is the existing entry, which will handle current
> .dts source files. The new "OF_MFD_CELL_REG" entries will handle new
> .dts source files.
Sorry, but I'm not sure what the above exercise is supposed to solve.
Could you explain it for me please?
>>>
>>> The OF_MFD_CELL() entry handles all of the existing .dts source files
>>> that only have one ab8500-pwm child nodes. So existing .dtb blobs
>>> continue to work.
>>>
>>> The OF_MFD_CELL_REG() entries will handle all of the new .dts source
>>> files that will have up to 3 ab8500-pwm child nodes.
>>>
>>> Compatibility is maintained for existing .dtb files. A new kernel
>>> version with the changes will support new .dtb files that contain
>>> multiple ab8500-pwm child nodes.
>>
>> I can see *what* you're trying to do. I was looking for an
>> explanation of *how* you think that will work. FWIW, I don't think
>> what you're proposing will work as you envisage. I thought that
>> perhaps I was missing something, which is why I requested further
>> explanation.
>>
> And of course the patch that creates OF_MFD_CELL_REG() needs to precede
> this change.
>
> I would remove the fallback code in the existing patch that tries to
> handle an incorrect binding. Just error out if the binding is not
> used
Re: [PATCH v2 1/3] mfd: core: Make a best effort attempt to match devices with the correct of_nodes
On 2020-06-22 10:10, Lee Jones wrote:
> On Mon, 22 Jun 2020, Frank Rowand wrote:
>
>> On 2020-06-22 03:50, Lee Jones wrote:
>>> On Thu, 18 Jun 2020, Frank Rowand wrote:
>>>
On 2020-06-15 04:26, Lee Jones wrote:
> On Sun, 14 Jun 2020, Frank Rowand wrote:
>
>> Hi Lee,
>>
>> I'm looking at 5.8-rc1.
>>
>> The only use of OF_MFD_CELL() where the same compatible is specified
>> for multiple elements of a struct mfd_cell array is for compatible
>> "stericsson,ab8500-pwm" in drivers/mfd/ab8500-core.c:
>>
>> OF_MFD_CELL("ab8500-pwm",
>> NULL, NULL, 0, 1, "stericsson,ab8500-pwm"),
>> OF_MFD_CELL("ab8500-pwm",
>> NULL, NULL, 0, 2, "stericsson,ab8500-pwm"),
>> OF_MFD_CELL("ab8500-pwm",
>> NULL, NULL, 0, 3, "stericsson,ab8500-pwm"),
OF_MFD_CELL("ab8500-pwm",
NULL, NULL, 0, 0, "stericsson,ab8500-pwm"),
OF_MFD_CELL_REG("ab8500-pwm-mc",
NULL, NULL, 0, 0, "stericsson,ab8500-pwm", 0),
OF_MFD_CELL_REG("ab8500-pwm-mc",
NULL, NULL, 0, 1, "stericsson,ab8500-pwm", 1),
OF_MFD_CELL_REG("ab8500-pwm-mc",
NULL, NULL, 0, 2, "stericsson,ab8500-pwm", 2),
>>
>> The only .dts or .dtsi files where I see compatible
>> "stericsson,ab8500-pwm"
>> are:
>>
>>arch/arm/boot/dts/ste-ab8500.dtsi
>>arch/arm/boot/dts/ste-ab8505.dtsi
>>
>> These two .dtsi files only have a single node with this compatible.
>> Chasing back to .dts and .dtsi files that include these two .dtsi
>> files, I see no case where there are multiple nodes with this
>> compatible.
>>
>> So it looks to me like there is no .dts in mainline that is providing
>> the three "stericsson,ab8500-pwm" nodes that drivers/mfd/ab8500-core.c
>> is expecting. No case that there are multiple mfd child nodes where
>> mfd_add_device() would assign the first of n child nodes with the
>> same compatible to multiple devices.
>>
>> So it appears to me that drivers/mfd/ab8500-core.c is currently broken.
>> Am I missing something here?
>>
>> If I am correct, then either drivers/mfd/ab8500-core.c or
>> ste-ab8500.dtsi and ste-ab8505.dtsi need to be fixed.
>
> Your analysis is correct.
OK, if I'm not overlooking anything, that is good news.
Existing .dts source files only have one "ab8500-pwm" child. They already
work correcly.
Create a new compatible for the case of multiple children. In my example
I will add "-mc" (multiple children) to the existing compatible. There
is likely a better name, but this lets me provide an example.
Modify drivers/mfd/ab8500-core.c to use the new compatible, and new .dts
source files with multiple children use the new compatible:
OF_MFD_CELL("ab8500-pwm",
NULL, NULL, 0, 0, "stericsson,ab8500-pwm"),
OF_MFD_CELL_REG("ab8500-pwm-mc",
NULL, NULL, 0, 0, "stericsson,ab8500-pwm", 0),
OF_MFD_CELL_REG("ab8500-pwm-mc",
NULL, NULL, 0, 1, "stericsson,ab8500-pwm", 1),
OF_MFD_CELL_REG("ab8500-pwm-mc",
NULL, NULL, 0, 2, "stericsson,ab8500-pwm", 2),
The "OF_MFD_CELL" entry is the existing entry, which will handle current
.dts source files. The new "OF_MFD_CELL_REG" entries will handle new
.dts source files.
>>>
>>> Sorry, but I'm not sure what the above exercise is supposed to solve.
>>>
>>> Could you explain it for me please?
>>
>> The OF_MFD_CELL() entry handles all of the existing .dts source files
>> that only have one ab8500-pwm child nodes. So existing .dtb blobs
>> continue to work.
>>
>> The OF_MFD_CELL_REG() entries will handle all of the new .dts source
>> files that will have up to 3 ab8500-pwm child nodes.
>>
>> Compatibility is maintained for existing .dtb files. A new kernel
>> version with the changes will support new .dtb files that contain
>> multiple ab8500-pwm child nodes.
>
> I can see *what* you're trying to do. I was looking for an
> explanation of *how* you think that will work. FWIW, I don't think
> what you're proposing will work as you envisage. I thought that
> perhaps I was missing something, which is why I requested further
> explanation.
>
And of course the patch that creates OF_MFD_CELL_REG() needs to precede
this change.
I would remove the fallback code in the existing patch that tries to
handle an incorrect binding. Just error out if the binding is not
used properly.
>>>
>>> What fallback code?
>>
>> Based on reading the patch description, I expected some extra code to try
>> to handle the case where the
Re: [PATCH v2 1/3] mfd: core: Make a best effort attempt to match devices with the correct of_nodes
On Mon, 22 Jun 2020, Frank Rowand wrote: > On 2020-06-22 03:09, Lee Jones wrote: > > On Thu, 11 Jun 2020, 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. > > > > Any more reviews/comments before I apply this? > > > > Yes, outstanding issues, so please do not apply. > > Shortly after you sent this email, you sent a reply to one of my > earlier emails in this thread. I have replied to that email, > so we still have an ongoing conversation where we are trying > to resolve my understanding of the problem and whether the > solution is appropriate. Happy to discuss whatever issues you're having. Looks like the ball is in your court. :) -- Lee Jones [李琼斯] Senior Technical Lead - Developer Services Linaro.org │ Open source software for Arm SoCs Follow Linaro: Facebook | Twitter | Blog
Re: [PATCH v2 1/3] mfd: core: Make a best effort attempt to match devices with the correct of_nodes
On 2020-06-22 03:09, Lee Jones wrote: > On Thu, 11 Jun 2020, 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. > > Any more reviews/comments before I apply this? > Yes, outstanding issues, so please do not apply. Shortly after you sent this email, you sent a reply to one of my earlier emails in this thread. I have replied to that email, so we still have an ongoing conversation where we are trying to resolve my understanding of the problem and whether the solution is appropriate. -Frank
Re: [PATCH v2 1/3] mfd: core: Make a best effort attempt to match devices with the correct of_nodes
On Mon, 22 Jun 2020, Frank Rowand wrote:
> On 2020-06-22 03:50, Lee Jones wrote:
> > On Thu, 18 Jun 2020, Frank Rowand wrote:
> >
> >> On 2020-06-15 04:26, Lee Jones wrote:
> >>> On Sun, 14 Jun 2020, Frank Rowand wrote:
> >>>
> Hi Lee,
>
> I'm looking at 5.8-rc1.
>
> The only use of OF_MFD_CELL() where the same compatible is specified
> for multiple elements of a struct mfd_cell array is for compatible
> "stericsson,ab8500-pwm" in drivers/mfd/ab8500-core.c:
>
> OF_MFD_CELL("ab8500-pwm",
> NULL, NULL, 0, 1, "stericsson,ab8500-pwm"),
> OF_MFD_CELL("ab8500-pwm",
> NULL, NULL, 0, 2, "stericsson,ab8500-pwm"),
> OF_MFD_CELL("ab8500-pwm",
> NULL, NULL, 0, 3, "stericsson,ab8500-pwm"),
> >>
> >> OF_MFD_CELL("ab8500-pwm",
> >> NULL, NULL, 0, 0, "stericsson,ab8500-pwm"),
> >>
> >> OF_MFD_CELL_REG("ab8500-pwm-mc",
> >> NULL, NULL, 0, 0, "stericsson,ab8500-pwm", 0),
> >> OF_MFD_CELL_REG("ab8500-pwm-mc",
> >> NULL, NULL, 0, 1, "stericsson,ab8500-pwm", 1),
> >> OF_MFD_CELL_REG("ab8500-pwm-mc",
> >> NULL, NULL, 0, 2, "stericsson,ab8500-pwm", 2),
> >>
>
> The only .dts or .dtsi files where I see compatible
> "stericsson,ab8500-pwm"
> are:
>
> arch/arm/boot/dts/ste-ab8500.dtsi
> arch/arm/boot/dts/ste-ab8505.dtsi
>
> These two .dtsi files only have a single node with this compatible.
> Chasing back to .dts and .dtsi files that include these two .dtsi
> files, I see no case where there are multiple nodes with this
> compatible.
>
> So it looks to me like there is no .dts in mainline that is providing
> the three "stericsson,ab8500-pwm" nodes that drivers/mfd/ab8500-core.c
> is expecting. No case that there are multiple mfd child nodes where
> mfd_add_device() would assign the first of n child nodes with the
> same compatible to multiple devices.
>
> So it appears to me that drivers/mfd/ab8500-core.c is currently broken.
> Am I missing something here?
>
> If I am correct, then either drivers/mfd/ab8500-core.c or
> ste-ab8500.dtsi and ste-ab8505.dtsi need to be fixed.
> >>>
> >>> Your analysis is correct.
> >>
> >> OK, if I'm not overlooking anything, that is good news.
> >>
> >> Existing .dts source files only have one "ab8500-pwm" child. They already
> >> work correcly.
> >>
> >> Create a new compatible for the case of multiple children. In my example
> >> I will add "-mc" (multiple children) to the existing compatible. There
> >> is likely a better name, but this lets me provide an example.
> >>
> >> Modify drivers/mfd/ab8500-core.c to use the new compatible, and new .dts
> >> source files with multiple children use the new compatible:
> >>
> >> OF_MFD_CELL("ab8500-pwm",
> >> NULL, NULL, 0, 0, "stericsson,ab8500-pwm"),
> >>
> >> OF_MFD_CELL_REG("ab8500-pwm-mc",
> >> NULL, NULL, 0, 0, "stericsson,ab8500-pwm", 0),
> >> OF_MFD_CELL_REG("ab8500-pwm-mc",
> >> NULL, NULL, 0, 1, "stericsson,ab8500-pwm", 1),
> >> OF_MFD_CELL_REG("ab8500-pwm-mc",
> >> NULL, NULL, 0, 2, "stericsson,ab8500-pwm", 2),
> >>
> >> The "OF_MFD_CELL" entry is the existing entry, which will handle current
> >> .dts source files. The new "OF_MFD_CELL_REG" entries will handle new
> >> .dts source files.
> >
> > Sorry, but I'm not sure what the above exercise is supposed to solve.
> >
> > Could you explain it for me please?
>
> The OF_MFD_CELL() entry handles all of the existing .dts source files
> that only have one ab8500-pwm child nodes. So existing .dtb blobs
> continue to work.
>
> The OF_MFD_CELL_REG() entries will handle all of the new .dts source
> files that will have up to 3 ab8500-pwm child nodes.
>
> Compatibility is maintained for existing .dtb files. A new kernel
> version with the changes will support new .dtb files that contain
> multiple ab8500-pwm child nodes.
I can see *what* you're trying to do. I was looking for an
explanation of *how* you think that will work. FWIW, I don't think
what you're proposing will work as you envisage. I thought that
perhaps I was missing something, which is why I requested further
explanation.
> >> And of course the patch that creates OF_MFD_CELL_REG() needs to precede
> >> this change.
> >>
> >> I would remove the fallback code in the existing patch that tries to
> >> handle an incorrect binding. Just error out if the binding is not
> >> used properly.
> >
> > What fallback code?
>
> Based on reading the patch description, I expected some extra code to try
> to handle the case where the compatible in more than one struct mfd_cell
> entry is
Re: [PATCH v2 1/3] mfd: core: Make a best effort attempt to match devices with the correct of_nodes
On 2020-06-22 09:32, Frank Rowand wrote:
> On 2020-06-22 03:50, Lee Jones wrote:
>> On Thu, 18 Jun 2020, Frank Rowand wrote:
>>
>>> On 2020-06-15 04:26, Lee Jones wrote:
On Sun, 14 Jun 2020, Frank Rowand wrote:
> Hi Lee,
>
> I'm looking at 5.8-rc1.
>
> The only use of OF_MFD_CELL() where the same compatible is specified
> for multiple elements of a struct mfd_cell array is for compatible
> "stericsson,ab8500-pwm" in drivers/mfd/ab8500-core.c:
>
> OF_MFD_CELL("ab8500-pwm",
> NULL, NULL, 0, 1, "stericsson,ab8500-pwm"),
> OF_MFD_CELL("ab8500-pwm",
> NULL, NULL, 0, 2, "stericsson,ab8500-pwm"),
> OF_MFD_CELL("ab8500-pwm",
> NULL, NULL, 0, 3, "stericsson,ab8500-pwm"),
>>>
>>> OF_MFD_CELL("ab8500-pwm",
>>> NULL, NULL, 0, 0, "stericsson,ab8500-pwm"),
>>>
>>> OF_MFD_CELL_REG("ab8500-pwm-mc",
>>> NULL, NULL, 0, 0, "stericsson,ab8500-pwm", 0),
>>> OF_MFD_CELL_REG("ab8500-pwm-mc",
>>> NULL, NULL, 0, 1, "stericsson,ab8500-pwm", 1),
>>> OF_MFD_CELL_REG("ab8500-pwm-mc",
>>> NULL, NULL, 0, 2, "stericsson,ab8500-pwm", 2),
>>>
>
> The only .dts or .dtsi files where I see compatible
> "stericsson,ab8500-pwm"
> are:
>
>arch/arm/boot/dts/ste-ab8500.dtsi
>arch/arm/boot/dts/ste-ab8505.dtsi
>
> These two .dtsi files only have a single node with this compatible.
> Chasing back to .dts and .dtsi files that include these two .dtsi
> files, I see no case where there are multiple nodes with this
> compatible.
>
> So it looks to me like there is no .dts in mainline that is providing
> the three "stericsson,ab8500-pwm" nodes that drivers/mfd/ab8500-core.c
> is expecting. No case that there are multiple mfd child nodes where
> mfd_add_device() would assign the first of n child nodes with the
> same compatible to multiple devices.
>
> So it appears to me that drivers/mfd/ab8500-core.c is currently broken.
> Am I missing something here?
>
> If I am correct, then either drivers/mfd/ab8500-core.c or
> ste-ab8500.dtsi and ste-ab8505.dtsi need to be fixed.
Your analysis is correct.
>>>
>>> OK, if I'm not overlooking anything, that is good news.
>>>
>>> Existing .dts source files only have one "ab8500-pwm" child. They already
>>> work correcly.
>>>
>>> Create a new compatible for the case of multiple children. In my example
>>> I will add "-mc" (multiple children) to the existing compatible. There
>>> is likely a better name, but this lets me provide an example.
>>>
>>> Modify drivers/mfd/ab8500-core.c to use the new compatible, and new .dts
>>> source files with multiple children use the new compatible:
>>>
>>> OF_MFD_CELL("ab8500-pwm",
>>> NULL, NULL, 0, 0, "stericsson,ab8500-pwm"),
>>>
>>> OF_MFD_CELL_REG("ab8500-pwm-mc",
>>> NULL, NULL, 0, 0, "stericsson,ab8500-pwm", 0),
>>> OF_MFD_CELL_REG("ab8500-pwm-mc",
>>> NULL, NULL, 0, 1, "stericsson,ab8500-pwm", 1),
>>> OF_MFD_CELL_REG("ab8500-pwm-mc",
>>> NULL, NULL, 0, 2, "stericsson,ab8500-pwm", 2),
>>>
>>> The "OF_MFD_CELL" entry is the existing entry, which will handle current
>>> .dts source files. The new "OF_MFD_CELL_REG" entries will handle new
>>> .dts source files.
>>
>> Sorry, but I'm not sure what the above exercise is supposed to solve.
>>
>> Could you explain it for me please?
>
> The OF_MFD_CELL() entry handles all of the existing .dts source files
> that only have one ab8500-pwm child nodes. So existing .dtb blobs
> continue to work.
>
> The OF_MFD_CELL_REG() entries will handle all of the new .dts source
> files that will have up to 3 ab8500-pwm child nodes.
>
> Compatibility is maintained for existing .dtb files. A new kernel
> version with the changes will support new .dtb files that contain
> multiple ab8500-pwm child nodes.
>
>>
>>> And of course the patch that creates OF_MFD_CELL_REG() needs to precede
>>> this change.
>>>
>>> I would remove the fallback code in the existing patch that tries to
>>> handle an incorrect binding. Just error out if the binding is not
>>> used properly.
>>
>> What fallback code?
>
> Based on reading the patch description, I expected some extra code to try
> to handle the case where the compatible in more than one struct mfd_cell
> entry is "stericsson,ab8500-pwm" and there are multiple ab8500-pwm child
> nodes.
>
> Looking at the actual code (which I had not done before), I see that the
> "best effort attempt to match" is keeping a list of child nodes that
> have already been used (mfd_of_node_list) and avoiding re-use of such
> nodes. This allows an invalid .dtb (one with multple "stericsson,ab8500-pwm"
> child
Re: [PATCH v2 1/3] mfd: core: Make a best effort attempt to match devices with the correct of_nodes
On 2020-06-22 03:50, Lee Jones wrote:
> On Thu, 18 Jun 2020, Frank Rowand wrote:
>
>> On 2020-06-15 04:26, Lee Jones wrote:
>>> On Sun, 14 Jun 2020, Frank Rowand wrote:
>>>
Hi Lee,
I'm looking at 5.8-rc1.
The only use of OF_MFD_CELL() where the same compatible is specified
for multiple elements of a struct mfd_cell array is for compatible
"stericsson,ab8500-pwm" in drivers/mfd/ab8500-core.c:
OF_MFD_CELL("ab8500-pwm",
NULL, NULL, 0, 1, "stericsson,ab8500-pwm"),
OF_MFD_CELL("ab8500-pwm",
NULL, NULL, 0, 2, "stericsson,ab8500-pwm"),
OF_MFD_CELL("ab8500-pwm",
NULL, NULL, 0, 3, "stericsson,ab8500-pwm"),
>>
>> OF_MFD_CELL("ab8500-pwm",
>> NULL, NULL, 0, 0, "stericsson,ab8500-pwm"),
>>
>> OF_MFD_CELL_REG("ab8500-pwm-mc",
>> NULL, NULL, 0, 0, "stericsson,ab8500-pwm", 0),
>> OF_MFD_CELL_REG("ab8500-pwm-mc",
>> NULL, NULL, 0, 1, "stericsson,ab8500-pwm", 1),
>> OF_MFD_CELL_REG("ab8500-pwm-mc",
>> NULL, NULL, 0, 2, "stericsson,ab8500-pwm", 2),
>>
The only .dts or .dtsi files where I see compatible "stericsson,ab8500-pwm"
are:
arch/arm/boot/dts/ste-ab8500.dtsi
arch/arm/boot/dts/ste-ab8505.dtsi
These two .dtsi files only have a single node with this compatible.
Chasing back to .dts and .dtsi files that include these two .dtsi
files, I see no case where there are multiple nodes with this
compatible.
So it looks to me like there is no .dts in mainline that is providing
the three "stericsson,ab8500-pwm" nodes that drivers/mfd/ab8500-core.c
is expecting. No case that there are multiple mfd child nodes where
mfd_add_device() would assign the first of n child nodes with the
same compatible to multiple devices.
So it appears to me that drivers/mfd/ab8500-core.c is currently broken.
Am I missing something here?
If I am correct, then either drivers/mfd/ab8500-core.c or
ste-ab8500.dtsi and ste-ab8505.dtsi need to be fixed.
>>>
>>> Your analysis is correct.
>>
>> OK, if I'm not overlooking anything, that is good news.
>>
>> Existing .dts source files only have one "ab8500-pwm" child. They already
>> work correcly.
>>
>> Create a new compatible for the case of multiple children. In my example
>> I will add "-mc" (multiple children) to the existing compatible. There
>> is likely a better name, but this lets me provide an example.
>>
>> Modify drivers/mfd/ab8500-core.c to use the new compatible, and new .dts
>> source files with multiple children use the new compatible:
>>
>> OF_MFD_CELL("ab8500-pwm",
>> NULL, NULL, 0, 0, "stericsson,ab8500-pwm"),
>>
>> OF_MFD_CELL_REG("ab8500-pwm-mc",
>> NULL, NULL, 0, 0, "stericsson,ab8500-pwm", 0),
>> OF_MFD_CELL_REG("ab8500-pwm-mc",
>> NULL, NULL, 0, 1, "stericsson,ab8500-pwm", 1),
>> OF_MFD_CELL_REG("ab8500-pwm-mc",
>> NULL, NULL, 0, 2, "stericsson,ab8500-pwm", 2),
>>
>> The "OF_MFD_CELL" entry is the existing entry, which will handle current
>> .dts source files. The new "OF_MFD_CELL_REG" entries will handle new
>> .dts source files.
>
> Sorry, but I'm not sure what the above exercise is supposed to solve.
>
> Could you explain it for me please?
The OF_MFD_CELL() entry handles all of the existing .dts source files
that only have one ab8500-pwm child nodes. So existing .dtb blobs
continue to work.
The OF_MFD_CELL_REG() entries will handle all of the new .dts source
files that will have up to 3 ab8500-pwm child nodes.
Compatibility is maintained for existing .dtb files. A new kernel
version with the changes will support new .dtb files that contain
multiple ab8500-pwm child nodes.
>
>> And of course the patch that creates OF_MFD_CELL_REG() needs to precede
>> this change.
>>
>> I would remove the fallback code in the existing patch that tries to
>> handle an incorrect binding. Just error out if the binding is not
>> used properly.
>
> What fallback code?
Based on reading the patch description, I expected some extra code to try
to handle the case where the compatible in more than one struct mfd_cell
entry is "stericsson,ab8500-pwm" and there are multiple ab8500-pwm child
nodes.
Looking at the actual code (which I had not done before), I see that the
"best effort attempt to match" is keeping a list of child nodes that
have already been used (mfd_of_node_list) and avoiding re-use of such
nodes. This allows an invalid .dtb (one with multple "stericsson,ab8500-pwm"
child nodes) to possibly be assigned unique child nodes for multiple
struct mfd_cell entries to be "stericsson,ab8500-pwm".
So it is confusing for me to call that "fallback code". It really
Re: [PATCH v2 1/3] mfd: core: Make a best effort attempt to match devices with the correct of_nodes
On Thu, 18 Jun 2020, Frank Rowand wrote:
> On 2020-06-15 04:26, Lee Jones wrote:
> > On Sun, 14 Jun 2020, Frank Rowand wrote:
> >
> >> Hi Lee,
> >>
> >> I'm looking at 5.8-rc1.
> >>
> >> The only use of OF_MFD_CELL() where the same compatible is specified
> >> for multiple elements of a struct mfd_cell array is for compatible
> >> "stericsson,ab8500-pwm" in drivers/mfd/ab8500-core.c:
> >>
> >> OF_MFD_CELL("ab8500-pwm",
> >> NULL, NULL, 0, 1, "stericsson,ab8500-pwm"),
> >> OF_MFD_CELL("ab8500-pwm",
> >> NULL, NULL, 0, 2, "stericsson,ab8500-pwm"),
> >> OF_MFD_CELL("ab8500-pwm",
> >> NULL, NULL, 0, 3, "stericsson,ab8500-pwm"),
>
> OF_MFD_CELL("ab8500-pwm",
> NULL, NULL, 0, 0, "stericsson,ab8500-pwm"),
>
> OF_MFD_CELL_REG("ab8500-pwm-mc",
> NULL, NULL, 0, 0, "stericsson,ab8500-pwm", 0),
> OF_MFD_CELL_REG("ab8500-pwm-mc",
> NULL, NULL, 0, 1, "stericsson,ab8500-pwm", 1),
> OF_MFD_CELL_REG("ab8500-pwm-mc",
> NULL, NULL, 0, 2, "stericsson,ab8500-pwm", 2),
>
> >>
> >> The only .dts or .dtsi files where I see compatible "stericsson,ab8500-pwm"
> >> are:
> >>
> >>arch/arm/boot/dts/ste-ab8500.dtsi
> >>arch/arm/boot/dts/ste-ab8505.dtsi
> >>
> >> These two .dtsi files only have a single node with this compatible.
> >> Chasing back to .dts and .dtsi files that include these two .dtsi
> >> files, I see no case where there are multiple nodes with this
> >> compatible.
> >>
> >> So it looks to me like there is no .dts in mainline that is providing
> >> the three "stericsson,ab8500-pwm" nodes that drivers/mfd/ab8500-core.c
> >> is expecting. No case that there are multiple mfd child nodes where
> >> mfd_add_device() would assign the first of n child nodes with the
> >> same compatible to multiple devices.
> >>
> >> So it appears to me that drivers/mfd/ab8500-core.c is currently broken.
> >> Am I missing something here?
> >>
> >> If I am correct, then either drivers/mfd/ab8500-core.c or
> >> ste-ab8500.dtsi and ste-ab8505.dtsi need to be fixed.
> >
> > Your analysis is correct.
>
> OK, if I'm not overlooking anything, that is good news.
>
> Existing .dts source files only have one "ab8500-pwm" child. They already
> work correcly.
>
> Create a new compatible for the case of multiple children. In my example
> I will add "-mc" (multiple children) to the existing compatible. There
> is likely a better name, but this lets me provide an example.
>
> Modify drivers/mfd/ab8500-core.c to use the new compatible, and new .dts
> source files with multiple children use the new compatible:
>
> OF_MFD_CELL("ab8500-pwm",
> NULL, NULL, 0, 0, "stericsson,ab8500-pwm"),
>
> OF_MFD_CELL_REG("ab8500-pwm-mc",
> NULL, NULL, 0, 0, "stericsson,ab8500-pwm", 0),
> OF_MFD_CELL_REG("ab8500-pwm-mc",
> NULL, NULL, 0, 1, "stericsson,ab8500-pwm", 1),
> OF_MFD_CELL_REG("ab8500-pwm-mc",
> NULL, NULL, 0, 2, "stericsson,ab8500-pwm", 2),
>
> The "OF_MFD_CELL" entry is the existing entry, which will handle current
> .dts source files. The new "OF_MFD_CELL_REG" entries will handle new
> .dts source files.
Sorry, but I'm not sure what the above exercise is supposed to solve.
Could you explain it for me please?
> And of course the patch that creates OF_MFD_CELL_REG() needs to precede
> this change.
>
> I would remove the fallback code in the existing patch that tries to
> handle an incorrect binding. Just error out if the binding is not
> used properly.
What fallback code?
> > Although it's not "broken", it just works when it really shouldn't.
> >
> > I will be fixing the 'ab8500-pwm' case in due course.
> >
> >> Moving forward, your proposed OF_MFD_CELL_REG() method seems a good
> >> approach (I have not completely read the actual code in the patch yet
> >> though).
> >
> > Thanks.
> >
>
--
Lee Jones [李琼斯]
Senior Technical Lead - Developer Services
Linaro.org │ Open source software for Arm SoCs
Follow Linaro: Facebook | Twitter | Blog
Re: [PATCH v2 1/3] mfd: core: Make a best effort attempt to match devices with the correct of_nodes
On Thu, 11 Jun 2020, 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. Any more reviews/comments before I apply this? -- Lee Jones [李琼斯] Senior Technical Lead - Developer Services Linaro.org │ Open source software for Arm SoCs Follow Linaro: Facebook | Twitter | Blog
Re: [PATCH v2 1/3] mfd: core: Make a best effort attempt to match devices with the correct of_nodes
On 2020-06-15 04:26, Lee Jones wrote:
> On Sun, 14 Jun 2020, Frank Rowand wrote:
>
>> Hi Lee,
>>
>> I'm looking at 5.8-rc1.
>>
>> The only use of OF_MFD_CELL() where the same compatible is specified
>> for multiple elements of a struct mfd_cell array is for compatible
>> "stericsson,ab8500-pwm" in drivers/mfd/ab8500-core.c:
>>
>> OF_MFD_CELL("ab8500-pwm",
>> NULL, NULL, 0, 1, "stericsson,ab8500-pwm"),
>> OF_MFD_CELL("ab8500-pwm",
>> NULL, NULL, 0, 2, "stericsson,ab8500-pwm"),
>> OF_MFD_CELL("ab8500-pwm",
>> NULL, NULL, 0, 3, "stericsson,ab8500-pwm"),
OF_MFD_CELL("ab8500-pwm",
NULL, NULL, 0, 0, "stericsson,ab8500-pwm"),
OF_MFD_CELL_REG("ab8500-pwm-mc",
NULL, NULL, 0, 0, "stericsson,ab8500-pwm", 0),
OF_MFD_CELL_REG("ab8500-pwm-mc",
NULL, NULL, 0, 1, "stericsson,ab8500-pwm", 1),
OF_MFD_CELL_REG("ab8500-pwm-mc",
NULL, NULL, 0, 2, "stericsson,ab8500-pwm", 2),
>>
>> The only .dts or .dtsi files where I see compatible "stericsson,ab8500-pwm"
>> are:
>>
>>arch/arm/boot/dts/ste-ab8500.dtsi
>>arch/arm/boot/dts/ste-ab8505.dtsi
>>
>> These two .dtsi files only have a single node with this compatible.
>> Chasing back to .dts and .dtsi files that include these two .dtsi
>> files, I see no case where there are multiple nodes with this
>> compatible.
>>
>> So it looks to me like there is no .dts in mainline that is providing
>> the three "stericsson,ab8500-pwm" nodes that drivers/mfd/ab8500-core.c
>> is expecting. No case that there are multiple mfd child nodes where
>> mfd_add_device() would assign the first of n child nodes with the
>> same compatible to multiple devices.
>>
>> So it appears to me that drivers/mfd/ab8500-core.c is currently broken.
>> Am I missing something here?
>>
>> If I am correct, then either drivers/mfd/ab8500-core.c or
>> ste-ab8500.dtsi and ste-ab8505.dtsi need to be fixed.
>
> Your analysis is correct.
OK, if I'm not overlooking anything, that is good news.
Existing .dts source files only have one "ab8500-pwm" child. They already
work correcly.
Create a new compatible for the case of multiple children. In my example
I will add "-mc" (multiple children) to the existing compatible. There
is likely a better name, but this lets me provide an example.
Modify drivers/mfd/ab8500-core.c to use the new compatible, and new .dts
source files with multiple children use the new compatible:
OF_MFD_CELL("ab8500-pwm",
NULL, NULL, 0, 0, "stericsson,ab8500-pwm"),
OF_MFD_CELL_REG("ab8500-pwm-mc",
NULL, NULL, 0, 0, "stericsson,ab8500-pwm", 0),
OF_MFD_CELL_REG("ab8500-pwm-mc",
NULL, NULL, 0, 1, "stericsson,ab8500-pwm", 1),
OF_MFD_CELL_REG("ab8500-pwm-mc",
NULL, NULL, 0, 2, "stericsson,ab8500-pwm", 2),
The "OF_MFD_CELL" entry is the existing entry, which will handle current
.dts source files. The new "OF_MFD_CELL_REG" entries will handle new
.dts source files.
And of course the patch that creates OF_MFD_CELL_REG() needs to precede
this change.
I would remove the fallback code in the existing patch that tries to
handle an incorrect binding. Just error out if the binding is not
used properly.
-Frank
>
> Although it's not "broken", it just works when it really shouldn't.
>
> I will be fixing the 'ab8500-pwm' case in due course.
>
>> Moving forward, your proposed OF_MFD_CELL_REG() method seems a good
>> approach (I have not completely read the actual code in the patch yet
>> though).
>
> Thanks.
>
Re: [PATCH v2 1/3] mfd: core: Make a best effort attempt to match devices with the correct of_nodes
On Sun, 14 Jun 2020, Frank Rowand wrote:
> Hi Lee,
>
> I'm looking at 5.8-rc1.
>
> The only use of OF_MFD_CELL() where the same compatible is specified
> for multiple elements of a struct mfd_cell array is for compatible
> "stericsson,ab8500-pwm" in drivers/mfd/ab8500-core.c:
>
> OF_MFD_CELL("ab8500-pwm",
> NULL, NULL, 0, 1, "stericsson,ab8500-pwm"),
> OF_MFD_CELL("ab8500-pwm",
> NULL, NULL, 0, 2, "stericsson,ab8500-pwm"),
> OF_MFD_CELL("ab8500-pwm",
> NULL, NULL, 0, 3, "stericsson,ab8500-pwm"),
>
> The only .dts or .dtsi files where I see compatible "stericsson,ab8500-pwm"
> are:
>
>arch/arm/boot/dts/ste-ab8500.dtsi
>arch/arm/boot/dts/ste-ab8505.dtsi
>
> These two .dtsi files only have a single node with this compatible.
> Chasing back to .dts and .dtsi files that include these two .dtsi
> files, I see no case where there are multiple nodes with this
> compatible.
>
> So it looks to me like there is no .dts in mainline that is providing
> the three "stericsson,ab8500-pwm" nodes that drivers/mfd/ab8500-core.c
> is expecting. No case that there are multiple mfd child nodes where
> mfd_add_device() would assign the first of n child nodes with the
> same compatible to multiple devices.
>
> So it appears to me that drivers/mfd/ab8500-core.c is currently broken.
> Am I missing something here?
>
> If I am correct, then either drivers/mfd/ab8500-core.c or
> ste-ab8500.dtsi and ste-ab8505.dtsi need to be fixed.
Your analysis is correct.
Although it's not "broken", it just works when it really shouldn't.
I will be fixing the 'ab8500-pwm' case in due course.
> Moving forward, your proposed OF_MFD_CELL_REG() method seems a good
> approach (I have not completely read the actual code in the patch yet
> though).
Thanks.
--
Lee Jones [李琼斯]
Senior Technical Lead - Developer Services
Linaro.org │ Open source software for Arm SoCs
Follow Linaro: Facebook | Twitter | Blog
Re: [PATCH v2 1/3] mfd: core: Make a best effort attempt to match devices with the correct of_nodes
Hi Lee,
I'm looking at 5.8-rc1.
The only use of OF_MFD_CELL() where the same compatible is specified
for multiple elements of a struct mfd_cell array is for compatible
"stericsson,ab8500-pwm" in drivers/mfd/ab8500-core.c:
OF_MFD_CELL("ab8500-pwm",
NULL, NULL, 0, 1, "stericsson,ab8500-pwm"),
OF_MFD_CELL("ab8500-pwm",
NULL, NULL, 0, 2, "stericsson,ab8500-pwm"),
OF_MFD_CELL("ab8500-pwm",
NULL, NULL, 0, 3, "stericsson,ab8500-pwm"),
The only .dts or .dtsi files where I see compatible "stericsson,ab8500-pwm"
are:
arch/arm/boot/dts/ste-ab8500.dtsi
arch/arm/boot/dts/ste-ab8505.dtsi
These two .dtsi files only have a single node with this compatible.
Chasing back to .dts and .dtsi files that include these two .dtsi
files, I see no case where there are multiple nodes with this
compatible.
So it looks to me like there is no .dts in mainline that is providing
the three "stericsson,ab8500-pwm" nodes that drivers/mfd/ab8500-core.c
is expecting. No case that there are multiple mfd child nodes where
mfd_add_device() would assign the first of n child nodes with the
same compatible to multiple devices.
So it appears to me that drivers/mfd/ab8500-core.c is currently broken.
Am I missing something here?
If I am correct, then either drivers/mfd/ab8500-core.c or
ste-ab8500.dtsi and ste-ab8505.dtsi need to be fixed.
Moving forward, your proposed OF_MFD_CELL_REG() method seems a good
approach (I have not completely read the actual code in the patch yet
though).
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@ {
> compatible = "mfd,of-test-child";
> reg = <0x 0x 0 0x11>,
> <0x 0x 0 0x22>;
> };
>
> child@ {
> compatible = "mfd,of-test-child";
> reg = <0x 0x 0 0x33>;
> };
>
> child@ {
> compatible = "mfd,of-test-child";
> reg = <0x 0x 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@
> [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@
> [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@
>
> 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@
> [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@
> [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@
>
> 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
Re: [PATCH v2 1/3] mfd: core: Make a best effort attempt to match devices with the correct of_nodes
+ Frank (me)
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@ {
> compatible = "mfd,of-test-child";
> reg = <0x 0x 0 0x11>,
> <0x 0x 0 0x22>;
> };
>
> child@ {
> compatible = "mfd,of-test-child";
> reg = <0x 0x 0 0x33>;
> };
>
> child@ {
> compatible = "mfd,of-test-child";
> reg = <0x 0x 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@
> [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@
> [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@
>
> 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@
> [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@
> [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@
>
> 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", 0x),
> OF_MFD_CELL_REG("mfd-of-test-child", NULL, NULL, 0, 2,
> "mfd,of-test-child", 0x),
> OF_MFD_CELL_REG("mfd-of-test-child", NULL, NULL, 0, 3,
> "mfd,of-test-child", 0x)
> };
>
> 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@
> [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@
> [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@
>
> 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
[PATCH v2 1/3] mfd: core: Make a best effort attempt to match devices with the correct of_nodes
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@ {
compatible = "mfd,of-test-child";
reg = <0x 0x 0 0x11>,
<0x 0x 0 0x22>;
};
child@ {
compatible = "mfd,of-test-child";
reg = <0x 0x 0 0x33>;
};
child@ {
compatible = "mfd,of-test-child";
reg = <0x 0x 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@
[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@
[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@
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@
[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@
[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@
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", 0x),
OF_MFD_CELL_REG("mfd-of-test-child", NULL, NULL, 0, 2,
"mfd,of-test-child", 0x),
OF_MFD_CELL_REG("mfd-of-test-child", NULL, NULL, 0, 3,
"mfd,of-test-child", 0x)
};
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@
[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@
[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@
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
[PATCH v2 1/3] mfd: core: Make a best effort attempt to match devices with the correct of_nodes
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@ {
compatible = "mfd,of-test-child";
reg = <0x 0x 0 0x11>,
<0x 0x 0 0x22>;
};
child@ {
compatible = "mfd,of-test-child";
reg = <0x 0x 0 0x33>;
};
child@ {
compatible = "mfd,of-test-child";
reg = <0x 0x 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@
[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@
[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@
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@
[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@
[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@
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", 0x),
OF_MFD_CELL_REG("mfd-of-test-child", NULL, NULL, 0, 2,
"mfd,of-test-child", 0x),
OF_MFD_CELL_REG("mfd-of-test-child", NULL, NULL, 0, 3,
"mfd,of-test-child", 0x)
};
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@
[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@
[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@
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
