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

[Andy Shevchenko]

On Fri, Jan 29, 2021 at 2:47 PM carlis <zhangxuezhi3 at gmail.com> wrote:
> On Fri, 29 Jan 2021 12:23:08 +0200
> Andy Shevchenko <andy.shevchenko at gmail.com> wrote:
> > On Fri, Jan 29, 2021 at 7:01 AM carlis <zhangxuezhi3 at gmail.com> wrote:
> > > On Thu, 28 Jan 2021 16:33:02 +0200
> > > Andy Shevchenko <andy.shevchenko at gmail.com> wrote:
> > > > On Thu, Jan 28, 2021 at 2:58 PM Carlis <zhangxuezhi3 at gmail.com>
> > > > wrote:

...

> > Please, go again thru my comments and comments from others and
> > carefully address all of them everywhere in your contribution. If you
> > have questions, ask them in reply in the corresponding context.

...

> > > /**
> > >  * init_tearing_effect_line() - init tearing effect line
> >
> > >  *
> >
> > For example, above was commented on and hasn't been addressed here.
> >
> hi,here i can not get you.....

The above is a blank line which is redundant. It also applied to the
other function in the code.

> > >  * @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) {
> >
> > This one is not like I suggested.
> Why? My thinking is that if the TE is not configured and NULL is
> returned, the initialization can still proceed.....

I have suggested to bail out immediately. It will reduce an
indentation level on the below code.

> > >                 par->irq_te = gpiod_to_irq(te);
> > >                 gpiod_put(te);
> > >
> >
> > >                 if (par->irq_te) {
> >
> > This is wrong.
>
> Why? i have read gpiod_to_irq code, if an error occurs, a negative
> value is returned, and zero is not possible,so I need this value to
> determine if TE IRQ is configured

It returns two possible cases:
 - error code (when negative)
 - Linux IRQ number otherwise

You check makes a no-op since in either variant it will proceed to the
request of IRQ, which is wrong in an error case.

> > >                         rc = devm_request_irq(dev,
> > >                                               par->irq_te,
> > >                 panel_te_handler,
> > >                                               IRQF_TRIGGER_RISING,
> > >                 "TE_GPIO", par);
> >
> > Try to use less LOCs.
> >
> > >                         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");
> > >                 }
> >
> > Again, it is not what had been suggested.
> >
> > >         }

-- 
With Best Regards,
Andy Shevchenko