Re: [PATCH 2/2] reset: add support for fwnode

[Clément Léger]

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