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

[Kari Argillander <kari.argillander@gmail.com>]

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.