| Message ID | 20180212124532.25776-1-linus.walleij@linaro.org |
|---|---|
| Headers | show |
| Series | Convert GPIO SPI to use descriptors only | expand |
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
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
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.
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
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