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
