mbox series

[0/3] Convert GPIO SPI to use descriptors only

Message ID 20180212124532.25776-1-linus.walleij@linaro.org
Headers show
Series Convert GPIO SPI to use descriptors only | expand

Message

Linus Walleij Feb. 12, 2018, 12:45 p.m. UTC
This series now has all prerequisite changes in gpiolib
merged upstream so the conversion should be smooth.

The boardfile patches have been under review forever now,
so the maintainers either think it's OK or do not care.

I have confirmed clean build on a plethora of platforms
using the zeroday build system.

I think we can safely queue this for v4.17.

Linus Walleij (3):
  spi: spi-gpio: Rewrite to use GPIO descriptors
  spi: spi-gpio: Delete references to non-GENERIC_BITBANG
  spi: spi-gpio: Augment device tree bindings

 Documentation/devicetree/bindings/spi/spi-gpio.txt |  24 +-
 arch/arm/mach-pxa/cm-x300.c                        |  21 +-
 arch/arm/mach-pxa/raumfeld.c                       |  26 +-
 arch/arm/mach-s3c24xx/mach-jive.c                  |  55 ++--
 arch/arm/mach-s3c24xx/mach-qt2410.c                |  26 +-
 arch/arm/mach-s3c64xx/mach-smartq.c                |  22 +-
 arch/mips/alchemy/devboards/db1000.c               |  24 +-
 arch/mips/jz4740/board-qi_lb60.c                   |  26 +-
 drivers/misc/eeprom/digsy_mtc_eeprom.c             |  29 +-
 drivers/spi/spi-gpio.c                             | 298 +++++++--------------
 include/linux/spi/spi_gpio.h                       |  49 +---
 11 files changed, 280 insertions(+), 320 deletions(-)

-- 
2.14.3

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

Comments

Andy Shevchenko Feb. 13, 2018, 2:17 p.m. UTC | #1
On Mon, Feb 12, 2018 at 2:45 PM, Linus Walleij <linus.walleij@linaro.org> 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@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Andy Shevchenko Feb. 13, 2018, 2:19 p.m. UTC | #2
On Mon, Feb 12, 2018 at 2:45 PM, Linus Walleij <linus.walleij@linaro.org> wrote:
> This series now has all prerequisite changes in gpiolib

> merged upstream so the conversion should be smooth.

>

> The boardfile patches have been under review forever now,

> so the maintainers either think it's OK or do not care.

>

> I have confirmed clean build on a plethora of platforms

> using the zeroday build system.

>

> I think we can safely queue this for v4.17.

>


I think it makes sense to address my comments, after that FWIW:

Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>


> Linus Walleij (3):

>   spi: spi-gpio: Rewrite to use GPIO descriptors

>   spi: spi-gpio: Delete references to non-GENERIC_BITBANG

>   spi: spi-gpio: Augment device tree bindings

>

>  Documentation/devicetree/bindings/spi/spi-gpio.txt |  24 +-

>  arch/arm/mach-pxa/cm-x300.c                        |  21 +-

>  arch/arm/mach-pxa/raumfeld.c                       |  26 +-

>  arch/arm/mach-s3c24xx/mach-jive.c                  |  55 ++--

>  arch/arm/mach-s3c24xx/mach-qt2410.c                |  26 +-

>  arch/arm/mach-s3c64xx/mach-smartq.c                |  22 +-

>  arch/mips/alchemy/devboards/db1000.c               |  24 +-

>  arch/mips/jz4740/board-qi_lb60.c                   |  26 +-

>  drivers/misc/eeprom/digsy_mtc_eeprom.c             |  29 +-

>  drivers/spi/spi-gpio.c                             | 298 +++++++--------------

>  include/linux/spi/spi_gpio.h                       |  49 +---

>  11 files changed, 280 insertions(+), 320 deletions(-)

>

> --

> 2.14.3

>

> --

> To unsubscribe from this list: send the line "unsubscribe linux-spi" in

> the body of a message to majordomo@vger.kernel.org

> More majordomo info at  http://vger.kernel.org/majordomo-info.html




-- 
With Best Regards,
Andy Shevchenko
--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Mark Brown Feb. 13, 2018, 4:17 p.m. UTC | #3
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@linaro.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.
Rob Herring (Arm) Feb. 19, 2018, 2:35 a.m. UTC | #4
On Mon, Feb 12, 2018 at 01:45:32PM +0100, Linus Walleij wrote:
> After we augmented the core to handle "gpio-sck"/"sck-gpios",

> "gpio-mosi"/"mosi-gpios", "gpio-miso"/"miso-gpios" alike,

> deprecate the old binding and put the strict modern and

> recommended binding practice into place as the default for

> GPIO-based SPI.

> 

> This reflects the similar change in I2C:

> commit 7d29f509d2cf

> ("dt-bindings: i2c: i2c-gpio: Add support for named gpios")

> 

> Cc: devicetree@vger.kernel.org

> Cc: Rob Herring <robh@kernel.org>

> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>

> ---

>  Documentation/devicetree/bindings/spi/spi-gpio.txt | 24 ++++++++++++++++------

>  1 file changed, 18 insertions(+), 6 deletions(-)


Reviewed-by: Rob Herring <robh@kernel.org>

--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Linus Walleij Feb. 23, 2018, 10:36 a.m. UTC | #5
On Tue, Feb 13, 2018 at 3:17 PM, Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
> On Mon, Feb 12, 2018 at 2:45 PM, Linus Walleij <linus.walleij@linaro.org> wrote:


>>  #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)?


Yes, unless I missed something. When I edited the files I
searched for gpio_* (well in EMACS just CTRL+s "gpio_"...)
and very often it turns out that the board files grab a few
GPIOs and so something.

Some of that can probably go away with the use of GPIO hogs,
some of it needs proper conversion to gpio descriptors (sigh)
So I guess I will get to it in the next 20 years or so, plenty
of clean-up work for everyone ;)

>> +       /* 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);


This looks neat and Mark seems to like this pattern so I will
go for this!

>> +       spi_gpio->cs_gpios = devm_kzalloc(&pdev->dev,

>> +                               pdata->num_chipselect * sizeof(*spi_gpio->cs_gpios),

>> +                               GFP_KERNEL);

>

> kcalloc


Okay!

Linus Walleij
--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html