Patchwork [v2] USB:serial:pl2303:add new Pull-Up mode to support PL2303HXD (TYPE_HX)

login
register
mail settings
Submitter Charles Yeh
Date Feb. 12, 2019, 12:50 p.m.
Message ID <20190212125049.12108-1-charlesyeh522@gmail.com>
Download mbox | patch
Permalink /patch/723887/
State New
Headers show

Comments

Charles Yeh - Feb. 12, 2019, 12:50 p.m.
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@gmail.com>
---
 drivers/usb/serial/pl2303.c | 24 ++++++++++++++++++++++++
 1 file changed, 24 insertions(+)
Charles Yeh - Feb. 19, 2019, 6:41 a.m.
Hi Johan & Greg,


Do you have received a new patch"[PATCH] [v2]USB:serial:pl2303:add new
Pull-Up mode to support PL2303HXD (TYPE_HX)"?

If you have received a new patch, has the content been confirmed?

Or tell me where needs to be modified.

Charles Yeh <charlesyeh522@gmail.com> 於 2019年2月12日 週二 下午8:51寫道:
>
> 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@gmail.com>
> ---
>  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
> +#define        TYPE_HX_PULLUP_MODE_DATA        0x08
> +#define        TYPE_HX_PULLUP_MODE_REG         0x09
> +#define        TYPE_HX_PUM_ADD0                0x00
> +#define        TYPE_HX_PUM_DATA0               0x31
> +#define        TYPE_HX_PUM_ADD1                0x01
> +#define        TYPE_HX_PUM_DATA1               0x08
> +
>  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);
> +               if (*buf == TYPE_HX_PULLUP_MODE_DATA) {
> +                       pl2303_vendor_write(serial, TYPE_HX_PUM_ADD0,
> +                                               TYPE_HX_PUM_DATA0);
> +                       pl2303_vendor_write(serial, TYPE_HX_PUM_ADD1,
> +                                               TYPE_HX_PUM_DATA1);
> +               }
> +       }
> +
>         kfree(buf);
>  }
>
> --
> 2.19.1
>
Johan Hovold - Feb. 19, 2019, 8:51 a.m.
On Tue, Feb 19, 2019 at 02:41:28PM +0800, Charles Yeh wrote:
> Hi Johan & Greg,
> 
> 
> Do you have received a new patch"[PATCH] [v2]USB:serial:pl2303:add new
> Pull-Up mode to support PL2303HXD (TYPE_HX)"?
> 
> If you have received a new patch, has the content been confirmed?
> 
> Or tell me where needs to be modified.

Yes, both patches you sent last week are in my queue. I just haven't had
time to review them yet.

Johan
Charles Yeh - March 5, 2019, 1:16 a.m.
Hi Johan,
      Do you have time to review the two patch?

Johan Hovold <johan@kernel.org> 於 2019年2月19日 週二 下午4:51寫道:
>
> On Tue, Feb 19, 2019 at 02:41:28PM +0800, Charles Yeh wrote:
> > Hi Johan & Greg,
> >
> >
> > Do you have received a new patch"[PATCH] [v2]USB:serial:pl2303:add new
> > Pull-Up mode to support PL2303HXD (TYPE_HX)"?
> >
> > If you have received a new patch, has the content been confirmed?
> >
> > Or tell me where needs to be modified.
>
> Yes, both patches you sent last week are in my queue. I just haven't had
> time to review them yet.
>
> Johan
Charles Yeh - March 6, 2019, 4:42 a.m.
Hi Johan / Greg,

Do you have time to review the patch?
If there is no time.. Who else can review this patch?
Because my boss has been asking me to update the Linux patch...

Charles.

Charles Yeh <charlesyeh522@gmail.com> 於 2019年3月5日 週二 上午9:16寫道:
>
> Hi Johan,
>       Do you have time to review the two patch?
>
> Johan Hovold <johan@kernel.org> 於 2019年2月19日 週二 下午4:51寫道:
> >
> > On Tue, Feb 19, 2019 at 02:41:28PM +0800, Charles Yeh wrote:
> > > Hi Johan & Greg,
> > >
> > >
> > > Do you have received a new patch"[PATCH] [v2]USB:serial:pl2303:add new
> > > Pull-Up mode to support PL2303HXD (TYPE_HX)"?
> > >
> > > If you have received a new patch, has the content been confirmed?
> > >
> > > Or tell me where needs to be modified.
> >
> > Yes, both patches you sent last week are in my queue. I just haven't had
> > time to review them yet.
> >
> > Johan
Greg Kroah-Hartman - March 6, 2019, 5:25 a.m.
On Wed, Mar 06, 2019 at 12:42:29PM +0800, Charles Yeh wrote:
> Hi Johan / Greg,
> 
> Do you have time to review the patch?
> If there is no time.. Who else can review this patch?
> Because my boss has been asking me to update the Linux patch...

It is the middle of the merge window and we can not accept any new
patches until after 5.1-rc1 is out in 1 1/2 weeks.  Please wait until
thne before we can get to reviewing this.

thanks,

greg k-h
Charles Yeh - March 20, 2019, 2:43 a.m.
Hi Greg / Johan,
      Is there free time to check the patch?
     Or I have to continue wait.

Thanks!

Charles

Greg KH <gregkh@linuxfoundation.org> 於 2019年3月6日 週三 下午1:25寫道:
>
> On Wed, Mar 06, 2019 at 12:42:29PM +0800, Charles Yeh wrote:
> > Hi Johan / Greg,
> >
> > Do you have time to review the patch?
> > If there is no time.. Who else can review this patch?
> > Because my boss has been asking me to update the Linux patch...
>
> It is the middle of the merge window and we can not accept any new
> patches until after 5.1-rc1 is out in 1 1/2 weeks.  Please wait until
> thne before we can get to reviewing this.
>
> thanks,
>
> greg k-h
Johan Hovold - March 20, 2019, 8:43 a.m.
On Wed, Mar 20, 2019 at 10:43:28AM +0800, Charles Yeh wrote:
> Hi Greg / Johan,
>       Is there free time to check the patch?
>      Or I have to continue wait.

The merge window is just over and I've started to process my queue.
Don't worry, I'll get to your patches as well.

Johan
Johan Hovold - April 2, 2019, 7:34 a.m.
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@gmail.com>
> ---
>  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
Charles Yeh - April 12, 2019, 2:44 a.m.
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@kernel.org> 於 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@gmail.com>
> > ---
> >  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

Patch

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
+#define	TYPE_HX_PULLUP_MODE_DATA	0x08
+#define	TYPE_HX_PULLUP_MODE_REG		0x09
+#define	TYPE_HX_PUM_ADD0		0x00
+#define	TYPE_HX_PUM_DATA0		0x31
+#define	TYPE_HX_PUM_ADD1		0x01
+#define	TYPE_HX_PUM_DATA1		0x08
+
 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);
+		if (*buf == TYPE_HX_PULLUP_MODE_DATA) {
+			pl2303_vendor_write(serial, TYPE_HX_PUM_ADD0,
+						TYPE_HX_PUM_DATA0);
+			pl2303_vendor_write(serial, TYPE_HX_PUM_ADD1,
+						TYPE_HX_PUM_DATA1);
+		}
+	}
+
 	kfree(buf);
 }