[1/4] Docs: usb: update usb_bulk_msg receiving example

Commit Message

Philipp Hortmann Nov. 29, 2021, 8:21 p.m. UTC

Clarification that this example is not in the driver template anymore.
Update code example so that it fits best to usb-skeleton.c

Signed-off-by: Philipp Hortmann <[e-mail]>
---
 .../driver-api/usb/writing_usb_driver.rst     | 30 +++++++++----------
 1 file changed, 15 insertions(+), 15 deletions(-)

Comments

Alan Stern Nov. 30, 2021, 8:10 p.m. UTC | #1

On Mon, Nov 29, 2021 at 09:21:41PM +0100, Philipp Hortmann wrote:
> Clarification that this example is not in the driver template anymore.
> Update code example so that it fits best to usb-skeleton.c
> 
> Signed-off-by: Philipp Hortmann <[e-mail]>
> ---
>  .../driver-api/usb/writing_usb_driver.rst     | 30 +++++++++----------
>  1 file changed, 15 insertions(+), 15 deletions(-)
> 
> diff --git a/Documentation/driver-api/usb/writing_usb_driver.rst b/Documentation/driver-api/usb/writing_usb_driver.rst
> index b43e1ce49f0e..a9608ad18d77 100644
> --- a/Documentation/driver-api/usb/writing_usb_driver.rst
> +++ b/Documentation/driver-api/usb/writing_usb_driver.rst
> @@ -218,36 +218,36 @@ do very much processing at that time. Our implementation of
>  ``skel_write_bulk_callback`` merely reports if the urb was completed
>  successfully or not and then returns.
>  
> -The read function works a bit differently from the write function in
> +This read function works a bit differently from the write function in
>  that we do not use an urb to transfer data from the device to the
> -driver. Instead we call the :c:func:`usb_bulk_msg` function, which can be used
> +driver. Instead we call `usb_bulk_msg` function, which can be used
>  to send or receive data from a device without having to create urbs and
> -handle urb completion callback functions. We call the :c:func:`usb_bulk_msg`
> +handle urb completion callback functions. We call `usb_bulk_msg`
>  function, giving it a buffer into which to place any data received from

The reason for the last two changes above isn't clear.  You removed some of the 
markup indicators and made the text ungrammatical.  This does not seem like an 
improvement.

Alan Stern

>  the device and a timeout value. If the timeout period expires without
>  receiving any data from the device, the function will fail and return an
>  error message. This can be shown with the following code::
>  
>      /* do an immediate bulk read to get data from the device */
> -    retval = usb_bulk_msg (skel->dev,
> -			   usb_rcvbulkpipe (skel->dev,
> -			   skel->bulk_in_endpointAddr),
> -			   skel->bulk_in_buffer,
> -			   skel->bulk_in_size,
> -			   &count, 5000);
> +    rv = usb_bulk_msg(dev->udev,
> +		      usb_rcvbulkpipe (dev->udev,
> +		      dev->bulk_in_endpointAddr),
> +		      dev->bulk_in_buffer,
> +	              dev->bulk_in_size,
> +		      &len, 5000);
>      /* if the read was successful, copy the data to user space */
> -    if (!retval) {
> -	    if (copy_to_user (buffer, skel->bulk_in_buffer, count))
> -		    retval = -EFAULT;
> +    if (!rv) {
> +	    if (copy_to_user (buffer, dev->bulk_in_buffer, len))
> +		    rv = -EFAULT;
>  	    else
> -		    retval = count;
> +		    rv = len;
>      }
>  
>  
> -The :c:func:`usb_bulk_msg` function can be very useful for doing single reads
> +Function `usb_bulk_msg` can be very useful for doing single reads
>  or writes to a device; however, if you need to read or write constantly to
>  a device, it is recommended to set up your own urbs and submit them to
> -the USB subsystem.
> +the USB subsystem. The template uses urbs for read and write.
>  
>  When the user program releases the file handle that it has been using to
>  talk to the device, the release function in the driver is called. In
> -- 
> 2.25.1
>

Philipp Hortmann Dec. 2, 2021, 4:49 a.m. UTC | #2

On 11/30/21 9:10 PM, Alan Stern wrote:
> On Mon, Nov 29, 2021 at 09:21:41PM +0100, Philipp Hortmann wrote:
>> Clarification that this example is not in the driver template anymore.
>> Update code example so that it fits best to usb-skeleton.c
>>
>> Signed-off-by: Philipp Hortmann <[e-mail]>
>> ---
>>   .../driver-api/usb/writing_usb_driver.rst     | 30 +++++++++----------
>>   1 file changed, 15 insertions(+), 15 deletions(-)
>>
>> diff --git a/Documentation/driver-api/usb/writing_usb_driver.rst b/Documentation/driver-api/usb/writing_usb_driver.rst
>> index b43e1ce49f0e..a9608ad18d77 100644
>> --- a/Documentation/driver-api/usb/writing_usb_driver.rst
>> +++ b/Documentation/driver-api/usb/writing_usb_driver.rst
>> @@ -218,36 +218,36 @@ do very much processing at that time. Our implementation of
>>   ``skel_write_bulk_callback`` merely reports if the urb was completed
>>   successfully or not and then returns.
>>   
>> -The read function works a bit differently from the write function in
>> +This read function works a bit differently from the write function in
>>   that we do not use an urb to transfer data from the device to the
>> -driver. Instead we call the :c:func:`usb_bulk_msg` function, which can be used
>> +driver. Instead we call `usb_bulk_msg` function, which can be used
>>   to send or receive data from a device without having to create urbs and
>> -handle urb completion callback functions. We call the :c:func:`usb_bulk_msg`
>> +handle urb completion callback functions. We call `usb_bulk_msg`
>>   function, giving it a buffer into which to place any data received from
> 
> The reason for the last two changes above isn't clear.  You removed some of the
> markup indicators and made the text ungrammatical.  This does not seem like an
> improvement.
> 
> Alan Stern
>
This two changes were made because of an earlier comment to the same 
document, but may be I understood this wrong:
On 10/19/21 11:17 PM, Jonathan Corbet wrote:
...
We shouldn't be using :c:func: anymore; just say usb_register() and the
right things will happen.  Definitely worth fixing while you are in the
neighborhood.
...
If you're making this change, take out "the" (as well as :c:func:).
...

Alan Stern Dec. 2, 2021, 7:49 p.m. UTC | #3

On Thu, Dec 02, 2021 at 05:49:47AM +0100, Philipp Hortmann wrote:
> On 11/30/21 9:10 PM, Alan Stern wrote:
> > On Mon, Nov 29, 2021 at 09:21:41PM +0100, Philipp Hortmann wrote:
> > > Clarification that this example is not in the driver template anymore.
> > > Update code example so that it fits best to usb-skeleton.c
> > > 
> > > Signed-off-by: Philipp Hortmann <[e-mail]>
> > > ---
> > >   .../driver-api/usb/writing_usb_driver.rst     | 30 +++++++++----------
> > >   1 file changed, 15 insertions(+), 15 deletions(-)
> > > 
> > > diff --git a/Documentation/driver-api/usb/writing_usb_driver.rst b/Documentation/driver-api/usb/writing_usb_driver.rst
> > > index b43e1ce49f0e..a9608ad18d77 100644
> > > --- a/Documentation/driver-api/usb/writing_usb_driver.rst
> > > +++ b/Documentation/driver-api/usb/writing_usb_driver.rst
> > > @@ -218,36 +218,36 @@ do very much processing at that time. Our implementation of
> > >   ``skel_write_bulk_callback`` merely reports if the urb was completed
> > >   successfully or not and then returns.
> > > -The read function works a bit differently from the write function in
> > > +This read function works a bit differently from the write function in
> > >   that we do not use an urb to transfer data from the device to the
> > > -driver. Instead we call the :c:func:`usb_bulk_msg` function, which can be used
> > > +driver. Instead we call `usb_bulk_msg` function, which can be used
> > >   to send or receive data from a device without having to create urbs and
> > > -handle urb completion callback functions. We call the :c:func:`usb_bulk_msg`
> > > +handle urb completion callback functions. We call `usb_bulk_msg`
> > >   function, giving it a buffer into which to place any data received from
> > 
> > The reason for the last two changes above isn't clear.  You removed some of the
> > markup indicators and made the text ungrammatical.  This does not seem like an
> > improvement.
> > 
> > Alan Stern
> > 
> This two changes were made because of an earlier comment to the same
> document, but may be I understood this wrong:
> On 10/19/21 11:17 PM, Jonathan Corbet wrote:
> ...
> We shouldn't be using :c:func: anymore; just say usb_register() and the
> right things will happen.  Definitely worth fixing while you are in the
> neighborhood.
> ...
> If you're making this change, take out "the" (as well as :c:func:).
> ...
> ___
> Please find the full email under the link:
> https://lore.kernel.org/linux-usb/[e-mail]/T/
> 
> Please give me an example for the right wording. I am not a native English
> speaker. Is the article in this case required?

Okay, now I see what's going on.  You should change it like this:

-driver. Instead we call the :c:func:`usb_bulk_msg` function, which can be used
+driver. Instead we call `usb_bulk_msg`, which can be used
 to send or receive data from a device without having to create urbs and
-handle urb completion callback functions. We call the :c:func:`usb_bulk_msg`
+handle urb completion callback functions. We call `usb_bulk_msg`,
 giving it a buffer into which to place any data received from

Alan Stern

Akira Yokosawa Dec. 3, 2021, 2:01 a.m. UTC | #4

Hi,

On Thu, 2 Dec 2021 14:49:08 -0500, Alan Stern wrote:
> On Thu, Dec 02, 2021 at 05:49:47AM +0100, Philipp Hortmann wrote:
[...]
>> Please find the full email under the link:
>> https://lore.kernel.org/linux-usb/[e-mail]/T/
>> 
>> Please give me an example for the right wording. I am not a native English
>> speaker. Is the article in this case required?
> 
> Okay, now I see what's going on.  You should change it like this:
> 
> -driver. Instead we call the :c:func:`usb_bulk_msg` function, which can be used
> +driver. Instead we call `usb_bulk_msg`, which can be used
>  to send or receive data from a device without having to create urbs and
> -handle urb completion callback functions. We call the :c:func:`usb_bulk_msg`
> +handle urb completion callback functions. We call `usb_bulk_msg`,
>  giving it a buffer into which to place any data received from

Well, for function names to be caught by kernel-doc tools,
you need to say usb_bulk_msg() (without ``, with () appended).

So, the diff should look:

-driver. Instead we call the :c:func:`usb_bulk_msg` function, which can be used
+driver. Instead we call usb_bulk_msg(), which can be used
 to send or receive data from a device without having to create urbs and
-handle urb completion callback functions. We call the :c:func:`usb_bulk_msg`
+handle urb completion callback functions. We call usb_bulk_msg(),
 giving it a buffer into which to place any data received from

You can find related documentation at:

   https://www.kernel.org/doc/html/latest/doc-guide/kernel-doc.html#cross-referencing-from-restructuredtext

        Thanks, Akira

> 
> Alan Stern

Patch

diff --git a/Documentation/driver-api/usb/writing_usb_driver.rst b/Documentation/driver-api/usb/writing_usb_driver.rst
index b43e1ce49f0e..a9608ad18d77 100644
--- a/Documentation/driver-api/usb/writing_usb_driver.rst
+++ b/Documentation/driver-api/usb/writing_usb_driver.rst
@@ -218,36 +218,36 @@  do very much processing at that time. Our implementation of
 ``skel_write_bulk_callback`` merely reports if the urb was completed
 successfully or not and then returns.
 
-The read function works a bit differently from the write function in
+This read function works a bit differently from the write function in
 that we do not use an urb to transfer data from the device to the
-driver. Instead we call the :c:func:`usb_bulk_msg` function, which can be used
+driver. Instead we call `usb_bulk_msg` function, which can be used
 to send or receive data from a device without having to create urbs and
-handle urb completion callback functions. We call the :c:func:`usb_bulk_msg`
+handle urb completion callback functions. We call `usb_bulk_msg`
 function, giving it a buffer into which to place any data received from
 the device and a timeout value. If the timeout period expires without
 receiving any data from the device, the function will fail and return an
 error message. This can be shown with the following code::
 
     /* do an immediate bulk read to get data from the device */
-    retval = usb_bulk_msg (skel->dev,
-			   usb_rcvbulkpipe (skel->dev,
-			   skel->bulk_in_endpointAddr),
-			   skel->bulk_in_buffer,
-			   skel->bulk_in_size,
-			   &count, 5000);
+    rv = usb_bulk_msg(dev->udev,
+		      usb_rcvbulkpipe (dev->udev,
+		      dev->bulk_in_endpointAddr),
+		      dev->bulk_in_buffer,
+	              dev->bulk_in_size,
+		      &len, 5000);
     /* if the read was successful, copy the data to user space */
-    if (!retval) {
-	    if (copy_to_user (buffer, skel->bulk_in_buffer, count))
-		    retval = -EFAULT;
+    if (!rv) {
+	    if (copy_to_user (buffer, dev->bulk_in_buffer, len))
+		    rv = -EFAULT;
 	    else
-		    retval = count;
+		    rv = len;
     }
 
 
-The :c:func:`usb_bulk_msg` function can be very useful for doing single reads
+Function `usb_bulk_msg` can be very useful for doing single reads
 or writes to a device; however, if you need to read or write constantly to
 a device, it is recommended to set up your own urbs and submit them to
-the USB subsystem.
+the USB subsystem. The template uses urbs for read and write.
 
 When the user program releases the file handle that it has been using to
 talk to the device, the release function in the driver is called. In