Re: [PATCH v4 2/2] gpio: add a reusable generic gpio_chip using regmap

[Michael Walle]

Hi,

Am 2020-05-28 02:31, schrieb Pierre-Louis Bossart:
> Hi Michael,
>
> > > > +struct gpio_regmap_config {
> > > > +ÂÂÂ struct device *parent;
> > > > +ÂÂÂ struct regmap *regmap;
> > > > +
> > > > +ÂÂÂ const char *label;
> > > > +ÂÂÂ int ngpio;
> > >
> > > could we add a .names field for the gpio_chip, I found this useful 
> > > for
> > > PCM512x GPIO support, e.g.
> >
> > Sure, I have the names in the device tree.
> >
> > But I'd prefer that you'd do a patch on top of this (assuming it is
> > applied soon), because you can actually test it and there might be
> > missing more.
>
> I am happy to report that this gpio-regmap worked like a charm for me,
> after I applied the minor diff below (complete code at
> https://github.com/plbossart/sound/tree/fix/regmap-gpios).
>
> I worked around my previous comments by forcing the GPIO internal
> routing directly in regmap, and that allowed me to only play with the
> _set and _dir bases. I see the LEDs and clock selected as before,
> quite nice indeed.
>
> The chip->label test is probably wrong, since the gpio_chip structure
> is zeroed out if(!chip->label) is always true so the label is always
> set to the device name. I don't know what the intent was so just
> removed that test - maybe the correct test should be if
> (!config->label) ?

yes, that was a typo. should have been if (!config->label).

I've send a v5 with that fix and your names property.

> I added the names support as well, and btw I don't understand how one
> would get them through device tree?

gpio-line-names property, see
Documentation/devicetree/bindings/gpio/gpio.txt.

> I still have a series of odd warnings I didn't have before:
>
> [  101.400263] WARNING: CPU: 3 PID: 1129 at
> drivers/gpio/gpiolib.c:4084 gpiod_set_value+0x3f/0x50
>
> This seems to come from
> 	/* Should be using gpiod_set_value_cansleep() */
> 	WARN_ON(desc->gdev->chip->can_sleep);

Right now, gpio-regmap hardcodes can_sleep to true. But the only regmap
which don't sleep is regmap-mmio. The PCM512x seems to be either I2C or
SPI, which can both sleep. So this warning is actually correct and
wherever this gpio is set should do it by calling the _cansleep()
version.

> so maybe we need an option here as well? Or use a different function?
>
> Anyways, that gpio-regmap does simplify my code a great deal so thanks
> for this work, much appreciated.

Glad to see that there are more users for it ;)

-michael