| Message ID | 20210423002852.3904-1-frank@zago.net |
|---|---|
| State | New |
| Headers | show |
| Series | [v2,1/2] Revert "USB: serial: ch341: add new Product ID for CH341A" | expand |
On Thu, Apr 22, 2021 at 07:28:51PM -0500, Frank Zago wrote: > From: frank zago <frank@zago.net> > > The 0x5512 USB PID is for the I2C/GPIO/SPI interfaces. UART is still > present but only the TX and RX pins are available; DTS, DTR, ... are > used for other things. Remove the PID, and let a I2C driver bind to > it. > > Existing CH341 boards usually have physical jumpers to switch between > the 3 modes. > > This reverts commit 46ee4abb10a07bd8f8ce910ee6b4ae6a947d7f63. You can't just revert something which people clearly depend on and therefore added to the kernel in the first place. Can you reprogram the device with a newly allocated PID to be used for i2c-only instead? Johan
On Thu, Apr 22, 2021 at 07:28:52PM -0500, Frank Zago wrote: > From: frank zago <frank@zago.net> > > The CH341 is a multifunction chip, presenting 3 different USB PID. One > of these functions is for I2C/SPI/GPIO. This new driver manages I2C > and GPIO. A future update will manage the SPI part as well. > > The I2C interface can run at 4 different speeds. This driver currently > only offer 100MHz. Tested with a variety of I2C sensors, and the IIO > subsystem. > > The GPIO interface offers 8 GPIOs. 6 are read/write, and 2 are > rea-only. However the SPI interface will use 6 of them, leaving 2 > available GPIOs. > > Signed-off-by: frank zago <frank@zago.net> > --- > > Changes from v1: > - Removed double Signed-off-by > - Move Kconfig into the same directory as the driver > > MAINTAINERS | 6 + > drivers/usb/misc/Kconfig | 2 + > drivers/usb/misc/Makefile | 1 + > drivers/usb/misc/ch341/Kconfig | 17 ++ > drivers/usb/misc/ch341/Makefile | 3 + > drivers/usb/misc/ch341/ch341-core.c | 87 +++++++++ > drivers/usb/misc/ch341/ch341-gpio.c | 249 ++++++++++++++++++++++++++ > drivers/usb/misc/ch341/ch341-i2c.c | 267 ++++++++++++++++++++++++++++ > drivers/usb/misc/ch341/ch341.h | 50 ++++++ > 9 files changed, 682 insertions(+) Not sure about sticking this in usb/misc. We already have an MFD driver (dln2) for something like this with i2c and spi iirc. The difference from regular MFDs here is that these modes are mutually exclusive. If it was just an i2c driver with some pins to toggle, I'd say just add it to drivers/i2c but that would probably make it harder to reuse code for the SPI driver. I'm not really reviewing the rest of the driver, just pointing out a few more things that stood out below. > +static void ch341_usb_free_device(struct ch341_device *dev) > +{ > + ch341_gpio_remove(dev); > + ch341_i2c_remove(dev); Have you verified that the i2c subsystem can handle a device being removed like this while in use. That wasn't the case a few years back. > + > + usb_set_intfdata(dev->iface, NULL); > + usb_put_dev(dev->usb_dev); > + > + kfree(dev); > +} > + > +static int ch341_usb_probe(struct usb_interface *iface, > + const struct usb_device_id *usb_id) > +{ > + struct ch341_device *dev; > + int error; > + > + dev = kzalloc(sizeof(struct ch341_device), GFP_KERNEL); > + if (!dev) > + return -ENOMEM; > + > + dev->usb_dev = usb_get_dev(interface_to_usbdev(iface)); > + dev->iface = iface; > + mutex_init(&dev->usb_lock); > + dev->ep_out = iface->cur_altsetting->endpoint[1].desc.bEndpointAddress; > + dev->ep_in = iface->cur_altsetting->endpoint[0].desc.bEndpointAddress; NULL-deref in case a device lacks the expected endpoints. > + > + usb_set_intfdata(iface, dev); > + > + error = ch341_i2c_init(dev); > + if (error) { > + ch341_usb_free_device(dev); > + return error; > + } > + > + error = ch341_gpio_init(dev); > + if (error) { > + ch341_usb_free_device(dev); > + return error; > + } > + > + return 0; > +} > + > +static void ch341_usb_disconnect(struct usb_interface *usb_if) > +{ > + struct ch341_device *dev = usb_get_intfdata(usb_if); > + > + ch341_usb_free_device(dev); > +} > +int ch341_gpio_init(struct ch341_device *dev) > +{ > + struct gpio_chip *gpio = &dev->gpio; > + int result; > + > + gpio->label = "ch341"; > + gpio->parent = &dev->usb_dev->dev; > + gpio->owner = THIS_MODULE; > + gpio->get_direction = ch341_gpio_get_direction; > + gpio->direction_input = ch341_gpio_direction_input; > + gpio->direction_output = ch341_gpio_direction_output; > + gpio->get = ch341_gpio_get; > + gpio->get_multiple = ch341_gpio_get_multiple; > + gpio->set = ch341_gpio_set; > + gpio->set_multiple = ch341_gpio_set_multiple; > + gpio->dbg_show = ch341_gpio_dbg_show; > + gpio->base = -1; > + gpio->ngpio = CH341_GPIO_NUM_PINS; > + gpio->names = gpio_names; Last time I checked (a few weeks ago) you could still not set names for hotpluggable devices of which there could be more than one or you'd get a warning when plugging a second device in. Not sure if Linus W fixed that yet. Johan
Hello, On 5/10/21 2:40 AM, Johan Hovold wrote: > On Thu, Apr 22, 2021 at 07:28:51PM -0500, Frank Zago wrote: >> From: frank zago <frank@zago.net> >> >> The 0x5512 USB PID is for the I2C/GPIO/SPI interfaces. UART is >> still present but only the TX and RX pins are available; DTS, DTR, >> ... are used for other things. Remove the PID, and let a I2C >> driver bind to it. >> >> Existing CH341 boards usually have physical jumpers to switch >> between the 3 modes. >> >> This reverts commit 46ee4abb10a07bd8f8ce910ee6b4ae6a947d7f63. > > You can't just revert something which people clearly depend on and > therefore added to the kernel in the first place. That device in UART mode was already supported by the serial driver. The original submitter just had to move a jumper on his board. There was no need to patch the kernel. That product ID also supports UART but in a limited way, as only the RX and TX pins are available. However it is the only one that supports i2c/spi/gpio, and that's why I have to revert the patch. If that's desired, the new driver could add support for that as well, but I don't think it's worth the effort. > > Can you reprogram the device with a newly allocated PID to be used > for i2c-only instead? It is possible if the device has an SPI flash connected to it, but none of the cheap boards have that. Frank.
On Tue, May 11, 2021 at 08:07:31PM -0500, Frank Zago wrote: > Hello, > > On 5/10/21 2:40 AM, Johan Hovold wrote: > > On Thu, Apr 22, 2021 at 07:28:51PM -0500, Frank Zago wrote: > >> From: frank zago <frank@zago.net> > >> > >> The 0x5512 USB PID is for the I2C/GPIO/SPI interfaces. UART is > >> still present but only the TX and RX pins are available; DTS, DTR, > >> ... are used for other things. Remove the PID, and let a I2C > >> driver bind to it. > >> > >> Existing CH341 boards usually have physical jumpers to switch > >> between the 3 modes. > >> > >> This reverts commit 46ee4abb10a07bd8f8ce910ee6b4ae6a947d7f63. > > > > You can't just revert something which people clearly depend on and > > therefore added to the kernel in the first place. > > That device in UART mode was already supported by the serial driver. The > original submitter just had to move a jumper on his board. There was no > need to patch the kernel. How do you know that the author used a dev board? And are you really sure that there are no devices out there which always operate in this mode? > That product ID also supports UART but in a limited way, as only the RX and TX > pins are available. However it is the only one that supports i2c/spi/gpio, and > that's why I have to revert the patch. I understand why you did it. My point is that you cannot just claim that PID and say that it's only to be used for I2C/SPI without even trying to make a case for why that should be ok. > If that's desired, the new driver could add support for that as well, but I don't > think it's worth the effort. We obviously don't want a second serial driver for these devices. > > Can you reprogram the device with a newly allocated PID to be used > > for i2c-only instead? > > It is possible if the device has an SPI flash connected to it, but none of > the cheap boards have that. That's unfortunate. In principle, your approach is the right one, that is, to use a dedicated PID do determine when to configure an alternate mode. But since we already know that some people are using the PID in question in serial mode, it's not that clear cut. How do you intend to switch between i2c and spi mode? Johan
On 5/12/21 4:55 AM, Johan Hovold wrote: > On Tue, May 11, 2021 at 08:07:31PM -0500, Frank Zago wrote: >> Hello, >> >> On 5/10/21 2:40 AM, Johan Hovold wrote: >>> On Thu, Apr 22, 2021 at 07:28:51PM -0500, Frank Zago wrote: >>>> From: frank zago <frank@zago.net> >>>> >>>> The 0x5512 USB PID is for the I2C/GPIO/SPI interfaces. UART is >>>> still present but only the TX and RX pins are available; DTS, >>>> DTR, ... are used for other things. Remove the PID, and let a >>>> I2C driver bind to it. >>>> >>>> Existing CH341 boards usually have physical jumpers to switch >>>> between the 3 modes. >>>> >>>> This reverts commit 46ee4abb10a07bd8f8ce910ee6b4ae6a947d7f63. >>> >>> You can't just revert something which people clearly depend on >>> and therefore added to the kernel in the first place. >> >> That device in UART mode was already supported by the serial >> driver. The original submitter just had to move a jumper on his >> board. There was no need to patch the kernel. > > How do you know that the author used a dev board? And are you really > sure that there are no devices out there which always operate in > this mode? The author of commit 46ee4abb10a07bd8 put a link to his device. I have the same one (or a clone) and it works fine in serial mode without the patch. I have a different model that works the same way. A jumper selects the mode. I can't be sure that no one has ever built a board with that chip, selecting the wrong mode. But the chip is about 10 years old now; someone would have noticed. > >> That product ID also supports UART but in a limited way, as only >> the RX and TX pins are available. However it is the only one that >> supports i2c/spi/gpio, and that's why I have to revert the patch. > > I understand why you did it. My point is that you cannot just claim > that PID and say that it's only to be used for I2C/SPI without even > trying to make a case for why that should be ok. That's the only PID that works for I2C/SPI/GPIO. Right now the serial driver is claiming it. I don't know what else to say. If I can't revert that patch, my driver can't be used without blacklisting the serial driver. > >> If that's desired, the new driver could add support for that as >> well, but I don't think it's worth the effort. > > We obviously don't want a second serial driver for these devices. > >>> Can you reprogram the device with a newly allocated PID to be >>> used for i2c-only instead? >> >> It is possible if the device has an SPI flash connected to it, but >> none of the cheap boards have that. > > That's unfortunate. In principle, your approach is the right one, > that is, to use a dedicated PID do determine when to configure an > alternate mode. But since we already know that some people are using > the PID in question in serial mode, it's not that clear cut. > > How do you intend to switch between i2c and spi mode? i2c, spi and gpio can all be used simultaneously. I have a working spi implementation, but I'm still testing it. Basically if a user wants to use spi, then 3 specific gpios will be reserved for MOSI/MISO/CLK (using gpiochip_request_own_desc), with possibly one or more used for the chip select. How a user books spi is up in the air right now. That might be done through a sysfs command. Frank.
On Wed, May 12, 2021 at 08:00:41PM -0500, Frank Zago wrote: > On 5/12/21 4:55 AM, Johan Hovold wrote: > > On Tue, May 11, 2021 at 08:07:31PM -0500, Frank Zago wrote: > >> Hello, > >> > >> On 5/10/21 2:40 AM, Johan Hovold wrote: > >>> On Thu, Apr 22, 2021 at 07:28:51PM -0500, Frank Zago wrote: > >>>> From: frank zago <frank@zago.net> > >>>> > >>>> The 0x5512 USB PID is for the I2C/GPIO/SPI interfaces. UART is > >>>> still present but only the TX and RX pins are available; DTS, > >>>> DTR, ... are used for other things. Remove the PID, and let a > >>>> I2C driver bind to it. > >>>> > >>>> Existing CH341 boards usually have physical jumpers to switch > >>>> between the 3 modes. > >>>> > >>>> This reverts commit 46ee4abb10a07bd8f8ce910ee6b4ae6a947d7f63. > >>> > >>> You can't just revert something which people clearly depend on > >>> and therefore added to the kernel in the first place. > >> > >> That device in UART mode was already supported by the serial > >> driver. The original submitter just had to move a jumper on his > >> board. There was no need to patch the kernel. > > > > How do you know that the author used a dev board? And are you really > > sure that there are no devices out there which always operate in > > this mode? > > The author of commit 46ee4abb10a07bd8 put a link to his device. I have > the same one (or a clone) and it works fine in serial mode without the > patch. I have a different model that works the same way. A jumper > selects the mode. > > I can't be sure that no one has ever built a board with that chip, > selecting the wrong mode. But the chip is about 10 years old now; > someone would have noticed. Since commit 46ee4abb10a0 ("USB: serial: ch341: add new Product ID for CH341A") went in fairly recently it may still be possible to revert it, especially if the author can speak up and explain why he added it. If I'm reading the datasheets correctly (which I should not have to, this should all be explained in the commit message), serial TX/RX isn't even available in the mode we're discussing here as those pins are used for parallel mode ERR#/PEMP: http://www.anok.ceti.pl/download/ch341ds1.pdf Someone might however be using the parallel interface with the current serial driver but that doesn't seem to be the case with the programmer in question based on some other random site which has some schematics (possibly a different device): https://www.onetransistor.eu/2017/08/ch341a-mini-programmer-schematic.html > >> That product ID also supports UART but in a limited way, as only > >> the RX and TX pins are available. However it is the only one that > >> supports i2c/spi/gpio, and that's why I have to revert the patch. > > > > I understand why you did it. My point is that you cannot just claim > > that PID and say that it's only to be used for I2C/SPI without even > > trying to make a case for why that should be ok. > > That's the only PID that works for I2C/SPI/GPIO. Right now the serial > driver is claiming it. I don't know what else to say. If I can't > revert that patch, my driver can't be used without blacklisting the > serial driver. You still need to make the case for why this is OK. Explain the various modes, how they are configured (e.g. the various bootstrap pins), and argue why claiming this PID might be acceptable. Getting the author who added the PID to say he's ok with reverting would also help. Just repeating "I need this" isn't good enough. > >>> Can you reprogram the device with a newly allocated PID to be > >>> used for i2c-only instead? > >> > >> It is possible if the device has an SPI flash connected to it, but > >> none of the cheap boards have that. > > > > That's unfortunate. In principle, your approach is the right one, > > that is, to use a dedicated PID do determine when to configure an > > alternate mode. But since we already know that some people are using > > the PID in question in serial mode, it's not that clear cut. > > > > How do you intend to switch between i2c and spi mode? > > i2c, spi and gpio can all be used simultaneously. I have a working spi > implementation, but I'm still testing it. Basically if a user wants > to use spi, then 3 specific gpios will be reserved for MOSI/MISO/CLK > (using gpiochip_request_own_desc), with possibly one or more used > for the chip select. Ok, thanks for clarifying. Sounds like this should be an MFD driver then even if the SPI and GPIO blocks aren't independent. > How a user books spi is up in the air right now. That might be done > through a sysfs command. Would be interesting to see a proposal since we don't currently have a good interface for handling this kind of runtime configuration. Johan
diff --git a/drivers/usb/serial/ch341.c b/drivers/usb/serial/ch341.c index 2db917eab799..235adc77ee0e 100644 --- a/drivers/usb/serial/ch341.c +++ b/drivers/usb/serial/ch341.c @@ -81,7 +81,6 @@ #define CH341_QUIRK_SIMULATE_BREAK BIT(1) static const struct usb_device_id id_table[] = { - { USB_DEVICE(0x1a86, 0x5512) }, { USB_DEVICE(0x1a86, 0x5523) }, { USB_DEVICE(0x1a86, 0x7522) }, { USB_DEVICE(0x1a86, 0x7523) },