Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <20210128170936.9222-1-mike.leach@linaro.org> <20210128170936.9222-3-mike.leach@linaro.org>
 <20210219184313.GA3065106@xps15>
In-Reply-To: <20210219184313.GA3065106@xps15>
From:   Mike Leach <mike.leach@linaro.org>
Date:   Fri, 26 Feb 2021 19:14:32 +0000
Message-ID: <CAJ9a7Vhh5xdbmMgcTLjU3qJEg2nbZoZ5j2HqvenwNGoqo+54Rw@mail.gmail.com>
Subject: Re: [PATCH v4 02/10] coresight: syscfg: Add registration and feature
 loading for cs devices
To:     Mathieu Poirier <mathieu.poirier@linaro.org>
Cc:     linux-arm-kernel <linux-arm-kernel@lists.infradead.org>,
        Coresight ML <coresight@lists.linaro.org>,
        "open list:DOCUMENTATION" <linux-doc@vger.kernel.org>,
        "Suzuki K. Poulose" <suzuki.poulose@arm.com>,
        Yabin Cui <yabinc@google.com>,
        Jonathan Corbet <corbet@lwn.net>, Leo Yan <leo.yan@linaro.org>,
        Alexander Shishkin <alexander.shishkin@linux.intel.com>,
        Tingwei Zhang <tingwei@codeaurora.org>,
        Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

Hi Mathieu,

On Fri, 19 Feb 2021 at 18:43, Mathieu Poirier
<mathieu.poirier@linaro.org> wrote:
>
> [...]
>
> > +/**
> > + * List entry for Coresight devices that are registered as supporting complex
> > + * config operations.
> > + *
> > + * @csdev:   The registered device.
> > + * @match_info: The matching type information.
> > + * @ops:     Operations supported by the registered device.
> > + * @item:    list entry.
> > + */
> > +struct cscfg_csdev_register {
> > +     struct coresight_device *csdev;
> > +     struct cscfg_match_desc match_info;
> > +     struct cscfg_csdev_feat_ops ops;
> > +     struct list_head item;
> > +};
>
> I would call this structure cscfg_registered_csdev and move it to
> coresight-config.h.  That way it is consistent with cscfg_config_csdev and
> cscfg_feature_csdev and located all in the same file.
>

I was trying to separate structures that are used to define
configurations and features, with those that are used to manage the
same. Hence, most things in coresight_config.h define configurations,
or their device loaded instance equivalents, and things in
coresight_syscfg.h are management items. I am happy to change the name
but would prefer is stay in coresight_syscfg.h

Thanks

Mike


> I may have to come back to this patch but for now it holds together.
>
> More comments to come on Monday.
>
> Thanks,
> Mathieu
>
> > +
> >  /* internal core operations for cscfg */
> >  int __init cscfg_init(void);
> >  void __exit cscfg_exit(void);
> > @@ -33,6 +49,10 @@ void __exit cscfg_exit(void);
> >  /* syscfg manager external API */
> >  int cscfg_load_config_sets(struct cscfg_config_desc **cfg_descs,
> >                          struct cscfg_feature_desc **feat_descs);
> > +int cscfg_register_csdev(struct coresight_device *csdev,
> > +                      struct cscfg_match_desc *info,
> > +                      struct cscfg_csdev_feat_ops *ops);
> > +void cscfg_unregister_csdev(struct coresight_device *csdev);
> >
> >  /**
> >   * System configuration manager device.
> > diff --git a/include/linux/coresight.h b/include/linux/coresight.h
> > index 976ec2697610..d0126ed326a6 100644
> > --- a/include/linux/coresight.h
> > +++ b/include/linux/coresight.h
> > @@ -219,6 +219,8 @@ struct coresight_sysfs_link {
> >   * @nr_links:   number of sysfs links created to other components from this
> >   *           device. These will appear in the "connections" group.
> >   * @has_conns_grp: Have added a "connections" group for sysfs links.
> > + * @feature_csdev_list: List of complex feature programming added to the device.
> > + * @config_csdev_list:  List of system configurations added to the device.
> >   */
> >  struct coresight_device {
> >       struct coresight_platform_data *pdata;
> > @@ -240,6 +242,9 @@ struct coresight_device {
> >       int nr_links;
> >       bool has_conns_grp;
> >       bool ect_enabled; /* true only if associated ect device is enabled */
> > +     /* system configuration and feature lists */
> > +     struct list_head feature_csdev_list;
> > +     struct list_head config_csdev_list;
> >  };
> >
> >  /*
> > --
> > 2.17.1
> >


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