The MFD-core iterates through all subdevices of the corresponding
MFD-device and checks, if the devicetree subnode has a fitting compatible.
When nothing is found, a warning is thrown. This can be the case, when it
is the intention to not use the MFD-device to it's full content.
Therefore, change the warning to a debug print instead, to also avoid
irritations.
Signed-off-by: Yunus Bas <[email protected]>
---
drivers/mfd/mfd-core.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/mfd/mfd-core.c b/drivers/mfd/mfd-core.c
index 6f02b8022c6d..e34c97088943 100644
--- a/drivers/mfd/mfd-core.c
+++ b/drivers/mfd/mfd-core.c
@@ -213,7 +213,7 @@ static int mfd_add_device(struct device *parent, int id,
}
if (!pdev->dev.of_node)
- pr_warn("%s: Failed to locate of_node [id: %d]\n",
+ pr_debug("%s: Failed to locate of_node [id: %d]\n",
cell->name, platform_id);
}
--
2.30.0
On Wed, 16 Jun 2021, Yunus Bas wrote:
> The MFD-core iterates through all subdevices of the corresponding
> MFD-device and checks, if the devicetree subnode has a fitting compatible.
> When nothing is found, a warning is thrown. This can be the case, when it
> is the intention to not use the MFD-device to it's full content.
> Therefore, change the warning to a debug print instead, to also avoid
> irritations.
>
> Signed-off-by: Yunus Bas <[email protected]>
> ---
> drivers/mfd/mfd-core.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/mfd/mfd-core.c b/drivers/mfd/mfd-core.c
> index 6f02b8022c6d..e34c97088943 100644
> --- a/drivers/mfd/mfd-core.c
> +++ b/drivers/mfd/mfd-core.c
> @@ -213,7 +213,7 @@ static int mfd_add_device(struct device *parent, int id,
> }
>
> if (!pdev->dev.of_node)
> - pr_warn("%s: Failed to locate of_node [id: %d]\n",
> + pr_debug("%s: Failed to locate of_node [id: %d]\n",
> cell->name, platform_id);
> }
Can you provide an example of a device tree where this is a problem?
Probably worth popping that in the commit message too.
--
Lee Jones [李琼斯]
Senior Technical Lead - Developer Services
Linaro.org │ Open source software for Arm SoCs
Follow Linaro: Facebook | Twitter | Blog
Hi Lee,
Am Mittwoch, dem 16.06.2021 um 10:03 +0100 schrieb Lee Jones:
> On Wed, 16 Jun 2021, Yunus Bas wrote:
>
> > The MFD-core iterates through all subdevices of the corresponding
> > MFD-device and checks, if the devicetree subnode has a fitting
> > compatible.
> > When nothing is found, a warning is thrown. This can be the case,
> > when it
> > is the intention to not use the MFD-device to it's full content.
> > Therefore, change the warning to a debug print instead, to also avoid
> > irritations.
> >
> > Signed-off-by: Yunus Bas <[email protected]>
> > ---
> > drivers/mfd/mfd-core.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/mfd/mfd-core.c b/drivers/mfd/mfd-core.c
> > index 6f02b8022c6d..e34c97088943 100644
> > --- a/drivers/mfd/mfd-core.c
> > +++ b/drivers/mfd/mfd-core.c
> > @@ -213,7 +213,7 @@ static int mfd_add_device(struct device *parent,
> > int id,
> > }
> >
> > if (!pdev->dev.of_node)
> > - pr_warn("%s: Failed to locate of_node [id:
> > %d]\n",
> > + pr_debug("%s: Failed to locate of_node [id:
> > %d]\n",
> > cell->name, platform_id);
> > }
>
> Can you provide an example of a device tree where this is a problem?
Of course, sorry for the poor description.
Here is an example of the imx6qdl-phytec-phycore-som.dtsi which uses
the DA9062 multi-functional device. The DA9062 has five mfd-cell
devices with compatibles defined as subfunctions. The devicetree needs
and uses just three of them:
...
pmic: pmic@58 {
compatible = "dlg,da9062";
pinctrl-names = "default";
pinctrl-0 = <&pinctrl_pmic>;
reg = <0x58>;
interrupt-parent = <&gpio1>;
interrupts = <2 IRQ_TYPE_LEVEL_LOW>;
interrupt-controller;
gpio-controller;
#gpio-cells = <2>;
da9062_rtc: rtc {
compatible = "dlg,da9062-rtc";
};
da9062_onkey: onkey {
compatible = "dlg,da9062-onkey";
};
watchdog {
compatible = "dlg,da9062-watchdog";
dlg,use-sw-pm;
}
...
Since the driver iterates through the mfd_cells-struct tries matching
compatibles in the devicetree MFD node, it throws a warning when there
is no counterpart in the devicetree.
In fact, I could also evalutate oder devicetree's using MFD-devices not
to it's full content.
>
> Probably worth popping that in the commit message too.
>
Yes, I will send a v2 ASAP. Thank you for the advice.
Regards,
Yunus
--
Mit freundlichen Grüßen
Yunus Bas
-Software Engineer-
PHYTEC Messtechnik GmbH
Robert-Koch-Str. 39
55129 Mainz
Germany
Tel.: +49 (0)6131 9221- 466
Web: http://www.phytec.de
Sie finden uns auch auf: Facebook, LinkedIn, Xing, YouTube
PHYTEC Messtechnik GmbH | Robert-Koch-Str. 39 | 55129 Mainz, Germany
Geschäftsführer: Dipl.-Ing. Michael Mitezki, Dipl.-Ing. Bodo Huber |
Handelsregister Mainz HRB 4656 | Finanzamt Mainz | St.Nr. 266500608, DE
149059855
This E-Mail may contain confidential or privileged information. If you
are not the intended recipient (or have received this E-Mail in error)
please notify the sender immediately and destroy this E-Mail. Any
unauthorized copying, disclosure or distribution of the material in
this E-Mail is strictly forbidden.
On Thu, 17 Jun 2021, Yunus Bas wrote:
> Hi Lee,
>
> Am Mittwoch, dem 16.06.2021 um 10:03 +0100 schrieb Lee Jones:
> > On Wed, 16 Jun 2021, Yunus Bas wrote:
> >
> > > The MFD-core iterates through all subdevices of the corresponding
> > > MFD-device and checks, if the devicetree subnode has a fitting
> > > compatible.
> > > When nothing is found, a warning is thrown. This can be the case,
> > > when it
> > > is the intention to not use the MFD-device to it's full content.
> > > Therefore, change the warning to a debug print instead, to also avoid
> > > irritations.
> > >
> > > Signed-off-by: Yunus Bas <[email protected]>
> > > ---
> > > drivers/mfd/mfd-core.c | 2 +-
> > > 1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/mfd/mfd-core.c b/drivers/mfd/mfd-core.c
> > > index 6f02b8022c6d..e34c97088943 100644
> > > --- a/drivers/mfd/mfd-core.c
> > > +++ b/drivers/mfd/mfd-core.c
> > > @@ -213,7 +213,7 @@ static int mfd_add_device(struct device *parent,
> > > int id,
> > > }
> > >
> > > if (!pdev->dev.of_node)
> > > - pr_warn("%s: Failed to locate of_node [id:
> > > %d]\n",
> > > + pr_debug("%s: Failed to locate of_node [id:
> > > %d]\n",
> > > cell->name, platform_id);
> > > }
> >
> > Can you provide an example of a device tree where this is a problem?
>
> Of course, sorry for the poor description.
>
> Here is an example of the imx6qdl-phytec-phycore-som.dtsi which uses
> the DA9062 multi-functional device. The DA9062 has five mfd-cell
> devices with compatibles defined as subfunctions. The devicetree needs
> and uses just three of them:
>
> ...
> pmic: pmic@58 {
> compatible = "dlg,da9062";
> pinctrl-names = "default";
> pinctrl-0 = <&pinctrl_pmic>;
> reg = <0x58>;
> interrupt-parent = <&gpio1>;
> interrupts = <2 IRQ_TYPE_LEVEL_LOW>;
> interrupt-controller;
> gpio-controller;
> #gpio-cells = <2>;
>
> da9062_rtc: rtc {
> compatible = "dlg,da9062-rtc";
> };
>
> da9062_onkey: onkey {
> compatible = "dlg,da9062-onkey";
> };
>
> watchdog {
> compatible = "dlg,da9062-watchdog";
> dlg,use-sw-pm;
> }
> ...
So, looking at the mfd_cells table, I see:
static const struct mfd_cell da9061_devs[] = {
{
.name = "da9061-core",
.num_resources = ARRAY_SIZE(da9061_core_resources),
.resources = da9061_core_resources,
},
{
.name = "da9062-regulators",
.num_resources = ARRAY_SIZE(da9061_regulators_resources),
.resources = da9061_regulators_resources,
},
{
.name = "da9061-watchdog",
.num_resources = ARRAY_SIZE(da9061_wdt_resources),
.resources = da9061_wdt_resources,
.of_compatible = "dlg,da9061-watchdog",
},
{
.name = "da9061-thermal",
.num_resources = ARRAY_SIZE(da9061_thermal_resources),
.resources = da9061_thermal_resources,
.of_compatible = "dlg,da9061-thermal",
},
{
.name = "da9061-onkey",
.num_resources = ARRAY_SIZE(da9061_onkey_resources),
.resources = da9061_onkey_resources,
.of_compatible = "dlg,da9061-onkey",
},
};
Not sure why "da9061-core" is even in there. It looks like this would
be referencing itself (if this driver's name contained the "-core"
element). So what from I can tell, I think this entry should actually
just be removed.
With regards to "da9062-regulators", this looks like an oversight at
best or a Linux hack at worst. Device Tree is designed to be OS
agnostic. It should describe the H/W as-is, which would include the
Regulator functionality. Choosing to opt-out and instead use Linux
specific systems (i.e. MFD) for device registration is a hack.
I've always said we should not mix DT and MFD in this way.
> Since the driver iterates through the mfd_cells-struct tries matching
> compatibles in the devicetree MFD node, it throws a warning when there
> is no counterpart in the devicetree.
>
> In fact, I could also evalutate oder devicetree's using MFD-devices not
> to it's full content.
>
> >
> > Probably worth popping that in the commit message too.
>
> Yes, I will send a v2 ASAP. Thank you for the advice.
I think the current code is fine as it is.
It's the implementation that needs to change.
Maybe Steve would like to comment?
--
Lee Jones [李琼斯]
Senior Technical Lead - Developer Services
Linaro.org │ Open source software for Arm SoCs
Follow Linaro: Facebook | Twitter | Blog
Am Donnerstag, dem 17.06.2021 um 09:27 +0100 schrieb Lee Jones:
> On Thu, 17 Jun 2021, Yunus Bas wrote:
>
> > Hi Lee,
> >
> > Am Mittwoch, dem 16.06.2021 um 10:03 +0100 schrieb Lee Jones:
> > > On Wed, 16 Jun 2021, Yunus Bas wrote:
> > >
> > > > The MFD-core iterates through all subdevices of the corresponding
> > > > MFD-device and checks, if the devicetree subnode has a fitting
> > > > compatible.
> > > > When nothing is found, a warning is thrown. This can be the case,
> > > > when it
> > > > is the intention to not use the MFD-device to it's full content.
> > > > Therefore, change the warning to a debug print instead, to also
> > > > avoid
> > > > irritations.
> > > >
> > > > Signed-off-by: Yunus Bas <[email protected]>
> > > > ---
> > > > drivers/mfd/mfd-core.c | 2 +-
> > > > 1 file changed, 1 insertion(+), 1 deletion(-)
> > > >
> > > > diff --git a/drivers/mfd/mfd-core.c b/drivers/mfd/mfd-core.c
> > > > index 6f02b8022c6d..e34c97088943 100644
> > > > --- a/drivers/mfd/mfd-core.c
> > > > +++ b/drivers/mfd/mfd-core.c
> > > > @@ -213,7 +213,7 @@ static int mfd_add_device(struct device
> > > > *parent,
> > > > int id,
> > > > }
> > > >
> > > > if (!pdev->dev.of_node)
> > > > - pr_warn("%s: Failed to locate of_node
> > > > [id:
> > > > %d]\n",
> > > > + pr_debug("%s: Failed to locate of_node
> > > > [id:
> > > > %d]\n",
> > > > cell->name, platform_id);
> > > > }
> > >
> > > Can you provide an example of a device tree where this is a
> > > problem?
> >
> > Of course, sorry for the poor description.
> >
> > Here is an example of the imx6qdl-phytec-phycore-som.dtsi which uses
> > the DA9062 multi-functional device. The DA9062 has five mfd-cell
> > devices with compatibles defined as subfunctions. The devicetree
> > needs
> > and uses just three of them:
> >
> > ...
> > pmic: pmic@58 {
> > compatible = "dlg,da9062";
> > pinctrl-names = "default";
> > pinctrl-0 = <&pinctrl_pmic>;
> > reg = <0x58>;
> > interrupt-parent = <&gpio1>;
> > interrupts = <2 IRQ_TYPE_LEVEL_LOW>;
> > #gpio-cells = <2>;
> > da9062_rtc: rtc {
> > compatible = "dlg,da9062-rtc";
> >
> > da9062_onkey: onkey {
> > compatible = "dlg,da9062-onkey";
> > watchdog {
> > compatible = "dlg,da9062-watchdog";
> > dlg,use-sw-pm;
> > }
> > ...
>
> So, looking at the mfd_cells table, I see:
>
> static const struct mfd_cell da9061_devs[] = {
> {
> .name = "da9061-core",
> .num_resources = ARRAY_SIZE(da9061_core_resources),
> .resources = da9061_core_resources,
> },
> {
> .name = "da9062-regulators",
> .num_resources =
> ARRAY_SIZE(da9061_regulators_resources),
> .resources = da9061_regulators_resources,
> },
> {
> .name = "da9061-watchdog",
> .num_resources = ARRAY_SIZE(da9061_wdt_resources),
> .resources = da9061_wdt_resources,
> .of_compatible = "dlg,da9061-watchdog",
> },
> {
> .name = "da9061-thermal",
> .num_resources = ARRAY_SIZE(da9061_thermal_resources),
> .resources = da9061_thermal_resources,
> .of_compatible = "dlg,da9061-thermal",
> },
> {
> .name = "da9061-onkey",
> .num_resources = ARRAY_SIZE(da9061_onkey_resources),
> .resources = da9061_onkey_resources,
> .of_compatible = "dlg,da9061-onkey",
> },
> };
First of all, this is the wrong device. Further down is listed a second
machine, the da9062, with more subdevices:
static const struct mfd_cell da9062_devs[] = {
{
.name = "da9062-core",
.num_resources = ARRAY_SIZE(da9062_core_resources),
.resources = da9062_core_resources,
},
{
.name = "da9062-regulators",
.num_resources = ARRAY_SIZE(da9062_regulators_resources),
.resources = da9062_regulators_resources,
},
{
.name = "da9062-watchdog",
.num_resources = ARRAY_SIZE(da9062_wdt_resources),
.resources = da9062_wdt_resources,
.of_compatible = "dlg,da9062-watchdog",
},
{
.name = "da9062-thermal",
.num_resources = ARRAY_SIZE(da9062_thermal_resources),
.resources = da9062_thermal_resources,
.of_compatible = "dlg,da9062-thermal",
},
{
.name = "da9062-rtc",
.num_resources = ARRAY_SIZE(da9062_rtc_resources),
.resources = da9062_rtc_resources,
.of_compatible = "dlg,da9062-rtc",
},
{
.name = "da9062-onkey",
.num_resources = ARRAY_SIZE(da9062_onkey_resources),
.resources = da9062_onkey_resources,
.of_compatible = "dlg,da9062-onkey",
},
{
.name = "da9062-gpio",
.num_resources = ARRAY_SIZE(da9062_gpio_resources),
.resources = da9062_gpio_resources,
.of_compatible = "dlg,da9062-gpio",
},
};
>
> Not sure why "da9061-core" is even in there. It looks like this would
> be referencing itself (if this driver's name contained the "-core"
> element). So what from I can tell, I think this entry should actually
> just be removed.
>
> With regards to "da9062-regulators", this looks like an oversight at
> best or a Linux hack at worst. Device Tree is designed to be OS
> agnostic. It should describe the H/W as-is, which would include the
> Regulator functionality. Choosing to opt-out and instead use Linux
> specific systems (i.e. MFD) for device registration is a hack.
I think you're right here. But this is design specific and has not much
to do with my request.
>
> I've always said we should not mix DT and MFD in this way.
>
> > Since the driver iterates through the mfd_cells-struct tries matching
> > compatibles in the devicetree MFD node, it throws a warning when
> > there
> > is no counterpart in the devicetree.
> >
> > In fact, I could also evalutate oder devicetree's using MFD-devices
> > not
> > to it's full content.
> >
> > >
> > > Probably worth popping that in the commit message too.
> >
> > Yes, I will send a v2 ASAP. Thank you for the advice.
>
> I think the current code is fine as it is.
>
> It's the implementation that needs to change.
>
> Maybe Steve would like to comment?
>
The problem I want to address lies in the mfd_add_device function in
the mfd-core:
if (IS_ENABLED(CONFIG_OF) && parent->of_node && cell-
>of_compatible) {
for_each_child_of_node(parent->of_node, np) {
if (of_device_is_compatible(np, cell->of_compatible)) {
/* Ignore 'disabled' devices error free */
if (!of_device_is_available(np)) {
ret = 0;
goto fail_alias;
}
ret = mfd_match_of_node_to_dev(pdev, np, cell);
if (ret == -EAGAIN)
continue;
if (ret)
goto fail_alias;
break;
}
}
if (!pdev->dev.of_node)
pr_info("%s: Failed to locate of_node [id: %d]\n",
cell->name, platform_id);
}
Interestingly, all subdevices defined in the driver are registered as
platform devices from the MFD framework, regardless of a devicetree
entry or not. The preceding code checks the subdevice cells with an
additional compatible. In case a device has no devicetree entry, an
irritating failed-message is printed on the display. I'm not sure if
this was the intention but the framework somehow forces the users to
describe all subdevices of an MFD. I think the info print is not
needed. It makes more sense to set it as a debug print.
Regards,
Yunus
--
Mit freundlichen Grüßen
Yunus Bas
-Software Entwicklung-
PHYTEC Messtechnik GmbH
Robert-Koch-Str. 39
55129 Mainz
Germany
Tel.: +49 (0)6131 9221-466
Web: http://www.phytec.de
Sie finden uns auch auf: Facebook, LinkedIn, Xing, YouTube
PHYTEC Messtechnik GmbH | Robert-Koch-Str. 39 | 55129 Mainz, Germany
Geschäftsführer: Dipl.-Ing. Michael Mitezki, Dipl.-Ing. Bodo Huber |
Handelsregister Mainz HRB 4656 | Finanzamt Mainz | St.Nr. 266500608, DE
149059855
This E-Mail may contain confidential or privileged information. If you
are not the intended recipient (or have received this E-Mail in error)
please notify the sender immediately and destroy this E-Mail. Any
unauthorized copying, disclosure or distribution of the material in
this E-Mail is strictly forbidden.
On Tue, 29 Jun 2021, Yunus Bas wrote:
> Am Donnerstag, dem 17.06.2021 um 09:27 +0100 schrieb Lee Jones:
> > On Thu, 17 Jun 2021, Yunus Bas wrote:
> >
> > > Hi Lee,
> > >
> > > Am Mittwoch, dem 16.06.2021 um 10:03 +0100 schrieb Lee Jones:
> > > > On Wed, 16 Jun 2021, Yunus Bas wrote:
> > > >
> > > > > The MFD-core iterates through all subdevices of the corresponding
> > > > > MFD-device and checks, if the devicetree subnode has a fitting
> > > > > compatible.
> > > > > When nothing is found, a warning is thrown. This can be the case,
> > > > > when it
> > > > > is the intention to not use the MFD-device to it's full content.
> > > > > Therefore, change the warning to a debug print instead, to also
> > > > > avoid
> > > > > irritations.
> > > > >
> > > > > Signed-off-by: Yunus Bas <[email protected]>
> > > > > ---
> > > > > drivers/mfd/mfd-core.c | 2 +-
> > > > > 1 file changed, 1 insertion(+), 1 deletion(-)
> > > > >
> > > > > diff --git a/drivers/mfd/mfd-core.c b/drivers/mfd/mfd-core.c
> > > > > index 6f02b8022c6d..e34c97088943 100644
> > > > > --- a/drivers/mfd/mfd-core.c
> > > > > +++ b/drivers/mfd/mfd-core.c
> > > > > @@ -213,7 +213,7 @@ static int mfd_add_device(struct device
> > > > > *parent,
> > > > > int id,
> > > > > }
> > > > >
> > > > > if (!pdev->dev.of_node)
> > > > > - pr_warn("%s: Failed to locate of_node
> > > > > [id:
> > > > > %d]\n",
> > > > > + pr_debug("%s: Failed to locate of_node
> > > > > [id:
> > > > > %d]\n",
> > > > > cell->name, platform_id);
> > > > > }
> > > >
> > > > Can you provide an example of a device tree where this is a
> > > > problem?
> > >
> > > Of course, sorry for the poor description.
> > >
> > > Here is an example of the imx6qdl-phytec-phycore-som.dtsi which uses
> > > the DA9062 multi-functional device. The DA9062 has five mfd-cell
> > > devices with compatibles defined as subfunctions. The devicetree
> > > needs
> > > and uses just three of them:
> > >
> > > ...
> > > pmic: pmic@58 {
> > > compatible = "dlg,da9062";
> > > pinctrl-names = "default";
> > > pinctrl-0 = <&pinctrl_pmic>;
> > > reg = <0x58>;
> > > interrupt-parent = <&gpio1>;
> > > interrupts = <2 IRQ_TYPE_LEVEL_LOW>;
> > > #gpio-cells = <2>;
> > > da9062_rtc: rtc {
> > > compatible = "dlg,da9062-rtc";
> > >
> > > da9062_onkey: onkey {
> > > compatible = "dlg,da9062-onkey";
> > > watchdog {
> > > compatible = "dlg,da9062-watchdog";
> > > dlg,use-sw-pm;
> > > }
> > > ...
> >
> > So, looking at the mfd_cells table, I see:
> >
> > static const struct mfd_cell da9061_devs[] = {
> > {
> > .name = "da9061-core",
> > .num_resources = ARRAY_SIZE(da9061_core_resources),
> > .resources = da9061_core_resources,
> > },
> > {
> > .name = "da9062-regulators",
> > .num_resources =
> > ARRAY_SIZE(da9061_regulators_resources),
> > .resources = da9061_regulators_resources,
> > },
> > {
> > .name = "da9061-watchdog",
> > .num_resources = ARRAY_SIZE(da9061_wdt_resources),
> > .resources = da9061_wdt_resources,
> > .of_compatible = "dlg,da9061-watchdog",
> > },
> > {
> > .name = "da9061-thermal",
> > .num_resources = ARRAY_SIZE(da9061_thermal_resources),
> > .resources = da9061_thermal_resources,
> > .of_compatible = "dlg,da9061-thermal",
> > },
> > {
> > .name = "da9061-onkey",
> > .num_resources = ARRAY_SIZE(da9061_onkey_resources),
> > .resources = da9061_onkey_resources,
> > .of_compatible = "dlg,da9061-onkey",
> > },
> > };
>
> First of all, this is the wrong device. Further down is listed a second
> machine, the da9062, with more subdevices:
>
> static const struct mfd_cell da9062_devs[] = {
> {
> .name = "da9062-core",
> .num_resources = ARRAY_SIZE(da9062_core_resources),
> .resources = da9062_core_resources,
> },
> {
> .name = "da9062-regulators",
> .num_resources = ARRAY_SIZE(da9062_regulators_resources),
> .resources = da9062_regulators_resources,
> },
> {
> .name = "da9062-watchdog",
> .num_resources = ARRAY_SIZE(da9062_wdt_resources),
> .resources = da9062_wdt_resources,
> .of_compatible = "dlg,da9062-watchdog",
> },
> {
> .name = "da9062-thermal",
> .num_resources = ARRAY_SIZE(da9062_thermal_resources),
> .resources = da9062_thermal_resources,
> .of_compatible = "dlg,da9062-thermal",
> },
> {
> .name = "da9062-rtc",
> .num_resources = ARRAY_SIZE(da9062_rtc_resources),
> .resources = da9062_rtc_resources,
> .of_compatible = "dlg,da9062-rtc",
> },
> {
> .name = "da9062-onkey",
> .num_resources = ARRAY_SIZE(da9062_onkey_resources),
> .resources = da9062_onkey_resources,
> .of_compatible = "dlg,da9062-onkey",
> },
> {
> .name = "da9062-gpio",
> .num_resources = ARRAY_SIZE(da9062_gpio_resources),
> .resources = da9062_gpio_resources,
> .of_compatible = "dlg,da9062-gpio",
> },
> };
>
> >
> > Not sure why "da9061-core" is even in there. It looks like this would
> > be referencing itself (if this driver's name contained the "-core"
> > element). So what from I can tell, I think this entry should actually
> > just be removed.
> >
> > With regards to "da9062-regulators", this looks like an oversight at
> > best or a Linux hack at worst. Device Tree is designed to be OS
> > agnostic. It should describe the H/W as-is, which would include the
> > Regulator functionality. Choosing to opt-out and instead use Linux
> > specific systems (i.e. MFD) for device registration is a hack.
>
> I think you're right here. But this is design specific and has not much
> to do with my request.
> >
> > I've always said we should not mix DT and MFD in this way.
> >
> > > Since the driver iterates through the mfd_cells-struct tries matching
> > > compatibles in the devicetree MFD node, it throws a warning when
> > > there
> > > is no counterpart in the devicetree.
> > >
> > > In fact, I could also evalutate oder devicetree's using MFD-devices
> > > not
> > > to it's full content.
> > >
> > > >
> > > > Probably worth popping that in the commit message too.
> > >
> > > Yes, I will send a v2 ASAP. Thank you for the advice.
> >
> > I think the current code is fine as it is.
> >
> > It's the implementation that needs to change.
> >
> > Maybe Steve would like to comment?
> >
>
> The problem I want to address lies in the mfd_add_device function in
> the mfd-core:
>
> if (IS_ENABLED(CONFIG_OF) && parent->of_node && cell-
> >of_compatible) {
> for_each_child_of_node(parent->of_node, np) {
> if (of_device_is_compatible(np, cell->of_compatible)) {
> /* Ignore 'disabled' devices error free */
> if (!of_device_is_available(np)) {
> ret = 0;
> goto fail_alias;
> }
>
> ret = mfd_match_of_node_to_dev(pdev, np, cell);
> if (ret == -EAGAIN)
> continue;
> if (ret)
> goto fail_alias;
>
> break;
> }
> }
>
> if (!pdev->dev.of_node)
> pr_info("%s: Failed to locate of_node [id: %d]\n",
> cell->name, platform_id);
> }
>
> Interestingly, all subdevices defined in the driver are registered as
> platform devices from the MFD framework, regardless of a devicetree
> entry or not. The preceding code checks the subdevice cells with an
> additional compatible. In case a device has no devicetree entry, an
> irritating failed-message is printed on the display. I'm not sure if
> this was the intention but the framework somehow forces the users to
> describe all subdevices of an MFD. I think the info print is not
> needed. It makes more sense to set it as a debug print.
My current understanding is that; it's far more likely that the
driver's use of the API is incorrect than the print itself. I need to
spend more time on this (time is no my friend at this present moment)
in order to fully understand what's happening though. Please bear
with me.
--
Lee Jones [李琼斯]
Senior Technical Lead - Developer Services
Linaro.org │ Open source software for Arm SoCs
Follow Linaro: Facebook | Twitter | Blog
Am Dienstag, dem 29.06.2021 um 10:07 +0100 schrieb Lee Jones:
> On Tue, 29 Jun 2021, Yunus Bas wrote:
>
> > Am Donnerstag, dem 17.06.2021 um 09:27 +0100 schrieb Lee Jones:
> > > On Thu, 17 Jun 2021, Yunus Bas wrote:
> > >
> > > > Hi Lee,
> > > >
> > > > Am Mittwoch, dem 16.06.2021 um 10:03 +0100 schrieb Lee Jones:
> > > > > On Wed, 16 Jun 2021, Yunus Bas wrote:
> > > > >
> > > > > > The MFD-core iterates through all subdevices of the
> > > > > > corresponding
> > > > > > MFD-device and checks, if the devicetree subnode has a
> > > > > > fitting
> > > > > > compatible.
> > > > > > When nothing is found, a warning is thrown. This can be the
> > > > > > case,
> > > > > > when it
> > > > > > is the intention to not use the MFD-device to it's full
> > > > > > content.
> > > > > > Therefore, change the warning to a debug print instead, to
> > > > > > also
> > > > > > avoid
> > > > > > irritations.
> > > > > >
> > > > > > Signed-off-by: Yunus Bas <[email protected]>
> > > > > > ---
> > > > > > drivers/mfd/mfd-core.c | 2 +-
> > > > > > 1 file changed, 1 insertion(+), 1 deletion(-)
> > > > > >
> > > > > > diff --git a/drivers/mfd/mfd-core.c b/drivers/mfd/mfd-
> > > > > > core.c
> > > > > > index 6f02b8022c6d..e34c97088943 100644
> > > > > > --- a/drivers/mfd/mfd-core.c
> > > > > > +++ b/drivers/mfd/mfd-core.c
> > > > > > @@ -213,7 +213,7 @@ static int mfd_add_device(struct device
> > > > > > *parent,
> > > > > > int id,
> > > > > > }
> > > > > >
> > > > > > if (!pdev->dev.of_node)
> > > > > > - pr_warn("%s: Failed to locate
> > > > > > of_node
> > > > > > [id:
> > > > > > %d]\n",
> > > > > > + pr_debug("%s: Failed to locate
> > > > > > of_node
> > > > > > [id:
> > > > > > %d]\n",
> > > > > > cell->name, platform_id);
> > > > > > }
> > > > >
> > > > > Can you provide an example of a device tree where this is a
> > > > > problem?
> > > >
> > > > Of course, sorry for the poor description.
> > > >
> > > > Here is an example of the imx6qdl-phytec-phycore-som.dtsi which
> > > > uses
> > > > the DA9062 multi-functional device. The DA9062 has five mfd-
> > > > cell
> > > > devices with compatibles defined as subfunctions. The
> > > > devicetree
> > > > needs
> > > > and uses just three of them:
> > > >
> > > > ...
> > > > pmic: pmic@58
> > > > {
> > > > compatible =
> > > > "dlg,da9062";
> > > > pinctrl-names =
> > > > "default";
> > > > pinctrl-0 =
> > > > <&pinctrl_pmic>;
> > > > reg =
> > > > <0x58>;
> > > > interrupt-parent =
> > > > <&gpio1>;
> > > > interrupts = <2
> > > > IRQ_TYPE_LEVEL_LOW>;
> > > > #gpio-cells =
> > > > <2>;
> > > > da9062_rtc: rtc
> > > > {
> > > > compatible = "dlg,da9062-
> > > > rtc";
> > > >
> > > > da9062_onkey: onkey
> > > > {
> > > > compatible = "dlg,da9062-
> > > > onkey";
> > > > watchdog
> > > > {
> > > > compatible = "dlg,da9062-
> > > > watchdog";
> > > > dlg,use-sw-
> > > > pm;
> > > > }
> > > > ...
> > >
> > > So, looking at the mfd_cells table, I see:
> > >
> > > static const struct mfd_cell da9061_devs[] = {
> > > {
> > > .name = "da9061-core",
> > > .num_resources =
> > > ARRAY_SIZE(da9061_core_resources),
> > > .resources = da9061_core_resources,
> > > },
> > > {
> > > .name = "da9062-regulators",
> > > .num_resources =
> > > ARRAY_SIZE(da9061_regulators_resources),
> > > .resources = da9061_regulators_resources,
> > > },
> > > {
> > > .name = "da9061-watchdog",
> > > .num_resources =
> > > ARRAY_SIZE(da9061_wdt_resources),
> > > .resources = da9061_wdt_resources,
> > > .of_compatible = "dlg,da9061-watchdog",
> > > },
> > > {
> > > .name = "da9061-thermal",
> > > .num_resources =
> > > ARRAY_SIZE(da9061_thermal_resources),
> > > .resources = da9061_thermal_resources,
> > > .of_compatible = "dlg,da9061-thermal",
> > > },
> > > {
> > > .name = "da9061-onkey",
> > > .num_resources =
> > > ARRAY_SIZE(da9061_onkey_resources),
> > > .resources = da9061_onkey_resources,
> > > .of_compatible = "dlg,da9061-onkey",
> > > },
> > > };
> >
> > First of all, this is the wrong device. Further down is listed a
> > second
> > machine, the da9062, with more subdevices:
> >
> > static const struct mfd_cell da9062_devs[] = {
> > {
> > .name = "da9062-core",
> > .num_resources = ARRAY_SIZE(da9062_core_resources),
> > .resources = da9062_core_resources,
> > },
> > {
> > .name = "da9062-regulators",
> > .num_resources = ARRAY_SIZE(da9062_regulators_resources),
> > .resources = da9062_regulators_resources,
> > },
> > {
> > .name = "da9062-watchdog",
> > .num_resources = ARRAY_SIZE(da9062_wdt_resources),
> > .resources = da9062_wdt_resources,
> > .of_compatible = "dlg,da9062-watchdog",
> > },
> > {
> > .name = "da9062-thermal",
> > .num_resources = ARRAY_SIZE(da9062_thermal_resources),
> > .resources = da9062_thermal_resources,
> > .of_compatible = "dlg,da9062-thermal",
> > },
> > {
> > .name = "da9062-rtc",
> > .num_resources = ARRAY_SIZE(da9062_rtc_resources),
> > .resources = da9062_rtc_resources,
> > .of_compatible = "dlg,da9062-rtc",
> > },
> > {
> > .name = "da9062-onkey",
> > .num_resources = ARRAY_SIZE(da9062_onkey_resources),
> > .resources = da9062_onkey_resources,
> > .of_compatible = "dlg,da9062-onkey",
> > },
> > {
> > .name = "da9062-gpio",
> > .num_resources = ARRAY_SIZE(da9062_gpio_resources),
> > .resources = da9062_gpio_resources,
> > .of_compatible = "dlg,da9062-gpio",
> > },
> > };
> >
> > >
> > > Not sure why "da9061-core" is even in there. It looks like this
> > > would
> > > be referencing itself (if this driver's name contained the "-
> > > core"
> > > element). So what from I can tell, I think this entry should
> > > actually
> > > just be removed.
> > >
> > > With regards to "da9062-regulators", this looks like an oversight
> > > at
> > > best or a Linux hack at worst. Device Tree is designed to be OS
> > > agnostic. It should describe the H/W as-is, which would include
> > > the
> > > Regulator functionality. Choosing to opt-out and instead use
> > > Linux
> > > specific systems (i.e. MFD) for device registration is a hack.
> >
> > I think you're right here. But this is design specific and has not
> > much
> > to do with my request.
> > >
> > > I've always said we should not mix DT and MFD in this way.
> > >
> > > > Since the driver iterates through the mfd_cells-struct tries
> > > > matching
> > > > compatibles in the devicetree MFD node, it throws a warning
> > > > when
> > > > there
> > > > is no counterpart in the devicetree.
> > > >
> > > > In fact, I could also evalutate oder devicetree's using MFD-
> > > > devices
> > > > not
> > > > to it's full content.
> > > >
> > > > >
> > > > > Probably worth popping that in the commit message too.
> > > >
> > > > Yes, I will send a v2 ASAP. Thank you for the advice.
> > >
> > > I think the current code is fine as it is.
> > >
> > > It's the implementation that needs to change.
> > >
> > > Maybe Steve would like to comment?
> > >
> >
> > The problem I want to address lies in the mfd_add_device function
> > in
> > the mfd-core:
> >
> > if (IS_ENABLED(CONFIG_OF) && parent->of_node && cell-
> > > of_compatible) {
> > for_each_child_of_node(parent->of_node, np)
> > {
> > if (of_device_is_compatible(np, cell->of_compatible))
> > {
> > /* Ignore 'disabled' devices error free
> > */
> > if (!of_device_is_available(np))
> > {
> > ret =
> > 0;
> > goto
> > fail_alias;
> >
> > }
> >
> >
> > ret = mfd_match_of_node_to_dev(pdev, np,
> > cell);
> > if (ret == -
> > EAGAIN)
> >
> > continue;
> > if
> > (ret)
> > goto
> > fail_alias;
> >
> >
> >
> > break;
> >
> > }
> >
> > }
> >
> >
> > if (!pdev-
> > >dev.of_node)
> > pr_info("%s: Failed to locate of_node [id:
> > %d]\n",
> > cell->name,
> > platform_id);
> > }
> >
> > Interestingly, all subdevices defined in the driver are registered
> > as
> > platform devices from the MFD framework, regardless of a devicetree
> > entry or not. The preceding code checks the subdevice cells with an
> > additional compatible. In case a device has no devicetree entry, an
> > irritating failed-message is printed on the display. I'm not sure
> > if
> > this was the intention but the framework somehow forces the users
> > to
> > describe all subdevices of an MFD. I think the info print is not
> > needed. It makes more sense to set it as a debug print.
>
> My current understanding is that; it's far more likely that the
> driver's use of the API is incorrect than the print itself. I need
> to
> spend more time on this (time is no my friend at this present moment)
> in order to fully understand what's happening though. Please bear
> with me.
Oh sorry, it wasn't my intention to push you :). Please take your time.
--
Mit freundlichen Grüßen
Yunus Bas
-Software Engineer-
PHYTEC Messtechnik GmbH
Robert-Koch-Str. 39
55129 Mainz
Germany
Tel.: +49 (0)6131 9221- 466
Web: http://www.phytec.de
Sie finden uns auch auf: Facebook, LinkedIn, Xing, YouTube
PHYTEC Messtechnik GmbH | Robert-Koch-Str. 39 | 55129 Mainz, Germany
Geschäftsführer: Dipl.-Ing. Michael Mitezki, Dipl.-Ing. Bodo Huber |
Handelsregister Mainz HRB 4656 | Finanzamt Mainz | St.Nr. 266500608, DE
149059855
This E-Mail may contain confidential or privileged information. If you
are not the intended recipient (or have received this E-Mail in error)
please notify the sender immediately and destroy this E-Mail. Any
unauthorized copying, disclosure or distribution of the material in
this E-Mail is strictly forbidden.
On Tue, 29 Jun 2021, Yunus Bas wrote:
> Am Donnerstag, dem 17.06.2021 um 09:27 +0100 schrieb Lee Jones:
> > On Thu, 17 Jun 2021, Yunus Bas wrote:
> >
> > > Hi Lee,
> > >
> > > Am Mittwoch, dem 16.06.2021 um 10:03 +0100 schrieb Lee Jones:
> > > > On Wed, 16 Jun 2021, Yunus Bas wrote:
> > > >
> > > > > The MFD-core iterates through all subdevices of the corresponding
> > > > > MFD-device and checks, if the devicetree subnode has a fitting
> > > > > compatible.
> > > > > When nothing is found, a warning is thrown. This can be the case,
> > > > > when it
> > > > > is the intention to not use the MFD-device to it's full content.
> > > > > Therefore, change the warning to a debug print instead, to also
> > > > > avoid
> > > > > irritations.
> > > > >
> > > > > Signed-off-by: Yunus Bas <[email protected]>
> > > > > ---
> > > > > drivers/mfd/mfd-core.c | 2 +-
> > > > > 1 file changed, 1 insertion(+), 1 deletion(-)
> > > > >
> > > > > diff --git a/drivers/mfd/mfd-core.c b/drivers/mfd/mfd-core.c
> > > > > index 6f02b8022c6d..e34c97088943 100644
> > > > > --- a/drivers/mfd/mfd-core.c
> > > > > +++ b/drivers/mfd/mfd-core.c
> > > > > @@ -213,7 +213,7 @@ static int mfd_add_device(struct device
> > > > > *parent,
> > > > > int id,
> > > > > }
> > > > >
> > > > > if (!pdev->dev.of_node)
> > > > > - pr_warn("%s: Failed to locate of_node
> > > > > [id:
> > > > > %d]\n",
> > > > > + pr_debug("%s: Failed to locate of_node
> > > > > [id:
> > > > > %d]\n",
> > > > > cell->name, platform_id);
> > > > > }
> > > >
> > > > Can you provide an example of a device tree where this is a
> > > > problem?
> > >
> > > Of course, sorry for the poor description.
> > >
> > > Here is an example of the imx6qdl-phytec-phycore-som.dtsi which uses
> > > the DA9062 multi-functional device. The DA9062 has five mfd-cell
> > > devices with compatibles defined as subfunctions. The devicetree
> > > needs
> > > and uses just three of them:
> > >
> > > ...
> > > pmic: pmic@58 {
> > > compatible = "dlg,da9062";
> > > pinctrl-names = "default";
> > > pinctrl-0 = <&pinctrl_pmic>;
> > > reg = <0x58>;
> > > interrupt-parent = <&gpio1>;
> > > interrupts = <2 IRQ_TYPE_LEVEL_LOW>;
> > > #gpio-cells = <2>;
> > > da9062_rtc: rtc {
> > > compatible = "dlg,da9062-rtc";
> > >
> > > da9062_onkey: onkey {
> > > compatible = "dlg,da9062-onkey";
> > > watchdog {
> > > compatible = "dlg,da9062-watchdog";
> > > dlg,use-sw-pm;
> > > }
> > > ...
> >
> > So, looking at the mfd_cells table, I see:
> >
> > static const struct mfd_cell da9061_devs[] = {
> > {
> > .name = "da9061-core",
> > .num_resources = ARRAY_SIZE(da9061_core_resources),
> > .resources = da9061_core_resources,
> > },
> > {
> > .name = "da9062-regulators",
> > .num_resources =
> > ARRAY_SIZE(da9061_regulators_resources),
> > .resources = da9061_regulators_resources,
> > },
> > {
> > .name = "da9061-watchdog",
> > .num_resources = ARRAY_SIZE(da9061_wdt_resources),
> > .resources = da9061_wdt_resources,
> > .of_compatible = "dlg,da9061-watchdog",
> > },
> > {
> > .name = "da9061-thermal",
> > .num_resources = ARRAY_SIZE(da9061_thermal_resources),
> > .resources = da9061_thermal_resources,
> > .of_compatible = "dlg,da9061-thermal",
> > },
> > {
> > .name = "da9061-onkey",
> > .num_resources = ARRAY_SIZE(da9061_onkey_resources),
> > .resources = da9061_onkey_resources,
> > .of_compatible = "dlg,da9061-onkey",
> > },
> > };
>
> First of all, this is the wrong device. Further down is listed a second
> machine, the da9062, with more subdevices:
>
> static const struct mfd_cell da9062_devs[] = {
> {
> .name = "da9062-core",
> .num_resources = ARRAY_SIZE(da9062_core_resources),
> .resources = da9062_core_resources,
> },
> {
> .name = "da9062-regulators",
> .num_resources = ARRAY_SIZE(da9062_regulators_resources),
> .resources = da9062_regulators_resources,
> },
> {
> .name = "da9062-watchdog",
> .num_resources = ARRAY_SIZE(da9062_wdt_resources),
> .resources = da9062_wdt_resources,
> .of_compatible = "dlg,da9062-watchdog",
> },
> {
> .name = "da9062-thermal",
> .num_resources = ARRAY_SIZE(da9062_thermal_resources),
> .resources = da9062_thermal_resources,
> .of_compatible = "dlg,da9062-thermal",
> },
> {
> .name = "da9062-rtc",
> .num_resources = ARRAY_SIZE(da9062_rtc_resources),
> .resources = da9062_rtc_resources,
> .of_compatible = "dlg,da9062-rtc",
> },
> {
> .name = "da9062-onkey",
> .num_resources = ARRAY_SIZE(da9062_onkey_resources),
> .resources = da9062_onkey_resources,
> .of_compatible = "dlg,da9062-onkey",
> },
> {
> .name = "da9062-gpio",
> .num_resources = ARRAY_SIZE(da9062_gpio_resources),
> .resources = da9062_gpio_resources,
> .of_compatible = "dlg,da9062-gpio",
> },
> };
So again the issues here are -core and -regulators, right?
Is that where you're seeing the warnings?
> > Not sure why "da9061-core" is even in there. It looks like this would
> > be referencing itself (if this driver's name contained the "-core"
> > element). So what from I can tell, I think this entry should actually
> > just be removed.
> >
> > With regards to "da9062-regulators", this looks like an oversight at
> > best or a Linux hack at worst. Device Tree is designed to be OS
> > agnostic. It should describe the H/W as-is, which would include the
> > Regulator functionality. Choosing to opt-out and instead use Linux
> > specific systems (i.e. MFD) for device registration is a hack.
So all of this is still correct.
> I think you're right here. But this is design specific and has not much
> to do with my request.
On the contrary.
Your request is to consciously ignore the hack I describe here.
> > I've always said we should not mix DT and MFD in this way.
> >
> > > Since the driver iterates through the mfd_cells-struct tries matching
> > > compatibles in the devicetree MFD node, it throws a warning when
> > > there
> > > is no counterpart in the devicetree.
> > >
> > > In fact, I could also evalutate oder devicetree's using MFD-devices
> > > not
> > > to it's full content.
> > >
> > > >
> > > > Probably worth popping that in the commit message too.
> > >
> > > Yes, I will send a v2 ASAP. Thank you for the advice.
> >
> > I think the current code is fine as it is.
> >
> > It's the implementation that needs to change.
> >
> > Maybe Steve would like to comment?
> >
>
> The problem I want to address lies in the mfd_add_device function in
> the mfd-core:
>
> if (IS_ENABLED(CONFIG_OF) && parent->of_node && cell-
> >of_compatible) {
> for_each_child_of_node(parent->of_node, np) {
> if (of_device_is_compatible(np, cell->of_compatible)) {
> /* Ignore 'disabled' devices error free */
> if (!of_device_is_available(np)) {
> ret = 0;
> goto fail_alias;
> }
>
> ret = mfd_match_of_node_to_dev(pdev, np, cell);
> if (ret == -EAGAIN)
> continue;
> if (ret)
> goto fail_alias;
>
> break;
> }
> }
>
> if (!pdev->dev.of_node)
> pr_info("%s: Failed to locate of_node [id: %d]\n",
> cell->name, platform_id);
> }
>
> Interestingly, all subdevices defined in the driver are registered as
> platform devices from the MFD framework, regardless of a devicetree
> entry or not. The preceding code checks the subdevice cells with an
> additional compatible. In case a device has no devicetree entry, an
> irritating failed-message is printed on the display. I'm not sure if
> this was the intention but the framework somehow forces the users to
> describe all subdevices of an MFD. I think the info print is not
> needed. It makes more sense to set it as a debug print.
Actually, this has served to highlight that your DTS is not correct.
Why are some devices represented in DT and some aren't?
If anything, I'm tempted to upgrade the info() print to warn().
--
Lee Jones [李琼斯]
Senior Technical Lead - Developer Services
Linaro.org │ Open source software for Arm SoCs
Follow Linaro: Facebook | Twitter | Blog
Am Dienstag, dem 29.06.2021 um 14:39 +0100 schrieb Lee Jones:
> On Tue, 29 Jun 2021, Yunus Bas wrote:
>
> > Am Donnerstag, dem 17.06.2021 um 09:27 +0100 schrieb Lee Jones:
> > > On Thu, 17 Jun 2021, Yunus Bas wrote:
> > >
> > > > Hi Lee,
> > > >
> > > > Am Mittwoch, dem 16.06.2021 um 10:03 +0100 schrieb Lee Jones:
> > > > > On Wed, 16 Jun 2021, Yunus Bas wrote:
> > > > >
> > > > > > The MFD-core iterates through all subdevices of the
> > > > > > corresponding
> > > > > > MFD-device and checks, if the devicetree subnode has a
> > > > > > fitting
> > > > > > compatible.
> > > > > > When nothing is found, a warning is thrown. This can be the
> > > > > > case,
> > > > > > when it
> > > > > > is the intention to not use the MFD-device to it's full
> > > > > > content.
> > > > > > Therefore, change the warning to a debug print instead, to
> > > > > > also
> > > > > > avoid
> > > > > > irritations.
> > > > > >
> > > > > > Signed-off-by: Yunus Bas <[email protected]>
> > > > > > ---
> > > > > > drivers/mfd/mfd-core.c | 2 +-
> > > > > > 1 file changed, 1 insertion(+), 1 deletion(-)
> > > > > >
> > > > > > diff --git a/drivers/mfd/mfd-core.c b/drivers/mfd/mfd-
> > > > > > core.c
> > > > > > index 6f02b8022c6d..e34c97088943 100644
> > > > > > --- a/drivers/mfd/mfd-core.c
> > > > > > +++ b/drivers/mfd/mfd-core.c
> > > > > > @@ -213,7 +213,7 @@ static int mfd_add_device(struct device
> > > > > > *parent,
> > > > > > int id,
> > > > > > }
> > > > > >
> > > > > > if (!pdev->dev.of_node)
> > > > > > - pr_warn("%s: Failed to locate
> > > > > > of_node
> > > > > > [id:
> > > > > > %d]\n",
> > > > > > + pr_debug("%s: Failed to locate
> > > > > > of_node
> > > > > > [id:
> > > > > > %d]\n",
> > > > > > cell->name, platform_id);
> > > > > > }
> > > > >
> > > > > Can you provide an example of a device tree where this is a
> > > > > problem?
> > > >
> > > > Of course, sorry for the poor description.
> > > >
> > > > Here is an example of the imx6qdl-phytec-phycore-som.dtsi which
> > > > uses
> > > > the DA9062 multi-functional device. The DA9062 has five mfd-
> > > > cell
> > > > devices with compatibles defined as subfunctions. The
> > > > devicetree
> > > > needs
> > > > and uses just three of them:
> > > >
> > > > ...
> > > > pmic: pmic@58
> > > > {
> > > > compatible =
> > > > "dlg,da9062";
> > > > pinctrl-names =
> > > > "default";
> > > > pinctrl-0 =
> > > > <&pinctrl_pmic>;
> > > > reg =
> > > > <0x58>;
> > > > interrupt-parent =
> > > > <&gpio1>;
> > > > interrupts = <2
> > > > IRQ_TYPE_LEVEL_LOW>;
> > > > #gpio-cells =
> > > > <2>;
> > > > da9062_rtc: rtc
> > > > {
> > > > compatible = "dlg,da9062-
> > > > rtc";
> > > >
> > > > da9062_onkey: onkey
> > > > {
> > > > compatible = "dlg,da9062-
> > > > onkey";
> > > > watchdog
> > > > {
> > > > compatible = "dlg,da9062-
> > > > watchdog";
> > > > dlg,use-sw-
> > > > pm;
> > > > }
> > > > ...
> > >
> > > So, looking at the mfd_cells table, I see:
> > >
> > > static const struct mfd_cell da9061_devs[] = {
> > > {
> > > .name = "da9061-core",
> > > .num_resources =
> > > ARRAY_SIZE(da9061_core_resources),
> > > .resources = da9061_core_resources,
> > > },
> > > {
> > > .name = "da9062-regulators",
> > > .num_resources =
> > > ARRAY_SIZE(da9061_regulators_resources),
> > > .resources = da9061_regulators_resources,
> > > },
> > > {
> > > .name = "da9061-watchdog",
> > > .num_resources =
> > > ARRAY_SIZE(da9061_wdt_resources),
> > > .resources = da9061_wdt_resources,
> > > .of_compatible = "dlg,da9061-watchdog",
> > > },
> > > {
> > > .name = "da9061-thermal",
> > > .num_resources =
> > > ARRAY_SIZE(da9061_thermal_resources),
> > > .resources = da9061_thermal_resources,
> > > .of_compatible = "dlg,da9061-thermal",
> > > },
> > > {
> > > .name = "da9061-onkey",
> > > .num_resources =
> > > ARRAY_SIZE(da9061_onkey_resources),
> > > .resources = da9061_onkey_resources,
> > > .of_compatible = "dlg,da9061-onkey",
> > > },
> > > };
> >
> > First of all, this is the wrong device. Further down is listed a
> > second
> > machine, the da9062, with more subdevices:
> >
> > static const struct mfd_cell da9062_devs[] = {
> > {
> > .name = "da9062-core",
> > .num_resources = ARRAY_SIZE(da9062_core_resources),
> > .resources = da9062_core_resources,
> > },
> > {
> > .name = "da9062-regulators",
> > .num_resources = ARRAY_SIZE(da9062_regulators_resources),
> > .resources = da9062_regulators_resources,
> > },
> > {
> > .name = "da9062-watchdog",
> > .num_resources = ARRAY_SIZE(da9062_wdt_resources),
> > .resources = da9062_wdt_resources,
> > .of_compatible = "dlg,da9062-watchdog",
> > },
> > {
> > .name = "da9062-thermal",
> > .num_resources = ARRAY_SIZE(da9062_thermal_resources),
> > .resources = da9062_thermal_resources,
> > .of_compatible = "dlg,da9062-thermal",
> > },
> > {
> > .name = "da9062-rtc",
> > .num_resources = ARRAY_SIZE(da9062_rtc_resources),
> > .resources = da9062_rtc_resources,
> > .of_compatible = "dlg,da9062-rtc",
> > },
> > {
> > .name = "da9062-onkey",
> > .num_resources = ARRAY_SIZE(da9062_onkey_resources),
> > .resources = da9062_onkey_resources,
> > .of_compatible = "dlg,da9062-onkey",
> > },
> > {
> > .name = "da9062-gpio",
> > .num_resources = ARRAY_SIZE(da9062_gpio_resources),
> > .resources = da9062_gpio_resources,
> > .of_compatible = "dlg,da9062-gpio",
> > },
> > };
>
> So again the issues here are -core and -regulators, right?
>
> Is that where you're seeing the warnings?
No, it's not.
>
> > > Not sure why "da9061-core" is even in there. It looks like this
> > > would
> > > be referencing itself (if this driver's name contained the "-
> > > core"
> > > element). So what from I can tell, I think this entry should
> > > actually
> > > just be removed.
> > >
> > > With regards to "da9062-regulators", this looks like an oversight
> > > at
> > > best or a Linux hack at worst. Device Tree is designed to be OS
> > > agnostic. It should describe the H/W as-is, which would include
> > > the
> > > Regulator functionality. Choosing to opt-out and instead use
> > > Linux
> > > specific systems (i.e. MFD) for device registration is a hack.
>
> So all of this is still correct.
>
> > I think you're right here. But this is design specific and has not
> > much
> > to do with my request.
>
> On the contrary.
>
> Your request is to consciously ignore the hack I describe here.
No. This is not about DA9061 or it's mfd_cell-entries at all. My
concern is about the general behaviour of the MFD-framework and how
mfd_cell entries with compatibles are initialized.
>
> > > I've always said we should not mix DT and MFD in this way.
> > >
> > > > Since the driver iterates through the mfd_cells-struct tries
> > > > matching
> > > > compatibles in the devicetree MFD node, it throws a warning
> > > > when
> > > > there
> > > > is no counterpart in the devicetree.
> > > >
> > > > In fact, I could also evalutate oder devicetree's using MFD-
> > > > devices
> > > > not
> > > > to it's full content.
> > > >
> > > > >
> > > > > Probably worth popping that in the commit message too.
> > > >
> > > > Yes, I will send a v2 ASAP. Thank you for the advice.
> > >
> > > I think the current code is fine as it is.
> > >
> > > It's the implementation that needs to change.
> > >
> > > Maybe Steve would like to comment?
> > >
> >
> > The problem I want to address lies in the mfd_add_device function
> > in
> > the mfd-core:
> >
> > if (IS_ENABLED(CONFIG_OF) && parent->of_node && cell-
> > > of_compatible) {
> > for_each_child_of_node(parent->of_node, np)
> > {
> > if (of_device_is_compatible(np, cell->of_compatible))
> > {
> > /* Ignore 'disabled' devices error free
> > */
> > if (!of_device_is_available(np))
> > {
> > ret =
> > 0;
> > goto
> > fail_alias;
> >
> > }
> >
> >
> > ret = mfd_match_of_node_to_dev(pdev, np,
> > cell);
> > if (ret == -
> > EAGAIN)
> >
> > continue;
> > if
> > (ret)
> > goto
> > fail_alias;
> >
> >
> >
> > break;
> >
> > }
> >
> > }
> >
> >
> > if (!pdev-
> > >dev.of_node)
> > pr_info("%s: Failed to locate of_node [id:
> > %d]\n",
> > cell->name,
> > platform_id);
> > }
> >
> > Interestingly, all subdevices defined in the driver are registered
> > as
> > platform devices from the MFD framework, regardless of a devicetree
> > entry or not. The preceding code checks the subdevice cells with an
> > additional compatible. In case a device has no devicetree entry, an
> > irritating failed-message is printed on the display. I'm not sure
> > if
> > this was the intention but the framework somehow forces the users
> > to
> > describe all subdevices of an MFD. I think the info print is not
> > needed. It makes more sense to set it as a debug print.
>
> Actually, this has served to highlight that your DTS is not correct.
>
> Why are some devices represented in DT and some aren't?
>
> If anything, I'm tempted to upgrade the info() print to warn().
>
Imagine only required parts of the MFD is connected to the designed
system and unrequired parts are not. In that case, fully describing the
MFD in the devicetree wouldn't represent the system at all.
Actually it would make more sense to check if mfd_cell-entries with
compatibles are represented in the devicetree and add them after
matching. This way the warning would serve it's purpose. What do you
think of it?
--
Mit freundlichen Grüßen
Yunus Bas
-Software Engineer-
PHYTEC Messtechnik GmbH
Robert-Koch-Str. 39
55129 Mainz
Germany
Tel.: +49 (0)6131 9221- 466
Web: http://www.phytec.de
Sie finden uns auch auf: Facebook, LinkedIn, Xing, YouTube
PHYTEC Messtechnik GmbH | Robert-Koch-Str. 39 | 55129 Mainz, Germany
Geschäftsführer: Dipl.-Ing. Michael Mitezki, Dipl.-Ing. Bodo Huber |
Handelsregister Mainz HRB 4656 | Finanzamt Mainz | St.Nr. 266500608, DE
149059855
This E-Mail may contain confidential or privileged information. If you
are not the intended recipient (or have received this E-Mail in error)
please notify the sender immediately and destroy this E-Mail. Any
unauthorized copying, disclosure or distribution of the material in
this E-Mail is strictly forbidden.
> No. This is not about DA9061 or it's mfd_cell-entries at all. My
> concern is about the general behaviour of the MFD-framework and how
> mfd_cell entries with compatibles are initialized.
I'm still not 100% sure I understand your use-case.
Let's take this back to basics.
What device are you trying to instantiate? A DA9062 derivative?
> > Actually, this has served to highlight that your DTS is not correct.
> >
> > Why are some devices represented in DT and some aren't?
> >
> > If anything, I'm tempted to upgrade the info() print to warn().
> >
>
> Imagine only required parts of the MFD is connected to the designed
> system and unrequired parts are not. In that case, fully describing the
> MFD in the devicetree wouldn't represent the system at all.
>
> Actually it would make more sense to check if mfd_cell-entries with
> compatibles are represented in the devicetree and add them after
> matching. This way the warning would serve it's purpose. What do you
> think of it?
I think the DT and MFD should match, so again, the way I currently
view this, doing it this way is just a different way to paper over the
cracks.
--
Lee Jones [李琼斯]
Senior Technical Lead - Developer Services
Linaro.org │ Open source software for Arm SoCs
Follow Linaro: Facebook | Twitter | Blog
On Wed, Jun 30, 2021 at 07:27:32AM +0000, Yunus Bas wrote:
> Am Dienstag, dem 29.06.2021 um 14:39 +0100 schrieb Lee Jones:
> > On Tue, 29 Jun 2021, Yunus Bas wrote:
> > > Interestingly, all subdevices defined in the driver are registered
> > > as platform devices from the MFD framework, regardless of a
> > > devicetree entry or not. The preceding code checks the subdevice
> > > cells with an additional compatible. In case a device has no
> > > devicetree entry, an irritating failed-message is printed on the
> > > display. I'm not sure if this was the intention but the framework
> > > somehow forces the users to describe all subdevices of an MFD. I
> > > think the info print is not needed. It makes more sense to set it
> > > as a debug print.
> >
> > Actually, this has served to highlight that your DTS is not correct.
> >
> > Why are some devices represented in DT and some aren't?
> >
> > If anything, I'm tempted to upgrade the info() print to warn().
>
> Imagine only required parts of the MFD is connected to the designed
> system and unrequired parts are not. In that case, fully describing the
> MFD in the devicetree wouldn't represent the system at all.
To describe hardware that is present but unused we would normally use
status = "disabled".
So if, for example, your board cannot use the RTC for some reason
(perhaps the board has no 32KHz oscillator?) then the DA9062 still
contains the hardware but it is useless. Such hardware could be
described as:
da9062_rtc: rtc {
compatible = "dlg,da9062-rtc";
status = "disabled";
}
Is this sufficient to suppress the warnings when the hardware is not
fully described?
There is almost certainly a problem here since there is a mismatch
between mfd-core and the DA9062 DT bindings. mfd-core warns when the
hardware description is incomplete and the DA9062 (and generic mfd) DT
bindings are ambiguous about whether sub-nodes are mandatory and include
an example that contains missing compatibles rather than disabled nodes
like the above.
However it is not entirely clear to me at this point whether this should
be fixed in mfd-core or by improving the bindings documentation.
Daniel.
On Wed, 30 Jun 2021, Daniel Thompson wrote:
> On Wed, Jun 30, 2021 at 07:27:32AM +0000, Yunus Bas wrote:
> > Am Dienstag, dem 29.06.2021 um 14:39 +0100 schrieb Lee Jones:
> > > On Tue, 29 Jun 2021, Yunus Bas wrote:
> > > > Interestingly, all subdevices defined in the driver are registered
> > > > as platform devices from the MFD framework, regardless of a
> > > > devicetree entry or not. The preceding code checks the subdevice
> > > > cells with an additional compatible. In case a device has no
> > > > devicetree entry, an irritating failed-message is printed on the
> > > > display. I'm not sure if this was the intention but the framework
> > > > somehow forces the users to describe all subdevices of an MFD. I
> > > > think the info print is not needed. It makes more sense to set it
> > > > as a debug print.
> > >
> > > Actually, this has served to highlight that your DTS is not correct.
> > >
> > > Why are some devices represented in DT and some aren't?
> > >
> > > If anything, I'm tempted to upgrade the info() print to warn().
> >
> > Imagine only required parts of the MFD is connected to the designed
> > system and unrequired parts are not. In that case, fully describing the
> > MFD in the devicetree wouldn't represent the system at all.
>
> To describe hardware that is present but unused we would normally use
> status = "disabled".
>
> So if, for example, your board cannot use the RTC for some reason
> (perhaps the board has no 32KHz oscillator?) then the DA9062 still
> contains the hardware but it is useless. Such hardware could be
> described as:
>
> da9062_rtc: rtc {
> compatible = "dlg,da9062-rtc";
> status = "disabled";
> }
>
> Is this sufficient to suppress the warnings when the hardware is not
> fully described?
>
> There is almost certainly a problem here since there is a mismatch
> between mfd-core and the DA9062 DT bindings. mfd-core warns when the
> hardware description is incomplete and the DA9062 (and generic mfd) DT
> bindings are ambiguous about whether sub-nodes are mandatory and include
> an example that contains missing compatibles rather than disabled nodes
> like the above.
>
> However it is not entirely clear to me at this point whether this should
> be fixed in mfd-core or by improving the bindings documentation.
Right. This is a potential solution.
NB: The suggestion above is usually the default for devices (at least
this was the case back when I was neck deep in DT). You usually have
the a device specified in a DTSI file with the generic properties
defined from within a top-level node which is usually disabled. Then
you link back to that node (usually with a &) from within your DTS
file where you provide platform specific properties and override the
status to 'okay' or what have you.
However before I provide any further assistance, I really want to get
an idea of the H/W you're working with. Is this a reduced function
DA9062? Or is the functionality actually present, you just don't want
to make use of it?
--
Lee Jones [李琼斯]
Senior Technical Lead - Developer Services
Linaro.org │ Open source software for Arm SoCs
Follow Linaro: Facebook | Twitter | Blog
Am Mittwoch, dem 30.06.2021 um 13:33 +0100 schrieb Lee Jones:
> On Wed, 30 Jun 2021, Daniel Thompson wrote:
>
> > On Wed, Jun 30, 2021 at 07:27:32AM +0000, Yunus Bas wrote:
> > > Am Dienstag, dem 29.06.2021 um 14:39 +0100 schrieb Lee Jones:
> > > > On Tue, 29 Jun 2021, Yunus Bas wrote:
> > > > > Interestingly, all subdevices defined in the driver are
> > > > > registered
> > > > > as platform devices from the MFD framework, regardless of a
> > > > > devicetree entry or not. The preceding code checks the
> > > > > subdevice
> > > > > cells with an additional compatible. In case a device has no
> > > > > devicetree entry, an irritating failed-message is printed on
> > > > > the
> > > > > display. I'm not sure if this was the intention but the
> > > > > framework
> > > > > somehow forces the users to describe all subdevices of an
> > > > > MFD. I
> > > > > think the info print is not needed. It makes more sense to
> > > > > set it
> > > > > as a debug print.
> > > >
> > > > Actually, this has served to highlight that your DTS is not
> > > > correct.
> > > >
> > > > Why are some devices represented in DT and some aren't?
> > > >
> > > > If anything, I'm tempted to upgrade the info() print to warn().
> > >
> > > Imagine only required parts of the MFD is connected to the
> > > designed
> > > system and unrequired parts are not. In that case, fully
> > > describing the
> > > MFD in the devicetree wouldn't represent the system at all.
> >
> > To describe hardware that is present but unused we would normally
> > use
> > status = "disabled".
> >
> > So if, for example, your board cannot use the RTC for some reason
> > (perhaps the board has no 32KHz oscillator?) then the DA9062 still
> > contains the hardware but it is useless. Such hardware could be
> > described as:
> >
> > da9062_rtc: rtc {
> > compatible = "dlg,da9062-rtc";
> > status = "disabled";
> > }
> >
> > Is this sufficient to suppress the warnings when the hardware is
> > not
> > fully described?
> >
> > There is almost certainly a problem here since there is a mismatch
> > between mfd-core and the DA9062 DT bindings. mfd-core warns when
> > the
> > hardware description is incomplete and the DA9062 (and generic mfd)
> > DT
> > bindings are ambiguous about whether sub-nodes are mandatory and
> > include
> > an example that contains missing compatibles rather than disabled
> > nodes
> > like the above.
> >
> > However it is not entirely clear to me at this point whether this
> > should
> > be fixed in mfd-core or by improving the bindings documentation.
>
> Right. This is a potential solution.
@Daniel, you hit the nail on the head :). Thank you for that.
This solution would indeed surpress the warnings, but what is the
benefit of this? We would define never used device nodes just to
satisfy the driver.
Also, this could lead potential users of the DTS to falsly assume that
the defined subfunction is actually supported and only needs a change
of status for that.
Actually I accept the fact, that changing the info() to debug() is not
a good idea, since the driver should naturally warn in case of a
compatible mismatch. But this should only apply for devices defined in
the devicetree.
But regardless, if defining all the MFD subnodes in the devicetree is
the way to go on with, it needs at least to be documented in the MFD-
bindings.
>
> NB: The suggestion above is usually the default for devices (at least
> this was the case back when I was neck deep in DT). You usually have
> the a device specified in a DTSI file with the generic properties
> defined from within a top-level node which is usually disabled. Then
> you link back to that node (usually with a &) from within your DTS
> file where you provide platform specific properties and override the
> status to 'okay' or what have you.
Yes, that's the common way which we also use. Normally, there is a
generic DTSI file representing a set of possible settings which is then
specified in the DTS-file it is included.
>
> However before I provide any further assistance, I really want to get
> an idea of the H/W you're working with. Is this a reduced function
> DA9062? Or is the functionality actually present, you just don't
> want
> to make use of it?
>
No it's not a reduced version. Some functions have been deliberately
omitted. Internally, MFD's normally have a common control register-
sets, but the functions have separate controllers and therefor separate
connections to the controllers. When a MFD has e.g. seven sub-functions
and five of them are needed, then the two unneeded functions can or
will be left out on purpose. This is common usage and can also be seen
on other devicetrees using MFD's.
--
Mit freundlichen Grüßen
Yunus Bas
-Software Engineer-
PHYTEC Messtechnik GmbH
Robert-Koch-Str. 39
55129 Mainz
Germany
Tel.: +49 (0)6131 9221- 466
Web: http://www.phytec.de
Sie finden uns auch auf: Facebook, LinkedIn, Xing, YouTube
PHYTEC Messtechnik GmbH | Robert-Koch-Str. 39 | 55129 Mainz, Germany
Geschäftsführer: Dipl.-Ing. Michael Mitezki, Dipl.-Ing. Bodo Huber |
Handelsregister Mainz HRB 4656 | Finanzamt Mainz | St.Nr. 266500608, DE
149059855
This E-Mail may contain confidential or privileged information. If you
are not the intended recipient (or have received this E-Mail in error)
please notify the sender immediately and destroy this E-Mail. Any
unauthorized copying, disclosure or distribution of the material in
this E-Mail is strictly forbidden.
On Thu, 01 Jul 2021, Yunus Bas wrote:
> Am Mittwoch, dem 30.06.2021 um 13:33 +0100 schrieb Lee Jones:
> > On Wed, 30 Jun 2021, Daniel Thompson wrote:
> >
> > > On Wed, Jun 30, 2021 at 07:27:32AM +0000, Yunus Bas wrote:
> > > > Am Dienstag, dem 29.06.2021 um 14:39 +0100 schrieb Lee Jones:
> > > > > On Tue, 29 Jun 2021, Yunus Bas wrote:
> > > > > > Interestingly, all subdevices defined in the driver are
> > > > > > registered
> > > > > > as platform devices from the MFD framework, regardless of a
> > > > > > devicetree entry or not. The preceding code checks the
> > > > > > subdevice
> > > > > > cells with an additional compatible. In case a device has no
> > > > > > devicetree entry, an irritating failed-message is printed on
> > > > > > the
> > > > > > display. I'm not sure if this was the intention but the
> > > > > > framework
> > > > > > somehow forces the users to describe all subdevices of an
> > > > > > MFD. I
> > > > > > think the info print is not needed. It makes more sense to
> > > > > > set it
> > > > > > as a debug print.
> > > > >
> > > > > Actually, this has served to highlight that your DTS is not
> > > > > correct.
> > > > >
> > > > > Why are some devices represented in DT and some aren't?
> > > > >
> > > > > If anything, I'm tempted to upgrade the info() print to warn().
> > > >
> > > > Imagine only required parts of the MFD is connected to the
> > > > designed
> > > > system and unrequired parts are not. In that case, fully
> > > > describing the
> > > > MFD in the devicetree wouldn't represent the system at all.
> > >
> > > To describe hardware that is present but unused we would normally
> > > use
> > > status = "disabled".
> > >
> > > So if, for example, your board cannot use the RTC for some reason
> > > (perhaps the board has no 32KHz oscillator?) then the DA9062 still
> > > contains the hardware but it is useless. Such hardware could be
> > > described as:
> > >
> > > da9062_rtc: rtc {
> > > compatible = "dlg,da9062-rtc";
> > > status = "disabled";
> > > }
> > >
> > > Is this sufficient to suppress the warnings when the hardware is
> > > not
> > > fully described?
> > >
> > > There is almost certainly a problem here since there is a mismatch
> > > between mfd-core and the DA9062 DT bindings. mfd-core warns when
> > > the
> > > hardware description is incomplete and the DA9062 (and generic mfd)
> > > DT
> > > bindings are ambiguous about whether sub-nodes are mandatory and
> > > include
> > > an example that contains missing compatibles rather than disabled
> > > nodes
> > > like the above.
> > >
> > > However it is not entirely clear to me at this point whether this
> > > should
> > > be fixed in mfd-core or by improving the bindings documentation.
> >
> > Right. This is a potential solution.
>
> @Daniel, you hit the nail on the head :). Thank you for that.
>
> This solution would indeed surpress the warnings, but what is the
> benefit of this? We would define never used device nodes just to
> satisfy the driver.
>
> Also, this could lead potential users of the DTS to falsly assume that
> the defined subfunction is actually supported and only needs a change
> of status for that.
>
> Actually I accept the fact, that changing the info() to debug() is not
> a good idea, since the driver should naturally warn in case of a
> compatible mismatch. But this should only apply for devices defined in
> the devicetree.
>
> But regardless, if defining all the MFD subnodes in the devicetree is
> the way to go on with, it needs at least to be documented in the MFD-
> bindings.
> >
> > However before I provide any further assistance, I really want to get
> > an idea of the H/W you're working with. Is this a reduced function
> > DA9062? Or is the functionality actually present, you just don't
> > want
> > to make use of it?
> >
> No it's not a reduced version. Some functions have been deliberately
> omitted. Internally, MFD's normally have a common control register-
> sets, but the functions have separate controllers and therefor separate
> connections to the controllers. When a MFD has e.g. seven sub-functions
> and five of them are needed, then the two unneeded functions can or
> will be left out on purpose. This is common usage and can also be seen
> on other devicetrees using MFD's.
I think what you describe here is a reduced function device, no?
> > NB: The suggestion above is usually the default for devices (at least
> > this was the case back when I was neck deep in DT). You usually have
> > the a device specified in a DTSI file with the generic properties
> > defined from within a top-level node which is usually disabled. Then
> > you link back to that node (usually with a &) from within your DTS
> > file where you provide platform specific properties and override the
> > status to 'okay' or what have you.
>
> Yes, that's the common way which we also use. Normally, there is a
> generic DTSI file representing a set of possible settings which is then
> specified in the DTS-file it is included.
Right, so no problem then.
Describe the full device in the DTSI file and disable all of the
functions by default. Then, in the DTS file for your particular
platform, only provide nodes for the functionality that is present.
Explanation:
Essentially, by populating the mfd_cell, you have told MFD what to
expect from a fully functional device. If any nodes are omitted
without explanation, it (rightfully) complains that something is
missing.
In order to describe the device properly in Device Tree, you need to
firstly describe the entire device, then only enable what is actually
present or what you wish to actually use on your H/W.
--
Lee Jones [李琼斯]
Senior Technical Lead - Developer Services
Linaro.org │ Open source software for Arm SoCs
Follow Linaro: Facebook | Twitter | Blog
On Thu, Jul 01, 2021 at 03:34:43PM +0000, Yunus Bas wrote:
> Am Mittwoch, dem 30.06.2021 um 13:33 +0100 schrieb Lee Jones:
> > On Wed, 30 Jun 2021, Daniel Thompson wrote:
> >
> > > On Wed, Jun 30, 2021 at 07:27:32AM +0000, Yunus Bas wrote:
> > > > Am Dienstag, dem 29.06.2021 um 14:39 +0100 schrieb Lee Jones:
> > > > Imagine only required parts of the MFD is connected to the
> > > > designed
> > > > system and unrequired parts are not. In that case, fully
> > > > describing the
> > > > MFD in the devicetree wouldn't represent the system at all.
> > >
> > > To describe hardware that is present but unused we would normally
> > > use
> > > status = "disabled".
> > >
> > > So if, for example, your board cannot use the RTC for some reason
> > > (perhaps the board has no 32KHz oscillator?) then the DA9062 still
> > > contains the hardware but it is useless. Such hardware could be
> > > described as:
> > >
> > > da9062_rtc: rtc {
> > > ??? compatible = "dlg,da9062-rtc";
> > > ??? status = "disabled";
> > > }
> > >
> > > Is this sufficient to suppress the warnings when the hardware is
> > > not fully described?
<snip>
> >
> > Right.? This is a potential solution.
>
> @Daniel, you hit the nail on the head :). Thank you for that.
>
> This solution would indeed surpress the warnings,?but what is the
> benefit of this? We would define never used device nodes just to
> satisfy the driver.
I would say that doing so resolves an awkward ambiguity of
interpretation w.r.t. the bindings.
1. The MFD device compatible "dlg,da9062" tells the OS that we
have an DA9062. An DA9062 contains six functions and this can be
inferred *entirely* from the MFD compatible string. We do not
need any subnodes to tell us that a DA9062 contains an RTC. The OS
can (and in this case, does) already know that there is an RTC
because we have a DA9062 (and a datasheet).
2. The default behaviour when a node has no status field is to
assume that is is *enabled*.
Based on #1 and #2 above then assuming that a DT that omits the
sub-nodes actually means "disable the RTC" is risky. #2 might
actually make it more natural to assume that the device is present and
functional because there is no status field to tell MFD *not* to
initialize it.
That leaves us in a situation where there is no way to correctly guess
the authors intent when sub-nodes are omitted from the DT. Given this is
something of a corner case and the documentation is ambiguous then a
warning of the author does not clearly resolve the ambiguity seems
reasonable.
Daniel.
On Fri, 02 Jul 2021, Daniel Thompson wrote:
> On Thu, Jul 01, 2021 at 03:34:43PM +0000, Yunus Bas wrote:
> > Am Mittwoch, dem 30.06.2021 um 13:33 +0100 schrieb Lee Jones:
> > > On Wed, 30 Jun 2021, Daniel Thompson wrote:
> > >
> > > > On Wed, Jun 30, 2021 at 07:27:32AM +0000, Yunus Bas wrote:
> > > > > Am Dienstag, dem 29.06.2021 um 14:39 +0100 schrieb Lee Jones:
> > > > > Imagine only required parts of the MFD is connected to the
> > > > > designed
> > > > > system and unrequired parts are not. In that case, fully
> > > > > describing the
> > > > > MFD in the devicetree wouldn't represent the system at all.
> > > >
> > > > To describe hardware that is present but unused we would normally
> > > > use
> > > > status = "disabled".
> > > >
> > > > So if, for example, your board cannot use the RTC for some reason
> > > > (perhaps the board has no 32KHz oscillator?) then the DA9062 still
> > > > contains the hardware but it is useless. Such hardware could be
> > > > described as:
> > > >
> > > > da9062_rtc: rtc {
> > > > compatible = "dlg,da9062-rtc";
> > > > status = "disabled";
> > > > }
> > > >
> > > > Is this sufficient to suppress the warnings when the hardware is
> > > > not fully described?
> <snip>
> > >
> > > Right. This is a potential solution.
> >
> > @Daniel, you hit the nail on the head :). Thank you for that.
> >
> > This solution would indeed surpress the warnings, but what is the
> > benefit of this? We would define never used device nodes just to
> > satisfy the driver.
>
> I would say that doing so resolves an awkward ambiguity of
> interpretation w.r.t. the bindings.
>
> 1. The MFD device compatible "dlg,da9062" tells the OS that we
> have an DA9062. An DA9062 contains six functions and this can be
> inferred *entirely* from the MFD compatible string. We do not
> need any subnodes to tell us that a DA9062 contains an RTC. The OS
> can (and in this case, does) already know that there is an RTC
> because we have a DA9062 (and a datasheet).
>
> 2. The default behaviour when a node has no status field is to
> assume that is is *enabled*.
>
> Based on #1 and #2 above then assuming that a DT that omits the
> sub-nodes actually means "disable the RTC" is risky. #2 might
> actually make it more natural to assume that the device is present and
> functional because there is no status field to tell MFD *not* to
> initialize it.
Exactly. Nicely put.
> That leaves us in a situation where there is no way to correctly guess
> the authors intent when sub-nodes are omitted from the DT.
> Given this is something of a corner case and the documentation is
> ambiguous then a warning of the author does not clearly resolve the
> ambiguity seems reasonable.
I'm having trouble parsing this part.
--
Lee Jones [李琼斯]
Senior Technical Lead - Developer Services
Linaro.org │ Open source software for Arm SoCs
Follow Linaro: Facebook | Twitter | Blog
On Fri, Jul 02, 2021 at 07:36:07PM +0100, Lee Jones wrote:
> On Fri, 02 Jul 2021, Daniel Thompson wrote:
>
> > On Thu, Jul 01, 2021 at 03:34:43PM +0000, Yunus Bas wrote:
> > > Am Mittwoch, dem 30.06.2021 um 13:33 +0100 schrieb Lee Jones:
> > > > On Wed, 30 Jun 2021, Daniel Thompson wrote:
> > > >
> > > > > On Wed, Jun 30, 2021 at 07:27:32AM +0000, Yunus Bas wrote:
> > > > > > Am Dienstag, dem 29.06.2021 um 14:39 +0100 schrieb Lee Jones:
> > > > > > Imagine only required parts of the MFD is connected to the
> > > > > > designed
> > > > > > system and unrequired parts are not. In that case, fully
> > > > > > describing the
> > > > > > MFD in the devicetree wouldn't represent the system at all.
> > > > >
> > > > > To describe hardware that is present but unused we would normally
> > > > > use
> > > > > status = "disabled".
> > > > >
> > > > > So if, for example, your board cannot use the RTC for some reason
> > > > > (perhaps the board has no 32KHz oscillator?) then the DA9062 still
> > > > > contains the hardware but it is useless. Such hardware could be
> > > > > described as:
> > > > >
> > > > > da9062_rtc: rtc {
> > > > > ??? compatible = "dlg,da9062-rtc";
> > > > > ??? status = "disabled";
> > > > > }
> > > > >
> > > > > Is this sufficient to suppress the warnings when the hardware is
> > > > > not fully described?
> > <snip>
> > > >
> > > > Right.? This is a potential solution.
> > >
> > > @Daniel, you hit the nail on the head :). Thank you for that.
> > >
> > > This solution would indeed surpress the warnings,?but what is the
> > > benefit of this? We would define never used device nodes just to
> > > satisfy the driver.
> >
> > I would say that doing so resolves an awkward ambiguity of
> > interpretation w.r.t. the bindings.
> >
> > 1. The MFD device compatible "dlg,da9062" tells the OS that we
> > have an DA9062. An DA9062 contains six functions and this can be
> > inferred *entirely* from the MFD compatible string. We do not
> > need any subnodes to tell us that a DA9062 contains an RTC. The OS
> > can (and in this case, does) already know that there is an RTC
> > because we have a DA9062 (and a datasheet).
> >
> > 2. The default behaviour when a node has no status field is to
> > assume that is is *enabled*.
> >
> > Based on #1 and #2 above then assuming that a DT that omits the
> > sub-nodes actually means "disable the RTC" is risky. #2 might
> > actually make it more natural to assume that the device is present and
> > functional because there is no status field to tell MFD *not* to
> > initialize it.
>
> Exactly. Nicely put.
>
> > That leaves us in a situation where there is no way to correctly guess
> > the authors intent when sub-nodes are omitted from the DT.
>
> > Given this is something of a corner case and the documentation is
> > ambiguous then a warning of the author does not clearly resolve the
> > ambiguity seems reasonable.
>
> I'm having trouble parsing this part.
That's quite reasonable because was is written is nonsense!
Perhaps s/warning of the author/warning if the author/ will help
but there are still too many words to say something very simple.
The whole last paragraph could simply say:
The bindings documentation is ambiguous so is it reasonable
for the OS to issue a warning when the devicetree author does
not clearly resolve the ambiguity.
This is still a long sentence but at least it is no longer a
complicated one!
Daniel.
Thank you for the clarification. I'm now aware on how to handle MFD's
in the devicetree. But given this, the default behavior of MFD's should
definitely be documented since i could see many other devicetree
examples handling this also not in the proper manner.
Regards, Yunus
Am Freitag, dem 02.07.2021 um 20:10 +0100 schrieb Daniel Thompson:
> On Fri, Jul 02, 2021 at 07:36:07PM +0100, Lee Jones wrote:
> > On Fri, 02 Jul 2021, Daniel Thompson wrote:
> >
> > > On Thu, Jul 01, 2021 at 03:34:43PM +0000, Yunus Bas wrote:
> > > > Am Mittwoch, dem 30.06.2021 um 13:33 +0100 schrieb Lee Jones:
> > > > > On Wed, 30 Jun 2021, Daniel Thompson wrote:
> > > > >
> > > > > > On Wed, Jun 30, 2021 at 07:27:32AM +0000, Yunus Bas wrote:
> > > > > > > Am Dienstag, dem 29.06.2021 um 14:39 +0100 schrieb Lee
> > > > > > > Jones:
> > > > > > > Imagine only required parts of the MFD is connected to
> > > > > > > the
> > > > > > > designed
> > > > > > > system and unrequired parts are not. In that case, fully
> > > > > > > describing the
> > > > > > > MFD in the devicetree wouldn't represent the system at
> > > > > > > all.
> > > > > >
> > > > > > To describe hardware that is present but unused we would
> > > > > > normally
> > > > > > use
> > > > > > status = "disabled".
> > > > > >
> > > > > > So if, for example, your board cannot use the RTC for some
> > > > > > reason
> > > > > > (perhaps the board has no 32KHz oscillator?) then the
> > > > > > DA9062 still
> > > > > > contains the hardware but it is useless. Such hardware
> > > > > > could be
> > > > > > described as:
> > > > > >
> > > > > > da9062_rtc: rtc {
> > > > > > compatible = "dlg,da9062-rtc";
> > > > > > status = "disabled";
> > > > > > }
> > > > > >
> > > > > > Is this sufficient to suppress the warnings when the
> > > > > > hardware is
> > > > > > not fully described?
> > > <snip>
> > > > >
> > > > > Right. This is a potential solution.
> > > >
> > > > @Daniel, you hit the nail on the head :). Thank you for that.
> > > >
> > > > This solution would indeed surpress the warnings, but what is
> > > > the
> > > > benefit of this? We would define never used device nodes just
> > > > to
> > > > satisfy the driver.
> > >
> > > I would say that doing so resolves an awkward ambiguity of
> > > interpretation w.r.t. the bindings.
> > >
> > > 1. The MFD device compatible "dlg,da9062" tells the OS that we
> > > have an DA9062. An DA9062 contains six functions and this can
> > > be
> > > inferred *entirely* from the MFD compatible string. We do not
> > > need any subnodes to tell us that a DA9062 contains an RTC.
> > > The OS
> > > can (and in this case, does) already know that there is an RTC
> > > because we have a DA9062 (and a datasheet).
> > >
> > > 2. The default behaviour when a node has no status field is to
> > > assume that is is *enabled*.
> > >
> > > Based on #1 and #2 above then assuming that a DT that omits the
> > > sub-nodes actually means "disable the RTC" is risky. #2 might
> > > actually make it more natural to assume that the device is
> > > present and
> > > functional because there is no status field to tell MFD *not* to
> > > initialize it.
> >
> > Exactly. Nicely put.
> >
> > > That leaves us in a situation where there is no way to correctly
> > > guess
> > > the authors intent when sub-nodes are omitted from the DT.
> >
> > > Given this is something of a corner case and the documentation is
> > > ambiguous then a warning of the author does not clearly resolve
> > > the
> > > ambiguity seems reasonable.
> >
> > I'm having trouble parsing this part.
>
> That's quite reasonable because was is written is nonsense!
> Perhaps s/warning of the author/warning if the author/ will help
> but there are still too many words to say something very simple.
> The whole last paragraph could simply say:
>
> The bindings documentation is ambiguous so is it reasonable
> for the OS to issue a warning when the devicetree author does
> not clearly resolve the ambiguity.
>
> This is still a long sentence but at least it is no longer a
> complicated one!
>
>
> Daniel.
--
Mit freundlichen Grüßen
Yunus Bas
-Software Engineer-
PHYTEC Messtechnik GmbH
Robert-Koch-Str. 39
55129 Mainz
Germany
Tel.: +49 (0)6131 9221- 466
Web: http://www.phytec.de
Sie finden uns auch auf: Facebook, LinkedIn, Xing, YouTube
PHYTEC Messtechnik GmbH | Robert-Koch-Str. 39 | 55129 Mainz, Germany
Geschäftsführer: Dipl.-Ing. Michael Mitezki, Dipl.-Ing. Bodo Huber |
Handelsregister Mainz HRB 4656 | Finanzamt Mainz | St.Nr. 266500608, DE
149059855
This E-Mail may contain confidential or privileged information. If you
are not the intended recipient (or have received this E-Mail in error)
please notify the sender immediately and destroy this E-Mail. Any
unauthorized copying, disclosure or distribution of the material in
this E-Mail is strictly forbidden.
On Mon, 05 Jul 2021, Yunus Bas wrote:
> Thank you for the clarification. I'm now aware on how to handle MFD's
> in the devicetree. But given this, the default behavior of MFD's should
> definitely be documented since i could see many other devicetree
> examples handling this also not in the proper manner.
In the 8 years I've been working with DT and MFD, this is the first
time this particular issue has arisen, but if you'd like to submit
such a document, it will be considered for inclusion.
Really pleased you have this figured out though.
--
Lee Jones [李琼斯]
Senior Technical Lead - Developer Services
Linaro.org │ Open source software for Arm SoCs
Follow Linaro: Facebook | Twitter | Blog
Am Montag, dem 05.07.2021 um 08:31 +0100 schrieb Lee Jones:
> On Mon, 05 Jul 2021, Yunus Bas wrote:
>
> > Thank you for the clarification. I'm now aware on how to handle
> > MFD's
> > in the devicetree. But given this, the default behavior of MFD's
> > should
> > definitely be documented since i could see many other devicetree
> > examples handling this also not in the proper manner.
>
> In the 8 years I've been working with DT and MFD, this is the first
> time this particular issue has arisen, but if you'd like to submit
> such a document, it will be considered for inclusion.
This is because on older kernel versions (or at least on the last LTS)
there was no warning in the first place. The warning was added with the
following patch of yours:
Commit 466a62d7642f ("mfd: core: Make a best effort attempt to match
devices with the correct of_nodes")
which was added around last year.
>
> Really pleased you have this figured out though.
>
--
Mit freundlichen Grüßen
Yunus Bas
-Software Engineer-
PHYTEC Messtechnik GmbH
Robert-Koch-Str. 39
55129 Mainz
Germany
Tel.: +49 (0)6131 9221- 466
Web: http://www.phytec.de
Sie finden uns auch auf: Facebook, LinkedIn, Xing, YouTube
PHYTEC Messtechnik GmbH | Robert-Koch-Str. 39 | 55129 Mainz, Germany
Geschäftsführer: Dipl.-Ing. Michael Mitezki, Dipl.-Ing. Bodo Huber |
Handelsregister Mainz HRB 4656 | Finanzamt Mainz | St.Nr. 266500608, DE
149059855
This E-Mail may contain confidential or privileged information. If you
are not the intended recipient (or have received this E-Mail in error)
please notify the sender immediately and destroy this E-Mail. Any
unauthorized copying, disclosure or distribution of the material in
this E-Mail is strictly forbidden.
On Mon, 05 Jul 2021, Yunus Bas wrote:
> Am Montag, dem 05.07.2021 um 08:31 +0100 schrieb Lee Jones:
> > On Mon, 05 Jul 2021, Yunus Bas wrote:
> >
> > > Thank you for the clarification. I'm now aware on how to handle
> > > MFD's
> > > in the devicetree. But given this, the default behavior of MFD's
> > > should
> > > definitely be documented since i could see many other devicetree
> > > examples handling this also not in the proper manner.
> >
> > In the 8 years I've been working with DT and MFD, this is the first
> > time this particular issue has arisen, but if you'd like to submit
> > such a document, it will be considered for inclusion.
>
> This is because on older kernel versions (or at least on the last LTS)
> there was no warning in the first place. The warning was added with the
> following patch of yours:
>
> Commit 466a62d7642f ("mfd: core: Make a best effort attempt to match
> devices with the correct of_nodes")
Right. The implementation was decidedly more broken before this and
many sins were lying dormant. Fortunately, we now have an early
warning system which should catch these kinds of misdemeanours during
initial implementation.
You make a good point about LTS' though.
I'll be sure to keep this in mind as more people update.
--
Lee Jones [李琼斯]
Senior Technical Lead - Developer Services
Linaro.org │ Open source software for Arm SoCs
Follow Linaro: Facebook | Twitter | Blog