lkml.org 
[lkml]   [2019]   [Aug]   [28]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [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@gmail.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

    \
     
     \ /
      Last update: 2019-08-28 17:38    [W:2.491 / U:0.528 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site