1*a0d58937SLukas WunnerContributions are solicited in particular to remedy the following issues: 2*a0d58937SLukas Wunner 3*a0d58937SLukas Wunnercpcihp: 4*a0d58937SLukas Wunner 5*a0d58937SLukas Wunner* There are no implementations of the ->hardware_test, ->get_power and 6*a0d58937SLukas Wunner ->set_power callbacks in struct cpci_hp_controller_ops. Why were they 7*a0d58937SLukas Wunner introduced? Can they be removed from the struct? 8*a0d58937SLukas Wunner 9*a0d58937SLukas Wunnercpqphp: 10*a0d58937SLukas Wunner 11*a0d58937SLukas Wunner* The driver spawns a kthread cpqhp_event_thread() which is woken by the 12*a0d58937SLukas Wunner hardirq handler cpqhp_ctrl_intr(). Convert this to threaded IRQ handling. 13*a0d58937SLukas Wunner The kthread is also woken from the timer pushbutton_helper_thread(), 14*a0d58937SLukas Wunner convert it to call irq_wake_thread(). Use pciehp as a template. 15*a0d58937SLukas Wunner 16*a0d58937SLukas Wunner* A large portion of cpqphp_ctrl.c and cpqphp_pci.c concerns resource 17*a0d58937SLukas Wunner management. Doesn't this duplicate functionality in the core? 18*a0d58937SLukas Wunner 19*a0d58937SLukas Wunneribmphp: 20*a0d58937SLukas Wunner 21*a0d58937SLukas Wunner* Implementations of hotplug_slot_ops callbacks such as get_adapter_present() 22*a0d58937SLukas Wunner in ibmphp_core.c create a copy of the struct slot on the stack, then perform 23*a0d58937SLukas Wunner the actual operation on that copy. Determine if this overhead is necessary, 24*a0d58937SLukas Wunner delete it if not. The functions also perform a NULL pointer check on the 25*a0d58937SLukas Wunner struct hotplug_slot, this seems superfluous. 26*a0d58937SLukas Wunner 27*a0d58937SLukas Wunner* Several functions access the pci_slot member in struct hotplug_slot even 28*a0d58937SLukas Wunner though pci_hotplug.h declares it private. See get_max_bus_speed() for an 29*a0d58937SLukas Wunner example. Either the pci_slot member should no longer be declared private 30*a0d58937SLukas Wunner or ibmphp should store a pointer to its bus in struct slot. Probably the 31*a0d58937SLukas Wunner former. 32*a0d58937SLukas Wunner 33*a0d58937SLukas Wunner* The functions get_max_adapter_speed() and get_bus_name() are commented out. 34*a0d58937SLukas Wunner Can they be deleted? There are also forward declarations at the top of 35*a0d58937SLukas Wunner ibmphp_core.c as well as pointers in ibmphp_hotplug_slot_ops, likewise 36*a0d58937SLukas Wunner commented out. 37*a0d58937SLukas Wunner 38*a0d58937SLukas Wunner* ibmphp_init_devno() takes a struct slot **, it could instead take a 39*a0d58937SLukas Wunner struct slot *. 40*a0d58937SLukas Wunner 41*a0d58937SLukas Wunner* The return value of pci_hp_register() is not checked. 42*a0d58937SLukas Wunner 43*a0d58937SLukas Wunner* iounmap(io_mem) is called in the error path of ebda_rsrc_controller() 44*a0d58937SLukas Wunner and once more in the error path of its caller ibmphp_access_ebda(). 45*a0d58937SLukas Wunner 46*a0d58937SLukas Wunner* The various slot data structures are difficult to follow and need to be 47*a0d58937SLukas Wunner simplified. A lot of functions are too large and too complex, they need 48*a0d58937SLukas Wunner to be broken up into smaller, manageable pieces. Negative examples are 49*a0d58937SLukas Wunner ebda_rsrc_controller() and configure_bridge(). 50*a0d58937SLukas Wunner 51*a0d58937SLukas Wunner* A large portion of ibmphp_res.c and ibmphp_pci.c concerns resource 52*a0d58937SLukas Wunner management. Doesn't this duplicate functionality in the core? 53*a0d58937SLukas Wunner 54*a0d58937SLukas Wunnersgi_hotplug: 55*a0d58937SLukas Wunner 56*a0d58937SLukas Wunner* Several functions access the pci_slot member in struct hotplug_slot even 57*a0d58937SLukas Wunner though pci_hotplug.h declares it private. See sn_hp_destroy() for an 58*a0d58937SLukas Wunner example. Either the pci_slot member should no longer be declared private 59*a0d58937SLukas Wunner or sgi_hotplug should store a pointer to it in struct slot. Probably the 60*a0d58937SLukas Wunner former. 61*a0d58937SLukas Wunner 62*a0d58937SLukas Wunnershpchp: 63*a0d58937SLukas Wunner 64*a0d58937SLukas Wunner* There is only a single implementation of struct hpc_ops. Can the struct be 65*a0d58937SLukas Wunner removed and its functions invoked directly? This has already been done in 66*a0d58937SLukas Wunner pciehp with commit 82a9e79ef132 ("PCI: pciehp: remove hpc_ops"). Clarify 67*a0d58937SLukas Wunner if there was a specific reason not to apply the same change to shpchp. 68*a0d58937SLukas Wunner 69*a0d58937SLukas Wunner* The ->get_mode1_ECC_cap callback in shpchp_hpc_ops is never invoked. 70*a0d58937SLukas Wunner Why was it introduced? Can it be removed? 71*a0d58937SLukas Wunner 72*a0d58937SLukas Wunner* The hardirq handler shpc_isr() queues events on a workqueue. It can be 73*a0d58937SLukas Wunner simplified by converting it to threaded IRQ handling. Use pciehp as a 74*a0d58937SLukas Wunner template. 75