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