Re: [PATCH v5 08/13] coresight: Simplify connection fixup mechanism

[Mike Leach]

On Tue, 4 Apr 2023 at 16:52, James Clark <james.clark@xxxxxxx> wrote:
>
> There is some duplication between coresight_fixup_device_conns() and
> coresight_fixup_orphan_conns(). They both do the same thing except for
> the fact that coresight_fixup_orphan_conns() can't handle iterating over
> itself.
>
> By making it able to handle fixing up it's own connections the other
> function can be removed.
>
> Signed-off-by: James Clark <james.clark@xxxxxxx>
> ---
>  drivers/hwtracing/coresight/coresight-core.c | 84 ++++++++------------
>  1 file changed, 32 insertions(+), 52 deletions(-)
>
> diff --git a/drivers/hwtracing/coresight/coresight-core.c b/drivers/hwtracing/coresight/coresight-core.c
> index 0b738960973b..8d377a59e0be 100644
> --- a/drivers/hwtracing/coresight/coresight-core.c
> +++ b/drivers/hwtracing/coresight/coresight-core.c
> @@ -1316,42 +1316,46 @@ static int coresight_orphan_match(struct device *dev, void *data)
>  {
>         int i, ret = 0;
>         bool still_orphan = false;
> -       struct coresight_device *csdev, *i_csdev;
> +       struct coresight_device *dst_csdev = data;
> +       struct coresight_device *src_csdev = to_coresight_device(dev);
>         struct coresight_connection *conn;
> -
> -       csdev = data;
> -       i_csdev = to_coresight_device(dev);
> -
> -       /* No need to check oneself */
> -       if (csdev == i_csdev)
> -               return 0;
> +       bool fixup_self = (src_csdev == dst_csdev);
>
>         /* Move on to another component if no connection is orphan */
> -       if (!i_csdev->orphan)
> +       if (!src_csdev->orphan)
>                 return 0;
>         /*
> -        * Circle throuch all the connection of that component.  If we find
> -        * an orphan connection whose name matches @csdev, link it.
> +        * Circle through all the connections of that component.  If we find
> +        * an orphan connection whose name matches @dst_csdev, link it.
>          */
> -       for (i = 0; i < i_csdev->pdata->nr_outconns; i++) {
> -               conn = i_csdev->pdata->out_conns[i];
> -
> -               /* We have found at least one orphan connection */
> -               if (conn->dest_dev == NULL) {
> -                       /* Does it match this newly added device? */
> -                       if (conn->dest_fwnode == csdev->dev.fwnode) {
> -                               ret = coresight_make_links(i_csdev,
> -                                                          conn, csdev);
> -                               if (ret)
> -                                       return ret;
> -                       } else {
> -                               /* This component still has an orphan */
> -                               still_orphan = true;
> -                       }
> +       for (i = 0; i < src_csdev->pdata->nr_outconns; i++) {
> +               conn = src_csdev->pdata->out_conns[i];
> +
> +               /* Skip the port if it's already connected. */
> +               if (conn->dest_dev)
> +                       continue;
> +
> +               /*
> +                * If we are at the "new" device, which triggered this search,
> +                * we must find the remote device from the fwnode in the
> +                * connection.
> +                */
> +               if (fixup_self)
> +                       dst_csdev = coresight_find_csdev_by_fwnode(
> +                               conn->dest_fwnode);
> +
> +               /* Does it match this newly added device? */
> +               if (dst_csdev && conn->dest_fwnode == dst_csdev->dev.fwnode) {
> +                       ret = coresight_make_links(src_csdev, conn, dst_csdev);
> +                       if (ret)
> +                               return ret;
> +               } else {
> +                       /* This component still has an orphan */
> +                       still_orphan = true;
>                 }
>         }
>
> -       i_csdev->orphan = still_orphan;
> +       src_csdev->orphan = still_orphan;
>
>         /*
>          * Returning '0' in case we didn't encounter any error,
> @@ -1366,28 +1370,6 @@ static int coresight_fixup_orphan_conns(struct coresight_device *csdev)
>                          csdev, coresight_orphan_match);
>  }
>
> -
> -static int coresight_fixup_device_conns(struct coresight_device *csdev)
> -{
> -       int i, ret = 0;
> -
> -       for (i = 0; i < csdev->pdata->nr_outconns; i++) {
> -               struct coresight_connection *conn = csdev->pdata->out_conns[i];
> -
> -               conn->dest_dev =
> -                       coresight_find_csdev_by_fwnode(conn->dest_fwnode);
> -               if (conn->dest_dev && conn->dest_dev->has_conns_grp) {
> -                       ret = coresight_make_links(csdev, conn, conn->dest_dev);
> -                       if (ret)
> -                               break;
> -               } else {
> -                       csdev->orphan = true;
> -               }
> -       }
> -
> -       return ret;
> -}
> -
>  static int coresight_remove_match(struct device *dev, void *data)
>  {
>         int i;
> @@ -1595,7 +1577,7 @@ struct coresight_device *coresight_register(struct coresight_desc *desc)
>         csdev->subtype = desc->subtype;
>         csdev->ops = desc->ops;
>         csdev->access = desc->access;
> -       csdev->orphan = false;
> +       csdev->orphan = true;
>
>         csdev->dev.type = &coresight_dev_type[desc->type];
>         csdev->dev.groups = desc->groups;
> @@ -1645,8 +1627,6 @@ struct coresight_device *coresight_register(struct coresight_desc *desc)
>         registered = true;
>
>         ret = coresight_create_conns_sysfs_group(csdev);
> -       if (!ret)
> -               ret = coresight_fixup_device_conns(csdev);
>         if (!ret)
>                 ret = coresight_fixup_orphan_conns(csdev);
>
> --
> 2.34.1
>

Reviewed-by: Mike Leach <mike.leach@xxxxxxxxxx>

-- 
Mike Leach
Principal Engineer, ARM Ltd.
Manchester Design Centre. UK