Re: [PATCH v3 6/6] gpiolib: notify user-space about in-kernel line state changes

[Kent Gibson <warthog618@gmail.com>]

On Wed, Oct 16, 2024 at 01:52:49AM -0700, Bartosz Golaszewski wrote:
> On Wed, 16 Oct 2024 10:37:47 +0200, Kent Gibson <warthog618@gmail.com> said:
> > On Wed, Oct 16, 2024 at 03:27:30PM +0800, Kent Gibson wrote:
> >> On Wed, Oct 16, 2024 at 01:19:44PM +0800, Kent Gibson wrote:
> >> > On Tue, Oct 15, 2024 at 12:56:18PM +0200, Bartosz Golaszewski wrote:
> >> > > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> >> > >
> >> > > -	return gpio_do_set_config(guard.gc, gpio_chip_hwgpio(desc), config);
> >> > > +	ret = gpio_do_set_config(guard.gc, gpio_chip_hwgpio(desc), config);
> >> > > +	if (ret == 0) {
> >> > > +		/* These are the only options we notify the userspace about. */
> >> > > +		switch (pinconf_to_config_param(config)) {
> >> > > +		case PIN_CONFIG_BIAS_DISABLE:
> >> > > +		case PIN_CONFIG_BIAS_PULL_DOWN:
> >> > > +		case PIN_CONFIG_BIAS_PULL_UP:
> >> > > +		case PIN_CONFIG_DRIVE_OPEN_DRAIN:
> >> > > +		case PIN_CONFIG_DRIVE_OPEN_SOURCE:
> >> > > +		case PIN_CONFIG_DRIVE_PUSH_PULL:
> >> > > +		case PIN_CONFIG_INPUT_DEBOUNCE:
> >> > > +			gpiod_line_state_notify(desc,
> >> > > +						GPIO_V2_LINE_CHANGED_CONFIG);
> >> > > +			break;
> >> > > +		default:
> >> > > +			break;
> >> > > +		}
> >> > > +	}
> >> > > +
> >> > > +	return ret;
> >> > >  }
> >> >
> >> > Ah, the debounce - I forgot about that, and other features that cdev
> >> > might emulate.
> >> >
> >> > What happens if userspace requests a line with debounce that is
> >> > supported by hardware?  Seems to me we'll see both a LINE_REQUESTED and a
> >> > LINE_CONFIG_CHANGED when the line is requested.
> >> >
> >>
> >> This is problematic for me to test at the moment, as gpiosim doesn't support
> >> debounce. Any chance we could make that configurable?  Similarly drive.
> >>
> >> > Conversely, what if a config change impacts features that don't result in a
> >> > notification from gpiod_set_config(), like active low, or emulated
> >> > drive or debounce?
> >> >
> >>
> >> Bah, drive is emulated in gpiolib itself, so that should be fine.
> >>
> >> When changing config cdev always calls gpiod_direction_input/output(), so I
> >> think that covers the active low case.
> >>
> >> But I have a test taking a line from input to output|open_drain and I
> >> get two change events.  The first is the most interesting as it reports
> >> input|open_drain, the second then reports output|open_drain.
> >> That is due to gpiod_direction_output() calling gpiod_set_config() to
> >> set the drive, and later to set the direction, in that order.
> >> Given it will be setting the direction, it should inhibit the event from
> >> the drive setting?
> >>
> >
> > Ok, I oversimplified, it was actually
> >
> > input -> output|active_low|open_drain
> >
> > and
> >
> > input -> output|open_source
> >
> > fails the same way.
> >
>
> Does the following help?
>
> @@ -2830,7 +2860,7 @@ int gpiod_direction_output(struct gpio_desc
> *desc, int value)
>  			goto set_output_value;
>  		/* Emulate open drain by not actively driving the line high */
>  		if (value) {
> -			ret = gpiod_direction_input(desc);
> +			ret = gpiod_direction_input_nonotify(desc);
>  			goto set_output_flag;
>  		}
>  	} else if (test_bit(FLAG_OPEN_SOURCE, &flags)) {
> @@ -2839,7 +2869,7 @@ int gpiod_direction_output(struct gpio_desc
> *desc, int value)
>  			goto set_output_value;
>  		/* Emulate open source by not actively driving the line low */
>  		if (!value) {
> -			ret = gpiod_direction_input(desc);
> +			ret = gpiod_direction_input_nonotify(desc);
>  			goto set_output_flag;
>  		}
>  	} else {
>

That fixes the drive problems.

> >> Still haven't tested any debounce changes...
> >>
> >
> > Have now.
> >
> > input -> input|debounce
> >
> > does not report the debounce.  Only get the one event and it does not
> > contain any debounce.
> >
>
> You mean, you get a CHANGED_CONFIG event but the debounce value is not
> in the associated line info?
>

Correct.

Cheers,
Kent.