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

Bartosz Golaszewski
2024-10-16T08:52:52Z

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 {

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