linux-kernel - Re: [PATCH] usb: cp210x: Added support for GPIO (CP2103/4/5)

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20120428190502.32eaf294@pyramind.ukuu.org.uk>
Date:	Sat, 28 Apr 2012 19:05:02 +0100
From:	Alan Cox <alan@...rguk.ukuu.org.uk>
To:	Preston Fick <pffick@...il.com>
Cc:	gregkh@...uxfoundation.org, linux-usb@...r.kernel.org,
	linux-kernel@...r.kernel.org, preston.fick@...abs.com,
	linux-serial@...r.kernel.org
Subject: Re: [PATCH] usb: cp210x: Added support for GPIO (CP2103/4/5)

>   */
> @@ -424,8 +430,8 @@ static int cp210x_open(struct tty_struct *tty, struct usb_serial_port *port)
>  
>  	dbg("%s - port %d", __func__, port->number);
>  
> -	result = cp210x_set_config_single(port, CP210X_IFC_ENABLE,
> -								UART_ENABLE);
> +	result = cp210x_set_config(port, REQTYPE_HOST_TO_INTERFACE,
> +				CP210X_IFC_ENABLE, UART_ENABLE, NULL, 0);

These kind of unrelated changes make review hard. Why are they there ?

> +	case IOCTL_GPIOGET:
> +		if ((port_priv->bPartNumber == CP2103_PARTNUM) ||

All the part specific stuff in ifs rapidly gets unmaintainable. Better to
set up a port_priv->get_gpio/set_gpio.

>  /*
>   * cp210x_get_termios
>   * Reads the baud rate, data bits, parity, stop bits and flow control mode
> @@ -490,14 +565,16 @@ static void cp210x_get_termios_port(struct usb_serial_port *port,
>  
>  	dbg("%s - port %d", __func__, port->number);
>  
> -	cp210x_get_config(port, CP210X_GET_BAUDRATE, &baud, 4);
> +	cp210x_get_config(port, REQTYPE_INTERFACE_TO_HOST,
> +			CP210X_GET_BAUDRATE, 0, &baud, 4);

And again it disturbs all the rest of the code for no apparent good
reason.

The large number of changes all over the code for a single localised
feature change ought to be flagging up that it's not being done in a
clean way.

The other question is whether having some custom gpio poking interface is
actually a good idea. I suspect probably not. The kernel gpio layer can
help a bit but doesn't really solve the problem as there is no way to tie
a gpio to a port. Given how many devices seem to have gpios these days I
wonder if we need a gpio setting interface via termiox.

We could also the agree how that maps onto the extra gpio lines used with
SIM card readers and the like so we can standardise that.

Alan
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/