From: Andy Shevchenko <andy.shevchenko@gmail.com>
To: Carlis <zhangxuezhi3@gmail.com>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
Colin King <colin.king@canonical.com>,
oliver.graute@kococonnector.com, zhangxuezhi1@yulong.com,
Deepak R Varma <mh12gx2825@gmail.com>,
Stefano Brivio <sbrivio@redhat.com>,
dri-devel <dri-devel@lists.freedesktop.org>,
"open list:FRAMEBUFFER LAYER" <linux-fbdev@vger.kernel.org>,
"open list:STAGING SUBSYSTEM" <devel@driverdev.osuosl.org>,
Linux Kernel Mailing List <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH v12] staging: fbtft: add tearing signal detect
Date: Thu, 28 Jan 2021 16:33:02 +0200 [thread overview]
Message-ID: <CAHp75Vd=ijxnamuSYuxNLeyhGMCod=HaXWrQ0W0+3QCsQAychg@mail.gmail.com> (raw)
In-Reply-To: <1611838435-151774-1-git-send-email-zhangxuezhi3@gmail.com>
On Thu, Jan 28, 2021 at 2:58 PM Carlis <zhangxuezhi3@gmail.com> wrote:
Thanks for your contribution, my comments below.
> From: zhangxuezhi <zhangxuezhi1@yulong.com>
You probably have to configure your Git to use the same account for
author and committer.
> For st7789v ic,when we need continuous full screen refresh, it is best to
'ic,when' -> 'IC, when'
> 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.
...
> #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.
> #include <video/mipi_display.h>
...
> +#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.
...
> +static struct completion spi_panel_te;
As Greg said.
...
> 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;
...
> + 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.
> + if (IS_ERR(par->gpio.te))
> + return dev_err_probe(par->info->device, PTR_ERR(par->gpio.te),
> + "Failed to request te gpio\n");
> + if (par->gpio.te) {
Instead you should probably do the following:
int irq;
irq = gpiod_to_irq(...);
if (irq > 0)
> + init_completion(&spi_panel_te);
> + rc = devm_request_irq(dev,
> + gpiod_to_irq(par->gpio.te),
...and here simply use irq.
> + spi_panel_te_handler, IRQF_TRIGGER_RISING,
> + "TE_GPIO", par);
> + 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?
> + return dev_err_probe(par->info->device, PTR_ERR(rc),
Use your temporary variable and move...
> + "TE request_irq failed.\n");
...this on the previous line.
> + disable_irq_nosync(gpiod_to_irq(par->gpio.te));
Why do you call gpio_to_irq() twice?
> + } 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.
> + }
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;
}
Note, when you define proper fields for IRQ line and completion in the
data structure the above can be amended accordingly.
...
> + /* tearing effect line on */
> + if (par->gpio.te)
> + write_reg(par, 0x35, 0x00);
0x35 is defined. use it and drop useless comments.
...
> /**
> + * st7789v_write_vmem16_bus8() - write data to display
> + *
Redundant blank line.
> + * @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"
> + */
> +
Redundant blank line
> +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.
> + 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);
> + if (par->gpio.dc)
Useless duplicate check.
> + gpiod_set_value(par->gpio.dc, 1);
> + 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.
> + 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));
> + }
> +
> + while (remain) {
> + to_copy = min(tx_array_size, remain);
> + 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.
> + 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.
> + 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;
> +}
--
With Best Regards,
Andy Shevchenko
next prev parent reply other threads:[~2021-01-28 14:34 UTC|newest]
Thread overview: 10+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-01-28 12:53 [PATCH v12] staging: fbtft: add tearing signal detect Carlis
2021-01-28 12:58 ` Greg KH
2021-01-28 14:33 ` Andy Shevchenko [this message]
2021-01-28 14:44 ` Andy Shevchenko
2021-01-28 15:23 ` Dan Carpenter
2021-01-29 5:01 ` carlis
[not found] ` <CAHp75Vdi4H_zY3+QPSq_wmdf20B9xPeqsOT10JHfMLJESX77gA@mail.gmail.com>
[not found] ` <20210129215638.000047b0@gmail.com>
[not found] ` <CAHp75VcdOibSRuSBZYhFtEcVxuammYMfcnrUQGvS6ttArFxj6g@mail.gmail.com>
2021-01-30 6:39 ` carlis
2021-02-01 17:40 ` Andy Shevchenko
2021-02-02 1:52 ` Carlis
2021-02-15 15:30 ` Andy Shevchenko
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to='CAHp75Vd=ijxnamuSYuxNLeyhGMCod=HaXWrQ0W0+3QCsQAychg@mail.gmail.com' \
--to=andy.shevchenko@gmail.com \
--cc=colin.king@canonical.com \
--cc=devel@driverdev.osuosl.org \
--cc=dri-devel@lists.freedesktop.org \
--cc=gregkh@linuxfoundation.org \
--cc=linux-fbdev@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=mh12gx2825@gmail.com \
--cc=oliver.graute@kococonnector.com \
--cc=sbrivio@redhat.com \
--cc=zhangxuezhi1@yulong.com \
--cc=zhangxuezhi3@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).