Messages in this thread | | | Subject | Re: [PATCH] drivers/perf: hisi: Add identifier sysfs file | From | John Garry <> | Date | Thu, 18 Jun 2020 10:18:42 +0100 |
| |
On 18/06/2020 02:40, Shaokun Zhang wrote: >> } >> >> + hha_pmu->identifier = readl(hha_pmu->base + HHA_VERSION); > Since we are now refactoring the PMU framework, the PMU version offset > is always the same except DDRC PMU and other uncore PMU modules will > also use this, how about we do it as the common code: > > #define HISI_PMU_VERSION_REG 0x1CF0 > int hisi_uncore_pmu_version(struct hisi_pmu *hisi_pmu) > { > return readl(hisi_pmu->base + HISI_PMU_VERSION_REG); > } > EXPORT_SYMBOL_GPL(hisi_uncore_pmu_version);
Hi Shaokun,
Some points to make:
- It's hardly worth having a separate function to do this 1-line readl() call, especially since it not even generic (DDRC is different)
- We would have to export it (or put in a common header file with static inline keywords) - less exports are good
- with factoring out common code, it's good to reduce total code - this change would increase it, AFAICS
- This is HW specific. The driver is currently layered such that all HW specific stuff is in the HW driver (like hisi_uncore_ddrc_pmu.c), and not library code (hisi_uncore_pmu.c). I don't see why you want to mix that, like you're proposing in the framework revision you proposed internally.
Thanks, John
> > hha_pmu->identifier = hisi_uncore_pmu_version(hha_pmu); > we can remove the duplicated PMU version register definition in each module. > > Thanks, > Shaokun > >> + >> return 0; >> } >> >> @@ -320,10 +323,23 @@ static const struct attribute_group hisi_hha_pmu_cpumask_attr_group = { >> .attrs = hisi_hha_pmu_cpumask_attrs, >> };
| |