| Message ID | 20230929-pxa-gpio-v3-5-af8d5e5d1f34@skole.hr |
|---|---|
| State | New |
| Headers | show |
| Series | ARM: pxa: GPIO descriptor conversions | expand |
Hi Duje, thanks for your patch! On Fri, Sep 29, 2023 at 3:15 PM Duje Mihanović <duje.mihanovic@skole.hr> wrote: > Sharp's Spitz still uses the legacy GPIO interface in its > wait_for_hsync() function. > > Convert it to use the GPIO descriptor interface. > > Signed-off-by: Duje Mihanović <duje.mihanovic@skole.hr> Overall this looks fine, but can't help but notice: > static void spitz_ads7846_wait_for_hsync(void) > { > - while (gpio_get_value(SPITZ_GPIO_HSYNC)) > + while (gpiod_get_value(hsync)) > cpu_relax(); Waits while the line is high... > - while (!gpio_get_value(SPITZ_GPIO_HSYNC)) > + while (!gpiod_get_value(hsync)) > cpu_relax(); Then as it goes low, waits for it to go high again. So the hsync signal is *active* when it is *low*. > @@ -543,6 +545,8 @@ static struct gpiod_lookup_table spitz_ads7846_gpio_table = { > .table = { > GPIO_LOOKUP("gpio-pxa", SPITZ_GPIO_TP_INT, > "pendown", GPIO_ACTIVE_LOW), > + GPIO_LOOKUP("gpio-pxa", SPITZ_GPIO_HSYNC, > + "hsync", GPIO_ACTIVE_LOW), Which is what you appropriately flag it for. BUT: the signal is now inverted in gpiolib, so the spitz_ads7846_wait_for_hsync() loops needs to be rewritten inverted, because the value from gpiod_get_value() now means "asserted" if high. /* Wait while de-asserted */ while (!gpiod_get_value(hsync)) cpu_relax(); /* Wait while asserted */ while (gpiod_get_value(hsync)) cpu_relax(); return; Right? > @@ -622,8 +626,13 @@ static void __init spitz_spi_init(void) > > gpiod_add_lookup_table(&spitz_ads7846_gpio_table); > gpiod_add_lookup_table(&spitz_spi_gpio_table); > + hsync = gpiod_get(NULL, "hsync", GPIOD_IN); You are getting the gpiod from device NULL which is probably correct when you do this in the board file. But the spitz_ads7846_gpio_table where you put the descriptor is associated with the ads7846 device so this will not work. You either have to add a one-gpio table just for this, or (better) move the whole spitz_ads7846_wait_for_hsync() down into the touchscreen driver instead, so the device driver can (optionally) obtain this gpio and deal with it. Which is easy because: [linus@Fecusia linux-nomadik]$ git grep ads7846_platform_data Documentation/spi/spi-summary.rst: static struct ads7846_platform_data ads_info = { arch/arm/mach-pxa/spitz.c:static struct ads7846_platform_data spitz_ads7846_info = { arch/mips/alchemy/devboards/db1000.c:static struct ads7846_platform_data db1100_touch_pd = { arch/powerpc/platforms/512x/pdm360ng.c:static struct ads7846_platform_data pdm360ng_ads7846_pdata = { Only three users in the kernel, and sptiz is the only one using the void (*wait_for_sync)(void) callback in ads7846_platform_data! So delete that callback from ads7846_platform_data in include/linux/spi/ads7846.h and augment drivers/input/touchscreen/ads7846.c to get the GPIO optionally with gpiod_get_optional() from the device, then copy the code from spitz_ads7846_wait_for_hsync() right into the driver and make sure it gets called if and only if the "hsync" gpio exists. Yours, Linus Walleij
diff --git a/arch/arm/mach-pxa/spitz.c b/arch/arm/mach-pxa/spitz.c index c789eeaf3c2c..25878daec986 100644 --- a/arch/arm/mach-pxa/spitz.c +++ b/arch/arm/mach-pxa/spitz.c @@ -520,12 +520,14 @@ static inline void spitz_leds_init(void) {} * SSP Devices ******************************************************************************/ #if defined(CONFIG_SPI_PXA2XX) || defined(CONFIG_SPI_PXA2XX_MODULE) +static struct gpio_desc *hsync; + static void spitz_ads7846_wait_for_hsync(void) { - while (gpio_get_value(SPITZ_GPIO_HSYNC)) + while (gpiod_get_value(hsync)) cpu_relax(); - while (!gpio_get_value(SPITZ_GPIO_HSYNC)) + while (!gpiod_get_value(hsync)) cpu_relax(); } @@ -543,6 +545,8 @@ static struct gpiod_lookup_table spitz_ads7846_gpio_table = { .table = { GPIO_LOOKUP("gpio-pxa", SPITZ_GPIO_TP_INT, "pendown", GPIO_ACTIVE_LOW), + GPIO_LOOKUP("gpio-pxa", SPITZ_GPIO_HSYNC, + "hsync", GPIO_ACTIVE_LOW), { } }, }; @@ -622,8 +626,13 @@ static void __init spitz_spi_init(void) gpiod_add_lookup_table(&spitz_ads7846_gpio_table); gpiod_add_lookup_table(&spitz_spi_gpio_table); + hsync = gpiod_get(NULL, "hsync", GPIOD_IN); pxa2xx_set_spi_info(2, &spitz_spi_info); - spi_register_board_info(ARRAY_AND_SIZE(spitz_spi_devices)); + if (IS_ERR(hsync)) { + pr_err("Failed to get hsync GPIO: %ld\n", PTR_ERR(hsync)); + spi_register_board_info(ARRAY_AND_SIZE(&spitz_spi_devices[1])); + } else + spi_register_board_info(ARRAY_AND_SIZE(spitz_spi_devices)); } #else static inline void spitz_spi_init(void) {}
Sharp's Spitz still uses the legacy GPIO interface in its wait_for_hsync() function. Convert it to use the GPIO descriptor interface. Signed-off-by: Duje Mihanović <duje.mihanovic@skole.hr> --- arch/arm/mach-pxa/spitz.c | 15 ++++++++++++--- 1 file changed, 12 insertions(+), 3 deletions(-)