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

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

[J.A. Bezemer]

Library enum was used to sanitize kernel flags.

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.

Signed-off-by: Anne Bezemer <j.a.bezemer@opensourcepartners.nl>
---
  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

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

[Kent Gibson]

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@opensourcepartners.nl>
> ---
>  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
>

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

[Kent Gibson]

On Sun, Dec 31, 2023 at 09:57:27AM +0800, Kent Gibson wrote:
> 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."
>

While I think if it, that also applies to the subject line.
And that should probably begin with "core:" as it indicates the section
of the code tree being modified.

Sorry - more nitpicking.

Cheers,
Kent.

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

[J.A. Bezemer]

On Sun, 31 Dec 2023, Kent Gibson wrote:
> On Sun, Dec 31, 2023 at 09:57:27AM +0800, Kent Gibson wrote:
>> 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."
>>
>
> While I think if it, that also applies to the subject line.
> And that should probably begin with "core:" as it indicates the section
> of the code tree being modified.
>
> Sorry - more nitpicking.

Hi Kent, thanks for the guidance. Keeping a high standard is very much
appreciated. Updated version upcoming.

Regards,
Anne Bezemer