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

From: Andy Shevchenko <andy.shevchenko@gmail.com>
To: carlis <zhangxuezhi3@gmail.com>
Cc: "open list:STAGING SUBSYSTEM" <devel@driverdev.osuosl.org>,
	"open list:FRAMEBUFFER LAYER" <linux-fbdev@vger.kernel.org>,
	Deepak R Varma <mh12gx2825@gmail.com>,
	Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
	oliver.graute@kococonnector.com,
	Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
	dri-devel <dri-devel@lists.freedesktop.org>,
	Stefano Brivio <sbrivio@redhat.com>,
	Colin King <colin.king@canonical.com>,
	zhangxuezhi1@yulong.com
Subject: Re: [PATCH v12] staging: fbtft: add tearing signal detect
Date: Fri, 29 Jan 2021 16:26:12 +0200	[thread overview]
Message-ID: <CAHp75VcdOibSRuSBZYhFtEcVxuammYMfcnrUQGvS6ttArFxj6g@mail.gmail.com> (raw)
In-Reply-To: <20210129215638.000047b0@gmail.com>

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 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
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel