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

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

Hi Linus,

sorry for the noise..

On 19-10-09 11:56, Marco Felsch wrote:
> 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.

4) won't work didn't recognize that 'struct gpio_device' is an internal
struct.

Regards,
  Marco

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

-- 
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 |