Message-ID: <YJ0OCS/sh+1ifD/q@hovoldconsulting.com>
Date:   Thu, 13 May 2021 13:31:21 +0200
From:   Johan Hovold <johan@...nel.org>
To:     Frank Zago <frank@...o.net>
Cc:     Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        linux-kernel@...r.kernel.org,
        Jan-Niklas Burfeind <kernel@...ionpri.me>,
        Bartosz Golaszewski <bgolaszewski@...libre.com>,
        Wolfram Sang <wsa@...nel.org>, linux-usb@...r.kernel.org,
        gunar@...orcht.net
Subject: Re: [PATCH v2 1/2] Revert "USB: serial: ch341: add new Product ID
 for CH341A"

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@...o.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

Powered by blists - more mailing lists