diff mbox series

[v3] PCI: hotplug: Fix kernel-doc formatting and add missing documentation

Message ID 20210702231541.1671875-1-kw@linux.com (mailing list archive)
State Superseded
Delegated to: Bjorn Helgaas
Headers show
Series [v3] PCI: hotplug: Fix kernel-doc formatting and add missing documentation | expand

Commit Message

Krzysztof Wilczyński July 2, 2021, 11:15 p.m. UTC 
Fix kernel-doc formatting and add missing documentation for the
parameters "bus" of the get_slot_mapping() function, "t" of the
cpqhp_pushbutton_thread() function and "controller" of the
inband_presence_disabled() function.

Additionally, fix formatting and add missing documentation for the
members "slot_list" and "pci_slot" of the struct hotplug_slot.

Also remove surplus parameter "slot" from the description of the
function cpqhp_pushbutton_thread().

Thus, resolve build time warnings related to kernel-doc:

  drivers/pci/hotplug/cpqphp_core.c:308: warning: Function parameter or member 'bus' not described in 'get_slot_mapping'
  drivers/pci/hotplug/cpqphp_core.c:308: warning: Function parameter or member 'bus_num' not described in 'get_slot_mapping'
  drivers/pci/hotplug/cpqphp_core.c:308: warning: Function parameter or member 'dev_num' not described in 'get_slot_mapping'
  drivers/pci/hotplug/cpqphp_core.c:308: warning: Function parameter or member 'slot' not described in 'get_slot_mapping'

  drivers/pci/hotplug/cpqphp_ctrl.c:1887: warning: Function parameter or member 't' not described in 'cpqhp_pushbutton_thread'
  drivers/pci/hotplug/cpqphp_ctrl.c:1887: warning: Excess function parameter 'slot' description in 'cpqhp_pushbutton_thread'

  drivers/pci/hotplug/pciehp.h:110: warning: Function parameter or member 'inband_presence_disabled' not described in 'controller'

  include/linux/pci_hotplug.h:64: warning: Function parameter or member 'slot_list' not described in 'hotplug_slot'
  include/linux/pci_hotplug.h:64: warning: Function parameter or member 'pci_slot' not described in 'hotplug_slot'

No change to functionality intended.

Signed-off-by: Krzysztof Wilczyński <kw@linux.com>
---
Changes in v3:
  Added kernel-doc fixes for the file include/linux/pci_hotplug.h.
Changes in v2:
  Added kernel-doc fixes for the files drivers/pci/hotplug/cpqphp_ctrl.c
  and drivers/pci/hotplug/pciehp.h.

 drivers/pci/hotplug/cpqphp_core.c | 16 ++++---
 drivers/pci/hotplug/cpqphp_ctrl.c |  4 +-
 drivers/pci/hotplug/pciehp.h      | 80 ++++++++++++++++++-------------
 include/linux/pci_hotplug.h       | 12 +++--
 4 files changed, 67 insertions(+), 45 deletions(-)

Comments

Lukas Wunner July 3, 2021, 6:48 a.m. UTC | #1 
On Fri, Jul 02, 2021 at 11:15:41PM +0000, Krzysztof Wilczy??ski wrote:
> Fix kernel-doc formatting and add missing documentation for the
[...]
>   drivers/pci/hotplug/pciehp.h:110: warning: Function parameter or member 'inband_presence_disabled' not described in 'controller'
[...]
> diff --git a/drivers/pci/hotplug/pciehp.h b/drivers/pci/hotplug/pciehp.h
> index 4fd200d8b0a9..c8b2b1e046e8 100644
> --- a/drivers/pci/hotplug/pciehp.h
> +++ b/drivers/pci/hotplug/pciehp.h
> @@ -44,38 +44,54 @@ extern int pciehp_poll_time;
>  #define SLOT_NAME_SIZE 10
>  
>  /**
> - * struct controller - PCIe hotplug controller
> - * @pcie: pointer to the controller's PCIe port service device
> - * @slot_cap: cached copy of the Slot Capabilities register
> - * @slot_ctrl: cached copy of the Slot Control register
[...]
> + * struct controller - PCIe hotplug controller.
> + * @pcie:			Pointer to the controller's PCIe port service
> + *				device.
> + * @slot_cap:			Cached copy of the Slot Capabilities register.

Is it really necessary to change the indentation and add the trailing
period *everywhere*?  What value does that add?

Changes like this make it more difficult to determine the commit from
which a certain line originates.

I respectfully submit that the formatting is fine and there's nothing
to be "fixed" here (as the commit message claims).


> + * @inband_presence_disabled:	Flag to used to track whether the in-band
> + *				presence detection is disabled.

That's not proper English and also not very useful because the documentation
merely repeats what the flag's name says.  I'd suggest something along the
lines of:

 * @inband_presence_disabled: whether In-Band Presence Detect Disable is
 *	supported by the controller and disabled per spec recommendation
 *	(PCIe r5.0, appendix I implementation note)

Thanks,

Lukas
Krzysztof Wilczyński July 3, 2021, 8:51 a.m. UTC | #2 
Hi Lukas,

Thank you for feedback!

[...]
> I respectfully submit that the formatting is fine and there's nothing
> to be "fixed" here (as the commit message claims).

The fixing it probably more relevant to the two first hunks, everything
else would be more of a style update so that kernel-doc is kept with the
following guideline:
  https://www.kernel.org/doc/html/latest/doc-guide/kernel-doc.html

Which is something other users of kernel-doc seldom embrace as I could
only find a handful of places where this format is somewhat followed.

Thus, like you say, keeping the scope of changes to only updating what
matters would be more appropriate.  I will send another revision that
does exactly that.

> > + * @inband_presence_disabled:	Flag to used to track whether the in-band
> > + *				presence detection is disabled.
> 
> That's not proper English and also not very useful because the documentation
> merely repeats what the flag's name says.  I'd suggest something along the
> lines of:
>  * @inband_presence_disabled: whether In-Band Presence Detect Disable is
>  *	supported by the controller and disabled per spec recommendation
>  *	(PCIe r5.0, appendix I implementation note)

Thank you!  I will use your version going forward.

	Krzysztof
diff mbox series

Patch

diff --git a/drivers/pci/hotplug/cpqphp_core.c b/drivers/pci/hotplug/cpqphp_core.c
index b8aacb41a83c..efe6031c1e68 100644
--- a/drivers/pci/hotplug/cpqphp_core.c
+++ b/drivers/pci/hotplug/cpqphp_core.c
@@ -292,15 +292,17 @@  static int ctrl_slot_cleanup(struct controller *ctrl)
 
 
 /**
- * get_slot_mapping - determine logical slot mapping for PCI device
+ * get_slot_mapping - Determine logical slot mapping for PCI device.
+ * @bus:	Pointer to the PCI bus structure.
+ * @bus_num:	Bus number of the PCI device.
+ * @dev_num:	Device number of the PCI device.
+ * @slot:	Pointer to where slot number will be returned.
  *
- * Won't work for more than one PCI-PCI bridge in a slot.
+ * Will not work for more than one PCI-PCI bridge in a slot.
  *
- * @bus_num - bus number of PCI device
- * @dev_num - device number of PCI device
- * @slot - Pointer to u8 where slot number will	be returned
- *
- * Output:	SUCCESS or FAILURE
+ * Return:
+ * * 0		- Logical slot mapping has been found for this PCI device.
+ * * < 0	- Unable to find entry in the routing table for this PCI device.
  */
 static int
 get_slot_mapping(struct pci_bus *bus, u8 bus_num, u8 dev_num, u8 *slot)
diff --git a/drivers/pci/hotplug/cpqphp_ctrl.c b/drivers/pci/hotplug/cpqphp_ctrl.c
index 68de958a9be8..035114423ebb 100644
--- a/drivers/pci/hotplug/cpqphp_ctrl.c
+++ b/drivers/pci/hotplug/cpqphp_ctrl.c
@@ -1876,8 +1876,8 @@  static void interrupt_event_handler(struct controller *ctrl)
 
 
 /**
- * cpqhp_pushbutton_thread - handle pushbutton events
- * @slot: target slot (struct)
+ * cpqhp_pushbutton_thread - Handle pushbutton events.
+ * @t: Pointer to struct timer_list which holds all timer related callbacks.
  *
  * Scheduled procedure to handle blocking stuff for the pushbuttons.
  * Handles all pending events and exits.
diff --git a/drivers/pci/hotplug/pciehp.h b/drivers/pci/hotplug/pciehp.h
index 4fd200d8b0a9..c8b2b1e046e8 100644
--- a/drivers/pci/hotplug/pciehp.h
+++ b/drivers/pci/hotplug/pciehp.h
@@ -44,38 +44,54 @@  extern int pciehp_poll_time;
 #define SLOT_NAME_SIZE 10
 
 /**
- * struct controller - PCIe hotplug controller
- * @pcie: pointer to the controller's PCIe port service device
- * @slot_cap: cached copy of the Slot Capabilities register
- * @slot_ctrl: cached copy of the Slot Control register
- * @ctrl_lock: serializes writes to the Slot Control register
- * @cmd_started: jiffies when the Slot Control register was last written;
- *	the next write is allowed 1 second later, absent a Command Completed
- *	interrupt (PCIe r4.0, sec 6.7.3.2)
- * @cmd_busy: flag set on Slot Control register write, cleared by IRQ handler
- *	on reception of a Command Completed event
- * @queue: wait queue to wake up on reception of a Command Completed event,
- *	used for synchronous writes to the Slot Control register
- * @pending_events: used by the IRQ handler to save events retrieved from the
- *	Slot Status register for later consumption by the IRQ thread
- * @notification_enabled: whether the IRQ was requested successfully
- * @power_fault_detected: whether a power fault was detected by the hardware
- *	that has not yet been cleared by the user
- * @poll_thread: thread to poll for slot events if no IRQ is available,
- *	enabled with pciehp_poll_mode module parameter
- * @state: current state machine position
- * @state_lock: protects reads and writes of @state;
- *	protects scheduling, execution and cancellation of @button_work
- * @button_work: work item to turn the slot on or off after 5 seconds
- *	in response to an Attention Button press
- * @hotplug_slot: structure registered with the PCI hotplug core
- * @reset_lock: prevents access to the Data Link Layer Link Active bit in the
- *	Link Status register and to the Presence Detect State bit in the Slot
- *	Status register during a slot reset which may cause them to flap
- * @ist_running: flag to keep user request waiting while IRQ thread is running
- * @request_result: result of last user request submitted to the IRQ thread
- * @requester: wait queue to wake up on completion of user request,
- *	used for synchronous slot enable/disable request via sysfs
+ * struct controller - PCIe hotplug controller.
+ * @pcie:			Pointer to the controller's PCIe port service
+ *				device.
+ * @slot_cap:			Cached copy of the Slot Capabilities register.
+ * @inband_presence_disabled:	Flag to used to track whether the in-band
+ *				presence detection is disabled.
+ * @slot_ctrl:			Cached copy of the Slot Control register.
+ * @ctrl_lock:			Serializes writes to the Slot Control register.
+ * @cmd_started:		Jiffies when the Slot Control register was last
+ *				written; the next write is allowed 1 second
+ *				later, absent a Command Completed interrupt
+ *				(PCIe r4.0, sec 6.7.3.2).
+ * @cmd_busy:			Flag set on Slot Control register write, cleared
+ *				by IRQ handler on reception of a Command
+ *				Completed event.
+ * @queue:			Wait queue to wake up on reception of a Command
+ *				Completed event, used for synchronous writes to
+ *				the Slot Control register.
+ * @pending_events:		Used by the IRQ handler to save events retrieved
+ *				from the Slot Status register for later
+ *				consumption by the IRQ thread.
+ * @notification_enabled:	Whether the IRQ was requested successfully.
+ * @power_fault_detected:	Whether a power fault was detected by the
+ *				hardware that has not yet been cleared by the
+ *				user.
+ * @poll_thread:		Thread to poll for slot events if no IRQ is
+ *				available, enabled with pciehp_poll_mode module
+ *				parameter.
+ * @state:			Current state machine position.
+ * @state_lock:			Protects reads and writes of @state; protects
+ *				scheduling, execution and cancellation of
+ *				@button_work.
+ * @button_work:		Work item to turn the slot on or off after
+ *				5 seconds in response to an Attention Button
+ *				press.
+ * @hotplug_slot:		Structure registered with the PCI hotplug core.
+ * @reset_lock:			Prevents access to the Data Link Layer Link
+ *				Active bit in the Link Status register and to
+ *				the Presence Detect State bit in the Slot Status
+ *				register during a slot reset which may cause
+ *				them to flap.
+ * @ist_running:		Flag to keep user request waiting while IRQ
+ *				thread is running.
+ * @request_result:		Result of last user request submitted to the IRQ
+ *				thread.
+ * @requester:			Wait queue to wake up on completion of user
+ *				request, used for synchronous slot
+ *				enable/disable request via sysfs.
  *
  * PCIe hotplug has a 1:1 relationship between controller and slot, hence
  * unlike other drivers, the two aren't represented by separate structures.
diff --git a/include/linux/pci_hotplug.h b/include/linux/pci_hotplug.h
index b482e42d7153..a73851884b81 100644
--- a/include/linux/pci_hotplug.h
+++ b/include/linux/pci_hotplug.h
@@ -48,10 +48,14 @@  struct hotplug_slot_ops {
 };
 
 /**
- * struct hotplug_slot - used to register a physical slot with the hotplug pci core
- * @ops: pointer to the &struct hotplug_slot_ops to be used for this slot
- * @owner: The module owner of this structure
- * @mod_name: The module name (KBUILD_MODNAME) of this structure
+ * struct hotplug_slot - Used to register a physical slot with the hotplug PCI
+ *			 core.
+ * @ops:	Pointer to the &struct hotplug_slot_ops to be used for this
+ *		slot.
+ * @slot_list:	Internal list used to track hotplug PCI slots.
+ * @pci_slot:	Pepresents a physical slot.
+ * @owner:	The module owner of this structure.
+ * @mod_name:	The module name (KBUILD_MODNAME) of this structure.
  */
 struct hotplug_slot {
 	const struct hotplug_slot_ops	*ops;