Date:   Mon, 18 Jan 2021 13:20:23 -0700
From:   Jonathan Corbet <corbet@....net>
To:     Michael Witten <mfwitten@...il.com>
Cc:     Mauro Carvalho Chehab <mchehab+samsung@...nel.org>,
        linux-doc@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2] docs: driver-model: bus.rst: Clean up the
 formatting, expound, modernize

On Wed, 13 Jan 2021 05:35:00 -0000
Michael Witten <mfwitten@...il.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@...il.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

Powered by blists - more mailing lists