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

[Bartosz Golaszewski <brgl@xxxxxxxx>]

On Wed, 16 Oct 2024 10:37:47 +0200, Kent Gibson <warthog618@xxxxxxxxx> 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@xxxxxxxxxx>
>> > >
>> > > -	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 {

>> 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?

> Similarly
>
> input -> input|edge|debounce
>
> which exercises a different path through cdev.
>
> Cheers,
> Kent.
>

Bart