Messages in this thread | | | Date | Tue, 1 Aug 2023 08:38:58 +0100 | From | Will Deacon <> | Subject | Re: [PATCH 1/3] arm_pmu: acpi: Add a representative platform device for TRBE |
| |
On Tue, Aug 01, 2023 at 09:05:54AM +0530, Anshuman Khandual wrote: > > > On 7/31/23 20:29, Will Deacon wrote: > > On Mon, Jul 31, 2023 at 05:38:38PM +0530, Anshuman Khandual wrote: > >> On 7/28/23 20:10, Will Deacon wrote: > >>> On Fri, Jul 28, 2023 at 04:57:31PM +0530, Anshuman Khandual wrote: > >>>> diff --git a/drivers/perf/arm_pmu_acpi.c b/drivers/perf/arm_pmu_acpi.c > >>>> index 90815ad762eb..dd3df6729808 100644 > >>>> --- a/drivers/perf/arm_pmu_acpi.c > >>>> +++ b/drivers/perf/arm_pmu_acpi.c > > > > [...] > > > >>>> + ret = platform_device_register(&trbe_acpi_dev); > >>>> + if (ret < 0) { > >>>> + pr_warn("ACPI: TRBE: Unable to register device\n"); > >>>> + acpi_unregister_gsi(gsi); > >>>> + } > >>>> +} > >>>> +#else > >>>> +static inline void arm_trbe_acpi_register_device(void) > >>>> +{ > >>>> + > >>>> +} > >>>> +#endif /* CONFIG_CORESIGHT_TRBE */ > >>> > >>> This looks like you ran s/spe/trbe/ over the SPE device registration > >>> code :) > >> > >> Yeah, almost :) > >> > >>> Please can you refactor things so we don't have all the duplication? I > >>> suspect this won't be the last device which needs the same treatement. > >> > >> Should the refactoring just accommodate SPE, and TRBE or make it more generic to > >> accommodate future devices as well. Something like the following enumeration. > >> > >> enum arm_platform_device { > >> ARM_PLATFORM_DEVICE_SPE, > >> ARM_PLATFORM_DEVICE_TRBE, > >> ARM_PLATFORM_DEVICE_MAX, > >> }; > >> > >> But that would require adding some helper functions to select these following > >> elements based on the above enumeration via a common function > >> > >> - gicc->XXX_interrupt > >> - ACPI_MADT_GICC_SPE/TRBE for header length comparison > >> - static struct platform_device/resources (static objects in the file) > >> > >> Seems like will add much more code for a refactor. Did you have something else > >> in mind for the refactor. > > > > All I'm saying is that we shouldn't have identical copies of the code to > > walk the MADT, pull out the irqs and register the device. > > > > So something like the totally untested hack below. I probably broke > > something, but hopefully you see what I mean. > > > > Will > > > > --->8 > > > > diff --git a/drivers/perf/arm_pmu_acpi.c b/drivers/perf/arm_pmu_acpi.c > > index 90815ad762eb..7f1cf36c6e69 100644 > > --- a/drivers/perf/arm_pmu_acpi.c > > +++ b/drivers/perf/arm_pmu_acpi.c > > @@ -69,6 +69,62 @@ static void arm_pmu_acpi_unregister_irq(int cpu) > > acpi_unregister_gsi(gsi); > > } > > > > +static int > > +arm_acpi_register_pmu_device(struct platform_device *pdev, u8 len, > > + u16 (*parse_gsi)(struct acpi_madt_generic_interrupt *)) > > This factored out helper should be wrapped inside CONFIG_ARM_SPE_PMU > and CONFIG_CORESIGHT_TRBE ? Otherwise, there will be no callers left > for this helper triggering warning. > > drivers/perf/arm_pmu_acpi.c:73:1: warning: ‘arm_acpi_register_pmu_device’ defined but not used [-Wunused-function] > 73 | arm_acpi_register_pmu_device(struct platform_device *pdev, u8 len, > | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~ > > But in that case, we have to keep adding new configs when new devices > require platform devices to be registered. Is there a better way ?
__maybe_unused?
Like I said, I didn't test that thing at all, I was just trying to illustrate the sort of refactoring I had in mind.
Will
| |