| Message ID | 20240226134656.608559-1-arturas.moskvinas@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | gpio: 74x164: Enable output pins after registers are reset | expand |
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.
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
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
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.
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 --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;
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