| Message ID | 0bd9336e18abad338b4967664a4667b96dae6be6.1638630342.git.philipp.g.hortmann@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | Docs: usb: Code and text updates from usb-skeleton | expand |
On Sat, Dec 04, 2021 at 05:35:11PM +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 > Update format of function names > > Signed-off-by: Philipp Hortmann <philipp.g.hortmann@gmail.com> > --- > V1 -> V2: Added "Update format of function names" to patch description > Corrected format of function names like the following example: > "`usb_bulk_msg` function" to "usb_bulk_msg()" > --- > .../driver-api/usb/writing_usb_driver.rst | 32 +++++++++---------- > 1 file changed, 16 insertions(+), 16 deletions(-) > > diff --git a/Documentation/driver-api/usb/writing_usb_driver.rst b/Documentation/driver-api/usb/writing_usb_driver.rst > index b43e1ce49f0e..ed11398837e5 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(), 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` > -function, giving it a buffer into which to place any data received from > +handle urb completion callback functions. We call usb_bulk_msg(), > +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, Why are you changing the varible name? That seems unnecessary. > + 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; > } Leaving the variable name alone keeps this patch much smaller. > > > -The :c:func:`usb_bulk_msg` function can be very useful for doing single reads > +usb_bulk_msg() can be very useful for doing single reads You are doing different things in this patch, one is converting the function style and one is updating the text. How about doing just the function name stuff first, all in one patch, and then the updates, as that would make it much easier to read. Also, any reason you aren't cc:ing the USB maintainer on these changes? :) thanks, greg k-h
On 12/5/21 12:00 PM, Greg KH wrote: > On Sat, Dec 04, 2021 at 05:35:11PM +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 >> Update format of function names >> >> Signed-off-by: Philipp Hortmann <philipp.g.hortmann@gmail.com> >> --- >> V1 -> V2: Added "Update format of function names" to patch description >> Corrected format of function names like the following example: >> "`usb_bulk_msg` function" to "usb_bulk_msg()" >> --- >> .../driver-api/usb/writing_usb_driver.rst | 32 +++++++++---------- >> 1 file changed, 16 insertions(+), 16 deletions(-) >> >> diff --git a/Documentation/driver-api/usb/writing_usb_driver.rst b/Documentation/driver-api/usb/writing_usb_driver.rst >> index b43e1ce49f0e..ed11398837e5 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(), 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` >> -function, giving it a buffer into which to place any data received from >> +handle urb completion callback functions. We call usb_bulk_msg(), >> +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, > > Why are you changing the varible name? That seems unnecessary. Reason is that retval does not exist in skel_read(). > >> + 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; >> } > > Leaving the variable name alone keeps this patch much smaller. Will leave the variable name in the next patch. > >> >> >> -The :c:func:`usb_bulk_msg` function can be very useful for doing single reads >> +usb_bulk_msg() can be very useful for doing single reads > > You are doing different things in this patch, one is converting the > function style and one is updating the text. How about doing just the > function name stuff first, all in one patch, and then the updates, as > that would make it much easier to read. Will be changed. > > Also, any reason you aren't cc:ing the USB maintainer on these changes? :) According to: perl scripts/get_maintainer.pl --separator , --nokeywords --nogit --nogit-fallback --norolestats -f Documentation/driver-api/usb/writing_usb_driver.rst Jonathan Corbet <corbet@lwn.net>,linux-doc@vger.kernel.org,linux-kernel@vger.kernel.org you are not in charge. thanks, Philipp Hortmann > thanks, > > greg k-h >
On Sun, Dec 05, 2021 at 06:04:48PM +0100, Philipp Hortmann wrote: > On 12/5/21 12:00 PM, Greg KH wrote: > > On Sat, Dec 04, 2021 at 05:35:11PM +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 > > > Update format of function names > > > > > > Signed-off-by: Philipp Hortmann <philipp.g.hortmann@gmail.com> > > > --- > > > V1 -> V2: Added "Update format of function names" to patch description > > > Corrected format of function names like the following example: > > > "`usb_bulk_msg` function" to "usb_bulk_msg()" > > > --- > > > .../driver-api/usb/writing_usb_driver.rst | 32 +++++++++---------- > > > 1 file changed, 16 insertions(+), 16 deletions(-) > > > > > > diff --git a/Documentation/driver-api/usb/writing_usb_driver.rst b/Documentation/driver-api/usb/writing_usb_driver.rst > > > index b43e1ce49f0e..ed11398837e5 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(), 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` > > > -function, giving it a buffer into which to place any data received from > > > +handle urb completion callback functions. We call usb_bulk_msg(), > > > +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, > > > > Why are you changing the varible name? That seems unnecessary. > Reason is that retval does not exist in skel_read(). Neither does any call to usb_bulk_msg(). So this is not code that is from that file at all. "retval" is easier to understand than "rv". > > Also, any reason you aren't cc:ing the USB maintainer on these changes? :) > According to: > perl scripts/get_maintainer.pl --separator , --nokeywords --nogit > --nogit-fallback --norolestats -f > Documentation/driver-api/usb/writing_usb_driver.rst > Jonathan Corbet > <corbet@lwn.net>,linux-doc@vger.kernel.org,linux-kernel@vger.kernel.org > you are not in charge. Ah, documentation isn't added to the maintainers entry for USB, I'll go fix that up... But note, my name is at the top of that file still, right? thanks, greg k-h
diff --git a/Documentation/driver-api/usb/writing_usb_driver.rst b/Documentation/driver-api/usb/writing_usb_driver.rst index b43e1ce49f0e..ed11398837e5 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(), 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` -function, giving it a buffer into which to place any data received from +handle urb completion callback functions. We call usb_bulk_msg(), +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 +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
Clarification that this example is not in the driver template anymore. Update code example so that it fits best to usb-skeleton.c Update format of function names Signed-off-by: Philipp Hortmann <philipp.g.hortmann@gmail.com> --- V1 -> V2: Added "Update format of function names" to patch description Corrected format of function names like the following example: "`usb_bulk_msg` function" to "usb_bulk_msg()" --- .../driver-api/usb/writing_usb_driver.rst | 32 +++++++++---------- 1 file changed, 16 insertions(+), 16 deletions(-)