Le Wed, 23 Mar 2022 16:29:41 +0100,
Philipp Zabel <p.zabel@xxxxxxxxxxxxxx> 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
