| Message ID | 20240312055350.205878-1-alexhenrie24@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | [1/7] docs: driver-api: parport-lowlevel: clarify purpose of PARPORT_MODE_PCSPP | expand |
On Mon, Mar 11, 2024 at 11:50:27PM -0600, Alex Henrie wrote:
This one and at least one of the later ones are also missing commit
messages. Please fix in a v2.
> Signed-off-by: Alex Henrie <alexhenrie24@gmail.com>
Johan
That link is to a question that the submitter asked, and nobody responded to it. It seems that this patch stems from an incomplete reading of the kernel documentation. Those docs say: > SPP (Standard Parallel Port) functions modify so-called SPP registers: > data, status, and control. The hardware may not actually have > registers exactly like that, but the PC does and this interface is > modelled after common PC implementations. Other low-level drivers may > be able to emulate most of the functionality. So, the PARPORT_MODE_PCSPP flag denotes the availability of the SPP port functions, not any fields in a struct. On Tue, 2024-03-12 at 08:38 +0100, Johan Hovold wrote: > On Mon, Mar 11, 2024 at 11:50:26PM -0600, Alex Henrie wrote: > > Please write a proper commit message here explaining why this patch is > needed. You can keep the link if it's relevant, but you still need to > make the patch self-contained. > > > Link: http://lists.infradead.org/pipermail/linux-parport/2024-February/001237.html > > Signed-off-by: Alex Henrie <alexhenrie24@gmail.com> > > Johan
You didn't add the "P80453-B" label in this patch nor in PATCH 5/7… On Mon, 2024-03-11 at 23:50 -0600, Alex Henrie wrote: > This device is a gray USB parallel port adapter with "F5U002" imprinted > on the plastic and a sticker that says both "F5U002" and "P80453-A". > It's identified in lsusb as "05ab:1001 In-System Design BAYI Printer > Class Support". It's subtly different from the other gray cable I have > that has "F5U002 Rev 2" and "P80453-B" on its sticker and device ID > 050d:0002. > > The uss720 driver appears to work flawlessly with this device, although > the device evidently does not support ECP. > > Signed-off-by: Alex Henrie <alexhenrie24@gmail.com> > --- > drivers/usb/misc/uss720.c | 7 +++++-- > 1 file changed, 5 insertions(+), 2 deletions(-) > > diff --git a/drivers/usb/misc/uss720.c b/drivers/usb/misc/uss720.c > index 15cafc7dfd22..5803919d7cc4 100644 > --- a/drivers/usb/misc/uss720.c > +++ b/drivers/usb/misc/uss720.c > @@ -693,7 +693,7 @@ static int uss720_probe(struct usb_interface *intf, > > interface = intf->cur_altsetting; > > - if (interface->desc.bNumEndpoints < 3) { > + if (interface->desc.bNumEndpoints < 2) { > usb_put_dev(usbdev); > return -ENODEV; > } > @@ -719,7 +719,9 @@ static int uss720_probe(struct usb_interface *intf, > > priv->pp = pp; > pp->private_data = priv; > - pp->modes = PARPORT_MODE_TRISTATE | PARPORT_MODE_EPP | PARPORT_MODE_ECP | PARPORT_MODE_COMPAT; > + pp->modes = PARPORT_MODE_TRISTATE | PARPORT_MODE_EPP | PARPORT_MODE_COMPAT; > + if (interface->desc.bNumEndpoints >= 3) > + pp->modes |= PARPORT_MODE_ECP; > pp->dev = &usbdev->dev; > > /* set the USS720 control register to manual mode, no ECP compression, enable all ints */ > @@ -774,6 +776,7 @@ static const struct usb_device_id uss720_table[] = { > { USB_DEVICE(0x050d, 0x1202) }, /* Belkin F5U120-PC */ > { USB_DEVICE(0x0557, 0x2001) }, > { USB_DEVICE(0x05ab, 0x0002) }, /* Belkin F5U002 ISD-101 */ > + { USB_DEVICE(0x05ab, 0x1001) }, /* Belkin F5U002 P80453-A */ > { USB_DEVICE(0x06c6, 0x0100) }, /* Infowave ISD-103 */ > { USB_DEVICE(0x0729, 0x1284) }, > { USB_DEVICE(0x1293, 0x0002) },
On Tue, Mar 12, 2024 at 9:24 AM Daniel Gimpelevich <daniel@gimpelevich.san-francisco.ca.us> wrote: > > That link is to a question that the submitter asked, and nobody > responded to it. It seems that this patch stems from an incomplete > reading of the kernel documentation. Those docs say: > > > SPP (Standard Parallel Port) functions modify so-called SPP registers: > > data, status, and control. The hardware may not actually have > > registers exactly like that, but the PC does and this interface is > > modelled after common PC implementations. Other low-level drivers may > > be able to emulate most of the functionality. > > So, the PARPORT_MODE_PCSPP flag denotes the availability of the SPP port > functions, not any fields in a struct. Hello Daniel, thanks for your reply. I still don't quite understand what it would mean for a driver to lack PARPORT_MODE_PCSPP. Is the idea that a parallel port could support EPP or ECP without supporting SPP? If that's the case then in my opinion the documentation should still be rewritten, but in a way to clarify that the SPP functions [1] are not available without the flag, and the flag does not imply low latency. -Alex [1] Namely: read_data, write_data, read_status, read_control, write_control, frob_control, enable_irq, disable_irq, data_forward, and data_reverse. See https://docs.kernel.org/driver-api/parport-lowlevel.html
On Tue, Mar 12, 2024 at 1:39 AM Johan Hovold <johan@kernel.org> wrote: > > On Mon, Mar 11, 2024 at 11:50:29PM -0600, Alex Henrie wrote: > > This avoids a "fix this legacy no-device port driver" warning. > > Please be more specific. Hello Johan, thanks for taking a look at these patches. The warning comes from parport_announce_port in drivers/parport/share.c. include/linux/parport.h says that dev is the "Physical device associated with IO/DMA." Commit 4edb38695d9a ("parisc: parport0: fix this legacy no-device port driver!", 2013-05-30) fixed a similar issue and says only "Fix the above kernel error from parport_announce_port() on 32bit GSC machines (e.g. B160L). The parport driver requires now a pointer to the device struct." Do I just need to include "The parport driver now requires a pointer to the device struct" in the commit message? If not, where can I learn more about what the dev field is for, to be able to write a better description of why it's necessary to fill it in? Thanks, -Alex
On Tue, Mar 12, 2024 at 07:30:21PM -0600, Alex Henrie wrote: > On Tue, Mar 12, 2024 at 1:39 AM Johan Hovold <johan@kernel.org> wrote: > > > > On Mon, Mar 11, 2024 at 11:50:29PM -0600, Alex Henrie wrote: > > > This avoids a "fix this legacy no-device port driver" warning. > > > > Please be more specific. > > Hello Johan, thanks for taking a look at these patches. > > The warning comes from parport_announce_port in > drivers/parport/share.c. include/linux/parport.h says that dev is the > "Physical device associated with IO/DMA." Commit 4edb38695d9a > ("parisc: parport0: fix this legacy no-device port driver!", > 2013-05-30) fixed a similar issue and says only "Fix the above kernel > error from parport_announce_port() on 32bit GSC machines (e.g. B160L). > The parport driver requires now a pointer to the device struct." > > Do I just need to include "The parport driver now requires a pointer > to the device struct" in the commit message? If not, where can I learn > more about what the dev field is for, to be able to write a better > description of why it's necessary to fill it in? Your commit messages need to be self-contained and explain *why* you think your proposed change is needed, and in enough detail that a reviewer can make a judgement as to whether the patch is correct or not. Basically all my comments were just pointing this out. Johan
diff --git a/Documentation/driver-api/parport-lowlevel.rst b/Documentation/driver-api/parport-lowlevel.rst index 0633d70ffda7..aaf02cf62363 100644 --- a/Documentation/driver-api/parport-lowlevel.rst +++ b/Documentation/driver-api/parport-lowlevel.rst @@ -155,10 +155,11 @@ hardware. It consists of flags which may be bitwise-ored together: ============================= =============================================== PARPORT_MODE_PCSPP IBM PC registers are available, - i.e. functions that act on data, - control and status registers are - probably writing directly to the - hardware. + i.e. ``base`` (and ``base_hi`` if ECP) + in ``struct parport`` is valid. + Functions that act on data, control + and status registers are probably + writing directly to the hardware. PARPORT_MODE_TRISTATE The data drivers may be turned off. This allows the data lines to be used for reverse (peripheral to host)
Link: http://lists.infradead.org/pipermail/linux-parport/2024-February/001237.html Signed-off-by: Alex Henrie <alexhenrie24@gmail.com> --- Documentation/driver-api/parport-lowlevel.rst | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-)