| Message ID | 20200814100357.209340-2-thomas.preston@codethink.co.uk |
|---|---|
| State | New |
| Headers | show |
| Series | pinctrl: mcp23s08: Fixups for mcp23x17 | expand |
On Fri, Aug 14, 2020 at 12:04 PM Thomas Preston <thomas.preston@codethink.co.uk> wrote: > - Fix a typo where mcp23x17 configs are referred to as mcp23x16. > - Fix precious range to include INTCAP{A,B}, which clear on read. > - Fix precious range to include GPIOB, which clears on read. > - Fix volatile range to include GPIOB, to fix debugfs registers > reporting different values than `gpioget gpiochip2 {0..15}`. > > Signed-off-by: Thomas Preston <thomas.preston@codethink.co.uk> Since the other two patches seem wrong, please resend this one patch, also include the people on TO: here: Andy, Phil and Jan, who all use this chip a lot. Yours, Linus Walleij
On Fri, Aug 28, 2020 at 11:06:21AM +0200, Linus Walleij wrote: > On Fri, Aug 14, 2020 at 12:04 PM Thomas Preston > <thomas.preston@codethink.co.uk> wrote: > > > - Fix a typo where mcp23x17 configs are referred to as mcp23x16. > > - Fix precious range to include INTCAP{A,B}, which clear on read. > > - Fix precious range to include GPIOB, which clears on read. > > - Fix volatile range to include GPIOB, to fix debugfs registers > > reporting different values than `gpioget gpiochip2 {0..15}`. > > > > Signed-off-by: Thomas Preston <thomas.preston@codethink.co.uk> > > Since the other two patches seem wrong, please resend this one patch, > also include the people on TO: here: Andy, Phil and Jan, who all use > this chip a lot. And it seems it combines a lot of stuff in one patch. Can we have a split with appropriate Fixes: tags?
On Fri, Aug 14, 2020 at 1:35 PM Thomas Preston <thomas.preston@codethink.co.uk> wrote: > > - Fix a typo where mcp23x17 configs are referred to as mcp23x16. I'm not sure it's correct. MPC23016 is an existing I²C IO expander. > - Fix precious range to include INTCAP{A,B}, which clear on read. > - Fix precious range to include GPIOB, which clears on read. > - Fix volatile range to include GPIOB, to fix debugfs registers > reporting different values than `gpioget gpiochip2 {0..15}`. I'm wondering if you read all the datasheets before doing these changes. MPC2308 MPC23016 MPC23017 ... > -static const struct regmap_range mcp23x16_volatile_range = { > +static const struct regmap_range mcp23x17_volatile_range = { > .range_min = MCP_INTF << 1, > - .range_max = MCP_GPIO << 1, > + .range_max = (MCP_GPIO << 1) + 1, This looks weird. Usually we do a mask or a bit based mask, like (1 << x) - 1. > }; ... > +static const struct regmap_range mcp23x17_precious_range = { > + .range_min = MCP_INTCAP << 1, > + .range_max = (MCP_GPIO << 1) + 1, Ditto. > };
On 28/08/2020 10:28, Andy Shevchenko wrote: > On Fri, Aug 28, 2020 at 11:06:21AM +0200, Linus Walleij wrote: >> On Fri, Aug 14, 2020 at 12:04 PM Thomas Preston >> <thomas.preston@codethink.co.uk> wrote: >> >>> - Fix a typo where mcp23x17 configs are referred to as mcp23x16. >>> - Fix precious range to include INTCAP{A,B}, which clear on read. >>> - Fix precious range to include GPIOB, which clears on read. >>> - Fix volatile range to include GPIOB, to fix debugfs registers >>> reporting different values than `gpioget gpiochip2 {0..15}`. >>> >>> Signed-off-by: Thomas Preston <thomas.preston@codethink.co.uk> >> >> Since the other two patches seem wrong, please resend this one patch, >> also include the people on TO: here: Andy, Phil and Jan, who all use >> this chip a lot. > > And it seems it combines a lot of stuff in one patch. Can we have a split with > appropriate Fixes: tags? > Yeah no problem, just looking at this now.
Hey Andy, Linus, Thanks for looking at this. On 28/08/2020 11:09, Andy Shevchenko wrote: > On Fri, Aug 14, 2020 at 1:35 PM Thomas Preston > <thomas.preston@codethink.co.uk> wrote: >> >> - Fix a typo where mcp23x17 configs are referred to as mcp23x16. > > I'm not sure it's correct. MPC23016 is an existing I²C IO expander. > The MCP23016 device is not mentioned anywhere else in this driver. The only place this string is used is in `struct regmap_config mcp23x17_regmap` (another device). It seems to me that this is a typo but I might be wrong. ~/w/linux$ git grep -h compatible drivers/pinctrl/pinctrl-mcp23s08* .compatible = "microchip,mcp23008", .compatible = "microchip,mcp23017", .compatible = "microchip,mcp23018", .compatible = "mcp,mcp23008", .compatible = "mcp,mcp23017", .compatible = "microchip,mcp23s08", .compatible = "microchip,mcp23s17", .compatible = "microchip,mcp23s18", .compatible = "mcp,mcp23s08", .compatible = "mcp,mcp23s17", Also I don't have an MC23016, so I can't test configuration for it. >> - Fix precious range to include INTCAP{A,B}, which clear on read. >> - Fix precious range to include GPIOB, which clears on read. >> - Fix volatile range to include GPIOB, to fix debugfs registers >> reporting different values than `gpioget gpiochip2 {0..15}`. > > I'm wondering if you read all the datasheets before doing these changes. > MPC2308 > MPC23016 > MPC23017 > ... > I did not! I was only changing configuration for MCP23017 devices. What have I missed? For reference, I think you are referring to [0], [1], [2]. I'm familiar with the last one. >> -static const struct regmap_range mcp23x16_volatile_range = { >> +static const struct regmap_range mcp23x17_volatile_range = { >> .range_min = MCP_INTF << 1, >> - .range_max = MCP_GPIO << 1, >> + .range_max = (MCP_GPIO << 1) + 1, > > This looks weird. Usually we do a mask or a bit based mask, like (1 << x) - 1. > I don't think these are masks, they're addresses. I believe the author has doubled the register indexing using a 1 bit shift, because the MCP23017 device is configured with sequential addresses (IOCON.BANK = 0). On page 12 of the datasheet [2] this looks like: 0x00 IODIRA, MCP_IODIR << 1 0x01 IODIRB 0x02 IPOLA, MCP_IPOL << 1 0x03 IPOLB ... 0x12 GPIOA, MCP_GPIO << 1 0x13 GPIOB This means you can read 16 bits from MCP_GPIO << 1 and get the register values for both banks, or even use this for .range_min. However, this trick doesn't work for .range_max: .range_max = MCP_GPIO << 1; /* 0x12 */ But I think it needs to be 0x13 to include GPIOB. Now that I'm looking into it, how does `mcp23x17_regmap.val_bits = 16` affect this? Perhaps `MCP_GPIO << 1` is fine after all. I will whip up a v2 and test this. I'll split the changes across patches and fix the typo last patch - in case you don't agree with me. Many thanks, Thomas [0] MCP23008 https://ww1.microchip.com/downloads/en/DeviceDoc/21919e.pdf [1] MCP23016 http://ww1.microchip.com/downloads/en/devicedoc/20090c.pdf [2] MCP23017 https://ww1.microchip.com/downloads/en/DeviceDoc/20001952C.pdf
diff --git a/drivers/pinctrl/pinctrl-mcp23s08.c b/drivers/pinctrl/pinctrl-mcp23s08.c index 42b12ea14d6b..0138638276e7 100644 --- a/drivers/pinctrl/pinctrl-mcp23s08.c +++ b/drivers/pinctrl/pinctrl-mcp23s08.c @@ -87,7 +87,7 @@ const struct regmap_config mcp23x08_regmap = { }; EXPORT_SYMBOL_GPL(mcp23x08_regmap); -static const struct reg_default mcp23x16_defaults[] = { +static const struct reg_default mcp23x17_defaults[] = { {.reg = MCP_IODIR << 1, .def = 0xffff}, {.reg = MCP_IPOL << 1, .def = 0x0000}, {.reg = MCP_GPINTEN << 1, .def = 0x0000}, @@ -98,23 +98,23 @@ static const struct reg_default mcp23x16_defaults[] = { {.reg = MCP_OLAT << 1, .def = 0x0000}, }; -static const struct regmap_range mcp23x16_volatile_range = { +static const struct regmap_range mcp23x17_volatile_range = { .range_min = MCP_INTF << 1, - .range_max = MCP_GPIO << 1, + .range_max = (MCP_GPIO << 1) + 1, }; -static const struct regmap_access_table mcp23x16_volatile_table = { - .yes_ranges = &mcp23x16_volatile_range, +static const struct regmap_access_table mcp23x17_volatile_table = { + .yes_ranges = &mcp23x17_volatile_range, .n_yes_ranges = 1, }; -static const struct regmap_range mcp23x16_precious_range = { - .range_min = MCP_GPIO << 1, - .range_max = MCP_GPIO << 1, +static const struct regmap_range mcp23x17_precious_range = { + .range_min = MCP_INTCAP << 1, + .range_max = (MCP_GPIO << 1) + 1, }; -static const struct regmap_access_table mcp23x16_precious_table = { - .yes_ranges = &mcp23x16_precious_range, +static const struct regmap_access_table mcp23x17_precious_table = { + .yes_ranges = &mcp23x17_precious_range, .n_yes_ranges = 1, }; @@ -124,10 +124,10 @@ const struct regmap_config mcp23x17_regmap = { .reg_stride = 2, .max_register = MCP_OLAT << 1, - .volatile_table = &mcp23x16_volatile_table, - .precious_table = &mcp23x16_precious_table, - .reg_defaults = mcp23x16_defaults, - .num_reg_defaults = ARRAY_SIZE(mcp23x16_defaults), + .volatile_table = &mcp23x17_volatile_table, + .precious_table = &mcp23x17_precious_table, + .reg_defaults = mcp23x17_defaults, + .num_reg_defaults = ARRAY_SIZE(mcp23x17_defaults), .cache_type = REGCACHE_FLAT, .val_format_endian = REGMAP_ENDIAN_LITTLE, };
- Fix a typo where mcp23x17 configs are referred to as mcp23x16. - Fix precious range to include INTCAP{A,B}, which clear on read. - Fix precious range to include GPIOB, which clears on read. - Fix volatile range to include GPIOB, to fix debugfs registers reporting different values than `gpioget gpiochip2 {0..15}`. Signed-off-by: Thomas Preston <thomas.preston@codethink.co.uk> --- drivers/pinctrl/pinctrl-mcp23s08.c | 28 ++++++++++++++-------------- 1 file changed, 14 insertions(+), 14 deletions(-)