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

On Thu, 28 Jan 2021 10:42:54 +0100
Geert Uytterhoeven [off-list ref] wrote:

Hi Kari,

On Thu, Jan 28, 2021 at 7:53 AM Kari Argillander
[off-list ref] wrote:

quoted

On Thu, Jan 28, 2021 at 09:42:58AM +0800, carlis wrote:  

quoted

On Thu, 28 Jan 2021 00:32:22 +0200
Kari Argillander [off-list ref] wrote:  

quoted

quoted

 #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  

quoted

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

quoted

quoted

quoted

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

quoted

quoted

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