Re: [PATCH 2/2] gpio: mcp23s08: add pinconf support

[Sebastian Reichel <sre@kernel.org>]

[-- Attachment #1: Type: text/plain, Size: 2881 bytes --]

Hi Linus,

On Mon, Jan 30, 2017 at 04:08:01PM +0100, Linus Walleij wrote:
> On Fri, Jan 27, 2017 at 3:47 PM, Sebastian Reichel <sre@kernel.org> wrote:
> 
> > mcp23xxx device have configurable 100k pullup resistors. This adds
> > support for enabling them using pinctrl's pinconf interface.
> >
> > Signed-off-by: Sebastian Reichel <sre@kernel.org>
> 
> That's the right approach!
>
> > diff --git a/drivers/gpio/gpio-mcp23s08.c b/drivers/gpio/gpio-mcp23s08.c
> 
> Can we move the patch to drivers/pinctrl/* like all other mixed drivers
> doing combined pinctrl and GPIO?

Sure. Two questions:

 * Should the config name change to PINCTRL_MCP23s08 (or PINCTRL_MCP23XXX)?
   This will mean people will have to fix their .config
 * Should there be a "SPI or I2C GPIO expanders" sub-menu for it as in the
   gpio Kconfig?

> Also: no Kconfig changes? Surely you must select the right pinctrl
> things, I guess you're just lucky that some other pin controller on your
> system selects your infrastructure. I think that is why the build robot
> is complaining.

Yes, it should select GENERIC_PINCONF as far as I can see.

> > +static int mcp_update_cache(struct mcp23s08 *mcp)
> > +{
> > +       int ret, reg, i;
> > +
> > +       for (i = 0; i < ARRAY_SIZE(mcp->cache); i++) {
> > +               ret = mcp_read(mcp, i, &reg);
> > +               if (ret < 0)
> > +                       return ret;
> > +               mcp->cache[i] = reg;
> > +       }
> > +
> > +       return 0;
> > +}
> 
> Why do you need a cache of register values when regmap already
> supports this?
>
> Instead I suggest: exploit the .volatile_reg() callback from
> struct regmap_config and use regmap to maintain the cache.

The mcp_update_cache method is just moved in this patch. I do not
need it, but the existing code uses it. Actually I only moved it, so
that it sticks together with the mcp_read and mcp_write functions,
which must be placed a bit earlier to be usable by the pinctrl stuff.

I did not convert the custom caching in the regmap patch, since it
should be done in its own cleanup patch. Introducing basic regmap
was much more straight forward than removing the open-coded caching.

> Apart from this is is nice!

I guess the custom platform data based pullup config could also
be removed. I just checked and there are not many users of the
platform data:

linux/arch $ git grep -l mcp23 | grep -v "/dts/"         
blackfin/mach-bf527/boards/tll6527m.c
blackfin/mach-bf609/boards/ezkit.c

None of them seems to use the pullups variable.

> With the recent changes from Mika Westerberg scheduled for v4.11
> the road is open to expose pullups all the way to userspace from
> GPIOs chardev if we want to (some patches would be needed).

I just added the pinctrl config to DT, but that sounds useful.

-- Sebastian

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]