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