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

On Thu, 28 Jan 2021 16:33:02 +0200
Andy Shevchenko [off-list ref] wrote:

On Thu, Jan 28, 2021 at 2:58 PM Carlis [off-list ref] wrote:

Thanks for your contribution, my comments below.

quoted

From: zhangxuezhi <redacted>  

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

hi,you mean like below:
	Carlis [off-list ref]
?

quoted

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

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

quoted

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.

...

quoted

 #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.

quoted

 #include <video/mipi_display.h>  

...

quoted

+#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.

...

quoted

+static struct completion spi_panel_te;  

As Greg said.

...

quoted

 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;

...

quoted

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

quoted

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

quoted

+       if (par->gpio.te) {  

Instead you should probably do the following:

int irq;

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

quoted

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

quoted

+                                     gpiod_to_irq(par->gpio.te),  

...and here simply use irq.

quoted

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

quoted

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

quoted

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

Use your temporary variable and move...

quoted

+                                            "TE request_irq
failed.\n");  

...this on the previous line.

quoted

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

Why do you call gpio_to_irq() twice?

quoted

+       } 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.

quoted

+       }  

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.

...

quoted

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

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

...

quoted

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

quoted

+ *  

Redundant blank line.

quoted

+ * @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"

quoted

+ */  

quoted

+  

Redundant blank line

quoted

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

quoted

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

quoted

+       if (par->gpio.dc)  

Useless duplicate check.

quoted

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

quoted

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

quoted

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

quoted

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

quoted

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

quoted

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

quoted

+               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