Hi Linus,
thanks for you feedback.
On 19-10-04 21:27, Linus Walleij wrote:
> 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.
Mh.. didn't checked the pinctrl framework and assumed that this place is
just fine due to the existing da9052/5 drivers which have an alternate
mode too.
> 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.
I will check the pinctrl framework. Maybe I will have upcoming question.
> > +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.
I saw your comments on the other patch. As you mentioned in the other
patche it is better to have a api within the gpiolib. I will add this
api within the other patch series.
> > +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.
Okay, I will change that.
> > + 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.
Okay.
> > +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?
Thanks for covering that.
> > + *
> > + * 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()?
Please check [1] table 54, the datasheet says it is only gpi
(gpio-input). So I assume it doesn't affect out values.
[1] https://www.dialog-semiconductor.com/sites/default/files/da9062-a_datasheet_2v3.pdf
Unfortunately the other gpio-da90* drivers sets this as active low hard
within the driver. I wanted to avoid this here since it isn't always
true.
> 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?
Not only interrupts.. The dialog pmics has a lot of options to use this
pins e.g. you can set it as voltage-selection input. You saw the patches
I made for the regulator :)
> > +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?
Nope, as I mentioned above this is only input related. If we can trust
the datasheet.
> > +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
okay.
> > + 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!
Okay, I will add more comments.
> > + 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.
Here too.
> > + 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).
This v1 had a hierarchy.. but I changed that upon Bartosz comments. So
there is no hierarchy in the next version.
> > +++ 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.
I will change it to have a common api call as you mentioned it in
the regulator-patch series.
Regards,
Marco
> Yours,
> Linus Walleij
>
--
Pengutronix e.K. | |
Industrial Linux Solutions | http://www.pengutronix.de/ |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
