linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] docs: driver-model: bus.rst: Clean up the formatting, expound, modernize
@ 2020-12-21  7:52 Michael Witten
  2021-01-11 20:31 ` Jonathan Corbet
  0 siblings, 1 reply; 4+ messages in thread
From: Michael Witten @ 2020-12-21  7:52 UTC (permalink / raw)
  To: Jonathan Corbet
  Cc: Mauro Carvalho Chehab, Jeff Kirsher, linux-doc, linux-kernel

* The reStructuredText had some indentation issues.

* The HTML output was not properly formatted in places.

* Some of the details were lacking or needed clarification (especially
  with regard to how a `struct bus_type` object should be defined).

* The sysfs example hierarchy appeared outdated; I've updated it with
  output based on what my own system currently displays.

Signed-off-by: Michael Witten <mfwitten@gmail.com>
---
 Documentation/driver-api/driver-model/bus.rst | 110 +++++++++++++--------
 1 file changed, 67 insertions(+), 43 deletions(-)

diff --git a/Documentation/driver-api/driver-model/bus.rst b/Documentation/driver-api/driver-model/bus.rst
index 016b15a6e8ea..68a95389b1eb 100644
--- a/Documentation/driver-api/driver-model/bus.rst
+++ b/Documentation/driver-api/driver-model/bus.rst
@@ -4,34 +4,58 @@ Bus Types
 
 Definition
 ~~~~~~~~~~
-See the kerneldoc for the struct bus_type.
-
-int bus_register(struct bus_type * bus);
+* ``struct bus_type``;
+* ``int bus_register(struct bus_type *bus);``
 
 
 Declaration
 ~~~~~~~~~~~
 
-Each bus type in the kernel (PCI, USB, etc) should declare one static
-object of this type. They must initialize the name field, and may
-optionally initialize the match callback::
+For each bus type (PCI, USB, etc), there should be code that defines
+one object of type ``struct bus_type``:
+
+1. The definition should declare a file-scope identifier that has
+   external linkage.
+
+   * There should be a header that provides a declaration of this
+     identifier.
+
+   * The identifier should be explicitly exported.
+
+2. The definition should initialize the ``name`` member. Other
+   members may also be initialized (such as the ``match`` callback
+   member).
+
+For instance, here is the definition for the PCI bus type::
 
-   struct bus_type pci_bus_type = {
-          .name	= "pci",
-          .match	= pci_bus_match,
-   };
+	struct bus_type pci_bus_type = {
+		.name          = "pci",               // REQUIRED
+		.match         = pci_bus_match,
+		.uevent        = pci_uevent,
+		.probe         = pci_device_probe,
+		.remove        = pci_device_remove,
+		.shutdown      = pci_device_shutdown,
+		.dev_groups    = pci_dev_groups,
+		.bus_groups    = pci_bus_groups,
+		.drv_groups    = pci_drv_groups,
+		.pm            = PCI_PM_OPS_PTR,
+		.num_vf        = pci_bus_num_vf,
+		.dma_configure = pci_dma_configure,
+	};
 
-The structure should be exported to drivers in a header file:
+	EXPORT_SYMBOL(pci_bus_type);
 
-extern struct bus_type pci_bus_type;
+The relevant API header should include the following declaration::
+
+	extern struct bus_type pci_bus_type;
 
 
 Registration
 ~~~~~~~~~~~~
 
-When a bus driver is initialized, it calls bus_register. This
-initializes the rest of the fields in the bus object and inserts it
-into a global list of bus types. Once the bus object is registered,
+During initialization of a bus driver, ``bus_register()`` is called; this
+initializes the rest of the fields in the bus type object and inserts it
+into a global list of bus types. Once the bus type object is registered,
 the fields in it are usable by the bus driver.
 
 
@@ -61,22 +85,25 @@ does not have a driver associated with it.
 Device and Driver Lists
 ~~~~~~~~~~~~~~~~~~~~~~~
 
-The lists of devices and drivers are intended to replace the local
-lists that many buses keep. They are lists of struct devices and
-struct device_drivers, respectively. Bus drivers are free to use the
-lists as they please, but conversion to the bus-specific type may be
-necessary.
+There are generic facilities for keeping lists of devices and drivers:
+
+* There is a list of ``struct device`` objects.
+* There is a list of ``struct device_driver`` objects.
+
+Bus drivers are free to use the lists as they please, but conversion
+to a bus-specific type may be necessary.
 
 The LDM core provides helper functions for iterating over each list::
 
-  int bus_for_each_dev(struct bus_type * bus, struct device * start,
-		       void * data,
-		       int (*fn)(struct device *, void *));
+	int bus_for_each_dev(struct bus_type *bus, struct device *start,
+	                     void *data,
+	                     int (*fn)(struct device *, void *));
 
-  int bus_for_each_drv(struct bus_type * bus, struct device_driver * start,
-		       void * data, int (*fn)(struct device_driver *, void *));
+	int bus_for_each_drv(struct bus_type *bus, struct device_driver *start,
+	                     void *data,
+	                     int (*fn)(struct device_driver *, void *));
 
-These helpers iterate over the respective list, and call the callback
+These helpers iterate over the respective list, and call the callback (``fn``)
 for each device or driver in the list. All list accesses are
 synchronized by taking the bus's lock (read currently). The reference
 count on each object in the list is incremented before the callback is
@@ -112,9 +139,9 @@ hierarchy::
 
 	/sys/bus/pci/
 	|-- devices
-	|   |-- 00:00.0 -> ../../../root/pci0/00:00.0
-	|   |-- 00:01.0 -> ../../../root/pci0/00:01.0
-	|   `-- 00:02.0 -> ../../../root/pci0/00:02.0
+	|   |-- 0000:00:00.0 -> ../../../devices/pci0000:00/0000:00:00.0
+	|   |-- 0000:00:02.0 -> ../../../devices/pci0000:00/0000:00:02.0
+	|   `-- 0000:00:14.0 -> ../../../devices/pci0000:00/0000:00:14.0
 	`-- drivers
 
 
@@ -123,23 +150,20 @@ Exporting Attributes
 
 ::
 
-  struct bus_attribute {
-	struct attribute	attr;
-	ssize_t (*show)(struct bus_type *, char * buf);
-	ssize_t (*store)(struct bus_type *, const char * buf, size_t count);
-  };
-
-Bus drivers can export attributes using the BUS_ATTR_RW macro that works
-similarly to the DEVICE_ATTR_RW macro for devices. For example, a
-definition like this::
-
-	static BUS_ATTR_RW(debug);
+	struct bus_attribute {
+		struct attribute attr;
+		ssize_t (*show)(struct bus_type *, char *buf);
+		ssize_t (*store)(struct bus_type *, const char *buf, size_t count);
+	};
 
-is equivalent to declaring::
+Bus drivers can export attributes using the ``BUS_ATTR_RW()`` macro that works
+similarly to the ``DEVICE_ATTR_RW()`` macro for devices. For example, the
+following are equivalent:
 
-	static bus_attribute bus_attr_debug;
+* ``static BUS_ATTR_RW(debug);``
+* ``static bus_attribute bus_attr_debug;``
 
-This can then be used to add and remove the attribute from the bus's
+This can then be used to add or remove the attribute from the bus's
 sysfs directory using::
 
 	int bus_create_file(struct bus_type *, struct bus_attribute *);
-- 
2.22.0


^ permalink raw reply related	[flat|nested] 4+ messages in thread

* Re: [PATCH] docs: driver-model: bus.rst: Clean up the formatting, expound, modernize
  2020-12-21  7:52 [PATCH] docs: driver-model: bus.rst: Clean up the formatting, expound, modernize Michael Witten
@ 2021-01-11 20:31 ` Jonathan Corbet
  2021-01-13  5:35   ` [PATCH v2] " Michael Witten
  0 siblings, 1 reply; 4+ messages in thread
From: Jonathan Corbet @ 2021-01-11 20:31 UTC (permalink / raw)
  To: Michael Witten
  Cc: Mauro Carvalho Chehab, Jeff Kirsher, linux-doc, linux-kernel

On Mon, 21 Dec 2020 07:52:00 -0000
Michael Witten <mfwitten@gmail.com> wrote:

> * The reStructuredText had some indentation issues.
> 
> * The HTML output was not properly formatted in places.
> 
> * Some of the details were lacking or needed clarification (especially
>   with regard to how a `struct bus_type` object should be defined).
> 
> * The sysfs example hierarchy appeared outdated; I've updated it with
>   output based on what my own system currently displays.
> 
> Signed-off-by: Michael Witten <mfwitten@gmail.com>
> ---
>  Documentation/driver-api/driver-model/bus.rst | 110 +++++++++++++--------
>  1 file changed, 67 insertions(+), 43 deletions(-)

Thanks for working to improve the docs.  I have a couple of requests,
though... 

> diff --git a/Documentation/driver-api/driver-model/bus.rst b/Documentation/driver-api/driver-model/bus.rst
> index 016b15a6e8ea..68a95389b1eb 100644
> --- a/Documentation/driver-api/driver-model/bus.rst
> +++ b/Documentation/driver-api/driver-model/bus.rst
> @@ -4,34 +4,58 @@ Bus Types
>  
>  Definition
>  ~~~~~~~~~~
> -See the kerneldoc for the struct bus_type.
> -
> -int bus_register(struct bus_type * bus);
> +* ``struct bus_type``;
> +* ``int bus_register(struct bus_type *bus);``

This should just be made into a literal block like the others.

>  Declaration
>  ~~~~~~~~~~~
>  
> -Each bus type in the kernel (PCI, USB, etc) should declare one static
> -object of this type. They must initialize the name field, and may
> -optionally initialize the match callback::
> +For each bus type (PCI, USB, etc), there should be code that defines
> +one object of type ``struct bus_type``:

It is better not to mark types as ``literal`` this way; the build system is
getting better at recognizing such things on its own and generating the
appropriate links.

[...]

>  Registration
>  ~~~~~~~~~~~~
>  
> -When a bus driver is initialized, it calls bus_register. This
> -initializes the rest of the fields in the bus object and inserts it
> -into a global list of bus types. Once the bus object is registered,
> +During initialization of a bus driver, ``bus_register()`` is called; this

*definitely* don't mark functions as literal in this way; simply say
 bus_register() and the Right Things will happen.

Thanks,

jon

^ permalink raw reply	[flat|nested] 4+ messages in thread

* [PATCH v2] docs: driver-model: bus.rst: Clean up the formatting, expound, modernize
  2021-01-11 20:31 ` Jonathan Corbet
@ 2021-01-13  5:35   ` Michael Witten
  2021-01-18 20:20     ` Jonathan Corbet
  0 siblings, 1 reply; 4+ messages in thread
From: Michael Witten @ 2021-01-13  5:35 UTC (permalink / raw)
  To: Jonathan Corbet; +Cc: Mauro Carvalho Chehab, linux-doc, linux-kernel

Thanks for your guidance.

  * Save this patch to:

      /path/to/email

    Then apply it with:

      git am --scissors /path/to/email

  * Here are the changes from the last patch:

       Definition
       ~~~~~~~~~~
      -* ``struct bus_type``;
      -* ``int bus_register(struct bus_type *bus);``
      +
      +::
      +
      +	struct bus_type;
      +	int bus_register(struct bus_type *bus);


       Declaration
       ~~~~~~~~~~~

       For each bus type (PCI, USB, etc), there should be code that defines
      -one object of type ``struct bus_type``:
      +one object of type struct bus_type:

       1. The definition should declare a file-scope identifier that has
          external linkage.
      @@ -53,7 +56,7 @@ The relevant API header should include the following declaration::
       Registration
       ~~~~~~~~~~~~

      -During initialization of a bus driver, ``bus_register()`` is called; this
      +During initialization of a bus driver, bus_register() is called; this
       initializes the rest of the fields in the bus type object and inserts it
       into a global list of bus types. Once the bus type object is registered,
       the fields in it are usable by the bus driver.
      @@ -77,8 +80,8 @@ particular device, without sacrificing bus-specific functionality or
       type-safety.

       When a driver is registered with the bus, the bus's list of devices is
      -iterated over, and the match callback is called for each device that
      -does not have a driver associated with it.
      +iterated over, and the ``match`` callback is called for each device that
      +does not already have a driver associated with it.



      @@ -87,8 +90,8 @@ Device and Driver Lists

       There are generic facilities for keeping lists of devices and drivers:

      -* There is a list of ``struct device`` objects.
      -* There is a list of ``struct device_driver`` objects.
      +* There is a list of struct device objects.
      +* There is a list of struct device_driver objects.

       Bus drivers are free to use the lists as they please, but conversion
       to a bus-specific type may be necessary.
      @@ -156,12 +159,21 @@ Exporting Attributes
       		ssize_t (*store)(struct bus_type *, const char *buf, size_t count);
       	};

      -Bus drivers can export attributes using the ``BUS_ATTR_RW()`` macro that works
      -similarly to the ``DEVICE_ATTR_RW()`` macro for devices. For example, the
      +Bus drivers can export attributes using the BUS_ATTR_RW() macro that works
      +similarly to the DEVICE_ATTR_RW() macro for devices. For example, the
       following are equivalent:

      -* ``static BUS_ATTR_RW(debug);``
      -* ``static bus_attribute bus_attr_debug;``
      +* ::
      +
      +	static BUS_ATTR_RW(something);
      +
      +* ::
      +
      +	static struct bus_attribute bus_attr_something = {
      +		.attr  = { .name = "something", .mode = 0644 },
      +		.show  = something_show,   // These functions must be
      +		.store = something_store   // defined or declared already.
      +	};

       This can then be used to add or remove the attribute from the bus's
       sysfs directory using::

Sincerely,
Michael Witten

--8<----8<----8<----8<----8<----8<----8<----8<----8<----8<----8<----8<----8<--

* The reStructuredText had some indentation issues (I only fixed the
  areas I touched).

* The HTML output was not properly formatted in places.

* Some of the details were lacking or needed clarification (especially
  with regard to how a `struct bus_type` object should be defined).

* The sysfs example hierarchy appeared outdated; I've updated it with
  output based on what my own system currently displays.

Signed-off-by: Michael Witten <mfwitten@gmail.com>
---
 Documentation/driver-api/driver-model/bus.rst | 120 ++++++++++++------
 1 file changed, 78 insertions(+), 42 deletions(-)

diff --git a/Documentation/driver-api/driver-model/bus.rst b/Documentation/driver-api/driver-model/bus.rst
index 016b15a6e8ea..9a4cf15df8b9 100644
--- a/Documentation/driver-api/driver-model/bus.rst
+++ b/Documentation/driver-api/driver-model/bus.rst
@@ -4,34 +4,61 @@ Bus Types
 
 Definition
 ~~~~~~~~~~
-See the kerneldoc for the struct bus_type.
 
-int bus_register(struct bus_type * bus);
+::
+
+	struct bus_type;
+	int bus_register(struct bus_type *bus);
 
 
 Declaration
 ~~~~~~~~~~~
 
-Each bus type in the kernel (PCI, USB, etc) should declare one static
-object of this type. They must initialize the name field, and may
-optionally initialize the match callback::
+For each bus type (PCI, USB, etc), there should be code that defines
+one object of type struct bus_type:
+
+1. The definition should declare a file-scope identifier that has
+   external linkage.
+
+   * There should be a header that provides a declaration of this
+     identifier.
+
+   * The identifier should be explicitly exported.
+
+2. The definition should initialize the ``name`` member. Other
+   members may also be initialized (such as the ``match`` callback
+   member).
 
-   struct bus_type pci_bus_type = {
-          .name	= "pci",
-          .match	= pci_bus_match,
-   };
+For instance, here is the definition for the PCI bus type::
 
-The structure should be exported to drivers in a header file:
+	struct bus_type pci_bus_type = {
+		.name          = "pci",               // REQUIRED
+		.match         = pci_bus_match,
+		.uevent        = pci_uevent,
+		.probe         = pci_device_probe,
+		.remove        = pci_device_remove,
+		.shutdown      = pci_device_shutdown,
+		.dev_groups    = pci_dev_groups,
+		.bus_groups    = pci_bus_groups,
+		.drv_groups    = pci_drv_groups,
+		.pm            = PCI_PM_OPS_PTR,
+		.num_vf        = pci_bus_num_vf,
+		.dma_configure = pci_dma_configure,
+	};
 
-extern struct bus_type pci_bus_type;
+	EXPORT_SYMBOL(pci_bus_type);
+
+The relevant API header should include the following declaration::
+
+	extern struct bus_type pci_bus_type;
 
 
 Registration
 ~~~~~~~~~~~~
 
-When a bus driver is initialized, it calls bus_register. This
-initializes the rest of the fields in the bus object and inserts it
-into a global list of bus types. Once the bus object is registered,
+During initialization of a bus driver, bus_register() is called; this
+initializes the rest of the fields in the bus type object and inserts it
+into a global list of bus types. Once the bus type object is registered,
 the fields in it are usable by the bus driver.
 
 
@@ -53,30 +80,33 @@ particular device, without sacrificing bus-specific functionality or
 type-safety.
 
 When a driver is registered with the bus, the bus's list of devices is
-iterated over, and the match callback is called for each device that
-does not have a driver associated with it.
+iterated over, and the ``match`` callback is called for each device that
+does not already have a driver associated with it.
 
 
 
 Device and Driver Lists
 ~~~~~~~~~~~~~~~~~~~~~~~
 
-The lists of devices and drivers are intended to replace the local
-lists that many buses keep. They are lists of struct devices and
-struct device_drivers, respectively. Bus drivers are free to use the
-lists as they please, but conversion to the bus-specific type may be
-necessary.
+There are generic facilities for keeping lists of devices and drivers:
+
+* There is a list of struct device objects.
+* There is a list of struct device_driver objects.
+
+Bus drivers are free to use the lists as they please, but conversion
+to a bus-specific type may be necessary.
 
 The LDM core provides helper functions for iterating over each list::
 
-  int bus_for_each_dev(struct bus_type * bus, struct device * start,
-		       void * data,
-		       int (*fn)(struct device *, void *));
+	int bus_for_each_dev(struct bus_type *bus, struct device *start,
+	                     void *data,
+	                     int (*fn)(struct device *, void *));
 
-  int bus_for_each_drv(struct bus_type * bus, struct device_driver * start,
-		       void * data, int (*fn)(struct device_driver *, void *));
+	int bus_for_each_drv(struct bus_type *bus, struct device_driver *start,
+	                     void *data,
+	                     int (*fn)(struct device_driver *, void *));
 
-These helpers iterate over the respective list, and call the callback
+These helpers iterate over the respective list, and call the callback (``fn``)
 for each device or driver in the list. All list accesses are
 synchronized by taking the bus's lock (read currently). The reference
 count on each object in the list is incremented before the callback is
@@ -112,9 +142,9 @@ hierarchy::
 
 	/sys/bus/pci/
 	|-- devices
-	|   |-- 00:00.0 -> ../../../root/pci0/00:00.0
-	|   |-- 00:01.0 -> ../../../root/pci0/00:01.0
-	|   `-- 00:02.0 -> ../../../root/pci0/00:02.0
+	|   |-- 0000:00:00.0 -> ../../../devices/pci0000:00/0000:00:00.0
+	|   |-- 0000:00:02.0 -> ../../../devices/pci0000:00/0000:00:02.0
+	|   `-- 0000:00:14.0 -> ../../../devices/pci0000:00/0000:00:14.0
 	`-- drivers
 
 
@@ -123,23 +153,29 @@ Exporting Attributes
 
 ::
 
-  struct bus_attribute {
-	struct attribute	attr;
-	ssize_t (*show)(struct bus_type *, char * buf);
-	ssize_t (*store)(struct bus_type *, const char * buf, size_t count);
-  };
+	struct bus_attribute {
+		struct attribute attr;
+		ssize_t (*show)(struct bus_type *, char *buf);
+		ssize_t (*store)(struct bus_type *, const char *buf, size_t count);
+	};
+
+Bus drivers can export attributes using the BUS_ATTR_RW() macro that works
+similarly to the DEVICE_ATTR_RW() macro for devices. For example, the
+following are equivalent:
 
-Bus drivers can export attributes using the BUS_ATTR_RW macro that works
-similarly to the DEVICE_ATTR_RW macro for devices. For example, a
-definition like this::
+* ::
 
-	static BUS_ATTR_RW(debug);
+	static BUS_ATTR_RW(something);
 
-is equivalent to declaring::
+* ::
 
-	static bus_attribute bus_attr_debug;
+	static struct bus_attribute bus_attr_something = {
+		.attr  = { .name = "something", .mode = 0644 },
+		.show  = something_show,   // These functions must be
+		.store = something_store   // defined or declared already.
+	};
 
-This can then be used to add and remove the attribute from the bus's
+This can then be used to add or remove the attribute from the bus's
 sysfs directory using::
 
 	int bus_create_file(struct bus_type *, struct bus_attribute *);
-- 
2.22.0


^ permalink raw reply related	[flat|nested] 4+ messages in thread

* Re: [PATCH v2] docs: driver-model: bus.rst: Clean up the formatting, expound, modernize
  2021-01-13  5:35   ` [PATCH v2] " Michael Witten
@ 2021-01-18 20:20     ` Jonathan Corbet
  0 siblings, 0 replies; 4+ messages in thread
From: Jonathan Corbet @ 2021-01-18 20:20 UTC (permalink / raw)
  To: Michael Witten; +Cc: Mauro Carvalho Chehab, linux-doc, linux-kernel

On Wed, 13 Jan 2021 05:35:00 -0000
Michael Witten <mfwitten@gmail.com> wrote:

We're getting closer...

> Thanks for your guidance.
> 
>   * Save this patch to:
> 
>       /path/to/email
> 
>     Then apply it with:
> 
>       git am --scissors /path/to/email

Folks in the kernel community don't need to be taught how to apply patches,
please leave this stuff out in the future.

As for this text:

>   * Here are the changes from the last patch:
> 
>        Definition
>        ~~~~~~~~~~
>       -* ``struct bus_type``;
>       -* ``int bus_register(struct bus_type *bus);``
>       +
>       +::
>       +
>       +	struct bus_type;
>       +	int bus_register(struct bus_type *bus);
> 
> 
>        Declaration
>        ~~~~~~~~~~~
> 
>        For each bus type (PCI, USB, etc), there should be code that defines
>       -one object of type ``struct bus_type``:
>       +one object of type struct bus_type:
> 
>        1. The definition should declare a file-scope identifier that has
>           external linkage.
>       @@ -53,7 +56,7 @@ The relevant API header should include the following declaration::
>        Registration
>        ~~~~~~~~~~~~
> 
>       -During initialization of a bus driver, ``bus_register()`` is called; this
>       +During initialization of a bus driver, bus_register() is called; this
>        initializes the rest of the fields in the bus type object and inserts it
>        into a global list of bus types. Once the bus type object is registered,
>        the fields in it are usable by the bus driver.
>       @@ -77,8 +80,8 @@ particular device, without sacrificing bus-specific functionality or
>        type-safety.
> 
>        When a driver is registered with the bus, the bus's list of devices is
>       -iterated over, and the match callback is called for each device that
>       -does not have a driver associated with it.
>       +iterated over, and the ``match`` callback is called for each device that
>       +does not already have a driver associated with it.
> 
> 
> 
>       @@ -87,8 +90,8 @@ Device and Driver Lists
> 
>        There are generic facilities for keeping lists of devices and drivers:
> 
>       -* There is a list of ``struct device`` objects.
>       -* There is a list of ``struct device_driver`` objects.
>       +* There is a list of struct device objects.
>       +* There is a list of struct device_driver objects.
> 
>        Bus drivers are free to use the lists as they please, but conversion
>        to a bus-specific type may be necessary.
>       @@ -156,12 +159,21 @@ Exporting Attributes
>        		ssize_t (*store)(struct bus_type *, const char *buf, size_t count);
>        	};
> 
>       -Bus drivers can export attributes using the ``BUS_ATTR_RW()`` macro that works
>       -similarly to the ``DEVICE_ATTR_RW()`` macro for devices. For example, the
>       +Bus drivers can export attributes using the BUS_ATTR_RW() macro that works
>       +similarly to the DEVICE_ATTR_RW() macro for devices. For example, the
>        following are equivalent:
> 
>       -* ``static BUS_ATTR_RW(debug);``
>       -* ``static bus_attribute bus_attr_debug;``
>       +* ::
>       +
>       +	static BUS_ATTR_RW(something);
>       +
>       +* ::
>       +
>       +	static struct bus_attribute bus_attr_something = {
>       +		.attr  = { .name = "something", .mode = 0644 },
>       +		.show  = something_show,   // These functions must be
>       +		.store = something_store   // defined or declared already.
>       +	};
> 
>        This can then be used to add or remove the attribute from the bus's
>        sysfs directory using::

...please:

 - Describe your changes *concisely*, at a higher level.  Don't make
   maintainers try to figure out what you did from a patch-to-a-patch like
   this.

- Put this information after the "---" line at the end of the changelog.

> * The reStructuredText had some indentation issues (I only fixed the
>   areas I touched).

This is good, but why not complete the job while you're in the area?

> * The HTML output was not properly formatted in places.
> 
> * Some of the details were lacking or needed clarification (especially
>   with regard to how a `struct bus_type` object should be defined).

Each patch should do one thing; here you're mixing formatting cleanups with
actual text changes.  Those should be split ingo separate patches, please.

> * The sysfs example hierarchy appeared outdated; I've updated it with
>   output based on what my own system currently displays.
> 
> Signed-off-by: Michael Witten <mfwitten@gmail.com>
> ---
>  Documentation/driver-api/driver-model/bus.rst | 120 ++++++++++++------
>  1 file changed, 78 insertions(+), 42 deletions(-)
> 
> diff --git a/Documentation/driver-api/driver-model/bus.rst b/Documentation/driver-api/driver-model/bus.rst
> index 016b15a6e8ea..9a4cf15df8b9 100644
> --- a/Documentation/driver-api/driver-model/bus.rst
> +++ b/Documentation/driver-api/driver-model/bus.rst
> @@ -4,34 +4,61 @@ Bus Types
>  
>  Definition
>  ~~~~~~~~~~
> -See the kerneldoc for the struct bus_type.
>  
> -int bus_register(struct bus_type * bus);
> +::
> +
> +	struct bus_type;
> +	int bus_register(struct bus_type *bus);

This seems not entirely helpful, somehow.  If you're really just going to
list these, consider pulling in the relevant kerneldoc comments instead.

>  Declaration
>  ~~~~~~~~~~~
>  
> -Each bus type in the kernel (PCI, USB, etc) should declare one static
> -object of this type. They must initialize the name field, and may
> -optionally initialize the match callback::
> +For each bus type (PCI, USB, etc), there should be code that defines
> +one object of type struct bus_type:
> +
> +1. The definition should declare a file-scope identifier that has
> +   external linkage.
> +
> +   * There should be a header that provides a declaration of this
> +     identifier.
> +
> +   * The identifier should be explicitly exported.

I'm honestly not sure why this change was made or what you are trying to
say here.

> +2. The definition should initialize the ``name`` member. Other
> +   members may also be initialized (such as the ``match`` callback
> +   member).
>  
> -   struct bus_type pci_bus_type = {
> -          .name	= "pci",
> -          .match	= pci_bus_match,
> -   };
> +For instance, here is the definition for the PCI bus type::
>  
> -The structure should be exported to drivers in a header file:
> +	struct bus_type pci_bus_type = {
> +		.name          = "pci",               // REQUIRED
> +		.match         = pci_bus_match,
> +		.uevent        = pci_uevent,
> +		.probe         = pci_device_probe,
> +		.remove        = pci_device_remove,
> +		.shutdown      = pci_device_shutdown,
> +		.dev_groups    = pci_dev_groups,
> +		.bus_groups    = pci_bus_groups,
> +		.drv_groups    = pci_drv_groups,
> +		.pm            = PCI_PM_OPS_PTR,
> +		.num_vf        = pci_bus_num_vf,
> +		.dma_configure = pci_dma_configure,
> +	};
>  
> -extern struct bus_type pci_bus_type;
> +	EXPORT_SYMBOL(pci_bus_type);
> +
> +The relevant API header should include the following declaration::
> +
> +	extern struct bus_type pci_bus_type;
>  
>  
>  Registration
>  ~~~~~~~~~~~~
>  
> -When a bus driver is initialized, it calls bus_register. This
> -initializes the rest of the fields in the bus object and inserts it
> -into a global list of bus types. Once the bus object is registered,
> +During initialization of a bus driver, bus_register() is called; this

Why the switch to passive voice here?

> +initializes the rest of the fields in the bus type object and inserts it
> +into a global list of bus types. Once the bus type object is registered,
>  the fields in it are usable by the bus driver.
>  
>  
> @@ -53,30 +80,33 @@ particular device, without sacrificing bus-specific functionality or
>  type-safety.
>  
>  When a driver is registered with the bus, the bus's list of devices is
> -iterated over, and the match callback is called for each device that
> -does not have a driver associated with it.
> +iterated over, and the ``match`` callback is called for each device that
> +does not already have a driver associated with it.
>  
>  
>  
>  Device and Driver Lists
>  ~~~~~~~~~~~~~~~~~~~~~~~
>  
> -The lists of devices and drivers are intended to replace the local
> -lists that many buses keep. They are lists of struct devices and
> -struct device_drivers, respectively. Bus drivers are free to use the
> -lists as they please, but conversion to the bus-specific type may be
> -necessary.
> +There are generic facilities for keeping lists of devices and drivers:
> +
> +* There is a list of struct device objects.
> +* There is a list of struct device_driver objects.
> +
> +Bus drivers are free to use the lists as they please, but conversion
> +to a bus-specific type may be necessary.

I'm not convinced this change is really helpful either.

Finally, changes like this should also be copied to the people who maintain
the bus code; that would be Greg KH if nobody else.

Thanks,

jon

^ permalink raw reply	[flat|nested] 4+ messages in thread

end of thread, other threads:[~2021-01-18 20:23 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-12-21  7:52 [PATCH] docs: driver-model: bus.rst: Clean up the formatting, expound, modernize Michael Witten
2021-01-11 20:31 ` Jonathan Corbet
2021-01-13  5:35   ` [PATCH v2] " Michael Witten
2021-01-18 20:20     ` Jonathan Corbet

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).