- 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 <[email protected]>
---
drivers/pinctrl/pinctrl-mcp23s08.c | 28 ++++++++++++++--------------
1 file changed, 14 insertions(+), 14 deletions(-)
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,
};
--
2.26.2
On Fri, Aug 14, 2020 at 12:04 PM Thomas Preston
<[email protected]> 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 <[email protected]>
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
> <[email protected]> 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 <[email protected]>
>
> 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?
--
With Best Regards,
Andy Shevchenko
On Fri, Aug 14, 2020 at 1:35 PM Thomas Preston
<[email protected]> 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.
> };
--
With Best Regards,
Andy Shevchenko
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
>> <[email protected]> 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 <[email protected]>
>>
>> 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
> <[email protected]> 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