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