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

[Philipp Zabel]

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?

>  
> +       if (rcdev->fwnode)
> +               return fwnode_get_name(rcdev->fwnode);
> +
>         return NULL;
>  }
>  
[...]
>  
>  /**
> @@ -98,9 +122,21 @@ static int of_reset_simple_xlate(struct reset_controller_dev *rcdev,
>   */
>  int reset_controller_register(struct reset_controller_dev *rcdev)
>  {
> -       if (!rcdev->of_xlate) {
> -               rcdev->of_reset_n_cells = 1;
> -               rcdev->of_xlate = of_reset_simple_xlate;
> +       if (!rcdev->fwnode) {
> +               rcdev->fwnode = of_fwnode_handle(rcdev->of_node);
> +       } else {
> +               if (is_acpi_node(rcdev->fwnode))
> +                       return -EINVAL;
> +       }
> +
> +       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.

> +               rcdev->fwnode_reset_n_cells = rcdev->of_reset_n_cells;
> +       }
> +
> +       if (!rcdev->fwnode_xlate) {
> +               rcdev->fwnode_reset_n_cells = 1;
> +               rcdev->fwnode_xlate = fwnode_reset_simple_xlate;
>         }
>  
>         INIT_LIST_HEAD(&rcdev->reset_control_head);
> @@ -810,29 +846,28 @@ static void __reset_control_put_internal(struct
> reset_control *rstc)
>  }
>  
>  struct reset_control *
> -__of_reset_control_get(struct device_node *node, const char *id, int index,
> -                      bool shared, bool optional, bool acquired)
> +__fwnode_reset_control_get(struct fwnode_handle *fwnode, const char *id,
> +                          int index, bool shared, bool optional, bool acquired)
>  {
>         struct reset_control *rstc;
>         struct reset_controller_dev *r, *rcdev;
> -       struct of_phandle_args args;
> +       struct fwnode_reference_args args;
>         int rstc_id;
>         int ret;
>  
> -       if (!node)
> +       if (!fwnode || is_acpi_node(fwnode))
>                 return ERR_PTR(-EINVAL);
>  
>         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.

[...]
> @@ -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:

> +       if (dev_fwnode(dev))
> +               return __fwnode_reset_control_get(dev_fwnode(dev), id, index,
> +                                                 shared, optional, acquired);
>  
>         return __reset_control_get_from_lookup(dev, id, shared, optional,
>                                                acquired);
> diff --git a/include/linux/reset-controller.h b/include/linux/reset-
> controller.h
> index 0fa4f60e1186..292552003d11 100644
> --- a/include/linux/reset-controller.h
> +++ b/include/linux/reset-controller.h
> @@ -24,7 +24,9 @@ struct reset_control_ops {
>  
>  struct module;
>  struct device_node;
> +struct fwnode_handle;
>  struct of_phandle_args;
> +struct fwnode_reference_args;
>  
>  /**
>   * struct reset_control_lookup - represents a single lookup entry
> @@ -60,10 +62,16 @@ struct reset_control_lookup {
>   * @reset_control_head: head of internal list of requested reset controls
>   * @dev: corresponding driver model device struct
>   * @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.


regards
Philipp