[PATCH v12] staging: fbtft: add tearing signal detect

[PATCH v12] staging: fbtft: add tearing signal detect

[Carlis]

From: zhangxuezhi <zhangxuezhi1@yulong.com>

For st7789v ic,when we need continuous full screen refresh, it is best to
wait for the TE signal arrive to avoid screen tearing

Signed-off-by: zhangxuezhi <zhangxuezhi1@yulong.com>
---
v12: change dev_err to dev_err_probe and add space in comments start, and
     delete te_mutex, change te wait logic
v11: remove devm_gpio_put and change a dev_err to dev_info
v10: additional notes
v9: change pr_* to dev_*
v8: delete a log line
v7: return error value when request fail
v6: add te gpio request fail deal logic
v5: fix log print
v4: modify some code style and change te irq set function name
v3: modify author and signed-off-by name
v2: add release te gpio after irq request fail
---
 drivers/staging/fbtft/fb_st7789v.c | 116 +++++++++++++++++++++++++++++++++++++
 drivers/staging/fbtft/fbtft.h      |   1 +
 2 files changed, 117 insertions(+)

diff --git a/drivers/staging/fbtft/fb_st7789v.c b/drivers/staging/fbtft/fb_st7789v.c
index 3a280cc..f08e9da 100644
--- a/drivers/staging/fbtft/fb_st7789v.c
+++ b/drivers/staging/fbtft/fb_st7789v.c
@@ -9,7 +9,11 @@
 #include <linux/delay.h>
 #include <linux/init.h>
 #include <linux/kernel.h>
+#include <linux/interrupt.h>
+#include <linux/completion.h>
 #include <linux/module.h>
+#include <linux/gpio/consumer.h>
+
 #include <video/mipi_display.h>
 
 #include "fbtft.h"
@@ -66,6 +70,15 @@ enum st7789v_command {
 #define MADCTL_MX BIT(6) /* bitmask for column address order */
 #define MADCTL_MY BIT(7) /* bitmask for page address order */
 
+#define SPI_PANEL_TE_TIMEOUT	400 /* msecs */
+static struct completion spi_panel_te;
+
+static irqreturn_t spi_panel_te_handler(int irq, void *data)
+{
+	complete(&spi_panel_te);
+	return IRQ_HANDLED;
+}
+
 /**
  * init_display() - initialize the display controller
  *
@@ -82,6 +95,29 @@ enum st7789v_command {
  */
 static int init_display(struct fbtft_par *par)
 {
+	int rc;
+	struct device *dev = par->info->device;
+
+	par->gpio.te = devm_gpiod_get_index_optional(dev, "te", 0, GPIOD_IN);
+	if (IS_ERR(par->gpio.te))
+		return dev_err_probe(par->info->device, PTR_ERR(par->gpio.te),
+				     "Failed to request te gpio\n");
+
+	if (par->gpio.te) {
+		init_completion(&spi_panel_te);
+		rc = devm_request_irq(dev,
+				      gpiod_to_irq(par->gpio.te),
+				     spi_panel_te_handler, IRQF_TRIGGER_RISING,
+				     "TE_GPIO", par);
+		if (IS_ERR(rc))
+			return dev_err_probe(par->info->device, PTR_ERR(rc),
+					     "TE request_irq failed.\n");
+
+		disable_irq_nosync(gpiod_to_irq(par->gpio.te));
+	} else {
+		dev_info(par->info->device, "%s:%d, TE gpio not specified\n",
+			 __func__, __LINE__);
+	}
 	/* turn off sleep mode */
 	write_reg(par, MIPI_DCS_EXIT_SLEEP_MODE);
 	mdelay(120);
@@ -137,6 +173,10 @@ static int init_display(struct fbtft_par *par)
 	 */
 	write_reg(par, PWCTRL1, 0xA4, 0xA1);
 
+	/* tearing effect line on */
+	if (par->gpio.te)
+		write_reg(par, 0x35, 0x00);
+
 	write_reg(par, MIPI_DCS_SET_DISPLAY_ON);
 
 	if (HSD20_IPS)
@@ -146,6 +186,81 @@ static int init_display(struct fbtft_par *par)
 }
 
 /**
+ * st7789v_write_vmem16_bus8() -  write data to display
+ *
+ * @par: FBTFT parameter object
+ * @offset: offset from screen_buffer
+ * @len: the length of data to be written
+ *
+ * 16 bit pixel over 8-bit databus
+ *
+ * Return: 0 on success, < 0 if error occurred.
+ */
+
+static int st7789v_write_vmem16_bus8(struct fbtft_par *par, size_t offset, size_t len)
+{
+	u16 *vmem16;
+	__be16 *txbuf16 = par->txbuf.buf;
+	size_t remain;
+	size_t to_copy;
+	size_t tx_array_size;
+	int i;
+	int ret = 0;
+	size_t startbyte_size = 0;
+
+	fbtft_par_dbg(DEBUG_WRITE_VMEM, par, "st7789v ---%s(offset=%zu, len=%zu)\n",
+		      __func__, offset, len);
+
+	remain = len / 2;
+	vmem16 = (u16 *)(par->info->screen_buffer + offset);
+
+	if (par->gpio.dc)
+		gpiod_set_value(par->gpio.dc, 1);
+
+	if (par->gpio.te) {
+		enable_irq(gpiod_to_irq(par->gpio.te));
+		reinit_completion(&spi_panel_te);
+		ret = wait_for_completion_timeout(&spi_panel_te,
+						  msecs_to_jiffies(SPI_PANEL_TE_TIMEOUT));
+		if (ret == 0)
+			dev_err(par->info->device, "wait panel TE time out\n");
+
+		disable_irq(gpiod_to_irq(par->gpio.te));
+	}
+	/* non buffered write */
+	if (!par->txbuf.buf)
+		return par->fbtftops.write(par, vmem16, len);
+
+	/* buffered write */
+	tx_array_size = par->txbuf.len / 2;
+
+	if (par->startbyte) {
+		txbuf16 = par->txbuf.buf + 1;
+		tx_array_size -= 2;
+		*(u8 *)(par->txbuf.buf) = par->startbyte | 0x2;
+		startbyte_size = 1;
+	}
+
+	while (remain) {
+		to_copy = min(tx_array_size, remain);
+		dev_dbg(par->info->device, "    to_copy=%zu, remain=%zu\n",
+			to_copy, remain - to_copy);
+
+		for (i = 0; i < to_copy; i++)
+			txbuf16[i] = cpu_to_be16(vmem16[i]);
+
+		vmem16 = vmem16 + to_copy;
+		ret = par->fbtftops.write(par, par->txbuf.buf,
+					 startbyte_size + to_copy * 2);
+		if (ret < 0)
+			return ret;
+		remain -= to_copy;
+	}
+
+	return ret;
+}
+
+/**
  * set_var() - apply LCD properties like rotation and BGR mode
  *
  * @par: FBTFT parameter object
@@ -259,6 +374,7 @@ static int blank(struct fbtft_par *par, bool on)
 	.gamma = HSD20_IPS_GAMMA,
 	.fbtftops = {
 		.init_display = init_display,
+		.write_vmem = st7789v_write_vmem16_bus8,
 		.set_var = set_var,
 		.set_gamma = set_gamma,
 		.blank = blank,
diff --git a/drivers/staging/fbtft/fbtft.h b/drivers/staging/fbtft/fbtft.h
index 76f8c09..93bac05 100644
--- a/drivers/staging/fbtft/fbtft.h
+++ b/drivers/staging/fbtft/fbtft.h
@@ -212,6 +212,7 @@ struct fbtft_par {
 		struct gpio_desc *wr;
 		struct gpio_desc *latch;
 		struct gpio_desc *cs;
+		struct gpio_desc *te;
 		struct gpio_desc *db[16];
 		struct gpio_desc *led[16];
 		struct gpio_desc *aux[16];
-- 
1.9.1

[PATCH v12] staging: fbtft: add tearing signal detect

[Carlis]

From: zhangxuezhi <zhangxuezhi1@yulong.com>

For st7789v ic,when we need continuous full screen refresh, it is best to
wait for the TE signal arrive to avoid screen tearing

Signed-off-by: zhangxuezhi <zhangxuezhi1@yulong.com>
---
v12: change dev_err to dev_err_probe and add space in comments start, and
     delete te_mutex, change te wait logic
v11: remove devm_gpio_put and change a dev_err to dev_info
v10: additional notes
v9: change pr_* to dev_*
v8: delete a log line
v7: return error value when request fail
v6: add te gpio request fail deal logic
v5: fix log print
v4: modify some code style and change te irq set function name
v3: modify author and signed-off-by name
v2: add release te gpio after irq request fail
---
 drivers/staging/fbtft/fb_st7789v.c | 116 +++++++++++++++++++++++++++++++++++++
 drivers/staging/fbtft/fbtft.h      |   1 +
 2 files changed, 117 insertions(+)

diff --git a/drivers/staging/fbtft/fb_st7789v.c b/drivers/staging/fbtft/fb_st7789v.c
index 3a280cc..f08e9da 100644
--- a/drivers/staging/fbtft/fb_st7789v.c
+++ b/drivers/staging/fbtft/fb_st7789v.c
@@ -9,7 +9,11 @@
 #include <linux/delay.h>
 #include <linux/init.h>
 #include <linux/kernel.h>
+#include <linux/interrupt.h>
+#include <linux/completion.h>
 #include <linux/module.h>
+#include <linux/gpio/consumer.h>
+
 #include <video/mipi_display.h>
 
 #include "fbtft.h"
@@ -66,6 +70,15 @@ enum st7789v_command {
 #define MADCTL_MX BIT(6) /* bitmask for column address order */
 #define MADCTL_MY BIT(7) /* bitmask for page address order */
 
+#define SPI_PANEL_TE_TIMEOUT	400 /* msecs */
+static struct completion spi_panel_te;
+
+static irqreturn_t spi_panel_te_handler(int irq, void *data)
+{
+	complete(&spi_panel_te);
+	return IRQ_HANDLED;
+}
+
 /**
  * init_display() - initialize the display controller
  *
@@ -82,6 +95,29 @@ enum st7789v_command {
  */
 static int init_display(struct fbtft_par *par)
 {
+	int rc;
+	struct device *dev = par->info->device;
+
+	par->gpio.te = devm_gpiod_get_index_optional(dev, "te", 0, GPIOD_IN);
+	if (IS_ERR(par->gpio.te))
+		return dev_err_probe(par->info->device, PTR_ERR(par->gpio.te),
+				     "Failed to request te gpio\n");
+
+	if (par->gpio.te) {
+		init_completion(&spi_panel_te);
+		rc = devm_request_irq(dev,
+				      gpiod_to_irq(par->gpio.te),
+				     spi_panel_te_handler, IRQF_TRIGGER_RISING,
+				     "TE_GPIO", par);
+		if (IS_ERR(rc))
+			return dev_err_probe(par->info->device, PTR_ERR(rc),
+					     "TE request_irq failed.\n");
+
+		disable_irq_nosync(gpiod_to_irq(par->gpio.te));
+	} else {
+		dev_info(par->info->device, "%s:%d, TE gpio not specified\n",
+			 __func__, __LINE__);
+	}
 	/* turn off sleep mode */
 	write_reg(par, MIPI_DCS_EXIT_SLEEP_MODE);
 	mdelay(120);
@@ -137,6 +173,10 @@ static int init_display(struct fbtft_par *par)
 	 */
 	write_reg(par, PWCTRL1, 0xA4, 0xA1);
 
+	/* tearing effect line on */
+	if (par->gpio.te)
+		write_reg(par, 0x35, 0x00);
+
 	write_reg(par, MIPI_DCS_SET_DISPLAY_ON);
 
 	if (HSD20_IPS)
@@ -146,6 +186,81 @@ static int init_display(struct fbtft_par *par)
 }
 
 /**
+ * st7789v_write_vmem16_bus8() -  write data to display
+ *
+ * @par: FBTFT parameter object
+ * @offset: offset from screen_buffer
+ * @len: the length of data to be written
+ *
+ * 16 bit pixel over 8-bit databus
+ *
+ * Return: 0 on success, < 0 if error occurred.
+ */
+
+static int st7789v_write_vmem16_bus8(struct fbtft_par *par, size_t offset, size_t len)
+{
+	u16 *vmem16;
+	__be16 *txbuf16 = par->txbuf.buf;
+	size_t remain;
+	size_t to_copy;
+	size_t tx_array_size;
+	int i;
+	int ret = 0;
+	size_t startbyte_size = 0;
+
+	fbtft_par_dbg(DEBUG_WRITE_VMEM, par, "st7789v ---%s(offset=%zu, len=%zu)\n",
+		      __func__, offset, len);
+
+	remain = len / 2;
+	vmem16 = (u16 *)(par->info->screen_buffer + offset);
+
+	if (par->gpio.dc)
+		gpiod_set_value(par->gpio.dc, 1);
+
+	if (par->gpio.te) {
+		enable_irq(gpiod_to_irq(par->gpio.te));
+		reinit_completion(&spi_panel_te);
+		ret = wait_for_completion_timeout(&spi_panel_te,
+						  msecs_to_jiffies(SPI_PANEL_TE_TIMEOUT));
+		if (ret == 0)
+			dev_err(par->info->device, "wait panel TE time out\n");
+
+		disable_irq(gpiod_to_irq(par->gpio.te));
+	}
+	/* non buffered write */
+	if (!par->txbuf.buf)
+		return par->fbtftops.write(par, vmem16, len);
+
+	/* buffered write */
+	tx_array_size = par->txbuf.len / 2;
+
+	if (par->startbyte) {
+		txbuf16 = par->txbuf.buf + 1;
+		tx_array_size -= 2;
+		*(u8 *)(par->txbuf.buf) = par->startbyte | 0x2;
+		startbyte_size = 1;
+	}
+
+	while (remain) {
+		to_copy = min(tx_array_size, remain);
+		dev_dbg(par->info->device, "    to_copy=%zu, remain=%zu\n",
+			to_copy, remain - to_copy);
+
+		for (i = 0; i < to_copy; i++)
+			txbuf16[i] = cpu_to_be16(vmem16[i]);
+
+		vmem16 = vmem16 + to_copy;
+		ret = par->fbtftops.write(par, par->txbuf.buf,
+					 startbyte_size + to_copy * 2);
+		if (ret < 0)
+			return ret;
+		remain -= to_copy;
+	}
+
+	return ret;
+}
+
+/**
  * set_var() - apply LCD properties like rotation and BGR mode
  *
  * @par: FBTFT parameter object
@@ -259,6 +374,7 @@ static int blank(struct fbtft_par *par, bool on)
 	.gamma = HSD20_IPS_GAMMA,
 	.fbtftops = {
 		.init_display = init_display,
+		.write_vmem = st7789v_write_vmem16_bus8,
 		.set_var = set_var,
 		.set_gamma = set_gamma,
 		.blank = blank,
diff --git a/drivers/staging/fbtft/fbtft.h b/drivers/staging/fbtft/fbtft.h
index 76f8c09..93bac05 100644
--- a/drivers/staging/fbtft/fbtft.h
+++ b/drivers/staging/fbtft/fbtft.h
@@ -212,6 +212,7 @@ struct fbtft_par {
 		struct gpio_desc *wr;
 		struct gpio_desc *latch;
 		struct gpio_desc *cs;
+		struct gpio_desc *te;
 		struct gpio_desc *db[16];
 		struct gpio_desc *led[16];
 		struct gpio_desc *aux[16];
-- 
1.9.1

_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH v12] staging: fbtft: add tearing signal detect

[Carlis]

From: zhangxuezhi <zhangxuezhi1@yulong.com>

For st7789v ic,when we need continuous full screen refresh, it is best to
wait for the TE signal arrive to avoid screen tearing

Signed-off-by: zhangxuezhi <zhangxuezhi1@yulong.com>
---
v12: change dev_err to dev_err_probe and add space in comments start, and
     delete te_mutex, change te wait logic
v11: remove devm_gpio_put and change a dev_err to dev_info
v10: additional notes
v9: change pr_* to dev_*
v8: delete a log line
v7: return error value when request fail
v6: add te gpio request fail deal logic
v5: fix log print
v4: modify some code style and change te irq set function name
v3: modify author and signed-off-by name
v2: add release te gpio after irq request fail
---
 drivers/staging/fbtft/fb_st7789v.c | 116 +++++++++++++++++++++++++++++++++++++
 drivers/staging/fbtft/fbtft.h      |   1 +
 2 files changed, 117 insertions(+)

diff --git a/drivers/staging/fbtft/fb_st7789v.c b/drivers/staging/fbtft/fb_st7789v.c
index 3a280cc..f08e9da 100644
--- a/drivers/staging/fbtft/fb_st7789v.c
+++ b/drivers/staging/fbtft/fb_st7789v.c
@@ -9,7 +9,11 @@
 #include <linux/delay.h>
 #include <linux/init.h>
 #include <linux/kernel.h>
+#include <linux/interrupt.h>
+#include <linux/completion.h>
 #include <linux/module.h>
+#include <linux/gpio/consumer.h>
+
 #include <video/mipi_display.h>
 
 #include "fbtft.h"
@@ -66,6 +70,15 @@ enum st7789v_command {
 #define MADCTL_MX BIT(6) /* bitmask for column address order */
 #define MADCTL_MY BIT(7) /* bitmask for page address order */
 
+#define SPI_PANEL_TE_TIMEOUT	400 /* msecs */
+static struct completion spi_panel_te;
+
+static irqreturn_t spi_panel_te_handler(int irq, void *data)
+{
+	complete(&spi_panel_te);
+	return IRQ_HANDLED;
+}
+
 /**
  * init_display() - initialize the display controller
  *
@@ -82,6 +95,29 @@ enum st7789v_command {
  */
 static int init_display(struct fbtft_par *par)
 {
+	int rc;
+	struct device *dev = par->info->device;
+
+	par->gpio.te = devm_gpiod_get_index_optional(dev, "te", 0, GPIOD_IN);
+	if (IS_ERR(par->gpio.te))
+		return dev_err_probe(par->info->device, PTR_ERR(par->gpio.te),
+				     "Failed to request te gpio\n");
+
+	if (par->gpio.te) {
+		init_completion(&spi_panel_te);
+		rc = devm_request_irq(dev,
+				      gpiod_to_irq(par->gpio.te),
+				     spi_panel_te_handler, IRQF_TRIGGER_RISING,
+				     "TE_GPIO", par);
+		if (IS_ERR(rc))
+			return dev_err_probe(par->info->device, PTR_ERR(rc),
+					     "TE request_irq failed.\n");
+
+		disable_irq_nosync(gpiod_to_irq(par->gpio.te));
+	} else {
+		dev_info(par->info->device, "%s:%d, TE gpio not specified\n",
+			 __func__, __LINE__);
+	}
 	/* turn off sleep mode */
 	write_reg(par, MIPI_DCS_EXIT_SLEEP_MODE);
 	mdelay(120);
@@ -137,6 +173,10 @@ static int init_display(struct fbtft_par *par)
 	 */
 	write_reg(par, PWCTRL1, 0xA4, 0xA1);
 
+	/* tearing effect line on */
+	if (par->gpio.te)
+		write_reg(par, 0x35, 0x00);
+
 	write_reg(par, MIPI_DCS_SET_DISPLAY_ON);
 
 	if (HSD20_IPS)
@@ -146,6 +186,81 @@ static int init_display(struct fbtft_par *par)
 }
 
 /**
+ * st7789v_write_vmem16_bus8() -  write data to display
+ *
+ * @par: FBTFT parameter object
+ * @offset: offset from screen_buffer
+ * @len: the length of data to be written
+ *
+ * 16 bit pixel over 8-bit databus
+ *
+ * Return: 0 on success, < 0 if error occurred.
+ */
+
+static int st7789v_write_vmem16_bus8(struct fbtft_par *par, size_t offset, size_t len)
+{
+	u16 *vmem16;
+	__be16 *txbuf16 = par->txbuf.buf;
+	size_t remain;
+	size_t to_copy;
+	size_t tx_array_size;
+	int i;
+	int ret = 0;
+	size_t startbyte_size = 0;
+
+	fbtft_par_dbg(DEBUG_WRITE_VMEM, par, "st7789v ---%s(offset=%zu, len=%zu)\n",
+		      __func__, offset, len);
+
+	remain = len / 2;
+	vmem16 = (u16 *)(par->info->screen_buffer + offset);
+
+	if (par->gpio.dc)
+		gpiod_set_value(par->gpio.dc, 1);
+
+	if (par->gpio.te) {
+		enable_irq(gpiod_to_irq(par->gpio.te));
+		reinit_completion(&spi_panel_te);
+		ret = wait_for_completion_timeout(&spi_panel_te,
+						  msecs_to_jiffies(SPI_PANEL_TE_TIMEOUT));
+		if (ret == 0)
+			dev_err(par->info->device, "wait panel TE time out\n");
+
+		disable_irq(gpiod_to_irq(par->gpio.te));
+	}
+	/* non buffered write */
+	if (!par->txbuf.buf)
+		return par->fbtftops.write(par, vmem16, len);
+
+	/* buffered write */
+	tx_array_size = par->txbuf.len / 2;
+
+	if (par->startbyte) {
+		txbuf16 = par->txbuf.buf + 1;
+		tx_array_size -= 2;
+		*(u8 *)(par->txbuf.buf) = par->startbyte | 0x2;
+		startbyte_size = 1;
+	}
+
+	while (remain) {
+		to_copy = min(tx_array_size, remain);
+		dev_dbg(par->info->device, "    to_copy=%zu, remain=%zu\n",
+			to_copy, remain - to_copy);
+
+		for (i = 0; i < to_copy; i++)
+			txbuf16[i] = cpu_to_be16(vmem16[i]);
+
+		vmem16 = vmem16 + to_copy;
+		ret = par->fbtftops.write(par, par->txbuf.buf,
+					 startbyte_size + to_copy * 2);
+		if (ret < 0)
+			return ret;
+		remain -= to_copy;
+	}
+
+	return ret;
+}
+
+/**
  * set_var() - apply LCD properties like rotation and BGR mode
  *
  * @par: FBTFT parameter object
@@ -259,6 +374,7 @@ static int blank(struct fbtft_par *par, bool on)
 	.gamma = HSD20_IPS_GAMMA,
 	.fbtftops = {
 		.init_display = init_display,
+		.write_vmem = st7789v_write_vmem16_bus8,
 		.set_var = set_var,
 		.set_gamma = set_gamma,
 		.blank = blank,
diff --git a/drivers/staging/fbtft/fbtft.h b/drivers/staging/fbtft/fbtft.h
index 76f8c09..93bac05 100644
--- a/drivers/staging/fbtft/fbtft.h
+++ b/drivers/staging/fbtft/fbtft.h
@@ -212,6 +212,7 @@ struct fbtft_par {
 		struct gpio_desc *wr;
 		struct gpio_desc *latch;
 		struct gpio_desc *cs;
+		struct gpio_desc *te;
 		struct gpio_desc *db[16];
 		struct gpio_desc *led[16];
 		struct gpio_desc *aux[16];
-- 
1.9.1

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v12] staging: fbtft: add tearing signal detect

[Greg KH]

On Thu, Jan 28, 2021 at 08:53:55PM +0800, Carlis wrote:
> From: zhangxuezhi <zhangxuezhi1@yulong.com>
> 
> For st7789v ic,when we need continuous full screen refresh, it is best to
> wait for the TE signal arrive to avoid screen tearing
> 
> Signed-off-by: zhangxuezhi <zhangxuezhi1@yulong.com>
> ---
> v12: change dev_err to dev_err_probe and add space in comments start, and
>      delete te_mutex, change te wait logic
> v11: remove devm_gpio_put and change a dev_err to dev_info
> v10: additional notes
> v9: change pr_* to dev_*
> v8: delete a log line
> v7: return error value when request fail
> v6: add te gpio request fail deal logic
> v5: fix log print
> v4: modify some code style and change te irq set function name
> v3: modify author and signed-off-by name
> v2: add release te gpio after irq request fail
> ---
>  drivers/staging/fbtft/fb_st7789v.c | 116 +++++++++++++++++++++++++++++++++++++
>  drivers/staging/fbtft/fbtft.h      |   1 +
>  2 files changed, 117 insertions(+)
> 
> diff --git a/drivers/staging/fbtft/fb_st7789v.c b/drivers/staging/fbtft/fb_st7789v.c
> index 3a280cc..f08e9da 100644
> --- a/drivers/staging/fbtft/fb_st7789v.c
> +++ b/drivers/staging/fbtft/fb_st7789v.c
> @@ -9,7 +9,11 @@
>  #include <linux/delay.h>
>  #include <linux/init.h>
>  #include <linux/kernel.h>
> +#include <linux/interrupt.h>
> +#include <linux/completion.h>
>  #include <linux/module.h>
> +#include <linux/gpio/consumer.h>
> +
>  #include <video/mipi_display.h>
>  
>  #include "fbtft.h"
> @@ -66,6 +70,15 @@ enum st7789v_command {
>  #define MADCTL_MX BIT(6) /* bitmask for column address order */
>  #define MADCTL_MY BIT(7) /* bitmask for page address order */
>  
> +#define SPI_PANEL_TE_TIMEOUT	400 /* msecs */
> +static struct completion spi_panel_te;

Doesn't this structure have to be per-device?  How can it be global for
all devices in the system controlled by this driver?

thanks,

greg k-h

Re: [PATCH v12] staging: fbtft: add tearing signal detect

[Greg KH]

On Thu, Jan 28, 2021 at 08:53:55PM +0800, Carlis wrote:
> From: zhangxuezhi <zhangxuezhi1@yulong.com>
> 
> For st7789v ic,when we need continuous full screen refresh, it is best to
> wait for the TE signal arrive to avoid screen tearing
> 
> Signed-off-by: zhangxuezhi <zhangxuezhi1@yulong.com>
> ---
> v12: change dev_err to dev_err_probe and add space in comments start, and
>      delete te_mutex, change te wait logic
> v11: remove devm_gpio_put and change a dev_err to dev_info
> v10: additional notes
> v9: change pr_* to dev_*
> v8: delete a log line
> v7: return error value when request fail
> v6: add te gpio request fail deal logic
> v5: fix log print
> v4: modify some code style and change te irq set function name
> v3: modify author and signed-off-by name
> v2: add release te gpio after irq request fail
> ---
>  drivers/staging/fbtft/fb_st7789v.c | 116 +++++++++++++++++++++++++++++++++++++
>  drivers/staging/fbtft/fbtft.h      |   1 +
>  2 files changed, 117 insertions(+)
> 
> diff --git a/drivers/staging/fbtft/fb_st7789v.c b/drivers/staging/fbtft/fb_st7789v.c
> index 3a280cc..f08e9da 100644
> --- a/drivers/staging/fbtft/fb_st7789v.c
> +++ b/drivers/staging/fbtft/fb_st7789v.c
> @@ -9,7 +9,11 @@
>  #include <linux/delay.h>
>  #include <linux/init.h>
>  #include <linux/kernel.h>
> +#include <linux/interrupt.h>
> +#include <linux/completion.h>
>  #include <linux/module.h>
> +#include <linux/gpio/consumer.h>
> +
>  #include <video/mipi_display.h>
>  
>  #include "fbtft.h"
> @@ -66,6 +70,15 @@ enum st7789v_command {
>  #define MADCTL_MX BIT(6) /* bitmask for column address order */
>  #define MADCTL_MY BIT(7) /* bitmask for page address order */
>  
> +#define SPI_PANEL_TE_TIMEOUT	400 /* msecs */
> +static struct completion spi_panel_te;

Doesn't this structure have to be per-device?  How can it be global for
all devices in the system controlled by this driver?

thanks,

greg k-h
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v12] staging: fbtft: add tearing signal detect

[Greg KH]

On Thu, Jan 28, 2021 at 08:53:55PM +0800, Carlis wrote:
> From: zhangxuezhi <zhangxuezhi1@yulong.com>
> 
> For st7789v ic,when we need continuous full screen refresh, it is best to
> wait for the TE signal arrive to avoid screen tearing
> 
> Signed-off-by: zhangxuezhi <zhangxuezhi1@yulong.com>
> ---
> v12: change dev_err to dev_err_probe and add space in comments start, and
>      delete te_mutex, change te wait logic
> v11: remove devm_gpio_put and change a dev_err to dev_info
> v10: additional notes
> v9: change pr_* to dev_*
> v8: delete a log line
> v7: return error value when request fail
> v6: add te gpio request fail deal logic
> v5: fix log print
> v4: modify some code style and change te irq set function name
> v3: modify author and signed-off-by name
> v2: add release te gpio after irq request fail
> ---
>  drivers/staging/fbtft/fb_st7789v.c | 116 +++++++++++++++++++++++++++++++++++++
>  drivers/staging/fbtft/fbtft.h      |   1 +
>  2 files changed, 117 insertions(+)
> 
> diff --git a/drivers/staging/fbtft/fb_st7789v.c b/drivers/staging/fbtft/fb_st7789v.c
> index 3a280cc..f08e9da 100644
> --- a/drivers/staging/fbtft/fb_st7789v.c
> +++ b/drivers/staging/fbtft/fb_st7789v.c
> @@ -9,7 +9,11 @@
>  #include <linux/delay.h>
>  #include <linux/init.h>
>  #include <linux/kernel.h>
> +#include <linux/interrupt.h>
> +#include <linux/completion.h>
>  #include <linux/module.h>
> +#include <linux/gpio/consumer.h>
> +
>  #include <video/mipi_display.h>
>  
>  #include "fbtft.h"
> @@ -66,6 +70,15 @@ enum st7789v_command {
>  #define MADCTL_MX BIT(6) /* bitmask for column address order */
>  #define MADCTL_MY BIT(7) /* bitmask for page address order */
>  
> +#define SPI_PANEL_TE_TIMEOUT	400 /* msecs */
> +static struct completion spi_panel_te;

Doesn't this structure have to be per-device?  How can it be global for
all devices in the system controlled by this driver?

thanks,

greg k-h
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v12] staging: fbtft: add tearing signal detect

[Andy Shevchenko]

On Thu, Jan 28, 2021 at 2:58 PM Carlis <zhangxuezhi3@gmail.com> wrote:

Thanks for your contribution, my comments below.

> From: zhangxuezhi <zhangxuezhi1@yulong.com>

You probably have to configure your Git to use the same account for
author and committer.

> For st7789v ic,when we need continuous full screen refresh, it is best to

'ic,when' -> 'IC, when'

> wait for the TE signal arrive to avoid screen tearing

Decode TE for people who are not familiar with the abbreviation.

Missed period at the end of sentence.

...

>  #include <linux/delay.h>
>  #include <linux/init.h>
>  #include <linux/kernel.h>
> +#include <linux/interrupt.h>
> +#include <linux/completion.h>
>  #include <linux/module.h>
> +#include <linux/gpio/consumer.h>
> +

Good, but I would rather squeeze it above to be more or less ordered,
like just after delay.h inclusion.

>  #include <video/mipi_display.h>

...

> +#define SPI_PANEL_TE_TIMEOUT   400 /* msecs */

Useless comment. Instead use _MS suffix in the name of constant.
Besides that please add a comment explaining why this value has been
chosen.

...

> +static struct completion spi_panel_te;

As Greg said.

...

>  static int init_display(struct fbtft_par *par)
>  {
> +       int rc;
> +       struct device *dev = par->info->device;

Keep reversed xmas tree order:

       struct device *dev = par->info->device;
       int rc;

...

> +       par->gpio.te = devm_gpiod_get_index_optional(dev, "te", 0, GPIOD_IN);

No need to have it requested for all time since you use it as an IRQ
later on. The IRQ chip will call the GPIO library framework to lock a
pin as IRQ anyway.

> +       if (IS_ERR(par->gpio.te))
> +               return dev_err_probe(par->info->device, PTR_ERR(par->gpio.te),
> +                                    "Failed to request te gpio\n");

> +       if (par->gpio.te) {

Instead you should probably do the following:

int irq;

irq = gpiod_to_irq(...);
if (irq > 0)

> +               init_completion(&spi_panel_te);
> +               rc = devm_request_irq(dev,

> +                                     gpiod_to_irq(par->gpio.te),

...and here simply use irq.

> +                                    spi_panel_te_handler, IRQF_TRIGGER_RISING,
> +                                    "TE_GPIO", par);

> +               if (IS_ERR(rc))

This is wrong. rc is integer no IS_ERR() is required. Ditto for
PTR_ERR(). Have you even looked for these macros implementations?

> +                       return dev_err_probe(par->info->device, PTR_ERR(rc),

Use your temporary variable and move...

> +                                            "TE request_irq failed.\n");

...this on the previous line.

> +               disable_irq_nosync(gpiod_to_irq(par->gpio.te));

Why do you call gpio_to_irq() twice?


> +       } else {
> +               dev_info(par->info->device, "%s:%d, TE gpio not specified\n",
> +                        __func__, __LINE__);

Remove this noise (besides the fact that we don't use __file__ and
__LINE__ in messages like this.

> +       }

Taking all together you probably need to create a helper and use it
inside init_display(), like

static int init_tearing_effect_line(struct fbtft_par *par)
{
  struct device *dev = par->info->device;
  struct gpio_desc *te;
  int irq, rc;

  te = gpiod_get_optional(dev, "te", GPIOD_IN);
  if (IS_ERR(te))
           return dev_err_probe(dev, PTR_ERR(te), "Failed to request
te GPIO\n");

  irq = gpiod_to_irq(te); // this value you have to save in the
driver's (per device) data structure.

  /* GPIO is locked as an IRQ, we may drop the reference */
  gpiod_put(te);

  init_completion(&spi_panel_te); // should be in the (per device)
data structure
  rc = devm_request_irq(dev, irq,  spi_panel_te_handler,
IRQF_TRIGGER_RISING, "TE_GPIO", par);
  if (rc)
                return dev_err_probe(dev, rc, "TE IRQ request failed.\n");
  disable_irq_nosync(irq);
  return irq;
}

Note, when you define proper fields for IRQ line and completion in the
data structure the above can be amended accordingly.

...

> +       /* tearing effect line on */
> +       if (par->gpio.te)
> +               write_reg(par, 0x35, 0x00);

0x35 is defined. use it and drop useless comments.

...

>  /**
> + * st7789v_write_vmem16_bus8() -  write data to display

> + *

Redundant blank line.

> + * @par: FBTFT parameter object
> + * @offset: offset from screen_buffer
> + * @len: the length of data to be written
> + *
> + * 16 bit pixel over 8-bit databus
> + *
> + * Return: 0 on success, < 0 if error occurred.

", or a negative error code otherwise"

> + */

> +
Redundant blank line

> +static int st7789v_write_vmem16_bus8(struct fbtft_par *par, size_t offset, size_t len)
> +{
> +       u16 *vmem16;
> +       __be16 *txbuf16 = par->txbuf.buf;
> +       size_t remain;
> +       size_t to_copy;
> +       size_t tx_array_size;
> +       int i;
> +       int ret = 0;
> +       size_t startbyte_size = 0;

Reversed xmas tree order.

> +       fbtft_par_dbg(DEBUG_WRITE_VMEM, par, "st7789v ---%s(offset=%zu, len=%zu)\n",
> +                     __func__, offset, len);
> +
> +       remain = len / 2;
> +       vmem16 = (u16 *)(par->info->screen_buffer + offset);

> +       if (par->gpio.dc)

Useless duplicate check.

> +               gpiod_set_value(par->gpio.dc, 1);

> +       if (par->gpio.te) {
> +               enable_irq(gpiod_to_irq(par->gpio.te));

Here you should use the IRQ line rather than the GPIO descriptor. See above.

> +               reinit_completion(&spi_panel_te);
> +               ret = wait_for_completion_timeout(&spi_panel_te,
> +                                                 msecs_to_jiffies(SPI_PANEL_TE_TIMEOUT));
> +               if (ret == 0)
> +                       dev_err(par->info->device, "wait panel TE time out\n");
> +
> +               disable_irq(gpiod_to_irq(par->gpio.te));
> +       }

> +
> +       while (remain) {
> +               to_copy = min(tx_array_size, remain);

> +               dev_dbg(par->info->device, "    to_copy=%zu, remain=%zu\n",
> +                       to_copy, remain - to_copy);

Like in previous functions create a temporary variable to keep a
pointer to struct device and use it here and everywhere else. It might
save you LOCs and make code easier to read and understand.

> +               for (i = 0; i < to_copy; i++)
> +                       txbuf16[i] = cpu_to_be16(vmem16[i]);

If both of them are 16-bit wide, consider moving this to a helper
which somebody can move to byteorder/generic.h in the future.

> +               vmem16 = vmem16 + to_copy;
> +               ret = par->fbtftops.write(par, par->txbuf.buf,
> +                                        startbyte_size + to_copy * 2);
> +               if (ret < 0)
> +                       return ret;
> +               remain -= to_copy;
> +       }
> +
> +       return ret;
> +}

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v12] staging: fbtft: add tearing signal detect

[Andy Shevchenko]

On Thu, Jan 28, 2021 at 2:58 PM Carlis <zhangxuezhi3@gmail.com> wrote:

Thanks for your contribution, my comments below.

> From: zhangxuezhi <zhangxuezhi1@yulong.com>

You probably have to configure your Git to use the same account for
author and committer.

> For st7789v ic,when we need continuous full screen refresh, it is best to

'ic,when' -> 'IC, when'

> wait for the TE signal arrive to avoid screen tearing

Decode TE for people who are not familiar with the abbreviation.

Missed period at the end of sentence.

...

>  #include <linux/delay.h>
>  #include <linux/init.h>
>  #include <linux/kernel.h>
> +#include <linux/interrupt.h>
> +#include <linux/completion.h>
>  #include <linux/module.h>
> +#include <linux/gpio/consumer.h>
> +

Good, but I would rather squeeze it above to be more or less ordered,
like just after delay.h inclusion.

>  #include <video/mipi_display.h>

...

> +#define SPI_PANEL_TE_TIMEOUT   400 /* msecs */

Useless comment. Instead use _MS suffix in the name of constant.
Besides that please add a comment explaining why this value has been
chosen.

...

> +static struct completion spi_panel_te;

As Greg said.

...

>  static int init_display(struct fbtft_par *par)
>  {
> +       int rc;
> +       struct device *dev = par->info->device;

Keep reversed xmas tree order:

       struct device *dev = par->info->device;
       int rc;

...

> +       par->gpio.te = devm_gpiod_get_index_optional(dev, "te", 0, GPIOD_IN);

No need to have it requested for all time since you use it as an IRQ
later on. The IRQ chip will call the GPIO library framework to lock a
pin as IRQ anyway.

> +       if (IS_ERR(par->gpio.te))
> +               return dev_err_probe(par->info->device, PTR_ERR(par->gpio.te),
> +                                    "Failed to request te gpio\n");

> +       if (par->gpio.te) {

Instead you should probably do the following:

int irq;

irq = gpiod_to_irq(...);
if (irq > 0)

> +               init_completion(&spi_panel_te);
> +               rc = devm_request_irq(dev,

> +                                     gpiod_to_irq(par->gpio.te),

...and here simply use irq.

> +                                    spi_panel_te_handler, IRQF_TRIGGER_RISING,
> +                                    "TE_GPIO", par);

> +               if (IS_ERR(rc))

This is wrong. rc is integer no IS_ERR() is required. Ditto for
PTR_ERR(). Have you even looked for these macros implementations?

> +                       return dev_err_probe(par->info->device, PTR_ERR(rc),

Use your temporary variable and move...

> +                                            "TE request_irq failed.\n");

...this on the previous line.

> +               disable_irq_nosync(gpiod_to_irq(par->gpio.te));

Why do you call gpio_to_irq() twice?


> +       } else {
> +               dev_info(par->info->device, "%s:%d, TE gpio not specified\n",
> +                        __func__, __LINE__);

Remove this noise (besides the fact that we don't use __file__ and
__LINE__ in messages like this.

> +       }

Taking all together you probably need to create a helper and use it
inside init_display(), like

static int init_tearing_effect_line(struct fbtft_par *par)
{
  struct device *dev = par->info->device;
  struct gpio_desc *te;
  int irq, rc;

  te = gpiod_get_optional(dev, "te", GPIOD_IN);
  if (IS_ERR(te))
           return dev_err_probe(dev, PTR_ERR(te), "Failed to request
te GPIO\n");

  irq = gpiod_to_irq(te); // this value you have to save in the
driver's (per device) data structure.

  /* GPIO is locked as an IRQ, we may drop the reference */
  gpiod_put(te);

  init_completion(&spi_panel_te); // should be in the (per device)
data structure
  rc = devm_request_irq(dev, irq,  spi_panel_te_handler,
IRQF_TRIGGER_RISING, "TE_GPIO", par);
  if (rc)
                return dev_err_probe(dev, rc, "TE IRQ request failed.\n");
  disable_irq_nosync(irq);
  return irq;
}

Note, when you define proper fields for IRQ line and completion in the
data structure the above can be amended accordingly.

...

> +       /* tearing effect line on */
> +       if (par->gpio.te)
> +               write_reg(par, 0x35, 0x00);

0x35 is defined. use it and drop useless comments.

...

>  /**
> + * st7789v_write_vmem16_bus8() -  write data to display

> + *

Redundant blank line.

> + * @par: FBTFT parameter object
> + * @offset: offset from screen_buffer
> + * @len: the length of data to be written
> + *
> + * 16 bit pixel over 8-bit databus
> + *
> + * Return: 0 on success, < 0 if error occurred.

", or a negative error code otherwise"

> + */

> +
Redundant blank line

> +static int st7789v_write_vmem16_bus8(struct fbtft_par *par, size_t offset, size_t len)
> +{
> +       u16 *vmem16;
> +       __be16 *txbuf16 = par->txbuf.buf;
> +       size_t remain;
> +       size_t to_copy;
> +       size_t tx_array_size;
> +       int i;
> +       int ret = 0;
> +       size_t startbyte_size = 0;

Reversed xmas tree order.

> +       fbtft_par_dbg(DEBUG_WRITE_VMEM, par, "st7789v ---%s(offset=%zu, len=%zu)\n",
> +                     __func__, offset, len);
> +
> +       remain = len / 2;
> +       vmem16 = (u16 *)(par->info->screen_buffer + offset);

> +       if (par->gpio.dc)

Useless duplicate check.

> +               gpiod_set_value(par->gpio.dc, 1);

> +       if (par->gpio.te) {
> +               enable_irq(gpiod_to_irq(par->gpio.te));

Here you should use the IRQ line rather than the GPIO descriptor. See above.

> +               reinit_completion(&spi_panel_te);
> +               ret = wait_for_completion_timeout(&spi_panel_te,
> +                                                 msecs_to_jiffies(SPI_PANEL_TE_TIMEOUT));
> +               if (ret == 0)
> +                       dev_err(par->info->device, "wait panel TE time out\n");
> +
> +               disable_irq(gpiod_to_irq(par->gpio.te));
> +       }

> +
> +       while (remain) {
> +               to_copy = min(tx_array_size, remain);

> +               dev_dbg(par->info->device, "    to_copy=%zu, remain=%zu\n",
> +                       to_copy, remain - to_copy);

Like in previous functions create a temporary variable to keep a
pointer to struct device and use it here and everywhere else. It might
save you LOCs and make code easier to read and understand.

> +               for (i = 0; i < to_copy; i++)
> +                       txbuf16[i] = cpu_to_be16(vmem16[i]);

If both of them are 16-bit wide, consider moving this to a helper
which somebody can move to byteorder/generic.h in the future.

> +               vmem16 = vmem16 + to_copy;
> +               ret = par->fbtftops.write(par, par->txbuf.buf,
> +                                        startbyte_size + to_copy * 2);
> +               if (ret < 0)
> +                       return ret;
> +               remain -= to_copy;
> +       }
> +
> +       return ret;
> +}

-- 
With Best Regards,
Andy Shevchenko
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v12] staging: fbtft: add tearing signal detect

[Andy Shevchenko]

On Thu, Jan 28, 2021 at 2:58 PM Carlis <zhangxuezhi3@gmail.com> wrote:

Thanks for your contribution, my comments below.

> From: zhangxuezhi <zhangxuezhi1@yulong.com>

You probably have to configure your Git to use the same account for
author and committer.

> For st7789v ic,when we need continuous full screen refresh, it is best to

'ic,when' -> 'IC, when'

> wait for the TE signal arrive to avoid screen tearing

Decode TE for people who are not familiar with the abbreviation.

Missed period at the end of sentence.

...

>  #include <linux/delay.h>
>  #include <linux/init.h>
>  #include <linux/kernel.h>
> +#include <linux/interrupt.h>
> +#include <linux/completion.h>
>  #include <linux/module.h>
> +#include <linux/gpio/consumer.h>
> +

Good, but I would rather squeeze it above to be more or less ordered,
like just after delay.h inclusion.

>  #include <video/mipi_display.h>

...

> +#define SPI_PANEL_TE_TIMEOUT   400 /* msecs */

Useless comment. Instead use _MS suffix in the name of constant.
Besides that please add a comment explaining why this value has been
chosen.

...

> +static struct completion spi_panel_te;

As Greg said.

...

>  static int init_display(struct fbtft_par *par)
>  {
> +       int rc;
> +       struct device *dev = par->info->device;

Keep reversed xmas tree order:

       struct device *dev = par->info->device;
       int rc;

...

> +       par->gpio.te = devm_gpiod_get_index_optional(dev, "te", 0, GPIOD_IN);

No need to have it requested for all time since you use it as an IRQ
later on. The IRQ chip will call the GPIO library framework to lock a
pin as IRQ anyway.

> +       if (IS_ERR(par->gpio.te))
> +               return dev_err_probe(par->info->device, PTR_ERR(par->gpio.te),
> +                                    "Failed to request te gpio\n");

> +       if (par->gpio.te) {

Instead you should probably do the following:

int irq;

irq = gpiod_to_irq(...);
if (irq > 0)

> +               init_completion(&spi_panel_te);
> +               rc = devm_request_irq(dev,

> +                                     gpiod_to_irq(par->gpio.te),

...and here simply use irq.

> +                                    spi_panel_te_handler, IRQF_TRIGGER_RISING,
> +                                    "TE_GPIO", par);

> +               if (IS_ERR(rc))

This is wrong. rc is integer no IS_ERR() is required. Ditto for
PTR_ERR(). Have you even looked for these macros implementations?

> +                       return dev_err_probe(par->info->device, PTR_ERR(rc),

Use your temporary variable and move...

> +                                            "TE request_irq failed.\n");

...this on the previous line.

> +               disable_irq_nosync(gpiod_to_irq(par->gpio.te));

Why do you call gpio_to_irq() twice?


> +       } else {
> +               dev_info(par->info->device, "%s:%d, TE gpio not specified\n",
> +                        __func__, __LINE__);

Remove this noise (besides the fact that we don't use __file__ and
__LINE__ in messages like this.

> +       }

Taking all together you probably need to create a helper and use it
inside init_display(), like

static int init_tearing_effect_line(struct fbtft_par *par)
{
  struct device *dev = par->info->device;
  struct gpio_desc *te;
  int irq, rc;

  te = gpiod_get_optional(dev, "te", GPIOD_IN);
  if (IS_ERR(te))
           return dev_err_probe(dev, PTR_ERR(te), "Failed to request
te GPIO\n");

  irq = gpiod_to_irq(te); // this value you have to save in the
driver's (per device) data structure.

  /* GPIO is locked as an IRQ, we may drop the reference */
  gpiod_put(te);

  init_completion(&spi_panel_te); // should be in the (per device)
data structure
  rc = devm_request_irq(dev, irq,  spi_panel_te_handler,
IRQF_TRIGGER_RISING, "TE_GPIO", par);
  if (rc)
                return dev_err_probe(dev, rc, "TE IRQ request failed.\n");
  disable_irq_nosync(irq);
  return irq;
}

Note, when you define proper fields for IRQ line and completion in the
data structure the above can be amended accordingly.

...

> +       /* tearing effect line on */
> +       if (par->gpio.te)
> +               write_reg(par, 0x35, 0x00);

0x35 is defined. use it and drop useless comments.

...

>  /**
> + * st7789v_write_vmem16_bus8() -  write data to display

> + *

Redundant blank line.

> + * @par: FBTFT parameter object
> + * @offset: offset from screen_buffer
> + * @len: the length of data to be written
> + *
> + * 16 bit pixel over 8-bit databus
> + *
> + * Return: 0 on success, < 0 if error occurred.

", or a negative error code otherwise"

> + */

> +
Redundant blank line

> +static int st7789v_write_vmem16_bus8(struct fbtft_par *par, size_t offset, size_t len)
> +{
> +       u16 *vmem16;
> +       __be16 *txbuf16 = par->txbuf.buf;
> +       size_t remain;
> +       size_t to_copy;
> +       size_t tx_array_size;
> +       int i;
> +       int ret = 0;
> +       size_t startbyte_size = 0;

Reversed xmas tree order.

> +       fbtft_par_dbg(DEBUG_WRITE_VMEM, par, "st7789v ---%s(offset=%zu, len=%zu)\n",
> +                     __func__, offset, len);
> +
> +       remain = len / 2;
> +       vmem16 = (u16 *)(par->info->screen_buffer + offset);

> +       if (par->gpio.dc)

Useless duplicate check.

> +               gpiod_set_value(par->gpio.dc, 1);

> +       if (par->gpio.te) {
> +               enable_irq(gpiod_to_irq(par->gpio.te));

Here you should use the IRQ line rather than the GPIO descriptor. See above.

> +               reinit_completion(&spi_panel_te);
> +               ret = wait_for_completion_timeout(&spi_panel_te,
> +                                                 msecs_to_jiffies(SPI_PANEL_TE_TIMEOUT));
> +               if (ret == 0)
> +                       dev_err(par->info->device, "wait panel TE time out\n");
> +
> +               disable_irq(gpiod_to_irq(par->gpio.te));
> +       }

> +
> +       while (remain) {
> +               to_copy = min(tx_array_size, remain);

> +               dev_dbg(par->info->device, "    to_copy=%zu, remain=%zu\n",
> +                       to_copy, remain - to_copy);

Like in previous functions create a temporary variable to keep a
pointer to struct device and use it here and everywhere else. It might
save you LOCs and make code easier to read and understand.

> +               for (i = 0; i < to_copy; i++)
> +                       txbuf16[i] = cpu_to_be16(vmem16[i]);

If both of them are 16-bit wide, consider moving this to a helper
which somebody can move to byteorder/generic.h in the future.

> +               vmem16 = vmem16 + to_copy;
> +               ret = par->fbtftops.write(par, par->txbuf.buf,
> +                                        startbyte_size + to_copy * 2);
> +               if (ret < 0)
> +                       return ret;
> +               remain -= to_copy;
> +       }
> +
> +       return ret;
> +}

-- 
With Best Regards,
Andy Shevchenko
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v12] staging: fbtft: add tearing signal detect

[Andy Shevchenko]

On Thu, Jan 28, 2021 at 4:33 PM Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
> On Thu, Jan 28, 2021 at 2:58 PM Carlis <zhangxuezhi3@gmail.com> wrote:

...

> Taking all together you probably need to create a helper and use it
> inside init_display(), like
>
> static int init_tearing_effect_line(struct fbtft_par *par)
> {
>   struct device *dev = par->info->device;
>   struct gpio_desc *te;
>   int irq, rc;
>
>   te = gpiod_get_optional(dev, "te", GPIOD_IN);
>   if (IS_ERR(te))
>            return dev_err_probe(dev, PTR_ERR(te), "Failed to request
> te GPIO\n");

Sorry, here I missed the following:

  /* Absence of TE IRQ is not critical */
  if (!te)
    return 0;

>   irq = gpiod_to_irq(te); // this value you have to save in the
> driver's (per device) data structure.
>
>   /* GPIO is locked as an IRQ, we may drop the reference */
>   gpiod_put(te);

...and here:

  if (irq < 0)
    return irq;

>   init_completion(&spi_panel_te); // should be in the (per device)
> data structure
>   rc = devm_request_irq(dev, irq,  spi_panel_te_handler,
> IRQF_TRIGGER_RISING, "TE_GPIO", par);
>   if (rc)
>                 return dev_err_probe(dev, rc, "TE IRQ request failed.\n");
>   disable_irq_nosync(irq);
>   return irq;
> }

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v12] staging: fbtft: add tearing signal detect

[Andy Shevchenko]

On Thu, Jan 28, 2021 at 4:33 PM Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
> On Thu, Jan 28, 2021 at 2:58 PM Carlis <zhangxuezhi3@gmail.com> wrote:

...

> Taking all together you probably need to create a helper and use it
> inside init_display(), like
>
> static int init_tearing_effect_line(struct fbtft_par *par)
> {
>   struct device *dev = par->info->device;
>   struct gpio_desc *te;
>   int irq, rc;
>
>   te = gpiod_get_optional(dev, "te", GPIOD_IN);
>   if (IS_ERR(te))
>            return dev_err_probe(dev, PTR_ERR(te), "Failed to request
> te GPIO\n");

Sorry, here I missed the following:

  /* Absence of TE IRQ is not critical */
  if (!te)
    return 0;

>   irq = gpiod_to_irq(te); // this value you have to save in the
> driver's (per device) data structure.
>
>   /* GPIO is locked as an IRQ, we may drop the reference */
>   gpiod_put(te);

...and here:

  if (irq < 0)
    return irq;

>   init_completion(&spi_panel_te); // should be in the (per device)
> data structure
>   rc = devm_request_irq(dev, irq,  spi_panel_te_handler,
> IRQF_TRIGGER_RISING, "TE_GPIO", par);
>   if (rc)
>                 return dev_err_probe(dev, rc, "TE IRQ request failed.\n");
>   disable_irq_nosync(irq);
>   return irq;
> }

-- 
With Best Regards,
Andy Shevchenko
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v12] staging: fbtft: add tearing signal detect

[Andy Shevchenko]

On Thu, Jan 28, 2021 at 4:33 PM Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
> On Thu, Jan 28, 2021 at 2:58 PM Carlis <zhangxuezhi3@gmail.com> wrote:

...

> Taking all together you probably need to create a helper and use it
> inside init_display(), like
>
> static int init_tearing_effect_line(struct fbtft_par *par)
> {
>   struct device *dev = par->info->device;
>   struct gpio_desc *te;
>   int irq, rc;
>
>   te = gpiod_get_optional(dev, "te", GPIOD_IN);
>   if (IS_ERR(te))
>            return dev_err_probe(dev, PTR_ERR(te), "Failed to request
> te GPIO\n");

Sorry, here I missed the following:

  /* Absence of TE IRQ is not critical */
  if (!te)
    return 0;

>   irq = gpiod_to_irq(te); // this value you have to save in the
> driver's (per device) data structure.
>
>   /* GPIO is locked as an IRQ, we may drop the reference */
>   gpiod_put(te);

...and here:

  if (irq < 0)
    return irq;

>   init_completion(&spi_panel_te); // should be in the (per device)
> data structure
>   rc = devm_request_irq(dev, irq,  spi_panel_te_handler,
> IRQF_TRIGGER_RISING, "TE_GPIO", par);
>   if (rc)
>                 return dev_err_probe(dev, rc, "TE IRQ request failed.\n");
>   disable_irq_nosync(irq);
>   return irq;
> }

-- 
With Best Regards,
Andy Shevchenko
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v12] staging: fbtft: add tearing signal detect

[Dan Carpenter]

On Thu, Jan 28, 2021 at 04:33:02PM +0200, Andy Shevchenko wrote:
> > +               init_completion(&spi_panel_te);
> > +               rc = devm_request_irq(dev,
> 
> > +                                     gpiod_to_irq(par->gpio.te),
> 
> ...and here simply use irq.
> 
> > +                                    spi_panel_te_handler, IRQF_TRIGGER_RISING,
> > +                                    "TE_GPIO", par);
> 
> > +               if (IS_ERR(rc))
> 
> This is wrong. rc is integer no IS_ERR() is required. Ditto for
> PTR_ERR(). Have you even looked for these macros implementations?
> 

Yeah...  It leads to a compile warning:

	warning: passing argument 1 of ‘IS_ERR’ makes pointer from integer without a cast [-Wint-conversion]

regards,
dan carpenter

Re: [PATCH v12] staging: fbtft: add tearing signal detect

[Dan Carpenter]

On Thu, Jan 28, 2021 at 04:33:02PM +0200, Andy Shevchenko wrote:
> > +               init_completion(&spi_panel_te);
> > +               rc = devm_request_irq(dev,
> 
> > +                                     gpiod_to_irq(par->gpio.te),
> 
> ...and here simply use irq.
> 
> > +                                    spi_panel_te_handler, IRQF_TRIGGER_RISING,
> > +                                    "TE_GPIO", par);
> 
> > +               if (IS_ERR(rc))
> 
> This is wrong. rc is integer no IS_ERR() is required. Ditto for
> PTR_ERR(). Have you even looked for these macros implementations?
> 

Yeah...  It leads to a compile warning:

	warning: passing argument 1 of ‘IS_ERR’ makes pointer from integer without a cast [-Wint-conversion]

regards,
dan carpenter

_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v12] staging: fbtft: add tearing signal detect

[Dan Carpenter]

On Thu, Jan 28, 2021 at 04:33:02PM +0200, Andy Shevchenko wrote:
> > +               init_completion(&spi_panel_te);
> > +               rc = devm_request_irq(dev,
> 
> > +                                     gpiod_to_irq(par->gpio.te),
> 
> ...and here simply use irq.
> 
> > +                                    spi_panel_te_handler, IRQF_TRIGGER_RISING,
> > +                                    "TE_GPIO", par);
> 
> > +               if (IS_ERR(rc))
> 
> This is wrong. rc is integer no IS_ERR() is required. Ditto for
> PTR_ERR(). Have you even looked for these macros implementations?
> 

Yeah...  It leads to a compile warning:

	warning: passing argument 1 of ‘IS_ERR’ makes pointer from integer without a cast [-Wint-conversion]

regards,
dan carpenter

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v12] staging: fbtft: add tearing signal detect

[carlis]

On Thu, 28 Jan 2021 16:33:02 +0200
Andy Shevchenko <andy.shevchenko@gmail.com> wrote:

> On Thu, Jan 28, 2021 at 2:58 PM Carlis <zhangxuezhi3@gmail.com> wrote:
> 
> Thanks for your contribution, my comments below.
> 
> > From: zhangxuezhi <zhangxuezhi1@yulong.com>  
> 
> You probably have to configure your Git to use the same account for
> author and committer.

hi,you mean like below:
	Carlis <zhangxuezhi1@yulong.com>
?
> 
> > For st7789v ic,when we need continuous full screen refresh, it is
> > best to  
> 
> 'ic,when' -> 'IC, when'
> 
> > wait for the TE signal arrive to avoid screen tearing  
> 
> Decode TE for people who are not familiar with the abbreviation.
> 
> Missed period at the end of sentence.
> 
> ...
> 
> >  #include <linux/delay.h>
> >  #include <linux/init.h>
> >  #include <linux/kernel.h>
> > +#include <linux/interrupt.h>
> > +#include <linux/completion.h>
> >  #include <linux/module.h>
> > +#include <linux/gpio/consumer.h>
> > +  
> 
> Good, but I would rather squeeze it above to be more or less ordered,
> like just after delay.h inclusion.
> 
> >  #include <video/mipi_display.h>  
> 
> ...
> 
> > +#define SPI_PANEL_TE_TIMEOUT   400 /* msecs */  
> 
> Useless comment. Instead use _MS suffix in the name of constant.
> Besides that please add a comment explaining why this value has been
> chosen.
> 
> ...
> 
> > +static struct completion spi_panel_te;  
> 
> As Greg said.
> 
> ...
> 
> >  static int init_display(struct fbtft_par *par)
> >  {
> > +       int rc;
> > +       struct device *dev = par->info->device;  
> 
> Keep reversed xmas tree order:
> 
>        struct device *dev = par->info->device;
>        int rc;
> 
> ...
> 
> > +       par->gpio.te = devm_gpiod_get_index_optional(dev, "te", 0,
> > GPIOD_IN);  
> 
> No need to have it requested for all time since you use it as an IRQ
> later on. The IRQ chip will call the GPIO library framework to lock a
> pin as IRQ anyway.
> 
> > +       if (IS_ERR(par->gpio.te))
> > +               return dev_err_probe(par->info->device,
> > PTR_ERR(par->gpio.te),
> > +                                    "Failed to request te
> > gpio\n");  
> 
> > +       if (par->gpio.te) {  
> 
> Instead you should probably do the following:
> 
> int irq;
> 
> irq = gpiod_to_irq(...);
> if (irq > 0)
> 
> > +               init_completion(&spi_panel_te);
> > +               rc = devm_request_irq(dev,  
> 
> > +                                     gpiod_to_irq(par->gpio.te),  
> 
> ...and here simply use irq.
> 
> > +                                    spi_panel_te_handler,
> > IRQF_TRIGGER_RISING,
> > +                                    "TE_GPIO", par);  
> 
> > +               if (IS_ERR(rc))  
> 
> This is wrong. rc is integer no IS_ERR() is required. Ditto for
> PTR_ERR(). Have you even looked for these macros implementations?
> 
> > +                       return dev_err_probe(par->info->device,
> > PTR_ERR(rc),  
> 
> Use your temporary variable and move...
> 
> > +                                            "TE request_irq
> > failed.\n");  
> 
> ...this on the previous line.
> 
> > +               disable_irq_nosync(gpiod_to_irq(par->gpio.te));  
> 
> Why do you call gpio_to_irq() twice?
> 
> 
> > +       } else {
> > +               dev_info(par->info->device, "%s:%d, TE gpio not
> > specified\n",
> > +                        __func__, __LINE__);  
> 
> Remove this noise (besides the fact that we don't use __file__ and
> __LINE__ in messages like this.
> 
> > +       }  
> 
> Taking all together you probably need to create a helper and use it
> inside init_display(), like
> 
> static int init_tearing_effect_line(struct fbtft_par *par)
> {
>   struct device *dev = par->info->device;
>   struct gpio_desc *te;
>   int irq, rc;
> 
>   te = gpiod_get_optional(dev, "te", GPIOD_IN);
>   if (IS_ERR(te))
>            return dev_err_probe(dev, PTR_ERR(te), "Failed to request
> te GPIO\n");
> 
>   irq = gpiod_to_irq(te); // this value you have to save in the
> driver's (per device) data structure.
> 
>   /* GPIO is locked as an IRQ, we may drop the reference */
>   gpiod_put(te);


 
> 
>   init_completion(&spi_panel_te); // should be in the (per device)
> data structure
>   rc = devm_request_irq(dev, irq,  spi_panel_te_handler,
> IRQF_TRIGGER_RISING, "TE_GPIO", par);
>   if (rc)
>                 return dev_err_probe(dev, rc, "TE IRQ request
> failed.\n"); disable_irq_nosync(irq);
>   return irq;
> }
>

hi, i have modified it according to your suggestion like below:

static irqreturn_t panel_te_handler(int irq, void *data)
{
	struct fbtft_par *par = (struct fbtft_par *)data;

	complete(&par->panel_te);
	return IRQ_HANDLED;
}

/**
 * init_tearing_effect_line() - init tearing effect line
 *
 * @par: FBTFT parameter object
 *
 * Return: 0 on success, < 0 if error occurred.
 */
static int init_tearing_effect_line(struct fbtft_par *par)
{
	struct device *dev = par->info->device;
	struct gpio_desc *te;
	int rc;

	te = gpiod_get_optional(dev, "te", GPIOD_IN);
	if (IS_ERR(te))
		return dev_err_probe(dev, PTR_ERR(te), "Failed to
	request te GPIO\n");

	if (te) {
		par->irq_te = gpiod_to_irq(te);
		gpiod_put(te);

		if (par->irq_te) {
			rc = devm_request_irq(dev,
					      par->irq_te,
		panel_te_handler,
					      IRQF_TRIGGER_RISING,
		"TE_GPIO", par);
			if (rc)
				return dev_err_probe(dev, rc, "TE IRQ
		request failed.\n");

			disable_irq_nosync(par->irq_te);
			init_completion(&par->panel_te);
		} else {
			return dev_err_probe(dev, par->irq_te, "gpiod
			to TE IRQ failed.\n");
		}
	}

	return 0;
}

/**
 * init_display() - initialize the display controller
 *
 * @par: FBTFT parameter object
 *
 * Most of the commands in this init function set their parameters to
the
 * same default values which are already in place after the display has
been
 * powered up. (The main exception to this rule is the pixel format
which
 * would default to 18 instead of 16 bit per pixel.)
 * Nonetheless, this sequence can be used as a template for concrete
 * displays which usually need some adjustments.
 *
 * Return: 0 on success, < 0 if error occurred.
 */
static int init_display(struct fbtft_par *par)
{
	int rc;

	rc = init_tearing_effect_line(par);
	if (rc < 0)
		return rc;



......
	if (par->irq_te)
	write_reg(par, MIPI_DCS_SET_TEAR_ON, 0x00);

......


/**
 * st7789v_write_vmem16_bus8() - write data to display
 *
 * @par: FBTFT parameter object
 * @offset: offset from screen_buffer
 * @len: the length of data to be written
 *
 * 16 bit pixel over 8-bit databus
 *
 * Return: 0 on success, or a negative error code otherwise.
 */
static int st7789v_write_vmem16_bus8(struct fbtft_par *par, size_t
offset, size_t len)
{
	struct device *dev = par->info->device;
	__be16 *txbuf16 = par->txbuf.buf;
	size_t startbyte_size = 0;
	size_t tx_array_size;
	size_t to_copy;
	size_t remain;
	u16 *vmem16;
	int ret = 0;
	int i;

	fbtft_par_dbg(DEBUG_WRITE_VMEM, par, "st7789v ---%s(offset=%zu,
	len=%zu)\n",
		      __func__, offset, len);

	remain = len / 2;
	vmem16 = (u16 *)(par->info->screen_buffer + offset);

	gpiod_set_value(par->gpio.dc, 1);

	if (par->irq_te) {
		enable_irq(par->irq_te);
		reinit_completion(&par->panel_te);
		ret = wait_for_completion_timeout(&par->panel_te,
						  msecs_to_jiffies(PANEL_TE_TIMEOUT_MS));
		if (ret == 0)
			dev_err(dev, "wait panel TE time out\n");

		disable_irq(par->irq_te);
	}




> Note, when you define proper fields for IRQ line and completion in the
> data structure the above can be amended accordingly.
> 
> ...
> 
> > +       /* tearing effect line on */
> > +       if (par->gpio.te)
> > +               write_reg(par, 0x35, 0x00);  
> 
> 0x35 is defined. use it and drop useless comments.
> 
> ...
> 
> >  /**
> > + * st7789v_write_vmem16_bus8() -  write data to display  
> 
> > + *  
> 
> Redundant blank line.
> 
> > + * @par: FBTFT parameter object
> > + * @offset: offset from screen_buffer
> > + * @len: the length of data to be written
> > + *
> > + * 16 bit pixel over 8-bit databus
> > + *
> > + * Return: 0 on success, < 0 if error occurred.  
> 
> ", or a negative error code otherwise"
> 
> > + */  
> 
> > +  
> Redundant blank line
> 
> > +static int st7789v_write_vmem16_bus8(struct fbtft_par *par, size_t
> > offset, size_t len) +{
> > +       u16 *vmem16;
> > +       __be16 *txbuf16 = par->txbuf.buf;
> > +       size_t remain;
> > +       size_t to_copy;
> > +       size_t tx_array_size;
> > +       int i;
> > +       int ret = 0;
> > +       size_t startbyte_size = 0;  
> 
> Reversed xmas tree order.
> 
> > +       fbtft_par_dbg(DEBUG_WRITE_VMEM, par, "st7789v
> > ---%s(offset=%zu, len=%zu)\n",
> > +                     __func__, offset, len);
> > +
> > +       remain = len / 2;
> > +       vmem16 = (u16 *)(par->info->screen_buffer + offset);  
> 
> > +       if (par->gpio.dc)  
> 
> Useless duplicate check.
> 
> > +               gpiod_set_value(par->gpio.dc, 1);  
> 
> > +       if (par->gpio.te) {
> > +               enable_irq(gpiod_to_irq(par->gpio.te));  
> 
> Here you should use the IRQ line rather than the GPIO descriptor. See
> above.
> 
> > +               reinit_completion(&spi_panel_te);
> > +               ret = wait_for_completion_timeout(&spi_panel_te,
> > +
> > msecs_to_jiffies(SPI_PANEL_TE_TIMEOUT));
> > +               if (ret == 0)
> > +                       dev_err(par->info->device, "wait panel TE
> > time out\n"); +
> > +               disable_irq(gpiod_to_irq(par->gpio.te));
> > +       }  
> 
> > +
> > +       while (remain) {
> > +               to_copy = min(tx_array_size, remain);  
> 
> > +               dev_dbg(par->info->device, "    to_copy=%zu,
> > remain=%zu\n",
> > +                       to_copy, remain - to_copy);  
> 
> Like in previous functions create a temporary variable to keep a
> pointer to struct device and use it here and everywhere else. It might
> save you LOCs and make code easier to read and understand.
> 
> > +               for (i = 0; i < to_copy; i++)
> > +                       txbuf16[i] = cpu_to_be16(vmem16[i]);  
> 
> If both of them are 16-bit wide, consider moving this to a helper
> which somebody can move to byteorder/generic.h in the future.
> 
> > +               vmem16 = vmem16 + to_copy;
> > +               ret = par->fbtftops.write(par, par->txbuf.buf,
> > +                                        startbyte_size + to_copy *
> > 2);
> > +               if (ret < 0)
> > +                       return ret;
> > +               remain -= to_copy;
> > +       }
> > +
> > +       return ret;
> > +}  
> 

regards,
Carlis

Re: [PATCH v12] staging: fbtft: add tearing signal detect

[carlis]

On Thu, 28 Jan 2021 16:33:02 +0200
Andy Shevchenko <andy.shevchenko@gmail.com> wrote:

> On Thu, Jan 28, 2021 at 2:58 PM Carlis <zhangxuezhi3@gmail.com> wrote:
> 
> Thanks for your contribution, my comments below.
> 
> > From: zhangxuezhi <zhangxuezhi1@yulong.com>  
> 
> You probably have to configure your Git to use the same account for
> author and committer.

hi,you mean like below:
	Carlis <zhangxuezhi1@yulong.com>
?
> 
> > For st7789v ic,when we need continuous full screen refresh, it is
> > best to  
> 
> 'ic,when' -> 'IC, when'
> 
> > wait for the TE signal arrive to avoid screen tearing  
> 
> Decode TE for people who are not familiar with the abbreviation.
> 
> Missed period at the end of sentence.
> 
> ...
> 
> >  #include <linux/delay.h>
> >  #include <linux/init.h>
> >  #include <linux/kernel.h>
> > +#include <linux/interrupt.h>
> > +#include <linux/completion.h>
> >  #include <linux/module.h>
> > +#include <linux/gpio/consumer.h>
> > +  
> 
> Good, but I would rather squeeze it above to be more or less ordered,
> like just after delay.h inclusion.
> 
> >  #include <video/mipi_display.h>  
> 
> ...
> 
> > +#define SPI_PANEL_TE_TIMEOUT   400 /* msecs */  
> 
> Useless comment. Instead use _MS suffix in the name of constant.
> Besides that please add a comment explaining why this value has been
> chosen.
> 
> ...
> 
> > +static struct completion spi_panel_te;  
> 
> As Greg said.
> 
> ...
> 
> >  static int init_display(struct fbtft_par *par)
> >  {
> > +       int rc;
> > +       struct device *dev = par->info->device;  
> 
> Keep reversed xmas tree order:
> 
>        struct device *dev = par->info->device;
>        int rc;
> 
> ...
> 
> > +       par->gpio.te = devm_gpiod_get_index_optional(dev, "te", 0,
> > GPIOD_IN);  
> 
> No need to have it requested for all time since you use it as an IRQ
> later on. The IRQ chip will call the GPIO library framework to lock a
> pin as IRQ anyway.
> 
> > +       if (IS_ERR(par->gpio.te))
> > +               return dev_err_probe(par->info->device,
> > PTR_ERR(par->gpio.te),
> > +                                    "Failed to request te
> > gpio\n");  
> 
> > +       if (par->gpio.te) {  
> 
> Instead you should probably do the following:
> 
> int irq;
> 
> irq = gpiod_to_irq(...);
> if (irq > 0)
> 
> > +               init_completion(&spi_panel_te);
> > +               rc = devm_request_irq(dev,  
> 
> > +                                     gpiod_to_irq(par->gpio.te),  
> 
> ...and here simply use irq.
> 
> > +                                    spi_panel_te_handler,
> > IRQF_TRIGGER_RISING,
> > +                                    "TE_GPIO", par);  
> 
> > +               if (IS_ERR(rc))  
> 
> This is wrong. rc is integer no IS_ERR() is required. Ditto for
> PTR_ERR(). Have you even looked for these macros implementations?
> 
> > +                       return dev_err_probe(par->info->device,
> > PTR_ERR(rc),  
> 
> Use your temporary variable and move...
> 
> > +                                            "TE request_irq
> > failed.\n");  
> 
> ...this on the previous line.
> 
> > +               disable_irq_nosync(gpiod_to_irq(par->gpio.te));  
> 
> Why do you call gpio_to_irq() twice?
> 
> 
> > +       } else {
> > +               dev_info(par->info->device, "%s:%d, TE gpio not
> > specified\n",
> > +                        __func__, __LINE__);  
> 
> Remove this noise (besides the fact that we don't use __file__ and
> __LINE__ in messages like this.
> 
> > +       }  
> 
> Taking all together you probably need to create a helper and use it
> inside init_display(), like
> 
> static int init_tearing_effect_line(struct fbtft_par *par)
> {
>   struct device *dev = par->info->device;
>   struct gpio_desc *te;
>   int irq, rc;
> 
>   te = gpiod_get_optional(dev, "te", GPIOD_IN);
>   if (IS_ERR(te))
>            return dev_err_probe(dev, PTR_ERR(te), "Failed to request
> te GPIO\n");
> 
>   irq = gpiod_to_irq(te); // this value you have to save in the
> driver's (per device) data structure.
> 
>   /* GPIO is locked as an IRQ, we may drop the reference */
>   gpiod_put(te);


 
> 
>   init_completion(&spi_panel_te); // should be in the (per device)
> data structure
>   rc = devm_request_irq(dev, irq,  spi_panel_te_handler,
> IRQF_TRIGGER_RISING, "TE_GPIO", par);
>   if (rc)
>                 return dev_err_probe(dev, rc, "TE IRQ request
> failed.\n"); disable_irq_nosync(irq);
>   return irq;
> }
>

hi, i have modified it according to your suggestion like below:

static irqreturn_t panel_te_handler(int irq, void *data)
{
	struct fbtft_par *par = (struct fbtft_par *)data;

	complete(&par->panel_te);
	return IRQ_HANDLED;
}

/**
 * init_tearing_effect_line() - init tearing effect line
 *
 * @par: FBTFT parameter object
 *
 * Return: 0 on success, < 0 if error occurred.
 */
static int init_tearing_effect_line(struct fbtft_par *par)
{
	struct device *dev = par->info->device;
	struct gpio_desc *te;
	int rc;

	te = gpiod_get_optional(dev, "te", GPIOD_IN);
	if (IS_ERR(te))
		return dev_err_probe(dev, PTR_ERR(te), "Failed to
	request te GPIO\n");

	if (te) {
		par->irq_te = gpiod_to_irq(te);
		gpiod_put(te);

		if (par->irq_te) {
			rc = devm_request_irq(dev,
					      par->irq_te,
		panel_te_handler,
					      IRQF_TRIGGER_RISING,
		"TE_GPIO", par);
			if (rc)
				return dev_err_probe(dev, rc, "TE IRQ
		request failed.\n");

			disable_irq_nosync(par->irq_te);
			init_completion(&par->panel_te);
		} else {
			return dev_err_probe(dev, par->irq_te, "gpiod
			to TE IRQ failed.\n");
		}
	}

	return 0;
}

/**
 * init_display() - initialize the display controller
 *
 * @par: FBTFT parameter object
 *
 * Most of the commands in this init function set their parameters to
the
 * same default values which are already in place after the display has
been
 * powered up. (The main exception to this rule is the pixel format
which
 * would default to 18 instead of 16 bit per pixel.)
 * Nonetheless, this sequence can be used as a template for concrete
 * displays which usually need some adjustments.
 *
 * Return: 0 on success, < 0 if error occurred.
 */
static int init_display(struct fbtft_par *par)
{
	int rc;

	rc = init_tearing_effect_line(par);
	if (rc < 0)
		return rc;



......
	if (par->irq_te)
	write_reg(par, MIPI_DCS_SET_TEAR_ON, 0x00);

......


/**
 * st7789v_write_vmem16_bus8() - write data to display
 *
 * @par: FBTFT parameter object
 * @offset: offset from screen_buffer
 * @len: the length of data to be written
 *
 * 16 bit pixel over 8-bit databus
 *
 * Return: 0 on success, or a negative error code otherwise.
 */
static int st7789v_write_vmem16_bus8(struct fbtft_par *par, size_t
offset, size_t len)
{
	struct device *dev = par->info->device;
	__be16 *txbuf16 = par->txbuf.buf;
	size_t startbyte_size = 0;
	size_t tx_array_size;
	size_t to_copy;
	size_t remain;
	u16 *vmem16;
	int ret = 0;
	int i;

	fbtft_par_dbg(DEBUG_WRITE_VMEM, par, "st7789v ---%s(offset=%zu,
	len=%zu)\n",
		      __func__, offset, len);

	remain = len / 2;
	vmem16 = (u16 *)(par->info->screen_buffer + offset);

	gpiod_set_value(par->gpio.dc, 1);

	if (par->irq_te) {
		enable_irq(par->irq_te);
		reinit_completion(&par->panel_te);
		ret = wait_for_completion_timeout(&par->panel_te,
						  msecs_to_jiffies(PANEL_TE_TIMEOUT_MS));
		if (ret == 0)
			dev_err(dev, "wait panel TE time out\n");

		disable_irq(par->irq_te);
	}




> Note, when you define proper fields for IRQ line and completion in the
> data structure the above can be amended accordingly.
> 
> ...
> 
> > +       /* tearing effect line on */
> > +       if (par->gpio.te)
> > +               write_reg(par, 0x35, 0x00);  
> 
> 0x35 is defined. use it and drop useless comments.
> 
> ...
> 
> >  /**
> > + * st7789v_write_vmem16_bus8() -  write data to display  
> 
> > + *  
> 
> Redundant blank line.
> 
> > + * @par: FBTFT parameter object
> > + * @offset: offset from screen_buffer
> > + * @len: the length of data to be written
> > + *
> > + * 16 bit pixel over 8-bit databus
> > + *
> > + * Return: 0 on success, < 0 if error occurred.  
> 
> ", or a negative error code otherwise"
> 
> > + */  
> 
> > +  
> Redundant blank line
> 
> > +static int st7789v_write_vmem16_bus8(struct fbtft_par *par, size_t
> > offset, size_t len) +{
> > +       u16 *vmem16;
> > +       __be16 *txbuf16 = par->txbuf.buf;
> > +       size_t remain;
> > +       size_t to_copy;
> > +       size_t tx_array_size;
> > +       int i;
> > +       int ret = 0;
> > +       size_t startbyte_size = 0;  
> 
> Reversed xmas tree order.
> 
> > +       fbtft_par_dbg(DEBUG_WRITE_VMEM, par, "st7789v
> > ---%s(offset=%zu, len=%zu)\n",
> > +                     __func__, offset, len);
> > +
> > +       remain = len / 2;
> > +       vmem16 = (u16 *)(par->info->screen_buffer + offset);  
> 
> > +       if (par->gpio.dc)  
> 
> Useless duplicate check.
> 
> > +               gpiod_set_value(par->gpio.dc, 1);  
> 
> > +       if (par->gpio.te) {
> > +               enable_irq(gpiod_to_irq(par->gpio.te));  
> 
> Here you should use the IRQ line rather than the GPIO descriptor. See
> above.
> 
> > +               reinit_completion(&spi_panel_te);
> > +               ret = wait_for_completion_timeout(&spi_panel_te,
> > +
> > msecs_to_jiffies(SPI_PANEL_TE_TIMEOUT));
> > +               if (ret == 0)
> > +                       dev_err(par->info->device, "wait panel TE
> > time out\n"); +
> > +               disable_irq(gpiod_to_irq(par->gpio.te));
> > +       }  
> 
> > +
> > +       while (remain) {
> > +               to_copy = min(tx_array_size, remain);  
> 
> > +               dev_dbg(par->info->device, "    to_copy=%zu,
> > remain=%zu\n",
> > +                       to_copy, remain - to_copy);  
> 
> Like in previous functions create a temporary variable to keep a
> pointer to struct device and use it here and everywhere else. It might
> save you LOCs and make code easier to read and understand.
> 
> > +               for (i = 0; i < to_copy; i++)
> > +                       txbuf16[i] = cpu_to_be16(vmem16[i]);  
> 
> If both of them are 16-bit wide, consider moving this to a helper
> which somebody can move to byteorder/generic.h in the future.
> 
> > +               vmem16 = vmem16 + to_copy;
> > +               ret = par->fbtftops.write(par, par->txbuf.buf,
> > +                                        startbyte_size + to_copy *
> > 2);
> > +               if (ret < 0)
> > +                       return ret;
> > +               remain -= to_copy;
> > +       }
> > +
> > +       return ret;
> > +}  
> 

regards,
Carlis
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v12] staging: fbtft: add tearing signal detect

[carlis]

On Thu, 28 Jan 2021 16:33:02 +0200
Andy Shevchenko <andy.shevchenko@gmail.com> wrote:

> On Thu, Jan 28, 2021 at 2:58 PM Carlis <zhangxuezhi3@gmail.com> wrote:
> 
> Thanks for your contribution, my comments below.
> 
> > From: zhangxuezhi <zhangxuezhi1@yulong.com>  
> 
> You probably have to configure your Git to use the same account for
> author and committer.

hi,you mean like below:
	Carlis <zhangxuezhi1@yulong.com>
?
> 
> > For st7789v ic,when we need continuous full screen refresh, it is
> > best to  
> 
> 'ic,when' -> 'IC, when'
> 
> > wait for the TE signal arrive to avoid screen tearing  
> 
> Decode TE for people who are not familiar with the abbreviation.
> 
> Missed period at the end of sentence.
> 
> ...
> 
> >  #include <linux/delay.h>
> >  #include <linux/init.h>
> >  #include <linux/kernel.h>
> > +#include <linux/interrupt.h>
> > +#include <linux/completion.h>
> >  #include <linux/module.h>
> > +#include <linux/gpio/consumer.h>
> > +  
> 
> Good, but I would rather squeeze it above to be more or less ordered,
> like just after delay.h inclusion.
> 
> >  #include <video/mipi_display.h>  
> 
> ...
> 
> > +#define SPI_PANEL_TE_TIMEOUT   400 /* msecs */  
> 
> Useless comment. Instead use _MS suffix in the name of constant.
> Besides that please add a comment explaining why this value has been
> chosen.
> 
> ...
> 
> > +static struct completion spi_panel_te;  
> 
> As Greg said.
> 
> ...
> 
> >  static int init_display(struct fbtft_par *par)
> >  {
> > +       int rc;
> > +       struct device *dev = par->info->device;  
> 
> Keep reversed xmas tree order:
> 
>        struct device *dev = par->info->device;
>        int rc;
> 
> ...
> 
> > +       par->gpio.te = devm_gpiod_get_index_optional(dev, "te", 0,
> > GPIOD_IN);  
> 
> No need to have it requested for all time since you use it as an IRQ
> later on. The IRQ chip will call the GPIO library framework to lock a
> pin as IRQ anyway.
> 
> > +       if (IS_ERR(par->gpio.te))
> > +               return dev_err_probe(par->info->device,
> > PTR_ERR(par->gpio.te),
> > +                                    "Failed to request te
> > gpio\n");  
> 
> > +       if (par->gpio.te) {  
> 
> Instead you should probably do the following:
> 
> int irq;
> 
> irq = gpiod_to_irq(...);
> if (irq > 0)
> 
> > +               init_completion(&spi_panel_te);
> > +               rc = devm_request_irq(dev,  
> 
> > +                                     gpiod_to_irq(par->gpio.te),  
> 
> ...and here simply use irq.
> 
> > +                                    spi_panel_te_handler,
> > IRQF_TRIGGER_RISING,
> > +                                    "TE_GPIO", par);  
> 
> > +               if (IS_ERR(rc))  
> 
> This is wrong. rc is integer no IS_ERR() is required. Ditto for
> PTR_ERR(). Have you even looked for these macros implementations?
> 
> > +                       return dev_err_probe(par->info->device,
> > PTR_ERR(rc),  
> 
> Use your temporary variable and move...
> 
> > +                                            "TE request_irq
> > failed.\n");  
> 
> ...this on the previous line.
> 
> > +               disable_irq_nosync(gpiod_to_irq(par->gpio.te));  
> 
> Why do you call gpio_to_irq() twice?
> 
> 
> > +       } else {
> > +               dev_info(par->info->device, "%s:%d, TE gpio not
> > specified\n",
> > +                        __func__, __LINE__);  
> 
> Remove this noise (besides the fact that we don't use __file__ and
> __LINE__ in messages like this.
> 
> > +       }  
> 
> Taking all together you probably need to create a helper and use it
> inside init_display(), like
> 
> static int init_tearing_effect_line(struct fbtft_par *par)
> {
>   struct device *dev = par->info->device;
>   struct gpio_desc *te;
>   int irq, rc;
> 
>   te = gpiod_get_optional(dev, "te", GPIOD_IN);
>   if (IS_ERR(te))
>            return dev_err_probe(dev, PTR_ERR(te), "Failed to request
> te GPIO\n");
> 
>   irq = gpiod_to_irq(te); // this value you have to save in the
> driver's (per device) data structure.
> 
>   /* GPIO is locked as an IRQ, we may drop the reference */
>   gpiod_put(te);


 
> 
>   init_completion(&spi_panel_te); // should be in the (per device)
> data structure
>   rc = devm_request_irq(dev, irq,  spi_panel_te_handler,
> IRQF_TRIGGER_RISING, "TE_GPIO", par);
>   if (rc)
>                 return dev_err_probe(dev, rc, "TE IRQ request
> failed.\n"); disable_irq_nosync(irq);
>   return irq;
> }
>

hi, i have modified it according to your suggestion like below:

static irqreturn_t panel_te_handler(int irq, void *data)
{
	struct fbtft_par *par = (struct fbtft_par *)data;

	complete(&par->panel_te);
	return IRQ_HANDLED;
}

/**
 * init_tearing_effect_line() - init tearing effect line
 *
 * @par: FBTFT parameter object
 *
 * Return: 0 on success, < 0 if error occurred.
 */
static int init_tearing_effect_line(struct fbtft_par *par)
{
	struct device *dev = par->info->device;
	struct gpio_desc *te;
	int rc;

	te = gpiod_get_optional(dev, "te", GPIOD_IN);
	if (IS_ERR(te))
		return dev_err_probe(dev, PTR_ERR(te), "Failed to
	request te GPIO\n");

	if (te) {
		par->irq_te = gpiod_to_irq(te);
		gpiod_put(te);

		if (par->irq_te) {
			rc = devm_request_irq(dev,
					      par->irq_te,
		panel_te_handler,
					      IRQF_TRIGGER_RISING,
		"TE_GPIO", par);
			if (rc)
				return dev_err_probe(dev, rc, "TE IRQ
		request failed.\n");

			disable_irq_nosync(par->irq_te);
			init_completion(&par->panel_te);
		} else {
			return dev_err_probe(dev, par->irq_te, "gpiod
			to TE IRQ failed.\n");
		}
	}

	return 0;
}

/**
 * init_display() - initialize the display controller
 *
 * @par: FBTFT parameter object
 *
 * Most of the commands in this init function set their parameters to
the
 * same default values which are already in place after the display has
been
 * powered up. (The main exception to this rule is the pixel format
which
 * would default to 18 instead of 16 bit per pixel.)
 * Nonetheless, this sequence can be used as a template for concrete
 * displays which usually need some adjustments.
 *
 * Return: 0 on success, < 0 if error occurred.
 */
static int init_display(struct fbtft_par *par)
{
	int rc;

	rc = init_tearing_effect_line(par);
	if (rc < 0)
		return rc;



......
	if (par->irq_te)
	write_reg(par, MIPI_DCS_SET_TEAR_ON, 0x00);

......


/**
 * st7789v_write_vmem16_bus8() - write data to display
 *
 * @par: FBTFT parameter object
 * @offset: offset from screen_buffer
 * @len: the length of data to be written
 *
 * 16 bit pixel over 8-bit databus
 *
 * Return: 0 on success, or a negative error code otherwise.
 */
static int st7789v_write_vmem16_bus8(struct fbtft_par *par, size_t
offset, size_t len)
{
	struct device *dev = par->info->device;
	__be16 *txbuf16 = par->txbuf.buf;
	size_t startbyte_size = 0;
	size_t tx_array_size;
	size_t to_copy;
	size_t remain;
	u16 *vmem16;
	int ret = 0;
	int i;

	fbtft_par_dbg(DEBUG_WRITE_VMEM, par, "st7789v ---%s(offset=%zu,
	len=%zu)\n",
		      __func__, offset, len);

	remain = len / 2;
	vmem16 = (u16 *)(par->info->screen_buffer + offset);

	gpiod_set_value(par->gpio.dc, 1);

	if (par->irq_te) {
		enable_irq(par->irq_te);
		reinit_completion(&par->panel_te);
		ret = wait_for_completion_timeout(&par->panel_te,
						  msecs_to_jiffies(PANEL_TE_TIMEOUT_MS));
		if (ret == 0)
			dev_err(dev, "wait panel TE time out\n");

		disable_irq(par->irq_te);
	}




> Note, when you define proper fields for IRQ line and completion in the
> data structure the above can be amended accordingly.
> 
> ...
> 
> > +       /* tearing effect line on */
> > +       if (par->gpio.te)
> > +               write_reg(par, 0x35, 0x00);  
> 
> 0x35 is defined. use it and drop useless comments.
> 
> ...
> 
> >  /**
> > + * st7789v_write_vmem16_bus8() -  write data to display  
> 
> > + *  
> 
> Redundant blank line.
> 
> > + * @par: FBTFT parameter object
> > + * @offset: offset from screen_buffer
> > + * @len: the length of data to be written
> > + *
> > + * 16 bit pixel over 8-bit databus
> > + *
> > + * Return: 0 on success, < 0 if error occurred.  
> 
> ", or a negative error code otherwise"
> 
> > + */  
> 
> > +  
> Redundant blank line
> 
> > +static int st7789v_write_vmem16_bus8(struct fbtft_par *par, size_t
> > offset, size_t len) +{
> > +       u16 *vmem16;
> > +       __be16 *txbuf16 = par->txbuf.buf;
> > +       size_t remain;
> > +       size_t to_copy;
> > +       size_t tx_array_size;
> > +       int i;
> > +       int ret = 0;
> > +       size_t startbyte_size = 0;  
> 
> Reversed xmas tree order.
> 
> > +       fbtft_par_dbg(DEBUG_WRITE_VMEM, par, "st7789v
> > ---%s(offset=%zu, len=%zu)\n",
> > +                     __func__, offset, len);
> > +
> > +       remain = len / 2;
> > +       vmem16 = (u16 *)(par->info->screen_buffer + offset);  
> 
> > +       if (par->gpio.dc)  
> 
> Useless duplicate check.
> 
> > +               gpiod_set_value(par->gpio.dc, 1);  
> 
> > +       if (par->gpio.te) {
> > +               enable_irq(gpiod_to_irq(par->gpio.te));  
> 
> Here you should use the IRQ line rather than the GPIO descriptor. See
> above.
> 
> > +               reinit_completion(&spi_panel_te);
> > +               ret = wait_for_completion_timeout(&spi_panel_te,
> > +
> > msecs_to_jiffies(SPI_PANEL_TE_TIMEOUT));
> > +               if (ret == 0)
> > +                       dev_err(par->info->device, "wait panel TE
> > time out\n"); +
> > +               disable_irq(gpiod_to_irq(par->gpio.te));
> > +       }  
> 
> > +
> > +       while (remain) {
> > +               to_copy = min(tx_array_size, remain);  
> 
> > +               dev_dbg(par->info->device, "    to_copy=%zu,
> > remain=%zu\n",
> > +                       to_copy, remain - to_copy);  
> 
> Like in previous functions create a temporary variable to keep a
> pointer to struct device and use it here and everywhere else. It might
> save you LOCs and make code easier to read and understand.
> 
> > +               for (i = 0; i < to_copy; i++)
> > +                       txbuf16[i] = cpu_to_be16(vmem16[i]);  
> 
> If both of them are 16-bit wide, consider moving this to a helper
> which somebody can move to byteorder/generic.h in the future.
> 
> > +               vmem16 = vmem16 + to_copy;
> > +               ret = par->fbtftops.write(par, par->txbuf.buf,
> > +                                        startbyte_size + to_copy *
> > 2);
> > +               if (ret < 0)
> > +                       return ret;
> > +               remain -= to_copy;
> > +       }
> > +
> > +       return ret;
> > +}  
> 

regards,
Carlis
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v12] staging: fbtft: add tearing signal detect

[Andy Shevchenko]

On Fri, Jan 29, 2021 at 7:01 AM carlis <zhangxuezhi3@gmail.com> wrote:
> On Thu, 28 Jan 2021 16:33:02 +0200
> Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
> > On Thu, Jan 28, 2021 at 2:58 PM Carlis <zhangxuezhi3@gmail.com> wrote:
> >
> > Thanks for your contribution, my comments below.
> >
> > > From: zhangxuezhi <zhangxuezhi1@yulong.com>
> >
> > You probably have to configure your Git to use the same account for
> > author and committer.
>
> hi,you mean like below:
>         Carlis <zhangxuezhi1@yulong.com>
> ?

I meant that you shouldn't probably have a From: line in the commit message.

...

> hi, i have modified it according to your suggestion like below:

Please, go again thru my comments and comments from others and
carefully address all of them everywhere in your contribution. If you
have questions, ask them in reply in the corresponding context.

...

> /**
>  * init_tearing_effect_line() - init tearing effect line

>  *

For example, above was commented on and hasn't been addressed here.

>  * @par: FBTFT parameter object
>  *
>  * Return: 0 on success, < 0 if error occurred.
>  */
> static int init_tearing_effect_line(struct fbtft_par *par)
> {
>         struct device *dev = par->info->device;
>         struct gpio_desc *te;
>         int rc;
>
>         te = gpiod_get_optional(dev, "te", GPIOD_IN);
>         if (IS_ERR(te))
>                 return dev_err_probe(dev, PTR_ERR(te), "Failed to
>         request te GPIO\n");
>

>         if (te) {

This one is not like I suggested.

>                 par->irq_te = gpiod_to_irq(te);
>                 gpiod_put(te);
>

>                 if (par->irq_te) {

This is wrong.

>                         rc = devm_request_irq(dev,
>                                               par->irq_te,
>                 panel_te_handler,
>                                               IRQF_TRIGGER_RISING,
>                 "TE_GPIO", par);

Try to use less LOCs.

>                         if (rc)
>                                 return dev_err_probe(dev, rc, "TE IRQ
>                 request failed.\n");
>
>                         disable_irq_nosync(par->irq_te);
>                         init_completion(&par->panel_te);

>                 } else {
>                         return dev_err_probe(dev, par->irq_te, "gpiod
>                         to TE IRQ failed.\n");
>                 }

Again, it is not what had been suggested.

>         }
>
>         return 0;
> }

The rest is better, but we will see later on when you submit a new
version (And I feel it won't be last).

-- 
With Best Regards,
Andy Shevchenko
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v12] staging: fbtft: add tearing signal detect

[Andy Shevchenko]

On Fri, Jan 29, 2021 at 7:01 AM carlis <zhangxuezhi3@gmail.com> wrote:
> On Thu, 28 Jan 2021 16:33:02 +0200
> Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
> > On Thu, Jan 28, 2021 at 2:58 PM Carlis <zhangxuezhi3@gmail.com> wrote:
> >
> > Thanks for your contribution, my comments below.
> >
> > > From: zhangxuezhi <zhangxuezhi1@yulong.com>
> >
> > You probably have to configure your Git to use the same account for
> > author and committer.
>
> hi,you mean like below:
>         Carlis <zhangxuezhi1@yulong.com>
> ?

I meant that you shouldn't probably have a From: line in the commit message.

...

> hi, i have modified it according to your suggestion like below:

Please, go again thru my comments and comments from others and
carefully address all of them everywhere in your contribution. If you
have questions, ask them in reply in the corresponding context.

...

> /**
>  * init_tearing_effect_line() - init tearing effect line

>  *

For example, above was commented on and hasn't been addressed here.

>  * @par: FBTFT parameter object
>  *
>  * Return: 0 on success, < 0 if error occurred.
>  */
> static int init_tearing_effect_line(struct fbtft_par *par)
> {
>         struct device *dev = par->info->device;
>         struct gpio_desc *te;
>         int rc;
>
>         te = gpiod_get_optional(dev, "te", GPIOD_IN);
>         if (IS_ERR(te))
>                 return dev_err_probe(dev, PTR_ERR(te), "Failed to
>         request te GPIO\n");
>

>         if (te) {

This one is not like I suggested.

>                 par->irq_te = gpiod_to_irq(te);
>                 gpiod_put(te);
>

>                 if (par->irq_te) {

This is wrong.

>                         rc = devm_request_irq(dev,
>                                               par->irq_te,
>                 panel_te_handler,
>                                               IRQF_TRIGGER_RISING,
>                 "TE_GPIO", par);

Try to use less LOCs.

>                         if (rc)
>                                 return dev_err_probe(dev, rc, "TE IRQ
>                 request failed.\n");
>
>                         disable_irq_nosync(par->irq_te);
>                         init_completion(&par->panel_te);

>                 } else {
>                         return dev_err_probe(dev, par->irq_te, "gpiod
>                         to TE IRQ failed.\n");
>                 }

Again, it is not what had been suggested.

>         }
>
>         return 0;
> }

The rest is better, but we will see later on when you submit a new
version (And I feel it won't be last).

-- 
With Best Regards,
Andy Shevchenko
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v12] staging: fbtft: add tearing signal detect

[Greg Kroah-Hartman]

On Fri, Jan 29, 2021 at 12:23:08PM +0200, Andy Shevchenko wrote:
> On Fri, Jan 29, 2021 at 7:01 AM carlis <zhangxuezhi3@gmail.com> wrote:
> > On Thu, 28 Jan 2021 16:33:02 +0200
> > Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
> > > On Thu, Jan 28, 2021 at 2:58 PM Carlis <zhangxuezhi3@gmail.com> wrote:
> > >
> > > Thanks for your contribution, my comments below.
> > >
> > > > From: zhangxuezhi <zhangxuezhi1@yulong.com>
> > >
> > > You probably have to configure your Git to use the same account for
> > > author and committer.
> >
> > hi,you mean like below:
> >         Carlis <zhangxuezhi1@yulong.com>
> > ?
> 
> I meant that you shouldn't probably have a From: line in the commit message.

It's fine, some people like to be sure about this, there is no problem
with it being there at all.

thanks,

greg k-h
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v12] staging: fbtft: add tearing signal detect

[Greg Kroah-Hartman]

On Fri, Jan 29, 2021 at 12:23:08PM +0200, Andy Shevchenko wrote:
> On Fri, Jan 29, 2021 at 7:01 AM carlis <zhangxuezhi3@gmail.com> wrote:
> > On Thu, 28 Jan 2021 16:33:02 +0200
> > Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
> > > On Thu, Jan 28, 2021 at 2:58 PM Carlis <zhangxuezhi3@gmail.com> wrote:
> > >
> > > Thanks for your contribution, my comments below.
> > >
> > > > From: zhangxuezhi <zhangxuezhi1@yulong.com>
> > >
> > > You probably have to configure your Git to use the same account for
> > > author and committer.
> >
> > hi,you mean like below:
> >         Carlis <zhangxuezhi1@yulong.com>
> > ?
> 
> I meant that you shouldn't probably have a From: line in the commit message.

It's fine, some people like to be sure about this, there is no problem
with it being there at all.

thanks,

greg k-h
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v12] staging: fbtft: add tearing signal detect

[carlis]

On Fri, 29 Jan 2021 12:23:08 +0200
Andy Shevchenko <andy.shevchenko@gmail.com> wrote:

> On Fri, Jan 29, 2021 at 7:01 AM carlis <zhangxuezhi3@gmail.com> wrote:
> > On Thu, 28 Jan 2021 16:33:02 +0200
> > Andy Shevchenko <andy.shevchenko@gmail.com> wrote:  
> > > On Thu, Jan 28, 2021 at 2:58 PM Carlis <zhangxuezhi3@gmail.com>
> > > wrote:
> > >
> > > Thanks for your contribution, my comments below.
> > >  
> > > > From: zhangxuezhi <zhangxuezhi1@yulong.com>  
> > >
> > > You probably have to configure your Git to use the same account
> > > for author and committer.  
> >
> > hi,you mean like below:
> >         Carlis <zhangxuezhi1@yulong.com>
> > ?  
> 
> I meant that you shouldn't probably have a From: line in the commit
> message.
> 
> ...
> 
> > hi, i have modified it according to your suggestion like below:  
> 
> Please, go again thru my comments and comments from others and
> carefully address all of them everywhere in your contribution. If you
> have questions, ask them in reply in the corresponding context.
> 
> ...
> 
> > /**
> >  * init_tearing_effect_line() - init tearing effect line  
> 
> >  *  
> 
> For example, above was commented on and hasn't been addressed here.
> 
hi,here i can not get you.....
> >  * @par: FBTFT parameter object
> >  *
> >  * Return: 0 on success, < 0 if error occurred.
> >  */
> > static int init_tearing_effect_line(struct fbtft_par *par)
> > {
> >         struct device *dev = par->info->device;
> >         struct gpio_desc *te;
> >         int rc;
> >
> >         te = gpiod_get_optional(dev, "te", GPIOD_IN);
> >         if (IS_ERR(te))
> >                 return dev_err_probe(dev, PTR_ERR(te), "Failed to
> >         request te GPIO\n");
> >  
> 
> >         if (te) {  
> 
> This one is not like I suggested.
Why? My thinking is that if the TE is not configured and NULL is
returned, the initialization can still proceed.....
> 
> >                 par->irq_te = gpiod_to_irq(te);
> >                 gpiod_put(te);
> >  
> 
> >                 if (par->irq_te) {  
> 
> This is wrong.

Why? i have read gpiod_to_irq code, if an error occurs, a negative
value is returned, and zero is not possible,so I need this value to
determine if TE IRQ is configured

> 
> >                         rc = devm_request_irq(dev,
> >                                               par->irq_te,
> >                 panel_te_handler,
> >                                               IRQF_TRIGGER_RISING,
> >                 "TE_GPIO", par);  
> 
> Try to use less LOCs.
> 
> >                         if (rc)
> >                                 return dev_err_probe(dev, rc, "TE
> > IRQ request failed.\n");
> >
> >                         disable_irq_nosync(par->irq_te);
> >                         init_completion(&par->panel_te);  
> 
> >                 } else {
> >                         return dev_err_probe(dev, par->irq_te,
> > "gpiod to TE IRQ failed.\n");
> >                 }  
> 
> Again, it is not what had been suggested.
> 
> >         }
> >
> >         return 0;
> > }  
> 
> The rest is better, but we will see later on when you submit a new
> version (And I feel it won't be last).
> 

_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v12] staging: fbtft: add tearing signal detect

[carlis]

On Fri, 29 Jan 2021 12:23:08 +0200
Andy Shevchenko <andy.shevchenko@gmail.com> wrote:

> On Fri, Jan 29, 2021 at 7:01 AM carlis <zhangxuezhi3@gmail.com> wrote:
> > On Thu, 28 Jan 2021 16:33:02 +0200
> > Andy Shevchenko <andy.shevchenko@gmail.com> wrote:  
> > > On Thu, Jan 28, 2021 at 2:58 PM Carlis <zhangxuezhi3@gmail.com>
> > > wrote:
> > >
> > > Thanks for your contribution, my comments below.
> > >  
> > > > From: zhangxuezhi <zhangxuezhi1@yulong.com>  
> > >
> > > You probably have to configure your Git to use the same account
> > > for author and committer.  
> >
> > hi,you mean like below:
> >         Carlis <zhangxuezhi1@yulong.com>
> > ?  
> 
> I meant that you shouldn't probably have a From: line in the commit
> message.
> 
> ...
> 
> > hi, i have modified it according to your suggestion like below:  
> 
> Please, go again thru my comments and comments from others and
> carefully address all of them everywhere in your contribution. If you
> have questions, ask them in reply in the corresponding context.
> 
> ...
> 
> > /**
> >  * init_tearing_effect_line() - init tearing effect line  
> 
> >  *  
> 
> For example, above was commented on and hasn't been addressed here.
> 
hi,here i can not get you.....
> >  * @par: FBTFT parameter object
> >  *
> >  * Return: 0 on success, < 0 if error occurred.
> >  */
> > static int init_tearing_effect_line(struct fbtft_par *par)
> > {
> >         struct device *dev = par->info->device;
> >         struct gpio_desc *te;
> >         int rc;
> >
> >         te = gpiod_get_optional(dev, "te", GPIOD_IN);
> >         if (IS_ERR(te))
> >                 return dev_err_probe(dev, PTR_ERR(te), "Failed to
> >         request te GPIO\n");
> >  
> 
> >         if (te) {  
> 
> This one is not like I suggested.
Why? My thinking is that if the TE is not configured and NULL is
returned, the initialization can still proceed.....
> 
> >                 par->irq_te = gpiod_to_irq(te);
> >                 gpiod_put(te);
> >  
> 
> >                 if (par->irq_te) {  
> 
> This is wrong.

Why? i have read gpiod_to_irq code, if an error occurs, a negative
value is returned, and zero is not possible,so I need this value to
determine if TE IRQ is configured

> 
> >                         rc = devm_request_irq(dev,
> >                                               par->irq_te,
> >                 panel_te_handler,
> >                                               IRQF_TRIGGER_RISING,
> >                 "TE_GPIO", par);  
> 
> Try to use less LOCs.
> 
> >                         if (rc)
> >                                 return dev_err_probe(dev, rc, "TE
> > IRQ request failed.\n");
> >
> >                         disable_irq_nosync(par->irq_te);
> >                         init_completion(&par->panel_te);  
> 
> >                 } else {
> >                         return dev_err_probe(dev, par->irq_te,
> > "gpiod to TE IRQ failed.\n");
> >                 }  
> 
> Again, it is not what had been suggested.
> 
> >         }
> >
> >         return 0;
> > }  
> 
> The rest is better, but we will see later on when you submit a new
> version (And I feel it won't be last).
> 

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v12] staging: fbtft: add tearing signal detect

[carlis]

On Fri, 29 Jan 2021 12:23:08 +0200
Andy Shevchenko <andy.shevchenko@gmail.com> wrote:

> On Fri, Jan 29, 2021 at 7:01 AM carlis <zhangxuezhi3@gmail.com> wrote:
> > On Thu, 28 Jan 2021 16:33:02 +0200
> > Andy Shevchenko <andy.shevchenko@gmail.com> wrote:  
> > > On Thu, Jan 28, 2021 at 2:58 PM Carlis <zhangxuezhi3@gmail.com>
> > > wrote:
> > >
> > > Thanks for your contribution, my comments below.
> > >  
> > > > From: zhangxuezhi <zhangxuezhi1@yulong.com>  
> > >
> > > You probably have to configure your Git to use the same account
> > > for author and committer.  
> >
> > hi,you mean like below:
> >         Carlis <zhangxuezhi1@yulong.com>
> > ?  
> 
> I meant that you shouldn't probably have a From: line in the commit
> message.
> 
> ...
> 
> > hi, i have modified it according to your suggestion like below:  
> 
> Please, go again thru my comments and comments from others and
> carefully address all of them everywhere in your contribution. If you
> have questions, ask them in reply in the corresponding context.
> 
> ...
> 
> > /**
> >  * init_tearing_effect_line() - init tearing effect line  
> 
> >  *  
> 
> For example, above was commented on and hasn't been addressed here.
> 
> >  * @par: FBTFT parameter object
> >  *
> >  * Return: 0 on success, < 0 if error occurred.
> >  */
> > static int init_tearing_effect_line(struct fbtft_par *par)
> > {
> >         struct device *dev = par->info->device;
> >         struct gpio_desc *te;
> >         int rc;
> >
> >         te = gpiod_get_optional(dev, "te", GPIOD_IN);
> >         if (IS_ERR(te))
> >                 return dev_err_probe(dev, PTR_ERR(te), "Failed to
> >         request te GPIO\n");
> >  
> 
> >         if (te) {  
> 
> This one is not like I suggested.
I don't think I have a problem here, if te GPIO is not configured, it
should return NULL, if it is configured, it should be greater than 0 
> 
> >                 par->irq_te = gpiod_to_irq(te);
> >                 gpiod_put(te);
> >  
> 
> >                 if (par->irq_te) {  
> 
> This is wrong.
> 
> >                         rc = devm_request_irq(dev,
> >                                               par->irq_te,
> >                 panel_te_handler,
> >                                               IRQF_TRIGGER_RISING,
> >                 "TE_GPIO", par);  
> 
> Try to use less LOCs.

LOCs???? i can not get you 
> 
> >                         if (rc)
> >                                 return dev_err_probe(dev, rc, "TE
> > IRQ request failed.\n");
> >
> >                         disable_irq_nosync(par->irq_te);
> >                         init_completion(&par->panel_te);  
> 
> >                 } else {
> >                         return dev_err_probe(dev, par->irq_te,
> > "gpiod to TE IRQ failed.\n");
> >                 }  
> 
> Again, it is not what had been suggested.
> 
> >         }
> >
> >         return 0;
> > }  
> 
> The rest is better, but we will see later on when you submit a new
> version (And I feel it won't be last).
> 

_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v12] staging: fbtft: add tearing signal detect

[carlis]

On Fri, 29 Jan 2021 12:23:08 +0200
Andy Shevchenko <andy.shevchenko@gmail.com> wrote:

> On Fri, Jan 29, 2021 at 7:01 AM carlis <zhangxuezhi3@gmail.com> wrote:
> > On Thu, 28 Jan 2021 16:33:02 +0200
> > Andy Shevchenko <andy.shevchenko@gmail.com> wrote:  
> > > On Thu, Jan 28, 2021 at 2:58 PM Carlis <zhangxuezhi3@gmail.com>
> > > wrote:
> > >
> > > Thanks for your contribution, my comments below.
> > >  
> > > > From: zhangxuezhi <zhangxuezhi1@yulong.com>  
> > >
> > > You probably have to configure your Git to use the same account
> > > for author and committer.  
> >
> > hi,you mean like below:
> >         Carlis <zhangxuezhi1@yulong.com>
> > ?  
> 
> I meant that you shouldn't probably have a From: line in the commit
> message.
> 
> ...
> 
> > hi, i have modified it according to your suggestion like below:  
> 
> Please, go again thru my comments and comments from others and
> carefully address all of them everywhere in your contribution. If you
> have questions, ask them in reply in the corresponding context.
> 
> ...
> 
> > /**
> >  * init_tearing_effect_line() - init tearing effect line  
> 
> >  *  
> 
> For example, above was commented on and hasn't been addressed here.
> 
> >  * @par: FBTFT parameter object
> >  *
> >  * Return: 0 on success, < 0 if error occurred.
> >  */
> > static int init_tearing_effect_line(struct fbtft_par *par)
> > {
> >         struct device *dev = par->info->device;
> >         struct gpio_desc *te;
> >         int rc;
> >
> >         te = gpiod_get_optional(dev, "te", GPIOD_IN);
> >         if (IS_ERR(te))
> >                 return dev_err_probe(dev, PTR_ERR(te), "Failed to
> >         request te GPIO\n");
> >  
> 
> >         if (te) {  
> 
> This one is not like I suggested.
I don't think I have a problem here, if te GPIO is not configured, it
should return NULL, if it is configured, it should be greater than 0 
> 
> >                 par->irq_te = gpiod_to_irq(te);
> >                 gpiod_put(te);
> >  
> 
> >                 if (par->irq_te) {  
> 
> This is wrong.
> 
> >                         rc = devm_request_irq(dev,
> >                                               par->irq_te,
> >                 panel_te_handler,
> >                                               IRQF_TRIGGER_RISING,
> >                 "TE_GPIO", par);  
> 
> Try to use less LOCs.

LOCs???? i can not get you 
> 
> >                         if (rc)
> >                                 return dev_err_probe(dev, rc, "TE
> > IRQ request failed.\n");
> >
> >                         disable_irq_nosync(par->irq_te);
> >                         init_completion(&par->panel_te);  
> 
> >                 } else {
> >                         return dev_err_probe(dev, par->irq_te,
> > "gpiod to TE IRQ failed.\n");
> >                 }  
> 
> Again, it is not what had been suggested.
> 
> >         }
> >
> >         return 0;
> > }  
> 
> The rest is better, but we will see later on when you submit a new
> version (And I feel it won't be last).
> 

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v12] staging: fbtft: add tearing signal detect

[carlis]

On Fri, 29 Jan 2021 12:23:08 +0200
Andy Shevchenko <andy.shevchenko@gmail.com> wrote:

> On Fri, Jan 29, 2021 at 7:01 AM carlis <zhangxuezhi3@gmail.com> wrote:
> > On Thu, 28 Jan 2021 16:33:02 +0200
> > Andy Shevchenko <andy.shevchenko@gmail.com> wrote:  
> > > On Thu, Jan 28, 2021 at 2:58 PM Carlis <zhangxuezhi3@gmail.com>
> > > wrote:
> > >
> > > Thanks for your contribution, my comments below.
> > >  
> > > > From: zhangxuezhi <zhangxuezhi1@yulong.com>  
> > >
> > > You probably have to configure your Git to use the same account
> > > for author and committer.  
> >
> > hi,you mean like below:
> >         Carlis <zhangxuezhi1@yulong.com>
> > ?  
> 
> I meant that you shouldn't probably have a From: line in the commit
> message.
> 
> ...
> 
> > hi, i have modified it according to your suggestion like below:  
> 
> Please, go again thru my comments and comments from others and
> carefully address all of them everywhere in your contribution. If you
> have questions, ask them in reply in the corresponding context.
> 
> ...
> 
> > /**
> >  * init_tearing_effect_line() - init tearing effect line  
> 
> >  *  
> 
> For example, above was commented on and hasn't been addressed here.
> 
> >  * @par: FBTFT parameter object
> >  *
> >  * Return: 0 on success, < 0 if error occurred.
> >  */
> > static int init_tearing_effect_line(struct fbtft_par *par)
> > {
> >         struct device *dev = par->info->device;
> >         struct gpio_desc *te;
> >         int rc;
> >
> >         te = gpiod_get_optional(dev, "te", GPIOD_IN);
> >         if (IS_ERR(te))
> >                 return dev_err_probe(dev, PTR_ERR(te), "Failed to
> >         request te GPIO\n");
> >  
> 
> >         if (te) {  
> 
> This one is not like I suggested.
> 
> >                 par->irq_te = gpiod_to_irq(te);
> >                 gpiod_put(te);
> >  
> 
> >                 if (par->irq_te) {  
> 
> This is wrong.
> 
> >                         rc = devm_request_irq(dev,
> >                                               par->irq_te,
> >                 panel_te_handler,
> >                                               IRQF_TRIGGER_RISING,
> >                 "TE_GPIO", par);  
> 
> Try to use less LOCs.
> 
> >                         if (rc)
> >                                 return dev_err_probe(dev, rc, "TE
> > IRQ request failed.\n");
> >
> >                         disable_irq_nosync(par->irq_te);
> >                         init_completion(&par->panel_te);  
> 
> >                 } else {
> >                         return dev_err_probe(dev, par->irq_te,
> > "gpiod to TE IRQ failed.\n");
> >                 }  
> 
> Again, it is not what had been suggested.
> 
> >         }
> >
> >         return 0;
> > }  
> 
> The rest is better, but we will see later on when you submit a new
> version (And I feel it won't be last).
> 

Hi, I apologize for what I said in the previous two emails. I missed
one email you sent before, and now I probably understand what you
meant. Here is a version I modified according to your suggestion:

From 399e7fb91d1dcba4924cd38cc8283393c80b97e4 Mon Sep 17 00:00:00 2001
From: Carlis <zhangxuezhi1@yulong.com>
Date: Sun, 24 Jan 2021 22:43:21 +0800
Subject: [PATCH v13] staging: fbtft: add tearing signal detect

For st7789v IC,when we need continuous full screen refresh, it is best
to wait for the tearing effect line signal arrive to avoid screen
tearing.

Signed-off-by: Carlis <zhangxuezhi1@yulong.com>
---
v13: change te completion to par data struct member and add a new
     function to deal te gpio request.
v12: change dev_err to dev_err_probe and add space in comments start,
and delete te_mutex, change te wait logic.
v11: remove devm_gpio_put and change a dev_err to dev_info.
v10: additional notes.
v9: change pr_* to dev_*.
v8: delete a log line.
v7: return error value when request fail.
v6: add te gpio request fail deal logic.
v5: fix log print.
v4: modify some code style and change te irq set function name.
v3: modify author and signed-off-by name.
v2: add release te gpio after irq request fail.
---
 drivers/staging/fbtft/fb_st7789v.c | 138
+++++++++++++++++++++++++++++++++++++ drivers/staging/fbtft/fbtft.h
 |   5 ++ 2 files changed, 143 insertions(+)

diff --git a/drivers/staging/fbtft/fb_st7789v.c
b/drivers/staging/fbtft/fb_st7789v.c index 3a280cc..8faae7a 100644
--- a/drivers/staging/fbtft/fb_st7789v.c
+++ b/drivers/staging/fbtft/fb_st7789v.c
@@ -7,9 +7,13 @@
 
 #include <linux/bitops.h>
 #include <linux/delay.h>
+#include <linux/gpio/consumer.h>
 #include <linux/init.h>
 #include <linux/kernel.h>
+#include <linux/interrupt.h>
+#include <linux/completion.h>
 #include <linux/module.h>
+
 #include <video/mipi_display.h>
 
 #include "fbtft.h"
@@ -66,6 +70,55 @@ enum st7789v_command {
 #define MADCTL_MX BIT(6) /* bitmask for column address order */
 #define MADCTL_MY BIT(7) /* bitmask for page address order */
 
+#define PANEL_TE_TIMEOUT_MS  34 /* 60Hz for 16.6ms, configured as
2*16.6ms */ +
+static irqreturn_t panel_te_handler(int irq, void *data)
+{
+	struct fbtft_par *par = (struct fbtft_par *)data;
+
+	complete(&par->panel_te);
+	return IRQ_HANDLED;
+}
+
+/**
+ * init_tearing_effect_line() - init tearing effect line
+ *
+ * @par: FBTFT parameter object
+ *
+ * Return: 0 on success, or a negative error code otherwise.
+ */
+static int init_tearing_effect_line(struct fbtft_par *par)
+{
+	struct device *dev = par->info->device;
+	struct gpio_desc *te;
+	int rc;
+
+	te = gpiod_get_optional(dev, "te", GPIOD_IN);
+	if (IS_ERR(te))
+		return dev_err_probe(dev, PTR_ERR(te), "Failed to
request te GPIO\n"); +
+	if (!te)
+		return 0;
+
+	par->irq_te = gpiod_to_irq(te);
+
+	/* GPIO is locked as an IRQ, we may drop the reference */
+	gpiod_put(te);
+
+	if (par->irq_te < 0)
+		return par->irq_te;
+
+	init_completion(&par->panel_te);
+	rc = devm_request_irq(dev, par->irq_te, panel_te_handler,
+			      IRQF_TRIGGER_RISING, "TE_GPIO", par);
+	if (rc)
+		return dev_err_probe(dev, rc, "TE IRQ request
failed.\n"); +
+	disable_irq_nosync(par->irq_te);
+
+	return 0;
+}
+
 /**
  * init_display() - initialize the display controller
  *
@@ -82,6 +135,12 @@ enum st7789v_command {
  */
 static int init_display(struct fbtft_par *par)
 {
+	int rc;
+
+	rc = init_tearing_effect_line(par);
+	if (rc < 0)
+		return rc;
+
 	/* turn off sleep mode */
 	write_reg(par, MIPI_DCS_EXIT_SLEEP_MODE);
 	mdelay(120);
@@ -137,6 +196,9 @@ static int init_display(struct fbtft_par *par)
 	 */
 	write_reg(par, PWCTRL1, 0xA4, 0xA1);
 
+	if (par->irq_te)
+		write_reg(par, MIPI_DCS_SET_TEAR_ON, 0x00);
+
 	write_reg(par, MIPI_DCS_SET_DISPLAY_ON);
 
 	if (HSD20_IPS)
@@ -146,6 +208,81 @@ static int init_display(struct fbtft_par *par)
 }
 
 /**
+ * st7789v_write_vmem16_bus8() - write data to display
+ *
+ * @par: FBTFT parameter object
+ * @offset: offset from screen_buffer
+ * @len: the length of data to be written
+ *
+ * 16 bit pixel over 8-bit databus
+ *
+ * Return: 0 on success, or a negative error code otherwise.
+ */
+static int st7789v_write_vmem16_bus8(struct fbtft_par *par, size_t
offset, size_t len) +{
+	struct device *dev = par->info->device;
+	__be16 *txbuf16 = par->txbuf.buf;
+	size_t startbyte_size = 0;
+	size_t tx_array_size;
+	size_t to_copy;
+	size_t remain;
+	u16 *vmem16;
+	int ret = 0;
+	int i;
+
+	fbtft_par_dbg(DEBUG_WRITE_VMEM, par, "st7789v
---%s(offset=%zu, len=%zu)\n",
+		      __func__, offset, len);
+
+	remain = len / 2;
+	vmem16 = (u16 *)(par->info->screen_buffer + offset);
+
+	gpiod_set_value(par->gpio.dc, 1);
+
+	if (par->irq_te) {
+		enable_irq(par->irq_te);
+		reinit_completion(&par->panel_te);
+		ret = wait_for_completion_timeout(&par->panel_te,
+
msecs_to_jiffies(PANEL_TE_TIMEOUT_MS));
+		if (ret == 0)
+			dev_err(dev, "wait panel TE time out\n");
+
+		disable_irq(par->irq_te);
+	}
+
+	/* non buffered write */
+	if (!par->txbuf.buf)
+		return par->fbtftops.write(par, vmem16, len);
+
+	/* buffered write */
+	tx_array_size = par->txbuf.len / 2;
+
+	if (par->startbyte) {
+		txbuf16 = par->txbuf.buf + 1;
+		tx_array_size -= 2;
+		*(u8 *)(par->txbuf.buf) = par->startbyte | 0x2;
+		startbyte_size = 1;
+	}
+
+	while (remain) {
+		to_copy = min(tx_array_size, remain);
+		dev_dbg(dev, "    to_copy=%zu, remain=%zu\n",
+			to_copy, remain - to_copy);
+
+		for (i = 0; i < to_copy; i++)
+			txbuf16[i] = cpu_to_be16(vmem16[i]);
+
+		vmem16 = vmem16 + to_copy;
+		ret = par->fbtftops.write(par, par->txbuf.buf,
+					 startbyte_size + to_copy * 2);
+		if (ret < 0)
+			return ret;
+		remain -= to_copy;
+	}
+
+	return ret;
+}
+
+/**
  * set_var() - apply LCD properties like rotation and BGR mode
  *
  * @par: FBTFT parameter object
@@ -259,6 +396,7 @@ static int blank(struct fbtft_par *par, bool on)
 	.gamma = HSD20_IPS_GAMMA,
 	.fbtftops = {
 		.init_display = init_display,
+		.write_vmem = st7789v_write_vmem16_bus8,
 		.set_var = set_var,
 		.set_gamma = set_gamma,
 		.blank = blank,
diff --git a/drivers/staging/fbtft/fbtft.h
b/drivers/staging/fbtft/fbtft.h index 76f8c09..cf8d83f 100644
--- a/drivers/staging/fbtft/fbtft.h
+++ b/drivers/staging/fbtft/fbtft.h
@@ -8,6 +8,7 @@
 #include <linux/spinlock.h>
 #include <linux/spi/spi.h>
 #include <linux/platform_device.h>
+#include <linux/completion.h>
 
 #define FBTFT_ONBOARD_BACKLIGHT 2
 
@@ -165,6 +166,8 @@ struct fbtft_platform_data {
  *             Format: 6 bit Device id + RS bit + RW bit
  * @fbtftops: FBTFT operations provided by driver or device
(platform_data)
  * @dirty_lock: Protects dirty_lines_start and dirty_lines_end
+ * @panel_te: completion for panel te line
+ * @irq_te: LCD Chip tearing effect line
  * @dirty_lines_start: Where to begin updating display
  * @dirty_lines_end: Where to end updating display
  * @gpio.reset: GPIO used to reset display
@@ -203,6 +206,8 @@ struct fbtft_par {
 	u8 startbyte;
 	struct fbtft_ops fbtftops;
 	spinlock_t dirty_lock;
+	struct completion panel_te;
+	int irq_te;
 	unsigned int dirty_lines_start;
 	unsigned int dirty_lines_end;
 	struct {
-- 
1.9.1


regards,
Carlis

_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v12] staging: fbtft: add tearing signal detect

[carlis]

On Fri, 29 Jan 2021 12:23:08 +0200
Andy Shevchenko <andy.shevchenko@gmail.com> wrote:

> On Fri, Jan 29, 2021 at 7:01 AM carlis <zhangxuezhi3@gmail.com> wrote:
> > On Thu, 28 Jan 2021 16:33:02 +0200
> > Andy Shevchenko <andy.shevchenko@gmail.com> wrote:  
> > > On Thu, Jan 28, 2021 at 2:58 PM Carlis <zhangxuezhi3@gmail.com>
> > > wrote:
> > >
> > > Thanks for your contribution, my comments below.
> > >  
> > > > From: zhangxuezhi <zhangxuezhi1@yulong.com>  
> > >
> > > You probably have to configure your Git to use the same account
> > > for author and committer.  
> >
> > hi,you mean like below:
> >         Carlis <zhangxuezhi1@yulong.com>
> > ?  
> 
> I meant that you shouldn't probably have a From: line in the commit
> message.
> 
> ...
> 
> > hi, i have modified it according to your suggestion like below:  
> 
> Please, go again thru my comments and comments from others and
> carefully address all of them everywhere in your contribution. If you
> have questions, ask them in reply in the corresponding context.
> 
> ...
> 
> > /**
> >  * init_tearing_effect_line() - init tearing effect line  
> 
> >  *  
> 
> For example, above was commented on and hasn't been addressed here.
> 
> >  * @par: FBTFT parameter object
> >  *
> >  * Return: 0 on success, < 0 if error occurred.
> >  */
> > static int init_tearing_effect_line(struct fbtft_par *par)
> > {
> >         struct device *dev = par->info->device;
> >         struct gpio_desc *te;
> >         int rc;
> >
> >         te = gpiod_get_optional(dev, "te", GPIOD_IN);
> >         if (IS_ERR(te))
> >                 return dev_err_probe(dev, PTR_ERR(te), "Failed to
> >         request te GPIO\n");
> >  
> 
> >         if (te) {  
> 
> This one is not like I suggested.
> 
> >                 par->irq_te = gpiod_to_irq(te);
> >                 gpiod_put(te);
> >  
> 
> >                 if (par->irq_te) {  
> 
> This is wrong.
> 
> >                         rc = devm_request_irq(dev,
> >                                               par->irq_te,
> >                 panel_te_handler,
> >                                               IRQF_TRIGGER_RISING,
> >                 "TE_GPIO", par);  
> 
> Try to use less LOCs.
> 
> >                         if (rc)
> >                                 return dev_err_probe(dev, rc, "TE
> > IRQ request failed.\n");
> >
> >                         disable_irq_nosync(par->irq_te);
> >                         init_completion(&par->panel_te);  
> 
> >                 } else {
> >                         return dev_err_probe(dev, par->irq_te,
> > "gpiod to TE IRQ failed.\n");
> >                 }  
> 
> Again, it is not what had been suggested.
> 
> >         }
> >
> >         return 0;
> > }  
> 
> The rest is better, but we will see later on when you submit a new
> version (And I feel it won't be last).
> 

Hi, I apologize for what I said in the previous two emails. I missed
one email you sent before, and now I probably understand what you
meant. Here is a version I modified according to your suggestion:

From 399e7fb91d1dcba4924cd38cc8283393c80b97e4 Mon Sep 17 00:00:00 2001
From: Carlis <zhangxuezhi1@yulong.com>
Date: Sun, 24 Jan 2021 22:43:21 +0800
Subject: [PATCH v13] staging: fbtft: add tearing signal detect

For st7789v IC,when we need continuous full screen refresh, it is best
to wait for the tearing effect line signal arrive to avoid screen
tearing.

Signed-off-by: Carlis <zhangxuezhi1@yulong.com>
---
v13: change te completion to par data struct member and add a new
     function to deal te gpio request.
v12: change dev_err to dev_err_probe and add space in comments start,
and delete te_mutex, change te wait logic.
v11: remove devm_gpio_put and change a dev_err to dev_info.
v10: additional notes.
v9: change pr_* to dev_*.
v8: delete a log line.
v7: return error value when request fail.
v6: add te gpio request fail deal logic.
v5: fix log print.
v4: modify some code style and change te irq set function name.
v3: modify author and signed-off-by name.
v2: add release te gpio after irq request fail.
---
 drivers/staging/fbtft/fb_st7789v.c | 138
+++++++++++++++++++++++++++++++++++++ drivers/staging/fbtft/fbtft.h
 |   5 ++ 2 files changed, 143 insertions(+)

diff --git a/drivers/staging/fbtft/fb_st7789v.c
b/drivers/staging/fbtft/fb_st7789v.c index 3a280cc..8faae7a 100644
--- a/drivers/staging/fbtft/fb_st7789v.c
+++ b/drivers/staging/fbtft/fb_st7789v.c
@@ -7,9 +7,13 @@
 
 #include <linux/bitops.h>
 #include <linux/delay.h>
+#include <linux/gpio/consumer.h>
 #include <linux/init.h>
 #include <linux/kernel.h>
+#include <linux/interrupt.h>
+#include <linux/completion.h>
 #include <linux/module.h>
+
 #include <video/mipi_display.h>
 
 #include "fbtft.h"
@@ -66,6 +70,55 @@ enum st7789v_command {
 #define MADCTL_MX BIT(6) /* bitmask for column address order */
 #define MADCTL_MY BIT(7) /* bitmask for page address order */
 
+#define PANEL_TE_TIMEOUT_MS  34 /* 60Hz for 16.6ms, configured as
2*16.6ms */ +
+static irqreturn_t panel_te_handler(int irq, void *data)
+{
+	struct fbtft_par *par = (struct fbtft_par *)data;
+
+	complete(&par->panel_te);
+	return IRQ_HANDLED;
+}
+
+/**
+ * init_tearing_effect_line() - init tearing effect line
+ *
+ * @par: FBTFT parameter object
+ *
+ * Return: 0 on success, or a negative error code otherwise.
+ */
+static int init_tearing_effect_line(struct fbtft_par *par)
+{
+	struct device *dev = par->info->device;
+	struct gpio_desc *te;
+	int rc;
+
+	te = gpiod_get_optional(dev, "te", GPIOD_IN);
+	if (IS_ERR(te))
+		return dev_err_probe(dev, PTR_ERR(te), "Failed to
request te GPIO\n"); +
+	if (!te)
+		return 0;
+
+	par->irq_te = gpiod_to_irq(te);
+
+	/* GPIO is locked as an IRQ, we may drop the reference */
+	gpiod_put(te);
+
+	if (par->irq_te < 0)
+		return par->irq_te;
+
+	init_completion(&par->panel_te);
+	rc = devm_request_irq(dev, par->irq_te, panel_te_handler,
+			      IRQF_TRIGGER_RISING, "TE_GPIO", par);
+	if (rc)
+		return dev_err_probe(dev, rc, "TE IRQ request
failed.\n"); +
+	disable_irq_nosync(par->irq_te);
+
+	return 0;
+}
+
 /**
  * init_display() - initialize the display controller
  *
@@ -82,6 +135,12 @@ enum st7789v_command {
  */
 static int init_display(struct fbtft_par *par)
 {
+	int rc;
+
+	rc = init_tearing_effect_line(par);
+	if (rc < 0)
+		return rc;
+
 	/* turn off sleep mode */
 	write_reg(par, MIPI_DCS_EXIT_SLEEP_MODE);
 	mdelay(120);
@@ -137,6 +196,9 @@ static int init_display(struct fbtft_par *par)
 	 */
 	write_reg(par, PWCTRL1, 0xA4, 0xA1);
 
+	if (par->irq_te)
+		write_reg(par, MIPI_DCS_SET_TEAR_ON, 0x00);
+
 	write_reg(par, MIPI_DCS_SET_DISPLAY_ON);
 
 	if (HSD20_IPS)
@@ -146,6 +208,81 @@ static int init_display(struct fbtft_par *par)
 }
 
 /**
+ * st7789v_write_vmem16_bus8() - write data to display
+ *
+ * @par: FBTFT parameter object
+ * @offset: offset from screen_buffer
+ * @len: the length of data to be written
+ *
+ * 16 bit pixel over 8-bit databus
+ *
+ * Return: 0 on success, or a negative error code otherwise.
+ */
+static int st7789v_write_vmem16_bus8(struct fbtft_par *par, size_t
offset, size_t len) +{
+	struct device *dev = par->info->device;
+	__be16 *txbuf16 = par->txbuf.buf;
+	size_t startbyte_size = 0;
+	size_t tx_array_size;
+	size_t to_copy;
+	size_t remain;
+	u16 *vmem16;
+	int ret = 0;
+	int i;
+
+	fbtft_par_dbg(DEBUG_WRITE_VMEM, par, "st7789v
---%s(offset=%zu, len=%zu)\n",
+		      __func__, offset, len);
+
+	remain = len / 2;
+	vmem16 = (u16 *)(par->info->screen_buffer + offset);
+
+	gpiod_set_value(par->gpio.dc, 1);
+
+	if (par->irq_te) {
+		enable_irq(par->irq_te);
+		reinit_completion(&par->panel_te);
+		ret = wait_for_completion_timeout(&par->panel_te,
+
msecs_to_jiffies(PANEL_TE_TIMEOUT_MS));
+		if (ret == 0)
+			dev_err(dev, "wait panel TE time out\n");
+
+		disable_irq(par->irq_te);
+	}
+
+	/* non buffered write */
+	if (!par->txbuf.buf)
+		return par->fbtftops.write(par, vmem16, len);
+
+	/* buffered write */
+	tx_array_size = par->txbuf.len / 2;
+
+	if (par->startbyte) {
+		txbuf16 = par->txbuf.buf + 1;
+		tx_array_size -= 2;
+		*(u8 *)(par->txbuf.buf) = par->startbyte | 0x2;
+		startbyte_size = 1;
+	}
+
+	while (remain) {
+		to_copy = min(tx_array_size, remain);
+		dev_dbg(dev, "    to_copy=%zu, remain=%zu\n",
+			to_copy, remain - to_copy);
+
+		for (i = 0; i < to_copy; i++)
+			txbuf16[i] = cpu_to_be16(vmem16[i]);
+
+		vmem16 = vmem16 + to_copy;
+		ret = par->fbtftops.write(par, par->txbuf.buf,
+					 startbyte_size + to_copy * 2);
+		if (ret < 0)
+			return ret;
+		remain -= to_copy;
+	}
+
+	return ret;
+}
+
+/**
  * set_var() - apply LCD properties like rotation and BGR mode
  *
  * @par: FBTFT parameter object
@@ -259,6 +396,7 @@ static int blank(struct fbtft_par *par, bool on)
 	.gamma = HSD20_IPS_GAMMA,
 	.fbtftops = {
 		.init_display = init_display,
+		.write_vmem = st7789v_write_vmem16_bus8,
 		.set_var = set_var,
 		.set_gamma = set_gamma,
 		.blank = blank,
diff --git a/drivers/staging/fbtft/fbtft.h
b/drivers/staging/fbtft/fbtft.h index 76f8c09..cf8d83f 100644
--- a/drivers/staging/fbtft/fbtft.h
+++ b/drivers/staging/fbtft/fbtft.h
@@ -8,6 +8,7 @@
 #include <linux/spinlock.h>
 #include <linux/spi/spi.h>
 #include <linux/platform_device.h>
+#include <linux/completion.h>
 
 #define FBTFT_ONBOARD_BACKLIGHT 2
 
@@ -165,6 +166,8 @@ struct fbtft_platform_data {
  *             Format: 6 bit Device id + RS bit + RW bit
  * @fbtftops: FBTFT operations provided by driver or device
(platform_data)
  * @dirty_lock: Protects dirty_lines_start and dirty_lines_end
+ * @panel_te: completion for panel te line
+ * @irq_te: LCD Chip tearing effect line
  * @dirty_lines_start: Where to begin updating display
  * @dirty_lines_end: Where to end updating display
  * @gpio.reset: GPIO used to reset display
@@ -203,6 +206,8 @@ struct fbtft_par {
 	u8 startbyte;
 	struct fbtft_ops fbtftops;
 	spinlock_t dirty_lock;
+	struct completion panel_te;
+	int irq_te;
 	unsigned int dirty_lines_start;
 	unsigned int dirty_lines_end;
 	struct {
-- 
1.9.1


regards,
Carlis

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v12] staging: fbtft: add tearing signal detect

[Andy Shevchenko]

On Fri, Jan 29, 2021 at 2:47 PM carlis <zhangxuezhi3@gmail.com> wrote:
> On Fri, 29 Jan 2021 12:23:08 +0200
> Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
> > On Fri, Jan 29, 2021 at 7:01 AM carlis <zhangxuezhi3@gmail.com> wrote:
> > > On Thu, 28 Jan 2021 16:33:02 +0200
> > > Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
> > > > On Thu, Jan 28, 2021 at 2:58 PM Carlis <zhangxuezhi3@gmail.com>
> > > > wrote:

...

> > Please, go again thru my comments and comments from others and
> > carefully address all of them everywhere in your contribution. If you
> > have questions, ask them in reply in the corresponding context.

...

> > > /**
> > >  * init_tearing_effect_line() - init tearing effect line
> >
> > >  *
> >
> > For example, above was commented on and hasn't been addressed here.
> >
> hi,here i can not get you.....

The above is a blank line which is redundant. It also applied to the
other function in the code.

> > >  * @par: FBTFT parameter object
> > >  *
> > >  * Return: 0 on success, < 0 if error occurred.
> > >  */
> > > static int init_tearing_effect_line(struct fbtft_par *par)
> > > {
> > >         struct device *dev = par->info->device;
> > >         struct gpio_desc *te;
> > >         int rc;
> > >
> > >         te = gpiod_get_optional(dev, "te", GPIOD_IN);
> > >         if (IS_ERR(te))
> > >                 return dev_err_probe(dev, PTR_ERR(te), "Failed to
> > >         request te GPIO\n");
> > >
> >
> > >         if (te) {
> >
> > This one is not like I suggested.
> Why? My thinking is that if the TE is not configured and NULL is
> returned, the initialization can still proceed.....

I have suggested to bail out immediately. It will reduce an
indentation level on the below code.

> > >                 par->irq_te = gpiod_to_irq(te);
> > >                 gpiod_put(te);
> > >
> >
> > >                 if (par->irq_te) {
> >
> > This is wrong.
>
> Why? i have read gpiod_to_irq code, if an error occurs, a negative
> value is returned, and zero is not possible,so I need this value to
> determine if TE IRQ is configured

It returns two possible cases:
 - error code (when negative)
 - Linux IRQ number otherwise

You check makes a no-op since in either variant it will proceed to the
request of IRQ, which is wrong in an error case.

> > >                         rc = devm_request_irq(dev,
> > >                                               par->irq_te,
> > >                 panel_te_handler,
> > >                                               IRQF_TRIGGER_RISING,
> > >                 "TE_GPIO", par);
> >
> > Try to use less LOCs.
> >
> > >                         if (rc)
> > >                                 return dev_err_probe(dev, rc, "TE
> > > IRQ request failed.\n");
> > >
> > >                         disable_irq_nosync(par->irq_te);
> > >                         init_completion(&par->panel_te);
> >
> > >                 } else {
> > >                         return dev_err_probe(dev, par->irq_te,
> > > "gpiod to TE IRQ failed.\n");
> > >                 }
> >
> > Again, it is not what had been suggested.
> >
> > >         }

-- 
With Best Regards,
Andy Shevchenko
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v12] staging: fbtft: add tearing signal detect

[Andy Shevchenko]

On Fri, Jan 29, 2021 at 2:47 PM carlis <zhangxuezhi3@gmail.com> wrote:
> On Fri, 29 Jan 2021 12:23:08 +0200
> Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
> > On Fri, Jan 29, 2021 at 7:01 AM carlis <zhangxuezhi3@gmail.com> wrote:
> > > On Thu, 28 Jan 2021 16:33:02 +0200
> > > Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
> > > > On Thu, Jan 28, 2021 at 2:58 PM Carlis <zhangxuezhi3@gmail.com>
> > > > wrote:

...

> > Please, go again thru my comments and comments from others and
> > carefully address all of them everywhere in your contribution. If you
> > have questions, ask them in reply in the corresponding context.

...

> > > /**
> > >  * init_tearing_effect_line() - init tearing effect line
> >
> > >  *
> >
> > For example, above was commented on and hasn't been addressed here.
> >
> hi,here i can not get you.....

The above is a blank line which is redundant. It also applied to the
other function in the code.

> > >  * @par: FBTFT parameter object
> > >  *
> > >  * Return: 0 on success, < 0 if error occurred.
> > >  */
> > > static int init_tearing_effect_line(struct fbtft_par *par)
> > > {
> > >         struct device *dev = par->info->device;
> > >         struct gpio_desc *te;
> > >         int rc;
> > >
> > >         te = gpiod_get_optional(dev, "te", GPIOD_IN);
> > >         if (IS_ERR(te))
> > >                 return dev_err_probe(dev, PTR_ERR(te), "Failed to
> > >         request te GPIO\n");
> > >
> >
> > >         if (te) {
> >
> > This one is not like I suggested.
> Why? My thinking is that if the TE is not configured and NULL is
> returned, the initialization can still proceed.....

I have suggested to bail out immediately. It will reduce an
indentation level on the below code.

> > >                 par->irq_te = gpiod_to_irq(te);
> > >                 gpiod_put(te);
> > >
> >
> > >                 if (par->irq_te) {
> >
> > This is wrong.
>
> Why? i have read gpiod_to_irq code, if an error occurs, a negative
> value is returned, and zero is not possible,so I need this value to
> determine if TE IRQ is configured

It returns two possible cases:
 - error code (when negative)
 - Linux IRQ number otherwise

You check makes a no-op since in either variant it will proceed to the
request of IRQ, which is wrong in an error case.

> > >                         rc = devm_request_irq(dev,
> > >                                               par->irq_te,
> > >                 panel_te_handler,
> > >                                               IRQF_TRIGGER_RISING,
> > >                 "TE_GPIO", par);
> >
> > Try to use less LOCs.
> >
> > >                         if (rc)
> > >                                 return dev_err_probe(dev, rc, "TE
> > > IRQ request failed.\n");
> > >
> > >                         disable_irq_nosync(par->irq_te);
> > >                         init_completion(&par->panel_te);
> >
> > >                 } else {
> > >                         return dev_err_probe(dev, par->irq_te,
> > > "gpiod to TE IRQ failed.\n");
> > >                 }
> >
> > Again, it is not what had been suggested.
> >
> > >         }

-- 
With Best Regards,
Andy Shevchenko
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v12] staging: fbtft: add tearing signal detect

[Andy Shevchenko]

On Fri, Jan 29, 2021 at 2:54 PM carlis <zhangxuezhi3@gmail.com> wrote:
> On Fri, 29 Jan 2021 12:23:08 +0200
> Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
> > On Fri, Jan 29, 2021 at 7:01 AM carlis <zhangxuezhi3@gmail.com> wrote:
> > > On Thu, 28 Jan 2021 16:33:02 +0200
> > > Andy Shevchenko <andy.shevchenko@gmail.com> wrote:

...

> > This one is not like I suggested.
> I don't think I have a problem here, if te GPIO is not configured, it
> should return NULL, if it is configured, it should be greater than 0

Pointers are always greater than 0 or a special NULL case. The
rationale I explained in the previous mail.

...

> > >                         rc = devm_request_irq(dev,
> > >                                               par->irq_te,
> > >                 panel_te_handler,
> > >                                               IRQF_TRIGGER_RISING,
> > >                 "TE_GPIO", par);
> >
> > Try to use less LOCs.
>
> LOCs???? i can not get you

Lines Of Code. Above can occupy less LOCs.

-- 
With Best Regards,
Andy Shevchenko
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v12] staging: fbtft: add tearing signal detect

[Andy Shevchenko]

On Fri, Jan 29, 2021 at 2:54 PM carlis <zhangxuezhi3@gmail.com> wrote:
> On Fri, 29 Jan 2021 12:23:08 +0200
> Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
> > On Fri, Jan 29, 2021 at 7:01 AM carlis <zhangxuezhi3@gmail.com> wrote:
> > > On Thu, 28 Jan 2021 16:33:02 +0200
> > > Andy Shevchenko <andy.shevchenko@gmail.com> wrote:

...

> > This one is not like I suggested.
> I don't think I have a problem here, if te GPIO is not configured, it
> should return NULL, if it is configured, it should be greater than 0

Pointers are always greater than 0 or a special NULL case. The
rationale I explained in the previous mail.

...

> > >                         rc = devm_request_irq(dev,
> > >                                               par->irq_te,
> > >                 panel_te_handler,
> > >                                               IRQF_TRIGGER_RISING,
> > >                 "TE_GPIO", par);
> >
> > Try to use less LOCs.
>
> LOCs???? i can not get you

Lines Of Code. Above can occupy less LOCs.

-- 
With Best Regards,
Andy Shevchenko
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v12] staging: fbtft: add tearing signal detect

[Andy Shevchenko]

On Fri, Jan 29, 2021 at 3:56 PM carlis <zhangxuezhi3@gmail.com> wrote:
> On Fri, 29 Jan 2021 12:23:08 +0200
> Andy Shevchenko <andy.shevchenko@gmail.com> wrote:

We are almost there, I have no idea what Noralf or others are going to
say though.

...

> Hi, I apologize for what I said in the previous two emails. I missed
> one email you sent before, and now I probably understand what you
> meant. Here is a version I modified according to your suggestion:
>
> From 399e7fb91d1dcba4924cd38cc8283393c80b97e4 Mon Sep 17 00:00:00 2001
> From: Carlis <zhangxuezhi1@yulong.com>
> Date: Sun, 24 Jan 2021 22:43:21 +0800
> Subject: [PATCH v13] staging: fbtft: add tearing signal detect
>
> For st7789v IC,when we need continuous full screen refresh, it is best

Missed space after comma.

> to wait for the tearing effect line signal arrive to avoid screen

to arrive

> tearing.

...

> +#define PANEL_TE_TIMEOUT_MS  34 /* 60Hz for 16.6ms, configured as
> 2*16.6ms */ +

Move comment before the definition
/* comment */
#define DEFINITION

Also consider to use 33 ms as closest to what you mentioned in the comment.
Or leave it with mention that you are using ceil() value.

...

> +/**
> + * init_tearing_effect_line() - init tearing effect line

> + *

As per a few previous reviews.
Okay, I have noticed that the existing kernel-doc is written like
this, but it doesn't prevent you from avoiding this little mistake.

> + * @par: FBTFT parameter object
> + *
> + * Return: 0 on success, or a negative error code otherwise.
> + */
> +static int init_tearing_effect_line(struct fbtft_par *par)
> +{
> +       struct device *dev = par->info->device;
> +       struct gpio_desc *te;
> +       int rc;
> +
> +       te = gpiod_get_optional(dev, "te", GPIOD_IN);
> +       if (IS_ERR(te))
> +               return dev_err_probe(dev, PTR_ERR(te), "Failed to
> request te GPIO\n"); +

Below is okay, but needs a comment explaining why we return a success.

> +       if (!te)
> +               return 0;
> +
> +       par->irq_te = gpiod_to_irq(te);
> +
> +       /* GPIO is locked as an IRQ, we may drop the reference */
> +       gpiod_put(te);
> +
> +       if (par->irq_te < 0)
> +               return par->irq_te;

I recommend using a temporary variable. In such a case you won't need
to specifically check for negative error code. So, something like

int irq;

irq = ...

if (irq < 0)
  return irq;

->irq_te = irq;

> +       init_completion(&par->panel_te);
> +       rc = devm_request_irq(dev, par->irq_te, panel_te_handler,
> +                             IRQF_TRIGGER_RISING, "TE_GPIO", par);

Right. Now it needs a comment explaining the choice of rising edge type of IRQ.

> +       if (rc)
> +               return dev_err_probe(dev, rc, "TE IRQ request
> failed.\n"); +
> +       disable_irq_nosync(par->irq_te);
> +
> +       return 0;
> +}

...

> +       rc = init_tearing_effect_line(par);

> +       if (rc < 0)

Here is no need to specifically check against less than 0,
  if (ret)
will work nicely.

> +               return rc;

...

> +       if (par->irq_te)
> +               write_reg(par, MIPI_DCS_SET_TEAR_ON, 0x00);

Do you need to call MIPI_DCS_SET_TEAR_SCANLINE in this case?

Alos, when there is no IRQ, shouldn't we explicitly call
               write_reg(par, MIPI_DCS_SET_TEAR_OFF);
?

...

>  /**
> + * st7789v_write_vmem16_bus8() - write data to display

> + *

Redundant.

> + * @par: FBTFT parameter object
> + * @offset: offset from screen_buffer
> + * @len: the length of data to be written
> + *

> + * 16 bit pixel over 8-bit databus

Write 16-bit pixels over 8-bit data bus.

> + * Return: 0 on success, or a negative error code otherwise.
> + */

...

> +       if (par->irq_te) {
> +               enable_irq(par->irq_te);
> +               reinit_completion(&par->panel_te);
> +               ret = wait_for_completion_timeout(&par->panel_te,
> +
> msecs_to_jiffies(PANEL_TE_TIMEOUT_MS));
> +               if (ret == 0)
> +                       dev_err(dev, "wait panel TE time out\n");

timeout

> +
> +               disable_irq(par->irq_te);
> +       }

...

> + * @panel_te: completion for panel te line

TE line

> + * @irq_te: LCD Chip tearing effect line

"Linux IRQ for LCD..."

-- 
With Best Regards,
Andy Shevchenko
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v12] staging: fbtft: add tearing signal detect

[Andy Shevchenko]

On Fri, Jan 29, 2021 at 3:56 PM carlis <zhangxuezhi3@gmail.com> wrote:
> On Fri, 29 Jan 2021 12:23:08 +0200
> Andy Shevchenko <andy.shevchenko@gmail.com> wrote:

We are almost there, I have no idea what Noralf or others are going to
say though.

...

> Hi, I apologize for what I said in the previous two emails. I missed
> one email you sent before, and now I probably understand what you
> meant. Here is a version I modified according to your suggestion:
>
> From 399e7fb91d1dcba4924cd38cc8283393c80b97e4 Mon Sep 17 00:00:00 2001
> From: Carlis <zhangxuezhi1@yulong.com>
> Date: Sun, 24 Jan 2021 22:43:21 +0800
> Subject: [PATCH v13] staging: fbtft: add tearing signal detect
>
> For st7789v IC,when we need continuous full screen refresh, it is best

Missed space after comma.

> to wait for the tearing effect line signal arrive to avoid screen

to arrive

> tearing.

...

> +#define PANEL_TE_TIMEOUT_MS  34 /* 60Hz for 16.6ms, configured as
> 2*16.6ms */ +

Move comment before the definition
/* comment */
#define DEFINITION

Also consider to use 33 ms as closest to what you mentioned in the comment.
Or leave it with mention that you are using ceil() value.

...

> +/**
> + * init_tearing_effect_line() - init tearing effect line

> + *

As per a few previous reviews.
Okay, I have noticed that the existing kernel-doc is written like
this, but it doesn't prevent you from avoiding this little mistake.

> + * @par: FBTFT parameter object
> + *
> + * Return: 0 on success, or a negative error code otherwise.
> + */
> +static int init_tearing_effect_line(struct fbtft_par *par)
> +{
> +       struct device *dev = par->info->device;
> +       struct gpio_desc *te;
> +       int rc;
> +
> +       te = gpiod_get_optional(dev, "te", GPIOD_IN);
> +       if (IS_ERR(te))
> +               return dev_err_probe(dev, PTR_ERR(te), "Failed to
> request te GPIO\n"); +

Below is okay, but needs a comment explaining why we return a success.

> +       if (!te)
> +               return 0;
> +
> +       par->irq_te = gpiod_to_irq(te);
> +
> +       /* GPIO is locked as an IRQ, we may drop the reference */
> +       gpiod_put(te);
> +
> +       if (par->irq_te < 0)
> +               return par->irq_te;

I recommend using a temporary variable. In such a case you won't need
to specifically check for negative error code. So, something like

int irq;

irq = ...

if (irq < 0)
  return irq;

->irq_te = irq;

> +       init_completion(&par->panel_te);
> +       rc = devm_request_irq(dev, par->irq_te, panel_te_handler,
> +                             IRQF_TRIGGER_RISING, "TE_GPIO", par);

Right. Now it needs a comment explaining the choice of rising edge type of IRQ.

> +       if (rc)
> +               return dev_err_probe(dev, rc, "TE IRQ request
> failed.\n"); +
> +       disable_irq_nosync(par->irq_te);
> +
> +       return 0;
> +}

...

> +       rc = init_tearing_effect_line(par);

> +       if (rc < 0)

Here is no need to specifically check against less than 0,
  if (ret)
will work nicely.

> +               return rc;

...

> +       if (par->irq_te)
> +               write_reg(par, MIPI_DCS_SET_TEAR_ON, 0x00);

Do you need to call MIPI_DCS_SET_TEAR_SCANLINE in this case?

Alos, when there is no IRQ, shouldn't we explicitly call
               write_reg(par, MIPI_DCS_SET_TEAR_OFF);
?

...

>  /**
> + * st7789v_write_vmem16_bus8() - write data to display

> + *

Redundant.

> + * @par: FBTFT parameter object
> + * @offset: offset from screen_buffer
> + * @len: the length of data to be written
> + *

> + * 16 bit pixel over 8-bit databus

Write 16-bit pixels over 8-bit data bus.

> + * Return: 0 on success, or a negative error code otherwise.
> + */

...

> +       if (par->irq_te) {
> +               enable_irq(par->irq_te);
> +               reinit_completion(&par->panel_te);
> +               ret = wait_for_completion_timeout(&par->panel_te,
> +
> msecs_to_jiffies(PANEL_TE_TIMEOUT_MS));
> +               if (ret == 0)
> +                       dev_err(dev, "wait panel TE time out\n");

timeout

> +
> +               disable_irq(par->irq_te);
> +       }

...

> + * @panel_te: completion for panel te line

TE line

> + * @irq_te: LCD Chip tearing effect line

"Linux IRQ for LCD..."

-- 
With Best Regards,
Andy Shevchenko
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v12] staging: fbtft: add tearing signal detect

[carlis]

On Fri, 29 Jan 2021 16:26:12 +0200
Andy Shevchenko <andy.shevchenko@gmail.com> wrote:

> On Fri, Jan 29, 2021 at 3:56 PM carlis <zhangxuezhi3@gmail.com> wrote:
> > On Fri, 29 Jan 2021 12:23:08 +0200
> > Andy Shevchenko <andy.shevchenko@gmail.com> wrote:  
> 
> We are almost there, I have no idea what Noralf or others are going to
> say though.
> 
> ...
> 
> > Hi, I apologize for what I said in the previous two emails. I missed
> > one email you sent before, and now I probably understand what you
> > meant. Here is a version I modified according to your suggestion:
> >
> > From 399e7fb91d1dcba4924cd38cc8283393c80b97e4 Mon Sep 17 00:00:00
> > 2001 From: Carlis <zhangxuezhi1@yulong.com>
> > Date: Sun, 24 Jan 2021 22:43:21 +0800
> > Subject: [PATCH v13] staging: fbtft: add tearing signal detect
> >
> > For st7789v IC,when we need continuous full screen refresh, it is
> > best  
> 
> Missed space after comma.
> 
> > to wait for the tearing effect line signal arrive to avoid screen  
> 
> to arrive
> 
> > tearing.  
> 
> ...
> 
> > +#define PANEL_TE_TIMEOUT_MS  34 /* 60Hz for 16.6ms, configured as
> > 2*16.6ms */ +  
> 
> Move comment before the definition
> /* comment */
> #define DEFINITION
> 
> Also consider to use 33 ms as closest to what you mentioned in the
> comment. Or leave it with mention that you are using ceil() value.
> 
> ...
> 
> > +/**
> > + * init_tearing_effect_line() - init tearing effect line  
> 
> > + *  
> 
> As per a few previous reviews.
> Okay, I have noticed that the existing kernel-doc is written like
> this, but it doesn't prevent you from avoiding this little mistake.
> 
> > + * @par: FBTFT parameter object
> > + *
> > + * Return: 0 on success, or a negative error code otherwise.
> > + */
> > +static int init_tearing_effect_line(struct fbtft_par *par)
> > +{
> > +       struct device *dev = par->info->device;
> > +       struct gpio_desc *te;
> > +       int rc;
> > +
> > +       te = gpiod_get_optional(dev, "te", GPIOD_IN);
> > +       if (IS_ERR(te))
> > +               return dev_err_probe(dev, PTR_ERR(te), "Failed to
> > request te GPIO\n"); +  
> 
> Below is okay, but needs a comment explaining why we return a success.
> 
> > +       if (!te)
> > +               return 0;
> > +
> > +       par->irq_te = gpiod_to_irq(te);
> > +
> > +       /* GPIO is locked as an IRQ, we may drop the reference */
> > +       gpiod_put(te);
> > +
> > +       if (par->irq_te < 0)
> > +               return par->irq_te;  
> 
> I recommend using a temporary variable. In such a case you won't need
> to specifically check for negative error code. So, something like
> 
> int irq;
> 
> irq = ...
> 
> if (irq < 0)
>   return irq;
> 
> ->irq_te = irq;  
> 
> > +       init_completion(&par->panel_te);
> > +       rc = devm_request_irq(dev, par->irq_te, panel_te_handler,
> > +                             IRQF_TRIGGER_RISING, "TE_GPIO", par);
> >  
> 
> Right. Now it needs a comment explaining the choice of rising edge
> type of IRQ.
> 
> > +       if (rc)
> > +               return dev_err_probe(dev, rc, "TE IRQ request
> > failed.\n"); +
> > +       disable_irq_nosync(par->irq_te);
> > +
> > +       return 0;
> > +}  
> 
> ...
> 
> > +       rc = init_tearing_effect_line(par);  
> 
> > +       if (rc < 0)  
> 
> Here is no need to specifically check against less than 0,
>   if (ret)
> will work nicely.
> 
> > +               return rc;  
> 
> ...
> 
> > +       if (par->irq_te)
> > +               write_reg(par, MIPI_DCS_SET_TEAR_ON, 0x00);  
> 
> Do you need to call MIPI_DCS_SET_TEAR_SCANLINE in this case?
> 
Hi, TE line output is off by default when powering on, and I use
Qualcomm SDX55 chip, SPI top speed is only 50MHz, its data transmission
speed( It takes about 24ms to transmit a frame) is slower than 60Hz
refresh(a frame only need 16.6ms), so i think there is no need to call
MIPI_DCS_SET_TEAR_SCANLINE 

> Alos, when there is no IRQ, shouldn't we explicitly call
>                write_reg(par, MIPI_DCS_SET_TEAR_OFF);
> ?
> 
> ...
> 
> >  /**
> > + * st7789v_write_vmem16_bus8() - write data to display  
> 
> > + *  
> 
> Redundant.
> 
> > + * @par: FBTFT parameter object
> > + * @offset: offset from screen_buffer
> > + * @len: the length of data to be written
> > + *  
> 
> > + * 16 bit pixel over 8-bit databus  
> 
> Write 16-bit pixels over 8-bit data bus.
> 
> > + * Return: 0 on success, or a negative error code otherwise.
> > + */  
> 
> ...
> 
> > +       if (par->irq_te) {
> > +               enable_irq(par->irq_te);
> > +               reinit_completion(&par->panel_te);
> > +               ret = wait_for_completion_timeout(&par->panel_te,
> > +
> > msecs_to_jiffies(PANEL_TE_TIMEOUT_MS));
> > +               if (ret == 0)
> > +                       dev_err(dev, "wait panel TE time out\n");  
> 
> timeout
> 
> > +
> > +               disable_irq(par->irq_te);
> > +       }  
> 
> ...
> 
> > + * @panel_te: completion for panel te line  
> 
> TE line
> 
> > + * @irq_te: LCD Chip tearing effect line  
> 
> "Linux IRQ for LCD..."
> 

hi, i modify new below:

From b65fc2bfbe123defc8ec838601a9c12707f70c53 Mon Sep 17 00:00:00 2001
From: Carlis <zhangxuezhi1@yulong.com>
Date: Sun, 24 Jan 2021 22:43:21 +0800
Subject: [PATCH v13] staging: fbtft: add tearing signal detect

For st7789v IC, when we need continuous full screen refresh, it is best
to wait for the tearing effect line signal to arrive to avoid screen
tearing.

Signed-off-by: Carlis <zhangxuezhi1@yulong.com>
---
v13: change TE completion to par data struct member and add a new
     function to deal te gpio request, move wait logic to
fbtft_write_vmem16_bus8. v12: change dev_err to dev_err_probe and add
space in comments start, and delete te_mutex, change te wait logic.
v11: remove devm_gpio_put and change a dev_err to dev_info.
v10: additional notes.
v9: change pr_* to dev_*.
v8: delete a log line.
v7: return error value when request fail.
v6: add te gpio request fail deal logic.
v5: fix log print.
v4: modify some code style and change te irq set function name.
v3: modify author and signed-off-by name.
v2: add release te gpio after irq request fail.
---
 drivers/staging/fbtft/fb_st7789v.c | 64
++++++++++++++++++++++++++++++++++++++
drivers/staging/fbtft/fbtft-bus.c  | 20 ++++++++++--
drivers/staging/fbtft/fbtft.h      |  5 +++ 3 files changed, 87
insertions(+), 2 deletions(-)

diff --git a/drivers/staging/fbtft/fb_st7789v.c
b/drivers/staging/fbtft/fb_st7789v.c index 3a280cc..695dcac 100644
--- a/drivers/staging/fbtft/fb_st7789v.c
+++ b/drivers/staging/fbtft/fb_st7789v.c
@@ -7,9 +7,13 @@
 
 #include <linux/bitops.h>
 #include <linux/delay.h>
+#include <linux/gpio/consumer.h>
 #include <linux/init.h>
 #include <linux/kernel.h>
+#include <linux/interrupt.h>
+#include <linux/completion.h>
 #include <linux/module.h>
+
 #include <video/mipi_display.h>
 
 #include "fbtft.h"
@@ -66,6 +70,56 @@ enum st7789v_command {
 #define MADCTL_MX BIT(6) /* bitmask for column address order */
 #define MADCTL_MY BIT(7) /* bitmask for page address order */
 
+static irqreturn_t panel_te_handler(int irq, void *data)
+{
+	struct fbtft_par *par = (struct fbtft_par *)data;
+
+	complete(&par->panel_te);
+	return IRQ_HANDLED;
+}
+
+/*
+ * init_tearing_effect_line() - init tearing effect line.
+ * @par: FBTFT parameter object.
+ *
+ * Return: 0 on success, or a negative error code otherwise.
+ */
+static int init_tearing_effect_line(struct fbtft_par *par)
+{
+	struct device *dev = par->info->device;
+	struct gpio_desc *te;
+	int rc, irq;
+
+	te = gpiod_get_optional(dev, "te", GPIOD_IN);
+	if (IS_ERR(te))
+		return dev_err_probe(dev, PTR_ERR(te), "Failed to
request te GPIO\n"); +
+	/* if te is NULL, indicating no configuration, directly return
success */
+	if (!te)
+		return 0;
+
+	irq = gpiod_to_irq(te);
+
+	/* GPIO is locked as an IRQ, we may drop the reference */
+	gpiod_put(te);
+
+	if (irq < 0)
+		return irq;
+
+	par->irq_te = irq;
+	init_completion(&par->panel_te);
+
+	/* The effective state is high and lasts no more than 1000
microseconds */
+	rc = devm_request_irq(dev, par->irq_te, panel_te_handler,
+			      IRQF_TRIGGER_RISING, "TE_GPIO", par);
+	if (rc)
+		return dev_err_probe(dev, rc, "TE IRQ request
failed.\n"); +
+	disable_irq_nosync(par->irq_te);
+
+	return 0;
+}
+
 /**
  * init_display() - initialize the display controller
  *
@@ -82,6 +136,12 @@ enum st7789v_command {
  */
 static int init_display(struct fbtft_par *par)
 {
+	int rc;
+
+	rc = init_tearing_effect_line(par);
+	if (rc)
+		return rc;
+
 	/* turn off sleep mode */
 	write_reg(par, MIPI_DCS_EXIT_SLEEP_MODE);
 	mdelay(120);
@@ -137,6 +197,10 @@ static int init_display(struct fbtft_par *par)
 	 */
 	write_reg(par, PWCTRL1, 0xA4, 0xA1);
 
+	/* TE line output is off by default when powering on */
+	if (par->irq_te)
+		write_reg(par, MIPI_DCS_SET_TEAR_ON, 0x00);
+
 	write_reg(par, MIPI_DCS_SET_DISPLAY_ON);
 
 	if (HSD20_IPS)
diff --git a/drivers/staging/fbtft/fbtft-bus.c
b/drivers/staging/fbtft/fbtft-bus.c index 63c65dd..5bc2fff 100644
--- a/drivers/staging/fbtft/fbtft-bus.c
+++ b/drivers/staging/fbtft/fbtft-bus.c
@@ -2,9 +2,15 @@
 #include <linux/export.h>
 #include <linux/errno.h>
 #include <linux/gpio/consumer.h>
+#include <linux/interrupt.h>
+#include <linux/completion.h>
 #include <linux/spi/spi.h>
+
 #include "fbtft.h"
 
+/* 60Hz for 16.6ms, configured as 2*16.6ms */
+#define PANEL_TE_TIMEOUT_MS  33
+
 /*****************************************************************************
  *
  *   void (*write_reg)(struct fbtft_par *par, int len, ...);
@@ -135,8 +141,18 @@ 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);
+
+	if (par->irq_te) {
+		enable_irq(par->irq_te);
+		reinit_completion(&par->panel_te);
+		ret = wait_for_completion_timeout(&par->panel_te,
+
msecs_to_jiffies(PANEL_TE_TIMEOUT_MS));
+		if (ret == 0)
+			dev_err(par->info->device, "wait panel TE
timeout\n"); +
+		disable_irq(par->irq_te);
+	}
 
 	/* non buffered write */
 	if (!par->txbuf.buf)
diff --git a/drivers/staging/fbtft/fbtft.h
b/drivers/staging/fbtft/fbtft.h index 76f8c09..61c7207 100644
--- a/drivers/staging/fbtft/fbtft.h
+++ b/drivers/staging/fbtft/fbtft.h
@@ -8,6 +8,7 @@
 #include <linux/spinlock.h>
 #include <linux/spi/spi.h>
 #include <linux/platform_device.h>
+#include <linux/completion.h>
 
 #define FBTFT_ONBOARD_BACKLIGHT 2
 
@@ -165,6 +166,8 @@ struct fbtft_platform_data {
  *             Format: 6 bit Device id + RS bit + RW bit
  * @fbtftops: FBTFT operations provided by driver or device
(platform_data)
  * @dirty_lock: Protects dirty_lines_start and dirty_lines_end
+ * @panel_te: completion for panel TE line
+ * @irq_te: Linux IRQ for LCD TE line
  * @dirty_lines_start: Where to begin updating display
  * @dirty_lines_end: Where to end updating display
  * @gpio.reset: GPIO used to reset display
@@ -203,6 +206,8 @@ struct fbtft_par {
 	u8 startbyte;
 	struct fbtft_ops fbtftops;
 	spinlock_t dirty_lock;
+	struct completion panel_te;
+	int irq_te;
 	unsigned int dirty_lines_start;
 	unsigned int dirty_lines_end;
 	struct {
-- 
1.9.1

regards,
Carlis 

Re: [PATCH v12] staging: fbtft: add tearing signal detect

[carlis]

On Fri, 29 Jan 2021 16:26:12 +0200
Andy Shevchenko <andy.shevchenko@gmail.com> wrote:

> On Fri, Jan 29, 2021 at 3:56 PM carlis <zhangxuezhi3@gmail.com> wrote:
> > On Fri, 29 Jan 2021 12:23:08 +0200
> > Andy Shevchenko <andy.shevchenko@gmail.com> wrote:  
> 
> We are almost there, I have no idea what Noralf or others are going to
> say though.
> 
> ...
> 
> > Hi, I apologize for what I said in the previous two emails. I missed
> > one email you sent before, and now I probably understand what you
> > meant. Here is a version I modified according to your suggestion:
> >
> > From 399e7fb91d1dcba4924cd38cc8283393c80b97e4 Mon Sep 17 00:00:00
> > 2001 From: Carlis <zhangxuezhi1@yulong.com>
> > Date: Sun, 24 Jan 2021 22:43:21 +0800
> > Subject: [PATCH v13] staging: fbtft: add tearing signal detect
> >
> > For st7789v IC,when we need continuous full screen refresh, it is
> > best  
> 
> Missed space after comma.
> 
> > to wait for the tearing effect line signal arrive to avoid screen  
> 
> to arrive
> 
> > tearing.  
> 
> ...
> 
> > +#define PANEL_TE_TIMEOUT_MS  34 /* 60Hz for 16.6ms, configured as
> > 2*16.6ms */ +  
> 
> Move comment before the definition
> /* comment */
> #define DEFINITION
> 
> Also consider to use 33 ms as closest to what you mentioned in the
> comment. Or leave it with mention that you are using ceil() value.
> 
> ...
> 
> > +/**
> > + * init_tearing_effect_line() - init tearing effect line  
> 
> > + *  
> 
> As per a few previous reviews.
> Okay, I have noticed that the existing kernel-doc is written like
> this, but it doesn't prevent you from avoiding this little mistake.
> 
> > + * @par: FBTFT parameter object
> > + *
> > + * Return: 0 on success, or a negative error code otherwise.
> > + */
> > +static int init_tearing_effect_line(struct fbtft_par *par)
> > +{
> > +       struct device *dev = par->info->device;
> > +       struct gpio_desc *te;
> > +       int rc;
> > +
> > +       te = gpiod_get_optional(dev, "te", GPIOD_IN);
> > +       if (IS_ERR(te))
> > +               return dev_err_probe(dev, PTR_ERR(te), "Failed to
> > request te GPIO\n"); +  
> 
> Below is okay, but needs a comment explaining why we return a success.
> 
> > +       if (!te)
> > +               return 0;
> > +
> > +       par->irq_te = gpiod_to_irq(te);
> > +
> > +       /* GPIO is locked as an IRQ, we may drop the reference */
> > +       gpiod_put(te);
> > +
> > +       if (par->irq_te < 0)
> > +               return par->irq_te;  
> 
> I recommend using a temporary variable. In such a case you won't need
> to specifically check for negative error code. So, something like
> 
> int irq;
> 
> irq = ...
> 
> if (irq < 0)
>   return irq;
> 
> ->irq_te = irq;  
> 
> > +       init_completion(&par->panel_te);
> > +       rc = devm_request_irq(dev, par->irq_te, panel_te_handler,
> > +                             IRQF_TRIGGER_RISING, "TE_GPIO", par);
> >  
> 
> Right. Now it needs a comment explaining the choice of rising edge
> type of IRQ.
> 
> > +       if (rc)
> > +               return dev_err_probe(dev, rc, "TE IRQ request
> > failed.\n"); +
> > +       disable_irq_nosync(par->irq_te);
> > +
> > +       return 0;
> > +}  
> 
> ...
> 
> > +       rc = init_tearing_effect_line(par);  
> 
> > +       if (rc < 0)  
> 
> Here is no need to specifically check against less than 0,
>   if (ret)
> will work nicely.
> 
> > +               return rc;  
> 
> ...
> 
> > +       if (par->irq_te)
> > +               write_reg(par, MIPI_DCS_SET_TEAR_ON, 0x00);  
> 
> Do you need to call MIPI_DCS_SET_TEAR_SCANLINE in this case?
> 
Hi, TE line output is off by default when powering on, and I use
Qualcomm SDX55 chip, SPI top speed is only 50MHz, its data transmission
speed( It takes about 24ms to transmit a frame) is slower than 60Hz
refresh(a frame only need 16.6ms), so i think there is no need to call
MIPI_DCS_SET_TEAR_SCANLINE 

> Alos, when there is no IRQ, shouldn't we explicitly call
>                write_reg(par, MIPI_DCS_SET_TEAR_OFF);
> ?
> 
> ...
> 
> >  /**
> > + * st7789v_write_vmem16_bus8() - write data to display  
> 
> > + *  
> 
> Redundant.
> 
> > + * @par: FBTFT parameter object
> > + * @offset: offset from screen_buffer
> > + * @len: the length of data to be written
> > + *  
> 
> > + * 16 bit pixel over 8-bit databus  
> 
> Write 16-bit pixels over 8-bit data bus.
> 
> > + * Return: 0 on success, or a negative error code otherwise.
> > + */  
> 
> ...
> 
> > +       if (par->irq_te) {
> > +               enable_irq(par->irq_te);
> > +               reinit_completion(&par->panel_te);
> > +               ret = wait_for_completion_timeout(&par->panel_te,
> > +
> > msecs_to_jiffies(PANEL_TE_TIMEOUT_MS));
> > +               if (ret == 0)
> > +                       dev_err(dev, "wait panel TE time out\n");  
> 
> timeout
> 
> > +
> > +               disable_irq(par->irq_te);
> > +       }  
> 
> ...
> 
> > + * @panel_te: completion for panel te line  
> 
> TE line
> 
> > + * @irq_te: LCD Chip tearing effect line  
> 
> "Linux IRQ for LCD..."
> 

hi, i modify new below:

From b65fc2bfbe123defc8ec838601a9c12707f70c53 Mon Sep 17 00:00:00 2001
From: Carlis <zhangxuezhi1@yulong.com>
Date: Sun, 24 Jan 2021 22:43:21 +0800
Subject: [PATCH v13] staging: fbtft: add tearing signal detect

For st7789v IC, when we need continuous full screen refresh, it is best
to wait for the tearing effect line signal to arrive to avoid screen
tearing.

Signed-off-by: Carlis <zhangxuezhi1@yulong.com>
---
v13: change TE completion to par data struct member and add a new
     function to deal te gpio request, move wait logic to
fbtft_write_vmem16_bus8. v12: change dev_err to dev_err_probe and add
space in comments start, and delete te_mutex, change te wait logic.
v11: remove devm_gpio_put and change a dev_err to dev_info.
v10: additional notes.
v9: change pr_* to dev_*.
v8: delete a log line.
v7: return error value when request fail.
v6: add te gpio request fail deal logic.
v5: fix log print.
v4: modify some code style and change te irq set function name.
v3: modify author and signed-off-by name.
v2: add release te gpio after irq request fail.
---
 drivers/staging/fbtft/fb_st7789v.c | 64
++++++++++++++++++++++++++++++++++++++
drivers/staging/fbtft/fbtft-bus.c  | 20 ++++++++++--
drivers/staging/fbtft/fbtft.h      |  5 +++ 3 files changed, 87
insertions(+), 2 deletions(-)

diff --git a/drivers/staging/fbtft/fb_st7789v.c
b/drivers/staging/fbtft/fb_st7789v.c index 3a280cc..695dcac 100644
--- a/drivers/staging/fbtft/fb_st7789v.c
+++ b/drivers/staging/fbtft/fb_st7789v.c
@@ -7,9 +7,13 @@
 
 #include <linux/bitops.h>
 #include <linux/delay.h>
+#include <linux/gpio/consumer.h>
 #include <linux/init.h>
 #include <linux/kernel.h>
+#include <linux/interrupt.h>
+#include <linux/completion.h>
 #include <linux/module.h>
+
 #include <video/mipi_display.h>
 
 #include "fbtft.h"
@@ -66,6 +70,56 @@ enum st7789v_command {
 #define MADCTL_MX BIT(6) /* bitmask for column address order */
 #define MADCTL_MY BIT(7) /* bitmask for page address order */
 
+static irqreturn_t panel_te_handler(int irq, void *data)
+{
+	struct fbtft_par *par = (struct fbtft_par *)data;
+
+	complete(&par->panel_te);
+	return IRQ_HANDLED;
+}
+
+/*
+ * init_tearing_effect_line() - init tearing effect line.
+ * @par: FBTFT parameter object.
+ *
+ * Return: 0 on success, or a negative error code otherwise.
+ */
+static int init_tearing_effect_line(struct fbtft_par *par)
+{
+	struct device *dev = par->info->device;
+	struct gpio_desc *te;
+	int rc, irq;
+
+	te = gpiod_get_optional(dev, "te", GPIOD_IN);
+	if (IS_ERR(te))
+		return dev_err_probe(dev, PTR_ERR(te), "Failed to
request te GPIO\n"); +
+	/* if te is NULL, indicating no configuration, directly return
success */
+	if (!te)
+		return 0;
+
+	irq = gpiod_to_irq(te);
+
+	/* GPIO is locked as an IRQ, we may drop the reference */
+	gpiod_put(te);
+
+	if (irq < 0)
+		return irq;
+
+	par->irq_te = irq;
+	init_completion(&par->panel_te);
+
+	/* The effective state is high and lasts no more than 1000
microseconds */
+	rc = devm_request_irq(dev, par->irq_te, panel_te_handler,
+			      IRQF_TRIGGER_RISING, "TE_GPIO", par);
+	if (rc)
+		return dev_err_probe(dev, rc, "TE IRQ request
failed.\n"); +
+	disable_irq_nosync(par->irq_te);
+
+	return 0;
+}
+
 /**
  * init_display() - initialize the display controller
  *
@@ -82,6 +136,12 @@ enum st7789v_command {
  */
 static int init_display(struct fbtft_par *par)
 {
+	int rc;
+
+	rc = init_tearing_effect_line(par);
+	if (rc)
+		return rc;
+
 	/* turn off sleep mode */
 	write_reg(par, MIPI_DCS_EXIT_SLEEP_MODE);
 	mdelay(120);
@@ -137,6 +197,10 @@ static int init_display(struct fbtft_par *par)
 	 */
 	write_reg(par, PWCTRL1, 0xA4, 0xA1);
 
+	/* TE line output is off by default when powering on */
+	if (par->irq_te)
+		write_reg(par, MIPI_DCS_SET_TEAR_ON, 0x00);
+
 	write_reg(par, MIPI_DCS_SET_DISPLAY_ON);
 
 	if (HSD20_IPS)
diff --git a/drivers/staging/fbtft/fbtft-bus.c
b/drivers/staging/fbtft/fbtft-bus.c index 63c65dd..5bc2fff 100644
--- a/drivers/staging/fbtft/fbtft-bus.c
+++ b/drivers/staging/fbtft/fbtft-bus.c
@@ -2,9 +2,15 @@
 #include <linux/export.h>
 #include <linux/errno.h>
 #include <linux/gpio/consumer.h>
+#include <linux/interrupt.h>
+#include <linux/completion.h>
 #include <linux/spi/spi.h>
+
 #include "fbtft.h"
 
+/* 60Hz for 16.6ms, configured as 2*16.6ms */
+#define PANEL_TE_TIMEOUT_MS  33
+
 /*****************************************************************************
  *
  *   void (*write_reg)(struct fbtft_par *par, int len, ...);
@@ -135,8 +141,18 @@ 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);
+
+	if (par->irq_te) {
+		enable_irq(par->irq_te);
+		reinit_completion(&par->panel_te);
+		ret = wait_for_completion_timeout(&par->panel_te,
+
msecs_to_jiffies(PANEL_TE_TIMEOUT_MS));
+		if (ret == 0)
+			dev_err(par->info->device, "wait panel TE
timeout\n"); +
+		disable_irq(par->irq_te);
+	}
 
 	/* non buffered write */
 	if (!par->txbuf.buf)
diff --git a/drivers/staging/fbtft/fbtft.h
b/drivers/staging/fbtft/fbtft.h index 76f8c09..61c7207 100644
--- a/drivers/staging/fbtft/fbtft.h
+++ b/drivers/staging/fbtft/fbtft.h
@@ -8,6 +8,7 @@
 #include <linux/spinlock.h>
 #include <linux/spi/spi.h>
 #include <linux/platform_device.h>
+#include <linux/completion.h>
 
 #define FBTFT_ONBOARD_BACKLIGHT 2
 
@@ -165,6 +166,8 @@ struct fbtft_platform_data {
  *             Format: 6 bit Device id + RS bit + RW bit
  * @fbtftops: FBTFT operations provided by driver or device
(platform_data)
  * @dirty_lock: Protects dirty_lines_start and dirty_lines_end
+ * @panel_te: completion for panel TE line
+ * @irq_te: Linux IRQ for LCD TE line
  * @dirty_lines_start: Where to begin updating display
  * @dirty_lines_end: Where to end updating display
  * @gpio.reset: GPIO used to reset display
@@ -203,6 +206,8 @@ struct fbtft_par {
 	u8 startbyte;
 	struct fbtft_ops fbtftops;
 	spinlock_t dirty_lock;
+	struct completion panel_te;
+	int irq_te;
 	unsigned int dirty_lines_start;
 	unsigned int dirty_lines_end;
 	struct {
-- 
1.9.1

regards,
Carlis 
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v12] staging: fbtft: add tearing signal detect

[carlis]

On Fri, 29 Jan 2021 16:26:12 +0200
Andy Shevchenko <andy.shevchenko@gmail.com> wrote:

> On Fri, Jan 29, 2021 at 3:56 PM carlis <zhangxuezhi3@gmail.com> wrote:
> > On Fri, 29 Jan 2021 12:23:08 +0200
> > Andy Shevchenko <andy.shevchenko@gmail.com> wrote:  
> 
> We are almost there, I have no idea what Noralf or others are going to
> say though.
> 
> ...
> 
> > Hi, I apologize for what I said in the previous two emails. I missed
> > one email you sent before, and now I probably understand what you
> > meant. Here is a version I modified according to your suggestion:
> >
> > From 399e7fb91d1dcba4924cd38cc8283393c80b97e4 Mon Sep 17 00:00:00
> > 2001 From: Carlis <zhangxuezhi1@yulong.com>
> > Date: Sun, 24 Jan 2021 22:43:21 +0800
> > Subject: [PATCH v13] staging: fbtft: add tearing signal detect
> >
> > For st7789v IC,when we need continuous full screen refresh, it is
> > best  
> 
> Missed space after comma.
> 
> > to wait for the tearing effect line signal arrive to avoid screen  
> 
> to arrive
> 
> > tearing.  
> 
> ...
> 
> > +#define PANEL_TE_TIMEOUT_MS  34 /* 60Hz for 16.6ms, configured as
> > 2*16.6ms */ +  
> 
> Move comment before the definition
> /* comment */
> #define DEFINITION
> 
> Also consider to use 33 ms as closest to what you mentioned in the
> comment. Or leave it with mention that you are using ceil() value.
> 
> ...
> 
> > +/**
> > + * init_tearing_effect_line() - init tearing effect line  
> 
> > + *  
> 
> As per a few previous reviews.
> Okay, I have noticed that the existing kernel-doc is written like
> this, but it doesn't prevent you from avoiding this little mistake.
> 
> > + * @par: FBTFT parameter object
> > + *
> > + * Return: 0 on success, or a negative error code otherwise.
> > + */
> > +static int init_tearing_effect_line(struct fbtft_par *par)
> > +{
> > +       struct device *dev = par->info->device;
> > +       struct gpio_desc *te;
> > +       int rc;
> > +
> > +       te = gpiod_get_optional(dev, "te", GPIOD_IN);
> > +       if (IS_ERR(te))
> > +               return dev_err_probe(dev, PTR_ERR(te), "Failed to
> > request te GPIO\n"); +  
> 
> Below is okay, but needs a comment explaining why we return a success.
> 
> > +       if (!te)
> > +               return 0;
> > +
> > +       par->irq_te = gpiod_to_irq(te);
> > +
> > +       /* GPIO is locked as an IRQ, we may drop the reference */
> > +       gpiod_put(te);
> > +
> > +       if (par->irq_te < 0)
> > +               return par->irq_te;  
> 
> I recommend using a temporary variable. In such a case you won't need
> to specifically check for negative error code. So, something like
> 
> int irq;
> 
> irq = ...
> 
> if (irq < 0)
>   return irq;
> 
> ->irq_te = irq;  
> 
> > +       init_completion(&par->panel_te);
> > +       rc = devm_request_irq(dev, par->irq_te, panel_te_handler,
> > +                             IRQF_TRIGGER_RISING, "TE_GPIO", par);
> >  
> 
> Right. Now it needs a comment explaining the choice of rising edge
> type of IRQ.
> 
> > +       if (rc)
> > +               return dev_err_probe(dev, rc, "TE IRQ request
> > failed.\n"); +
> > +       disable_irq_nosync(par->irq_te);
> > +
> > +       return 0;
> > +}  
> 
> ...
> 
> > +       rc = init_tearing_effect_line(par);  
> 
> > +       if (rc < 0)  
> 
> Here is no need to specifically check against less than 0,
>   if (ret)
> will work nicely.
> 
> > +               return rc;  
> 
> ...
> 
> > +       if (par->irq_te)
> > +               write_reg(par, MIPI_DCS_SET_TEAR_ON, 0x00);  
> 
> Do you need to call MIPI_DCS_SET_TEAR_SCANLINE in this case?
> 
Hi, TE line output is off by default when powering on, and I use
Qualcomm SDX55 chip, SPI top speed is only 50MHz, its data transmission
speed( It takes about 24ms to transmit a frame) is slower than 60Hz
refresh(a frame only need 16.6ms), so i think there is no need to call
MIPI_DCS_SET_TEAR_SCANLINE 

> Alos, when there is no IRQ, shouldn't we explicitly call
>                write_reg(par, MIPI_DCS_SET_TEAR_OFF);
> ?
> 
> ...
> 
> >  /**
> > + * st7789v_write_vmem16_bus8() - write data to display  
> 
> > + *  
> 
> Redundant.
> 
> > + * @par: FBTFT parameter object
> > + * @offset: offset from screen_buffer
> > + * @len: the length of data to be written
> > + *  
> 
> > + * 16 bit pixel over 8-bit databus  
> 
> Write 16-bit pixels over 8-bit data bus.
> 
> > + * Return: 0 on success, or a negative error code otherwise.
> > + */  
> 
> ...
> 
> > +       if (par->irq_te) {
> > +               enable_irq(par->irq_te);
> > +               reinit_completion(&par->panel_te);
> > +               ret = wait_for_completion_timeout(&par->panel_te,
> > +
> > msecs_to_jiffies(PANEL_TE_TIMEOUT_MS));
> > +               if (ret == 0)
> > +                       dev_err(dev, "wait panel TE time out\n");  
> 
> timeout
> 
> > +
> > +               disable_irq(par->irq_te);
> > +       }  
> 
> ...
> 
> > + * @panel_te: completion for panel te line  
> 
> TE line
> 
> > + * @irq_te: LCD Chip tearing effect line  
> 
> "Linux IRQ for LCD..."
> 

hi, i modify new below:

From b65fc2bfbe123defc8ec838601a9c12707f70c53 Mon Sep 17 00:00:00 2001
From: Carlis <zhangxuezhi1@yulong.com>
Date: Sun, 24 Jan 2021 22:43:21 +0800
Subject: [PATCH v13] staging: fbtft: add tearing signal detect

For st7789v IC, when we need continuous full screen refresh, it is best
to wait for the tearing effect line signal to arrive to avoid screen
tearing.

Signed-off-by: Carlis <zhangxuezhi1@yulong.com>
---
v13: change TE completion to par data struct member and add a new
     function to deal te gpio request, move wait logic to
fbtft_write_vmem16_bus8. v12: change dev_err to dev_err_probe and add
space in comments start, and delete te_mutex, change te wait logic.
v11: remove devm_gpio_put and change a dev_err to dev_info.
v10: additional notes.
v9: change pr_* to dev_*.
v8: delete a log line.
v7: return error value when request fail.
v6: add te gpio request fail deal logic.
v5: fix log print.
v4: modify some code style and change te irq set function name.
v3: modify author and signed-off-by name.
v2: add release te gpio after irq request fail.
---
 drivers/staging/fbtft/fb_st7789v.c | 64
++++++++++++++++++++++++++++++++++++++
drivers/staging/fbtft/fbtft-bus.c  | 20 ++++++++++--
drivers/staging/fbtft/fbtft.h      |  5 +++ 3 files changed, 87
insertions(+), 2 deletions(-)

diff --git a/drivers/staging/fbtft/fb_st7789v.c
b/drivers/staging/fbtft/fb_st7789v.c index 3a280cc..695dcac 100644
--- a/drivers/staging/fbtft/fb_st7789v.c
+++ b/drivers/staging/fbtft/fb_st7789v.c
@@ -7,9 +7,13 @@
 
 #include <linux/bitops.h>
 #include <linux/delay.h>
+#include <linux/gpio/consumer.h>
 #include <linux/init.h>
 #include <linux/kernel.h>
+#include <linux/interrupt.h>
+#include <linux/completion.h>
 #include <linux/module.h>
+
 #include <video/mipi_display.h>
 
 #include "fbtft.h"
@@ -66,6 +70,56 @@ enum st7789v_command {
 #define MADCTL_MX BIT(6) /* bitmask for column address order */
 #define MADCTL_MY BIT(7) /* bitmask for page address order */
 
+static irqreturn_t panel_te_handler(int irq, void *data)
+{
+	struct fbtft_par *par = (struct fbtft_par *)data;
+
+	complete(&par->panel_te);
+	return IRQ_HANDLED;
+}
+
+/*
+ * init_tearing_effect_line() - init tearing effect line.
+ * @par: FBTFT parameter object.
+ *
+ * Return: 0 on success, or a negative error code otherwise.
+ */
+static int init_tearing_effect_line(struct fbtft_par *par)
+{
+	struct device *dev = par->info->device;
+	struct gpio_desc *te;
+	int rc, irq;
+
+	te = gpiod_get_optional(dev, "te", GPIOD_IN);
+	if (IS_ERR(te))
+		return dev_err_probe(dev, PTR_ERR(te), "Failed to
request te GPIO\n"); +
+	/* if te is NULL, indicating no configuration, directly return
success */
+	if (!te)
+		return 0;
+
+	irq = gpiod_to_irq(te);
+
+	/* GPIO is locked as an IRQ, we may drop the reference */
+	gpiod_put(te);
+
+	if (irq < 0)
+		return irq;
+
+	par->irq_te = irq;
+	init_completion(&par->panel_te);
+
+	/* The effective state is high and lasts no more than 1000
microseconds */
+	rc = devm_request_irq(dev, par->irq_te, panel_te_handler,
+			      IRQF_TRIGGER_RISING, "TE_GPIO", par);
+	if (rc)
+		return dev_err_probe(dev, rc, "TE IRQ request
failed.\n"); +
+	disable_irq_nosync(par->irq_te);
+
+	return 0;
+}
+
 /**
  * init_display() - initialize the display controller
  *
@@ -82,6 +136,12 @@ enum st7789v_command {
  */
 static int init_display(struct fbtft_par *par)
 {
+	int rc;
+
+	rc = init_tearing_effect_line(par);
+	if (rc)
+		return rc;
+
 	/* turn off sleep mode */
 	write_reg(par, MIPI_DCS_EXIT_SLEEP_MODE);
 	mdelay(120);
@@ -137,6 +197,10 @@ static int init_display(struct fbtft_par *par)
 	 */
 	write_reg(par, PWCTRL1, 0xA4, 0xA1);
 
+	/* TE line output is off by default when powering on */
+	if (par->irq_te)
+		write_reg(par, MIPI_DCS_SET_TEAR_ON, 0x00);
+
 	write_reg(par, MIPI_DCS_SET_DISPLAY_ON);
 
 	if (HSD20_IPS)
diff --git a/drivers/staging/fbtft/fbtft-bus.c
b/drivers/staging/fbtft/fbtft-bus.c index 63c65dd..5bc2fff 100644
--- a/drivers/staging/fbtft/fbtft-bus.c
+++ b/drivers/staging/fbtft/fbtft-bus.c
@@ -2,9 +2,15 @@
 #include <linux/export.h>
 #include <linux/errno.h>
 #include <linux/gpio/consumer.h>
+#include <linux/interrupt.h>
+#include <linux/completion.h>
 #include <linux/spi/spi.h>
+
 #include "fbtft.h"
 
+/* 60Hz for 16.6ms, configured as 2*16.6ms */
+#define PANEL_TE_TIMEOUT_MS  33
+
 /*****************************************************************************
  *
  *   void (*write_reg)(struct fbtft_par *par, int len, ...);
@@ -135,8 +141,18 @@ 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);
+
+	if (par->irq_te) {
+		enable_irq(par->irq_te);
+		reinit_completion(&par->panel_te);
+		ret = wait_for_completion_timeout(&par->panel_te,
+
msecs_to_jiffies(PANEL_TE_TIMEOUT_MS));
+		if (ret == 0)
+			dev_err(par->info->device, "wait panel TE
timeout\n"); +
+		disable_irq(par->irq_te);
+	}
 
 	/* non buffered write */
 	if (!par->txbuf.buf)
diff --git a/drivers/staging/fbtft/fbtft.h
b/drivers/staging/fbtft/fbtft.h index 76f8c09..61c7207 100644
--- a/drivers/staging/fbtft/fbtft.h
+++ b/drivers/staging/fbtft/fbtft.h
@@ -8,6 +8,7 @@
 #include <linux/spinlock.h>
 #include <linux/spi/spi.h>
 #include <linux/platform_device.h>
+#include <linux/completion.h>
 
 #define FBTFT_ONBOARD_BACKLIGHT 2
 
@@ -165,6 +166,8 @@ struct fbtft_platform_data {
  *             Format: 6 bit Device id + RS bit + RW bit
  * @fbtftops: FBTFT operations provided by driver or device
(platform_data)
  * @dirty_lock: Protects dirty_lines_start and dirty_lines_end
+ * @panel_te: completion for panel TE line
+ * @irq_te: Linux IRQ for LCD TE line
  * @dirty_lines_start: Where to begin updating display
  * @dirty_lines_end: Where to end updating display
  * @gpio.reset: GPIO used to reset display
@@ -203,6 +206,8 @@ struct fbtft_par {
 	u8 startbyte;
 	struct fbtft_ops fbtftops;
 	spinlock_t dirty_lock;
+	struct completion panel_te;
+	int irq_te;
 	unsigned int dirty_lines_start;
 	unsigned int dirty_lines_end;
 	struct {
-- 
1.9.1

regards,
Carlis 
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v12] staging: fbtft: add tearing signal detect

[Andy Shevchenko]

On Sat, Jan 30, 2021 at 8:39 AM carlis <zhangxuezhi3@gmail.com> wrote:
> On Fri, 29 Jan 2021 16:26:12 +0200
> Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
> > On Fri, Jan 29, 2021 at 3:56 PM carlis <zhangxuezhi3@gmail.com> wrote:
> > > On Fri, 29 Jan 2021 12:23:08 +0200
> > > Andy Shevchenko <andy.shevchenko@gmail.com> wrote:

...

> > > Hi, I apologize for what I said in the previous two emails. I missed
> > > one email you sent before, and now I probably understand what you
> > > meant. Here is a version I modified according to your suggestion:

I have realized that you are mocking stuff in the generic fbtft
structure for all drivers while only a single one is going to use
that. Consider moving everything to the driver in question.


-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v12] staging: fbtft: add tearing signal detect

[Andy Shevchenko]

On Sat, Jan 30, 2021 at 8:39 AM carlis <zhangxuezhi3@gmail.com> wrote:
> On Fri, 29 Jan 2021 16:26:12 +0200
> Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
> > On Fri, Jan 29, 2021 at 3:56 PM carlis <zhangxuezhi3@gmail.com> wrote:
> > > On Fri, 29 Jan 2021 12:23:08 +0200
> > > Andy Shevchenko <andy.shevchenko@gmail.com> wrote:

...

> > > Hi, I apologize for what I said in the previous two emails. I missed
> > > one email you sent before, and now I probably understand what you
> > > meant. Here is a version I modified according to your suggestion:

I have realized that you are mocking stuff in the generic fbtft
structure for all drivers while only a single one is going to use
that. Consider moving everything to the driver in question.


-- 
With Best Regards,
Andy Shevchenko
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v12] staging: fbtft: add tearing signal detect

[Andy Shevchenko]

On Sat, Jan 30, 2021 at 8:39 AM carlis <zhangxuezhi3@gmail.com> wrote:
> On Fri, 29 Jan 2021 16:26:12 +0200
> Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
> > On Fri, Jan 29, 2021 at 3:56 PM carlis <zhangxuezhi3@gmail.com> wrote:
> > > On Fri, 29 Jan 2021 12:23:08 +0200
> > > Andy Shevchenko <andy.shevchenko@gmail.com> wrote:

...

> > > Hi, I apologize for what I said in the previous two emails. I missed
> > > one email you sent before, and now I probably understand what you
> > > meant. Here is a version I modified according to your suggestion:

I have realized that you are mocking stuff in the generic fbtft
structure for all drivers while only a single one is going to use
that. Consider moving everything to the driver in question.


-- 
With Best Regards,
Andy Shevchenko
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v12] staging: fbtft: add tearing signal detect

[Carlis]

On Mon, 1 Feb 2021 19:40:21 +0200
Andy Shevchenko <andy.shevchenko@gmail.com> wrote:

> On Sat, Jan 30, 2021 at 8:39 AM carlis <zhangxuezhi3@gmail.com> wrote:
> > On Fri, 29 Jan 2021 16:26:12 +0200
> > Andy Shevchenko <andy.shevchenko@gmail.com> wrote:  
> > > On Fri, Jan 29, 2021 at 3:56 PM carlis <zhangxuezhi3@gmail.com>
> > > wrote:  
> > > > On Fri, 29 Jan 2021 12:23:08 +0200
> > > > Andy Shevchenko <andy.shevchenko@gmail.com> wrote:  
> 
> ...
> 
> > > > Hi, I apologize for what I said in the previous two emails. I
> > > > missed one email you sent before, and now I probably understand
> > > > what you meant. Here is a version I modified according to your
> > > > suggestion:  
> 
> I have realized that you are mocking stuff in the generic fbtft
> structure for all drivers while only a single one is going to use
> that. Consider moving everything to the driver in question.
> 
> 

hi, 
   Do you mean that i define the TE completion and irq_te in the
   fb_st7789v.c as i did before?

Re: [PATCH v12] staging: fbtft: add tearing signal detect

[Carlis]

On Mon, 1 Feb 2021 19:40:21 +0200
Andy Shevchenko <andy.shevchenko@gmail.com> wrote:

> On Sat, Jan 30, 2021 at 8:39 AM carlis <zhangxuezhi3@gmail.com> wrote:
> > On Fri, 29 Jan 2021 16:26:12 +0200
> > Andy Shevchenko <andy.shevchenko@gmail.com> wrote:  
> > > On Fri, Jan 29, 2021 at 3:56 PM carlis <zhangxuezhi3@gmail.com>
> > > wrote:  
> > > > On Fri, 29 Jan 2021 12:23:08 +0200
> > > > Andy Shevchenko <andy.shevchenko@gmail.com> wrote:  
> 
> ...
> 
> > > > Hi, I apologize for what I said in the previous two emails. I
> > > > missed one email you sent before, and now I probably understand
> > > > what you meant. Here is a version I modified according to your
> > > > suggestion:  
> 
> I have realized that you are mocking stuff in the generic fbtft
> structure for all drivers while only a single one is going to use
> that. Consider moving everything to the driver in question.
> 
> 

hi, 
   Do you mean that i define the TE completion and irq_te in the
   fb_st7789v.c as i did before?
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v12] staging: fbtft: add tearing signal detect

[Carlis]

On Mon, 1 Feb 2021 19:40:21 +0200
Andy Shevchenko <andy.shevchenko@gmail.com> wrote:

> On Sat, Jan 30, 2021 at 8:39 AM carlis <zhangxuezhi3@gmail.com> wrote:
> > On Fri, 29 Jan 2021 16:26:12 +0200
> > Andy Shevchenko <andy.shevchenko@gmail.com> wrote:  
> > > On Fri, Jan 29, 2021 at 3:56 PM carlis <zhangxuezhi3@gmail.com>
> > > wrote:  
> > > > On Fri, 29 Jan 2021 12:23:08 +0200
> > > > Andy Shevchenko <andy.shevchenko@gmail.com> wrote:  
> 
> ...
> 
> > > > Hi, I apologize for what I said in the previous two emails. I
> > > > missed one email you sent before, and now I probably understand
> > > > what you meant. Here is a version I modified according to your
> > > > suggestion:  
> 
> I have realized that you are mocking stuff in the generic fbtft
> structure for all drivers while only a single one is going to use
> that. Consider moving everything to the driver in question.
> 
> 

hi, 
   Do you mean that i define the TE completion and irq_te in the
   fb_st7789v.c as i did before?
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v12] staging: fbtft: add tearing signal detect

[Andy Shevchenko]

On Tue, Feb 2, 2021 at 3:52 AM Carlis <zhangxuezhi3@gmail.com> wrote:
> On Mon, 1 Feb 2021 19:40:21 +0200
> Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
>
> > On Sat, Jan 30, 2021 at 8:39 AM carlis <zhangxuezhi3@gmail.com> wrote:
> > > On Fri, 29 Jan 2021 16:26:12 +0200
> > > Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
> > > > On Fri, Jan 29, 2021 at 3:56 PM carlis <zhangxuezhi3@gmail.com>
> > > > wrote:
> > > > > On Fri, 29 Jan 2021 12:23:08 +0200
> > > > > Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
> >
> > ...
> >
> > > > > Hi, I apologize for what I said in the previous two emails. I
> > > > > missed one email you sent before, and now I probably understand
> > > > > what you meant. Here is a version I modified according to your
> > > > > suggestion:
> >
> > I have realized that you are mocking stuff in the generic fbtft
> > structure for all drivers while only a single one is going to use
> > that. Consider moving everything to the driver in question.

>    Do you mean that i define the TE completion and irq_te in the
>    fb_st7789v.c as i did before?

Not in global variables. Perhaps it will require to add/update the
custom (to the specific driver) data structure.
But the idea is that all changes should be isolated to that driver.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v12] staging: fbtft: add tearing signal detect

[Andy Shevchenko]

On Tue, Feb 2, 2021 at 3:52 AM Carlis <zhangxuezhi3@gmail.com> wrote:
> On Mon, 1 Feb 2021 19:40:21 +0200
> Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
>
> > On Sat, Jan 30, 2021 at 8:39 AM carlis <zhangxuezhi3@gmail.com> wrote:
> > > On Fri, 29 Jan 2021 16:26:12 +0200
> > > Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
> > > > On Fri, Jan 29, 2021 at 3:56 PM carlis <zhangxuezhi3@gmail.com>
> > > > wrote:
> > > > > On Fri, 29 Jan 2021 12:23:08 +0200
> > > > > Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
> >
> > ...
> >
> > > > > Hi, I apologize for what I said in the previous two emails. I
> > > > > missed one email you sent before, and now I probably understand
> > > > > what you meant. Here is a version I modified according to your
> > > > > suggestion:
> >
> > I have realized that you are mocking stuff in the generic fbtft
> > structure for all drivers while only a single one is going to use
> > that. Consider moving everything to the driver in question.

>    Do you mean that i define the TE completion and irq_te in the
>    fb_st7789v.c as i did before?

Not in global variables. Perhaps it will require to add/update the
custom (to the specific driver) data structure.
But the idea is that all changes should be isolated to that driver.

-- 
With Best Regards,
Andy Shevchenko
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v12] staging: fbtft: add tearing signal detect

[Andy Shevchenko]

On Tue, Feb 2, 2021 at 3:52 AM Carlis <zhangxuezhi3@gmail.com> wrote:
> On Mon, 1 Feb 2021 19:40:21 +0200
> Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
>
> > On Sat, Jan 30, 2021 at 8:39 AM carlis <zhangxuezhi3@gmail.com> wrote:
> > > On Fri, 29 Jan 2021 16:26:12 +0200
> > > Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
> > > > On Fri, Jan 29, 2021 at 3:56 PM carlis <zhangxuezhi3@gmail.com>
> > > > wrote:
> > > > > On Fri, 29 Jan 2021 12:23:08 +0200
> > > > > Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
> >
> > ...
> >
> > > > > Hi, I apologize for what I said in the previous two emails. I
> > > > > missed one email you sent before, and now I probably understand
> > > > > what you meant. Here is a version I modified according to your
> > > > > suggestion:
> >
> > I have realized that you are mocking stuff in the generic fbtft
> > structure for all drivers while only a single one is going to use
> > that. Consider moving everything to the driver in question.

>    Do you mean that i define the TE completion and irq_te in the
>    fb_st7789v.c as i did before?

Not in global variables. Perhaps it will require to add/update the
custom (to the specific driver) data structure.
But the idea is that all changes should be isolated to that driver.

-- 
With Best Regards,
Andy Shevchenko
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel