Re: [PATCH v8 2/2] coresight: Add label sysfs node support

[Jinlong Mao]

On 2025/7/16 18:45, Mike Leach wrote:
> Hi,
>
> On Wed, 16 Jul 2025 at 03:43, Jinlong Mao <quic_jinlmao@xxxxxxxxxxx> wrote:
> > On 2025/7/3 22:19, Leo Yan wrote:
> > > On Thu, Jul 03, 2025 at 09:04:53PM +0800, Mao Jinlong wrote:
> > >
> > > [...]
> > >
> > > > +static ssize_t label_show(struct device *dev,
> > > > +            struct device_attribute *attr, char *buf)
> > > > +{
> > > > +
> > > > +    const char *str;
> > > > +    int ret = 0;
> > >
> > > No need to init ret to 0.
> > >
> > > > +    ret = fwnode_property_read_string(dev_fwnode(dev), "label", &str);
> > > > +    if (ret == 0)
> > > > +            return scnprintf(buf, PAGE_SIZE, "%s\n", str);
> > > > +    else
> > > > +            return ret;
> > > > +}
> > > > +static DEVICE_ATTR_RO(label);
> > > > +
> > > >    static struct attribute *coresight_sink_attrs[] = {
> > > >       &dev_attr_enable_sink.attr,
> > > > +    &dev_attr_label.attr,
> > > >       NULL,
> > > >    };
> > > >    ATTRIBUTE_GROUPS(coresight_sink);
> > > >
> > > >    static struct attribute *coresight_source_attrs[] = {
> > > >       &dev_attr_enable_source.attr,
> > > > +    &dev_attr_label.attr,
> > > >       NULL,
> > > >    };
> > > >    ATTRIBUTE_GROUPS(coresight_source);
> > > >
> > > > +static struct attribute *coresight_link_attrs[] = {
> > > > +    &dev_attr_label.attr,
> > > > +    NULL,
> > > > +};
> > > > +ATTRIBUTE_GROUPS(coresight_link);
> > > > +
> > > > +static struct attribute *coresight_helper_attrs[] = {
> > > > +    &dev_attr_label.attr,
> > > > +    NULL,
> > > > +};
> > > > +ATTRIBUTE_GROUPS(coresight_helper);
> > > > +
> > >
> > > This change adds a 'label' entry for source, link, helper, and sink
> > > components, but the documentation has only updated for three components:
> > > CTI, funnel, and TPDM.
> > >
> > > Should we also update the documentation for all relevant components,
> > > such as ETM, ETR, etc.?
> > >
> > > Additionally, patch 01 is missing the update to the ETM yaml file for
> > > the new property. I checked patch v4 [1], which includes a change to
> > > etm.yaml, but this change was dropped since v5. I briefly read the
> > > v4 discussion thread and didn't see any mention of removing the ETM
> > > related change. Did you see any particular issue when add label for
> > > ETM devices?
> > >
> > > Overall, this series is fine for me. Just please ensure that all
> > > relevant components are covered for completeness.
> > >
> > > Thanks,
> > > Leo
> >
> > I will update all coresight docs.
> >
> > Thanks
> > Jinlong Mao
> >
> > > [1] https://patchwork.kernel.org/project/linux-arm-msm/cover/20240703122340.26864-1-quic_jinlmao@xxxxxxxxxxx/
> > >
> > > >    const struct device_type coresight_dev_type[] = {
> > > >       [CORESIGHT_DEV_TYPE_SINK] = {
> > > >               .name = "sink",
> > > > @@ -390,6 +420,7 @@ const struct device_type coresight_dev_type[] = {
> > > >       },
> > > >       [CORESIGHT_DEV_TYPE_LINK] = {
> > > >               .name = "link",
> > > > +            .groups = coresight_link_groups,
> > > >       },
> > > >       [CORESIGHT_DEV_TYPE_LINKSINK] = {
> > > >               .name = "linksink",
> > > > @@ -401,6 +432,7 @@ const struct device_type coresight_dev_type[] = {
> > > >       },
> > > >       [CORESIGHT_DEV_TYPE_HELPER] = {
> > > >               .name = "helper",
> > > > +            .groups = coresight_helper_groups,
> > > >       }
> > > >    };
> > > >    /* Ensure the enum matches the names and groups */
> > > > --
> > > > 2.17.1
> > > >
> > > > _______________________________________________
> > > > CoreSight mailing list -- coresight@xxxxxxxxxxxxxxxx
> > > > To unsubscribe send an email to coresight-leave@xxxxxxxxxxxxxxxx
>
> Revisiting this - the label DT attribute is purely optional, and
> provides context for the hardware instance.
> This code as written appears to add a "label" file to all devices,
> irrespective of if the label is set in the DT.or not, with blank
> labels where  the attribute is not present.
> The visibility of the sysfs attribute should be controlled so that it
> only appears if label is present in the DT.
I will follow this.

Thanks
Jinlong Mao
> Mike