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

[Andy Shevchenko <andy.shevchenko@xxxxxxxxx>]

On Mon, Feb 12, 2018 at 2:45 PM, Linus Walleij <linus.walleij@xxxxxxxxxx> wrote:
> This converts the bit-banged GPIO SPI driver to looking up and
> using GPIO descriptors to get a handle on GPIO lines for SCK,
> MOSI, MISO and all CS lines.
>
> All existing board files are converted in one go to keep it all
> consistent. With these conversions I rarely find any interrim

interim

> steps that makes any sense.
>
> Device tree probing and GPIO handling should work like before
> also after this patch.
>
> For board files, we stop using controller data to pass the GPIO
> line for chip select, instead we pass this as a GPIO descriptor
> lookup like everything else.
>
> In some s3c24xx machines the names of the SPI devices were set to
> "spi-gpio" rather than "spi_gpio" which can never have worked, I
> fixed it working (I guess) as part of this patch set. Sometimes
> I wonder how this code got upstream in the first place, it
> obviously is not tested.
>
> mach-s3c64xx/mach-smartq.c has the same problem and additionally
> defines the *same* GPIO line for MOSI and MISO which is not going
> to be accepted by gpiolib. As the lines were number 1,2,2 I assumed
> it was a typo and use lines 1,2,3. A comment gives awat that line 0
> is chip select though no actual SPI device is provided for the LCD
> supposed to be on this bit-banged SPI bus. I left it intact instead
> of just deleting the bus though.
>
> Kill off board file code that try to initialize the SPI lines
> to the same values that they will later be set by the spi_gpio
> driver anyways. Given the huge number of weird things in these
> board files I do not think this code is very tested or put in
> with much afterthought anyways.
>
> In order to assert that we do not get performance regressions on
> this crucial bing-banged driver, a ran a script like this dumping the
> Ilitek ILI9322 regmap 10000 times (it has no caching obviously) on
> an otherwise idle system in two iterations before and after the
> patches:

> +               { },

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

> +               { },

> +               { },

> +               { },

> +               { },

> +               { },

> +               { },

> +               { },

>  #include <linux/gpio.h>
> +#include <linux/gpio/machine.h>

Do I understand correctly that gpio.h is left due to dependencies in
other parts of the code (here and in the rest of changed drivers)?

> +               { },

>  static void spi_gpio_chipselect(struct spi_device *spi, int is_active)
>  {
>         struct spi_gpio *spi_gpio = spi_to_spi_gpio(spi);
>
> +       /* set initial clock line level */
>         if (is_active)
> +               gpiod_set_value_cansleep(spi_gpio->sck, spi->mode & SPI_CPOL);
> +
> +       /* Drive chip select line, if we have one */
> +       if (spi_gpio->has_cs) {
> +               struct gpio_desc *cs = spi_gpio->cs_gpios[spi->chip_select];
>
> +               /* 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);

?

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

>         }
>  }

> +       spi_gpio->cs_gpios = devm_kzalloc(&pdev->dev,
> +                               pdata->num_chipselect * sizeof(*spi_gpio->cs_gpios),
> +                               GFP_KERNEL);

kcalloc

> +       if (!spi_gpio->cs_gpios)
> +               return -ENOMEM;

-- 
With Best Regards,
Andy Shevchenko
--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html