Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1258780

Re: [PATCH] gpio: add tps65218 gpio driver

Show all headers | View raw

On Fri, Oct 23, 2015 at 6:33 PM, Nicolas Saenz Julienne
<nicolassaenzj@gmail.com> wrote:

> Driver for the GPIO block found in ti's tps65218 pmics.
>
> The device has two GPIOs and one GPO pin which can be configured as follows:
> GPIO1:
>         -general-purpose, open-drain output controlled by GPO1 user bit and/or
>          sequencer
>         -DDR3 reset input signal from SOC. Signal is either latched or
>          passed-trough to GPO2 pin. See below for details.
> GPO2:
>         -general-purpose output controlled by GPO2 user bit
>         -DDR3 reset output signal. Signal is controlled by GPIO1 and PGOOD.
>          See below for details.
>         -Output buffer can be configured as open-drain or push-pull.
> GPIO3:
>         -general-purpose, open-drain output controlled by GPO3 user bit and/or
>          sequencer
>         -reset input-signal for DCDC1 and DCDC2.
>
> The input configurations are not meant to be used by the user so the driver
> only offers GPOs.
>
> v2: Added request routine that evaluates the fw config flags and removed module
>     owner
>
> Signed-off-by: Nicolas Saenz Julienne <nicolassaenzj@gmail.com>
(...)

(Attend to the mail robot's comments)

> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/errno.h>
> +#include <linux/gpio.h>

Should be <linux/gpio/driver.h>

> +#include <linux/platform_device.h>
> +#include <linux/mfd/tps65218.h>
> +#include "gpiolib.h"
> +
> +struct tps65218_gpio {
> +       struct tps65218 *tps65218;
> +       struct gpio_chip gpio_chip;
> +};
> +
> +#define to_tg(gc)      container_of(gc, struct tps65218_gpio, gpio_chip)

Use a static inline function for this instead.

> +static int tps65218_gpio_get(struct gpio_chip *gc, unsigned offset)
> +{
> +       struct tps65218_gpio *tps65218_gpio = to_tg(gc);
> +       struct tps65218 *tps65218 = tps65218_gpio->tps65218;
> +       unsigned int val;
> +       int ret;
> +
> +       ret = tps65218_reg_read(tps65218, TPS65218_REG_ENABLE2, &val);
> +       if (ret)
> +               return ret;
> +
> +       return val & (TPS65218_ENABLE2_GPIO1 << offset);

Clamp to bool:
return !!(val & (TPS65218_ENABLE2_GPIO1 << offset));

> +static int tps65218_gpio_output(struct gpio_chip *gc, unsigned offset,
> +                               int value)
> +{
> +       /* Only drives GPOs */
> +       return 0;
> +}

So shouldn't you implement a .set_direction() callback that will fail if
the user tries to set a line as input?

> +static int tps65218_gpio_request(struct gpio_chip *gc, unsigned offset)
> +{
> +       struct tps65218_gpio *tps65218_gpio = to_tg(gc);
> +       struct tps65218 *tps65218 = tps65218_gpio->tps65218;
> +       unsigned long flags = gc->desc->flags;
> +       int ret;
> +
> +       if (flags & FLAG_OPEN_SOURCE) {
> +               dev_err(gc->dev, "can't work as open source\n");
> +               return -EINVAL;
> +       }

This requires Laurent's recent patches I guess.

> +static struct gpio_chip template_chip = {
> +       .label                  = "gpio-tps65218",
> +       .owner                  = THIS_MODULE,
> +       .request                = tps65218_gpio_request,
> +       .direction_output       = tps65218_gpio_output,
> +       .get                    = tps65218_gpio_get,
> +       .set                    = tps65218_gpio_set,
> +       .can_sleep              = true,
> +       .ngpio                  = 3,
> +       .base                   = -1,
> +};

As mentioned you need to implement .set_direction() and fail
to set any line as input.

Apart from this it looks very nice.

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

Back to linux.kernel | Previous | Next — Previous in thread | Find similar | Unroll thread

Thread

[PATCH] gpio: add tps65218 gpio driver Nicolas Saenz Julienne <nicolassaenzj@gmail.com> - 2015-10-23 18:40 +0200
  Re: [PATCH] gpio: add tps65218 gpio driver kbuild test robot <lkp@intel.com> - 2015-10-24 19:00 +0200
  Re: [PATCH] gpio: add tps65218 gpio driver Linus Walleij <linus.walleij@linaro.org> - 2015-10-29 14:20 +0100

csiph-web