diff mbox series

gpio: 74x164: Enable output pins after registers are reset

Message ID 20240226134656.608559-1-arturas.moskvinas@gmail.com
State New
Headers
Series gpio: 74x164: Enable output pins after registers are reset |

Commit Message

Arturas Moskvinas Feb. 26, 2024, 1:46 p.m. UTC 
  Move output enabling after chip registers are cleared.

Signed-off-by: Arturas Moskvinas <arturas.moskvinas@gmail.com>
---
 drivers/gpio/gpio-74x164.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)


base-commit: d206a76d7d2726f3b096037f2079ce0bd3ba329b

Comments

Andy Shevchenko Feb. 26, 2024, 2:01 p.m. UTC | #1 
On Mon, Feb 26, 2024 at 03:46:56PM +0200, Arturas Moskvinas wrote:
> Move output enabling after chip registers are cleared.

Does this fix anything? If so, maybe elaborate a bit the potential behavioural
changes on the real lines.
Arturas Moskvinas Feb. 27, 2024, 6:58 a.m. UTC | #2 
Hello,

On 2/26/24 16:01, Andy Shevchenko wrote:
> On Mon, Feb 26, 2024 at 03:46:56PM +0200, Arturas Moskvinas wrote:
>> Move output enabling after chip registers are cleared.
> Does this fix anything? If so, maybe elaborate a bit the potential behavioural
> changes on the real lines.

Chip outputs are enabled[1] before actual reset is performed[2] which 
might cause pin output value to flip flop if previous pin value was set 
to 1 in chip. Change fixes that behavior by making sure chip is fully 
reset before all outputs are enabled.

Flip-flop can be noticed when module is removed and inserted again and 
one of the pins was changed to 1 before removal. 100 microsecond 
flipping is noticeable on oscilloscope (100khz SPI bus).

For a properly reset chip - output is enabled around 100 microseconds 
(on 100khz SPI bus) later during probing process hence should be 
irrelevant behavioral change.

[1] - 
https://elixir.bootlin.com/linux/v6.7.4/source/drivers/gpio/gpio-74x164.c#L130
[2] - 
https://elixir.bootlin.com/linux/v6.7.4/source/drivers/gpio/gpio-74x164.c#L150

Arturas Moskvinas
Bartosz Golaszewski Feb. 27, 2024, 1:14 p.m. UTC | #3 
On Tue, Feb 27, 2024 at 7:58 AM Arturas Moskvinas
<arturas.moskvinas@gmail.com> wrote:
>
> Hello,
>
> On 2/26/24 16:01, Andy Shevchenko wrote:
> > On Mon, Feb 26, 2024 at 03:46:56PM +0200, Arturas Moskvinas wrote:
> >> Move output enabling after chip registers are cleared.
> > Does this fix anything? If so, maybe elaborate a bit the potential behavioural
> > changes on the real lines.
>
> Chip outputs are enabled[1] before actual reset is performed[2] which
> might cause pin output value to flip flop if previous pin value was set
> to 1 in chip. Change fixes that behavior by making sure chip is fully
> reset before all outputs are enabled.
>
> Flip-flop can be noticed when module is removed and inserted again and
> one of the pins was changed to 1 before removal. 100 microsecond
> flipping is noticeable on oscilloscope (100khz SPI bus).
>
> For a properly reset chip - output is enabled around 100 microseconds
> (on 100khz SPI bus) later during probing process hence should be
> irrelevant behavioral change.
>
> [1] -
> https://elixir.bootlin.com/linux/v6.7.4/source/drivers/gpio/gpio-74x164.c#L130
> [2] -
> https://elixir.bootlin.com/linux/v6.7.4/source/drivers/gpio/gpio-74x164.c#L150
>
> Arturas Moskvinas

And this is precisely the kind of information that needs to go into
commit messages. I can tell *what* you're doing by looking at the
code. What I can't tell is *why*.

Bartosz
Andy Shevchenko Feb. 27, 2024, 2:53 p.m. UTC | #4 
On Tue, Feb 27, 2024 at 02:14:40PM +0100, Bartosz Golaszewski wrote:
> On Tue, Feb 27, 2024 at 7:58 AM Arturas Moskvinas
> <arturas.moskvinas@gmail.com> wrote:
> > On 2/26/24 16:01, Andy Shevchenko wrote:
> > > On Mon, Feb 26, 2024 at 03:46:56PM +0200, Arturas Moskvinas wrote:
> > >> Move output enabling after chip registers are cleared.
> > > Does this fix anything? If so, maybe elaborate a bit the potential behavioural
> > > changes on the real lines.
> >
> > Chip outputs are enabled[1] before actual reset is performed[2] which
> > might cause pin output value to flip flop if previous pin value was set
> > to 1 in chip. Change fixes that behavior by making sure chip is fully
> > reset before all outputs are enabled.
> >
> > Flip-flop can be noticed when module is removed and inserted again and
> > one of the pins was changed to 1 before removal. 100 microsecond
> > flipping is noticeable on oscilloscope (100khz SPI bus).
> >
> > For a properly reset chip - output is enabled around 100 microseconds
> > (on 100khz SPI bus) later during probing process hence should be
> > irrelevant behavioral change.
> >
> > [1] -
> > https://elixir.bootlin.com/linux/v6.7.4/source/drivers/gpio/gpio-74x164.c#L130
> > [2] -
> > https://elixir.bootlin.com/linux/v6.7.4/source/drivers/gpio/gpio-74x164.c#L150
> >
> > Arturas Moskvinas
> 
> And this is precisely the kind of information that needs to go into
> commit messages. I can tell *what* you're doing by looking at the
> code. What I can't tell is *why*.

+1. Please, add this to the commit message of v2, also try to find the commit
that you can mark to be fixed with help of Fixes tag.
Arturas Moskvinas Feb. 27, 2024, 7:11 p.m. UTC | #5 
Hello,

..
> > And this is precisely the kind of information that needs to go into
> > commit messages. I can tell *what* you're doing by looking at the
> > code. What I can't tell is *why*.
>
> +1. Please, add this to the commit message of v2, also try to find the commit
> that you can mark to be fixed with help of Fixes tag.

Thanks for suggestion regarding Fixes!

I thought maybe I should as well move whole GPIO initialization[0]
down to the same place I move "gpiod_set_value_cansleep(
chip->gpiod_oe, 1)" in patch v2? I think knowledge that a pin is
brought up later during probing process might be forgotten later, it
will slightly complicate code due to need to clean mutex though.

[0] https://elixir.bootlin.com/linux/v6.7.6/source/drivers/gpio/gpio-74x164.c#L125

Arturas Moskvinas
diff mbox series

Patch

diff --git a/drivers/gpio/gpio-74x164.c b/drivers/gpio/gpio-74x164.c
index e00c33310517..753e7be039e4 100644
--- a/drivers/gpio/gpio-74x164.c
+++ b/drivers/gpio/gpio-74x164.c
@@ -127,8 +127,6 @@  static int gen_74x164_probe(struct spi_device *spi)
 	if (IS_ERR(chip->gpiod_oe))
 		return PTR_ERR(chip->gpiod_oe);
 
-	gpiod_set_value_cansleep(chip->gpiod_oe, 1);
-
 	spi_set_drvdata(spi, chip);
 
 	chip->gpio_chip.label = spi->modalias;
@@ -153,6 +151,8 @@  static int gen_74x164_probe(struct spi_device *spi)
 		goto exit_destroy;
 	}
 
+	gpiod_set_value_cansleep(chip->gpiod_oe, 1);
+
 	ret = gpiochip_add_data(&chip->gpio_chip, chip);
 	if (!ret)
 		return 0;