Le Wed, 23 Mar 2022 16:29:41 +0100,
Philipp Zabel <[email protected]> a écrit :
> On Mi, 2022-03-23 at 10:50 +0100, Clément Léger wrote:
> [...]
> > diff --git a/drivers/reset/core.c b/drivers/reset/core.c
> > index 61e688882643..f014da03b7c1 100644
> > --- a/drivers/reset/core.c
> > +++ b/drivers/reset/core.c
> > @@ -4,6 +4,7 @@
> > *
> > * Copyright 2013 Philipp Zabel, Pengutronix
> > */
> > +#include <linux/acpi.h>
> > #include <linux/atomic.h>
> > #include <linux/device.h>
> > #include <linux/err.h>
> > @@ -70,26 +71,49 @@ static const char *rcdev_name(struct
> > reset_controller_dev *rcdev)
> > if (rcdev->of_node)
> > return rcdev->of_node->full_name;
>
> Could the above be removed, since reset_controller_register() set
> rcdev->fwnode to of_fwnode_handle(rcdev->of_node) earlier?
Yes, this should work in all cases, the only difference is that
fwnode_get_name() returns the basename of the of_node full_name field.
This is potentially a change from what was displayed before. If you are
ok with that, I'll drop these lines.
[...]
> > + }
> > +
> > + if (rcdev->of_xlate) {
> > + rcdev->fwnode_xlate = fwnode_of_reset_xlate;
>
> It should be documented that .fwnode_xlate/.fwnode_reset_n_cells are
> ignored if .of_xlate is set.
Acked.
[...]
> > if (id) {
> > - index = of_property_match_string(node,
> > - "reset-names", id);
> > + index = fwnode_property_match_string(fwnode, "reset-names", id);
> > if (index == -EILSEQ)
> > return ERR_PTR(index);
>
> I don't think this is good enough any more. At least -ENOMEM is added
> as a possible error return code by this change.
Yes indeed, errors are clearly not correctly handled anymore. At least
-EILSEQ won't be triggered.
>
> [...]
> > @@ -945,6 +989,9 @@ struct reset_control *__reset_control_get(struct device *dev, const char *id,
> > if (dev->of_node)
> > return __of_reset_control_get(dev->of_node, id, index, shared,
> > optional, acquired);
>
> Could the above be removed, given that __of_reset_control_get() just
> wraps __fwnode_reset_control_get(), which is called right below:
Oh yes, sorry for that. It can clearly be removed.
[...]
> > * @of_node: corresponding device tree node as phandle target
> > + * @fwnode: corresponding firmware node as reference target
> > * @of_reset_n_cells: number of cells in reset line specifiers
> > * @of_xlate: translation function to translate from specifier as found in the
> > * device tree to id as given to the reset control ops, defaults
> > - * to :c:func:`of_reset_simple_xlate`.
> > + * to :c:func:`fwnode_of_reset_xlate`.
> > + * @fwnode_reset_n_cells: number of cells in reset line reference specifiers
> > + * @fwnode_xlate: translation function to translate from reference specifier as
> > + * found in the firmware node description to id as given to the
> > + * reset control ops, defaults to
> > + * :c:func:`fwnode_reset_simple_xlate`.
>
> This should mention that .fwnode_xlate is ignored/overwritten when
> .of_xlate is set.
Acked.
>
>
> regards
> Philipp
Regards,
--
Clément Léger,
Embedded Linux and Kernel engineer at Bootlin
https://bootlin.com
On Mi, 2022-03-23 at 17:21 +0100, Clément Léger wrote:
> Le Wed, 23 Mar 2022 16:29:41 +0100,
> Philipp Zabel <[email protected]> a écrit :
>
> > On Mi, 2022-03-23 at 10:50 +0100, Clément Léger wrote:
> > [...]
> > > diff --git a/drivers/reset/core.c b/drivers/reset/core.c
> > > index 61e688882643..f014da03b7c1 100644
> > > --- a/drivers/reset/core.c
> > > +++ b/drivers/reset/core.c
> > > @@ -4,6 +4,7 @@
> > > *
> > > * Copyright 2013 Philipp Zabel, Pengutronix
> > > */
> > > +#include <linux/acpi.h>
> > > #include <linux/atomic.h>
> > > #include <linux/device.h>
> > > #include <linux/err.h>
> > > @@ -70,26 +71,49 @@ static const char *rcdev_name(struct
> > > reset_controller_dev *rcdev)
> > > if (rcdev->of_node)
> > > return rcdev->of_node->full_name;
> >
> > Could the above be removed, since reset_controller_register() set
> > rcdev->fwnode to of_fwnode_handle(rcdev->of_node) earlier?
>
> Yes, this should work in all cases, the only difference is that
> fwnode_get_name() returns the basename of the of_node full_name
> field.
> This is potentially a change from what was displayed before. If you are
> ok with that, I'll drop these lines.
Yes, that should be fine. rcdev_name() is currently only used in
warnings that let driver developers know which reset controller they
used incorrectly.
regards
Philipp
Le Wed, 23 Mar 2022 17:21:21 +0100,
Clément Léger <[email protected]> a écrit :
> > > if (id) {
> > > - index = of_property_match_string(node,
> > > - "reset-names", id);
> > > + index = fwnode_property_match_string(fwnode, "reset-names", id);
> > > if (index == -EILSEQ)
> > > return ERR_PTR(index);
> >
> > I don't think this is good enough any more. At least -ENOMEM is added
> > as a possible error return code by this change.
>
> Yes indeed, errors are clearly not correctly handled anymore. At least
> -EILSEQ won't be triggered.
> >
By the way, even after looking at this more carefully, I'm not sure to
understand why there is a special handling for -EILSEQ ? From what I
understand, EILSEQ is returned in case the device tree is malformed
(string longer than returned property length) but why is it handled
differently in this case ?
Thanks,
--
Clément Léger,
Embedded Linux and Kernel engineer at Bootlin
https://bootlin.com
Hi Clément,
On Do, 2022-03-24 at 10:19 +0100, Clément Léger wrote:
> Le Wed, 23 Mar 2022 17:21:21 +0100,
> Clément Léger <[email protected]> a écrit :
>
> > > > if (id) {
> > > > - index = of_property_match_string(node,
> > > > - "reset-names",
> > > > id);
> > > > + index = fwnode_property_match_string(fwnode,
> > > > "reset-names", id);
> > > > if (index == -EILSEQ)
> > > > return ERR_PTR(index);
> > >
> > > I don't think this is good enough any more. At least -ENOMEM is added
> > > as a possible error return code by this change.
> >
> > Yes indeed, errors are clearly not correctly handled anymore. At least
> > -EILSEQ won't be triggered.
> > >
>
> By the way, even after looking at this more carefully, I'm not sure to
> understand why there is a special handling for -EILSEQ ? From what I
> understand, EILSEQ is returned in case the device tree is malformed
> (string longer than returned property length) but why is it handled
> differently in this case ?
of_property_match_string() can return four error codes:
-EINVAL (no reset-names property in node)
-EILSEQ (real error)
-ENODATA (id is not found in reset-names)
-ENOSYS (CONFIG_OF is not set)
-EINVAL, -ENODATA and -ENOSYS map to -ENOENT (no reset control
corresponding to id found), or no error if it is optional.
regards
Philipp