Re: [PATCH v1 1/1] gpiolib: Deduplicate gpiod_direction_input_nonotify() call

From: Bartosz Golaszewski
Date: Thu Oct 31 2024 - 15:15:24 EST


On Thu, Oct 31, 2024 at 10:22 AM Andy Shevchenko
<andriy.shevchenko@xxxxxxxxxxxxxxx> wrote:
>
> Deduplicate gpiod_direction_input_nonotify() call in
> gpiod_direction_output_nonotify() when emulating open-drain
> or open-source behaviour. It also aligns the error check
> approaches in set_output_value and set_output_flag labels.
>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@xxxxxxxxxxxxxxx>
> ---
> drivers/gpio/gpiolib.c | 16 +++++++---------
> 1 file changed, 7 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
> index 5666c462248c..9376dd270344 100644
> --- a/drivers/gpio/gpiolib.c
> +++ b/drivers/gpio/gpiolib.c
> @@ -2877,19 +2877,15 @@ int gpiod_direction_output_nonotify(struct gpio_desc *desc, int value)
> if (!ret)
> goto set_output_value;
> /* Emulate open drain by not actively driving the line high */
> - if (value) {
> - ret = gpiod_direction_input_nonotify(desc);
> + if (value)
> goto set_output_flag;
> - }
> } else if (test_bit(FLAG_OPEN_SOURCE, &flags)) {
> ret = gpio_set_config(desc, PIN_CONFIG_DRIVE_OPEN_SOURCE);
> if (!ret)
> goto set_output_value;
> /* Emulate open source by not actively driving the line low */
> - if (!value) {
> - ret = gpiod_direction_input_nonotify(desc);
> + if (!value)
> goto set_output_flag;
> - }
> } else {
> gpio_set_config(desc, PIN_CONFIG_DRIVE_PUSH_PULL);
> }
> @@ -2901,15 +2897,17 @@ int gpiod_direction_output_nonotify(struct gpio_desc *desc, int value)
> return gpiod_direction_output_raw_commit(desc, value);
>
> set_output_flag:

Ok, I'll take it but please change this label to "emulate_output" or
something similar that's more indicative on what the goal here is.

Bart

> + ret = gpiod_direction_input_nonotify(desc);
> + if (ret)
> + return ret;
> /*
> * When emulating open-source or open-drain functionalities by not
> * actively driving the line (setting mode to input) we still need to
> * set the IS_OUT flag or otherwise we won't be able to set the line
> * value anymore.
> */
> - if (ret == 0)
> - set_bit(FLAG_IS_OUT, &desc->flags);
> - return ret;
> + set_bit(FLAG_IS_OUT, &desc->flags);
> + return 0;
> }
>
> /**
> --
> 2.43.0.rc1.1336.g36b5255a03ac
>