[3/4] pinctrl: mcp23s08: debugfs: Do not restore the INTF register

Apart from the usual stuff, the debugfs code is currently also used for
one non-obvious side effect. It attempts to check whether the chip's
registers are still set to an expected value, and if not, re-initializes
them. It seems that the driver has "always" done so.

The code, however, also checks the INTF register which normally
indicates which pins have caused the recent interrupt. That's a volatile
register, and the datasheet says that writes are ignored in there.

When I'm just cat-ing /sys/kernel/debug/gpio with no SPI traffic and no
nosie on the GPIO lines, I'm not getting any warnings. Once I actually
use these GPIOs and cat that file in parallel, I always seem to get a:

  mcp23s08 spi1.1: restoring reg 0x07 from 0x0000 to 0xffff (power-loss?)

This might be a sign that I should not leave my unused inputs floating,
but the code should not be checking a volatile register, anyway. Let's
simply skip this last item in the iteration. I was also considering
removing this enitre re-initialization because it's non-obvious, but the
code survived various refactorings already and has sign-offs by people
who know more than I do, so let's leave it as-is. For now :).

Signed-off-by: Jan Kundrát <jan.kundrat@cesnet.cz>
---
 drivers/pinctrl/pinctrl-mcp23s08.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

On 21/02/2018 02:04, Jan Kundrát wrote:
> Apart from the usual stuff, the debugfs code is currently also used for
> one non-obvious side effect. It attempts to check whether the chip's
> registers are still set to an expected value, and if not, re-initializes
> them. It seems that the driver has "always" done so.
> 
> The code, however, also checks the INTF register which normally
> indicates which pins have caused the recent interrupt. That's a volatile
> register, and the datasheet says that writes are ignored in there.
> 
> When I'm just cat-ing /sys/kernel/debug/gpio with no SPI traffic and no
> nosie on the GPIO lines, I'm not getting any warnings. Once I actually
> use these GPIOs and cat that file in parallel, I always seem to get a:
> 
>    mcp23s08 spi1.1: restoring reg 0x07 from 0x0000 to 0xffff (power-loss?)
> 
> This might be a sign that I should not leave my unused inputs floating,
> but the code should not be checking a volatile register, anyway. Let's
> simply skip this last item in the iteration. I was also considering
> removing this enitre re-initialization because it's non-obvious, but the
> code survived various refactorings already and has sign-offs by people
> who know more than I do, so let's leave it as-is. For now :).
> 
> Signed-off-by: Jan Kundrát <jan.kundrat@cesnet.cz>
> ---
>   drivers/pinctrl/pinctrl-mcp23s08.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/pinctrl/pinctrl-mcp23s08.c b/drivers/pinctrl/pinctrl-mcp23s08.c
> index fd9d27e8126e..bbef9833edb8 100644
> --- a/drivers/pinctrl/pinctrl-mcp23s08.c
> +++ b/drivers/pinctrl/pinctrl-mcp23s08.c
> @@ -664,7 +664,7 @@ static int mcp23s08_irq_setup(struct mcp23s08 *mcp)
>    * can be used to fix state for MCP23xxx, that temporary
>    * lost its power supply.
>    */
> -#define MCP23S08_CONFIG_REGS 8
> +#define MCP23S08_CONFIG_REGS 7
>   static int __check_mcp23s08_reg_cache(struct mcp23s08 *mcp)
>   {
>   	int cached[MCP23S08_CONFIG_REGS];
> 

I agree INTF shouldn't be part of this loop.

I also agree that the entire function is non-obvious.
Comparing the in-chip values to the cached values seems reasonable for debugging
But rewriting them doesn't.

Reviewed-by: Phil Reid <preid@electromag.com.au>

On Tue, Feb 20, 2018 at 7:04 PM, Jan Kundrát <jan.kundrat@cesnet.cz> wrote:

> Apart from the usual stuff, the debugfs code is currently also used for
> one non-obvious side effect. It attempts to check whether the chip's
> registers are still set to an expected value, and if not, re-initializes
> them. It seems that the driver has "always" done so.
>
> The code, however, also checks the INTF register which normally
> indicates which pins have caused the recent interrupt. That's a volatile
> register, and the datasheet says that writes are ignored in there.
>
> When I'm just cat-ing /sys/kernel/debug/gpio with no SPI traffic and no
> nosie on the GPIO lines, I'm not getting any warnings. Once I actually
> use these GPIOs and cat that file in parallel, I always seem to get a:
>
>   mcp23s08 spi1.1: restoring reg 0x07 from 0x0000 to 0xffff (power-loss?)
>
> This might be a sign that I should not leave my unused inputs floating,
> but the code should not be checking a volatile register, anyway. Let's
> simply skip this last item in the iteration. I was also considering
> removing this enitre re-initialization because it's non-obvious, but the
> code survived various refactorings already and has sign-offs by people
> who know more than I do, so let's leave it as-is. For now :).
>
> Signed-off-by: Jan Kundrát <jan.kundrat@cesnet.cz>

Patch applied with Phil's review tag.

Yours,
Linus Walleij
--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html