Message ID | 20240229084555.43701-2-arturas.moskvinas@gmail.com |
---|---|
State | New |
Headers | show |
Series | [v2] gpio: 74x164: Enable output pins after registers are reset | expand |
On Thu, Feb 29, 2024 at 9:47 AM Arturas Moskvinas <arturas.moskvinas@gmail.com> wrote: > > 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. > Use imperative mood in commit messages - make it: "Fix that behavior by making sure ...". > 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 > > Signed-off-by: Arturas Moskvinas <arturas.moskvinas@gmail.com> This looks much better, can you add a Fixes: tag as well? Bartosz > > --- > v1 -> v2 > * Updated commit message to contain more information why a change is made. > --- > drivers/gpio/gpio-74x164.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > 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; > > base-commit: d206a76d7d2726f3b096037f2079ce0bd3ba329b > -- > 2.44.0 >
On Thu, Feb 29, 2024 at 10:45:56AM +0200, Arturas Moskvinas wrote: > 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 Please, convert these to be Link tags, so at the end it will look like Fixes: Link: URL1 [1] Link: URL2 [2] Signed-off-by:
On 2/29/24 10:54, Bartosz Golaszewski wrote: > On Thu, Feb 29, 2024 at 9:47 AM Arturas Moskvinas > <arturas.moskvinas@gmail.com> wrote: >> 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. >> > Use imperative mood in commit messages - make it: "Fix that behavior > by making sure ...". 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. Fix that behavior by making sure chip is fully reset before all outputs are enabled. Bartosz, does this look better? Arturas Moskvinas
On Thu, Feb 29, 2024 at 1:23 PM Arturas Moskvinas <arturas.moskvinas@gmail.com> wrote: > > > > On 2/29/24 10:54, Bartosz Golaszewski wrote: > > On Thu, Feb 29, 2024 at 9:47 AM Arturas Moskvinas > > <arturas.moskvinas@gmail.com> wrote: > >> 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. > >> > > Use imperative mood in commit messages - make it: "Fix that behavior > > by making sure ...". > 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. > Fix that behavior by making sure chip is fully reset before all outputs are > enabled. > > Bartosz, does this look better? ... if previous pin value was set to 1 period Bart > > Arturas Moskvinas
On Thu, Feb 29, 2024 at 6:24 PM Arturas Moskvinas <arturas.moskvinas@gmail.com> wrote: > > > > On 2/29/24 15:25, Bartosz Golaszewski wrote: > > On Thu, Feb 29, 2024 at 1:23 PM Arturas Moskvinas > <arturas.moskvinas@gmail.com> wrote: > > 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. > Fix that behavior by making sure chip is fully reset before all outputs are > enabled. > > Bartosz, does this look better? > > ... if previous pin value was set to 1 period > > Andriy, Bartosz, is commit message ok? > === > gpio: 74x164: Enable output pins after registers are reset > > 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. > Fix 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. > > Fixes: 7ebc194d0fd4 (gpio: 74x164: Introduce 'enable-gpios' property) > Link: https://elixir.bootlin.com/linux/v6.7.4/source/drivers/gpio/gpio-74x164.c#L130 [1] > Link: https://elixir.bootlin.com/linux/v6.7.4/source/drivers/gpio/gpio-74x164.c#L150 [2] > Signed-off-by: Arturas Moskvinas <arturas.moskvinas@gmail.com> > === > > Arturas Moskvinas Looks good to me. Bart
On Thu, Feb 29, 2024 at 07:24:09PM +0200, Arturas Moskvinas wrote: > On 2/29/24 15:25, Bartosz Golaszewski wrote: > > On Thu, Feb 29, 2024 at 1:23 PM Arturas Moskvinas > > <arturas.moskvinas@gmail.com> wrote: > > > 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. > > > Fix that behavior by making sure chip is fully reset before all outputs are > > > enabled. > > > > > > Bartosz, does this look better? > > ... if previous pin value was set to 1 period > Andriy, Bartosz, is commit message ok? LGTM! Please, send a v3 with it.
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;
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 Signed-off-by: Arturas Moskvinas <arturas.moskvinas@gmail.com> --- v1 -> v2 * Updated commit message to contain more information why a change is made. --- drivers/gpio/gpio-74x164.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) base-commit: d206a76d7d2726f3b096037f2079ce0bd3ba329b