On Fri, Jan 29, 2021 at 3:56 PM carlis <zhangxuezhi3@xxxxxxxxx> wrote:
> On Fri, 29 Jan 2021 12:23:08 +0200
> Andy Shevchenko <andy.shevchenko@xxxxxxxxx> 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@xxxxxxxxxx>
> 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 to
> request 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 request
> failed.\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..."
--
With Best Regards,
Andy Shevchenko
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
