Re: [PATCH] gpio: lp87565: Add support for GPIO

[Linus Walleij <linus.walleij@xxxxxxxxxx>]

On Thu, Jun 1, 2017 at 5:10 AM, Keerthy <j-keerthy@xxxxxx> wrote:

> Add driver for lp87565 PMIC family GPIOs. Three GPIOs are supported
> and can be configured in Open-drain output or Push-pull output.
>
> Signed-off-by: Keerthy <j-keerthy@xxxxxx>

(...)
> The latest version of mfd driver for this pmic is posted:
> https://lkml.org/lkml/2017/5/30/463
(...)
> +config GPIO_LP87565
> +       tristate "TI LP87565 GPIO"
> +       depends on MFD_TI_LP87565

Hm I guess that means I could merge it since it will only compile once
that symbol turns up in the kernel tree.

> +#include <linux/gpio.h>

Please use
#include <linux/gpio/driver.h>
only.

> +#include <linux/mfd/lp87565.h>

Is this API stable enough that I could merge this and count on it to
"just work" once the MFD driver lands?

> +struct lp87565_gpio {
> +       struct gpio_chip chip;
> +       struct lp87565 *lp87565;
> +};

It seems the code would be easier to read if you store the struct regmap *map
pointer here instead of the whole struct lp87565.

But it's no strong preference.

> +static int lp87565_gpio_get(struct gpio_chip *chip, unsigned int offset)
> +{
> +       struct lp87565_gpio *gpio = gpiochip_get_data(chip);
> +       int ret, val;
> +
> +       ret = regmap_read(gpio->lp87565->regmap, LP87565_REG_GPIO_IN, &val);
> +       if (ret < 0)
> +               return ret;
> +
> +       return val & BIT(offset);

return !!(val & BIT(offset));

please, so it's clear that we clamp to [0,1].

> +static int lp87565_gpio_request(struct gpio_chip *gc, unsigned int offset)
> +{
> +       struct lp87565_gpio *gpio = gpiochip_get_data(gc);
> +       int ret;
> +
> +       switch (offset) {
> +       case 0:
> +       case 1:
> +       case 2:
> +               /* Setup the GPIO*_SEL MUX to GPIO mode */
> +               ret = regmap_update_bits(gpio->lp87565->regmap,
> +                                        LP87565_REG_PIN_FUNCTION,
> +                                        BIT(offset), BIT(offset));

Hm. Hm.

If this IC has several function modes for the pins it should also
be a pin controller... I know it is a lot of upfront code, but... it will
benefit you in the long run. Is it really just these three pins?

Maybe we should merge it into
drivers/pinctrl/pinctrl-lp87565.c so that at least file placement does
not become a problem later?

> +static int lp87565_gpio_set_config(struct gpio_chip *gc, unsigned int offset,
> +                                  unsigned long config)
> +{
> +       struct lp87565_gpio *gpio = gpiochip_get_data(gc);
> +
> +       switch (pinconf_to_config_param(config)) {
> +       case PIN_CONFIG_DRIVE_OPEN_DRAIN:
> +               return regmap_update_bits(gpio->lp87565->regmap,
> +                                         LP87565_REG_GPIO_CONFIG,
> +                                         BIT(offset +
> +                                             __ffs(LP87565_GOIO1_OD)),
> +                                         BIT(offset +
> +                                             __ffs(LP87565_GOIO1_OD)));
> +       case PIN_CONFIG_DRIVE_PUSH_PULL:
> +               return regmap_update_bits(gpio->lp87565->regmap,
> +                                         LP87565_REG_GPIO_CONFIG,
> +                                         BIT(offset +
> +                                             __ffs(LP87565_GOIO1_OD)), 0);
> +       default:
> +               return -ENOTSUPP;
> +       }
> +}

Nice.

If this was a split GPIO+pin control driver this would just be a call
into the pinctrl back-end from the GPIO controller, like
drivers/pinctrl/intel/pinctrl-intel.c does with just using
gpiochip_generic_config().

> +static int lp87565_gpio_probe(struct platform_device *pdev)
> +{
> +       struct lp87565_gpio *gpio;
> +       int ret;
> +
> +       gpio = devm_kzalloc(&pdev->dev, sizeof(*gpio), GFP_KERNEL);
> +       if (!gpio)
> +               return -ENOMEM;
> +
> +       platform_set_drvdata(pdev, gpio);

Is this pointer used anywhere?

> +       gpio->lp87565 = dev_get_drvdata(pdev->dev.parent);

So maybe assign the regmap instead.

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