Hi Will, > > > On Wed, Jan 31, 2024 at 01:58:07PM +0800, Xu Yang wrote: > > This driver is initinally used to support imx93 Soc and now it's time to > > add support for imx95 Soc. However, some macro definitions and events are > > different on these two Socs. For preparing imx95 supports, this will > > refactor driver for imx93. > > > > Signed-off-by: Xu Yang <xu.yang_2@xxxxxxx> > > > > --- > > Changes in v4: > > - new patch > > --- > > drivers/perf/fsl_imx9_ddr_perf.c | 121 ++++++++++++++++++++++--------- > > 1 file changed, 87 insertions(+), 34 deletions(-) > > [...] > > > @@ -476,12 +490,12 @@ static int ddr_perf_event_add(struct perf_event *event, int flags) > > hwc->idx = counter; > > hwc->state |= PERF_HES_STOPPED; > > > > + /* read trans, write trans, read beat */ > > + imx93_ddr_perf_monitor_config(pmu, cfg, cfg1, cfg2); > > + > > if (flags & PERF_EF_START) > > ddr_perf_event_start(event, flags); > > > > - /* read trans, write trans, read beat */ > > - ddr_perf_monitor_config(pmu, cfg, cfg1, cfg2); > > - > > return 0; > > This change looks like more than just refactoring and should probably be a > separate patch. Is it a bug fix for the existing code? Yes, I found the order has an impact. I will create a patch for it. > > > +static int ddr_perf_add_events(struct ddr_pmu *pmu) > > +{ > > + int i, ret; > > + struct attribute **attrs = pmu->devtype_data->attrs; > > + struct device *pmu_dev = pmu->pmu.dev; > > + > > + if (!attrs) > > + return 0; > > + > > + for (i = 0; attrs[i]; i++) { > > + ret = sysfs_add_file_to_group(&pmu_dev->kobj, attrs[i], "events"); > > + if (ret) { > > + dev_warn(pmu->dev, "i.MX9 DDR Perf add events failed (%d)\n", ret); > > + return ret; > > Can you use the '.is_visible' callback in 'struct attribute_group' to avoid > having to manipulate sysfs directly like this? For example, create separate > groups for the imx93 and imx95-specific events and only make them visible > if we're on the appropriate hardware. Yes, I will try your suggestion. Thanks, Xu Yang