linux-fbdev.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v10] staging: fbtft: add tearing signal detect
@ 2021-01-27 13:42 Carlis
  2021-01-27 13:51 ` Greg KH
  2021-01-27 22:32 ` Kari Argillander
  0 siblings, 2 replies; 21+ messages in thread
From: Carlis @ 2021-01-27 13:42 UTC (permalink / raw)
  To: gregkh
  Cc: colin.king, oliver.graute, zhangxuezhi1, mh12gx2825, sbrivio,
	dri-devel, linux-fbdev, devel, linux-kernel

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>
---
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 | 132 ++++++++++++++++++++++++++++++++++++-
 drivers/staging/fbtft/fbtft.h      |   1 +
 2 files changed, 132 insertions(+), 1 deletion(-)

diff --git a/drivers/staging/fbtft/fb_st7789v.c b/drivers/staging/fbtft/fb_st7789v.c
index 3a280cc..cba08a8 100644
--- a/drivers/staging/fbtft/fb_st7789v.c
+++ b/drivers/staging/fbtft/fb_st7789v.c
@@ -9,9 +9,12 @@
 #include <linux/delay.h>
 #include <linux/init.h>
 #include <linux/kernel.h>
+#include <linux/mutex.h>
+#include <linux/interrupt.h>
+#include <linux/completion.h>
 #include <linux/module.h>
 #include <video/mipi_display.h>
-
+#include <linux/gpio/consumer.h>
 #include "fbtft.h"
 
 #define DRVNAME "fb_st7789v"
@@ -66,6 +69,32 @@ 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 mutex te_mutex;/* mutex for set te gpio irq status */
+static struct completion spi_panel_te;
+
+static irqreturn_t spi_panel_te_handler(int irq, void *data)
+{
+	complete(&spi_panel_te);
+	return IRQ_HANDLED;
+}
+
+static void set_spi_panel_te_irq_status(struct fbtft_par *par, bool enable)
+{
+	static int te_irq_count;
+
+	mutex_lock(&te_mutex);
+
+	if (enable) {
+		if (++te_irq_count == 1)
+			enable_irq(gpiod_to_irq(par->gpio.te));
+	} else {
+		if (--te_irq_count == 0)
+			disable_irq(gpiod_to_irq(par->gpio.te));
+	}
+	mutex_unlock(&te_mutex);
+}
+
 /**
  * init_display() - initialize the display controller
  *
@@ -82,6 +111,33 @@ 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)) {
+		rc = PTR_ERR(par->gpio.te);
+		dev_err(par->info->device, "Failed to request te gpio: %d\n", rc);
+		return rc;
+	}
+	if (par->gpio.te) {
+		init_completion(&spi_panel_te);
+		mutex_init(&te_mutex);
+		rc = devm_request_irq(dev,
+				      gpiod_to_irq(par->gpio.te),
+				     spi_panel_te_handler, IRQF_TRIGGER_RISING,
+				     "TE_GPIO", par);
+		if (rc) {
+			dev_err(par->info->device, "TE request_irq failed.\n");
+			devm_gpiod_put(dev, par->gpio.te);
+			return rc;
+		}
+
+		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 +193,9 @@ 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)
@@ -145,6 +204,76 @@ static int init_display(struct fbtft_par *par)
 	return 0;
 }
 
+/*****************************************************************************
+ *
+ *   int (*write_vmem)(struct fbtft_par *par);
+ *
+ *****************************************************************************/
+
+/* 16 bit pixel over 8-bit databus */
+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);
+
+	/* 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;
+		if (par->gpio.te) {
+			set_spi_panel_te_irq_status(par, true);
+			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");
+		}
+		ret = par->fbtftops.write(par, par->txbuf.buf,
+					 startbyte_size + to_copy * 2);
+		if (par->gpio.te)
+			set_spi_panel_te_irq_status(par, false);
+		if (ret < 0)
+			return ret;
+		remain -= to_copy;
+	}
+
+	return ret;
+}
+
 /**
  * set_var() - apply LCD properties like rotation and BGR mode
  *
@@ -259,6 +388,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


^ permalink raw reply related	[flat|nested] 21+ messages in thread

* Re: [PATCH v10] staging: fbtft: add tearing signal detect
  2021-01-27 13:42 [PATCH v10] staging: fbtft: add tearing signal detect Carlis
@ 2021-01-27 13:51 ` Greg KH
  2021-01-27 14:08   ` carlis
  2021-01-27 22:32 ` Kari Argillander
  1 sibling, 1 reply; 21+ messages in thread
From: Greg KH @ 2021-01-27 13:51 UTC (permalink / raw)
  To: Carlis
  Cc: devel, linux-fbdev, mh12gx2825, oliver.graute, linux-kernel,
	dri-devel, sbrivio, colin.king, zhangxuezhi1

On Wed, Jan 27, 2021 at 09:42:52PM +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>

Please slow down and wait at least a day between patch submissions,
there is no rush here.

And also, ALWAYS run scripts/checkpatch.pl on your submissions, so that
you don't have a maintainer asking you about basic problems, like are in
this current patch :(

thanks,

greg k-h

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH v10] staging: fbtft: add tearing signal detect
  2021-01-27 13:51 ` Greg KH
@ 2021-01-27 14:08   ` carlis
  2021-01-27 14:13     ` Greg KH
  0 siblings, 1 reply; 21+ messages in thread
From: carlis @ 2021-01-27 14:08 UTC (permalink / raw)
  To: Greg KH
  Cc: devel, linux-fbdev, mh12gx2825, oliver.graute, linux-kernel,
	dri-devel, sbrivio, colin.king, zhangxuezhi1

On Wed, 27 Jan 2021 14:51:55 +0100
Greg KH <gregkh@linuxfoundation.org> wrote:

> On Wed, Jan 27, 2021 at 09:42:52PM +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>  
> 
> Please slow down and wait at least a day between patch submissions,
> there is no rush here.
> 
> And also, ALWAYS run scripts/checkpatch.pl on your submissions, so
> that you don't have a maintainer asking you about basic problems,
> like are in this current patch :(
> 
> thanks,
> 
> greg k-h

hi,
  This is my first patch contribution to Linux, so some of the rules
are not very clear .In addition, I can confirm that before sending
patch, I check it with checkPatch.py every time.Thank you very much for
your help

regards
zhangxuezhi

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH v10] staging: fbtft: add tearing signal detect
  2021-01-27 14:08   ` carlis
@ 2021-01-27 14:13     ` Greg KH
  2021-01-27 14:17       ` carlis
  0 siblings, 1 reply; 21+ messages in thread
From: Greg KH @ 2021-01-27 14:13 UTC (permalink / raw)
  To: carlis
  Cc: devel, linux-fbdev, mh12gx2825, oliver.graute, linux-kernel,
	dri-devel, sbrivio, colin.king, zhangxuezhi1

On Wed, Jan 27, 2021 at 10:08:09PM +0800, carlis wrote:
> On Wed, 27 Jan 2021 14:51:55 +0100
> Greg KH <gregkh@linuxfoundation.org> wrote:
> 
> > On Wed, Jan 27, 2021 at 09:42:52PM +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>  
> > 
> > Please slow down and wait at least a day between patch submissions,
> > there is no rush here.
> > 
> > And also, ALWAYS run scripts/checkpatch.pl on your submissions, so
> > that you don't have a maintainer asking you about basic problems,
> > like are in this current patch :(
> > 
> > thanks,
> > 
> > greg k-h
> 
> hi,
>   This is my first patch contribution to Linux, so some of the rules
> are not very clear .In addition, I can confirm that before sending
> patch, I check it with checkPatch.py every time.Thank you very much for
> your help

Please read Documentation/SubmittingPatches which has a link to the
checklist and other documentation you should read.

And I doubt you are running checkpatch on your submission, as there is
obvious coding style issues in it.  If so, please provide the output as
it must be broken :(

thanks,

greg k-h

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH v10] staging: fbtft: add tearing signal detect
  2021-01-27 14:13     ` Greg KH
@ 2021-01-27 14:17       ` carlis
  2021-01-27 14:25         ` Greg KH
  0 siblings, 1 reply; 21+ messages in thread
From: carlis @ 2021-01-27 14:17 UTC (permalink / raw)
  To: Greg KH
  Cc: devel, linux-fbdev, mh12gx2825, oliver.graute, linux-kernel,
	dri-devel, sbrivio, colin.king, zhangxuezhi1

On Wed, 27 Jan 2021 15:13:05 +0100
Greg KH <gregkh@linuxfoundation.org> wrote:

> On Wed, Jan 27, 2021 at 10:08:09PM +0800, carlis wrote:
> > On Wed, 27 Jan 2021 14:51:55 +0100
> > Greg KH <gregkh@linuxfoundation.org> wrote:
> >   
> > > On Wed, Jan 27, 2021 at 09:42:52PM +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>    
> > > 
> > > Please slow down and wait at least a day between patch
> > > submissions, there is no rush here.
> > > 
> > > And also, ALWAYS run scripts/checkpatch.pl on your submissions, so
> > > that you don't have a maintainer asking you about basic problems,
> > > like are in this current patch :(
> > > 
> > > thanks,
> > > 
> > > greg k-h  
> > 
> > hi,
> >   This is my first patch contribution to Linux, so some of the rules
> > are not very clear .In addition, I can confirm that before sending
> > patch, I check it with checkPatch.py every time.Thank you very much
> > for your help  
> 
> Please read Documentation/SubmittingPatches which has a link to the
> checklist and other documentation you should read.
> 
> And I doubt you are running checkpatch on your submission, as there is
> obvious coding style issues in it.  If so, please provide the output
> as it must be broken :(
> 
> thanks,
> 
> greg k-h
hi, the patch v11 checkpatch.pl output is below:

carlis@bf-rmsz-10:~/work/linux-kernel/linux$ ./scripts/checkpatch.pl
0001-staging-fbtft-add-tearing-signal-detect.patch total: 0 errors, 0
warnings, 0 checks, 176 lines checked

0001-staging-fbtft-add-tearing-signal-detect.patch has no obvious style
problems and is ready for submission.


regards
zhangxuezhi


^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH v10] staging: fbtft: add tearing signal detect
  2021-01-27 14:17       ` carlis
@ 2021-01-27 14:25         ` Greg KH
  2021-01-27 14:49           ` Dan Carpenter
  0 siblings, 1 reply; 21+ messages in thread
From: Greg KH @ 2021-01-27 14:25 UTC (permalink / raw)
  To: carlis, Andy Whitcroft, Joe Perches
  Cc: devel, linux-fbdev, mh12gx2825, oliver.graute, linux-kernel,
	dri-devel, sbrivio, colin.king, zhangxuezhi1

On Wed, Jan 27, 2021 at 10:17:08PM +0800, carlis wrote:
> On Wed, 27 Jan 2021 15:13:05 +0100
> Greg KH <gregkh@linuxfoundation.org> wrote:
> 
> > On Wed, Jan 27, 2021 at 10:08:09PM +0800, carlis wrote:
> > > On Wed, 27 Jan 2021 14:51:55 +0100
> > > Greg KH <gregkh@linuxfoundation.org> wrote:
> > >   
> > > > On Wed, Jan 27, 2021 at 09:42:52PM +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>    
> > > > 
> > > > Please slow down and wait at least a day between patch
> > > > submissions, there is no rush here.
> > > > 
> > > > And also, ALWAYS run scripts/checkpatch.pl on your submissions, so
> > > > that you don't have a maintainer asking you about basic problems,
> > > > like are in this current patch :(
> > > > 
> > > > thanks,
> > > > 
> > > > greg k-h  
> > > 
> > > hi,
> > >   This is my first patch contribution to Linux, so some of the rules
> > > are not very clear .In addition, I can confirm that before sending
> > > patch, I check it with checkPatch.py every time.Thank you very much
> > > for your help  
> > 
> > Please read Documentation/SubmittingPatches which has a link to the
> > checklist and other documentation you should read.
> > 
> > And I doubt you are running checkpatch on your submission, as there is
> > obvious coding style issues in it.  If so, please provide the output
> > as it must be broken :(
> > 
> > thanks,
> > 
> > greg k-h
> hi, the patch v11 checkpatch.pl output is below:
> 
> carlis@bf-rmsz-10:~/work/linux-kernel/linux$ ./scripts/checkpatch.pl
> 0001-staging-fbtft-add-tearing-signal-detect.patch total: 0 errors, 0
> warnings, 0 checks, 176 lines checked
> 
> 0001-staging-fbtft-add-tearing-signal-detect.patch has no obvious style
> problems and is ready for submission.

Wow, my apologies!

Andy and Joe, there's something wrong here that is missing the fact that
a line is being indented with spaces and not tabs in the patch
at https://lore.kernel.org/r/1611754972-151016-1-git-send-email-zhangxuezhi3@gmail.com

Any ideas what broke?

thanks,

greg k-h

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH v10] staging: fbtft: add tearing signal detect
  2021-01-27 14:25         ` Greg KH
@ 2021-01-27 14:49           ` Dan Carpenter
  2021-01-27 15:02             ` Greg KH
                               ` (2 more replies)
  0 siblings, 3 replies; 21+ messages in thread
From: Dan Carpenter @ 2021-01-27 14:49 UTC (permalink / raw)
  To: Greg KH
  Cc: carlis, Andy Whitcroft, Joe Perches, devel, linux-fbdev,
	mh12gx2825, oliver.graute, linux-kernel, dri-devel, sbrivio,
	colin.king, zhangxuezhi1

On Wed, Jan 27, 2021 at 03:25:20PM +0100, Greg KH wrote:
> On Wed, Jan 27, 2021 at 10:17:08PM +0800, carlis wrote:
> > On Wed, 27 Jan 2021 15:13:05 +0100
> > Greg KH <gregkh@linuxfoundation.org> wrote:
> > 
> > > On Wed, Jan 27, 2021 at 10:08:09PM +0800, carlis wrote:
> > > > On Wed, 27 Jan 2021 14:51:55 +0100
> > > > Greg KH <gregkh@linuxfoundation.org> wrote:
> > > >   
> > > > > On Wed, Jan 27, 2021 at 09:42:52PM +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>    
> > > > > 
> > > > > Please slow down and wait at least a day between patch
> > > > > submissions, there is no rush here.
> > > > > 
> > > > > And also, ALWAYS run scripts/checkpatch.pl on your submissions, so
> > > > > that you don't have a maintainer asking you about basic problems,
> > > > > like are in this current patch :(
> > > > > 
> > > > > thanks,
> > > > > 
> > > > > greg k-h  
> > > > 
> > > > hi,
> > > >   This is my first patch contribution to Linux, so some of the rules
> > > > are not very clear .In addition, I can confirm that before sending
> > > > patch, I check it with checkPatch.py every time.Thank you very much
> > > > for your help  
> > > 
> > > Please read Documentation/SubmittingPatches which has a link to the
> > > checklist and other documentation you should read.
> > > 
> > > And I doubt you are running checkpatch on your submission, as there is
> > > obvious coding style issues in it.  If so, please provide the output
> > > as it must be broken :(
> > > 
> > > thanks,
> > > 
> > > greg k-h
> > hi, the patch v11 checkpatch.pl output is below:
> > 
> > carlis@bf-rmsz-10:~/work/linux-kernel/linux$ ./scripts/checkpatch.pl
> > 0001-staging-fbtft-add-tearing-signal-detect.patch total: 0 errors, 0
> > warnings, 0 checks, 176 lines checked
> > 
> > 0001-staging-fbtft-add-tearing-signal-detect.patch has no obvious style
> > problems and is ready for submission.
> 
> Wow, my apologies!
> 
> Andy and Joe, there's something wrong here that is missing the fact that
> a line is being indented with spaces and not tabs in the patch
> at https://lore.kernel.org/r/1611754972-151016-1-git-send-email-zhangxuezhi3@gmail.com
> 
> Any ideas what broke?
> 

    /*Tearing Effect Line On*/

Comments are the exception to the "no spaces at the start of a line"
rule.  I was expecting that the kbuild-bot would send a Smatch warning
for inconsistent indenting, but comments are not counted there either.

I'm sort of surprised that we don't have checkpatch rule about the
missing space characters.  It should be: "/* Tearing Effect Line On */".

regards,
dan carpenter


^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH v10] staging: fbtft: add tearing signal detect
  2021-01-27 14:49           ` Dan Carpenter
@ 2021-01-27 15:02             ` Greg KH
  2021-01-28  1:12               ` carlis
  2021-01-27 17:42             ` Joe Perches
  2021-01-27 18:21             ` Joe Perches
  2 siblings, 1 reply; 21+ messages in thread
From: Greg KH @ 2021-01-27 15:02 UTC (permalink / raw)
  To: Dan Carpenter
  Cc: devel, linux-fbdev, mh12gx2825, sbrivio, oliver.graute,
	linux-kernel, dri-devel, Joe Perches, carlis, Andy Whitcroft,
	colin.king, zhangxuezhi1

On Wed, Jan 27, 2021 at 05:49:46PM +0300, Dan Carpenter wrote:
> On Wed, Jan 27, 2021 at 03:25:20PM +0100, Greg KH wrote:
> > On Wed, Jan 27, 2021 at 10:17:08PM +0800, carlis wrote:
> > > On Wed, 27 Jan 2021 15:13:05 +0100
> > > Greg KH <gregkh@linuxfoundation.org> wrote:
> > > 
> > > > On Wed, Jan 27, 2021 at 10:08:09PM +0800, carlis wrote:
> > > > > On Wed, 27 Jan 2021 14:51:55 +0100
> > > > > Greg KH <gregkh@linuxfoundation.org> wrote:
> > > > >   
> > > > > > On Wed, Jan 27, 2021 at 09:42:52PM +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>    
> > > > > > 
> > > > > > Please slow down and wait at least a day between patch
> > > > > > submissions, there is no rush here.
> > > > > > 
> > > > > > And also, ALWAYS run scripts/checkpatch.pl on your submissions, so
> > > > > > that you don't have a maintainer asking you about basic problems,
> > > > > > like are in this current patch :(
> > > > > > 
> > > > > > thanks,
> > > > > > 
> > > > > > greg k-h  
> > > > > 
> > > > > hi,
> > > > >   This is my first patch contribution to Linux, so some of the rules
> > > > > are not very clear .In addition, I can confirm that before sending
> > > > > patch, I check it with checkPatch.py every time.Thank you very much
> > > > > for your help  
> > > > 
> > > > Please read Documentation/SubmittingPatches which has a link to the
> > > > checklist and other documentation you should read.
> > > > 
> > > > And I doubt you are running checkpatch on your submission, as there is
> > > > obvious coding style issues in it.  If so, please provide the output
> > > > as it must be broken :(
> > > > 
> > > > thanks,
> > > > 
> > > > greg k-h
> > > hi, the patch v11 checkpatch.pl output is below:
> > > 
> > > carlis@bf-rmsz-10:~/work/linux-kernel/linux$ ./scripts/checkpatch.pl
> > > 0001-staging-fbtft-add-tearing-signal-detect.patch total: 0 errors, 0
> > > warnings, 0 checks, 176 lines checked
> > > 
> > > 0001-staging-fbtft-add-tearing-signal-detect.patch has no obvious style
> > > problems and is ready for submission.
> > 
> > Wow, my apologies!
> > 
> > Andy and Joe, there's something wrong here that is missing the fact that
> > a line is being indented with spaces and not tabs in the patch
> > at https://lore.kernel.org/r/1611754972-151016-1-git-send-email-zhangxuezhi3@gmail.com
> > 
> > Any ideas what broke?
> > 
> 
>     /*Tearing Effect Line On*/
> 
> Comments are the exception to the "no spaces at the start of a line"
> rule.  I was expecting that the kbuild-bot would send a Smatch warning
> for inconsistent indenting, but comments are not counted there either.
> 
> I'm sort of surprised that we don't have checkpatch rule about the
> missing space characters.  It should be: "/* Tearing Effect Line On */".

That was going to be my next question, lots of comments added in this
patch don't have spaces...

thanks,

greg k-h

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH v10] staging: fbtft: add tearing signal detect
  2021-01-27 14:49           ` Dan Carpenter
  2021-01-27 15:02             ` Greg KH
@ 2021-01-27 17:42             ` Joe Perches
  2021-01-27 18:21             ` Joe Perches
  2 siblings, 0 replies; 21+ messages in thread
From: Joe Perches @ 2021-01-27 17:42 UTC (permalink / raw)
  To: Dan Carpenter, Greg KH
  Cc: carlis, Andy Whitcroft, devel, linux-fbdev, mh12gx2825,
	oliver.graute, linux-kernel, dri-devel, sbrivio, colin.king,
	zhangxuezhi1

On Wed, 2021-01-27 at 17:49 +0300, Dan Carpenter wrote:
> On Wed, Jan 27, 2021 at 03:25:20PM +0100, Greg KH wrote:

> > Andy and Joe, there's something wrong here that is missing the fact that
> > a line is being indented with spaces and not tabs in the patch
> > at https://lore.kernel.org/r/1611754972-151016-1-git-send-email-zhangxuezhi3@gmail.com
> > 
> > Any ideas what broke?
> 
>     /*Tearing Effect Line On*/
> 
> Comments are the exception to the "no spaces at the start of a line"
> rule.  I was expecting that the kbuild-bot would send a Smatch warning
> for inconsistent indenting, but comments are not counted there either.
> 
> I'm sort of surprised that we don't have checkpatch rule about the
> missing space characters.  It should be: "/* Tearing Effect Line On */".

You could always write your own rule...

checkpatch doesn't care if a comment looks like

    /********************/
or
    /*foobarfoobarfoobar*/



^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH v10] staging: fbtft: add tearing signal detect
  2021-01-27 14:49           ` Dan Carpenter
  2021-01-27 15:02             ` Greg KH
  2021-01-27 17:42             ` Joe Perches
@ 2021-01-27 18:21             ` Joe Perches
  2 siblings, 0 replies; 21+ messages in thread
From: Joe Perches @ 2021-01-27 18:21 UTC (permalink / raw)
  To: Dan Carpenter, Greg KH
  Cc: carlis, Andy Whitcroft, devel, linux-fbdev, mh12gx2825,
	oliver.graute, linux-kernel, dri-devel, sbrivio, colin.king,
	zhangxuezhi1

> Comments are the exception to the "no spaces at the start of a line"
> rule.  I was expecting that the kbuild-bot would send a Smatch warning
> for inconsistent indenting, but comments are not counted there either.
> 
> I'm sort of surprised that we don't have checkpatch rule about the
> missing space characters.  It should be: "/* Tearing Effect Line On */".

Maybe this but the "preceded by a tab" test is pretty noisy.

---
diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 4f8494527139..72347e82d384 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -3720,6 +3720,22 @@ sub process {
 				    s/(\(\s*$Type\s*\))[ \t]+/$1/;
 			}
 		}
+
+# Comment styles
+# Initial comment only lines that have a leading space
+		if ($rawline =~ m{^\+([ \t]+)(?:/\*|//)} && $1 =~ / /) {
+			WARN("COMMENT_STYLE",
+			     "Initial comment lines should be indented only with tabs\n" . $herecurr);
+# comments not aligned on tabs
+		} elsif ($rawline !~ m{^\+(?:/\*|//)} &&
+			 $rawline =~ m{^\+.*[^\t](?:/\*|//)}) {
+			CHK("COMMENT_STYLE",
+			    "Comments should generally be preceded by a tab\n" . $herecurr);
+		}
+
+# comment initiators should generally be followed by a space if using words
+		if ($rawline =~ m{^\+.*(?:/\*|//)\w}) {
+			WARN("COMMENT_STYLE",
+			     "Comment text should use a space after the comment initiator\n" . $herecurr);
+		}
 
 # Block comment styles
 # Networking with an initial /*



^ permalink raw reply related	[flat|nested] 21+ messages in thread

* Re: [PATCH v10] staging: fbtft: add tearing signal detect
  2021-01-27 13:42 [PATCH v10] staging: fbtft: add tearing signal detect Carlis
  2021-01-27 13:51 ` Greg KH
@ 2021-01-27 22:32 ` Kari Argillander
  2021-01-28  1:42   ` carlis
  2021-01-28 15:13   ` Dan Carpenter
  1 sibling, 2 replies; 21+ messages in thread
From: Kari Argillander @ 2021-01-27 22:32 UTC (permalink / raw)
  To: Carlis
  Cc: gregkh, colin.king, oliver.graute, zhangxuezhi1, mh12gx2825,
	sbrivio, dri-devel, linux-fbdev, devel, linux-kernel

On Wed, Jan 27, 2021 at 09:42:52PM +0800, Carlis wrote:
> For st7789v ic,when we need continuous full screen refresh, it is best to
> wait for the TE signal arrive to avoid screen tearing
 
> diff --git a/drivers/staging/fbtft/fb_st7789v.c b/drivers/staging/fbtft/fb_st7789v.c
> index 3a280cc..cba08a8 100644
> --- a/drivers/staging/fbtft/fb_st7789v.c
> +++ b/drivers/staging/fbtft/fb_st7789v.c
> @@ -9,9 +9,12 @@
>  #include <linux/delay.h>
>  #include <linux/init.h>
>  #include <linux/kernel.h>
> +#include <linux/mutex.h>
> +#include <linux/interrupt.h>
> +#include <linux/completion.h>
>  #include <linux/module.h>
>  #include <video/mipi_display.h>
> -
> +#include <linux/gpio/consumer.h>

Space after local headers. Also this should one up so all Linux headers
are group together. You agreed?

>  #include "fbtft.h"
>  
>  #define DRVNAME "fb_st7789v"
> @@ -66,6 +69,32 @@ 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 mutex te_mutex;/* mutex for set te gpio irq status */

Space after ;

> +static struct completion spi_panel_te;

What if multiple displays? Is this possible for user?

> +
> +static irqreturn_t spi_panel_te_handler(int irq, void *data)
> +{
> +	complete(&spi_panel_te);
> +	return IRQ_HANDLED;
> +}
> +
> +static void set_spi_panel_te_irq_status(struct fbtft_par *par, bool enable)
> +{
> +	static int te_irq_count;

Same here. Maybe you can think better way and then this code would also be
cleaner.

> +
> +	mutex_lock(&te_mutex);

So locking should be done if we really do action and not just in case.

> +
> +	if (enable) {
> +		if (++te_irq_count == 1)
> +			enable_irq(gpiod_to_irq(par->gpio.te));
> +	} else {
> +		if (--te_irq_count == 0)
> +			disable_irq(gpiod_to_irq(par->gpio.te));
> +	}
> +	mutex_unlock(&te_mutex);
> +}
> +
>  /**
>   * init_display() - initialize the display controller
>   *
> @@ -82,6 +111,33 @@ 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)) {
> +		rc = PTR_ERR(par->gpio.te);
> +		dev_err(par->info->device, "Failed to request te gpio: %d\n", rc);
> +		return rc;
> +	}

You request with optinal and you still want to error out? We could just
continue and not care about that error. User will be happier if device
still works somehow.

> +	if (par->gpio.te) {
> +		init_completion(&spi_panel_te);
> +		mutex_init(&te_mutex);
> +		rc = devm_request_irq(dev,
> +				      gpiod_to_irq(par->gpio.te),
> +				     spi_panel_te_handler, IRQF_TRIGGER_RISING,
> +				     "TE_GPIO", par);
> +		if (rc) {
> +			dev_err(par->info->device, "TE request_irq failed.\n");
> +			devm_gpiod_put(dev, par->gpio.te);
> +			return rc;
> +		}
> +
> +		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 +193,9 @@ static int init_display(struct fbtft_par *par)
>  	 */
>  	write_reg(par, PWCTRL1, 0xA4, 0xA1);
>  
> +    /*Tearing Effect Line On*/

Spaces and why upcase everything?

> +	if (par->gpio.te)
> +		write_reg(par, 0x35, 0x00);
>  	write_reg(par, MIPI_DCS_SET_DISPLAY_ON);
>  
>  	if (HSD20_IPS)
> @@ -145,6 +204,76 @@ static int init_display(struct fbtft_par *par)
>  	return 0;
>  }
>  
> +/*****************************************************************************
> + *
> + *   int (*write_vmem)(struct fbtft_par *par);
> + *
> + *****************************************************************************/
> +

Why this kind of function comment? Please use same as another function
comments in this file. They are atleast almoust like kernel-doc style.

> +/* 16 bit pixel over 8-bit databus */
> +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);
> +
> +	/* 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) {

for (remain = len / 2; remain; remain -= to_copy) {

or even use len = len / 2 if you wanna save variable.

> +		to_copy = min(tx_array_size, remain);

Care must be taken that this will not be endless loop if another is 0. I
will not check this further but hopefully you have.

> +		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;

+= Or you can ++ vmem16 at the for loop but that is not so readable
sometimes with pointers.

> +		if (par->gpio.te) {
> +			set_spi_panel_te_irq_status(par, true);
> +			reinit_completion(&spi_panel_te);
> +			ret = wait_for_completion_timeout(&spi_panel_te,
> +							  msecs_to_jiffies(SPI_PANEL_TE_TIMEOUT));
> +			if (ret == 0)

!ret

> +				dev_err(par->info->device, "wait panel TE time out\n");
> +		}
> +		ret = par->fbtftops.write(par, par->txbuf.buf,
> +					 startbyte_size + to_copy * 2);
> +		if (par->gpio.te)
> +			set_spi_panel_te_irq_status(par, false);
> +		if (ret < 0)
> +			return ret;
> +		remain -= to_copy;
> +	}
> +
> +	return ret;

Do we want to return something over 0? If not then this can be return 0.
And then you do not need to even init ret value at the beginning.

Also wait little bit like Greg sayd before sending new version. Someone
might nack about what I say or say something more.

> +}
> +
>  /**
>   * set_var() - apply LCD properties like rotation and BGR mode
>   *
> @@ -259,6 +388,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
> 

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH v10] staging: fbtft: add tearing signal detect
  2021-01-27 15:02             ` Greg KH
@ 2021-01-28  1:12               ` carlis
  0 siblings, 0 replies; 21+ messages in thread
From: carlis @ 2021-01-28  1:12 UTC (permalink / raw)
  To: Greg KH
  Cc: Dan Carpenter, devel, linux-fbdev, mh12gx2825, sbrivio,
	oliver.graute, linux-kernel, dri-devel, Joe Perches,
	Andy Whitcroft, colin.king, zhangxuezhi1

On Wed, 27 Jan 2021 16:02:35 +0100
Greg KH <gregkh@linuxfoundation.org> wrote:

> On Wed, Jan 27, 2021 at 05:49:46PM +0300, Dan Carpenter wrote:
> > On Wed, Jan 27, 2021 at 03:25:20PM +0100, Greg KH wrote:  
> > > On Wed, Jan 27, 2021 at 10:17:08PM +0800, carlis wrote:  
> > > > On Wed, 27 Jan 2021 15:13:05 +0100
> > > > Greg KH <gregkh@linuxfoundation.org> wrote:
> > > >   
> > > > > On Wed, Jan 27, 2021 at 10:08:09PM +0800, carlis wrote:  
> > > > > > On Wed, 27 Jan 2021 14:51:55 +0100
> > > > > > Greg KH <gregkh@linuxfoundation.org> wrote:
> > > > > >     
> > > > > > > On Wed, Jan 27, 2021 at 09:42:52PM +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>
> > > > > > > >   
> > > > > > > 
> > > > > > > Please slow down and wait at least a day between patch
> > > > > > > submissions, there is no rush here.
> > > > > > > 
> > > > > > > And also, ALWAYS run scripts/checkpatch.pl on your
> > > > > > > submissions, so that you don't have a maintainer asking
> > > > > > > you about basic problems, like are in this current patch
> > > > > > > :(
> > > > > > > 
> > > > > > > thanks,
> > > > > > > 
> > > > > > > greg k-h    
> > > > > > 
> > > > > > hi,
> > > > > >   This is my first patch contribution to Linux, so some of
> > > > > > the rules are not very clear .In addition, I can confirm
> > > > > > that before sending patch, I check it with checkPatch.py
> > > > > > every time.Thank you very much for your help    
> > > > > 
> > > > > Please read Documentation/SubmittingPatches which has a link
> > > > > to the checklist and other documentation you should read.
> > > > > 
> > > > > And I doubt you are running checkpatch on your submission, as
> > > > > there is obvious coding style issues in it.  If so, please
> > > > > provide the output as it must be broken :(
> > > > > 
> > > > > thanks,
> > > > > 
> > > > > greg k-h  
> > > > hi, the patch v11 checkpatch.pl output is below:
> > > > 
> > > > carlis@bf-rmsz-10:~/work/linux-kernel/linux$
> > > > ./scripts/checkpatch.pl
> > > > 0001-staging-fbtft-add-tearing-signal-detect.patch total: 0
> > > > errors, 0 warnings, 0 checks, 176 lines checked
> > > > 
> > > > 0001-staging-fbtft-add-tearing-signal-detect.patch has no
> > > > obvious style problems and is ready for submission.  
> > > 
> > > Wow, my apologies!
> > > 
> > > Andy and Joe, there's something wrong here that is missing the
> > > fact that a line is being indented with spaces and not tabs in
> > > the patch at
> > > https://lore.kernel.org/r/1611754972-151016-1-git-send-email-zhangxuezhi3@gmail.com
> > > 
> > > Any ideas what broke?
> > >   
> > 
> >     /*Tearing Effect Line On*/
> > 
> > Comments are the exception to the "no spaces at the start of a line"
> > rule.  I was expecting that the kbuild-bot would send a Smatch
> > warning for inconsistent indenting, but comments are not counted
> > there either.
> > 
> > I'm sort of surprised that we don't have checkpatch rule about the
> > missing space characters.  It should be: "/* Tearing Effect Line On
> > */".  
> 
> That was going to be my next question, lots of comments added in this
> patch don't have spaces...
> 
> thanks,
> 
> greg k-h

Ok,i will fix it in patch v12 tomorrow

regards,
zhangxuezhi

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH v10] staging: fbtft: add tearing signal detect
  2021-01-27 22:32 ` Kari Argillander
@ 2021-01-28  1:42   ` carlis
  2021-01-28  6:52     ` Kari Argillander
  2021-01-28 15:13   ` Dan Carpenter
  1 sibling, 1 reply; 21+ messages in thread
From: carlis @ 2021-01-28  1:42 UTC (permalink / raw)
  To: Kari Argillander
  Cc: gregkh, colin.king, oliver.graute, zhangxuezhi1, mh12gx2825,
	sbrivio, dri-devel, linux-fbdev, devel, linux-kernel

On Thu, 28 Jan 2021 00:32:22 +0200
Kari Argillander <kari.argillander@gmail.com> wrote:

> On Wed, Jan 27, 2021 at 09:42:52PM +0800, Carlis wrote:
> > For st7789v ic,when we need continuous full screen refresh, it is
> > best to wait for the TE signal arrive to avoid screen tearing  
>  
> > diff --git a/drivers/staging/fbtft/fb_st7789v.c
> > b/drivers/staging/fbtft/fb_st7789v.c index 3a280cc..cba08a8 100644
> > --- a/drivers/staging/fbtft/fb_st7789v.c
> > +++ b/drivers/staging/fbtft/fb_st7789v.c
> > @@ -9,9 +9,12 @@
> >  #include <linux/delay.h>
> >  #include <linux/init.h>
> >  #include <linux/kernel.h>
> > +#include <linux/mutex.h>
> > +#include <linux/interrupt.h>
> > +#include <linux/completion.h>
> >  #include <linux/module.h>
> >  #include <video/mipi_display.h>
> > -
> > +#include <linux/gpio/consumer.h>  
> 
> Space after local headers. Also this should one up so all Linux
> headers are group together. You agreed?
> 
OK,i will fix it in patch v12 tomorrow

> >  #include "fbtft.h"
> >  
> >  #define DRVNAME "fb_st7789v"
> > @@ -66,6 +69,32 @@ 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 mutex te_mutex;/* mutex for set te gpio irq status
> > */  
> 
> Space after ;
hi, i have fix it in the patch v11
> 
> > +static struct completion spi_panel_te;  
> 
> What if multiple displays? Is this possible for user?
I will check it carefully again about this logic.
> 
> > +
> > +static irqreturn_t spi_panel_te_handler(int irq, void *data)
> > +{
> > +	complete(&spi_panel_te);
> > +	return IRQ_HANDLED;
> > +}
> > +
> > +static void set_spi_panel_te_irq_status(struct fbtft_par *par,
> > bool enable) +{
> > +	static int te_irq_count;  
> 
> Same here. Maybe you can think better way and then this code would
> also be cleaner.
> 
> > +
> > +	mutex_lock(&te_mutex);  
> 
> So locking should be done if we really do action and not just in case.
> 
> > +
> > +	if (enable) {
> > +		if (++te_irq_count == 1)
> > +			enable_irq(gpiod_to_irq(par->gpio.te));
> > +	} else {
> > +		if (--te_irq_count == 0)
> > +			disable_irq(gpiod_to_irq(par->gpio.te));
> > +	}
> > +	mutex_unlock(&te_mutex);
> > +}
> > +
> >  /**
> >   * init_display() - initialize the display controller
> >   *
> > @@ -82,6 +111,33 @@ 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)) {
> > +		rc = PTR_ERR(par->gpio.te);
> > +		dev_err(par->info->device, "Failed to request te
> > gpio: %d\n", rc);
> > +		return rc;
> > +	}  
> 
> You request with optinal and you still want to error out? We could
> just continue and not care about that error. User will be happier if
> device still works somehow.
You mean i just delete this dev_err print ?!
like this:
	par->gpio.te = devm_gpiod_get_index_optional(dev, "te",
0,GPIOD_IN); 
        if (IS_ERR(par->gpio.te))
		return PTR_ERR(par->gpio.te);
> 
> > +	if (par->gpio.te) {
> > +		init_completion(&spi_panel_te);
> > +		mutex_init(&te_mutex);
> > +		rc = devm_request_irq(dev,
> > +				      gpiod_to_irq(par->gpio.te),
> > +				     spi_panel_te_handler,
> > IRQF_TRIGGER_RISING,
> > +				     "TE_GPIO", par);
> > +		if (rc) {
> > +			dev_err(par->info->device, "TE request_irq
> > failed.\n");
> > +			devm_gpiod_put(dev, par->gpio.te);
> > +			return rc;
> > +		}
> > +
> > +		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 +193,9 @@ static int init_display(struct fbtft_par *par)
> >  	 */
> >  	write_reg(par, PWCTRL1, 0xA4, 0xA1);
> >  
> > +    /*Tearing Effect Line On*/  
> 
> Spaces and why upcase everything?
i will fix it in patch v12 tomorrow
> 
> > +	if (par->gpio.te)
> > +		write_reg(par, 0x35, 0x00);
> >  	write_reg(par, MIPI_DCS_SET_DISPLAY_ON);
> >  
> >  	if (HSD20_IPS)
> > @@ -145,6 +204,76 @@ static int init_display(struct fbtft_par *par)
> >  	return 0;
> >  }
> >  
> > +/*****************************************************************************
> > + *
> > + *   int (*write_vmem)(struct fbtft_par *par);
> > + *
> > +
> > *****************************************************************************/
> > +  
> 
> Why this kind of function comment? Please use same as another function
> comments in this file. They are atleast almoust like kernel-doc style.
i will fix it in patch v12 tomorrow
> > +/* 16 bit pixel over 8-bit databus */
> > +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);
> > +
> > +	/* 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) {  
> 
> for (remain = len / 2; remain; remain -= to_copy) {
> 
> or even use len = len / 2 if you wanna save variable.
> 
> > +		to_copy = min(tx_array_size, remain);  
> 
> Care must be taken that this will not be endless loop if another is
> 0. I will not check this further but hopefully you have.
> 
> > +		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;  
> 
> += Or you can ++ vmem16 at the for loop but that is not so readable
> sometimes with pointers.
> 
> > +		if (par->gpio.te) {
> > +			set_spi_panel_te_irq_status(par, true);
> > +			reinit_completion(&spi_panel_te);
> > +			ret =
> > wait_for_completion_timeout(&spi_panel_te,
> > +
> > msecs_to_jiffies(SPI_PANEL_TE_TIMEOUT));
> > +			if (ret == 0)  
> 
> !ret
> 
> > +				dev_err(par->info->device, "wait
> > panel TE time out\n");
> > +		}
> > +		ret = par->fbtftops.write(par, par->txbuf.buf,
> > +					 startbyte_size + to_copy
> > * 2);
> > +		if (par->gpio.te)
> > +			set_spi_panel_te_irq_status(par, false);
> > +		if (ret < 0)
> > +			return ret;
> > +		remain -= to_copy;
> > +	}
> > +
> > +	return ret;  
> 
> Do we want to return something over 0? If not then this can be return
> 0. And then you do not need to even init ret value at the beginning.
> 
> Also wait little bit like Greg sayd before sending new version.
> Someone might nack about what I say or say something more.
> 
hi, i copy fbtft_write_vmem16_bus8 from file fbtft_bus.c and modify it
,just add te wait logic, i will take more time to check this original
function.
> > +}
> > +
> >  /**
> >   * set_var() - apply LCD properties like rotation and BGR mode
> >   *
> > @@ -259,6 +388,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
> >   


^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH v10] staging: fbtft: add tearing signal detect
  2021-01-28  1:42   ` carlis
@ 2021-01-28  6:52     ` Kari Argillander
  2021-01-28  9:24       ` carlis
  2021-01-28  9:42       ` Geert Uytterhoeven
  0 siblings, 2 replies; 21+ messages in thread
From: Kari Argillander @ 2021-01-28  6:52 UTC (permalink / raw)
  To: carlis
  Cc: gregkh, colin.king, oliver.graute, zhangxuezhi1, mh12gx2825,
	sbrivio, dri-devel, linux-fbdev, devel, linux-kernel

On Thu, Jan 28, 2021 at 09:42:58AM +0800, carlis wrote:
> On Thu, 28 Jan 2021 00:32:22 +0200
> Kari Argillander <kari.argillander@gmail.com> wrote:
> > >  #include "fbtft.h"
> > >  
> > >  #define DRVNAME "fb_st7789v"
> > > @@ -66,6 +69,32 @@ 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 mutex te_mutex;/* mutex for set te gpio irq status
> > > */  
> > 
> > Space after ;
> hi, i have fix it in the patch v11
> > 

Yeah sorry. I accidentally review wrong patch. But mostly stuff are
still relevant.

> > > @@ -82,6 +111,33 @@ 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)) {
> > > +		rc = PTR_ERR(par->gpio.te);
> > > +		dev_err(par->info->device, "Failed to request te
> > > gpio: %d\n", rc);
> > > +		return rc;
> > > +	}  
> > 
> > You request with optinal and you still want to error out? We could
> > just continue and not care about that error. User will be happier if
> > device still works somehow.
> You mean i just delete this dev_err print ?!
> like this:
> 	par->gpio.te = devm_gpiod_get_index_optional(dev, "te",
> 0,GPIOD_IN); 
>         if (IS_ERR(par->gpio.te))
> 		return PTR_ERR(par->gpio.te);

Not exactly. I'm suggesting something like this.

if (IS_ERR(par->gpio.te) == -EPROBE_DEFER) {
	return -EPROBE_DEFER;

if (IS_ERR(par->gpio.te))
	par-gpio.te = NULL;

This like beginning of your patch series but the difference is that if
EPROBE_DEFER then we will try again later. Any other error and we will
just ignore TE gpio. But this is up to you what you want to do. To me
this just seems place where this kind of logic can work.

> > > +		if (par->gpio.te) {
> > > +			set_spi_panel_te_irq_status(par, true);
> > > +			reinit_completion(&spi_panel_te);
> > > +			ret =
> > > wait_for_completion_timeout(&spi_panel_te,
> > > +
> > > msecs_to_jiffies(SPI_PANEL_TE_TIMEOUT));
> > > +			if (ret == 0)  
> > 
> > !ret
> > 
> > > +				dev_err(par->info->device, "wait
> > > panel TE time out\n");
> > > +		}
> > > +		ret = par->fbtftops.write(par, par->txbuf.buf,
> > > +					 startbyte_size + to_copy
> > > * 2);
> > > +		if (par->gpio.te)
> > > +			set_spi_panel_te_irq_status(par, false);
> > > +		if (ret < 0)
> > > +			return ret;
> > > +		remain -= to_copy;
> > > +	}
> > > +
> > > +	return ret;  
> > 
> > Do we want to return something over 0? If not then this can be return
> > 0. And then you do not need to even init ret value at the beginning.
> > 
> > Also wait little bit like Greg sayd before sending new version.
> > Someone might nack about what I say or say something more.
> > 
> hi, i copy fbtft_write_vmem16_bus8 from file fbtft_bus.c and modify it
> ,just add te wait logic, i will take more time to check this original
> function.

It might be ok or not. You should still check.

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH v10] staging: fbtft: add tearing signal detect
  2021-01-28  6:52     ` Kari Argillander
@ 2021-01-28  9:24       ` carlis
  2021-01-28  9:42       ` Geert Uytterhoeven
  1 sibling, 0 replies; 21+ messages in thread
From: carlis @ 2021-01-28  9:24 UTC (permalink / raw)
  To: Kari Argillander
  Cc: gregkh, colin.king, oliver.graute, zhangxuezhi1, mh12gx2825,
	sbrivio, dri-devel, linux-fbdev, devel, linux-kernel

On Thu, 28 Jan 2021 08:52:33 +0200
Kari Argillander <kari.argillander@gmail.com> wrote:

> On Thu, Jan 28, 2021 at 09:42:58AM +0800, carlis wrote:
> > On Thu, 28 Jan 2021 00:32:22 +0200
> > Kari Argillander <kari.argillander@gmail.com> wrote:  
> > > >  #include "fbtft.h"
> > > >  
> > > >  #define DRVNAME "fb_st7789v"
> > > > @@ -66,6 +69,32 @@ 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 mutex te_mutex;/* mutex for set te gpio irq
> > > > status */    
> > > 
> > > Space after ;  
> > hi, i have fix it in the patch v11  
> > >   
> 
> Yeah sorry. I accidentally review wrong patch. But mostly stuff are
> still relevant.
> 
> > > > @@ -82,6 +111,33 @@ 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)) {
> > > > +		rc = PTR_ERR(par->gpio.te);
> > > > +		dev_err(par->info->device, "Failed to request
> > > > te gpio: %d\n", rc);
> > > > +		return rc;
> > > > +	}    
> > > 
> > > You request with optinal and you still want to error out? We could
> > > just continue and not care about that error. User will be happier
> > > if device still works somehow.  
> > You mean i just delete this dev_err print ?!
> > like this:
> > 	par->gpio.te = devm_gpiod_get_index_optional(dev, "te",
> > 0,GPIOD_IN); 
> >         if (IS_ERR(par->gpio.te))
> > 		return PTR_ERR(par->gpio.te);  
> 
> Not exactly. I'm suggesting something like this.
> 
> if (IS_ERR(par->gpio.te) == -EPROBE_DEFER) {
> 	return -EPROBE_DEFER;
> 
> if (IS_ERR(par->gpio.te))
> 	par-gpio.te = NULL;
> 
> This like beginning of your patch series but the difference is that if
> EPROBE_DEFER then we will try again later. Any other error and we will
> just ignore TE gpio. But this is up to you what you want to do. To me
> this just seems place where this kind of logic can work.
> 
> > > > +		if (par->gpio.te) {
> > > > +			set_spi_panel_te_irq_status(par, true);
> > > > +			reinit_completion(&spi_panel_te);
> > > > +			ret =
> > > > wait_for_completion_timeout(&spi_panel_te,
> > > > +
> > > > msecs_to_jiffies(SPI_PANEL_TE_TIMEOUT));
> > > > +			if (ret == 0)    
> > > 
> > > !ret
> > >   
> > > > +				dev_err(par->info->device,
> > > > "wait panel TE time out\n");
> > > > +		}
> > > > +		ret = par->fbtftops.write(par, par->txbuf.buf,
> > > > +					 startbyte_size +
> > > > to_copy
> > > > * 2);
> > > > +		if (par->gpio.te)
> > > > +			set_spi_panel_te_irq_status(par,
> > > > false);
> > > > +		if (ret < 0)
> > > > +			return ret;
> > > > +		remain -= to_copy;
> > > > +	}
> > > > +
> > > > +	return ret;    
> > > 
> > > Do we want to return something over 0? If not then this can be
> > > return 0. And then you do not need to even init ret value at the
> > > beginning.
> > > 
> > > Also wait little bit like Greg sayd before sending new version.
> > > Someone might nack about what I say or say something more.
> > >   
> > hi, i copy fbtft_write_vmem16_bus8 from file fbtft_bus.c and modify
> > it ,just add te wait logic, i will take more time to check this
> > original function.  
> 
> It might be ok or not. You should still check.

hi, i will check more carefully, now i have a new problem, Is there a
way to clear the interrupt pending state before opening it again?




^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH v10] staging: fbtft: add tearing signal detect
  2021-01-28  6:52     ` Kari Argillander
  2021-01-28  9:24       ` carlis
@ 2021-01-28  9:42       ` Geert Uytterhoeven
  2021-01-28  9:59         ` Kari Argillander
  2021-01-28 11:03         ` carlis
  1 sibling, 2 replies; 21+ messages in thread
From: Geert Uytterhoeven @ 2021-01-28  9:42 UTC (permalink / raw)
  To: Kari Argillander
  Cc: carlis, Greg KH, Colin King, oliver.graute, zhangxuezhi1,
	mh12gx2825, Stefano Brivio, DRI Development,
	Linux Fbdev development list, driverdevel,
	Linux Kernel Mailing List

Hi Kari,

On Thu, Jan 28, 2021 at 7:53 AM Kari Argillander
<kari.argillander@gmail.com> wrote:
> On Thu, Jan 28, 2021 at 09:42:58AM +0800, carlis wrote:
> > On Thu, 28 Jan 2021 00:32:22 +0200
> > Kari Argillander <kari.argillander@gmail.com> wrote:
> > > >  #include "fbtft.h"
> > > >
> > > >  #define DRVNAME "fb_st7789v"
> > > > @@ -66,6 +69,32 @@ 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 mutex te_mutex;/* mutex for set te gpio irq status
> > > > */
> > >
> > > Space after ;
> > hi, i have fix it in the patch v11
> > >
>
> Yeah sorry. I accidentally review wrong patch. But mostly stuff are
> still relevant.
>
> > > > @@ -82,6 +111,33 @@ 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)) {
> > > > +         rc = PTR_ERR(par->gpio.te);
> > > > +         dev_err(par->info->device, "Failed to request te
> > > > gpio: %d\n", rc);
> > > > +         return rc;
> > > > + }
> > >
> > > You request with optinal and you still want to error out? We could
> > > just continue and not care about that error. User will be happier if
> > > device still works somehow.

devm_gpiod_get_index_optional() returns NULL, not an error, if the
GPIO is not found.  So if IS_ERR() is the right check.

And checks for -EPROBE_DEFER can be handled automatically
by using dev_err_probe() instead of dev_err().

> > You mean i just delete this dev_err print ?!
> > like this:
> >       par->gpio.te = devm_gpiod_get_index_optional(dev, "te",
> > 0,GPIOD_IN);
> >         if (IS_ERR(par->gpio.te))
> >               return PTR_ERR(par->gpio.te);
>
> Not exactly. I'm suggesting something like this.
>
> if (IS_ERR(par->gpio.te) == -EPROBE_DEFER) {
>         return -EPROBE_DEFER;
>
> if (IS_ERR(par->gpio.te))
>         par-gpio.te = NULL;
>
> This like beginning of your patch series but the difference is that if
> EPROBE_DEFER then we will try again later. Any other error and we will
> just ignore TE gpio. But this is up to you what you want to do. To me
> this just seems place where this kind of logic can work.

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH v10] staging: fbtft: add tearing signal detect
  2021-01-28  9:42       ` Geert Uytterhoeven
@ 2021-01-28  9:59         ` Kari Argillander
  2021-01-28 11:03         ` carlis
  1 sibling, 0 replies; 21+ messages in thread
From: Kari Argillander @ 2021-01-28  9:59 UTC (permalink / raw)
  To: Geert Uytterhoeven
  Cc: carlis, Greg KH, Colin King, oliver.graute, zhangxuezhi1,
	mh12gx2825, Stefano Brivio, DRI Development,
	Linux Fbdev development list, driverdevel,
	Linux Kernel Mailing List

On Thu, Jan 28, 2021 at 10:42:54AM +0100, Geert Uytterhoeven wrote:
> Hi Kari,
> 
> On Thu, Jan 28, 2021 at 7:53 AM Kari Argillander
> <kari.argillander@gmail.com> wrote:
> > On Thu, Jan 28, 2021 at 09:42:58AM +0800, carlis wrote:
> > > On Thu, 28 Jan 2021 00:32:22 +0200
> > > Kari Argillander <kari.argillander@gmail.com> wrote:
> > > > > @@ -82,6 +111,33 @@ 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)) {
> > > > > +         rc = PTR_ERR(par->gpio.te);
> > > > > +         dev_err(par->info->device, "Failed to request te
> > > > > gpio: %d\n", rc);
> > > > > +         return rc;
> > > > > + }
> > > >
> > > > You request with optinal and you still want to error out? We could
> > > > just continue and not care about that error. User will be happier if
> > > > device still works somehow.
> 
> devm_gpiod_get_index_optional() returns NULL, not an error, if the
> GPIO is not found.  So if IS_ERR() is the right check.
> 
> And checks for -EPROBE_DEFER can be handled automatically
> by using dev_err_probe() instead of dev_err().

Yeah. Thanks for pointing that clearly.

> > > You mean i just delete this dev_err print ?!
> > > like this:
> > >       par->gpio.te = devm_gpiod_get_index_optional(dev, "te",
> > > 0,GPIOD_IN);
> > >         if (IS_ERR(par->gpio.te))
> > >               return PTR_ERR(par->gpio.te);
> >
> > Not exactly. I'm suggesting something like this.
> >
> > if (IS_ERR(par->gpio.te) == -EPROBE_DEFER) {
> >         return -EPROBE_DEFER;
> >
> > if (IS_ERR(par->gpio.te))
> >         par-gpio.te = NULL;
> >
> > This like beginning of your patch series but the difference is that if
> > EPROBE_DEFER then we will try again later. Any other error and we will
> > just ignore TE gpio. But this is up to you what you want to do. To me
> > this just seems place where this kind of logic can work.


^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH v10] staging: fbtft: add tearing signal detect
  2021-01-28  9:42       ` Geert Uytterhoeven
  2021-01-28  9:59         ` Kari Argillander
@ 2021-01-28 11:03         ` carlis
  2021-01-28 11:15           ` Geert Uytterhoeven
  1 sibling, 1 reply; 21+ messages in thread
From: carlis @ 2021-01-28 11:03 UTC (permalink / raw)
  To: Geert Uytterhoeven
  Cc: Kari Argillander, Greg KH, Colin King, oliver.graute,
	zhangxuezhi1, mh12gx2825, Stefano Brivio, DRI Development,
	Linux Fbdev development list, driverdevel,
	Linux Kernel Mailing List

On Thu, 28 Jan 2021 10:42:54 +0100
Geert Uytterhoeven <geert@linux-m68k.org> wrote:

> Hi Kari,
> 
> On Thu, Jan 28, 2021 at 7:53 AM Kari Argillander
> <kari.argillander@gmail.com> wrote:
> > On Thu, Jan 28, 2021 at 09:42:58AM +0800, carlis wrote:  
> > > On Thu, 28 Jan 2021 00:32:22 +0200
> > > Kari Argillander <kari.argillander@gmail.com> wrote:  
> > > > >  #include "fbtft.h"
> > > > >
> > > > >  #define DRVNAME "fb_st7789v"
> > > > > @@ -66,6 +69,32 @@ 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 mutex te_mutex;/* mutex for set te gpio irq
> > > > > status */  
> > > >
> > > > Space after ;  
> > > hi, i have fix it in the patch v11  
> > > >  
> >
> > Yeah sorry. I accidentally review wrong patch. But mostly stuff are
> > still relevant.
> >  
> > > > > @@ -82,6 +111,33 @@ 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)) {
> > > > > +         rc = PTR_ERR(par->gpio.te);
> > > > > +         dev_err(par->info->device, "Failed to request te
> > > > > gpio: %d\n", rc);
> > > > > +         return rc;
> > > > > + }  
> > > >
> > > > You request with optinal and you still want to error out? We
> > > > could just continue and not care about that error. User will be
> > > > happier if device still works somehow.  
> 
> devm_gpiod_get_index_optional() returns NULL, not an error, if the
> GPIO is not found.  So if IS_ERR() is the right check.
> 
> And checks for -EPROBE_DEFER can be handled automatically
> by using dev_err_probe() instead of dev_err().
> 
hi, i fix it like below!?
	par->gpio.te = devm_gpiod_get_index_optional(dev, "te", 0,
	GPIOD_IN); if (IS_ERR(par->gpio.te)) {
		rc = PTR_ERR(par->gpio.te);
		dev_err_probe(par->info->device, rc, "Failed to request
	te gpio\n"); return rc;
	}
	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 (rc) {
			dev_err(par->info->device, "TE request_irq
	failed.\n"); return rc;
		}

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


> > > You mean i just delete this dev_err print ?!
> > > like this:
> > >       par->gpio.te = devm_gpiod_get_index_optional(dev, "te",
> > > 0,GPIOD_IN);
> > >         if (IS_ERR(par->gpio.te))
> > >               return PTR_ERR(par->gpio.te);  
> >
> > Not exactly. I'm suggesting something like this.
> >
> > if (IS_ERR(par->gpio.te) == -EPROBE_DEFER) {
> >         return -EPROBE_DEFER;
> >
> > if (IS_ERR(par->gpio.te))
> >         par-gpio.te = NULL;
> >
> > This like beginning of your patch series but the difference is that
> > if EPROBE_DEFER then we will try again later. Any other error and
> > we will just ignore TE gpio. But this is up to you what you want to
> > do. To me this just seems place where this kind of logic can work.  
> 
> Gr{oetje,eeting}s,
> 
>                         Geert
> 

regards,
zhangxuezhi

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH v10] staging: fbtft: add tearing signal detect
  2021-01-28 11:03         ` carlis
@ 2021-01-28 11:15           ` Geert Uytterhoeven
  2021-01-28 11:42             ` carlis
  0 siblings, 1 reply; 21+ messages in thread
From: Geert Uytterhoeven @ 2021-01-28 11:15 UTC (permalink / raw)
  To: carlis
  Cc: Kari Argillander, Greg KH, Colin King, oliver.graute,
	zhangxuezhi1, mh12gx2825, Stefano Brivio, DRI Development,
	Linux Fbdev development list, driverdevel,
	Linux Kernel Mailing List

Hi Carlis,

On Thu, Jan 28, 2021 at 12:03 PM carlis <zhangxuezhi3@gmail.com> wrote:
> On Thu, 28 Jan 2021 10:42:54 +0100
> Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> > On Thu, Jan 28, 2021 at 7:53 AM Kari Argillander
> > <kari.argillander@gmail.com> wrote:
> > > On Thu, Jan 28, 2021 at 09:42:58AM +0800, carlis wrote:
> > > > On Thu, 28 Jan 2021 00:32:22 +0200
> > > > Kari Argillander <kari.argillander@gmail.com> wrote:
> > > > > >  #include "fbtft.h"
> > > > > >
> > > > > >  #define DRVNAME "fb_st7789v"
> > > > > > @@ -66,6 +69,32 @@ 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 mutex te_mutex;/* mutex for set te gpio irq
> > > > > > status */
> > > > >
> > > > > Space after ;
> > > > hi, i have fix it in the patch v11
> > > > >
> > >
> > > Yeah sorry. I accidentally review wrong patch. But mostly stuff are
> > > still relevant.
> > >
> > > > > > @@ -82,6 +111,33 @@ 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)) {
> > > > > > +         rc = PTR_ERR(par->gpio.te);
> > > > > > +         dev_err(par->info->device, "Failed to request te
> > > > > > gpio: %d\n", rc);
> > > > > > +         return rc;
> > > > > > + }
> > > > >
> > > > > You request with optinal and you still want to error out? We
> > > > > could just continue and not care about that error. User will be
> > > > > happier if device still works somehow.
> >
> > devm_gpiod_get_index_optional() returns NULL, not an error, if the
> > GPIO is not found.  So if IS_ERR() is the right check.
> >
> > And checks for -EPROBE_DEFER can be handled automatically
> > by using dev_err_probe() instead of dev_err().
> >
> hi, i fix it like below!?
>         par->gpio.te = devm_gpiod_get_index_optional(dev, "te", 0,
>         GPIOD_IN); if (IS_ERR(par->gpio.te)) {
>                 rc = PTR_ERR(par->gpio.te);
>                 dev_err_probe(par->info->device, rc, "Failed to request
>         te gpio\n"); return rc;
>         }
>         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 (rc) {
>                         dev_err(par->info->device, "TE request_irq
>         failed.\n"); return rc;

dev_err_probe()

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

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH v10] staging: fbtft: add tearing signal detect
  2021-01-28 11:15           ` Geert Uytterhoeven
@ 2021-01-28 11:42             ` carlis
  0 siblings, 0 replies; 21+ messages in thread
From: carlis @ 2021-01-28 11:42 UTC (permalink / raw)
  To: Geert Uytterhoeven
  Cc: Kari Argillander, Greg KH, Colin King, oliver.graute,
	zhangxuezhi1, mh12gx2825, Stefano Brivio, DRI Development,
	Linux Fbdev development list, driverdevel,
	Linux Kernel Mailing List

On Thu, 28 Jan 2021 12:15:28 +0100
Geert Uytterhoeven <geert@linux-m68k.org> wrote:

> Hi Carlis,
> 
> On Thu, Jan 28, 2021 at 12:03 PM carlis <zhangxuezhi3@gmail.com>
> wrote:
> > On Thu, 28 Jan 2021 10:42:54 +0100
> > Geert Uytterhoeven <geert@linux-m68k.org> wrote:  
> > > On Thu, Jan 28, 2021 at 7:53 AM Kari Argillander
> > > <kari.argillander@gmail.com> wrote:  
> > > > On Thu, Jan 28, 2021 at 09:42:58AM +0800, carlis wrote:  
> > > > > On Thu, 28 Jan 2021 00:32:22 +0200
> > > > > Kari Argillander <kari.argillander@gmail.com> wrote:  
> > > > > > >  #include "fbtft.h"
> > > > > > >
> > > > > > >  #define DRVNAME "fb_st7789v"
> > > > > > > @@ -66,6 +69,32 @@ 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 mutex te_mutex;/* mutex for set te gpio irq
> > > > > > > status */  
> > > > > >
> > > > > > Space after ;  
> > > > > hi, i have fix it in the patch v11  
> > > > > >  
> > > >
> > > > Yeah sorry. I accidentally review wrong patch. But mostly stuff
> > > > are still relevant.
> > > >  
> > > > > > > @@ -82,6 +111,33 @@ 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)) {
> > > > > > > +         rc = PTR_ERR(par->gpio.te);
> > > > > > > +         dev_err(par->info->device, "Failed to request te
> > > > > > > gpio: %d\n", rc);
> > > > > > > +         return rc;
> > > > > > > + }  
> > > > > >
> > > > > > You request with optinal and you still want to error out? We
> > > > > > could just continue and not care about that error. User
> > > > > > will be happier if device still works somehow.  
> > >
> > > devm_gpiod_get_index_optional() returns NULL, not an error, if the
> > > GPIO is not found.  So if IS_ERR() is the right check.
> > >
> > > And checks for -EPROBE_DEFER can be handled automatically
> > > by using dev_err_probe() instead of dev_err().
> > >  
> > hi, i fix it like below!?
> >         par->gpio.te = devm_gpiod_get_index_optional(dev, "te", 0,
> >         GPIOD_IN); if (IS_ERR(par->gpio.te)) {
> >                 rc = PTR_ERR(par->gpio.te);
> >                 dev_err_probe(par->info->device, rc, "Failed to
> > request te gpio\n"); return rc;
> >         }
> >         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 (rc) {
> >                         dev_err(par->info->device, "TE request_irq
> >         failed.\n"); return rc;  
> 
> dev_err_probe()
> 
> >                 }
> >
> >                 disable_irq_nosync(gpiod_to_irq(par->gpio.te));
> >         } else {
> >                 dev_info(par->info->device, "%s:%d, TE gpio not
> >                 specified\n", __func__, __LINE__);
> >         }  
> 
> Gr{oetje,eeting}s,
> 
>                         Geert
> 

hi,i will fix it like below:


	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__);
	}


regards,
zhangxuezhi

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH v10] staging: fbtft: add tearing signal detect
  2021-01-27 22:32 ` Kari Argillander
  2021-01-28  1:42   ` carlis
@ 2021-01-28 15:13   ` Dan Carpenter
  1 sibling, 0 replies; 21+ messages in thread
From: Dan Carpenter @ 2021-01-28 15:13 UTC (permalink / raw)
  To: Kari Argillander
  Cc: Carlis, devel, linux-fbdev, mh12gx2825, gregkh, oliver.graute,
	linux-kernel, dri-devel, sbrivio, colin.king, zhangxuezhi1

On Thu, Jan 28, 2021 at 12:32:22AM +0200, Kari Argillander wrote:
> On Wed, Jan 27, 2021 at 09:42:52PM +0800, Carlis wrote:
> > @@ -82,6 +111,33 @@ 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)) {
> > +		rc = PTR_ERR(par->gpio.te);
> > +		dev_err(par->info->device, "Failed to request te gpio: %d\n", rc);
> > +		return rc;
> > +	}
> 
> You request with optinal and you still want to error out? We could just
> continue and not care about that error. User will be happier if device
> still works somehow.
> 

Carlis tried that approach in previous versions.  See the discussion
about -EPROBEi_DEFER.

That's not the right way to think about it anyway.  It's optional but
the user *chose* to enable it so if an error occurs then it's still an
error and should be treated like an error.  The user should fix the
error or disable the feature if they want to continue.

There are lots of places in the kernel where the error handling could
be written to try continue but in a crippled state.  It's not the right
approach.  Over engineering like that just leads to bugs.

regards,
dan carpenter


^ permalink raw reply	[flat|nested] 21+ messages in thread

end of thread, other threads:[~2021-01-28 15:16 UTC | newest]

Thread overview: 21+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-01-27 13:42 [PATCH v10] staging: fbtft: add tearing signal detect Carlis
2021-01-27 13:51 ` Greg KH
2021-01-27 14:08   ` carlis
2021-01-27 14:13     ` Greg KH
2021-01-27 14:17       ` carlis
2021-01-27 14:25         ` Greg KH
2021-01-27 14:49           ` Dan Carpenter
2021-01-27 15:02             ` Greg KH
2021-01-28  1:12               ` carlis
2021-01-27 17:42             ` Joe Perches
2021-01-27 18:21             ` Joe Perches
2021-01-27 22:32 ` Kari Argillander
2021-01-28  1:42   ` carlis
2021-01-28  6:52     ` Kari Argillander
2021-01-28  9:24       ` carlis
2021-01-28  9:42       ` Geert Uytterhoeven
2021-01-28  9:59         ` Kari Argillander
2021-01-28 11:03         ` carlis
2021-01-28 11:15           ` Geert Uytterhoeven
2021-01-28 11:42             ` carlis
2021-01-28 15:13   ` Dan Carpenter

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).