USB:Serial:pl2303:Add new Pull-UP Mode to support PL2303HXD(TYPE_HX)

USB:Serial:pl2303:Add new Pull-UP Mode to support PL2303HXD(TYPE_HX)

[Johan Hovold]

On Tue, Jan 15, 2019 at 11:15:36PM +0800, Charles Yeh wrote:
> The Pull-UP mode only support PL2303HXD,it needs to use addition
> schematic design.

I need a better explain of why this is needed here.

Your are also enabling this for all current devices, which seems to just
fine without this.

> Signed-off-by: Charles Yeh <charlesyeh522@gmail.com>
> ---
>  drivers/usb/serial/pl2303.c | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
> 
> diff --git a/drivers/usb/serial/pl2303.c b/drivers/usb/serial/pl2303.c
> index 98e7a5df0f6d..72544e5c928d 100644
> --- a/drivers/usb/serial/pl2303.c
> +++ b/drivers/usb/serial/pl2303.c
> @@ -144,6 +144,9 @@ MODULE_DEVICE_TABLE(usb, id_table);
>  #define UART_OVERRUN_ERROR		0x40
>  #define UART_CTS			0x80
>  
> +#define TYPE_HX_PULLUP_MODE_DATA	0x08
> +#define TYPE_HX_PULLUP_MODE_REG		0x09
> +
>  static void pl2303_set_break(struct usb_serial_port *port, bool enable);
>  
>  enum pl2303_type {
> @@ -687,6 +690,15 @@ static void pl2303_set_termios(struct tty_struct *tty,
>  		pl2303_vendor_write(serial, 0x0, 0x0);
>  	}
>  
> +	pl2303_vendor_read(serial, 0x8484, buf);
> +	pl2303_vendor_write(serial, 0x0404, TYPE_HX_PULLUP_MODE_REG);
> +	pl2303_vendor_read(serial, 0x8484, buf);
> +	pl2303_vendor_read(serial, 0x8383, buf);
> +	if (*buf == TYPE_HX_PULLUP_MODE_DATA) {
> +		pl2303_vendor_write(serial, 0x0, 0x31);
> +		pl2303_vendor_write(serial, 0x1, 0x01);
> +	}

There are more magic constants in the above, which needs a define.

> +
>  	kfree(buf);
>  }

Thanks,
Johan

USB:Serial:pl2303:Add new Pull-UP Mode to support PL2303HXD(TYPE_HX)

[Johan Hovold]

On Thu, Jan 17, 2019 at 06:19:56AM +0000, Yeh.Charles [葉榮鑫] wrote:
> Hi Johan,
>   Please refer to https://electronics.stackexchange.com/questions/28091/push-pull-open-drain-pull-up-pull-down

Thanks, I know that bit.

I still need a proper commit message describing why the change is
needed. You provide some more information below, but it's still not
enough for a proper commit message it seems.

What pins are you changing the properties off here? The UART pins or
some GPIOs. Please explain in more details what this is used for.

And the magic constants needs to go away in favour of defines.

>   " Your are also enabling this for all current devices " <<--No, it
>   needs to use addition schematic design.

I meant that you are enabling this new code for all device types
unconditionally yet you say it's only needed for HXD.

>     It needs special PCB and special PL2303HXD, and then enable Pull-up mode..
> 
>    " special PL2303HXD ": Prolific will release a custom-made
>    PL2303HXD OTP( 6.5V ) writer to customer. The OTP writer will
>    record a "Data:0x08" at "Address: 0x09".

Ok, that's another piece to the pussle, thanks. Remember to include it
in the commit message too.

> pl2303_startup function:
> pl2303_vendor_read(serial, 0x8484, buf);
> pl2303_vendor_write(serial, 0x0404, 0);
> pl2303_vendor_read(serial, 0x8484, buf);
> pl2303_vendor_read(serial, 0x8383, buf);
> 
> pl2303_vendor_read(serial, 0x8484, buf);
> pl2303_vendor_write(serial, 0x0404, 1);
> pl2303_vendor_read(serial, 0x8484, buf);
> pl2303_vendor_read(serial, 0x8383, buf);
> 
> 
> pl2303_set_termios function:
> pl2303_vendor_read(serial, 0x8484, buf);
> pl2303_vendor_write(serial, 0x0404, TYPE_HX_PULLUP_MODE_REG);
> pl2303_vendor_read(serial, 0x8484, buf);
> pl2303_vendor_read(serial, 0x8383, buf);
> 
> You can see the same code in "pl2303_startup" function, it can be
> found from v.2.4.31 and above Linux kernel.

It's not really the same code is it? First of all it's only run at
probe, not at every set_termios call; why did you add it there?

Second, the current code appears to read from address 0 and 1, not
address 9.

> So you want me to also change pl2303_startup function ?

If you could clean up that code with proper defines and a comment about
what is going on, that would be great.

Thanks,
Johan

USB:Serial:pl2303:Add new Pull-UP Mode to support PL2303HXD(TYPE_HX)

[Yeh.Charles [葉榮鑫]]

Hi Johan,
  Please refer to https://electronics.stackexchange.com/questions/28091/push-pull-open-drain-pull-up-pull-down

  " Your are also enabling this for all current devices " <<--No, it needs to use addition schematic design.

    It needs special PCB and special PL2303HXD, and then enable Pull-up mode..

   " special PL2303HXD ": Prolific will release a custom-made PL2303HXD OTP( 6.5V ) writer to customer. The OTP writer will record a "Data:0x08" at "Address: 0x09".

pl2303_startup function:
pl2303_vendor_read(serial, 0x8484, buf);
pl2303_vendor_write(serial, 0x0404, 0);
pl2303_vendor_read(serial, 0x8484, buf);
pl2303_vendor_read(serial, 0x8383, buf);

pl2303_vendor_read(serial, 0x8484, buf);
pl2303_vendor_write(serial, 0x0404, 1);
pl2303_vendor_read(serial, 0x8484, buf);
pl2303_vendor_read(serial, 0x8383, buf);


pl2303_set_termios function:
pl2303_vendor_read(serial, 0x8484, buf);
pl2303_vendor_write(serial, 0x0404, TYPE_HX_PULLUP_MODE_REG);
pl2303_vendor_read(serial, 0x8484, buf);
pl2303_vendor_read(serial, 0x8383, buf);

You can see the same code in "pl2303_startup" function, it can be found from v.2.4.31 and above Linux kernel.
So you want me to also change pl2303_startup function ?


Charles Yeh(葉榮鑫)
TeL: +886-2-26546363 ext.522
Prolific Technology Inc. (旺玖科技股份有限公司)


-----Original Message-----
From: Johan Hovold [mailto:johan@kernel.org]
Sent: Wednesday, January 16, 2019 7:19 PM
To: Charles Yeh
Cc: gregkh@linuxfoundation.org; johan@kernel.org; linux-usb@vger.kernel.org; Yeh.Charles [葉榮鑫]
Subject: Re: [PATCH] USB:Serial:pl2303:Add new Pull-UP Mode to support PL2303HXD(TYPE_HX)

On Tue, Jan 15, 2019 at 11:15:36PM +0800, Charles Yeh wrote:
> The Pull-UP mode only support PL2303HXD,it needs to use addition
> schematic design

I need a better explain of why this is needed here.

Your are also enabling this for all current devices, which seems to just
fine without this.

> Signed-off-by: Charles Yeh <charlesyeh522@gmail.com>
> ---
>  drivers/usb/serial/pl2303.c | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
>
> diff --git a/drivers/usb/serial/pl2303.c b/drivers/usb/serial/pl2303.c
> index 98e7a5df0f6d..72544e5c928d 100644
> --- a/drivers/usb/serial/pl2303.c
> +++ b/drivers/usb/serial/pl2303.c
> @@ -144,6 +144,9 @@ MODULE_DEVICE_TABLE(usb, id_table);
>  #define UART_OVERRUN_ERROR0x40
>  #define UART_CTS0x80
>
> +#define TYPE_HX_PULLUP_MODE_DATA0x08
> +#define TYPE_HX_PULLUP_MODE_REG0x09
> +
>  static void pl2303_set_break(struct usb_serial_port *port, bool enable);
>
>  enum pl2303_type {
> @@ -687,6 +690,15 @@ static void pl2303_set_termios(struct tty_struct *tty,
>  pl2303_vendor_write(serial, 0x0, 0x0);
>  }
>
> +pl2303_vendor_read(serial, 0x8484, buf);
> +pl2303_vendor_write(serial, 0x0404, TYPE_HX_PULLUP_MODE_REG);
> +pl2303_vendor_read(serial, 0x8484, buf);
> +pl2303_vendor_read(serial, 0x8383, buf);
> +if (*buf == TYPE_HX_PULLUP_MODE_DATA) {
> +pl2303_vendor_write(serial, 0x0, 0x31);
> +pl2303_vendor_write(serial, 0x1, 0x01);
> +}

There are more magic constants in the above, which needs a define.

> +
>  kfree(buf);
>  }

Thanks,
Johan
保密警語: 本電子郵件內容及其附加檔案均視為機密資料，受保密合約保護或依法不得洩漏。其內容僅供指定收件人按限定範圍或特殊目的合法使用，未經授權者收到此信息均無權閱讀、使用、複製、洩漏或散佈。若您並非本郵件之指定收件人，請即刻回覆郵件並永久刪除此郵件及其附件和銷毀所有複印文件。電子郵件的傳輸可能遭攔截、損毀、遺失、破壞、遲到或不完整、或包含病毒，無法保證其安全或無誤。寄件人不承擔因本電子郵件的錯誤或遺漏所產生的任何損害賠償責任。 Confidentiality Notice: This e-mail message together with any attachments thereto (if any) is confidential, protected under an enforceable non-disclosure agreement, intended only for the use of the named recipient(s) above and may contain information that is privileged, belonging to professional work products or exempt from disclosure under applicable laws. Any unauthorized review, use, copying, disclosure, or distribution of any information contained in or attached to this transmission is strictly prohibited and may be against the laws. If you have received this message in error, or are not the intended recipient(s), please immediately notify the sender by e-mail and delete this e-mail message, all copies, and any attached documentation from your computer. E-mail transmission cannot be guaranteed to be secure or error-free as information could be intercepted, corrupted, lost, destroyed, arrive late or incomplete, or contain viruses. The sender therefore does not accept liability for any damage caused by any errors or omissions in the contents of this email.

USB:Serial:pl2303:Add new Pull-UP Mode to support PL2303HXD(TYPE_HX)

[Charles Yeh]

The Pull-UP mode only support PL2303HXD,it needs to use addition schematic design.

Signed-off-by: Charles Yeh <charlesyeh522@gmail.com>
---
 drivers/usb/serial/pl2303.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/drivers/usb/serial/pl2303.c b/drivers/usb/serial/pl2303.c
index 98e7a5df0f6d..72544e5c928d 100644
--- a/drivers/usb/serial/pl2303.c
+++ b/drivers/usb/serial/pl2303.c
@@ -144,6 +144,9 @@ MODULE_DEVICE_TABLE(usb, id_table);
 #define UART_OVERRUN_ERROR		0x40
 #define UART_CTS			0x80
 
+#define TYPE_HX_PULLUP_MODE_DATA	0x08
+#define TYPE_HX_PULLUP_MODE_REG		0x09
+
 static void pl2303_set_break(struct usb_serial_port *port, bool enable);
 
 enum pl2303_type {
@@ -687,6 +690,15 @@ static void pl2303_set_termios(struct tty_struct *tty,
 		pl2303_vendor_write(serial, 0x0, 0x0);
 	}
 
+	pl2303_vendor_read(serial, 0x8484, buf);
+	pl2303_vendor_write(serial, 0x0404, TYPE_HX_PULLUP_MODE_REG);
+	pl2303_vendor_read(serial, 0x8484, buf);
+	pl2303_vendor_read(serial, 0x8383, buf);
+	if (*buf == TYPE_HX_PULLUP_MODE_DATA) {
+		pl2303_vendor_write(serial, 0x0, 0x31);
+		pl2303_vendor_write(serial, 0x1, 0x01);
+	}
+
 	kfree(buf);
 }