Re: [PATCH] USB: serial: pl2303: add PL2303G GPIO_A and GPIO_B functions

[Greg KH <gregkh@linuxfoundation.org>]

On Wed, Jun 18, 2025 at 08:35:13PM +0800, Charles Yeh wrote:
> PL2303G (TYPE_HXN) series ICs: PL2303GC, PL2303GS, PL2303GT, PL2303GR,.. etc.
> 
> For example:
> PL2303GC can provide up to 16 GPIO (general purpose input/output) signals.
> they are composed of GPIO_A0~A7 and GPIO_B0~B7 respectively.
> 
> In addition to the original UART functions such as Tx, Rx, RTS, CTS, etc.,
> users can also use GPIO to implement certain applications,
> such as turning on/off the LED power.
> 
> Users can download the PL2303G Linux GPIO app: PL2303G_Linux_GPIO_Test.c
> from the Prolific website.
> 
> Signed-off-by: Charles Yeh <charlesyeh522@gmail.com>
> ---
>  drivers/usb/serial/pl2303.c | 182 ++++++++++++++++++++++++++++++++++++
>  1 file changed, 182 insertions(+)
> 
> diff --git a/drivers/usb/serial/pl2303.c b/drivers/usb/serial/pl2303.c
> index 22579d0d8ab8..be7e695bea60 100644
> --- a/drivers/usb/serial/pl2303.c
> +++ b/drivers/usb/serial/pl2303.c
> @@ -175,6 +175,37 @@ MODULE_DEVICE_TABLE(usb, id_table);
>  #define PL2303_HXN_FLOWCTRL_RTS_CTS	0x18
>  #define PL2303_HXN_FLOWCTRL_XON_XOFF	0x0c
>  
> +
> +struct PL2303G_GPIO {

Didn't checkpatch complain about this?  Linux structures are all
lowercase.

> +	u8 GP_Branch;
> +	u8 Number;
> +	u8 Value;

Same here, always use checkpatch.pl on your changes before sending them
out to catch basic coding style issues please.

> +};
> +
> +
> +#define GPIO_AB_CONTROL_MODE		_IOW(0x81, 10, struct PL2303G_GPIO)
> +#define GPIO_AB_OUTPUT_MODE		_IOW(0x81, 11, struct PL2303G_GPIO)
> +#define GPIO_AB_SET_VALUE		_IOW(0x81, 12, struct PL2303G_GPIO)
> +#define GPIO_AB_GET_VALUE		_IOR(0x81, 13, struct PL2303G_GPIO)

Why are you adding custom ioctls for just this driver?

And where is the user/kernel api definition of this so that userspace
could know about it properly?  You can't bury it in a driver like this.

And where did 0x81 and the other numbers come from?

Also, the structure here will not work in the user/kernel api at all,
always use the proper user/kernel data structures if you are going to do
this.

But overall, no, don't add a new user/kernel api for gpio devices, we
have at least 2 already, please don't add another one.  What is wrong
with the current ones?

> +	switch (cmd) {
> +	case TIOCGSERIAL:
> +		memset(&ser, 0, sizeof(ser));
> +		ser.type = PORT_16654;
> +		ser.line = port->minor;
> +		ser.port = port->port_number;
> +		ser.baud_base = 460800;

Are you sure this baud_base is correct?

thanks,

greg k-h