On Fri, Jan 29, 2021 at 2:47 PM carlis <zhangxuezhi3@xxxxxxxxx> wrote:
> On Fri, 29 Jan 2021 12:23:08 +0200
> Andy Shevchenko <andy.shevchenko@xxxxxxxxx> wrote:
> > On Fri, Jan 29, 2021 at 7:01 AM carlis <zhangxuezhi3@xxxxxxxxx> wrote:
> > > On Thu, 28 Jan 2021 16:33:02 +0200
> > > Andy Shevchenko <andy.shevchenko@xxxxxxxxx> wrote:
> > > > On Thu, Jan 28, 2021 at 2:58 PM Carlis <zhangxuezhi3@xxxxxxxxx>
> > > > 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
_______________________________________________
devel mailing list
devel@xxxxxxxxxxxxxxxxxxxxxx
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
