Re: [PATCH 3/3] gpio: da9062: add driver support

[Marco Felsch <m.felsch@pengutronix.de>]

Hi Linus,

On 19-10-07 10:51, Marco Felsch wrote:
> 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@pengutronix.de> wrote:
> > 
> > > +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);

This won't work anymore since I moved the driver to pinctrl and can't
include the drivers/gpio/gpiolib.h anymore. What is the right way to
get the same result within the pinctrl space? There are three possible
ways:
1) Revert commit 1bd6b601fe196b6fbce2c93536ce0f3f53577cec which isn't
   the best due to safeness.
2) Set the gpio as active low hard as the other da90*-gpio drivers did
3) Introduce a dt-binding (seems wrong because the information is
   already there).
4) "Re-implement" the gpiochip_get_desc() functionality driver
   internally.

Thanks for your advice.

Regards,
  Marco

> > > +       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 :)