On 1/29/19 3:31 AM, Michael Ellerman wrote: > Tyrel Datwyler <turtle.in.the.ker...@gmail.com> writes: >> On 12/14/2018 12:50 PM, Michael Bringmann wrote: >>> Define interface to acquire arch-specific drc info to match against >>> hotpluggable devices. The current implementation exposes several >>> pseries-specific dynamic memory properties in generic kernel code. >>> This patch set provides an interface to pull that code out of the >>> generic kernel. >>> >>> Signed-off-by: Michael Bringmann <m...@linux.vnet.ibm.com> >>> --- >>> include/linux/topology.h | 9 +++++++++ >>> 1 file changed, 9 insertions(+) >>> >>> diff --git a/include/linux/topology.h b/include/linux/topology.h >>> index cb0775e..df97f5f 100644 >>> --- a/include/linux/topology.h >>> +++ b/include/linux/topology.h >>> @@ -44,6 +44,15 @@ >> >> As far as I know pseries is the only platform that uses DR connectors, and I >> highly doubt that any other powerpc platform or arch ever will. So, I'm not >> sure >> that this is really generic enough to belong in topology.h. > > Right. This does not belong in include/linux. > >> If anything I would >> suggest putting this in an include in arch/powerpc/include/ named something >> like >> drcinfo.h or pseries-drc.h. That will make it visible to modules like rpaphp >> that want/need to use this functionality. > > Yeah that would make more sense.
If you see no objection to referencing a powerpc-specific function from the code ... > > Using "arch" in the name is wrong, it's pseries specific so > pseries_find_drc_match() would be more appropriate. > >>> +int arch_find_drc_match(struct device_node *dn, >>> + bool (*usercb)(struct device_node *dn, >>> + u32 drc_index, char *drc_name, >>> + char *drc_type, u32 drc_power_domain, >>> + void *data), >>> + char *opt_drc_type, char *opt_drc_name, >>> + bool match_drc_index, bool ck_php_type, >>> + void *data); > > This function signature is kind of insane. > > You end with calls like: > > + return arch_find_drc_match(dn, rpaphp_add_slot_cb, > + NULL, NULL, false, true, NULL); > > Which is impossible to parse. > > I feel like maybe this isn't the right level of abstraction. ... I had already been considering simplifying the interface for these calls to something like the following: int rpaphp_check_drc_props(struct device_node *dn, char *drc_name, char *drc_type) { return pseries_find_drc_match(dn, drc_type, drc_name); } ... int rpaphp_add_slot(struct device_node *dn) { if (!dn->name || strcmp(dn->name, "pci")) return 0; return pseries_add_drc_slot(dn, rpaphp_add_slot_cb); } ... Further details would be hidden within the pseries code. > > cheers Regards -- Michael W. Bringmann Linux Technology Center IBM Corporation Tie-Line 363-5196 External: (512) 286-5196 Cell: (512) 466-0650 m...@linux.vnet.ibm.com