Hi Johan,
A Patch:
[PATCH] USB:serial:pl2303:Add new PID to support PL2303HXN (TYPE_HXN)
B Patch:
[PATCH] [v2]USB:serial:pl2303:add new Pull-Up mode to
support PL2303HXD (TYPE_HX)
To save time, let you have more time to process A patch,
B patch's reply will wait until A patch passes,
then I will repeat the problem of B patch.
Charles.
Johan Hovold <johan@xxxxxxxxxx> 於 2019年4月2日 週二 下午3:34寫道：
>
> On Tue, Feb 12, 2019 at 08:50:49PM +0800, Charles Yeh wrote:
> > Pull-Up mode is disabled (default) in PL2303HXD.
> > When the Pull-Up mode is activated, its TX/DTR/RTS external resistor will start the output function.
> >
> > How to enable the Pull-Up mode of PL2303HXD
> > 1.TX/DTR/RTS external resistor is required on the circuit diagram (PCB)
> > 2.PL2303HXD OTP needs to be programmed to have a Pull-Up mode through 6.5V (USB_VCC,5V->6.5V)
> >
> > The patch driver will read whether the PL2303HXD has a Pull-Up mode,and if so,
> > it will be set to enable Pull-Up mode function.
> >
> > Signed-off-by: Charles Yeh <charlesyeh522@xxxxxxxxx>
> > ---
> > drivers/usb/serial/pl2303.c | 24 ++++++++++++++++++++++++
> > 1 file changed, 24 insertions(+)
> >
> > diff --git a/drivers/usb/serial/pl2303.c b/drivers/usb/serial/pl2303.c
> > index bb3f9aa4a909..e5d00e4a495d 100644
> > --- a/drivers/usb/serial/pl2303.c
> > +++ b/drivers/usb/serial/pl2303.c
> > @@ -145,6 +145,16 @@ MODULE_DEVICE_TABLE(usb, id_table);
> > #define UART_OVERRUN_ERROR 0x40
> > #define UART_CTS 0x80
> >
> > +#define TYPE_HX_READ_PUM_STATUS_REG 0x8484
> > +#define TYPE_HX_READ_PUM_ADD 0x0404
> > +#define TYPE_HX_READ_PUM_DATA_REG 0x8383
>
> Again, the defines need to reflect what the registers are for, not what
> use happen to use them for in on specific code path.
>
> These are used to access the EEPROM/OTP, right?
>
> > +#define TYPE_HX_PULLUP_MODE_DATA 0x08
>
> What is bit 0x08?
>
> > +#define TYPE_HX_PULLUP_MODE_REG 0x09
>
> And this register isn't just used for pull-up mode as far as I can tell.
>
> > +#define TYPE_HX_PUM_ADD0 0x00
> > +#define TYPE_HX_PUM_DATA0 0x31
> > +#define TYPE_HX_PUM_ADD1 0x01
> > +#define TYPE_HX_PUM_DATA1 0x08
>
> Same for these, we don't want multiple defines for register 0 for
> example. What are bits 0x31 of register 0?
>
> > +
> > static void pl2303_set_break(struct usb_serial_port *port, bool enable);
> >
> > enum pl2303_type {
> > @@ -688,6 +698,20 @@ static void pl2303_set_termios(struct tty_struct *tty,
> > pl2303_vendor_write(serial, 0x0, 0x0);
> > }
> >
> > + if (spriv->type == &pl2303_type_data[TYPE_HX]) {
> > + pl2303_vendor_read(serial, TYPE_HX_READ_PUM_STATUS_REG, buf);
> > + pl2303_vendor_write(serial, TYPE_HX_READ_PUM_ADD,
> > + TYPE_HX_PULLUP_MODE_REG);
> > + pl2303_vendor_read(serial, TYPE_HX_READ_PUM_STATUS_REG, buf);
> > + pl2303_vendor_read(serial, TYPE_HX_READ_PUM_DATA_REG, buf);
>
> Why do you need to access the OTP on every set_termios() call? I thought
> those settings where copied to the corresponding control registers
> during boot?
>
> > + if (*buf == TYPE_HX_PULLUP_MODE_DATA) {
>
> Don't you want to just check bit 0x80 here?
>
> > + pl2303_vendor_write(serial, TYPE_HX_PUM_ADD0,
> > + TYPE_HX_PUM_DATA0);
>
> So this looks broken since you're overwriting the flow control settings
> that were just set above. And again, what are bits 0x31 of register 0?
> Doesn't 0x30 control auto-rts?
>
> > + pl2303_vendor_write(serial, TYPE_HX_PUM_ADD1,
> > + TYPE_HX_PUM_DATA1);
>
> And why do you need to (over-)write register 2?
>
> Either way, configuring pull-up mode should be done at probe and not on
> every set_termios() call.
>
> > + }
> > + }
> > +
> > kfree(buf);
> > }
>
> Thanks,
> Johan
