Re: [PATCH v9 10/20] gpiolib: cdev: support GPIO_V2_LINE_SET_CONFIG_IOCTL

[Andy Shevchenko]

On Thu, Sep 24, 2020 at 12:26 PM Kent Gibson <warthog618@xxxxxxxxx> wrote:
> On Thu, Sep 24, 2020 at 11:26:49AM +0300, Andy Shevchenko wrote:
> > On Thu, Sep 24, 2020 at 6:24 AM Kent Gibson <warthog618@xxxxxxxxx> wrote:
> > > On Wed, Sep 23, 2020 at 07:15:46PM +0300, Andy Shevchenko wrote:
> > > > On Wed, Sep 23, 2020 at 7:14 PM Andy Shevchenko
> > > > <andy.shevchenko@xxxxxxxxx> wrote:
> > > > > On Tue, Sep 22, 2020 at 5:35 AM Kent Gibson <warthog618@xxxxxxxxx> wrote:

...

> > > > > > +               polarity_change =
> > > > > > +                       (test_bit(FLAG_ACTIVE_LOW, &desc->flags) !=
> > > > > > +                        ((flags & GPIO_V2_LINE_FLAG_ACTIVE_LOW) != 0));
> > > > >
> > > > > Comparison
> > > >
> > > > Comparison between int / long (not all archs are agreed on this) and
> > > > boolean is not the best we can do.
> > > >
> > >
> > > There is no bool to int comparision here.
> >
> > test_bit() returns int or long depending on arch... Then you compare
> > it to bool (which is a product of != 0).

> Really - I thought it returned bool.
> It is a test - why would it return int or long?

I assume due to arch relation. Some archs may convert test_bit() to a
single assembly instruction that returns a register which definitely
fits long or int depending on case.

> Surely it is guaranteed to return 0 or 1?

Not sure about this, it's all in arch/* and needs to be investigated.
Would be nice to have it cleaned up if there is any inconsistency (and
document if not yet). But It's out of scope of this series I believe.

> > > There are two comparisons - the inner int vs int => bool and the
> > > outer bool vs bool.  The "!= 0" is effectively an implicit cast to
> > > bool, as is your new_polarity initialisation below.
> > >
> > > > What about
> > > >
> > > >   bool old_polarity = test_bit(FLAG_ACTIVE_LOW, &desc->flags);
> > > >   bool new_polarity = flags & GPIO_V2_LINE_FLAG_ACTIVE_LOW;
> > > >
> > > >   old_polarity ^ new_polarity
> > >
> > > So using bitwise operators on bools is ok??
> >
> > XOR is special. There were never bitwise/boolean XORs.
> >
>
> We must live in different universes, cos there has been a bitwise XOR in
> mine since K&R.  The logical XOR is '!='.

Oops, you are right, It was never boolean XOR because it's the same as
a simple comparison.

...

> > > > and move this under INPUT conditional?
> > >
> > > It has to be before the gpio_v2_line_config_flags_to_desc_flags() call,
> > > as that modifies the desc flags, including the new polarity, so
> > > polarity_change would then always be false :-).
> >
> > I really don't see in the code how polarity_change value is used in
> > FLAG_OUTPUT branch below.
>
> It isn't.  But desc->flags is modified before both - and so the
> polarity_change initialization has to go before both SINCE IT TESTS
> THE FLAGS.

I see now. Sorry for being too blind.

-- 
With Best Regards,
Andy Shevchenko