This series fixes a number of GPIO handling issues after converting this driver to use descriptors.
The series has been tested on HX8347d display with parallel interface. Without first patch it's not working.
In v3: - added staging prefix (Fabio) - slightly amended commit message in the patch 1 - added Rb tag (Phil) - dropped Fixes tag from the patch 2 (Greg)
Andy Shevchenko (4): staging: fbtft: Rectify GPIO handling staging: fbtft: Replace custom ->reset() with generic one staging: fbtft: Don't spam logs when probe is deferred staging: fbtft: Update TODO
drivers/staging/fbtft/TODO | 5 ----- drivers/staging/fbtft/fb_agm1264k-fl.c | 30 +++++++------------------- drivers/staging/fbtft/fb_bd663474.c | 4 ---- drivers/staging/fbtft/fb_ili9163.c | 4 ---- drivers/staging/fbtft/fb_ili9320.c | 1 - drivers/staging/fbtft/fb_ili9325.c | 4 ---- drivers/staging/fbtft/fb_ili9340.c | 1 - drivers/staging/fbtft/fb_s6d1121.c | 4 ---- drivers/staging/fbtft/fb_sh1106.c | 1 - drivers/staging/fbtft/fb_ssd1289.c | 4 ---- drivers/staging/fbtft/fb_ssd1325.c | 2 -- drivers/staging/fbtft/fb_ssd1331.c | 6 ++---- drivers/staging/fbtft/fb_ssd1351.c | 1 - drivers/staging/fbtft/fb_upd161704.c | 4 ---- drivers/staging/fbtft/fb_watterott.c | 1 - drivers/staging/fbtft/fbtft-bus.c | 3 +-- drivers/staging/fbtft/fbtft-core.c | 25 +++++++++------------ drivers/staging/fbtft/fbtft-io.c | 12 +++++------ 18 files changed, 27 insertions(+), 85 deletions(-)
The infamous commit c440eee1a7a1 ("Staging: staging: fbtft: Switch to the GPIO descriptor interface") broke GPIO handling completely. It has already four commits to rectify and it seems not enough. In order to fix the mess here we:
1) Set default to "inactive" for all requested pins
2) Fix CS#, RD#, and WR# pins polarity since it's active low and GPIO descriptor interface takes it into consideration from the Device Tree or ACPI
3) Consolidate chip activation (CS# assertion) under default ->reset() callback
To summarize the expectations about polarity for GPIOs:
RD# Low WR# Low CS# Low RESET# Low DC or RS High RW High Data 0 .. 15 High
See also Adafruit learning course [1] for the example of the schematics.
While at it, drop unneeded NULL checks, since GPIO API is tolerant to that.
[1]: https://learn.adafruit.com/adafruit-2-8-and-3-2-color-tft-touchscreen-breako...
Fixes: 92e3e884887c ("Staging: staging: fbtft: Fix GPIO handling") Fixes: b918d1c27066 ("Staging: staging: fbtft: Fix reset assertion when using gpio descriptor") Fixes: dbc4f989c878 ("Staging: staging: fbtft: Fix probing of gpio descriptor") Fixes: c440eee1a7a1 ("Staging: staging: fbtft: Switch to the gpio descriptor interface") Cc: Jan Sebastian Götte linux@jaseg.net Cc: Nishad Kamdar nishadkamdar@gmail.com Signed-off-by: Andy Shevchenko andriy.shevchenko@linux.intel.com Reviewed-by: Phil Reid preid@electromag.com.au --- drivers/staging/fbtft/fb_agm1264k-fl.c | 20 ++++++++++---------- drivers/staging/fbtft/fb_bd663474.c | 4 ---- drivers/staging/fbtft/fb_ili9163.c | 4 ---- drivers/staging/fbtft/fb_ili9320.c | 1 - drivers/staging/fbtft/fb_ili9325.c | 4 ---- drivers/staging/fbtft/fb_ili9340.c | 1 - drivers/staging/fbtft/fb_s6d1121.c | 4 ---- drivers/staging/fbtft/fb_sh1106.c | 1 - drivers/staging/fbtft/fb_ssd1289.c | 4 ---- drivers/staging/fbtft/fb_ssd1325.c | 2 -- drivers/staging/fbtft/fb_ssd1331.c | 6 ++---- drivers/staging/fbtft/fb_ssd1351.c | 1 - drivers/staging/fbtft/fb_upd161704.c | 4 ---- drivers/staging/fbtft/fb_watterott.c | 1 - drivers/staging/fbtft/fbtft-bus.c | 3 +-- drivers/staging/fbtft/fbtft-core.c | 13 ++++++------- drivers/staging/fbtft/fbtft-io.c | 12 ++++++------ 17 files changed, 25 insertions(+), 60 deletions(-)
diff --git a/drivers/staging/fbtft/fb_agm1264k-fl.c b/drivers/staging/fbtft/fb_agm1264k-fl.c index eeeeec97ad27..b545c2ca80a4 100644 --- a/drivers/staging/fbtft/fb_agm1264k-fl.c +++ b/drivers/staging/fbtft/fb_agm1264k-fl.c @@ -84,9 +84,9 @@ static void reset(struct fbtft_par *par)
dev_dbg(par->info->device, "%s()\n", __func__);
- gpiod_set_value(par->gpio.reset, 0); - udelay(20); gpiod_set_value(par->gpio.reset, 1); + udelay(20); + gpiod_set_value(par->gpio.reset, 0); mdelay(120); }
@@ -194,12 +194,12 @@ static void write_reg8_bus8(struct fbtft_par *par, int len, ...) /* select chip */ if (*buf) { /* cs1 */ - gpiod_set_value(par->CS0, 1); - gpiod_set_value(par->CS1, 0); - } else { - /* cs0 */ gpiod_set_value(par->CS0, 0); gpiod_set_value(par->CS1, 1); + } else { + /* cs0 */ + gpiod_set_value(par->CS0, 1); + gpiod_set_value(par->CS1, 0); }
gpiod_set_value(par->RS, 0); /* RS->0 (command mode) */ @@ -397,8 +397,8 @@ static int write_vmem(struct fbtft_par *par, size_t offset, size_t len) } kfree(convert_buf);
- gpiod_set_value(par->CS0, 1); - gpiod_set_value(par->CS1, 1); + gpiod_set_value(par->CS0, 0); + gpiod_set_value(par->CS1, 0);
return ret; } @@ -419,10 +419,10 @@ static int write(struct fbtft_par *par, void *buf, size_t len) for (i = 0; i < 8; ++i) gpiod_set_value(par->gpio.db[i], data & (1 << i)); /* set E */ - gpiod_set_value(par->EPIN, 1); + gpiod_set_value(par->EPIN, 0); udelay(5); /* unset E - write */ - gpiod_set_value(par->EPIN, 0); + gpiod_set_value(par->EPIN, 1); udelay(1); }
diff --git a/drivers/staging/fbtft/fb_bd663474.c b/drivers/staging/fbtft/fb_bd663474.c index e2c7646588f8..1629c2c440a9 100644 --- a/drivers/staging/fbtft/fb_bd663474.c +++ b/drivers/staging/fbtft/fb_bd663474.c @@ -12,7 +12,6 @@ #include <linux/module.h> #include <linux/kernel.h> #include <linux/init.h> -#include <linux/gpio/consumer.h> #include <linux/delay.h>
#include "fbtft.h" @@ -24,9 +23,6 @@
static int init_display(struct fbtft_par *par) { - if (par->gpio.cs) - gpiod_set_value(par->gpio.cs, 0); /* Activate chip */ - par->fbtftops.reset(par);
/* Initialization sequence from Lib_UTFT */ diff --git a/drivers/staging/fbtft/fb_ili9163.c b/drivers/staging/fbtft/fb_ili9163.c index 05648c3ffe47..6582a2c90aaf 100644 --- a/drivers/staging/fbtft/fb_ili9163.c +++ b/drivers/staging/fbtft/fb_ili9163.c @@ -11,7 +11,6 @@ #include <linux/module.h> #include <linux/kernel.h> #include <linux/init.h> -#include <linux/gpio/consumer.h> #include <linux/delay.h> #include <video/mipi_display.h>
@@ -77,9 +76,6 @@ static int init_display(struct fbtft_par *par) { par->fbtftops.reset(par);
- if (par->gpio.cs) - gpiod_set_value(par->gpio.cs, 0); /* Activate chip */ - write_reg(par, MIPI_DCS_SOFT_RESET); /* software reset */ mdelay(500); write_reg(par, MIPI_DCS_EXIT_SLEEP_MODE); /* exit sleep */ diff --git a/drivers/staging/fbtft/fb_ili9320.c b/drivers/staging/fbtft/fb_ili9320.c index f2e72d14431d..a8f4c618b754 100644 --- a/drivers/staging/fbtft/fb_ili9320.c +++ b/drivers/staging/fbtft/fb_ili9320.c @@ -8,7 +8,6 @@ #include <linux/module.h> #include <linux/kernel.h> #include <linux/init.h> -#include <linux/gpio/consumer.h> #include <linux/spi/spi.h> #include <linux/delay.h>
diff --git a/drivers/staging/fbtft/fb_ili9325.c b/drivers/staging/fbtft/fb_ili9325.c index c9aa4cb43123..16d3b17ca279 100644 --- a/drivers/staging/fbtft/fb_ili9325.c +++ b/drivers/staging/fbtft/fb_ili9325.c @@ -10,7 +10,6 @@ #include <linux/module.h> #include <linux/kernel.h> #include <linux/init.h> -#include <linux/gpio/consumer.h> #include <linux/delay.h>
#include "fbtft.h" @@ -85,9 +84,6 @@ static int init_display(struct fbtft_par *par) { par->fbtftops.reset(par);
- if (par->gpio.cs) - gpiod_set_value(par->gpio.cs, 0); /* Activate chip */ - bt &= 0x07; vc &= 0x07; vrh &= 0x0f; diff --git a/drivers/staging/fbtft/fb_ili9340.c b/drivers/staging/fbtft/fb_ili9340.c index 415183c7054a..704236bcaf3f 100644 --- a/drivers/staging/fbtft/fb_ili9340.c +++ b/drivers/staging/fbtft/fb_ili9340.c @@ -8,7 +8,6 @@ #include <linux/module.h> #include <linux/kernel.h> #include <linux/init.h> -#include <linux/gpio/consumer.h> #include <linux/delay.h> #include <video/mipi_display.h>
diff --git a/drivers/staging/fbtft/fb_s6d1121.c b/drivers/staging/fbtft/fb_s6d1121.c index 8c7de3290343..62f27172f844 100644 --- a/drivers/staging/fbtft/fb_s6d1121.c +++ b/drivers/staging/fbtft/fb_s6d1121.c @@ -12,7 +12,6 @@ #include <linux/module.h> #include <linux/kernel.h> #include <linux/init.h> -#include <linux/gpio/consumer.h> #include <linux/delay.h>
#include "fbtft.h" @@ -29,9 +28,6 @@ static int init_display(struct fbtft_par *par) { par->fbtftops.reset(par);
- if (par->gpio.cs) - gpiod_set_value(par->gpio.cs, 0); /* Activate chip */ - /* Initialization sequence from Lib_UTFT */
write_reg(par, 0x0011, 0x2004); diff --git a/drivers/staging/fbtft/fb_sh1106.c b/drivers/staging/fbtft/fb_sh1106.c index 6f7249493ea3..7b9ab39e1c1a 100644 --- a/drivers/staging/fbtft/fb_sh1106.c +++ b/drivers/staging/fbtft/fb_sh1106.c @@ -9,7 +9,6 @@ #include <linux/module.h> #include <linux/kernel.h> #include <linux/init.h> -#include <linux/gpio/consumer.h> #include <linux/delay.h>
#include "fbtft.h" diff --git a/drivers/staging/fbtft/fb_ssd1289.c b/drivers/staging/fbtft/fb_ssd1289.c index 7a3fe022cc69..f27bab38b3ec 100644 --- a/drivers/staging/fbtft/fb_ssd1289.c +++ b/drivers/staging/fbtft/fb_ssd1289.c @@ -10,7 +10,6 @@ #include <linux/module.h> #include <linux/kernel.h> #include <linux/init.h> -#include <linux/gpio/consumer.h>
#include "fbtft.h"
@@ -28,9 +27,6 @@ static int init_display(struct fbtft_par *par) { par->fbtftops.reset(par);
- if (par->gpio.cs) - gpiod_set_value(par->gpio.cs, 0); /* Activate chip */ - write_reg(par, 0x00, 0x0001); write_reg(par, 0x03, 0xA8A4); write_reg(par, 0x0C, 0x0000); diff --git a/drivers/staging/fbtft/fb_ssd1325.c b/drivers/staging/fbtft/fb_ssd1325.c index 8a3140d41d8b..796a2ac3e194 100644 --- a/drivers/staging/fbtft/fb_ssd1325.c +++ b/drivers/staging/fbtft/fb_ssd1325.c @@ -35,8 +35,6 @@ static int init_display(struct fbtft_par *par) { par->fbtftops.reset(par);
- gpiod_set_value(par->gpio.cs, 0); - write_reg(par, 0xb3); write_reg(par, 0xf0); write_reg(par, 0xae); diff --git a/drivers/staging/fbtft/fb_ssd1331.c b/drivers/staging/fbtft/fb_ssd1331.c index 37622c9462aa..ec5eced7f8cb 100644 --- a/drivers/staging/fbtft/fb_ssd1331.c +++ b/drivers/staging/fbtft/fb_ssd1331.c @@ -81,8 +81,7 @@ static void write_reg8_bus8(struct fbtft_par *par, int len, ...) va_start(args, len);
*buf = (u8)va_arg(args, unsigned int); - if (par->gpio.dc) - gpiod_set_value(par->gpio.dc, 0); + gpiod_set_value(par->gpio.dc, 0); ret = par->fbtftops.write(par, par->buf, sizeof(u8)); if (ret < 0) { va_end(args); @@ -104,8 +103,7 @@ static void write_reg8_bus8(struct fbtft_par *par, int len, ...) return; } } - if (par->gpio.dc) - gpiod_set_value(par->gpio.dc, 1); + gpiod_set_value(par->gpio.dc, 1); va_end(args); }
diff --git a/drivers/staging/fbtft/fb_ssd1351.c b/drivers/staging/fbtft/fb_ssd1351.c index 900b28d826b2..cf263a58a148 100644 --- a/drivers/staging/fbtft/fb_ssd1351.c +++ b/drivers/staging/fbtft/fb_ssd1351.c @@ -2,7 +2,6 @@ #include <linux/module.h> #include <linux/kernel.h> #include <linux/init.h> -#include <linux/gpio/consumer.h> #include <linux/spi/spi.h> #include <linux/delay.h>
diff --git a/drivers/staging/fbtft/fb_upd161704.c b/drivers/staging/fbtft/fb_upd161704.c index c77832ae5e5b..c680160d6380 100644 --- a/drivers/staging/fbtft/fb_upd161704.c +++ b/drivers/staging/fbtft/fb_upd161704.c @@ -12,7 +12,6 @@ #include <linux/module.h> #include <linux/kernel.h> #include <linux/init.h> -#include <linux/gpio/consumer.h> #include <linux/delay.h>
#include "fbtft.h" @@ -26,9 +25,6 @@ static int init_display(struct fbtft_par *par) { par->fbtftops.reset(par);
- if (par->gpio.cs) - gpiod_set_value(par->gpio.cs, 0); /* Activate chip */ - /* Initialization sequence from Lib_UTFT */
/* register reset */ diff --git a/drivers/staging/fbtft/fb_watterott.c b/drivers/staging/fbtft/fb_watterott.c index 76b25df376b8..a57e1f4feef3 100644 --- a/drivers/staging/fbtft/fb_watterott.c +++ b/drivers/staging/fbtft/fb_watterott.c @@ -8,7 +8,6 @@ #include <linux/module.h> #include <linux/kernel.h> #include <linux/init.h> -#include <linux/gpio/consumer.h> #include <linux/delay.h>
#include "fbtft.h" diff --git a/drivers/staging/fbtft/fbtft-bus.c b/drivers/staging/fbtft/fbtft-bus.c index 63c65dd67b17..3d422bc11641 100644 --- a/drivers/staging/fbtft/fbtft-bus.c +++ b/drivers/staging/fbtft/fbtft-bus.c @@ -135,8 +135,7 @@ int fbtft_write_vmem16_bus8(struct fbtft_par *par, size_t offset, size_t len) remain = len / 2; vmem16 = (u16 *)(par->info->screen_buffer + offset);
- if (par->gpio.dc) - gpiod_set_value(par->gpio.dc, 1); + gpiod_set_value(par->gpio.dc, 1);
/* non buffered write */ if (!par->txbuf.buf) diff --git a/drivers/staging/fbtft/fbtft-core.c b/drivers/staging/fbtft/fbtft-core.c index 4f362dad4436..67c3b1975a4d 100644 --- a/drivers/staging/fbtft/fbtft-core.c +++ b/drivers/staging/fbtft/fbtft-core.c @@ -38,8 +38,7 @@ int fbtft_write_buf_dc(struct fbtft_par *par, void *buf, size_t len, int dc) { int ret;
- if (par->gpio.dc) - gpiod_set_value(par->gpio.dc, dc); + gpiod_set_value(par->gpio.dc, dc);
ret = par->fbtftops.write(par, buf, len); if (ret < 0) @@ -79,7 +78,7 @@ static int fbtft_request_one_gpio(struct fbtft_par *par, int ret = 0;
*gpiop = devm_gpiod_get_index_optional(dev, name, index, - GPIOD_OUT_HIGH); + GPIOD_OUT_LOW); if (IS_ERR(*gpiop)) { ret = PTR_ERR(*gpiop); dev_err(dev, @@ -226,11 +225,15 @@ static void fbtft_reset(struct fbtft_par *par) { if (!par->gpio.reset) return; + fbtft_par_dbg(DEBUG_RESET, par, "%s()\n", __func__); + gpiod_set_value_cansleep(par->gpio.reset, 1); usleep_range(20, 40); gpiod_set_value_cansleep(par->gpio.reset, 0); msleep(120); + + gpiod_set_value_cansleep(par->gpio.cs, 1); /* Activate chip */ }
static void fbtft_update_display(struct fbtft_par *par, unsigned int start_line, @@ -922,8 +925,6 @@ static int fbtft_init_display_from_property(struct fbtft_par *par) goto out_free;
par->fbtftops.reset(par); - if (par->gpio.cs) - gpiod_set_value(par->gpio.cs, 0); /* Activate chip */
index = -1; val = values[++index]; @@ -1018,8 +1019,6 @@ int fbtft_init_display(struct fbtft_par *par) }
par->fbtftops.reset(par); - if (par->gpio.cs) - gpiod_set_value(par->gpio.cs, 0); /* Activate chip */
i = 0; while (i < FBTFT_MAX_INIT_SEQUENCE) { diff --git a/drivers/staging/fbtft/fbtft-io.c b/drivers/staging/fbtft/fbtft-io.c index 0863d257d762..de1904a443c2 100644 --- a/drivers/staging/fbtft/fbtft-io.c +++ b/drivers/staging/fbtft/fbtft-io.c @@ -142,12 +142,12 @@ int fbtft_write_gpio8_wr(struct fbtft_par *par, void *buf, size_t len) data = *(u8 *)buf;
/* Start writing by pulling down /WR */ - gpiod_set_value(par->gpio.wr, 0); + gpiod_set_value(par->gpio.wr, 1);
/* Set data */ #ifndef DO_NOT_OPTIMIZE_FBTFT_WRITE_GPIO if (data == prev_data) { - gpiod_set_value(par->gpio.wr, 0); /* used as delay */ + gpiod_set_value(par->gpio.wr, 1); /* used as delay */ } else { for (i = 0; i < 8; i++) { if ((data & 1) != (prev_data & 1)) @@ -165,7 +165,7 @@ int fbtft_write_gpio8_wr(struct fbtft_par *par, void *buf, size_t len) #endif
/* Pullup /WR */ - gpiod_set_value(par->gpio.wr, 1); + gpiod_set_value(par->gpio.wr, 0);
#ifndef DO_NOT_OPTIMIZE_FBTFT_WRITE_GPIO prev_data = *(u8 *)buf; @@ -192,12 +192,12 @@ int fbtft_write_gpio16_wr(struct fbtft_par *par, void *buf, size_t len) data = *(u16 *)buf;
/* Start writing by pulling down /WR */ - gpiod_set_value(par->gpio.wr, 0); + gpiod_set_value(par->gpio.wr, 1);
/* Set data */ #ifndef DO_NOT_OPTIMIZE_FBTFT_WRITE_GPIO if (data == prev_data) { - gpiod_set_value(par->gpio.wr, 0); /* used as delay */ + gpiod_set_value(par->gpio.wr, 1); /* used as delay */ } else { for (i = 0; i < 16; i++) { if ((data & 1) != (prev_data & 1)) @@ -215,7 +215,7 @@ int fbtft_write_gpio16_wr(struct fbtft_par *par, void *buf, size_t len) #endif
/* Pullup /WR */ - gpiod_set_value(par->gpio.wr, 1); + gpiod_set_value(par->gpio.wr, 0);
#ifndef DO_NOT_OPTIMIZE_FBTFT_WRITE_GPIO prev_data = *(u16 *)buf;
On Wed, Apr 28, 2021 at 04:04:12PM +0300, Andy Shevchenko wrote:
The infamous commit c440eee1a7a1 ("Staging: staging: fbtft: Switch to the GPIO descriptor interface") broke GPIO handling completely. It has already four commits to rectify and it seems not enough. In order to fix the mess here we:
Set default to "inactive" for all requested pins
Fix CS#, RD#, and WR# pins polarity since it's active low and GPIO descriptor interface takes it into consideration from the Device Tree or ACPI
Consolidate chip activation (CS# assertion) under default ->reset() callback
To summarize the expectations about polarity for GPIOs:
RD# Low WR# Low CS# Low RESET# Low DC or RS High RW High Data 0 .. 15 High
See also Adafruit learning course [1] for the example of the schematics.
While at it, drop unneeded NULL checks, since GPIO API is tolerant to that.
Fixes: 92e3e884887c ("Staging: staging: fbtft: Fix GPIO handling") Fixes: b918d1c27066 ("Staging: staging: fbtft: Fix reset assertion when using gpio descriptor") Fixes: dbc4f989c878 ("Staging: staging: fbtft: Fix probing of gpio descriptor") Fixes: c440eee1a7a1 ("Staging: staging: fbtft: Switch to the gpio descriptor interface")
I get the following error when trying to apply this:
Fixes tag: Fixes: 92e3e884887c ("Staging: staging: fbtft: Fix GPIO handling") Has these problem(s): - Subject does not match target commit subject Just use git log -1 --format='Fixes: %h ("%s")' Fixes tag: Fixes: b918d1c27066 ("Staging: staging: fbtft: Fix reset assertion when using gpio descriptor") Has these problem(s): - Subject does not match target commit subject Just use git log -1 --format='Fixes: %h ("%s")' Fixes tag: Fixes: dbc4f989c878 ("Staging: staging: fbtft: Fix probing of gpio descriptor") Has these problem(s): - Subject does not match target commit subject Just use git log -1 --format='Fixes: %h ("%s")' Fixes tag: Fixes: c440eee1a7a1 ("Staging: staging: fbtft: Switch to the gpio descriptor interface") Has these problem(s): - Subject does not match target commit subject Just use git log -1 --format='Fixes: %h ("%s")'
Please fix up for your next version of this series.
thanks,
greg k-h
On Mon, May 3, 2021 at 7:46 PM Greg Kroah-Hartman gregkh@linuxfoundation.org wrote:
On Wed, Apr 28, 2021 at 04:04:12PM +0300, Andy Shevchenko wrote:
Fixes: 92e3e884887c ("Staging: staging: fbtft: Fix GPIO handling") Fixes: b918d1c27066 ("Staging: staging: fbtft: Fix reset assertion when using gpio descriptor") Fixes: dbc4f989c878 ("Staging: staging: fbtft: Fix probing of gpio descriptor") Fixes: c440eee1a7a1 ("Staging: staging: fbtft: Switch to the gpio descriptor interface")
I get the following error when trying to apply this:
Argh, i replaced over all commit messages when added staging prefix. Thanks for catching this!
Please fix up for your next version of this series.
Will do!
The custom ->reset() repeats the generic one, replace it.
Note, in newer kernels the context of the function is a sleeping one, it's fine to switch over to the sleeping functions. Keeping the reset line asserted longer than 20 microseconds is also okay, it's an idling state of the hardware.
Signed-off-by: Andy Shevchenko andriy.shevchenko@linux.intel.com --- drivers/staging/fbtft/fb_agm1264k-fl.c | 14 -------------- 1 file changed, 14 deletions(-)
diff --git a/drivers/staging/fbtft/fb_agm1264k-fl.c b/drivers/staging/fbtft/fb_agm1264k-fl.c index b545c2ca80a4..207d578547cd 100644 --- a/drivers/staging/fbtft/fb_agm1264k-fl.c +++ b/drivers/staging/fbtft/fb_agm1264k-fl.c @@ -77,19 +77,6 @@ static int init_display(struct fbtft_par *par) return 0; }
-static void reset(struct fbtft_par *par) -{ - if (!par->gpio.reset) - return; - - dev_dbg(par->info->device, "%s()\n", __func__); - - gpiod_set_value(par->gpio.reset, 1); - udelay(20); - gpiod_set_value(par->gpio.reset, 0); - mdelay(120); -} - /* Check if all necessary GPIOS defined */ static int verify_gpios(struct fbtft_par *par) { @@ -439,7 +426,6 @@ static struct fbtft_display display = { .set_addr_win = set_addr_win, .verify_gpios = verify_gpios, .request_gpios_match = request_gpios_match, - .reset = reset, .write = write, .write_register = write_reg8_bus8, .write_vmem = write_vmem,
When requesting GPIO line the probe can be deferred. In such case don't spam logs with an error message. This can be achieved by switching to dev_err_probe().
Signed-off-by: Andy Shevchenko andriy.shevchenko@linux.intel.com --- drivers/staging/fbtft/fbtft-core.c | 12 ++++-------- 1 file changed, 4 insertions(+), 8 deletions(-)
diff --git a/drivers/staging/fbtft/fbtft-core.c b/drivers/staging/fbtft/fbtft-core.c index 67c3b1975a4d..a564907c4fa1 100644 --- a/drivers/staging/fbtft/fbtft-core.c +++ b/drivers/staging/fbtft/fbtft-core.c @@ -75,20 +75,16 @@ static int fbtft_request_one_gpio(struct fbtft_par *par, struct gpio_desc **gpiop) { struct device *dev = par->info->device; - int ret = 0;
*gpiop = devm_gpiod_get_index_optional(dev, name, index, GPIOD_OUT_LOW); - if (IS_ERR(*gpiop)) { - ret = PTR_ERR(*gpiop); - dev_err(dev, - "Failed to request %s GPIO: %d\n", name, ret); - return ret; - } + if (IS_ERR(*gpiop)) + dev_err_probe(dev, PTR_ERR(*gpiop), "Failed to request %s GPIO\n", name); + fbtft_par_dbg(DEBUG_REQUEST_GPIOS, par, "%s: '%s' GPIO\n", __func__, name);
- return ret; + return 0; }
static int fbtft_request_gpios(struct fbtft_par *par)
On Wed, Apr 28, 2021 at 04:04:14PM +0300, Andy Shevchenko wrote:
@@ -75,20 +75,16 @@ static int fbtft_request_one_gpio(struct fbtft_par *par, struct gpio_desc **gpiop) { struct device *dev = par->info->device;
int ret = 0;
*gpiop = devm_gpiod_get_index_optional(dev, name, index, GPIOD_OUT_LOW);
if (IS_ERR(*gpiop)) {
ret = PTR_ERR(*gpiop);dev_err(dev,"Failed to request %s GPIO: %d\n", name, ret);return ret;}
- if (IS_ERR(*gpiop))
dev_err_probe(dev, PTR_ERR(*gpiop), "Failed to request %s GPIO\n", name);
This should be a return statement:
return dev_err_probe(dev, PTR_ERR(*gpiop), "Failed to request %s GPIO\n", name);
- fbtft_par_dbg(DEBUG_REQUEST_GPIOS, par, "%s: '%s' GPIO\n", __func__, name);
- return ret;
- return 0;
}
regards, dan carpenter
On Thu, Apr 29, 2021 at 05:42:44PM +0300, Dan Carpenter wrote:
On Wed, Apr 28, 2021 at 04:04:14PM +0300, Andy Shevchenko wrote:
@@ -75,20 +75,16 @@ static int fbtft_request_one_gpio(struct fbtft_par *par, struct gpio_desc **gpiop) { struct device *dev = par->info->device;
int ret = 0;
*gpiop = devm_gpiod_get_index_optional(dev, name, index, GPIOD_OUT_LOW);
if (IS_ERR(*gpiop)) {
ret = PTR_ERR(*gpiop);dev_err(dev,"Failed to request %s GPIO: %d\n", name, ret);return ret;}
- if (IS_ERR(*gpiop))
dev_err_probe(dev, PTR_ERR(*gpiop), "Failed to request %s GPIO\n", name);This should be a return statement:
return dev_err_probe(dev, PTR_ERR(*gpiop), "Failed to request %s GPIO\n", name);
I've created a new Smatch check for these:
drivers/net/can/spi/mcp251xfd/mcp251xfd-core.c:2890 mcp251xfd_probe() warn: pointer error 'PTR_ERR(clk)' not handled
There aren't that many bugs... Anyway, I'm running a test now and I guess we'll see tomorrow how it goes.
regards, dan carpenter
On Thu, Apr 29, 2021 at 05:42:44PM +0300, Dan Carpenter wrote:
On Wed, Apr 28, 2021 at 04:04:14PM +0300, Andy Shevchenko wrote:
- if (IS_ERR(*gpiop))
dev_err_probe(dev, PTR_ERR(*gpiop), "Failed to request %s GPIO\n", name);This should be a return statement:
return dev_err_probe(dev, PTR_ERR(*gpiop), "Failed to request %s GPIO\n", name);
Thanks!
Funny that I have trapped to this and my patch that adds __must_check to avoid exactly this situations had been reverted :-(
Now, after a few fixes we may consider the conversion to the GPIO descriptor API is done.
Signed-off-by: Andy Shevchenko andriy.shevchenko@linux.intel.com --- drivers/staging/fbtft/TODO | 5 ----- 1 file changed, 5 deletions(-)
diff --git a/drivers/staging/fbtft/TODO b/drivers/staging/fbtft/TODO index a9f4802bb6be..e72a08bf221c 100644 --- a/drivers/staging/fbtft/TODO +++ b/drivers/staging/fbtft/TODO @@ -1,8 +1,3 @@ -* convert all uses of the old GPIO API from <linux/gpio.h> to the - GPIO descriptor API in <linux/gpio/consumer.h> and look up GPIO - lines from device tree, ACPI or board files, board files should - use <linux/gpio/machine.h> - * convert all these over to drm_simple_display_pipe and submit for inclusion into the DRM subsystem under drivers/gpu/drm - fbdev doesn't take any new drivers anymore.
dri-devel@lists.freedesktop.org
-
Andy Shevchenko -
Andy Shevchenko -
Dan Carpenter -
Greg Kroah-Hartman