Re: [PATCH v2] gpiolib: notify user-space about line state changes triggered by kernel

[Kent Gibson <warthog618@xxxxxxxxx>]

On Thu, Aug 24, 2023 at 10:55:44AM +0200, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <bartosz.golaszewski@xxxxxxxxxx>
> 
> We currently only emit CHANGED_CONFIG events when the user-space changes
> GPIO config. We won't be notified if changes come from in-kernel. Let's
> call the notifier chain whenever kernel users change direction or any of
> the active-low, debounce or consumer name settings. We don't notify the
> user-space about the persistence as the uAPI has no notion of it.
> 
> Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@xxxxxxxxxx>
> ---
> v1 -> v2:
> - use the gpiod_line_state_notify() helper
> - reorder the code in gpiod_set_debounce() for better readability
> 
>  drivers/gpio/gpiolib.c | 16 ++++++++++++++--
>  1 file changed, 14 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
> index 40a0022ea719..1cb7731550ca 100644
> --- a/drivers/gpio/gpiolib.c
> +++ b/drivers/gpio/gpiolib.c
> @@ -2439,6 +2439,7 @@ int gpiod_direction_input(struct gpio_desc *desc)
>  	}
>  	if (ret == 0) {
>  		clear_bit(FLAG_IS_OUT, &desc->flags);
> +		gpiod_line_state_notify(desc, GPIO_V2_LINE_CHANGED_CONFIG);
>  		ret = gpio_set_bias(desc);
>  	}
>  

gpiod_direction_input() is called by cdev when a line is requested.
So requesting a line now creates two events - REQUESTED and
HANGED_CONFIG? Even worse - it calls gpiod_direction_input() first, so we
will get CHANGED_CONFIG then REQUESTED??

And a config change from cdev can call this and then generate a
CHANGED_CONFIG event itself, so again double events.

Same for output and probably debounce too (that one is a bit more
convoluted).

Cheers,
Kent.

> @@ -2484,8 +2485,10 @@ static int gpiod_direction_output_raw_commit(struct gpio_desc *desc, int value)
>  		gc->set(gc, gpio_chip_hwgpio(desc), val);
>  	}
>  
> -	if (!ret)
> +	if (!ret) {
>  		set_bit(FLAG_IS_OUT, &desc->flags);
> +		gpiod_line_state_notify(desc, GPIO_V2_LINE_CHANGED_CONFIG);
> +	}
>  	trace_gpio_value(desc_to_gpio(desc), 0, val);
>  	trace_gpio_direction(desc_to_gpio(desc), 0, ret);
>  	return ret;
> @@ -2672,9 +2675,16 @@ EXPORT_SYMBOL_GPL(gpiod_set_config);
>  int gpiod_set_debounce(struct gpio_desc *desc, unsigned int debounce)
>  {
>  	unsigned long config;
> +	int ret;
>  
>  	config = pinconf_to_config_packed(PIN_CONFIG_INPUT_DEBOUNCE, debounce);
> -	return gpiod_set_config(desc, config);
> +	ret = gpiod_set_config(desc, config);
> +	if (ret)
> +		return ret;
> +
> +	gpiod_line_state_notify(desc, GPIO_V2_LINE_CHANGED_CONFIG);
> +
> +	return 0;
>  }
>  EXPORT_SYMBOL_GPL(gpiod_set_debounce);
>  
> @@ -2723,6 +2733,7 @@ void gpiod_toggle_active_low(struct gpio_desc *desc)
>  {
>  	VALIDATE_DESC_VOID(desc);
>  	change_bit(FLAG_ACTIVE_LOW, &desc->flags);
> +	gpiod_line_state_notify(desc, GPIO_V2_LINE_CHANGED_CONFIG);
>  }
>  EXPORT_SYMBOL_GPL(gpiod_toggle_active_low);
>  
> @@ -3330,6 +3341,7 @@ int gpiod_set_consumer_name(struct gpio_desc *desc, const char *name)
>  
>  	kfree_const(desc->label);
>  	desc_set_label(desc, name);
> +	gpiod_line_state_notify(desc, GPIO_V2_LINE_CHANGED_CONFIG);
>  
>  	return 0;
>  }
> -- 
> 2.39.2
>