Re: [PATCH 1/3] spi: spi-gpio: Rewrite to use GPIO descriptors

[Mark Brown <broonie-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>]

[-- Attachment #1: Type: text/plain, Size: 1194 bytes --]

On Tue, Feb 13, 2018 at 04:17:38PM +0200, Andy Shevchenko wrote:
> On Mon, Feb 12, 2018 at 2:45 PM, Linus Walleij <linus.walleij-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> wrote:

> > +               { },

> As commented earlier to the similar change, terminator would terminate
> even at compile time.
> To achieve that just remove comma.

It's fine either way - not having the comma bothers some people just for
the consistency.

> > +               /* SPI chip selects are normally active-low */
> > +               gpiod_set_value_cansleep(cs, (spi->mode & SPI_CS_HIGH) ? is_active : !is_active);

>   bool invert = !(spi->mode & SPI_CS_HIGH);

>   gpiod_set_value_cansleep(cs, !!is_active ^ invert);

Neither of these options is a triumph of legibility but even with my
antipithy for the use of the ternery operator the new suggestion is
still not great - the combination of double negation and exclusive or
is really not super obvious what it's supposed to do.

> But I guess most used pattern is just explicit conditional

> if (spi->mode & SPI_CS_HIGH)
>         gpiod_set_value_cansleep(cs, is_active);
> else
>         gpiod_set_value_cansleep(cs, !is_active);

Yup, that's very clear.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]