On Tue, Sep 17, 2019 at 12:59 PM Marco Felsch <m.felsch@xxxxxxxxxxxxxx> wrote:
> The DA9062 is a mfd pmic device which supports 5 GPIOs. The GPIOs can
> be used as input, output or have a special use-case.
>
> The patch adds the support for the normal input/output use-case.
>
> Signed-off-by: Marco Felsch <m.felsch@xxxxxxxxxxxxxx>
Nice to get this covered!
> +#define DA9062_TYPE(offset) (4 * (offset % 2))
> +#define DA9062_PIN_SHIFT(offset) (4 * (offset % 2))
> +#define DA9062_PIN_ALTERNATE 0x00 /* gpio alternate mode */
The there is pin control related to the DA9062, we should put the driver
in drivers/pinctrl/pinctrl-da9062.c from day one.
I am not saying you have to implement pin control from scratch, just
have it there and look at sibling drivers to make sure we don't
screw something up for also adding pin control later on.
> +int da9062_gpio_get_hwgpio(struct gpio_desc *desc)
> +{
> + return gpio_chip_hwgpio(desc);
> +}
> +EXPORT_SYMBOL_GPL(da9062_gpio_get_hwgpio);
I would have to look at the other patch to see why this is needed.
Normally other drivers should just have their GPIOs assigned
like anyone else, no shortcuts into the hardware offsets
like this.
> +static int da9062_gpio_get(struct gpio_chip *gc, unsigned int offset)
> +{
> + struct da9062_gpio *gpio = gpiochip_get_data(gc);
> + struct regmap *regmap = gpio->da9062->regmap;
> + int gpio_dir, val;
> + int ret;
> +
> + gpio_dir = da9062_gpio_get_pin_mode(regmap, offset);
This includes other things than the direction of the pin so call
the variable gpio_mode or something rather than gpio_dir.
> + if (gpio_dir < 0)
> + return gpio_dir;
> +
> + switch (gpio_dir) {
> + case DA9062_PIN_ALTERNATE:
> + return -ENOTSUPP;
This is fine for now. Toss in a comment that we may add pin muxing
later.
> +static int da9062_gpio_get_direction(struct gpio_chip *gc, unsigned int offset)
Same comments as above.
> +static int da9062_gpio_direction_input(struct gpio_chip *gc,
> + unsigned int offset)
> +{
> + struct da9062_gpio *gpio = gpiochip_get_data(gc);
> + struct regmap *regmap = gpio->da9062->regmap;
> + struct gpio_desc *desc = gpiochip_get_desc(gc, offset);
> + unsigned int gpi_type;
> + int ret;
> +
> + ret = da9062_gpio_set_pin_mode(regmap, offset, DA9062_PIN_GPI);
> + if (ret)
> + return ret;
Fair enough.
> + /*
> + * If the gpio is active low we should set it in hw too. No worries
> + * about gpio_get() because we read and return the gpio-level. So the
> + * gpiolob active_low handling is still correct.
gpiolib?
> + *
> + * 0 - active low, 1 - active high
> + */
> + gpi_type = !gpiod_is_active_low(desc);
> + return regmap_update_bits(regmap, DA9062AA_GPIO_0_1 + (offset >> 1),
> + DA9062AA_GPIO0_TYPE_MASK << DA9062_TYPE(offset),
> + gpi_type << DA9062_TYPE(offset));
> +}
So this does not affect the value out set by da9062_gpio_set()?
What is the electrical effect of this then, really? To me that seems like
something that is mostly going to be related to how interrupts
trigger (like whether to trig on rising or falling edge) and then it
should really be in the .set_type() callback, should it not?
> +static int da9062_gpio_direction_output(struct gpio_chip *gc,
> + unsigned int offset, int value)
> +{
> + /* Push-Pull / Open-Drain options are configured during set_config */
> + da9062_gpio_set(gc, offset, value);
> +
> + return 0;
> +}
You probably want to make sure to clear the active low bit in this function,
right?
> +static int da9062_gpio_set_config(struct gpio_chip *gc, unsigned int offset,
> + unsigned long config)
> +{
> + struct da9062_gpio *gpio = gpiochip_get_data(gc);
> + struct regmap *regmap = gpio->da9062->regmap;
> + int gpio_dir;
Name this gpio_mode
> + switch (pinconf_to_config_param(config)) {
> + case PIN_CONFIG_BIAS_PULL_DOWN:
> + /* PD only if pin is input */
Info from datasheet? Electrical limitation? Add to comment!
> + gpio_dir = da9062_gpio_get_pin_mode(regmap, offset);
> + if (gpio_dir < 0)
> + return -EINVAL;
> + else if (gpio_dir != DA9062_PIN_GPI)
> + return -ENOTSUPP;
> + return regmap_update_bits(regmap, DA9062AA_CONFIG_K,
> + BIT(offset), BIT(offset));
> + case PIN_CONFIG_BIAS_PULL_UP:
> + /* PU only if pin is output open-drain */
Dito.
> + gpio_dir = da9062_gpio_get_pin_mode(regmap, offset);
> + if (gpio_dir < 0)
> + return -EINVAL;
> + else if (gpio_dir != DA9062_PIN_GPO_OD)
> + return -ENOTSUPP;
> + return regmap_update_bits(regmap, DA9062AA_CONFIG_K,
> + BIT(offset), BIT(offset));
> + case PIN_CONFIG_DRIVE_OPEN_DRAIN:
> + return da9062_gpio_set_pin_mode(regmap, offset,
> + DA9062_PIN_GPO_OD);
> + case PIN_CONFIG_DRIVE_PUSH_PULL:
> + return da9062_gpio_set_pin_mode(regmap, offset,
> + DA9062_PIN_GPO_PP);
> + default:
> + return -ENOTSUPP;
> + }
Overall very nice use of this callback.
> +static int da9062_gpio_to_irq(struct gpio_chip *gc, unsigned int offset)
> +{
> + struct da9062_gpio *gpio = gpiochip_get_data(gc);
> + struct da9062 *da9062 = gpio->da9062;
> +
> + return regmap_irq_get_virq(da9062->regmap_irq,
> + DA9062_IRQ_GPI0 + offset);
> +}
That's interesting. I never saw that before. It's fine, I guess,
I can't figure out whether these slow expanders could be set as
hierarchical though, what do you think? Saw a comment on this
from Bartosz as well. But I generally trust his comment on that
it should be fine unless you have subnodes (i.e. hierarchy).
> +++ b/include/linux/mfd/da9062/gpio.h
> @@ -0,0 +1,13 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * Copyright (C) 2019 Pengutronix, Marco Felsch <kernel@xxxxxxxxxxxxxx>
> + */
> +
> +#ifndef __MFD_DA9062_GPIO_H__
> +#define __MFD_DA9062_GPIO_H__
> +
> +struct gpio_desc;
> +
> +int da9062_gpio_get_hwgpio(struct gpio_desc *desc);
> +
> +#endif /* __MFD_DA9062_GPIO_H__ */
So I'm sceptical about this and it needs a very good motivation.
Yours,
Linus Walleij
