| 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
| ||
|
Date: Wed, 28 Aug 2019 17:37:40 +0200
From: Johan Hovold <johan@...nel.org>
To: "Ji-Ze Hong (Peter Hong)" <hpeter@...il.com>
Cc: johan@...nel.org, gregkh@...uxfoundation.org,
linux-usb@...r.kernel.org, linux-kernel@...r.kernel.org,
peter_hong@...tek.com.tw,
"Ji-Ze Hong (Peter Hong)" <hpeter+linux_kernel@...il.com>
Subject: Re: [PATCH V1 6/6] USB: serial: f81232: Add gpiolib to GPIO device
On Thu, Jun 06, 2019 at 10:54:16AM +0800, Ji-Ze Hong (Peter Hong) wrote:
> The Fintek F81534A series contains 3 GPIOs per UART and The max GPIOs
> is 12x3 = 36 GPIOs.
How does this relate to the GPIOs used for transceiver setup? Are these
really general purpose?
Side note: Please explain the relationship to f81534 which you already
have a driver for. Is f81534a all that different that it belongs with
f81232? Confusing...
> Signed-off-by: Ji-Ze Hong (Peter Hong) <hpeter+linux_kernel@...il.com>
> ---
> drivers/usb/serial/f81232.c | 210 ++++++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 210 insertions(+)
>
> diff --git a/drivers/usb/serial/f81232.c b/drivers/usb/serial/f81232.c
> index 708d85c7d822..a53240bc164a 100644
> --- a/drivers/usb/serial/f81232.c
> +++ b/drivers/usb/serial/f81232.c
> @@ -18,6 +18,7 @@
> #include <linux/moduleparam.h>
> #include <linux/mutex.h>
> #include <linux/uaccess.h>
> +#include <linux/gpio.h>
> #include <linux/usb.h>
> #include <linux/usb/serial.h>
> #include <linux/serial_reg.h>
> @@ -132,6 +133,7 @@ struct f81232_private {
>
> struct f81534a_ctrl_private {
> struct usb_interface *intf;
> + struct gpio_chip chip;
> struct mutex lock;
> int device_idx;
> };
> @@ -1007,6 +1009,204 @@ static int f81534a_ctrl_set_register(struct usb_device *dev, u16 reg, u16 size,
> return status;
> }
>
> +static int f81534a_ctrl_set_mask_register(struct usb_device *dev, u16 reg,
> + u8 mask, u8 val)
> +{
> + int status;
> + u8 tmp;
> +
> + status = f81534a_ctrl_get_register(dev, reg, 1, &tmp);
> + if (status)
> + return status;
> +
> +
> + tmp = (tmp & ~mask) | (val & mask);
> +
> + status = f81534a_ctrl_set_register(dev, reg, 1, &tmp);
> + if (status)
> + return status;
> +
> + return 0;
> +}
You'll get a warning about this one being unused with !GPIOLIB.
> +#ifdef CONFIG_GPIOLIB
> +static int f81534a_gpio_get(struct gpio_chip *chip, unsigned int gpio_num)
> +{
> + struct f81534a_ctrl_private *priv = gpiochip_get_data(chip);
> + struct usb_interface *intf = priv->intf;
> + struct usb_device *dev = interface_to_usbdev(intf);
> + int status;
> + u8 tmp[2];
> + int set;
> + int idx;
> +
> + set = gpio_num / F81534A_CTRL_GPIO_MAX_PIN;
> + idx = gpio_num % F81534A_CTRL_GPIO_MAX_PIN;
> +
> + status = mutex_lock_interruptible(&priv->lock);
Why _interruptible?
> + if (status)
> + return -EINTR;
> +
> + status = f81534a_ctrl_get_register(dev, F81534A_CTRL_GPIO_REG + set,
> + sizeof(tmp), tmp);
> + if (status) {
> + mutex_unlock(&priv->lock);
> + return status;
> + }
> +
> + mutex_unlock(&priv->lock);
> +
> + return !!(tmp[1] & BIT(idx));
> +}
> +
> +static int f81534a_gpio_direction_in(struct gpio_chip *chip,
> + unsigned int gpio_num)
> +{
> + struct f81534a_ctrl_private *priv = gpiochip_get_data(chip);
> + struct usb_interface *intf = priv->intf;
> + struct usb_device *dev = interface_to_usbdev(intf);
> + int status;
> + int set;
> + int idx;
> + u8 mask;
> +
> + set = gpio_num / F81534A_CTRL_GPIO_MAX_PIN;
> + idx = gpio_num % F81534A_CTRL_GPIO_MAX_PIN;
> + mask = F81534A_GPIO_MODE0_DIR << idx;
> +
> + status = mutex_lock_interruptible(&priv->lock);
> + if (status)
> + return -EINTR;
> +
> + status = f81534a_ctrl_set_mask_register(dev, F81534A_CTRL_GPIO_REG +
> + set, mask, mask);
> + if (status) {
> + mutex_unlock(&priv->lock);
> + return status;
Just return status below instead.
> + }
> +
> + mutex_unlock(&priv->lock);
> +
> + return 0;
> +}
> +
> +static int f81534a_gpio_direction_out(struct gpio_chip *chip,
> + unsigned int gpio_num, int val)
> +{
> + struct f81534a_ctrl_private *priv = gpiochip_get_data(chip);
> + struct usb_interface *intf = priv->intf;
> + struct usb_device *dev = interface_to_usbdev(intf);
> + int status;
> + int set;
> + int idx;
> + u8 mask;
> + u8 data;
> +
> + set = gpio_num / F81534A_CTRL_GPIO_MAX_PIN;
> + idx = gpio_num % F81534A_CTRL_GPIO_MAX_PIN;
> + mask = (F81534A_GPIO_MODE0_DIR << idx) | BIT(idx);
> + data = val ? BIT(idx) : 0;
> +
> + status = mutex_lock_interruptible(&priv->lock);
> + if (status)
> + return -EINTR;
> +
> + status = f81534a_ctrl_set_mask_register(dev, F81534A_CTRL_GPIO_REG +
> + set, mask, data);
Please keep set on the same line as the reg define, but why not
calculate a reg temporary above instead?
> + if (status) {
> + mutex_unlock(&priv->lock);
> + return status;
As above.
> + }
> +
> + mutex_unlock(&priv->lock);
> +
> + return 0;
> +}
> +
> +static void f81534a_gpio_set(struct gpio_chip *chip, unsigned int gpio_num,
> + int val)
> +{
> + f81534a_gpio_direction_out(chip, gpio_num, val);
> +}
> +
> +static int f81534a_gpio_get_direction(struct gpio_chip *chip,
> + unsigned int gpio_num)
> +{
> + struct f81534a_ctrl_private *priv = gpiochip_get_data(chip);
> + struct usb_interface *intf = priv->intf;
> + struct usb_device *dev = interface_to_usbdev(intf);
> + int status;
> + u8 tmp[2];
> + int set;
> + int idx;
> + u8 mask;
> +
> + set = gpio_num / F81534A_CTRL_GPIO_MAX_PIN;
> + idx = gpio_num % F81534A_CTRL_GPIO_MAX_PIN;
> + mask = F81534A_GPIO_MODE0_DIR << idx;
> +
> + status = mutex_lock_interruptible(&priv->lock);
> + if (status)
> + return -EINTR;
> +
> + status = f81534a_ctrl_get_register(dev, F81534A_CTRL_GPIO_REG + set,
> + sizeof(tmp), tmp);
> + if (status) {
> + mutex_unlock(&priv->lock);
> + return status;
> + }
> +
> + mutex_unlock(&priv->lock);
> +
> + if (tmp[0] & mask)
> + return GPIOF_DIR_IN;
> +
> + return GPIOF_DIR_OUT;
> +}
> +
> +static int f81534a_prepare_gpio(struct usb_interface *intf)
> +{
> + struct f81534a_ctrl_private *priv = usb_get_intfdata(intf);
> + int status;
> +
> + priv->chip.parent = &intf->dev;
> + priv->chip.owner = THIS_MODULE;
> + priv->chip.get_direction = f81534a_gpio_get_direction,
> + priv->chip.get = f81534a_gpio_get;
> + priv->chip.direction_input = f81534a_gpio_direction_in;
> + priv->chip.set = f81534a_gpio_set;
> + priv->chip.direction_output = f81534a_gpio_direction_out;
> + priv->chip.label = "f81534a";
> + /* M0(SD)/M1/M2 */
> + priv->chip.ngpio = F81534A_CTRL_GPIO_MAX_PIN * F81534A_MAX_PORT;
It looks like you should have one gpiochip per port.
> + priv->chip.base = -1;
You need to set the can_sleep flag.
> +
> + priv->intf = intf;
> + mutex_init(&priv->lock);
> +
> + status = devm_gpiochip_add_data(&intf->dev, &priv->chip, priv);
> + if (status) {
> + dev_err(&intf->dev, "%s: failed: %d\n", __func__, status);
No need for __func__. Spell what went wrong (e.g. "failed to register
gpiochip: %d\n").
> + return status;
> + }
> +
> + return 0;
> +}
> +#else
> +static int f81534a_prepare_gpio(struct usb_interface *intf)
> +{
> + dev_warn(&intf->dev, "CONFIG_GPIOLIB is not enabled\n");
> + dev_warn(&intf->dev, "The GPIOLIB interface will not register\n");
Please remove this.
> +
> + return 0;
> +}
> +#endif
> +
> +static int f81534a_release_gpio(struct usb_interface *intf)
> +{
> + return 0;
> +}
Remove.
> +
> static int f81534a_ctrl_generate_ports(struct usb_interface *intf,
> struct f81534a_device *device)
> {
> @@ -1118,6 +1318,7 @@ static int f81534a_ctrl_probe(struct usb_interface *intf,
> struct usb_device *dev = interface_to_usbdev(intf);
> struct f81534a_ctrl_private *priv;
> struct f81534a_device *device;
> + int status;
>
> priv = devm_kzalloc(&intf->dev, sizeof(*priv), GFP_KERNEL);
> if (!priv)
> @@ -1130,6 +1331,10 @@ static int f81534a_ctrl_probe(struct usb_interface *intf,
> mutex_init(&priv->lock);
> usb_set_intfdata(intf, priv);
>
> + status = f81534a_prepare_gpio(intf);
> + if (status)
> + return status;
> +
> INIT_LIST_HEAD(&device->list);
> device->intf = intf;
>
> @@ -1158,6 +1363,11 @@ static void f81534a_ctrl_disconnect(struct usb_interface *intf)
>
> priv = usb_get_intfdata(intf);
> dev = interface_to_usbdev(intf);
> +
> + mutex_lock(&priv->lock);
> + f81534a_release_gpio(intf);
> + mutex_unlock(&priv->lock);
> +
> usb_put_dev(dev);
> break;
> }
Johan
Powered by blists - more mailing lists