Re: [libgpiod][PATCH] line-config.c: Fix library enum used for kernel flags bitfield

[Kent Gibson <warthog618@xxxxxxxxx>]

On Sat, Dec 30, 2023 at 01:48:53PM +0100, J.A. Bezemer wrote:
> Library enum was used to sanitize kernel flags.
>

Rephrase to make imperative and better describe what is being fixed.
e.g.

"Fix deselection of output direction when edge detection is selected in
make_kernel_flags(). Use correct flag to perform deselection rather than
a library enum."

If you do select both then the kernel will return an error when
the config is applied, so worst case outcome is a confusing errror.

> This will probably not have broken any "correct" usage: it would clear
> GPIO_V2_LINE_FLAG_USED, which is not used on setting, and
> GPIO_V2_LINE_FLAG_ACTIVE_LOW, which is set correctly later on.
>

It would be clearer to say there are no other visible side-effects, for
the reasons you list.

I'm just nitpicking the wording here - the patch itself looks good.
Well spotted.

Cheers,
Kent.

> Signed-off-by: Anne Bezemer <j.a.bezemer@xxxxxxxxxxxxxxxxxxxxx>
> ---
>  lib/line-config.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/lib/line-config.c b/lib/line-config.c
> index 2749a2a..9bf7734 100644
> --- a/lib/line-config.c
> +++ b/lib/line-config.c
> @@ -381,18 +381,18 @@ static uint64_t make_kernel_flags(struct gpiod_line_settings *settings)
>  	case GPIOD_LINE_EDGE_FALLING:
>  		flags |= (GPIO_V2_LINE_FLAG_EDGE_FALLING |
>  			  GPIO_V2_LINE_FLAG_INPUT);
> -		flags &= ~GPIOD_LINE_DIRECTION_OUTPUT;
> +		flags &= ~GPIO_V2_LINE_FLAG_OUTPUT;
>  		break;
>  	case GPIOD_LINE_EDGE_RISING:
>  		flags |= (GPIO_V2_LINE_FLAG_EDGE_RISING |
>  			  GPIO_V2_LINE_FLAG_INPUT);
> -		flags &= ~GPIOD_LINE_DIRECTION_OUTPUT;
> +		flags &= ~GPIO_V2_LINE_FLAG_OUTPUT;
>  		break;
>  	case GPIOD_LINE_EDGE_BOTH:
>  		flags |= (GPIO_V2_LINE_FLAG_EDGE_FALLING |
>  			  GPIO_V2_LINE_FLAG_EDGE_RISING |
>  			  GPIO_V2_LINE_FLAG_INPUT);
> -		flags &= ~GPIOD_LINE_DIRECTION_OUTPUT;
> +		flags &= ~GPIO_V2_LINE_FLAG_OUTPUT;
>  		break;
>  	default:
>  		break;
> --
> 2.30.2
>